fix: pay the empty-witness byte per legacy input, not per candidate

[LLFourn]

An alternative to #61 — same bug, same diagnosis, smaller fix.

The bug

CoinSelector::input_weight undercounts any Candidate representing more than one input. In a segwit transaction every input serializes a witness, and a legacy input's empty one costs 1 WU — but the code pays that byte once per candidate:

if is_segwit_tx && !candidate.is_segwit {
    weight += 1;
}

A group of N legacy inputs comes out N−1 short. Single-input candidates are correct, and they were the only shape the tests covered.

The fix

A candidate already carries input_count and is_segwit. That is enough to price this exactly, provided a group does not mix script types — so this PR states that restriction and uses it:

let witness_weight = match self.selected().any(|(_, wv)| wv.is_segwit) {
    true => {
        let legacy_inputs: u64 = self
            .selected()
            .filter(|(_, wv)| !wv.is_segwit)
            .map(|(_, wv)| wv.input_count as u64)
            .sum();
        2 + legacy_inputs
    }
    false => 0,
};

input_varint_weight + selected_weight + witness_weight

is_segwit is now documented as describing all of a candidate's inputs rather than merely asserting one of them is segwit. That is a precondition of the same kind the type already depends on — nothing enforces that weight or is_segwit are truthful in the first place — and grouping exists to spend UTXOs together, which gives no reason to straddle script types.

No struct change, no new field, and Candidate::weight keeps its meaning.

Why this rather than #61

#61 redefines Candidate::weight as the input weight as serialized in a segwit transaction — legacy inputs pre-pay the byte at construction — then refunds input_count when the selection turns out to have no segwit input. Three things follow from that refund:

Candidate::weight keeps its meaning here. Under #61 a caller passing txin.legacy_weight().to_wu() still compiles and is now 1 WU short per legacy input, so fees come out low and transactions may not relay; #61's own CHANGELOG flags this as a silent break. Nothing here changes what callers must compute, so there is no migration at all.

No subtraction, so no underflow. weight is public with no documented minimum, and #61's input_varint_weight + selected_weight - input_count can wrap:

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: attempt to subtract with overflow

Debug panics; release wraps to ~1.8e19 and silently poisons every downstream weight(), excess() and BnB bound.

min_input_weight stays admissible. Its doc promises "a lower bound on the extra input weight any descendant selection must take on". Under #61, adding a candidate to an all-legacy selection moves input_weight by c.weight − c.input_count + Δvarint, so c.weight becomes an over-estimate and both Target::max_weight prunes (metrics/lowest_fee.rs:170 and :264) can discard a branch holding a valid within-cap solution — the comment above :264 arguing the fractional relaxation "never prunes a branch with an (integer) within-cap solution" stops holding. Here every branch only adds, so c.weight is at or below true marginal cost in every context and neither prune needs to change.

The trade is that mixed-script groups stop being supported, where #61 makes them weigh correctly. That is a deliberate scope reduction: there is no use case for one in this repo, and the cost of supporting it is everything above.

Tests

Two groupings of one real mainnet transaction (2 legacy + 1 segwit inputs), each of which must equal tx.weight() and both of which fail on master:

• one candidate per input
• the legacy pair grouped into a single candidate

Plus an all-legacy transaction carried by a single 3-input candidate, guarding the no-witness-section path. That one is a guard rather than a regression test — it passes under the old per-candidate accounting too, since a transaction with no witness section adds nothing either way.

cargo test (debug and release), cargo fmt --check, cargo clippy -D warnings and --no-default-features all pass.

[evanlinjin]

Thank you for working on this.

The problem is that not being able to mix types in a candidate causes problems for bdk_tx.

1. We need a new error variant if someone tries to do this.
2. Internally, bdk_tx puts all the must-select inputs into one candidate (for RBF, CPFP, etc.) so we will need to rework this substantially after this change.

I propose replacing is_segwit and input_count fields with legacy_count and segwit_count.

[evanlinjin]

Replaced by #63