fix!: exact weight pricing for grouped legacy inputs in mixed txs

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
 
+- **Breaking:** Replace `Candidate`'s `input_count` and `is_segwit` fields with `segwit_count` and `legacy_count`, fixing `CoinSelector::input_weight` undercounting candidates that group multiple inputs: in a segwit transaction every legacy input still serializes an empty witness (1 WU), which was previously paid once per candidate instead of once per legacy input, so a group of N legacy inputs came out N-1 WU short. Splitting the count by script type also means a single candidate may now mix legacy and segwit inputs and still be priced exactly. Replaces `Candidate::new` with `Candidate::new_segwit` and `Candidate::new_legacy`.
 - **Breaking:** `BnbMetric`'s `score`, `bound`, and `drain` take the `target: Target` as a parameter, and `CoinSelector::run_bnb`/`bnb_solutions` gain a leading `target` argument. Consequently `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless<M>` previously had to keep in sync with its inner metric, and aligns the metric API with the rest of `CoinSelector`, where `target` is always passed in.
 - **Breaking:** `BnbMetric` metrics now decide the change output themselves. The trait gains a `drain(&mut self, cs) -> Drain` method; call it on a branch-and-bound solution (or the `LowestFee` metric directly) to get the change output the metric optimized against, instead of computing a separate `ChangePolicy`.
 - **Breaking:** `CoinSelector::run_bnb` now returns `(Ordf32, Drain)` instead of just `Ordf32`, handing back the change output the metric decided on for the winning selection.

README.md

@@ -33,23 +33,22 @@ let candidates = vec![
     Candidate {
         // How many inputs does this candidate represents. Needed so we can 
         // figure out the weight of the varint that encodes the number of inputs
-        input_count: 1,
+        // and whether segwit transaction fields need to be counted in.
+        segwit_count: 1,
+        legacy_count: 0,
         // the value of the input
         value: 1_000_000,
         // the total weight of the input(s) including their witness/scriptSig
         // you may need to use miniscript to figure out the correct value here.
         weight: TR_KEYSPEND_TXIN_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 to be spent together.
-        input_count: 2,
+        segwit_count: 2,
+        legacy_count: 0,
         weight: 2*TR_KEYSPEND_TXIN_WEIGHT,
         value: 3_000_000,
-        is_segwit: true
     }
 ];
 
@@ -105,22 +104,22 @@ let outputs = vec![TxOut {
 
 let candidates = [
     Candidate {
-        input_count: 1,
+        segwit_count: 1,
+        legacy_count: 0,
         value: 400_000,
         weight: TR_KEYSPEND_TXIN_WEIGHT,
-        is_segwit: true
     },
     Candidate {
-        input_count: 1,
+        segwit_count: 1,
+        legacy_count: 0,
         value: 200_000,
         weight: TR_KEYSPEND_TXIN_WEIGHT,
-        is_segwit: true
     },
     Candidate {
-        input_count: 1,
+        segwit_count: 1,
+        legacy_count: 0,
         value: 11_000,
         weight: TR_KEYSPEND_TXIN_WEIGHT,
-        is_segwit: true
     }
 ];
 let drain_weights = bdk_coin_select::DrainWeights::default();

benches/coin_selector.rs

@@ -33,8 +33,8 @@ fn make_candidates(n: usize) -> Vec<Candidate> {
             Candidate {
                 value,
                 weight: TXIN_BASE_WEIGHT + P2WPKH_SAT_W,
-                input_count: 1,
-                is_segwit: true,
+                segwit_count: 1,
+                legacy_count: 0,
             }
         })
         .collect()

src/coin_selector.rs

@@ -142,20 +142,23 @@ impl<'a> CoinSelector<'a> {
     /// The weight of the inputs including the witness header and the varint for the number of
     /// inputs.
     pub fn input_weight(&self) -> u64 {
-        let is_segwit_tx = self.selected().any(|(_, wv)| wv.is_segwit);
+        let is_segwit_tx = self.selected().any(|(_, wv)| wv.segwit_count > 0);
         let witness_header_extra_weight = is_segwit_tx as u64 * 2;
 
-        let input_count = self.selected().map(|(_, wv)| wv.input_count).sum::<usize>();
+        let input_count = self
+            .selected()
+            .map(|(_, wv)| wv.segwit_count + wv.legacy_count)
+            .sum::<usize>();
         let input_varint_weight = varint_size(input_count) * 4;
 
         let selected_weight: u64 = 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;
+                if is_segwit_tx {
+                    // Legacy inputs do not have the witness length included in their weight field
+                    // so we need to add 1 to each if it's a segwit tx.
+                    weight += candidate.legacy_count as u64;
                 }
                 weight
             })
@@ -889,39 +892,78 @@ impl std::error::Error for NoBnbSolution {}
 
 /// A `Candidate` represents an input candidate for [`CoinSelector`].
 ///
-/// This can either be a single UTXO, or a group of UTXOs that should be spent together.
+/// This can either be a single UTXO, or a group of UTXOs that should be spent together. A group
+/// may mix legacy and segwit inputs; set [`legacy_count`] and [`segwit_count`] accordingly.
+///
+/// [`legacy_count`]: Candidate::legacy_count
+/// [`segwit_count`]: Candidate::segwit_count
 #[derive(Debug, Clone, Copy)]
 pub struct Candidate {
     /// Total value of the UTXO(s) that this [`Candidate`] represents.
     pub value: u64,
     /// Total weight of including this/these UTXO(s).
     /// `txin` fields: `prevout`, `nSequence`, `scriptSigLen`, `scriptSig`, `scriptWitnessLen`,
     /// `scriptWitness` should all be included.
+    ///
+    /// For legacy inputs, do *not* include the `scriptWitnessLen` byte: a legacy input only
+    /// serializes an (empty) witness when the transaction has a witness section, and
+    /// [`CoinSelector::input_weight`] adds that 1 WU per legacy input once any segwit input is
+    /// selected.
     pub weight: u64,
-    /// Total number of inputs; so we can calculate extra `varint` weight due to `vin` len changes.
-    pub input_count: usize,
-    /// Whether this [`Candidate`] contains at least one segwit spend.
-    pub is_segwit: bool,
+    /// Total number of segwit inputs.
+    ///
+    /// If any selected candidate has a non-zero `segwit_count`, the transaction serializes a
+    /// witness section (marker + flag, 2 WU) and every input — including legacy ones — pays for
+    /// a witness.
+    pub segwit_count: usize,
+    /// Total number of legacy (non-segwit) inputs.
+    ///
+    /// Each legacy input serializes an empty witness (1 WU) when the transaction has a witness
+    /// section; [`CoinSelector::input_weight`] prices this per legacy input, so grouped legacy
+    /// inputs are counted exactly.
+    pub legacy_count: usize,
 }
 
 impl Candidate {
     /// Create a [`Candidate`] input that spends a single taproot keyspend output.
     pub fn new_tr_keyspend(value: u64) -> Self {
         let weight = TR_KEYSPEND_SATISFACTION_WEIGHT;
-        Self::new(value, weight, true)
+        Self::new_segwit(value, weight)
     }
 
-    /// Create a new [`Candidate`] that represents a single input.
+    /// Create a new [`Candidate`] that represents a single segwit input.
+    ///
+    /// `satisfaction_weight` is the additional weight (in weight units) required to satisfy the input
+    /// beyond [`TXIN_BASE_WEIGHT`] (e.g. `scriptWitnessLen + scriptWitness` in WU at 1 WU/byte, plus
+    /// any `scriptSig` data and extra `scriptSigLen` varint bytes if nested/wrapped segwit).
+    ///
+    /// Note that [`TXIN_BASE_WEIGHT`] already accounts for the outpoint, `nSequence`, and 1 byte for
+    /// `scriptSigLen`.
+    pub fn new_segwit(value: u64, satisfaction_weight: u64) -> Candidate {
+        let weight = TXIN_BASE_WEIGHT + satisfaction_weight;
+        Candidate {
+            value,
+            weight,
+            segwit_count: 1,
+            legacy_count: 0,
+        }
+    }
+
+    /// Create a new [`Candidate`] that represents a single legacy (non-segwit) input.
+    ///
+    /// `satisfaction_weight` is the additional weight (in weight units) required to satisfy the input
+    /// beyond [`TXIN_BASE_WEIGHT`] (e.g. `scriptSig` at 4 WU/byte, plus 4 WU per extra `scriptSigLen`
+    /// varint byte if `scriptSig` exceeds 252 bytes).
     ///
-    /// `satisfaction_weight` is the weight of `scriptSigLen + scriptSig + scriptWitnessLen +
-    /// scriptWitness`.
-    pub fn new(value: u64, satisfaction_weight: u64, is_segwit: bool) -> Candidate {
+    /// Note that [`TXIN_BASE_WEIGHT`] already accounts for the outpoint, `nSequence`, and 1 byte for
+    /// `scriptSigLen`.
+    pub fn new_legacy(value: u64, satisfaction_weight: u64) -> Candidate {
         let weight = TXIN_BASE_WEIGHT + satisfaction_weight;
         Candidate {
             value,
             weight,
-            input_count: 1,
-            is_segwit,
+            segwit_count: 0,
+            legacy_count: 1,
         }
     }
 

tests/bnb.rs

@@ -11,16 +11,16 @@ use proptest::{prelude::*, proptest, test_runner::*};
 fn test_wv(mut rng: impl RngCore) -> impl Iterator<Item = Candidate> {
     core::iter::repeat_with(move || {
         let value = rng.random_range(0..1_000);
-        let mut candidate = Candidate {
+        let candidate = Candidate {
             value,
             weight: 100,
-            input_count: rng.random_range(1..2),
-            is_segwit: rng.random_bool(0.5),
+            segwit_count: rng.random_range(1..2),
+            legacy_count: 0,
         };
-        // HACK: set is_segwit = true for all these tests because you can't actually lower bound
-        // things easily with how segwit inputs interfere with their weights. We can't modify the
-        // above since that would change what we pull from rng.
-        candidate.is_segwit = true;
+        // Keep drawing the bool these tests always drew so the rng stream (and therefore the
+        // generated cases) is unchanged. All candidates are segwit: mixing in legacy inputs makes
+        // their weights context-dependent, which these tests can't lower-bound easily.
+        let _ = rng.random_bool(0.5);
         candidate
     })
 }
@@ -62,10 +62,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() {
     let num_additional_canidates = 12;
 
     let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha);
-    let mut wv = test_wv(&mut rng).map(|mut candidate| {
-        candidate.is_segwit = true;
-        candidate
-    });
+    let mut wv = test_wv(&mut rng);
 
     let solution: Vec<Candidate> = (0..solution_len).map(|_| wv.next().unwrap()).collect();
     let solution_weight = {

tests/changeless.rs

@@ -14,8 +14,8 @@ fn test_wv(mut rng: impl RngCore) -> impl Iterator<Item = Candidate> {
         Candidate {
             value,
             weight: rng.random_range(0..100),
-            input_count: rng.random_range(1..2),
-            is_segwit: false,
+            segwit_count: rng.random_range(1..2),
+            legacy_count: 0,
         }
     })
 }

tests/common.rs

@@ -269,14 +269,21 @@ pub fn gen_candidates(n: usize) -> Vec<Candidate> {
     core::iter::repeat_with(move || {
         let value = rng.random_range(1..500_001);
         let weight = rng.random_range(1..2001);
-        let input_count = rng.random_range(1..3);
-        let is_segwit = rng.random_bool(0.01);
+
+        let (mut legacy_count, mut segwit_count);
+        loop {
+            legacy_count = rng.random_range(0..3);
+            segwit_count = if rng.random_bool(0.01) { 1 } else { 0 };
+            if legacy_count > 0 || segwit_count > 0 {
+                break;
+            }
+        }
 
         Candidate {
             value,
             weight,
-            input_count,
-            is_segwit,
+            segwit_count,
+            legacy_count,
         }
     })
     .take(n)

tests/lowest_fee.rs

@@ -1,5 +1,4 @@
 #![allow(unused_imports)]
-
 mod common;
 use bdk_coin_select::metrics::{Changeless, LowestFee};
 use bdk_coin_select::{
@@ -85,8 +84,8 @@ proptest! {
             Candidate {
                 value: 20_000,
                 weight: (32 + 4 + 4 + 1) * 4 + 64 + 32,
-                input_count: 1,
-                is_segwit: true,
+                segwit_count: 1,
+                legacy_count: 0,
             };
             params.n_candidates
         ];
@@ -238,21 +237,21 @@ fn does_not_create_change_below_spend_cost() {
         Candidate {
             value: 100_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
         Candidate {
             value: 50_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
         // NOTE: this input has negative effective value
         Candidate {
             value: 10,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
     ];
 
@@ -321,14 +320,14 @@ fn zero_fee_tx() {
         Candidate {
             value: 100_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
         Candidate {
             value: 50_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
     ];
 
@@ -354,8 +353,8 @@ fn err_candidate(value: u64) -> Candidate {
     Candidate {
         value,
         weight: 272, // ~1 P2WPKH input
-        input_count: 1,
-        is_segwit: true,
+        segwit_count: 1,
+        legacy_count: 0,
     }
 }
 

tests/srd.rs

@@ -75,20 +75,20 @@ fn srd_insufficient_funds() {
         Candidate {
             value: 50_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
         Candidate {
             value: 50_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
         Candidate {
             value: 50_000,
             weight: 100,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         },
     ];
     let target = target(200_000, 5.0);
@@ -117,8 +117,8 @@ fn srd_max_weight_exceeded() {
         Candidate {
             value: 100_000,
             weight: 1000,
-            input_count: 1,
-            is_segwit: true,
+            segwit_count: 1,
+            legacy_count: 0,
         };
         10
     ];