fix: define Candidate::weight as segwit-serialized, fixing multi-input candidates

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<M>`. 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<usize>`. `Bitset` exposes `contains`/`len`/`is_empty`/`iter` (#46)
 - Replace the internal `Cow<BTreeSet>`/`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

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::<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();
+        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,

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::<Vec<_>>();
 
-    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