Skip to content

Commit fc3d41c

Browse files
ZocoLiniclaude
andcommitted
fix(key-wallet): defer CoinSelector candidate filter to Utxo::is_spendable
`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>
1 parent a5c95be commit fc3d41c

2 files changed

Lines changed: 15 additions & 62 deletions

File tree

dash-spv/tests/dashd_sync/tests_transaction_builder.rs

Lines changed: 6 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -154,47 +154,23 @@ async fn test_spend_unconfirmed_change_balance() {
154154

155155
// Step 1: fund the SPV wallet with a confirmed UTXO from dashd's
156156
// default wallet (an external sender, since it uses a different
157-
// mnemonic). We then send a *second* small tx in a separate block so
158-
// the wallet's `last_processed_height` advances past the funding UTXO's
159-
// height — the coin selector filter `current_height - utxo.height >=
160-
// min_confirmations` (default 1) silently skips UTXOs whose height
161-
// equals the wallet's tip, even though they're visibly confirmed.
157+
// mnemonic).
162158
let receive_address = reserve_first_address(EMPTY_MNEMONIC).await;
163159
let funding_amount = Amount::from_sat(500_000_000);
164160
let funding_txid = ctx.dashd.node.send_to_address(&receive_address, funding_amount);
165161
tracing::info!("Funded fresh SPV wallet with {}, txid: {}", funding_amount, funding_txid);
166162

167163
let miner_address = ctx.dashd.node.get_new_address_from_wallet("default");
168164
ctx.dashd.node.generate_blocks(1, &miner_address);
169-
let h_funded = ctx.dashd.initial_height + 1;
170-
wait_for_sync(&mut client_handle.progress_receiver, h_funded).await;
171-
wait_for_wallet_synced(&wallet, &wallet_id, h_funded).await;
172-
173-
// Second tx + block so `last_processed_height` ticks past `h_funded`.
174-
// SPV only "processes" blocks whose filter matches one of our addresses,
175-
// so an empty miner block alone won't advance `last_processed_height`.
176-
// Sending a small follow-up payment to the wallet's next receive
177-
// address gives block N+1 a wallet-relevant filter match, which forces
178-
// process_block to run and advance the height.
179-
let receive_address_2 = {
180-
let mut wallet_lock = wallet.write().await;
181-
let (w_ref, info) = wallet_lock.get_wallet_and_info_mut(&wallet_id).expect("wallet");
182-
let xpub = w_ref.get_bip44_account(0).expect("bip44 0").account_xpub;
183-
let acc = info.accounts_mut().standard_bip44_accounts.get_mut(&0).expect("acc 0");
184-
acc.next_receive_address(Some(&xpub), true).expect("derive next receive address")
185-
};
186-
let _bump_txid =
187-
ctx.dashd.node.send_to_address(&receive_address_2, Amount::from_sat(20_000_000));
188-
ctx.dashd.node.generate_blocks(1, &miner_address);
189-
let funded_height = h_funded + 1;
165+
let funded_height = ctx.dashd.initial_height + 1;
190166
wait_for_sync(&mut client_handle.progress_receiver, funded_height).await;
191167
wait_for_wallet_synced(&wallet, &wallet_id, funded_height).await;
192168

193169
let initial_spendable = reported_spendable(&wallet, &wallet_id).await;
194-
assert!(
195-
initial_spendable >= funding_amount.to_sat(),
196-
"wallet should report at least the funding amount as spendable, got {}",
197-
initial_spendable
170+
assert_eq!(
171+
initial_spendable,
172+
funding_amount.to_sat(),
173+
"wallet should report exactly the funding amount as spendable"
198174
);
199175

200176
// Step 2: build + sign + broadcast a tx that sends part of the funding

key-wallet/src/wallet/managed_wallet_info/coin_selection.rs

Lines changed: 9 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -79,34 +79,23 @@ pub struct SelectionResult {
7979
/// - High-frequency receivers: **SmallestFirstTill(10)** (balanced approach)
8080
pub struct CoinSelector {
8181
strategy: SelectionStrategy,
82-
min_confirmations: u32,
83-
include_unconfirmed: bool,
8482
dust_threshold: u64,
8583
}
8684

8785
impl CoinSelector {
88-
/// Create a new coin selector
86+
/// Create a new coin selector with defaults that match the wallet's
87+
/// spendable-balance semantics: confirmed, InstantSend-locked, trusted
88+
/// self-send change, and untrusted unconfirmed incoming are all
89+
/// candidates. Callers that want stricter filtering (e.g. exclude
90+
/// mempool incoming, or require N confirmations) opt in via
91+
/// [`with_min_confirmations`] / [`exclude_unconfirmed`].
8992
pub fn new(strategy: SelectionStrategy) -> Self {
9093
Self {
9194
strategy,
92-
min_confirmations: 1,
93-
include_unconfirmed: false,
9495
dust_threshold: 546, // Standard dust threshold
9596
}
9697
}
9798

98-
/// Set minimum confirmations required
99-
pub fn with_min_confirmations(mut self, confirmations: u32) -> Self {
100-
self.min_confirmations = confirmations;
101-
self
102-
}
103-
104-
/// Include unconfirmed UTXOs
105-
pub fn include_unconfirmed(mut self) -> Self {
106-
self.include_unconfirmed = true;
107-
self
108-
}
109-
11099
/// Set dust threshold
111100
pub fn with_dust_threshold(mut self, threshold: u64) -> Self {
112101
self.dust_threshold = threshold;
@@ -159,15 +148,8 @@ impl CoinSelector {
159148
| SelectionStrategy::BranchAndBound
160149
| SelectionStrategy::OptimalConsolidation => {
161150
// These strategies need all UTXOs to sort/analyze
162-
let mut available: Vec<&'a Utxo> = utxos
163-
.into_iter()
164-
.filter(|u| {
165-
u.is_spendable(current_height)
166-
&& (self.include_unconfirmed || u.is_confirmed || u.is_instantlocked)
167-
&& (current_height.saturating_sub(u.height) >= self.min_confirmations
168-
|| u.height == 0)
169-
})
170-
.collect();
151+
let mut available: Vec<&'a Utxo> =
152+
utxos.into_iter().filter(|u| u.is_spendable(current_height)).collect();
171153

172154
if available.is_empty() {
173155
return Err(SelectionError::NoUtxosAvailable);
@@ -261,12 +243,7 @@ impl CoinSelector {
261243
}
262244
SelectionStrategy::Random => {
263245
// Random can work with iterators directly
264-
let filtered = utxos.into_iter().filter(|u| {
265-
u.is_spendable(current_height)
266-
&& (self.include_unconfirmed || u.is_confirmed || u.is_instantlocked)
267-
&& (current_height.saturating_sub(u.height) >= self.min_confirmations
268-
|| u.height == 0)
269-
});
246+
let filtered = utxos.into_iter().filter(|u| u.is_spendable(current_height));
270247

271248
// For Random (currently just uses accumulate as-is)
272249
// TODO: Implement proper random selection for privacy

0 commit comments

Comments
 (0)