fix!: price every transaction as segwit and drop Candidate::is_segwit

[evanlinjin]

#84 is stacked on this one.

Fixes CoinSelector::input_weight undercounting candidates that group multiple legacy inputs in a segwit transaction (where each legacy input serializes a 1 WU empty witness). Previously it added 1 WU per non-segwit candidate, so a candidate grouping N legacy inputs came out N-1 WU short.

Instead of tracking segwit and legacy input counts to price mixed transactions exactly (the previous revision of this PR), this prices every transaction as segwit:

1. Candidate::is_segwit is removed. Candidate::weight is the input's segwit-serialized weight (TxIn::segwit_weight), so legacy inputs include their 1 WU empty witness, and input_weight always counts the 2 WU witness header.
2. Candidate::new(value, satisfaction_weight, is_segwit) becomes Candidate::new(value, satisfaction_weight). satisfaction_weight is now the weight over an unsatisfied TxIn::default(), which is exactly miniscript's Descriptor::max_weight_to_satisfy for every script type.

Trade-off: a transaction that spends only legacy inputs is overestimated by 2 WU plus 1 WU per input (under 1 sat per input at 1 sat/vB). It only ever overpays, so it never undershoots the target feerate. In return:

• Mixed and grouped candidates are priced exactly with no per-type bookkeeping, and a candidate can freely mix legacy and segwit inputs.
• One constructor with one contract, so a segwit max_weight_to_satisfy can't come out 1 WU short.
• Branch and bound's duplicate-banning stays on (value, weight), so there's no second notion of candidate equality that the sort has to keep contiguous.
• The is_segwit lower-bounding hack in tests/bnb.rs goes away.

Note: This replaces #62 and #61. #61 introduced empty_witness_refund, and #62 forbade mixing legacy and segwit inputs in a candidate. Both are unnecessary once every transaction is priced as segwit.

🤖 Generated with Claude Code

[LLFourn]

The contract here is correct against the serialization rules, but it isn't what the usual source of this number returns. rust-miniscript's Descriptor::max_weight_to_satisfy is defined as txin.segwit_weight() - TxIn::default().segwit_weight(), and TxIn::default() already carries the 1 WU empty-witness count, so it excludes scriptWitnessLen. With miniscript 12.3.7:

descriptor	max_weight_to_satisfy	what this doc asks for
pkh → new_legacy	428	428 ✓
wpkh → new_segwit	107	108
sh(wpkh) → new_segwit	199	200
tr → new_segwit	66	67

So new_legacy(v, desc.max_weight_to_satisfy()) is exact, and new_segwit(v, desc.max_weight_to_satisfy()) is 1 WU short on every segwit input. That's the same shape of bug this PR fixes, and the compiler won't flag it.

The split constructors let us make both take the same number. Define satisfaction_weight as the weight the input adds over an unsatisfied TxIn (which is exactly max_weight_to_satisfy), and have new_segwit add the witness-count byte itself:

pub fn new_segwit(value: u64, satisfaction_weight: u64) -> Candidate {
    Candidate {
        value,
        // `TxIn::default()`'s empty witness count is 1 WU, which the satisfaction weight excludes.
        weight: TXIN_BASE_WEIGHT + 1 + satisfaction_weight,
        segwit_count: 1,
        legacy_count: 0,
    }
}

Then new_tr_keyspend would pass TR_KEYSPEND_SATISFACTION_WEIGHT - 1 (or the constant drops its witness_len term), and both docs can say "e.g. Descriptor::max_weight_to_satisfy". If you'd rather keep the current contract, the doc should at least say that max_weight_to_satisfy needs + 1 here.

[LLFourn]

This loop only bans a contiguous run of identical candidates, so it relies on the sort placing identical candidates next to each other. Before this PR that held because (value, weight) also fixes the sort key (value_pwu, value). Now the ban key includes the counts and the sort key doesn't, so equal-value, equal-weight candidates of different script types can end up interleaved, and nothing gets co-banned. In a LowestFee probe (candidates of 10,000 sats / 400 WU, half segwit and half legacy, 10 sat/vB, 78,000 sat output):

candidates	interleaved	grouped by type
16	24,541 rounds	46
20	293,939	55
24	>2,000,000	55

The real problem is two independent definitions of "same candidate". The tuple here also repeats Candidate's field list, which is how input_count was left out of the old key: two candidates of equal value and weight with 1 and 2 inputs were banned together. I'd define the equivalence in one place. Add PartialEq, Eq to Candidate's derive (additive, non-breaking) and compare whole candidates:

