diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index f01fbcb59..17311d684 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -273,6 +273,12 @@ impl self.progress.filter_header_tip_height() { // Park the idle pipeline at the download frontier so a later @@ -1025,6 +1031,20 @@ impl SyncResult> { self.progress.update_filter_header_tip_height(tip_height); self.update_target_height(tip_height); + self.publish_scan_target(tip_height).await; match self.state() { SyncState::Syncing | SyncState::Synced diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index b751ada64..06b5775ea 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -278,6 +278,12 @@ impl WalletInterface for WalletM self.wallet_infos.get(wallet_id).map(|info| info.account_generation()).unwrap_or(0) } + fn update_scan_target_height(&mut self, height: CoreBlockHeight) { + for info in self.wallet_infos.values_mut() { + info.update_scan_target_height(height); + } + } + fn update_wallet_synced_height(&mut self, wallet_id: &WalletId, height: CoreBlockHeight) { if let Some(info) = self.wallet_infos.get_mut(wallet_id) { if height > info.synced_height() { diff --git a/key-wallet-manager/src/wallet_interface.rs b/key-wallet-manager/src/wallet_interface.rs index 3b175d9fe..e0b1f3d37 100644 --- a/key-wallet-manager/src/wallet_interface.rs +++ b/key-wallet-manager/src/wallet_interface.rs @@ -137,6 +137,18 @@ pub trait WalletInterface: Send + Sync + 'static { /// Return the per-wallet committed sync checkpoint, or `0` if unknown. fn wallet_synced_height(&self, wallet_id: &WalletId) -> CoreBlockHeight; + /// Publish the chain height the spend scan is working toward — the tip of + /// the filter-header chain the scanner has committed to covering. It is a + /// property of the chain rather than of any one wallet, so it applies to + /// all of them. + /// + /// Until a wallet's `synced_height` reaches this, that wallet is catching + /// up and cannot tell whether an output it just discovered was already + /// spent in a block it has not scanned yet, so such outputs are held back + /// from coin selection. The default is a no-op, leaving the gate open — + /// a scanner must call this for it to engage. + fn update_scan_target_height(&mut self, _height: CoreBlockHeight) {} + /// Return the generation of one wallet's account set — a counter bumped /// whenever an account is added to that wallet (`0` if unknown). Filter /// sync snapshots this per wallet when scanning a range and refuses to diff --git a/key-wallet/src/test_utils/wallet.rs b/key-wallet/src/test_utils/wallet.rs index 7f7859a5a..318b1213d 100644 --- a/key-wallet/src/test_utils/wallet.rs +++ b/key-wallet/src/test_utils/wallet.rs @@ -37,6 +37,19 @@ impl TestWalletContext { /// accounts. pub fn new_random_with_options(options: WalletAccountCreationOptions) -> Self { let wallet = Wallet::new_random(Network::Testnet, options).expect("Should create wallet"); + Self::from_wallet(wallet) + } + + /// Creates a testnet wallet from a fixed seed, so a failing test replays + /// with the same keys and addresses every run. + pub fn new_with_seed(seed: [u8; 64]) -> Self { + let wallet = + Wallet::from_seed_bytes(seed, Network::Testnet, WalletAccountCreationOptions::Default) + .expect("Should create wallet"); + Self::from_wallet(wallet) + } + + fn from_wallet(wallet: Wallet) -> Self { let mut managed_wallet = ManagedWalletInfo::from_wallet_with_name(&wallet, "Test".to_string(), 0); diff --git a/key-wallet/src/tests/mod.rs b/key-wallet/src/tests/mod.rs index 8a91ccf52..c7b7c5e1e 100644 --- a/key-wallet/src/tests/mod.rs +++ b/key-wallet/src/tests/mod.rs @@ -39,3 +39,5 @@ mod spent_outpoints_tests; mod unit_variant_wallet_tests; mod wallet_tests; + +mod spend_scan_frontier_tests; diff --git a/key-wallet/src/tests/spend_scan_frontier_tests.rs b/key-wallet/src/tests/spend_scan_frontier_tests.rs new file mode 100644 index 000000000..aaeee024f --- /dev/null +++ b/key-wallet/src/tests/spend_scan_frontier_tests.rs @@ -0,0 +1,211 @@ +//! Tests for the spend-scan frontier gate on coin selection. +//! +//! A wallet catching up on history applies blocks in ascending order, so a +//! receive it discovers says nothing about whether some higher, not-yet-scanned +//! block already spends it. Selecting such an output builds a transaction the +//! network settled as a double-spend long ago; peers drop it silently, with no +//! reject message, and whatever it was funding is stranded forever. +//! +//! The heights here are the ones from the incident that motivated this: a +//! restored wallet found a 1000 DASH receive in block 758983 and funded an +//! identity top-up from it three seconds later, while the block that had +//! already spent that outpoint — height 1510203 — was still six minutes of +//! scanning away. + +use dashcore::blockdata::transaction::{OutPoint, Transaction}; +use dashcore::hashes::Hash; +use dashcore::{BlockHash, TxIn}; + +use crate::test_utils::TestWalletContext; +use crate::transaction_checking::{BlockInfo, TransactionContext}; +use crate::wallet::managed_wallet_info::coin_selection::{ + CoinSelector, SelectionError, SelectionStrategy, +}; +use crate::wallet::managed_wallet_info::fee::FeeRate; +use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; + +/// Fixed wallet seed so a failing test replays with the same keys every run. +const SEED: [u8; 64] = [7; 64]; + +/// Heights from the incident. +const RECEIVE_HEIGHT: u32 = 758_983; +const SPEND_HEIGHT: u32 = 1_510_203; +const CHAIN_TIP: u32 = 2_200_000; + +fn block_at(height: u32) -> TransactionContext { + TransactionContext::InBlock(BlockInfo::new( + height, + BlockHash::from_slice(&[(height % 251) as u8; 32]).expect("hash"), + 1_700_000_000, + )) +} + +/// A transaction spending `outpoint`, standing in for the wallet's own earlier +/// asset lock that consumed the coin long before this restore. +fn spending_tx(outpoint: OutPoint) -> Transaction { + Transaction { + version: 1, + lock_time: 0, + input: vec![TxIn { + previous_output: outpoint, + ..Default::default() + }], + output: Vec::new(), + special_transaction_payload: None, + } +} + +/// Ask coin selection for `target` duffs out of the wallet's BIP44 account, +/// exactly as the transaction builder does. +fn select(ctx: &TestWalletContext, target: u64) -> Result { + let account = ctx.bip44_account(); + let utxos: Vec<_> = account.utxos.values().collect(); + CoinSelector::new(SelectionStrategy::BranchAndBound) + .select_coins(utxos, target, FeeRate::normal(), ctx.managed_wallet.last_processed_height()) + .map(|selection| selection.total_value) +} + +/// Put the wallet in the state a restore lands in: scanning toward the chain +/// tip from far below it, having applied a receive at `RECEIVE_HEIGHT`. +async fn restored_wallet_mid_catch_up(amount: u64) -> (TestWalletContext, Transaction) { + let mut ctx = TestWalletContext::new_with_seed(SEED); + ctx.managed_wallet.update_scan_target_height(CHAIN_TIP); + ctx.managed_wallet.update_synced_height(RECEIVE_HEIGHT); + ctx.managed_wallet.update_last_processed_height(RECEIVE_HEIGHT); + + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[amount]); + let result = ctx.check_transaction(&tx, block_at(RECEIVE_HEIGHT)).await; + assert!(result.is_relevant, "wallet must recognise its own receive"); + + (ctx, tx) +} + +#[tokio::test] +async fn receive_found_below_the_scan_target_is_tracked_but_not_selectable() { + let (ctx, tx) = restored_wallet_mid_catch_up(100_000_000_000).await; + + let utxo = ctx.first_utxo(); + assert_eq!(utxo.outpoint.txid, tx.txid(), "the receive is tracked"); + assert!(utxo.is_confirmed, "and it is confirmed in a block"); + assert!(!utxo.spend_scanned, "but the scan has not passed it yet"); + + assert!( + !ctx.managed_wallet.spend_scan_complete(), + "the wallet must report itself mid-catch-up" + ); + assert!( + ctx.managed_wallet.get_spendable_utxos().is_empty(), + "nothing is spendable while the scan is behind" + ); + assert!( + matches!(select(&ctx, 1_000_000), Err(SelectionError::NoUtxosAvailable)), + "coin selection must refuse to fund from an unscanned receive" + ); +} + +#[tokio::test] +async fn scan_reaching_its_target_releases_the_held_back_receive() { + let (mut ctx, _tx) = restored_wallet_mid_catch_up(100_000_000_000).await; + + // Catch-up finishes: every block up to the tip has been scanned and no + // spend of this outpoint turned up, so the coin is genuinely unspent. + ctx.managed_wallet.update_synced_height(CHAIN_TIP); + ctx.managed_wallet.update_last_processed_height(CHAIN_TIP); + + assert!(ctx.managed_wallet.spend_scan_complete()); + assert!(ctx.first_utxo().spend_scanned, "the scan has now covered it"); + assert_eq!(ctx.managed_wallet.get_spendable_utxos().len(), 1, "and it becomes spendable"); + assert_eq!(select(&ctx, 1_000_000).expect("selection succeeds"), 100_000_000_000); +} + +/// The incident itself: the coin was already spent, by this wallet's own +/// earlier transaction, in a block the restore had not reached yet. +#[tokio::test] +async fn coin_already_spent_above_the_frontier_is_never_selectable() { + let (mut ctx, tx) = restored_wallet_mid_catch_up(100_000_000_000).await; + let outpoint = ctx.first_utxo().outpoint; + + // This is the window in which the top-up asset lock was built. + assert!( + select(&ctx, 1_000_000).is_err(), + "the double-spend must not be fundable during catch-up" + ); + + // The scan reaches the block that spent it. + ctx.managed_wallet.update_synced_height(SPEND_HEIGHT); + let spend = spending_tx(outpoint); + ctx.check_transaction(&spend, block_at(SPEND_HEIGHT)).await; + + // ...and then finishes. + ctx.managed_wallet.update_synced_height(CHAIN_TIP); + ctx.managed_wallet.update_last_processed_height(CHAIN_TIP); + + assert!(!ctx.bip44_account().utxos.contains_key(&outpoint), "the spend removed the coin"); + assert!( + ctx.managed_wallet.get_spendable_utxos().is_empty(), + "so a completed scan releases nothing" + ); + assert_ne!(tx.txid(), spend.txid()); +} + +#[tokio::test] +async fn receive_at_the_scan_target_is_selectable_immediately() { + let mut ctx = TestWalletContext::new_with_seed(SEED); + ctx.managed_wallet.update_scan_target_height(CHAIN_TIP); + ctx.managed_wallet.update_synced_height(CHAIN_TIP); + ctx.managed_wallet.update_last_processed_height(CHAIN_TIP); + + // A block at the tip: nothing above it exists to have spent this output, + // so a caught-up wallet must not be made to wait for the next batch commit. + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[500_000]); + ctx.check_transaction(&tx, block_at(CHAIN_TIP)).await; + + assert!(ctx.first_utxo().spend_scanned); + assert_eq!(select(&ctx, 100_000).expect("selection succeeds"), 500_000); +} + +#[tokio::test] +async fn mempool_receive_during_catch_up_stays_selectable() { + let mut ctx = TestWalletContext::new_with_seed(SEED); + ctx.managed_wallet.update_scan_target_height(CHAIN_TIP); + ctx.managed_wallet.update_synced_height(RECEIVE_HEIGHT); + ctx.managed_wallet.update_last_processed_height(RECEIVE_HEIGHT); + + // A mempool transaction is at the tip by definition — no historical block + // can have spent an output that does not exist historically. + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[500_000]); + ctx.check_transaction(&tx, TransactionContext::Mempool).await; + + assert!(ctx.first_utxo().spend_scanned); + assert_eq!(select(&ctx, 100_000).expect("selection succeeds"), 500_000); +} + +/// Consumers with no scanner never report a target, and must keep the +/// pre-existing unconditional behavior. +#[tokio::test] +async fn without_a_reported_scan_target_the_gate_stays_open() { + let mut ctx = TestWalletContext::new_with_seed(SEED); + ctx.managed_wallet.update_last_processed_height(RECEIVE_HEIGHT); + + let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[500_000]); + ctx.check_transaction(&tx, block_at(RECEIVE_HEIGHT)).await; + + assert_eq!(ctx.managed_wallet.scan_target_height(), 0); + assert!(ctx.managed_wallet.spend_scan_complete()); + assert!(ctx.first_utxo().spend_scanned); + assert_eq!(select(&ctx, 100_000).expect("selection succeeds"), 500_000); +} + +/// A UTXO deserialized from persistence written before `spend_scanned` existed +/// must not become unspendable on upgrade. +#[cfg(feature = "serde")] +#[test] +fn utxo_missing_the_field_deserializes_as_scanned() { + let utxo = crate::Utxo::dummy(1, 500_000, 100, false, true); + let mut value = serde_json::to_value(&utxo).expect("serialize"); + value.as_object_mut().expect("object").remove("spend_scanned"); + + let restored: crate::Utxo = serde_json::from_value(value).expect("deserialize"); + assert!(restored.spend_scanned, "legacy rows keep the old behavior"); + assert!(restored.is_spendable(200)); +} diff --git a/key-wallet/src/transaction_checking/wallet_checker.rs b/key-wallet/src/transaction_checking/wallet_checker.rs index cb0272f2a..01d949f04 100644 --- a/key-wallet/src/transaction_checking/wallet_checker.rs +++ b/key-wallet/src/transaction_checking/wallet_checker.rs @@ -258,6 +258,13 @@ impl WalletTransactionChecker for ManagedWalletInfo { ); } + // Any UTXO just created from a block below the spend-scan target is + // only known-unspent as of that block, not as of the tip. Hold it back + // from coin selection until the scan proves it survived to the target. + if let Some(height) = block_height { + self.hold_back_unscanned_utxos(tx, height); + } + if update_balance { self.update_balance(); } diff --git a/key-wallet/src/utxo.rs b/key-wallet/src/utxo.rs index 5672f85be..298d14b71 100644 --- a/key-wallet/src/utxo.rs +++ b/key-wallet/src/utxo.rs @@ -39,6 +39,27 @@ pub struct Utxo { /// it is just our previously-tracked balance returning to us. Mirrors /// Bitcoin Core's `CWalletTx::IsTrusted` concept. pub is_trusted: bool, + /// Whether the wallet has scanned every block that could already have + /// spent this output. + /// + /// An output found while catching up on history is only *known* to be + /// unspent up to the scan frontier, which during catch-up sits far below + /// the chain tip. Spending it then builds a transaction the network has + /// long since seen double-spent, and peers drop it silently forever. So a + /// receive applied below the frontier starts unscanned, and is promoted in + /// bulk once the frontier reaches the height the scan is working toward. + /// + /// Defaults to `true` so wallets assembled by hand, and UTXOs + /// deserialized from persistence written before this field existed, keep + /// the previous unconditional behavior. Only a live scan demotes it. + #[cfg_attr(feature = "serde", serde(default = "spend_scanned_default"))] + pub spend_scanned: bool, +} + +/// Serde default for [`Utxo::spend_scanned`]; see that field's docs. +#[cfg(feature = "serde")] +fn spend_scanned_default() -> bool { + true } impl Utxo { @@ -60,6 +81,7 @@ impl Utxo { is_instantlocked: false, is_locked: false, is_trusted: false, + spend_scanned: true, } } @@ -70,13 +92,14 @@ impl Utxo { /// Check if this UTXO can be spent at the given height. /// - /// A UTXO is spendable unless it is locked or (for coinbase) + /// A UTXO is spendable unless it is locked, not yet covered by the + /// wallet's spend scan (see [`Utxo::spend_scanned`]), or — for coinbase — /// immature. Mempool 0-conf outputs are spendable — callers that /// want to restrict to confirmed/InstantLocked UTXOs (e.g. the /// "spendable" balance bucket or conservative coin selection) /// should check `is_confirmed || is_instantlocked` themselves. pub fn is_spendable(&self, current_height: u32) -> bool { - if self.is_locked { + if self.is_locked || !self.spend_scanned { return false; } self.is_mature(current_height) diff --git a/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs b/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs index 6aca94b34..49372ce39 100644 --- a/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs @@ -595,6 +595,7 @@ mod tests { is_instantlocked: false, is_locked: false, is_trusted: false, + spend_scanned: true, }; account.utxos.insert(outpoint, utxo); outpoint @@ -639,6 +640,7 @@ mod tests { is_instantlocked: false, is_locked: false, is_trusted: false, + spend_scanned: true, }; account.utxos.insert(outpoint, utxo); outpoint diff --git a/key-wallet/src/wallet/managed_wallet_info/mod.rs b/key-wallet/src/wallet/managed_wallet_info/mod.rs index 698e3327b..7ee5eba2f 100644 --- a/key-wallet/src/wallet/managed_wallet_info/mod.rs +++ b/key-wallet/src/wallet/managed_wallet_info/mod.rs @@ -376,6 +376,64 @@ impl ManagedWalletInfo { self.observed_spent_outpoints.retain(|_, height| *height > boundary); } + /// Hold back the UTXOs `tx` created from coin selection, because the wallet + /// applied `tx` from a block below the height its spend scan is working + /// toward. + /// + /// Blocks are scanned in ascending order, so a receive discovered during + /// catch-up says nothing about whether some higher, not-yet-scanned block + /// already spends it. Selecting it there builds a transaction the network + /// settled as a double-spend long ago; peers drop it silently, with no + /// reject message, and the funds it was meant to move are stranded. + /// + /// No-op when no scanner has reported a target, or when `tx` came from a + /// block at or above it — nothing above the tip can have spent it yet. + /// [`Self::promote_spend_scanned_utxos`] releases what this holds back. + pub(crate) fn hold_back_unscanned_utxos( + &mut self, + tx: &Transaction, + block_height: CoreBlockHeight, + ) { + let target = self.metadata.scan_target_height; + if target == 0 || block_height >= target { + return; + } + let txid = tx.txid(); + for account in self.accounts.all_funding_accounts_mut() { + for vout in 0..tx.output.len() as u32 { + if let Some(utxo) = account.utxos.get_mut(&OutPoint { + txid, + vout, + }) { + utxo.spend_scanned = false; + } + } + } + } + + /// Release every UTXO held back by [`Self::hold_back_unscanned_utxos`], + /// once the spend scan reaches the height it was working toward. + /// + /// At that point every block up to the target has been matched against the + /// wallet's scripts and every matching block body applied, so a spend of a + /// tracked UTXO would already have removed it. Whatever is still in the set + /// is genuinely unspent as of the target. + /// + /// No-op while the scan is still behind, or when no scanner has reported a + /// target (in which case nothing was ever held back). + pub(crate) fn promote_spend_scanned_utxos(&mut self) { + if self.metadata.scan_target_height == 0 + || self.metadata.synced_height < self.metadata.scan_target_height + { + return; + } + for account in self.accounts.all_funding_accounts_mut() { + for utxo in account.utxos.values_mut() { + utxo.spend_scanned = true; + } + } + } + /// Invalidate the wallet's sync certificate when an account is added. /// /// `synced_height` certifies "every filter at or below this height was diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs index 50aa36277..a88be0694 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs @@ -805,6 +805,7 @@ mod tests { is_instantlocked: false, is_locked: false, is_trusted: false, + spend_scanned: true, }; info.accounts .standard_bip44_accounts @@ -988,6 +989,7 @@ mod tests { is_instantlocked: false, is_locked: false, is_trusted: false, + spend_scanned: true, }, ); outpoint 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 db795d200..a563748f0 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 @@ -182,6 +182,26 @@ pub trait WalletInfoInterface: Sized + WalletTransactionChecker + ManagedAccount /// Return the durable wallet sync checkpoint height. fn synced_height(&self) -> CoreBlockHeight; + /// Return the chain height the spend scan is working toward, or `0` if no + /// scanner has reported one. See + /// [`WalletMetadata::scan_target_height`](crate::wallet::metadata::WalletMetadata::scan_target_height). + fn scan_target_height(&self) -> CoreBlockHeight { + 0 + } + + /// Record the chain height the spend scan is working toward. Called by the + /// scanner as the filter-header chain grows. + fn update_scan_target_height(&mut self, _height: CoreBlockHeight) {} + + /// Whether the spend scan has covered every block up to the height it is + /// working toward — i.e. whether the wallet's UTXO set is net of all + /// on-chain spends rather than mid-catch-up. Always `true` when no + /// scanner has reported a target. + fn spend_scan_complete(&self) -> bool { + let target = self.scan_target_height(); + target == 0 || self.synced_height() >= target + } + /// Return the current generation of the wallet's account set — a counter /// bumped on every account addition. Filter-sync layers snapshot it at /// scan time and refuse to certify coverage at commit time when it has @@ -311,6 +331,17 @@ impl WalletInfoInterface for ManagedWalletInfo { self.metadata.synced_height } + fn scan_target_height(&self) -> CoreBlockHeight { + self.metadata.scan_target_height + } + + fn update_scan_target_height(&mut self, height: CoreBlockHeight) { + self.metadata.scan_target_height = height; + // A target the scan has already reached must not strand UTXOs that + // were held back under an older, higher target. + self.promote_spend_scanned_utxos(); + } + fn account_generation(&self) -> u64 { self.account_generation } @@ -512,6 +543,9 @@ impl WalletInfoInterface for ManagedWalletInfo { // 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(); + // ...and can complete the spend scan, releasing UTXOs held back + // during catch-up. + self.promote_spend_scanned_utxos(); } fn matured_coinbase_records( diff --git a/key-wallet/src/wallet/metadata.rs b/key-wallet/src/wallet/metadata.rs index b09edf9f3..78e13ad0a 100644 --- a/key-wallet/src/wallet/metadata.rs +++ b/key-wallet/src/wallet/metadata.rs @@ -19,6 +19,21 @@ pub struct WalletMetadata { pub last_processed_height: CoreBlockHeight, /// Sync checkpoint height pub synced_height: CoreBlockHeight, + /// The chain height the spend scan is working toward — the tip of the + /// filter-header chain the scanner has committed to covering. + /// + /// `synced_height` is where the scan has reached; this is where it is + /// going. While `synced_height < scan_target_height` the wallet is + /// catching up and cannot know whether a freshly discovered output has + /// already been spent in a block it has not scanned yet, so receives + /// applied in that window are held back from coin selection. See + /// [`crate::utxo::Utxo::spend_scanned`]. + /// + /// `0` means "unknown": no scanner has reported a target, and the + /// spend-scan gate stays open. A scanner must set this for the gate to + /// have any effect. + #[cfg_attr(feature = "serde", serde(default))] + pub scan_target_height: CoreBlockHeight, /// Highest chainlock that has been applied to this wallet, /// establishing the finality boundary: every block at or below /// `chain_lock.block_height` is final for this wallet. `None` until