fix(platform-wallet): spv client deadlocking when sending a tx - #3730
Conversation
📝 WalkthroughWalkthroughSPV runtime lifecycle management is reorganized: ChangesSPV Runtime Client Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/spv/runtime.rs (1)
138-149:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix SPV shutdown + cleanup in
SpvRuntime::run(packages/rs-platform-wallet/src/spv/runtime.rs)
run()holdsself.client.read().await(client_guard) acrossclient.run().await(lines 138–146);stop()needsself.client.write().await(line 157) totake()and callc.stop(), so shutdown can hang on the documented “until calling stop” path.- If
client.run().awaitreturnsErr, the?at line 146 skips thetake()cleanup (lines 148–150), leavingself.clientpopulated and the runtime stuck “started”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/spv/runtime.rs` around lines 138 - 149, SpvRuntime::run holds the read lock (self.client.read().await -> client_guard) across the await to client.run().await which blocks stop() because stop() needs the write lock to take() and stop the client; also on error the early return skips the cleanup that should take and stop the client. Fix by releasing the read guard before awaiting client.run() (drop client_guard or restructure so you clone needed handle), and ensure cleanup always runs on both Ok and Err paths by using a match/let result = client.run().await; then always acquire self.client.write().await, take() the client and call c.stop() (or call stop() inside a finally-like block) so the runtime is properly cleared even if client.run().await errors; reference SpvRuntime::run, client_guard/self.client.read(), client.run().await, self.client.write().await, take(), and stop().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 143-152: SpvRuntime::run() currently leaves self.client set if
client.run().await errors, causing SpvAlreadyRunning on subsequent starts;
modify the error path so that before returning the PlatformWalletError (from
client.run().await), you acquire the write lock on self.client (via
self.client.write().await) and call take() to clear it, then return the error
(e.g., in the map_err closure or a match/if let Err branch). Ensure the existing
successful-path drop(client_guard) + write-lock + take() remains as-is and that
the clearing logic references the same self.client.write().await and take() used
elsewhere.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 138-149: SpvRuntime::run holds the read lock
(self.client.read().await -> client_guard) across the await to
client.run().await which blocks stop() because stop() needs the write lock to
take() and stop the client; also on error the early return skips the cleanup
that should take and stop the client. Fix by releasing the read guard before
awaiting client.run() (drop client_guard or restructure so you clone needed
handle), and ensure cleanup always runs on both Ok and Err paths by using a
match/let result = client.run().await; then always acquire
self.client.write().await, take() the client and call c.stop() (or call stop()
inside a finally-like block) so the runtime is properly cleared even if
client.run().await errors; reference SpvRuntime::run,
client_guard/self.client.read(), client.run().await, self.client.write().await,
take(), and stop().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccf69bca-614e-416f-a9e9-5bc8d4383ad9
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/spv/runtime.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3730 +/- ##
==========================================
Coverage 87.15% 87.16%
==========================================
Files 2606 2607 +1
Lines 319221 319420 +199
==========================================
+ Hits 278216 278412 +196
- Misses 41005 41008 +3
🚀 New features to boost your workflow:
|
389443f to
f7e8381
Compare
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "bdbf020b86dcb1aa9ad55ccdf7414e876534d92bd58b3640447f37973ec5843a"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR correctly fixes the broadcast self-deadlock that PR #3729 introduced by moving the client.take() cleanup out of the broadcast/start hot path and into run(). However, the cleanup is gated behind a ? so any error from DashSpvClient::run() leaks the client and permanently wedges the runtime in SpvAlreadyRunning state. The broader lock design (read guard held across the entire run() await) also means stop() cannot preempt run() without deadlocking, perpetuating the class of issue this PR is trying to fix.
🔴 1 blocking | 🟡 2 suggestion(s)
3 additional finding(s)
blocking: `self.client` is never cleared when `client.run()` returns an error
packages/rs-platform-wallet/src/spv/runtime.rs (line 143)
The ? on client.run().await (line 146) short-circuits before the cleanup block at lines 148-150 runs. If DashSpvClient::run() returns Err, the function exits early, self.client stays Some(_), and is_started() keeps returning true even though the inner SPV loop has terminated. From that point on every call to start() deterministically returns SpvAlreadyRunning via the pre-check at lines 50-53, making the runtime non-recoverable from any transient SPV failure until the process is restarted. The failure is especially silent through spawn_in_background() (line 169), which logs and drops the error.
Capture the result first, drop the read guard, then take the write lock and clear unconditionally before propagating the original error.
let run_result = client
.run()
.await
.map_err(|e| PlatformWalletError::SpvError(e.to_string()));
drop(client_guard);
let mut client = self.client.write().await;
let _ = client.take();
run_result
suggestion: Read guard held across `client.run().await` makes `stop()` impossible to invoke concurrently
packages/rs-platform-wallet/src/spv/runtime.rs (line 138)
client_guard (a tokio::sync::RwLock read guard on self.client) is held for the full duration of client.run().await. stop() (lines 156-164) starts with self.client.write().await; tokio's RwLock does not let a writer cut in front of active readers and a pending writer blocks new readers. Consequences while run() is alive:
stop()cannot acquire the write lock and will hang untilrun()returns of its own accord — defeating the purpose of having an externalstop().- The pending writer in
stop()will then also block every read-side accessor (broadcast_transaction,sync_progress,update_config,clear_storage,tip_block_time,get_quorum_public_key), reproducing the broader self-deadlock class of bug this PR is supposed to fix.
Expose a shutdown signal (cancellation token or oneshot) that stop() can fire without taking the write lock, or restructure ownership so client.run() is driven via a clone/Arc and the guard is dropped before awaiting the loop.
suggestion: No regression test exercises the broadcast-after-start lifecycle
packages/rs-platform-wallet/src/spv/runtime.rs (line 132)
The original defect (broadcast_transaction returning SpvNotRunning immediately after a successful start() because the client was taken out) is reproducible with a small async test that starts the runtime, spawns run(), and awaits broadcast_transaction. Adding such a test would lock in the fix and would also catch a regression of the error-path cleanup bug flagged above. The existing tests/spv_sync.rs is #[ignore]d and does not cover this state machine.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet/src/spv/runtime.rs`:143-152: `self.client` is never cleared when `client.run()` returns an error
The `?` on `client.run().await` (line 146) short-circuits before the cleanup block at lines 148-150 runs. If `DashSpvClient::run()` returns `Err`, the function exits early, `self.client` stays `Some(_)`, and `is_started()` keeps returning true even though the inner SPV loop has terminated. From that point on every call to `start()` deterministically returns `SpvAlreadyRunning` via the pre-check at lines 50-53, making the runtime non-recoverable from any transient SPV failure until the process is restarted. The failure is especially silent through `spawn_in_background()` (line 169), which logs and drops the error.
Capture the result first, drop the read guard, then take the write lock and clear unconditionally before propagating the original error.
- [SUGGESTION] In `packages/rs-platform-wallet/src/spv/runtime.rs`:138-153: Read guard held across `client.run().await` makes `stop()` impossible to invoke concurrently
`client_guard` (a `tokio::sync::RwLock` read guard on `self.client`) is held for the full duration of `client.run().await`. `stop()` (lines 156-164) starts with `self.client.write().await`; tokio's `RwLock` does not let a writer cut in front of active readers and a pending writer blocks new readers. Consequences while `run()` is alive:
1. `stop()` cannot acquire the write lock and will hang until `run()` returns of its own accord — defeating the purpose of having an external `stop()`.
2. The pending writer in `stop()` will then also block every read-side accessor (`broadcast_transaction`, `sync_progress`, `update_config`, `clear_storage`, `tip_block_time`, `get_quorum_public_key`), reproducing the broader self-deadlock class of bug this PR is supposed to fix.
Expose a shutdown signal (cancellation token or oneshot) that `stop()` can fire without taking the write lock, or restructure ownership so `client.run()` is driven via a clone/`Arc` and the guard is dropped before awaiting the loop.
- [SUGGESTION] In `packages/rs-platform-wallet/src/spv/runtime.rs`:132-153: No regression test exercises the broadcast-after-start lifecycle
The original defect (`broadcast_transaction` returning `SpvNotRunning` immediately after a successful `start()` because the client was taken out) is reproducible with a small async test that starts the runtime, spawns `run()`, and awaits `broadcast_transaction`. Adding such a test would lock in the fix and would also catch a regression of the error-path cleanup bug flagged above. The existing `tests/spv_sync.rs` is `#[ignore]`d and does not cover this state machine.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The blocking error-path cleanup bug from the prior review (prior-1) is FIXED at f7e8381: run() now captures the result of client.run().await, drops the read guard, takes the write lock to clear self.client, and then returns the captured result. Two prior suggestions remain STILL VALID and are carried forward: the read guard held across the entire inner client.run().await blocks stop() from preempting run() (and pending writers also block all read-side accessors), and no regression test exercises the start/run/broadcast/stop lifecycle that originally regressed. No new latest-delta-only issues identified.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: Read guard held across `client.run().await` still prevents `stop()` from preempting `run()` (carried forward)
packages/rs-platform-wallet/src/spv/runtime.rs (line 138)
Carried forward from the prior review at 389443f and STILL VALID at f7e8381. client_guard (a tokio::sync::RwLock read guard on self.client) is acquired at line 138 and kept alive for the full duration of client.run().await at lines 143-146. stop() (lines 156-164) begins with self.client.write().await; tokio's RwLock does not let a writer cut in front of an active reader, and once stop()'s write waiter is queued it also blocks every new reader.
Consequences while run() is alive:
stop()cannot acquire the write lock and will hang untilrun()returns of its own accord — defeating the documented contract at line 132 thatrun()continues untilstop()is called, and making external shutdown impossible.- The pending writer in
stop()will additionally block every read-side accessor (broadcast_transaction,sync_progress,update_config,clear_storage,tip_block_time,get_quorum_public_key), reproducing the same self-deadlock class of bug this PR is trying to eliminate.
The incremental delta in this PR correctly fixes the error-path cleanup, but the underlying lock geometry that produced the original wedge (a read guard held across an indefinite await) is unchanged. Preferred shapes: expose a shutdown signal (cancellation token or oneshot) that stop() can fire without taking the write lock; drive client.run() via a clone/Arc so the guard can be dropped before awaiting the loop; or Option::take() the client out of the slot before awaiting and restore it on completion.
suggestion: No regression test exercises the broadcast-after-start lifecycle or the new error-path cleanup (carried forward)
packages/rs-platform-wallet/src/spv/runtime.rs (line 132)
Carried forward from the prior review at 389443f and STILL VALID at f7e8381. The original defect from PR #3729 (broadcast_transaction returning SpvNotRunning immediately after a successful start() because the client had been taken out of the slot) is reproducible with a small async test that drives SpvRuntime::run() in a spawned task and then awaits broadcast_transaction. The same harness, with an injected failure on client.run(), would also lock in the newly-added cleanup-on-error behavior at lines 148-150 and catch any future regression that reintroduces a ? short-circuit before cleanup.
Verified against the worktree at f7e8381: packages/rs-platform-wallet/tests/ contains spv_sync.rs, contact_workflow_tests.rs, and thread_safety.rs; none reference SpvRuntime, broadcast_transaction, or spawn_in_background, and the existing tests/spv_sync.rs cases remain #[ignore]d and depend on live testnet connectivity. A focused async test with a controllable/fake client (or a thin test seam around the runtime) would give deterministic CI coverage for this state machine.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/rs-platform-wallet/src/spv/runtime.rs`:138-153: Read guard held across `client.run().await` still prevents `stop()` from preempting `run()` (carried forward)
Carried forward from the prior review at 389443ff and STILL VALID at f7e8381c. `client_guard` (a `tokio::sync::RwLock` read guard on `self.client`) is acquired at line 138 and kept alive for the full duration of `client.run().await` at lines 143-146. `stop()` (lines 156-164) begins with `self.client.write().await`; tokio's `RwLock` does not let a writer cut in front of an active reader, and once `stop()`'s write waiter is queued it also blocks every new reader.
Consequences while `run()` is alive:
1. `stop()` cannot acquire the write lock and will hang until `run()` returns of its own accord — defeating the documented contract at line 132 that `run()` continues until `stop()` is called, and making external shutdown impossible.
2. The pending writer in `stop()` will additionally block every read-side accessor (`broadcast_transaction`, `sync_progress`, `update_config`, `clear_storage`, `tip_block_time`, `get_quorum_public_key`), reproducing the same self-deadlock class of bug this PR is trying to eliminate.
The incremental delta in this PR correctly fixes the error-path cleanup, but the underlying lock geometry that produced the original wedge (a read guard held across an indefinite await) is unchanged. Preferred shapes: expose a shutdown signal (cancellation token or oneshot) that `stop()` can fire without taking the write lock; drive `client.run()` via a clone/`Arc` so the guard can be dropped before awaiting the loop; or `Option::take()` the client out of the slot before awaiting and restore it on completion.
- [SUGGESTION] In `packages/rs-platform-wallet/src/spv/runtime.rs`:132-153: No regression test exercises the broadcast-after-start lifecycle or the new error-path cleanup (carried forward)
Carried forward from the prior review at 389443ff and STILL VALID at f7e8381c. The original defect from PR #3729 (`broadcast_transaction` returning `SpvNotRunning` immediately after a successful `start()` because the client had been taken out of the slot) is reproducible with a small async test that drives `SpvRuntime::run()` in a spawned task and then awaits `broadcast_transaction`. The same harness, with an injected failure on `client.run()`, would also lock in the newly-added cleanup-on-error behavior at lines 148-150 and catch any future regression that reintroduces a `?` short-circuit before cleanup.
Verified against the worktree at f7e8381c: `packages/rs-platform-wallet/tests/` contains `spv_sync.rs`, `contact_workflow_tests.rs`, and `thread_safety.rs`; none reference `SpvRuntime`, `broadcast_transaction`, or `spawn_in_background`, and the existing `tests/spv_sync.rs` cases remain `#[ignore]`d and depend on live testnet connectivity. A focused async test with a controllable/fake client (or a thin test seam around the runtime) would give deterministic CI coverage for this state machine.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
QuantumExplorer
changed the title
QuantumExplorer
changed the title
QuantumExplorer
changed the title
4 participants
I misplaced two lines of code in PR #3729 blocking the spa client when sending a tx, this fixes that
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit