chore: update to latest platform SDK and dashcore - #806
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates the Changes
Sequence Diagram(s)sequenceDiagram
participant DashSpvClient
participant SpvEventBridge
participant SpvManager
participant ConnectionStatus
participant Channels as Finality & Reconcile<br/>Channels
DashSpvClient->>SpvEventBridge: on_sync_event(event)
SpvEventBridge->>SpvManager: Update status & progress
SpvEventBridge->>ConnectionStatus: Sync status changed
DashSpvClient->>SpvEventBridge: on_network_event(event)
SpvEventBridge->>SpvManager: Update connected_peers
SpvEventBridge->>ConnectionStatus: Peer count changed
DashSpvClient->>SpvEventBridge: on_wallet_event(InstantLock/ChainLock)
SpvEventBridge->>Channels: Send to finality channel
SpvEventBridge->>SpvManager: Trigger reconcile
DashSpvClient->>SpvEventBridge: on_progress(progress)
SpvEventBridge->>SpvManager: Update progress state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Review GateCommit:
|
![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)
src/spv/manager.rs (1)
1370-1390:⚠️ Potential issue | 🟠 MajorError messages include technical details that should be separated — use
BannerHandle::with_details()instead.The
SpvEventBridgeconstruction and graceful handling of poisoned locks are sound, but line 1390 (and similar patterns at lines 1358, 1368, 1416) violates the error message guideline. User-facing error messages must not include raw error details; these belong in the structured error chain viaBannerHandle::with_details(e).Update messages to:
- Line 1358:
"Failed to initialize SPV network manager."- Line 1368:
"Failed to initialize SPV storage."- Line 1390:
"Failed to create SPV client."Pass the error
eseparately to the error handling mechanism instead of embedding it in the message string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 1370 - 1390, Replace user-facing error strings that embed raw error details with calls that attach details via BannerHandle::with_details; specifically, where DashSpvClient::new(...).await.map_err(|e| format!("Failed to create SPV client: {e}")) is used, change the map_err to return the simple message "Failed to create SPV client." and pass the original error `e` into the structured error chain via BannerHandle::with_details(e) (do the same pattern for the similar map_err occurrences around initializing the SPV network manager and storage). Locate these by the SpvEventBridge construction and the DashSpvClient::new call and update the closure that currently formats the error string to separate the user message from the detailed error via BannerHandle::with_details.
🧹 Nitpick comments (2)
Cargo.toml (1)
21-30: Temporary branch reference — add a tracking comment.The dependency points to
refactor/sdk-rpitit-fetch-traits, a development branch. Per PR notes, this should switch tov3.1-devoncedashpay/platform#3376merges. Consider adding a comment above this dependency to track the planned migration:+# TODO: Switch to branch = "v3.1-dev" after dashpay/platform#3376 merges dash-sdk = { git = "https://github.com/dashpay/platform", branch = "refactor/sdk-rpitit-fetch-traits", features = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 21 - 30, Add a tracking comment above the dash-sdk dependency entry that notes the current branch refactor/sdk-rpitit-fetch-traits is temporary and should be switched to tag v3.1-dev (or the merged ref dashpay/platform#3376) once that upstream PR merges; reference the dependency name dash-sdk and the branch name in the comment so it’s clear to reviewers and future maintainers what to change and why.src/spv/manager.rs (1)
186-195: Potential event loss withtry_sendon full channel.When
finality_txbuffer (capacity 64) is full,try_sendfails and theInstantLockevent is dropped with only a warning logged. Per context snippet 1, the consumer uses blockingrecv(), so if it processes slowly during high activity, events could be lost.For asset lock finality events, this could delay proof construction. Consider:
- Increasing buffer size for
finality_tx, or- Using
send().awaitin a spawned task (though this is a sync callback), or- Documenting this as acceptable given the debounce pattern downstream.
If event loss is acceptable (e.g., the next event will trigger the same proof check), the current approach is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 186 - 195, The current use of ftx.try_send(AssetLockFinalityEvent::InstantLock { ... }) can drop events when the finality_tx buffer is full; replace the sync try_send call with an async send executed in a spawned task: capture/clone the needed values (instant_lock and ftx), tokio::spawn an async block that awaits ftx.send(AssetLockFinalityEvent::InstantLock { ... }).await and logs on Err, so the sync callback doesn't drop events but also doesn't block; alternatively, if you prefer the simpler change, increase the finality_tx buffer capacity where it's created to reduce contention (referencing finality_tx / ftx and AssetLockFinalityEvent::InstantLock / try_send).
🤖 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/spv/manager.rs`:
- Line 14: Move the use dash_sdk::dash_spv::client::EventHandler import so it is
grouped with the other dash_spv::client imports (or reorder the client imports
alphabetically) in src/spv/manager.rs to satisfy rustfmt; specifically locate
the EventHandler use and place it adjacent to other dash_spv::client use
statements (or alphabetize the dash_spv::client entries) so import ordering
matches project/style conventions.
---
Outside diff comments:
In `@src/spv/manager.rs`:
- Around line 1370-1390: Replace user-facing error strings that embed raw error
details with calls that attach details via BannerHandle::with_details;
specifically, where DashSpvClient::new(...).await.map_err(|e| format!("Failed to
create SPV client: {e}")) is used, change the map_err to return the simple
message "Failed to create SPV client." and pass the original error `e` into the
structured error chain via BannerHandle::with_details(e) (do the same pattern
for the similar map_err occurrences around initializing the SPV network manager
and storage). Locate these by the SpvEventBridge construction and the
DashSpvClient::new call and update the closure that currently formats the error
string to separate the user message from the detailed error via
BannerHandle::with_details.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 21-30: Add a tracking comment above the dash-sdk dependency entry
that notes the current branch refactor/sdk-rpitit-fetch-traits is temporary and
should be switched to tag v3.1-dev (or the merged ref dashpay/platform#3376)
once that upstream PR merges; reference the dependency name dash-sdk and the
branch name in the comment so it’s clear to reviewers and future maintainers
what to change and why.
In `@src/spv/manager.rs`:
- Around line 186-195: The current use of
ftx.try_send(AssetLockFinalityEvent::InstantLock { ... }) can drop events when
the finality_tx buffer is full; replace the sync try_send call with an async
send executed in a spawned task: capture/clone the needed values (instant_lock
and ftx), tokio::spawn an async block that awaits
ftx.send(AssetLockFinalityEvent::InstantLock { ... }).await and logs on Err, so
the sync callback doesn't drop events but also doesn't block; alternatively, if
you prefer the simpler change, increase the finality_tx buffer capacity where
it's created to reduce contention (referencing finality_tx / ftx and
AssetLockFinalityEvent::InstantLock / try_send).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b7944c9-0aef-40dc-9b44-569b86fd45f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/backend_task/core/mod.rssrc/spv/manager.rs
c91c140 to
0e4d55e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/spv/manager.rs`:
- Around line 315-336: The code only clears ConnectionStatus's last error but
leaves self.last_error populated, causing status()/status_async() and
SpvStatusSnapshot to report stale errors; update the same write guard used for
setting errors (self.last_error.write()) so that when !is_error you clear
*err_guard = None as well (in the block that currently sets
cs.set_spv_last_error(None) or when no error_msg), ensuring both
ConnectionStatus (cs.set_spv_last_error) and the internal last_error field are
cleared together; refer to last_error, connection_status, status(),
status_async(), SpvStatusSnapshot, and set_spv_last_error to locate and
implement the symmetric clear.
- Around line 1364-1373: SpvEventBridge currently clones reconcile_tx and
finality_tx once at construction so later calls to
register_reconcile_channel()/register_finality_channel() on SpvManager replace
the sender but the bridge keeps using the stale handles; change SpvEventBridge
to not freeze senders: have it hold the same shared Mutex/Arc used by SpvManager
(e.g., store Arc<Mutex<Option<...>>> or a reference to the
reconcile_tx/finality_tx Mutex) and, in the bridge's send code, lock and clone
the current sender each time before sending; update SpvEventBridge construction
to capture the shared Mutex/Arc (not a one-time clone) and adjust send paths
accordingly so subscribers registered after start() receive events.
- Around line 230-249: The UI-facing last_error strings currently include raw
SDK/implementation details (e.g., the manager name and the `error` text) —
change the code that writes `self.last_error` and calls
`cs.set_spv_last_error(Some(msg))` so the message is a generic, user-friendly
sentence (e.g., "SPV synchronization failed; see details for more information")
and do NOT include `manager`, the `error` variable, or any SPV internals; keep
full diagnostics only in logs (leave the `tracing::warn!` and include `error`
there) or attach details via BannerHandle::with_details(error) instead; apply
the same change for the similar occurrence referenced around 319-320.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: feb0a2ba-118f-4171-b3e5-3737c817c1a3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/backend_task/core/mod.rssrc/spv/manager.rs
✅ Files skipped from review due to trivial changes (2)
- src/backend_task/core/mod.rs
- Cargo.toml
0e4d55e to
1ba0995
Compare
shumkov
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/spv/manager.rs (2)
153-170:⚠️ Potential issue | 🟠 MajorSenders are frozen at construction time; late subscribers won't receive events.
SpvEventBridgecapturesreconcile_txandfinality_txby cloning them once when the client is built (lines 1370-1371). Ifregister_reconcile_channel()orregister_finality_channel()is called afterstart(), the new senders are stored inSpvManagerbut the running bridge continues using the stale handles.This matters because
spv_setup_finality_listener()inwallet_lifecycle.rscallsregister_finality_channel()— if this happens after the SPV client is already running, finality events (InstantLock/ChainLock) will never reach the listener.Consider having
SpvEventBridgehold references to the sharedMutex<Option<mpsc::Sender<...>>>and lock/clone the sender on each send, or ensure registration always happens beforestart().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 153 - 170, SpvEventBridge currently stores reconcile_tx and finality_tx as cloned Sender handles at construction, so late registrations (via SpvManager::register_reconcile_channel or register_finality_channel called after start(), e.g. spv_setup_finality_listener) are ignored; change SpvEventBridge to hold Arc<Mutex<Option<mpsc::Sender<()>>>> for reconcile_tx and Arc<Mutex<Option<mpsc::Sender<AssetLockFinalityEvent>>>> for finality_tx (or equivalent synchronized shared holder), update the bridge send paths to lock the mutex and clone the Option Sender for each send attempt (instead of using the original frozen clone), and ensure SpvManager::register_* methods update the shared Mutex<Option<...>> so new subscribers are visible to the running bridge; alternatively enforce registration before start() where appropriate (reference symbols: SpvEventBridge, reconcile_tx, finality_tx, SpvManager::register_reconcile_channel, SpvManager::register_finality_channel, start, spv_setup_finality_listener).
325-338:⚠️ Potential issue | 🟠 Major
self.last_erroris not cleared when progress recovers.Lines 334-336 clear
ConnectionStatus::set_spv_last_error(None)when!is_error, butself.last_error(theArc<RwLock<Option<String>>>shared withSpvManager) is not cleared. Sincestatus()andstatus_async()read fromself.last_error, theSpvStatusSnapshotwill continue reporting a stale error message even after SPV recovers to a healthy state.Add
*err_guard = Nonewhen!is_errorto keep both error stores in sync:Proposed fix
// Push to ConnectionStatus if let Some(cs) = &self.connection_status { if let Some(s) = new_status { cs.set_spv_status(s); } // Only update last_error when we have a new message to set, // or when syncing successfully (clear stale errors). if let Some(msg) = error_msg { cs.set_spv_last_error(Some(msg)); } else if !is_error { + // Clear internal last_error to match ConnectionStatus + if let Ok(mut err_guard) = self.last_error.write() { + *err_guard = None; + } cs.set_spv_last_error(None); } cs.refresh_state(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 325 - 338, The ConnectionStatus error is cleared via cs.set_spv_last_error(None) but the SpvManager's shared self.last_error (Arc<RwLock<Option<String>>>) is not updated, leaving status()/status_async() to return a stale error; fix by, in the same branch where !is_error is handled (near the cs.set_spv_last_error(None) call in the block that updates ConnectionStatus), acquire a write lock on self.last_error (e.g., let mut err_guard = self.last_error.write().await or write() depending on sync context) and set *err_guard = None so both the ConnectionStatus (set_spv_last_error) and SpvManager's self.last_error stay in sync.
🧹 Nitpick comments (1)
src/spv/manager.rs (1)
182-210: Silent event loss when finality channel is full.
try_send()drops events if the 64-message buffer is full, logging only a warning. Context snippet 2 shows the consumer holds locks and performs database lookups, which could cause backpressure. LostInstantLockorChainLockevents may delay or prevent asset lock proof construction.The current approach is reasonable given that blocking the SPV event loop would be worse, but consider increasing the buffer size or adding metrics/alerting for dropped events in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spv/manager.rs` around lines 182 - 210, The forwarder silently drops finality events when finality_tx.try_send(...) returns Err (channel full); update the handling in the SyncEvent match to increment a drop metric/emit an alert when TrySendError::Full occurs (so production can detect backpressure) and include the event type and identifier (e.g., InstantLock txid or ChainLock height) in the metric/log; additionally, wherever finality_tx is created (the channel allocator for finality_tx), increase the channel buffer from 64 to a larger size or make it configurable to reduce drops under backpressure from the consumer that holds DB locks.
🤖 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/spv/manager.rs`:
- Around line 153-170: SpvEventBridge currently stores reconcile_tx and
finality_tx as cloned Sender handles at construction, so late registrations (via
SpvManager::register_reconcile_channel or register_finality_channel called after
start(), e.g. spv_setup_finality_listener) are ignored; change SpvEventBridge to
hold Arc<Mutex<Option<mpsc::Sender<()>>>> for reconcile_tx and
Arc<Mutex<Option<mpsc::Sender<AssetLockFinalityEvent>>>> for finality_tx (or
equivalent synchronized shared holder), update the bridge send paths to lock the
mutex and clone the Option Sender for each send attempt (instead of using the
original frozen clone), and ensure SpvManager::register_* methods update the
shared Mutex<Option<...>> so new subscribers are visible to the running bridge;
alternatively enforce registration before start() where appropriate (reference
symbols: SpvEventBridge, reconcile_tx, finality_tx,
SpvManager::register_reconcile_channel, SpvManager::register_finality_channel,
start, spv_setup_finality_listener).
- Around line 325-338: The ConnectionStatus error is cleared via
cs.set_spv_last_error(None) but the SpvManager's shared self.last_error
(Arc<RwLock<Option<String>>>) is not updated, leaving status()/status_async() to
return a stale error; fix by, in the same branch where !is_error is handled
(near the cs.set_spv_last_error(None) call in the block that updates
ConnectionStatus), acquire a write lock on self.last_error (e.g., let mut
err_guard = self.last_error.write().await or write() depending on sync context)
and set *err_guard = None so both the ConnectionStatus (set_spv_last_error) and
SpvManager's self.last_error stay in sync.
---
Nitpick comments:
In `@src/spv/manager.rs`:
- Around line 182-210: The forwarder silently drops finality events when
finality_tx.try_send(...) returns Err (channel full); update the handling in the
SyncEvent match to increment a drop metric/emit an alert when TrySendError::Full
occurs (so production can detect backpressure) and include the event type and
identifier (e.g., InstantLock txid or ChainLock height) in the metric/log;
additionally, wherever finality_tx is created (the channel allocator for
finality_tx), increase the channel buffer from 64 to a larger size or make it
configurable to reduce drops under backpressure from the consumer that holds DB
locks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e84a817d-c124-4c09-b572-fe9b1b3e5b27
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/app.rssrc/backend_task/core/mod.rssrc/spv/manager.rs
✅ Files skipped from review due to trivial changes (1)
- src/backend_task/core/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
shumkov
changed the title
shumkov
changed the title
1ba0995 to
9dec367
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app.rs (1)
951-954: Stop after the first failed result send.In practice, once
sender.send(...)fails here, the receiver is already gone. Continuing the loop just repeats the same error for every remaining result and can spam shutdown logs for large batches.Suggested change
for result in results { if let Err(e) = sender.send(result.into()).await { tracing::error!("Failed to send task result: {}", e); + break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 951 - 954, The loop over results currently continues after sender.send(result.into()).await returns Err, causing repeated identical errors; update the loop in the block iterating `for result in results` so that when `sender.send(...).await` yields `Err(e)` you log the error and then immediately stop processing further results (e.g., break/return from the loop) instead of continuing; reference the `sender.send`, `results`, and `result.into()` call sites when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app.rs`:
- Around line 951-954: The loop over results currently continues after
sender.send(result.into()).await returns Err, causing repeated identical errors;
update the loop in the block iterating `for result in results` so that when
`sender.send(...).await` yields `Err(e)` you log the error and then immediately
stop processing further results (e.g., break/return from the loop) instead of
continuing; reference the `sender.send`, `results`, and `result.into()` call
sites when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b87e8d73-990f-4270-8324-8389cf94239d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/app.rssrc/backend_task/core/mod.rssrc/spv/manager.rs
✅ Files skipped from review due to trivial changes (2)
- src/backend_task/core/mod.rs
- Cargo.toml
1849221 to
1b5bdfc
Compare
fe06f3b to
f3ae7c4
Compare
Point dash-sdk at dashpay/platform v3.1-dev rev 94cefb30d9. Fix key_wallet_manager import path (no manager submodule). Keep spawn_blocking workaround for rust-lang/rust#96865. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f3ae7c4 to
98cff02
Compare
3 participants
Summary
Update to the recent SDK and dashcore versions.
Remove the
spawn_blocking + block_onworkaround fromhandle_backend_taskandhandle_backend_tasks. Replace with directtokio::spawn.This is possible because
dashpay/platform#3376replaces#[async_trait]with explicitBoxFutureon SDK traits, fixing the HRTB Send inference bug (rust-lang/rust#96865) that previously preventedtokio::spawnfrom compiling with 28+ Fetch impl types.Changes
src/app.rs:spawn_blocking + block_on->tokio::spawnsrc/backend_task/core/mod.rs: fixkey_wallet_managerimport pathsrc/spv/manager.rs: fixkey_wallet_managerimport path, clippy fixesCargo.toml: point dash-sdk at platformrefactor/sdk-rpitit-fetch-traitsbranchWhy this matters
The
spawn_blocking + block_onpattern:With
tokio::spawn, backend tasks run on the async executor efficiently.Depends on
dashpay/platform#3376must merge first, then this PR switches tov3.1-devSummary by CodeRabbit
Chores
Refactor