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..7a2abf355 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 @@ -1316,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/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(); diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 803f3984e..8bb336ac9 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,40 @@ 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. + /// + /// 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, /// Post-event balance **snapshots** for accounts whose balance @@ -412,14 +446,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..1520b60da 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,15 +96,23 @@ 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(), }; 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( @@ -208,6 +217,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 +226,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) @@ -227,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/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 91eac9d5c..3a3158ea2 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,29 @@ 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() + // 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(), + 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..9e71f3143 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,20 @@ 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. + /// + /// 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, } /// Enum representing the type of Core account that matched with embedded data @@ -417,6 +431,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..efea71a5f 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. @@ -2753,6 +2768,264 @@ 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 + /// 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 8e3842588..908a181b8 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -61,6 +61,74 @@ fn collect_spenders_of_records( } } +/// What [`ManagedWalletInfo::sweep_conflicts`] removed from the wallet: the +/// 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 + /// 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 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.released_outpoints.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. + /// + /// 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, + ) { + if self.released_outpoints.is_empty() { + return; + } + 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) + }) + }) + }); + } +} + impl ManagedWalletInfo { /// Drop the outputs of every recorded transaction that `tx` provably beat /// to one of its inputs, across the whole wallet. @@ -80,21 +148,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 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, + 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(); + result.retain_unclaimed(&self.accounts); } - swept + result } /// Whether any account holds `txid` as settled by the network.