Repository navigation
feat!: add dedicated LowestFeeChangeless metric - #3
Closed
evanlinjin wants to merge 17 commits into
Closed
evanlinjin wants to merge 17 commits into
evanlinjin wants to merge 17 commits into
Conversation
Squash of the two-commit perf series (cd1017a "delta-aware BnbMetric via SelectionView/SelectionCache" + 536a03a "replace duplicated CoinSelector read methods with compute_view()"), rebased onto the new BnbMetric API (target passed as a parameter; metric decides its own change output). Squashed because the first commit's intermediate state (src/selection_cache.rs, SelectionView-by-value) is fully superseded by the second (src/selection_view.rs, Cow-backed cache, &SelectionView, compute_view()); replaying both would mean resolving the same BnbMetric merge against master twice. The flamegraph on fix/better-memory showed ~65% of run_bnb_lowest_fee time was in cs.selected_value() (47.5%) and cs.input_weight() (17.3%) -- both O(|selected|) walks recomputed many times per branch via excess/is_target_met/drain. This makes the metric evaluator delta-aware. BnB maintains a SelectionCache (running aggregates over the selection: value_sum, weight_sum, input_count, segwit_count, candidate_count) per Branch; inclusion expansions call cache.add(c), which is O(1). The metric trait now takes a `&SelectionView<'_>` -- a handle over (&CoinSelector, Cow<SelectionCache>) -- with O(1) versions of every read method (selected_value, input_weight, excess, is_target_met, drain, ...). CoinSelector's ~15 duplicated O(|selected|) read methods collapse into a single `cs.compute_view()` entry point (fresh cache built once on demand); the BnB hot path borrows the per-branch cache instead (zero clone). Adapted to the master-side API changes: - score/bound/drain take `target: Target` as a parameter (metrics no longer store target); BnbIter threads its `target` through to the metric. - LowestFee owns the change decision (dust_relay_feerate + drain_weights, no change_policy) and implements `drain`; its delta-aware not-target-met bound loop is preserved. - Changeless<M> wraps an inner metric, using &SelectionView + target. All lib, integration and doc tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The original benches capped at 4k candidates for `clone` and 200 for
`run_bnb_lowest_fee`. Real callers span a much wider range -- a typical
wallet has ~1k UTXOs, a large exchange ~10M.
For the O(n)-ish operations (`new`, `clone`, `compute_view`) extend the
parameter list to 64 / 1k / 16k / 256k / 1M / 10M. These all scale
roughly linearly:
clone/64 51 ns
clone/1024 52 ns
clone/16384 260 ns
clone/262144 2.0 us
clone/1048576 6.3 us
clone/10000000 98 us
At 10M UTXOs the `Candidate` slice itself is ~320MB and the selector's
`candidate_order` Vec is ~80MB -- commented as a heads-up for memory-
constrained hosts.
Add new groups:
- `new`: cost of `CoinSelector::new(candidates)` -- allocations grow with
pool size.
- `compute_view`: cost of building a SelectionView. Scales with
|selected| rather than |pool|; benched against a fixed sparse
selection of ~100 candidates regardless of pool size, matching how
wallets actually use selection.
The BnB bench splits into two explicitly-named groups, because at
n >= 200 best-first exploration does not complete any target-meeting
selection within the round cap (run_bnb returns NoBnbSolution after
exactly 100k rounds):
- `run_bnb_lowest_fee` (n = 20/50/100): end-to-end solution finding.
- `run_bnb_lowest_fee_exhaust_cap` (n = 200/500/1000): exactly
MAX_ROUNDS rounds of frontier expansion (bound() + branch cloning) --
the hot path the delta-aware cache optimizes. Per-round cost grows
roughly linearly:
run_bnb_lowest_fee_exhaust_cap/200 193 ms
run_bnb_lowest_fee_exhaust_cap/500 211 ms
run_bnb_lowest_fee_exhaust_cap/1000 377 ms
Each group asserts at startup that run_bnb's solution-found outcome
matches what the group claims to measure, so a size silently flipping
between the two paths (metric change, bound tightening) fails loudly
instead of corrupting cross-version comparisons.
10M-scale BnB is intentionally not benchmarked: it's impractical at any
finite round budget, and real callers pre-filter / pre-group at that
scale.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The flamegraph after the delta-aware refactor showed ~32% of run_bnb time spent walking candidate_order and checking Bitset::contains(selected) || Bitset::contains(banned) per element, inlined into insert_new_branches's `cs.unselected().next()`. As BnB descends and more candidates get selected/banned, each .next() call scans further before finding the next viable candidate. But BnB never re-considers a position: each branch's exploration only moves forward in candidate_order. Inclusion advances by 1; exclusion advances past every consecutive same-(value, weight) candidate. So we can store a per-Branch cursor and avoid the scan entirely. Add `Branch::cursor: usize` (the position the branch will expand on next). The init branch starts at 0; insert_new_branches advances past any pre-selected/pre-banned positions on demand, then expands at the located cursor and hands children their new cursors directly. One subtlety: the exclusion branch's same-(value, weight) dedup run now walks raw candidate_order positions, where the old `unselected()` scan skipped already-decided candidates implicitly. The run must do the same explicitly -- skip pre-selected/pre-banned positions (advancing the cursor past them) rather than banning them or letting them end the run. Otherwise a caller-pre-selected candidate that duplicates an excluded one ends up simultaneously selected and banned in the selector that run_bnb hands back, and a pre-decided candidate inside a duplicate run fragments the equivalence class into redundant branches. Covered by `bnb_exclusion_dedup_skips_decided_candidates`. Bench (run_bnb_lowest_fee, n = pool size): n=20 166 us -> 147 us (11%) n=50 5.3 ms -> 4.5 ms (16%) n=100 12.6 ms -> 11.5 ms (9%) n=500 200 ms -> 171 ms (14%) n=1000 365 ms -> 248 ms (32%) (n >= 200 rows are the exhaust-cap group: fixed 100k rounds of frontier expansion, so they measure pure per-round cost.) Largest win at large n where the unselected scan was burning the most time. Flamegraph confirms the unselected-scan hot spot is gone; new top is LowestFee::bound itself (the metric's float math + lookahead). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y_count 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). Tracking segwit and legacy input counts separately also allows a single Candidate to mix legacy and segwit inputs.
…legacy Replaces the boolean is_segwit parameter in Candidate::new with explicit new_segwit and new_legacy constructors. Clarifies in doc comments that satisfaction_weight is the additional weight required beyond TXIN_BASE_WEIGHT (which already accounts for a 1-byte scriptSigLen).
…call
A selector was built for one target and evaluated against it throughout,
but every method took the target as a parameter, so nothing stopped
`cs.excess(target_a, drain)` being followed by `cs.is_funded(target_b)`.
The correctness arguments in the metrics are all stated at a fixed target
-- `LowestFee::bound`'s proof that a changeless superset always costs
more, `Changeless::change_unavoidable`'s assumption that the drain
decision is monotone in the excess -- and were held together by
convention rather than by types.
`CoinSelector::new` now takes the target and owns it. Twenty signatures
*lose* a parameter rather than gaining one: fifteen public methods
(`excess`, `implied_fee`, `is_funded`, `drain`, `select_until_target_met`,
the four `*_excess`, ...), plus `bnb_solutions` and `run_bnb`, plus all
three `BnbMetric` methods.
The crate had already reached this conclusion one layer down: `BnbIter`
stored the target as a field, took it once in `BnbIter::new`, and then
re-passed it into `metric.score` and `metric.bound` at every node. That
field and the re-threading are both gone.
This is a breaking change, and it reaches `BnbMetric`, so metrics
implemented outside this crate need their signatures updated:
fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>;
fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>;
fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain;
`CoinSelector::target()` exposes the target for metrics that need to read
it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ancestor-aware selection needs target, candidates, and bump data as one unit of work. SelectionProblem owns that; CoinSelector holds a reference for its lifetime. Candidate stays a plain Copy description of inputs.
Selecting an unconfirmed coin means paying to bump its ancestors. The feerate obligation includes the shortfall of the union of ancestors the selected candidates drag in (each charged once; weight and fee netted; saturates at 0). Score is still the child fee — the bump is already inside it. With ancestors, LowestFee falls back to a loose but admissible fee floor; tightening is a follow-up. BnB only batch-bans look-alikes with the same drags_in; Changeless disables its prune when ancestors are present.
Two complementary improvements, neither changing what a selection owes: 1. Bound: credit the least any descendant could still owe for ancestors. Shed only surplus that is actually reachable; when nothing overpays the floor is the full bump. Cuts search rounds sharply on typical CPFP. 2. Representation: fold ancestors only one candidate can reach into a per-candidate (weight, fee) pair. Shared ancestors still de-dupe per selection. No bitset work when nothing is shared; local_bump is derived on demand rather than stored. Also add ancestor BnB benches (private and shared paths).
It is the least bump any descendant could still owe, not this selection's current bump. Drop the S∪D/shed notation.
For funded nodes, subtract the ancestor surplus still reachable by a descendant. For unfunded nodes, derive a minimum added child weight from independent fractional relaxations of the target-rate, absolute-fee, and RBF constraints, then evaluate the fee floor at that weight. Candidate ancestry is deliberately represented only by the global bump lower bound: package surplus can absorb a later private deficit, so a per-candidate ancestor cost is not admissible. Keep infeasibility prunes off because ancestor funding is non-monotone. Add regressions for package subsidy, absolute/RBF double counting, and large-float cancellation, plus the existing exhaustive proptests.
evanlinjin
force-pushed
the
feature/ancestor-aware-selection-no-clustor
branch
from
August 14, 2026 04:13
2f59d13 to
4934ca4
Compare
evanlinjin
force-pushed
the
feature/lowest-fee-changeless
branch
from
August 14, 2026 04:14
e45bb58 to
0c7db31
Compare
evanlinjin
force-pushed
the
feature/ancestor-aware-selection-no-clustor
branch
from
August 14, 2026 05:09
4934ca4 to
0c97d3b
Compare
Owner
Author
|
Folded this work into bitcoindevkit#64 so the ancestor-aware PR ships the dedicated changeless metric and its tighter bound as one coherent change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacking
This is stacked on bitcoindevkit/coin-select#64. The PR contains only the dedicated changeless metric commit.
Summary
LowestFeeChangeless, a dedicated lowest-fee metric that only accepts selections for whichLowestFeechooses no change output.LowestFeechange eligibility and funding-bound logic.LowestFeeordering because the tighter best-first ordering can starve useful subtrees under a finite round budget.Changeless<M>wrapper and addFrom<LowestFee> for LowestFeeChangeless.max_weight.Benchmark Findings
Full report: coinselect-benchmark/FINDINGS.md
The report covers Bitcoin Core v31.1 and coin-select
e45bb58over 33 fixtures, three tracks, and a 100,000-round budget.nested_ancestry_20improves from no result at 100,000 rounds to the oracle-optimal result in 200 rounds.subsidizing_ancestry_20improves from no result at 100,000 rounds to the oracle-optimal result in 886 rounds.shared_ancestry_200andsubsidizing_ancestry_200.The large-pool bound fallback also fixes a search-order regression on
subsidizing_ancestry_100: it matches the pinned runner with score19006at 100,000 rounds and score18925at 20 million rounds.Verification
cargo test --all-targetscargo test --all-features --releasecargo clippy --all-targets --all-features -- -D warningscargo check --no-default-featurescargo fmt --all -- --check