From 51eafd8c82371ba88305ceb35b4c1604062bc6c8 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:51:28 +0300 Subject: [PATCH 1/5] feat(key-wallet-manager): name the outpoints a sweep releases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `WalletEvent::TransactionsSwept` told a consumer which transactions were deleted but not what to do with the coins they claimed to spend. `ManagedCoreFundsAccount::release_spent_marks` already computes exactly that distinction internally — freed minus still-spent, so a loser spending A+B against a winner spending only A leaves A marked and frees only B — and then discards it. A persistence mirror (Dash Platform's SwiftData/Room/SQLite seam) cannot re-derive the set on its own: the winning transaction that triggers a sweep does not have to be wallet-relevant at all (it can spend our coin and pay only external addresses), so it may never appear anywhere else in the wallet's event stream. Guessing either re-credits a coin the chain has already spent or strands a genuinely free one as spent forever. Add `released_outpoints` to `WalletEvent::TransactionsSwept`, carrying the authoritative release set computed once, threaded up unchanged: - `ManagedCoreFundsAccount::release_spent_marks` now returns the outpoints it actually released, and `drop_conflicted_transactions` returns them alongside the removed txids as a new `ConflictSweep`. - `ManagedWalletInfo::sweep_conflicts` unions this across every account swept into a new `WalletConflictSweep` (one transaction can be recorded in several accounts). - `TransactionCheckResult::released_outpoints` and `CheckTransactionsResult::per_wallet_released_outpoints` carry it through the existing per-wallet aggregation, parallel to `swept_transactions` / `per_wallet_swept`. - Both `WalletEvent::TransactionsSwept` emission sites in key-wallet-manager/src/process_block.rs (block and mempool paths) fill in the new field. - The C ABI mirror (`OnTransactionsSweptCallback` / `on_transactions_swept` in dash-spv-ffi) gains a matching `released_outpoints` array of a new `FFIOutPoint`, following the same borrowed-pointer/count contract as the existing txid array. Wallet-scoped rather than attributed per removed transaction: a consumer holds every input of every transaction it deletes, so it only needs to know which of them came free, not which removal freed which. No behavioral change to the sweep itself — this only surfaces data it already computed. Extends the existing sweep coverage in key-wallet/src/transaction_checking/wallet_checker.rs: the A+B / winner-takes-only-A case now asserts the released set is exactly {B}, and the ordinary winner-takes-everything case asserts it is empty. --- dash-spv-ffi/src/bin/ffi_cli.rs | 14 ++++- dash-spv-ffi/src/callbacks.rs | 43 +++++++++++++++ key-wallet-manager/src/events.rs | 27 +++++++++- key-wallet-manager/src/lib.rs | 12 +++++ key-wallet-manager/src/process_block.rs | 7 +++ .../managed_core_funds_account.rs | 52 ++++++++++++++++--- .../transaction_checking/account_checker.rs | 16 +++++- .../transaction_checking/wallet_checker.rs | 37 +++++++++---- .../src/wallet/managed_wallet_info/helpers.rs | 48 ++++++++++++++--- 9 files changed, 226 insertions(+), 30 deletions(-) diff --git a/dash-spv-ffi/src/bin/ffi_cli.rs b/dash-spv-ffi/src/bin/ffi_cli.rs index 198fc8975..a9715bbf5 100644 --- a/dash-spv-ffi/src/bin/ffi_cli.rs +++ b/dash-spv-ffi/src/bin/ffi_cli.rs @@ -231,6 +231,8 @@ extern "C" fn on_transactions_swept( txids: *const [u8; 32], txids_count: usize, superseded_by: *const [u8; 32], + released_outpoints: *const dash_spv_ffi::FFIOutPoint, + released_outpoints_count: usize, balance: *const FFIBalance, _account_balances: *const dash_spv_ffi::FFIAccountBalance, _account_balances_count: u32, @@ -243,12 +245,22 @@ extern "C" fn on_transactions_swept( } let list = unsafe { std::slice::from_raw_parts(txids, txids_count) }; let winner = unsafe { &*superseded_by }; + let released = if released_outpoints.is_null() { + &[][..] + } else { + unsafe { std::slice::from_raw_parts(released_outpoints, released_outpoints_count) } + }; let b = read_balance(balance); println!( - "[Wallet] TXs swept: wallet={}..., removed=[{}], superseded_by={}, balance[confirmed={}, unconfirmed={}]", + "[Wallet] TXs swept: wallet={}..., removed=[{}], superseded_by={}, released=[{}], balance[confirmed={}, unconfirmed={}]", wallet_short, list.iter().map(hex::encode).collect::>().join(","), hex::encode(winner), + released + .iter() + .map(|o| format!("{}:{}", hex::encode(o.txid), o.vout)) + .collect::>() + .join(","), b.confirmed, b.unconfirmed, ); diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index 921f99041..b98dda37e 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -752,6 +752,28 @@ pub type OnTransactionDetectedCallback = Option< ), >; +/// C representation of a Core [`OutPoint`](dashcore::OutPoint): the parent +/// txid and output index of a coin. +#[repr(C)] +pub struct FFIOutPoint { + /// Parent transaction id. + pub txid: [u8; 32], + /// Output index within the parent transaction. + pub vout: u32, +} + +impl FFIOutPoint { + fn from_slice(outpoints: &[dashcore::OutPoint]) -> Vec { + outpoints + .iter() + .map(|outpoint| FFIOutPoint { + txid: *outpoint.txid.as_byte_array(), + vout: outpoint.vout, + }) + .collect() + } +} + /// Callback for `WalletEvent::TransactionsSwept`. /// /// Fires when the wallet removes transactions that a later, final transaction @@ -766,6 +788,15 @@ pub type OnTransactionDetectedCallback = Option< /// /// `txids` points to `txids_count` consecutive 32-byte txids. /// `superseded_by` is the transaction whose arrival settled the inputs. +/// `released_outpoints` points to `released_outpoints_count` outpoints freed +/// by the removal: inputs the removed transactions claimed to spend that no +/// surviving record spends too. Mark these coins spendable again. This is +/// not the same set as `txids`' inputs — a loser spending A+B against a +/// winner spending only A leaves A marked and frees only B — and it cannot +/// be recomputed from `txids` on the consumer side: `superseded_by` need not +/// be wallet-relevant at all (it can spend our coin while paying only +/// external addresses), so it may never appear in any other callback. Null +/// with a zero count when the removal released nothing. /// 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 @@ -776,6 +807,8 @@ pub type OnTransactionsSweptCallback = Option< txids: *const [u8; 32], txids_count: usize, superseded_by: *const [u8; 32], + released_outpoints: *const FFIOutPoint, + released_outpoints_count: usize, balance: *const FFIBalance, account_balances: *const FFIAccountBalance, account_balances_count: u32, @@ -1058,6 +1091,7 @@ impl FFIWalletEventCallbacks { wallet_id, txids, superseded_by, + released_outpoints, balance, account_balances, } => { @@ -1067,6 +1101,12 @@ impl FFIWalletEventCallbacks { 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_released_outpoints = FFIOutPoint::from_slice(released_outpoints); + let released_outpoints_ptr = if ffi_released_outpoints.is_empty() { + ptr::null() + } else { + ffi_released_outpoints.as_ptr() + }; 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() { @@ -1080,6 +1120,8 @@ impl FFIWalletEventCallbacks { raw_txids.as_ptr(), raw_txids.len(), &raw_superseded_by as *const [u8; 32], + released_outpoints_ptr, + ffi_released_outpoints.len(), &ffi_balance as *const FFIBalance, account_balances_ptr, ffi_account_balances.len() as u32, @@ -1087,6 +1129,7 @@ impl FFIWalletEventCallbacks { ); drop(ffi_account_balances); + drop(ffi_released_outpoints); } else { // Deliberately loud: every other wallet callback is // additive, so a consumer that leaves this one unset keeps diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 803f3984e..5355bd023 100644 --- a/key-wallet-manager/src/events.rs +++ b/key-wallet-manager/src/events.rs @@ -10,7 +10,7 @@ use std::fmt; use dashcore::ephemerealdata::chain_lock::ChainLock; use dashcore::ephemerealdata::instant_lock::InstantLock; use dashcore::prelude::CoreBlockHeight; -use dashcore::{PublicKey, Txid}; +use dashcore::{OutPoint, PublicKey, Txid}; use key_wallet::account::AccountType; use key_wallet::managed_account::address_pool::{AddressPoolType, PublicKeyType}; use key_wallet::managed_account::transaction_record::TransactionRecord; @@ -238,6 +238,27 @@ pub enum WalletEvent { txids: Vec, /// The transaction whose arrival settled the inputs, for provenance. superseded_by: Txid, + /// Outpoints the sweep released: inputs the removed transactions + /// claimed to spend that no surviving record spends too (a loser + /// spending A+B against a winner spending only A leaves A marked and + /// frees B). Mark these coins spendable again. + /// + /// Upstream computes this distinction — see + /// `ManagedCoreFundsAccount::release_spent_marks` in key-wallet — and + /// then has nowhere else to put it: `superseded_by` need not be + /// wallet-relevant at all, so it can spend our coin while paying only + /// external addresses and never appear anywhere else in this + /// wallet's event stream. A consumer mirroring wallet state to disk + /// cannot recompute this set from the deleted `txids` alone — it + /// would have to know which of their inputs a *different*, + /// possibly-invisible transaction also claims — so guessing either + /// re-credits a coin the chain has already spent or leaves a + /// genuinely free one stranded as spent forever. Wallet-scoped + /// rather than attributed per removed transaction: a consumer holds + /// every input of every transaction it deletes here, so it only + /// needs to know which of them came free, not which removal freed + /// which. + released_outpoints: Vec, /// Wallet balance after the removal. balance: WalletCoreBalance, /// Post-event balance **snapshots** for accounts whose balance @@ -412,14 +433,16 @@ impl fmt::Display for WalletEvent { WalletEvent::TransactionsSwept { txids, superseded_by, + released_outpoints, balance, account_balances, .. } => write!( f, - "TransactionsSwept(count={}, superseded_by={}, balance={}, account_balances={})", + "TransactionsSwept(count={}, superseded_by={}, released={}, balance={}, account_balances={})", txids.len(), superseded_by, + released_outpoints.len(), balance, format_account_balances(account_balances), ), diff --git a/key-wallet-manager/src/lib.rs b/key-wallet-manager/src/lib.rs index 13e7b1c2b..46954b353 100644 --- a/key-wallet-manager/src/lib.rs +++ b/key-wallet-manager/src/lib.rs @@ -102,6 +102,13 @@ pub struct CheckTransactionsResult { /// — a consumer mirroring wallet state must delete these rows, since no /// other signal on the bus reports a removal. pub per_wallet_swept: BTreeMap>, + /// Outpoints released as a side effect of `per_wallet_swept`, grouped by + /// wallet: inputs the removed transactions claimed to spend that no + /// surviving record spends too. Parallels `per_wallet_swept` rather than + /// folding into it because the two have different owners downstream — + /// see [`crate::events::WalletEvent::TransactionsSwept`] for why a + /// consumer needs this set named explicitly instead of re-deriving it. + pub per_wallet_released_outpoints: BTreeMap>, } impl CheckTransactionsResult { @@ -647,6 +654,11 @@ impl WalletManager { .entry(*wallet_id) .or_default() .extend(check_result.swept_transactions); + result + .per_wallet_released_outpoints + .entry(*wallet_id) + .or_default() + .extend(check_result.released_outpoints); } if !check_result.new_addresses.is_empty() { diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index f889083d7..23864758c 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -88,6 +88,7 @@ impl WalletInterface for WalletM // event: a sweep names the transaction that superseded the // removed ones, and that attribution is lost once the block's // transactions are folded together. + let mut per_wallet_released = check_result.per_wallet_released_outpoints; for (wallet_id, txids) in check_result.per_wallet_swept { if txids.is_empty() { continue; @@ -95,10 +96,12 @@ impl WalletInterface for WalletM let Some(info) = self.wallet_infos.get(&wallet_id) else { continue; }; + let released_outpoints = per_wallet_released.remove(&wallet_id).unwrap_or_default(); let event = WalletEvent::TransactionsSwept { wallet_id, txids, superseded_by: tx.txid(), + released_outpoints, balance: info.balance(), account_balances: BTreeMap::new(), }; @@ -208,6 +211,8 @@ 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. + let mut per_wallet_released = + std::mem::take(&mut check_result.per_wallet_released_outpoints); for (wallet_id, txids) in std::mem::take(&mut check_result.per_wallet_swept) { if txids.is_empty() { continue; @@ -215,10 +220,12 @@ impl WalletInterface for WalletM let Some(info) = self.wallet_infos.get(&wallet_id) else { continue; }; + let released_outpoints = per_wallet_released.remove(&wallet_id).unwrap_or_default(); let event = WalletEvent::TransactionsSwept { wallet_id, txids, superseded_by: tx.txid(), + released_outpoints, balance: info.balance(), account_balances: per_wallet_account_diff .get(&wallet_id) 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 91eac9d5c..c0f086d9c 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -80,6 +80,21 @@ pub(crate) struct AbandonRemoval { pub records: usize, } +/// What [`ManagedCoreFundsAccount::drop_conflicted_transactions`] removed +/// from one account. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub(crate) struct ConflictSweep { + /// Loser txids removed from this account. + pub txids: Vec, + /// Outpoints released from `spent_outpoints` as a side effect: inputs + /// the removed losers claimed that no surviving record claims too. See + /// [`ManagedCoreFundsAccount::release_spent_marks`] — this is exactly + /// its return value, carried out so a caller mirroring wallet state can + /// learn which coins actually came free without redoing the + /// freed-versus-still-spent comparison itself. + pub released_outpoints: Vec, +} + impl ManagedCoreFundsAccount { /// Create a new managed funds account pub fn new(managed_account_type: ManagedAccountType, network: Network) -> Self { @@ -421,13 +436,23 @@ impl ManagedCoreFundsAccount { /// 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) { + /// + /// Returns exactly the outpoints this call released — `freed` minus + /// whatever `still_spent` shows a survivor still claims. That + /// distinction is computed nowhere else: once this returns, freed-and- + /// released and freed-but-retained are indistinguishable in + /// `spent_outpoints` itself, so a caller that needs to tell a + /// persistence mirror which coins are genuinely free again has to catch + /// it here or not at all. + fn release_spent_marks(&mut self, freed: &HashSet) -> HashSet { if freed.is_empty() { - return; + return HashSet::new(); } let still_spent = rebuild_spent_outpoints(&self.keys); + let released: HashSet = freed.difference(&still_spent).copied().collect(); self.spent_outpoints .retain(|outpoint| !freed.contains(outpoint) || still_spent.contains(outpoint)); + released } /// Remove every trace of `abandoned` from this account. @@ -535,14 +560,20 @@ 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 the txids it removed. + /// Returns the txids it removed, together with the outpoints that + /// removal released from `spent_outpoints` (see + /// [`Self::release_spent_marks`]). The latter is not derivable by a + /// caller from the txids alone: the winner that triggers this sweep does + /// not have to be wallet-relevant, so it may hold none of the loser's + /// inputs anywhere the caller can see, and the loser's own record is + /// already gone by the time this returns. pub(crate) fn drop_conflicted_transactions( &mut self, tx: &Transaction, context: &TransactionContext, - ) -> Vec { + ) -> ConflictSweep { if !(context.confirmed() || matches!(context, TransactionContext::InstantSend(_))) { - return Vec::new(); + return ConflictSweep::default(); } let winner = tx.txid(); @@ -577,7 +608,7 @@ impl ManagedCoreFundsAccount { .collect(); if losers.is_empty() { - return Vec::new(); + return ConflictSweep::default(); } // A loser's change may already have funded further unconfirmed @@ -639,12 +670,17 @@ impl ManagedCoreFundsAccount { // 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); + let released = self.release_spent_marks(&freed); if changed { self.keys.bump_monitor_revision(); } - losers.into_iter().collect() + let mut released_outpoints: Vec = released.into_iter().collect(); + released_outpoints.sort_unstable(); + ConflictSweep { + txids: losers.into_iter().collect(), + released_outpoints, + } } /// 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 c138ce994..c72840bd0 100644 --- a/key-wallet/src/transaction_checking/account_checker.rs +++ b/key-wallet/src/transaction_checking/account_checker.rs @@ -13,7 +13,7 @@ use crate::managed_account::managed_account_type::ManagedAccountType; use crate::managed_account::transaction_record::TransactionRecord; use crate::Address; use dashcore::address::Payload; -use dashcore::blockdata::transaction::Transaction; +use dashcore::blockdata::transaction::{OutPoint, Transaction}; use dashcore::hashes::Hash as _; use dashcore::transaction::TransactionPayload; use dashcore::ScriptBuf; @@ -87,6 +87,19 @@ pub struct TransactionCheckResult { /// replay the dead transaction on the next load and re-create the phantom /// balance this removal just cleared. pub swept_transactions: Vec, + /// Outpoints released from the wallet's spent-marks as a side effect of + /// `swept_transactions`: inputs the removed losers claimed to spend that + /// no surviving record spends too. Empty whenever `swept_transactions` + /// is. + /// + /// A consumer mirroring wallet state needs this named explicitly rather + /// than inferring it from the deleted records: the winner that triggered + /// the sweep does not have to be wallet-relevant at all (it can spend our + /// coin and pay only external addresses), so it may never appear + /// anywhere else in this wallet's output, leaving no other way to learn + /// which of a loser's inputs are genuinely free again versus still + /// claimed by a surviving transaction. + pub released_outpoints: Vec, } /// Enum representing the type of Core account that matched with embedded data @@ -417,6 +430,7 @@ impl ManagedAccountCollection { new_records: Vec::new(), updated_records: Vec::new(), swept_transactions: Vec::new(), + released_outpoints: 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 f16bfdb5d..1282ee270 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -90,10 +90,12 @@ impl WalletTransactionChecker for ManagedWalletInfo { // to `record_observed_spends` above for the same reason it is // unconditional. if update_state && (context.confirmed() || context.is_instant_send()) { - result.swept_transactions = self.sweep_conflicts(tx, &context); - if !result.swept_transactions.is_empty() { + let sweep = self.sweep_conflicts(tx, &context); + if !sweep.is_empty() { result.state_modified = true; } + result.swept_transactions = sweep.txids; + result.released_outpoints = sweep.released_outpoints; } if !update_state || !result.is_relevant { @@ -2235,6 +2237,10 @@ mod tests { 0, "and its change must stop counting as confirmed money" ); + assert!( + result.released_outpoints.is_empty(), + "the ordinary case: the winner spends the loser's only input, so nothing is freed" + ); } /// Pooled funding puts a loser's change in an account the winner never @@ -2727,21 +2733,30 @@ mod tests { .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; + let result = 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)); + // The event carries exactly what was released: B, and not A — A is + // the winner's own input, still spent on chain by `winner` itself. + assert_eq!( + result.released_outpoints, + vec![coin_b], + "the sweep must name B as released and must not name A" + ); + // 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. diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index 8e3842588..49790ae5f 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -61,6 +61,28 @@ fn collect_spenders_of_records( } } +/// What [`ManagedWalletInfo::sweep_conflicts`] removed from the wallet: the +/// union, across every account swept, of a single [`ConflictSweep`]. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct WalletConflictSweep { + /// Loser txids removed, deduplicated — one transaction can be recorded + /// in several accounts, so the per-account results overlap. + pub txids: Vec, + /// Outpoints released from `spent_outpoints` across every account swept, + /// deduplicated. Wallet-scoped rather than attributed per loser: a + /// caller mirroring wallet state holds every input of every loser it + /// deletes, so it only needs to know which of them came free, not which + /// loser freed which. + pub released_outpoints: Vec, +} + +impl WalletConflictSweep { + /// Whether the sweep removed anything. + pub fn is_empty(&self) -> bool { + self.txids.is_empty() + } +} + impl ManagedWalletInfo { /// Drop the outputs of every recorded transaction that `tx` provably beat /// to one of its inputs, across the whole wallet. @@ -80,21 +102,33 @@ impl ManagedWalletInfo { /// 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(); + /// Also returns the outpoints released as a side effect, for the same + /// reason: the winner that triggered this sweep is not guaranteed to + /// appear anywhere else the caller can see, so it cannot re-derive which + /// of a loser's inputs are genuinely free again. + pub fn sweep_conflicts( + &mut self, + tx: &Transaction, + context: &TransactionContext, + ) -> WalletConflictSweep { + let mut result = WalletConflictSweep::default(); for account in self.accounts.all_accounts_mut() { if let ManagedAccountRefMut::Funds(funds) = account { - swept.extend(funds.drop_conflicted_transactions(tx, context)); + let swept = funds.drop_conflicted_transactions(tx, context); + result.txids.extend(swept.txids); + result.released_outpoints.extend(swept.released_outpoints); } } - if !swept.is_empty() { + if !result.txids.is_empty() { self.update_balance(); // One transaction can be recorded in several accounts, so the // per-account results overlap. - swept.sort_unstable(); - swept.dedup(); + result.txids.sort_unstable(); + result.txids.dedup(); + result.released_outpoints.sort_unstable(); + result.released_outpoints.dedup(); } - swept + result } /// Whether any account holds `txid` as settled by the network. From 292875dc578b25bb9081a3a2bb1f29f3ad11f512 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:43:42 +0300 Subject: [PATCH 2/5] fix(key-wallet): withhold a released outpoint another account still claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each account decides what it released from its own records alone — `release_spent_marks` rebuilds the retained set from that account's transactions — while a loser is removed from every account it was recorded in. Pooled funding separates the two: the loser's change lands in an account that knows nothing about the coins it spent, so when that account removes it, nothing there retains those coins and it reports them free. Unioning the per-account answers then carried the mistake out of the wallet, telling a mirror a coin was spendable while a surviving record still claimed it. Re-check the union against every account's surviving inputs before returning it. This is only about records that outlived the sweep — the winner that triggered it is already handled inside `drop_conflicted_transactions`, which withholds the inputs it spends, and must, since on the checker path the sweep runs before the winner is recorded anywhere. Also drops an intra-doc link to `ConflictSweep` from `WalletConflictSweep`: that type is `pub(crate)` in another module, so rustdoc could not resolve it and the documentation build failed on `-D warnings`. --- .../transaction_checking/wallet_checker.rs | 130 ++++++++++++++++++ .../src/wallet/managed_wallet_info/helpers.rs | 37 ++++- 2 files changed, 166 insertions(+), 1 deletion(-) diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 1282ee270..c953656be 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2768,6 +2768,136 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.confirmed(), 499_000, "B plus the winner's change"); } + /// A loser is removed from every account it was recorded in, and each of + /// those accounts decides what it released from its own records alone. An + /// account that never recorded the transaction still claiming one of the + /// loser's inputs therefore sees nothing retaining that coin and calls it + /// free — so the wallet-level union has to re-check the released set + /// against every account before reporting it. + /// + /// Reachable through pooled funding: the loser's change lands in a second + /// account, which is where its removal reports the release, while the + /// surviving claim on that coin lives back in the funding account. + #[tokio::test] + async fn test_a_released_outpoint_another_account_still_claims_is_withheld() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // The BIP32 account is where the loser's change will land, which is + // what gets the loser recorded in an account that knows nothing about + // the coins it spent. + let bip32_xpub = ctx + .wallet + .accounts + .standard_bip32_accounts + .get(&0) + .expect("default options create BIP32 account 0") + .account_xpub; + let bip32_change = ctx + .managed_wallet + .first_bip32_managed_account_mut() + .expect("BIP32 account") + .next_receive_address(Some(&bip32_xpub), true) + .expect("BIP32 address"); + + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..2, &[500_000, 400_000]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + 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, paying its change into the BIP32 account. + let loser = spend(vec![coin_a, coin_b], &bip32_change, 99_000, 800_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + // A second unconfirmed transaction also claims B. Neither is final, so + // neither sweeps the other, and this one survives what follows. + let rival_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let rival = spend(vec![coin_b], &rival_change, 50_000, 340_000); + ctx.check_transaction(&rival, TransactionContext::Mempool).await; + + // The winner takes A and confirms, sweeping the loser — but not the + // rival, which shares no input with it. + 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); + let result = 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().transactions().contains_key(&loser.txid()), + "sanity: the loser was swept" + ); + assert!( + ctx.bip44_account().transactions().contains_key(&rival.txid()), + "sanity: the rival is unconfirmed but shares no input with the winner" + ); + assert!(!result.released_outpoints.contains(&coin_a), "A is the winner's own input"); + assert!( + !result.released_outpoints.contains(&coin_b), + "B is still claimed by the rival, whichever account noticed" + ); + } + /// 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 diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index 49790ae5f..c2641f98e 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -62,7 +62,7 @@ fn collect_spenders_of_records( } /// What [`ManagedWalletInfo::sweep_conflicts`] removed from the wallet: the -/// union, across every account swept, of a single [`ConflictSweep`]. +/// union, across every account swept, of the per-account `ConflictSweep`s. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct WalletConflictSweep { /// Loser txids removed, deduplicated — one transaction can be recorded @@ -81,6 +81,40 @@ impl WalletConflictSweep { pub fn is_empty(&self) -> bool { self.txids.is_empty() } + + /// Drop outpoints some surviving record elsewhere in the wallet still + /// spends. + /// + /// Each account decides what it released from its own records alone + /// (`release_spent_marks` rebuilds the retained set from that account's + /// transactions), and a loser is removed from every account it was + /// recorded in. Pooled funding puts those accounts and the spender of a + /// given coin in different places: an account that removed a loser but + /// never recorded the transaction still claiming one of its inputs sees + /// nothing retaining that coin and reports it free. Unioning the + /// per-account answers then carries that mistake out of the wallet. + /// + /// Re-checking against every account's surviving records is the only + /// view that can settle it. Note this does not need to cover the winner + /// that triggered the sweep: `drop_conflicted_transactions` already + /// withholds the inputs it spends, which it must, since on the checker + /// path the sweep runs before the winner is recorded anywhere. + fn retain_unclaimed( + &mut self, + accounts: &crate::managed_account::managed_account_collection::ManagedAccountCollection, + ) { + if self.released_outpoints.is_empty() { + return; + } + let claimed: BTreeSet = accounts + .all_accounts() + .into_iter() + .flat_map(|account| account.transactions().values()) + .flat_map(|record| record.transaction.input.iter()) + .map(|input| input.previous_output) + .collect(); + self.released_outpoints.retain(|outpoint| !claimed.contains(outpoint)); + } } impl ManagedWalletInfo { @@ -127,6 +161,7 @@ impl ManagedWalletInfo { result.txids.dedup(); result.released_outpoints.sort_unstable(); result.released_outpoints.dedup(); + result.retain_unclaimed(&self.accounts); } result } From 58a36b6f9f9e138507eb4078de251d448e11a344 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:49:22 +0300 Subject: [PATCH 3/5] test(key-wallet-manager): cover the sweep event at the manager level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TransactionsSwept` had no manager-level test at all, so neither the per-wallet aggregation in `check_transactions` nor either emission site in `process_block` was exercised — the sweep's own coverage stops at the account layer, which never builds the event. Drive a block whose transaction beats a recorded mempool spend and assert the emitted event: the beaten txid, the transaction it is attributed to, and a released set holding only the coin the winner did not take. The last of those is the half a consumer cannot recompute, which makes it the part worth pinning where it is actually assembled. --- key-wallet-manager/src/event_tests.rs | 105 ++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/key-wallet-manager/src/event_tests.rs b/key-wallet-manager/src/event_tests.rs index 3f8eb4052..40628c567 100644 --- a/key-wallet-manager/src/event_tests.rs +++ b/key-wallet-manager/src/event_tests.rs @@ -533,6 +533,111 @@ async fn test_block_with_index_less_account_tx_carries_account_type() { } } +/// The sweep event's own coverage at the manager level: a block whose +/// transaction beats a recorded mempool spend must emit `TransactionsSwept` +/// naming the removed transaction and the coins its removal freed. +/// +/// The released set is the half a consumer cannot recompute, so it is worth +/// pinning where it is actually assembled — this exercises the per-wallet +/// aggregation in `check_transactions` and the block path's emission +/// together, neither of which the account-level sweep tests reach. +#[tokio::test] +async fn test_block_winner_emits_swept_event_naming_the_released_outpoints() { + let (mut manager, wallet_id, addr) = setup_manager_with_wallet(); + + // One funding transaction pays us twice, so the loser can spend a coin + // the winner does not. + let funding = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: Txid::from_byte_array([0x5a; 32]), + vout: 0, + }, + script_sig: ScriptBuf::new(), + sequence: u32::MAX, + witness: Witness::default(), + }], + output: vec![ + TxOut { + value: 500_000, + script_pubkey: addr.script_pubkey(), + }, + TxOut { + value: 400_000, + script_pubkey: addr.script_pubkey(), + }, + ], + special_transaction_payload: None, + }; + let funding_block = make_block(vec![funding.clone()], 0x5a, 1000); + let wallets = BTreeSet::from([wallet_id]); + manager + .process_block_for_wallets(&funding_block, funding_block.block_hash(), 100, &wallets) + .await; + + let coin_a = OutPoint { + txid: funding.txid(), + vout: 0, + }; + let coin_b = OutPoint { + txid: funding.txid(), + vout: 1, + }; + let spend = |inputs: Vec, value: u64| Transaction { + version: 2, + lock_time: 0, + input: inputs + .into_iter() + .map(|previous_output| TxIn { + previous_output, + script_sig: ScriptBuf::new(), + sequence: u32::MAX, + witness: Witness::default(), + }) + .collect(), + output: vec![TxOut { + value, + script_pubkey: addr.script_pubkey(), + }], + special_transaction_payload: None, + }; + + let loser = spend(vec![coin_a, coin_b], 800_000); + manager.process_mempool_transaction(&loser, None).await; + + // Subscribe only now: the funding block and the loser's arrival are + // setup, and the sweep is what this test is about. + let mut rx = manager.subscribe_events(); + + let winner = spend(vec![coin_a], 400_000); + let winner_block = make_block(vec![winner.clone()], 0x5b, 1100); + manager + .process_block_for_wallets(&winner_block, winner_block.block_hash(), 101, &wallets) + .await; + + let events = drain_events(&mut rx); + let swept = events + .iter() + .find_map(|event| match event { + WalletEvent::TransactionsSwept { + wallet_id: wid, + txids, + superseded_by, + released_outpoints, + .. + } => Some((wid, txids, superseded_by, released_outpoints)), + _ => None, + }) + .unwrap_or_else(|| panic!("a sweep must be emitted, got {:?}", events)); + + assert_eq!(swept.0, &wallet_id); + assert_eq!(swept.1, &vec![loser.txid()], "the beaten transaction is named"); + assert_eq!(swept.2, &winner.txid(), "attributed to the transaction that beat it"); + assert_eq!(swept.3, &vec![coin_b], "only the coin the winner did not take is released"); +} + #[tokio::test] async fn test_empty_block_for_idle_wallet_emits_nothing() { let (mut manager, wallet_id, _addr) = setup_manager_with_wallet(); From f93d28d9dc077bd232ac692dc0704bb2d1fe15b8 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:29:17 +0300 Subject: [PATCH 4/5] fix(key-wallet): make the released-outpoint invariants hold explicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups, all of them about the same thing: the released set is carried on the strength of an invariant nothing states or checks. `WalletConflictSweep::is_empty` looked only at `txids`. It decides whether wallet state was modified, so a release that ever stopped riding along with a removal would stop marking the wallet dirty — silently, surfacing much later as a coin still marked spent after a restart. Check both fields. Both `TransactionsSwept` emission sites take the released map apart entry by entry and never look at what is left. Today nothing is left, because a wallet with released outpoints always has swept txids; if that ever stops holding, those coins are dropped on the floor and stay marked spent forever. A `debug_assert!` after each loop turns that into a test failure instead. `retain_unclaimed` built the wallet's entire spent-input set to check a handful of candidates. Scan per candidate and stop at the first claim: the released set is small and bounded by one sweep, while the set it was being checked against grows with the whole transaction history. Adds the FFI dispatch coverage the description admitted was missing — `null`/`0` for an empty release (the ordinary resend, so the common case), and the marshalled values otherwise, including that `balance` still reads as `balance` after the new parameters were inserted ahead of it. That is the one layer where a mistake corrupts memory instead of failing an assertion. Also trims the released-set rationale where it was restated nearly verbatim. `key-wallet` cannot intra-doc-link up to `WalletEvent::TransactionsSwept`, so those two sites stay self-contained, just shorter. --- dash-spv-ffi/src/callbacks.rs | 109 ++++++++++++++++++ key-wallet-manager/src/process_block.rs | 12 ++ .../transaction_checking/account_checker.rs | 15 +-- .../src/wallet/managed_wallet_info/helpers.rs | 37 +++--- 4 files changed, 153 insertions(+), 20 deletions(-) diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index b98dda37e..7a2abf355 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -1359,6 +1359,115 @@ mod tests { use std::collections::{BTreeMap, BTreeSet}; use std::sync::atomic::{AtomicU32, Ordering}; + /// A sweep with nothing released must hand the callback `null` and `0`, + /// not a dangling pointer into an empty `Vec`. + /// + /// This is the layer where getting it wrong does not fail an assertion: + /// a consumer reading `count` first sees zero and stops, but one that + /// dereferences the pointer defensively would read whatever the empty + /// allocation points at. The ordinary resend — the winner takes every + /// input the loser named — releases nothing, so this is the common case, + /// not the edge one. + #[test] + fn test_transactions_swept_dispatch_passes_null_when_nothing_released() { + static RELEASED_PTR_WAS_NULL: AtomicU32 = AtomicU32::new(u32::MAX); + static RELEASED_COUNT: AtomicU32 = AtomicU32::new(u32::MAX); + + extern "C" fn cb( + _wallet_id: *const c_char, + _txids: *const [u8; 32], + _txids_count: usize, + _superseded_by: *const [u8; 32], + released: *const FFIOutPoint, + released_count: usize, + _balance: *const FFIBalance, + _account_balances: *const FFIAccountBalance, + _account_balances_count: u32, + _user: *mut c_void, + ) { + RELEASED_PTR_WAS_NULL.store(u32::from(released.is_null()), Ordering::SeqCst); + RELEASED_COUNT.store(released_count as u32, Ordering::SeqCst); + } + + let callbacks = FFIWalletEventCallbacks { + on_transactions_swept: Some(cb), + ..FFIWalletEventCallbacks::default() + }; + + callbacks.dispatch(&WalletEvent::TransactionsSwept { + wallet_id: [7u8; 32], + txids: vec![Txid::from_byte_array([1u8; 32])], + superseded_by: Txid::from_byte_array([2u8; 32]), + released_outpoints: Vec::new(), + balance: WalletCoreBalance::default(), + account_balances: BTreeMap::new(), + }); + + assert_eq!(RELEASED_PTR_WAS_NULL.load(Ordering::SeqCst), 1, "expected a null pointer"); + assert_eq!(RELEASED_COUNT.load(Ordering::SeqCst), 0); + } + + /// The released outpoints must arrive intact and in order, and the + /// parameters after them must not be shifted by their insertion — the + /// balance a consumer reads has to be the balance, not an outpoint. + #[test] + fn test_transactions_swept_dispatch_marshals_released_outpoints() { + static FIRST_TXID: std::sync::Mutex<[u8; 32]> = std::sync::Mutex::new([0u8; 32]); + static VOUTS: std::sync::Mutex> = std::sync::Mutex::new(Vec::new()); + static CONFIRMED: AtomicU32 = AtomicU32::new(u32::MAX); + + extern "C" fn cb( + _wallet_id: *const c_char, + _txids: *const [u8; 32], + _txids_count: usize, + _superseded_by: *const [u8; 32], + released: *const FFIOutPoint, + released_count: usize, + balance: *const FFIBalance, + _account_balances: *const FFIAccountBalance, + _account_balances_count: u32, + _user: *mut c_void, + ) { + assert!(!released.is_null()); + let entries = unsafe { std::slice::from_raw_parts(released, released_count) }; + *FIRST_TXID.lock().expect("txid") = entries[0].txid; + *VOUTS.lock().expect("vouts") = entries.iter().map(|e| e.vout).collect(); + CONFIRMED.store(unsafe { (*balance).confirmed } as u32, Ordering::SeqCst); + } + + let callbacks = FFIWalletEventCallbacks { + on_transactions_swept: Some(cb), + ..FFIWalletEventCallbacks::default() + }; + + let parent = Txid::from_byte_array([9u8; 32]); + callbacks.dispatch(&WalletEvent::TransactionsSwept { + wallet_id: [7u8; 32], + txids: vec![Txid::from_byte_array([1u8; 32])], + superseded_by: Txid::from_byte_array([2u8; 32]), + released_outpoints: vec![ + dashcore::OutPoint { + txid: parent, + vout: 3, + }, + dashcore::OutPoint { + txid: parent, + vout: 7, + }, + ], + balance: WalletCoreBalance::new(123_456, 0, 0, 0), + account_balances: BTreeMap::new(), + }); + + assert_eq!(*FIRST_TXID.lock().expect("txid"), *parent.as_byte_array()); + assert_eq!(*VOUTS.lock().expect("vouts"), vec![3, 7]); + assert_eq!( + CONFIRMED.load(Ordering::SeqCst), + 123_456, + "the balance parameter must not be shifted by the released-outpoint insertion" + ); + } + /// `BlocksNeeded` dispatch must pass exactly one entry per /// `FilterMatchKey` to the FFI callback (i.e. iterate keys, not /// inflated by the per-block wallet attribution). diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index 23864758c..1520b60da 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -107,6 +107,12 @@ impl WalletInterface for WalletM }; self.emit_event(event); } + debug_assert!( + per_wallet_released.is_empty(), + "released outpoints for a wallet that emitted no sweep would be \ + dropped here, stranding those coins marked spent forever: {:?}", + per_wallet_released + ); } self.finalize_block_advance( @@ -234,6 +240,12 @@ impl WalletInterface for WalletM }; self.emit_event(event); } + debug_assert!( + per_wallet_released.is_empty(), + "released outpoints for a wallet that emitted no sweep would be dropped \ + here, stranding those coins marked spent forever: {:?}", + per_wallet_released + ); if let Some(lock) = instant_lock { for (wallet_id, records) in per_wallet_updated_records { diff --git a/key-wallet/src/transaction_checking/account_checker.rs b/key-wallet/src/transaction_checking/account_checker.rs index c72840bd0..9e71f3143 100644 --- a/key-wallet/src/transaction_checking/account_checker.rs +++ b/key-wallet/src/transaction_checking/account_checker.rs @@ -92,13 +92,14 @@ pub struct TransactionCheckResult { /// no surviving record spends too. Empty whenever `swept_transactions` /// is. /// - /// A consumer mirroring wallet state needs this named explicitly rather - /// than inferring it from the deleted records: the winner that triggered - /// the sweep does not have to be wallet-relevant at all (it can spend our - /// coin and pay only external addresses), so it may never appear - /// anywhere else in this wallet's output, leaving no other way to learn - /// which of a loser's inputs are genuinely free again versus still - /// claimed by a surviving transaction. + /// Named explicitly because it cannot be inferred downstream: the winner + /// that triggered the sweep need not be wallet-relevant, so it may never + /// appear in this wallet's output at all. See + /// [`ManagedCoreFundsAccount::drop_conflicted_transactions`], which + /// computes the distinction. + /// + /// [`ManagedCoreFundsAccount::drop_conflicted_transactions`]: + /// crate::managed_account::ManagedCoreFundsAccount pub released_outpoints: Vec, } diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index c2641f98e..908a181b8 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -77,9 +77,16 @@ pub struct WalletConflictSweep { } impl WalletConflictSweep { - /// Whether the sweep removed anything. + /// Whether the sweep changed nothing. + /// + /// Both fields are checked even though only a removal can free an + /// outpoint today, so the second can never be non-empty on its own. + /// Callers use this to decide whether wallet state was modified, and a + /// release that stopped riding along with a removal would otherwise stop + /// marking the wallet dirty — silently, and only visible later as a coin + /// still marked spent after a restart. pub fn is_empty(&self) -> bool { - self.txids.is_empty() + self.txids.is_empty() && self.released_outpoints.is_empty() } /// Drop outpoints some surviving record elsewhere in the wallet still @@ -99,6 +106,11 @@ impl WalletConflictSweep { /// that triggered the sweep: `drop_conflicted_transactions` already /// withholds the inputs it spends, which it must, since on the checker /// path the sweep runs before the winner is recorded anywhere. + /// + /// Scans the records per candidate and stops at the first claim rather + /// than building the wallet's whole spent-input set: a sweep frees a + /// handful of coins at most, while the set it would be checked against + /// grows with the entire transaction history. fn retain_unclaimed( &mut self, accounts: &crate::managed_account::managed_account_collection::ManagedAccountCollection, @@ -106,14 +118,14 @@ impl WalletConflictSweep { if self.released_outpoints.is_empty() { return; } - let claimed: BTreeSet = accounts - .all_accounts() - .into_iter() - .flat_map(|account| account.transactions().values()) - .flat_map(|record| record.transaction.input.iter()) - .map(|input| input.previous_output) - .collect(); - self.released_outpoints.retain(|outpoint| !claimed.contains(outpoint)); + let accounts = accounts.all_accounts(); + self.released_outpoints.retain(|outpoint| { + !accounts.iter().any(|account| { + account.transactions().values().any(|record| { + record.transaction.input.iter().any(|input| input.previous_output == *outpoint) + }) + }) + }); } } @@ -137,9 +149,8 @@ impl ManagedWalletInfo { /// learn those rows are gone — nothing else in the event surface reports /// a removal, and a mirror that misses it replays the dead transaction. /// Also returns the outpoints released as a side effect, for the same - /// reason: the winner that triggered this sweep is not guaranteed to - /// appear anywhere else the caller can see, so it cannot re-derive which - /// of a loser's inputs are genuinely free again. + /// reason: the winner is not guaranteed to appear anywhere the caller can + /// see, so the set cannot be re-derived from the txids. pub fn sweep_conflicts( &mut self, tx: &Transaction, From 31b81a69e35e908eb82b7ebf13aab78fd6d4ad4e Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:51:33 +0300 Subject: [PATCH 5/5] fix(key-wallet): never report a swept transaction's own output as released MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The descendant closure removes transactions that spent a loser's own change, and their inputs land in `freed` like any other — including the ones pointing at a removed loser's own output. Nothing filtered those out, so an ordinary chained resend (build A, spend A's change in B before A confirms, then have a winner beat A on its original input) reported A's change outpoint as released. That is not a coin becoming spendable. A is being deleted precisely because it can never confirm, so telling a mirror to mark its output spendable re-credits money that does not exist — the class of bug the sweep exists to remove, reintroduced through the set meant to prevent it. Filtered out of the reported set rather than out of `freed`, so the internal release is unchanged and this stays a reporting fix: an outpoint of a dead transaction is dead weight in `spent_outpoints` either way. Also documents a pre-existing limitation the same review surfaced. `release_spent_marks` decides what stays spent from live records, and a chainlocked record is pruned to its txid under the default features, so a second spend of an already-pruned chainlocked coin — recorded only if it arrived after the pruning, since otherwise the chainlocked arrival would have swept it — can have that coin reported released when it is spent on chain. The inputs of a pruned record survive nowhere else, so it cannot be resolved at this layer; naming it beats leaving it implicit. --- key-wallet-manager/src/events.rs | 13 ++ .../managed_core_funds_account.rs | 14 +- .../transaction_checking/wallet_checker.rs | 128 ++++++++++++++++++ 3 files changed, 154 insertions(+), 1 deletion(-) diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 5355bd023..8bb336ac9 100644 --- a/key-wallet-manager/src/events.rs +++ b/key-wallet-manager/src/events.rs @@ -258,6 +258,19 @@ pub enum WalletEvent { /// every input of every transaction it deletes here, so it only /// needs to know which of them came free, not which removal freed /// which. + /// + /// One pre-existing limitation, inherited from `release_spent_marks` + /// rather than introduced with this field: it decides what stays + /// spent from the wallet's *live* records, and under the default + /// `keep-finalized-transactions = off` a chainlocked record is pruned + /// to just its txid. So if this wallet ever recorded a second spend + /// of a coin an already-pruned chainlocked transaction took — which + /// needs that second spend to arrive after the pruning, since + /// otherwise the chainlocked arrival would have swept it — and that + /// second spend is later swept on a different input, the coin is + /// reported released though it is spent on chain. The inputs of a + /// pruned record survive nowhere else, so this cannot be resolved at + /// this layer. released_outpoints: Vec, /// Wallet balance after the removal. balance: WalletCoreBalance, 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 c0f086d9c..3a3158ea2 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -675,7 +675,19 @@ impl ManagedCoreFundsAccount { self.keys.bump_monitor_revision(); } - let mut released_outpoints: Vec = released.into_iter().collect(); + // Report only coins that outlive this sweep. The descendant closure + // above removes transactions that spent a loser's *own* change, so + // `freed` holds outpoints belonging to the losers themselves — and + // those are not coins becoming spendable, they are outputs of + // transactions being deleted for never being able to confirm. + // Telling a mirror to mark one spendable re-credits money that does + // not exist, which is the class of bug this sweep exists to remove. + // + // Filtered here rather than out of `freed`, so the internal release + // is unchanged: an outpoint of a dead transaction is dead weight in + // `spent_outpoints` either way, and this stays a reporting change. + let mut released_outpoints: Vec = + released.into_iter().filter(|outpoint| !losers.contains(&outpoint.txid)).collect(); released_outpoints.sort_unstable(); ConflictSweep { txids: losers.into_iter().collect(), diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index c953656be..efea71a5f 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2768,6 +2768,134 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.confirmed(), 499_000, "B plus the winner's change"); } + /// A loser's change may already have funded further unconfirmed + /// transactions, and the sweep removes those too — that is the descendant + /// closure `drop_conflicted_transactions` walks. Their inputs land in + /// `freed` like any other, including the ones pointing at a removed + /// loser's own output. + /// + /// Such an outpoint must not be reported released. It is not a coin + /// becoming spendable: it is an output of a transaction being deleted for + /// never being able to confirm, so telling a mirror to mark it spendable + /// re-credits money that does not exist — the exact class of bug the + /// sweep exists to remove. + #[tokio::test] + async fn test_a_swept_descendants_claim_on_its_parents_output_is_not_released() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // Two confirmed coins: X, which the winner will take, and C, which + // only the descendant spends. + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..2, &[500_000, 400_000]); + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + + let coin_x = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let coin_c = 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 parent spends X and pays itself change. + let parent_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let parent = spend(vec![coin_x], &parent_change, 99_000, 400_000); + ctx.check_transaction(&parent, TransactionContext::Mempool).await; + let parent_change_outpoint = OutPoint { + txid: parent.txid(), + vout: 1, + }; + + // The descendant spends that change plus C — the ordinary shape of + // chaining a second spend before the first confirms. + let child_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let child = spend(vec![parent_change_outpoint, coin_c], &child_change, 89_000, 400_000); + ctx.check_transaction(&child, TransactionContext::Mempool).await; + + // The winner takes X and confirms, sweeping the parent and, through + // the descendant closure, the child. + 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_x], &winner_change, 99_000, 400_000); + 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.swept_transactions.contains(&child.txid()), + "sanity: the descendant is swept with its parent" + ); + assert!( + !result.released_outpoints.contains(&parent_change_outpoint), + "an output of a transaction being deleted is not a coin becoming \ + spendable, got {:?}", + result.released_outpoints + ); + assert_eq!( + result.released_outpoints, + vec![coin_c], + "only the real coin the descendant spent is released" + ); + } + /// A loser is removed from every account it was recorded in, and each of /// those accounts decides what it released from its own records alone. An /// account that never recorded the transaction still claiming one of the