Fix weight calculations for mixed legacy and segwit

see:
#924 (comment)

Was a PITA since branch and bound is hard to do with this interference
between segiwt and legacy weights. It would find solutions that looked
good until you add the final input which was segwit and then the
solution would be suboptimal and fail the test.

Author: LLFourn

crates/bdk/src/wallet/coin_selection.rs

@@ -836,7 +836,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 250_000 + FEE_AMOUNT;
 
-        let result = LargestFirstCoinSelection::default()
+        let result = LargestFirstCoinSelection
             .coin_select(
                 utxos,
                 vec![],
@@ -857,7 +857,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 20_000 + FEE_AMOUNT;
 
-        let result = LargestFirstCoinSelection::default()
+        let result = LargestFirstCoinSelection
             .coin_select(
                 utxos,
                 vec![],
@@ -878,7 +878,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 20_000 + FEE_AMOUNT;
 
-        let result = LargestFirstCoinSelection::default()
+        let result = LargestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,
@@ -900,7 +900,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 500_000 + FEE_AMOUNT;
 
-        LargestFirstCoinSelection::default()
+        LargestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,
@@ -918,7 +918,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 250_000 + FEE_AMOUNT;
 
-        LargestFirstCoinSelection::default()
+        LargestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,
@@ -935,7 +935,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 180_000 + FEE_AMOUNT;
 
-        let result = OldestFirstCoinSelection::default()
+        let result = OldestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,
@@ -956,7 +956,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 20_000 + FEE_AMOUNT;
 
-        let result = OldestFirstCoinSelection::default()
+        let result = OldestFirstCoinSelection
             .coin_select(
                 utxos,
                 vec![],
@@ -977,7 +977,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 20_000 + FEE_AMOUNT;
 
-        let result = OldestFirstCoinSelection::default()
+        let result = OldestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,
@@ -999,7 +999,7 @@ mod test {
         let drain_script = ScriptBuf::default();
         let target_amount = 600_000 + FEE_AMOUNT;
 
-        OldestFirstCoinSelection::default()
+        OldestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,
@@ -1018,7 +1018,7 @@ mod test {
         let target_amount: u64 = utxos.iter().map(|wu| wu.utxo.txout().value).sum::<u64>() - 50;
         let drain_script = ScriptBuf::default();
 
-        OldestFirstCoinSelection::default()
+        OldestFirstCoinSelection
             .coin_select(
                 vec![],
                 utxos,

nursery/coin_select/README.md

@@ -10,8 +10,8 @@ use bdk_coin_select::{CoinSelector, Candidate, TXIN_BASE_WEIGHT};
 use bitcoin::{ Transaction, TxIn };
 
 // You should use miniscript to figure out the satisfaction weight for your coins!
-const tr_satisfaction_weight: u32 = 66;
-const tr_input_weight: u32 = txin_base_weight + tr_satisfaction_weight;
+const TR_SATISFACTION_WEIGHT: u32 = 66;
+const TR_INPUT_WEIGHT: u32 = TXIN_BASE_WEIGHT + TR_SATISFACTION_WEIGHT;
 
 
 let candidates = vec![
@@ -21,17 +21,17 @@ let candidates = vec![
         input_count: 1,
         // the value of the input
         value: 1_000_000,
-        // the total weight of the input(s). This doesn't include 
+        // the total weight of the input(s). This doesn't include
         weight: TR_INPUT_WEIGHT,
         // wether it's a segwit input. Needed so we know whether to include the segwit header
         // in total weight calculations.
         is_segwit: true
     },
     Candidate {
-        // A candidate can represent multiple inputs in the case where you always want some inputs 
+        // A candidate can represent multiple inputs in the case where you always want some inputs
         // to be spent together.
         input_count: 2,
-        weight: 2*tr_input_weight,
+        weight: 2*TR_INPUT_WEIGHT,
         value: 3_000_000,
         is_segwit: true
     },
@@ -50,10 +50,7 @@ let base_weight = Transaction {
     version: 1,
 }.weight().to_wu() as u32;
 
-panic!("{}", base_weight);
+println!("base weight: {}", base_weight);
 
 let mut coin_selector = CoinSelector::new(&candidates,base_weight);
-
-
 ```
-

nursery/coin_select/src/bnb.rs

@@ -42,13 +42,13 @@ impl<'a, M: BnBMetric> Iterator for BnbIter<'a, M> {
             None => return Some(None),
         };
 
-        match &self.best {
-            Some(best_score) if score >= *best_score => Some(None),
-            _ => {
-                self.best = Some(score.clone());
-                Some(Some((selector, score)))
+        if let Some(best_score) = &self.best {
+            if score >= *best_score {
+                return Some(None);
             }
         }
+        self.best = Some(score.clone());
+        Some(Some((selector, score)))
     }
 }
 

nursery/coin_select/src/coin_selector.rs

@@ -167,28 +167,31 @@ impl<'a> CoinSelector<'a> {
     pub fn is_empty(&self) -> bool {
         self.selected.is_empty()
     }
-    /// Weight sum of all selected inputs.
-    pub fn selected_weight(&self) -> u32 {
-        self.selected
-            .iter()
-            .map(|&index| self.candidates[index].weight)
-            .sum()
-    }
 
     /// The weight of the inputs including the witness header and the varint for the number of
     /// inputs.
-    fn input_weight(&self) -> u32 {
-        let witness_header_extra_weight = self
-            .selected()
-            .find(|(_, wv)| wv.is_segwit)
-            .map(|_| 2)
-            .unwrap_or(0);
+    pub fn input_weight(&self) -> u32 {
+        let is_segwit_tx = self.selected().any(|(_, wv)| wv.is_segwit);
+        let witness_header_extra_weight = is_segwit_tx as u32 * 2;
         let vin_count_varint_extra_weight = {
             let input_count = self.selected().map(|(_, wv)| wv.input_count).sum::<usize>();
             (varint_size(input_count) - 1) * 4
         };
 
-        self.selected_weight() + witness_header_extra_weight + vin_count_varint_extra_weight
+        let selected_weight: u32 = self
+            .selected()
+            .map(|(_, candidate)| {
+                let mut weight = candidate.weight;
+                if is_segwit_tx && !candidate.is_segwit {
+                    // non-segwit candidates do not have the witness length field included in their
+                    // weight field so we need to add 1 here if it's in a segwit tx.
+                    weight += 1;
+                }
+                weight
+            })
+            .sum();
+
+        selected_weight + witness_header_extra_weight + vin_count_varint_extra_weight
     }
 
     /// Absolute value sum of all selected inputs.
@@ -202,8 +205,6 @@ impl<'a> CoinSelector<'a> {
     /// Current weight of template tx + selected inputs.
     pub fn weight(&self, drain_weight: u32) -> u32 {
         // TODO take into account whether drain tips over varint for number of outputs
-        //
-        // TODO: take into account the witness stack length for each input
         self.base_weight + self.input_weight() + drain_weight
     }
 
@@ -235,8 +236,8 @@ impl<'a> CoinSelector<'a> {
             - target.min_fee as i64
     }
 
-    /// The feerate the transaction would have if we were to use this selection of inputs to achieve
-    /// the ???
+    /// The feerate the transaction would have if we were to use this selection of inputs to acheive
+    /// the `target_value`
     pub fn implied_feerate(&self, target_value: u64, drain: Drain) -> FeeRate {
         let numerator = self.selected_value() as i64 - target_value as i64 - drain.value as i64;
         let denom = self.weight(drain.weight);
@@ -258,8 +259,8 @@ impl<'a> CoinSelector<'a> {
     }
 
     // /// Waste sum of all selected inputs.
-    fn selected_waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 {
-        self.selected_weight() as f32 * (feerate.spwu() - long_term_feerate.spwu())
+    fn input_waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 {
+        self.input_weight() as f32 * (feerate.spwu() - long_term_feerate.spwu())
     }
 
     /// Sorts the candidates by the comparision function.
@@ -315,7 +316,7 @@ impl<'a> CoinSelector<'a> {
         excess_discount: f32,
     ) -> f32 {
         debug_assert!((0.0..=1.0).contains(&excess_discount));
-        let mut waste = self.selected_waste(target.feerate, long_term_feerate);
+        let mut waste = self.input_waste(target.feerate, long_term_feerate);
 
         if drain.is_none() {
             // We don't allow negative excess waste since negative excess just means you haven't

nursery/coin_select/src/lib.rs

@@ -28,7 +28,8 @@ pub mod change_policy;
 /// length.
 pub const TXIN_BASE_WEIGHT: u32 = (32 + 4 + 4 + 1) * 4;
 
-/// The weight of a TXOUT with a zero length `scriptPubkey`
+/// The weight of a TXOUT with a zero length `scriptPubKey`
+#[allow(clippy::identity_op)]
 pub const TXOUT_BASE_WEIGHT: u32 =
     // The value
     4 * core::mem::size_of::<u64>() as u32

nursery/coin_select/src/metrics/waste.rs

@@ -3,19 +3,21 @@ use crate::{bnb::BnBMetric, float::Ordf32, Candidate, CoinSelector, Drain, FeeRa
 
 /// The "waste" metric used by bitcoin core.
 ///
-/// See this [great
-/// explanation](https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection) for an understanding of the waste metric.
+/// See this [great explanation](https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection)
+/// for an understanding of the waste metric.
 ///
 /// ## WARNING: Waste metric considered wasteful
 ///
 /// Note that bitcoin core at the time of writing use the waste metric to
 ///
 /// 1. minimise the waste while searching for changeless solutions.
-/// 2. It tiebreaks multiple valid selections from different algorithms (which do not try and minimise waste) with waste.
+/// 2. It tiebreaks multiple valid selections from different algorithms (which do not try and
+///    minimise waste) with waste.
 ///
-/// This is **very** different from minimising waste in general which is what this metric will do when used in [`CoinSelector::branch_and_bound`].
-/// The waste metric tends to over consolidate funds. If the `long_term_feerate` is even slightly
-/// higher than the current feerate (specified in `target`) it will select all your coins!
+/// This is **very** different from minimising waste in general which is what this metric will do
+/// when used in [`CoinSelector::branch_and_bound`]. The waste metric tends to over consolidate
+/// funds. If the `long_term_feerate` is even slightly higher than the current feerate (specified
+/// in `target`) it will select all your coins!
 pub struct Waste<'c, C> {
     pub target: Target,
     pub long_term_feerate: FeeRate,
@@ -154,7 +156,7 @@ where
                     debug_assert!(weight_to_satisfy <= to_slurp.weight as f32);
                     weight_to_satisfy
                 };
-                let weight_lower_bound = cs.selected_weight() as f32 + ideal_next_weight;
+                let weight_lower_bound = cs.input_weight() as f32 + ideal_next_weight;
                 let mut waste = weight_lower_bound * rate_diff;
                 waste += change_lower_bound.waste(self.target.feerate, self.long_term_feerate);