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

[evanlinjin]

Bug: CoinSelector::input_weight undercounts the transaction weight when a Candidate represents more than one input.

In a segwit transaction, every input serializes a witness — a legacy input serializes an empty one, costing 1 WU. A segwit input already counts its witness in Candidate::weight; a legacy input doesn't, so input_weight compensated:

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

But the cost is 1 WU per legacy input, not per candidate:

• candidate = 3 legacy inputs → needs +3, got +1
• candidate = 2 legacy + 1 segwit input → needs +2, got +0 (is_segwit is true, so the branch is skipped)
• candidate = 1 legacy input → needs +1, got +1 ✅ … the only case the tests exercised

Fix: put the byte into weight when the candidate is built, while each input is still individually visible. Candidate::weight now means the weight of the inputs as serialized in a segwit transaction: legacy inputs include their empty-witness byte.

• A multi-input candidate is now a plain sum, mixed script types included.
• is_segwit only decides whether the tx pays the 2 WU witness marker + flag.
• If the selection ends up with no segwit inputs, the tx has no witness section at all, so input_weight gives 1 WU per input back.

⚠ Silent breaking change: hand-built candidates that use txin.legacy_weight().to_wu() still compile but are now 1 WU short per legacy input — use txin.legacy_weight().to_wu() + 1. Candidate::new already does this for you.

Test: one real tx (2 legacy + 1 segwit inputs) weighed under three groupings — one candidate per input, the legacy pair grouped, all three in one candidate. Each must equal tx.weight(); all three fail on unfixed master.

🤖 Generated with Claude Code

[LLFourn]

I think the fix is right — it just needs a stronger invariant around segwit accounting to go with it.

The diagnosis is worth restating plainly: the empty-witness byte costs 1 WU per legacy input, and input_weight was paying it once per candidate. Any candidate representing more than one input was mispriced — a group of N legacy inputs short by N−1, and a group mixing script types short by its entire legacy count, since is_segwit == true suppressed the adjustment altogether. The tests only ever exercised one-input candidates, the single case where the two readings coincide.

Moving the byte into Candidate::weight at construction, where each input is still individually visible, is the right shape for the fix. Keeping input_weight conditioned on the selection is right too: whether the transaction has a witness section genuinely depends on which inputs are selected, and weight() is documented as the weight of the transaction implied by the selection. It should stay exact — is_within_max_weight and is_funded are hard gates, and an approximation there turns into spurious MaxWeightExceeded and InsufficientFunds results on transactions that are actually fine.

Three things to address.

1. min_input_weight is no longer admissible

This is the one correctness issue. min_input_weight (src/coin_selector.rs:432) still returns candidate.weight directly, but the refund means that is no longer a lower bound on marginal cost:

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

(Δvarint ≥ 0 is the CompactSize step, zero away from a boundary.)

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

c.weight − c.input_count sits below all three rows, so it serves as a context-independent lower bound with no change to input_weight and no new state. It is looser than necessary once the transaction is segwit, but bounds are allowed to underestimate — unlike the weight itself, which is consumed by hard gates and must stay exact.

Worth noting tests/bnb.rs:20 pins is_segwit = true on every generated candidate ("you can't actually lower bound things easily with how segwit inputs interfere with their weights"), so the regressed path is one the BnB proptests structurally cannot reach.

2. weight >= input_count should be stated and checked

The refund introduces the first subtraction in input_weight, and weight is a public field with no documented minimum, so it can underflow — repro inline; debug panics, release wraps to ~1.8e19 and silently poisons every downstream weight(), excess() and BnB bound.

This isn't an argument against subtracting. Under the new convention weight >= input_count is a genuine invariant: every input serializes at least its empty-witness byte. It just needs stating on the field and a debug_assert! in CoinSelector::new. That is also exactly the invariant the lower bound in (1) relies on, so documenting it does double duty — it's what makes c.weight − c.input_count well-defined.

3. Test coverage for the refund

grouping_inputs_does_not_change_weight asserts the right property, but every grouping in it includes the segwit input, so is_segwit_tx is true on each iteration and the refund arm is never reached with input_count > 1. The only all-legacy coverage is legacy_three_inputs, one input per candidate, where a per-candidate refund and a per-input refund are indistinguishable — precisely the confusion this PR exists to fix.

Adding the three inputs of legacy_three_inputs grouped into a single Candidate { input_count: 3, is_segwit: false }, asserting tx.weight(), closes that.

Also

README.md is 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.

[LLFourn]

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.

[LLFourn]

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.

[LLFourn]

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.

[LLFourn]

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().

[evanlinjin]

Replaced by #63