From 7b24b0da9b53afe327683834a45e8c7c2cf11f71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Thu, 6 Aug 2026 12:36:59 +0000 Subject: [PATCH] fix: define `Candidate::weight` as segwit-serialized, fixing input groups In a segwit transaction every legacy input still serializes an empty witness. `input_weight` accounted for that byte by adding it once per non-segwit *candidate*, which is only correct when a candidate represents a single input. A candidate grouping N legacy inputs was undercounted by N-1 weight units, and one mixing legacy and segwit spends by its full legacy count -- `is_segwit` had to be true for the marker and flag, which suppressed the adjustment its legacy inputs still needed. Fix it at the definition instead of the use. `Candidate::weight` now means the input weight as serialized in a segwit transaction, so a legacy input counts its empty witness up front, where each input is still visible individually. `is_segwit` is then only the transaction-level marker and flag question, and grouping inputs of either script type into one candidate works. `input_weight` correspondingly stops adjusting per candidate: if nothing selected is segwit the transaction has no witness section at all, so every input takes its byte back. Existing tests only used single-input candidates, which is why this went unnoticed. The new test weighs one transaction under three groupings of the same three inputs -- one per candidate, the two legacy inputs shared, and all three shared -- and asserts all agree with the real serialized weight. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + src/coin_selector.rs | 47 ++++++++++++--------- tests/weight.rs | 99 +++++++++++++++++++++++++++++++++++++------- 3 files changed, 112 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e5daf0..d2f00a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - **Breaking:** Removed the `BnbMetric` tuple implementations (`impl BnbMetric for ((A, f32), ...)`). Weighted composition of independent metrics is no longer supported; the only composition still provided is the changeless constraint, now expressed as `Changeless`. If you relied on tuples to blend multiple objectives, there is no drop-in replacement. - **Breaking:** `CoinSelector::selected_indices` and `CoinSelector::banned` now return `&Bitset` instead of `&BTreeSet`. `Bitset` exposes `contains`/`len`/`is_empty`/`iter` (#46) - Replace the internal `Cow`/`Cow<[usize]>` selection state with a `Bitset` and an `Arc`-shared candidate order, making the per-branch clones in branch-and-bound substantially cheaper (#46) +- **Breaking:** `Candidate::weight` is now defined as the input weight *as serialized in a segwit transaction*, so a legacy input must count the 1 weight unit its empty witness takes. `Candidate::new` adds this for you, but hand-built `Candidate` literals do not — code passing `TxIn::legacy_weight()` still compiles and is now short by 1 per legacy input. This fixes `CoinSelector::input_weight` for candidates representing more than one input: it previously added the empty-witness byte once per *candidate*, undercounting a group of N legacy inputs by N-1, and a group mixing legacy and segwit spends by its full legacy count (since `is_segwit` being `true` suppressed the adjustment entirely). Candidates may now freely group inputs of either script type. - Fix compilation error when building with `--no-default-features` (#36) # 0.4.0 diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 604abd8..9f0c156 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -143,25 +143,21 @@ impl<'a> CoinSelector<'a> { /// inputs. 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(); + let selected_weight: u64 = self.selected().map(|(_, wv)| wv.weight).sum(); + + // Candidate weights assume a segwit tx, where every input serializes a witness. A tx with + // no segwit inputs is serialized without a witness section at all, so the marker and flag + // are not paid for and each input takes its empty witness back. + let (witness_header_weight, empty_witness_refund) = match is_segwit_tx { + true => (2, 0), + false => (0, input_count as u64), + }; - input_varint_weight + selected_weight + witness_header_extra_weight + input_varint_weight + selected_weight + witness_header_weight - empty_witness_refund } /// Absolute value sum of all selected inputs. @@ -894,13 +890,24 @@ 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 including this/these UTXO(s), **as serialized in a segwit transaction**. + /// + /// Include these `txin` fields for every input: `prevout`, `nSequence`, `scriptSigLen`, + /// `scriptSig`, `scriptWitnessLen`, `scriptWitness`. A legacy input has no witness, but in a + /// segwit transaction it still serializes an empty one, so count 1 weight unit for it. + /// + /// If the selection turns out to hold no segwit inputs at all, + /// [`CoinSelector::input_weight`] takes those bytes back off — a transaction with no witnesses + /// is serialized without a witness section. 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. + /// + /// One segwit spend anywhere in the transaction adds the witness marker and flag. Whether the + /// *individual* inputs here are segwit is already priced into [`weight`]. + /// + /// [`weight`]: Self::weight pub is_segwit: bool, } @@ -914,9 +921,11 @@ impl Candidate { /// Create a new [`Candidate`] that represents a single input. /// /// `satisfaction_weight` is the weight of `scriptSigLen + scriptSig + scriptWitnessLen + - /// scriptWitness`. + /// scriptWitness`. For a legacy input that is just the `scriptSig` part; the empty witness a + /// segwit transaction would give it is added here, per [`Candidate::weight`]. pub fn new(value: u64, satisfaction_weight: u64, is_segwit: bool) -> Candidate { - let weight = TXIN_BASE_WEIGHT + satisfaction_weight; + let empty_witness_weight = !is_segwit as u64; + let weight = TXIN_BASE_WEIGHT + satisfaction_weight + empty_witness_weight; Candidate { value, weight, diff --git a/tests/weight.rs b/tests/weight.rs index 6a8dbb5..6959bb7 100644 --- a/tests/weight.rs +++ b/tests/weight.rs @@ -1,7 +1,13 @@ #![allow(clippy::zero_prefixed_literal)] use bdk_coin_select::{Candidate, CoinSelector, Drain, DrainWeights, TargetOutputs}; -use bitcoin::{consensus::Decodable, ScriptBuf, Transaction}; +use bitcoin::{consensus::Decodable, ScriptBuf, Transaction, TxIn}; + +/// Weight of a legacy input under the `Candidate::weight` convention, which assumes a segwit +/// transaction: its `legacy_weight()` plus the one byte its empty witness serializes to. +fn legacy_weight_in_segwit_tx(txin: &TxIn) -> u64 { + txin.legacy_weight().to_wu() + 1 +} fn hex_val(c: u8) -> u8 { match c { @@ -121,7 +127,7 @@ fn legacy_three_inputs() { .zip(input_values) .map(|(txin, value)| Candidate { value, - weight: txin.legacy_weight().to_wu(), + weight: legacy_weight_in_segwit_tx(txin), input_count: 1, is_segwit: false, }) @@ -151,10 +157,9 @@ fn legacy_three_inputs() { ); } -#[test] -fn legacy_three_inputs_one_segwit() { - // FROM https://mempool.space/tx/5f231df4f73694b3cca9211e336451c20dab136e0a843c2e3166cdcb093e91f4 - // Except we change the middle input to segwit +/// FROM https://mempool.space/tx/5f231df4f73694b3cca9211e336451c20dab136e0a843c2e3166cdcb093e91f4 +/// Except we change the middle input to segwit +fn legacy_tx_with_middle_input_segwit() -> Transaction { let tx_bytes = hex_decode("0100000003fe785783e14669f638ba902c26e8e3d7036fb183237bc00f8a10542191c7171300000000fdfd00004730440220418996f20477d143d02ad47e74e5949641b6c2904159ab7c592d2cfc659f9bd802205b18f18ac86b714971f84a8b74a4cb14ad5c1a5b9d0d939bb32c6ae4032f4ea10148304502210091296ff8dd87b5ebfc3d47cb82cfe4750d52c544a2b88a85970354a4d0d4b1db022069632067ee6f30f06145f649bc76d5e5d5e6404dbe985e006fcde938f778c297014c695221030502b8ade694d57a6e86998180a64f4ce993372830dc796c3d561ad8b2a504de210272b68e1c037c4630eff7ea5858640cc0748e36f5de82fb38529ef1fd0a89670d2103ba0544a3a2aa9f2314022760b78b5c833aebf6f88468a089550f93834a2886ed53aeffffffff7e048a7c53a8af656e24442c65fe4c4299b1494f6c7579fe0fd9fa741ce83e3279000000fc004730440220018fa343acccd048ed8f8f179e1b6ae27435a41b5fb2c1d96a5a772777acc6dc022074783814f2100c6fc4d4c976f941212be50825814502ca0cbe3f929db789979e0147304402206373f01b73fb09876d0f5ee3087e0614cab3be249934bc2b7eb64ee67f53dc8302200b50f8a327020172b82aaba7480c77ecf07bb32322a05f4afbc543aa97d2fde8014c69522103039d906b2494e310f6c7774c98618be552720d04781e073dd3ff25d5906f22662103d82026baa529619b103ec6341d548a7eb6d924061a8469a7416155513a3071c12102e452bc4aa726d44646ba80db70465683b30efde282a19aa35c6029ae8925df5e53aeffffffffef80f0b1cc543de4f73d59c02a3c575ae5d0af17c1e11e6be7abe3325c777507ad000000fdfd00004730440220220fee11bf836621a11a8ea9100a4600c109c13895f11468d3e2062210c5481902201c5c8a462175538e87b8248e1ed3927c3a461c66d1b46215641c875e86eb22c4014830450221008d2de8c2f20a720129c372791e595b9602b1a9bce99618497aec5266148ffc1302203a493359d700ed96323f8805ed03e909959ff0f22eff359028db6861486b1555014c6952210374a4add33567f09967592c5bcdc3db421fdbba67bac4636328f96d941da31bd221039636c2ffac90afb7499b16e265078113dfb2d77b54270e37353217c9eaeaf3052103d0bcea6d10cdd2f16018ea71572631708e26f457f67cda36a7f816a87f7791d253aeffffffff04977261000000000016001470385d054721987f41521648d7b2f5c77f735d6bee92030000000000225120d0cda1b675a0b369964cbfa381721aae3549dd2c9c6f2cf71ff67d5bc277afd3f2aaf30000000000160014ed2d41ba08313dbb2630a7106b2fedafc14aa121d4f0c70000000000220020e5c7c00d174631d2d1e365d6347b016fb87b6a0c08902d8e443989cb771fa7ec00000000"); let mut tx = Transaction::consensus_decode(&mut tx_bytes.as_slice()).unwrap(); tx.input[1].script_sig = ScriptBuf::default(); @@ -163,7 +168,24 @@ fn legacy_three_inputs_one_segwit() { hex_decode("3045022100bdc115b86e9c863279132b4808459cf9b266c8f6a9c14a3dfd956986b807e3320220265833b85197679687c5d5eed1b2637489b34249d44cf5d2d40bc7b514181a5101"), hex_decode("02077741a668889ce15d59365886375aea47a7691941d7a0d301697edbc773b45b"), ].into(); - let input_values = vec![022_680_000, 006_558_175, 006_558_200]; + tx +} + +/// Input values of [`legacy_tx_with_middle_input_segwit`], in input order. +const MIXED_TX_INPUT_VALUES: [u64; 3] = [022_680_000, 006_558_175, 006_558_200]; + +fn target_outputs_of(tx: &Transaction) -> TargetOutputs { + TargetOutputs { + value_sum: tx.output.iter().map(|output| output.value.to_sat()).sum(), + weight_sum: tx.output.iter().map(|output| output.weight().to_wu()).sum(), + n_outputs: tx.output.len(), + } +} + +#[test] +fn legacy_three_inputs_one_segwit() { + let tx = legacy_tx_with_middle_input_segwit(); + let input_values = MIXED_TX_INPUT_VALUES; let candidates = tx .input .iter() @@ -174,22 +196,17 @@ fn legacy_three_inputs_one_segwit() { Candidate { value, weight: if is_segwit { - txin.segwit_weight() + txin.segwit_weight().to_wu() } else { - txin.legacy_weight() - } - .to_wu(), + legacy_weight_in_segwit_tx(txin) + }, input_count: 1, is_segwit, } }) .collect::>(); - let target_ouputs = TargetOutputs { - value_sum: tx.output.iter().map(|output| output.value.to_sat()).sum(), - weight_sum: tx.output.iter().map(|output| output.weight().to_wu()).sum(), - n_outputs: tx.output.len(), - }; + let target_ouputs = target_outputs_of(&tx); let mut coin_selector = CoinSelector::new(&candidates); coin_selector.select_all(); @@ -200,6 +217,56 @@ fn legacy_three_inputs_one_segwit() { ); } +/// How inputs are grouped into candidates must not change the weight of the transaction the +/// selection implies. Each grouping below covers the same three inputs as +/// `legacy_three_inputs_one_segwit` does one-per-candidate. +#[test] +fn grouping_inputs_does_not_change_weight() { + let tx = legacy_tx_with_middle_input_segwit(); + let [v0, v1, v2] = MIXED_TX_INPUT_VALUES; + // Inputs 0 and 2 are legacy, input 1 is segwit. + let (l0, sw1, l2) = ( + legacy_weight_in_segwit_tx(&tx.input[0]), + tx.input[1].segwit_weight().to_wu(), + legacy_weight_in_segwit_tx(&tx.input[2]), + ); + + // The two legacy inputs share a candidate. + let legacy_grouped = [ + Candidate { + value: v0 + v2, + weight: l0 + l2, + input_count: 2, + is_segwit: false, + }, + Candidate { + value: v1, + weight: sw1, + input_count: 1, + is_segwit: true, + }, + ]; + + // All three share one candidate, mixing legacy and segwit spends. + let all_grouped = [Candidate { + value: v0 + v1 + v2, + weight: l0 + sw1 + l2, + input_count: 3, + is_segwit: true, + }]; + + let target_ouputs = target_outputs_of(&tx); + for candidates in [&legacy_grouped[..], &all_grouped[..]] { + let mut coin_selector = CoinSelector::new(candidates); + coin_selector.select_all(); + + assert_eq!( + coin_selector.weight(target_ouputs, DrainWeights::NONE), + tx.weight().to_wu() + ); + } +} + #[test] fn new_tr_keyspend_correct_weight() { // FROM https://mempool.space/tx/4936a1a4ea1a0085b9dc2a1d5b59d361f5b1b41241772f3e465153712b6d8dc0