fix(key-wallet): rewrite branch-and-bound coin selection (#918) - #919
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughChangesThe coin-selection API now assumes Branch-and-Bound coin selection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TransactionBuilder
participant CoinSelector
participant BnbSearch
participant FeeCalculation
TransactionBuilder->>CoinSelector: request selection with fixed TX_INPUT_SIZE
CoinSelector->>FeeCalculation: calculate no-change effective target
CoinSelector->>BnbSearch: search effective-value candidates
BnbSearch-->>CoinSelector: return bounded best-surplus selection
CoinSelector->>FeeCalculation: re-check actual transaction fee
CoinSelector-->>TransactionBuilder: return zero-change SelectionResult
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/coin_selection.rs`:
- Around line 1185-1198: The tests
test_branch_and_bound_near_total_target_is_bounded and
test_branch_and_bound_mid_range_target_is_bounded should rely on a deterministic
BnbSearch node-budget or visited-node count, asserting it does not exceed
BNB_NODE_BUDGET. Replace the strict one-second correctness gate with that
deterministic assertion, retaining only a sufficiently generous timing check as
an optional smoke bound.
- Around line 507-529: Update the BnB selection flow rooted at
branch_and_bound_with_size to propagate the caller-provided input_size through
branch_and_bound and its estimated-size calculations, replacing hardcoded
TX_INPUT_SIZE usage. Also pass the caller’s change_output_size into
cost_of_change, and ensure the resulting SelectionResult fee and size
calculations match the accumulate fallback for custom per-input sizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4cbcf35-7b77-40c1-948f-e20abd71e615
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #919 +/- ##
==========================================
+ Coverage 74.67% 74.81% +0.13%
==========================================
Files 328 328
Lines 76451 76593 +142
==========================================
+ Hits 57093 57302 +209
+ Misses 19358 19291 -67
|
ZocoLini
force-pushed
the
fix/bnb-coin-selection-918
branch
2 times, most recently
from
July 29, 2026 15:04
b2b7684 to
eb5528d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rs (1)
451-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment on
branch_and_bound_with_size.The doc comment still describes the old design ("Recursively explores combinations looking for exact matches", "Prunes branches that exceed the target by too much") rather than the new no-change/effective-value search with a
cost_of_changeacceptance window implemented below. Worth updating to avoid misleading future readers.📝 Suggested doc update
- /// Branch and bound coin selection with custom sizes (finds exact match if possible) + /// Branch and bound coin selection with custom sizes (finds a no-change selection if possible) /// /// This algorithm: - /// - Sorts UTXOs by value descending (largest first) - /// - Recursively explores combinations looking for exact matches - /// - Prunes branches that exceed the target by too much + /// - Sorts UTXOs by value descending (largest first) + /// - Depth-first searches subsets using effective values, accepting a surplus within + /// `cost_of_change` of the target (an exact match is just the zero-surplus case) + /// - Prunes branches that overshoot the acceptance window or cannot reach the target /// - Falls back to simple accumulation if no exact match found🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/managed_wallet_info/coin_selection.rs` around lines 451 - 463, Update the doc comment for branch_and_bound_with_size to describe the current no-change/effective-value search and its cost_of_change acceptance window, removing claims about the obsolete recursive exact-match and oversized-target pruning behavior. Keep the documented trade-offs aligned with the implementation below.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/coin_selection.rs`:
- Around line 1147-1166: The test_branch_and_bound_mid_range_target_is_bounded
test needs a deterministic assertion that the search respects its node budget
instead of relying only on elapsed time. Add node-count instrumentation or the
existing budget-tracking mechanism, mirroring
test_feasibility_bound_settles_a_near_total_target_in_few_nodes, and assert the
count stays within the configured limit of 200; retain the current 30-second
timing check only as a generous smoke bound.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/coin_selection.rs`:
- Around line 451-463: Update the doc comment for branch_and_bound_with_size to
describe the current no-change/effective-value search and its cost_of_change
acceptance window, removing claims about the obsolete recursive exact-match and
oversized-target pruning behavior. Keep the documented trade-offs aligned with
the implementation below.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1b5e31d-9d50-4b68-862e-ab8015543dea
📒 Files selected for processing (2)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
ZocoLini
force-pushed
the
fix/bnb-coin-selection-918
branch
from
July 29, 2026 19:53
eb5528d to
7a5a07f
Compare
xdustinface
approved these changes
Jul 30, 2026
6 tasks
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
2 participants
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.
BranchAndBound only pruned branches that overshot the target, so a target near
the total balance — where almost nothing overshoots — degenerated into an
exhaustive 2^N walk: ~3s at 22 UTXOs, non-terminating past 32.
Rewrite the search in effective values (each UTXO net of the fee to spend it),
which makes the acceptance test independent of how many inputs are chosen:
target even taking every UTXO left is abandoned. Node budget as a backstop.
surplus, instead of demanding the exact satoshi — a near-exact match avoids
a change output, which is the whole point of the strategy.
made an exact match overpay one input's worth of fee.
Closes #918
Summary by CodeRabbit