fix(key-wallet): let CoinSelector spend mempool UTXOs #748
Conversation
…g UTXOs
Adds two regression tests in `tests/dashd_sync/tests_transaction_builder.rs`
that drive `ManagedWalletInfo::build_and_sign_transaction` end-to-end against
a real dashd regtest node, broadcasting via the SPV client.
Both tests use a fresh-mnemonic wallet (EMPTY/SECONDARY) so the SPV side has
no preexisting confirmed UTXOs from the test chain that would mask the bug.
- test_spend_unconfirmed_change_balance: confirms the wallet reports trusted
mempool change as spendable, then fails to actually spend it through coin
selection (the filter ignores `is_trusted`).
- test_spend_unconfirmed_incoming_balance: confirms the wallet reports
unconfirmed incoming as spendable, then fails to spend it.
Both tests are red against the current coin selector and document the user-
reported symptom ("balance shows up but cannot be spent").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dable
`CoinSelector` carried its own confirmation policy
(`min_confirmations`, `include_unconfirmed`) and filtered candidates
with `(include_unconfirmed || is_confirmed || is_instantlocked)` plus
a `current_height - utxo.height >= min_confirmations` depth check.
Two consequences:
1. `is_trusted` was missing from the OR — mempool change from a tx
the wallet itself authored was rejected, even though the balance
UI shows it as spendable. Users hit this as
`CoinSelection(NoUtxosAvailable)` immediately after sending.
2. The depth check used `>= 1`, which silently rejected a UTXO mined
into the wallet's tip block (depth 0, but 1 confirmation in the
usual sense), and gated all unconfirmed incoming through the same
path.
The `Utxo` itself already has `is_spendable(current_height)` — which
checks not-locked and coinbase maturity — and the wallet decides what
to put in the UTXO map (with `is_confirmed`, `is_instantlocked`,
`is_trusted` flags reflecting its policy). The coin selector having a
*second*, divergent policy on top is what produced the bug.
Drop the duplicated machinery: remove `min_confirmations`,
`include_unconfirmed`, and the per-call helpers. Both filter sites
now defer to `Utxo::is_spendable`, so coin selection accepts every
UTXO the wallet tracks as spendable — confirmed, IS-locked, trusted
self-send change, and incoming mempool alike.
Tightens `tests_transaction_builder.rs` accordingly: the workaround
that mined an extra "bump" tx so `last_processed_height` advanced past
the funding UTXO is no longer needed.
All 482 key-wallet unit tests and 26 dashd_sync integration tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR simplifies CoinSelector's spendability filtering by removing confirmation count and unconfirmed transaction configuration options, then adds comprehensive test coverage for mempool transaction spending scenarios. The CoinSelector now relies solely on ChangesMempool and Unconfirmed Transaction Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🤖 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 86-91: The doc comment on CoinSelector still references removed
methods (`with_min_confirmations`, `exclude_unconfirmed`/`include_unconfirmed`)
causing broken intra-doc links; update the comment on CoinSelector to remove
those link references and instead state that spendability is determined solely
by Utxo::is_spendable(current_height) (which checks lock status and coinbase
maturity), and that callers who need stricter policies must pre-filter the UTXO
iterator before calling select_coins; mention CoinSelector and select_coins by
name so readers can locate the relevant API.
🪄 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: 8f8abd56-6f53-4a17-87c6-376496243a9b
📒 Files selected for processing (2)
dash-spv/tests/dashd_sync/tests_transaction.rskey-wallet/src/wallet/managed_wallet_info/coin_selection.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #748 +/- ##
=============================================
+ Coverage 71.73% 71.91% +0.17%
=============================================
Files 321 320 -1
Lines 70013 69451 -562
=============================================
- Hits 50227 49948 -279
+ Misses 19786 19503 -283
|
ZocoLini
force-pushed
the
fix/coin-selector-spend-mempool
branch
from
80e95ce to
a6cb4d4
Compare
#751) `#740` moved `create_test_wallet` from `dashd_sync/setup.rs` to `dash_spv::test_utils` and changed `setup.rs` to a private re-import. `#748` was authored against an earlier base where `setup.rs` still defined a local `pub(super) fn create_test_wallet`, so the new tests in `tests_transaction.rs` imported it via `super::setup`. Each PR was green against its own base, but their merged state fails to compile because the `super::setup` import is now private. Switch the import to `dash_spv::test_utils`, matching `tests_basic.rs`, `tests_mempool.rs`, `tests_multi_wallet.rs`, and `tests_restart.rs`.
2 participants
WalletCoreBalance::spendable()already counts trusted self-send change and untrusted incoming mempool UTXOs, butCoinSelectorfiltered them out, building a tx right after a send returnedNoUtxosAvailabledespite the UI showing funds.Defer candidate filtering to
Utxo::is_spendable(the wallet already decides what's spendable upstream). Adds two dashd integration tests covering both mempool flavors: trusted change and untrusted incoming.Summary by CodeRabbit
Release Notes
Tests
Refactor