fix(spv): clear stale SPV state and banner on RPC mode switch#668
fix(spv): clear stale SPV state and banner on RPC mode switch#668thepastaclaw wants to merge 2 commits intodashpay:v1.0-devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded logic in the RPC branch of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🤖 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 444-450: Reset the SPV status in the same RPC-branch cleanup where
you clear spv_connected_peers and spv_no_peers_since: set self.spv_status to a
non-Stopping state (e.g., SpvStatus::Stopped or the equivalent default) so that
code checking `spv_status == Stopping` (used to enforce the 200ms fast refresh)
won't remain true after switching to RPC; update the same block that touches
spv_connected_peers and spv_no_peers_since to also update spv_status via the
same atomic/mutex accessors used elsewhere.
| // Clear stale SPV state that may linger if the backend mode | ||
| // switched from SPV to RPC without a full `reset()`. | ||
| self.spv_connected_peers.store(0, Ordering::Relaxed); | ||
| *self | ||
| .spv_no_peers_since | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) = None; |
There was a problem hiding this comment.
Reset spv_status in RPC branch to prevent stale fast-refresh cadence.
At Line [444]-Line [450], peer-related SPV fields are cleared, but spv_status is not. Since Line [394] uses spv_status == Stopping to force a 200ms refresh interval, switching to RPC during SPV shutdown can leave RPC polling stuck at the fast cadence and spam GetBestChainLocks.
Proposed minimal fix
CoreBackendMode::Rpc => {
// Clear stale SPV state that may linger if the backend mode
// switched from SPV to RPC without a full `reset()`.
+ self.set_spv_status(SpvStatus::Idle);
self.spv_connected_peers.store(0, Ordering::Relaxed);
*self
.spv_no_peers_since
.lock()
.unwrap_or_else(|e| e.into_inner()) = None;🤖 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 444 - 450, Reset the SPV
status in the same RPC-branch cleanup where you clear spv_connected_peers and
spv_no_peers_since: set self.spv_status to a non-Stopping state (e.g.,
SpvStatus::Stopped or the equivalent default) so that code checking `spv_status
== Stopping` (used to enforce the 200ms fast refresh) won't remain true after
switching to RPC; update the same block that touches spv_connected_peers and
spv_no_peers_since to also update spv_status via the same atomic/mutex accessors
used elsewhere.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/context/connection_status.rs (1)
444-450:⚠️ Potential issue | 🟠 MajorReset
spv_statusin RPC cleanup to avoid stale 200ms polling.In Line [444]-Line [450], peer/timer SPV fields are cleared, but
spv_statusis not. Because Line [394] forces a 200ms refresh when status isStopping, an SPV→RPC switch can keep RPC in fast polling mode.Proposed minimal fix
CoreBackendMode::Rpc => { // Clear stale SPV state that may linger if the backend mode // switched from SPV to RPC without a full `reset()`. + self.set_spv_status(SpvStatus::Idle); self.spv_connected_peers.store(0, Ordering::Relaxed); *self .spv_no_peers_since .lock() .unwrap_or_else(|e| e.into_inner()) = None;Based on learnings: "when reviewing Rust files, avoid proposing extensive changes to RPC-related code paths... keep scope small and document the deprecation context."
🤖 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 444 - 450, The RPC cleanup currently clears spv_connected_peers and spv_no_peers_since but leaves spv_status intact, which can leave the status as Stopping and force 200ms polling; update the same cleanup block to reset spv_status as well (e.g., set the spv_status holder to None or its neutral/idle state) so that spv_status no longer triggers fast polling after an SPV→RPC switch; locate the fields spv_connected_peers, spv_no_peers_since and spv_status in the same struct and add the spv_status reset alongside the two existing clears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/context/connection_status.rs`:
- Around line 444-450: The RPC cleanup currently clears spv_connected_peers and
spv_no_peers_since but leaves spv_status intact, which can leave the status as
Stopping and force 200ms polling; update the same cleanup block to reset
spv_status as well (e.g., set the spv_status holder to None or its neutral/idle
state) so that spv_status no longer triggers fast polling after an SPV→RPC
switch; locate the fields spv_connected_peers, spv_no_peers_since and spv_status
in the same struct and add the spv_status reset alongside the two existing
clears.
|
@thepastaclaw please rebase |
When the backend switches from SPV to RPC without a full reset(), stale spv_no_peers_since and spv_connected_peers values could persist, causing the degraded timer to remain active incorrectly. Additionally, the SPV degraded banner was only managed inside the SPV mode branch, so switching away from SPV while the banner was showing would leave it stuck on screen. Fixes: 1. Clear SPV peer fields at the top of the RPC branch in refresh_zmq_and_spv(). 2. Clear the SPV degraded banner unconditionally when not in SPV mode.
thepastaclaw
force-pushed
the
fix/spv-state-leak-banner-cleanup
branch
from
b16afeb to
675c518
Compare
|
Checked — branch is already up to date with |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused fix that clears stale SPV peer-tracking state when in RPC mode. The core fix is correct but the cleanup is incomplete — spv_status is not cleared, which can leave the connection poll loop on fast 200ms cadence indefinitely after SPV shutdown.
Reviewed commit: 2b7f722
🟡 2 suggestion(s)
1 additional finding
🟡 suggestion: Banner not updated when backend mode changes but connection state stays the same
src/app.rs (line 914)
update_connection_banner() only compares previous vs current overall state. If SPV Disconnected → RPC Disconnected, the function returns early and leaves the old SPV banner. The PR description mentions banner cleanup but the diff doesn't touch app.rs.
🤖 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.
In `src/context/connection_status.rs`:
- [SUGGESTION] lines 461-468: RPC cleanup doesn't reset spv_status — stale Stopping state affects poll cadence
The new RPC branch clears `spv_connected_peers` and `spv_no_peers_since`, but doesn't reset `spv_status`. After `stop_spv()` sets status to `Stopping`, `trigger_refresh()` continues using the 200ms shutdown poll cadence instead of the normal 5s interval. Also, `spv_last_error` is not cleared, which `reset()` does handle (lines 99-100, 113-114).
Consider adding `self.spv_status = SpvStatus::Idle;` and `self.spv_last_error = None;` to match the full cleanup that `reset()` performs.
In `src/app.rs`:
- [SUGGESTION] line 914: Banner not updated when backend mode changes but connection state stays the same
`update_connection_banner()` only compares previous vs current overall state. If SPV Disconnected → RPC Disconnected, the function returns early and leaves the old SPV banner. The PR description mentions banner cleanup but the diff doesn't touch app.rs.
| CoreBackendMode::Rpc => { | ||
| // Clear stale SPV state that may linger if the backend mode | ||
| // switched from SPV to RPC without a full `reset()`. | ||
| self.spv_connected_peers.store(0, Ordering::Relaxed); | ||
| *self | ||
| .spv_no_peers_since | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) = None; |
There was a problem hiding this comment.
🟡 Suggestion: RPC cleanup doesn't reset spv_status — stale Stopping state affects poll cadence
The new RPC branch clears spv_connected_peers and spv_no_peers_since, but doesn't reset spv_status. After stop_spv() sets status to Stopping, trigger_refresh() continues using the 200ms shutdown poll cadence instead of the normal 5s interval. Also, spv_last_error is not cleared, which reset() does handle (lines 99-100, 113-114).
Consider adding self.spv_status = SpvStatus::Idle; and self.spv_last_error = None; to match the full cleanup that reset() performs.
source: ['codex-general', 'claude-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/connection_status.rs`:
- [SUGGESTION] lines 461-468: RPC cleanup doesn't reset spv_status — stale Stopping state affects poll cadence
The new RPC branch clears `spv_connected_peers` and `spv_no_peers_since`, but doesn't reset `spv_status`. After `stop_spv()` sets status to `Stopping`, `trigger_refresh()` continues using the 200ms shutdown poll cadence instead of the normal 5s interval. Also, `spv_last_error` is not cleared, which `reset()` does handle (lines 99-100, 113-114).
Consider adding `self.spv_status = SpvStatus::Idle;` and `self.spv_last_error = None;` to match the full cleanup that `reset()` performs.
2 participants
Issue Being Fixed
After switching from SPV to RPC backend mode, two pieces of stale SPV state could linger:
spv_connected_peersandspv_no_peers_sinceinConnectionStatuswere not cleared when entering the RPC branch oftrigger_refresh(). If a user switched backend modes without a fullreset(), the degraded peer timer could still be active, potentially causingspv_peer_degraded()to returntrueeven though SPV was no longer in use.The SPV degraded banner (
SPV_DEGRADED_BANNER) was only cleared inside theif backend_mode == CoreBackendMode::Spvblock. After switching to RPC, the banner would persist on screen since the clear path was never reached.Solution
connection_status.rs: Clearspv_connected_peers(→ 0) andspv_no_peers_since(→ None) at the top of theCoreBackendMode::Rpcbranch intrigger_refresh().app.rs: Add anelsebranch to clear the SPV degraded banner when not in SPV mode.Validation
reset()Follow-up from #658 which introduced the degraded peer detection and banner.
Summary by CodeRabbit