fix!: price every transaction as segwit and drop Candidate::is_segwit by evanlinjin · Pull Request #63 · bitcoindevkit/coin-select

evanlinjin · 2026-08-12T07:27:34Z

#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:

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. TXIN_BASE_WEIGHT now includes the empty witness count byte (165, was 164) and TX_FIXED_FIELD_WEIGHT includes the segwit marker and flag (34, was 32).
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. TR_KEYSPEND_SATISFACTION_WEIGHT follows this contract (65, was 66), and new_tr_keyspend is just new with it. TR_KEYSPEND_TXIN_WEIGHT is unchanged.

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

…ough every call 57114a8 Give `CoinSelector` its target instead of threading it through every call (志宇) Pull request description: ## Why A `CoinSelector` is built for one target and evaluated against it throughout — yet every method took the target as a parameter. Nothing stopped `cs.excess(target_a, drain)` being followed by `cs.is_funded(target_b)`, and the metrics' correctness arguments (e.g. `LowestFee::bound`'s proof that a changeless superset always costs more) are all stated at a *fixed* target, held together by convention rather than by types. The crate had already reached this conclusion one layer down: `BnbIter` stored the target as a field, took it once in `new`, and re-passed it into `metric.score`/`metric.bound` at every node. This moves the binding up to where it belongs and deletes the re-threading. It also unblocks follow-up work: with the selector knowing its target feerate, ancestor-aware CPFP pricing (#24) can be derived internally at the right rate instead of being validated at every call site. That branch is based on this one. ## What `CoinSelector::new(candidates, target)` owns the target; `CoinSelector::target()` exposes it for metrics that read it. Twenty signatures **lose** a parameter: fifteen public methods (`excess`, `implied_fee`, `is_funded`, `drain`, `select_until_target_met`, the four `*_excess`, …), `bnb_solutions`/`run_bnb`, and all three `BnbMetric` methods. Across the existing tests and benches, no selector was ever evaluated against more than one target — the per-call flexibility had no consumer. ## Breaking changes External `BnbMetric` implementations drop the `target: Target` parameter: ```rust fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>; fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>; fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain; ``` Call sites move the target from each method call to `CoinSelector::new`. To evaluate a second target, `CoinSelector::with_target` copies the selector — selection, bans and candidate order — onto it. Arguments that merely restated part of the target go the same way: `weight` and `implied_feerate` no longer take `TargetOutputs`, `fee` no longer takes `target_value`, and `effective_value` and `select_all_effective` no longer take a `FeeRate`. The unreleased CHANGELOG entry that described the previous target-as-a-parameter API is rewritten to describe this one. ## Stack This is the base of a stack: #59 → #63 → #84. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: noahjoeris: ACK 57114a8 Tree-SHA512: bd51a7b4a1a91881e60efffa8ca57b8f2eb52be9aa03162b2420c5f1b24145375450cfdc96e43b0dc40de4ae56be8317fe77a211b22aa2972519a5473b385486

`CoinSelector::input_weight` added the 1 WU empty witness once per legacy candidate, undercounting candidates that group several legacy inputs in a segwit transaction. Instead of tracking segwit and legacy input counts to price mixed transactions exactly, always price the transaction as segwit: `Candidate::weight` is the input's segwit serialized weight (`TxIn::segwit_weight`), `TXIN_BASE_WEIGHT` includes the empty witness count byte, and `TX_FIXED_FIELD_WEIGHT` includes the segwit marker and flag. An all-legacy transaction is overestimated by 2 WU plus 1 WU per input, which never undershoots the target feerate. `Candidate::new` loses its `is_segwit` argument, and `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. `TR_KEYSPEND_SATISFACTION_WEIGHT` follows that contract and drops the witness item count, so `new_tr_keyspend` is `new` with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKrCjaVEA5NaeQukuhvaWW

The exclusion branch banned every following candidate with the same `(value, weight)`, a tuple that repeated `Candidate`'s fields and left out `input_count`: candidates of equal value and weight but different input counts were banned together, as if interchangeable. `Candidate` now derives `PartialEq`, `Eq`, `PartialOrd` and `Ord`, and branch and bound compares whole candidates. The banning only works on a contiguous run, so `sort_candidates_by_descending_value_pwu` tie-breaks on the whole candidate too, keeping identical candidates next to each other. Equality and order are both defined by the struct, so a new field is picked up by both. A test pins the interleaved case: 16 candidates of equal value and weight with alternating input counts take 52 rounds, and 35,749 without the tie-break. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKrCjaVEA5NaeQukuhvaWW

