Skip to content

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

Open
evanlinjin wants to merge 1 commit into
bitcoindevkit:masterfrom
evanlinjin:refactor/selector-owns-target
Open

Give CoinSelector its target instead of threading it through every call#59
evanlinjin wants to merge 1 commit into
bitcoindevkit:masterfrom
evanlinjin:refactor/selector-owns-target

Conversation

@evanlinjin

Copy link
Copy Markdown
Member

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:

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, build a second selector.

🤖 Generated with Claude Code

…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>
@evanlinjin
evanlinjin force-pushed the refactor/selector-owns-target branch from 2b749af to c0c8ae2 Compare August 4, 2026 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant