feat!: depth-first branch and bound, and remove the Changeless metric by evanlinjin · Pull Request #84 · bitcoindevkit/coin-select

evanlinjin · 2026-09-22T08:49:14Z

Replaces #73. #73 had the same changes on top of the ancestor-aware selection stack. This PR puts them directly on master so they can land ahead of that stack.

Remove the `Changeless` metric

LowestFee already decides for itself whether a selection should carry change. It adds change only when doing so lowers the long-term fee, clears the dust threshold, and fits Target::max_weight. A separate changeless objective repeats that decision and then constrains it.

This is a breaking change. Callers that required a changeless transaction should use LowestFee and inspect the returned Drain.

Keep running sums of the selection in `CoinSelector`

input_weight() scanned the selected set twice and selected_value() once. Metrics call them several times per node through excess, rate_excess, implied_fee and similar methods. CoinSelector now tracks selected value, weight, and input count as running sums, updated in select and deselect.

A new proptest (running_sums_match_recompute) checks the sums against a recompute from the selected set after random select/deselect sequences.

Depth-first branch and bound

The priority-queue traversal has a structural problem. LowestFee's bound grows with depth, so a min-heap always pops the shallowest node. It expands every 1-input prefix, then every 2-input prefix, and on a large pool the round budget runs out before it reaches any funded leaf.

Depth-first search with in-place backtracking visits the child with the better bound first. It reaches a funded leaf in as many expansions as the solution has inputs, and it keeps only the current path in memory instead of a frontier of cloned selectors. The running sums make its in-place select/deselect O(1). Like #63's fix, it only treats candidates as interchangeable when value, weight, and input count all match.

Seed with a greedy incumbent

Before expanding the first node, the search scores the greedy selection (select_until_target_met), yields it, and adopts its score as the incumbent. Only the incumbent changes, not the bound, so the optimum stays reachable. It costs one round, so the two round counts in tests/bnb.rs each go up by one.

This is a guarantee rather than a speedup. run_bnb no longer returns RoundLimit when a valid greedy selection exists. That matters when the solution needs many inputs (on the 1000-coin synthetic pool below, unseeded depth-first still had nothing after 1,000 rounds because the answer is about 220 inputs deep), or when a caller stops on elapsed time rather than a fixed round count. At the 100k-round cap below it changes no result.

Benchmarks

