Skip to content

refactor!: Replace Selector with a single into_selection pass#82

Open
evanlinjin wants to merge 3 commits into
bitcoindevkit:masterfrom
evanlinjin:fix/cannot-meet-target
Open

refactor!: Replace Selector with a single into_selection pass#82
evanlinjin wants to merge 3 commits into
bitcoindevkit:masterfrom
evanlinjin:fix/cannot-meet-target

Conversation

@evanlinjin

Copy link
Copy Markdown
Member

Description

Reworks the coin-selection API around a single InputCandidates::into_selection pass and cleans up its error surface.

The public Selector struct is removed as nothing used it. Its methods were only ever called in sequence by into_selection, so it was public surface with no consumer. That logic now lives inside into_selection, and algorithms take a bdk_coin_select::CoinSelector plus a pure-data SelectionContext instead of a &mut Selector.

This also:

  • unifies the two redundant long-term feerates (change policy + bnb metric) into one SelectionParams::longterm_feerate;
  • splits the catch-all SelectorError so each error type only carries variants it can actually produce;
  • fixes a reachability bug that produced a false CannotMeetTarget under RBF.

Notes to the reviewers

  • Behavior change (please scrutinize): the change policy is now always waste-aware (min_value_and_waste). Previously a None long-term feerate selected a plain dust-only policy. Consequence: raw-script change must now supply a real satisfaction_weight (descriptor change is unaffected — it's derived). Rationale: min_value_and_waste with longterm == target does not collapse to dust-only; it still charges the change output's lifetime cost, which is the correct thing to weigh.
  • Bug fix detail: the reachability pre-check called select_all_effective(raw target_feerate) but scored with excess(target.fee.rate) (the RBF-resolved, higher rate). This could result in an under-selection. Fixed by selecting at target.fee.rate.
  • Error split: to_cs_change_policy previously returned a SelectorError that included a LockTypeMismatch it could never produce. It now returns ChangePolicyError (only Miniscript / InsufficientAssets), and LockTypeMismatch is a top-level IntoSelectionError variant.

Changelog notice

Added:
- Add `SelectionContext`, passed to selection algorithms alongside the `CoinSelector`.
- Add `ChangePolicyError` (`Miniscript`, `InsufficientAssets`) as the return type of `SelectionParams::to_cs_change_policy`.
- Add `IntoSelectionError` variants `ChangePolicy`, `LockTypeMismatch`, `CannotMeetTarget { missing }`, and `AlgorithmFellShort`.

Changed:
- Rename `SelectorParams` to `SelectionParams`.
- `InputCandidates::into_selection` no longer takes a `&mut Selector` closure; algorithms are now `FnOnce(&mut bdk_coin_select::CoinSelector, SelectionContext) -> Result<(), E>`.
- Replace `SelectionParams::change_longterm_feerate` with `SelectionParams::longterm_feerate`; `None` now means "use the target feerate", and the change policy is always waste-aware. The single value feeds both the change policy and the lowest-fee-bnb metric.
- `selection_algorithm_lowest_fee_bnb` no longer takes a `longterm_feerate` argument (it reads it from `SelectionContext`).
- Fix a false `CannotMeetTarget` under RBF: the reachability check now selects effective inputs at the resolved target feerate.

Removed:
- Remove the `Selector` struct (its lifecycle is folded into `InputCandidates::into_selection`).
- Remove `SelectorError` (replaced by `ChangePolicyError` and dedicated `IntoSelectionError` variants).

Before submitting

evanlinjin and others added 3 commits June 27, 2026 00:52
Fold the whole selection lifecycle into `InputCandidates::into_selection`
and remove the public `Selector` struct. Algorithms now operate directly
on a `bdk_coin_select::CoinSelector` plus a pure-data `SelectionContext`,
instead of a wrapper that conflated lifecycle plumbing with the algorithm
handle.

- `SelectorParams`/`SelectorError` -> `SelectionParams`/`SelectionError`.
- Unify the long-term feerate into one `SelectionParams::longterm_feerate`
  (`None` = target feerate), feeding both the change policy and the bnb
  metric; the change policy is now always waste-aware.
- `IntoSelectionError` carries distinct `CannotMeetTarget { missing }`
  (impossible) vs `AlgorithmFellShort` (algorithm under-selected) variants.
- Fix the reachability pre-check to select effective inputs at the resolved
  target feerate (`target.fee.rate`), not the raw one, avoiding a false
  `CannotMeetTarget` under RBF.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…match`

`SelectionError` lumped change-policy construction failures together with
candidate timelock validation, so `to_cs_change_policy` advertised a
`LockTypeMismatch` variant it could never return.

- `SelectionError` -> `ChangePolicyError` (just `Miniscript` /
  `InsufficientAssets`), now the honest return type of `to_cs_change_policy`.
- `IntoSelectionError::Setup(_)` -> `ChangePolicy(ChangePolicyError)`, and
  `LockTypeMismatch` is promoted to a top-level `IntoSelectionError` variant.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The module no longer holds a `Selector`; it now holds `SelectionParams`
and its supporting config/context/error types.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evanlinjin evanlinjin force-pushed the fix/cannot-meet-target branch from ad96c68 to e162434 Compare June 27, 2026 01:33
@evanlinjin evanlinjin self-assigned this Jun 27, 2026
@evanlinjin evanlinjin added the api Changes the public API label Jun 27, 2026
@evanlinjin evanlinjin marked this pull request as ready for review June 27, 2026 01:34
evanlinjin added a commit to evanlinjin/bdk-tx that referenced this pull request Jun 27, 2026
API:
- Move longterm_feerate from SelectionStrategy::LowestFee to SelectParams: it
  feeds the waste-aware change policy for every strategy, and post-bitcoindevkit#82 the bnb
  algorithm reads it from SelectionParams rather than as an argument. LowestFee
  now carries only max_rounds.
- Rename SelectParams::fee_rate -> feerate.

Cleanup:
- Inline single-use, Wallet-bound helpers (change_keychain, try_plan,
  wallet_assets, list_indexed_txouts) and drop the bdk_coin_select_insufficient
  wrapper. Pure, individually-testable helpers (status_from_position,
  extended_keys) are kept.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Changes the public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant