From d3af15ede9ca4244b8defdc0ea70c08acd2c550a Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:53:37 +0300 Subject: [PATCH 01/15] fix(key-wallet): resolve trusted self-sends across the whole wallet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update_utxos` decided "are all these inputs ours and final" by looking only at `self.utxos` — the UTXOs of the single account being updated. Pooled funding breaks that assumption: an asset lock draws inputs from BIP44, BIP32 and the DashPay contact-receiving accounts at once, so the account holding the change routinely cannot see the other inputs' parents. It then denied trust to the wallet's own transfer and filed the change under `unconfirmed`, where nothing later corrects it. Assemble the parent view at the wallet level instead. The checker unions each funds account's final parents for the transaction before any account is borrowed mutably — and before `update_utxos` starts removing spent parents — and threads the set down through `record_transaction` / `confirm_transaction`. The per-account lookup is unchanged and still runs first; the set only supplies parents this account cannot see, so callers driving a single account directly pass an empty set and keep today's behavior. Co-Authored-By: Claude Opus 5 --- .../managed_account_collection.rs | 26 +++- .../managed_account/managed_account_ref.rs | 34 ++++-- .../managed_core_funds_account.rs | 49 +++++++- .../transaction_checking/wallet_checker.rs | 112 +++++++++++++++++- 4 files changed, 209 insertions(+), 12 deletions(-) diff --git a/key-wallet/src/managed_account/managed_account_collection.rs b/key-wallet/src/managed_account/managed_account_collection.rs index cf60f086b..34e20ef1b 100644 --- a/key-wallet/src/managed_account/managed_account_collection.rs +++ b/key-wallet/src/managed_account/managed_account_collection.rs @@ -3,7 +3,10 @@ //! This module provides a structure for managing multiple accounts //! across different networks in a hierarchical manner. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; + +use dashcore::blockdata::transaction::OutPoint; +use dashcore::Transaction; use crate::account::account_collection::{DashpayAccountKey, PlatformPaymentAccountKey}; use crate::gap_limit::DIP17_GAP_LIMIT; @@ -934,6 +937,27 @@ impl ManagedAccountCollection { accounts } + /// Union, across every funds-bearing account, of the outpoints among + /// `tx`'s inputs that the wallet holds as final UTXOs. + /// + /// A single account can only answer this for the coins it owns, but + /// pooled funding (asset locks draw from BIP44 + BIP32 + the DashPay + /// contact-receiving accounts) routinely spreads one transaction's inputs + /// across several. The union is what makes the trusted-self-send check in + /// [`ManagedCoreFundsAccount::record_transaction`] see the whole wallet. + /// + /// Must be taken before any account processes `tx` — `update_utxos` + /// removes spent parents as it goes. + pub(crate) fn final_parents_of(&self, tx: &Transaction) -> BTreeSet { + let mut parents = BTreeSet::new(); + for account in self.all_accounts() { + if let ManagedAccountRef::Funds(funds) = account { + funds.collect_final_parents(tx, &mut parents); + } + } + parents + } + /// Get all accounts in the collection as mutable /// [`ManagedAccountRefMut`] values. pub fn all_accounts_mut(&mut self) -> Vec> { diff --git a/key-wallet/src/managed_account/managed_account_ref.rs b/key-wallet/src/managed_account/managed_account_ref.rs index b94895b79..172d40327 100644 --- a/key-wallet/src/managed_account/managed_account_ref.rs +++ b/key-wallet/src/managed_account/managed_account_ref.rs @@ -23,7 +23,7 @@ use crate::Network; use dashcore::blockdata::transaction::OutPoint; use dashcore::prelude::CoreBlockHeight; use dashcore::{Address, ScriptBuf, Transaction, Txid}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; /// Immutable reference to a managed core account, either funds-bearing or /// keys-only. @@ -314,6 +314,7 @@ impl<'a> ManagedAccountRefMut<'a> { context, transaction_type, &BTreeMap::new(), + &BTreeSet::new(), ) } @@ -321,6 +322,9 @@ impl<'a> ManagedAccountRefMut<'a> { /// the wallet-level `observed_spent_outpoints` view /// (dashpay/rust-dashcore#649); only the funds variant consults it (keys /// accounts track no UTXOs/output details). + /// + /// `external_final_parents` is the wallet-level view of input parents held + /// by sibling accounts, used for the trusted-self-send determination. pub(crate) fn record_transaction_with_observed_spends( &mut self, tx: &Transaction, @@ -328,11 +332,17 @@ impl<'a> ManagedAccountRefMut<'a> { context: TransactionContext, transaction_type: TransactionType, observed_spent: &BTreeMap, + external_final_parents: &BTreeSet, ) -> TransactionRecord { match self { - ManagedAccountRefMut::Funds(a) => { - a.record_transaction(tx, account_match, context, transaction_type, observed_spent) - } + ManagedAccountRefMut::Funds(a) => a.record_transaction( + tx, + account_match, + context, + transaction_type, + observed_spent, + external_final_parents, + ), ManagedAccountRefMut::Keys(a) => { a.record_transaction(tx, account_match, context, transaction_type) } @@ -361,12 +371,16 @@ impl<'a> ManagedAccountRefMut<'a> { context, transaction_type, &BTreeMap::new(), + &BTreeSet::new(), ) } /// Re-process an existing transaction, reconciling refreshed UTXO state /// against `observed_spent` — the wallet-level `observed_spent_outpoints` /// view (dashpay/rust-dashcore#649); only the funds variant consults it. + /// + /// `external_final_parents` is the wallet-level view of input parents held + /// by sibling accounts, used for the trusted-self-send determination. pub(crate) fn confirm_transaction_with_observed_spends( &mut self, tx: &Transaction, @@ -374,11 +388,17 @@ impl<'a> ManagedAccountRefMut<'a> { context: TransactionContext, transaction_type: TransactionType, observed_spent: &BTreeMap, + external_final_parents: &BTreeSet, ) -> Option { match self { - ManagedAccountRefMut::Funds(a) => { - a.confirm_transaction(tx, account_match, context, transaction_type, observed_spent) - } + ManagedAccountRefMut::Funds(a) => a.confirm_transaction( + tx, + account_match, + context, + transaction_type, + observed_spent, + external_final_parents, + ), ManagedAccountRefMut::Keys(a) => { a.confirm_transaction(tx, account_match, context, transaction_type) } diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 663e24dc6..4f2d9376e 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -164,6 +164,23 @@ impl ManagedCoreFundsAccount { self.spent_outpoints.contains(outpoint) } + /// Collect the outpoints among `tx`'s inputs that this account holds as a + /// final UTXO — confirmed, InstantSend-locked, or trusted. + /// + /// Used at the wallet level to assemble the cross-account parent view that + /// [`Self::record_transaction`] needs: one account cannot tell whether a + /// pooled transaction's other inputs are ours, but the wallet can ask every + /// account and union the answers. + pub(crate) fn collect_final_parents(&self, tx: &Transaction, into: &mut BTreeSet) { + for input in &tx.input { + if self.utxos.get(&input.previous_output).is_some_and(|parent| { + parent.is_confirmed || parent.is_instantlocked || parent.is_trusted + }) { + into.insert(input.previous_output); + } + } + } + /// Cached scriptPubKeys for every address that could still receive or hold /// funds under a single-use address discipline: addresses not yet used /// (the gap-limit lookahead, including reserved ones) plus used addresses @@ -190,12 +207,18 @@ impl ManagedCoreFundsAccount { /// /// Skips any output whose outpoint is already in `observed_spent` — it is /// spent on-chain (dashpay/rust-dashcore#649), so the record stays consistent. + /// + /// `external_final_parents` carries the wallet-level view of the inputs: + /// outpoints that a *sibling* account of the same wallet holds as final. + /// See [`Self::record_transaction`] for why a per-account view is not + /// enough. An empty set degrades this to the account-local check. fn update_utxos( &mut self, tx: &Transaction, account_match: &AccountMatch, context: TransactionContext, observed_spent: &BTreeMap, + external_final_parents: &BTreeSet, ) { // Update UTXOs only for spendable account types match self.keys.managed_account_type() { @@ -233,10 +256,19 @@ impl ManagedCoreFundsAccount { // they are only removed after the insert loop below. An unknown // or non-final parent denies trust, so funds that the network // may still drop never surface as confirmed. + // + // "Ours" is a wallet-level question, not an account-level one: + // pooled funding (asset locks draw from BIP44 + BIP32 + the + // DashPay contact-receiving accounts) routinely puts inputs + // from a sibling account into a transaction whose change lands + // here. Consulting only `self.utxos` would deny trust to our + // own transfer and file its change under `unconfirmed`, so the + // caller's wallet-wide view fills in the parents this account + // cannot see. let all_inputs_final_and_ours = tx.input.iter().all(|input| { self.utxos.get(&input.previous_output).is_some_and(|parent| { parent.is_confirmed || parent.is_instantlocked || parent.is_trusted - }) + }) || external_final_parents.contains(&input.previous_output) }); let txid = tx.txid(); @@ -366,6 +398,7 @@ impl ManagedCoreFundsAccount { context: TransactionContext, transaction_type: TransactionType, observed_spent: &BTreeMap, + external_final_parents: &BTreeSet, ) -> Option { let txid = tx.txid(); @@ -384,6 +417,7 @@ impl ManagedCoreFundsAccount { context, transaction_type, observed_spent, + external_final_parents, ); return Some(record); } @@ -423,7 +457,7 @@ impl ManagedCoreFundsAccount { // chainlock catches up. #[cfg(not(feature = "keep-finalized-transactions"))] let drop_now = context.is_chain_locked(); - self.update_utxos(tx, account_match, context, observed_spent); + self.update_utxos(tx, account_match, context, observed_spent, external_final_parents); #[cfg(not(feature = "keep-finalized-transactions"))] if drop_now { self.keys.drop_finalized_transaction(&txid); @@ -439,6 +473,14 @@ impl ManagedCoreFundsAccount { /// for any output whose outpoint is already observed spent on-chain, so a /// coin whose spend was seen in an earlier-processed block is never /// (re-)tracked as spendable. + /// + /// `external_final_parents` is the wallet-level answer to "are these + /// inputs ours and final" for parents this account does not hold. A + /// transaction funded from several accounts — the normal shape for asset + /// locks — is still our own self-send, and its change must not be filed + /// under `unconfirmed` merely because the sibling account's UTXOs are + /// invisible from here. Callers driving a single account directly pass an + /// empty set and get the account-local behavior. pub(crate) fn record_transaction( &mut self, tx: &Transaction, @@ -446,6 +488,7 @@ impl ManagedCoreFundsAccount { context: TransactionContext, transaction_type: TransactionType, observed_spent: &BTreeMap, + external_final_parents: &BTreeSet, ) -> TransactionRecord { let net_amount = account_match.received as i64 - account_match.sent as i64; @@ -553,7 +596,7 @@ impl ManagedCoreFundsAccount { // feature is on (we want to keep the full record). #[cfg(not(feature = "keep-finalized-transactions"))] let drop_now = context.is_chain_locked(); - self.update_utxos(tx, account_match, context, observed_spent); + self.update_utxos(tx, account_match, context, observed_spent, external_final_parents); #[cfg(not(feature = "keep-finalized-transactions"))] if drop_now { self.keys.drop_finalized_transaction(&txid); diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index cb0272f2a..900d77f51 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -85,6 +85,15 @@ impl WalletTransactionChecker for ManagedWalletInfo { return result; } + // Wallet-wide view of this transaction's input parents, taken while + // every account is still readable and before any `update_utxos` call + // starts removing spent parents. Without it a pooled self-send — the + // normal shape for asset locks, which fund from BIP44 + BIP32 + the + // DashPay contact-receiving accounts — is not recognised as ours by + // the account holding the change, and that change lands in the + // `unconfirmed` bucket. + let external_final_parents = self.accounts.final_parents_of(tx); + // Check if this transaction already exists in any affected account let txid = tx.txid(); let mut is_new = true; @@ -150,6 +159,7 @@ impl WalletTransactionChecker for ManagedWalletInfo { context.clone(), tx_type, &self.observed_spent_outpoints, + &external_final_parents, ); account.mark_utxos_instant_send(&txid); result.new_records.push(record); @@ -182,6 +192,7 @@ impl WalletTransactionChecker for ManagedWalletInfo { context.clone(), tx_type, &self.observed_spent_outpoints, + &external_final_parents, ); result.new_records.push(record); result.state_modified = true; @@ -193,6 +204,7 @@ impl WalletTransactionChecker for ManagedWalletInfo { context.clone(), tx_type, &self.observed_spent_outpoints, + &external_final_parents, ) { result.state_modified = true; if existed_before { @@ -286,7 +298,7 @@ mod tests { use dashcore::TxOut; use dashcore::{Address, BlockHash, TxIn, Txid}; use dashcore_hashes::Hash; - use std::collections::BTreeMap; + use std::collections::{BTreeMap, BTreeSet}; /// Test wallet checker with unrelated transaction #[tokio::test] @@ -1412,6 +1424,7 @@ mod tests { block_context, tx_type, &BTreeMap::new(), + &BTreeSet::new(), ); assert!(backfilled.is_some(), "Should return Some when backfilling a missing record"); @@ -1469,6 +1482,7 @@ mod tests { block_context, tx_type, &BTreeMap::new(), + &BTreeSet::new(), ); assert!(confirmed.is_some(), "Should return Some when confirming unconfirmed tx"); @@ -2038,6 +2052,102 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// Pooled funding spans account families — an asset lock draws from BIP44, + /// BIP32 and the DashPay contact-receiving accounts at once — so the + /// trusted-self-send check must be answered by the whole wallet, not by + /// the single account that happens to hold the change. Here the only input + /// belongs to the BIP32 account while the change lands on BIP44: the + /// transaction is still entirely ours, and its change must be trusted. + #[tokio::test] + async fn test_self_send_change_is_trusted_when_parent_is_in_a_sibling_account() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // Fund the *BIP32* account, confirmed in a block. + let bip32_xpub = ctx + .wallet + .accounts + .standard_bip32_accounts + .get(&0) + .expect("BIP32 account") + .account_xpub; + let bip32_address = ctx + .managed_wallet + .first_bip32_managed_account_mut() + .expect("BIP32 managed account") + .next_receive_address(Some(&bip32_xpub), true) + .expect("BIP32 receive address"); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&bip32_address, 0..1, &[funding_value]); + let block_context = TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[7u8; 32]).expect("hash"), + 1_700_000_000, + )); + ctx.check_transaction(&funding_tx, block_context).await; + assert_eq!(ctx.managed_wallet.balance.confirmed(), funding_value); + + // Change goes to the BIP44 account, which holds none of the inputs. + let change_address = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + + let send_amount = 600_000u64; + let fee = 1_000u64; + let change_amount = funding_value - send_amount - fee; + let spend_tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: funding_tx.txid(), + vout: 0, + }, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: send_amount, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change_address.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + + let result = ctx.check_transaction(&spend_tx, TransactionContext::Mempool).await; + assert!(result.is_relevant); + + let change_outpoint = OutPoint { + txid: spend_tx.txid(), + vout: 1, + }; + let change_utxo = + ctx.bip44_account().utxos.get(&change_outpoint).expect("change UTXO recorded"); + assert!(!change_utxo.is_confirmed); + assert!( + change_utxo.is_trusted, + "change of a wallet-owned transfer must be trusted even when the spent \ + parent lives in a sibling account" + ); + + // And therefore it is confirmed, not unconfirmed, in the balance split. + assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); + assert_eq!(ctx.managed_wallet.balance.confirmed(), change_amount); + } + /// Sibling of `test_self_send_change_in_mempool_lands_in_confirmed_balance`: /// a self-send change output is only trusted when the spent parent is /// itself final. `Utxo::is_trusted` mirrors Bitcoin Core's From 0611ba6b7755127c515b871d33597451a1f5362e Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:08:05 +0300 Subject: [PATCH 02/15] fix(key-wallet): drop the outputs of a transaction that lost its inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A self-originated transaction that is beaten to its inputs can never confirm, but the wallet kept crediting the change it contributed. Nothing removed it: the loser is in no block, so no block processing revisits it, and dash-spv's mempool expiry only drops its own tracking without telling the wallet. The change sat in the `unconfirmed` bucket permanently — money the wallet displays and does not have. When a transaction arrives with a final context, every input it spends is settled under consensus, so any other recorded unconfirmed transaction spending the same outpoint is provably dead. Drop that transaction's outputs and its record. Deliberately narrow, on two counts. It fires only on proof of a conflicting final spend, never on a timeout: the p2p network has no negative signal since BIP61 `reject` was removed, so a transaction that merely went quiet may still be live in a miner's mempool, and un-applying it would re-expose its inputs to coin selection and invite a double-spend. And it reverts only the loser's outputs, which needs no recovery of discarded state — its inputs are already correctly accounted for by the transaction that actually spent them. Reverting a transaction of unknown fate would additionally require restoring the spent parents, whose `Utxo` values are not retained anywhere; that case needs an explicit abandon primitive driven by the layer that owns broadcast policy, not this. Co-Authored-By: Claude Opus 5 --- .../managed_core_funds_account.rs | 84 ++++++++++++++ .../transaction_checking/wallet_checker.rs | 108 ++++++++++++++++++ 2 files changed, 192 insertions(+) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 4f2d9376e..8e91a8807 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -367,6 +367,8 @@ impl ManagedCoreFundsAccount { } } + utxos_changed |= self.drop_conflicted_transactions(tx, &context); + if utxos_changed { self.keys.bump_monitor_revision(); } @@ -375,6 +377,88 @@ impl ManagedCoreFundsAccount { } } + /// Drop the outputs of any recorded unconfirmed transaction that `tx` + /// provably beat to one of its inputs. + /// + /// When `tx` arrives with a final context — in a block, or InstantSend + /// locked — every input it spends is settled under Dash consensus. Any + /// *other* transaction we recorded that spends the same outpoint can + /// therefore never confirm, and the UTXOs it contributed (its change) are + /// money that does not exist. Nothing else removes them: the loser is not + /// in a block, so no block processing revisits it, and mempool expiry in + /// dash-spv only drops its own tracking without telling the wallet. Left + /// alone they sit in the `unconfirmed` bucket permanently. + /// + /// This is deliberately narrow. It fires only on proof — a conflicting + /// spend that is itself final — never on a timeout: the p2p network has no + /// negative signal (modern Dash Core removed BIP61 `reject`), so a + /// transaction that merely went quiet may still be alive in a miner's + /// mempool, and un-applying it would re-expose its inputs to coin + /// selection and invite a double-spend. + /// + /// Only the loser's *outputs* are reverted, which needs no recovery of + /// discarded state: its inputs are correctly accounted for by `tx`, the + /// transaction that actually spent them. Reverting a transaction whose + /// fate is unknown would additionally require restoring the spent parents, + /// and the `Utxo` values removed for them — with their flags — are not + /// retained anywhere. + /// + /// Scope: account-local. A loser recorded here has its outputs dropped + /// here; a loser whose change landed in a *different* account is not + /// reached, because both the transaction records and the UTXO set are + /// per-account. That covers the ordinary shape — a resend keeps the same + /// funding account and so the same change account — but not every one. + /// + /// Returns whether any UTXO was removed. + fn drop_conflicted_transactions( + &mut self, + tx: &Transaction, + context: &TransactionContext, + ) -> bool { + if !(context.confirmed() || matches!(context, TransactionContext::InstantSend(_))) { + return false; + } + + let winner = tx.txid(); + let spent: BTreeSet = + tx.input.iter().map(|input| input.previous_output).collect(); + + // A finalized transaction keeps only its txid, so a chainlocked record + // can never be a loser here — and must not be, since it is settled. + let losers: Vec = self + .keys + .transactions() + .iter() + .filter(|(txid, record)| { + **txid != winner + && !record.is_confirmed() + && record + .transaction + .input + .iter() + .any(|input| spent.contains(&input.previous_output)) + }) + .map(|(txid, _)| *txid) + .collect(); + + let mut changed = false; + for loser in losers { + let removed: Vec = + self.utxos.keys().filter(|outpoint| outpoint.txid == loser).copied().collect(); + for outpoint in removed { + self.utxos.remove(&outpoint); + changed = true; + } + self.keys.transactions_mut().remove(&loser); + tracing::info!( + conflicted_txid = %loser, + winning_txid = %winner, + "Dropped a conflicted transaction: its input was spent by a final transaction" + ); + } + changed + } + /// Re-process an existing transaction with updated context (e.g., /// mempool→block confirmation) and potentially new address matches /// from gap limit rescans. diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 900d77f51..6c15331bf 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2052,6 +2052,114 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// A transaction that loses a race for its inputs can never confirm, so + /// the change it contributed is money that does not exist. Nothing else + /// removes it — the loser is in no block, so no block processing revisits + /// it — and it would otherwise sit in the `unconfirmed` bucket forever. + #[tokio::test] + async fn test_conflicting_confirmed_spend_drops_the_losing_transactions_outputs() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // Confirmed funding UTXO. + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + let funding_outpoint = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let spend_of = |change_address: &Address, change_amount: u64, sent: u64| Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change_address.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + + // First attempt: broadcast into the mempool, change comes back to us. + let first_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend_of(&first_change, 399_000, 600_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + let loser_change = OutPoint { + txid: loser.txid(), + vout: 1, + }; + assert!( + ctx.bip44_account().utxos.contains_key(&loser_change), + "the first attempt's change is tracked while it is still live" + ); + + // Second attempt spends the same input and confirms in a block. The + // first attempt can now never confirm. + let second_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let winner = spend_of(&second_change, 299_000, 700_000); + ctx.check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[2u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + + assert!( + !ctx.bip44_account().utxos.contains_key(&loser_change), + "the losing transaction's change must not survive as spendable money" + ); + assert!( + !ctx.bip44_account().transactions().contains_key(&loser.txid()), + "the losing transaction's record must be dropped too" + ); + + // Only the winner's change remains, and it is the whole balance. + let winner_change = OutPoint { + txid: winner.txid(), + vout: 1, + }; + assert!(ctx.bip44_account().utxos.contains_key(&winner_change)); + assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); + assert_eq!(ctx.managed_wallet.balance.confirmed(), 299_000); + } + /// Pooled funding spans account families — an asset lock draws from BIP44, /// BIP32 and the DashPay contact-receiving accounts at once — so the /// trusted-self-send check must be answered by the whole wallet, not by From 157b9d98da063d6001f4c7844022089b21d81448 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:00:11 +0300 Subject: [PATCH 03/15] feat(key-wallet): abandon a dead transaction and everything built on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A transaction the network never accepted still mutated the wallet: its outputs were credited and its inputs marked spent. Nothing reverses that — it is in no block, so no block processing revisits it — and further transactions get built on its change, each inheriting the same fiction. A testnet device carried three such transactions chained together, 1.57 DASH of outputs the network had never seen, permanently in `unconfirmed`. `abandon_transaction` takes a root txid, walks the recorded spenders transitively, and drops the whole chain: outputs, records, and the reservations they held. The walk is wallet-wide because pooled funding spreads a transaction's inputs across account families, so a descendant's change can land in an account holding none of the root. Confirmed and finalized transactions are never followed — they are settled, so what they spent was real. The coins the chain consumed are released from `spent_outpoints` rather than re-credited. The `Utxo` removed for a spent parent is discarded by `update_utxos` and `InputDetail` keeps only index/value/address, so the flags deciding a restored coin's balance bucket survive nowhere; inventing them would be a guess. What those coins actually are is unspent on chain, so the rescan that the release enables is the honest source. The call asserts the root is dead, it does not establish it — silence is not proof on a network with no reject message. The judgement stays with the layer that owns broadcast policy. Co-Authored-By: Claude Opus 5 --- .../managed_core_funds_account.rs | 73 +++++++++++ .../transaction_checking/wallet_checker.rs | 123 ++++++++++++++++++ .../src/wallet/managed_wallet_info/helpers.rs | 83 ++++++++++++ .../src/wallet/managed_wallet_info/mod.rs | 1 + 4 files changed, 280 insertions(+) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 8e91a8807..5c7e4a34d 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -377,6 +377,79 @@ impl ManagedCoreFundsAccount { } } + /// Collect the txids of recorded unconfirmed transactions that spend an + /// output of any transaction in `abandoned`. + /// + /// One step of the descendant walk driven by + /// [`ManagedWalletInfo::abandon_transaction`]. A confirmed or finalized + /// transaction is never a descendant for this purpose: it is settled on + /// chain, so whatever it spent was real. + /// + /// [`ManagedWalletInfo::abandon_transaction`]: crate::wallet::managed_wallet_info::ManagedWalletInfo::abandon_transaction + pub(crate) fn collect_spenders_of( + &self, + abandoned: &BTreeSet, + into: &mut BTreeSet, + ) { + for (txid, record) in self.keys.transactions() { + if record.is_confirmed() || abandoned.contains(txid) { + continue; + } + let spends_abandoned = record + .transaction + .input + .iter() + .any(|input| abandoned.contains(&input.previous_output.txid)); + if spends_abandoned { + into.insert(*txid); + } + } + } + + /// Remove every trace of `abandoned` from this account. + /// + /// Drops the outputs those transactions contributed and their records, and + /// releases the outpoints they spent from `spent_outpoints` so the coins + /// become eligible for rediscovery. + /// + /// The released parents are deliberately **not** re-inserted into `utxos`. + /// `update_utxos` discards the `Utxo` when it removes a spent parent, and + /// `InputDetail` keeps only index/value/address, so the flags that decide + /// which balance bucket a restored coin belongs in are not retained + /// anywhere. Inventing them would be a guess. What these coins genuinely + /// are is unspent on chain — the abandoned transaction never reached the + /// network — so the correct source of truth is a rescan, which releasing + /// them from `spent_outpoints` now permits. + /// + /// Returns the number of UTXOs removed. + pub(crate) fn apply_abandon(&mut self, abandoned: &BTreeSet) -> usize { + let doomed: Vec = self + .utxos + .keys() + .filter(|outpoint| abandoned.contains(&outpoint.txid)) + .copied() + .collect(); + let removed = doomed.len(); + for outpoint in doomed { + self.utxos.remove(&outpoint); + } + + for txid in abandoned { + if let Some(record) = self.keys.transactions_mut().remove(txid) { + for input in &record.transaction.input { + self.spent_outpoints.remove(&input.previous_output); + } + self.reservations + .release(record.transaction.input.iter().map(|input| &input.previous_output)); + } + } + + if removed > 0 { + self.keys.bump_monitor_revision(); + } + removed + } + /// Drop the outputs of any recorded unconfirmed transaction that `tx` /// provably beat to one of its inputs. /// diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 6c15331bf..8816f4aca 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2052,6 +2052,129 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// Abandoning a transaction that never reached the network must take the + /// transactions built on its change with it. This reproduces the shape + /// seen on a testnet device: an asset-lock funding transaction stuck at + /// `Built` whose broadcast never happened, then two further self-sends + /// chained onto its phantom change. Five UTXOs from three transactions, + /// none of which the network ever saw, and the whole chain has to go. + #[tokio::test] + async fn test_abandoning_an_unbroadcast_root_cascades_to_its_descendants() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // A real, confirmed coin funds the chain. + let funding_value = 100_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + assert_eq!(ctx.managed_wallet.balance.confirmed(), funding_value); + + // Build a three-link chain, each link spending its parent's change. + // Every one of them stays in the mempool: nothing was ever broadcast. + let mut parent = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let mut change_left = funding_value; + let mut chain = Vec::new(); + for sent in [40_000_000u64, 20_000_000, 5_000_000] { + let fee = 226u64; + let change = change_left - sent - fee; + let change_address = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: parent, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change, + script_pubkey: change_address.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + ctx.check_transaction(&tx, TransactionContext::Mempool).await; + parent = OutPoint { + txid: tx.txid(), + vout: 1, + }; + change_left = change; + chain.push(tx); + } + + let root = chain[0].txid(); + // Only the tip's change is tracked — each link consumed its parent's. + assert_eq!(ctx.bip44_account().utxos.len(), 1, "one live change output"); + + let outcome = ctx.managed_wallet.abandon_transaction(root); + ctx.managed_wallet.update_balance(); + + assert_eq!( + outcome.abandoned.len(), + 3, + "the root and both descendants must be abandoned, got {:?}", + outcome.abandoned + ); + for tx in &chain { + assert!( + outcome.abandoned.contains(&tx.txid()), + "chain member {} must be abandoned", + tx.txid() + ); + assert!( + !ctx.bip44_account().transactions().contains_key(&tx.txid()), + "chain member {} must lose its record", + tx.txid() + ); + } + assert!(ctx.bip44_account().utxos.is_empty(), "no phantom output may survive the cascade"); + assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); + + // The real coin the chain consumed is released from the spent set, so + // a rescan can rediscover it rather than being told it is spent. + let rediscovered = ctx + .check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + assert!(rediscovered.is_relevant); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + funding_value, + "the funding coin comes back on rescan — the chain never spent it on chain" + ); + } + /// A transaction that loses a race for its inputs can never confirm, so /// the change it contributed is money that does not exist. Nothing else /// removes it — the loser is in no block, so no block processing revisits diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index 5237f1c5f..8d0d5c9eb 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -3,10 +3,93 @@ use super::ManagedWalletInfo; use crate::account::account_collection::PlatformPaymentAccountKey; use crate::account::ManagedCoreFundsAccount; +use crate::managed_account::managed_account_ref::{ManagedAccountRef, ManagedAccountRefMut}; use crate::managed_account::managed_platform_account::ManagedPlatformAccount; use crate::managed_account::ManagedCoreKeysAccount; +use dashcore::Txid; +use std::collections::BTreeSet; + +/// What [`ManagedWalletInfo::abandon_transaction`] removed. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AbandonOutcome { + /// Every transaction dropped: the root and its recorded descendants. + pub abandoned: BTreeSet, + /// How many UTXOs those transactions had contributed. + pub utxos_removed: usize, +} + +impl AbandonOutcome { + /// Whether anything was actually removed. + pub fn is_empty(&self) -> bool { + self.abandoned.is_empty() && self.utxos_removed == 0 + } +} impl ManagedWalletInfo { + /// Abandon `root` and every recorded transaction descending from it. + /// + /// A transaction the network never accepted still mutated this wallet: + /// its outputs were credited and its inputs marked spent. Nothing reverses + /// that on its own — the transaction is in no block, so no block + /// processing revisits it — and further transactions can be built on its + /// change, each inheriting the same fiction. Left alone the whole chain + /// sits in the `unconfirmed` bucket permanently, as money the wallet + /// displays and does not have. + /// + /// The walk is transitive and wallet-wide: pooled funding spreads a + /// transaction's inputs across account families, so a descendant's change + /// can land in an account that holds none of the root. Confirmed and + /// finalized transactions are never followed — they are settled on chain, + /// so whatever they spent was real. + /// + /// **This call asserts that the root is dead; it does not establish it.** + /// The p2p network has no negative signal — modern Dash Core removed BIP61 + /// `reject` — so silence is not proof, and a transaction that merely went + /// quiet may still be live in a miner's mempool. Abandoning such a + /// transaction re-exposes its inputs to coin selection and invites a + /// double-spend. Only call this where the death is known: a build that + /// provably never reached the network, or an explicit user decision. The + /// judgement belongs to the layer that owns broadcast policy. + /// + /// The coins the abandoned transactions consumed are released from the + /// spent set so a rescan can rediscover them; see + /// [`ManagedCoreFundsAccount::apply_abandon`] for why they are not + /// re-credited directly. + /// + /// Does not recompute the balance — callers batching several abandons + /// should run [`update_balance`](Self::update_balance) once at the end. + pub fn abandon_transaction(&mut self, root: Txid) -> AbandonOutcome { + let mut abandoned = BTreeSet::from([root]); + + // Transitive closure over recorded spenders. Each pass can only add + // txids, and the set is bounded by the recorded transactions, so this + // terminates; a spend cycle is impossible anyway. + loop { + let mut found = BTreeSet::new(); + for account in self.accounts.all_accounts() { + if let ManagedAccountRef::Funds(funds) = account { + funds.collect_spenders_of(&abandoned, &mut found); + } + } + let before = abandoned.len(); + abandoned.extend(found); + if abandoned.len() == before { + break; + } + } + + let mut utxos_removed = 0; + for account in self.accounts.all_accounts_mut() { + if let ManagedAccountRefMut::Funds(funds) = account { + utxos_removed += funds.apply_abandon(&abandoned); + } + } + + AbandonOutcome { + abandoned, + utxos_removed, + } + } // BIP44 Account Helpers /// Get the first BIP44 managed account diff --git a/key-wallet/src/wallet/managed_wallet_info/mod.rs b/key-wallet/src/wallet/managed_wallet_info/mod.rs index 698e3327b..6bfece722 100644 --- a/key-wallet/src/wallet/managed_wallet_info/mod.rs +++ b/key-wallet/src/wallet/managed_wallet_info/mod.rs @@ -7,6 +7,7 @@ pub mod asset_lock_builder; pub mod coin_selection; pub mod fee; pub mod helpers; +pub use helpers::AbandonOutcome; pub mod managed_account_operations; pub mod managed_accounts; pub mod transaction_builder; From b45603c95591777b4e7ca388fb2d94bb0bf90967 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:18:51 +0300 Subject: [PATCH 04/15] feat(key-wallet): let the abandon cascade follow an external spend view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The descendant walk reads recorded transactions, which is enough while the wallet is live but not after a restore that brings back UTXOs without their creating transactions. On a testnet device the walk stopped at the root and left two descendants credited — their records were never in the map to be found. `abandon_transaction_with_spends` takes an outpoint-to-spender map from the caller's persistence mirror and follows both views. The no-argument form is unchanged. Co-Authored-By: Claude Opus 5 --- .../src/wallet/managed_wallet_info/helpers.rs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index 8d0d5c9eb..e0aee1c12 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -6,8 +6,8 @@ use crate::account::ManagedCoreFundsAccount; use crate::managed_account::managed_account_ref::{ManagedAccountRef, ManagedAccountRefMut}; use crate::managed_account::managed_platform_account::ManagedPlatformAccount; use crate::managed_account::ManagedCoreKeysAccount; -use dashcore::Txid; -use std::collections::BTreeSet; +use dashcore::{OutPoint, Txid}; +use std::collections::{BTreeMap, BTreeSet}; /// What [`ManagedWalletInfo::abandon_transaction`] removed. #[derive(Debug, Clone, PartialEq, Eq)] @@ -59,6 +59,23 @@ impl ManagedWalletInfo { /// Does not recompute the balance — callers batching several abandons /// should run [`update_balance`](Self::update_balance) once at the end. pub fn abandon_transaction(&mut self, root: Txid) -> AbandonOutcome { + self.abandon_transaction_with_spends(root, &BTreeMap::new()) + } + + /// [`abandon_transaction`](Self::abandon_transaction), with an external + /// view of who spent what. + /// + /// The descendant walk normally reads recorded transactions, but a caller + /// restoring a wallet may hold UTXOs whose creating transactions were + /// never put back into the in-memory map — leaving the walk unable to see + /// that one abandoned output funded the next transaction along. Callers + /// with a persistence mirror can supply `external_spends`, mapping an + /// outpoint to the transaction that spent it, and the walk follows both. + pub fn abandon_transaction_with_spends( + &mut self, + root: Txid, + external_spends: &BTreeMap, + ) -> AbandonOutcome { let mut abandoned = BTreeSet::from([root]); // Transitive closure over recorded spenders. Each pass can only add @@ -71,6 +88,13 @@ impl ManagedWalletInfo { funds.collect_spenders_of(&abandoned, &mut found); } } + // Same step over the external view: anything spending an output of + // an abandoned transaction is itself abandoned. + for (outpoint, spender) in external_spends { + if abandoned.contains(&outpoint.txid) { + found.insert(*spender); + } + } let before = abandoned.len(); abandoned.extend(found); if abandoned.len() == before { From 4db83340decd21cbdef7d460479d034e57a8f9fb Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:52:21 +0300 Subject: [PATCH 05/15] docs(key-wallet): fix the two broken intra-doc links on abandon_transaction `apply_abandon` is private, so linking to it from a public item fails the docs build; state the reasoning inline instead. `update_balance` comes from `WalletInfoInterface`, not from `Self`. Co-Authored-By: Claude Opus 5 --- key-wallet/src/wallet/managed_wallet_info/helpers.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index e0aee1c12..2fdc998a9 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -52,12 +52,16 @@ impl ManagedWalletInfo { /// judgement belongs to the layer that owns broadcast policy. /// /// The coins the abandoned transactions consumed are released from the - /// spent set so a rescan can rediscover them; see - /// [`ManagedCoreFundsAccount::apply_abandon`] for why they are not - /// re-credited directly. + /// spent set so a rescan can rediscover them, rather than being + /// re-credited directly: the `Utxo` removed for a spent parent is + /// discarded by `update_utxos` and `InputDetail` keeps only + /// index/value/address, so the flags that decide a restored coin's + /// balance bucket are not retained anywhere. /// /// Does not recompute the balance — callers batching several abandons - /// should run [`update_balance`](Self::update_balance) once at the end. + /// should run `update_balance` + /// ([`WalletInfoInterface`](crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface)) + /// once at the end. pub fn abandon_transaction(&mut self, root: Txid) -> AbandonOutcome { self.abandon_transaction_with_spends(root, &BTreeMap::new()) } From 241f7cfa0fe9347ecc92796ba267cec260901a82 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:59:26 +0300 Subject: [PATCH 06/15] fix(key-wallet): address the review findings on the abandon path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AbandonOutcome::is_empty` could never return true: `abandoned` always holds the root, whether or not the wallet had anything recorded for it, so a caller guarding on `!is_empty()` acted on every call — including one that removed nothing. Count what was actually dropped instead, and report records alongside UTXOs. `apply_abandon` removed each abandoned record's inputs from `spent_outpoints` one record at a time, which un-marks an outpoint that a *surviving* transaction also spends — precisely the double-spend shape this work exists for, where the loser and the winner share an input. Re-derive the set from the surviving records instead; only they can say which outpoints are still spent. `rebuild_spent_outpoints` loses its serde/test cfg gate accordingly. `final_parents_of` and the cascade walk now iterate `all_funding_accounts` rather than filtering `all_accounts`. Simpler, and it drops DashPay *external* watch-only accounts from the scan — a coin the wallet cannot spend was never an input to a transaction the wallet built, so it has no business granting trust. Co-Authored-By: Claude Opus 5 --- .../managed_account_collection.rs | 6 ++-- .../managed_core_funds_account.rs | 36 ++++++++++++++----- .../src/wallet/managed_wallet_info/helpers.rs | 26 ++++++++------ 3 files changed, 45 insertions(+), 23 deletions(-) diff --git a/key-wallet/src/managed_account/managed_account_collection.rs b/key-wallet/src/managed_account/managed_account_collection.rs index 34e20ef1b..d5ef903b4 100644 --- a/key-wallet/src/managed_account/managed_account_collection.rs +++ b/key-wallet/src/managed_account/managed_account_collection.rs @@ -950,10 +950,8 @@ impl ManagedAccountCollection { /// removes spent parents as it goes. pub(crate) fn final_parents_of(&self, tx: &Transaction) -> BTreeSet { let mut parents = BTreeSet::new(); - for account in self.all_accounts() { - if let ManagedAccountRef::Funds(funds) = account { - funds.collect_final_parents(tx, &mut parents); - } + for funds in self.all_funding_accounts() { + funds.collect_final_parents(tx, &mut parents); } parents } diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 5c7e4a34d..bf007eb51 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -70,6 +70,16 @@ pub struct ManagedCoreFundsAccount { reservations: ReservationSet, } +/// What [`ManagedCoreFundsAccount::apply_abandon`] removed from one account. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub(crate) struct AbandonRemoval { + /// UTXOs the abandoned transactions had contributed. + pub utxos: usize, + /// Transaction records actually dropped — a txid the account never held + /// removes nothing. + pub records: usize, +} + impl ManagedCoreFundsAccount { /// Create a new managed funds account pub fn new(managed_account_type: ManagedAccountType, network: Network) -> Self { @@ -421,33 +431,42 @@ impl ManagedCoreFundsAccount { /// network — so the correct source of truth is a rescan, which releasing /// them from `spent_outpoints` now permits. /// - /// Returns the number of UTXOs removed. - pub(crate) fn apply_abandon(&mut self, abandoned: &BTreeSet) -> usize { + /// Returns what was actually removed. + pub(crate) fn apply_abandon(&mut self, abandoned: &BTreeSet) -> AbandonRemoval { let doomed: Vec = self .utxos .keys() .filter(|outpoint| abandoned.contains(&outpoint.txid)) .copied() .collect(); - let removed = doomed.len(); + let utxos = doomed.len(); for outpoint in doomed { self.utxos.remove(&outpoint); } + let mut records = 0; for txid in abandoned { if let Some(record) = self.keys.transactions_mut().remove(txid) { - for input in &record.transaction.input { - self.spent_outpoints.remove(&input.previous_output); - } + records += 1; self.reservations .release(record.transaction.input.iter().map(|input| &input.previous_output)); } } - if removed > 0 { + // Re-derive rather than removing each abandoned record's inputs + // individually: an outpoint a surviving transaction also spends must + // stay marked, and only the surviving set can say which those are. + if records > 0 { + self.spent_outpoints = rebuild_spent_outpoints(&self.keys); + } + + if utxos > 0 { self.keys.bump_monitor_revision(); } - removed + AbandonRemoval { + utxos, + records, + } } /// Drop the outputs of any recorded unconfirmed transaction that `tx` @@ -1121,7 +1140,6 @@ impl ManagedAccountTrait for ManagedCoreFundsAccount { /// so its `previous_output` belongs in the derived set. The field is not /// persisted (`#[serde(skip)]`), so both [`Deserialize`] and the test reload /// simulation reconstruct it through here to stay in lockstep. -#[cfg(any(feature = "serde", test))] fn rebuild_spent_outpoints(keys: &ManagedCoreKeysAccount) -> HashSet { keys.transactions() .values() diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index 2fdc998a9..de87527a0 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -3,7 +3,6 @@ use super::ManagedWalletInfo; use crate::account::account_collection::PlatformPaymentAccountKey; use crate::account::ManagedCoreFundsAccount; -use crate::managed_account::managed_account_ref::{ManagedAccountRef, ManagedAccountRefMut}; use crate::managed_account::managed_platform_account::ManagedPlatformAccount; use crate::managed_account::ManagedCoreKeysAccount; use dashcore::{OutPoint, Txid}; @@ -16,12 +15,19 @@ pub struct AbandonOutcome { pub abandoned: BTreeSet, /// How many UTXOs those transactions had contributed. pub utxos_removed: usize, + /// How many transaction records were actually dropped. Distinct from + /// `abandoned.len()`, which counts what was *asked* for. + pub records_removed: usize, } impl AbandonOutcome { /// Whether anything was actually removed. + /// + /// `abandoned` always contains the root, whether or not the wallet held + /// anything for it, so it cannot answer this on its own — a root the + /// wallet never recorded removes nothing. pub fn is_empty(&self) -> bool { - self.abandoned.is_empty() && self.utxos_removed == 0 + self.records_removed == 0 && self.utxos_removed == 0 } } @@ -87,10 +93,8 @@ impl ManagedWalletInfo { // terminates; a spend cycle is impossible anyway. loop { let mut found = BTreeSet::new(); - for account in self.accounts.all_accounts() { - if let ManagedAccountRef::Funds(funds) = account { - funds.collect_spenders_of(&abandoned, &mut found); - } + for funds in self.accounts.all_funding_accounts() { + funds.collect_spenders_of(&abandoned, &mut found); } // Same step over the external view: anything spending an output of // an abandoned transaction is itself abandoned. @@ -107,15 +111,17 @@ impl ManagedWalletInfo { } let mut utxos_removed = 0; - for account in self.accounts.all_accounts_mut() { - if let ManagedAccountRefMut::Funds(funds) = account { - utxos_removed += funds.apply_abandon(&abandoned); - } + let mut records_removed = 0; + for funds in self.accounts.all_funding_accounts_mut() { + let removed = funds.apply_abandon(&abandoned); + utxos_removed += removed.utxos; + records_removed += removed.records; } AbandonOutcome { abandoned, utxos_removed, + records_removed, } } // BIP44 Account Helpers From 15a597f8683ee53acea2de14516515d3dcf9639f Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:21:16 +0300 Subject: [PATCH 07/15] fix(key-wallet): close four gaps in the conflict sweep and the cascade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings, all four reachable: **The sweep skipped the InstantSend transition.** When the winner was already recorded — both spends sitting in the mempool, then an IS lock arrives — the update-in-place branch marks UTXOs and returns without reaching `update_utxos`, so the sweep it carries never ran and the loser's outputs stayed credited. An IS lock settles the inputs exactly as a block does; run the sweep there too. Regression test included, verified to fail without the fix. **The sweep did not cascade.** It dropped the direct loser only, so a further unconfirmed transaction spending the loser's change kept its own outputs — the parent gone, the child still credited, which is the same phantom-balance shape this PR removes. Walk the unconfirmed descendant closure instead; confirmed records are never followed. **`spent_outpoints` kept the loser's inputs.** Deleting the record left them marked, so an input the winner does not spend could not be rediscovered until a restart rebuilt the set. Re-derive from the surviving records, as `apply_abandon` already does — a loser spending A+B against a winner spending only A must leave A marked and free B. **The external cascade could delete settled state.** `external_spends` carries no confirmation state, so a stale mirror row naming a confirmed transaction would have had its record and UTXOs removed. Check finality against the wallet before following a spender, and refuse a settled root outright. Co-Authored-By: Claude Opus 5 --- .../managed_account/managed_account_ref.rs | 9 ++ .../managed_core_funds_account.rs | 49 ++++++++- .../transaction_checking/wallet_checker.rs | 104 ++++++++++++++++++ .../src/wallet/managed_wallet_info/helpers.rs | 32 +++++- 4 files changed, 188 insertions(+), 6 deletions(-) diff --git a/key-wallet/src/managed_account/managed_account_ref.rs b/key-wallet/src/managed_account/managed_account_ref.rs index 172d40327..0af1c240d 100644 --- a/key-wallet/src/managed_account/managed_account_ref.rs +++ b/key-wallet/src/managed_account/managed_account_ref.rs @@ -409,6 +409,15 @@ impl<'a> ManagedAccountRefMut<'a> { /// /// Returns `true` if any UTXO was newly marked. Always returns `false` /// for the [`Keys`](Self::Keys) variant (no UTXOs to mark). + /// Drop the outputs of any recorded unconfirmed transaction that `tx` + /// provably beat to one of its inputs. No-op for the + /// [`Keys`](Self::Keys) variant, which tracks no UTXOs. + pub(crate) fn sweep_conflicts_for(&mut self, tx: &Transaction, context: &TransactionContext) { + if let ManagedAccountRefMut::Funds(a) = self { + a.drop_conflicted_transactions(tx, context); + } + } + pub fn mark_utxos_instant_send(&mut self, txid: &Txid) -> bool { match self { ManagedAccountRefMut::Funds(a) => a.mark_utxos_instant_send(txid), diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index bf007eb51..5ff850d5a 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -502,7 +502,7 @@ impl ManagedCoreFundsAccount { /// funding account and so the same change account — but not every one. /// /// Returns whether any UTXO was removed. - fn drop_conflicted_transactions( + pub(crate) fn drop_conflicted_transactions( &mut self, tx: &Transaction, context: &TransactionContext, @@ -517,7 +517,7 @@ impl ManagedCoreFundsAccount { // A finalized transaction keeps only its txid, so a chainlocked record // can never be a loser here — and must not be, since it is settled. - let losers: Vec = self + let mut losers: BTreeSet = self .keys .transactions() .iter() @@ -533,21 +533,60 @@ impl ManagedCoreFundsAccount { .map(|(txid, _)| *txid) .collect(); + if losers.is_empty() { + return false; + } + + // A loser's change may already have funded further unconfirmed + // transactions. Those can never exist either — their parent cannot — + // so leaving their outputs credited would preserve the very + // phantom-balance class this sweep exists to remove. Walk the + // unconfirmed descendant closure; confirmed records are never + // followed, since a transaction in a block spent something real. + loop { + let mut found = BTreeSet::new(); + for (txid, record) in self.keys.transactions() { + if record.is_confirmed() || losers.contains(txid) || *txid == winner { + continue; + } + if record + .transaction + .input + .iter() + .any(|input| losers.contains(&input.previous_output.txid)) + { + found.insert(*txid); + } + } + let before = losers.len(); + losers.extend(found); + if losers.len() == before { + break; + } + } + let mut changed = false; - for loser in losers { + for loser in &losers { let removed: Vec = - self.utxos.keys().filter(|outpoint| outpoint.txid == loser).copied().collect(); + self.utxos.keys().filter(|outpoint| outpoint.txid == *loser).copied().collect(); for outpoint in removed { self.utxos.remove(&outpoint); changed = true; } - self.keys.transactions_mut().remove(&loser); + self.keys.transactions_mut().remove(loser); tracing::info!( conflicted_txid = %loser, winning_txid = %winner, "Dropped a conflicted transaction: its input was spent by a final transaction" ); } + + // Re-derive rather than removing each loser's inputs individually: a + // loser spending A+B against a winner that spends only A must leave A + // marked (the winner still spends it) and free B, and only the + // surviving records can draw that line. + self.spent_outpoints = rebuild_spent_outpoints(&self.keys); + changed } diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 8816f4aca..9e7a9b21e 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -148,6 +148,14 @@ impl WalletTransactionChecker for ManagedWalletInfo { }; if account.transactions().contains_key(&txid) { account.mark_utxos_instant_send(&txid); + // An IS lock is final, so it settles this transaction's + // inputs just as a block would. A competing spend + // recorded earlier in the mempool can now never + // confirm, and this sweep is the only thing that drops + // its outputs — the `record_transaction` path below + // runs it via `update_utxos`, but a transaction we + // already hold never reaches that path. + account.sweep_conflicts_for(tx, &context); if let Some(record) = account.transactions_mut().get_mut(&txid) { record.update_context(context.clone()); result.updated_records.push(record.clone()); @@ -2052,6 +2060,102 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// An InstantSend lock is final, so it settles the winner's inputs just as + /// a block would — including when the winner was already sitting in the + /// mempool alongside its loser, which is the transition that skips + /// `record_transaction` and so skips the sweep it carries. + #[tokio::test] + async fn test_instant_send_on_an_existing_mempool_tx_drops_its_conflict() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + let funding_outpoint = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let spend_of = |change: &Address, change_amount: u64, sent: u64| Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + + // Both competing spends land in the mempool, loser first. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend_of(&loser_change, 399_000, 600_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + let winner_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let winner = spend_of(&winner_change, 299_000, 700_000); + ctx.check_transaction(&winner, TransactionContext::Mempool).await; + + let loser_change_outpoint = OutPoint { + txid: loser.txid(), + vout: 1, + }; + assert!( + ctx.bip44_account().utxos.contains_key(&loser_change_outpoint), + "both are live while neither is final" + ); + + // The winner is InstantSend-locked. It is already recorded, so this + // takes the update-in-place branch rather than recording afresh. + let is_lock = InstantLock { + txid: winner.txid(), + ..InstantLock::default() + }; + ctx.check_transaction(&winner, TransactionContext::InstantSend(is_lock)).await; + + assert!( + !ctx.bip44_account().utxos.contains_key(&loser_change_outpoint), + "an IS lock settles the input, so the loser's change must not survive" + ); + assert!( + !ctx.bip44_account().transactions().contains_key(&loser.txid()), + "the loser's record must be dropped too" + ); + } + /// Abandoning a transaction that never reached the network must take the /// transactions built on its change with it. This reproduces the shape /// seen on a testnet device: an asset-lock funding transaction stuck at diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index de87527a0..ebf02066a 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -3,6 +3,7 @@ use super::ManagedWalletInfo; use crate::account::account_collection::PlatformPaymentAccountKey; use crate::account::ManagedCoreFundsAccount; +use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::managed_platform_account::ManagedPlatformAccount; use crate::managed_account::ManagedCoreKeysAccount; use dashcore::{OutPoint, Txid}; @@ -32,6 +33,17 @@ impl AbandonOutcome { } impl ManagedWalletInfo { + /// Whether any account holds `txid` as confirmed or chainlock-finalized. + /// + /// A finalized transaction may keep only its txid, so both the retained + /// set and the live record have to be consulted. + fn transaction_is_settled(&self, txid: &Txid) -> bool { + self.accounts.all_funding_accounts().into_iter().any(|funds| { + funds.transaction_is_finalized(txid) + || funds.transactions().get(txid).is_some_and(|r| r.is_confirmed()) + }) + } + /// Abandon `root` and every recorded transaction descending from it. /// /// A transaction the network never accepted still mutated this wallet: @@ -81,11 +93,29 @@ impl ManagedWalletInfo { /// that one abandoned output funded the next transaction along. Callers /// with a persistence mirror can supply `external_spends`, mapping an /// outpoint to the transaction that spent it, and the walk follows both. + /// + /// An external spender the wallet holds as confirmed or finalized is + /// **not** followed: the mirror carries no confirmation state of its own, + /// so without this check a stale row could name a settled transaction and + /// have its record and UTXOs deleted. The same guard rejects a confirmed + /// root outright — a transaction in a block spent something real, and + /// nothing built on it is fiction. pub fn abandon_transaction_with_spends( &mut self, root: Txid, external_spends: &BTreeMap, ) -> AbandonOutcome { + if self.transaction_is_settled(&root) { + tracing::warn!( + txid = %root, + "refusing to abandon a transaction the wallet holds as settled" + ); + return AbandonOutcome { + abandoned: BTreeSet::new(), + utxos_removed: 0, + records_removed: 0, + }; + } let mut abandoned = BTreeSet::from([root]); // Transitive closure over recorded spenders. Each pass can only add @@ -99,7 +129,7 @@ impl ManagedWalletInfo { // Same step over the external view: anything spending an output of // an abandoned transaction is itself abandoned. for (outpoint, spender) in external_spends { - if abandoned.contains(&outpoint.txid) { + if abandoned.contains(&outpoint.txid) && !self.transaction_is_settled(spender) { found.insert(*spender); } } From ba0ad6f949ded5ced8370b74de2017220db8d002 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:51:53 +0300 Subject: [PATCH 08/15] fix(key-wallet): three deeper review findings on the sweep and abandon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A loser arriving after its winner re-created the phantom, spendably.** The sweep only fires when the *arriving* transaction is final, and nothing checked an arriving transaction's inputs at all — the two guards in `update_utxos` test its own output outpoints. So the reverse order (winner confirms, loser delivered afterwards as mempool) credited the loser's outputs with nothing left to remove them, and `is_spendable` gates only on `is_locked` and maturity, so coin selection could spend them. Refuse to credit a non-final transaction whose input a block has already spent. The record still stands, so history keeps the attempt. **A swept loser's non-overlapping input silently vanished.** The doc claimed reverting only the outputs needs no recovery, which holds only when the winner spends every input the loser did. It often does not: a loser spending A+B against a winner spending only A leaves B freed from `spent_outpoints` but with no `Utxo` — discarded when the loser was recorded, and `InputDetail` cannot rebuild it. The release is what makes B recoverable by a rescan; the doc now says so, including that a chainlock-finalized funding record needs a deeper rescan. Regression test covers the round trip. **`apply_abandon` used the release `ReservationSet` forbids here.** Its doc reserves the unconditional form for coins *known spent*, and requires `release_if_owner` from a caller abandoning an in-flight build — exactly this one — so it cannot free a reservation a newer build has taken over. There is nothing of this build's left to release anyway: recording the transaction already handed its inputs to `spent_outpoints`. Drop the call. Co-Authored-By: Claude Opus 5 --- .../managed_core_funds_account.rs | 56 +++++++++-- .../transaction_checking/wallet_checker.rs | 99 +++++++++++++++++++ 2 files changed, 146 insertions(+), 9 deletions(-) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 5ff850d5a..019218e4e 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -284,6 +284,30 @@ impl ManagedCoreFundsAccount { let txid = tx.txid(); let mut utxos_changed = false; + // A transaction whose input a *block* already spent can never + // confirm — unless this arrival is that block delivery itself. + // The conflict sweep below only fires when the arriving + // transaction is final, so without this the reverse order + // (winner confirms, loser arrives afterwards as mempool) + // credits the loser's outputs with nothing left to remove + // them, and `is_spendable` would hand them to coin selection. + let doomed_by_a_settled_spend = !context.confirmed() + && !matches!(context, TransactionContext::InstantSend(_)) + && tx + .input + .iter() + .any(|input| observed_spent.contains_key(&input.previous_output)); + if doomed_by_a_settled_spend { + // Deliberately before any mutation: the record built by + // the caller stands, so history still shows the attempt, + // but nothing it created enters the UTXO set. + tracing::info!( + %txid, + "Not crediting a transaction whose input a block already spent" + ); + return; + } + let network = self.keys.network(); // Insert UTXOs for outputs paying to our addresses @@ -431,6 +455,14 @@ impl ManagedCoreFundsAccount { /// network — so the correct source of truth is a rescan, which releasing /// them from `spent_outpoints` now permits. /// + /// Reservations are deliberately left alone. A recorded transaction has + /// already handed its inputs from the ephemeral set to `spent_outpoints` + /// (see `update_utxos`), so there is nothing of this build's left to + /// release — while an unconditional release here could free a reservation + /// a *newer* build has since taken over the same outpoints, which + /// `ReservationSet::release` documents as forbidden for exactly this + /// caller shape. + /// /// Returns what was actually removed. pub(crate) fn apply_abandon(&mut self, abandoned: &BTreeSet) -> AbandonRemoval { let doomed: Vec = self @@ -446,10 +478,8 @@ impl ManagedCoreFundsAccount { let mut records = 0; for txid in abandoned { - if let Some(record) = self.keys.transactions_mut().remove(txid) { + if self.keys.transactions_mut().remove(txid).is_some() { records += 1; - self.reservations - .release(record.transaction.input.iter().map(|input| &input.previous_output)); } } @@ -488,12 +518,20 @@ impl ManagedCoreFundsAccount { /// mempool, and un-applying it would re-expose its inputs to coin /// selection and invite a double-spend. /// - /// Only the loser's *outputs* are reverted, which needs no recovery of - /// discarded state: its inputs are correctly accounted for by `tx`, the - /// transaction that actually spent them. Reverting a transaction whose - /// fate is unknown would additionally require restoring the spent parents, - /// and the `Utxo` values removed for them — with their flags — are not - /// retained anywhere. + /// Only the loser's *outputs* are reverted. Where the winner spends every + /// input the loser did — the ordinary resend — that is complete: those + /// inputs are correctly accounted for by `tx`, the transaction that + /// actually spent them. + /// + /// A loser may also spend inputs the winner does not. Those coins are + /// freed from `spent_outpoints` below, but they cannot be re-credited + /// here: `update_utxos` discarded their `Utxo` — and its flags — when the + /// loser was recorded, and `InputDetail` keeps only index/value/address. + /// The release is what makes them recoverable: a rescan re-delivering the + /// funding transaction inserts them again. Until that rescan they are + /// absent from the balance, and if the funding transaction was itself + /// chainlock-finalized its record may already have been dropped, in which + /// case recovery needs a rescan deep enough to re-fetch the block. /// /// Scope: account-local. A loser recorded here has its outputs dropped /// here; a loser whose change landed in a *different* account is not diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 9e7a9b21e..8d0c7eab2 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2060,6 +2060,105 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// A loser can spend inputs the winner does not. Sweeping it frees those + /// coins from the spent set, but their `Utxo` values were discarded when + /// the loser was recorded — so the sweep alone cannot put them back, and + /// the coins must be recoverable by the rescan the release enables. + #[tokio::test] + async fn test_a_swept_losers_extra_input_is_recoverable_by_rescan() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // One funding transaction pays us twice: A and B. + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..2, &[500_000, 400_000]); + let funding_context = TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )); + ctx.check_transaction(&funding_tx, funding_context.clone()).await; + assert_eq!(ctx.managed_wallet.balance.confirmed(), 900_000); + + let coin_a = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let coin_b = OutPoint { + txid: funding_tx.txid(), + vout: 1, + }; + let spend = + |inputs: Vec, change: &Address, change_amount: u64, sent: u64| Transaction { + version: 2, + lock_time: 0, + input: inputs + .into_iter() + .map(|previous_output| TxIn { + previous_output, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }) + .collect(), + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + + // The loser spends A and B; the winner spends only A, and confirms. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend(vec![coin_a, coin_b], &loser_change, 99_000, 800_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + let winner_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let winner = spend(vec![coin_a], &winner_change, 99_000, 400_000); + ctx.check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[2u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + + // The loser is gone, and B is not credited — its `Utxo` was + // discarded when the loser was recorded and cannot be invented. + assert!(!ctx.bip44_account().transactions().contains_key(&loser.txid())); + assert!(!ctx.bip44_account().utxos.contains_key(&coin_b)); + + // But B was freed from the spent set, so re-delivering the funding + // block restores it. That is what makes the loss recoverable rather + // than permanent. + ctx.check_transaction(&funding_tx, funding_context).await; + assert!( + ctx.bip44_account().utxos.contains_key(&coin_b), + "a rescan must be able to rediscover the loser's extra input" + ); + assert_eq!(ctx.managed_wallet.balance.confirmed(), 499_000, "B plus the winner's change"); + } + /// An InstantSend lock is final, so it settles the winner's inputs just as /// a block would — including when the winner was already sitting in the /// mempool alongside its loser, which is the transition that skips From d8c99117fa84d4d1b3fe97e0cbcc2c48ae1ba268 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:03:25 +0300 Subject: [PATCH 09/15] fix(key-wallet): restore the doc block, and make three assertions load-bearing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **#15** `sweep_conflicts_for` was inserted between `mark_utxos_instant_send`'s doc block and its signature, so it rendered with that method's docs while `mark_utxos_instant_send` was left with none. Moved back. Two stale claims alongside it. The phantom outputs do not "sit in the `unconfirmed` bucket" — the trusted-self-send rule this PR widens files them as *confirmed* and therefore spendable, which understated the bug. And `rebuild_spent_outpoints` still claimed only `Deserialize` and the test reload reach it, the assumption that made its records-only derivation look safe now that there are runtime callers. **#14** Both new balance assertions were vacuous: trusted self-send change is bucketed as confirmed, so `unconfirmed() == 0` held before the cascade as well as after, and deleting the cascade left them passing. Assert the confirmed total instead — verified to fail with the removal neutered. Also every transaction in these tests had exactly one wallet-owned output, so the per-txid removal loops were never exercised against more than one and a first-only filter would have passed. The cascade's tip now pays us twice; confirmed that a `seen`-guarded first-only filter fails the test. Co-Authored-By: Claude Opus 5 --- .../managed_account/managed_account_ref.rs | 8 +-- .../managed_core_funds_account.rs | 14 +++- .../transaction_checking/wallet_checker.rs | 65 +++++++++++++++---- 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/key-wallet/src/managed_account/managed_account_ref.rs b/key-wallet/src/managed_account/managed_account_ref.rs index 0af1c240d..0bcf81690 100644 --- a/key-wallet/src/managed_account/managed_account_ref.rs +++ b/key-wallet/src/managed_account/managed_account_ref.rs @@ -405,10 +405,6 @@ impl<'a> ManagedAccountRefMut<'a> { } } - /// Mark all UTXOs belonging to `txid` as InstantSend-locked. - /// - /// Returns `true` if any UTXO was newly marked. Always returns `false` - /// for the [`Keys`](Self::Keys) variant (no UTXOs to mark). /// Drop the outputs of any recorded unconfirmed transaction that `tx` /// provably beat to one of its inputs. No-op for the /// [`Keys`](Self::Keys) variant, which tracks no UTXOs. @@ -418,6 +414,10 @@ impl<'a> ManagedAccountRefMut<'a> { } } + /// Mark all UTXOs belonging to `txid` as InstantSend-locked. + /// + /// Returns `true` if any UTXO was newly marked. Always returns `false` + /// for the [`Keys`](Self::Keys) variant (no UTXOs to mark). pub fn mark_utxos_instant_send(&mut self, txid: &Txid) -> bool { match self { ManagedAccountRefMut::Funds(a) => a.mark_utxos_instant_send(txid), diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 019218e4e..a423a88f2 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -509,7 +509,9 @@ impl ManagedCoreFundsAccount { /// money that does not exist. Nothing else removes them: the loser is not /// in a block, so no block processing revisits it, and mempool expiry in /// dash-spv only drops its own tracking without telling the wallet. Left - /// alone they sit in the `unconfirmed` bucket permanently. + /// alone they are counted permanently — and as *confirmed*, not merely + /// unconfirmed, whenever the trusted-self-send rule applies to them, + /// which also makes them selectable by coin selection. /// /// This is deliberately narrow. It fires only on proof — a conflicting /// spend that is itself final — never on a timeout: the p2p network has no @@ -1215,8 +1217,14 @@ impl ManagedAccountTrait for ManagedCoreFundsAccount { /// /// Every input of every recorded transaction is a spend this account has seen, /// so its `previous_output` belongs in the derived set. The field is not -/// persisted (`#[serde(skip)]`), so both [`Deserialize`] and the test reload -/// simulation reconstruct it through here to stay in lockstep. +/// serialized (`#[serde(skip_serializing)]`), so [`Deserialize`] and the test +/// reload simulation reconstruct it through here to stay in lockstep. +/// +/// **Derives only from live records.** Under the default +/// `keep-finalized-transactions = off`, a chainlocked record is dropped to +/// just its txid, so its inputs survive only as entries already in the set — +/// which a wholesale rebuild would discard. Callers pruning a subset of +/// records must retain the rest rather than reassigning from this. fn rebuild_spent_outpoints(keys: &ManagedCoreKeysAccount) -> HashSet { keys.transactions() .values() diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 8d0c7eab2..ea9a925c5 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2291,7 +2291,7 @@ mod tests { }; let mut change_left = funding_value; let mut chain = Vec::new(); - for sent in [40_000_000u64, 20_000_000, 5_000_000] { + for (link, sent) in [40_000_000u64, 20_000_000, 5_000_000].into_iter().enumerate() { let fee = 226u64; let change = change_left - sent - fee; let change_address = ctx @@ -2300,6 +2300,39 @@ mod tests { .expect("account") .next_change_address(Some(&ctx.xpub), true) .expect("change address"); + // The tip pays us twice, and nothing spends it onward, so both + // outputs are still live when the cascade runs — exercising the + // per-txid removal loops against a transaction contributing more + // than one UTXO. A filter that dropped only the first would + // otherwise pass every test here. + let split = link == 2; + let (change_a, change_b) = if split { + (change / 2, change - change / 2) + } else { + (change, 0) + }; + let mut outputs = vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_a, + script_pubkey: change_address.script_pubkey(), + }, + ]; + if split { + let second = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + outputs.push(TxOut { + value: change_b, + script_pubkey: second.script_pubkey(), + }); + } let tx = Transaction { version: 2, lock_time: 0, @@ -2309,16 +2342,7 @@ mod tests { sequence: 0xffffffff, witness: dashcore::Witness::new(), }], - output: vec![ - TxOut { - value: sent, - script_pubkey: external_address.script_pubkey(), - }, - TxOut { - value: change, - script_pubkey: change_address.script_pubkey(), - }, - ], + output: outputs, special_transaction_payload: None, }; ctx.check_transaction(&tx, TransactionContext::Mempool).await; @@ -2331,8 +2355,10 @@ mod tests { } let root = chain[0].txid(); - // Only the tip's change is tracked — each link consumed its parent's. - assert_eq!(ctx.bip44_account().utxos.len(), 1, "one live change output"); + // The tip's change, plus the first link's second output — that one is + // never spent onward, so the cascade has to drop two UTXOs for one of + // the txids rather than assuming one each. + assert_eq!(ctx.bip44_account().utxos.len(), 2, "live change outputs"); let outcome = ctx.managed_wallet.abandon_transaction(root); ctx.managed_wallet.update_balance(); @@ -2356,6 +2382,14 @@ mod tests { ); } assert!(ctx.bip44_account().utxos.is_empty(), "no phantom output may survive the cascade"); + // The load-bearing assertion. Trusted self-send change is bucketed as + // *confirmed*, so `unconfirmed() == 0` holds before the abandon too + // and proves nothing on its own. + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 0, + "the phantom counts as confirmed, so that is where its absence must show" + ); assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); // The real coin the chain consumed is released from the spent set, so @@ -2482,8 +2516,11 @@ mod tests { vout: 1, }; assert!(ctx.bip44_account().utxos.contains_key(&winner_change)); - assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); + // Without the sweep this is 698_000 — the loser's change is trusted + // self-send change and lands in the confirmed bucket, so only the + // exact total catches a regression. assert_eq!(ctx.managed_wallet.balance.confirmed(), 299_000); + assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); } /// Pooled funding spans account families — an asset lock draws from BIP44, From 9944d6e875373f008e34297ca89807446fe9229b Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:23:20 +0300 Subject: [PATCH 10/15] fix(key-wallet): InstantSend finality, wallet-wide abandon, targeted spent-marks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **#6 — abandon could delete network-settled money.** `transaction_is_settled` and the cascade walk both gated on `is_confirmed()`, which excludes `InstantSend`. An IS lock is final against a double spend under DIP-10, so an IS-locked root passed the guard and IS-locked descendants were followed; their inputs were then released and a rescan could re-credit coins the network has irreversibly moved. Both now treat a lock as settled. **#5 — a plain block deleted an IS-locked record.** The sweep's filter admitted any `!is_confirmed()` record, and its gate accepts a non-chainlocked `InBlock`, so an ordinary tip delivery unrecoverably dropped a record the network had locked — with no reorg recovery anywhere in the stack. Precedence is now explicit: a chainlock overrides anything, and an IS-locked loser may only be evicted by a chainlocked arrival. **#4 — the walk was not wallet-wide despite saying so.** It iterated funding accounts only, skipping keys-only accounts. An asset-lock funding transaction is recorded in *both* its funding account and the identity account it pays, so abandoning it left the keys-account copy behind — which makes `is_new` false on re-sighting, so the funds account never re-records it and never re-marks its input spent. The walk and the removal now span every account that holds records. **#7 — the rebuild erased chainlock-pruned marks.** This PR dropped the `cfg` gate on `rebuild_spent_outpoints` and added runtime callers, but it derives only from live records — and under the default `keep-finalized-transactions = off` a chainlocked spend keeps only its txid, so its inputs live solely as marks already in the set. A wholesale reassignment discarded them, letting a later backfill re-credit coins spent on chain. Replaced with a targeted retain over only the outpoints the removed records contributed. Co-Authored-By: Claude Opus 5 --- .../managed_core_funds_account.rs | 79 +++++++++--------- .../src/wallet/managed_wallet_info/helpers.rs | 83 ++++++++++++++++--- 2 files changed, 110 insertions(+), 52 deletions(-) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index a423a88f2..ea9e20fac 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -411,33 +411,25 @@ impl ManagedCoreFundsAccount { } } - /// Collect the txids of recorded unconfirmed transactions that spend an - /// output of any transaction in `abandoned`. + /// Drop the spent-marks that `freed` contributed, keeping every mark a + /// surviving record still claims. /// - /// One step of the descendant walk driven by - /// [`ManagedWalletInfo::abandon_transaction`]. A confirmed or finalized - /// transaction is never a descendant for this purpose: it is settled on - /// chain, so whatever it spent was real. - /// - /// [`ManagedWalletInfo::abandon_transaction`]: crate::wallet::managed_wallet_info::ManagedWalletInfo::abandon_transaction - pub(crate) fn collect_spenders_of( - &self, - abandoned: &BTreeSet, - into: &mut BTreeSet, - ) { - for (txid, record) in self.keys.transactions() { - if record.is_confirmed() || abandoned.contains(txid) { - continue; - } - let spends_abandoned = record - .transaction - .input - .iter() - .any(|input| abandoned.contains(&input.previous_output.txid)); - if spends_abandoned { - into.insert(*txid); - } + /// Deliberately *not* a wholesale rebuild from the live records. Under the + /// default `keep-finalized-transactions = off` a chainlocked spend's + /// record is reduced to its txid, so its inputs survive only as marks + /// already in this set — reassigning from the record map would silently + /// drop them and let a later backfill re-credit coins that are spent on + /// chain. Only outpoints the removed records actually contributed are + /// considered, and a removed record's input stays marked when a survivor + /// spends it too (a loser spending A+B against a winner spending only A + /// must leave A marked and free B). + fn release_spent_marks(&mut self, freed: &HashSet) { + if freed.is_empty() { + return; } + let still_spent = rebuild_spent_outpoints(&self.keys); + self.spent_outpoints + .retain(|outpoint| !freed.contains(outpoint) || still_spent.contains(outpoint)); } /// Remove every trace of `abandoned` from this account. @@ -477,17 +469,15 @@ impl ManagedCoreFundsAccount { } let mut records = 0; + let mut freed: HashSet = HashSet::new(); for txid in abandoned { - if self.keys.transactions_mut().remove(txid).is_some() { + if let Some(record) = self.keys.transactions_mut().remove(txid) { records += 1; + freed.extend(record.transaction.input.iter().map(|input| input.previous_output)); } } - - // Re-derive rather than removing each abandoned record's inputs - // individually: an outpoint a surviving transaction also spends must - // stay marked, and only the surviving set can say which those are. if records > 0 { - self.spent_outpoints = rebuild_spent_outpoints(&self.keys); + self.release_spent_marks(&freed); } if utxos > 0 { @@ -562,8 +552,17 @@ impl ManagedCoreFundsAccount { .transactions() .iter() .filter(|(txid, record)| { + // Precedence, per DIP-10: a chainlock is final over + // everything, an InstantSend lock is final against a double + // spend, and a plain block is provisional until its own + // chainlock lands. So an IS-locked record may only be evicted + // by a chainlocked arrival — a plain `InBlock` winner cannot + // overrule a lock the network already signed, and the block + // it arrived in can still reorg away. + let loser_is_locked = record.context.is_instant_send(); **txid != winner && !record.is_confirmed() + && (!loser_is_locked || context.is_chain_locked()) && record .transaction .input @@ -586,7 +585,11 @@ impl ManagedCoreFundsAccount { loop { let mut found = BTreeSet::new(); for (txid, record) in self.keys.transactions() { - if record.is_confirmed() || losers.contains(txid) || *txid == winner { + if record.is_confirmed() + || record.context.is_instant_send() + || losers.contains(txid) + || *txid == winner + { continue; } if record @@ -606,6 +609,7 @@ impl ManagedCoreFundsAccount { } let mut changed = false; + let mut freed: HashSet = HashSet::new(); for loser in &losers { let removed: Vec = self.utxos.keys().filter(|outpoint| outpoint.txid == *loser).copied().collect(); @@ -613,19 +617,16 @@ impl ManagedCoreFundsAccount { self.utxos.remove(&outpoint); changed = true; } - self.keys.transactions_mut().remove(loser); + if let Some(record) = self.keys.transactions_mut().remove(loser) { + freed.extend(record.transaction.input.iter().map(|input| input.previous_output)); + } tracing::info!( conflicted_txid = %loser, winning_txid = %winner, "Dropped a conflicted transaction: its input was spent by a final transaction" ); } - - // Re-derive rather than removing each loser's inputs individually: a - // loser spending A+B against a winner that spends only A must leave A - // marked (the winner still spends it) and free B, and only the - // surviving records can draw that line. - self.spent_outpoints = rebuild_spent_outpoints(&self.keys); + self.release_spent_marks(&freed); changed } diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index ebf02066a..ef9c01b7b 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -3,6 +3,8 @@ use super::ManagedWalletInfo; use crate::account::account_collection::PlatformPaymentAccountKey; use crate::account::ManagedCoreFundsAccount; +use crate::account::TransactionRecord; +use crate::managed_account::managed_account_ref::ManagedAccountRefMut; use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::managed_platform_account::ManagedPlatformAccount; use crate::managed_account::ManagedCoreKeysAccount; @@ -32,15 +34,50 @@ impl AbandonOutcome { } } +/// Txids in `records` that spend an output of anything in `abandoned`. +/// +/// Settled records are never followed — what they spent was real. An +/// InstantSend lock settles a transaction against a double spend just as a +/// block does, and `is_confirmed()` does not cover it. +fn collect_spenders_of_records( + records: &std::collections::BTreeMap, + abandoned: &BTreeSet, + into: &mut BTreeSet, +) { + for (txid, record) in records { + if record.is_confirmed() || record.context.is_instant_send() || abandoned.contains(txid) { + continue; + } + if record + .transaction + .input + .iter() + .any(|input| abandoned.contains(&input.previous_output.txid)) + { + into.insert(*txid); + } + } +} + impl ManagedWalletInfo { - /// Whether any account holds `txid` as confirmed or chainlock-finalized. + /// Whether any account holds `txid` as settled by the network. + /// + /// Settled means chainlock-finalized, in a block, **or InstantSend-locked** + /// — an IS lock is final against a double spend under DIP-10, so the coins + /// it moved are as irreversibly gone as a block's. `is_confirmed()` covers + /// only the first two, which is why the lock is checked explicitly. /// /// A finalized transaction may keep only its txid, so both the retained - /// set and the live record have to be consulted. + /// set and the live record have to be consulted. Keys-only accounts are + /// included: they hold records too, and a settled record there is just as + /// authoritative. fn transaction_is_settled(&self, txid: &Txid) -> bool { - self.accounts.all_funding_accounts().into_iter().any(|funds| { - funds.transaction_is_finalized(txid) - || funds.transactions().get(txid).is_some_and(|r| r.is_confirmed()) + self.accounts.all_accounts().into_iter().any(|account| { + account.transaction_is_finalized(txid) + || account + .transactions() + .get(txid) + .is_some_and(|r| r.is_confirmed() || r.context.is_instant_send()) }) } @@ -54,9 +91,12 @@ impl ManagedWalletInfo { /// sits in the `unconfirmed` bucket permanently, as money the wallet /// displays and does not have. /// - /// The walk is transitive and wallet-wide: pooled funding spreads a + /// The walk is transitive and wallet-wide — every account that holds + /// records, funds-bearing or keys-only. Pooled funding spreads a /// transaction's inputs across account families, so a descendant's change - /// can land in an account that holds none of the root. Confirmed and + /// can land in an account holding none of the root; and an asset-lock + /// funding transaction is recorded in both its funding account and the + /// identity account it pays. Confirmed and /// finalized transactions are never followed — they are settled on chain, /// so whatever they spent was real. /// @@ -123,8 +163,8 @@ impl ManagedWalletInfo { // terminates; a spend cycle is impossible anyway. loop { let mut found = BTreeSet::new(); - for funds in self.accounts.all_funding_accounts() { - funds.collect_spenders_of(&abandoned, &mut found); + for account in self.accounts.all_accounts() { + collect_spenders_of_records(account.transactions(), &abandoned, &mut found); } // Same step over the external view: anything spending an output of // an abandoned transaction is itself abandoned. @@ -142,10 +182,27 @@ impl ManagedWalletInfo { let mut utxos_removed = 0; let mut records_removed = 0; - for funds in self.accounts.all_funding_accounts_mut() { - let removed = funds.apply_abandon(&abandoned); - utxos_removed += removed.utxos; - records_removed += removed.records; + for account in self.accounts.all_accounts_mut() { + match account { + ManagedAccountRefMut::Funds(funds) => { + let removed = funds.apply_abandon(&abandoned); + utxos_removed += removed.utxos; + records_removed += removed.records; + } + // Keys-only accounts hold no UTXOs, but they do hold records + // — an asset-lock funding transaction is recorded in both its + // funding account and the identity account it pays. Leaving + // the record here makes `is_new` false on a later re-sighting, + // so the funds account never re-records the transaction and + // never re-marks its input spent. + ManagedAccountRefMut::Keys(keys) => { + for txid in &abandoned { + if keys.transactions_mut().remove(txid).is_some() { + records_removed += 1; + } + } + } + } } AbandonOutcome { From 127ed34e2084ab8c677993638b391592d945b1b4 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:34:58 +0300 Subject: [PATCH 11/15] fix(key-wallet): make the conflict sweep wallet-wide and ungated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, one design gap: the sweep was attached to *account* processing, but "a competing spend just became provably dead" is a wallet-wide fact the moment any account observes a final spend. **#8 — the sweep never ran when the winner looked irrelevant.** `check_core_transaction` returns before touching any account, and relevance is computed from matching outputs and from inputs still in `utxos` — but a recorded loser already removed the shared input. So a winner that spends our coin and pays only external addresses matches nothing, and the loser stayed credited. Worse, as trusted self-send change it counts as *confirmed* and is spendable. The sweep now runs before that gate, next to `record_observed_spends`, which is unconditional for the same reason. **#13 — a loser in a sibling account survived.** The per-account sweep only visited `result.affected_accounts`, i.e. the winner's. Pooled funding routinely puts the loser's change elsewhere — the shape this PR's own commits call normal for asset locks. `ManagedWalletInfo:: sweep_conflicts` now asks every funds account. **#9 — the IS-lock sweep was dead code.** The live pipeline reaches `process_instant_send_lock` → `mark_instant_send_utxos`, which marks UTXOs and rewrites context and had no sweep at all; the branch I had added in `check_core_transaction` is only reachable on a first sighting that already carries the lock. The sweep now hangs off `mark_instant_send_utxos`, and the superseded per-account entry point is gone. Both new tests were confirmed to fail with the wallet-level sweep disabled. Co-Authored-By: Claude Opus 5 --- .../managed_account/managed_account_ref.rs | 9 - .../transaction_checking/wallet_checker.rs | 226 +++++++++++++++++- .../src/wallet/managed_wallet_info/helpers.rs | 35 ++- .../wallet_info_interface.rs | 19 +- 4 files changed, 269 insertions(+), 20 deletions(-) diff --git a/key-wallet/src/managed_account/managed_account_ref.rs b/key-wallet/src/managed_account/managed_account_ref.rs index 0bcf81690..172d40327 100644 --- a/key-wallet/src/managed_account/managed_account_ref.rs +++ b/key-wallet/src/managed_account/managed_account_ref.rs @@ -405,15 +405,6 @@ impl<'a> ManagedAccountRefMut<'a> { } } - /// Drop the outputs of any recorded unconfirmed transaction that `tx` - /// provably beat to one of its inputs. No-op for the - /// [`Keys`](Self::Keys) variant, which tracks no UTXOs. - pub(crate) fn sweep_conflicts_for(&mut self, tx: &Transaction, context: &TransactionContext) { - if let ManagedAccountRefMut::Funds(a) = self { - a.drop_conflicted_transactions(tx, context); - } - } - /// Mark all UTXOs belonging to `txid` as InstantSend-locked. /// /// Returns `true` if any UTXO was newly marked. Always returns `false` diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index ea9a925c5..8cefcd198 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -81,6 +81,21 @@ impl WalletTransactionChecker for ManagedWalletInfo { } } + // A final arrival settles its inputs whether or not this transaction + // looks relevant to us. Relevance is computed from matching outputs + // and from inputs still present in `utxos` — but a recorded loser + // already removed the shared input, so a winner that spends our coin + // and pays only external addresses matches nothing and would return + // below with the loser still credited. Sweep first, wallet-wide, next + // to `record_observed_spends` above for the same reason it is + // unconditional. + if update_state + && (context.confirmed() || context.is_instant_send()) + && self.sweep_conflicts(tx, &context) + { + result.state_modified = true; + } + if !update_state || !result.is_relevant { return result; } @@ -148,14 +163,6 @@ impl WalletTransactionChecker for ManagedWalletInfo { }; if account.transactions().contains_key(&txid) { account.mark_utxos_instant_send(&txid); - // An IS lock is final, so it settles this transaction's - // inputs just as a block would. A competing spend - // recorded earlier in the mempool can now never - // confirm, and this sweep is the only thing that drops - // its outputs — the `record_transaction` path below - // runs it via `update_utxos`, but a transaction we - // already hold never reaches that path. - account.sweep_conflicts_for(tx, &context); if let Some(record) = account.transactions_mut().get_mut(&txid) { record.update_context(context.clone()); result.updated_records.push(record.clone()); @@ -2060,6 +2067,209 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// A winner that spends our coin but pays only external addresses matches + /// nothing: its outputs are not ours, and the input it shares with the + /// loser was already removed from `utxos` when the loser was recorded. It + /// is therefore classified irrelevant — and the sweep still has to run, + /// or the loser's change stays credited with nothing left to clear it. + #[tokio::test] + async fn test_an_irrelevant_winner_still_sweeps_its_loser() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + let funding_outpoint = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let input = || TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }; + + // The loser pays us change, so it is relevant and gets recorded. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = Transaction { + version: 2, + lock_time: 0, + input: vec![input()], + output: vec![ + TxOut { + value: 600_000, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: 399_000, + script_pubkey: loser_change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 399_000, + "trusted self-send change counts as confirmed" + ); + + // The winner spends the same coin and pays only outside the wallet. + let winner = Transaction { + version: 2, + lock_time: 0, + input: vec![input()], + output: vec![TxOut { + value: 999_000, + script_pubkey: external_address.script_pubkey(), + }], + special_transaction_payload: None, + }; + let result = ctx + .check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[2u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + assert!( + !result.is_relevant, + "the precondition: nothing about this winner matches the wallet" + ); + + assert!( + !ctx.bip44_account().transactions().contains_key(&loser.txid()), + "the loser must be swept even though the winner is irrelevant" + ); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 0, + "and its change must stop counting as confirmed money" + ); + } + + /// Pooled funding puts a loser's change in an account the winner never + /// touches. Sweeping only the winner's matched accounts leaves it behind. + #[tokio::test] + async fn test_a_loser_in_a_sibling_account_is_swept() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // Fund the BIP32 account. + let bip32_xpub = ctx + .wallet + .accounts + .standard_bip32_accounts + .get(&0) + .expect("BIP32 account") + .account_xpub; + let bip32_address = ctx + .managed_wallet + .first_bip32_managed_account_mut() + .expect("BIP32 managed account") + .next_receive_address(Some(&bip32_xpub), true) + .expect("BIP32 receive address"); + let funding_tx = Transaction::dummy(&bip32_address, 0..1, &[1_000_000]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[3u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + let funding_outpoint = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let spend = |change: &Address, change_amount: u64, sent: u64| Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + + // The loser's change lands on BIP44 — an account the winner, whose + // change goes back to BIP32, never matches. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend(&loser_change, 399_000, 600_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + let winner_change = ctx + .managed_wallet + .first_bip32_managed_account_mut() + .expect("BIP32 managed account") + .next_change_address(Some(&bip32_xpub), true) + .expect("BIP32 change address"); + let winner = spend(&winner_change, 299_000, 700_000); + ctx.check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[4u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + + assert!( + !ctx.bip44_account().transactions().contains_key(&loser.txid()), + "a loser in a sibling account must be swept too" + ); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 299_000, + "only the winner's change survives" + ); + } + /// A loser can spend inputs the winner does not. Sweeping it frees those /// coins from the spent set, but their `Utxo` values were discarded when /// the loser was recorded — so the sweep alone cannot put them back, and diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index ef9c01b7b..a36d24053 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -8,7 +8,9 @@ use crate::managed_account::managed_account_ref::ManagedAccountRefMut; use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::managed_platform_account::ManagedPlatformAccount; use crate::managed_account::ManagedCoreKeysAccount; -use dashcore::{OutPoint, Txid}; +use crate::transaction_checking::TransactionContext; +use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use dashcore::{OutPoint, Transaction, Txid}; use std::collections::{BTreeMap, BTreeSet}; /// What [`ManagedWalletInfo::abandon_transaction`] removed. @@ -60,6 +62,35 @@ fn collect_spenders_of_records( } impl ManagedWalletInfo { + /// Drop the outputs of every recorded transaction that `tx` provably beat + /// to one of its inputs, across the whole wallet. + /// + /// Wallet-wide on purpose, and deliberately not gated on relevance. Two + /// separate gaps make an account-local, relevance-gated sweep miss the + /// cases that matter: + /// + /// * Pooled funding puts a loser's change in an account the winner never + /// touches, so sweeping only the winner's accounts leaves it credited — + /// and as *trusted* change it is counted confirmed and is spendable. + /// * Relevance is computed from matching outputs and from inputs still + /// present in `utxos`, but the loser already removed the shared input. + /// A winner that spends our coin and pays only external addresses is + /// therefore classified irrelevant, and no account is visited at all. + /// + /// Returns whether anything was removed. + pub fn sweep_conflicts(&mut self, tx: &Transaction, context: &TransactionContext) -> bool { + let mut changed = false; + for account in self.accounts.all_accounts_mut() { + if let ManagedAccountRefMut::Funds(funds) = account { + changed |= funds.drop_conflicted_transactions(tx, context); + } + } + if changed { + self.update_balance(); + } + changed + } + /// Whether any account holds `txid` as settled by the network. /// /// Settled means chainlock-finalized, in a block, **or InstantSend-locked** @@ -118,7 +149,7 @@ impl ManagedWalletInfo { /// /// Does not recompute the balance — callers batching several abandons /// should run `update_balance` - /// ([`WalletInfoInterface`](crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface)) + /// (from [`WalletInfoInterface`]) /// once at the end. pub fn abandon_transaction(&mut self, root: Txid) -> AbandonOutcome { self.abandon_transaction_with_spends(root, &BTreeMap::new()) diff --git a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs index c2f298d5d..09bbbbda1 100644 --- a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs +++ b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs @@ -580,15 +580,32 @@ impl WalletInfoInterface for ManagedWalletInfo { return false; } let mut any_changed = false; + // Kept for the sweep below: it needs the locked transaction's inputs, + // and this signature carries only its txid. + let mut locked_transaction = None; for mut account in self.accounts.all_accounts_mut() { if account.mark_utxos_instant_send(txid) { any_changed = true; } if let Some(record) = account.transactions_mut().get_mut(txid) { record.update_context(TransactionContext::InstantSend(lock.clone())); + if locked_transaction.is_none() { + locked_transaction = Some(record.transaction.clone()); + } } } - if any_changed { + // An IS lock settles this transaction's inputs, so any recorded + // competing spend can never confirm. This is the path the live + // dash-spv pipeline takes for a lock arriving after the transaction is + // already tracked (`process_instant_send_lock`), and it had no sweep — + // the one in `check_core_transaction` is only reachable on a first + // sighting that already carries the lock. + let swept = locked_transaction.is_some_and(|tx| { + self.sweep_conflicts(&tx, &TransactionContext::InstantSend(lock.clone())) + }); + if any_changed && !swept { + // `sweep_conflicts` recomputes on its own when it removes + // something, so this only covers the marking-only case. self.update_balance(); } any_changed From 800b04384dde129f4d0f5d480c094b088927d58d Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:40:04 +0300 Subject: [PATCH 12/15] feat(key-wallet-manager): expose abandon, and pin the rescan-recovery limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **#10's mitigation.** `d3af15ed` widens trusted-self-send resolution to the whole wallet, which files a never-broadcast phantom under *confirmed* rather than unconfirmed. The sweep cannot clear that shape — it needs a competing final spend to prove the loser dead, and a transaction nobody ever saw has no competitor — so `abandon_transaction` is the only path that reaches it. Left as a `ManagedWalletInfo` method it was reachable only by a caller already holding the info; `WalletManager:: abandon_transaction` makes it a first-class entry point for the layer that owns broadcast policy, alongside the existing per-wallet operations. The underlying gap — key-wallet has no `fInMempool` equivalent, so trust is a structural check with no acceptance signal — is pre-existing and wants its own design pass. This does not close it; it makes the one remedy for its worst outcome callable. **#11.** `apply_abandon`'s "a rescan can rediscover them" holds only while the funding record is live. A chainlock-finalized funding transaction keeps just its txid, so `has_transaction` stays true, `is_new` stays false, and `confirm_transaction` returns before `update_utxos` — the only production insert site. The coin does not come back. Documented at the promise, and pinned by a test so the boundary cannot drift silently. Fixing it needs a rescan deep enough to re-fetch the block, which is above this layer. Co-Authored-By: Claude Opus 5 --- key-wallet-manager/src/lib.rs | 49 ++++++++++++- .../managed_core_funds_account.rs | 12 ++- .../transaction_checking/wallet_checker.rs | 73 ++++++++++++++++++- 3 files changed, 129 insertions(+), 5 deletions(-) diff --git a/key-wallet-manager/src/lib.rs b/key-wallet-manager/src/lib.rs index 89002d0ac..7bb892294 100644 --- a/key-wallet-manager/src/lib.rs +++ b/key-wallet-manager/src/lib.rs @@ -24,14 +24,16 @@ pub use events::{DerivedAddress, WalletEvent}; pub use matching::{check_compact_filters_for_elements, FilterMatchKey}; pub use wallet_interface::{BlockProcessingResult, MempoolTransactionResult, WalletInterface}; -use dashcore::blockdata::transaction::Transaction; +use dashcore::blockdata::transaction::{OutPoint, Transaction}; use dashcore::prelude::CoreBlockHeight; +use dashcore::Txid; use key_wallet::account::AccountCollection; use key_wallet::managed_account::transaction_record::TransactionRecord; use key_wallet::transaction_checking::{DerivedAddressInfo, TransactionContext}; use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use key_wallet::wallet::managed_wallet_info::AbandonOutcome; use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; use key_wallet::{AccountType, Address, ExtendedPrivKey, Mnemonic, Network, Wallet}; use key_wallet::{ExtendedPubKey, WalletCoreBalance}; @@ -662,6 +664,51 @@ impl WalletManager { } impl WalletManager { + /// Abandon `root` in `wallet_id`, and every recorded transaction + /// descending from it, then recompute the balance. + /// + /// The manager-level entry point for + /// [`ManagedWalletInfo::abandon_transaction_with_spends`] — the only + /// production path that clears a transaction the network never accepted. + /// The conflict sweep cannot reach that case: it needs a competing final + /// spend to prove the loser dead, and a transaction nobody ever saw has + /// no competitor. Its outputs would otherwise be credited forever, and as + /// trusted self-send change they are counted confirmed and are spendable. + /// + /// `external_spends` maps an outpoint to the transaction a caller's + /// persistence mirror recorded as spending it, for descendants whose own + /// records the load path never restored. Pass an empty map to walk only + /// the recorded transactions. + /// + /// **This asserts the root is dead; it does not establish it.** There is + /// no negative signal on the p2p network — Dash Core removed BIP61 + /// `reject` — so silence is not proof, and abandoning a transaction that + /// is merely quiet re-exposes its inputs to coin selection. Settled roots + /// are refused, but the judgement otherwise belongs to the caller that + /// owns broadcast policy. + /// + /// Returns `None` when the wallet is unknown. + pub fn abandon_transaction( + &mut self, + wallet_id: &WalletId, + root: Txid, + external_spends: &BTreeMap, + ) -> Option { + let info = self.get_wallet_info_mut(wallet_id)?; + let outcome = info.abandon_transaction_with_spends(root, external_spends); + if !outcome.is_empty() { + info.update_balance(); + tracing::info!( + %root, + abandoned = outcome.abandoned.len(), + records_removed = outcome.records_removed, + utxos_removed = outcome.utxos_removed, + "Abandoned a dead transaction and everything built on it" + ); + } + Some(outcome) + } + /// Get receive address from a specific wallet and account pub fn next_receive_address( &mut self, diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index ea9e20fac..37b7a871b 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -521,9 +521,15 @@ impl ManagedCoreFundsAccount { /// loser was recorded, and `InputDetail` keeps only index/value/address. /// The release is what makes them recoverable: a rescan re-delivering the /// funding transaction inserts them again. Until that rescan they are - /// absent from the balance, and if the funding transaction was itself - /// chainlock-finalized its record may already have been dropped, in which - /// case recovery needs a rescan deep enough to re-fetch the block. + /// absent from the balance. + /// + /// That recovery has a boundary worth knowing. A funding transaction that + /// was chainlock-finalized keeps only its txid, so `has_transaction` stays + /// true and re-delivery is not a new sighting — `confirm_transaction` + /// returns before `update_utxos`, the only production insert site, and the + /// coin does not come back. Recovering it needs a rescan deep enough to + /// re-fetch the block, which is above this layer. Since Dash chainlocks + /// within a block or two, that is the normal posture for older coins. /// /// Scope: account-local. A loser recorded here has its outputs dropped /// here; a loser whose change landed in a *different* account is not diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 8cefcd198..77e95806e 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2067,6 +2067,73 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } + /// The rescan recovery above has a boundary: a funding transaction that + /// was chainlock-finalized keeps only its txid, so re-delivering it is not + /// a new sighting and never reaches the only production UTXO insert site. + /// The coin stays absent. Documented rather than fixed — recovering it + /// needs a rescan deep enough to re-fetch the block, which is above this + /// layer. + #[tokio::test] + async fn test_rescan_recovery_does_not_reach_a_finalized_funding_transaction() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[1_000_000]); + let finalized = TransactionContext::InChainLockedBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[5u8; 32]).expect("hash"), + 1_700_000_000, + )); + ctx.check_transaction(&funding_tx, finalized.clone()).await; + + let change_address = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let spend = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: funding_tx.txid(), + vout: 0, + }, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: 900_000, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: 99_000, + script_pubkey: change_address.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + ctx.check_transaction(&spend, TransactionContext::Mempool).await; + + ctx.managed_wallet.abandon_transaction(spend.txid()); + ctx.managed_wallet.update_balance(); + + // Re-delivering the funding block does not bring the coin back. + ctx.check_transaction(&funding_tx, finalized).await; + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 0, + "a finalized funding record blocks the redelivery path this \ + recovery depends on" + ); + } + /// A winner that spends our coin but pays only external addresses matches /// nothing: its outputs are not ours, and the input it shares with the /// loser was already removed from `utxos` when the loser was recorded. It @@ -2603,7 +2670,11 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); // The real coin the chain consumed is released from the spent set, so - // a rescan can rediscover it rather than being told it is spent. + // a rescan can rediscover it — but only while its funding record is + // still live. A chainlock-finalized funding transaction keeps just its + // txid, so `has_transaction` stays true, `is_new` stays false, and + // `confirm_transaction` returns before reaching `update_utxos` — the + // only production insert site. See the sibling test below. let rediscovered = ctx .check_transaction( &funding_tx, From cf7ed1fb8db41bea8b7424acde504dacd4b5612b Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:07:42 +0300 Subject: [PATCH 13/15] feat(key-wallet-manager): report swept transactions so mirrors can delete them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **#12.** The sweep corrected wallet state in memory and told nobody. `TransactionCheckResult` and all five `WalletEvent` variants are purely additive, yet that bus is the documented persistence channel — so a consumer mirroring wallet state had no way to learn a row was gone. It kept the dead transaction, replayed it on the next load, and re-created the balance the wallet had already corrected. Observed on a testnet device: the correction landed every launch and never stuck. `drop_conflicted_transactions` now returns the txids it removed rather than a bool, `sweep_conflicts` unions them across accounts (one transaction can be recorded in several), and they surface as `TransactionCheckResult::swept_transactions` → `CheckTransactionsResult::per_wallet_swept` → a new `WalletEvent::TransactionsSwept` carrying the removed txids, the transaction that superseded them, and the post-removal balances. Gathered outside the relevance branch, because a sweep can fire for a transaction the wallet finds irrelevant — the shared input is already gone from `utxos` and the winner may pay only external addresses — and the removal still has to reach the consumer. Emitted before the additive events on both paths, so a consumer applying them in order deletes the dead rows before writing anything that replaces them. `dash-spv-ffi` has no C callback for the variant; it logs, with a `TODO(sweep-ffi)` naming the gap, rather than inventing ABI surface here. Also two review points from the same pass: `mark_instant_send_utxos` returned `false` when the sweep alone changed state, so the caller skipped the balance refresh and never emitted `TransactionInstantLocked` — it now reports a context rewrite and a sweep as changes. And a test comment named the wrong chain link as the multi-output one. Co-Authored-By: Claude Opus 5 --- dash-spv-ffi/src/callbacks.rs | 18 ++++++++ key-wallet-manager/src/events.rs | 41 ++++++++++++++++++ key-wallet-manager/src/lib.rs | 19 ++++++++ key-wallet-manager/src/process_block.rs | 43 +++++++++++++++++++ .../managed_core_funds_account.rs | 17 ++++---- .../transaction_checking/account_checker.rs | 12 ++++++ .../transaction_checking/wallet_checker.rs | 17 ++++---- .../src/wallet/managed_wallet_info/helpers.rs | 18 +++++--- .../wallet_info_interface.rs | 13 +++++- 9 files changed, 174 insertions(+), 24 deletions(-) diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index c9fc45ff5..346b92280 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -1021,6 +1021,24 @@ impl FFIWalletEventCallbacks { /// Dispatch a WalletEvent to the appropriate callback. pub fn dispatch(&self, event: &WalletEvent) { match event { + // TODO(sweep-ffi): no C callback is exposed for this variant yet, + // so a consumer of this FFI does not learn that the wallet dropped + // a superseded transaction and will keep mirroring the dead rows. + // Adding one is new ABI surface and wants its own review; logged + // meanwhile so the gap is observable rather than silent. + WalletEvent::TransactionsSwept { + wallet_id, + txids, + superseded_by, + .. + } => { + tracing::info!( + wallet_id = %hex::encode(wallet_id), + swept = txids.len(), + %superseded_by, + "TransactionsSwept has no FFI callback; consumers keep the removed rows" + ); + } WalletEvent::TransactionDetected { wallet_id, record, diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 77a5af1e7..803f3984e 100644 --- a/key-wallet-manager/src/events.rs +++ b/key-wallet-manager/src/events.rs @@ -222,6 +222,29 @@ pub enum WalletEvent { /// full balance after the change — not a delta. account_balances: BTreeMap, }, + /// Transactions were removed from the wallet: each was a recorded spend + /// that a later, final transaction provably beat to one of its inputs, so + /// it can never confirm. Their outputs are gone from the UTXO set and + /// their records deleted. + /// + /// The only removal-shaped event on this bus. A consumer mirroring wallet + /// state to disk must act on it — every other variant is additive, so + /// without this the mirror keeps the dead rows and replays them on the + /// next load, re-creating a balance the wallet has already corrected. + TransactionsSwept { + /// ID of the affected wallet. + wallet_id: WalletId, + /// Transactions removed. Delete these rows and any UTXO they created. + txids: Vec, + /// The transaction whose arrival settled the inputs, for provenance. + superseded_by: Txid, + /// Wallet balance after the removal. + balance: WalletCoreBalance, + /// Post-event balance **snapshots** for accounts whose balance + /// changed as a result of this event. Each value is the account's + /// full balance after the change — not a delta. + account_balances: BTreeMap, + }, /// A block was processed for a wallet. Carries records bucketed by what /// happened to them in this block, plus the post-block balance. /// `inserted` is records first stored in this block, `updated` is @@ -332,6 +355,10 @@ impl WalletEvent { wallet_id, .. } + | WalletEvent::TransactionsSwept { + wallet_id, + .. + } | WalletEvent::TransactionInstantLocked { wallet_id, .. @@ -382,6 +409,20 @@ impl fmt::Display for WalletEvent { balance, format_account_balances(account_balances), ), + WalletEvent::TransactionsSwept { + txids, + superseded_by, + balance, + account_balances, + .. + } => write!( + f, + "TransactionsSwept(count={}, superseded_by={}, balance={}, account_balances={})", + txids.len(), + superseded_by, + balance, + format_account_balances(account_balances), + ), WalletEvent::BlockProcessed { height, chain_lock, diff --git a/key-wallet-manager/src/lib.rs b/key-wallet-manager/src/lib.rs index 7bb892294..13e7b1c2b 100644 --- a/key-wallet-manager/src/lib.rs +++ b/key-wallet-manager/src/lib.rs @@ -96,6 +96,12 @@ pub struct CheckTransactionsResult { /// Records whose state was updated by this check (confirmation or /// InstantSend lock on a previously stored record), grouped by wallet. pub per_wallet_updated_records: BTreeMap>, + /// Transactions this check *removed*, grouped by wallet: recorded spends + /// the arriving transaction provably beat to one of its inputs. See + /// [`crate::events::WalletEvent::TransactionsSwept`] + /// — a consumer mirroring wallet state must delete these rows, since no + /// other signal on the bus reports a removal. + pub per_wallet_swept: BTreeMap>, } impl CheckTransactionsResult { @@ -630,6 +636,19 @@ impl WalletManager { } } + // Gathered outside the relevance branch above: a sweep can + // fire for a transaction this wallet finds irrelevant — the + // shared input is gone from `utxos` and the winner may pay + // only external addresses — and the removal still has to + // reach the consumer. + if !check_result.swept_transactions.is_empty() { + result + .per_wallet_swept + .entry(*wallet_id) + .or_default() + .extend(check_result.swept_transactions); + } + if !check_result.new_addresses.is_empty() { result .new_addresses diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index 28e0f986f..f889083d7 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -84,6 +84,26 @@ impl WalletInterface for WalletM for (wallet_id, records) in check_result.per_wallet_updated_records { per_wallet_updated.entry(wallet_id).or_default().extend(records); } + // Emitted per transaction rather than batched into the block + // event: a sweep names the transaction that superseded the + // removed ones, and that attribution is lost once the block's + // transactions are folded together. + for (wallet_id, txids) in check_result.per_wallet_swept { + if txids.is_empty() { + continue; + } + let Some(info) = self.wallet_infos.get(&wallet_id) else { + continue; + }; + let event = WalletEvent::TransactionsSwept { + wallet_id, + txids, + superseded_by: tx.txid(), + balance: info.balance(), + account_balances: BTreeMap::new(), + }; + self.emit_event(event); + } } self.finalize_block_advance( @@ -185,6 +205,29 @@ impl WalletInterface for WalletM } } + // Removals, before the additive events: a consumer applying these in + // order sees the dead rows deleted first, so a replacement paying the + // same address cannot be clobbered by the delete that follows it. + for (wallet_id, txids) in std::mem::take(&mut check_result.per_wallet_swept) { + if txids.is_empty() { + continue; + } + let Some(info) = self.wallet_infos.get(&wallet_id) else { + continue; + }; + let event = WalletEvent::TransactionsSwept { + wallet_id, + txids, + superseded_by: tx.txid(), + balance: info.balance(), + account_balances: per_wallet_account_diff + .get(&wallet_id) + .cloned() + .unwrap_or_default(), + }; + self.emit_event(event); + } + if let Some(lock) = instant_lock { for (wallet_id, records) in per_wallet_updated_records { if records.is_empty() { diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 37b7a871b..2aa2e9d90 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -401,8 +401,6 @@ impl ManagedCoreFundsAccount { } } - utxos_changed |= self.drop_conflicted_transactions(tx, &context); - if utxos_changed { self.keys.bump_monitor_revision(); } @@ -537,14 +535,14 @@ impl ManagedCoreFundsAccount { /// per-account. That covers the ordinary shape — a resend keeps the same /// funding account and so the same change account — but not every one. /// - /// Returns whether any UTXO was removed. + /// Returns the txids it removed. pub(crate) fn drop_conflicted_transactions( &mut self, tx: &Transaction, context: &TransactionContext, - ) -> bool { + ) -> Vec { if !(context.confirmed() || matches!(context, TransactionContext::InstantSend(_))) { - return false; + return Vec::new(); } let winner = tx.txid(); @@ -579,7 +577,7 @@ impl ManagedCoreFundsAccount { .collect(); if losers.is_empty() { - return false; + return Vec::new(); } // A loser's change may already have funded further unconfirmed @@ -614,8 +612,8 @@ impl ManagedCoreFundsAccount { } } - let mut changed = false; let mut freed: HashSet = HashSet::new(); + let mut changed = false; for loser in &losers { let removed: Vec = self.utxos.keys().filter(|outpoint| outpoint.txid == *loser).copied().collect(); @@ -633,8 +631,11 @@ impl ManagedCoreFundsAccount { ); } self.release_spent_marks(&freed); + if changed { + self.keys.bump_monitor_revision(); + } - changed + losers.into_iter().collect() } /// Re-process an existing transaction with updated context (e.g., diff --git a/key-wallet/src/transaction_checking/account_checker.rs b/key-wallet/src/transaction_checking/account_checker.rs index e1a907d8b..c138ce994 100644 --- a/key-wallet/src/transaction_checking/account_checker.rs +++ b/key-wallet/src/transaction_checking/account_checker.rs @@ -17,6 +17,7 @@ use dashcore::blockdata::transaction::Transaction; use dashcore::hashes::Hash as _; use dashcore::transaction::TransactionPayload; use dashcore::ScriptBuf; +use dashcore::Txid; /// Classification of an address within an account #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -76,6 +77,16 @@ pub struct TransactionCheckResult { /// applied to a previously stored record). Each record carries its owning /// `AccountType` on `record.account_type`. pub updated_records: Vec, + /// Transactions this check *removed*: a recorded spend that the arriving + /// transaction provably beat to one of its inputs, plus anything built on + /// its outputs. They can never confirm, so their outputs were dropped + /// from the UTXO set and their records deleted. + /// + /// The only non-additive field here, and it exists because a consumer + /// mirroring wallet state cannot otherwise learn a row is gone — it would + /// replay the dead transaction on the next load and re-create the phantom + /// balance this removal just cleared. + pub swept_transactions: Vec, } /// Enum representing the type of Core account that matched with embedded data @@ -405,6 +416,7 @@ impl ManagedAccountCollection { new_addresses: Vec::new(), new_records: Vec::new(), updated_records: Vec::new(), + swept_transactions: Vec::new(), }; for account_type in account_types { diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 77e95806e..167a389ba 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -89,11 +89,11 @@ impl WalletTransactionChecker for ManagedWalletInfo { // below with the loser still credited. Sweep first, wallet-wide, next // to `record_observed_spends` above for the same reason it is // unconditional. - if update_state - && (context.confirmed() || context.is_instant_send()) - && self.sweep_conflicts(tx, &context) - { - result.state_modified = true; + if update_state && (context.confirmed() || context.is_instant_send()) { + result.swept_transactions = self.sweep_conflicts(tx, &context); + if !result.swept_transactions.is_empty() { + result.state_modified = true; + } } if !update_state || !result.is_relevant { @@ -2632,9 +2632,10 @@ mod tests { } let root = chain[0].txid(); - // The tip's change, plus the first link's second output — that one is - // never spent onward, so the cascade has to drop two UTXOs for one of - // the txids rather than assuming one each. + // Both live UTXOs belong to the tip, which pays us twice and is spent + // onward by nothing — so the cascade has to drop two UTXOs for that + // one txid rather than assuming one each. The earlier links' change + // is consumed by the next link. assert_eq!(ctx.bip44_account().utxos.len(), 2, "live change outputs"); let outcome = ctx.managed_wallet.abandon_transaction(root); diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index a36d24053..8e3842588 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -77,18 +77,24 @@ impl ManagedWalletInfo { /// A winner that spends our coin and pays only external addresses is /// therefore classified irrelevant, and no account is visited at all. /// - /// Returns whether anything was removed. - pub fn sweep_conflicts(&mut self, tx: &Transaction, context: &TransactionContext) -> bool { - let mut changed = false; + /// Returns the txids removed, so a caller mirroring wallet state can + /// learn those rows are gone — nothing else in the event surface reports + /// a removal, and a mirror that misses it replays the dead transaction. + pub fn sweep_conflicts(&mut self, tx: &Transaction, context: &TransactionContext) -> Vec { + let mut swept = Vec::new(); for account in self.accounts.all_accounts_mut() { if let ManagedAccountRefMut::Funds(funds) = account { - changed |= funds.drop_conflicted_transactions(tx, context); + swept.extend(funds.drop_conflicted_transactions(tx, context)); } } - if changed { + if !swept.is_empty() { self.update_balance(); + // One transaction can be recorded in several accounts, so the + // per-account results overlap. + swept.sort_unstable(); + swept.dedup(); } - changed + swept } /// Whether any account holds `txid` as settled by the network. diff --git a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs index 09bbbbda1..b66293ce0 100644 --- a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs +++ b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs @@ -270,6 +270,14 @@ pub trait WalletInfoInterface: Sized + WalletTransactionChecker + ManagedAccount /// Mark UTXOs for a transaction as InstantSend-locked across all accounts /// and update the corresponding transaction record context. /// Returns `true` if any UTXO was newly marked. + /// Apply an InstantSend lock: mark the transaction's UTXOs, rewrite its + /// record context, and drop any competing spend the lock now settles. + /// + /// Returns whether wallet state changed in any of those ways — callers + /// use it to refresh balances and to decide whether to emit + /// `TransactionInstantLocked`. An outgoing transaction can own no UTXOs + /// of ours and still change state by rewriting its context or by the + /// sweep removing a loser, so this is broader than "a UTXO was marked". fn mark_instant_send_utxos(&mut self, txid: &Txid, lock: &InstantLock) -> bool; /// Return the aggregated monitor revision across all accounts. @@ -589,6 +597,7 @@ impl WalletInfoInterface for ManagedWalletInfo { } if let Some(record) = account.transactions_mut().get_mut(txid) { record.update_context(TransactionContext::InstantSend(lock.clone())); + any_changed = true; if locked_transaction.is_none() { locked_transaction = Some(record.transaction.clone()); } @@ -601,14 +610,14 @@ impl WalletInfoInterface for ManagedWalletInfo { // the one in `check_core_transaction` is only reachable on a first // sighting that already carries the lock. let swept = locked_transaction.is_some_and(|tx| { - self.sweep_conflicts(&tx, &TransactionContext::InstantSend(lock.clone())) + !self.sweep_conflicts(&tx, &TransactionContext::InstantSend(lock.clone())).is_empty() }); if any_changed && !swept { // `sweep_conflicts` recomputes on its own when it removes // something, so this only covers the marking-only case. self.update_balance(); } - any_changed + any_changed || swept } fn monitor_revision(&self) -> u64 { From 6aa7b5581a4ffa09a8a1eb52858cdbc0c4413b11 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:19:28 +0300 Subject: [PATCH 14/15] fix(key-wallet): stop the sweep freeing the outpoint the winner spends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `freed` collected every input of every removed loser — including the shared one the winner itself consumed. `release_spent_marks` keeps a mark only while a live record claims it, and on the checker path the sweep runs *before* the winner is recorded, so nothing did: the outpoint was released, and a later rescan could re-insert a coin that is spent on chain. Coin selection would then build a guaranteed double spend — the mirror image of the bug this PR exists to fix. Two arrival paths hid it. A block winner is already in `observed_spent_outpoints`, so the #649 guard blocks re-insertion; a relevant winner re-marks the outpoint moments later when it is recorded. An InstantSend winner has neither — the context carries no block info, so no observed spend is recorded, and an irrelevant one is never recorded at all. Reachable through `process_mempool_transaction`, which builds an `InstantSend` context for a mempool transaction arriving with its lock. Retain only the loser's *extra* inputs, independent of record ordering. `test_the_sweep_never_frees_the_winners_own_input` reproduces the InstantSend shape and was verified to fail without the retain. Two coverage gaps closed alongside, both for code this PR added: `abandon_transaction_with_spends`' external view — a descendant reachable only through the map, a stale row naming a settled transaction that must not be followed, and a settled root refused — and the reverse arrival order that `doomed_by_a_settled_spend` guards, where the winner confirms first and the loser turns up afterwards. Both verified to fail with their respective fix disabled. Co-Authored-By: Claude Opus 5 --- .../managed_core_funds_account.rs | 9 + .../transaction_checking/wallet_checker.rs | 317 ++++++++++++++++++ 2 files changed, 326 insertions(+) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 2aa2e9d90..91eac9d5c 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -630,6 +630,15 @@ impl ManagedCoreFundsAccount { "Dropped a conflicted transaction: its input was spent by a final transaction" ); } + // Never free an outpoint the winner itself spends. `freed` collects + // every input of every removed loser, and the shared one is exactly + // what the winner consumed — releasing it would let a later rescan + // re-insert a coin that is spent on chain, and coin selection would + // then build a guaranteed double spend. `release_spent_marks` cannot + // catch this on its own: on the checker path the sweep runs before + // the winner is recorded, so no live record claims the outpoint yet. + // Only the loser's *extra* inputs are genuinely released. + freed.retain(|outpoint| !spent.contains(outpoint)); self.release_spent_marks(&freed); if changed { self.keys.bump_monitor_revision(); diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 167a389ba..f16bfdb5d 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2337,6 +2337,323 @@ mod tests { ); } + /// The reverse arrival order: the winner confirms first, and the loser + /// turns up afterwards from the mempool. No sweep can help — the sweep + /// fires on the *arriving* transaction being final, and here the arrival + /// is the loser. The refusal has to happen at record time. + #[tokio::test] + async fn test_a_loser_arriving_after_its_winner_is_never_credited() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[1_000_000]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[8u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + let shared = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let spend = |change: &Address, change_amount: u64, sent: u64| Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: shared, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + + // The winner confirms first. + let winner_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let winner = spend(&winner_change, 299_000, 700_000); + ctx.check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[9u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + assert_eq!(ctx.managed_wallet.balance.confirmed(), 299_000); + + // Then the loser turns up. Its input is provably spent, so its + // outputs must never be credited. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend(&loser_change, 399_000, 600_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + assert!( + !ctx.bip44_account().utxos.keys().any(|o| o.txid == loser.txid()), + "a transaction whose input a block already spent must not be credited" + ); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 299_000, + "only the winner's change counts" + ); + } + + /// `abandon_transaction_with_spends`' external view: a descendant whose + /// own record the load path never restored, a stale row naming a settled + /// transaction that must not be followed, and a settled root refused + /// outright. None of these are reachable through the no-argument form. + #[tokio::test] + async fn test_abandon_honours_the_external_spend_view_and_refuses_settled() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[1_000_000]); + let block = TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[7u8; 32]).expect("hash"), + 1_700_000_000, + )); + ctx.check_transaction(&funding_tx, block.clone()).await; + + let spend_of = + |parent: OutPoint, change: &Address, change_amount: u64, sent: u64| Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: parent, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }], + output: vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_amount, + script_pubkey: change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + let next_change = |ctx: &mut TestWalletContext| { + ctx.managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address") + }; + + // Root, then a child spending its change. + let root_change = next_change(&mut ctx); + let root = spend_of( + OutPoint { + txid: funding_tx.txid(), + vout: 0, + }, + &root_change, + 399_000, + 600_000, + ); + ctx.check_transaction(&root, TransactionContext::Mempool).await; + let child_change = next_change(&mut ctx); + let child = spend_of( + OutPoint { + txid: root.txid(), + vout: 1, + }, + &child_change, + 298_000, + 100_000, + ); + ctx.check_transaction(&child, TransactionContext::Mempool).await; + + // A settled root is refused outright, whatever the map says. + ctx.check_transaction(&funding_tx, block).await; + let refused = ctx.managed_wallet.abandon_transaction(funding_tx.txid()); + assert!(refused.is_empty(), "a settled root must be refused: {refused:?}"); + + // Simulate the restore: the child's record is absent, so the plain + // walk cannot reach it — only the mirror's linkage can. + ctx.managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .keys_mut() + .transactions_mut() + .remove(&child.txid()); + + let plain = ctx.managed_wallet.abandon_transaction(root.txid()); + assert_eq!(plain.abandoned.len(), 1, "without the map the walk stops at the root"); + assert!( + ctx.bip44_account().utxos.keys().any(|o| o.txid == child.txid()), + "the child's output is still credited" + ); + + // Now with the map, plus a stale row naming the settled funding tx — + // which must not be followed. + let mut external = BTreeMap::new(); + external.insert( + OutPoint { + txid: root.txid(), + vout: 1, + }, + child.txid(), + ); + external.insert( + OutPoint { + txid: child.txid(), + vout: 1, + }, + funding_tx.txid(), + ); + let outcome = ctx.managed_wallet.abandon_transaction_with_spends(root.txid(), &external); + ctx.managed_wallet.update_balance(); + + assert!( + outcome.abandoned.contains(&child.txid()), + "the map must reach a descendant whose record is gone" + ); + assert!( + !outcome.abandoned.contains(&funding_tx.txid()), + "a settled spender named by a stale row must not be followed" + ); + assert!( + !ctx.bip44_account().utxos.keys().any(|o| o.txid == child.txid()), + "the child's outputs must be gone" + ); + assert!( + ctx.bip44_account().transactions().contains_key(&funding_tx.txid()), + "the settled funding record must survive" + ); + } + + /// The sweep must never free the outpoint the winner itself spends. + /// + /// Two of the three arrival paths hide this: a block winner is already in + /// `observed_spent_outpoints`, and a relevant winner re-marks the outpoint + /// when it is recorded. An InstantSend winner has neither — the context + /// carries no block info, so no observed spend is recorded, and an + /// irrelevant one is never recorded at all. Releasing the shared coin + /// there lets a rescan re-insert money that is spent on chain. + #[tokio::test] + async fn test_the_sweep_never_frees_the_winners_own_input() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[1_000_000]); + let funding_context = TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[6u8; 32]).expect("hash"), + 1_700_000_000, + )); + ctx.check_transaction(&funding_tx, funding_context.clone()).await; + + let shared_input = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let input = || TxIn { + previous_output: shared_input, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: dashcore::Witness::new(), + }; + + // Loser first: recorded, so the coin leaves `utxos`. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = Transaction { + version: 2, + lock_time: 0, + input: vec![input()], + output: vec![ + TxOut { + value: 600_000, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: 399_000, + script_pubkey: loser_change.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + // The winner arrives InstantSend-locked and pays only outside the + // wallet: no block info to record an observed spend, and nothing + // about it matches, so it is never recorded either. + let winner = Transaction { + version: 2, + lock_time: 0, + input: vec![input()], + output: vec![TxOut { + value: 999_000, + script_pubkey: external_address.script_pubkey(), + }], + special_transaction_payload: None, + }; + let is_lock = InstantLock { + txid: winner.txid(), + ..InstantLock::default() + }; + let result = ctx.check_transaction(&winner, TransactionContext::InstantSend(is_lock)).await; + assert!(!result.is_relevant, "the precondition: the winner matches nothing"); + assert!( + !ctx.bip44_account().transactions().contains_key(&loser.txid()), + "the loser is still swept" + ); + + // The shared coin is spent on chain by the winner. Re-delivering the + // funding block must not bring it back. + ctx.check_transaction(&funding_tx, funding_context).await; + assert!( + !ctx.bip44_account().utxos.contains_key(&shared_input), + "a rescan must not resurrect a coin the winner consumed" + ); + assert_eq!(ctx.managed_wallet.balance.confirmed(), 0); + } + /// A loser can spend inputs the winner does not. Sweeping it frees those /// coins from the spent set, but their `Utxo` values were discarded when /// the loser was recorded — so the sweep alone cannot put them back, and From ed360ccac6f73432e0edd77f0ea7b7aa54fe09dd Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:17:57 +0300 Subject: [PATCH 15/15] feat(dash-spv-ffi): expose the sweep as a C callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the `TODO(sweep-ffi)` placeholder, per review. Leaving it as a log line meant every C consumer silently kept mirroring transactions the wallet had already dropped — which is the same class of bug the sweep exists to fix, just one layer out. `on_transactions_swept` delivers the removed txids, the transaction whose arrival settled the inputs, and the post-removal balances. Its doc says plainly that this is the only removal-shaped wallet callback and that a consumer mirroring state must act on it, since every other one is additive. An unset callback now warns rather than logging at info: a consumer that never wires it up is in exactly the state this event exists to prevent, and that should be loud. The bundled CLI consumes it, so the shape has at least one caller in-tree. Co-Authored-By: Claude Opus 5 --- dash-spv-ffi/src/bin/ffi_cli.rs | 29 ++++++++ dash-spv-ffi/src/callbacks.rs | 85 +++++++++++++++++++--- dash-spv-ffi/tests/dashd_sync/callbacks.rs | 2 + 3 files changed, 104 insertions(+), 12 deletions(-) diff --git a/dash-spv-ffi/src/bin/ffi_cli.rs b/dash-spv-ffi/src/bin/ffi_cli.rs index 38aeb8098..198fc8975 100644 --- a/dash-spv-ffi/src/bin/ffi_cli.rs +++ b/dash-spv-ffi/src/bin/ffi_cli.rs @@ -226,6 +226,34 @@ extern "C" fn on_transaction_detected( ); } +extern "C" fn on_transactions_swept( + wallet_id: *const c_char, + txids: *const [u8; 32], + txids_count: usize, + superseded_by: *const [u8; 32], + balance: *const FFIBalance, + _account_balances: *const dash_spv_ffi::FFIAccountBalance, + _account_balances_count: u32, + _user_data: *mut c_void, +) { + let wallet_short = short_wallet(wallet_id); + if txids.is_null() || superseded_by.is_null() { + println!("[Wallet] TXs swept: wallet={}..., null payload", wallet_short); + return; + } + let list = unsafe { std::slice::from_raw_parts(txids, txids_count) }; + let winner = unsafe { &*superseded_by }; + let b = read_balance(balance); + println!( + "[Wallet] TXs swept: wallet={}..., removed=[{}], superseded_by={}, balance[confirmed={}, unconfirmed={}]", + wallet_short, + list.iter().map(hex::encode).collect::>().join(","), + hex::encode(winner), + b.confirmed, + b.unconfirmed, + ); +} + extern "C" fn on_transaction_instant_locked( wallet_id: *const c_char, txid: *const [u8; 32], @@ -548,6 +576,7 @@ fn main() { wallet: FFIWalletEventCallbacks { on_transaction_detected: Some(on_transaction_detected), on_transaction_instant_locked: Some(on_transaction_instant_locked), + on_transactions_swept: Some(on_transactions_swept), on_block_processed: Some(on_wallet_block_processed), on_sync_height_advanced: Some(on_sync_height_advanced), on_chain_lock_processed: Some(on_wallet_chain_lock_processed), diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index 346b92280..921f99041 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -752,6 +752,37 @@ pub type OnTransactionDetectedCallback = Option< ), >; +/// Callback for `WalletEvent::TransactionsSwept`. +/// +/// Fires when the wallet removes transactions that a later, final transaction +/// provably beat to one of their inputs: they can never confirm, so their +/// outputs are gone from the UTXO set and their records deleted. +/// +/// **The only removal-shaped wallet callback.** Every other one is additive, +/// so a consumer mirroring wallet state to disk must act on this — delete the +/// named transactions and any UTXO they created. Ignoring it leaves the dead +/// rows in the mirror, which replays them on the next load and re-creates a +/// balance the wallet has already corrected. +/// +/// `txids` points to `txids_count` consecutive 32-byte txids. +/// `superseded_by` is the transaction whose arrival settled the inputs. +/// All pointer parameters are borrowed and only valid for the duration of the +/// callback. `balance` is the wallet's balance *after* the removal; +/// `account_balances` follows the same contract as on +/// [`OnTransactionDetectedCallback`]. +pub type OnTransactionsSweptCallback = Option< + extern "C" fn( + wallet_id: *const c_char, + txids: *const [u8; 32], + txids_count: usize, + superseded_by: *const [u8; 32], + balance: *const FFIBalance, + account_balances: *const FFIAccountBalance, + account_balances_count: u32, + user_data: *mut c_void, + ), +>; + /// Callback for `WalletEvent::TransactionInstantLocked`. /// /// Fires when an InstantSend lock is applied to a previously-seen off-chain @@ -909,6 +940,7 @@ pub type OnWalletChainLockProcessedCallback = Option< pub struct FFIWalletEventCallbacks { pub on_transaction_detected: OnTransactionDetectedCallback, pub on_transaction_instant_locked: OnTransactionInstantLockedCallback, + pub on_transactions_swept: OnTransactionsSweptCallback, pub on_block_processed: OnWalletBlockProcessedCallback, pub on_sync_height_advanced: OnSyncHeightAdvancedCallback, pub on_chain_lock_processed: OnWalletChainLockProcessedCallback, @@ -924,6 +956,7 @@ impl Default for FFIWalletEventCallbacks { Self { on_transaction_detected: None, on_transaction_instant_locked: None, + on_transactions_swept: None, on_block_processed: None, on_sync_height_advanced: None, on_chain_lock_processed: None, @@ -1021,23 +1054,51 @@ impl FFIWalletEventCallbacks { /// Dispatch a WalletEvent to the appropriate callback. pub fn dispatch(&self, event: &WalletEvent) { match event { - // TODO(sweep-ffi): no C callback is exposed for this variant yet, - // so a consumer of this FFI does not learn that the wallet dropped - // a superseded transaction and will keep mirroring the dead rows. - // Adding one is new ABI surface and wants its own review; logged - // meanwhile so the gap is observable rather than silent. WalletEvent::TransactionsSwept { wallet_id, txids, superseded_by, - .. + balance, + account_balances, } => { - tracing::info!( - wallet_id = %hex::encode(wallet_id), - swept = txids.len(), - %superseded_by, - "TransactionsSwept has no FFI callback; consumers keep the removed rows" - ); + if let Some(cb) = self.on_transactions_swept { + let wallet_id_hex = hex::encode(wallet_id); + let c_wallet_id = CString::new(wallet_id_hex).unwrap_or_default(); + let raw_txids: Vec<[u8; 32]> = + txids.iter().map(|t| t.to_byte_array()).collect(); + let raw_superseded_by = superseded_by.to_byte_array(); + let ffi_balance = FFIBalance::from(*balance); + let ffi_account_balances = FFIAccountBalance::from_map(account_balances); + let account_balances_ptr = if ffi_account_balances.is_empty() { + ptr::null() + } else { + ffi_account_balances.as_ptr() + }; + + cb( + c_wallet_id.as_ptr(), + raw_txids.as_ptr(), + raw_txids.len(), + &raw_superseded_by as *const [u8; 32], + &ffi_balance as *const FFIBalance, + account_balances_ptr, + ffi_account_balances.len() as u32, + self.user_data, + ); + + drop(ffi_account_balances); + } else { + // Deliberately loud: every other wallet callback is + // additive, so a consumer that leaves this one unset keeps + // transactions the wallet has already dropped. + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + swept = txids.len(), + %superseded_by, + "no on_transactions_swept callback set; the consumer will keep \ + mirroring transactions the wallet removed" + ); + } } WalletEvent::TransactionDetected { wallet_id, diff --git a/dash-spv-ffi/tests/dashd_sync/callbacks.rs b/dash-spv-ffi/tests/dashd_sync/callbacks.rs index c5335dead..c9e58b108 100644 --- a/dash-spv-ffi/tests/dashd_sync/callbacks.rs +++ b/dash-spv-ffi/tests/dashd_sync/callbacks.rs @@ -622,6 +622,8 @@ pub(super) fn create_wallet_callbacks(tracker: &Arc) -> FFIWall on_block_processed: Some(on_wallet_block_processed), on_sync_height_advanced: Some(on_sync_height_advanced), on_chain_lock_processed: None, + // Not exercised by these tests: they never build a conflicting spend. + on_transactions_swept: None, user_data: Arc::as_ptr(tracker) as *mut c_void, } }