refactor(spv): eliminate separate thread/runtime, improve shutdown and UX

[lklimek]

Summary

• Eliminate separate OS thread and tokio runtime for SPV — SPV now runs on the main tokio runtime via subtasks.spawn_sync(), simplifying shutdown and reducing resource usage
• Fix shutdown hangs — derive stop_token as a child of the global cancellation token so SPV shuts down automatically on app exit; bump rust-dashcore to include fixes for sync manager tick loop exits and cancellation-aware peer connections
• Improve disconnect speed — pin rust-dashcore to d8bc066 which signals sync coordinator shutdown before network disconnect and races Peer::connect() against the shutdown token (was ~28s, now near-instant)
• Improve sync progress UX — use dash-spv SyncProgress API directly, height-based blocks progress, resilient progress bar across resume/network switch
• UI feedback improvements — disable Disconnect button during stopping, poll status faster during shutdown, update ConnectionStatus immediately on connect/disconnect
• Add mandatory task names to spawn_sync for shutdown diagnostics
• Add shutdown tracing to measure client.stop() duration and trace SPV status polling

Test plan

• All existing tests pass (cargo test --all-features --workspace)
• Clippy clean, fmt clean
• Manual testing: connect to testnet SPV, sync headers/filters/blocks, disconnect — verify disconnect completes in <2s
• Manual testing: close app window during SPV sync — verify clean shutdown without timeout warnings
• Live testnet test available (#[ignore] test that connects, syncs 10s, asserts clean shutdown)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • SPV now runs on the main runtime with immediate status refresh on start/stop; Disconnect is disabled while stopping
  • Named tasks and enhanced shutdown diagnostics for clearer runtime visibility

• Bug Fixes

  • Devnet data-dir sanitization to prevent path traversal
  • Improved zeroization of sensitive wallet material, more responsive shutdown/cancellation, and refreshed connection throttle behavior during shutdown

• Documentation

  • Security audit, architecture and code-review guidance for the SPV refactor; added CI "safe cargo" wrapper docs

• Tests

  • Extensive SPV manager integration test suite covering lifecycle and concurrency

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

Moves SPV from its own OS thread/runtime into the main Tokio runtime as named TaskManager tasks; unifies SPV progress types to SpvSyncProgress; adds task naming/tracking and shutdown diagnostics; updates connection/wallet lifecycle timing and cancellation behavior; adds docs, tests, and security audit.

Changes

Cohort / File(s)	Summary
Documentation & Design CLAUDE.md, docs/ai-design/2026-02-16-spv-single-runtime-refactor/*	Adds architecture, code-review, and security-audit docs describing the SPV single-runtime refactor, risk findings, remediation guidance, and migration/testing notes.
Cargo / Tooling Cargo.toml	Pins multiple dash-* patch deps to a specific Git revision and adds a rust lint config ([lints.rust.unexpected_cfgs] checking cfg(tokio_unstable) at warn).
SPV Core & Tests src/spv/manager.rs, src/spv/mod.rs, src/spv/tests.rs	SpvManager now runs on main runtime via TaskManager; consolidates progress types to SpvSyncProgress (removes DetailedSyncProgress APIs); adds devnet path sanitization, improved zeroization, logging, cancellation fixes, and a comprehensive integration test suite.
Task Management src/utils/tasks.rs	TaskManager gains active_names, JoinSet yields &'static str; spawn_sync requires a name: &'static str; shutdown/join flow enhanced with named-task tracking and timeout diagnostics.
Connection & Wallet Lifecycle src/context/connection_status.rs, src/context/wallet_lifecycle.rs	Adds ConnectionStatus::reset_timer, faster polling during SPV stopping, immediate status refresh after SPV start/stop, replaces anonymous spawns with named tasks, and introduces cancellation-aware debounce/listeners.
UI & Theme src/ui/network_chooser_screen.rs, src/ui/theme.rs, src/backend_task/core/mod.rs	UI uses SpvSyncProgress/ProgressPercentage; disables Disconnect while SPV is stopping; simplifies system theme error handling; imports ProgressPercentage where needed.
Backend Tasks src/backend_task/core/start_dash_qt.rs	dash_qt watcher spawned via spawn_sync with explicit task name; no behavior change beyond labeling.
Misc / Tests-only src/spv/mod.rs, src/spv/tests.rs	Adds tests-only module and extensive SpvManager integration tests covering lifecycle, concurrency, and state transitions.

Sequence Diagram(s)

sequenceDiagram
  participant UI as "UI / NetworkChooser"
  participant TM as "TaskManager"
  participant RT as "Tokio Runtime"
  participant SM as "SpvManager"
  participant NW as "Network / SPV backend"

  UI->>TM: request SPV start (spawn_sync "spv_main_loop")
  TM->>RT: submit named task
  RT->>SM: run_spv_loop (task)
  SM->>NW: start sync / event subscriptions
  NW-->>SM: sync progress events
  SM->>TM: spawn named event handlers
  UI->>SM: status snapshot request
  SM-->>UI: SpvSyncProgress / status
  UI->>SM: request stop
  SM->>TM: cancel stop_token
  TM->>RT: cancel tasks / join (report active_names)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• fix: connection status not clear #532 — Overlapping edits to ConnectionStatus trigger/refresh logic and timer/reset behavior.

Poem

🐰 I hopped from thread to runtime with a nimble spin,
Tasks wear tidy names and graceful logs begin,
SPV now runs in one bright, bustling room,
Tests, audits, and fixes chase away the gloom,
A rabbit cheers the single-runtime bloom! 🎉

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main refactoring effort: eliminating the separate SPV thread/runtime and improving shutdown and user experience.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/spv-connect-button-feedback

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (7)

src/context/connection_status.rs (1)

110-115: Minor inconsistency in mutex poisoning handling.

reset_timer silently ignores a poisoned last_update mutex (if let Ok), while trigger_refresh on line 236 recovers from it via poisoned.into_inner(). If the mutex is poisoned, reset_timer becomes a no-op and the faster polling during shutdown won't kick in.

Align with the existing recovery pattern

     pub fn reset_timer(&self) {
-        if let Ok(mut last) = self.last_update.lock() {
-            *last = Instant::now() - REFRESH_CONNECTED;
-        }
+        let mut last = match self.last_update.lock() {
+            Ok(guard) => guard,
+            Err(poisoned) => poisoned.into_inner(),
+        };
+        *last = Instant::now() - REFRESH_CONNECTED;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/context/connection_status.rs` around lines 110 - 115, reset_timer
currently ignores a poisoned last_update mutex because it uses `if let Ok(...)`,
causing a no-op on poisoning; update reset_timer to handle poisoning the same
way trigger_refresh does by calling lock() and using the poison guard's
into_inner() on Err so you always get a mutable Instant and set `*last =
Instant::now() - REFRESH_CONNECTED`; reference the reset_timer function and the
last_update mutex and mirror the poison recovery pattern used in
trigger_refresh.

src/ui/dashpay/mod.rs (1)

40-41: The contains("seconds") check is coupled to chrono_humanize's English output format.

If chrono_humanize ever changes its output wording or if localization is added, this heuristic would silently break. This is low-risk today since the library's output is stable and the crate is English-only, but worth noting.

A more robust alternative would be to check the time delta directly:

Duration-based approach

-    match dt_result {
-        LocalResult::Single(dt) => {
-            let human = HumanTime::from(dt).to_string();
-            if human.contains("seconds") {
-                Some("just now".to_string())
-            } else {
-                Some(human)
-            }
-        }
-        _ => None,
-    }
+    match dt_result {
+        LocalResult::Single(dt) => {
+            let delta = Utc::now() - dt;
+            if delta.num_seconds().abs() < 60 {
+                Some("just now".to_string())
+            } else {
+                Some(HumanTime::from(dt).to_string())
+            }
+        }
+        _ => None,
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/dashpay/mod.rs` around lines 40 - 41, The heuristic using
human.contains("seconds") is brittle because it depends on chrono_humanize's
English text; replace that text-match with a direct duration check instead:
compute the time delta between now and the timestamp (use the same
DateTime/created_at variable used to produce human), convert to seconds (or use
chrono::Duration) and if delta.num_seconds() < threshold (e.g., 60) return "just
now", otherwise fall back to the humanized string. Update the branch that
currently checks human.contains("seconds") to use the duration comparison so the
logic no longer relies on chrono_humanize output.

Cargo.toml (1)

93-102: Minor URL inconsistency between patch key and replacement URLs.

The [patch] section key uses https://github.com/dashpay/rust-dashcore (no www) while all replacement entries use https://www.github.com/dashpay/rust-dashcore (with www). This works because Cargo resolves git URLs independently, but it's an inconsistency that could confuse future maintainers. Consider aligning the URLs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Cargo.toml` around lines 93 - 102, The patch entries under
[patch."https://github.com/dashpay/rust-dashcore"] use replacement git URLs with
"https://www.github.com/…" which is inconsistent; update the replacement URLs
for all patched crates (dash-network, dash-spv, dashcore, dashcore-private,
dashcore-rpc, dashcore-rpc-json, dashcore_hashes, key-wallet,
key-wallet-manager) to match the patch key by removing the "www" prefix (use
https://github.com/dashpay/rust-dashcore for each) so the patch key and
replacement URLs are consistent.

docs/ai-design/2026-02-16-spv-single-runtime-refactor/security-audit.md (1)

1-8: Documentation note: audit is labeled as automated.

Line 4 states Auditor: Security Engineer (automated). Consider adding context about the tool/process that generated this audit (e.g., AI-assisted) so readers understand the provenance and calibrate their trust level accordingly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/ai-design/2026-02-16-spv-single-runtime-refactor/security-audit.md`
around lines 1 - 8, Update the audit metadata so the "Auditor: Security Engineer
(automated)" entry clearly identifies the tool/process and provenance: either
expand that line to include the tool name and "AI-assisted" (e.g., "Auditor:
Security Engineer (AI-assisted automated tool: TOOL_NAME)") or add a short
"Provenance" or "Tooling" field immediately after the Auditor line that states
the generator (tool/version), whether AI assistance was used, and a brief note
about limitations; modify the header metadata in the document (the "Auditor"
metadata line and/or add a new "Provenance" field) accordingly so readers can
calibrate trust.

src/utils/tasks.rs (1)

32-42: active_names grows unboundedly during normal (non-shutdown) operation.

Names are pushed in spawn_sync but only removed during the shutdown join loop (lines 81–85). For a long-running desktop session that spawns many short-lived tasks, this Vec will grow continuously. In practice this is negligible for a desktop app, but if you want precision in the remaining-tasks diagnostic, consider removing names as tasks complete outside of shutdown as well (e.g., a background reaper that polls join_next).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/tasks.rs` around lines 32 - 42, spawn_sync currently pushes the
task name into active_names but names are only removed during shutdown; change
the lifecycle so each spawned task removes its name when it completes by adding
cleanup to the task wrapper (the spawn_subtask path) — after the future
finishes, lock active_names and remove the matching name (e.g., search by
equality and remove the first match), handling poisoned lock errors by using if
let Ok(mut names) { ... } as elsewhere; this keeps the join/shutdown logic
intact but prevents active_names from growing unbounded during normal operation.

src/spv/manager.rs (1)

1317-1338: Solid path traversal mitigation for devnet names.

The sanitization replaces anything outside [a-zA-Z0-9_-] with _ and falls back to "devnet" on empty. This prevents directory traversal via names like ../../etc. One optional thought: consider enforcing a reasonable max length (e.g., 64 chars) to prevent excessively long directory paths, though this is low risk since devnet names come from configuration.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/spv/manager.rs` around lines 1317 - 1338, The devnet name sanitization in
the Network::Devnet branch (using config.devnet_name and producing sanitized)
should also enforce a maximum length to avoid excessively long filesystem names;
after building sanitized (and after the empty-check fallback), truncate it to a
reasonable limit (e.g., 64 characters) before returning so the returned string
is at most that length. Update the logic around the sanitized variable (and the
final returned value) to apply the truncation step and use the truncated value
in place of sanitized when non-empty.

src/spv/tests.rs (1)

219-242: Consider asserting Idle is not a valid post-start status.

The assertion on Line 231-237 allows Starting, Syncing, or Error. Since start() synchronously sets status to Starting before spawning the async task, it should not be possible for the status to still be Idle at this point. The current assertion implicitly handles this correctly, but adding an explicit exclusion of Idle in the message would make the intent clearer. This is very minor — the test logic is sound.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/spv/tests.rs` around lines 219 - 242, The test
test_start_sets_starting_status should explicitly exclude SpvStatus::Idle after
calling manager.start(0): update the assertion (or add an assert_ne) using
manager.status() / snapshot to ensure snapshot.status is not SpvStatus::Idle and
still allow Starting, Syncing, or Error; reference manager.start,
manager.status, test_start_sets_starting_status and SpvStatus::Idle when making
the change so the intent that start() synchronously transitions out of Idle is
explicit.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@Cargo.toml`:
- Around line 93-102: The patch entries under
[patch."https://github.com/dashpay/rust-dashcore"] use replacement git URLs with
"https://www.github.com/…" which is inconsistent; update the replacement URLs
for all patched crates (dash-network, dash-spv, dashcore, dashcore-private,
dashcore-rpc, dashcore-rpc-json, dashcore_hashes, key-wallet,
key-wallet-manager) to match the patch key by removing the "www" prefix (use
https://github.com/dashpay/rust-dashcore for each) so the patch key and
replacement URLs are consistent.

In `@docs/ai-design/2026-02-16-spv-single-runtime-refactor/security-audit.md`:
- Around line 1-8: Update the audit metadata so the "Auditor: Security Engineer
(automated)" entry clearly identifies the tool/process and provenance: either
expand that line to include the tool name and "AI-assisted" (e.g., "Auditor:
Security Engineer (AI-assisted automated tool: TOOL_NAME)") or add a short
"Provenance" or "Tooling" field immediately after the Auditor line that states
the generator (tool/version), whether AI assistance was used, and a brief note
about limitations; modify the header metadata in the document (the "Auditor"
metadata line and/or add a new "Provenance" field) accordingly so readers can
calibrate trust.

In `@src/context/connection_status.rs`:
- Around line 110-115: reset_timer currently ignores a poisoned last_update
mutex because it uses `if let Ok(...)`, causing a no-op on poisoning; update
reset_timer to handle poisoning the same way trigger_refresh does by calling
lock() and using the poison guard's into_inner() on Err so you always get a
mutable Instant and set `*last = Instant::now() - REFRESH_CONNECTED`; reference
the reset_timer function and the last_update mutex and mirror the poison
recovery pattern used in trigger_refresh.

In `@src/spv/manager.rs`:
- Around line 1317-1338: The devnet name sanitization in the Network::Devnet
branch (using config.devnet_name and producing sanitized) should also enforce a
maximum length to avoid excessively long filesystem names; after building
sanitized (and after the empty-check fallback), truncate it to a reasonable
limit (e.g., 64 characters) before returning so the returned string is at most
that length. Update the logic around the sanitized variable (and the final
returned value) to apply the truncation step and use the truncated value in
place of sanitized when non-empty.

In `@src/spv/tests.rs`:
- Around line 219-242: The test test_start_sets_starting_status should
explicitly exclude SpvStatus::Idle after calling manager.start(0): update the
assertion (or add an assert_ne) using manager.status() / snapshot to ensure
snapshot.status is not SpvStatus::Idle and still allow Starting, Syncing, or
Error; reference manager.start, manager.status, test_start_sets_starting_status
and SpvStatus::Idle when making the change so the intent that start()
synchronously transitions out of Idle is explicit.

In `@src/ui/dashpay/mod.rs`:
- Around line 40-41: The heuristic using human.contains("seconds") is brittle
because it depends on chrono_humanize's English text; replace that text-match
with a direct duration check instead: compute the time delta between now and the
timestamp (use the same DateTime/created_at variable used to produce human),
convert to seconds (or use chrono::Duration) and if delta.num_seconds() <
threshold (e.g., 60) return "just now", otherwise fall back to the humanized
string. Update the branch that currently checks human.contains("seconds") to use
the duration comparison so the logic no longer relies on chrono_humanize output.

In `@src/utils/tasks.rs`:
- Around line 32-42: spawn_sync currently pushes the task name into active_names
but names are only removed during shutdown; change the lifecycle so each spawned
task removes its name when it completes by adding cleanup to the task wrapper
(the spawn_subtask path) — after the future finishes, lock active_names and
remove the matching name (e.g., search by equality and remove the first match),
handling poisoned lock errors by using if let Ok(mut names) { ... } as
elsewhere; this keeps the join/shutdown logic intact but prevents active_names
from growing unbounded during normal operation.

[lklimek]

@QuantumExplorer I will get rid of patch in Cargo.toml in a separate PR, waiting for dashpay/rust-dashcore#440

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/context/connection_status.rs`:
- Around line 114-119: The function reset_timer currently opens an if let and
closes it but is missing the final closing brace for the function, causing a
compile error; add the missing closing brace to terminate the pub fn
reset_timer(&self) { ... } block (referencing the reset_timer method and the
self.last_update.lock() if-let) so the function is properly closed.

In `@src/ui/network_chooser_screen.rs`:
- Line 19: Remove the unused import ProgressPercentage from the use statement
that currently imports dash_sdk::dash_spv::sync::{ProgressPercentage,
SyncProgress as SpvSyncProgress, SyncState}; — edit the use line to only import
the symbols actually used (e.g., keep SyncProgress as SpvSyncProgress and
SyncState) so the redundant ProgressPercentage identifier is eliminated.