diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index d9fc901ec..46b731524 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -970,11 +970,7 @@ impl = Vec::new(); for wallet_id in &behind { let synced = wallet.wallet_synced_height(wallet_id); - // The scan query, not the full monitored set: spent single-use - // (CoinJoin) addresses are pruned so the per-filter match cost - // stays bounded by active UTXOs + gap lookahead instead of - // growing with every historical mixing round - // (dashpay/rust-dashcore#948). + // The scan query, which equals the monitored set today. let scripts = wallet.scan_script_pubkeys_for(wallet_id); // Bare owner/voting key hashes a compact filter carries beyond the // wallet's scriptPubKeys. @@ -2188,8 +2184,7 @@ mod tests { /// `scan_batch` matches filters against the wallet's scan query /// (`scan_script_pubkeys_for`), not the full monitored set: a monitored - /// script pruned from the scan query — a spent single-use CoinJoin - /// address (dashpay/rust-dashcore#948) — must not pull its block in. + /// script the scan query does not carry must not pull its block in. #[tokio::test] async fn test_scan_batch_uses_pruned_scan_query() { let wallet_id: WalletId = [0x03; 32]; diff --git a/key-wallet-manager/benches/filter_scan.rs b/key-wallet-manager/benches/filter_scan.rs index e9eed83d2..e7638c554 100644 --- a/key-wallet-manager/benches/filter_scan.rs +++ b/key-wallet-manager/benches/filter_scan.rs @@ -1,16 +1,12 @@ -//! Compact-filter matching cost: full monitored set vs the pruned -//! forward-scan set for a mixing-heavy CoinJoin wallet -//! (dashpay/rust-dashcore#948). +//! How compact-filter matching cost grows with the size of a mixing-heavy +//! CoinJoin wallet's query. //! //! Mimics a wallet mid-recovery after many mixing rounds. Every CoinJoin //! round pays a fresh single-use address, so the account accumulates `used` //! spent addresses, keeps a small set of still-funded denominations //! ([`LIVE_UTXOS`]), and watches the usual gap-limit lookahead on top. One //! scan batch of BIP158 filters is then matched with -//! `monitored_script_pubkeys_for` (the pre-#948 query, which drags every -//! historical address through SipHash + sort per filter) and with -//! `scan_script_pubkeys_for` (the pruned query, bounded by live UTXOs + gap -//! lookahead). +//! `monitored_script_pubkeys_for`, the query the scan runs. //! //! BIP158 keys each filter's SipHashes off the block hash, so the whole //! query set is re-hashed and re-sorted per filter — which is exactly why @@ -142,29 +138,13 @@ fn bench_filter_scan(c: &mut Criterion) { for used in USED_ADDRESSES { let (manager, wallet_id) = wallet_with_mixing_history(used); let monitored = manager.monitored_script_pubkeys_for(&wallet_id); - let pruned = manager.scan_script_pubkeys_for(&wallet_id); - assert!( - pruned.len() < monitored.len(), - "the scan query must shrink once CoinJoin addresses are spent" - ); - println!( - "used={used}: monitored query = {} scripts, pruned scan query = {} scripts", - monitored.len(), - pruned.len() - ); + println!("used={used}: query = {} scripts", monitored.len()); - for (name, scripts) in [("monitored", &monitored), ("pruned", &pruned)] { - group.bench_with_input(BenchmarkId::new(name, used), scripts, |b, scripts| { - b.iter(|| { - check_compact_filters_for_elements( - black_box(&filters), - black_box(scripts), - &[], - 0, - ) - }) - }); - } + group.bench_with_input(BenchmarkId::new("monitored", used), &monitored, |b, scripts| { + b.iter(|| { + check_compact_filters_for_elements(black_box(&filters), black_box(scripts), &[], 0) + }) + }); } group.finish(); diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index 1e3322e47..cc442df16 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -592,19 +592,59 @@ impl WalletManager { #[cfg(test)] mod tests { use super::*; + use crate::matching::{check_compact_filters_for_elements, FilterMatchKey}; use crate::test_helpers::*; + use dashcore::bip158::BlockFilter; use dashcore::block::{Header, Version}; use dashcore::hashes::Hash; use dashcore::pow::CompactTarget; use dashcore::{ BlockHash, Network, OutPoint, ScriptBuf, TxIn, TxMerkleNode, TxOut, Txid, Witness, }; + use key_wallet::account::ManagedAccountTrait as _; use key_wallet::account::StandardAccountType; use key_wallet::mnemonic::Language; use key_wallet::wallet::initialization::WalletAccountCreationOptions; use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference; use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; use key_wallet::{AccountType, Mnemonic}; + use std::collections::HashMap; + + fn coinjoin_account<'a>( + manager: &'a WalletManager, + wallet_id: &WalletId, + ) -> &'a key_wallet::managed_account::ManagedCoreFundsAccount { + manager + .get_wallet_info(wallet_id) + .expect("wallet info") + .accounts + .coinjoin_accounts + .get(&0) + .expect("CoinJoin account 0") + } + + fn spend_first_output_of(tx: &Transaction) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: tx.txid(), + vout: 0, + }, + script_sig: ScriptBuf::new(), + sequence: u32::MAX, + witness: Witness::default(), + }], + output: vec![TxOut { + value: tx.output[0].value, + script_pubkey: ScriptBuf::new_p2pkh(&dashcore::PubkeyHash::from_byte_array( + [0x77; 20], + )), + }], + special_transaction_payload: None, + } + } fn make_block(txdata: Vec) -> Block { Block { @@ -820,32 +860,57 @@ mod tests { ); } + /// A CoinJoin address used and left with no unspent output was once + /// dropped from the filter scan as unpayable. Mainnet pays such addresses + /// again, and the scan then never matched the block carrying the payment. #[tokio::test] - async fn test_scan_script_pubkeys_for_prunes_spent_coinjoin_addresses() { - use key_wallet::account::ManagedAccountTrait; - + async fn test_filter_scan_matches_a_second_payment_to_an_emptied_coinjoin_address() { let (mut manager, wallet_id, _addr) = setup_manager_with_wallet(); + let wallets = BTreeSet::from([wallet_id]); + + let coinjoin_addr = coinjoin_account(&manager, &wallet_id) + .all_addresses() + .first() + .cloned() + .expect("CoinJoin address"); + + let received = create_tx_paying_to(&coinjoin_addr, 0x11); + let block = Block::dummy(100, vec![received.clone()]); + manager.process_block_for_wallets(&block, block.block_hash(), 100, &wallets).await; + + let spend = spend_first_output_of(&received); + let block = Block::dummy(101, vec![spend]); + manager.process_block_for_wallets(&block, block.block_hash(), 101, &wallets).await; - // Untouched wallet: the scan set equals the monitored set. - let monitored = manager.monitored_script_pubkeys_for(&wallet_id); - assert_eq!(manager.scan_script_pubkeys_for(&wallet_id), monitored); - - // Mark a CoinJoin address used with no unspent output — a spent - // single-use address. The scan query drops it; the monitored set - // keeps it. - let info = manager.get_wallet_info_mut(&wallet_id).expect("wallet info"); - let coinjoin = info.accounts.coinjoin_accounts.get_mut(&0).expect("CoinJoin account 0"); - let spent_addr = coinjoin.all_addresses().first().cloned().expect("CoinJoin address"); - assert!(coinjoin.mark_address_used(&spent_addr)); - - let monitored = manager.monitored_script_pubkeys_for(&wallet_id); - let scan = manager.scan_script_pubkeys_for(&wallet_id); - assert!(monitored.contains(&spent_addr.script_pubkey())); - assert!(!scan.contains(&spent_addr.script_pubkey())); - assert_eq!(scan.len(), monitored.len() - 1); - - // Unknown wallet id yields an empty scan set. + // Used, and holding nothing: the state that used to drop it. + assert!(coinjoin_account(&manager, &wallet_id).all_addresses().contains(&coinjoin_addr)); + assert_eq!(manager.get_wallet_balance(&wallet_id).expect("balance").total(), 0); + assert_eq!( + manager.scan_script_pubkeys_for(&wallet_id), + manager.monitored_script_pubkeys_for(&wallet_id) + ); assert!(manager.scan_script_pubkeys_for(&[0xff; 32]).is_empty()); + + // The block carrying the second payment must match the scan query. + let later = Block::dummy(102, vec![create_tx_paying_to(&coinjoin_addr, 0x33)]); + let filters = HashMap::from([( + FilterMatchKey::new(102, later.block_hash()), + BlockFilter::dummy(&later), + )]); + let matched = check_compact_filters_for_elements( + &filters, + &manager.scan_script_pubkeys_for(&wallet_id), + &[], + 0, + ); + assert_eq!(matched.len(), 1, "the scan query must still watch the emptied address"); + + manager.process_block_for_wallets(&later, later.block_hash(), 102, &wallets).await; + assert_eq!( + manager.get_wallet_balance(&wallet_id).expect("balance").confirmed(), + TX_AMOUNT, + "the second payment must be credited" + ); } #[tokio::test] diff --git a/key-wallet-manager/src/test_utils/mock_wallet.rs b/key-wallet-manager/src/test_utils/mock_wallet.rs index a559f520c..63e02b487 100644 --- a/key-wallet-manager/src/test_utils/mock_wallet.rs +++ b/key-wallet-manager/src/test_utils/mock_wallet.rs @@ -391,8 +391,8 @@ pub struct MultiMockWallet { wallets: std::collections::BTreeMap, /// Per-wallet override for `scan_script_pubkeys_for`. Wallets absent here /// fall back to the monitored set, mirroring the trait default. Lets tests - /// hand the filter scan a pruned query while the monitored set stays full - /// (dashpay/rust-dashcore#948). + /// hand the filter scan a query that does not cover everything the wallet + /// watches. scan_addresses: std::collections::BTreeMap>, event_sender: broadcast::Sender, /// Track every block processed for assertions. diff --git a/key-wallet-manager/src/wallet_interface.rs b/key-wallet-manager/src/wallet_interface.rs index 484fb2c84..6f43d99de 100644 --- a/key-wallet-manager/src/wallet_interface.rs +++ b/key-wallet-manager/src/wallet_interface.rs @@ -91,14 +91,12 @@ pub trait WalletInterface: Send + Sync + 'static { /// Get the scriptPubKeys `wallet_id` wants matched during a forward /// compact-filter scan. /// - /// Defaults to [`Self::monitored_script_pubkeys_for`]. Implementations may - /// return a subset when some monitored scripts can no longer be paid in - /// practice — the managed-wallet implementation drops CoinJoin addresses - /// whose outputs are all spent, since those are single-use by protocol and - /// their monotonic growth dominates per-filter matching cost late in a - /// mixing-heavy recovery scan (dashpay/rust-dashcore#948). Block - /// processing still checks transactions against the full monitored set, so - /// pruning only narrows which blocks the filter scan downloads. + /// Defaults to [`Self::monitored_script_pubkeys_for`], and nothing narrows + /// it today — see [`scan_script_pubkeys`]. Block processing always uses + /// the full monitored set, so this only narrows which blocks get + /// downloaded. + /// + /// [`scan_script_pubkeys`]: key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface::scan_script_pubkeys fn scan_script_pubkeys_for(&self, wallet_id: &WalletId) -> Vec { self.monitored_script_pubkeys_for(wallet_id) } diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index ca3210bb6..5d0cca3d8 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -30,7 +30,7 @@ use crate::wallet::balance::WalletCoreBalance; use crate::{ExtendedPubKey, Network}; use dashcore::blockdata::transaction::OutPoint; use dashcore::prelude::CoreBlockHeight; -use dashcore::{Address, ScriptBuf, Transaction, Txid}; +use dashcore::{Address, Transaction, Txid}; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; @@ -206,28 +206,6 @@ impl ManagedCoreFundsAccount { } } - /// Cached scriptPubKeys for every address that could still receive or hold - /// funds under a single-use address discipline: addresses not yet used - /// (the gap-limit lookahead, including reserved ones) plus used addresses - /// that still hold at least one unspent output. - /// - /// A used address whose outputs are all spent is omitted. That is only - /// sound for account types whose addresses are single-use by protocol - /// (CoinJoin — reuse would link mixing rounds), where nothing ever pays a - /// spent-and-emptied address again; callers must not apply this to - /// account types where address reuse is merely discouraged. - pub fn unspent_or_unused_script_pubkeys(&self) -> Vec { - let funded: HashSet<&ScriptBuf> = - self.utxos.values().map(|utxo| &utxo.txout.script_pubkey).collect(); - self.managed_account_type() - .address_pools() - .iter() - .flat_map(|pool| pool.addresses.values()) - .filter(|info| !info.is_used() || funded.contains(&info.script_pubkey)) - .map(|info| info.script_pubkey.clone()) - .collect() - } - /// Add new UTXOs for received outputs, remove spent ones. /// /// Skips any output whose outpoint is already in `observed_spent` — it is @@ -1368,6 +1346,7 @@ mod conflict_sweep_walk_tests { use crate::transaction_checking::BlockInfo; use dashcore::ephemerealdata::instant_lock::InstantLock; use dashcore::hashes::Hash; + use dashcore::ScriptBuf; use dashcore::{BlockHash, TxIn, TxOut, Witness}; fn outpoint(seed: u32, vout: u32) -> OutPoint { diff --git a/key-wallet/src/tests/mod.rs b/key-wallet/src/tests/mod.rs index 66e42abde..8a91ccf52 100644 --- a/key-wallet/src/tests/mod.rs +++ b/key-wallet/src/tests/mod.rs @@ -28,8 +28,6 @@ mod performance_tests; mod provider_key_derivation_tests; -mod scan_script_pubkeys_tests; - mod special_transaction_matching_tests; mod special_transaction_tests; diff --git a/key-wallet/src/tests/scan_script_pubkeys_tests.rs b/key-wallet/src/tests/scan_script_pubkeys_tests.rs deleted file mode 100644 index 0158a6f52..000000000 --- a/key-wallet/src/tests/scan_script_pubkeys_tests.rs +++ /dev/null @@ -1,116 +0,0 @@ -//! Tests for the forward-scan query pruning of spent single-use (CoinJoin) -//! addresses (dashpay/rust-dashcore#948). -//! -//! `scan_script_pubkeys` must drop CoinJoin addresses that are used and hold -//! no unspent output, while keeping unused (gap-window) CoinJoin addresses, -//! used CoinJoin addresses that still hold a UTXO, and every address of every -//! other account type — used or not. - -use crate::account::ManagedAccountTrait; -use crate::wallet::initialization::WalletAccountCreationOptions; -use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; -use crate::wallet::{ManagedWalletInfo, Wallet}; -use crate::{Network, Utxo}; -use dashcore::blockdata::transaction::txout::TxOut; -use dashcore::hashes::Hash; -use dashcore::{Address, OutPoint, ScriptBuf, Txid}; - -/// Known test mnemonic for deterministic testing -const TEST_MNEMONIC: &str = - "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; - -fn setup_wallet_info() -> ManagedWalletInfo { - let mnemonic = crate::mnemonic::Mnemonic::from_phrase(TEST_MNEMONIC).unwrap(); - let wallet = - Wallet::from_mnemonic(mnemonic, Network::Testnet, WalletAccountCreationOptions::Default) - .unwrap(); - ManagedWalletInfo::from_wallet(&wallet, 0) -} - -fn dummy_utxo_for(address: &Address, salt: u8) -> Utxo { - Utxo::new( - OutPoint::new(Txid::from_byte_array([salt; 32]), 0), - TxOut { - value: 100_000, - script_pubkey: address.script_pubkey(), - }, - address.clone(), - 100, - false, - ) -} - -#[test] -fn test_scan_set_prunes_spent_and_empty_coinjoin_addresses() { - let mut info = setup_wallet_info(); - - let coinjoin = info.accounts.coinjoin_accounts.get_mut(&0).expect("CoinJoin account 0"); - let addresses = coinjoin.all_addresses(); - assert!(addresses.len() >= 2, "CoinJoin pools should pre-generate addresses"); - - // Address 0: used, all outputs spent (no UTXO left) — must be pruned. - let spent_addr = addresses[0].clone(); - // Address 1: used, but still holds an unspent output — must be kept. - let funded_addr = addresses[1].clone(); - - assert!(coinjoin.mark_address_used(&spent_addr)); - assert!(coinjoin.mark_address_used(&funded_addr)); - let utxo = dummy_utxo_for(&funded_addr, 0xaa); - coinjoin.utxos.insert(utxo.outpoint, utxo); - - let monitored = info.monitored_script_pubkeys(); - let scan = info.scan_script_pubkeys(); - - let spent_script = spent_addr.script_pubkey(); - let funded_script = funded_addr.script_pubkey(); - - assert!(monitored.contains(&spent_script), "monitored set keeps the spent address"); - assert!(!scan.contains(&spent_script), "scan set drops the spent-and-empty address"); - assert!(scan.contains(&funded_script), "scan set keeps the address still holding a UTXO"); - - // Exactly one script was pruned; every unused gap-window address stays. - assert_eq!(scan.len(), monitored.len() - 1); -} - -#[test] -fn test_scan_set_keeps_used_and_empty_standard_addresses() { - let mut info = setup_wallet_info(); - - let standard = - info.accounts.standard_bip44_accounts.get_mut(&0).expect("standard BIP44 account 0"); - let addr = standard.all_addresses().first().cloned().expect("pre-generated address"); - // Used with no remaining UTXO: a standard address can always be paid - // again, so the scan set must keep watching it. - assert!(standard.mark_address_used(&addr)); - - let scan = info.scan_script_pubkeys(); - assert!( - scan.contains(&addr.script_pubkey()), - "used-and-empty standard addresses stay in the scan set" - ); - assert_eq!(scan.len(), info.monitored_script_pubkeys().len()); -} - -#[test] -fn test_unspent_or_unused_script_pubkeys_on_funds_account() { - let mut info = setup_wallet_info(); - let coinjoin = info.accounts.coinjoin_accounts.get_mut(&0).expect("CoinJoin account 0"); - - let all: Vec = coinjoin.all_script_pubkeys(); - // Untouched account: nothing is used, so nothing is pruned. - assert_eq!(coinjoin.unspent_or_unused_script_pubkeys().len(), all.len()); - - // Mark one address used without a UTXO: it drops out. - let addr = coinjoin.all_addresses()[0].clone(); - assert!(coinjoin.mark_address_used(&addr)); - let pruned = coinjoin.unspent_or_unused_script_pubkeys(); - assert_eq!(pruned.len(), all.len() - 1); - assert!(!pruned.contains(&addr.script_pubkey())); - - // Give it back an unspent output: it returns to the scan set. - let utxo = dummy_utxo_for(&addr, 0xbb); - coinjoin.utxos.insert(utxo.outpoint, utxo); - let restored = coinjoin.unspent_or_unused_script_pubkeys(); - assert_eq!(restored.len(), all.len()); - assert!(restored.contains(&addr.script_pubkey())); -} diff --git a/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs b/key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs index b66293ce0..585effb53 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 @@ -7,7 +7,7 @@ use std::collections::{BTreeMap, BTreeSet}; use super::managed_account_operations::ManagedAccountOperations; use crate::account::{AccountType, ManagedAccountTrait}; use crate::managed_account::managed_account_collection::ManagedAccountCollection; -use crate::managed_account::managed_account_ref::{ManagedAccountRef, ManagedAccountRefMut}; +use crate::managed_account::managed_account_ref::ManagedAccountRefMut; use crate::managed_account::managed_account_type::ManagedAccountType; use crate::managed_account::ManagedCoreFundsAccount; use crate::transaction_checking::TransactionContext; @@ -92,14 +92,12 @@ pub trait WalletInfoInterface: Sized + WalletTransactionChecker + ManagedAccount /// Get the scriptPubKeys worth matching in a forward compact-filter scan. /// - /// Defaults to [`Self::monitored_script_pubkeys`]. Implementations may - /// return a subset when some monitored scripts can no longer be paid in - /// practice — [`ManagedWalletInfo`] drops CoinJoin addresses that are used - /// and hold no unspent output, since CoinJoin addresses are single-use by - /// protocol (reuse would link mixing rounds) and the query-set growth they - /// cause dominates late-scan filter matching for mixing-heavy wallets - /// (dashpay/rust-dashcore#948). Block processing and gap-limit maintenance - /// keep using the full monitored set; only the filter-scan query shrinks. + /// Returns the full monitored set, CoinJoin addresses included, however + /// long ago they were emptied. They are single-use by protocol, but + /// mainnet pays them again — 287 of one mixing-heavy wallet's were, some + /// nearly 100 000 blocks later — so dropping them loses transactions and + /// overstates the balance. Narrow this only with proof the dropped + /// scripts cannot be paid. fn scan_script_pubkeys(&self) -> Vec { self.monitored_script_pubkeys() } @@ -408,26 +406,6 @@ impl WalletInfoInterface for ManagedWalletInfo { scripts } - fn scan_script_pubkeys(&self) -> Vec { - let mut scripts = Vec::new(); - for account in self.accounts.all_accounts() { - // Only CoinJoin accounts are pruned: their addresses are - // single-use by protocol, so one that is used and holds no - // unspent output will never be paid again and contributes - // nothing to a forward scan. Every other account type keeps its - // full monitored set — address reuse there is possible even if - // discouraged. - if let ManagedAccountRef::Funds(funds) = account { - if matches!(funds.managed_account_type(), ManagedAccountType::CoinJoin { .. }) { - scripts.extend(funds.unspent_or_unused_script_pubkeys()); - continue; - } - } - scripts.extend(account.all_script_pubkeys()); - } - scripts - } - fn monitored_filter_elements(&self) -> Vec> { let mut elements = Vec::new(); for account in self.accounts.all_accounts() {