From 19c02bb77ecd257121461c440c54e54f53e608c3 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:43:46 -0400 Subject: [PATCH 1/2] fix(key-wallet): emit the removals the islock and abandon paths already performed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in one family: wallet state changes that no event reports, so a consumer mirroring the wallet to disk diverges from it and replays the difference on its next load. 1. `mark_instant_send_utxos` ran `sweep_conflicts` and kept only a bool, discarding the `WalletConflictSweep`. `process_instant_send_lock` could therefore emit only `TransactionInstantLocked`, never `TransactionsSwept`, although the sweep had deleted the loser's record and freed its coins. This is the live common path — dash-spv routes an islock for an already-tracked mempool transaction here, which is the ordinary transaction-first ordering. The mirror kept the dead row and left the released coins marked spent forever. The sweep is now carried out through a named `InstantSendLockOutcome` and emitted before the lock event, matching the ordering the block and mempool paths already use. 2. The abandon path had the same shape: `apply_abandon` discarded `release_spent_marks`' return, `AbandonOutcome` had nowhere to put it, and `WalletManager::abandon_transaction` emitted nothing at all. Released outpoints are now reconciled across accounts and carried on the outcome, and the abandon emits `TransactionsSwept`. An abandon has no competing transaction, so `superseded_by` carries the abandoned root; both the event and the FFI callback now document that and the two paths that produce a removal. 3. Released outpoints were never put back into the in-core `utxos` map. `update_utxos` is the only insert site and it runs on a new sighting, which re-delivery of a known funding transaction is not — for a finalized one `confirm_transaction` returns before reaching it. So the event told consumers a coin was spendable again while this library's own coin selection could not spend it. The coin is now rebuilt from the funding transaction's own retained record — the sweep removes the loser, not what funded it — so nothing is invented: exact `TxOut`, and the account's own classification of that output as ours. Restoring a coin is the dangerous direction, so it is gated on proof rather than absence of evidence: the funding record must be held by the account restoring it and classify the output as received or change; the outpoint must survive the cross-account reconciliation, must not still be spent-marked, and must not appear in `observed_spent_outpoints` — a block spend we could not attribute leaves no record for `release_spent_marks` to answer from, and re-crediting one of those would hand coin selection a coin the chain has already spent (#649). `is_trusted` and `is_locked` are left false rather than guessed; both err toward understating what is spendable. One residual, stated plainly: a funding transaction already pruned to its txid by a chainlock keeps no output to rebuild from, so its coins stay absent until a rescan re-fetches the block. That is bounded by `keep-finalized-transactions` and is the pre-existing posture, now pinned by a test that asserts both feature configurations. Co-Authored-By: Claude Opus 4.8 --- dash-spv-ffi/src/callbacks.rs | 18 +- key-wallet-manager/src/event_tests.rs | 216 +++++++++++++ key-wallet-manager/src/events.rs | 39 ++- key-wallet-manager/src/lib.rs | 49 ++- key-wallet-manager/src/process_block.rs | 36 ++- .../managed_core_funds_account.rs | 202 ++++++++++-- .../transaction_checking/wallet_checker.rs | 290 +++++++++++++++--- .../src/wallet/managed_wallet_info/helpers.rs | 199 +++++++++--- .../src/wallet/managed_wallet_info/mod.rs | 2 +- .../wallet_info_interface.rs | 62 +++- 10 files changed, 968 insertions(+), 145 deletions(-) diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index 7a2abf355..79c0bb6ed 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -776,9 +776,11 @@ impl FFIOutPoint { /// Callback for `WalletEvent::TransactionsSwept`. /// -/// Fires when the wallet removes transactions that a later, final transaction -/// provably beat to one of their inputs: they can never confirm, so their -/// outputs are gone from the UTXO set and their records deleted. +/// Fires when the wallet removes transactions that can never confirm — either +/// because a later, final transaction provably beat them to one of their +/// inputs, or because the root was explicitly abandoned as never having +/// reached the network and the removal cascaded to everything built on its +/// change. Their outputs are gone from the UTXO set and their records deleted. /// /// **The only removal-shaped wallet callback.** Every other one is additive, /// so a consumer mirroring wallet state to disk must act on this — delete the @@ -786,8 +788,14 @@ impl FFIOutPoint { /// rows in the mirror, which replays them on the next load and re-creates a /// balance the wallet has already corrected. /// -/// `txids` points to `txids_count` consecutive 32-byte txids. -/// `superseded_by` is the transaction whose arrival settled the inputs. +/// `txids` points to `txids_count` consecutive 32-byte txids. On the abandon +/// path this is what the abandon asked for, which may include a txid the +/// wallet held no record for — a mirror can hold rows the load path never +/// restored, so the delete is driven by the requested set. +/// `superseded_by` is the transaction whose arrival settled the inputs. On the +/// abandon path no such transaction exists and this repeats the abandoned +/// root, which therefore also appears in `txids`; never assume it names a row +/// that survives. /// `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 diff --git a/key-wallet-manager/src/event_tests.rs b/key-wallet-manager/src/event_tests.rs index 40628c567..f71aa63f3 100644 --- a/key-wallet-manager/src/event_tests.rs +++ b/key-wallet-manager/src/event_tests.rs @@ -1163,6 +1163,222 @@ async fn test_instant_send_lock_event_does_not_carry_addresses_derived_field() { } } +/// An InstantSend lock settles the locked transaction's inputs, so it beats a +/// recorded competing spend exactly as a block does — and the removal has to +/// reach consumers. +/// +/// This is the live ordering for the ordinary case: dash-spv routes a lock +/// arriving for an already-tracked mempool transaction straight to +/// `process_instant_send_lock`, which is a different code path from the one a +/// first sighting that already carries its lock takes. That path emitted only +/// the additive `TransactionInstantLocked` while the sweep quietly deleted the +/// loser's record and freed its coins, so a mirror kept the dead row, replayed +/// it on the next load, and left the released coin marked spent forever. +#[tokio::test] +async fn test_instant_send_lock_emits_swept_event_before_the_lock_event() { + 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 — that second coin is what gets released. + let funding = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: Txid::from_byte_array([0x7a; 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()], 0x7a, 2000); + let wallets = BTreeSet::from([wallet_id]); + manager + .process_block_for_wallets(&funding_block, funding_block.block_hash(), 200, &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, + }; + + // Both competing spends sit in the mempool, neither final, so neither + // sweeps the other yet. + let loser = spend(vec![coin_a, coin_b], 880_000); + manager.process_mempool_transaction(&loser, None).await; + let winner = spend(vec![coin_a], 480_000); + manager.process_mempool_transaction(&winner, None).await; + + // Subscribe only now: the setup above is not what this test is about. + let mut rx = manager.subscribe_events(); + + // The lock arrives for the already-tracked winner — the transition that + // goes through `process_instant_send_lock`, not through the checker. + let lock = InstantLock { + txid: winner.txid(), + cyclehash: CycleHash::from_byte_array([0x1a; 32]), + signature: BLSSignature::from([0x2b; 96]), + ..InstantLock::default() + }; + manager.process_instant_send_lock(lock); + + let events = drain_events(&mut rx); + let swept_index = events + .iter() + .position(|event| matches!(event, WalletEvent::TransactionsSwept { .. })) + .unwrap_or_else(|| { + panic!("the lock swept a conflict, so a removal must be emitted, got {:?}", events) + }); + let locked_index = events + .iter() + .position(|event| matches!(event, WalletEvent::TransactionInstantLocked { .. })) + .unwrap_or_else(|| panic!("expected TransactionInstantLocked, got {:?}", events)); + // Removal before the additive event, as on the block and mempool paths: a + // consumer applying them in order must not have a delete land on top of + // something that came after it. + assert!( + swept_index < locked_index, + "the removal must precede the lock event, got {:?}", + events + ); + + match &events[swept_index] { + WalletEvent::TransactionsSwept { + wallet_id: wid, + txids, + superseded_by, + released_outpoints, + .. + } => { + assert_eq!(*wid, wallet_id); + assert_eq!(txids, &vec![loser.txid()], "the beaten transaction is named"); + assert_eq!( + *superseded_by, + winner.txid(), + "attributed to the transaction the lock settled" + ); + assert_eq!( + released_outpoints, + &vec![coin_b], + "only the coin the winner did not take is released" + ); + } + _ => unreachable!(), + } +} + +/// Abandoning a transaction is a removal like any other, and consumers learn +/// about removals from exactly one event. Without it a mirror keeps rows for +/// transactions this wallet has decided never existed and replays them on the +/// next load, re-creating the phantom balance the abandon just removed. +#[tokio::test] +async fn test_abandon_emits_swept_event_with_released_outpoints() { + let (mut manager, wallet_id, addr) = setup_manager_with_wallet(); + + // A real, confirmed coin. + let funding = create_tx_paying_to(&addr, 0x8a); + let funding_block = make_block(vec![funding.clone()], 0x8a, 2100); + let wallets = BTreeSet::from([wallet_id]); + manager + .process_block_for_wallets(&funding_block, funding_block.block_hash(), 300, &wallets) + .await; + let funding_outpoint = OutPoint { + txid: funding.txid(), + vout: 0, + }; + + // A spend of it that never reached the network. Its change is credited + // and, as a trusted self-send, counted confirmed and spendable. + let dead = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: u32::MAX, + witness: Witness::default(), + }], + output: vec![TxOut { + value: TX_AMOUNT - 1_000, + script_pubkey: addr.script_pubkey(), + }], + special_transaction_payload: None, + }; + manager.process_mempool_transaction(&dead, None).await; + + let mut rx = manager.subscribe_events(); + let outcome = manager + .abandon_transaction(&wallet_id, dead.txid(), &BTreeMap::new()) + .expect("the wallet exists"); + assert!(outcome.abandoned.contains(&dead.txid())); + + 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, + balance, + .. + } => Some((wid, txids, superseded_by, released_outpoints, balance)), + _ => None, + }) + .unwrap_or_else(|| panic!("an abandon must emit a removal, got {:?}", events)); + + assert_eq!(swept.0, &wallet_id); + assert_eq!(swept.1, &vec![dead.txid()], "the abandoned transaction is named"); + // No competing transaction exists — that is what an abandon is — so the + // root stands in as its own provenance. + assert_eq!(swept.2, &dead.txid()); + assert_eq!( + swept.3, + &vec![funding_outpoint], + "the coin the dead transaction claimed comes free" + ); + // And the balance carried is the post-abandon one: the phantom change is + // gone, the real coin is back. + assert_eq!(swept.4.confirmed(), TX_AMOUNT); + assert_eq!(swept.4.spendable(), TX_AMOUNT); +} + // --------------------------------------------------------------------------- // ChainLock path // --------------------------------------------------------------------------- diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 8bb336ac9..8e39aaf17 100644 --- a/key-wallet-manager/src/events.rs +++ b/key-wallet-manager/src/events.rs @@ -222,10 +222,17 @@ pub enum WalletEvent { /// full balance after the change — not a delta. account_balances: BTreeMap, }, - /// Transactions were removed from the wallet: each was a recorded spend - /// that a later, final transaction provably beat to one of its inputs, so - /// it can never confirm. Their outputs are gone from the UTXO set and - /// their records deleted. + /// Transactions were removed from the wallet because they can never + /// confirm. Their outputs are gone from the UTXO set and their records + /// deleted. Two paths produce this: + /// + /// * **Swept** — a later, final transaction (in a block, or InstantSend + /// locked) provably beat them to one of their inputs. + /// * **Abandoned** — the caller asserted the root never reached the + /// network, via [`crate::WalletManager::abandon_transaction`], and the + /// removal cascaded to everything built on its change. There is no + /// competing transaction in this case, by construction: it is the one + /// the sweep cannot reach. /// /// The only removal-shaped event on this bus. A consumer mirroring wallet /// state to disk must act on it — every other variant is additive, so @@ -235,8 +242,22 @@ pub enum WalletEvent { /// ID of the affected wallet. wallet_id: WalletId, /// Transactions removed. Delete these rows and any UTXO they created. + /// + /// On the abandon path this is everything the abandon *asked* for, + /// which can include a txid the in-memory wallet held no record for. + /// That is deliberate: a mirror can hold rows the load path never + /// restored — that asymmetry is exactly why + /// `abandon_transaction_with_spends` takes an external spend view — + /// so the delete has to be driven by the requested set, not by what + /// happened to be in memory. txids: Vec, /// The transaction whose arrival settled the inputs, for provenance. + /// + /// On the abandon path there is no such transaction, and this carries + /// the abandoned root itself — which therefore also appears in + /// `txids`. Consumers must not assume it names a row that survives, or + /// indeed any row at all: on the sweep path it need not be + /// wallet-relevant either (see `released_outpoints`). superseded_by: Txid, /// Outpoints the sweep released: inputs the removed transactions /// claimed to spend that no surviving record spends too (a loser @@ -271,6 +292,16 @@ pub enum WalletEvent { /// 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. + /// + /// The wallet re-credits these coins to its own UTXO set as it emits + /// them, so "spendable again" means the same thing on both sides of + /// this event. One case cannot be: a funding transaction pruned to + /// its txid by a chainlock keeps no `TxOut` to rebuild the coin from, + /// so it stays absent from coin selection until a rescan re-fetches + /// the block, even though it is named here. Consumers are unaffected + /// — their own record of the coin is what they restore — but a caller + /// reading this library's balance back will see the difference. See + /// `ManagedCoreFundsAccount::recredit_released_outpoints`. released_outpoints: Vec, /// Wallet balance after the removal. balance: WalletCoreBalance, diff --git a/key-wallet-manager/src/lib.rs b/key-wallet-manager/src/lib.rs index 46954b353..ee505c85d 100644 --- a/key-wallet-manager/src/lib.rs +++ b/key-wallet-manager/src/lib.rs @@ -718,6 +718,16 @@ impl WalletManager { /// are refused, but the judgement otherwise belongs to the caller that /// owns broadcast policy. /// + /// Emits [`WalletEvent::TransactionsSwept`] naming the abandoned + /// transactions and the coins their removal freed, so a consumer mirroring + /// wallet state deletes the same rows. That is the only removal-shaped + /// event on the bus; without it the mirror keeps records for transactions + /// this wallet has decided never existed and replays them on its next + /// load, re-creating the phantom balance the abandon just removed. + /// `superseded_by` carries `root` — an abandon has no competing + /// transaction to attribute the removal to, which is precisely why it + /// exists (see the field's documentation). + /// /// Returns `None` when the wallet is unknown. pub fn abandon_transaction( &mut self, @@ -725,18 +735,39 @@ impl WalletManager { root: Txid, external_spends: &BTreeMap, ) -> Option { + // Snapshot before the abandon so the event can carry the diff: the + // cached per-account balances are still the pre-abandon ones here. + let prior = self.wallet_infos.get(wallet_id)?.account_balances(); + let info = self.get_wallet_info_mut(wallet_id)?; let outcome = info.abandon_transaction_with_spends(root, external_spends); - if !outcome.is_empty() { - info.update_balance(); - tracing::info!( - %root, - abandoned = outcome.abandoned.len(), - records_removed = outcome.records_removed, - utxos_removed = outcome.utxos_removed, - "Abandoned a dead transaction and everything built on it" - ); + if outcome.is_empty() { + return Some(outcome); } + info.update_balance(); + tracing::info!( + %root, + abandoned = outcome.abandoned.len(), + records_removed = outcome.records_removed, + utxos_removed = outcome.utxos_removed, + released_outpoints = outcome.released_outpoints.len(), + "Abandoned a dead transaction and everything built on it" + ); + + // Read the post-abandon balances out while the borrow is still live, + // so the emit below needs no second lookup that could fail after the + // wallet has already been mutated. + let balance = info.balance(); + let account_balances = events::diff_account_balances(&prior, &info.account_balances()); + self.emit_event(WalletEvent::TransactionsSwept { + wallet_id: *wallet_id, + txids: outcome.abandoned.iter().copied().collect(), + superseded_by: root, + released_outpoints: outcome.released_outpoints.clone(), + balance, + account_balances, + }); + Some(outcome) } diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index 1520b60da..09eea1dc7 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -11,6 +11,7 @@ use key_wallet::account::AccountType; use key_wallet::managed_account::transaction_record::TransactionRecord; use key_wallet::transaction_checking::{BlockInfo, DerivedAddressInfo, TransactionContext}; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use key_wallet::wallet::managed_wallet_info::WalletConflictSweep; use key_wallet::WalletCoreBalance; use std::collections::{BTreeMap, BTreeSet}; use tokio::sync::broadcast; @@ -419,10 +420,23 @@ impl WalletInterface for WalletM > = self.wallet_infos.iter().map(|(id, info)| (*id, info.account_balances())).collect(); let mut affected_wallets = Vec::new(); + // An IS lock settles the locked transaction's inputs, so it can beat a + // recorded competing spend just as a block does — and this is the live + // path for the ordinary ordering, transaction first and lock second + // (dash-spv routes an islock on an already-tracked mempool tx here). + // The removal has to reach consumers: every other event on this bus is + // additive, so a mirror that never hears it keeps the dead row and + // replays it on the next load, with the coins the sweep freed left + // marked spent forever. + let mut per_wallet_sweep: BTreeMap = BTreeMap::new(); for (wallet_id, info) in self.wallet_infos.iter_mut() { - if info.mark_instant_send_utxos(&txid, &instant_lock) { + let outcome = info.mark_instant_send_utxos(&txid, &instant_lock); + if outcome.changed { info.update_balance(); affected_wallets.push(*wallet_id); + if !outcome.sweep.is_empty() { + per_wallet_sweep.insert(*wallet_id, outcome.sweep); + } } } @@ -436,6 +450,20 @@ impl WalletInterface for WalletM }; let prior = prior_account_balances.remove(&wallet_id).unwrap_or_default(); let account_balances = diff_account_balances(&prior, &info.account_balances()); + // Removal before the additive event, matching the block and + // mempool paths: a consumer applying these in order sees the dead + // rows deleted first, so nothing that follows can be clobbered by + // a delete arriving after it. + if let Some(sweep) = per_wallet_sweep.remove(&wallet_id) { + self.emit_event(WalletEvent::TransactionsSwept { + wallet_id, + txids: sweep.txids, + superseded_by: txid, + released_outpoints: sweep.released_outpoints, + balance: info.balance(), + account_balances: account_balances.clone(), + }); + } self.emit_event(WalletEvent::TransactionInstantLocked { wallet_id, txid, @@ -444,6 +472,12 @@ impl WalletInterface for WalletM account_balances, }); } + debug_assert!( + per_wallet_sweep.is_empty(), + "a sweep for a wallet that reported no change would be dropped here, \ + stranding its released coins marked spent forever: {:?}", + per_wallet_sweep + ); } async fn describe(&self) -> String { 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 ca3210bb6..6c6301362 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -71,13 +71,24 @@ pub struct ManagedCoreFundsAccount { } /// What [`ManagedCoreFundsAccount::apply_abandon`] removed from one account. -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +#[derive(Debug, Clone, Default, PartialEq, Eq)] pub(crate) struct AbandonRemoval { /// UTXOs the abandoned transactions had contributed. pub utxos: usize, /// Transaction records actually dropped — a txid the account never held /// removes nothing. pub records: usize, + /// Outpoints released from `spent_outpoints` as a side effect: inputs the + /// abandoned transactions claimed that no surviving record claims too. + /// Exactly [`ManagedCoreFundsAccount::release_spent_marks`]' return value, + /// minus outpoints belonging to the abandoned transactions themselves — + /// those are outputs of records being deleted for never having existed, + /// not coins becoming spendable again. + /// + /// Carried out for the same reason [`ConflictSweep`] carries its own: the + /// freed-versus-still-spent distinction is computed nowhere else, so a + /// caller mirroring wallet state cannot recover it afterwards. + pub released_outpoints: Vec, } /// What [`ManagedCoreFundsAccount::drop_conflicted_transactions`] removed @@ -459,16 +470,15 @@ impl ManagedCoreFundsAccount { /// /// Drops the outputs those transactions contributed and their records, and /// releases the outpoints they spent from `spent_outpoints` so the coins - /// become eligible for rediscovery. + /// become spendable again. /// - /// The released parents are deliberately **not** re-inserted into `utxos`. - /// `update_utxos` discards the `Utxo` when it removes a spent parent, and - /// `InputDetail` keeps only index/value/address, so the flags that decide - /// which balance bucket a restored coin belongs in are not retained - /// anywhere. Inventing them would be a guess. What these coins genuinely - /// are is unspent on chain — the abandoned transaction never reached the - /// network — so the correct source of truth is a rescan, which releasing - /// them from `spent_outpoints` now permits. + /// The released outpoints are *reported*, not re-credited here. Whether a + /// coin one account released is genuinely free is a wallet-level question + /// — a surviving record in a sibling account may still claim it — so the + /// re-credit runs once, from + /// [`ManagedWalletInfo::abandon_transaction_with_spends`], after the + /// per-account answers have been reconciled. See + /// [`Self::recredit_released_outpoints`]. /// /// Reservations are deliberately left alone. A recorded transaction has /// already handed its inputs from the ephemeral set to `spent_outpoints` @@ -499,8 +509,18 @@ impl ManagedCoreFundsAccount { freed.extend(record.transaction.input.iter().map(|input| input.previous_output)); } } + let mut released_outpoints = Vec::new(); if records > 0 { - self.release_spent_marks(&freed); + // Same filter the conflict sweep applies: an outpoint belonging to + // one of the abandoned transactions is an output of a record being + // deleted for never having existed, not a coin coming free. + // Reporting it would re-credit money that never was. + released_outpoints = self + .release_spent_marks(&freed) + .into_iter() + .filter(|outpoint| !abandoned.contains(&outpoint.txid)) + .collect(); + released_outpoints.sort_unstable(); } if utxos > 0 { @@ -509,7 +529,139 @@ impl ManagedCoreFundsAccount { AbandonRemoval { utxos, records, + released_outpoints, + } + } + + /// Re-credit the coins `released` names, for each one this account can + /// prove it owns and still holds unspent. + /// + /// A sweep or an abandon removes the record that claimed a coin and drops + /// the coin's spent-mark, but nothing puts the coin back in `utxos`: + /// `update_utxos` is the only insert site and it runs on a *new sighting*, + /// which a re-delivery of an already-known funding transaction is not. + /// Left there, `WalletEvent::TransactionsSwept` tells consumers the coin is + /// spendable again while this library's own coin selection cannot see it — + /// the wallet and its mirror disagree, in the direction that strands funds. + /// + /// The coin is rebuilt from the funding transaction's own retained record, + /// never invented. The sweep removes only the loser, so the parent record + /// is normally still here, and it carries the exact `TxOut` plus this + /// account's own classification of that output. Every field that decides + /// which balance bucket the coin lands in comes from there: + /// + /// * `txout` — `record.transaction.output[vout]`, exact script and value. + /// * `address` — the `OutputDetail` this account built for that index. + /// * `height` / `is_coinbase` / `is_confirmed` / `is_instantlocked` — the + /// parent record's context and transaction, the same sources + /// `update_utxos` reads. + /// + /// Two flags are not recoverable and are left at their defaults rather + /// than guessed, both conservatively: + /// + /// * `is_trusted` — `update_utxos` derives it from a wallet-wide view of + /// the parent's *own* inputs, which are gone from `utxos` by now. + /// `false` files an unconfirmed coin under `unconfirmed` instead of + /// `confirmed`; understating a balance is the safe direction, and a + /// confirmed parent does not depend on the flag at all. + /// * `is_locked` — a user-set spending lock that lived on the removed + /// `Utxo` and nowhere else. `false` matches what a rescan re-delivering + /// the funding transaction would produce, so this does not introduce a + /// new divergence. + /// + /// Ownership is proven, not assumed: the funding record must be held by + /// *this* account and must classify the output as + /// [`OutputRole::Received`] or [`OutputRole::Change`]. That is this + /// account's own answer, computed when the transaction was recorded, so a + /// pooled transaction paying several accounts re-credits each output + /// exactly once, in the account that owns it. Outpoints whose funding + /// record this account does not hold are skipped in silence — another + /// account may own them, and the caller runs this over all of them. + /// + /// Skips anything already in `utxos` (nothing to restore) and anything + /// still in `spent_outpoints` (a surviving record here claims it, so + /// `release_spent_marks` deliberately kept the mark). + /// + /// **Residual.** A funding transaction that was chainlock-finalized keeps + /// only its txid under the default `keep-finalized-transactions = off`: + /// its record — and with it the `TxOut` — is gone, so its coins cannot be + /// rebuilt and stay absent until a rescan deep enough to re-fetch the + /// block, which is above this layer. Since Dash chainlocks within a block + /// or two, that is the normal posture for older coins. This is the one + /// case where the event still outruns in-core state, and it is bounded by + /// the same feature flag that causes it. + /// + /// Returns the outpoints actually re-credited. + pub(crate) fn recredit_released_outpoints(&mut self, released: &[OutPoint]) -> Vec { + if released.is_empty() { + return Vec::new(); + } + // Only account types that hold coins at all. Mirrors `update_utxos`, + // which is a no-op for everything else, so this cannot introduce a + // UTXO into an account that would never have had one. + if !matches!( + self.keys.managed_account_type(), + ManagedAccountType::Standard { .. } + | ManagedAccountType::CoinJoin { .. } + | ManagedAccountType::DashpayReceivingFunds { .. } + | ManagedAccountType::DashpayExternalAccount { .. } + ) { + return Vec::new(); + } + + let mut recredited = Vec::new(); + for outpoint in released { + if self.utxos.contains_key(outpoint) || self.spent_outpoints.contains(outpoint) { + continue; + } + let Some(record) = self.keys.transactions().get(&outpoint.txid) else { + continue; + }; + // This account's own verdict on that output, from when it recorded + // the transaction. Anything but a coin of ours is not ours to + // restore. + let Some(detail) = + record.output_details.iter().find(|detail| detail.index == outpoint.vout).filter( + |detail| matches!(detail.role, OutputRole::Received | OutputRole::Change), + ) + else { + continue; + }; + let Some(address) = detail.address.clone() else { + continue; + }; + let Some(output) = record.transaction.output.get(outpoint.vout as usize) else { + continue; + }; + debug_assert_eq!( + detail.value, output.value, + "output detail and transaction disagree on the value of {}", + outpoint + ); + + let txout = dashcore::TxOut { + value: output.value, + script_pubkey: output.script_pubkey.clone(), + }; + let height = record.context.block_info().map_or(0, |info| info.height); + let mut utxo = + Utxo::new(*outpoint, txout, address, height, record.transaction.is_coin_base()); + utxo.is_confirmed = record.context.confirmed(); + utxo.is_instantlocked = matches!(record.context, TransactionContext::InstantSend(_)); + self.utxos.insert(*outpoint, utxo); + recredited.push(*outpoint); + + tracing::info!( + %outpoint, + funding_txid = %outpoint.txid, + "Re-credited a coin freed by a removed spend" + ); + } + + if !recredited.is_empty() { + self.keys.bump_monitor_revision(); } + recredited } /// Drop the outputs of any recorded unconfirmed transaction that `tx` @@ -539,20 +691,24 @@ impl ManagedCoreFundsAccount { /// actually spent them. /// /// A loser may also spend inputs the winner does not. Those coins are - /// freed from `spent_outpoints` below, but they cannot be re-credited - /// here: `update_utxos` discarded their `Utxo` — and its flags — when the - /// loser was recorded, and `InputDetail` keeps only index/value/address. - /// The release is what makes them recoverable: a rescan re-delivering the - /// funding transaction inserts them again. Until that rescan they are - /// absent from the balance. + /// freed from `spent_outpoints` below and reported in the result, but they + /// are not re-credited *here*: whether a coin one account released is + /// genuinely free is a wallet-level question, since a surviving record in + /// a sibling account may still claim it. The re-credit therefore runs once + /// from [`ManagedWalletInfo::sweep_conflicts`], after the per-account + /// answers have been reconciled — see + /// [`Self::recredit_released_outpoints`], which rebuilds each coin from + /// its funding transaction's own retained record. /// /// That recovery has a boundary worth knowing. A funding transaction that - /// was chainlock-finalized keeps only its txid, so `has_transaction` stays - /// true and re-delivery is not a new sighting — `confirm_transaction` - /// returns before `update_utxos`, the only production insert site, and the - /// coin does not come back. Recovering it needs a rescan deep enough to - /// re-fetch the block, which is above this layer. Since Dash chainlocks - /// within a block or two, that is the normal posture for older coins. + /// was chainlock-finalized keeps only its txid under the default + /// `keep-finalized-transactions = off`, so its `TxOut` is gone and the + /// coin cannot be rebuilt from anything the wallet still holds; nor can + /// re-delivery insert it, since `has_transaction` stays true and + /// `confirm_transaction` returns before `update_utxos`, the only other + /// insert site. Recovering it needs a rescan deep enough to re-fetch the + /// block, which is above this layer. Since Dash chainlocks within a block + /// or two, that is the normal posture for older coins. /// /// Scope: account-local. A loser recorded here has its outputs dropped /// here; a loser whose change landed in a *different* account is not diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index efea71a5f..ae59bb4c5 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2069,14 +2069,20 @@ mod tests { assert_eq!(ctx.managed_wallet.balance.spendable(), change_amount); } - /// The rescan recovery above has a boundary: a funding transaction that - /// was chainlock-finalized keeps only its txid, so re-delivering it is not - /// a new sighting and never reaches the only production UTXO insert site. - /// The coin stays absent. Documented rather than fixed — recovering it - /// needs a rescan deep enough to re-fetch the block, which is above this - /// layer. + /// The in-core restore above has one boundary, and it is the pruning of + /// finalized records, not the restore itself. + /// + /// Under the default `keep-finalized-transactions = off` a + /// chainlock-finalized funding transaction keeps only its txid: its + /// `TxOut` is gone, so the released coin cannot be rebuilt from anything + /// the wallet still holds, and re-delivery cannot insert it either + /// (`has_transaction` stays true, so `confirm_transaction` returns before + /// `update_utxos`). The coin stays absent until a rescan deep enough to + /// re-fetch the block, which is above this layer. With the feature on + /// nothing is pruned and the restore works normally — which is what pins + /// the pruning as the cause. #[tokio::test] - async fn test_rescan_recovery_does_not_reach_a_finalized_funding_transaction() { + async fn test_restore_cannot_reach_a_chainlock_pruned_funding_record() { let mut ctx = TestWalletContext::new_random(); let external_address = Address::p2pkh( &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), @@ -2123,16 +2129,33 @@ mod tests { }; ctx.check_transaction(&spend, TransactionContext::Mempool).await; - ctx.managed_wallet.abandon_transaction(spend.txid()); + let outcome = ctx.managed_wallet.abandon_transaction(spend.txid()); ctx.managed_wallet.update_balance(); - // Re-delivering the funding block does not bring the coin back. + // Either way the coin is *reported* free: the release is decided from + // the spent-mark bookkeeping, which pruning does not touch. + let funding_outpoint = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + assert_eq!(outcome.released_outpoints, vec![funding_outpoint]); + + // Re-delivering the funding block cannot help either — this is the + // path the restore used to depend on. ctx.check_transaction(&funding_tx, finalized).await; + + #[cfg(not(feature = "keep-finalized-transactions"))] assert_eq!( ctx.managed_wallet.balance.confirmed(), 0, - "a finalized funding record blocks the redelivery path this \ - recovery depends on" + "a chainlock-pruned funding record keeps no output to rebuild the \ + coin from, and blocks the redelivery path too" + ); + #[cfg(feature = "keep-finalized-transactions")] + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 1_000_000, + "with the record retained there is nothing to stop the restore" ); } @@ -2661,11 +2684,17 @@ mod tests { } /// A loser can spend inputs the winner does not. Sweeping it frees those - /// coins from the spent set, but their `Utxo` values were discarded when - /// the loser was recorded — so the sweep alone cannot put them back, and - /// the coins must be recoverable by the rescan the release enables. + /// coins from the spent set — and must put them back in the UTXO set in + /// the same breath. + /// + /// The sweep reports them released, which tells a consumer to mark them + /// spendable again; if this library's own coin selection could not see + /// them until some later rescan, the two would disagree about the same + /// coin, in the direction that strands funds. Nothing has to be invented + /// to avoid that: the sweep removes the *loser*, so the funding + /// transaction's record — and with it the exact `TxOut` — is still here. #[tokio::test] - async fn test_a_swept_losers_extra_input_is_recoverable_by_rescan() { + async fn test_a_swept_losers_extra_input_is_recredited_in_core() { let mut ctx = TestWalletContext::new_random(); let external_address = Address::p2pkh( &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), @@ -2744,10 +2773,23 @@ mod tests { ) .await; - // The loser is gone, and B is not credited — its `Utxo` was - // discarded when the loser was recorded and cannot be invented. + // The loser is gone, and B is credited again — rebuilt from the + // funding transaction's own retained record, which the sweep never + // touched. assert!(!ctx.bip44_account().transactions().contains_key(&loser.txid())); - assert!(!ctx.bip44_account().utxos.contains_key(&coin_b)); + let restored = ctx + .bip44_account() + .utxos + .get(&coin_b) + .expect("the sweep must put the loser's extra input back"); + assert_eq!(restored.txout.value, 400_000, "rebuilt from the funding output, not guessed"); + assert_eq!(restored.address, ctx.receive_address, "and from its own address"); + assert!(restored.is_confirmed, "the funding transaction is in a block"); + assert!(!restored.is_locked, "a restored coin is selectable"); + assert!( + !ctx.bip44_account().utxos.contains_key(&coin_a), + "A is the winner's own input and stays spent" + ); // 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. @@ -2757,15 +2799,157 @@ mod tests { "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. + // The balance agrees with the UTXO set: coin selection can spend B + // again, without waiting for a rescan. + assert_eq!(ctx.managed_wallet.balance.confirmed(), 499_000, "B plus the winner's change"); + assert_eq!( + ctx.managed_wallet.balance.spendable(), + 499_000, + "the released coin has to be spendable, not merely reported free" + ); + + // Re-delivering the funding block changes nothing — the restore is + // idempotent, not a race against the rescan it used to depend on. ctx.check_transaction(&funding_tx, funding_context).await; + assert!(ctx.bip44_account().utxos.contains_key(&coin_b)); + assert_eq!(ctx.managed_wallet.balance.confirmed(), 499_000); + } + + /// The restore must not undo dashpay/rust-dashcore#649. + /// + /// A block spend of our coin that we could not attribute — the spender + /// pays only external addresses, and during an out-of-order rescan the + /// funding transaction had not been processed yet, so nothing matched — + /// leaves no record for `release_spent_marks` to answer from. It reports + /// the coin free, because from the live records it is. `update_utxos` + /// already refuses to credit such an output; the restore has to refuse + /// too, or coin selection is handed a coin the chain has spent. + #[tokio::test] + async fn test_the_restore_withholds_a_coin_seen_spent_in_a_block() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // The funding transaction pays us twice, but is not delivered yet. + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..2, &[500_000, 400_000]); + let coin_a = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let coin_b = OutPoint { + txid: funding_tx.txid(), + vout: 1, + }; + let spend = |inputs: Vec, change: Option<&Address>, 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: match change { + Some(change) => vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: 99_000, + script_pubkey: change.script_pubkey(), + }, + ], + None => vec![TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }], + }, + special_transaction_payload: None, + }; + + // Out-of-order rescan: the block that spends B arrives first. Nothing + // of ours matches — B is not in `utxos` yet and the outputs are + // external — but the spend is remembered. + let thief = spend(vec![coin_b], None, 390_000); + let thief_result = ctx + .check_transaction( + &thief, + TransactionContext::InBlock(BlockInfo::new( + 99, + BlockHash::from_slice(&[9u8; 32]).expect("hash"), + 1_699_999_000, + )), + ) + .await; + assert!(!thief_result.is_relevant, "the precondition: we cannot attribute this spend"); assert!( - ctx.bip44_account().utxos.contains_key(&coin_b), - "a rescan must be able to rediscover the loser's extra input" + ctx.managed_wallet.observed_spent_outpoints().contains_key(&coin_b), + "the precondition: #649 remembered the spend" + ); + + // Now the funding block arrives. A is credited; B is not — #649. + ctx.check_transaction( + &funding_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + assert!(ctx.bip44_account().utxos.contains_key(&coin_a)); + assert!(!ctx.bip44_account().utxos.contains_key(&coin_b), "#649 withheld it"); + + // A loser claims both coins, then a winner takes A and confirms, + // sweeping the loser. B is freed from the spent bookkeeping — no live + // record claims it — so the sweep reports it released. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend(vec![coin_a, coin_b], Some(&loser_change), 800_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + let winner_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let winner = spend(vec![coin_a], Some(&winner_change), 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.released_outpoints.contains(&coin_b), + "the precondition: from the live records alone B looks free" + ); + + // The load-bearing assertion: reported free, still not credited. + assert!( + !ctx.bip44_account().utxos.contains_key(&coin_b), + "a coin seen spent in a block must never be restored to coin selection" + ); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 99_000, + "only the winner's change; B contributes nothing" ); - 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 @@ -3024,6 +3208,22 @@ mod tests { !result.released_outpoints.contains(&coin_b), "B is still claimed by the rival, whichever account noticed" ); + // And the withholding has to hold for the UTXO set too, not just the + // report. Re-crediting a coin a surviving record still spends would + // hand coin selection a guaranteed double spend — the reason the + // restore runs only over the wallet-reconciled released set. + let bip32_account = + ctx.managed_wallet.first_bip32_managed_account().expect("BIP32 account"); + for account in [ctx.bip44_account(), bip32_account] { + assert!( + !account.utxos.contains_key(&coin_a), + "A must not be re-credited: the winner spent it on chain" + ); + assert!( + !account.utxos.contains_key(&coin_b), + "B must not be re-credited: the rival still claims it" + ); + } } /// An InstantSend lock is final, so it settles the winner's inputs just as @@ -3249,23 +3449,37 @@ mod tests { tx.txid() ); } - assert!(ctx.bip44_account().utxos.is_empty(), "no phantom output may survive the cascade"); + // Every phantom output is gone, and the one real coin the chain + // consumed is back — rebuilt from the funding transaction's own + // retained record, which the abandon never touched. The chain never + // reached the network, so nothing ever spent that coin. + let funding_outpoint = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + assert_eq!( + ctx.bip44_account().utxos.keys().collect::>(), + vec![&funding_outpoint], + "only the real funding coin may survive the cascade" + ); + assert_eq!( + outcome.released_outpoints, + vec![funding_outpoint], + "and the outcome must name it, so a persistence mirror follows" + ); // The load-bearing assertion. Trusted self-send change is bucketed as - // *confirmed*, so `unconfirmed() == 0` holds before the abandon too - // and proves nothing on its own. + // *confirmed*, so the phantom counted as confirmed too; the balance + // returning to exactly the funding value is what proves the phantoms + // are gone and the real coin is not. assert_eq!( ctx.managed_wallet.balance.confirmed(), - 0, - "the phantom counts as confirmed, so that is where its absence must show" + funding_value, + "the phantoms counted as confirmed, so that is where their absence must show" ); assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); - // The real coin the chain consumed is released from the spent set, so - // a rescan can rediscover it — but only while its funding record is - // still live. A chainlock-finalized funding transaction keeps just its - // txid, so `has_transaction` stays true, `is_new` stays false, and - // `confirm_transaction` returns before reaching `update_utxos` — the - // only production insert site. See the sibling test below. + // Re-delivering the funding block changes nothing: the coin is + // already back, and the restore does not depend on a rescan. let rediscovered = ctx .check_transaction( &funding_tx, @@ -3277,11 +3491,7 @@ mod tests { ) .await; assert!(rediscovered.is_relevant); - assert_eq!( - ctx.managed_wallet.balance.confirmed(), - funding_value, - "the funding coin comes back on rescan — the chain never spent it on chain" - ); + assert_eq!(ctx.managed_wallet.balance.confirmed(), funding_value); } /// A transaction that loses a race for its inputs can never confirm, so diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index 9e7791e97..d201c9e31 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -23,6 +23,24 @@ pub struct AbandonOutcome { /// How many transaction records were actually dropped. Distinct from /// `abandoned.len()`, which counts what was *asked* for. pub records_removed: usize, + /// Outpoints the abandon released, deduplicated and reconciled across + /// every account: inputs the abandoned transactions claimed that nothing + /// surviving in this wallet claims too. Outpoints belonging to the + /// abandoned transactions themselves are excluded — those are outputs of + /// records being deleted, not coins coming free. + /// + /// The same set, and for the same reason, as + /// [`WalletConflictSweep::released_outpoints`]: a consumer mirroring + /// wallet state has to mark these coins spendable again, and cannot + /// derive the set from `abandoned` alone — that would require knowing + /// which of their inputs some *other* surviving transaction also claims. + /// + /// These coins are also re-credited to this wallet's own UTXO set where + /// their funding record survives, so the event and in-core coin selection + /// agree. See + /// `ManagedCoreFundsAccount::recredit_released_outpoints` for the one + /// case that cannot be — a chainlock-pruned funding record. + pub released_outpoints: Vec, } impl AbandonOutcome { @@ -31,11 +49,61 @@ impl AbandonOutcome { /// `abandoned` always contains the root, whether or not the wallet held /// anything for it, so it cannot answer this on its own — a root the /// wallet never recorded removes nothing. + /// + /// `released_outpoints` is checked too, on the same reasoning + /// [`WalletConflictSweep::is_empty`] gives: only a record removal can free + /// an outpoint today, so it can never be non-empty on its own, but a + /// release that stopped riding along with one would otherwise stop marking + /// the wallet dirty — silently, and visible only later as a coin still + /// marked spent after a restart. pub fn is_empty(&self) -> bool { - self.records_removed == 0 && self.utxos_removed == 0 + self.records_removed == 0 && self.utxos_removed == 0 && self.released_outpoints.is_empty() } } +/// Drop from `released` every outpoint some surviving record in `accounts` +/// 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 removed transaction is dropped 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 record but never +/// held 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. +/// +/// The surviving inputs are collected once and probed by hash, rather than +/// rescanning the records per candidate. The released set is not inherently +/// small: a peer can hand the wallet a transaction whose input vector is as +/// large as it likes and whose output pays an address the wallet owns, and a +/// later final transaction need conflict with only one of those inputs for the +/// rest to become candidates. Scanning per candidate is `O(released × retained +/// history)` against a wallet whose history the peer does not control either — +/// tens of millions of comparisons, run while the manager holds the winner +/// mutably and before the event can even reach persistence. Building the set is +/// one pass over that same history and is never the worse trade: a single +/// candidate already costs a full pass under the alternative. +fn retain_unclaimed_outpoints( + released: &mut Vec, + accounts: &crate::managed_account::managed_account_collection::ManagedAccountCollection, +) { + if released.is_empty() { + return; + } + let claimed: HashSet = accounts + .all_accounts() + .into_iter() + .flat_map(|account| account.transactions().values()) + .flat_map(|record| record.transaction.input.iter()) + .map(|input| input.previous_output) + .collect(); + released.retain(|outpoint| !claimed.contains(outpoint)); +} + /// Txids in `records` that spend an output of anything in `abandoned`. /// /// Settled records are never followed — what they spent was real. An @@ -90,50 +158,18 @@ impl WalletConflictSweep { } /// 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. + /// spends. See [`retain_unclaimed_outpoints`], which this shares with the + /// abandon path. /// - /// The surviving inputs are collected once and probed by hash, rather - /// than rescanning the records per candidate. The released set is not - /// inherently small: a peer can hand the wallet a transaction whose - /// input vector is as large as it likes and whose output pays an address - /// the wallet owns, and a later final transaction need conflict with - /// only one of those inputs for the rest to become candidates. Scanning - /// per candidate is `O(released × retained history)` against a wallet - /// whose history the peer does not control either — tens of millions of - /// comparisons, run while the manager holds the winner mutably and - /// before the event can even reach persistence. Building the set is one - /// pass over that same history and is never the worse trade: a single - /// candidate already costs a full pass under the alternative. + /// 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: HashSet = 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)); + retain_unclaimed_outpoints(&mut self.released_outpoints, accounts); } } @@ -159,6 +195,11 @@ impl ManagedWalletInfo { /// 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. + /// + /// The released coins are re-credited to this wallet's own UTXO set here + /// too, once the per-account answers have been reconciled — the reported + /// set and what coin selection can actually spend must not disagree. See + /// `ManagedCoreFundsAccount::recredit_released_outpoints`. pub fn sweep_conflicts( &mut self, tx: &Transaction, @@ -173,7 +214,6 @@ impl ManagedWalletInfo { } } if !result.txids.is_empty() { - self.update_balance(); // One transaction can be recorded in several accounts, so the // per-account results overlap. result.txids.sort_unstable(); @@ -181,10 +221,59 @@ impl ManagedWalletInfo { result.released_outpoints.sort_unstable(); result.released_outpoints.dedup(); result.retain_unclaimed(&self.accounts); + // Strictly after `retain_unclaimed`: a coin a sibling account's + // surviving record still spends must never be put back, and only + // the reconciled set is safe to act on. + self.recredit_released_outpoints(&result.released_outpoints); + // Last, so the balance reflects both the removals and the + // re-credits. + self.update_balance(); } result } + /// Offer `released` to every funds-bearing account; each takes the coins + /// it can prove are its own. Returns what was actually re-credited across + /// the wallet. + /// + /// An outpoint is owned by at most one account — ownership is decided from + /// the funding record's per-account output classification — so offering + /// the whole set to each account re-credits each coin exactly once. + /// + /// Outpoints this wallet has *seen spent in a block* are withheld, even + /// when the release said they were free. `release_spent_marks` answers + /// from the wallet's own records, and a block spend of our coin that we + /// could not attribute leaves no record to answer from: the spender pays + /// only external addresses and, if the funding transaction had not been + /// processed yet, never even matched (dashpay/rust-dashcore#649 — the same + /// reason `update_utxos` refuses to insert such an output in the first + /// place). Re-crediting one of those would hand coin selection a coin the + /// chain has already spent. The released set is still *reported* as-is: + /// that set's contract is a consumer-facing one with its own documented + /// limits, and narrowing it is a separate change from making in-core state + /// match it — this withholding only ever errs toward understating what we + /// can spend. + fn recredit_released_outpoints(&mut self, released: &[OutPoint]) -> Vec { + if released.is_empty() { + return Vec::new(); + } + let candidates: Vec = released + .iter() + .filter(|outpoint| !self.observed_spent_outpoints.contains_key(outpoint)) + .copied() + .collect(); + if candidates.is_empty() { + return Vec::new(); + } + let mut recredited = Vec::new(); + for account in self.accounts.all_accounts_mut() { + if let ManagedAccountRefMut::Funds(funds) = account { + recredited.extend(funds.recredit_released_outpoints(&candidates)); + } + } + recredited + } + /// Whether any account holds `txid` as settled by the network. /// /// Settled means chainlock-finalized, in a block, **or InstantSend-locked** @@ -235,11 +324,13 @@ impl ManagedWalletInfo { /// judgement belongs to the layer that owns broadcast policy. /// /// The coins the abandoned transactions consumed are released from the - /// spent set so a rescan can rediscover them, rather than being - /// re-credited directly: the `Utxo` removed for a spent parent is - /// discarded by `update_utxos` and `InputDetail` keeps only - /// index/value/address, so the flags that decide a restored coin's - /// balance bucket are not retained anywhere. + /// spent set and re-credited to the UTXO set, rebuilt from their funding + /// transactions' own retained records — the abandon removes the spenders, + /// not what funded them. They are reported in + /// [`AbandonOutcome::released_outpoints`] so a persistence mirror can + /// follow. See `ManagedCoreFundsAccount::recredit_released_outpoints` + /// for the one case that cannot be rebuilt: a funding record already + /// pruned to its txid by a chainlock, which needs a rescan. /// /// Does not recompute the balance — callers batching several abandons /// should run `update_balance` @@ -279,6 +370,7 @@ impl ManagedWalletInfo { abandoned: BTreeSet::new(), utxos_removed: 0, records_removed: 0, + released_outpoints: Vec::new(), }; } let mut abandoned = BTreeSet::from([root]); @@ -307,12 +399,14 @@ impl ManagedWalletInfo { let mut utxos_removed = 0; let mut records_removed = 0; + let mut released_outpoints = Vec::new(); for account in self.accounts.all_accounts_mut() { match account { ManagedAccountRefMut::Funds(funds) => { let removed = funds.apply_abandon(&abandoned); utxos_removed += removed.utxos; records_removed += removed.records; + released_outpoints.extend(removed.released_outpoints); } // Keys-only accounts hold no UTXOs, but they do hold records // — an asset-lock funding transaction is recorded in both its @@ -330,10 +424,23 @@ impl ManagedWalletInfo { } } + // One transaction is recorded in every account it touched, so the + // per-account releases overlap. + released_outpoints.sort_unstable(); + released_outpoints.dedup(); + // A coin another account's surviving record still spends was never + // free; the per-account view cannot see that. Same reconciliation the + // conflict sweep runs, and for the same reason. + retain_unclaimed_outpoints(&mut released_outpoints, &self.accounts); + // Strictly after the reconciliation: only the settled set is safe to + // put back into coin selection. + self.recredit_released_outpoints(&released_outpoints); + AbandonOutcome { abandoned, utxos_removed, records_removed, + released_outpoints, } } // BIP44 Account Helpers diff --git a/key-wallet/src/wallet/managed_wallet_info/mod.rs b/key-wallet/src/wallet/managed_wallet_info/mod.rs index 6bfece722..4b64db797 100644 --- a/key-wallet/src/wallet/managed_wallet_info/mod.rs +++ b/key-wallet/src/wallet/managed_wallet_info/mod.rs @@ -7,7 +7,7 @@ pub mod asset_lock_builder; pub mod coin_selection; pub mod fee; pub mod helpers; -pub use helpers::AbandonOutcome; +pub use helpers::{AbandonOutcome, WalletConflictSweep}; pub mod managed_account_operations; pub mod managed_accounts; pub mod transaction_builder; diff --git a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs index b66293ce0..94f66de22 100644 --- a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs +++ b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs @@ -14,6 +14,7 @@ use crate::transaction_checking::TransactionContext; use crate::transaction_checking::WalletTransactionChecker; use crate::wallet::managed_wallet_info::transaction_building::AccountTypePreference; use crate::wallet::managed_wallet_info::TransactionRecord; +use crate::wallet::managed_wallet_info::WalletConflictSweep; use crate::wallet::ManagedWalletInfo; use crate::{Network, Utxo, Wallet, WalletCoreBalance}; use dashcore::address::Payload; @@ -45,6 +46,28 @@ pub struct ApplyChainLockOutcome { pub metadata_advanced: bool, } +/// Outcome of [`WalletInfoInterface::mark_instant_send_utxos`]. +/// +/// Captures both effects of applying an InstantSend lock so the manager-level +/// emitter (in `key-wallet-manager`) can fire the removal event the sweep +/// earns as well as `WalletEvent::TransactionInstantLocked`. Collapsing this +/// to a bare "something changed" flag is what left the lock path emitting only +/// the additive event while the sweep silently deleted records and freed +/// coins: a consumer mirroring wallet state kept the dead rows and replayed +/// them on its next load. +#[derive(Debug, Clone, Default)] +pub struct InstantSendLockOutcome { + /// Whether wallet state changed in any way — a UTXO newly marked + /// InstantSend-locked, a record's context rewritten, or a conflicting + /// spend swept. Callers use it to decide whether to refresh balances and + /// emit at all. An outgoing transaction can own no UTXOs of ours and + /// still change state, so this is broader than "a UTXO was marked". + pub changed: bool, + /// What the lock's conflict sweep removed, if anything. Empty when the + /// lock settled inputs nothing else claimed — the ordinary case. + pub sweep: WalletConflictSweep, +} + /// Trait that wallet info types must implement to work with WalletManager pub trait WalletInfoInterface: Sized + WalletTransactionChecker + ManagedAccountOperations { /// Create a wallet info from an existing wallet, seeding the sync checkpoint at @@ -267,18 +290,18 @@ pub trait WalletInfoInterface: Sized + WalletTransactionChecker + ManagedAccount new_height: CoreBlockHeight, ) -> Vec; - /// Mark UTXOs for a transaction as InstantSend-locked across all accounts - /// and update the corresponding transaction record context. - /// Returns `true` if any UTXO was newly marked. /// Apply an InstantSend lock: mark the transaction's UTXOs, rewrite its /// record context, and drop any competing spend the lock now settles. /// - /// Returns whether wallet state changed in any of those ways — callers - /// use it to refresh balances and to decide whether to emit - /// `TransactionInstantLocked`. An outgoing transaction can own no UTXOs - /// of ours and still change state by rewriting its context or by the - /// sweep removing a loser, so this is broader than "a UTXO was marked". - fn mark_instant_send_utxos(&mut self, txid: &Txid, lock: &InstantLock) -> bool; + /// Returns both effects — see [`InstantSendLockOutcome`]. The sweep half + /// must be carried out to the caller, not just folded into a changed flag: + /// it names records that were deleted and coins that came free, and + /// nothing else in the event surface reports a removal. + fn mark_instant_send_utxos( + &mut self, + txid: &Txid, + lock: &InstantLock, + ) -> InstantSendLockOutcome; /// Return the aggregated monitor revision across all accounts. /// Increments whenever the monitored address set changes. @@ -583,9 +606,13 @@ impl WalletInfoInterface for ManagedWalletInfo { matured } - fn mark_instant_send_utxos(&mut self, txid: &Txid, lock: &InstantLock) -> bool { + fn mark_instant_send_utxos( + &mut self, + txid: &Txid, + lock: &InstantLock, + ) -> InstantSendLockOutcome { if !self.instant_send_locks.insert(*txid) { - return false; + return InstantSendLockOutcome::default(); } let mut any_changed = false; // Kept for the sweep below: it needs the locked transaction's inputs, @@ -609,15 +636,18 @@ impl WalletInfoInterface for ManagedWalletInfo { // already tracked (`process_instant_send_lock`), and it had no sweep — // the one in `check_core_transaction` is only reachable on a first // sighting that already carries the lock. - let swept = locked_transaction.is_some_and(|tx| { - !self.sweep_conflicts(&tx, &TransactionContext::InstantSend(lock.clone())).is_empty() - }); - if any_changed && !swept { + let sweep = locked_transaction + .map(|tx| self.sweep_conflicts(&tx, &TransactionContext::InstantSend(lock.clone()))) + .unwrap_or_default(); + if any_changed && sweep.is_empty() { // `sweep_conflicts` recomputes on its own when it removes // something, so this only covers the marking-only case. self.update_balance(); } - any_changed || swept + InstantSendLockOutcome { + changed: any_changed || !sweep.is_empty(), + sweep, + } } fn monitor_revision(&self) -> u64 { From bbb92370417c4b61854b653eac64dcd5c71dd11f Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:05:31 -0400 Subject: [PATCH 2/2] fix(key-wallet): make the released-coin re-credit share update_utxos' invariants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-credit added in the previous commit inserts a coin straight into `utxos` but gated only on `observed_spent_outpoints` membership. That is a fresh insert, not a redelivery, and it cleared neither of the two bars the credit path clears. Both gaps are reproduced below, and both reproduce under the default feature set — `keep-finalized-transactions` widens them but is not what causes them. 1. The observed-spent map is bounded state, not a ledger. `prune_finalized_observed_spends` evicts every entry at or below the finality boundary, so beneath it "was this coin spent in a block?" is unanswerable and a miss is not a no. A coin a block spent at height 101 was rebuilt into `utxos` after the eviction and picked by coin selection, which then constructed a spend of a chain-consumed output. The map's safety argument only ever covered redelivery paths, where the funding transaction arriving again is itself the evidence; a re-credit has no such arrival. The re-credit now additionally requires the coin's funding record to sit in a block strictly above `ManagedWalletInfo::spend_proof_horizon`. A spend can never precede the output it spends, so any block spend of such a coin is above the horizon too and would still be in the map for the membership test to find. The horizon is the applied chainlock height, not the prune's own `min(chain_lock, synced_height)`: that boundary describes a single run, while the gate has to bound everything ever evicted across every run. The chainlock height is monotonic — `apply_chain_lock` replaces the stored lock only on a strictly greater height — and every past prune boundary was at most the chainlock height of its moment. `rewind_sync_checkpoint_for_new_account` deliberately drops `synced_height` to just below wallet birth on an account add, which would move a `min()`-derived boundary back down and re-admit coins whose spend records an earlier, higher boundary had already evicted. 2. `update_utxos` refuses to credit the outputs of a transaction a settled spend has doomed — it can never confirm, so its change is money that does not exist — while deliberately keeping the record for history. The re-credit read that record back as a funding source and materialised the change of a never-confirmable transaction: 500,000 phantom duffs, and spendable. `doomed_by_a_settled_spend` becomes a shared function and is now applied to the funding record before any of its outputs is rebuilt. Coins failing either gate are withheld with a `tracing::debug` naming the outpoint. Withholding leaves them exactly where the code before the re-credit left them: absent from coin selection until a rescan re-delivers the funding transaction. It understates the spendable balance and can never inflate it. Notably it also brings the two feature configurations back into agreement — under the default set a chainlocked funding record is already pruned to its txid and could never be rebuilt anyway. The reported `released_outpoints` set is deliberately unchanged: its contract is consumer-facing, consumers restore from their own record of the coin, and narrowing it is a separate change. The event-field docs now spell out the widened in-core divergence and that it is one-directional — core holds at most what the event reports, never more. Both scenarios are covered by regression tests that fail before this change and pass after it, in both feature configurations. Co-Authored-By: Claude Opus 4.8 --- key-wallet-manager/src/events.rs | 26 +- .../managed_core_funds_account.rs | 123 ++++++- .../transaction_checking/wallet_checker.rs | 321 ++++++++++++++++++ .../src/wallet/managed_wallet_info/helpers.rs | 59 +++- .../src/wallet/managed_wallet_info/mod.rs | 29 ++ 5 files changed, 529 insertions(+), 29 deletions(-) diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 8e39aaf17..71aeb71e8 100644 --- a/key-wallet-manager/src/events.rs +++ b/key-wallet-manager/src/events.rs @@ -295,12 +295,26 @@ pub enum WalletEvent { /// /// The wallet re-credits these coins to its own UTXO set as it emits /// them, so "spendable again" means the same thing on both sides of - /// this event. One case cannot be: a funding transaction pruned to - /// its txid by a chainlock keeps no `TxOut` to rebuild the coin from, - /// so it stays absent from coin selection until a rescan re-fetches - /// the block, even though it is named here. Consumers are unaffected - /// — their own record of the coin is what they restore — but a caller - /// reading this library's balance back will see the difference. See + /// this event wherever the re-credit can be proven safe. Where it + /// cannot, the coin is named here but deliberately withheld from this + /// library's own coin selection until a rescan re-delivers its funding + /// transaction: + /// + /// * the funding transaction was pruned to its txid by a chainlock, so + /// no `TxOut` survives to rebuild the coin from; + /// * the funding transaction can never confirm, because a block + /// already spent one of *its* inputs — the credit path refuses such + /// outputs too, and the re-credit must not disagree with it; + /// * the coin's own spent-status is no longer verifiable, because the + /// observed-spent map that records block spends is pruned at the + /// finality boundary and this coin's funding record sits at or below + /// it. Absence from a pruned map is not evidence a coin is unspent. + /// + /// Consumers are unaffected — their own record of the coin is what + /// they restore, and this set is reported in full in every case — but + /// a caller reading this library's balance back will see the + /// difference. The divergence is one-directional by construction: core + /// holds at most what the event reports, never more. See /// `ManagedCoreFundsAccount::recredit_released_outpoints`. released_outpoints: Vec, /// Wallet balance after the removal. 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 6c6301362..0abb1f512 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -106,6 +106,27 @@ pub(crate) struct ConflictSweep { pub released_outpoints: Vec, } +/// Whether `tx` can never confirm because a block already spent one of its +/// inputs (dashpay/rust-dashcore#649). +/// +/// A block spend is settled under Dash consensus, so any *other* transaction +/// claiming the same outpoint is dead — its outputs are money that does not +/// exist. A block or InstantSend context on `tx` itself exempts it: that is +/// the settled spend arriving, not a competitor for it. +/// +/// The observed-spent map is authoritative only above the pruning boundary +/// (see [`ManagedCoreFundsAccount::recredit_released_outpoints`]); a `false` +/// here therefore means "not known doomed", not "provably alive". +fn doomed_by_a_settled_spend( + tx: &Transaction, + context: &TransactionContext, + observed_spent: &BTreeMap, +) -> bool { + !context.confirmed() + && !context.is_instant_send() + && tx.input.iter().any(|input| observed_spent.contains_key(&input.previous_output)) +} + impl ManagedCoreFundsAccount { /// Create a new managed funds account pub fn new(managed_account_type: ManagedAccountType, network: Network) -> Self { @@ -200,6 +221,14 @@ impl ManagedCoreFundsAccount { self.spent_outpoints.contains(outpoint) } + /// Test-only read of the spent-mark set, which is otherwise private to + /// this module. Tests in sibling modules assert on spend bookkeeping + /// directly rather than inferring it from the balance. + #[cfg(test)] + pub(crate) fn is_outpoint_spent_for_test(&self, outpoint: &OutPoint) -> bool { + self.spent_outpoints.contains(outpoint) + } + /// Collect the outpoints among `tx`'s inputs that this account holds as a /// final UTXO — confirmed, InstantSend-locked, or trusted. /// @@ -317,13 +346,11 @@ impl ManagedCoreFundsAccount { // (winner confirms, loser arrives afterwards as mempool) // credits the loser's outputs with nothing left to remove // them, and `is_spendable` would hand them to coin selection. - let doomed_by_a_settled_spend = !context.confirmed() - && !matches!(context, TransactionContext::InstantSend(_)) - && tx - .input - .iter() - .any(|input| observed_spent.contains_key(&input.previous_output)); - if doomed_by_a_settled_spend { + // + // `recredit_released_outpoints` applies the same test to a + // funding record before rebuilding any of its outputs, which + // is why this lives in a shared function rather than inline. + if doomed_by_a_settled_spend(tx, &context, observed_spent) { // Deliberately before any mutation: the record built by // the caller stands, so history still shows the attempt, // but nothing it created enters the UTXO set. @@ -582,6 +609,47 @@ impl ManagedCoreFundsAccount { /// still in `spent_outpoints` (a surviving record here claims it, so /// `release_spent_marks` deliberately kept the mark). /// + /// # Sharing `update_utxos`' invariants + /// + /// This is a *fresh insert* into `utxos`, not a redelivery of a + /// transaction the chain has shown us again, so it has to clear the same + /// bars the credit path clears. Two of them cannot be answered by the + /// caller's `observed_spent_outpoints` membership test alone. + /// + /// **The funding record must still be able to confirm.** `update_utxos` + /// refuses to credit the outputs of a transaction whose input a block + /// already spent ([`doomed_by_a_settled_spend`]) while deliberately + /// keeping the record, for history. Reading that record back as a funding + /// source would materialise the very output the credit path rejected, so + /// the same test is applied here and a doomed funding record contributes + /// nothing. + /// + /// **The coin's spent-status must still be verifiable.** + /// `observed_spent_outpoints` is bounded state, not a permanent ledger: + /// `prune_finalized_observed_spends` evicts every entry at or below the + /// finality boundary. The map's safety argument was written for redelivery + /// paths, where the funding transaction arriving again is itself the + /// evidence; a re-credit has no such arrival, and absence from an evicting + /// map is not evidence that a coin is unspent. `spend_proof_horizon` is + /// the height at or below which an entry may already be gone. A coin is + /// re-credited only when its funding record sits in a block *above* that + /// horizon: a spend can never precede the output it spends, so any block + /// spend of such a coin is also above the horizon and would still be in + /// the map for the caller's test to find. + /// + /// Everything else is withheld — including every unconfirmed funding + /// record, which pins no lower bound on the spend window at all. A + /// mempool-context record does not mean the transaction is unmined: an + /// out-of-order rescan routinely holds one for a transaction the chain + /// mined long ago, which is exactly the shape dashpay/rust-dashcore#649 + /// describes. + /// + /// Withholding leaves the coin where the code before this re-credit + /// existed left it — absent from coin selection until a rescan re-delivers + /// its funding transaction. That understates the spendable balance and can + /// never inflate it. The *reported* release set is unchanged either way; + /// see the note on `WalletConflictSweep::released_outpoints`. + /// /// **Residual.** A funding transaction that was chainlock-finalized keeps /// only its txid under the default `keep-finalized-transactions = off`: /// its record — and with it the `TxOut` — is gone, so its coins cannot be @@ -592,7 +660,12 @@ impl ManagedCoreFundsAccount { /// the same feature flag that causes it. /// /// Returns the outpoints actually re-credited. - pub(crate) fn recredit_released_outpoints(&mut self, released: &[OutPoint]) -> Vec { + pub(crate) fn recredit_released_outpoints( + &mut self, + released: &[OutPoint], + observed_spent: &BTreeMap, + spend_proof_horizon: Option, + ) -> Vec { if released.is_empty() { return Vec::new(); } @@ -639,6 +712,40 @@ impl ManagedCoreFundsAccount { outpoint ); + // The output is provably ours; the remaining question is whether + // it is still a coin. Both gates below mirror `update_utxos`, and + // both are evaluated here — after ownership — so the diagnostics + // name only coins this account would otherwise have restored. + if doomed_by_a_settled_spend(&record.transaction, &record.context, observed_spent) { + tracing::debug!( + %outpoint, + funding_txid = %outpoint.txid, + "Withholding a released coin: a block already spent an input of its \ + funding transaction, which can therefore never confirm" + ); + continue; + } + if let Some(horizon) = spend_proof_horizon { + // A spend can never precede the output it spends, so a coin + // funded strictly above the horizon can only have been spent + // above it too — where the observed-spent map is still + // authoritative and the caller's membership test was + // conclusive. Anything else, unconfirmed funding records + // included, leaves the question open. + let spend_window_is_provable = + record.context.block_info().is_some_and(|info| info.height > horizon); + if !spend_window_is_provable { + tracing::debug!( + %outpoint, + funding_txid = %outpoint.txid, + horizon, + "Withholding a released coin whose spent-status can no longer be \ + cross-checked against the observed-spent map; a rescan recovers it" + ); + continue; + } + } + let txout = dashcore::TxOut { value: output.value, script_pubkey: output.script_pubkey.clone(), diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index ae59bb4c5..22102d5a9 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -2952,6 +2952,327 @@ mod tests { ); } + /// The #649 withholding must survive the eviction of the very map it + /// reads. + /// + /// `observed_spent_outpoints` is not permanent state: + /// `prune_finalized_observed_spends` drops every entry at or below the + /// finality boundary, which is exactly what keeps the map bounded. Gating + /// the re-credit on that map alone therefore answers "was this coin spent + /// in a block?" with "no" once the entry is gone — and rebuilds a coin the + /// chain has already consumed. + /// + /// The funding record here is learned from the mempool, so it carries no + /// block context and `apply_chain_lock` (which promotes and prunes only + /// `InBlock` records) never touches it. That is what makes this reproduce + /// under the default feature set too: the record-pruning that hides the + /// hole for confirmed funding records does not apply. + #[tokio::test] + async fn test_a_block_spent_coin_is_not_recredited_once_its_observed_spend_is_evicted() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..2, &[500_000, 400_000]); + let coin_a = OutPoint { + txid: funding_tx.txid(), + vout: 0, + }; + let coin_b = OutPoint { + txid: funding_tx.txid(), + vout: 1, + }; + let spend = |inputs: Vec, change: Option<&Address>, 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: match change { + Some(change) => vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: 99_000, + script_pubkey: change.script_pubkey(), + }, + ], + None => vec![TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }], + }, + special_transaction_payload: None, + }; + + // 1. Out-of-order delivery: the block at 101 that spends B is processed + // before the funding block at 100. Unattributable, so only #649 + // remembers it. + let thief = spend(vec![coin_b], None, 390_000); + let thief_result = ctx + .check_transaction( + &thief, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[9u8; 32]).expect("hash"), + 1_699_999_000, + )), + ) + .await; + assert!(!thief_result.is_relevant); + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&coin_b)); + + // 2. The funding transaction is learned from the mempool (its record + // therefore never carries a block context, so no chainlock ever + // prunes it). A credited, B withheld by #649. + ctx.check_transaction(&funding_tx, TransactionContext::Mempool).await; + assert!(ctx.bip44_account().utxos.contains_key(&coin_a)); + assert!(!ctx.bip44_account().utxos.contains_key(&coin_b), "#649 withheld it"); + + // 3. A loser claims A and B while the observed spend is still known, so + // `doomed_by_a_settled_spend` refuses to credit it and never marks + // B spent in the account. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend(vec![coin_a, coin_b], Some(&loser_change), 800_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + assert!( + !ctx.bip44_account().is_outpoint_spent_for_test(&coin_b), + "precondition: the doomed loser never marked B spent" + ); + + // 4. Sync advances: a chainlock lands over both blocks and the sync + // checkpoint commits. #649's memory of the spend is evicted. + ctx.managed_wallet.apply_chain_lock(dashcore::ephemerealdata::chain_lock::ChainLock { + block_height: 101, + block_hash: BlockHash::from_slice(&[9u8; 32]).expect("hash"), + signature: dashcore::bls_sig_utils::BLSSignature::from([0u8; 96]), + }); + ctx.managed_wallet.update_synced_height(101); + assert!( + !ctx.managed_wallet.observed_spent_outpoints().contains_key(&coin_b), + "precondition: the finality boundary evicted the observed spend" + ); + assert!( + ctx.bip44_account().transactions().contains_key(&funding_tx.txid()), + "precondition: a mempool funding record survives the chainlock in every \ + feature configuration, so record pruning is not a backstop here" + ); + + // 5. A winner takes A and confirms, sweeping the loser. B is released. + 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], Some(&winner_change), 400_000); + let result = ctx + .check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 102, + BlockHash::from_slice(&[2u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + assert!( + result.released_outpoints.contains(&coin_b), + "the reported set is unchanged: B is still named as released" + ); + + // B is spent on chain at height 101. Its spent-status can no longer be + // cross-checked against `observed_spent_outpoints`, so the re-credit + // has to withhold it rather than guess. + assert!( + !ctx.bip44_account().utxos.contains_key(&coin_b), + "a coin a block already spent must never be re-credited to coin selection" + ); + assert!( + !ctx.bip44_account().spendable_utxos(102).iter().any(|utxo| utxo.outpoint == coin_b), + "and it must not reach coin selection by any other route" + ); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 99_000, + "only the winner's change; B contributes nothing" + ); + } + + /// The re-credit must not resurrect a coin `update_utxos` refused to + /// create. + /// + /// `update_utxos` never credits the outputs of a transaction whose input a + /// block already spent (`doomed_by_a_settled_spend`) — it can never + /// confirm, so its change is money that does not exist. The record is + /// deliberately kept, for history. A re-credit that reads that record as a + /// funding source without re-checking the same verdict materialises the + /// output the credit path had already rejected. + #[tokio::test] + async fn test_the_recredit_refuses_a_funding_record_that_can_never_confirm() { + let mut ctx = TestWalletContext::new_random(); + let external_address = Address::p2pkh( + &dashcore::PublicKey::from_slice(&[0x02; 33]).expect("pubkey"), + Network::Testnet, + ); + + // Block 100: two real coins. + let genesis_tx = Transaction::dummy(&ctx.receive_address, 0..2, &[1_000_000, 300_000]); + let coin_p = OutPoint { + txid: genesis_tx.txid(), + vout: 0, + }; + let coin_q = OutPoint { + txid: genesis_tx.txid(), + vout: 1, + }; + ctx.check_transaction( + &genesis_tx, + TransactionContext::InBlock(BlockInfo::new( + 100, + BlockHash::from_slice(&[1u8; 32]).expect("hash"), + 1_700_000_000, + )), + ) + .await; + assert!(ctx.bip44_account().utxos.contains_key(&coin_p)); + + let spend = + |inputs: Vec, change: Option<&Address>, change_value: 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: match change { + Some(change) => vec![ + TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }, + TxOut { + value: change_value, + script_pubkey: change.script_pubkey(), + }, + ], + None => vec![TxOut { + value: sent, + script_pubkey: external_address.script_pubkey(), + }], + }, + special_transaction_payload: None, + } + }; + + // Block 101: someone else spends P on chain, paying only externally. + let thief = spend(vec![coin_p], None, 0, 990_000); + ctx.check_transaction( + &thief, + TransactionContext::InBlock(BlockInfo::new( + 101, + BlockHash::from_slice(&[9u8; 32]).expect("hash"), + 1_700_000_100, + )), + ) + .await; + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&coin_p)); + + // A doomed transaction of ours also spends P. `update_utxos` refuses to + // credit its change X — P is already spent in a block, so this can + // never confirm — but keeps the record. + let doomed_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let doomed = spend(vec![coin_p], Some(&doomed_change), 500_000, 490_000); + ctx.check_transaction(&doomed, TransactionContext::Mempool).await; + let coin_x = OutPoint { + txid: doomed.txid(), + vout: 1, + }; + assert!( + !ctx.bip44_account().utxos.contains_key(&coin_x), + "precondition: the doomed transaction's change was never credited" + ); + assert!( + ctx.bip44_account().transactions().contains_key(&doomed.txid()), + "precondition: but its record is kept for history" + ); + + // A later unconfirmed transaction claims that phantom output X plus a + // real coin Q. + let loser_change = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let loser = spend(vec![coin_x, coin_q], Some(&loser_change), 90_000, 700_000); + ctx.check_transaction(&loser, TransactionContext::Mempool).await; + + // A winner takes Q and confirms, sweeping the loser. X is released. + 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_q], Some(&winner_change), 200_000, 90_000); + let result = ctx + .check_transaction( + &winner, + TransactionContext::InBlock(BlockInfo::new( + 102, + BlockHash::from_slice(&[2u8; 32]).expect("hash"), + 1_700_000_200, + )), + ) + .await; + assert!( + result.released_outpoints.contains(&coin_x), + "the reported set is unchanged: X is still named as released" + ); + + assert!( + !ctx.bip44_account().utxos.contains_key(&coin_x), + "a coin must never be rebuilt from a transaction that can never confirm" + ); + assert!( + !ctx.bip44_account().spendable_utxos(102).iter().any(|utxo| utxo.outpoint == coin_x), + "and it must not reach coin selection by any other route" + ); + assert_eq!( + ctx.managed_wallet.balance.confirmed(), + 200_000, + "only the winner's change; the phantom change X contributes nothing" + ); + assert_eq!(ctx.managed_wallet.balance.unconfirmed(), 0); + } + /// 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 diff --git a/key-wallet/src/wallet/managed_wallet_info/helpers.rs b/key-wallet/src/wallet/managed_wallet_info/helpers.rs index d201c9e31..67fa6cc4f 100644 --- a/key-wallet/src/wallet/managed_wallet_info/helpers.rs +++ b/key-wallet/src/wallet/managed_wallet_info/helpers.rs @@ -35,11 +35,15 @@ pub struct AbandonOutcome { /// derive the set from `abandoned` alone — that would require knowing /// which of their inputs some *other* surviving transaction also claims. /// - /// These coins are also re-credited to this wallet's own UTXO set where - /// their funding record survives, so the event and in-core coin selection - /// agree. See - /// `ManagedCoreFundsAccount::recredit_released_outpoints` for the one - /// case that cannot be — a chainlock-pruned funding record. + /// These coins are also re-credited to this wallet's own UTXO set, so the + /// event and in-core coin selection agree wherever the re-credit can be + /// proven safe. Where it cannot, this set is still reported in full and + /// the coin is withheld in core until a rescan — a deliberate, + /// balance-understating divergence. See + /// `ManagedCoreFundsAccount::recredit_released_outpoints` for the three + /// cases: a chainlock-pruned funding record, a funding record that can + /// never confirm, and a coin whose spent-status is no longer verifiable + /// against the observed-spent map. pub released_outpoints: Vec, } @@ -198,8 +202,10 @@ impl ManagedWalletInfo { /// /// The released coins are re-credited to this wallet's own UTXO set here /// too, once the per-account answers have been reconciled — the reported - /// set and what coin selection can actually spend must not disagree. See - /// `ManagedCoreFundsAccount::recredit_released_outpoints`. + /// set and what coin selection can actually spend should not disagree. + /// Where the re-credit cannot be proven safe it is withheld and the two + /// do diverge, always with core holding *less* than the event reports. + /// See `ManagedCoreFundsAccount::recredit_released_outpoints`. pub fn sweep_conflicts( &mut self, tx: &Transaction, @@ -248,11 +254,23 @@ impl ManagedWalletInfo { /// processed yet, never even matched (dashpay/rust-dashcore#649 — the same /// reason `update_utxos` refuses to insert such an output in the first /// place). Re-crediting one of those would hand coin selection a coin the - /// chain has already spent. The released set is still *reported* as-is: - /// that set's contract is a consumer-facing one with its own documented - /// limits, and narrowing it is a separate change from making in-core state - /// match it — this withholding only ever errs toward understating what we - /// can spend. + /// chain has already spent. + /// + /// That membership test is necessary but not sufficient, and the map alone + /// cannot make it sufficient: `prune_finalized_observed_spends` evicts + /// entries at the finality boundary, so below + /// [`ManagedWalletInfo::spend_proof_horizon`] a miss means "no longer + /// recorded", not "never spent". The map and the horizon are therefore + /// both handed down to the accounts, which withhold any coin whose + /// spent-status is no longer cross-checkable and any coin whose funding + /// record can never confirm. See + /// [`ManagedCoreFundsAccount::recredit_released_outpoints`] for both + /// rules. + /// + /// The released set is still *reported* as-is: that set's contract is a + /// consumer-facing one with its own documented limits, and narrowing it is + /// a separate change from making in-core state match it — this withholding + /// only ever errs toward understating what we can spend. fn recredit_released_outpoints(&mut self, released: &[OutPoint]) -> Vec { if released.is_empty() { return Vec::new(); @@ -265,10 +283,19 @@ impl ManagedWalletInfo { if candidates.is_empty() { return Vec::new(); } + let horizon = self.spend_proof_horizon(); + // Disjoint field borrows: the accounts are taken mutably while the + // observed-spent map is read, so the per-account gate can consult it + // without cloning a map that is bounded only by `MAX_OBSERVED_SPENT_OUTPOINTS`. + let observed_spent = &self.observed_spent_outpoints; let mut recredited = Vec::new(); for account in self.accounts.all_accounts_mut() { if let ManagedAccountRefMut::Funds(funds) = account { - recredited.extend(funds.recredit_released_outpoints(&candidates)); + recredited.extend(funds.recredit_released_outpoints( + &candidates, + observed_spent, + horizon, + )); } } recredited @@ -329,8 +356,10 @@ impl ManagedWalletInfo { /// not what funded them. They are reported in /// [`AbandonOutcome::released_outpoints`] so a persistence mirror can /// follow. See `ManagedCoreFundsAccount::recredit_released_outpoints` - /// for the one case that cannot be rebuilt: a funding record already - /// pruned to its txid by a chainlock, which needs a rescan. + /// for the cases that cannot be rebuilt and are withheld until a rescan: + /// a funding record already pruned to its txid by a chainlock, one a + /// settled spend has doomed, and one whose coin's spent-status can no + /// longer be cross-checked. /// /// Does not recompute the balance — callers batching several abandons /// should run `update_balance` diff --git a/key-wallet/src/wallet/managed_wallet_info/mod.rs b/key-wallet/src/wallet/managed_wallet_info/mod.rs index 4b64db797..30e806009 100644 --- a/key-wallet/src/wallet/managed_wallet_info/mod.rs +++ b/key-wallet/src/wallet/managed_wallet_info/mod.rs @@ -377,6 +377,35 @@ impl ManagedWalletInfo { self.observed_spent_outpoints.retain(|_, height| *height > boundary); } + /// Height at or below which [`Self::observed_spent_outpoints`] can no + /// longer answer "was this outpoint spent in a block?" — entries there may + /// already have been evicted by [`Self::prune_finalized_observed_spends`], + /// so absence from the map proves nothing about them. + /// + /// `None` until a chainlock has been applied: the prune is a no-op until + /// then, so nothing has ever been evicted and the map is authoritative at + /// every height. + /// + /// Once a chainlock exists this is *its* height, deliberately not the + /// `min(chain_lock, synced_height)` the prune itself computes. That + /// boundary describes a single run; a gate has to bound everything ever + /// evicted, across every run this wallet has performed. The applied + /// chainlock height is monotonic — [`WalletInfoInterface::apply_chain_lock`] + /// replaces the stored lock only on a strictly greater height — and every + /// past prune boundary was `min(chain_lock_then, synced_then)`, which is at + /// most `chain_lock_then`, which is at most this. So this is a sound upper + /// bound on the evicted range, where a recomputed `min()` is not: + /// [`Self::rewind_sync_checkpoint_for_new_account`] deliberately drops + /// `synced_height` to just below wallet birth when an account is added, + /// which would move a `min()`-derived boundary back *down* and re-admit + /// coins whose spend records an earlier, higher boundary had already + /// evicted. + /// + /// [`WalletInfoInterface::apply_chain_lock`]: crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface::apply_chain_lock + pub(crate) fn spend_proof_horizon(&self) -> Option { + self.metadata.last_applied_chain_lock.as_ref().map(|lock| lock.block_height) + } + /// Invalidate the wallet's sync certificate when an account is added. /// /// `synced_height` certifies "every filter at or below this height was