evanlinjin · 2026-10-01T23:51:25Z

@LLFourn Thanks for the review. Working through it, I decided exact legacy pricing wasn't worth its complexity, so this now prices every tx as segwit. It's much easier to reason about: one meaning for weight, and nothing that depends on script type. A tx that spends only legacy inputs overpays slightly (2 WU + 1 WU per input), and never underpays. Legacy transaction creators are already making a fee trade-off anyway.

LLFourn

ACK 6a9bed9

evanlinjin requested a review from LLFourn August 12, 2026 07:32

This was referenced Aug 12, 2026

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

Closed

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

Closed

feat!: ancestor-aware coin selection (CPFP bump via SelectionProblem) #64

Closed

evanlinjin force-pushed the fix/legacy-pricing-mix branch 2 times, most recently from 9d6072b to 85523d8 Compare September 22, 2026 08:34

evanlinjin self-assigned this Sep 22, 2026

This was referenced Sep 22, 2026

feat!: depth-first branch and bound, and remove the Changeless metric #84

Open

feat!: depth-first branch and bound, and remove the changeless metrics #73

Closed

evanlinjin force-pushed the fix/legacy-pricing-mix branch from 85523d8 to 6a0d396 Compare September 22, 2026 09:52

This was referenced Sep 22, 2026

Give CoinSelector its target instead of threading it through every call #59

Merged

feat!: ancestor-aware coin selection (CPFP bump via SelectionProblem) #85

Open

evanlinjin added this to the bdk_coin_select v0.5 milestone Sep 22, 2026

LLFourn requested changes Sep 23, 2026

View reviewed changes

Comment thread src/coin_selector.rs Outdated

Comment thread src/bnb.rs Outdated

Comment thread src/coin_selector.rs Outdated

Comment thread CHANGELOG.md Outdated

Comment thread tests/weight.rs Outdated

Comment thread README.md Outdated

evanlinjin force-pushed the fix/legacy-pricing-mix branch from 6a0d396 to e51ab23 Compare September 23, 2026 07:13

evanlinjin force-pushed the fix/legacy-pricing-mix branch from e51ab23 to 26af408 Compare September 25, 2026 03:55

evanlinjin marked this pull request as draft September 25, 2026 05:10

evanlinjin force-pushed the fix/legacy-pricing-mix branch from 26af408 to ad5e6a2 Compare September 25, 2026 05:10

evanlinjin changed the title ~~fix!: exact weight pricing for grouped legacy inputs in mixed txs~~ fix!: price every transaction as segwit and drop Candidate::is_segwit Sep 25, 2026

evanlinjin force-pushed the fix/legacy-pricing-mix branch 10 times, most recently from 4e59ae8 to 3731b8f Compare October 1, 2026 06:08

evanlinjin force-pushed the fix/legacy-pricing-mix branch 3 times, most recently from e15cbb6 to a2756cb Compare October 1, 2026 22:39

evanlinjin marked this pull request as ready for review October 1, 2026 23:24

evanlinjin and others added 2 commits October 1, 2026 23:45

evanlinjin force-pushed the fix/legacy-pricing-mix branch from 50ed68f to 6a9bed9 Compare October 1, 2026 23:47

LLFourn approved these changes Oct 2, 2026

View reviewed changes

evanlinjin merged commit dcb1ce9 into bitcoindevkit:master Oct 2, 2026
5 checks passed

This was referenced Oct 19, 2026

BnB exclusion dedup keys on (value, weight) only, merging candidates with different is_segwit/input_count #58

Closed

LowestFee::bound's resize lower bound is inadmissible: greedy prefix's segwit corrections inflate ideal_fee #56

Closed

Ansh-699 mentioned this pull request Oct 21, 2026

DrainWeights::dust_threshold is below Bitcoin Core's dust limit for P2TR change #81

Open

Navigation Menu

Repository navigation

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

fix!: price every transaction as segwit and drop Candidate::is_segwit#63
evanlinjin merged 2 commits into
bitcoindevkit:masterfrom
evanlinjin:fix/legacy-pricing-mix

evanlinjin commented Aug 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Oct 1, 2026 •

edited

Loading

Uh oh!

LLFourn left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

evanlinjin commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LLFourn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

evanlinjin commented Aug 12, 2026 •

edited

Loading

evanlinjin commented Oct 1, 2026 •

edited

Loading

LLFourn left a comment •

edited

Loading