From ad5e6a2a4932e21eadcb254f1317d9d79a455688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Fri, 25 Sep 2026 05:10:07 +0000 Subject: [PATCH] fix!: price every transaction as segwit and drop Candidate::is_segwit `CoinSelector::input_weight` added the 1 WU empty witness once per legacy candidate, undercounting candidates that group several legacy inputs in a segwit transaction. Instead of tracking segwit and legacy input counts to price mixed transactions exactly, always price the transaction as segwit: `Candidate::weight` is the input's segwit serialized weight (`TxIn::segwit_weight`) and the 2 WU witness header is always counted. An all-legacy transaction is overestimated by 2 WU plus 1 WU per input, which never undershoots the target feerate. `Candidate::new` loses its `is_segwit` argument, and `satisfaction_weight` is now the weight over an unsatisfied `TxIn::default()`, which is exactly miniscript's `Descriptor::max_weight_to_satisfy` for every script type. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ README.md | 13 +++------- benches/coin_selector.rs | 1 - src/coin_selector.rs | 56 +++++++++++++++++----------------------- src/lib.rs | 7 +++++ tests/bnb.rs | 14 +++------- tests/changeless.rs | 1 - tests/common.rs | 4 +-- tests/lowest_fee.rs | 7 ----- tests/srd.rs | 4 --- tests/weight.rs | 39 ++++++++++++++-------------- 11 files changed, 62 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3891a90..de0d05c 100644 --- a/CHANGELOG.md +++ b/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`, `fn bound(&mut self, cs: &CoinSelector<'_>) -> Option` and `fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain` — so `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless` 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. diff --git a/README.md b/README.md index 922dd25..af35cb7 100644 --- a/README.md +++ b/README.md @@ -36,12 +36,11 @@ 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 @@ -49,7 +48,6 @@ let candidates = vec![ 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(); diff --git a/benches/coin_selector.rs b/benches/coin_selector.rs index 18e847e..6b25066 100644 --- a/benches/coin_selector.rs +++ b/benches/coin_selector.rs @@ -34,7 +34,6 @@ fn make_candidates(n: usize) -> Vec { value, weight: TXIN_BASE_WEIGHT + P2WPKH_SAT_W, input_count: 1, - is_segwit: true, } }) .collect() diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 2866c3b..adec5ca 100644 --- a/src/coin_selector.rs +++ b/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::(); 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, } } diff --git a/src/lib.rs b/src/lib.rs index 34c86ad..957f406 100644 --- a/src/lib.rs +++ b/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 = diff --git a/tests/bnb.rs b/tests/bnb.rs index 55cf5e7..dbd5fad 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -11,16 +11,13 @@ use proptest::{prelude::*, proptest, test_runner::*}; fn test_wv(mut rng: impl RngCore) -> impl Iterator { 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 = (0..solution_len).map(|_| wv.next().unwrap()).collect(); let target_value = solution.iter().map(|c| c.value).sum(); diff --git a/tests/changeless.rs b/tests/changeless.rs index 4e9ca81..194b1d4 100644 --- a/tests/changeless.rs +++ b/tests/changeless.rs @@ -15,7 +15,6 @@ fn test_wv(mut rng: impl RngCore) -> impl Iterator { value, weight: rng.random_range(0..100), input_count: rng.random_range(1..2), - is_segwit: false, } }) } diff --git a/tests/common.rs b/tests/common.rs index ffbaadc..1218f87 100644 --- a/tests/common.rs +++ b/tests/common.rs @@ -264,13 +264,13 @@ pub fn gen_candidates(n: usize) -> Vec { 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) diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 1d7538b..35c3a9c 100644 --- a/tests/lowest_fee.rs +++ b/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, } } diff --git a/tests/srd.rs b/tests/srd.rs index 92dc251..70c8b81 100644 --- a/tests/srd.rs +++ b/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 ]; diff --git a/tests/weight.rs b/tests/weight.rs index 3163fc8..96f2b0a 100644 --- a/tests/weight.rs +++ b/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::>(); @@ -86,7 +85,6 @@ fn segwit_two_inputs_one_output() { value, weight: txin.segwit_weight().to_wu(), input_count: 1, - is_segwit: true, }) .collect::>(); @@ -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::>(); @@ -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::>(); @@ -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() + ); +}