fix!: price every transaction as segwit and drop Candidate::is_segwit

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Unreleased
 
+- **Breaking:** Every transaction is now priced as segwit, and `Candidate::is_segwit` is removed. `Candidate::weight` is the input's segwit-serialized weight (`TxIn::segwit_weight`), so legacy inputs include their 1 WU empty witness. A transaction that spends only legacy inputs is overestimated by 2 WU plus 1 WU per input, and never undershoots the target feerate. This fixes `CoinSelector::input_weight` undercounting candidates that group several legacy inputs in a segwit transaction.
+- **Breaking:** `Candidate::new(value, satisfaction_weight, is_segwit)` is now `Candidate::new(value, satisfaction_weight)`, and `satisfaction_weight` now means the weight over an unsatisfied `TxIn::default()`, which is what miniscript's `Descriptor::max_weight_to_satisfy` returns for every script type. Previously it was documented as including `scriptSigLen` and `scriptWitnessLen`. To migrate, pass `max_weight_to_satisfy()` directly.
 - **Breaking:** `CoinSelector` now owns its `Target`. `CoinSelector::new(candidates, target)` takes it and `CoinSelector::target()` returns it, and it is fixed for the life of the selector. Every method that took a `target: Target` argument no longer does, including `excess`, `missing`, `rate_excess`, `implied_fee`, `is_funded`, `is_within_max_weight`, `drain`, `select_until_target_met`, `select_srd`, `run_bnb`, and `bnb_solutions`. The same goes for arguments that merely restated part of the target: `weight` and `implied_feerate` no longer take `TargetOutputs`, `fee` no longer takes `target_value`, and `effective_value` and `select_all_effective` no longer take a `FeeRate`. `BnbMetric`'s methods read the target from the `CoinSelector` they are given — they are now `fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>`, `fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>` and `fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain` — so `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. To measure a selection against a second target, use `CoinSelector::with_target(target)`, which copies the selection, the bans and the candidate order over to the new target; `CoinSelector::new` starts from an empty selection.
 - **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

@@ -36,20 +36,18 @@ let candidates = vec![
         input_count: 1,
         // 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.
+        // the total weight of the input(s) as serialized in a segwit tx, i.e.
+        // `TxIn::segwit_weight`. Legacy inputs include their 1 WU empty witness.
+        // `Candidate::new(value, descriptor.max_weight_to_satisfy())` computes
+        // this for you.
         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,
         weight: 2*TR_KEYSPEND_TXIN_WEIGHT,
         value: 3_000_000,
-        is_segwit: true
     }
 ];
 
@@ -108,19 +106,16 @@ let candidates = [
         input_count: 1,
         value: 400_000,
         weight: TR_KEYSPEND_TXIN_WEIGHT,
-        is_segwit: true
     },
     Candidate {
         input_count: 1,
         value: 200_000,
         weight: TR_KEYSPEND_TXIN_WEIGHT,
-        is_segwit: true
     },
     Candidate {
         input_count: 1,
         value: 11_000,
         weight: TR_KEYSPEND_TXIN_WEIGHT,
-        is_segwit: true
     }
 ];
 let drain_weights = bdk_coin_select::DrainWeights::default();

benches/coin_selector.rs

@@ -34,7 +34,6 @@ fn make_candidates(n: usize) -> Vec<Candidate> {
                 value,
                 weight: TXIN_BASE_WEIGHT + P2WPKH_SAT_W,
                 input_count: 1,
-                is_segwit: true,
             }
         })
         .collect()

src/coin_selector.rs

@@ -185,27 +185,14 @@ impl<'a> CoinSelector<'a> {
 
     /// The weight of the inputs including the witness header and the varint for the number of
     /// inputs.
+    ///
+    /// The transaction is always priced as segwit, so a selection of only legacy inputs is
+    /// overestimated by the 2 WU witness header plus 1 WU per input (see [`Candidate::weight`]).
     pub fn input_weight(&self) -> u64 {
-        let is_segwit_tx = self.selected().any(|(_, wv)| wv.is_segwit);
-        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_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;
-                }
-                weight
-            })
-            .sum();
-
-        input_varint_weight + selected_weight + witness_header_extra_weight
+        let selected_weight: u64 = self.selected().map(|(_, wv)| wv.weight).sum();
+        input_varint_weight + selected_weight + SEGWIT_HEADER_WEIGHT
     }
 
     /// Absolute value sum of all selected inputs.
@@ -921,34 +908,39 @@ impl std::error::Error for NoBnbSolution {}
 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.
+    /// Total weight of the input(s) as serialized in a segwit transaction, i.e. the sum of
+    /// `TxIn::segwit_weight` from rust-bitcoin. That is `prevout`, `nSequence`, `scriptSigLen`,
+    /// `scriptSig`, `scriptWitnessLen` and `scriptWitness`, including the 1 WU empty witness a legacy
+    /// input serializes in a segwit transaction.
+    ///
+    /// [`CoinSelector`] always prices the transaction as segwit. A transaction that spends only
+    /// legacy inputs has no witness section, so its weight is overestimated by 2 WU plus 1 WU per
+    /// input. This never undershoots the target feerate.
     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,
 }
 
 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)
+        Candidate {
+            value,
+            weight: TR_KEYSPEND_TXIN_WEIGHT,
+            input_count: 1,
+        }
     }
 
-    /// Create a new [`Candidate`] that represents a single input.
+    /// Create a new [`Candidate`] that represents a single input of any script type.
     ///
-    /// `satisfaction_weight` is the weight of `scriptSigLen + scriptSig + scriptWitnessLen +
-    /// scriptWitness`.
-    pub fn new(value: u64, satisfaction_weight: u64, is_segwit: bool) -> Candidate {
-        let weight = TXIN_BASE_WEIGHT + satisfaction_weight;
+    /// `satisfaction_weight` is the weight the input adds over an unsatisfied `TxIn::default()`,
+    /// which is exactly what miniscript's `Descriptor::max_weight_to_satisfy` returns. It excludes
+    /// the 1-byte `scriptSigLen` and the 1-byte `scriptWitnessLen`, which this adds.
+    pub fn new(value: u64, satisfaction_weight: u64) -> Candidate {
         Candidate {
             value,
-            weight,
+            weight: TXIN_BASE_WEIGHT + EMPTY_WITNESS_WEIGHT + satisfaction_weight,
             input_count: 1,
-            is_segwit,
         }
     }
 

src/lib.rs

@@ -33,6 +33,13 @@ pub use drain::*;
 /// length.
 pub const TXIN_BASE_WEIGHT: u64 = (32 + 4 + 4 + 1) * 4;
 
+/// The segwit marker and flag bytes. Every transaction is priced as segwit, so this is always
+/// counted.
+const SEGWIT_HEADER_WEIGHT: u64 = 2;
+
+/// The `scriptWitnessLen` byte of an input with an empty witness.
+const EMPTY_WITNESS_WEIGHT: u64 = 1;
+
 /// The weight of a TXOUT with a zero length `scriptPubKey`
 #[allow(clippy::identity_op)]
 pub const TXOUT_BASE_WEIGHT: u64 =

tests/bnb.rs

@@ -11,16 +11,13 @@ 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),
         };
-        // 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;
+        // This used to draw `is_segwit`. Keep drawing so the rng stream stays the same.
+        let _ = rng.random_bool(0.5);
         candidate
     })
 }
@@ -62,10 +59,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 target_value = solution.iter().map(|c| c.value).sum();

tests/changeless.rs

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

tests/common.rs

@@ -264,13 +264,13 @@ pub fn gen_candidates(n: usize) -> Vec<Candidate> {
         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);
+        // This used to draw `is_segwit`. Keep drawing so the rng stream stays the same.
+        let _ = rng.random_bool(0.01);
 
         Candidate {
             value,
             weight,
             input_count,
-            is_segwit,
         }
     })
     .take(n)

tests/lowest_fee.rs

@@ -86,7 +86,6 @@ proptest! {
                 value: 20_000,
                 weight: (32 + 4 + 4 + 1) * 4 + 64 + 32,
                 input_count: 1,
-                is_segwit: true,
             };
             params.n_candidates
         ];
@@ -237,20 +236,17 @@ fn does_not_create_change_below_spend_cost() {
             value: 100_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
         Candidate {
             value: 50_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
         // NOTE: this input has negative effective value
         Candidate {
             value: 10,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
     ];
 
@@ -315,13 +311,11 @@ fn zero_fee_tx() {
             value: 100_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
         Candidate {
             value: 50_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
     ];
 
@@ -347,7 +341,6 @@ fn err_candidate(value: u64) -> Candidate {
         value,
         weight: 272, // ~1 P2WPKH input
         input_count: 1,
-        is_segwit: true,
     }
 }
 

tests/srd.rs

@@ -73,19 +73,16 @@ fn srd_insufficient_funds() {
             value: 50_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
         Candidate {
             value: 50_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
         Candidate {
             value: 50_000,
             weight: 100,
             input_count: 1,
-            is_segwit: true,
         },
     ];
     let target = target(200_000, 5.0);
@@ -115,7 +112,6 @@ fn srd_max_weight_exceeded() {
             value: 100_000,
             weight: 1000,
             input_count: 1,
-            is_segwit: true,
         };
         10
     ];

tests/weight.rs

@@ -38,7 +38,6 @@ fn segwit_one_input_one_output() {
             value,
             weight: txin.segwit_weight().to_wu(),
             input_count: 1,
-            is_segwit: true,
         })
         .collect::<Vec<_>>();
 
@@ -86,7 +85,6 @@ fn segwit_two_inputs_one_output() {
             value,
             weight: txin.segwit_weight().to_wu(),
             input_count: 1,
-            is_segwit: true,
         })
         .collect::<Vec<_>>();
 
@@ -132,9 +130,8 @@ fn legacy_three_inputs() {
         .zip(input_values)
         .map(|(txin, value)| Candidate {
             value,
-            weight: txin.legacy_weight().to_wu(),
+            weight: txin.segwit_weight().to_wu(),
             input_count: 1,
-            is_segwit: false,
         })
         .collect::<Vec<_>>();
 
@@ -152,9 +149,11 @@ fn legacy_three_inputs() {
     let mut coin_selector = CoinSelector::new(&candidates, target);
     coin_selector.select_all();
 
+    // Every tx is priced as segwit, so an all-legacy tx pays for the 2 WU witness header and a
+    // 1 WU empty witness per input that it doesn't actually serialize.
     assert_eq!(
         coin_selector.weight(DrainWeights::NONE),
-        orig_weight.to_wu()
+        orig_weight.to_wu() + 2 + 3
     );
     assert_eq!(
         (coin_selector
@@ -163,7 +162,7 @@ fn legacy_three_inputs() {
             .as_sat_vb()
             * 10.0)
             .round(),
-        99.2 * 10.0
+        99.1 * 10.0
     );
 }
 
@@ -184,20 +183,10 @@ fn legacy_three_inputs_one_segwit() {
         .input
         .iter()
         .zip(input_values)
-        .enumerate()
-        .map(|(i, (txin, value))| {
-            let is_segwit = i == 1;
-            Candidate {
-                value,
-                weight: if is_segwit {
-                    txin.segwit_weight()
-                } else {
-                    txin.legacy_weight()
-                }
-                .to_wu(),
-                input_count: 1,
-                is_segwit,
-            }
+        .map(|(txin, value)| Candidate {
+            value,
+            weight: txin.segwit_weight().to_wu(),
+            input_count: 1,
         })
         .collect::<Vec<_>>();
 
@@ -232,3 +221,13 @@ fn new_tr_keyspend_correct_weight() {
         Candidate::new_tr_keyspend(420).weight
     );
 }
+
+#[test]
+fn new_adds_satisfaction_weight_to_unsatisfied_txin() {
+    // miniscript's `max_weight_to_satisfy` is the weight over `TxIn::default()`, so passing it
+    // straight to `Candidate::new` must give the real `segwit_weight` for any script type.
+    assert_eq!(
+        Candidate::new(0, 0).weight,
+        bitcoin::TxIn::default().segwit_weight().to_wu()
+    );
+}