diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index b117b58d2..d008f853d 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -575,7 +575,15 @@ impl= batch_start { + wallet.update_wallet_synced_height(wallet_id, end); + } } } } @@ -1373,6 +1381,154 @@ mod tests { assert_eq!(multi.read().await.wallet_synced_height(&wallet_b), 0); } + /// Contiguity guard (dashpay/rust-dashcore#649): a batch scanned before an + /// account-add rewinds the wallet's checkpoint must NOT clobber the rewound + /// value forward at commit time — otherwise the account-addition rescan is + /// silently cancelled. The wallet stays behind and the tick picks it up. + #[tokio::test] + async fn mid_flight_account_add_does_not_clobber_rescan_floor() { + let wallet_a: WalletId = [0xAA; 32]; + let multi = Arc::new(RwLock::new(MultiMockWallet::new())); + { + let mut w = multi.write().await; + // Already synced up to 4999, so the next batch legitimately starts at 5000. + w.insert_wallet( + wallet_a, + MockWalletState { + synced_height: 4999, + ..MockWalletState::default() + }, + ); + } + let mut manager = create_multi_test_manager(multi.clone()).await; + manager.set_state(SyncState::Syncing); + + // Batch [5000..9999] scanned with wallet_a recorded, ready to commit. + let mut batch = FiltersBatch::new(5000, 9999, HashMap::new()); + batch.set_pending_blocks(0); + batch.mark_scanned(); + batch.mark_rescan_complete(); + batch.set_scanned_wallets(BTreeSet::from([wallet_a])); + manager.active_batches.insert(5000, batch); + + // Simulate an account being added mid-flight: the wallet's checkpoint is + // rewound to just below birth, far below this batch's start. + multi.write().await.wallet_mut(&wallet_a).synced_height = 49; + + manager.try_commit_batches().await.unwrap(); + + // committed_height still advances (chain progress), but the wallet's + // checkpoint is NOT dragged forward over the gap it must rescan. + assert_eq!(manager.progress.committed_height(), 9999); + assert_eq!( + multi.read().await.wallet_synced_height(&wallet_a), + 49, + "the rewound checkpoint must survive commit — the batch is non-contiguous with it" + ); + // The wallet is still behind, so the next tick rescans it. + assert!( + multi.read().await.wallets_behind(9999).contains(&wallet_a), + "the wallet remains behind and is picked up by the tick rescan" + ); + } + + /// The contiguity guard is transparent in normal operation: contiguous + /// ascending batches advance each wallet's checkpoint exactly as before. + #[tokio::test] + async fn contiguity_guard_permits_normal_advance() { + let wallet_a: WalletId = [0xCC; 32]; + let multi = Arc::new(RwLock::new(MultiMockWallet::new())); + multi.write().await.insert_wallet(wallet_a, MockWalletState::default()); + let mut manager = create_multi_test_manager(multi.clone()).await; + manager.set_state(SyncState::Syncing); + + // First batch starts at 0 = synced_height (0) + ... the wallet is fresh, + // so this batch is contiguous and advances the checkpoint. + let mut batch1 = FiltersBatch::new(0, 4999, HashMap::new()); + batch1.set_pending_blocks(0); + batch1.mark_scanned(); + batch1.mark_rescan_complete(); + batch1.set_scanned_wallets(BTreeSet::from([wallet_a])); + manager.active_batches.insert(0, batch1); + + manager.try_commit_batches().await.unwrap(); + assert_eq!(multi.read().await.wallet_synced_height(&wallet_a), 4999); + + // Second batch starts exactly at synced_height + 1 = 5000: still + // contiguous, so it advances too. + let mut batch2 = FiltersBatch::new(5000, 9999, HashMap::new()); + batch2.set_pending_blocks(0); + batch2.mark_scanned(); + batch2.mark_rescan_complete(); + batch2.set_scanned_wallets(BTreeSet::from([wallet_a])); + manager.active_batches.insert(5000, batch2); + + manager.try_commit_batches().await.unwrap(); + assert_eq!( + multi.read().await.wallet_synced_height(&wallet_a), + 9999, + "contiguous ascending batches advance the checkpoint unchanged by the guard" + ); + } + + /// Marvin QA-001 round 2: two wallets in the SAME batch, only one of them + /// rewound mid-flight (simulating an account add on wallet_a only). The + /// contiguity guard must block wallet_a's advance while still advancing + /// wallet_b normally — a per-wallet leak here would either strand a + /// healthy wallet or silently clobber the rewound one. + #[tokio::test] + async fn contiguity_guard_is_per_wallet_not_batch_wide() { + let wallet_a: WalletId = [0xAA; 32]; + let wallet_b: WalletId = [0xBB; 32]; + let multi = Arc::new(RwLock::new(MultiMockWallet::new())); + { + let mut w = multi.write().await; + w.insert_wallet( + wallet_a, + MockWalletState { + synced_height: 4999, + ..MockWalletState::default() + }, + ); + w.insert_wallet( + wallet_b, + MockWalletState { + synced_height: 4999, + ..MockWalletState::default() + }, + ); + } + let mut manager = create_multi_test_manager(multi.clone()).await; + manager.set_state(SyncState::Syncing); + + // Batch [5000..9999] scanned with BOTH wallets recorded. + let mut batch = FiltersBatch::new(5000, 9999, HashMap::new()); + batch.set_pending_blocks(0); + batch.mark_scanned(); + batch.mark_rescan_complete(); + batch.set_scanned_wallets(BTreeSet::from([wallet_a, wallet_b])); + manager.active_batches.insert(5000, batch); + + // Only wallet_a gets an account added mid-flight (rewound). wallet_b + // is untouched and remains legitimately contiguous with this batch. + multi.write().await.wallet_mut(&wallet_a).synced_height = 49; + + manager.try_commit_batches().await.unwrap(); + + assert_eq!(manager.progress.committed_height(), 9999); + assert_eq!( + multi.read().await.wallet_synced_height(&wallet_a), + 49, + "wallet_a's rewound checkpoint must survive commit" + ); + assert_eq!( + multi.read().await.wallet_synced_height(&wallet_b), + 9999, + "wallet_b was legitimately contiguous and must still advance despite \ + sharing a batch with a rewound wallet" + ); + } + /// `scan_batch` with two wallets at different `synced_height` values: /// only the wallet whose synced_height is below the matching block's /// height should be attributed. diff --git a/key-wallet-manager/tests/common/mod.rs b/key-wallet-manager/tests/common/mod.rs new file mode 100644 index 000000000..e1bec28f2 --- /dev/null +++ b/key-wallet-manager/tests/common/mod.rs @@ -0,0 +1,74 @@ +//! Shared helpers for the observed-spent-outpoint guard integration tests +//! (dashpay/rust-dashcore#649). +//! +//! Each test binary pulls this in via `mod common;` and uses a subset of the +//! helpers, so unused-item warnings are expected per binary and silenced here. +#![allow(dead_code)] + +use dashcore::blockdata::block::Block; +use dashcore::blockdata::transaction::OutPoint; +use dashcore::{Address, Network, ScriptBuf, Transaction, TxIn, TxOut, Witness}; +use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; +use key_wallet_manager::{WalletId, WalletInterface, WalletManager}; +use std::collections::BTreeSet; + +/// Deliver `block` at `height` to the given `wallets`. +pub async fn process_block_for( + manager: &mut WalletManager, + block: &Block, + height: u32, + wallets: &BTreeSet, +) { + manager.process_block_for_wallets(block, block.block_hash(), height, wallets).await; +} + +/// Deliver `block` at `height` to every wallet the manager holds. +pub async fn process_block_all_wallets( + manager: &mut WalletManager, + block: &Block, + height: u32, +) { + let wallet_ids: BTreeSet = manager.list_wallets().into_iter().copied().collect(); + process_block_for(manager, block, height, &wallet_ids).await; +} + +/// A transaction paying `value` to `address` from one synthetic, unrelated +/// input (`input_seed` makes its txid deterministic and distinct). Only the +/// transaction's own txid/vout matter to the tests, as the coin later spent. +pub fn funding_tx(address: &Address, value: u64, input_seed: u8) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint::new(dashcore::Txid::from([input_seed; 32]), 0), + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value, + script_pubkey: address.script_pubkey(), + }], + special_transaction_payload: None, + } +} + +/// A transaction spending `outpoint` and paying `value` to an unrelated +/// external dummy address (`ext_id` selects a distinct address). +pub fn spend_tx(outpoint: OutPoint, value: u64, ext_id: usize) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value, + script_pubkey: Address::dummy(Network::Testnet, ext_id).script_pubkey(), + }], + special_transaction_payload: None, + } +} diff --git a/key-wallet-manager/tests/observed_spent_large_block_stress_test.rs b/key-wallet-manager/tests/observed_spent_large_block_stress_test.rs new file mode 100644 index 000000000..dbbd1bede --- /dev/null +++ b/key-wallet-manager/tests/observed_spent_large_block_stress_test.rs @@ -0,0 +1,143 @@ +//! Adversarial stress test for the observed-spent-outpoint guard +//! (dashpay/rust-dashcore#649) at the *single-block* granularity, as opposed +//! to spreading growth across many small blocks. +//! +//! `process_block_for_wallets` (`key-wallet-manager/src/process_block.rs`) +//! iterates every transaction in `block.txdata` and calls +//! `check_transaction_in_wallets` for EACH ONE, unconditionally — a matched +//! block is delivered whole once ANY of its transactions matches a wallet's +//! compact filter. `ManagedWalletInfo::check_core_transaction` +//! (`key-wallet/src/transaction_checking/wallet_checker.rs`) then records +//! every input of every transaction it sees in block context into +//! `observed_spent_outpoints`, and — for transactions it judges irrelevant — +//! also calls `remove_spent_from_accounts`, which allocates a +//! `Vec<&mut ManagedCoreFundsAccount>` via `all_funding_accounts_mut()` and +//! attempts a UTXO-map removal per input, for every one of those irrelevant +//! transactions. +//! +//! The sibling `many_orphaned_spend_blocks_stay_bounded_and_linear` +//! (`key-wallet/src/tests/observed_spent_outpoints_tests.rs`) spreads growth +//! across many separate `check_transaction` calls at distinct heights. That +//! does not exercise what a real matched block looks like: many transactions +//! sharing ONE height/block, most of them entirely unrelated to the wallet. +//! This test constructs a single `Block` with 5,000 unrelated 3-input +//! transactions plus one wallet-relevant spend, and verifies both correctness +//! (the bug is still caught when the relevant tx is buried in a large noisy +//! block) and the actual growth/cost this produces. + +mod common; + +use common::process_block_all_wallets; +use dashcore::blockdata::block::Block; +use dashcore::blockdata::transaction::OutPoint; +use dashcore::{Network, ScriptBuf, Transaction, TxIn, TxOut, Witness}; +use key_wallet::wallet::initialization::WalletAccountCreationOptions; +use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; +use key_wallet_manager::WalletManager; +use std::time::Instant; + +const NOISE_TXS_PER_BLOCK: usize = 5_000; +const INPUTS_PER_NOISE_TX: usize = 3; + +/// An unrelated 3-input transaction paying an external address, with inputs +/// deterministically derived from `seed` so every one is distinct and none +/// resolves to any outpoint the wallet will ever fund. +fn noise_tx(seed: u32) -> Transaction { + let input = (0..INPUTS_PER_NOISE_TX as u32) + .map(|v| { + let mut bytes = [0u8; 32]; + bytes[..4].copy_from_slice(&seed.to_le_bytes()); + bytes[4] = v as u8; + TxIn { + previous_output: OutPoint::new(dashcore::Txid::from(bytes), v), + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + } + }) + .collect(); + Transaction { + version: 2, + lock_time: 0, + input, + output: vec![TxOut { + value: 1_000, + script_pubkey: dashcore::Address::dummy(Network::Testnet, (seed % 90) as usize + 1) + .script_pubkey(), + }], + special_transaction_payload: None, + } +} + +#[tokio::test] +async fn wallet_relevant_spend_buried_in_large_noisy_block_still_caught() { + let mut manager = WalletManager::::new(Network::Testnet); + let wallet_id = manager + .create_wallet_with_random_mnemonic(WalletAccountCreationOptions::Default) + .expect("failed to create wallet"); + let funding_address = + manager.monitored_addresses().first().cloned().expect("wallet must have addresses"); + + let funding_value = 1_000_000u64; + let funding_tx = common::funding_tx(&funding_address, funding_value, 0xAB); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + + let spend_tx = common::spend_tx(funding_outpoint, funding_value - 1_000, 99); + + // One large block: 5,000 unrelated noise transactions with the single + // wallet-relevant spend buried in the middle — this is the realistic + // shape of "a matched block" (dash-spv delivers the whole block once any + // tx in it matches the filter), not 5,000 separate block deliveries. + let mut txdata: Vec = (0..NOISE_TXS_PER_BLOCK as u32).map(noise_tx).collect(); + let mid = txdata.len() / 2; + txdata.insert(mid, spend_tx.clone()); + let expected_inputs_in_spend_block = + NOISE_TXS_PER_BLOCK * INPUTS_PER_NOISE_TX + 1 /* spend_tx's own input */; + + let spend_block = Block::dummy(200, txdata); + + let start = Instant::now(); + process_block_all_wallets(&mut manager, &spend_block, 200).await; + let large_block_elapsed = start.elapsed(); + + let funding_block = Block::dummy(100, vec![funding_tx.clone()]); + process_block_all_wallets(&mut manager, &funding_block, 100).await; + + let utxos_after = manager.wallet_utxos(&wallet_id).expect("wallet must be registered"); + let still_tracked = utxos_after.iter().any(|u| u.outpoint == funding_outpoint); + assert!( + !still_tracked, + "BUG #649 regression: the relevant spend buried among {NOISE_TXS_PER_BLOCK} unrelated \ + transactions in the same block was not correctly reconciled against its \ + out-of-order funding" + ); + + let observed_len = + manager.get_wallet_info(&wallet_id).expect("wallet info").observed_spent_outpoints().len(); + let expected_total = expected_inputs_in_spend_block + 1 /* funding_tx's own input */; + assert_eq!( + observed_len, expected_total, + "a single matched block with {NOISE_TXS_PER_BLOCK} unrelated txs records one \ + observed-spent entry per input in the WHOLE block, not just the relevant tx — \ + set grew to {observed_len} entries from ONE block, confirming growth scales with \ + transactions-per-block, not with block count" + ); + + eprintln!( + "single block with {} txs ({} inputs) processed in {:?} ({:.1} us/input); \ + observed_spent_outpoints now holds {} entries after ONE block", + NOISE_TXS_PER_BLOCK + 1, + expected_inputs_in_spend_block, + large_block_elapsed, + large_block_elapsed.as_micros() as f64 / expected_inputs_in_spend_block as f64, + observed_len, + ); + + // Per-block processing cost, reported as a diagnostic rather than asserted: + // a wall-clock threshold flakes on shared CI runners, while correctness is + // already pinned above. A multi-second time here would flag the un-gated + // recording/removal seam as a production concern for busy blocks. + eprintln!( + "single block with {NOISE_TXS_PER_BLOCK} unrelated txs processed in {large_block_elapsed:?}" + ); +} diff --git a/key-wallet-manager/tests/observed_spent_multi_wallet_test.rs b/key-wallet-manager/tests/observed_spent_multi_wallet_test.rs new file mode 100644 index 000000000..1acbdb5f8 --- /dev/null +++ b/key-wallet-manager/tests/observed_spent_multi_wallet_test.rs @@ -0,0 +1,67 @@ +//! Multi-wallet isolation for the observed-spent-outpoint guard +//! (dashpay/rust-dashcore#649). +//! +//! `observed_spent_outpoints` lives on `ManagedWalletInfo` (per wallet), not on +//! a shared `WalletManager` structure. So one wallet observing a spend for an +//! outpoint must never suppress a *different* wallet's legitimate funding of the +//! same outpoint — even when both wallets, in the same process, happen to match +//! it (e.g. a synthetic/colliding outpoint, or a reused xpub-derived address +//! across two independently imported wallets, which the SDK does not forbid). + +mod common; + +use common::{funding_tx, process_block_for, spend_tx}; +use dashcore::blockdata::block::Block; +use dashcore::blockdata::transaction::OutPoint; +use dashcore::Network; +use key_wallet::wallet::initialization::WalletAccountCreationOptions; +use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; +use key_wallet_manager::{WalletId, WalletManager}; +use std::collections::BTreeSet; + +#[tokio::test] +async fn observed_spends_are_isolated_per_wallet() { + let mut manager = WalletManager::::new(Network::Testnet); + + // Create wallet B first and capture one of its own addresses while it is the + // only wallet, so the funding transaction pays an address B recognizes. + let wallet_b = manager + .create_wallet_with_random_mnemonic(WalletAccountCreationOptions::Default) + .expect("create wallet B"); + let b_address = manager.monitored_addresses().first().cloned().expect("wallet B has addresses"); + + // Then create wallet A, which observes the spend but never funds the coin. + let wallet_a = manager + .create_wallet_with_random_mnemonic(WalletAccountCreationOptions::Default) + .expect("create wallet A"); + + // Funding pays B's address; the outpoint it creates is O. + let funding_value = 1_000_000u64; + let funding = funding_tx(&b_address, funding_value, 0xB0); + let outpoint = OutPoint::new(funding.txid(), 0); + + // A spend of O, delivered to wallet A out of order (before any funding A sees). + let spend = spend_tx(outpoint, funding_value - 1_000, 77); + + let only_a: BTreeSet = [wallet_a].into_iter().collect(); + let only_b: BTreeSet = [wallet_b].into_iter().collect(); + + // Wallet A observes the spend (records O in A's set only). + let spend_block = Block::dummy(200, vec![spend]); + process_block_for(&mut manager, &spend_block, 200, &only_a).await; + + // Wallet B funds O in order — B never saw the spend, so B's set is empty. + let funding_block = Block::dummy(100, vec![funding]); + process_block_for(&mut manager, &funding_block, 100, &only_b).await; + + let b_utxos = manager.wallet_utxos(&wallet_b).expect("wallet B registered"); + assert!( + b_utxos.iter().any(|u| u.outpoint == outpoint), + "wallet B's legitimate funding must NOT be suppressed by wallet A's spend observation" + ); + let a_utxos = manager.wallet_utxos(&wallet_a).expect("wallet A registered"); + assert!( + !a_utxos.iter().any(|u| u.outpoint == outpoint), + "wallet A never funded O and must not track it" + ); +} diff --git a/key-wallet-manager/tests/out_of_order_spend_repro_test.rs b/key-wallet-manager/tests/out_of_order_spend_repro_test.rs new file mode 100644 index 000000000..80a4bbbf4 --- /dev/null +++ b/key-wallet-manager/tests/out_of_order_spend_repro_test.rs @@ -0,0 +1,81 @@ +//! A spend that is processed BEFORE the transaction that created the UTXO it +//! spends (out-of-order block delivery during rescan) must not leave that UTXO +//! permanently in the wallet's tracked set (dashpay/rust-dashcore#649). +//! +//! Guard site: `key-wallet/src/managed_account/managed_core_funds_account.rs` +//! (`update_utxos`). The function guards exactly this ordering ("Check if this +//! outpoint was already spent by a transaction we've seen. This handles +//! out-of-order block processing during rescan...") via +//! `self.is_outpoint_spent(&outpoint)` checked against `self.spent_outpoints`, +//! populated by the same function's spend-processing loop +//! (`self.spent_outpoints.insert(input.previous_output)` for every input of +//! every processed transaction, regardless of whether that input is recognized +//! as one of this account's own UTXOs). +//! +//! During a cold rescan, block heights are processed in essentially arbitrary +//! order (the parallel-filters matching completes in whatever order worker +//! threads finish, not height order). A spend can therefore be processed before +//! the funding transaction that created the coin it spends — the spending +//! transaction shows `sent = 0` because the wallet does not yet recognize the +//! input as its own, and the funding transaction is then inserted as a fresh, +//! spendable UTXO. If the guard fails, `key-wallet` can later select that coin +//! as `require_final_inputs`-eligible funding for an asset-lock transaction the +//! network rejects because the input is already spent. +//! +//! This test reproduces the same ORDER — process the spending block first, then +//! the funding block — deterministically, and checks that the out-of-order +//! guard prevents the funding UTXO from being (re-)inserted once its spend has +//! already been observed. + +mod common; + +use common::{funding_tx, process_block_all_wallets, spend_tx}; +use dashcore::blockdata::block::Block; +use dashcore::blockdata::transaction::OutPoint; +use key_wallet::wallet::initialization::WalletAccountCreationOptions; +use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; +use key_wallet_manager::WalletManager; + +#[tokio::test] +async fn spend_processed_before_its_funding_tx_leaves_utxo_permanently_tracked() { + let mut manager = WalletManager::::new(dashcore::Network::Testnet); + let wallet_id = manager + .create_wallet_with_random_mnemonic(WalletAccountCreationOptions::Default) + .expect("failed to create wallet"); + + // The address that will receive the funding output — this account's first + // monitored (external, index 0) address. + let funding_address = + manager.monitored_addresses().first().cloned().expect("wallet must have addresses"); + + // Funding pays 1,000,000 duffs to our own address; only its own txid/vout + // (the coin later spent) matters. + let funding_value = 1_000_000u64; + let funding = funding_tx(&funding_address, funding_value, 0xAB); + let funding_outpoint = OutPoint::new(funding.txid(), 0); + + // Spend the funding UTXO (by its already-known txid) out to an external + // address, so the wallet no longer owns this specific output afterward. + let spend = spend_tx(funding_outpoint, funding_value - 1_000, 99); + + // Out-of-order delivery: the spend's block (height 200) is processed before + // the funding's block (height 100). + let spend_block = Block::dummy(200, vec![spend]); + process_block_all_wallets(&mut manager, &spend_block, 200).await; + + let funding_block = Block::dummy(100, vec![funding]); + process_block_all_wallets(&mut manager, &funding_block, 100).await; + + let utxos_after = manager.wallet_utxos(&wallet_id).expect("wallet must be registered"); + let still_tracked = utxos_after.iter().any(|u| u.outpoint == funding_outpoint); + + assert!( + !still_tracked, + "BUG #649 regression: funding outpoint {funding_outpoint} is still present in the \ + wallet's tracked UTXO set after its spend was processed FIRST (height 200) and its \ + funding transaction was processed SECOND (height 100). `update_utxos`'s own \ + `is_outpoint_spent` guard (managed_core_funds_account.rs) exists to handle this \ + out-of-order case but did not prevent the funding output from being (re-)inserted as \ + tracked/spendable once its spend had already been observed." + ); +} diff --git a/key-wallet/src/managed_account/address_pool.rs b/key-wallet/src/managed_account/address_pool.rs index 0bb38e061..1ba2540aa 100644 --- a/key-wallet/src/managed_account/address_pool.rs +++ b/key-wallet/src/managed_account/address_pool.rs @@ -1013,6 +1013,7 @@ impl AddressPool { for idx in indices_to_remove { if let Some(info) = self.addresses.remove(&idx) { self.address_index.remove(&info.address); + self.script_pubkey_index.remove(&info.script_pubkey); pruned += 1; } } diff --git a/key-wallet/src/managed_account/managed_account_ref.rs b/key-wallet/src/managed_account/managed_account_ref.rs index e95e36a7a..6bd9145c8 100644 --- a/key-wallet/src/managed_account/managed_account_ref.rs +++ b/key-wallet/src/managed_account/managed_account_ref.rs @@ -20,6 +20,8 @@ use crate::transaction_checking::account_checker::AccountMatch; use crate::transaction_checking::transaction_router::TransactionType; use crate::transaction_checking::TransactionContext; use crate::Network; +use dashcore::blockdata::transaction::OutPoint; +use dashcore::prelude::CoreBlockHeight; use dashcore::{Address, ScriptBuf, Transaction, Txid}; use std::collections::BTreeMap; @@ -293,16 +295,21 @@ impl<'a> ManagedAccountRefMut<'a> { /// Funds variants update UTXO state and balance; keys variants update /// only the transaction history. Both are subject to the /// `keep-finalized-transactions` Cargo feature for chainlocked records. - pub fn record_transaction( + /// + /// `observed_spent` is the wallet-level `observed_spent_outpoints` view + /// (dashpay/rust-dashcore#649); only the funds variant consults it (keys + /// accounts track no UTXOs/output details). + pub(crate) fn record_transaction( &mut self, tx: &Transaction, account_match: &AccountMatch, context: TransactionContext, transaction_type: TransactionType, + observed_spent: &BTreeMap, ) -> TransactionRecord { match self { ManagedAccountRefMut::Funds(a) => { - a.record_transaction(tx, account_match, context, transaction_type) + a.record_transaction(tx, account_match, context, transaction_type, observed_spent) } ManagedAccountRefMut::Keys(a) => { a.record_transaction(tx, account_match, context, transaction_type) @@ -314,16 +321,20 @@ impl<'a> ManagedAccountRefMut<'a> { /// /// Funds variants additionally refresh UTXO state. Returns the updated /// record only when confirmation status actually changes. - pub fn confirm_transaction( + /// + /// `observed_spent` is the wallet-level `observed_spent_outpoints` view + /// (dashpay/rust-dashcore#649); only the funds variant consults it. + pub(crate) fn confirm_transaction( &mut self, tx: &Transaction, account_match: &AccountMatch, context: TransactionContext, transaction_type: TransactionType, + observed_spent: &BTreeMap, ) -> Option { match self { ManagedAccountRefMut::Funds(a) => { - a.confirm_transaction(tx, account_match, context, transaction_type) + a.confirm_transaction(tx, account_match, context, transaction_type, observed_spent) } ManagedAccountRefMut::Keys(a) => { a.confirm_transaction(tx, account_match, context, transaction_type) diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index da4c2c13a..6fd200258 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -130,6 +130,16 @@ impl ManagedCoreFundsAccount { self.reservations.release(tx.input.iter().map(|input| &input.previous_output)); } + /// Release any build reservation held on a single `outpoint`. + /// + /// Used when a coin is removed outside the normal spend path — the + /// wallet-level out-of-order guard (dashpay/rust-dashcore#649) — so the + /// reservation is freed immediately instead of lingering until the TTL + /// backstop, matching what [`Self::update_utxos`] does for ordinary spends. + pub(crate) fn release_reservation_for(&self, outpoint: &OutPoint) { + self.reservations.release(std::iter::once(outpoint)); + } + /// Get a reference to the inner keys-account state. pub fn keys(&self) -> &ManagedCoreKeysAccount { &self.keys @@ -145,12 +155,42 @@ impl ManagedCoreFundsAccount { self.spent_outpoints.contains(outpoint) } + /// Register `outpoint` in the account-local spent set so [`Self::update_utxos`] + /// will not re-insert a UTXO for it (its [`Self::is_outpoint_spent`] guard). + /// + /// Used by the wallet-level out-of-order guard (dashpay/rust-dashcore#649), + /// which removes coins whose spend was observed in a block but never recorded + /// as one of this account's transactions — so the normal spend path in + /// `update_utxos` never populated `spent_outpoints` for them, and a + /// reprocessing of the funding transaction (rescan / duplicate delivery) + /// would otherwise resurrect the coin. Marking it here makes that reprocess a + /// no-op within a session; the wallet-level set remains the source of truth + /// across a serialize/deserialize (where this derived set is rebuilt from + /// recorded transactions and would not include the unrecorded spend). + pub(crate) fn mark_outpoint_spent(&mut self, outpoint: OutPoint) { + self.spent_outpoints.insert(outpoint); + } + + /// Test-only: rebuild `spent_outpoints` exactly the way [`Deserialize`] + /// does, to simulate a save/reload cycle for cross-restart tests. A real + /// full-struct serde round-trip is blocked for a populated account by + /// `AddressPool::script_pubkey_index` (`HashMap`, not a valid + /// JSON map key), so this mirrors the same reconstruction logic directly. + #[cfg(test)] + pub(crate) fn simulate_reload_rebuild_spent_outpoints(&mut self) { + self.spent_outpoints = rebuild_spent_outpoints(&self.keys); + } + /// Add new UTXOs for received outputs, remove spent ones. + /// + /// Skips any output whose outpoint is already in `observed_spent` — it is + /// spent on-chain (dashpay/rust-dashcore#649), so the record stays consistent. fn update_utxos( &mut self, tx: &Transaction, account_match: &AccountMatch, context: TransactionContext, + observed_spent: &BTreeMap, ) { // Update UTXOs only for spendable account types match self.keys.managed_account_type() { @@ -223,6 +263,18 @@ impl ManagedCoreFundsAccount { continue; } + // #649 spend-first ordering: the spend was observed in an + // earlier-processed block, so this output is genuinely spent + // on-chain even though this account has never seen it before — + // never insert it, so the record built below is born correct. + if observed_spent.contains_key(&outpoint) { + tracing::debug!( + outpoint = %outpoint, + "Skipping UTXO already observed spent in an earlier-processed block (#649)" + ); + continue; + } + // Flag outputs from a "trusted" mempool transaction we created — // one whose inputs all spend our own final UTXOs and which pays // this output back to one of our internal (change) addresses. @@ -308,6 +360,7 @@ impl ManagedCoreFundsAccount { account_match: &AccountMatch, context: TransactionContext, transaction_type: TransactionType, + observed_spent: &BTreeMap, ) -> Option { let txid = tx.txid(); @@ -320,7 +373,13 @@ impl ManagedCoreFundsAccount { if !self.keys.has_transaction(&txid) { // Genuinely new sighting — delegate to record_transaction // (which handles finalize-on-record itself). - let record = self.record_transaction(tx, account_match, context, transaction_type); + let record = self.record_transaction( + tx, + account_match, + context, + transaction_type, + observed_spent, + ); return Some(record); } @@ -359,7 +418,7 @@ impl ManagedCoreFundsAccount { // chainlock catches up. #[cfg(not(feature = "keep-finalized-transactions"))] let drop_now = context.is_chain_locked(); - self.update_utxos(tx, account_match, context); + self.update_utxos(tx, account_match, context, observed_spent); #[cfg(not(feature = "keep-finalized-transactions"))] if drop_now { self.keys.drop_finalized_transaction(&txid); @@ -367,13 +426,19 @@ impl ManagedCoreFundsAccount { record_after } - /// Record a new transaction and update UTXOs for spendable account types + /// Record a new transaction and update UTXOs for spendable account types. + /// + /// `observed_spent` is the wallet-level `observed_spent_outpoints` view + /// (dashpay/rust-dashcore#649), read-only from `check_core_transaction`. + /// The freshly built record is compensated against it before insertion — + /// see [`TransactionRecord::compensate_for_observed_spends`]. pub(crate) fn record_transaction( &mut self, tx: &Transaction, account_match: &AccountMatch, context: TransactionContext, transaction_type: TransactionType, + observed_spent: &BTreeMap, ) -> TransactionRecord { let net_amount = account_match.received as i64 - account_match.sent as i64; @@ -404,10 +469,11 @@ impl ManagedCoreFundsAccount { } } - // Use both UTXO-based input details and `account_match.sent` as signals - // that we created this transaction. The UTXO set may be incomplete - // (e.g., partial rescan) so `account_match.sent > 0` catches cases where - // the transaction still spent our funds even without matching UTXOs. + // Marks a transaction that spends our coins. `input_details` and + // `account_match.sent` both derive from the same `self.utxos` lookup on + // this account, so they populate together and cannot diverge — either + // signal alone is sufficient (see + // `scenario2_sent_and_input_details_cannot_diverge_in_current_code`). let has_inputs = !input_details.is_empty() || account_match.sent > 0; let network = self.keys.network(); @@ -458,7 +524,7 @@ impl ManagedCoreFundsAccount { TransactionDirection::Incoming }; - let tx_record = TransactionRecord::new( + let mut tx_record = TransactionRecord::new( tx.clone(), self.keys.managed_account_type().to_account_type(), context.clone(), @@ -468,6 +534,9 @@ impl ManagedCoreFundsAccount { output_details, net_amount, ); + // #649: drop outputs already in observed_spent from output_details/net_amount + // before insert, so the record is consistent with the observed spend. + tx_record.compensate_for_observed_spends(observed_spent); let record = tx_record.clone(); let txid = tx.txid(); @@ -479,7 +548,7 @@ impl ManagedCoreFundsAccount { // feature is on (we want to keep the full record). #[cfg(not(feature = "keep-finalized-transactions"))] let drop_now = context.is_chain_locked(); - self.update_utxos(tx, account_match, context); + self.update_utxos(tx, account_match, context, observed_spent); #[cfg(not(feature = "keep-finalized-transactions"))] if drop_now { self.keys.drop_finalized_transaction(&txid); @@ -754,6 +823,21 @@ impl ManagedAccountTrait for ManagedCoreFundsAccount { } } +/// Rebuild the account-local `spent_outpoints` set from recorded transactions. +/// +/// Every input of every recorded transaction is a spend this account has seen, +/// so its `previous_output` belongs in the derived set. The field is not +/// persisted (`#[serde(skip)]`), so both [`Deserialize`] and the test reload +/// simulation reconstruct it through here to stay in lockstep. +#[cfg(any(feature = "serde", test))] +fn rebuild_spent_outpoints(keys: &ManagedCoreKeysAccount) -> HashSet { + keys.transactions() + .values() + .flat_map(|record| &record.transaction.input) + .map(|input| input.previous_output) + .collect() +} + #[cfg(feature = "serde")] impl<'de> Deserialize<'de> for ManagedCoreFundsAccount { fn deserialize(deserializer: D) -> Result @@ -769,13 +853,7 @@ impl<'de> Deserialize<'de> for ManagedCoreFundsAccount { let helper = Helper::deserialize(deserializer)?; - let spent_outpoints = helper - .keys - .transactions() - .values() - .flat_map(|record| &record.transaction.input) - .map(|input| input.previous_output) - .collect(); + let spent_outpoints = rebuild_spent_outpoints(&helper.keys); Ok(ManagedCoreFundsAccount { keys: helper.keys, diff --git a/key-wallet/src/managed_account/transaction_record.rs b/key-wallet/src/managed_account/transaction_record.rs index b51aee6f1..478a339eb 100644 --- a/key-wallet/src/managed_account/transaction_record.rs +++ b/key-wallet/src/managed_account/transaction_record.rs @@ -8,10 +8,12 @@ use crate::error::Error; use crate::transaction_checking::transaction_router::TransactionType; use crate::transaction_checking::{BlockInfo, TransactionContext}; use crate::Address; -use dashcore::blockdata::transaction::Transaction; +use dashcore::blockdata::transaction::{OutPoint, Transaction}; +use dashcore::prelude::CoreBlockHeight; use dashcore::Txid; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; /// Maximum length of a transaction label in bytes. pub const MAX_LABEL_LENGTH: usize = 256; @@ -197,6 +199,33 @@ impl TransactionRecord { pub fn amount(&self) -> u64 { self.net_amount.unsigned_abs() } + + /// Drop any `Received`/`Change` [`OutputDetail`] whose outpoint is in + /// `observed_spent` and recompute `net_amount` from the surviving details + /// (dashpay/rust-dashcore#649): such an output was spent on-chain, so it is + /// not live received value. Recomputes declaratively (never a delta), so + /// re-running it on an already-compensated record is a no-op. + pub(crate) fn compensate_for_observed_spends( + &mut self, + observed_spent: &BTreeMap, + ) { + let txid = self.txid; + self.output_details.retain(|detail| { + !matches!(detail.role, OutputRole::Received | OutputRole::Change) + || !observed_spent.contains_key(&OutPoint { + txid, + vout: detail.index, + }) + }); + let received: u64 = self + .output_details + .iter() + .filter(|d| matches!(d.role, OutputRole::Received | OutputRole::Change)) + .map(|d| d.value) + .sum(); + let sent: u64 = self.input_details.iter().map(|d| d.value).sum(); + self.net_amount = received as i64 - sent as i64; + } } #[cfg(test)] @@ -234,6 +263,77 @@ mod tests { ) } + /// Pins that `compensate_for_observed_spends` is declarative, not + /// incremental (dashpay/rust-dashcore#649): calling it twice in a row on the + /// SAME record (as a rescan rebuilding an already-compensated record would) + /// is a complete no-op the second time — no double-drop of a surviving + /// output, no `net_amount` drift — because it recomputes from + /// `output_details`/`input_details` on every call rather than subtracting a + /// delta. + #[test] + fn compensate_for_observed_spends_is_idempotent_on_direct_repeated_calls() { + let tx = Transaction::dummy_empty(); + let txid = tx.txid(); + let mut record = TransactionRecord::new( + tx, + test_account_type(), + TransactionContext::Mempool, + TransactionType::Standard, + TransactionDirection::Incoming, + Vec::new(), + vec![ + OutputDetail { + index: 0, + role: OutputRole::Received, + address: None, + value: 2_000_000, + }, + OutputDetail { + index: 1, + role: OutputRole::Received, + address: None, + value: 500_000, + }, + ], + 2_500_000, + ); + + let mut observed_spent = BTreeMap::new(); + observed_spent.insert( + OutPoint { + txid, + vout: 0, + }, + 200u32, + ); + + record.compensate_for_observed_spends(&observed_spent); + assert_eq!(record.net_amount, 500_000, "output 0 compensated away, output 1 survives"); + assert_eq!(record.output_details.len(), 1); + assert_eq!(record.output_details[0].index, 1); + + // Second call, same observed_spent map, same record: must be a + // complete no-op, not a second compensation of output 1. + record.compensate_for_observed_spends(&observed_spent); + assert_eq!(record.net_amount, 500_000, "second call must not drift net_amount"); + assert_eq!(record.output_details.len(), 1, "surviving output must not be dropped"); + assert_eq!(record.output_details[0].index, 1); + + // Third call with an EXPANDED observed_spent set (output 1 now also + // spent) proves the recompute is driven by current input state, not + // cached from the first call. + observed_spent.insert( + OutPoint { + txid, + vout: 1, + }, + 201u32, + ); + record.compensate_for_observed_spends(&observed_spent); + assert_eq!(record.net_amount, 0, "newly-observed second spend is compensated in this pass"); + assert!(record.output_details.is_empty()); + } + #[test] fn test_transaction_record_creation() { let tx = Transaction::dummy_empty(); diff --git a/key-wallet/src/test_utils/blocks.rs b/key-wallet/src/test_utils/blocks.rs new file mode 100644 index 000000000..8b6d62f75 --- /dev/null +++ b/key-wallet/src/test_utils/blocks.rs @@ -0,0 +1,64 @@ +//! Block-context and synthetic-transaction helpers shared across +//! transaction-checking tests. + +use dashcore::blockdata::transaction::OutPoint; +use dashcore::{Address, Network, ScriptBuf, Transaction, TxIn, TxOut, Witness}; + +use crate::transaction_checking::{BlockInfo, TransactionContext}; +use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use crate::wallet::managed_wallet_info::ManagedWalletInfo; + +/// A block-confirmed context at `height` with a height-derived block hash. +pub fn block_ctx(height: u32) -> TransactionContext { + TransactionContext::InBlock(BlockInfo::new( + height, + dashcore::BlockHash::dummy(height), + 1_650_000_000 + height, + )) +} + +/// A chain-locked block context at `height` — the strongest finality signal, +/// which drops the transaction's full record under the default +/// `keep-finalized-transactions = OFF` feature. A full historical rescan +/// delivers old blocks already chainlocked, so this is the realistic "first +/// sighting" context for a coin funded and spent long ago. +pub fn chain_locked_block_ctx(height: u32) -> TransactionContext { + TransactionContext::InChainLockedBlock(BlockInfo::new( + height, + dashcore::BlockHash::dummy(height), + 1_650_000_000 + height, + )) +} + +/// A transaction spending `inputs` and paying `value` to an unrelated external +/// address (`ext_id` selects a distinct dummy address). No change back to us. +pub fn spend_to_external(inputs: &[OutPoint], value: u64, ext_id: usize) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: inputs + .iter() + .map(|op| TxIn { + previous_output: *op, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }) + .collect(), + output: vec![TxOut { + value, + script_pubkey: Address::dummy(Network::Testnet, ext_id).script_pubkey(), + }], + special_transaction_payload: None, + } +} + +/// Does any funding account currently hold a live UTXO at `outpoint`? +pub fn utxo_tracked(wallet: &ManagedWalletInfo, outpoint: &OutPoint) -> bool { + wallet.accounts.all_funding_accounts().iter().any(|a| a.utxos.contains_key(outpoint)) +} + +/// Sum of `net_amount` across the wallet's live transaction history. +pub fn history_net(wallet: &ManagedWalletInfo) -> i64 { + wallet.transaction_history().iter().map(|r| r.net_amount).sum() +} diff --git a/key-wallet/src/test_utils/mod.rs b/key-wallet/src/test_utils/mod.rs index d9de8de38..86f021a75 100644 --- a/key-wallet/src/test_utils/mod.rs +++ b/key-wallet/src/test_utils/mod.rs @@ -1,5 +1,7 @@ mod account; +mod blocks; mod utxo; mod wallet; +pub use blocks::{block_ctx, chain_locked_block_ctx, history_net, spend_to_external, utxo_tracked}; pub use wallet::TestWalletContext; diff --git a/key-wallet/src/tests/async_chainlock_prune_race_test.rs b/key-wallet/src/tests/async_chainlock_prune_race_test.rs new file mode 100644 index 000000000..762902246 --- /dev/null +++ b/key-wallet/src/tests/async_chainlock_prune_race_test.rs @@ -0,0 +1,124 @@ +//! Pins the async ChainLock-prune vs. out-of-order-spend race for the #649 +//! restructure (dashpay/rust-dashcore#649). +//! +//! `apply_chain_lock` is driven asynchronously, decoupled from funding/spend +//! processing. Scenario: a funding transaction is recorded `InBlock` (record +//! fully live), a ChainLock event arrives and prunes the record to +//! `finalized_txids` under the default `keep-finalized-transactions = OFF` +//! feature, and only THEN does an out-of-order, unattributed spend of that coin +//! arrive — so `finalize_guard_removed_utxo` looks up a record that has, in the +//! interim, ceased to exist. +//! +//! The record's absence is a no-op for the declarative (not incremental) +//! recompute, and balance correctness never depended on the record. This test +//! constructs the exact race and checks: no panic, no phantom balance, and +//! reprocessing safety (the account-local spent marker is still set even though +//! the record lookup was a no-op). +//! +//! The scenario is specific to the default `keep-finalized-transactions = OFF` +//! pruning path (see +//! `plain_chainlocked_funding_diverges_history_from_balance_by_design` in +//! `observed_spent_outpoints_tests.rs`): with the feature on, records are never +//! pruned and this race cannot occur, so the module is gated out entirely under +//! `--features keep-finalized-transactions`. +#![cfg(not(feature = "keep-finalized-transactions"))] + +use crate::managed_account::managed_account_trait::ManagedAccountTrait; +use crate::test_utils::{ + block_ctx, history_net, spend_to_external, utxo_tracked, TestWalletContext, +}; +use crate::transaction_checking::{TransactionRouter, TransactionType}; +use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use dashcore::blockdata::transaction::special_transaction::asset_lock::AssetLockPayload; +use dashcore::blockdata::transaction::special_transaction::TransactionPayload; +use dashcore::blockdata::transaction::OutPoint; +use dashcore::ephemerealdata::chain_lock::ChainLock; +use dashcore::Transaction; + +#[tokio::test] +async fn async_chain_lock_prune_races_unattributed_spend() { + let mut ctx = TestWalletContext::new_random(); + + let cj_xpub = ctx.wallet.get_coinjoin_account(0).expect("coinjoin account").account_xpub; + let cj_address = ctx + .managed_wallet + .first_coinjoin_managed_account_mut() + .expect("managed coinjoin account") + .next_address(Some(&cj_xpub), true) + .expect("coinjoin address"); + + let funding_value = 1_700_000u64; + let funding_tx = Transaction::dummy(&cj_address, 0..1, &[funding_value]); + let funding_txid = funding_tx.txid(); + let funding_outpoint = OutPoint::new(funding_txid, 0); + + // Funding recorded InBlock — NOT chainlocked at first sighting, so the + // record is fully live (unlike a chainlocked-first-sighting funding). + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!(utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "coin live after funding"); + assert!( + ctx.managed_wallet + .first_coinjoin_managed_account() + .expect("account") + .transactions() + .contains_key(&funding_txid), + "record must be live before the chainlock — not a chainlocked-first-sighting funding" + ); + + // Async chain-lock event prunes the record — entirely decoupled from any + // spend processing. + ctx.managed_wallet.update_last_processed_height(100); + let outcome = ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(100)); + assert!(!outcome.locked_transactions.is_empty(), "the funding record must have been promoted"); + assert!( + !ctx.managed_wallet + .first_coinjoin_managed_account() + .expect("account") + .transactions() + .contains_key(&funding_txid), + "record pruned by the chainlock BEFORE any spend is known to the wallet" + ); + assert!( + utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "the coin remains live in the UTXO set — balance never depended on the record" + ); + + // The out-of-order, unattributed spend now arrives (AssetLock-classified, + // routes away from the CoinJoin account — same routing case as + // `coinjoin_utxo_spent_by_asset_lock_classified_tx_still_invalidated` in + // observed_spent_outpoints_tests.rs) — but this time the funding record + // is ALREADY gone, which `finalize_guard_removed_utxo` must tolerate. + let mut spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 120); + spend_tx.special_transaction_payload = + Some(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new(vec![]))); + assert_eq!( + TransactionRouter::classify_transaction(&spend_tx), + TransactionType::AssetLock, + "spend must classify as AssetLock for the unattributed routing gap to bite" + ); + + // Must not panic on a record-lookup miss. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "coin correctly invalidated regardless of the record's prior pruning" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no phantom balance"); + assert_eq!( + history_net(&ctx.managed_wallet), + 0, + "no live record survives to contribute to history — pruned by design, not a #649 defect" + ); + + // Reprocessing the now-fully-spent funding transaction (rescan/duplicate + // delivery) must not resurrect the coin: `finalize_guard_removed_utxo` + // must still call `mark_outpoint_spent` even though the record lookup + // was a no-op, so `update_utxos`'s guard rejects the reinsertion. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "reprocessing the funding after the pruned-record race must not resurrect the coin" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "balance stays zero across reprocessing"); +} diff --git a/key-wallet/src/tests/late_account_finality_boundary_test.rs b/key-wallet/src/tests/late_account_finality_boundary_test.rs new file mode 100644 index 000000000..eb55fc805 --- /dev/null +++ b/key-wallet/src/tests/late_account_finality_boundary_test.rs @@ -0,0 +1,261 @@ +//! Late-added-account interaction with finality-boundary pruning +//! (dashpay/rust-dashcore#649). +//! +//! `synced_height` certifies "every filter at or below this height was matched +//! against the wallet's scripts" — but only for the account set that existed +//! while those filters were scanned. Adding an account grows the script set +//! without invalidating that certificate, so `prune_finalized_observed_spends` +//! could consume a stale certificate and evict the only protection a +//! newly-added account has for a coin spent before it existed. +//! +//! The fix rewinds `synced_height` to just below wallet birth on every account +//! addition, collapsing the prune boundary until the sync layer re-certifies +//! the new account set by backfilling from birth. These tests pin that rewind +//! and the convergence of the backfill replay. A transient window (funding +//! replayed before its spend) is accepted — the same shape as the other +//! documented residuals; the durable resurrection is what is eliminated. + +use dashcore::blockdata::transaction::{OutPoint, Transaction}; +use dashcore::ephemerealdata::chain_lock::ChainLock; + +use crate::account::{AccountType, StandardAccountType}; +use crate::managed_account::ManagedCoreFundsAccount; +use crate::test_utils::{ + block_ctx, chain_locked_block_ctx, spend_to_external, utxo_tracked, TestWalletContext, +}; +use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use crate::wallet::managed_wallet_info::ManagedAccountOperations; + +const BIRTH: u32 = 50; + +fn bip44(index: u32) -> AccountType { + AccountType::Standard { + index, + standard_account_type: StandardAccountType::BIP44Account, + } +} + +/// Add BIP44 account `index` to the wallet and derive its first receive address +/// (deterministic, so the funding tx can be built before the managed account is +/// added). Returns `(account_type, receive_address)`. +fn prepare_account(ctx: &mut TestWalletContext, index: u32) -> (AccountType, dashcore::Address) { + ctx.wallet.add_account(bip44(index), None).expect("add wallet account"); + let account = ctx.wallet.get_bip44_account(index).expect("wallet account"); + let xpub = account.account_xpub; + let mut detached = ManagedCoreFundsAccount::from_account(account); + let address = detached.next_receive_address(Some(&xpub), true).expect("receive address"); + (bip44(index), address) +} + +/// The certificate is invalidated the moment the account set grows: adding an +/// account rewinds `synced_height` to just below birth, so a later chainlock +/// prunes nothing (the boundary has collapsed). +#[tokio::test] +async fn late_added_account_rewinds_sync_checkpoint() { + let mut ctx = TestWalletContext::new_random(); + ctx.managed_wallet.metadata.birth_height = BIRTH; + + let (type1, addr1) = prepare_account(&mut ctx, 1); + let funding_value = 2_500_000u64; + let funding_tx = Transaction::dummy(&addr1, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 900); + + // Observe the spend and prune, all while account 1 is unknown. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + assert!(!ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Adding the account rewinds the certificate to birth - 1. + ctx.managed_wallet.add_managed_account(&ctx.wallet, type1).expect("add managed account 1"); + assert_eq!( + ctx.managed_wallet.synced_height(), + BIRTH - 1, + "adding an account rewinds synced_height to just below birth" + ); + + // A later chainlock now prunes nothing: the boundary is min(cl, synced) and + // synced has collapsed to birth - 1. + let stale_outpoint = OutPoint::new(dashcore::Txid::from([0x5A; 32]), 0); + ctx.managed_wallet.record_observed_spends(&spend_to_external(&[stale_outpoint], 1, 5), 60); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(250)); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&stale_outpoint), + "with synced rewound to birth-1, an entry at height 60 is not prunable despite cl=250" + ); +} + +/// The rewind drives a backfill (modeled here as a forward replay). Delivering +/// the funding then the spend converges: no live UTXO, zero balance, and the +/// repopulated entry is re-prunable once the checkpoint re-advances. +#[tokio::test] +async fn late_added_account_converges_after_forward_replay() { + let mut ctx = TestWalletContext::new_random(); + ctx.managed_wallet.metadata.birth_height = BIRTH; + + let (type1, addr1) = prepare_account(&mut ctx, 1); + let funding_value = 2_500_000u64; + let funding_tx = Transaction::dummy(&addr1, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 901); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + ctx.managed_wallet.add_managed_account(&ctx.wallet, type1).expect("add managed account 1"); + + // Forward replay from birth: funding (born-chainlocked, since 100 <= cl 200) + // then its spend. + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(100)).await; + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "after the full replay the coin is reconciled away" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no resurrected coin in the balance"); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "the spend replay repopulated the wallet-level entry" + ); + + // Re-advancing the checkpoint past the spend re-certifies coverage, so the + // repopulated entry becomes prunable again. + ctx.managed_wallet.update_synced_height(200); + assert!( + !ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "entry re-prunable once synced_height honestly re-advances past the spend" + ); + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "the finalized record short-circuits any further funding redelivery" + ); +} + +/// Mid-backfill pruning cannot reopen the hole: while the checkpoint has only +/// partially re-advanced (below the spend height), the repopulated entry sits +/// above the boundary and survives pruning, so a redelivered funding stays +/// guarded. +#[tokio::test] +async fn late_added_account_interrupted_replay_entry_not_prunable() { + let mut ctx = TestWalletContext::new_random(); + ctx.managed_wallet.metadata.birth_height = BIRTH; + + let (type1, addr1) = prepare_account(&mut ctx, 1); + let funding_value = 2_500_000u64; + let funding_tx = Transaction::dummy(&addr1, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 902); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + ctx.managed_wallet.add_managed_account(&ctx.wallet, type1).expect("add managed account 1"); + + // Replay funding + spend; entry (O, 200) repopulated. + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(100)).await; + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Partial backfill: checkpoint advances only to 150, below the spend at 200. + // Pruning must not evict the entry (200 > boundary min(cl 200, 150) = 150). + ctx.managed_wallet.update_synced_height(150); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "a repopulated entry above the partial-backfill boundary must survive pruning" + ); + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "mid-backfill funding redelivery stays guarded — the hole cannot reopen" + ); + + // Completing the backfill re-certifies coverage and the entry prunes safely. + ctx.managed_wallet.update_synced_height(200); + assert!(!ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "converged to zero balance"); +} + +/// Every add variant rewinds the certificate; adding during initial sync is a +/// no-op; `birth_height == 0` does not underflow. +#[tokio::test] +async fn every_add_variant_rewinds_checkpoint() { + let mut ctx = TestWalletContext::new_random(); + ctx.managed_wallet.metadata.birth_height = BIRTH; + + // Baseline: certify coverage well above birth. + ctx.managed_wallet.update_synced_height(1_000); + assert_eq!(ctx.managed_wallet.synced_height(), 1_000); + + // add_managed_account (funds, from wallet). + ctx.wallet.add_account(bip44(1), None).expect("wallet account 1"); + ctx.managed_wallet.add_managed_account(&ctx.wallet, bip44(1)).expect("add managed 1"); + assert_eq!(ctx.managed_wallet.synced_height(), BIRTH - 1, "add_managed_account rewinds"); + + // Re-certify, then add_managed_account_from_xpub. + ctx.managed_wallet.update_synced_height(1_000); + ctx.wallet.add_account(bip44(2), None).expect("wallet account 2"); + let xpub2 = ctx.wallet.get_bip44_account(2).expect("account 2").account_xpub; + ctx.managed_wallet + .add_managed_account_from_xpub(bip44(2), xpub2) + .expect("add managed from xpub"); + assert_eq!( + ctx.managed_wallet.synced_height(), + BIRTH - 1, + "add_managed_account_from_xpub rewinds" + ); + + // No-op during initial sync: synced already at/below the floor. + ctx.managed_wallet.update_synced_height(BIRTH - 1); + ctx.wallet.add_account(bip44(3), None).expect("wallet account 3"); + ctx.managed_wallet.add_managed_account(&ctx.wallet, bip44(3)).expect("add managed 3"); + assert_eq!( + ctx.managed_wallet.synced_height(), + BIRTH - 1, + "adding during initial sync leaves the checkpoint untouched" + ); + + // birth_height == 0 saturates rather than underflowing. + let mut ctx0 = TestWalletContext::new_random(); + ctx0.managed_wallet.metadata.birth_height = 0; + ctx0.managed_wallet.update_synced_height(1_000); + ctx0.wallet.add_account(bip44(1), None).expect("wallet account 1"); + ctx0.managed_wallet.add_managed_account(&ctx0.wallet, bip44(1)).expect("add managed 1"); + assert_eq!( + ctx0.managed_wallet.synced_height(), + 0, + "birth_height == 0 rewinds to 0 without underflow" + ); +} + +/// Marvin QA-001 round 2: two accounts added back-to-back with NO intervening +/// rescan/re-certification (no `update_synced_height` call between the two +/// adds) — the race the task explicitly flagged. Since the rewind floor is +/// wallet-level `birth_height` (identical for every account, not a per-account +/// value), the second rewind's `min` is idempotent against the first: it must +/// land at exactly `birth_height - 1`, never lower (no double-decrement) and +/// never higher (no accidental un-rewind). +#[tokio::test] +async fn back_to_back_account_adds_rewind_idempotently() { + let mut ctx = TestWalletContext::new_random(); + ctx.managed_wallet.metadata.birth_height = BIRTH; + ctx.managed_wallet.update_synced_height(1_000); + + ctx.wallet.add_account(bip44(1), None).expect("wallet account 1"); + ctx.managed_wallet.add_managed_account(&ctx.wallet, bip44(1)).expect("add managed 1"); + assert_eq!(ctx.managed_wallet.synced_height(), BIRTH - 1, "first add rewinds to birth - 1"); + + // Second add fires immediately after, with no rescan tick / re-certify in + // between — exactly the in-flight sequencing the task asked about. + ctx.wallet.add_account(bip44(2), None).expect("wallet account 2"); + ctx.managed_wallet.add_managed_account(&ctx.wallet, bip44(2)).expect("add managed 2"); + assert_eq!( + ctx.managed_wallet.synced_height(), + BIRTH - 1, + "second back-to-back add is idempotent: still exactly birth - 1, not further reduced" + ); +} diff --git a/key-wallet/src/tests/mod.rs b/key-wallet/src/tests/mod.rs index acb7a6378..8feb0cdf7 100644 --- a/key-wallet/src/tests/mod.rs +++ b/key-wallet/src/tests/mod.rs @@ -20,6 +20,8 @@ mod keep_finalized_transactions_tests; mod managed_account_collection_tests; +mod late_account_finality_boundary_test; + mod performance_tests; mod provider_key_derivation_tests; @@ -30,6 +32,12 @@ mod special_transaction_tests; mod transaction_tests; +mod observed_spent_outpoints_tests; + +mod async_chainlock_prune_race_test; + +mod net_amount_source_of_truth_test; + mod spent_outpoints_tests; mod unit_variant_wallet_tests; diff --git a/key-wallet/src/tests/net_amount_source_of_truth_test.rs b/key-wallet/src/tests/net_amount_source_of_truth_test.rs new file mode 100644 index 000000000..09c780b1d --- /dev/null +++ b/key-wallet/src/tests/net_amount_source_of_truth_test.rs @@ -0,0 +1,455 @@ +//! Pins `net_amount`'s source of truth after the #649 restructure +//! (dashpay/rust-dashcore#649). `record_transaction` now calls +//! `TransactionRecord::compensate_for_observed_spends` unconditionally, so +//! `net_amount` is a recompute driven by `output_details`/`input_details` +//! rather than `account_match.received - account_match.sent`. The two are equal +//! in the normal flow; the tests below pin (a) that equivalence in the common +//! case, and (b) the one constructed condition where they differ +//! (`contains_script_pub_key` matches a scriptPubKey whose address then fails +//! `get_address_info` resolution). + +use crate::managed_account::address_pool::PublicKeyType; +use crate::managed_account::managed_account_trait::ManagedAccountTrait; +use crate::test_utils::{block_ctx, TestWalletContext}; +use dashcore::blockdata::transaction::OutPoint; +use dashcore::{Address, PublicKey, ScriptBuf, Transaction, TxIn, TxOut, Txid, Witness}; + +/// A synthetic-input transaction paying each `(address, value)` pair in +/// `targets` to its own output, in order. Unlike `Transaction::dummy` (which +/// sends every output to the SAME address), this allows constructing a +/// multi-recipient funding transaction. +fn fund_multi(targets: &[(&Address, u64)]) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint::new(Txid::from([0xABu8; 32]), 0), + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: targets + .iter() + .map(|(addr, value)| TxOut { + value: *value, + script_pubkey: addr.script_pubkey(), + }) + .collect(), + special_transaction_payload: None, + } +} + +// ── (a) equivalence in the common case (observed_spent empty) ───────────── + +/// Incoming funding: `net_amount` must equal `account_match.received - +/// account_match.sent`, computed independently via the same +/// `check_transaction_for_match` `record_transaction` itself calls. +#[tokio::test] +async fn net_amount_matches_account_match_formula_for_incoming_funding() { + let mut ctx = TestWalletContext::new_random(); + let funding_value = 1_234_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + + let account_match = ctx + .managed_wallet + .first_bip44_managed_account() + .expect("account") + .check_transaction_for_match(&funding_tx, Some(0)) + .expect("funding must match the account"); + let expected_net = account_match.received as i64 - account_match.sent as i64; + assert_eq!(expected_net, funding_value as i64, "sanity: account_match sees the funding"); + + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + let record = ctx.transaction(&funding_tx.txid()); + assert_eq!( + record.net_amount, expected_net, + "net_amount must equal account_match.received - account_match.sent with an empty \ + observed_spent set — the unconditional compensate_for_observed_spends call must not \ + silently change the common-case value" + ); +} + +/// Outgoing spend to an external address: same equivalence check, this time +/// with `sent > 0` and `received == 0`. +#[tokio::test] +async fn net_amount_matches_account_match_formula_for_outgoing_spend() { + let mut ctx = TestWalletContext::new_random(); + let funding_value = 2_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let external = Address::dummy(dashcore::Network::Testnet, 200); + let spend_tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value: funding_value - 1_000, + script_pubkey: external.script_pubkey(), + }], + special_transaction_payload: None, + }; + + // Computed against the SAME pre-spend account state `record_transaction` + // itself observes (input_details/account_match are built before + // `update_utxos` removes the spent coin). + let account_match = ctx + .managed_wallet + .first_bip44_managed_account() + .expect("account") + .check_transaction_for_match(&spend_tx, Some(0)) + .expect("spend must match the account (spends our own UTXO)"); + let expected_net = account_match.received as i64 - account_match.sent as i64; + assert_eq!(expected_net, -(funding_value as i64), "sanity: pure spend, nothing received back"); + + ctx.check_transaction(&spend_tx, block_ctx(101)).await; + + let record = ctx.transaction(&spend_tx.txid()); + assert_eq!( + record.net_amount, expected_net, + "net_amount must equal account_match.received - account_match.sent for an outgoing spend" + ); +} + +/// Self-transfer / consolidation: funding spent entirely into this account's +/// own change address (no fee modeled), so both `received` and `sent` are +/// nonzero and, absent any #649 concern, must cancel out. +#[tokio::test] +async fn net_amount_matches_account_match_formula_for_self_transfer() { + let mut ctx = TestWalletContext::new_random(); + let funding_value = 3_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let change_address = ctx + .managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .next_change_address(Some(&ctx.xpub), true) + .expect("change address"); + let self_transfer_tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value: funding_value, // no fee modeled: exact round-trip + script_pubkey: change_address.script_pubkey(), + }], + special_transaction_payload: None, + }; + + let account_match = ctx + .managed_wallet + .first_bip44_managed_account() + .expect("account") + .check_transaction_for_match(&self_transfer_tx, Some(0)) + .expect("self-transfer must match the account"); + let expected_net = account_match.received as i64 - account_match.sent as i64; + assert_eq!(expected_net, 0, "sanity: exact round-trip self-transfer nets to zero"); + + ctx.check_transaction(&self_transfer_tx, block_ctx(101)).await; + + let record = ctx.transaction(&self_transfer_tx.txid()); + assert_eq!( + record.net_amount, expected_net, + "net_amount must equal account_match.received - account_match.sent for a self-transfer" + ); +} + +// ── (b) the specific divergence condition ──────────────── + +/// Constructs the exact edge case: an address whose scriptPubKey is present +/// in `AddressPool::script_pubkey_index` but whose `address_index` entry is +/// missing, forced via direct field manipulation. (`AddressPool::prune_unused` +/// used to leave exactly this desync behind — but it now keeps both maps in +/// sync, so this exact state is no longer reachable through it; both maps +/// remain independently `pub` fields, so nothing rules the desync out at the +/// type level.) +/// +/// A funding tx pays two outputs: one to a live, tracked address (so the +/// transaction is still recognized as relevant — `has_addresses` requires at +/// least one resolved address or `sent > 0`, and `received > 0` alone does +/// NOT satisfy that in `account_checker.rs`) and one to the desynced address +/// (`contains_script_pub_key` still matches it, but `get_address_info` now +/// returns `None`). +/// +/// Result: `account_match.received` (account_checker.rs:646-665) counts BOTH +/// outputs (the `received += output.value` at line 664 is unconditional on +/// `get_address_info` success), so the OLD pre-restructure net_amount formula +/// would have shown `value0 + value1`. But `update_utxos`'s UTXO-insertion +/// eligibility (`involved_addrs`, built from the SAME `get_address_info`- +/// gated `involved_receive_addresses`/`involved_change_addresses` sets) never +/// inserts a UTXO for the pruned output either — so `balance.total()` was +/// ALREADY only ever going to be `value0` regardless of this diff. The new +/// `compensate_for_observed_spends`-driven recompute (which uses the same +/// `get_address_info`-gated address sets via `record_transaction`'s +/// `receive_addrs`/`change_addrs`) produces `net_amount == value0` — LOWER +/// than the old formula's `value0 + value1`, so it does under-report relative +/// to the old formula, but the new number is the one that agrees with +/// `balance`, not a new inconsistency: +/// the old formula was already reporting money the wallet could never +/// actually spend or see as a UTXO. The restructure does not create a new +/// spendable-fund miscount; it removes a pre-existing (unrelated, +/// `prune_unused`-only-reachable) history/balance mismatch in this exact +/// corner instead of preserving it. +#[tokio::test] +async fn pruned_address_script_desync_does_not_under_report_relative_to_balance() { + let mut ctx = TestWalletContext::new_random(); + + let desynced_address = { + let account = ctx.managed_wallet.first_bip44_managed_account_mut().expect("account"); + let pools = account.managed_account_type_mut().address_pools_mut(); + let external_pool = pools.into_iter().next().expect("external pool"); + + let desynced_address = + external_pool.address_at_index(1).expect("pool pre-generates past index 0"); + assert!(external_pool.contains_address(&desynced_address), "generated, so indexed"); + + // Force the desync directly: prune_unused itself keeps both maps in + // sync now, so removing only the address-keyed entry is the only way + // left to construct this state. + external_pool.address_index.remove(&desynced_address); + + assert!( + !external_pool.contains_address(&desynced_address), + "address_index no longer resolves the desynced address" + ); + assert!( + external_pool.contains_script_pubkey(&desynced_address.script_pubkey()), + "script_pubkey_index still matches — the constructed desync" + ); + desynced_address + }; + + let value0 = 500_000u64; + let value1 = 750_000u64; + let funding_tx = fund_multi(&[(&ctx.receive_address, value0), (&desynced_address, value1)]); + + // The OLD net_amount formula, evaluated for real via the account's own + // (unchanged-by-this-diff) `check_transaction_for_match`. + let account_match = ctx + .managed_wallet + .first_bip44_managed_account() + .expect("account") + .check_transaction_for_match(&funding_tx, Some(0)) + .expect("tx must still be relevant via output 0"); + assert_eq!( + account_match.received, + value0 + value1, + "confirms the divergence mechanism: account_match.received counts the stale \ + script-only match (value1) despite get_address_info failing for it" + ); + let old_formula_net = account_match.received as i64 - account_match.sent as i64; + + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + // Balance was ALWAYS going to exclude value1 — it was never a real UTXO, + // independent of the #649 restructure (update_utxos gates insertion on + // the same get_address_info-derived address sets). + assert_eq!( + ctx.managed_wallet.balance.total(), + value0, + "value1 was never inserted as a spendable UTXO — a pre-existing address_pool gap, \ + not a #649 regression" + ); + + let record = ctx.transaction(&funding_tx.txid()); + assert_eq!( + record.net_amount, value0 as i64, + "the new declarative recompute reports only the actually-spendable value0" + ); + assert_ne!( + record.net_amount, old_formula_net, + "the new number genuinely differs from the old account_match-derived formula \ + in this constructed edge case" + ); + assert_eq!( + record.net_amount, + ctx.managed_wallet.balance.total() as i64, + "critically: the NEW net_amount agrees with balance — the OLD formula (value0+value1) \ + would have been the one diverging from balance, not the new one. The restructure does \ + not introduce a new spendable-fund miscount here; it happens to resolve a pre-existing, \ + prune_unused-only-reachable history/balance mismatch instead of preserving it" + ); +} + +// ── structural bounds on the divergence condition ─ + +/// Pins that a bare P2PK scriptPubKey the account owns cannot reach the +/// divergence condition: it would need to match `contains_script_pub_key` yet +/// fail to resolve via `Address::from_script` / `get_address_info`. Attempts +/// this using the account's OWN derived public key (the most favorable case) +/// built into a real P2PK scriptPubKey via `ScriptBuf::new_p2pk`. +/// +/// Result: UNREACHABLE. `contains_script_pub_key` is an exact byte-match +/// against `AddressPool::script_pubkey_index` +/// (`address_pool.rs::contains_script_pubkey`), and nothing in this crate +/// ever inserts a P2PK-shaped script into that index: `AddressType` +/// (`dash/src/address.rs`) has no P2PK variant (only P2pkh/P2sh/P2wpkh/P2wsh/ +/// P2tr), and `AddressPool::generate_address_at_index` always builds +/// `Address::p2pkh(&dash_pubkey, network)` for the ECDSA path regardless of +/// `address_type`. So `contains_script_pub_key(p2pk_script)` is always +/// `false` for a script this crate did not itself register — and it never +/// registers a P2PK script. The transaction below is not even recognized +/// as touching this account at all (confirmed below), let alone reaching +/// the `received += value` unconditional-counting line. +/// +/// The general class — `contains_script_pub_key` true but `Address::from_script` +/// / `get_address_info` resolution failing — is real and demonstrated with a +/// genuinely constructible trigger in +/// `pruned_address_script_desync_does_not_under_report_relative_to_balance` +/// above (an `AddressPool::prune_unused`-induced `script_pubkey_index` / +/// `address_index` desync). P2PK specifically just isn't how it happens in +/// this codebase. +#[tokio::test] +async fn scenario1_bare_p2pk_is_structurally_unreachable() { + let ctx = TestWalletContext::new_random(); + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let info = account.get_address_info(&ctx.receive_address).expect("own address info"); + let public_key = info.public_key.clone().expect("ECDSA key"); + let PublicKeyType::ECDSA(pubkey_bytes) = &public_key else { + panic!("BIP44 account must derive ECDSA keys"); + }; + let pubkey = PublicKey::from_slice(pubkey_bytes).expect("valid pubkey"); + let p2pk_script = ScriptBuf::new_p2pk(&pubkey); + + assert_ne!( + p2pk_script, + ctx.receive_address.script_pubkey(), + "sanity: P2PK and P2PKH scripts for the same key are byte-distinct" + ); + assert!( + !account.managed_account_type().contains_script_pub_key(&p2pk_script), + "the account's own script_pubkey_index never contains a P2PK-shaped script — \ + confirms contains_script_pub_key cannot match a bare P2PK output for OUR key, \ + so this mechanism cannot trigger the divergence condition" + ); + + let funding_tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint::new(Txid::from([0xEEu8; 32]), 0), + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value: 1_000_000, + script_pubkey: p2pk_script, + }], + special_transaction_payload: None, + }; + assert!( + account.check_transaction_for_match(&funding_tx, Some(0)).is_none(), + "a P2PK-only output to our own key is not even recognized as relevant to the account \ + — confirms the whole scenario never reaches record_transaction/compensate_for_observed_spends" + ); +} + +/// Pins that `account_match.sent > 0` cannot coexist with an empty +/// `input_details` in the current code, so the unconditional recompute never +/// silently subtracts 0 for a real spend. +/// +/// There is exactly ONE `sent =`/`sent +=` assignment site in the entire +/// `account_checker.rs` (`check_transaction_for_match`): +/// `sent = sent.saturating_add(utxo.txout.value)`, gated by +/// `self.utxos.get(&input.previous_output)` — the IDENTICAL predicate, on the +/// SAME `ManagedCoreFundsAccount` instance, that `record_transaction`'s +/// `input_details` loop uses. In `wallet_checker.rs`, `account_match` is +/// computed once via `self.accounts.check_transaction`, then +/// `record_transaction` runs on the SAME account handle with no intervening +/// mutation of `self.utxos` for this transaction. So for the standard +/// funds-account path, `account_match.sent > 0` structurally implies +/// `input_details` is non-empty — they cannot diverge. +/// +/// Demonstrated empirically below: the same "partial rescan" precondition +/// (this account's `utxos` genuinely lacks the spent coin) drives BOTH +/// signals to zero/empty together, never one without the other. +#[tokio::test] +async fn scenario2_sent_and_input_details_cannot_diverge_in_current_code() { + let mut ctx = TestWalletContext::new_random(); + + // Model "partial rescan": a coin this account actually owns on-chain, + // but whose funding was never processed, so `self.utxos` never learned + // about it — the exact condition the code comment names. + let unscanned_outpoint = OutPoint::new(Txid::from([0x77u8; 32]), 3); + let external = Address::dummy(dashcore::Network::Testnet, 210); + let spend_of_unscanned_coin = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: unscanned_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value: 999_000, + script_pubkey: external.script_pubkey(), + }], + special_transaction_payload: None, + }; + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + assert!( + !account.utxos.contains_key(&unscanned_outpoint), + "sanity: the coin is genuinely absent from this account's utxo set" + ); + let match_result = account.check_transaction_for_match(&spend_of_unscanned_coin, Some(0)); + assert!( + match_result.is_none_or(|m| m.sent == 0), + "when self.utxos lacks the coin, account_match.sent is ALSO 0 (not '> 0 with empty \ + input_details') — sent and input_details are governed by the identical predicate \ + and cannot diverge in the current code" + ); + + // Contrast: once the SAME coin is genuinely funded and tracked, both + // signals become nonzero together — never independently. + let funding_value = 999_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funding_outpoint, + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: vec![TxOut { + value: funding_value - 1_000, + script_pubkey: external.script_pubkey(), + }], + special_transaction_payload: None, + }; + let account_match = ctx + .managed_wallet + .first_bip44_managed_account() + .expect("account") + .check_transaction_for_match(&spend_tx, Some(0)) + .expect("must match — the coin is tracked"); + assert_eq!(account_match.sent, funding_value, "tracked coin: sent is populated"); + ctx.check_transaction(&spend_tx, block_ctx(101)).await; + let record = ctx.transaction(&spend_tx.txid()); + assert_eq!( + record.input_details.len(), + 1, + "input_details is populated in lockstep with account_match.sent, never independently" + ); +} diff --git a/key-wallet/src/tests/observed_spent_outpoints_tests.rs b/key-wallet/src/tests/observed_spent_outpoints_tests.rs new file mode 100644 index 000000000..101ae433f --- /dev/null +++ b/key-wallet/src/tests/observed_spent_outpoints_tests.rs @@ -0,0 +1,1573 @@ +//! Tests for the wallet-level observed-spent-outpoint guard +//! (dashpay/rust-dashcore#649). +//! +//! Issue #649: a spend delivered out of order (before the transaction that +//! funded the coin it spends, as happens during a scrambled rescan) left the +//! funding UTXO permanently tracked once its funding block was finally +//! processed. The fix records every outpoint observed spent in a processed +//! block into a wallet-level, classification-independent, persisted set +//! (`ManagedWalletInfo::observed_spent_outpoints`) and reconciles it against +//! both the spend side (drop the coin whichever account holds it) and the +//! funding side (drop a just-inserted output already observed spent). +//! +//! Invariants exercised here: +//! - **K-INV-649** (primary): an outpoint observed spent in a block is never +//! present in any account's live `utxos`, in either delivery order. +//! - **K-INV-649-PERSIST**: the invalidation survives a serialize/deserialize +//! (restart) cycle. +//! - **K-INV-649-BOUND**: set growth is bounded by the inputs of matched +//! blocks — no dedup loss, no superlinear cost. +//! - **K-INV-649-SCOPE**: only block-confirmed contexts populate the set; +//! mempool / InstantSend-only spends do not, and self-heal on confirmation. +//! - **K-INV-649-NOREGRESS**: in-order matched spends behave exactly as before. + +use std::collections::BTreeMap; +use std::time::Instant; + +use dashcore::blockdata::transaction::special_transaction::asset_lock::AssetLockPayload; +use dashcore::blockdata::transaction::special_transaction::TransactionPayload; +use dashcore::blockdata::transaction::{OutPoint, Transaction}; +use dashcore::ephemerealdata::chain_lock::ChainLock; +use dashcore::ephemerealdata::instant_lock::InstantLock; +use dashcore::prelude::CoreBlockHeight; +use dashcore::Network; + +use crate::account::{AccountType, StandardAccountType}; +use crate::managed_account::managed_account_trait::ManagedAccountTrait; +use crate::managed_account::ManagedCoreFundsAccount; +use crate::test_utils::{ + block_ctx, chain_locked_block_ctx, history_net, spend_to_external, utxo_tracked, + TestWalletContext, +}; +use crate::transaction_checking::{TransactionContext, TransactionRouter, TransactionType}; +use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use crate::wallet::managed_wallet_info::{ManagedAccountOperations, ManagedWalletInfo}; + +/// Round-trip only the `observed_spent_outpoints` field through JSON (its +/// persisted, `(OutPoint, height)`-pair form) and return the reloaded map. +/// +/// This helper round-trips the field alone rather than the whole wallet because +/// a full `ManagedWalletInfo` cannot serialize to JSON once its address pools +/// are populated: `AddressPool::script_pubkey_index` is a +/// `HashMap`, and `ScriptBuf` is not a valid JSON object key. +/// That pool limitation is unrelated to this field — `OutPoint` *is* a valid +/// JSON key; the field's pair-form adapter exists for format-agnosticism across +/// non-human-readable encoders (validated by +/// `observed_spent_outpoints_round_trips_through_serde` on a pool-free wallet). +/// The helper exercises that same pair encoding to model what a reload restores. +fn reload_observed_field(wallet: &ManagedWalletInfo) -> BTreeMap { + let pairs: Vec<(OutPoint, CoreBlockHeight)> = + wallet.observed_spent_outpoints().iter().map(|(k, v)| (*k, *v)).collect(); + let json = serde_json::to_string(&pairs).expect("serialize field"); + serde_json::from_str::>(&json) + .expect("deserialize field") + .into_iter() + .collect() +} + +// ── observed_spent_outpoints serde round-trip + default ────── + +/// The field's `#[serde(default, with = ...)]` adapter round-trips through a +/// real full-struct JSON serialize/deserialize (on a pool-free wallet, which is +/// the only kind that can JSON-serialize) and defaults to empty when the key is +/// absent — the persistence + backward-compat contract (K-INV-649-PERSIST). +#[test] +fn observed_spent_outpoints_round_trips_through_serde() { + let mut wallet = ManagedWalletInfo::new(Network::Testnet, [7u8; 32]); + let outpoint = OutPoint::new(dashcore::Txid::from([0x5A; 32]), 3); + wallet.record_observed_spends(&spend_to_external(&[outpoint], 1, 70), 1234); + + // Full-struct round-trip (pool-free wallet → JSON works). + let json = serde_json::to_string(&wallet).expect("serialize minimal wallet"); + let restored: ManagedWalletInfo = serde_json::from_str(&json).expect("deserialize"); + assert_eq!( + restored.observed_spent_outpoints().get(&outpoint).copied(), + Some(1234), + "the field survives a full-struct serde round-trip" + ); + + // Backward compat: a snapshot missing the key loads to an empty set. + let mut value: serde_json::Value = serde_json::from_str(&json).expect("to value"); + value.as_object_mut().expect("object").remove("observed_spent_outpoints"); + let stripped = serde_json::to_string(&value).expect("reserialize"); + let loaded: ManagedWalletInfo = + serde_json::from_str(&stripped).expect("load pre-field snapshot"); + assert!(loaded.observed_spent_outpoints().is_empty(), "#[serde(default)] supplies empty set"); +} + +// ── reject funding after a reload of the persisted set ── + +/// The spend recorded in session 1 both (a) already excludes the coin in-memory +/// before the restart and (b) still excludes it after the persisted set is +/// reloaded and the funding arrives post-restart — proving the reject depends +/// solely on the persisted `observed_spent_outpoints`, not transient state. +#[tokio::test] +async fn restored_observed_spend_rejects_funding_after_reload() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 51); + + // Session 1: process the spend block first; assert the pre-restart state. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "spend seen in a block must record the observed-spent outpoint" + ); + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "coin must not be tracked pre-restart (recording actually happened)" + ); + + // Restart: wipe the in-memory set and reload it from its persisted form. + let reloaded = reload_observed_field(&ctx.managed_wallet); + ctx.managed_wallet.observed_spent_outpoints.clear(); + ctx.managed_wallet.observed_spent_outpoints = reloaded; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "persisted set restores the recorded spend" + ); + + // Session 2: the funding block finally arrives. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "BUG #649: funding coin resurrected after restart despite reloaded spend record" + ); +} + +// ── normal order unaffected, no double removal ─────────────────────── + +/// In-order funding-then-spend: the coin is inserted then removed exactly once +/// via the existing `update_utxos` path, the spend record still attributes the +/// input value (recording is insert-only, so `input_details` survive), and the +/// new seam adds no second monitor-revision bump (K-INV-649-NOREGRESS). +#[tokio::test] +async fn in_order_spend_records_observed_outpoint_without_double_removal() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 5_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + assert!(account.utxos.contains_key(&funding_outpoint), "funding coin inserted in order"); + let rev_after_funding = account.monitor_revision(); + + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 52); + ctx.check_transaction(&spend_tx, block_ctx(101)).await; + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + assert!(account.utxos.is_empty(), "spent coin removed"); + assert_eq!( + account.monitor_revision(), + rev_after_funding + 1, + "the spend must bump the monitor revision exactly once — no double removal" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no phantom balance after the spend"); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "the input is recorded even on the in-order (matched) path" + ); + // The record still carries the spent input's value: recording is insert-only + // and never removes the coin before `record_transaction` reads it. + let record = account.transactions().get(&spend_tx.txid()).expect("spend recorded"); + assert_eq!(record.net_amount, -(funding_value as i64), "input value attributed to the spend"); + assert_eq!(record.input_details.len(), 1, "input_details preserved by insert-only recording"); +} + +// ── multi-input spend, partial ownership ───────────────────────────── + +/// A single spend consuming two owned outpoints (A, B) and one the wallet never +/// tracks (C), delivered before any funding: every input is recorded (set grows +/// by exactly 3), processing never panics on the unowned C, and neither A nor B +/// resurfaces once their funding blocks arrive. +#[tokio::test] +async fn multi_input_spend_records_each_input_including_unowned() { + let mut ctx = TestWalletContext::new_random(); + + let fund_a = Transaction::dummy(&ctx.receive_address, 0..1, &[700_000]); + let fund_b = Transaction::dummy(&ctx.receive_address, 1..2, &[800_000]); + let outpoint_a = OutPoint::new(fund_a.txid(), 0); + let outpoint_b = OutPoint::new(fund_b.txid(), 0); + // C belongs to nobody the wallet tracks. + let outpoint_c = OutPoint::new(dashcore::Txid::from([0xCC; 32]), 7); + + let spend = spend_to_external(&[outpoint_a, outpoint_b, outpoint_c], 1_400_000, 53); + let before = ctx.managed_wallet.observed_spent_outpoints().len(); + ctx.check_transaction(&spend, block_ctx(300)).await; // must not panic on C + let after = ctx.managed_wallet.observed_spent_outpoints().len(); + assert_eq!(after - before, 3, "one observed-spent entry per input, not conflated per-tx"); + for op in [outpoint_a, outpoint_b, outpoint_c] { + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&op)); + } + + // Funding for A then B arrives out of order — neither may become live. + ctx.check_transaction(&fund_a, block_ctx(200)).await; + ctx.check_transaction(&fund_b, block_ctx(201)).await; + assert!(!utxo_tracked(&ctx.managed_wallet, &outpoint_a), "A must not resurface"); + assert!(!utxo_tracked(&ctx.managed_wallet, &outpoint_b), "B must not resurface"); +} + +// ── irrelevant-block noise, bounded growth ─────────────────────────── + +/// Interleaving many unrelated multi-input spends with the wallet's own +/// funding/spend pair yields the same final wallet state as the pair alone, and +/// grows the set by exactly the total input count observed — never the whole +/// chain (K-INV-649-BOUND). +#[tokio::test] +async fn irrelevant_block_noise_grows_set_by_total_inputs_only() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 2_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 54); + + let mut expected_inputs = 0usize; + // 50 unrelated 2-input spends touching nothing of ours. + for i in 0..50u32 { + let a = OutPoint::new(dashcore::Txid::from([(i as u8).wrapping_add(1); 32]), 0); + let b = OutPoint::new(dashcore::Txid::from([(i as u8).wrapping_add(1); 32]), 1); + let noise = spend_to_external(&[a, b], 10_000, 100 + i as usize); + expected_inputs += 2; + ctx.check_transaction(&noise, block_ctx(400 + i)).await; + } + + ctx.check_transaction(&funding_tx, block_ctx(500)).await; + expected_inputs += 1; // funding's own synthetic input + ctx.check_transaction(&spend_tx, block_ctx(501)).await; + expected_inputs += 1; // the spend's input (= funding_outpoint) + + assert_eq!( + ctx.managed_wallet.observed_spent_outpoints().len(), + expected_inputs, + "set size equals total observed inputs — bounded to block activity, not chain history" + ); + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "the real funding/spend pair still resolves correctly amid the noise" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0); +} + +// ── backward-compatible load of a pre-fix snapshot ─────────────────── + +/// A wallet loaded from a pre-fix snapshot (empty `observed_spent_outpoints`, as +/// `#[serde(default)]` supplies) protects newly observed out-of-order spends, and +/// is explicitly NOT retroactively corrected for spends it processed incorrectly +/// before the fix existed — the empty loaded set means no magic backfill. +#[tokio::test] +async fn pre_fix_snapshot_without_field_loads_and_protects_new_spends() { + let mut ctx = TestWalletContext::new_random(); + // A pre-fix load presents an empty set (the #[serde(default)] result, checked + // directly in `observed_spent_outpoints_round_trips_through_serde`). + assert!( + ctx.managed_wallet.observed_spent_outpoints().is_empty(), + "no historical record is fabricated for a pre-fix wallet" + ); + + // A new out-of-order spend observed post-load IS protected. + let funding_value = 3_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 55); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "post-load out-of-order spend is protected" + ); +} + +// ── unbounded-growth stress, bounded and roughly linear ────────────── + +/// Many filter-matched blocks of orphaned spends (funding never arrives) do not +/// panic, keep exactly one entry per observed input (no dedup loss), and do not +/// exhibit superlinear per-block cost as the set grows (K-INV-649-BOUND, §5.5). +#[tokio::test] +async fn many_orphaned_spend_blocks_stay_bounded_and_linear() { + let mut ctx = TestWalletContext::new_random(); + + const CHUNKS: u32 = 4; + const PER_CHUNK: u32 = 500; + const INPUTS_PER_TX: usize = 3; + let mut durations = Vec::new(); + + for chunk in 0..CHUNKS { + let start = Instant::now(); + for i in 0..PER_CHUNK { + let n = chunk * PER_CHUNK + i; + // Distinct, never-owned outpoints per tx (unique txid seed per block). + let seed = n.to_le_bytes(); + let inputs: Vec = (0..INPUTS_PER_TX as u32) + .map(|v| { + let mut b = [0u8; 32]; + b[..4].copy_from_slice(&seed); + b[4] = v as u8; + OutPoint::new(dashcore::Txid::from(b), v) + }) + .collect(); + let tx = spend_to_external(&inputs, 1_000, 200 + (n as usize % 40)); + ctx.check_transaction(&tx, block_ctx(1_000 + n)).await; + } + durations.push(start.elapsed()); + } + + let total = (CHUNKS * PER_CHUNK) as usize * INPUTS_PER_TX; + assert_eq!( + ctx.managed_wallet.observed_spent_outpoints().len(), + total, + "exactly one entry per observed input — no silent dedup or loss" + ); + // Superlinear (O(n^2)) growth would make the last chunk dramatically slower + // than the first. Reported as a diagnostic rather than asserted: wall-clock + // ratios flake on shared CI runners, and correctness is already pinned by + // the exact-entry-count assertion above. + let first = durations.first().copied().unwrap().as_secs_f64().max(1e-4); + let last = durations.last().copied().unwrap().as_secs_f64(); + eprintln!( + "per-chunk cost: first={first:.4}s last={last:.4}s (ratio {:.1}x, flat expected)", + last / first + ); +} + +// ── same outpoint spent in two blocks, last-write-wins ─────────────── + +/// Recording the same outpoint from two different-height blocks keeps a single +/// entry at the last-inserted height (`BTreeMap::insert` overwrite semantics) — +/// pinned so a future height-keyed pruning policy can rely on it. +#[tokio::test] +async fn same_outpoint_spent_in_two_blocks_keeps_last_height() { + let ctx = TestWalletContext::new_random(); + let mut wallet = ctx.managed_wallet; + + let outpoint = OutPoint::new(dashcore::Txid::from([0xA1; 32]), 0); + let spend = spend_to_external(&[outpoint], 9_000, 60); + + wallet.record_observed_spends(&spend, 111); + wallet.record_observed_spends(&spend, 222); + + assert_eq!(wallet.observed_spent_outpoints().len(), 1, "single entry, not a multimap"); + assert_eq!( + wallet.observed_spent_outpoints().get(&outpoint).copied(), + Some(222), + "last write wins on height" + ); +} + +// ── last_processed_height alone does not prune ─────────────────────── + +/// Pruning is gated only on the finality boundary (chainlock + synced_height), +/// not on `last_processed_height` (#649): however far `last_processed_height` +/// advances past the observed spend, with no chainlock the entry is retained and +/// the funding coin is still rejected when it finally arrives. +#[tokio::test] +async fn orphaned_spend_not_pruned_by_last_processed_height() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 4_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 61); + + ctx.check_transaction(&spend_tx, block_ctx(50)).await; + // Advance far past any plausible pruning horizon. + ctx.managed_wallet.update_last_processed_height(1_000_050); + + ctx.check_transaction(&funding_tx, block_ctx(60)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "no TTL/expiry: the observed spend still invalidates the funding a million blocks later" + ); + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); +} + +// ── cross-account-type routing gap ─────────────────────────────────── + +/// A CoinJoin-owned coin spent by an AssetLock-classified transaction (whose +/// `relevant_types` exclude CoinJoin) must still be invalidated when its funding +/// arrives later — recording and reconciliation are independent of the spending +/// transaction's classification (the cross-account-type routing gap). +#[tokio::test] +async fn coinjoin_utxo_spent_by_asset_lock_classified_tx_still_invalidated() { + let mut ctx = TestWalletContext::new_random(); + + // Derive a CoinJoin address (Default account options create the account). + // CoinJoin accounts are spend-only for the Standard receive/change paths, so + // their addresses come from the generic `next_address` pool accessor. + let cj_xpub = ctx.wallet.get_coinjoin_account(0).expect("coinjoin account").account_xpub; + let cj_address = ctx + .managed_wallet + .first_coinjoin_managed_account_mut() + .expect("managed coinjoin account") + .next_address(Some(&cj_xpub), true) + .expect("coinjoin address"); + + let funding_value = 1_500_000u64; + let funding_tx = Transaction::dummy(&cj_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + + // The spend classifies as AssetLock via its special payload, which narrows + // relevant account types to exclude CoinJoin. + let mut spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 62); + spend_tx.special_transaction_payload = + Some(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new(vec![]))); + assert_eq!( + TransactionRouter::classify_transaction(&spend_tx), + TransactionType::AssetLock, + "spend must classify as AssetLock for this scenario to bite", + ); + + // Spend block processed before the CoinJoin funding block. + ctx.check_transaction(&spend_tx, block_ctx(700)).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "recording must be un-gated by the spend's AssetLock classification" + ); + + ctx.check_transaction(&funding_tx, block_ctx(600)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "BUG #649: CoinJoin coin resurrected because the spend routed away from CoinJoin" + ); +} + +// ── dual bookkeeping consistency ───────────────────────────────────── + +/// An in-order matched spend populates both the per-account derived +/// `spent_outpoints` (via the existing `update_utxos` path — proven behaviorally +/// by a re-processed funding staying rejected) and the wallet-level +/// `observed_spent_outpoints`, with no double-decrement or double bump. +#[tokio::test] +async fn in_order_spend_populates_both_peraccount_and_wallet_sets() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 6_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + let rev_after_funding = + ctx.managed_wallet.first_bip44_managed_account().expect("account").monitor_revision(); + + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 63); + ctx.check_transaction(&spend_tx, block_ctx(101)).await; + + // Wallet-level set: directly observable. + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + assert!(account.utxos.is_empty()); + assert_eq!(account.monitor_revision(), rev_after_funding + 1, "exactly one bump for the spend"); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no double-decrement"); + + // Per-account derived set: proven behaviorally — re-processing the funding + // (as a rescan would) must not re-insert the coin. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "re-processed funding stays rejected — both guards agree" + ); +} + +// ── account created after the spend was observed ───────────────────── + +/// The wallet-level (not per-account) placement of the set means a spend +/// observed while an owning account does not yet exist still invalidates the +/// funding once that account is added and its funding block is processed. +#[tokio::test] +async fn account_created_after_spend_observed_still_rejects_funding() { + let mut ctx = TestWalletContext::new_random(); + + // Build account 1 and its first receive address off to the side (in the + // wallet + a detached managed account), so we can construct the funding tx + // that pays it — but do NOT put the managed account into the collection yet. + // At spend-observation time the managed wallet still only knows account 0. + ctx.wallet + .add_account( + AccountType::Standard { + index: 1, + standard_account_type: StandardAccountType::BIP44Account, + }, + None, + ) + .expect("add account 1"); + let acct1 = ctx.wallet.get_bip44_account(1).expect("account 1"); + let xpub1 = acct1.account_xpub; + let mut managed_acct1 = ManagedCoreFundsAccount::from_account(acct1); + let addr1 = managed_acct1.next_receive_address(Some(&xpub1), true).expect("addr1"); + + let funding_value = 2_500_000u64; + let funding_tx = Transaction::dummy(&addr1, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 64); + + // Observe the spend while account 1 is NOT yet in the managed collection. + ctx.check_transaction(&spend_tx, block_ctx(800)).await; + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Now the managed account 1 appears (gap-limit/lazy discovery). + ctx.managed_wallet + .accounts + .insert_funds_bearing_account(managed_acct1) + .expect("insert managed account 1"); + + // Its funding block is processed — the coin must not become live. + ctx.check_transaction(&funding_tx, block_ctx(700)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "wallet-level recording protects an account that did not exist at spend time" + ); +} + +// ── mempool / InstantSend spends excluded, self-heal on block ──────── + +/// Mempool- and InstantSend-only spends do not populate the set (K-INV-649-SCOPE) +/// — so the coin is momentarily spendable — and the state self-heals when the +/// same spend is later seen in a block. +#[tokio::test] +async fn mempool_and_instantsend_spends_not_recorded_but_self_heal_on_block() { + for non_block_ctx in + [TransactionContext::Mempool, TransactionContext::InstantSend(InstantLock::default())] + { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_200_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 65); + + // Spend seen only via a non-block context: NOT recorded. + ctx.check_transaction(&spend_tx, non_block_ctx.clone()).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().is_empty(), + "non-block spend must not populate the observed-spent set: {non_block_ctx}" + ); + + // Funding arrives: the coin is (correctly, per current scope) spendable. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "transient window: coin is spendable until the spend confirms in a block" + ); + + // The same spend is later mined: now it records and self-heals. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "block confirmation records the spend" + ); + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "self-healed: the coin is dropped on the block-confirmed spend" + ); + } +} + +// ── idempotent re-processing of the same spend block ───────────────── + +/// Re-delivering the identical spend block (rescan overlap / retry) is +/// idempotent: one entry for the outpoint, no extra monitor bump, and the +/// funding is still rejected afterward. +#[tokio::test] +async fn duplicate_spend_block_is_idempotent() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_800_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 66); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + let len_after_first = ctx.managed_wallet.observed_spent_outpoints().len(); + let rev_after_first = + ctx.managed_wallet.first_bip44_managed_account().expect("account").monitor_revision(); + + // Byte-identical re-delivery at the same height. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert_eq!( + ctx.managed_wallet.observed_spent_outpoints().len(), + len_after_first, + "duplicate delivery keeps a single entry (BTreeMap keyed by OutPoint)" + ); + assert_eq!( + ctx.managed_wallet.first_bip44_managed_account().expect("account").monitor_revision(), + rev_after_first, + "nothing changed on re-delivery: no extra monitor-revision bump" + ); + + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "funding still rejected"); +} + +// ── pre-fix load composed with an in-flight out-of-order spend ─────── + +/// The real cold-rescan shape: a wallet that started from a pre-fix snapshot +/// (empty observed set) runs the whole out-of-order sequence in the same +/// session. Outcome must match a fresh wallet — no latent state alters the fix. +#[tokio::test] +async fn pre_fix_snapshot_then_out_of_order_matches_fresh_wallet() { + let mut ctx = TestWalletContext::new_random(); + // Model the loaded old snapshot: the observed set starts empty. + assert!(ctx.managed_wallet.observed_spent_outpoints().is_empty()); + + let funding_value = 3_300_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 67); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "old-format load + out-of-order delivery behaves identically to a fresh wallet" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0); +} + +// ── bounded permanence: removal only at the finality boundary ──────── + +/// Pins the bounded-permanence invariant (#649): an observed spend is removed +/// ONLY at or below the finality boundary `min(chainlock, synced_height)`, and +/// only through [`ManagedWalletInfo::prune_finalized_observed_spends`]. No +/// contributor may add another removal path — reorg/rollback, age, or recency — +/// without a deliberate decision, since that would reintroduce #649. +#[tokio::test] +async fn observed_spend_removal_only_via_finality_boundary() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 2_100_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 68); + + // Observe the spend in a block at height 200. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // A context flip to Mempool must NOT retract the entry (no reorg/rollback path). + ctx.check_transaction(&spend_tx, TransactionContext::Mempool).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "no reorg/rollback removal path: a context flip to Mempool does not retract the entry" + ); + + // Advancing synced_height alone, with no chainlock, must NOT prune: without + // a chainlock there is no finality boundary. + ctx.managed_wallet.update_synced_height(1_000_000); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "synced_height without a chainlock defines no boundary, so nothing is pruned" + ); + + // A chainlock BELOW the entry height must NOT prune it (boundary 199 < 200). + ctx.managed_wallet.update_last_processed_height(1_000_000); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(199)); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "a chainlock below the spend height leaves the entry (height 200 > boundary 199)" + ); + + // Only a chainlock at/above the entry height (with synced_height past it) + // removes it — exactly at the finality boundary and nowhere else. + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + assert!( + !ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "removal happens exactly at the finality boundary min(chainlock, synced_height)" + ); + + // Adding an account invalidates the sync certificate (the boundary input): + // a freshly observed spend is not removed until the checkpoint re-advances + // past it, even though the chainlock is already well ahead. + ctx.managed_wallet.metadata.birth_height = 100; + let op2 = OutPoint::new(dashcore::Txid::from([0x7C; 32]), 0); + ctx.managed_wallet.record_observed_spends(&spend_to_external(&[op2], 1, 69), 150); + ctx.wallet + .add_account( + AccountType::Standard { + index: 1, + standard_account_type: StandardAccountType::BIP44Account, + }, + None, + ) + .expect("add wallet account 1"); + ctx.managed_wallet + .add_managed_account( + &ctx.wallet, + AccountType::Standard { + index: 1, + standard_account_type: StandardAccountType::BIP44Account, + }, + ) + .expect("add managed account 1"); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(300)); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&op2), + "after an account addition, no removal occurs until the checkpoint re-advances past it" + ); + + // Re-advancing the checkpoint re-certifies coverage and the entry prunes. + ctx.managed_wallet.update_synced_height(300); + assert!( + !ctx.managed_wallet.observed_spent_outpoints().contains_key(&op2), + "entry prunable again once synced_height re-advances past it" + ); +} + +// ── history/balance consistency — no phantom "received" after a guarded spend ── + +/// Documents the #649-restructure architectural finding: `history_net == +/// balance.total()` is NOT a system invariant under default features +/// (`keep-finalized-transactions = OFF`) — it never holds for ANY +/// chainlocked-and-pruned funding, #649 or not. `transaction_history()` +/// returns only live records; a funding record whose first sighting is +/// already chainlocked (as in a full historical rescan) is dropped to +/// `finalized_txids` immediately, while its coin stays live in `balance`. +/// No spend, no #649 guard, no compensation involved — pinned here so this +/// divergence is recognized as by-design, not a regression (#649). +#[cfg(not(feature = "keep-finalized-transactions"))] +#[tokio::test] +async fn plain_chainlocked_funding_diverges_history_from_balance_by_design() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 900_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(50)).await; + + assert!(utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "coin is live"); + assert_eq!( + ctx.managed_wallet.balance.total(), + funding_value, + "balance correctly reflects the live coin" + ); + assert_eq!( + history_net(&ctx.managed_wallet), + 0, + "the record was pruned to finalized_txids on first sighting, so history omits it \ + entirely — NOT a #649 regression, this holds for any chainlocked-and-pruned funding" + ); + assert_ne!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "documents: history_net == balance.total() is not a system invariant under default \ + features for finalized (pruned) records" + ); +} + +/// Mirror of the above under `keep-finalized-transactions`: with the feature +/// on, the record is never pruned, so β (`history_net == balance.total()`) +/// DOES hold — confirming the divergence above is purely a consequence of +/// pruning, not of anything #649-specific. +#[cfg(feature = "keep-finalized-transactions")] +#[tokio::test] +async fn plain_chainlocked_funding_matches_balance_when_records_are_kept() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 900_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(50)).await; + + assert!(utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "coin is live"); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "with keep-finalized-transactions on, the record is never pruned, so history net \ + matches balance even for a chainlocked-first-sighting funding" + ); +} + +/// Spend-first ordering: when the funding block arrives after its spend was +/// observed, the funding UTXO is reconciled away — and the funding transaction's +/// "received" history record must be compensated so `transaction_history()`'s +/// net equals `balance.total()`. Without the fix the record shows "+funding" +/// against a zero balance with no transaction explaining where it went. +#[tokio::test] +async fn transaction_history_net_matches_balance_after_spend_before_funding() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 90); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "coin invalidated"); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no phantom balance"); + assert_eq!(history_net(&ctx.managed_wallet), 0, "no phantom 'received' in history"); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "transaction_history net must equal balance", + ); + // The fully-compensated funding record is KEPT at net 0 (not deleted), so a + // later reprocessing takes the already-known path instead of re-recording a + // positive net. Net 0 keeps history consistent with the zero balance. + let records = ctx.managed_wallet.transaction_history(); + assert_eq!(records.len(), 1, "the funding record is kept, fully compensated"); + assert_eq!(records[0].net_amount, 0, "kept record sits at net 0"); + assert!(records[0].output_details.is_empty(), "its received output is compensated away"); +} + +/// Funding-first mirror: a coin funded and recorded, then spent by a transaction +/// whose classification excludes the owning account (AssetLock spend of a +/// CoinJoin coin), is removed via the un-gated spend path. Its funding record +/// must likewise be compensated so history net stays equal to balance. +#[tokio::test] +async fn transaction_history_net_matches_balance_after_unattributed_spend() { + let mut ctx = TestWalletContext::new_random(); + + let cj_xpub = ctx.wallet.get_coinjoin_account(0).expect("coinjoin account").account_xpub; + let cj_address = ctx + .managed_wallet + .first_coinjoin_managed_account_mut() + .expect("managed coinjoin account") + .next_address(Some(&cj_xpub), true) + .expect("coinjoin address"); + + let funding_value = 1_500_000u64; + let funding_tx = Transaction::dummy(&cj_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + + // Funding first: recorded and tracked on the CoinJoin account. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!(utxo_tracked(&ctx.managed_wallet, &funding_outpoint)); + assert_eq!(history_net(&ctx.managed_wallet), funding_value as i64); + + // Spend classified as AssetLock — excludes CoinJoin from relevant types, so + // it is unattributed and removed by the un-gated spend path. + let mut spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 91); + spend_tx.special_transaction_payload = + Some(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new(vec![]))); + assert_eq!(TransactionRouter::classify_transaction(&spend_tx), TransactionType::AssetLock); + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "coin invalidated"); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no phantom balance"); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "transaction_history net must equal balance after an unattributed spend", + ); +} + +// ── reconciliation is intentionally NOT gated to block context ──────────────── + +/// Recording is restricted to block contexts, but reconciliation is not: the +/// observed-spent set only ever holds block-confirmed spends, so a coin present +/// in it is genuinely spent on-chain and must be dropped even when its funding +/// is (re)delivered via mempool. This pins that intentional asymmetry. +#[tokio::test] +async fn mempool_funding_is_reconciled_against_block_observed_spend() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_100_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 92); + + // The spend is seen in a block first (records the observed spend). + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // The funding is then delivered via mempool (unconfirmed) — it must still be + // reconciled away, because the spend it collides with was block-confirmed. + ctx.check_transaction(&funding_tx, TransactionContext::Mempool).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "a mempool funding must not resurrect a coin whose spend was seen in a block" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0); + assert_eq!(history_net(&ctx.managed_wallet), 0, "history stays consistent for mempool funding"); +} + +// ── serde adapter streams many entries (defensive-cap path) ─────────── + +/// The capped, streaming deserialize visitor round-trips many entries correctly +/// (well under the defensive cap). Exercises the visitor beyond the trivial +/// single-entry case; the multi-hundred-MB cap boundary itself is not asserted +/// here as constructing >10M entries is impractical for a unit test. +#[test] +fn observed_spent_outpoints_serde_streams_many_entries() { + let mut wallet = ManagedWalletInfo::new(Network::Testnet, [9u8; 32]); + for i in 0..3_000u32 { + let mut bytes = [0u8; 32]; + bytes[..4].copy_from_slice(&i.to_le_bytes()); + let op = OutPoint::new(dashcore::Txid::from(bytes), i % 4); + wallet.record_observed_spends(&spend_to_external(&[op], 1, 70), 1_000 + i); + } + assert_eq!(wallet.observed_spent_outpoints().len(), 3_000); + + let json = serde_json::to_string(&wallet).expect("serialize"); + let restored: ManagedWalletInfo = serde_json::from_str(&json).expect("deserialize"); + assert_eq!( + restored.observed_spent_outpoints(), + wallet.observed_spent_outpoints(), + "the streaming adapter round-trips every entry with no loss or reordering" + ); +} + +// ── multi-output partial compensation: idempotency + output_details sync ────── + +/// A funding tx with two of our outputs where only one is observed-spent, then +/// reprocessed (guaranteed by rescan/duplicate-block delivery). The guard must +/// be idempotent: reprocessing must not compensate the removed output a second +/// time and drive `net_amount` negative. +#[tokio::test] +async fn reprocessing_partially_compensated_funding_does_not_double_compensate() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let out1 = OutPoint::new(funding_txid, 1); + let spend_tx = spend_to_external(&[out0], out0_value - 1_000, 93); + + // Spend of out0 observed first, then the funding, then the funding AGAIN. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; // rescan/duplicate delivery + + // out1 survives; out0 stays invalidated. + assert!(utxo_tracked(&ctx.managed_wallet, &out1), "surviving output stays tracked"); + assert!(!utxo_tracked(&ctx.managed_wallet, &out0), "observed-spent output stays invalidated"); + assert_eq!(ctx.managed_wallet.balance.total(), out1_value, "balance is the surviving output"); + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("funding record"); + assert_eq!( + record.net_amount, out1_value as i64, + "net_amount reflects only the surviving output — not compensated twice", + ); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "history net stays equal to balance across reprocessing", + ); +} + +/// Single pass, multi-output partial compensation: the removed output's +/// `output_details` entry must be dropped so per-output line items agree with +/// the compensated `net_amount`. +#[tokio::test] +async fn partial_compensation_updates_output_details() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let spend_tx = spend_to_external(&[out0], out0_value - 1_000, 94); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("funding record"); + + assert_eq!(record.net_amount, out1_value as i64, "net_amount reflects surviving output"); + assert!( + !record.output_details.iter().any(|d| d.index == 0), + "the removed output must not remain in output_details as a phantom received entry", + ); + let received_sum: u64 = record.output_details.iter().map(|d| d.value).sum(); + assert_eq!( + received_sum, out1_value, + "sum of surviving output_details values equals the compensated net_amount", + ); +} + +/// A fully-spent-before-seen funding tx (single output, all of it observed +/// spent) is reprocessed. The fully-compensated record is kept at net 0 rather +/// than dropped, so the reprocess takes the `is_new == false` path and does not +/// re-record a positive net with no coin to reconcile it against. +#[tokio::test] +async fn reprocessing_fully_compensated_funding_stays_consistent() { + let mut ctx = TestWalletContext::new_random(); + let value = 2_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let spend_tx = spend_to_external(&[out0], value - 1_000, 95); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; // reprocess + + assert!(!utxo_tracked(&ctx.managed_wallet, &out0), "coin stays invalidated"); + assert_eq!(ctx.managed_wallet.balance.total(), 0); + assert_eq!( + history_net(&ctx.managed_wallet), + 0, + "history net stays equal to balance across reprocessing" + ); + // The record is kept, fully compensated to net 0, with no surviving output. + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("record kept"); + assert_eq!(record.net_amount, 0, "fully-compensated record sits at net 0"); + assert!(record.output_details.is_empty(), "no surviving received output remains"); +} + +// ── deeper idempotency / persistence stress ──────────────────────────────── +// +// Pins the compensation guard across harder cases (#649): repeated +// reprocessing beyond a single retry, a multi-output tx where every output is +// eventually observed spent, and the cross-serialize/deserialize boundary. + +/// Idempotency must hold for an unbounded number of replays, not just one +/// extra retry. Reprocess the funding tx a 2nd, 3rd, AND 4th time and assert +/// `net_amount`/balance are stable after every single pass, not just at the +/// end. +#[tokio::test] +async fn reprocessing_partially_compensated_funding_stays_idempotent_across_many_replays() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let out1 = OutPoint::new(funding_txid, 1); + let spend_tx = spend_to_external(&[out0], out0_value - 1_000, 96); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + for replay in 1..=4 { + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + assert!(utxo_tracked(&ctx.managed_wallet, &out1), "replay {replay}: out1 stays tracked"); + assert!( + !utxo_tracked(&ctx.managed_wallet, &out0), + "replay {replay}: out0 stays invalidated" + ); + assert_eq!( + ctx.managed_wallet.balance.total(), + out1_value, + "replay {replay}: balance unaffected" + ); + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("funding record"); + assert_eq!( + record.net_amount, out1_value as i64, + "replay {replay}: net_amount must not drift with repeated replay" + ); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "replay {replay}: history net stays equal to balance" + ); + } +} + +/// A multi-output funding tx where BOTH outputs are independently observed +/// spent (by two separate spending transactions, not one) before the funding +/// is processed. The record must reach net 0 in a single pass and stay +/// consistent — not just the single-output-fully-spent case, and not just +/// the single-output-partially-spent case already covered. +#[tokio::test] +async fn all_outputs_independently_observed_spent_reaches_net_zero_and_stays_consistent() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let out1 = OutPoint::new(funding_txid, 1); + let spend_tx0 = spend_to_external(&[out0], out0_value - 1_000, 97); + let spend_tx1 = spend_to_external(&[out1], out1_value - 1_000, 98); + + // Both spends observed, independently, before the funding tx is seen. + ctx.check_transaction(&spend_tx0, block_ctx(200)).await; + ctx.check_transaction(&spend_tx1, block_ctx(201)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + assert!(!utxo_tracked(&ctx.managed_wallet, &out0), "out0 invalidated"); + assert!(!utxo_tracked(&ctx.managed_wallet, &out1), "out1 invalidated"); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no surviving output"); + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("record kept"); + assert_eq!(record.net_amount, 0, "both outputs compensated in one pass reaches net 0"); + assert!(record.output_details.is_empty(), "both output_details entries are gone"); + assert_eq!(history_net(&ctx.managed_wallet), 0, "history net stays equal to the zero balance"); + + // Reprocess: must stay at net 0, not go negative. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("record kept"); + assert_eq!(record.net_amount, 0, "reprocess after both-outputs-spent stays at net 0"); + assert_eq!(ctx.managed_wallet.balance.total(), 0); +} + +/// Pins that the `output_details` compensation survives a serialize/deserialize +/// even though the account-local `spent_outpoints` set does NOT (it is rebuilt +/// from recorded transactions on load and omits the never-recorded spend). This +/// exercises that boundary +/// directly via `simulate_reload_rebuild_spent_outpoints`, which performs the +/// exact same reconstruction the real `Deserialize` impl does — a literal +/// full-struct serde round-trip is unavailable for a populated account (see +/// that helper's doc comment). +/// +/// Two reload cycles are exercised, not one, to confirm the persisted +/// wallet-level `observed_spent_outpoints` set — not the transient +/// account-local mark — is what makes this survive a restart. +#[tokio::test] +async fn compensation_survives_simulated_reload_across_two_restart_cycles() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let out1 = OutPoint::new(funding_txid, 1); + let spend_tx = spend_to_external(&[out0], out0_value - 1_000, 99); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + let assert_consistent = |ctx: &TestWalletContext, cycle: u32| { + assert!(utxo_tracked(&ctx.managed_wallet, &out1), "cycle {cycle}: out1 tracked"); + assert!(!utxo_tracked(&ctx.managed_wallet, &out0), "cycle {cycle}: out0 invalidated"); + assert_eq!( + ctx.managed_wallet.balance.total(), + out1_value, + "cycle {cycle}: balance is the surviving output only" + ); + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("funding record"); + assert_eq!( + record.net_amount, out1_value as i64, + "cycle {cycle}: net_amount must not be compensated twice across a reload" + ); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "cycle {cycle}: history net stays equal to balance across a reload" + ); + }; + assert_consistent(&ctx, 0); + + for cycle in 1..=2 { + // Simulate a restart: rebuild the account-local `spent_outpoints` + // from recorded transactions exactly as `Deserialize` would. The + // persisted wallet-level `observed_spent_outpoints` is left intact, + // as it would be across a real save/reload. + ctx.managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .simulate_reload_rebuild_spent_outpoints(); + + // Post-restart rescan / duplicate delivery reprocesses the funding tx. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + assert_consistent(&ctx, cycle); + } +} + +/// A full historical rescan can deliver a funding tx whose FIRST sighting is +/// already `InChainLockedBlock`. `record_transaction` builds the record with +/// out0 excluded (its spend is in `observed_spent_outpoints`) before the +/// default feature prunes the record to `finalized_txids`. So `history_net` +/// is 0 while `balance` keeps out1 — the general pruning non-invariant pinned +/// by [`plain_chainlocked_funding_diverges_history_from_balance_by_design`], +/// not a #649 defect. +#[cfg(not(feature = "keep-finalized-transactions"))] +#[tokio::test] +async fn spend_first_ordering_with_chainlocked_first_sighting_prunes_record_by_design() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let out1 = OutPoint::new(funding_txid, 1); + let spend_tx = spend_to_external(&[out0], out0_value - 1_000, 100); + + // Spend of out0 observed first (still an ordinary InBlock context — the + // spend itself need not be chainlocked for this to trigger). + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + // The funding tx's FIRST sighting is already chainlocked, as it would be + // during a full rescan of old history. + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(100)).await; + + assert!(!utxo_tracked(&ctx.managed_wallet, &out0), "out0 stays invalidated"); + assert!(utxo_tracked(&ctx.managed_wallet, &out1), "out1 survives"); + assert_eq!( + ctx.managed_wallet.balance.total(), + out1_value, + "balance correctly reflects the surviving output" + ); + assert_eq!( + history_net(&ctx.managed_wallet), + 0, + "the born-correct record was pruned to finalized_txids on first sighting, so history \ + omits it entirely — the general pruning non-invariant, not a #649 defect" + ); +} + +/// Mirror of the above under `keep-finalized-transactions`: the record is +/// never pruned, so β (`history_net == balance.total()`) holds — and it holds +/// because the record was born correct at construction, not because of any +/// post-hoc compensation. Confirms the pruning-driven divergence above is +/// purely a consequence of pruning, not an unclosed #649 gap. +#[cfg(feature = "keep-finalized-transactions")] +#[tokio::test] +async fn spend_first_ordering_with_chainlocked_first_sighting_matches_balance_when_kept() { + let mut ctx = TestWalletContext::new_random(); + + let out0_value = 2_000_000u64; + let out1_value = 500_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[out0_value, out1_value]); + let funding_txid = funding_tx.txid(); + let out0 = OutPoint::new(funding_txid, 0); + let out1 = OutPoint::new(funding_txid, 1); + let spend_tx = spend_to_external(&[out0], out0_value - 1_000, 100); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, chain_locked_block_ctx(100)).await; + + assert!(!utxo_tracked(&ctx.managed_wallet, &out0), "out0 stays invalidated"); + assert!(utxo_tracked(&ctx.managed_wallet, &out1), "out1 survives"); + assert_eq!( + history_net(&ctx.managed_wallet), + ctx.managed_wallet.balance.total() as i64, + "with keep-finalized-transactions on, the born-correct record stays live and \ + history net matches balance even for a chainlocked-first-sighting funding" + ); + + let account = ctx.managed_wallet.first_bip44_managed_account().expect("account"); + let record = account.transactions().get(&funding_txid).expect("record kept live"); + assert_eq!(record.net_amount, out1_value as i64, "born correct: only out1 counted"); + assert!(!record.output_details.iter().any(|d| d.index == 0), "out0 was never inserted"); +} + +// ── guard-removal path releases reservations ───────────────────────────── + +/// A coin removed by the wallet-level #649 guard (funding-first ordering, via +/// `remove_spent_from_accounts` → `finalize_guard_removed_utxo`) must release +/// any build reservation held on it immediately, exactly as the normal spend +/// path in `update_utxos` does — not leave it reserved until the TTL backstop. +/// +/// Uses a CoinJoin coin: the AssetLock-classified spend routes away from the +/// CoinJoin account, so the coin is removed through the guard path rather than +/// the normal `update_utxos` spend path (which already releases reservations). +#[tokio::test] +async fn guard_removed_coin_releases_its_reservation_immediately() { + let mut ctx = TestWalletContext::new_random(); + + let cj_xpub = ctx.wallet.get_coinjoin_account(0).expect("coinjoin account").account_xpub; + let cj_address = ctx + .managed_wallet + .first_coinjoin_managed_account_mut() + .expect("managed coinjoin account") + .next_address(Some(&cj_xpub), true) + .expect("coinjoin address"); + + // Fund the coin in order, so the funding record is live and the coin tracked. + let funding_value = 1_500_000u64; + let funding_tx = Transaction::dummy(&cj_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + // Reserve it as an in-flight transaction build would. + let account = ctx.managed_wallet.first_coinjoin_managed_account().expect("account"); + assert!(account.utxos.contains_key(&funding_outpoint), "coin tracked before the guard removal"); + account.reservations().reserve(&[funding_outpoint], 0); + assert!( + account.reservations().reserved(0).contains(&funding_outpoint), + "reservation is held before the spend arrives" + ); + + // An AssetLock-classified spend routes away from the CoinJoin account, so the + // coin is removed via the wallet-level guard path (funding-first ordering), + // not the normal `update_utxos` spend path. + let mut spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 121); + spend_tx.special_transaction_payload = + Some(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new(vec![]))); + assert_eq!( + TransactionRouter::classify_transaction(&spend_tx), + TransactionType::AssetLock, + "spend must classify as AssetLock to route through the guard-removal path" + ); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + + let account = ctx.managed_wallet.first_coinjoin_managed_account().expect("account"); + assert!( + !account.utxos.contains_key(&funding_outpoint), + "the guard removed the coin from the tracked set" + ); + assert!( + !account.reservations().reserved(0).contains(&funding_outpoint), + "the guard-removal path must release the reservation immediately, not wait out the TTL" + ); +} + +// ── Phase 1: finality-boundary pruning (dashpay/rust-dashcore#649) ──────── + +/// Advance chainlock + synced_height past an observed spend's height so its +/// entry is evicted, then redeliver the funding tx in block context. The +/// finalized funding record short-circuits reinsertion, so the coin is not +/// resurrected and balance/history are unchanged. +#[tokio::test] +async fn pruned_entry_does_not_resurrect_on_funding_redelivery() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 130); + + // Spend-first at height 200, funding at height 100. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "coin reconciled away"); + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Advance the finality boundary to the spend height (200): evicts the entry. + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + assert!( + !ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "entry at height 200 evicted once chainlock and synced_height reach 200" + ); + + let balance_before = ctx.managed_wallet.balance.total(); + let history_before = history_net(&ctx.managed_wallet); + + // Redeliver the funding tx: the finalized record must short-circuit. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "a pruned entry must not resurrect the coin on funding redelivery" + ); + assert_eq!(ctx.managed_wallet.balance.total(), balance_before, "balance unchanged"); + assert_eq!(history_net(&ctx.managed_wallet), history_before, "history unchanged"); +} + +/// As above, but rebuild the account-local `spent_outpoints` from records (a +/// reload) before the redelivery. The account-local set never held this spend +/// (it was never recorded there), so this pins that the durable protection is +/// the finalized/chainlocked record, not the account-local spent set. +#[tokio::test] +async fn pruned_entry_does_not_resurrect_across_reload() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 131); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + assert!(!ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Reload: rebuild the account-local spent set from recorded transactions. + ctx.managed_wallet + .first_bip44_managed_account_mut() + .expect("account") + .simulate_reload_rebuild_spent_outpoints(); + + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "protection survives reload via the finalized record, not the rebuilt spent set" + ); + assert_eq!(ctx.managed_wallet.balance.total(), 0, "no phantom balance after reload"); +} + +/// Direct pin of the §2 disproof: a guard-removed spend's protection is the +/// wallet-level map (the account-local set does not hold it after a reload). +/// The entry must survive the reload and keep guarding; only once its height is +/// final may it be pruned, and even then the finalized record prevents +/// resurrection. +#[tokio::test] +async fn guard_removed_entry_survives_reload_then_prunes_safely() { + let mut ctx = TestWalletContext::new_random(); + + // Fund a CoinJoin coin in order (AssetLock spend routes away from it). + let cj_xpub = ctx.wallet.get_coinjoin_account(0).expect("coinjoin account").account_xpub; + let cj_address = ctx + .managed_wallet + .first_coinjoin_managed_account_mut() + .expect("managed coinjoin account") + .next_address(Some(&cj_xpub), true) + .expect("coinjoin address"); + + let funding_value = 1_500_000u64; + let funding_tx = Transaction::dummy(&cj_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + + // Guard-remove via an AssetLock-classified spend at height 200. + let mut spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 132); + spend_tx.special_transaction_payload = + Some(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new(vec![]))); + assert_eq!(TransactionRouter::classify_transaction(&spend_tx), TransactionType::AssetLock); + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "guard removed the coin"); + assert!(ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Reload: the rebuilt account-local set lacks the spend (never recorded), + // so the wallet-level map is the sole guard. + ctx.managed_wallet + .first_coinjoin_managed_account_mut() + .expect("account") + .simulate_reload_rebuild_spent_outpoints(); + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "the wallet-level entry survives reload and remains the guard" + ); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "the surviving map entry still guards the coin after reload" + ); + + // Finalize past the spend height, then prune: the finalized record now + // guards, so eviction is safe. + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + assert!( + !ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "entry pruned once its spend height is final" + ); + ctx.managed_wallet + .first_coinjoin_managed_account_mut() + .expect("account") + .simulate_reload_rebuild_spent_outpoints(); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!( + !utxo_tracked(&ctx.managed_wallet, &funding_outpoint), + "after prune the finalized record short-circuits any resurrection" + ); +} + +/// Nothing is pruned above the finality boundary: entries with height above the +/// chainlock height are retained regardless of `synced_height`, and entries at +/// or below the chainlock but above `synced_height` are retained too (the +/// rescan-in-progress guard). +#[test] +fn no_prune_above_finality_boundary() { + let op = |b: u8| OutPoint::new(dashcore::Txid::from([b; 32]), 0); + + let mut wallet = ManagedWalletInfo::new(Network::Testnet, [21u8; 32]); + wallet.record_observed_spends(&spend_to_external(&[op(1)], 1, 70), 100); + wallet.record_observed_spends(&spend_to_external(&[op(2)], 1, 71), 200); + wallet.record_observed_spends(&spend_to_external(&[op(3)], 1, 72), 300); + + // boundary = min(chainlock 200, synced 250) = 200. + wallet.update_last_processed_height(250); + wallet.apply_chain_lock(ChainLock::dummy(200)); + wallet.update_synced_height(250); + assert!( + wallet.observed_spent_outpoints().contains_key(&op(3)), + "entry above the chainlock height (300 > 200) retained regardless of synced_height" + ); + assert!( + !wallet.observed_spent_outpoints().contains_key(&op(1)) + && !wallet.observed_spent_outpoints().contains_key(&op(2)), + "entries at or below the boundary (100, 200) evicted" + ); + + // Rescan-in-progress: chainlock ahead, synced behind → boundary follows synced. + let mut w2 = ManagedWalletInfo::new(Network::Testnet, [22u8; 32]); + w2.record_observed_spends(&spend_to_external(&[op(4)], 1, 73), 150); + w2.update_last_processed_height(300); + w2.apply_chain_lock(ChainLock::dummy(300)); + w2.update_synced_height(100); + assert!( + w2.observed_spent_outpoints().contains_key(&op(4)), + "entry at 150 retained while synced_height (100) < its height, despite chainlock 300" + ); +} + +/// Anti-LRU pin: an old entry above the boundary survives arbitrary block-height +/// advances as long as no chainlock defines a finality boundary. Pruning is +/// event-driven (chainlock/checkpoint), never age- or recency-based. +#[test] +fn prune_is_event_driven_not_age_based() { + let mut wallet = ManagedWalletInfo::new(Network::Testnet, [23u8; 32]); + let op = OutPoint::new(dashcore::Txid::from([9u8; 32]), 0); + wallet.record_observed_spends(&spend_to_external(&[op], 1, 70), 100); + + // Advance processed/synced height far ahead, but never apply a chainlock. + wallet.update_last_processed_height(1_000_000); + wallet.update_synced_height(1_000_000); + assert!( + wallet.observed_spent_outpoints().contains_key(&op), + "no chainlock ⇒ no finality boundary ⇒ entry retained regardless of height/age" + ); +} + +/// After an entry is evicted, replaying the funding and spend blocks in either +/// order converges to the correct final state (no live UTXO, no double-counted +/// `net_amount`) and repopulates the map so it is re-prunable. +#[tokio::test] +async fn evicted_entry_self_heals_on_replay() { + let mut ctx = TestWalletContext::new_random(); + + let funding_value = 1_000_000u64; + let funding_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[funding_value]); + let funding_outpoint = OutPoint::new(funding_tx.txid(), 0); + let spend_tx = spend_to_external(&[funding_outpoint], funding_value - 1_000, 133); + + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + ctx.managed_wallet.update_last_processed_height(200); + ctx.managed_wallet.apply_chain_lock(ChainLock::dummy(200)); + ctx.managed_wallet.update_synced_height(200); + assert!(!ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + let balance = ctx.managed_wallet.balance.total(); + let history = history_net(&ctx.managed_wallet); + + // Replay order A — spend first: repopulates the map, coin stays reconciled. + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "replaying the spend block repopulates the observed-spent entry" + ); + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "order A: no resurrection"); + assert_eq!(ctx.managed_wallet.balance.total(), balance, "order A: balance not double-counted"); + assert_eq!(history_net(&ctx.managed_wallet), history, "order A: history not double-counted"); + + // The repopulated entry is re-prunable at the same, already-final boundary. + ctx.managed_wallet.prune_finalized_observed_spends(); + assert!(!ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint)); + + // Replay order B — funding first: the finalized record short-circuits, then + // the spend re-guards; final state is identical. + ctx.check_transaction(&funding_tx, block_ctx(100)).await; + assert!(!utxo_tracked(&ctx.managed_wallet, &funding_outpoint), "order B: no resurrection"); + ctx.check_transaction(&spend_tx, block_ctx(200)).await; + assert!( + ctx.managed_wallet.observed_spent_outpoints().contains_key(&funding_outpoint), + "order B: the replayed spend re-guards the coin" + ); + assert_eq!(ctx.managed_wallet.balance.total(), balance, "order B: balance stable"); + assert_eq!(history_net(&ctx.managed_wallet), history, "order B: history stable"); +} diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index 7ea653924..3507e3a1a 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -6,8 +6,6 @@ pub(crate) use super::account_checker::TransactionCheckResult; use super::transaction_context::TransactionContext; use super::transaction_router::TransactionRouter; -#[cfg(test)] -use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::wallet::managed_wallet_info::ManagedWalletInfo; use crate::{KeySource, Wallet}; @@ -62,7 +60,37 @@ impl WalletTransactionChecker for ManagedWalletInfo { // Check only relevant account types let mut result = self.accounts.check_transaction(tx, &relevant_types); + // #649: remember every spend seen in a block, independent of the + // spending tx's classification or whether the wallet can attribute it. + // Insert-only here (no account mutation), so a spend that IS attributed + // still has its `input_details` built from the live funding UTXO by + // `record_transaction` below. Mempool / IS-lock spends are deliberately + // never recorded — an unconfirmed spend must not invalidate a coin. + let block_height = if update_state { + context.block_info().map(|info| info.height()) + } else { + None + }; + if let Some(height) = block_height { + self.record_observed_spends(tx, height); + } + if !update_state || !result.is_relevant { + // A block spend the wallet cannot attribute to the owning account + // (routed away by tx-type narrowing, or unmatched because its + // funding lives in another account) still consumes a real coin. + // Drop it now, un-gated by classification — safe on this path + // precisely because no `record_transaction` runs for an unmatched + // tx, so no `input_details` depend on the coin still being present + // (#649: the funding-first mirror of the out-of-order ordering, + // including a spend whose classification excludes the owning + // account's type). + if block_height.is_some() && self.remove_spent_from_accounts(tx) { + result.state_modified = true; + if update_balance { + self.update_balance(); + } + } return result; } @@ -130,6 +158,7 @@ impl WalletTransactionChecker for ManagedWalletInfo { &account_match, context.clone(), tx_type, + &self.observed_spent_outpoints, ); account.mark_utxos_instant_send(&txid); result.new_records.push(record); @@ -156,15 +185,24 @@ impl WalletTransactionChecker for ManagedWalletInfo { }; if is_new { - let record = - account.record_transaction(tx, &account_match, context.clone(), tx_type); + let record = account.record_transaction( + tx, + &account_match, + context.clone(), + tx_type, + &self.observed_spent_outpoints, + ); result.new_records.push(record); result.state_modified = true; } else { let existed_before = account.has_transaction(&tx.txid()); - if let Some(record) = - account.confirm_transaction(tx, &account_match, context.clone(), tx_type) - { + if let Some(record) = account.confirm_transaction( + tx, + &account_match, + context.clone(), + tx_type, + &self.observed_spent_outpoints, + ) { result.state_modified = true; if existed_before { result.updated_records.push(record); @@ -212,6 +250,14 @@ impl WalletTransactionChecker for ManagedWalletInfo { } } + // #649 funding-first ordering: drop any coin this tx spends that the + // matched-account path missed (spend routed to another account type). + // Block-context only; idempotent. Spend-first is handled at insert time. + let spent_removed = block_height.is_some() && self.remove_spent_from_accounts(tx); + if spent_removed { + result.state_modified = true; + } + if is_new { // Populate dedup sets when a tx arrives with an initial IS status if context.is_instant_send() { @@ -241,6 +287,7 @@ impl WalletTransactionChecker for ManagedWalletInfo { mod tests { use super::*; use crate::account::account_type::StandardAccountType; + use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::transaction_record::{OutputRole, TransactionDirection}; use crate::test_utils::TestWalletContext; use crate::transaction_checking::BlockInfo; @@ -256,6 +303,7 @@ mod tests { use dashcore::TxOut; use dashcore::{Address, BlockHash, TxIn, Txid}; use dashcore_hashes::Hash; + use std::collections::BTreeMap; /// Test wallet checker with unrelated transaction #[tokio::test] @@ -1375,7 +1423,13 @@ mod tests { let block_context = TransactionContext::InBlock(BlockInfo::new(600, block_hash, 1700000000)); let tx_type = TransactionRouter::classify_transaction(&tx); - let backfilled = account.confirm_transaction(&tx, &account_match, block_context, tx_type); + let backfilled = account.confirm_transaction( + &tx, + &account_match, + block_context, + tx_type, + &BTreeMap::new(), + ); assert!(backfilled.is_some(), "Should return Some when backfilling a missing record"); // Verify the transaction was recorded with block context @@ -1426,7 +1480,13 @@ mod tests { .first_bip44_managed_account_mut() .expect("Should have BIP44 account"); let tx_type = TransactionRouter::classify_transaction(&tx); - let confirmed = account.confirm_transaction(&tx, &account_match, block_context, tx_type); + let confirmed = account.confirm_transaction( + &tx, + &account_match, + block_context, + tx_type, + &BTreeMap::new(), + ); assert!(confirmed.is_some(), "Should return Some when confirming unconfirmed tx"); let record = account.transactions().get(&txid).expect("Should have record"); diff --git a/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs b/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs index 40610e6d2..498172e68 100644 --- a/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs +++ b/key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs @@ -78,6 +78,9 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Insert into the collection self.accounts.insert(managed_account)?; + // A newly added account has no filter coverage, so the sync certificate + // must be invalidated or pruning could reopen #649 for its coins. + self.rewind_sync_checkpoint_for_new_account(); Ok(()) } @@ -110,6 +113,9 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Insert into the collection self.accounts.insert(managed_account)?; + // A newly added account has no filter coverage, so the sync certificate + // must be invalidated or pruning could reopen #649 for its coins. + self.rewind_sync_checkpoint_for_new_account(); Ok(()) } @@ -155,6 +161,9 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Insert into the collection self.accounts.insert(managed_account)?; + // A newly added account has no filter coverage, so the sync certificate + // must be invalidated or pruning could reopen #649 for its coins. + self.rewind_sync_checkpoint_for_new_account(); Ok(()) } @@ -188,6 +197,9 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Insert into the collection self.accounts.insert(managed_account)?; + // A newly added account has no filter coverage, so the sync certificate + // must be invalidated or pruning could reopen #649 for its coins. + self.rewind_sync_checkpoint_for_new_account(); Ok(()) } @@ -234,6 +246,9 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Insert into the collection self.accounts.insert(managed_account)?; + // A newly added account has no filter coverage, so the sync certificate + // must be invalidated or pruning could reopen #649 for its coins. + self.rewind_sync_checkpoint_for_new_account(); Ok(()) } @@ -272,6 +287,9 @@ impl ManagedAccountOperations for ManagedWalletInfo { // Insert into the collection self.accounts.insert(managed_account)?; + // A newly added account has no filter coverage, so the sync certificate + // must be invalidated or pruning could reopen #649 for its coins. + self.rewind_sync_checkpoint_for_new_account(); Ok(()) } } diff --git a/key-wallet/src/wallet/managed_wallet_info/mod.rs b/key-wallet/src/wallet/managed_wallet_info/mod.rs index 62b3d9e71..6ca688dfe 100644 --- a/key-wallet/src/wallet/managed_wallet_info/mod.rs +++ b/key-wallet/src/wallet/managed_wallet_info/mod.rs @@ -19,14 +19,16 @@ use super::balance::WalletCoreBalance; use super::metadata::WalletMetadata; use crate::account::ManagedAccountCollection; use crate::managed_account::managed_account_trait::ManagedAccountTrait; +use crate::managed_account::ManagedCoreFundsAccount; use crate::wallet::managed_wallet_info::transaction_building::AccountTypePreference; use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::{Network, Wallet}; +use dashcore::blockdata::transaction::OutPoint; use dashcore::prelude::CoreBlockHeight; -use dashcore::{Address, Txid}; +use dashcore::{Address, Transaction, Txid}; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; /// Information about a managed wallet /// @@ -46,15 +48,169 @@ pub struct ManagedWalletInfo { pub description: Option, /// Wallet metadata pub metadata: WalletMetadata, - /// All managed accounts + /// All managed accounts. + /// + /// Prefer the `add_managed_*` methods to add accounts to a live wallet: + /// they rewind the sync checkpoint so the new account's coins get filter + /// coverage. Inserting directly here without that rewind can reopen + /// dashpay/rust-dashcore#649 for the added account. pub accounts: ManagedAccountCollection, /// Cached wallet core balance - should be updated when accounts change pub balance: WalletCoreBalance, /// Transactions that have received an InstantSend lock. #[cfg_attr(feature = "serde", serde(skip))] pub(crate) instant_send_locks: HashSet, + /// Outpoints observed spent by transactions seen in a processed block, + /// mapped to the height of the block that spent them. + /// + /// This is the wallet-level, classification-independent record of "this + /// coin was consumed on-chain" used to keep out-of-order rescan delivery + /// from resurrecting a spent UTXO (dashpay/rust-dashcore#649). A spend can + /// be delivered before the transaction that funded the coin it spends, and + /// it may be routed away from — or fail to match — the account that owns + /// the coin; recording every block-observed spend here, independent of the + /// spending transaction's classification, lets the funding-side insert be + /// reconciled away whichever order the two blocks arrive in. + /// + /// Membership is bounded-permanent: an entry is retained until its spend + /// height is provably final, then evicted by + /// [`Self::prune_finalized_observed_spends`]. Until then it is only ever + /// added, never removed, and reorg rollback does NOT retract it — treating a + /// coin as spendable again after a spend was observed is the failure this + /// set exists to prevent. Only spends seen in a block (`InBlock` / + /// `InChainLockedBlock`) are recorded; mempool-context spends are never + /// recorded, so an unconfirmed spend can never wrongly invalidate a coin. + /// + /// # Bounded permanence + /// + /// An entry `(outpoint, height)` is removed only when + /// `height <= min(last_applied_chain_lock.block_height, synced_height)` — + /// the finality boundary. At that boundary the spend is chain-locked (it can + /// never be reorged out) and any funding transaction for the outpoint has + /// been delivered (BIP158 filters have no false negatives below + /// `synced_height`) and finalized (promoted into `finalized_txids`, or kept + /// as a chainlocked record), so every redelivery path short-circuits before + /// a coin could be re-inserted — in both `keep-finalized-transactions` + /// configurations and across a reload. No other removal path may be added + /// without a deliberate decision. + /// + /// Eviction is event-driven (chainlock application, sync-checkpoint commit), + /// never age- or recency-based: during an out-of-order rescan `synced_height` + /// is low, so nothing is pruned in exactly the window where #649 ordering + /// hazards live, and the set self-repopulates on any replay since + /// `record_observed_spends` runs unconditionally per checked tx. A naive + /// age/LRU eviction would instead evict the cold entries whose funding tx may + /// still arrive out of order, reopening #649 for that coin. Steady-state size + /// is the above-boundary window only — roughly one block's inputs on a + /// healthy chain. A defensive cap on the deserialized entry count (see the + /// serde adapter) guards against a corrupted or hostile wallet file forcing + /// an unbounded allocation on load. + /// + /// # Persistence + /// + /// The field is serde-serializable so it is preserved wherever a + /// `ManagedWalletInfo` is serialized. Its real value, though, is + /// order-independent correctness *within a single continuous processing run* + /// — which is what the observed #649 symptom needs, since a cold rescan is a + /// full fresh replay in one process. `#[serde(default)]` keeps snapshots + /// written before this field existed loadable, seeding an empty set (a + /// pre-fix wallet gets no retroactive backfill; only spends observed after + /// the field exists are protected). That default applies only to + /// self-describing formats (e.g. JSON) that can detect the absent field; + /// non-self-describing serde formats (bincode-serde, `platform_value::Value`) + /// cannot default a missing field, so consumers using those need a versioned + /// migration to load pre-field snapshots. + #[cfg_attr(feature = "serde", serde(default, with = "observed_spent_outpoints_serde"))] + pub(crate) observed_spent_outpoints: BTreeMap, } +/// Serde adapter for [`ManagedWalletInfo::observed_spent_outpoints`] that +/// encodes the map as a sequence of `(OutPoint, height)` pairs rather than as a +/// map. +/// +/// This is for format-agnosticism, not a JSON limitation: `OutPoint` serializes +/// to a string in human-readable formats (so it is a fine `serde_json` object +/// key) but to a struct `{ txid, vout }` in non-human-readable / strict-key +/// encoders (bincode's serde layer, `platform_value::Value`), where a struct +/// cannot be a map key. Encoding the map as a sequence of key/value pairs +/// round-trips identically across every serde format. +/// +/// Deserialization applies a defensive cap ([`MAX_OBSERVED_SPENT_OUTPOINTS`]): +/// a corrupted or hostile wallet file declaring an absurd number of entries is +/// rejected rather than driving an unbounded allocation. +#[cfg(feature = "serde")] +mod observed_spent_outpoints_serde { + use super::{BTreeMap, CoreBlockHeight, OutPoint, MAX_OBSERVED_SPENT_OUTPOINTS}; + use core::fmt; + use serde::de::{Error as _, SeqAccess, Visitor}; + use serde::ser::SerializeSeq; + use serde::{Deserializer, Serializer}; + + pub(super) fn serialize( + map: &BTreeMap, + serializer: S, + ) -> Result + where + S: Serializer, + { + let mut seq = serializer.serialize_seq(Some(map.len()))?; + for entry in map { + seq.serialize_element(&entry)?; + } + seq.end() + } + + pub(super) fn deserialize<'de, D>( + deserializer: D, + ) -> Result, D::Error> + where + D: Deserializer<'de>, + { + struct PairsVisitor; + + impl<'de> Visitor<'de> for PairsVisitor { + type Value = BTreeMap; + + fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "a sequence of (OutPoint, height) pairs") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: SeqAccess<'de>, + { + // Stream elements one at a time and cap the count so an + // oversized declared length cannot force an unbounded + // allocation before we notice. + let mut map = BTreeMap::new(); + while let Some((outpoint, height)) = + seq.next_element::<(OutPoint, CoreBlockHeight)>()? + { + if map.len() >= MAX_OBSERVED_SPENT_OUTPOINTS { + return Err(A::Error::custom(format!( + "observed_spent_outpoints exceeds the maximum of {MAX_OBSERVED_SPENT_OUTPOINTS} entries" + ))); + } + map.insert(outpoint, height); + } + Ok(map) + } + } + + deserializer.deserialize_seq(PairsVisitor) + } +} + +/// Defensive upper bound on the number of [`ManagedWalletInfo::observed_spent_outpoints`] +/// entries accepted when deserializing a wallet. +/// +/// Chosen far above any plausible legitimate size (matched-block spend activity +/// over a realistic rescan) so it only ever rejects a corrupted or hostile +/// wallet file, never a real one. At ~40 bytes per entry this caps the load-time +/// allocation for this field at a few hundred MB. +#[cfg(feature = "serde")] +const MAX_OBSERVED_SPENT_OUTPOINTS: usize = 10_000_000; + impl ManagedWalletInfo { /// Create new managed wallet info with network and wallet ID pub fn new(network: Network, wallet_id: [u8; 32]) -> Self { @@ -67,6 +223,7 @@ impl ManagedWalletInfo { accounts: ManagedAccountCollection::new(), balance: WalletCoreBalance::default(), instant_send_locks: HashSet::new(), + observed_spent_outpoints: BTreeMap::new(), } } @@ -81,6 +238,7 @@ impl ManagedWalletInfo { accounts: ManagedAccountCollection::new(), balance: WalletCoreBalance::default(), instant_send_locks: HashSet::new(), + observed_spent_outpoints: BTreeMap::new(), } } @@ -105,6 +263,7 @@ impl ManagedWalletInfo { accounts: ManagedAccountCollection::from_account_collection(&wallet.accounts), balance: WalletCoreBalance::default(), instant_send_locks: HashSet::new(), + observed_spent_outpoints: BTreeMap::new(), } } @@ -136,6 +295,142 @@ impl ManagedWalletInfo { &self.instant_send_locks } + /// Read-only access to the wallet-level observed-spent outpoint set + /// (dashpay/rust-dashcore#649), mapping each on-chain-spent outpoint to the + /// height of the block that spent it. See the field's doc comment for the + /// invariants; exposed for diagnostics and tests. + pub fn observed_spent_outpoints(&self) -> &BTreeMap { + &self.observed_spent_outpoints + } + + /// Record every outpoint `tx` spends into [`Self::observed_spent_outpoints`] + /// at `height`. Insert-only bookkeeping — it never touches account UTXO sets, + /// so it is safe to call before `record_transaction` builds a spend's + /// `input_details` from the still-present funding UTXO. + /// + /// A coinbase spends nothing real (its single input is the null prevout), so + /// it is skipped. Call this only for block-context transactions + /// (`InBlock` / `InChainLockedBlock`); mempool spends must never be recorded + /// (see the field doc — an unconfirmed spend may never be mined). + pub(crate) fn record_observed_spends(&mut self, tx: &Transaction, height: CoreBlockHeight) { + if tx.is_coin_base() { + return; + } + for input in &tx.input { + self.observed_spent_outpoints.insert(input.previous_output, height); + } + } + + /// Evict [`Self::observed_spent_outpoints`] entries at or below the finality + /// boundary `min(last_applied_chain_lock.block_height, synced_height)`. + /// + /// An entry `(outpoint, height)` with `height <= boundary` is safe to + /// forget: the spend at that height is chain-locked (never reorged out) and + /// any funding transaction for the outpoint has been delivered and finalized, + /// so no redelivery path can re-insert the coin (dashpay/rust-dashcore#649). + /// No-op until a chainlock has been applied (`last_applied_chain_lock` is + /// `None`) — without a finality boundary nothing can be proven final. + /// + /// Called on chainlock application and sync-checkpoint commit; not age- or + /// recency-based. + pub(crate) fn prune_finalized_observed_spends(&mut self) { + let Some(chain_lock) = self.metadata.last_applied_chain_lock.as_ref() else { + return; + }; + let boundary = chain_lock.block_height.min(self.metadata.synced_height); + self.observed_spent_outpoints.retain(|_, height| *height > boundary); + } + + /// Invalidate the wallet's sync certificate when an account is added. + /// + /// `synced_height` certifies "every filter at or below this height was + /// matched against the wallet's scripts" — but only for the account set that + /// existed while those filters were scanned. A newly added account has had no + /// filter coverage, so the certificate no longer holds for the current + /// account set, and [`Self::prune_finalized_observed_spends`] must not + /// consume it (dashpay/rust-dashcore#649). Rewinding `synced_height` to just + /// below wallet birth collapses the prune boundary and makes the sync layer + /// detect the wallet as behind and backfill from birth (the dash-spv filter + /// tick). A wallet still in initial sync (`synced_height` already at or below + /// the floor) is left untouched; `birth_height == 0` saturates safely. + /// + /// `last_processed_height` and `last_applied_chain_lock` are intentionally + /// left as-is: they are chain facts, not coverage facts, and keeping the + /// chainlock is what lets the backfilled history arrive born-chainlocked. + fn rewind_sync_checkpoint_for_new_account(&mut self) { + let floor = self.metadata.birth_height.saturating_sub(1); + self.metadata.synced_height = self.metadata.synced_height.min(floor); + } + + /// Drop any live UTXO consumed by `tx`'s inputs from whichever funding + /// account currently holds it, scanning ALL funding accounts independently + /// of the spending transaction's classification (dashpay/rust-dashcore#649). + /// Returns whether any UTXO was removed. + /// + /// This is the un-gated counterpart to the normal spend path in + /// `update_utxos`: it covers spends the wallet cannot attribute to the + /// owning account (routed away by tx-type narrowing, or unmatched because + /// the funding was seen in a different account). It is idempotent — a coin + /// the normal path already removed is simply absent — so it never + /// double-counts a spend. Coinbase inputs are skipped (null prevout). + /// + /// This is the funding-first ordering: the funding record is already + /// live, so its history is recomputed in place — see + /// [`Self::finalize_guard_removed_utxo`]. + pub(crate) fn remove_spent_from_accounts(&mut self, tx: &Transaction) -> bool { + if tx.is_coin_base() { + return false; + } + let mut removed = false; + // Fetch the account handles once, not once per input. + let mut accounts = self.accounts.all_funding_accounts_mut(); + for input in &tx.input { + let outpoint = input.previous_output; + for account in accounts.iter_mut() { + let account: &mut ManagedCoreFundsAccount = account; + if let Some(utxo) = account.utxos.remove(&outpoint) { + account.bump_monitor_revision(); + Self::finalize_guard_removed_utxo( + account, + &utxo, + &self.observed_spent_outpoints, + ); + removed = true; + break; + } + } + } + removed + } + + /// Finalize the account-local bookkeeping for a UTXO the #649 guard + /// removed (funding-first ordering: the record was already live). + /// + /// 1. Marks the outpoint spent so reprocessing won't resurrect the coin. + /// 2. Releases any build reservation held on the coin, so it is freed + /// immediately rather than lingering until the reservation TTL backstop + /// — matching the normal spend path in `update_utxos`. + /// 3. Recomputes the funding record's history via + /// [`TransactionRecord::compensate_for_observed_spends`]. The record is + /// kept at `net_amount == 0` rather than deleted when fully + /// compensated: a later reprocessing would otherwise re-record it as a + /// fresh sighting with no coin left to reconcile against, reopening + /// the divergence. This step is skipped when the record is absent + /// (pruned/finalized); steps 1 and 2 still run. + fn finalize_guard_removed_utxo( + account: &mut ManagedCoreFundsAccount, + removed: &Utxo, + observed_spent: &BTreeMap, + ) { + account.mark_outpoint_spent(removed.outpoint); + account.release_reservation_for(&removed.outpoint); + + let funding_txid = removed.outpoint.txid; + if let Some(record) = account.transactions_mut().get_mut(&funding_txid) { + record.compensate_for_observed_spends(observed_spent); + } + } + pub fn next_change_address( &mut self, wallet: &Wallet, 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 ca7aca184..f581a33c3 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 @@ -329,6 +329,11 @@ impl WalletInfoInterface for ManagedWalletInfo { self.metadata.last_applied_chain_lock = Some(chain_lock); } + // Evict observed-spent entries the promotion above just made final. + // Must run after both the per-account promotion and the metadata + // advance, so the finality boundary reflects this chainlock. + self.prune_finalized_observed_spends(); + ApplyChainLockOutcome { locked_transactions, metadata_advanced: advance, @@ -478,6 +483,9 @@ impl WalletInfoInterface for ManagedWalletInfo { fn update_synced_height(&mut self, current_height: u32) { self.metadata.synced_height = current_height; + // A newly committed checkpoint can lift the finality boundary when the + // chainlock was already ahead of the old synced_height. + self.prune_finalized_observed_spends(); } fn matured_coinbase_records(