Suggested change

	// for the exclusion branch, we keep banning if candidates have the same weight, value and
	// input counts. The counts matter because a segwit and a legacy input of equal weight
	// change the tx weight differently.
	let mut is_first_ban = true;
	let mut exclusion_cs = cs.clone();
	let to_ban = (next.value, next.weight);
	let to_ban = (
	next.value,
	next.weight,
	next.segwit_count,
	next.legacy_count,
	);
	for (next_index, next) in cs.unselected() {
	if (next.value, next.weight) != to_ban {
	if (
	next.value,
	next.weight,
	next.segwit_count,
	next.legacy_count,
	) != to_ban
	{
	break;
	}
	// for the exclusion branch, we keep banning identical candidates since selecting any one of
	// them is equivalent to selecting another. This relies on the sort placing identical
	// candidates next to each other.
	let mut is_first_ban = true;
	let mut exclusion_cs = cs.clone();
	let to_ban = next;
	for (next_index, next) in cs.unselected() {
	if next != to_ban {
	break;
	}

Then make sort_candidates_by_descending_value_pwu tie-break on every field of the candidate, not just value, so identical candidates are always contiguous. Please also pin the interleaved case with a round-count test (e.g. the 16-candidate case above must finish within a small bound). I checked that the suggestion builds and passes the existing BnB tests.

[LLFourn]

These two paragraphs contradict each other: the first says scriptWitnessLen is always included, the second says not for legacy inputs. Say it once, per input type:

Suggested change

	/// Total weight of including this/these UTXO(s).
	/// `txin` fields: `prevout`, `nSequence`, `scriptSigLen`, `scriptSig`, `scriptWitnessLen`,
	/// `scriptWitness` should all be included.
	///
	/// For legacy inputs, do *not* include the `scriptWitnessLen` byte: a legacy input only
	/// serializes an (empty) witness when the transaction has a witness section, and
	/// [`CoinSelector::input_weight`] adds that 1 WU per legacy input once any segwit input is
	/// selected.
	/// Total weight of including this/these UTXO(s): `prevout`, `nSequence`, `scriptSigLen` and
	/// `scriptSig` for every input, plus `scriptWitnessLen` and `scriptWitness` for segwit inputs only.
	///
	/// Legacy inputs must not include the 1 WU empty witness they serialize in a segwit transaction;
	/// [`CoinSelector::input_weight`] adds it per legacy input once any segwit input is selected.

[LLFourn]

Callers migrating from Candidate::new need to know that what they pass has changed, not only the name. The old doc said satisfaction_weight covers scriptSigLen + scriptSig + scriptWitnessLen + scriptWitness, but TXIN_BASE_WEIGHT already includes the scriptSigLen byte, so anyone who followed it over-counted by 4 WU. Please add a migration line after the last sentence, e.g.:

Candidate::new(value, w, true) becomes Candidate::new_segwit(value, w) and Candidate::new(value, w, false) becomes Candidate::new_legacy(value, w). w no longer includes the 1-byte scriptSigLen, which TXIN_BASE_WEIGHT already counts; the old docs said it did.

(Adjust the wording if the new_segwit contract changes per the comment there.)

[LLFourn]

Nothing pins the new constructor docs, and they're the part callers can get wrong without the compiler noticing. The mixed tx already exercises the tricky cases: a 253-byte scriptSig (3-byte varint) and a P2WPKH witness. Something like this checks each constructor against the real input weight (I ran it locally and it passes against the current docs):

#[test]
fn new_segwit_and_new_legacy_match_real_inputs() {
    let tx = legacy_three_input_tx_mixed();
    for (i, txin) in tx.input.iter().enumerate() {
        let script_sig = txin.script_sig.len() as u64;
        let extra_script_sig_len = if script_sig < 253 { 0 } else { 2 };
        let script_sig_weight = (script_sig + extra_script_sig_len) * 4;
        if i == 1 {
            let candidate = Candidate::new_segwit(0, txin.witness.size() as u64 + script_sig_weight);
            assert_eq!(candidate.weight, txin.segwit_weight().to_wu());
        } else {
            let candidate = Candidate::new_legacy(0, script_sig_weight);
            assert_eq!(candidate.weight, txin.legacy_weight().to_wu());
        }
    }
}

Nit while you're here: legacy_three_inputs and legacy_three_inputs_one_segwit still inline the same hex that THREE_INPUT_LEGACY_TX_HEX now holds. They could use legacy_three_input_tx() / legacy_three_input_tx_mixed().

[LLFourn]

This still says weight includes the witness for every input. A caller who follows it for a legacy candidate adds the empty-witness byte themselves, and input_weight then adds it again, overpricing each legacy input by 1 WU. The example candidates are segwit, but this comment is the first thing people read about weight. Please state the legacy exception here, in the same words as the Candidate::weight docs.