Conversation
📝 WalkthroughWalkthroughCoin selection now models fees with or without a change output, reports fee-inclusive insufficient-funds requirements, handles dust and surplus remainders explicitly, and applies shared transaction-size constants in transaction building. ChangesCoin selection and transaction sizing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant TransactionBuilder
participant CoinSelector
participant Accumulator
Caller->>TransactionBuilder: build_unsigned()
TransactionBuilder->>CoinSelector: select_coins_with_size(change_output_size)
CoinSelector->>Accumulator: estimate no-change and with-change requirements
Accumulator-->>CoinSelector: selected inputs, fee, and remainder
CoinSelector-->>TransactionBuilder: selection result or error
TransactionBuilder-->>Caller: unsigned transaction or NoChangeAddress
🚥 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}
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 330-334: Update the with-change eligibility condition in the
coin-selection logic to require the remaining change to be strictly greater than
self.dust_threshold, matching the transaction builder’s boundary. Preserve the
existing required_with_change calculation and other condition unchanged.
- Around line 317-323: Update the BranchAndBound and OptimalConsolidation
fallback calls to pass the original base_size into the selection helper, rather
than base_size + 34. Preserve the helper’s existing contract where base_size
includes one change output, so its with-change and no-change fee calculations
remain consistent.
🪄 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
Run ID: a7267a0f-d930-447e-96af-a20b7ea42f60
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rs
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
The approach here is right, and I verified it against Dash Core: selecting against the no-change fee is exactly what CWallet::CreateTransactionInternal does. Two gaps though, both reproduced by running the branch, and the first one means the originally reported send still fails.
1. The fix doesn't reach the default strategy, so #911 still reproduces
TransactionBuilder::new() defaults to SelectionStrategy::BranchAndBound. Both BranchAndBound and OptimalConsolidation fall back into the rewritten accumulator, but they pass base_size + 34 into it:
coin_selection.rs:424(branch_and_bound_with_size)coin_selection.rs:543(optimal_consolidation_with_size)
TransactionBuilder::calculate_base_size() has already added those 34 bytes whenever a change address is set (transaction_builder.rs:180-192). So inside the accumulator size_no_change = base_size + 34 - 34 = base_size, which still budgets a change output, and required_no_change is still the change-inclusive requirement this PR set out to stop using.
With the exact figures from the issue (one 10,000,000-duff UTXO, target 9,999,780, FeeRate::normal(), builder base_size 78, input size 148):
LargestFirst => Ok(change 0, estimated_fee 220) fixed
BranchAndBound => Err(InsufficientFunds{available:10000000, required:10000006}) still rejected
OptimalConsolidation => Err(InsufficientFunds{available:10000000, required:10000006}) still rejected
The error is at least self-consistent now (available < required), which is half of what the issue asked for. But the send is still rejected on the path a caller hits by default. The new regression test only exercises SmallestFirst, which is why this isn't caught.
2. CHANGE_OUTPUT_SIZE is subtracted unconditionally, but base_size doesn't always contain a change output
calculate_base_size() adds the 34 bytes only when change_addr.is_some(). With no change address, saturating_sub(CHANGE_OUTPUT_SIZE) shaves 34 bytes off a size that never had a change output in it, and the selection underpays:
base_size 44 (one output, no change), 1 input, target 10_000_000 - 158
=> Ok(estimated_size: 158, estimated_fee: 158)
real tx size is 192 bytes and needs a fee of 192
FeeRate::normal() is 1000 duffs/kB, which is exactly Dash Core's DEFAULT_MIN_RELAY_TX_FEE (policy.h:51), so there is no headroom. 158 duffs on a 192-byte transaction is below min relay fee and the node rejects it. Reachable whenever next_change_address fails and set_funding swallows it with .ok() (non-Standard account type, or an exhausted watch-only pool), or when a caller never calls set_change_address.
Suggested fix for both
Stop inferring the change-output cost, pass it in explicitly. Add a change_output_size: usize parameter to accumulate_coins_with_size, sourced once at the top from TransactionBuilder as if self.change_addr.is_some() { CHANGE_OUTPUT_SIZE } else { 0 }, and drop the + 34 at coin_selection.rs:424 and :543. That makes the with-change and no-change sizes derive from one authoritative number instead of three places guessing independently. Then extend the new test to loop over every strategy.
Worth adding Dash Core's backstop too (spend.cpp:1010-1012): after assembly, recompute the size-based fee for the transaction actually built and reject if the fee paid falls below it. Core keeps that as an internal-bug guard, and it is precisely what would have caught gap 2.
Cross-check against Dash Core
For the record, the parts that do line up:
- Core's selection target is
recipients_sum + not_input_fees, where the comment is explicit that it covers "things that aren't inputs, excluding the change output" (spend.cpp:876-896). That is this PR'srequired_no_change. The old change-inclusive bar was the real defect. - Core always builds a change output, subtracts the fee, drops it if dust or too small, then recomputes
nBytesandfee_neededfor the shrunken transaction (spend.cpp:993-1001). Same move assize_no_change/fee_no_change. - Core's keep-change bar is
cost_of_change = change_output_size * effective_feerate + change_spend_size * discard_feerate(spend.cpp:871-872), roughly 1514 duffs with default settings. So Core folds change into the fee more aggressively than the flat 546 used here. This PR is on the conservative side of Core, which is fine. - The 546 dust default is correct:
GetDustThresholdis(34 + 148) * 3000/1000 = 546withDUST_RELAY_TX_FEE = 3000(policy.cpp:26-42). The 34-byte P2PKH output and 148-byte P2PKH input also match Core's serialized sizes.
Minor
CHANGE_OUTPUT_SIZEis introduced but the bare34literal still appears atcoin_selection.rs:118,:424,:499,:543and three times incalculate_base_size. Using the new constant in those places would have made gap 1 visible.transaction_builder.rs:360hardcodeschange_amount > 546while the selector uses the configurabledust_thresholdwith>=. A change of exactly 546 is planned as change by the selector and then silently folded into the fee by the builder. Pre-existing and harmless (build_unsignedreports the true fee), so a separate issue rather than something for this PR.- The commit and PR title "fix issue 911" don't say what changed. Something like
fix(key-wallet): select against the no-change fee so zero/dust-change sends aren't rejectedwould read better ingit log.
Everything else is clean: one commit, no leftovers, fmt and clippy --all-features --all-targets clean, 549 lib tests green, all CI green.
ZocoLini
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #914 +/- ##
==========================================
+ Coverage 74.54% 74.60% +0.05%
==========================================
Files 327 328 +1
Lines 75032 75814 +782
==========================================
+ Hits 55936 56564 +628
- Misses 19096 19250 +154
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rs (1)
136-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider bundling the sizing parameters to avoid repeated
too_many_argumentsallows.
base_size,input_size, andchange_output_sizeare now threaded as three loose parameters throughselect_coins_with_size,accumulate_coins_with_size,branch_and_bound_with_size, andoptimal_consolidation_with_size, each requiring#[allow(clippy::too_many_arguments)]. A smallTxSizeParams { base_size, input_size, change_output_size }struct passed by value would remove the repeated allows and make the "these three always travel together" contract explicit at the type level.Not blocking — purely a maintainability polish, given the current threading is correct and well-tested.
🤖 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 136 - 270, Bundle base_size, input_size, and change_output_size into a TxSizeParams value object and pass it through select_coins_with_size, accumulate_coins_with_size, branch_and_bound_with_size, and optimal_consolidation_with_size. Update all call sites and internal accesses to use the struct, then remove the now-unnecessary too_many_arguments allowances while preserving existing sizing behavior.
🤖 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.
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/coin_selection.rs`:
- Around line 136-270: Bundle base_size, input_size, and change_output_size into
a TxSizeParams value object and pass it through select_coins_with_size,
accumulate_coins_with_size, branch_and_bound_with_size, and
optimal_consolidation_with_size. Update all call sites and internal accesses to
use the struct, then remove the now-unnecessary too_many_arguments allowances
while preserving existing sizing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cded14fa-053d-4d13-9865-544ca257a3e4
📒 Files selected for processing (2)
key-wallet/src/wallet/managed_wallet_info/coin_selection.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
2 participants
Tx with no change where reporting Insufficient funds, added test to replicate the issue and adjusted the CoinSelector to properly manage this scenario
Fixes #911
Summary by CodeRabbit