Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
evanlinjin wants to merge 1 commit into from
+127 −35
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
62 changes: 43 additions & 19 deletions src/coin_selector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

@LLFourn LLFourn Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refund itself is right. What's missing is that min_input_weight (src/coin_selector.rs:432) still derives from candidate.weight directly, so it stops being the "lower bound on the extra input weight any descendant selection must take on" its doc promises.

Marginal cost of adding candidate c, with Δvarint ≥ 0 the CompactSize step:

selection c marginal cost
all-legacy legacy c.weight − c.input_count + Δvarint
all-legacy segwit c.weight + 2 + #legacy_selected + Δvarint
has segwit either c.weight + Δvarint

c.weight exceeds the first row, so both Target::max_weight prunes — src/metrics/lowest_fee.rs:170 and :264 — can discard a branch that holds a valid within-cap solution, and the comment above :264 arguing the fractional relaxation "never prunes a branch with an (integer) within-cap solution" no longer holds.

c.weight − c.input_count sits below all three rows, so it works as a context-independent lower bound. Looser than necessary once the tx is segwit, but that's allowed — bounds may underestimate, whereas input_weight feeds hard gates (is_within_max_weight, is_funded) and needs to stay exact.

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

@LLFourn LLFourn Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can underflow. weight is a public field with no documented minimum:

let candidates: Vec<Candidate> = (0..5)
    .map(|_| Candidate { value: 1000, weight: 1, input_count: 2, is_segwit: false })
    .collect();
let mut cs = CoinSelector::new(&candidates);
cs.select_all();
cs.input_weight();
panicked at src/coin_selector.rs:160:9: attempt to subtract with overflow

Debug panics; release wraps to ~1.8e19 and quietly poisons every downstream weight(), excess(), implied_feerate() and BnB bound — the worse outcome. The crate's own generators already build candidates in this shape (tests/changeless.rs:16, tests/common.rs:271); they stay clear only because the weight sums happen to outrun the input counts.

Not an argument against subtracting, though — weight >= input_count is a genuine invariant of the new convention. It just needs stating on the field and a debug_assert! in CoinSelector::new.

}

/// Absolute value sum of all selected inputs.
Expand Down Expand Up @@ -894,13 +890,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 including this/these UTXO(s), **as serialized in a segwit transaction**.

@LLFourn LLFourn Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth stating the invariant this convention implies: weight >= input_count. Under "segwit-serialized" every input contributes at least its empty-witness byte, so a correctly-built candidate can't violate it — but it's what makes the refund on line 160 safe and what makes the c.weight − c.input_count lower bound well-defined, and right now it's neither documented nor checked.

Separately, README.md needs a pass. It's include_str!'d as the crate-level docs and its weight: comment — "the total weight of the input(s) including their witness/scriptSig" — is where a user goes to learn this convention. It doesn't mention the empty-witness byte under the old definition or the new one.

///
/// 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.
///
/// # Constructing this from the [`miniscript`] crate
///
/// The `Plan::satisfaction_weight` method assumes that all legacy inputs belong to non-segwit
/// transactions and therefore the 1 WU that records an empty witness size of 0 is not counted.
/// It also excludes `prevout` and `nSequence`, so compute each input's weight as:
///
/// ```text
/// TXIN_BASE_WEIGHT + plan.satisfaction_weight() + plan.witness_version().is_none() as u64
/// ```
///
/// [`Candidate::new`] already does this for single-input candidates -- pass in
/// `plan.satisfaction_weight()` unadjusted.
///
/// [`miniscript`]: https://docs.rs/miniscript
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,
}

Expand All @@ -914,9 +936,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,
Expand Down
99 changes: 83 additions & 16 deletions tests/weight.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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,
})
Expand Down Expand Up @@ -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();
Expand All @@ -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()
Expand All @@ -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();
Expand 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() {

@LLFourn LLFourn Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good property to assert, but every grouping here includes the segwit input, so is_segwit_tx is true on each iteration and the refund arm — the actually-new logic — is never reached with input_count > 1.

The only all-legacy coverage is legacy_three_inputs, which is one input per candidate, so input_count equals the candidate count and a per-candidate refund is indistinguishable from a per-input one. That's precisely the confusion this PR exists to fix.

Worth adding the three inputs of legacy_three_inputs grouped into a single Candidate { input_count: 3, is_segwit: false }, asserting tx.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
Expand Down
Loading