From 079e69b32f18d6313d7a0bf3a6fe59f7c5cc593e Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 4 Sep 2026 18:35:07 +0300 Subject: [PATCH 1/5] fix(dash-spv): make backward coverage durable by rewinding synced_height instead of sweeping in memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #866/#974 the filter sync covers scripts derived after a range committed by rescanning the committed range at the forward drain (`rescan_committed_range`). On a mixing-heavy wallet (~13k CoinJoin scripts derived during the scan) that is a single multi-minute pass over ~2.3M filters that matches tens of thousands of blocks, with no persisted progress: the sweep's block requests are charged to the tail batch's commit gate, and an iOS suspension a few seconds after the client reports "synced" drops the whole sweep. The user sees a synced wallet with the newly derived scripts' transactions missing until a manual rescan. On a relaunch with those blocks already in storage the same pass drains the matched blocks through the `SyncEvent` broadcast channel faster than the monitor consumes them, the monitor hits `Lagged` and the client shuts down. Replace the in-memory sweep with a durable re-walk: - `WalletInterface::rewind_wallet_synced_height(wallet_id, height)` — a new hook that lowers one wallet's committed sync checkpoint. It emits the same `SyncHeightAdvanced` persistence event an advance emits, so the persisters store the lowered height verbatim and the rewind survives a restart. Only lowers; a value at or above the current is ignored. Default no-op for implementations that predate backward coverage; implemented for `WalletManager` and the mock wallet. - `FilterSyncManager`: at the forward drain, when there are scripts that were derived after their range committed, rewind the affected wallets to `earliest_required_height - 1` instead of sweeping. The existing wallet-behind path ("Wallet synced_height fell below committed_height, restarting scan") then re-walks committed history in the normal 5,000-height batches, each persisting its own progress. Commit-time advance skips a wallet rewound at the same drain so the rewind is not clobbered by the batch's own `SyncHeightAdvanced`. - `rescan_committed_range` is kept (now unused) with progress logging and a `yield_now` per batch; it can be removed once the re-walk has soaked. Two sweep-shaped tests in `coinjoin_gap_discovery_tests` are `#[ignore]`d: their harness drives the filter manager directly and never runs the wallet-behind tick that now does the work. Cost: the re-walk starts at the wallet's birth height and re-delivers already-known transactions through the persistence channel, so it is slower than the targeted sweep (about +7 minutes on a 6.7k-transaction wallet from a fresh restore in the simulator). Rewinding to the lowest matched height and persisting only deltas are follow-ups. Verified with the same wallet: fresh restore, relaunch on an existing store, and a process kill mid re-walk with relaunch — every run reached the tip with the persisted store matching the chain, no `Lagged`. --- .../filters/coinjoin_gap_discovery_tests.rs | 9 ++ dash-spv/src/sync/filters/manager.rs | 101 ++++++++++++++++-- key-wallet-manager/src/process_block.rs | 19 ++++ .../src/test_utils/mock_wallet.rs | 12 +++ key-wallet-manager/src/wallet_interface.rs | 10 ++ 5 files changed, 143 insertions(+), 8 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 bf064fd97..e84261ac2 100644 --- a/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs +++ b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs @@ -345,6 +345,11 @@ async fn coinjoin_gap_limit_inversion_within_batch_recovers() { /// flow through the `track_for_new_scripts` re-download path to the same /// commit-time fixpoint. `highest_used` reaches G+21. #[tokio::test] +#[ignore = "LOCAL DIAGNOSTIC BUILD: backward coverage now happens through a \ +synced_height rewind picked up by the sync-manager tick (durable re-walk), \ +not the in-manager sweep this harness can observe — the tick never runs \ +here, so recovery cannot complete inside this test. Field-verified instead; \ +un-ignore when reverting to the sweep."] 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, (G + 22) as u32).await; @@ -436,6 +441,10 @@ async fn coinjoin_gap_limit_stall_across_committed_batch() { /// applied. `committed_range_sweeps` counts sweeps that reach the chunk walk /// in `rescan_committed_range`. #[tokio::test] +#[ignore = "LOCAL DIAGNOSTIC BUILD: the tail batch now commits before the \ +sweep's blocks drain (see try_commit_batches), so the 'sweep completes before \ +FiltersSyncComplete' invariant this test asserts is deliberately relaxed on \ +this branch. Upstream keeps the invariant; un-ignore when reverting."] async fn committed_range_sweep_coalesces_across_batch_commits() { let (mut manager, wallet, wallet_id) = setup().await; let addresses = coinjoin_external_addresses(&wallet, &wallet_id, (G + 22) as u32).await; diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index 9e7834e48..bcd8d26e8 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -594,6 +594,14 @@ impl SyncResult> { let mut events = Vec::new(); + // Wallets whose synced_height was rewound this pass for backward + // coverage (see the forward-drained branch). Their commit-time + // advance below is skipped: advancing them to the batch end would + // both undo the in-memory rewind and race the persisted checkpoint + // back up to tip, losing the re-walk across a restart. + let mut rewound_wallets: std::collections::HashSet = + std::collections::HashSet::new(); + // Lowest height any scan can ever reach. Read once, before the wallet // write lock below, so header storage is never locked underneath it. let scan_floor = self.header_storage.read().await.get_start_height().await.unwrap_or(0); @@ -670,16 +678,56 @@ impl 0 { - // Found more blocks, can't commit yet - break; + // LOCAL BUILD: durable backward coverage via a persisted + // checkpoint rewind, replacing the in-memory monolithic + // sweep (`rescan_committed_range`). + // + // The sweep held three losing properties on a large + // restored wallet: it ran minutes of silent compute + // inside this task, it charged tens of thousands of + // BIP-158 false-positive downloads to this batch's + // commit gate, and every bit of it lived in memory — an + // iOS suspension eight seconds after "synced" was + // observed to drop 28,616 queued blocks irrecoverably. + // + // The scripts are already in the wallet's watch set + // (deriving them is what put them in `backward_scripts`), + // so backward coverage is exactly "re-walk committed + // history with the enlarged set". The existing + // wallet-behind restart does that durably: rewinding the + // wallet's synced_height makes the next tick restart the + // batch scan, which re-commits (and re-persists) progress + // every BATCH_PROCESSING_SIZE filters. The persisted + // checkpoint applies verbatim on the app side, so a + // suspension mid-walk resumes from the last committed + // batch instead of losing the debt. Already-stored blocks + // are served from storage on the re-walk, so completed + // work is not re-downloaded. + let script_count: usize = + backward_scripts.values().map(|scripts| scripts.len()).sum(); + // One floor for every wallet: the earliest height any of + // them requires. The restart tick clamps its actual + // resume point to the birth/storage floor anyway, so a + // conservative target here only ever means "re-walk from + // the beginning of what is locally scannable". + let wallet_base = self.wallet.read().await.earliest_required_height().await; + let target = wallet_base.saturating_sub(1); + let mut wallet = self.wallet.write().await; + for wallet_id in backward_scripts.keys() { + if target < wallet.wallet_synced_height(wallet_id) { + wallet.rewind_wallet_synced_height(wallet_id, target); + rewound_wallets.insert(*wallet_id); } } + drop(wallet); + if !rewound_wallets.is_empty() { + tracing::info!( + "Backward coverage: {} new script(s) across {} wallet(s) — rewound their synced_height for a durable re-walk of committed history", + script_count, + rewound_wallets.len() + ); + } } // Mark rescan as complete if let Some(batch) = self.active_batches.get_mut(&batch_start) { @@ -729,6 +777,15 @@ impl= batch_start { wallet.update_wallet_synced_height(wallet_id, end); // A committed batch certifies the whole range for @@ -1182,6 +1239,11 @@ impl WalletInterface for WalletM } } + fn rewind_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() { + info.update_synced_height(height); + // Deliberately the same event an advance emits: the + // persisters apply `synced_height` verbatim (no monotonic + // clamp at the row), so this is what makes the rewind + // durable across a restart. Only the in-batch changeset + // merge is monotonic-max; the caller keeps the rewind out + // of a batch that also carries a higher advance by not + // advancing a rewound wallet at the same commit. + self.emit_event(WalletEvent::SyncHeightAdvanced { + wallet_id: *wallet_id, + height, + }); + } + } + } + fn update_wallet_last_processed_height( &mut self, wallet_id: &WalletId, diff --git a/key-wallet-manager/src/test_utils/mock_wallet.rs b/key-wallet-manager/src/test_utils/mock_wallet.rs index 63e02b487..2b328e13d 100644 --- a/key-wallet-manager/src/test_utils/mock_wallet.rs +++ b/key-wallet-manager/src/test_utils/mock_wallet.rs @@ -223,6 +223,12 @@ impl WalletInterface for MockWallet { } } + fn rewind_wallet_synced_height(&mut self, wallet_id: &WalletId, height: CoreBlockHeight) { + if wallet_id == &self.wallet_id && height < self.synced_height { + self.synced_height = height; + } + } + fn update_wallet_last_processed_height( &mut self, wallet_id: &WalletId, @@ -351,6 +357,12 @@ impl WalletInterface for NonMatchingMockWallet { } } + fn rewind_wallet_synced_height(&mut self, wallet_id: &WalletId, height: CoreBlockHeight) { + if wallet_id == &self.wallet_id && height < self.synced_height { + self.synced_height = height; + } + } + fn update_wallet_last_processed_height( &mut self, wallet_id: &WalletId, diff --git a/key-wallet-manager/src/wallet_interface.rs b/key-wallet-manager/src/wallet_interface.rs index 6f43d99de..b6d0d1519 100644 --- a/key-wallet-manager/src/wallet_interface.rs +++ b/key-wallet-manager/src/wallet_interface.rs @@ -165,6 +165,16 @@ pub trait WalletInterface: Send + Sync + 'static { /// only advance forward (a value below the current is silently ignored). fn update_wallet_synced_height(&mut self, wallet_id: &WalletId, height: CoreBlockHeight); + /// Rewind one wallet's committed sync checkpoint below its current value, + /// so the filter sync re-walks committed history — used when scripts + /// derived after a range committed must still be tested against it. + /// Implementations must only lower (a value at or above the current is + /// silently ignored) and must emit the same persistence signal an advance + /// emits, so the rewound checkpoint survives a restart and the re-walk + /// resumes from its own committed progress. The default is a no-op for + /// implementations that predate backward coverage. + fn rewind_wallet_synced_height(&mut self, _wallet_id: &WalletId, _height: CoreBlockHeight) {} + /// Advance one wallet's last-processed height after a block has been applied /// to its state. Implementations must only advance forward. fn update_wallet_last_processed_height( From b5312ad87b4741f20463b76d02f0e809d72163de Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:05:20 +0300 Subject: [PATCH 2/5] fix(dash-spv): clamp the backward-coverage rewind to each wallet's own birth height The forward drain rewinds every wallet with newly derived scripts to one floor, the earliest height any wallet requires. When a wallet with a lower birth height is added at runtime, that floor dragged an older wallet's checkpoint below its own birth (CI: `test_runtime_add_during_initial_sync`, W1 rewound 20999 -> 0), re-walking history the wallet cannot have touched and, on a persisted store, reading as a reset. `WalletManager` now clamps the rewind to `birth_height - 1` per wallet; the trait contract says so. Two dashd integration tests asserted the old invariant that a wallet's synced_height never decreases. It now legitimately dips at the drain and climbs back during the re-walk: - `tests_multi_wallet::test_runtime_add_during_initial_sync` checks that W1 never goes below its own birth height and still converges to the tip. - `dash-spv-ffi tests_callback::test_all_callbacks_during_sync` waits (up to 60 s) for `on_synced_height_updated` to report the tip again instead of sampling the last value once, which could land on the rewind. --- .../tests/dashd_sync/tests_callback.rs | 29 +++++++++++++++---- .../tests/dashd_sync/tests_multi_wallet.rs | 14 +++++++-- key-wallet-manager/src/process_block.rs | 8 +++++ key-wallet-manager/src/wallet_interface.rs | 10 ++++--- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/dash-spv-ffi/tests/dashd_sync/tests_callback.rs b/dash-spv-ffi/tests/dashd_sync/tests_callback.rs index 4acd4b53c..fc00b44e3 100644 --- a/dash-spv-ffi/tests/dashd_sync/tests_callback.rs +++ b/dash-spv-ffi/tests/dashd_sync/tests_callback.rs @@ -146,16 +146,33 @@ fn test_all_callbacks_during_sync() { // so observing block-processed records does not guarantee it has fired yet. tracker.wait_for_callback(&tracker.synced_height_updated_count, 0, "synced_height_updated"); let synced_height_fired = tracker.synced_height_updated_count.load(Ordering::SeqCst); - let last_synced_height = tracker.last_synced_height.load(Ordering::SeqCst); assert!( synced_height_fired > 0, "on_synced_height_updated should fire at least once during sync" ); - assert!( - last_synced_height >= dashd.initial_height, - "last_synced_height ({}) should be at least initial_height ({}) after sync", - last_synced_height, - dashd.initial_height + // The callback is not monotonic: scripts derived during the scan are + // covered by rewinding the wallet's checkpoint and re-walking + // committed history (backward coverage), and the rewind is reported + // through this same callback. Wait for the re-walk to bring the + // reported height back to the tip instead of sampling it once. + let synced_deadline = std::time::Instant::now() + Duration::from_secs(60); + let last_synced_height = loop { + let h = tracker.last_synced_height.load(Ordering::SeqCst); + if h >= dashd.initial_height { + break h; + } + assert!( + std::time::Instant::now() < synced_deadline, + "last_synced_height ({}) did not reach initial_height ({}) within 60s", + h, + dashd.initial_height + ); + std::thread::sleep(Duration::from_millis(100)); + }; + tracing::info!( + "SyncedHeightUpdated: fired {} time(s), last {}", + synced_height_fired, + last_synced_height ); // Validate sync cycle (initial sync is cycle 0) diff --git a/dash-spv/tests/dashd_sync/tests_multi_wallet.rs b/dash-spv/tests/dashd_sync/tests_multi_wallet.rs index 276f43cbf..aa7027fd1 100644 --- a/dash-spv/tests/dashd_sync/tests_multi_wallet.rs +++ b/dash-spv/tests/dashd_sync/tests_multi_wallet.rs @@ -425,6 +425,8 @@ async fn test_runtime_add_during_initial_sync() { } let (w1_synced_at_add, w1_processed_at_add) = wallet_heights(&wallet, &w1_id).await; + let w1_birth_height = + wallet.read().await.get_wallet_info(&w1_id).expect("wallet info").birth_height(); assert!( w1_synced_at_add < initial_height, "W1 must be in mid-flight at the moment W2 is added (synced_height={}, tip={})", @@ -443,11 +445,17 @@ async fn test_runtime_add_during_initial_sync() { loop { let (w1_synced_now, w1_processed_now) = wallet_heights(&wallet, &w1_id).await; let (w2_synced_now, _) = wallet_heights(&wallet, &w2_id).await; + // W1's synced_height may legitimately drop below its add-time value: + // scripts W1 derives while scanning are covered by rewinding its + // checkpoint and re-walking committed history (backward coverage), + // and that rewind is persisted. What must hold is that it never goes + // under W1's own start — W2's lower birth height must not drag it + // there — and that it converges to the tip below. assert!( - w1_synced_now >= w1_synced_at_add, - "W1 synced_height regressed during mid-flight rescan: {} -> {}", - w1_synced_at_add, + w1_synced_now + 1 >= w1_birth_height, + "W1 synced_height rewound below its own birth height: {} (birth {})", w1_synced_now, + w1_birth_height, ); assert!( w1_processed_now >= w1_processed_at_add, diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index 17a55bc61..86a6e901b 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -365,6 +365,14 @@ impl WalletInterface for WalletM fn rewind_wallet_synced_height(&mut self, wallet_id: &WalletId, height: CoreBlockHeight) { if let Some(info) = self.wallet_infos.get_mut(wallet_id) { + // Never below this wallet's own start: the caller passes one + // floor for every wallet it rewinds (the earliest height any of + // them requires), and a wallet added at runtime with a lower + // birth height must not drag an older wallet's checkpoint under + // its own birth — that re-walks history the wallet cannot have + // touched, and on a persisted store it looks like the wallet + // was reset. + let height = height.max(info.birth_height().saturating_sub(1)); if height < info.synced_height() { info.update_synced_height(height); // Deliberately the same event an advance emits: the diff --git a/key-wallet-manager/src/wallet_interface.rs b/key-wallet-manager/src/wallet_interface.rs index b6d0d1519..ac963bf1a 100644 --- a/key-wallet-manager/src/wallet_interface.rs +++ b/key-wallet-manager/src/wallet_interface.rs @@ -169,10 +169,12 @@ pub trait WalletInterface: Send + Sync + 'static { /// so the filter sync re-walks committed history — used when scripts /// derived after a range committed must still be tested against it. /// Implementations must only lower (a value at or above the current is - /// silently ignored) and must emit the same persistence signal an advance - /// emits, so the rewound checkpoint survives a restart and the re-walk - /// resumes from its own committed progress. The default is a no-op for - /// implementations that predate backward coverage. + /// silently ignored), must clamp `height` to the wallet's own earliest + /// required height so a floor computed across several wallets never + /// drags one below its birth, and must emit the same persistence signal + /// an advance emits, so the rewound checkpoint survives a restart and the + /// re-walk resumes from its own committed progress. The default is a + /// no-op for implementations that predate backward coverage. fn rewind_wallet_synced_height(&mut self, _wallet_id: &WalletId, _height: CoreBlockHeight) {} /// Advance one wallet's last-processed height after a block has been applied From 4abfece69edecc58ea6e2c87432e1c9572990962 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:20:23 +0300 Subject: [PATCH 3/5] test(dash-spv-ffi): assert the first sync cycle is 0, not the last The callback test's wallet has transactions, so the scan derives scripts and a backward-coverage re-walk follows, completing as a later cycle. Track the cycle of the first on_sync_complete in the tracker and assert on that; the last cycle is logged. --- dash-spv-ffi/tests/dashd_sync/callbacks.rs | 9 ++++++++- .../tests/dashd_sync/tests_callback.rs | 18 +++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/dash-spv-ffi/tests/dashd_sync/callbacks.rs b/dash-spv-ffi/tests/dashd_sync/callbacks.rs index c9e58b108..b5304dd5d 100644 --- a/dash-spv-ffi/tests/dashd_sync/callbacks.rs +++ b/dash-spv-ffi/tests/dashd_sync/callbacks.rs @@ -110,6 +110,11 @@ pub(super) struct CallbackTracker { // Completion tracking pub(super) last_sync_cycle: AtomicU32, + /// Cycle number of the FIRST `on_sync_complete`. A wallet that derives + /// scripts while scanning is followed by a backward-coverage re-walk, + /// which completes as a further cycle, so `last_sync_cycle` is not the + /// initial one for such wallets. + pub(super) first_sync_cycle: AtomicU32, // Baseline for `wait_for_sync`: captured before the client starts so that // a SyncComplete firing between client start and `wait_for_sync` entry is @@ -346,7 +351,9 @@ extern "C" fn on_sync_complete(header_tip: u32, cycle: u32, user_data: *mut c_vo tracker.last_sync_cycle.store(cycle, Ordering::SeqCst); let seq = tracker.sequence_counter.fetch_add(1, Ordering::SeqCst); tracker.sync_complete_seq.store(seq, Ordering::SeqCst); - tracker.sync_complete_count.fetch_add(1, Ordering::SeqCst); + if tracker.sync_complete_count.fetch_add(1, Ordering::SeqCst) == 0 { + tracker.first_sync_cycle.store(cycle, Ordering::SeqCst); + } tracing::info!("on_sync_complete: header_tip={}, cycle={}, seq={}", header_tip, cycle, seq); } diff --git a/dash-spv-ffi/tests/dashd_sync/tests_callback.rs b/dash-spv-ffi/tests/dashd_sync/tests_callback.rs index fc00b44e3..dbc74bbdd 100644 --- a/dash-spv-ffi/tests/dashd_sync/tests_callback.rs +++ b/dash-spv-ffi/tests/dashd_sync/tests_callback.rs @@ -175,9 +175,21 @@ fn test_all_callbacks_during_sync() { last_synced_height ); - // Validate sync cycle (initial sync is cycle 0) - let last_sync_cycle = tracker.last_sync_cycle.load(Ordering::SeqCst); - assert_eq!(last_sync_cycle, 0, "Initial sync should be cycle 0"); + // Validate sync cycle (initial sync is cycle 0). This wallet has + // transactions, so the scan derives scripts and a backward-coverage + // re-walk follows, completing as a later cycle — check the first + // completion, not the last. + assert!( + tracker.sync_complete_count.load(Ordering::SeqCst) > 0, + "on_sync_complete should have fired" + ); + let first_sync_cycle = tracker.first_sync_cycle.load(Ordering::SeqCst); + assert_eq!(first_sync_cycle, 0, "Initial sync should be cycle 0"); + tracing::info!( + "Sync cycles: first={}, last={}", + first_sync_cycle, + tracker.last_sync_cycle.load(Ordering::SeqCst) + ); // Validate callback lifecycle ordering let sync_start_seq = tracker.sync_start_seq.load(Ordering::SeqCst); From 8843421a98e07659c75518b8b4196104e1374ae8 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:33:29 +0300 Subject: [PATCH 4/5] fix(dash-spv): hold filter completion while a rewound wallet's re-walk is pending; cover the rewind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI (Ubuntu ARM / ffi): `test_ffi_multiple_transactions_across_blocks` read 24 transactions instead of 25 right after `wait_for_sync`. The forward drain rewound the wallet and then declared the filters complete in the same pass, so `SyncComplete` fired with the re-walk still to run; on a slow runner the tip block's transaction landed after the test read the count. The same ordering is what produced a spurious extra sync cycle in `test_all_callbacks_during_sync`. `try_process_batch` now skips `FiltersSyncComplete` while `rewalk_pending()` — a wallet below the committed frontier that the sync-manager tick will restart the scan for, tested exactly as the tick tests it (lowest stale synced_height + 1, floored at birth height and stored-header start, reaching the frontier). The state stays Syncing through the re-walk and completion is emitted once, after it. Coverage: - `backward_coverage_rewinds_and_holds_completion_until_rewalked` replaces the first ignored sweep test: the committed-batch shape now asserts the rewind to birth_height - 1, `rewalk_pending()`, no completion while behind, and completion once the wallet has caught up. The second ignored test keeps its `#[ignore]` with an updated reason. - `WalletManager::rewind_wallet_synced_height`: lowers and emits SyncHeightAdvanced, ignores a non-lowering value and an unknown wallet, clamps to the wallet's own birth_height - 1. --- .../filters/coinjoin_gap_discovery_tests.rs | 94 ++++++++++--------- dash-spv/src/sync/filters/manager.rs | 33 +++++++ key-wallet-manager/src/event_tests.rs | 75 +++++++++++++++ 3 files changed, 159 insertions(+), 43 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 e84261ac2..2399087cc 100644 --- a/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs +++ b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs @@ -50,6 +50,7 @@ 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::wallet_info_interface::WalletInfoInterface; use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; use key_wallet_manager::WalletManager; use tokio::sync::mpsc::unbounded_channel; @@ -325,32 +326,25 @@ async fn coinjoin_gap_limit_inversion_within_batch_recovers() { ); } -/// Gap-window outputs in an already-COMMITTED batch (#846). +/// Backward coverage across a committed batch is a durable rewind, not an +/// in-memory sweep. /// -/// Same funding shape as the within-batch inversion test, but the early -/// block (indices G+10..=G+21, 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 extends the window past G+21, 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 G+10..=G+21 — -/// squarely inside the BIP-44/CoinJoin gap-limit recovery contract -/// (G+21 < 29 + 1 + G) — used to stay invisible forever, along with their -/// funds; a fresh re-sync from genesis hit the same wall deterministically. +/// Block A (height 10, batch 0) funds CoinJoin External indices G+10..=G+21, +/// beyond the initial gap window; block B (height 110, batch 1) funds +/// 0..=29 and its processing derives the scripts that would have matched +/// block A — after batch 0 has already committed. At the forward drain the +/// manager must not sweep the committed range in memory (an iOS suspension +/// drops such a sweep whole) but rewind the wallet's `synced_height` so the +/// sync-manager tick re-walks committed history in persisted batches, and +/// it must NOT declare the filters complete while that re-walk is pending: +/// one "synced" cycle with the walk still to run is exactly what the host +/// would mistake for a caught-up wallet. Once the wallet is back at the +/// committed frontier, completion is emitted. /// -/// GREEN since `rescan_committed_range`: newly derived scripts are re-tested -/// against the persisted filters below the committing batch (BIP-158 filters -/// are address-independent, so re-matching needs no re-download), and hits -/// flow through the `track_for_new_scripts` re-download path to the same -/// commit-time fixpoint. `highest_used` reaches G+21. +/// The tick itself does not run in this harness, so the re-walk is +/// represented by advancing the wallet's checkpoint by hand. #[tokio::test] -#[ignore = "LOCAL DIAGNOSTIC BUILD: backward coverage now happens through a \ -synced_height rewind picked up by the sync-manager tick (durable re-walk), \ -not the in-manager sweep this harness can observe — the tick never runs \ -here, so recovery cannot complete inside this test. Field-verified instead; \ -un-ignore when reverting to the sweep."] -async fn coinjoin_gap_limit_stall_across_committed_batch() { +async fn backward_coverage_rewinds_and_holds_completion_until_rewalked() { let (mut manager, wallet, wallet_id) = setup().await; let addresses = coinjoin_external_addresses(&wallet, &wallet_id, (G + 22) as u32).await; @@ -397,29 +391,42 @@ async fn coinjoin_gap_limit_stall_across_committed_batch() { 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 G+21, so the missed indices ARE inside the watched range by now. + let (_, highest_generated, _) = coinjoin_pool_state(&wallet, &wallet_id).await; assert!( highest_generated >= Some((G + 21) as u32), "gap maintenance must have extended the watch window past index G+21 \ (got {highest_generated:?})" ); + + // The drain rewound the wallet to its own floor instead of sweeping. + let (synced_height, birth_height) = { + let reader = wallet.read().await; + let info = reader.get_wallet_info(&wallet_id).expect("wallet info"); + (info.synced_height(), info.birth_height()) + }; assert_eq!( - highest_used, - Some((G + 21) as u32), - "CoinJoin External indices G+10..=G+21 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." + synced_height, + birth_height.saturating_sub(1), + "wallet synced_height must be rewound to birth_height - 1 for the durable re-walk" + ); + assert!(manager.rewalk_pending().await, "a re-walk must be pending after the rewind"); + assert_eq!( + manager.state(), + SyncState::Syncing, + "filters must not be declared complete while a rewound wallet is below the frontier" + ); + + // Stand in for the tick's re-walk: the wallet catches up to the + // committed frontier. Only now may the filters complete. + let committed = manager.progress.committed_height(); + wallet.write().await.update_wallet_synced_height(&wallet_id, committed); + assert!(!manager.rewalk_pending().await); + let events = manager.try_process_batch().await.unwrap(); + assert!( + events.iter().any(|e| matches!(e, SyncEvent::FiltersSyncComplete { .. })), + "FiltersSyncComplete must be emitted once the rewound wallet has caught up" ); + assert_eq!(manager.state(), SyncState::Synced); } /// Committed-range sweeps coalesce across batch commits. @@ -441,10 +448,11 @@ async fn coinjoin_gap_limit_stall_across_committed_batch() { /// applied. `committed_range_sweeps` counts sweeps that reach the chunk walk /// in `rescan_committed_range`. #[tokio::test] -#[ignore = "LOCAL DIAGNOSTIC BUILD: the tail batch now commits before the \ -sweep's blocks drain (see try_commit_batches), so the 'sweep completes before \ -FiltersSyncComplete' invariant this test asserts is deliberately relaxed on \ -this branch. Upstream keeps the invariant; un-ignore when reverting."] +#[ignore = "backward coverage no longer sweeps the committed range in the \ +manager; it rewinds the wallet and the sync-manager tick re-walks, which this \ +harness cannot drive. The coalescing this test measured has no counterpart \ +now — see backward_coverage_rewinds_and_holds_completion_until_rewalked for \ +the contract that replaced it. Remove together with rescan_committed_range."] async fn committed_range_sweep_coalesces_across_batch_commits() { let (mut manager, wallet, wallet_id) = setup().await; let addresses = coinjoin_external_addresses(&wallet, &wallet_id, (G + 22) as u32).await; diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index bcd8d26e8..73c45d7f3 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -561,7 +561,18 @@ impl= self.progress.filter_header_tip_height() && self.progress.committed_height() >= self.progress.target_height() @@ -590,6 +601,28 @@ impl bool { + let committed = self.progress.committed_height(); + let wallet_read = self.wallet.read().await; + let behind = wallet_read.wallets_behind(committed); + let stale_min_synced = behind.iter().map(|id| wallet_read.wallet_synced_height(id)).min(); + let birth_height = wallet_read.earliest_required_height().await; + drop(wallet_read); + let Some(stale_min_synced) = stale_min_synced else { + return false; + }; + let scan_floor = birth_height + .max(self.header_storage.read().await.get_start_height().await.unwrap_or(0)); + stale_min_synced.saturating_add(1).max(scan_floor) <= committed + } + /// Commit completed batches in order (lowest batch_start first). async fn try_commit_batches(&mut self) -> SyncResult> { let mut events = Vec::new(); diff --git a/key-wallet-manager/src/event_tests.rs b/key-wallet-manager/src/event_tests.rs index afccc35ae..be6e1a69c 100644 --- a/key-wallet-manager/src/event_tests.rs +++ b/key-wallet-manager/src/event_tests.rs @@ -1270,6 +1270,81 @@ async fn test_update_wallet_synced_height_does_not_re_emit_when_unchanged() { ); } +#[tokio::test] +async fn test_rewind_wallet_synced_height_lowers_and_emits() { + let (mut manager, wallet_id, _addr) = setup_manager_with_wallet(); + let mut rx = manager.subscribe_events(); + manager.update_wallet_synced_height(&wallet_id, 2000); + drain_events(&mut rx); + + // A rewind lowers the checkpoint and is reported through the same event + // an advance emits, so persisters store it verbatim and the rewind + // survives a restart. + manager.rewind_wallet_synced_height(&wallet_id, 1200); + assert_eq!(manager.wallet_synced_height(&wallet_id), 1200); + let synced_events: Vec<_> = drain_events(&mut rx) + .into_iter() + .filter_map(|e| match e { + WalletEvent::SyncHeightAdvanced { + wallet_id, + height, + } => Some((wallet_id, height)), + _ => None, + }) + .collect(); + assert_eq!(synced_events, vec![(wallet_id, 1200)]); + + // At or above the current checkpoint is ignored — no change, no event. + manager.rewind_wallet_synced_height(&wallet_id, 1200); + manager.rewind_wallet_synced_height(&wallet_id, 5000); + assert_eq!(manager.wallet_synced_height(&wallet_id), 1200); + let events = drain_events(&mut rx); + assert!( + !events.iter().any(|e| matches!(e, WalletEvent::SyncHeightAdvanced { .. })), + "no SyncHeightAdvanced for a non-lowering rewind, got {:?}", + events + ); + + // An unknown wallet is a no-op. + manager.rewind_wallet_synced_height(&[9u8; 32], 0); + assert!(drain_events(&mut rx).is_empty()); +} + +#[tokio::test] +async fn test_rewind_wallet_synced_height_clamps_to_own_birth_height() { + // The caller passes one floor for every wallet it rewinds; a wallet must + // never be dragged below its own start by another wallet's lower birth. + let mut manager = WalletManager::::new(Network::Testnet); + let wallet_id = manager + .create_wallet_from_mnemonic( + TEST_MNEMONIC, + 500, + key_wallet::wallet::initialization::WalletAccountCreationOptions::Default, + ) + .unwrap(); + manager.update_wallet_synced_height(&wallet_id, 3000); + let mut rx = manager.subscribe_events(); + + manager.rewind_wallet_synced_height(&wallet_id, 0); + assert_eq!(manager.wallet_synced_height(&wallet_id), 499); + let synced_events: Vec<_> = drain_events(&mut rx) + .into_iter() + .filter_map(|e| match e { + WalletEvent::SyncHeightAdvanced { + height, + .. + } => Some(height), + _ => None, + }) + .collect(); + assert_eq!(synced_events, vec![499]); + + // Already at the floor: a second rewind changes nothing. + manager.rewind_wallet_synced_height(&wallet_id, 0); + assert_eq!(manager.wallet_synced_height(&wallet_id), 499); + assert!(drain_events(&mut rx).is_empty()); +} + // --------------------------------------------------------------------------- // Dry run and irrelevant paths // --------------------------------------------------------------------------- From 7814f8fe7617da33bbca7f42d75c2e9cb63bb97a Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Sun, 6 Sep 2026 17:16:02 +0300 Subject: [PATCH 5/5] test(dash-spv): drive the real re-walk, and confirm the rewind landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (CodeRabbit) on the backward-coverage rewind. `MultiMockWallet` inherited the no-op default of `rewind_wallet_synced_height`, so multi-wallet coverage ran against a mock that silently never rewound. Implement it. `try_commit_batches` then read the checkpoint back instead of assuming the call landed: the trait's default is a no-op, and skipping a wallet's commit-time advance on the strength of a call that did nothing would strand that batch's certified coverage with no re-walk to replace it. An implementation that opts out now warns and commits forward as it always did. A rewind clamped up to the wallet's own floor still sits below where it started, so it still counts as one. The trait doc said the default was for implementations predating backward coverage; it now also says what opting out costs. `backward_coverage_rewinds_and_holds_completion_until_rewalked` stood in for the re-walk by advancing the wallet checkpoint by hand, which asserts the completion gate but not that the re-walk finds anything. The harness turns out to be able to drive the real thing: it was only missing a filter-header frontier, without which `start_download` takes its "nothing to download" early return and the rescan is a silent no-op — the exact failure the test exists to catch. It now seeds filters across the whole committed prefix, sets the frontier, and drives `tick` → `start_download` → `BlocksNeeded` → block processing. Block A's beyond-window outputs are asserted absent before the re-walk and recovered after it, and `FiltersSyncComplete` is checked at the moment it is emitted, so a completion that precedes the recovery fails. `drive_to_quiescence` returns the events it does not consume so that check is possible. The sibling ignored test's reason claimed this harness could not drive the tick — corrected. `on_sync_height_advanced` only kept the latest height, so the FFI sync test could exit its wait on the value stored before any rewind and pass with the whole backward-coverage path dead. The tracker now records that a reported height went below one already seen, and the test requires that observation before accepting the recovered tip. Validation: cargo test -p dash-spv --lib (569 passed, 3 ignored), cargo test -p key-wallet-manager --lib (66 passed), cargo clippy on both (clean), cargo fmt --check (clean), cargo check -p dash-spv-ffi --tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014bk8aoTkF8TUS7LyhRBNwH --- dash-spv-ffi/tests/dashd_sync/callbacks.rs | 14 ++- .../tests/dashd_sync/tests_callback.rs | 13 ++- .../filters/coinjoin_gap_discovery_tests.rs | 89 +++++++++++++++---- dash-spv/src/sync/filters/manager.rs | 26 +++++- .../src/test_utils/mock_wallet.rs | 8 ++ key-wallet-manager/src/wallet_interface.rs | 10 ++- 6 files changed, 136 insertions(+), 24 deletions(-) diff --git a/dash-spv-ffi/tests/dashd_sync/callbacks.rs b/dash-spv-ffi/tests/dashd_sync/callbacks.rs index b5304dd5d..6fcb53f6a 100644 --- a/dash-spv-ffi/tests/dashd_sync/callbacks.rs +++ b/dash-spv-ffi/tests/dashd_sync/callbacks.rs @@ -3,7 +3,7 @@ use std::ffi::CStr; use std::os::raw::{c_char, c_void}; use std::slice; -use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -42,8 +42,13 @@ pub(super) struct CallbackTracker { pub(super) block_processed_wallet_count: AtomicU32, pub(super) block_processed_wallet_record_count: AtomicU32, pub(super) synced_height_updated_count: AtomicU32, - /// Highest synced-height value observed from any `SyncedHeightUpdated`. + /// The most recent synced-height value from `SyncedHeightUpdated`. Not + /// monotonic: backward coverage rewinds a wallet's checkpoint and reports + /// the lower value through this same callback. pub(super) last_synced_height: AtomicU32, + /// Set the first time a `SyncedHeightUpdated` reports a height BELOW one + /// already reported — the observable signature of that rewind. + pub(super) synced_height_rewound: AtomicBool, // Data from callbacks pub(super) last_header_tip: AtomicU32, @@ -574,7 +579,10 @@ extern "C" fn on_sync_height_advanced( // Store the height before bumping the counter so a test that waits on the // counter and then reads `last_synced_height` is guaranteed to observe the // height for the same callback invocation. - tracker.last_synced_height.store(height, Ordering::SeqCst); + let previous = tracker.last_synced_height.swap(height, Ordering::SeqCst); + if height < previous { + tracker.synced_height_rewound.store(true, Ordering::SeqCst); + } tracker.synced_height_updated_count.fetch_add(1, Ordering::SeqCst); let wallet_str = unsafe { cstr_or_unknown(wallet_id) }; tracing::info!("on_sync_height_advanced: wallet={}, height={}", wallet_str, height); diff --git a/dash-spv-ffi/tests/dashd_sync/tests_callback.rs b/dash-spv-ffi/tests/dashd_sync/tests_callback.rs index dbc74bbdd..0c141eafa 100644 --- a/dash-spv-ffi/tests/dashd_sync/tests_callback.rs +++ b/dash-spv-ffi/tests/dashd_sync/tests_callback.rs @@ -155,15 +155,24 @@ fn test_all_callbacks_during_sync() { // committed history (backward coverage), and the rewind is reported // through this same callback. Wait for the re-walk to bring the // reported height back to the tip instead of sampling it once. + // Two conditions, not one: the checkpoint must be seen going DOWN + // (the rewind itself) and then coming back to the tip (the re-walk + // that follows it). Waiting only for the tip would be satisfied by + // the value already stored before the rewind ever happened, so the + // whole backward-coverage path could be dead and this test would + // still pass. let synced_deadline = std::time::Instant::now() + Duration::from_secs(60); let last_synced_height = loop { let h = tracker.last_synced_height.load(Ordering::SeqCst); - if h >= dashd.initial_height { + let rewound = tracker.synced_height_rewound.load(Ordering::SeqCst); + if rewound && h >= dashd.initial_height { break h; } assert!( std::time::Instant::now() < synced_deadline, - "last_synced_height ({}) did not reach initial_height ({}) within 60s", + "backward coverage did not complete within 60s: rewind observed={}, \ + last_synced_height={} (initial_height={})", + rewound, h, dashd.initial_height ); 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 2399087cc..6a82bac2f 100644 --- a/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs +++ b/dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rs @@ -202,11 +202,14 @@ async fn drive_to_quiescence( wallet: &Arc>>, blocks: &HashMap, initial_events: Vec, -) { +) -> Vec { let (tx, _rx) = unbounded_channel(); let requests = RequestSender::new(tx); let mut events = initial_events; + // Everything the run emitted that was not a block request — the caller's + // window into completion, which is otherwise consumed here. + let mut observed = Vec::new(); for _round in 0..64 { let mut pending: BTreeMap<(u32, BlockHash), BTreeSet> = BTreeMap::new(); for event in events.drain(..) { @@ -217,10 +220,12 @@ async fn drive_to_quiescence( for (key, wallets) in needed { pending.entry((key.height(), *key.hash())).or_default().extend(wallets); } + } else { + observed.push(event); } } if pending.is_empty() { - return; + return observed; } let mut next_events = Vec::new(); @@ -361,9 +366,11 @@ async fn backward_coverage_rewinds_and_holds_completion_until_rewalked() { { let mut header_storage = manager.header_storage.write().await; let mut filter_storage = manager.filter_storage.write().await; - for height in 0..=99u32 { + for height in 0..=199u32 { let (header, filter_bytes) = if height == 10 { (block_a.header, filter_a.content.clone()) + } else if height == 110 { + (block_b.header, filter_b.content.clone()) } else { let filler = Block::dummy(height, vec![]); let filter = BlockFilter::dummy(&filler); @@ -387,6 +394,12 @@ async fn backward_coverage_rewinds_and_holds_completion_until_rewalked() { batch_1.mark_verified(); manager.active_batches.insert(100, batch_1); manager.progress.update_stored_height(199); + // The re-walk below re-enters `start_download`, which scans against the + // filter-header frontier rather than the injected batches. Without a tip + // it takes the "nothing to download" early return and the rescan is a + // silent no-op — which is precisely the failure this test must not miss. + manager.progress.update_filter_header_tip_height(199); + manager.progress.update_target_height(199); let initial_events = manager.try_process_batch().await.unwrap(); drive_to_quiescence(&mut manager, &wallet, &blocks, initial_events).await; @@ -416,15 +429,60 @@ async fn backward_coverage_rewinds_and_holds_completion_until_rewalked() { "filters must not be declared complete while a rewound wallet is below the frontier" ); - // Stand in for the tick's re-walk: the wallet catches up to the - // committed frontier. Only now may the filters complete. - let committed = manager.progress.committed_height(); - wallet.write().await.update_wallet_synced_height(&wallet_id, committed); - assert!(!manager.rewalk_pending().await); - let events = manager.try_process_batch().await.unwrap(); + // Block A's outputs are still missing at this point — the rewind exists + // to recover them, so the re-walk below has real work to do. + let (highest_used_before, _, _) = coinjoin_pool_state(&wallet, &wallet_id).await; + assert!( + highest_used_before < Some((G + 10) as u32), + "block A's beyond-window outputs must still be unapplied before the re-walk \ + (highest_used={highest_used_before:?})" + ); + + // Drive the real re-walk, not a stand-in: the tick is what notices a + // wallet below the committed frontier, restarts the scan at its rewound + // checkpoint, and re-requests the blocks whose filters match the scripts + // derived since. Feeding those blocks back through the wallet is the + // blocks-manager's job, which `drive_to_quiescence` performs. + let (tx, _rx) = unbounded_channel(); + let requests = RequestSender::new(tx); + let mut sync_complete_seen = false; + for _round in 0..64 { + let events = manager.tick(&requests).await.expect("tick"); + let quiesced = events.is_empty(); + let observed = drive_to_quiescence(&mut manager, &wallet, &blocks, events).await; + for event in observed { + if let SyncEvent::FiltersSyncComplete { + .. + } = event + { + // Completion is only honest once the re-walk has applied what + // it was rewound to find. + let (highest_used, _, _) = coinjoin_pool_state(&wallet, &wallet_id).await; + assert_eq!( + highest_used, + Some((G + 21) as u32), + "FiltersSyncComplete was emitted before the re-walk recovered block A" + ); + sync_complete_seen = true; + } + } + if !manager.rewalk_pending().await && quiesced { + break; + } + } + + let (highest_used, _, used_count) = coinjoin_pool_state(&wallet, &wallet_id).await; + assert_eq!( + highest_used, + Some((G + 21) as u32), + "the re-walk must recover block A's beyond-window outputs (used_count={used_count})" + ); + assert_eq!(used_count, 30 + 12, "indices 0..=29 and G+10..=G+21 must all be marked used"); + + assert!(!manager.rewalk_pending().await, "the re-walk must have completed"); assert!( - events.iter().any(|e| matches!(e, SyncEvent::FiltersSyncComplete { .. })), - "FiltersSyncComplete must be emitted once the rewound wallet has caught up" + sync_complete_seen, + "FiltersSyncComplete must be emitted once the rewound wallet has been re-walked" ); assert_eq!(manager.state(), SyncState::Synced); } @@ -449,10 +507,11 @@ async fn backward_coverage_rewinds_and_holds_completion_until_rewalked() { /// in `rescan_committed_range`. #[tokio::test] #[ignore = "backward coverage no longer sweeps the committed range in the \ -manager; it rewinds the wallet and the sync-manager tick re-walks, which this \ -harness cannot drive. The coalescing this test measured has no counterpart \ -now — see backward_coverage_rewinds_and_holds_completion_until_rewalked for \ -the contract that replaced it. Remove together with rescan_committed_range."] +manager; it rewinds the wallet and the sync-manager tick re-walks it. The \ +per-commit coalescing this test measured has no counterpart now — see \ +backward_coverage_rewinds_and_holds_completion_until_rewalked, which drives \ +that re-walk through the tick and asserts the same recovery. Remove together \ +with rescan_committed_range."] async fn committed_range_sweep_coalesces_across_batch_commits() { let (mut manager, wallet, wallet_id) = setup().await; let addresses = coinjoin_external_addresses(&wallet, &wallet_id, (G + 22) as u32).await; diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index 73c45d7f3..d30dad772 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -748,9 +748,31 @@ impl= before { + continue; + } + wallet.rewind_wallet_synced_height(wallet_id, target); + // Read the checkpoint back instead of assuming the + // rewind landed. `rewind_wallet_synced_height` returns + // nothing and defaults to a no-op, so an implementation + // that predates backward coverage would otherwise have + // its commit-time advance skipped below on the strength + // of a call that did nothing — stranding this batch's + // certified coverage with no re-walk to replace it. A + // rewind clamped up to the wallet's own floor still + // sits under `before`, so it counts as what it is. + if wallet.wallet_synced_height(wallet_id) < before { rewound_wallets.insert(*wallet_id); + } else { + tracing::warn!( + "Backward coverage: wallet {} did not honor the rewind to {} (still at {}); \ + committing its coverage forward instead — scripts derived after this range \ + committed stay untested against it", + hex::encode(wallet_id), + target, + before + ); } } drop(wallet); diff --git a/key-wallet-manager/src/test_utils/mock_wallet.rs b/key-wallet-manager/src/test_utils/mock_wallet.rs index 2b328e13d..2110395b4 100644 --- a/key-wallet-manager/src/test_utils/mock_wallet.rs +++ b/key-wallet-manager/src/test_utils/mock_wallet.rs @@ -538,6 +538,14 @@ impl WalletInterface for MultiMockWallet { } } + fn rewind_wallet_synced_height(&mut self, wallet_id: &WalletId, height: CoreBlockHeight) { + if let Some(state) = self.wallets.get_mut(wallet_id) { + if height < state.synced_height { + state.synced_height = height; + } + } + } + fn update_wallet_last_processed_height( &mut self, wallet_id: &WalletId, diff --git a/key-wallet-manager/src/wallet_interface.rs b/key-wallet-manager/src/wallet_interface.rs index ac963bf1a..a587b57df 100644 --- a/key-wallet-manager/src/wallet_interface.rs +++ b/key-wallet-manager/src/wallet_interface.rs @@ -173,8 +173,14 @@ pub trait WalletInterface: Send + Sync + 'static { /// required height so a floor computed across several wallets never /// drags one below its birth, and must emit the same persistence signal /// an advance emits, so the rewound checkpoint survives a restart and the - /// re-walk resumes from its own committed progress. The default is a - /// no-op for implementations that predate backward coverage. + /// re-walk resumes from its own committed progress. + /// + /// The default is a no-op, for implementations that predate backward + /// coverage. Opting out that way costs backward coverage itself, not + /// correctness of the forward scan: the caller reads the checkpoint back + /// and, seeing it unmoved, commits the range forward as it always did + /// (and warns), rather than skipping the advance for a re-walk that will + /// never happen. fn rewind_wallet_synced_height(&mut self, _wallet_id: &WalletId, _height: CoreBlockHeight) {} /// Advance one wallet's last-processed height after a block has been applied