Metric is LowestFee (2 sat/vB target, 10 sat/vB long-term, P2TR drain). Each commit is measured separately (numbers are from before the rebase onto the always-segwit #63 and the f64 fee math of #88; commit hashes are updated): cca7f66 is the base (the changeless removal doesn't touch the search), 31ef70b adds running sums, b44a26b adds depth-first search, and 27ed48c adds the seed.

cargo bench run_bnb_lowest_fee (the repo's synthetic pool, 100k-round cap, criterion median):

n	base	+ running sums	+ depth-first	+ seed
20	219 µs	100 µs	58 µs	60 µs
50	8.75 ms	2.84 ms	1.44 ms	1.42 ms
100	34.4 ms	7.27 ms	5.23 ms	5.92 ms
200	601 ms	107 ms	36.4 ms	36.2 ms

clone/* stays at 20–31 ns through the running-sums and depth-first commits (one noisy seed-commit sample reached 40 ns), so the extra sum fields don't cost anything measurable.

Search outcome at a 100k-round cap. Each cell shows rounds / score / inputs / time (median of 3) / peak RSS. - means no solution was found. The "random" pool is wallet-like: log-uniform values from 1k to 10M sat, mixing P2WPKH, P2TR, and P2PKH, with the target at 5% of the pool. Times vary by up to ~50% between runs of the same binary, so read small differences as noise.

pool, n	best-first (+ running sums)	depth-first	depth-first + seed
synthetic 100	9,635 / 3902 / 28 / 14.7 ms / 5.4 MB — exhausted	15,275 / 3902 / 28 / 6.7 ms / 3.0 MB — exhausted	15,275 / 3902 / 28 / 7.5 ms / 3.0 MB — exhausted
synthetic 200	100k / – / 106 ms / 32 MB	100k / 7018 / 51 / 45 ms / 3.0 MB	100k / 7018 / 51 / 41 ms / 3.0 MB
synthetic 500	100k / – / 108 ms / 39 MB	100k / 15726 / 115 / 66 ms / 3.1 MB	100k / 15726 / 115 / 74 ms / 3.1 MB
synthetic 1000	100k / – / 211 ms / 51 MB	100k / 29934 / 220 / 129 ms / 3.1 MB	100k / 29934 / 220 / 121 ms / 3.1 MB
random 100	2,174 / 628 / 4 / 3.2 ms — exhausted	4,150 / 628 / 4 / 2.5 ms — exhausted	4,131 / 628 / 4 / 2.7 ms — exhausted
random 200	8,641 / 526 / 3 / 10.8 ms / 5.5 MB — exhausted	26,847 / 526 / 3 / 10.6 ms / 3.2 MB — exhausted	26,841 / 526 / 3 / 16.4 ms / 3.2 MB — exhausted
random 500	100k / – / 101 ms / 39 MB	100k / 608 / 4 / 52 ms / 3.3 MB	100k / 608 / 4 / 40 ms / 3.3 MB
random 1000	100k / – / 118 ms / 51 MB	100k / 1035 / 8 / 90 ms / 3.3 MB	100k / 1035 / 8 / 84 ms / 3.3 MB

Same optimum wherever both finish. On every case where both searches ran to completion (all n ≤ 100 on the synthetic pool, n ≤ 200 on the random pool), they return the same score and input count.
Answers where best-first has none. Once the cap is hit, best-first returns no solution, and depth-first returns one with flat ~3 MB memory. Raising the cap to 1M rounds doesn't change that for best-first: it still finds nothing on synthetic 200/500/1000 or random 1000, and its peak RSS reaches 300–490 MB.
Depth-first's capped answers are close to optimal. On random 500 both searches finish with no cap (177k and 182k rounds) and agree on 608, the same score depth-first had already returned at 100k rounds. With a 10M-round cap, depth-first improves synthetic 1000 from 29934 to 29782 and random 1000 from 1035 to 938.
Where it's slower. On small pools that finish, depth-first can take more rounds to prove optimality: random 200 takes 8.6k → 26.8k rounds. The same shows up in tests/bnb.rs: 164 → 95 rounds for the feasibility search, but 3194 → 62453 for the exhaustive exact-match search.

How these were measured

Each commit was checked out in its own worktree. Criterion ran cargo bench --bench coin_selector. The outcome table comes from a small untracked example that runs bnb_solutions(LowestFee).take(cap) and reads VmHWM from /proc/self/status. Its baseline RSS is about 3 MB. The 1M- and 10M-round figures were measured before the #63 ban fix was rebased in; spot checks after the rebase gave identical rounds, scores and input counts.

🤖 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

…::is_segwit 6a9bed9 fix: exclude only identical candidates together in branch and bound (志宇) 68a5642 fix!: price every transaction as segwit and drop Candidate::is_segwit (志宇) Pull request description: #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. `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). 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. `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](https://claude.com/claude-code) ACKs for top commit: LLFourn: ACK 6a9bed9 Tree-SHA512: 485dc7b6a181a79c612256b244df27277b39278576f73be57176c3f22ab5f3350684a479f4a131bd7552325105043e4bd5fa9261bf024cd11644f62f8a56a5ff

f2eef93 fix(dev-deps): cap proptest below 1.12 (志宇) 7922106 ci: check no_std and WASM builds, harden workflow (志宇) Pull request description: ### Description Stage 1 of fixing the required status checks on `master`, plus a fix for the `test` and `clippy_check` jobs, which fail on `master` and every open PR (e.g. #84). The `Default Branch` ruleset requires 14 checks copied from an older bdk_wallet CI (`miniscript/no-std`, `bdk_chain/hashbrown`, and so on). Nothing in this repo reports them, so every PR sits on "Expected — waiting for status" indefinitely. This PR ports the bdk_wallet jobs that make sense for this crate: - **`Check no_std`**: `cargo check --no-default-features`. The crate is `#![no_std]` with an optional `std` feature, but CI never built it without default features. - **`Check WASM`**: `cargo check --target wasm32-unknown-unknown --no-default-features`. The crate is pure Rust, so bdk_wallet's clang setup isn't needed. - **Hardening**: `permissions: {}` at the workflow level and `persist-credentials: false` on every checkout. Left out: the ARM runner matrix (little value for integer arithmetic), code coverage (needs a `CODECOV_TOKEN` secret) and zizmor. #### Fix: cap `proptest` below 1.12 proptest 1.12 depends on rand 0.10, but our `rand` dev-dependency is 0.9. `Cargo.lock` isn't committed, so CI resolves the newest proptest and the tests fail to build (`TestRng: rand::Rng` is not satisfied). The dev-dependency is now `proptest = ">=1.7, <1.12"`. Moving to rand 0.10 and proptest 1.12 together is left for a follow-up, since rand 0.10 deprecates `RngCore` and the tests would need changes. Once this is merged, open PRs need a rebase onto `master` to go green. ### Follow-up (needs a repo admin) Replace the ruleset's required checks with the jobs this repo actually runs: `Rustfmt`, `clippy_check`, `build-msrv`, `test`, `doc-build`, `Check no_std`, `Check WASM`. ### Checklists - [x] `cargo check --no-default-features` and the WASM check pass locally - [x] `cargo fmt`, `clippy`, `doc` and `test` pass locally on a fresh resolve (no `Cargo.lock`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: notmandatory: ACK f2eef93 noahjoeris: ACK f2eef93 oleonardolima: ACK f2eef93 Tree-SHA512: 6adb92881f91a3d0b7e75a9dde46f9a0e33bae5a011f5452d6788aff0a8183df54c003b4fc6e5702064638e19cbedfe81a3ba109a18e22974798fd38ca6f3feb

`LowestFee` already decides for itself whether a selection should carry change, adding one only when it lowers the long-term fee, clears the dust threshold, and fits `Target::max_weight`. A separate changeless objective duplicates that decision and then constrains it. Callers that required a changeless transaction should use `LowestFee` and inspect the returned `Drain`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`input_weight` scanned the selected set twice and `selected_value` once, and metrics call them several times per branch-and-bound node through `excess`, `rate_excess`, `implied_fee`, and friends. Track the selected value, weight, and input count as running sums updated in `select`/`deselect`, so every aggregate is O(1). `select_all_effective` now goes through `select` so the sums cannot drift. A proptest in `tests/weight.rs` checks the sums against a recompute after random selects and deselects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Replace the best-first `BinaryHeap` frontier with a depth-first search that visits the child with the better bound first and backtracks in place. Only the current path is held, instead of a cloned selector per frontier node. An exclusion node has the same selection as its parent, so it is not scored again. Both `LowestFee`'s bound and the exact-match test metric grow with depth, so a min-heap always pops the shallowest node: it expands every 1-input prefix, then every 2-input prefix, and on a large pool the round budget runs out before it reaches a funded leaf. Depth-first reaches a funded leaf in as many expansions as the solution has inputs. Backtracking needs two crate-internal `CoinSelector` helpers: `unban`, and `order_pos_to_candidate_index` to recover a frame's candidate from its position in the sort order. The round-count assertions in `tests/bnb.rs` move: 164 -> 94 for the feasibility search, and 3194 -> 62452 for the exhaustive exact-match search, where depth-first expands more nodes before proving optimality. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Yield the greedy selection before expanding the first node, and adopt its score as the incumbent. Otherwise a caller whose round budget runs out before the first complete selection gets `NoBnbSolution::RoundLimit` and falls through to whatever fallback it has, which on a large pool is far worse than the selection a single greedy pass would have handed it. Only the incumbent changes, not the bound, so the optimum stays reachable and the improving-solutions contract is unaffected. The `bnb_solutions` doc now says the first item is the greedy selection, and is corrected to say that each later solution has a lower score than the last (the metric is minimised), not a higher one. The two round-count assertions in `tests/bnb.rs` each move by one: the seed is a round. A new test checks that `run_bnb` with a budget of one round returns the greedy selection instead of `RoundLimit`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

evanlinjin mentioned this pull request Sep 22, 2026

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

Closed

evanlinjin self-assigned this Sep 22, 2026

evanlinjin marked this pull request as draft September 22, 2026 08:57

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch 2 times, most recently from 5dc9fcd to 2434ec6 Compare September 22, 2026 09:52

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

This was referenced Sep 23, 2026

perf!: delta-aware BnbMetric via SelectionView/compute_view #53

Closed

perf: port Bitcoin Core's incumbent-free branch-and-bound prunes #69

Closed

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch from c495ffc to 6cf6fc4 Compare September 23, 2026 07:13

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch 4 times, most recently from 713c356 to 9c8706f Compare October 1, 2026 01:15

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch from 9c8706f to 04ada8a Compare October 2, 2026 08:56

chukwudiikeh mentioned this pull request Oct 9, 2026

LowestFee score loses precision for fees above 2^24 sats due to f32 conversion #82

Closed

evanlinjin marked this pull request as ready for review October 19, 2026 16:56

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch 3 times, most recently from 5da59d3 to 14a2969 Compare October 20, 2026 01:15

evanlinjin mentioned this pull request Oct 20, 2026

ci: check no_std and WASM builds, harden workflow #86

Merged

2 tasks done

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch from 14a2969 to 27ed48c Compare October 22, 2026 08:52

evanlinjin and others added 2 commits October 23, 2026 05:49

evanlinjin force-pushed the feat/dfs-remove-changeless-v2 branch from 27ed48c to 573b4fe Compare October 23, 2026 06:19

Navigation Menu

Repository navigation

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

feat!: depth-first branch and bound, and remove the Changeless metric#84
evanlinjin wants to merge 4 commits into
bitcoindevkit:masterfrom
evanlinjin:feat/dfs-remove-changeless-v2

evanlinjin commented Sep 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

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

Uh oh!

Remove the Changeless metric

Keep running sums of the selection in CoinSelector

Depth-first branch and bound

Seed with a greedy incumbent

Benchmarks

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

evanlinjin commented Sep 22, 2026 •

edited

Loading

Remove the `Changeless` metric

Keep running sums of the selection in `CoinSelector`