From 022379bedb6eeae8412c6094e4c5c33df8221994 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Sun, 12 Jul 2026 14:54:05 -0400 Subject: [PATCH 1/3] test(dash-spv): port CoinJoin gap-limit discovery repros (#846) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port the dash-spv portion of the platform#3549 crate-level bug-pin repro onto the current `dev` architecture (post-#820: FiltersManager + BlockMatchTracker). Three tests drive the real filter -> block -> wallet pipeline with a deterministic seed, synthetic blocks and real BIP-158 filters, no network: - coinjoin_gap_limit_dense_same_batch_recovers — GREEN (#820 guard) - coinjoin_gap_limit_inversion_within_batch_recovers — GREEN (#820) - coinjoin_gap_limit_stall_across_committed_batch — RED: gap-window outputs in an already-committed batch are never recovered because the new-script rescan only reaches `active_batches`. Ported from dashpay/rust-dashcore@c24912c1 (repro/pr3549-rdc); the key-wallet #763/#764 repros in that commit are out of scope here. Co-Authored-By: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-Authored-By: Claude Fable 5 --- .../filters/coinjoin_gap_discovery_tests.rs | 377 ++++++++++++++++++ dash-spv/src/sync/filters/manager.rs | 4 + 2 files changed, 381 insertions(+) create mode 100644 dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs diff --git a/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs new file mode 100644 index 000000000..66c6186a9 --- /dev/null +++ b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs @@ -0,0 +1,377 @@ +//! CoinJoin gap-limit discovery repros against the real filter → block → +//! wallet pipeline (`FiltersManager` + `BlockMatchTracker` + a real +//! `WalletManager` with a CoinJoin account). +//! +//! Background: a wallet with dense CoinJoin usage (dozens of consecutive +//! external indices funded per block) historically stalled at +//! `highest_used = 59` (initial 30-address watch window + one gap-limit +//! extension) because a matched block was applied to a wallet exactly once — +//! the `BlockMatchTracker` gate was keyed by `(wallet, block)`, not +//! `(wallet, address)` — so outputs paying addresses derived *from that same +//! block's matches* were never re-tested. PR #820 fixed the in-flight part: +//! `rescan_batch` now re-queues already-processed blocks of ACTIVE batches +//! against newly derived scripts, to a fixpoint. +//! +//! These tests pin both the fixed behaviour and the remaining hole: +//! +//! 1. [`coinjoin_gap_limit_dense_same_batch_recovers`] — the empirical +//! stall-at-59 shape (two dense blocks in one batch). GREEN since #820; +//! kept as a regression guard. +//! 2. [`coinjoin_gap_limit_inversion_within_batch_recovers`] — a gap-window +//! output in an EARLIER block of the SAME (still-active) batch is +//! recovered by the commit-time rescan. GREEN since #820. +//! 3. [`coinjoin_gap_limit_stall_across_committed_batch`] — the SAME shape +//! with the earlier block in an already-COMMITTED batch is never +//! recovered: rescans only reach `active_batches`, and committed batches +//! are gone. RED — the residual `(wallet, block-progress)` vs +//! `(wallet, address)` keying defect. +//! +//! Each test drives the manager exactly the way the production event loop +//! does: `try_process_batch` → `BlocksNeeded` → (blocks-manager stand-in) +//! `WalletInterface::process_block_for_wallets` → `BlockProcessed` (with the +//! wallet's freshly derived scripts) → `handle_sync_event` → commit-time +//! rescan, iterated to quiescence. Deterministic: fixed mnemonic, synthetic +//! blocks, real BIP-158 filters, no network. + +use super::*; +use crate::storage::{ + DiskStorageManager, PersistentBlockHeaderStorage, PersistentFilterHeaderStorage, + PersistentFilterStorage, StorageManager, +}; +use dashcore::{ + Address, Block, BlockHash, Network, OutPoint, Transaction, TxIn, TxOut, Txid, Witness, +}; +use key_wallet::account::ManagedAccountTrait; +use key_wallet::gap_limit::DEFAULT_COINJOIN_GAP_LIMIT; +use key_wallet::managed_account::address_pool::{AddressPool, AddressPoolType, KeySource}; +use key_wallet::wallet::initialization::WalletAccountCreationOptions; +use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; +use key_wallet_manager::WalletManager; +use tokio::sync::mpsc::unbounded_channel; + +/// Deterministic test wallet seed (standard BIP-39 test vector mnemonic). +const TEST_MNEMONIC: &str = + "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + +/// CoinJoin-denomination-shaped output value (0.001 DASH + per-round fee). +/// The matcher is purely script-based; the value is for realism only. +const DENOMINATION: u64 = 100_001; + +type CjFiltersManager = FiltersManager< + PersistentBlockHeaderStorage, + PersistentFilterHeaderStorage, + PersistentFilterStorage, + WalletManager, +>; + +/// Real wallet manager with one seed wallet (Default accounts incl. CoinJoin +/// account 0, external pool pre-generated to `DEFAULT_COINJOIN_GAP_LIMIT`), +/// wired into a `FiltersManager` over temp-dir storage. +async fn setup() -> (CjFiltersManager, Arc>>, WalletId) { + let mut wm = WalletManager::::new(Network::Regtest); + let wallet_id = wm + .create_wallet_from_mnemonic(TEST_MNEMONIC, 0, WalletAccountCreationOptions::Default) + .expect("create deterministic test wallet"); + let wallet = Arc::new(RwLock::new(wm)); + + let storage = DiskStorageManager::with_temp_dir().await.unwrap(); + let mut manager = FiltersManager::new( + Arc::clone(&wallet), + storage.block_headers(), + storage.filter_headers(), + storage.filters(), + ) + .await; + manager.set_state(SyncState::Syncing); + (manager, wallet, wallet_id) +} + +/// Derive the wallet's first `count` CoinJoin-account-0 External addresses +/// OFFLINE (via a standalone pool over the same xpub + base path), so test +/// blocks can pay indices far beyond the wallet's live watch window without +/// touching the wallet's own state. +async fn coinjoin_external_addresses( + wallet: &Arc>>, + wallet_id: &WalletId, + count: u32, +) -> Vec
{ + let wm = wallet.read().await; + let signing_wallet = wm.get_wallet(wallet_id).expect("wallet present"); + let xpub = signing_wallet + .accounts + .coinjoin_accounts + .get(&0) + .expect("CoinJoin account 0 (Default options)") + .account_xpub; + let info = wm.get_wallet_info(wallet_id).expect("wallet info present"); + let live_pool = coinjoin_external_pool(info); + let mut pool = AddressPool::new( + live_pool.base_path.clone(), + AddressPoolType::External, + count, + Network::Regtest, + &KeySource::Public(xpub), + ) + .expect("derive standalone CoinJoin External pool"); + // `AddressPool::new` pre-generates `count` addresses; extend defensively + // in case the constructor semantics change. + if pool.highest_generated.map(|h| h + 1).unwrap_or(0) < count { + let missing = count - pool.highest_generated.map(|h| h + 1).unwrap_or(0); + pool.generate_addresses(missing, &KeySource::Public(xpub), true) + .expect("extend standalone pool"); + } + (0..count).map(|i| pool.address_at_index(i).expect("address generated")).collect() +} + +/// The wallet's LIVE CoinJoin-account-0 External pool. +fn coinjoin_external_pool(info: &ManagedWalletInfo) -> &AddressPool { + let account = + info.accounts.coinjoin_accounts.get(&0).expect("CoinJoin account 0 (Default options)"); + account + .managed_account_type() + .address_pools() + .into_iter() + .find(|p| p.pool_type == AddressPoolType::External) + .expect("CoinJoin External pool") +} + +/// `(highest_used, highest_generated, used_count)` of the live CoinJoin +/// External pool — the discovery frontier the assertions pin. +async fn coinjoin_pool_state( + wallet: &Arc>>, + wallet_id: &WalletId, +) -> (Option, Option, usize) { + let wm = wallet.read().await; + let info = wm.get_wallet_info(wallet_id).expect("wallet info present"); + let pool = coinjoin_external_pool(info); + (pool.highest_used, pool.highest_generated, pool.used_indices.len()) +} + +/// One block at `height` whose single transaction pays every address in +/// `addresses` one denomination output. Returns the block, its real BIP-158 +/// filter, and the filter match key. +fn block_paying(height: u32, addresses: &[Address]) -> (Block, BlockFilter, FilterMatchKey) { + let mut prev_txid = [0xABu8; 32]; + prev_txid[..4].copy_from_slice(&height.to_le_bytes()); + let tx = Transaction { + version: 1, + lock_time: 0, + // One non-null input so the tx is not coinbase-shaped. + input: vec![TxIn { + previous_output: OutPoint::new(Txid::from(prev_txid), 0), + script_sig: ScriptBuf::new(), + sequence: 0xffffffff, + witness: Witness::new(), + }], + output: addresses + .iter() + .map(|a| TxOut { + value: DENOMINATION, + script_pubkey: a.script_pubkey(), + }) + .collect(), + special_transaction_payload: None, + }; + let block = Block::dummy(height, vec![tx]); + let filter = BlockFilter::dummy(&block); + let key = FilterMatchKey::new(height, block.block_hash()); + (block, filter, key) +} + +/// Drive the production event loop to quiescence: consume `BlocksNeeded` by +/// processing the named blocks through the real wallet (the blocks-manager's +/// job), feed the resulting `BlockProcessed` events (carrying the wallet's +/// freshly derived scripts) back into the filters manager, and repeat until +/// no more blocks are requested. Blocks are processed in height order, like +/// the real `BlocksPipeline`. +async fn drive_to_quiescence( + manager: &mut CjFiltersManager, + wallet: &Arc>>, + blocks: &HashMap, + initial_events: Vec, +) { + let (tx, _rx) = unbounded_channel(); + let requests = RequestSender::new(tx); + + let mut events = initial_events; + for _round in 0..64 { + let mut pending: BTreeMap<(u32, BlockHash), BTreeSet> = BTreeMap::new(); + for event in events.drain(..) { + if let SyncEvent::BlocksNeeded { + blocks: needed, + } = event + { + for (key, wallets) in needed { + pending.entry((key.height(), *key.hash())).or_default().extend(wallets); + } + } + } + if pending.is_empty() { + return; + } + + let mut next_events = Vec::new(); + for ((height, block_hash), wallets) in pending { + let block = blocks.get(&block_hash).expect("BlocksNeeded for an unknown test block"); + let result = wallet + .write() + .await + .process_block_for_wallets(block, block_hash, height, &wallets) + .await; + let confirmed_txids = result.relevant_txids().cloned().collect(); + let event = SyncEvent::BlockProcessed { + block_hash, + height, + wallets, + new_scripts: result.new_scripts, + confirmed_txids, + }; + next_events.extend( + manager.handle_sync_event(&event, &requests).await.expect("BlockProcessed"), + ); + } + events = next_events; + } + panic!("drive_to_quiescence did not settle within 64 rounds — runaway rescan loop?"); +} + +/// The empirical stall-at-59 shape: block A funds CoinJoin External indices +/// 0..=51, the NEXT block funds 52..=139, both inside one active batch. +/// Before PR #820 the `(wallet, block)` gate stopped discovery at index 59 +/// (29 initial + one 30-wide gap extension); the commit-time fixpoint rescan +/// now climbs the whole dense range. Regression guard for #820. +#[tokio::test] +async fn coinjoin_gap_limit_dense_same_batch_recovers() { + let (mut manager, wallet, wallet_id) = setup().await; + let addresses = coinjoin_external_addresses(&wallet, &wallet_id, 140).await; + + let (highest_used, highest_generated, _) = coinjoin_pool_state(&wallet, &wallet_id).await; + assert_eq!(highest_used, None, "fresh wallet must have no used CoinJoin indices"); + assert_eq!( + highest_generated, + Some(DEFAULT_COINJOIN_GAP_LIMIT - 1), + "fresh wallet must watch exactly the initial CoinJoin gap window" + ); + + let (block_a, filter_a, key_a) = block_paying(10, &addresses[0..=51]); + let (block_b, filter_b, key_b) = block_paying(11, &addresses[52..=139]); + let blocks: HashMap = + HashMap::from([(block_a.block_hash(), block_a), (block_b.block_hash(), block_b)]); + + let mut batch = FiltersBatch::new(0, 99, HashMap::from([(key_a, filter_a), (key_b, filter_b)])); + batch.mark_verified(); + manager.active_batches.insert(0, batch); + manager.progress.update_stored_height(99); + + let initial_events = manager.try_process_batch().await.unwrap(); + drive_to_quiescence(&mut manager, &wallet, &blocks, initial_events).await; + + let (highest_used, _, used_count) = coinjoin_pool_state(&wallet, &wallet_id).await; + assert_eq!( + highest_used, + Some(139), + "dense same-batch CoinJoin range must be fully discovered via the commit-time \ + fixpoint rescan (PR #820); a stall at Some(59) means the (wallet, block) \ + once-per-block gate regressed. used_count={used_count}" + ); + assert_eq!(used_count, 140, "every funded index 0..=139 must be marked used"); +} + +/// Height inversion INSIDE one active batch: indices 40..=51 are funded at +/// height 10, the in-window indices 0..=29 only at height 20. The initial +/// scan cannot see block A (nothing watched matches it), but once block B's +/// processing derives 30..=59 the commit-time rescan re-tests block A's +/// filter and recovers it. GREEN since #820 — contrast for the RED +/// cross-batch test below, isolating the commit boundary as the broken seam. +#[tokio::test] +async fn coinjoin_gap_limit_inversion_within_batch_recovers() { + let (mut manager, wallet, wallet_id) = setup().await; + let addresses = coinjoin_external_addresses(&wallet, &wallet_id, 60).await; + + let (block_a, filter_a, key_a) = block_paying(10, &addresses[40..=51]); + let (block_b, filter_b, key_b) = block_paying(20, &addresses[0..=29]); + let blocks: HashMap = + HashMap::from([(block_a.block_hash(), block_a), (block_b.block_hash(), block_b)]); + + let mut batch = FiltersBatch::new(0, 99, HashMap::from([(key_a, filter_a), (key_b, filter_b)])); + batch.mark_verified(); + manager.active_batches.insert(0, batch); + manager.progress.update_stored_height(99); + + let initial_events = manager.try_process_batch().await.unwrap(); + drive_to_quiescence(&mut manager, &wallet, &blocks, initial_events).await; + + let (highest_used, _, used_count) = coinjoin_pool_state(&wallet, &wallet_id).await; + assert_eq!( + highest_used, + Some(51), + "a gap-window output in an earlier block of the SAME active batch must be \ + recovered by the commit-time rescan (PR #820). used_count={used_count}" + ); +} + +/// (RED-by-design): gap-window outputs in an already-COMMITTED batch are +/// never recovered — the new-script rescan is keyed by batch/commit progress +/// (`(wallet, block)` lineage), not `(wallet, address)`. +/// +/// Same funding shape as the within-batch inversion test, but the early +/// block (indices 40..=51, height 10) sits in batch 0..=99 while the +/// in-window block (indices 0..=29) sits at height 110 in batch 100..=199. +/// Batch 0 scans clean (nothing watched matches) and commits. Processing the +/// height-110 block derives 30..=59, and those scripts DO match block 10's +/// filter — but `rescan_batch` only reaches `active_batches`, and committed +/// batches are gone (`try_commit_batches` removes them; the tracker prunes +/// at-or-below the committed height). Indices 40..=51 — squarely inside the +/// BIP-44/CoinJoin gap-limit recovery contract (40 < 29 + 1 + 30) — stay +/// invisible forever, along with their funds. A fresh re-sync from genesis +/// hits the same wall, so the funds are unrecoverable without a manual +/// pre-derivation workaround. +/// +/// GREEN once newly derived scripts can re-open committed ranges (track +/// processed SCRIPTS per block / trigger a below-committed-height rescan for +/// the owning wallet), at which point `highest_used` reaches 51. +#[tokio::test] +async fn coinjoin_gap_limit_stall_across_committed_batch() { + let (mut manager, wallet, wallet_id) = setup().await; + let addresses = coinjoin_external_addresses(&wallet, &wallet_id, 60).await; + + let (block_a, filter_a, key_a) = block_paying(10, &addresses[40..=51]); + let (block_b, filter_b, key_b) = block_paying(110, &addresses[0..=29]); + let blocks: HashMap = + HashMap::from([(block_a.block_hash(), block_a), (block_b.block_hash(), block_b)]); + + let mut batch_0 = FiltersBatch::new(0, 99, HashMap::from([(key_a, filter_a)])); + batch_0.mark_verified(); + manager.active_batches.insert(0, batch_0); + let mut batch_1 = FiltersBatch::new(100, 199, HashMap::from([(key_b, filter_b)])); + batch_1.mark_verified(); + manager.active_batches.insert(100, batch_1); + manager.progress.update_stored_height(199); + + let initial_events = manager.try_process_batch().await.unwrap(); + drive_to_quiescence(&mut manager, &wallet, &blocks, initial_events).await; + + let (highest_used, highest_generated, used_count) = + coinjoin_pool_state(&wallet, &wallet_id).await; + // Sanity: the in-window block was found and the gap window extended past + // index 51, so the missed indices ARE inside the watched range by now. + assert!( + highest_generated >= Some(51), + "gap maintenance must have extended the watch window past index 51 \ + (got {highest_generated:?})" + ); + assert_eq!( + highest_used, + Some(51), + "CoinJoin External indices 40..=51 were funded at height 10 in a batch that \ + committed before their scripts were derived, and the new-script rescan never \ + looks below the committed boundary (rescan_batch only reaches active_batches; \ + BlockMatchTracker/commit pruning drops the range). The addresses are within \ + the gap-limit recovery contract and are watched now (highest_generated = \ + {highest_generated:?}), yet their outputs stay invisible: highest_used stalls \ + at {highest_used:?}, used_count={used_count}. Fix direction: key re-scan \ + suppression by (wallet, address/script) instead of block/commit progress, or \ + trigger a below-committed-height rescan for a wallet whose gap maintenance \ + derives scripts mid-sync." + ); +} diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index 58192af13..40647cbed 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -988,6 +988,10 @@ impl Date: Sun, 12 Jul 2026 15:10:44 -0400 Subject: [PATCH 2/3] fix(dash-spv): re-open committed ranges for gap-limit scripts (#846) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gap-limit scripts derived after a filter batch commits were never tested against the committed range, so outputs paying gap-window addresses in an already-committed block stayed permanently invisible. PR #820 fixed the in-flight (active-batch) half via the commit-time fixpoint rescan; this closes the cross-commit-boundary half. Root cause: discovery suppression was keyed by scan progress (which heights a wallet had committed), not by which scripts a block was tested against. `rescan_batch` only reaches `active_batches`, and commit removes the batch and prunes the tracker at/below the committed height, so a forward index<->height inversion — a low external index funded in a later block whose gap-window extension covers a higher index paid in an earlier, already-committed block — left the earlier block's outputs unrecoverable. Fix: retain committed batches' filters in a bounded sliding window (`committed_filters`, moved out of the batch on commit — no clone, so the common path is untouched). On every `BlockProcessed` that derives new scripts, queue them with the committed_height snapshot at derivation time; `try_process_batch` drains the queue, re-tests the fresh scripts against the retained committed filters at/below that ceiling, and re-queues any matched committed block through the existing `track_for_new_scripts` path to a fixpoint. Per-wallet `committed_tested_scripts` dedups so each script pays one pass; heights that commit later already saw the script via the normal per-batch scan. `FiltersSyncComplete` is held off while a re-opened below-frontier block is in flight, and the retained filters are released on full sync. Trade-offs: - Zero overhead on the common no-new-scripts path (queue empty, early return). Extra work is bounded to the CoinJoin-style derive-mid-sync path and to the retained window (`MAX_RETAINED_COMMITTED_FILTERS`). - Reach/memory are the same knob: inversions spanning more than the window are not recovered by the in-memory path; a birth-height rescan remains the backstop. - In-memory only. A wallet that syncs (or re-syncs) with this code self-heals within the pass; a wallet that already completed its sync and persisted the stall before this fix is not retroactively healed and still needs a rescan from birth. Makes `coinjoin_gap_limit_stall_across_committed_batch` green while keeping the two #820 guards green. Co-Authored-By: Claude Fable 5 --- dash-spv/src/sync/filters/batch.rs | 6 + .../src/sync/filters/block_match_tracker.rs | 8 + dash-spv/src/sync/filters/manager.rs | 266 +++++++++++++++++- dash-spv/src/sync/filters/sync_manager.rs | 12 +- 4 files changed, 289 insertions(+), 3 deletions(-) diff --git a/dash-spv/src/sync/filters/batch.rs b/dash-spv/src/sync/filters/batch.rs index 386e0d16e..9249c2934 100644 --- a/dash-spv/src/sync/filters/batch.rs +++ b/dash-spv/src/sync/filters/batch.rs @@ -69,6 +69,12 @@ impl FiltersBatch { pub(super) fn filters_mut(&mut self) -> &mut HashMap { &mut self.filters } + /// Take the loaded filters out of this batch, leaving it empty. Used at + /// commit to move a committed batch's filters into the manager's retained + /// committed-filter set without cloning. + pub(super) fn take_filters(&mut self) -> HashMap { + std::mem::take(&mut self.filters) + } /// Returns whether this batch is verified (filters verified against their headers). pub(super) fn verified(&self) -> bool { self.verified diff --git a/dash-spv/src/sync/filters/block_match_tracker.rs b/dash-spv/src/sync/filters/block_match_tracker.rs index e0d911b47..698956201 100644 --- a/dash-spv/src/sync/filters/block_match_tracker.rs +++ b/dash-spv/src/sync/filters/block_match_tracker.rs @@ -153,6 +153,14 @@ impl BlockMatchTracker { self.blocks_remaining.is_empty() && self.processed_blocks_per_wallet.is_empty() } + /// True while any matched block is still awaiting its `BlockProcessed`. + /// Used to hold off `FiltersSyncComplete` while a committed-range re-open + /// (issue #846) has a below-frontier block in flight that no active batch + /// accounts for. + pub(super) fn has_in_flight(&self) -> bool { + !self.blocks_remaining.is_empty() + } + /// Drop all in-flight and processed-record state. pub(super) fn clear(&mut self) { self.blocks_remaining.clear(); diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index 40647cbed..528c09dc8 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -8,7 +8,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::sync::Arc; use dashcore::bip158::{BlockFilter, FilterQuery}; -use dashcore::ScriptBuf; +use dashcore::{BlockHash, ScriptBuf}; use super::batch::FiltersBatch; use super::block_match_tracker::{BlockMatchTracker, BlockTrackResult}; @@ -45,6 +45,34 @@ struct WalletScanState { /// Maximum number of batches to scan ahead while waiting for blocks. const MAX_LOOKAHEAD_BATCHES: usize = 3; +/// Sliding window of filters retained from committed batches for the #846 +/// committed-range re-test, capping both retained memory and how far back the +/// re-test reaches. Committed filters are *moved* out of the batch on commit +/// (no clone), so the common no-new-scripts path is untouched; this bounds only +/// the CoinJoin-style path where gap-limit maintenance derives scripts +/// mid-sync. When exceeded, the lowest committed heights are evicted first, so +/// an index↔height inversion spanning more than this many blocks is not +/// recovered by the in-memory path (a birth-height rescan remains the backstop; +/// see `drain_committed_rescans`). At ~5000 heights/batch this keeps roughly the +/// last four committed batches — far beyond the local span of a CoinJoin mixing +/// session — while keeping peak retention modest for mobile hosts. +const MAX_RETAINED_COMMITTED_FILTERS: usize = 20_000; + +/// A deferred re-test of newly derived scripts against the already-committed +/// filter range on disk. See [`FiltersManager::drain_committed_rescans`]. +/// +/// `ceiling` is the manager's `committed_height` captured at the moment the +/// scripts were derived. Heights above the ceiling were still in active +/// batches at derivation time and were tested against these scripts by the +/// normal per-batch scan, so only the range at or below the ceiling needs +/// re-opening. +struct PendingCommittedRescan { + /// Highest committed height at the time these scripts were derived. + ceiling: u32, + /// Newly derived scriptPubKeys, attributed per owning wallet. + scripts: HashMap>, +} + /// Filters manager for downloading and matching compact block filters. /// /// Generic over: @@ -84,6 +112,25 @@ pub struct FiltersManager< /// `BlockProcessed` and the per-wallet record of which wallets already /// have a given processed block applied. pub(super) tracker: BlockMatchTracker, + /// Newly derived scripts awaiting a re-test against the already-committed + /// filter range on disk (issue #846). Populated on every `BlockProcessed` + /// that carried gap-limit-derived scripts and drained to a fixpoint by + /// `drain_committed_rescans`. In-memory only; see the restart caveat in + /// that method's docs. + pending_committed_rescans: Vec, + /// Per-wallet scripts already tested against a committed range, so the same + /// script is never re-matched against committed filters more than once. + /// Bounded by the wallet's derived key count, not by block count. Correct + /// because a committed range is always scanned against every script that + /// existed when the range committed: a script only needs its one pass over + /// the range that was committed *before* it was derived. + committed_tested_scripts: HashMap>, + /// Filters of committed batches, keyed by height, retained for the #846 + /// committed-range re-test. Filters are moved here on commit rather than + /// dropped, so newly derived scripts can be tested against ranges the + /// wallet already advanced past. Capped at `MAX_RETAINED_COMMITTED_FILTERS` + /// (evicting the lowest heights) and cleared on full sync / rescan reset. + committed_filters: BTreeMap, } impl @@ -128,6 +175,9 @@ impl= self.progress.filter_header_tip_height() && self.progress.committed_height() >= self.progress.target_height() @@ -463,6 +534,13 @@ impl self.progress.committed_height() { self.progress.update_committed_height(end); @@ -541,6 +619,19 @@ impl MAX_RETAINED_COMMITTED_FILTERS { + let Some((&lowest, _)) = self.committed_filters.iter().next() else { + break; + }; + self.committed_filters.remove(&lowest); + } // Drop processed-wallet records for the committed range. Below the // new committed_height a new wallet can only get here via the // `tick` rescan trigger, which already wipes the map via @@ -738,6 +829,177 @@ impl>, + ) { + let scripts: HashMap> = new_scripts + .iter() + .filter(|(_, s)| !s.is_empty()) + .map(|(id, s)| (*id, s.iter().cloned().collect())) + .collect(); + if scripts.is_empty() { + return; + } + self.pending_committed_rescans.push(PendingCommittedRescan { + ceiling: self.progress.committed_height(), + scripts, + }); + } + + /// Re-test queued newly derived scripts against the retained committed + /// filter range, re-opening any committed block whose outputs pay a script + /// that did not exist when that block was first scanned (issue #846). + /// + /// Root cause this closes: discovery suppression was keyed by scan progress + /// (which heights a wallet already committed), not by which scripts a block + /// was tested against. A forward index↔height inversion — a low external + /// index funded in a later block, whose gap-limit window extension covers a + /// higher index paid in an earlier, already-committed block — left the + /// earlier block's outputs permanently invisible: `rescan_batch` only + /// reaches `active_batches`, and commit removed the batch and pruned the + /// tracker at or below the committed height. Committed filters are now + /// retained (`committed_filters`) so this re-test has something to match. + /// + /// Cost model: zero work in the common case (queue empty). When scripts are + /// queued, each is matched against the retained committed range at most once + /// (`committed_tested_scripts` dedup); a range that commits later is already + /// scanned against these scripts by `scan_batch`, so no re-test is owed for + /// it. Matching walks in-memory retained filters (bounded by + /// `MAX_RETAINED_COMMITTED_FILTERS`), no disk I/O. + /// + /// Restart caveat: the queue, `committed_tested_scripts`, and the retained + /// `committed_filters` are all in-memory only. The re-open is driven by a + /// `BlockProcessed` deriving new scripts, which happens whenever a funding + /// block is (re)processed during a sync pass — so a wallet that syncs (or + /// re-syncs) with this code present self-heals within that pass. A wallet + /// that had already completed its sync and persisted the stall *before* this + /// fix does not re-derive scripts on restart (nothing below its persisted + /// `synced_height` is reprocessed) and is not retroactively healed; such a + /// wallet still needs a rescan from its birth height. New syncs never enter + /// the stalled state. + async fn drain_committed_rescans(&mut self) -> SyncResult> { + if self.pending_committed_rescans.is_empty() || self.committed_filters.is_empty() { + // Nothing queued, or nothing committed yet to re-test against. Drop + // the queue either way: with no committed range there is no owed + // re-test (every height was still active when these scripts were + // derived and saw them via the normal per-batch scan). + self.pending_committed_rescans.clear(); + return Ok(vec![]); + } + let pending = std::mem::take(&mut self.pending_committed_rescans); + + let mut events = Vec::new(); + for rescan in pending { + let ceiling = rescan.ceiling; + + // Drop scripts already tested against a committed range; keep only + // the fresh ones and mark them tested. + let mut fresh_queries: Vec<(WalletId, FilterQuery)> = Vec::new(); + for (wallet_id, scripts) in rescan.scripts { + let tested = self.committed_tested_scripts.entry(wallet_id).or_default(); + let fresh: Vec = + scripts.into_iter().filter(|s| tested.insert(s.clone())).collect(); + if fresh.is_empty() { + continue; + } + // Include the wallet's bare owner/voting filter elements too, + // exactly as `scan_batch` builds its per-wallet query. + let elements = self.wallet.read().await.monitored_filter_elements_for(&wallet_id); + let mut query: FilterQuery = fresh.iter().map(|s| s.as_bytes()).collect(); + for element in &elements { + query.push(element); + } + fresh_queries.push((wallet_id, query)); + } + if fresh_queries.is_empty() { + continue; + } + + // Match the fresh queries against every retained committed filter at + // or below the derivation-time ceiling. Collect first, then touch + // the tracker, so the immutable borrow of `committed_filters` ends + // before the mutable tracker calls. + let mut block_to_wallets: BTreeMap> = + BTreeMap::new(); + for (&height, (hash, filter)) in self.committed_filters.range(..=ceiling) { + let key = FilterMatchKey::new(height, *hash); + for (wallet_id, query) in &fresh_queries { + match filter.match_any(key.hash(), query) { + Ok(true) => { + block_to_wallets.entry(key.clone()).or_default().insert(*wallet_id); + } + Ok(false) => {} + Err(e) => { + tracing::warn!( + "committed-range rescan match_any error at height {}: {}; treating as non-match", + height, + e + ); + } + } + } + } + + if block_to_wallets.is_empty() { + continue; + } + + let mut blocks_needed: BTreeMap> = BTreeMap::new(); + let mut matched_count = 0u32; + for (key, wallets) in block_to_wallets { + // Below the committed frontier there is no owning active batch, + // so no `pending_blocks` counter to touch. Attribution uses the + // block's own height, which is <= committed_height < every active + // batch start, so a later `BlockProcessed` for the re-opened + // block cannot decrement a live batch. Matches are driven by + // scripts absent when the block was first processed, so + // `track_for_new_scripts` always re-queues past the + // processed-record gate. + let attributed_start = key.height(); + match self.tracker.track_for_new_scripts(&key, attributed_start, wallets) { + BlockTrackResult::NewlyTracked { + wallets, + } => { + blocks_needed.insert(key, wallets); + matched_count += 1; + } + BlockTrackResult::InFlight { + wallets, + } => { + blocks_needed.insert(key, wallets); + } + // Never returned by track_for_new_scripts. + BlockTrackResult::AlreadyProcessed => {} + } + } + + if matched_count > 0 { + self.progress.add_matched(matched_count); + tracing::info!( + "Committed-range rescan re-opened {} block(s) at/below height {} for newly derived scripts (#846)", + matched_count, + ceiling, + ); + } + if !blocks_needed.is_empty() { + events.push(SyncEvent::BlocksNeeded { + blocks: blocks_needed, + }); + } + } + + Ok(events) + } + /// Scan a specific batch, matching its filters against each behind-wallet's /// addresses individually so already-synced wallets are not redundantly /// rescanned. diff --git a/dash-spv/src/sync/filters/sync_manager.rs b/dash-spv/src/sync/filters/sync_manager.rs index ecce171fa..879937713 100644 --- a/dash-spv/src/sync/filters/sync_manager.rs +++ b/dash-spv/src/sync/filters/sync_manager.rs @@ -176,7 +176,9 @@ impl< ); } - // Collect per-wallet new scripts for deferred rescan at commit time. + // Collect per-wallet new scripts for the active-batch + // deferred rescan at commit time. A re-opened committed + // block has no owning active batch, so this no-ops for it. for (wallet_id, scripts) in new_scripts { if scripts.is_empty() { continue; @@ -186,6 +188,14 @@ impl< } } + // Also queue the derived scripts for a re-test against the + // already-committed filter range. This is what re-opens a + // committed batch when gap-limit maintenance derives the + // matching script only after that batch committed (#846), + // and it fires for re-opened committed blocks too, driving + // the cascade to a fixpoint. + self.enqueue_committed_rescan(new_scripts); + return self.try_process_batch().await; } } From 18351f2ad27b7799c8e4728b07c5312c904e4bc9 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:24:23 -0400 Subject: [PATCH 3/3] =?UTF-8?q?docs(dash-spv):=20update=20gap-discovery=20?= =?UTF-8?q?test=20narrative=20=E2=80=94=20committed-batch=20case=20is=20no?= =?UTF-8?q?w=20a=20GREEN=20#846=20regression=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review note on #873: the module header, doc comment, and assertion message still described the cross-commit stall as an open RED defect; they now document the recovered behavior and read as a regression guard. Co-Authored-By: Claude Fable 5 --- .../filters/coinjoin_gap_discovery_tests.rs | 54 +++++++++---------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs index 66c6186a9..b7e870229 100644 --- a/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs +++ b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs @@ -12,19 +12,20 @@ //! `rescan_batch` now re-queues already-processed blocks of ACTIVE batches //! against newly derived scripts, to a fixpoint. //! -//! These tests pin both the fixed behaviour and the remaining hole: +//! These tests pin all three recovery shapes as regression guards: //! //! 1. [`coinjoin_gap_limit_dense_same_batch_recovers`] — the empirical -//! stall-at-59 shape (two dense blocks in one batch). GREEN since #820; -//! kept as a regression guard. +//! stall-at-59 shape (two dense blocks in one batch). GREEN since #820. //! 2. [`coinjoin_gap_limit_inversion_within_batch_recovers`] — a gap-window //! output in an EARLIER block of the SAME (still-active) batch is //! recovered by the commit-time rescan. GREEN since #820. //! 3. [`coinjoin_gap_limit_stall_across_committed_batch`] — the SAME shape -//! with the earlier block in an already-COMMITTED batch is never -//! recovered: rescans only reach `active_batches`, and committed batches -//! are gone. RED — the residual `(wallet, block-progress)` vs -//! `(wallet, address)` keying defect. +//! with the earlier block in an already-COMMITTED batch. Formerly RED +//! (the residual `(wallet, block-progress)` vs `(wallet, script)` keying +//! defect, #846): rescans only reached `active_batches`, so committed +//! ranges were permanently out of reach. GREEN since the #846 fix — +//! commit-time filter retention + `drain_committed_rescans` give newly +//! derived scripts backward reach across the commit boundary. //! //! Each test drives the manager exactly the way the production event loop //! does: `try_process_batch` → `BlocksNeeded` → (blocks-manager stand-in) @@ -310,26 +311,24 @@ async fn coinjoin_gap_limit_inversion_within_batch_recovers() { ); } -/// (RED-by-design): gap-window outputs in an already-COMMITTED batch are -/// never recovered — the new-script rescan is keyed by batch/commit progress -/// (`(wallet, block)` lineage), not `(wallet, address)`. +/// Regression guard for #846: gap-window outputs in an already-COMMITTED +/// batch are recovered by the committed-range re-scan. /// /// Same funding shape as the within-batch inversion test, but the early /// block (indices 40..=51, height 10) sits in batch 0..=99 while the /// in-window block (indices 0..=29) sits at height 110 in batch 100..=199. /// Batch 0 scans clean (nothing watched matches) and commits. Processing the /// height-110 block derives 30..=59, and those scripts DO match block 10's -/// filter — but `rescan_batch` only reaches `active_batches`, and committed -/// batches are gone (`try_commit_batches` removes them; the tracker prunes -/// at-or-below the committed height). Indices 40..=51 — squarely inside the -/// BIP-44/CoinJoin gap-limit recovery contract (40 < 29 + 1 + 30) — stay -/// invisible forever, along with their funds. A fresh re-sync from genesis -/// hits the same wall, so the funds are unrecoverable without a manual -/// pre-derivation workaround. +/// filter. Pre-fix this stalled forever: `rescan_batch` only reached +/// `active_batches`, and committed batches were gone (`try_commit_batches` +/// removed them; the tracker pruned at-or-below the committed height), so +/// indices 40..=51 — squarely inside the BIP-44/CoinJoin gap-limit recovery +/// contract (40 < 29 + 1 + 30) — stayed invisible along with their funds, +/// deterministically, even on a fresh re-sync from genesis. /// -/// GREEN once newly derived scripts can re-open committed ranges (track -/// processed SCRIPTS per block / trigger a below-committed-height rescan for -/// the owning wallet), at which point `highest_used` reaches 51. +/// Now GREEN: commit-time filter retention + `drain_committed_rescans` +/// re-tests newly derived scripts against the retained committed range and +/// re-queues matched blocks, so `highest_used` reaches 51. #[tokio::test] async fn coinjoin_gap_limit_stall_across_committed_batch() { let (mut manager, wallet, wallet_id) = setup().await; @@ -364,14 +363,11 @@ async fn coinjoin_gap_limit_stall_across_committed_batch() { highest_used, Some(51), "CoinJoin External indices 40..=51 were funded at height 10 in a batch that \ - committed before their scripts were derived, and the new-script rescan never \ - looks below the committed boundary (rescan_batch only reaches active_batches; \ - BlockMatchTracker/commit pruning drops the range). The addresses are within \ - the gap-limit recovery contract and are watched now (highest_generated = \ - {highest_generated:?}), yet their outputs stay invisible: highest_used stalls \ - at {highest_used:?}, used_count={used_count}. Fix direction: key re-scan \ - suppression by (wallet, address/script) instead of block/commit progress, or \ - trigger a below-committed-height rescan for a wallet whose gap maintenance \ - derives scripts mid-sync." + committed before their scripts were derived; the #846 committed-range re-scan \ + (retained committed filters + drain_committed_rescans) must recover them. \ + A stall here (highest_used={highest_used:?}, used_count={used_count}, \ + highest_generated={highest_generated:?}) means the re-scan is again keyed by \ + block/commit progress instead of (wallet, script) and the pre-fix silent \ + fund-loss defect has regressed." ); }