From c39a3d0abcf2badf78083f859a36c44a11725425 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:25:30 +0300 Subject: [PATCH 1/3] fix(dash-spv): resume on the invariant start_download asserts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FiltersManager::start_sync` decided whether in-flight work survived a disconnect by asking `!self.active_batches.is_empty()`. `start_download`, which the other branch falls into, asserts something stricter: debug_assert!(self.is_idle(), "manager should have no in-flight state on start"); and `is_idle()` is four conditions — `active_batches`, `tracker`, `pending_batches` and `filter_pipeline` all empty. The guard checked one of them. They come apart routinely. `on_disconnect` deliberately preserves the first three and calls `requeue_in_flight()`, which moves the pipeline's in-flight slots to *pending* — making the pipeline non-idle rather than restoring it. And a batch that finished downloading leaves `active_batches` while its verified output waits in `pending_batches` and its blocks wait in the tracker. Any of those outlasting the last active batch sent a reconnect through the guard and into the assert. In a build with debug assertions on — which `dev-ios` and every other `dev`-profile build is — that aborts the host process. It has: a TestFlight build of the Dash iOS wallet (9.0.0/25, iOS 26.6) died with SIGABRT in exactly this frame after 33 minutes in the background, where network teardown and restore make the disconnect/reconnect cycle routine. The guard now asks `is_idle()`. Resuming is safe for all four cases: the path sets `Syncing`, and `tick` in that state runs `send_pending`, `store_and_match_batches` and `try_process_batch` unconditionally, with `send_pending` a no-op when the pipeline has nothing queued. `is_idle` becomes `pub(super)` so both the decision and the assertion read the same predicate. The regression test fails without the guard change, panicking at the assert — the production crash reproduced in a unit test. cargo test -p dash-spv --lib # 538 passed cargo clippy --all-targets + cargo fmt --check # clean --- dash-spv/src/sync/filters/manager.rs | 39 ++++++++++++++++++++++- dash-spv/src/sync/filters/sync_manager.rs | 17 +++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index 521af307c..61c4063fd 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -139,7 +139,11 @@ impl bool { + /// + /// `pub(super)` so the resume decision in `start_sync` can be taken on the + /// same predicate `start_download` asserts. Checking a subset there is how + /// a disconnect/reconnect cycle used to reach that assert. + pub(super) fn is_idle(&self) -> bool { self.active_batches.is_empty() && self.tracker.is_empty() && self.pending_batches.is_empty() @@ -3077,6 +3081,39 @@ mod tests { assert_eq!(manager.active_batches.keys().next(), Some(&101)); } + /// A verified batch waiting in `pending_batches` outlives the last active + /// one, so a reconnect used to fall through the resume guard — which asked + /// only about `active_batches` — into `start_download`, whose + /// `debug_assert!(is_idle())` then aborted the process. It is reachable in + /// the field: `on_disconnect` preserves this state by design, and a build + /// with debug assertions on turns the mismatch into a crash rather than a + /// log line. + #[tokio::test] + async fn test_start_sync_resumes_when_only_pending_batches_survive() { + let (mut manager, _headers, _filter) = setup_synced_manager_at_tip().await; + + // What a disconnect leaves behind after the last active batch finished + // downloading but before its verified output was processed. + manager.pending_batches.insert(FiltersBatch::new(0, 99, HashMap::new())); + assert!(manager.active_batches.is_empty(), "the guard's old predicate says idle"); + assert!(!manager.is_idle(), "the invariant start_download asserts says otherwise"); + + manager.set_state(SyncState::WaitingForConnections); + let (tx, _rx) = unbounded_channel(); + let requests = RequestSender::new(tx); + + // Must resume rather than start a fresh download over the top of it. + let events = manager.start_sync(&requests).await.unwrap(); + + assert!(events.is_empty(), "resuming emits no SyncStart"); + assert_eq!(manager.state(), SyncState::Syncing); + assert_eq!( + manager.pending_batches.len(), + 1, + "the surviving batch must not be discarded by the resume" + ); + } + /// A fully synced node that reconnects and then sees one new block must /// commit it. `start_sync` takes the `stored == committed == tip` branch, /// reports `Synced`, and anchors the store and processing cursors at the diff --git a/dash-spv/src/sync/filters/sync_manager.rs b/dash-spv/src/sync/filters/sync_manager.rs index 6cc9d7a9e..2dbd8ad5e 100644 --- a/dash-spv/src/sync/filters/sync_manager.rs +++ b/dash-spv/src/sync/filters/sync_manager.rs @@ -59,7 +59,22 @@ impl< // any pending verified batches; calling `start_download` here would // insert a fresh batch at `scan_start` and clobber the existing one, // leaking its `pending_blocks` counter forever. - if !self.active_batches.is_empty() { + // + // The condition is `is_idle()` — the whole invariant `start_download` + // asserts — and not `active_batches` alone. They are not the same + // question: `on_disconnect` calls `requeue_in_flight`, which moves the + // pipeline's in-flight slots to *pending*, and a batch that finished + // downloading leaves `active_batches` while its verified output waits + // in `pending_batches` and its blocks wait in the tracker. Any of those + // three outlasting the last active batch used to fall through to + // `start_download` and trip its `debug_assert!(is_idle())` — an abort + // in any build with debug assertions on. + // + // Resuming is safe for all of them: this path sets `Syncing`, and + // `tick` in that state runs `send_pending`, `store_and_match_batches` + // and `try_process_batch` unconditionally. `send_pending` is a no-op + // when the pipeline has nothing queued. + if !self.is_idle() { self.filter_pipeline.send_pending(requests, &*self.header_storage.read().await).await?; self.set_state(SyncState::Syncing); return Ok(vec![]); From ee7027c8f0cb8c3a67accf7e076c259e18b44867 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:15:07 +0300 Subject: [PATCH 2/3] fix(dash-spv): close the second route into start_download's idle assert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit widened `start_sync`'s resume guard to the four-part `is_idle()` the assert actually states. It left the other route in. `handle_new_filter_headers`, taken when a filter-header event lands while the manager sits in `WaitingForConnections` or `WaitForEvents`, asked the same subset question — `!self.active_batches.is_empty()` — before falling through to `start_download`. A batch that finished downloading leaves `active_batches` while its verified output waits in `pending_batches` and its blocks wait in the tracker, and `on_disconnect` moves the pipeline's in-flight slots to pending rather than clearing them. Any of those outlasting the last active batch reached the same `debug_assert!(self.is_idle())` — an abort in any build with debug assertions on, which is what killed a TestFlight build of the wallet. Extending the target and returning is the correct answer for all four components, not only for active batches: `wanted_message_types()` for filters is unconditionally `[CFilter]`, so `handle_message` keeps running regardless of state and `store_and_match_batches` drains the leftovers. Once genuinely idle, the next filter-header event re-enters this arm and starts the download, or `stop_sync` resets the state on a full peer loss. Found by review on #955 — I fixed one call site and missed this one. The regression test fails without the change, at the assert. cargo test -p dash-spv --lib # 548 passed cargo clippy --all-targets + cargo fmt --check # clean --- dash-spv/src/sync/filters/manager.rs | 42 +++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index 61c4063fd..fac199cb4 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -1080,7 +1080,21 @@ impl Date: Thu, 13 Aug 2026 11:56:25 +0300 Subject: [PATCH 3/3] test(dash-spv): prove the resume guards leave no fresh download behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both regression tests asserted `pending_batches.len() == 1` after the guarded call. That does not separate resuming from restarting: `start_download` preserves `pending_batches` too, so a build with the old subset predicate — and with `debug_assert!` compiled out, as in release — reinitializes the pipeline, inserts a fresh active batch over the surviving work, and still leaves the count at 1. The tests would pass on the bug they exist to catch. Assert what actually distinguishes the two: an empty `active_batches` after each call, plus the unchanged `WaitForEvents` state on the `handle_new_filter_headers` arm, which must return without touching the state machine. cargo test -p dash-spv --lib sync::filters::manager # 49 passed --- dash-spv/src/sync/filters/manager.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dash-spv/src/sync/filters/manager.rs b/dash-spv/src/sync/filters/manager.rs index fac199cb4..f3f0c93c4 100644 --- a/dash-spv/src/sync/filters/manager.rs +++ b/dash-spv/src/sync/filters/manager.rs @@ -3118,6 +3118,21 @@ mod tests { let events = manager.handle_new_filter_headers(200, &requests).await.unwrap(); assert!(events.is_empty(), "extending the target emits nothing"); + // `pending_batches` alone does not prove the guard held: `start_download` + // preserves it too, so a broken guard would reinitialize the pipeline and + // still leave this at 1. The state and the absence of a fresh active + // batch are what tell resuming from restarting — and unlike the + // `debug_assert` inside `start_download`, they still hold in a release + // build, where that assert is compiled out. + assert_eq!( + manager.state(), + SyncState::WaitForEvents, + "the arm must return without changing state" + ); + assert!( + manager.active_batches.is_empty(), + "must not start a fresh download over the surviving work" + ); assert_eq!(manager.pending_batches.len(), 1, "the surviving batch must not be discarded"); } @@ -3147,6 +3162,13 @@ mod tests { assert!(events.is_empty(), "resuming emits no SyncStart"); assert_eq!(manager.state(), SyncState::Syncing); + // See the sibling test: `start_download` keeps `pending_batches`, so only + // the absence of a new active batch proves this resumed rather than + // restarted, and it proves it without relying on a debug assertion. + assert!( + manager.active_batches.is_empty(), + "must not start a fresh download over the surviving work" + ); assert_eq!( manager.pending_batches.len(), 1,