refactor: cleanup DashSpvClient to have single run() entry point#457
refactor: cleanup DashSpvClient to have single run() entry point#457xdustinface wants to merge 1 commit intov0.42-devfrom
DashSpvClient to have single run() entry point#457Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes remove the separate FFI Changes
Sequence DiagramsequenceDiagram
participant User
participant Client
participant SyncCoordinator
participant EventMonitor
participant SPVCore
Note over User,SPVCore: New Flow: Unified Run with Background Tasks
User->>Client: run(shutdown_token)
Client->>SyncCoordinator: subscribe/register callbacks
SyncCoordinator->>EventMonitor: register monitors
Client->>EventMonitor: spawn monitoring tasks
Client->>SPVCore: spawn background run(task, shutdown_token)
Client-->>User: return immediately
EventMonitor-->>SyncCoordinator: emit progress/network events
SPVCore-->>SyncCoordinator: sync updates
User->>Client: shutdown_token.cancel()
SPVCore->>Client: background task observes cancellation
Client->>SyncCoordinator: stop()/cleanup
SyncCoordinator-->>Client: stopped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
xdustinface
force-pushed
the
refactor/client-start-in-run
branch
from
ba63b6a to
1d325f3
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
refactor/client-start-in-run
branch
from
1d325f3 to
de756b8
Compare
github-actions
bot
removed
the
merge-conflict
xdustinface
force-pushed
the
refactor/client-start-in-run
branch
from
de756b8 to
4f782c0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
dash-spv-ffi/scripts/generate_ffi_docs.py (1)
145-146:⚠️ Potential issue | 🟡 Minor
dash_spv_ffi_client_runwill be miscategorized as "Utility Functions" instead of "Client Management".The condition checks for
client_startbut notclient_run. Since this PR removedclient_startand replaced it withclient_run, the new function won't match this filter.Proposed fix
- if 'client_new' in name or 'client_start' in name or 'client_stop' in name or 'client_destroy' in name: + if 'client_new' in name or 'client_run' in name or 'client_stop' in name or 'client_destroy' in name:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/scripts/generate_ffi_docs.py` around lines 145 - 146, The categorization check in generate_ffi_docs.py currently looks for 'client_new', 'client_start', 'client_stop', and 'client_destroy' and will miss the renamed function dash_spv_ffi_client_run; update the condition that appends to categories['Client Management'] to include 'client_run' (or better, match on the 'client_' prefix with the set of management verbs) so that functions like dash_spv_ffi_client_run are classified under "Client Management" (refer to the if condition that currently checks for 'client_new'/'client_start'/'client_stop'/'client_destroy' and the categories['Client Management'].append(func) call).dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
67-67:⚠️ Potential issue | 🟡 MinorStale comment still references
client_start.The
#[ignore]comments on lines 67, 111, and 185 still say "client_start hangs without peers" but the function is nowdash_spv_ffi_client_run. Update for consistency.Suggested fix
- #[ignore] // Requires network - client_start hangs without peers + #[ignore] // Requires network - client_run hangs without peersApply similarly on lines 111 and 185.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/tests/unit/test_client_lifecycle.rs` at line 67, Update the stale ignore comment text so it references the current function name: replace "client_start hangs without peers" with "dash_spv_ffi_client_run hangs without peers" in the three #[ignore] attributes in this test file (the ones currently annotating the tests around the client lifecycle checks), and apply the same replacement for the other two occurrences mentioned (the comments near the other #[ignore] attributes).dash-spv/tests/wallet_integration_test.rs (1)
22-22:⚠️ Potential issue | 🟠 MajorFix TempDir lifetime issue that may cause flaky tests.
TempDir::new().unwrap().path()creates a temporary that is dropped at the end of the statement, deleting the directory beforeDiskStorageManager::new()tries to create directories and open storage files. This can cause test failures depending on timing.Hold the
TempDirin a variable to keep it alive:Suggested fix
+ let temp_dir = TempDir::new().unwrap(); let config = ClientConfig::testnet() .without_filters() - .with_storage_path(TempDir::new().unwrap().path()) + .with_storage_path(temp_dir.path()) .without_masternodes();Then return or hold
temp_diralongside the client to keep it alive for the entire test duration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/wallet_integration_test.rs` at line 22, The TempDir created inline is dropped immediately causing its directory to be removed before DiskStorageManager::new() uses it; fix by creating and binding the TempDir to a variable (e.g., let temp_dir = TempDir::new().unwrap();) and pass temp_dir.path() into with_storage_path, then keep temp_dir alive for the test duration (either return it from the helper or store it alongside the client variable) so DiskStorageManager::new()/with_storage_path sees a valid directory throughout the test.dash-spv-ffi/src/client.rs (4)
415-436:⚠️ Potential issue | 🟡 Minor
dash_spv_ffi_client_clear_storagebypasses task cancellationThe function calls
client.inner.stop()directly without cancelling the shutdown token or aborting monitoring tasks. Active monitoring tasks will only exit when their channels close naturally (which happens as a side effect ofinner.stop()). More critically, once theis_runningguard proposed above is added, this function must also reset that flag; otherwise a subsequentrun()call afterclear_storagewill be rejected.Consider routing through
stop_client_internal(or an equivalent helper) to ensure consistent teardown:🛡️ Proposed fix
pub unsafe extern "C" fn dash_spv_ffi_client_clear_storage(client: *mut FFIDashSpvClient) -> i32 { null_check!(client); let client = &mut (*client); + + // Ensure all tasks are stopped before wiping storage + if let Err(e) = stop_client_internal(client) { + tracing::warn!("Failed to stop client before clearing storage: {e}"); + } let result = client.runtime.block_on(async { - if let Err(e) = client.inner.stop().await { - tracing::warn!("Failed to stop client before clearing storage: {}", e); - } client.inner.clear_storage().await });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 415 - 436, dash_spv_ffi_client_clear_storage currently calls client.inner.stop() directly which bypasses proper cancellation and does not reset the is_running guard; route teardown through the existing stop_client_internal (or create an equivalent helper) so it cancels the shutdown token, aborts/joins monitoring tasks, performs inner.stop(), and resets the is_running flag so a subsequent run() is allowed; update dash_spv_ffi_client_clear_storage to invoke that helper (and propagate errors via set_last_error/FFIErrorCode as before) instead of calling client.inner.stop() directly.
309-343:⚠️ Potential issue | 🟠 Major
.unwrap()on mutex locks insideunsafe extern "C"is undefined behaviour on panicA poisoned mutex (caused by a prior panic while the lock was held) will cause
unwrap()to panic. Panicking across an FFI boundary is UB in Rust. The same applies tocancel_active_tasks()(line 196) andstop_client_internal()(line 214).Replace each
.lock().unwrap()in FFI function bodies with an explicit error return:🛡️ Proposed fix (example for `dash_spv_ffi_client_run`)
- let mut tasks = client.active_tasks.lock().unwrap(); + let mut tasks = match client.active_tasks.lock() { + Ok(guard) => guard, + Err(e) => { + set_last_error(&format!("Internal mutex error: {e}")); + return FFIErrorCode::InternalError as i32; + } + }; - if client.sync_event_callbacks.lock().unwrap().is_some() { + if matches!(client.sync_event_callbacks.lock(), Ok(g) if g.is_some()) {Apply the same pattern to all
lock().unwrap()calls inextern "C"function bodies and incancel_active_tasks/stop_client_internal.As per coding guidelines: "Avoid
unwrap()andexpect()in library code" and "Exercise careful memory safety handling at FFI boundaries."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 309 - 343, The FFI functions (e.g., dash_spv_ffi_client_run and the helper functions cancel_active_tasks and stop_client_internal) currently call .lock().unwrap() on Mutexes (client.active_tasks, client.sync_event_callbacks, client.network_event_callbacks, client.progress_callback, client.wallet_event_callbacks) which can panic if the mutex is poisoned and thus cause undefined behavior across the FFI boundary; replace each .lock().unwrap() with explicit locking that handles PoisonError (e.g., match client.X.lock() { Ok(guard) => ..., Err(e) => return an appropriate error code/result to the caller } ), returning a failure code or error value from the extern "C" function instead of panicking, and apply the same pattern for cancel_active_tasks() and stop_client_internal() so no unwrap() can panic in any FFI-exposed path.
287-371:⚠️ Potential issue | 🔴 Critical
dash_spv_ffi_client_runhas no guard against double invocation — duplicate tasks will be spawnedIf called while already running, the function will:
- Subscribe to all event channels again (creating new receivers)
- Append another set of monitoring tasks to
active_tasks- Spawn a second
spv_client.run(shutdown_token)concurrent run loopThis results in duplicate event callbacks being fired and two concurrent sync loops operating on the same inner client, which can cause data races and undefined behaviour.
Add an
is_runningatomic flag to prevent re-entry:🔒 Proposed fix
Add to
FFIDashSpvClient:pub struct FFIDashSpvClient { pub(crate) inner: InnerClient, pub(crate) runtime: Arc<Runtime>, active_tasks: Mutex<Vec<JoinHandle<()>>>, shutdown_token: CancellationToken, + is_running: std::sync::atomic::AtomicBool, sync_event_callbacks: Arc<Mutex<Option<FFISyncEventCallbacks>>>, ... }Initialise in
dash_spv_ffi_client_new:let ffi_client = FFIDashSpvClient { inner: client, runtime, active_tasks: Mutex::new(Vec::new()), shutdown_token: CancellationToken::new(), + is_running: std::sync::atomic::AtomicBool::new(false), ... };Guard at the start of
dash_spv_ffi_client_run:+ use std::sync::atomic::Ordering; + if client.is_running + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { + set_last_error("Client is already running"); + return FFIErrorCode::AlreadyRunning as i32; // or suitable variant + }Clear in
stop_client_internal:+ use std::sync::atomic::Ordering; + client.is_running.store(false, Ordering::Release); client.shutdown_token = CancellationToken::new(); result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 287 - 371, dash_spv_ffi_client_run allows re-entry and can spawn duplicate tasks; add an AtomicBool flag is_running on FFIDashSpvClient, initialize it false in dash_spv_ffi_client_new, then at the top of dash_spv_ffi_client_run perform an atomic compare-and-swap (or swap with Ordering::SeqCst) to set is_running->true and return an error code if it was already true, proceed only if you successfully set it; ensure stop_client_internal (and any early-error paths from run) atomically clear is_running->false so subsequent runs are allowed. Use the FFIDashSpvClient type, dash_spv_ffi_client_run, dash_spv_ffi_client_new and stop_client_internal identifiers when applying the changes.
484-501:⚠️ Potential issue | 🟠 MajorAlign
dash_spv_ffi_client_destroyshutdown order withstop_client_internalfor consistency and efficiency.
stop_client_internal(line 213):cancel_token→abort_tasks→inner.stop()
dash_spv_ffi_client_destroy(line 484):cancel_token→inner.stop()→abort_tasksThe background
run()task (spawned at client.rs:356) listens to token cancellation and callsself.stop()when exiting (sync_coordinator.rs:59). Indestroy, this causesinner.stop()to be called twice: once by the exiting background task and once by the destroy function. Whilestop()is idempotent (checks running flag before proceeding), the double-call is inefficient. Reorderdestroyto abort tasks first, preventing the background task from callingstop(), consistent withstop_client_internal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 484 - 501, dash_spv_ffi_client_destroy currently cancels the shutdown token, then calls client.inner.stop() and only afterward aborts active tasks, causing the background run() task to race and call stop() again; reorder the shutdown to match stop_client_internal by: call client.shutdown_token.cancel(), then immediately call client.cancel_active_tasks() to abort/wait active background tasks, and only after tasks are aborted call client.runtime.block_on(async { let _ = client.inner.stop().await; }); ensure you reference the same symbols (dash_spv_ffi_client_destroy, client.shutdown_token.cancel(), client.cancel_active_tasks(), client.inner.stop()) when making the change.
🧹 Nitpick comments (2)
dash-spv-ffi/FFI_API.md (1)
125-125:dash_spv_ffi_client_runis categorized under "Utility Functions" instead of "Client Management".Since
runis the primary entry point for starting and syncing the client, it logically belongs alongsidenew,stop, anddestroyin the Client Management section. This would make the Client Management count 4 (matching the pre-refactor count). Since this doc is auto-generated, the fix would be in the generation script (dash-spv-ffi/scripts/generate_ffi_docs.py).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/FFI_API.md` at line 125, The table entry for `dash_spv_ffi_client_run` is currently placed under "Utility Functions" but should be in the "Client Management" section; update the documentation generation logic in the script generate_ffi_docs.py to classify `dash_spv_ffi_client_run` alongside `dash_spv_ffi_client_new`, `dash_spv_ffi_client_stop`, and `dash_spv_ffi_client_destroy` so the Client Management group contains four entries, ensuring the generator uses the `dash_spv_ffi_client_run` symbol (or its signature) to assign its category accordingly and regenerate the markdown.dash-spv-ffi/src/client.rs (1)
311-352: Unnecessaryresubscribe()— original receivers can be moved directly
sync_event_rx,network_event_rx, andwallet_event_rxare each used exactly once (only inside the conditional block) and are dropped at the end of the function. Calling.resubscribe()creates a new subscription starting at the current instant while the original receiver remains alive until the function returns, introducing a tiny window where events could be buffered in the original and silently dropped. Moving the original receiver into the spawn closure is both simpler and semantically correct.♻️ Proposed refactor (sync event example)
- if client.sync_event_callbacks.lock().unwrap().is_some() { + if matches!(client.sync_event_callbacks.lock(), Ok(g) if g.is_some()) { tasks.push(spawn_broadcast_monitor( "Sync event", - sync_event_rx.resubscribe(), + sync_event_rx, ... )); }Apply the same pattern to
network_event_rxandwallet_event_rx.Similarly, the
let mut receiver = receiver;rebinding on line 32 ofspawn_broadcast_monitoris unnecessary — the parameter itself can be declaredmut:-fn spawn_broadcast_monitor<E, C, F>( - name: &'static str, - receiver: broadcast::Receiver<E>, +fn spawn_broadcast_monitor<E, C, F>( + name: &'static str, + mut receiver: broadcast::Receiver<E>, ... ) -> JoinHandle<()> { - let mut receiver = receiver; rt.spawn(async move {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 311 - 352, Replace the unnecessary .resubscribe() calls by moving the original receivers into the spawned monitors: call spawn_broadcast_monitor with sync_event_rx, network_event_rx, and wallet_event_rx (not resubscribe()), and keep spawn_progress_monitor using progress_rx.clone() as before; this prevents the original receiver from lingering and losing events. Also remove the redundant rebinding in spawn_broadcast_monitor by making the receiver parameter mut (modify the spawn_broadcast_monitor signature to accept a mut receiver) and delete the `let mut receiver = receiver;` line so the function uses the parameter directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/README.md`:
- Around line 61-64: The README example uses DashSpvClient::new(config).await?
but the actual constructor requires additional arguments; update the example to
call DashSpvClient::new(config, network_manager, storage_manager, wallet).await?
(or, if you prefer the simplified snippet, add a short comment above the example
saying the real signature is DashSpvClient::new(config, network_manager,
storage_manager, wallet) and the example omits dependency setup for brevity) to
match the real DashSpvClient::new signature and avoid confusion.
In `@dash-spv/tests/peer_test.rs`:
- Around line 63-64: The teardown currently calls cancel.cancel();
handle.await.unwrap().unwrap(); which lets any SpvError returned by run() turn
cancellation races into test failures; update each occurrence (lines with
handle.await.unwrap().unwrap()) to await the task, unwrap the JoinHandle result,
then match the inner Result from run() and treat shutdown/cancellation-related
SpvError variants (e.g., SpvError::Shutdown or the network-disconnect variant
your code uses) as non-fatal (ignore/ok) while still propagating other errors as
panics. Locate the uses by the variables cancel and handle and the run()
function/SpvError type and replace the double unwrap with an explicit match that
only fails the test on true unexpected errors.
---
Outside diff comments:
In `@dash-spv-ffi/scripts/generate_ffi_docs.py`:
- Around line 145-146: The categorization check in generate_ffi_docs.py
currently looks for 'client_new', 'client_start', 'client_stop', and
'client_destroy' and will miss the renamed function dash_spv_ffi_client_run;
update the condition that appends to categories['Client Management'] to include
'client_run' (or better, match on the 'client_' prefix with the set of
management verbs) so that functions like dash_spv_ffi_client_run are classified
under "Client Management" (refer to the if condition that currently checks for
'client_new'/'client_start'/'client_stop'/'client_destroy' and the
categories['Client Management'].append(func) call).
In `@dash-spv-ffi/src/client.rs`:
- Around line 415-436: dash_spv_ffi_client_clear_storage currently calls
client.inner.stop() directly which bypasses proper cancellation and does not
reset the is_running guard; route teardown through the existing
stop_client_internal (or create an equivalent helper) so it cancels the shutdown
token, aborts/joins monitoring tasks, performs inner.stop(), and resets the
is_running flag so a subsequent run() is allowed; update
dash_spv_ffi_client_clear_storage to invoke that helper (and propagate errors
via set_last_error/FFIErrorCode as before) instead of calling
client.inner.stop() directly.
- Around line 309-343: The FFI functions (e.g., dash_spv_ffi_client_run and the
helper functions cancel_active_tasks and stop_client_internal) currently call
.lock().unwrap() on Mutexes (client.active_tasks, client.sync_event_callbacks,
client.network_event_callbacks, client.progress_callback,
client.wallet_event_callbacks) which can panic if the mutex is poisoned and thus
cause undefined behavior across the FFI boundary; replace each .lock().unwrap()
with explicit locking that handles PoisonError (e.g., match client.X.lock() {
Ok(guard) => ..., Err(e) => return an appropriate error code/result to the
caller } ), returning a failure code or error value from the extern "C" function
instead of panicking, and apply the same pattern for cancel_active_tasks() and
stop_client_internal() so no unwrap() can panic in any FFI-exposed path.
- Around line 287-371: dash_spv_ffi_client_run allows re-entry and can spawn
duplicate tasks; add an AtomicBool flag is_running on FFIDashSpvClient,
initialize it false in dash_spv_ffi_client_new, then at the top of
dash_spv_ffi_client_run perform an atomic compare-and-swap (or swap with
Ordering::SeqCst) to set is_running->true and return an error code if it was
already true, proceed only if you successfully set it; ensure
stop_client_internal (and any early-error paths from run) atomically clear
is_running->false so subsequent runs are allowed. Use the FFIDashSpvClient type,
dash_spv_ffi_client_run, dash_spv_ffi_client_new and stop_client_internal
identifiers when applying the changes.
- Around line 484-501: dash_spv_ffi_client_destroy currently cancels the
shutdown token, then calls client.inner.stop() and only afterward aborts active
tasks, causing the background run() task to race and call stop() again; reorder
the shutdown to match stop_client_internal by: call
client.shutdown_token.cancel(), then immediately call
client.cancel_active_tasks() to abort/wait active background tasks, and only
after tasks are aborted call client.runtime.block_on(async { let _ =
client.inner.stop().await; }); ensure you reference the same symbols
(dash_spv_ffi_client_destroy, client.shutdown_token.cancel(),
client.cancel_active_tasks(), client.inner.stop()) when making the change.
In `@dash-spv-ffi/tests/unit/test_client_lifecycle.rs`:
- Line 67: Update the stale ignore comment text so it references the current
function name: replace "client_start hangs without peers" with
"dash_spv_ffi_client_run hangs without peers" in the three #[ignore] attributes
in this test file (the ones currently annotating the tests around the client
lifecycle checks), and apply the same replacement for the other two occurrences
mentioned (the comments near the other #[ignore] attributes).
In `@dash-spv/tests/wallet_integration_test.rs`:
- Line 22: The TempDir created inline is dropped immediately causing its
directory to be removed before DiskStorageManager::new() uses it; fix by
creating and binding the TempDir to a variable (e.g., let temp_dir =
TempDir::new().unwrap();) and pass temp_dir.path() into with_storage_path, then
keep temp_dir alive for the test duration (either return it from the helper or
store it alongside the client variable) so
DiskStorageManager::new()/with_storage_path sees a valid directory throughout
the test.
---
Nitpick comments:
In `@dash-spv-ffi/FFI_API.md`:
- Line 125: The table entry for `dash_spv_ffi_client_run` is currently placed
under "Utility Functions" but should be in the "Client Management" section;
update the documentation generation logic in the script generate_ffi_docs.py to
classify `dash_spv_ffi_client_run` alongside `dash_spv_ffi_client_new`,
`dash_spv_ffi_client_stop`, and `dash_spv_ffi_client_destroy` so the Client
Management group contains four entries, ensuring the generator uses the
`dash_spv_ffi_client_run` symbol (or its signature) to assign its category
accordingly and regenerate the markdown.
In `@dash-spv-ffi/src/client.rs`:
- Around line 311-352: Replace the unnecessary .resubscribe() calls by moving
the original receivers into the spawned monitors: call spawn_broadcast_monitor
with sync_event_rx, network_event_rx, and wallet_event_rx (not resubscribe()),
and keep spawn_progress_monitor using progress_rx.clone() as before; this
prevents the original receiver from lingering and losing events. Also remove the
redundant rebinding in spawn_broadcast_monitor by making the receiver parameter
mut (modify the spawn_broadcast_monitor signature to accept a mut receiver) and
delete the `let mut receiver = receiver;` line so the function uses the
parameter directly.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv-ffi/FFI_API.md (1)
960-967:⚠️ Potential issue | 🟡 MinorStale callback example references non-existent
dash_spv_ffi_client_set_event_callbacks.The Event Callbacks example on line 967 calls
dash_spv_ffi_client_set_event_callbackswith anFFIEventCallbacksstruct, but the current API uses separate setters (set_sync_event_callbacks,set_network_event_callbacks,set_wallet_event_callbacks) with their respective types. This predates the PR but is user-facing documentation that could confuse consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/FFI_API.md` around lines 960 - 967, The example uses a stale helper dash_spv_ffi_client_set_event_callbacks and FFIEventCallbacks; update the example to call the current per-domain setters instead: replace the single FFIEventCallbacks usage with the appropriate structs (e.g. FFIClientSyncEventCallbacks, FFIClientNetworkEventCallbacks, FFIClientWalletEventCallbacks) and call the corresponding APIs set_sync_event_callbacks, set_network_event_callbacks, and set_wallet_event_callbacks on the client, wiring the on_block/on_transaction handlers into the correct struct fields and passing the client and struct to each setter.
🧹 Nitpick comments (5)
dash-spv-ffi/src/client.rs (2)
354-364: Backgroundrun()errors are only logged, not surfaced to the caller.Errors from
spv_client.run(shutdown_token)on line 359 are logged viatracing::error!but never propagated to the FFI caller. Sincedash_spv_ffi_client_runreturns immediately (fire-and-forget), this is the expected design — but callers have no way to learn about fatal runtime errors asynchronously other than through event callbacks. Worth a brief doc note if not already covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 354 - 364, The spawned sync task currently logs errors from spv_client.run(shutdown_token) but never surfaces them to the FFI caller; update the task started in client.runtime.spawn to propagate fatal run() errors via the existing FFI event/error path (e.g., send an Error/RuntimeFailure event through the client's event sender or a shared error channel) and ensure dash_spv_ffi_client_run's docs note that async runtime errors are reported via that event callback; locate the task by the closure that calls spv_client.run, use the client's event sender (or add one) to send a serialized error (including context) instead of only tracing::error!, and update any docs/comments for dash_spv_ffi_client_run to explain the async error reporting mechanism.
287-371: No guard against callingrun()more than once withoutstop()in between.If a caller invokes
dash_spv_ffi_client_runtwice, a second set of monitoring tasks and a secondspv_client.run(shutdown_token)are spawned concurrently with the first, leading to duplicate event dispatching and unpredictable behavior. Consider adding a simple "running" check (e.g., anAtomicBool) and returning an error code if already running.♻️ Sketch of a guard
Add an
is_running: AtomicBoolfield toFFIDashSpvClient, then at the top ofdash_spv_ffi_client_run:+ use std::sync::atomic::{AtomicBool, Ordering}; + // In FFIDashSpvClient struct: + // is_running: AtomicBool, + let client = &(*client); + + if client.is_running.swap(true, Ordering::SeqCst) { + set_last_error("Client is already running"); + return FFIErrorCode::InvalidState as i32; + }Reset it in
stop_client_internal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 287 - 371, Add a running guard to prevent double runs: add an is_running: AtomicBool field to FFIDashSpvClient, then at the top of dash_spv_ffi_client_run check and set it atomically (e.g., compare_exchange(false, true)) and return a suitable FFI error code if it was already true; ensure you clear/reset is_running to false in stop_client_internal (and on any early failure paths) so the client can be started again. Use a consistent memory ordering (e.g., Ordering::SeqCst) and reference FFIDashSpvClient, dash_spv_ffi_client_run, stop_client_internal, and is_running in your changes.dash-spv/src/client/sync_coordinator.rs (1)
25-26: Note: The previous learning about internal Ctrl-C handling is now outdated.Based on learnings, "The DashSpvClient::run method in dash-spv internally handles Ctrl-C shutdown by spawning a task that listens for tokio::signal::ctrl_c()". This PR intentionally moves Ctrl-C handling out of the client (as seen in
dash-spv/src/main.rslines 327-336 where it's now the caller's responsibility). The learning should be updated to reflect that callers must cancel the token themselves (e.g., on Ctrl-C).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/client/sync_coordinator.rs` around lines 25 - 26, Update the documentation/comments for DashSpvClient::run (the async run(&self, token: CancellationToken) method in sync_coordinator.rs) to remove any mention of internal Ctrl-C handling and explicitly state that run does not capture or handle Ctrl-C; callers are responsible for cancelling the provided CancellationToken (for example by listening for tokio::signal::ctrl_c() in main). Also verify any README or inline docs referencing this behavior are changed to reflect that shutdown must be initiated by the caller.dash-spv/src/main.rs (1)
330-331: Consider logging the Ctrl-C registration error instead of silently discarding it.If
tokio::signal::ctrl_c()fails (e.g., signal registration unavailable),.ok()silently drops the error. The task then callsctrl_c_token.cancel()after the select, which is fine, but the actual OS signal will never be received. A brief warning log would aid diagnostics.🔧 Proposed improvement
- result.ok(); - tracing::debug!("Shutdown signal received"); + if let Err(e) = result { + tracing::warn!("Failed to listen for Ctrl-C signal: {}", e); + } else { + tracing::debug!("Shutdown signal received"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/main.rs` around lines 330 - 331, Replace the silent discard of the ctrl-c registration error: where the select arm handles result = tokio::signal::ctrl_c() and currently calls result.ok(), change it to explicitly handle the Err case by logging a warning including the error details (e.g., via the project logger) before proceeding to call ctrl_c_token.cancel(); keep the existing flow for the Ok case but ensure any Err is surfaced in the logs so failures to register the OS signal are visible.dash-spv/tests/wallet_integration_test.rs (1)
65-65: Use.expect()for both unwraps to get actionable failure messages.🔧 Proposed improvement
- handle.await.unwrap().unwrap(); + handle.await.expect("run task panicked").expect("client.run returned an error");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/wallet_integration_test.rs` at line 65, Replace the double unwrap on the task handle to provide actionable failure messages: instead of calling handle.await.unwrap().unwrap(), call handle.await.expect("task join panicked in test").expect("wallet operation returned error") so the outer .await join error and the inner result both yield clear messages; locate the expression using handle.await.unwrap().unwrap() in the test and swap each unwrap for an expect with a short, specific message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dash-spv-ffi/FFI_API.md`:
- Around line 960-967: The example uses a stale helper
dash_spv_ffi_client_set_event_callbacks and FFIEventCallbacks; update the
example to call the current per-domain setters instead: replace the single
FFIEventCallbacks usage with the appropriate structs (e.g.
FFIClientSyncEventCallbacks, FFIClientNetworkEventCallbacks,
FFIClientWalletEventCallbacks) and call the corresponding APIs
set_sync_event_callbacks, set_network_event_callbacks, and
set_wallet_event_callbacks on the client, wiring the on_block/on_transaction
handlers into the correct struct fields and passing the client and struct to
each setter.
---
Nitpick comments:
In `@dash-spv-ffi/src/client.rs`:
- Around line 354-364: The spawned sync task currently logs errors from
spv_client.run(shutdown_token) but never surfaces them to the FFI caller; update
the task started in client.runtime.spawn to propagate fatal run() errors via the
existing FFI event/error path (e.g., send an Error/RuntimeFailure event through
the client's event sender or a shared error channel) and ensure
dash_spv_ffi_client_run's docs note that async runtime errors are reported via
that event callback; locate the task by the closure that calls spv_client.run,
use the client's event sender (or add one) to send a serialized error (including
context) instead of only tracing::error!, and update any docs/comments for
dash_spv_ffi_client_run to explain the async error reporting mechanism.
- Around line 287-371: Add a running guard to prevent double runs: add an
is_running: AtomicBool field to FFIDashSpvClient, then at the top of
dash_spv_ffi_client_run check and set it atomically (e.g.,
compare_exchange(false, true)) and return a suitable FFI error code if it was
already true; ensure you clear/reset is_running to false in stop_client_internal
(and on any early failure paths) so the client can be started again. Use a
consistent memory ordering (e.g., Ordering::SeqCst) and reference
FFIDashSpvClient, dash_spv_ffi_client_run, stop_client_internal, and is_running
in your changes.
In `@dash-spv/src/client/sync_coordinator.rs`:
- Around line 25-26: Update the documentation/comments for DashSpvClient::run
(the async run(&self, token: CancellationToken) method in sync_coordinator.rs)
to remove any mention of internal Ctrl-C handling and explicitly state that run
does not capture or handle Ctrl-C; callers are responsible for cancelling the
provided CancellationToken (for example by listening for tokio::signal::ctrl_c()
in main). Also verify any README or inline docs referencing this behavior are
changed to reflect that shutdown must be initiated by the caller.
In `@dash-spv/src/main.rs`:
- Around line 330-331: Replace the silent discard of the ctrl-c registration
error: where the select arm handles result = tokio::signal::ctrl_c() and
currently calls result.ok(), change it to explicitly handle the Err case by
logging a warning including the error details (e.g., via the project logger)
before proceeding to call ctrl_c_token.cancel(); keep the existing flow for the
Ok case but ensure any Err is surfaced in the logs so failures to register the
OS signal are visible.
In `@dash-spv/tests/wallet_integration_test.rs`:
- Line 65: Replace the double unwrap on the task handle to provide actionable
failure messages: instead of calling handle.await.unwrap().unwrap(), call
handle.await.expect("task join panicked in test").expect("wallet operation
returned error") so the outer .await join error and the inner result both yield
clear messages; locate the expression using handle.await.unwrap().unwrap() in
the test and swap each unwrap for an expect with a short, specific message.
Combine `start()` + `monitor_network()` + `stop()` into a single `run(token)` entry point. Callers no longer need to call `start()` separately since `run()` handles the full lifecycle and returns after the token is cancelled. - Make `start()` an internal function - Remove `monitor_network()` its loop is now inside `run()` - Remove `dash_spv_ffi_client_start` so that `dash_spv_ffi_client_run` is the only FFI entry point - Move ctrl-c handling out of the client
xdustinface
force-pushed
the
refactor/client-start-in-run
branch
from
4f782c0 to
dfb0355
Compare
1 participant
Combine
start()+monitor_network()+stop()into a singlerun(token)entry point. Callers no longer need to callstart()separately sincerun()handles the full lifecycle and returns after the token is cancelled.start()an internal functionmonitor_network()its loop is now insiderun()dash_spv_ffi_client_startso thatdash_spv_ffi_client_runis the only FFI entry pointBased on:
DashSpvClientcloneable #453Summary by CodeRabbit