From b7eda1cffa7caea8275c2001edf42c37a2736dfc Mon Sep 17 00:00:00 2001 From: xdustinface Date: Wed, 18 Feb 2026 20:42:01 +0100 Subject: [PATCH 1/2] refactor: cleanup `DashSpvClient` to have single `run()` entry point 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 --- dash-spv-ffi/FFI_API.md | 27 ++------- dash-spv-ffi/README.md | 6 +- dash-spv-ffi/include/dash_spv_ffi.h | 23 ++------ dash-spv-ffi/scripts/generate_ffi_docs.py | 4 +- dash-spv-ffi/src/client.rs | 55 ++++--------------- dash-spv-ffi/tests/c_tests/test_basic.c | 2 +- dash-spv-ffi/tests/test_client.rs | 4 +- .../tests/unit/test_async_operations.rs | 2 +- .../tests/unit/test_client_lifecycle.rs | 12 ++-- dash-spv/README.md | 10 +--- dash-spv/examples/filter_sync.rs | 3 - dash-spv/examples/simple_sync.rs | 3 - dash-spv/examples/spv_with_wallet.rs | 4 -- dash-spv/src/client/lifecycle.rs | 2 +- dash-spv/src/client/sync_coordinator.rs | 52 +++--------------- dash-spv/src/lib.rs | 4 +- dash-spv/src/main.rs | 20 ++++--- dash-spv/tests/peer_test.rs | 28 +++++++--- dash-spv/tests/wallet_integration_test.rs | 30 ++++++---- 19 files changed, 100 insertions(+), 191 deletions(-) diff --git a/dash-spv-ffi/FFI_API.md b/dash-spv-ffi/FFI_API.md index 4dbda39c0..95ef02125 100644 --- a/dash-spv-ffi/FFI_API.md +++ b/dash-spv-ffi/FFI_API.md @@ -4,7 +4,7 @@ This document provides a comprehensive reference for all FFI (Foreign Function I **Auto-generated**: This documentation is automatically generated from the source code. Do not edit manually. -**Total Functions**: 49 +**Total Functions**: 48 ## Table of Contents @@ -22,13 +22,12 @@ This document provides a comprehensive reference for all FFI (Foreign Function I ### Client Management -Functions: 4 +Functions: 3 | Function | Description | Module | |----------|-------------|--------| | `dash_spv_ffi_client_destroy` | Destroy the client and free associated resources | client | | `dash_spv_ffi_client_new` | Create a new SPV client and return an opaque pointer | client | -| `dash_spv_ffi_client_start` | Start the SPV client | client | | `dash_spv_ffi_client_stop` | Stop the SPV client | client | ### Configuration @@ -168,22 +167,6 @@ Create a new SPV client and return an opaque pointer. # Safety - `config` must --- -#### `dash_spv_ffi_client_start` - -```c -dash_spv_ffi_client_start(client: *mut FFIDashSpvClient) -> i32 -``` - -**Description:** -Start the SPV client. # Safety - `client` must be a valid, non-null pointer to a created client. - -**Safety:** -- `client` must be a valid, non-null pointer to a created client. - -**Module:** `client` - ---- - #### `dash_spv_ffi_client_stop` ```c @@ -791,7 +774,7 @@ dash_spv_ffi_client_run(client: *mut FFIDashSpvClient) -> i32 ``` **Description:** -Start the SPV client and begin syncing in the background. This is the streamlined entry point that combines `start()` and continuous monitoring into a single non-blocking call. Use event callbacks (set via `set_sync_event_callbacks`, `set_network_event_callbacks`, `set_wallet_event_callbacks`) to receive notifications about sync progress, peer connections, and wallet activity. Workflow: 1. Configure event callbacks before calling `run()` 2. Call `run()` - it returns immediately after spawning background tasks 3. Receive notifications via callbacks as sync progresses 4. Call `stop()` when done # Safety - `client` must be a valid, non-null pointer to a created client. # Returns 0 on success, error code on failure. +Start the SPV client and begin syncing in the background. Subscribes to events, spawns monitoring threads, then spawns a background thread that calls `run()` (which handles start + sync loop + stop internally). Returns immediately after spawning. Use event callbacks (set via `set_sync_event_callbacks`, `set_network_event_callbacks`, `set_wallet_event_callbacks`) to receive notifications. Configure callbacks before calling `run()`. # Safety - `client` must be a valid, non-null pointer to a created client. # Returns 0 on success, error code on failure. **Safety:** - `client` must be a valid, non-null pointer to a created client. @@ -946,8 +929,8 @@ FFIClientConfig* config = dash_spv_ffi_config_testnet(); // Create client FFIDashSpvClient* client = dash_spv_ffi_client_new(config); -// Start the client -int32_t result = dash_spv_ffi_client_start(client); +// Start the client and begin syncing +int32_t result = dash_spv_ffi_client_run(client); if (result != 0) { const char* error = dash_spv_ffi_get_last_error(); // Handle error diff --git a/dash-spv-ffi/README.md b/dash-spv-ffi/README.md index 29c7c5af4..74d4e49f9 100644 --- a/dash-spv-ffi/README.md +++ b/dash-spv-ffi/README.md @@ -60,8 +60,8 @@ if (client == NULL) { // Handle error } -// Start the client -if (dash_spv_ffi_client_start(client) != 0) { +// Start the client and begin syncing in the background +if (dash_spv_ffi_client_run(client) != 0) { // Handle error } @@ -88,7 +88,7 @@ dash_spv_ffi_config_destroy(config); ### Client Operations - `dash_spv_ffi_client_new(config)` - Create new client -- `dash_spv_ffi_client_start(client)` - Start the client +- `dash_spv_ffi_client_run(client)` - Start the client and begin syncing in the background - `dash_spv_ffi_client_stop(client)` - Stop the client - `dash_spv_ffi_client_get_sync_progress(client)` - Get sync progress - `dash_spv_ffi_client_get_stats(client)` - Get client statistics diff --git a/dash-spv-ffi/include/dash_spv_ffi.h b/dash-spv-ffi/include/dash_spv_ffi.h index bf38b6e04..b9599e917 100644 --- a/dash-spv-ffi/include/dash_spv_ffi.h +++ b/dash-spv-ffi/include/dash_spv_ffi.h @@ -468,14 +468,6 @@ int32_t dash_spv_ffi_client_update_config(struct FFIDashSpvClient *client, const struct FFIClientConfig *config) ; -/** - * Start the SPV client. - * - * # Safety - * - `client` must be a valid, non-null pointer to a created client. - */ - int32_t dash_spv_ffi_client_start(struct FFIDashSpvClient *client) ; - /** * Stop the SPV client. * @@ -487,16 +479,13 @@ int32_t dash_spv_ffi_client_update_config(struct FFIDashSpvClient *client, /** * Start the SPV client and begin syncing in the background. * - * This is the streamlined entry point that combines `start()` and continuous monitoring - * into a single non-blocking call. Use event callbacks (set via `set_sync_event_callbacks`, - * `set_network_event_callbacks`, `set_wallet_event_callbacks`) to receive notifications - * about sync progress, peer connections, and wallet activity. + * Subscribes to events, spawns monitoring threads, then spawns a background + * thread that calls `run()` (which handles start + sync loop + stop internally). + * Returns immediately after spawning. * - * Workflow: - * 1. Configure event callbacks before calling `run()` - * 2. Call `run()` - it returns immediately after spawning background tasks - * 3. Receive notifications via callbacks as sync progresses - * 4. Call `stop()` when done + * Use event callbacks (set via `set_sync_event_callbacks`, + * `set_network_event_callbacks`, `set_wallet_event_callbacks`) to receive + * notifications. Configure callbacks before calling `run()`. * * # Safety * - `client` must be a valid, non-null pointer to a created client. diff --git a/dash-spv-ffi/scripts/generate_ffi_docs.py b/dash-spv-ffi/scripts/generate_ffi_docs.py index da1ea462c..784844fad 100755 --- a/dash-spv-ffi/scripts/generate_ffi_docs.py +++ b/dash-spv-ffi/scripts/generate_ffi_docs.py @@ -308,8 +308,8 @@ def generate_markdown(functions: List[FFIFunction]) -> str: md.append("// Create client") md.append("FFIDashSpvClient* client = dash_spv_ffi_client_new(config);") md.append("") - md.append("// Start the client") - md.append("int32_t result = dash_spv_ffi_client_start(client);") + md.append("// Start the client and begin syncing") + md.append("int32_t result = dash_spv_ffi_client_run(client);") md.append("if (result != 0) {") md.append(" const char* error = dash_spv_ffi_get_last_error();") md.append(" // Handle error") diff --git a/dash-spv-ffi/src/client.rs b/dash-spv-ffi/src/client.rs index e3703a2af..2c0ddbdb9 100644 --- a/dash-spv-ffi/src/client.rs +++ b/dash-spv-ffi/src/client.rs @@ -250,27 +250,6 @@ pub unsafe extern "C" fn dash_spv_ffi_client_update_config( } } -/// Start the SPV client. -/// -/// # Safety -/// - `client` must be a valid, non-null pointer to a created client. -#[no_mangle] -pub unsafe extern "C" fn dash_spv_ffi_client_start(client: *mut FFIDashSpvClient) -> i32 { - null_check!(client); - - let client = &(*client); - - let result = client.runtime.block_on(async { client.inner.start().await }); - - match result { - Ok(()) => FFIErrorCode::Success as i32, - Err(e) => { - set_last_error(&e.to_string()); - FFIErrorCode::from(e) as i32 - } - } -} - /// Stop the SPV client. /// /// # Safety @@ -291,16 +270,13 @@ pub unsafe extern "C" fn dash_spv_ffi_client_stop(client: *mut FFIDashSpvClient) /// Start the SPV client and begin syncing in the background. /// -/// This is the streamlined entry point that combines `start()` and continuous monitoring -/// into a single non-blocking call. Use event callbacks (set via `set_sync_event_callbacks`, -/// `set_network_event_callbacks`, `set_wallet_event_callbacks`) to receive notifications -/// about sync progress, peer connections, and wallet activity. +/// Subscribes to events, spawns monitoring threads, then spawns a background +/// thread that calls `run()` (which handles start + sync loop + stop internally). +/// Returns immediately after spawning. /// -/// Workflow: -/// 1. Configure event callbacks before calling `run()` -/// 2. Call `run()` - it returns immediately after spawning background tasks -/// 3. Receive notifications via callbacks as sync progresses -/// 4. Call `stop()` when done +/// Use event callbacks (set via `set_sync_event_callbacks`, +/// `set_network_event_callbacks`, `set_wallet_event_callbacks`) to receive +/// notifications. Configure callbacks before calling `run()`. /// /// # Safety /// - `client` must be a valid, non-null pointer to a created client. @@ -313,18 +289,7 @@ pub unsafe extern "C" fn dash_spv_ffi_client_run(client: *mut FFIDashSpvClient) let client = &(*client); - tracing::info!("dash_spv_ffi_client_run: starting client"); - - // Start the client first - let start_result = client.runtime.block_on(async { client.inner.start().await }); - - if let Err(e) = start_result { - tracing::error!("dash_spv_ffi_client_run: start failed: {}", e); - set_last_error(&e.to_string()); - return FFIErrorCode::from(e) as i32; - } - - tracing::info!("dash_spv_ffi_client_run: client started, setting up event monitoring"); + tracing::info!("dash_spv_ffi_client_run: setting up event monitoring"); let shutdown_token = client.shutdown_token.clone(); @@ -389,10 +354,10 @@ pub unsafe extern "C" fn dash_spv_ffi_client_run(client: *mut FFIDashSpvClient) // Spawn the sync monitoring task let spv_client = client.inner.clone(); tasks.push(client.runtime.spawn(async move { - tracing::debug!("Sync task: starting monitor_network"); + tracing::debug!("Sync task: starting run"); - if let Err(e) = spv_client.monitor_network(shutdown_token).await { - tracing::error!("Sync task: sync error: {}", e); + if let Err(e) = spv_client.run(shutdown_token).await { + tracing::error!("Sync task: error: {}", e); } tracing::debug!("Sync task: exiting"); diff --git a/dash-spv-ffi/tests/c_tests/test_basic.c b/dash-spv-ffi/tests/c_tests/test_basic.c index ad451e8d4..2de817012 100644 --- a/dash-spv-ffi/tests/c_tests/test_basic.c +++ b/dash-spv-ffi/tests/c_tests/test_basic.c @@ -228,7 +228,7 @@ void test_null_pointer_handling() { // Client functions TEST_ASSERT(dash_spv_ffi_client_new(NULL) == NULL); - TEST_ASSERT(dash_spv_ffi_client_start(NULL) == FFIErrorCode_NullPointer); + TEST_ASSERT(dash_spv_ffi_client_run(NULL) == FFIErrorCode_NullPointer); TEST_ASSERT(dash_spv_ffi_client_stop(NULL) == FFIErrorCode_NullPointer); // Destruction functions (should handle NULL gracefully) diff --git a/dash-spv-ffi/tests/test_client.rs b/dash-spv-ffi/tests/test_client.rs index f5b2e49d6..61ca469b3 100644 --- a/dash-spv-ffi/tests/test_client.rs +++ b/dash-spv-ffi/tests/test_client.rs @@ -56,7 +56,7 @@ mod tests { let client = dash_spv_ffi_client_new(config); // Note: Start/stop may fail in test environment without network - let _result = dash_spv_ffi_client_start(client); + let _result = dash_spv_ffi_client_run(client); let _result = dash_spv_ffi_client_stop(client); dash_spv_ffi_client_destroy(client); @@ -68,7 +68,7 @@ mod tests { #[serial] fn test_client_null_checks() { unsafe { - let result = dash_spv_ffi_client_start(std::ptr::null_mut()); + let result = dash_spv_ffi_client_run(std::ptr::null_mut()); assert_eq!(result, FFIErrorCode::NullPointer as i32); let result = dash_spv_ffi_client_stop(std::ptr::null_mut()); diff --git a/dash-spv-ffi/tests/unit/test_async_operations.rs b/dash-spv-ffi/tests/unit/test_async_operations.rs index 14c974ba6..e28a513ec 100644 --- a/dash-spv-ffi/tests/unit/test_async_operations.rs +++ b/dash-spv-ffi/tests/unit/test_async_operations.rs @@ -114,7 +114,7 @@ mod tests { println!("Testing callback thread safety with concurrent invocations"); // Start the client - let start_result = dash_spv_ffi_client_start(client); + let start_result = dash_spv_ffi_client_run(client); assert_eq!(start_result, 0); thread::sleep(Duration::from_millis(100)); diff --git a/dash-spv-ffi/tests/unit/test_client_lifecycle.rs b/dash-spv-ffi/tests/unit/test_client_lifecycle.rs index d79fe333a..6bda25e8f 100644 --- a/dash-spv-ffi/tests/unit/test_client_lifecycle.rs +++ b/dash-spv-ffi/tests/unit/test_client_lifecycle.rs @@ -1,5 +1,5 @@ // Note: Many tests in this file are marked with #[ignore] because they call -// dash_spv_ffi_client_start() which hangs indefinitely when using regtest +// dash_spv_ffi_client_run() which hangs indefinitely when using regtest // network with no configured peers. These tests should be run with a proper // test network setup or mocked networking layer. @@ -72,14 +72,14 @@ mod tests { assert!(!client.is_null()); // Start - let _result = dash_spv_ffi_client_start(client); + let _result = dash_spv_ffi_client_run(client); // May fail in test environment, but should handle gracefully // Stop let _result = dash_spv_ffi_client_stop(client); // Restart - let _result = dash_spv_ffi_client_start(client); + let _result = dash_spv_ffi_client_run(client); let _result = dash_spv_ffi_client_stop(client); dash_spv_ffi_client_destroy(client); @@ -98,7 +98,7 @@ mod tests { // Start a sync operation in background // Start sync (non-blocking) - dash_spv_ffi_client_start(client); + dash_spv_ffi_client_run(client); // Immediately destroy client (should handle pending operations) dash_spv_ffi_client_destroy(client); @@ -121,7 +121,7 @@ mod tests { assert!(!client.is_null()); // Try to start (should handle no peers gracefully) - let _result = dash_spv_ffi_client_start(client); + let _result = dash_spv_ffi_client_run(client); dash_spv_ffi_client_destroy(client); dash_spv_ffi_config_destroy(config); @@ -164,7 +164,7 @@ mod tests { unsafe { // Test all client operations with null assert_eq!( - dash_spv_ffi_client_start(std::ptr::null_mut()), + dash_spv_ffi_client_run(std::ptr::null_mut()), FFIErrorCode::NullPointer as i32 ); diff --git a/dash-spv/README.md b/dash-spv/README.md index 0bd1e6ac5..247f93c75 100644 --- a/dash-spv/README.md +++ b/dash-spv/README.md @@ -58,14 +58,10 @@ async fn main() -> Result<(), Box> { let config = ClientConfig::mainnet() .with_storage_path("/path/to/data".into()); - // Create and start client - let mut client = DashSpvClient::new(config).await?; - client.start().await?; - - let (_command_sender, command_receiver) = tokio::sync::mpsc::unbounded_channel(); + // Create and run the client + let client = DashSpvClient::new(config).await?; let shutdown_token = CancellationToken::new(); - - client.run(command_receiver, shutdown_token).await?; + client.run(shutdown_token).await?; Ok(()) } diff --git a/dash-spv/examples/filter_sync.rs b/dash-spv/examples/filter_sync.rs index e483fef93..9b89490f8 100644 --- a/dash-spv/examples/filter_sync.rs +++ b/dash-spv/examples/filter_sync.rs @@ -38,9 +38,6 @@ async fn main() -> Result<(), Box> { // Create the client let client = DashSpvClient::new(config, network_manager, storage_manager, wallet).await?; - // Start the client - client.start().await?; - println!("Starting synchronization with filter support..."); println!("Watching address: {:?}", watch_address); diff --git a/dash-spv/examples/simple_sync.rs b/dash-spv/examples/simple_sync.rs index f412d1a79..a5f601609 100644 --- a/dash-spv/examples/simple_sync.rs +++ b/dash-spv/examples/simple_sync.rs @@ -33,9 +33,6 @@ async fn main() -> Result<(), Box> { // Create the client let client = DashSpvClient::new(config, network_manager, storage_manager, wallet).await?; - // Start the client - client.start().await?; - println!("Starting header synchronization..."); let shutdown_token = CancellationToken::new(); diff --git a/dash-spv/examples/spv_with_wallet.rs b/dash-spv/examples/spv_with_wallet.rs index 5230165b6..8d5f5c591 100644 --- a/dash-spv/examples/spv_with_wallet.rs +++ b/dash-spv/examples/spv_with_wallet.rs @@ -33,10 +33,6 @@ async fn main() -> Result<(), Box> { // Create the SPV client with all components let client = DashSpvClient::new(config, network_manager, storage_manager, wallet).await?; - // Start the client - println!("Starting SPV client..."); - client.start().await?; - // The wallet will automatically be notified of: // - New blocks via process_block() // - Mempool transactions via process_mempool_transaction() diff --git a/dash-spv/src/client/lifecycle.rs b/dash-spv/src/client/lifecycle.rs index cb2688b17..2716aa65e 100644 --- a/dash-spv/src/client/lifecycle.rs +++ b/dash-spv/src/client/lifecycle.rs @@ -124,7 +124,7 @@ impl DashSpvClient Result<()> { + pub(super) async fn start(&self) -> Result<()> { { let running = self.running.read().await; if *running { diff --git a/dash-spv/src/client/sync_coordinator.rs b/dash-spv/src/client/sync_coordinator.rs index 440393b2a..42ac541a0 100644 --- a/dash-spv/src/client/sync_coordinator.rs +++ b/dash-spv/src/client/sync_coordinator.rs @@ -1,7 +1,7 @@ //! Sync coordination and orchestration. use super::DashSpvClient; -use crate::error::{Result, SpvError}; +use crate::error::Result; use crate::network::NetworkManager; use crate::storage::StorageManager; use crate::sync::SyncProgress; @@ -17,16 +17,13 @@ impl DashSpvClient Result<()> { - let running = self.running.read().await; - if !*running { - return Err(SpvError::Config("Client not running".to_string())); - } - drop(running); + /// Calls `start()` internally, runs continuous network monitoring for new + /// blocks, ChainLocks, InstantLocks, etc., and calls `stop()` before returning. + /// The caller is responsible for cancelling the token (e.g. on ctrl-c). + pub async fn run(&self, token: CancellationToken) -> Result<()> { + self.start().await?; tracing::info!("Starting continuous network monitoring..."); @@ -59,39 +56,6 @@ impl DashSpvClient Result<()> { - let client_token = shutdown_token.clone(); - let client = self.clone(); - - let client_task = tokio::spawn(async move { - let result = client.monitor_network(client_token).await; - if let Err(e) = &result { - tracing::error!("Error running client: {}", e); - } - if let Err(e) = client.stop().await { - tracing::error!("Error stopping client: {}", e); - } - result - }); - - let shutdown_task = tokio::spawn(async move { - if let Err(e) = tokio::signal::ctrl_c().await { - tracing::error!("Error waiting for ctrl_c: {}", e); - } - tracing::debug!("Shutdown signal received"); - shutdown_token.cancel(); - }); - - let (client_result, _) = tokio::join!(client_task, shutdown_task); - client_result.map_err(|e| SpvError::General(format!("client_task panicked: {e}")))? + self.stop().await } } diff --git a/dash-spv/src/lib.rs b/dash-spv/src/lib.rs index eec8f5366..87398b720 100644 --- a/dash-spv/src/lib.rs +++ b/dash-spv/src/lib.rs @@ -33,10 +33,8 @@ //! let storage = DiskStorageManager::new(&config).await?; //! let wallet = Arc::new(RwLock::new(WalletManager::::new(config.network))); //! -//! // Create and start the client +//! // Create and run the client //! let client = DashSpvClient::new(config.clone(), network, storage, wallet).await?; -//! client.start().await?; -//! //! let shutdown_token = CancellationToken::new(); //! //! client.run(shutdown_token).await?; diff --git a/dash-spv/src/main.rs b/dash-spv/src/main.rs index 1c284ea08..ea5d60c01 100644 --- a/dash-spv/src/main.rs +++ b/dash-spv/src/main.rs @@ -323,14 +323,20 @@ async fn run_client( } }; - if let Err(e) = client.start().await { - eprintln!("Failed to start SPV client: {}", e); - process::exit(1); - } - - tracing::info!("SPV client started successfully"); - let shutdown_token = CancellationToken::new(); + let ctrl_c_token = shutdown_token.clone(); + tokio::spawn(async move { + tokio::select! { + result = tokio::signal::ctrl_c() => { + result.ok(); + tracing::debug!("Shutdown signal received"); + } + _ = ctrl_c_token.cancelled() => { + tracing::debug!("Shutdown token cancelled"); + } + } + ctrl_c_token.cancel(); + }); client.run(shutdown_token).await?; diff --git a/dash-spv/tests/peer_test.rs b/dash-spv/tests/peer_test.rs index 0af7841f9..683ce34c0 100644 --- a/dash-spv/tests/peer_test.rs +++ b/dash-spv/tests/peer_test.rs @@ -6,6 +6,7 @@ use std::time::Duration; use tempfile::TempDir; use tokio::sync::RwLock; use tokio::time; +use tokio_util::sync::CancellationToken; use dash_spv::client::{ClientConfig, DashSpvClient}; use dash_spv::network::PeerNetworkManager; @@ -47,8 +48,10 @@ async fn test_peer_connection() { let client = DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); - // Start the client - client.start().await.unwrap(); + let token = CancellationToken::new(); + let cancel = token.clone(); + let run_client = client.clone(); + let handle = tokio::spawn(async move { run_client.run(token).await }); // Give it time to connect to peers time::sleep(Duration::from_secs(5)).await; @@ -57,8 +60,8 @@ async fn test_peer_connection() { let peer_count = client.peer_count().await; assert!(peer_count > 0, "Should have connected to at least one peer"); - // Stop the client - client.stop().await.unwrap(); + cancel.cancel(); + let _ = handle.await; } #[tokio::test] @@ -83,13 +86,18 @@ async fn test_peer_persistence() { .await .unwrap(); - client.start().await.unwrap(); + let token = CancellationToken::new(); + let cancel = token.clone(); + let run_client = client.clone(); + let handle = tokio::spawn(async move { run_client.run(token).await }); + time::sleep(Duration::from_secs(5)).await; let peer_count = client.peer_count().await; assert!(peer_count > 0, "Should have connected to peers"); - client.stop().await.unwrap(); + cancel.cancel(); + let _ = handle.await; } // Second run: should load saved peers @@ -107,8 +115,11 @@ async fn test_peer_persistence() { DashSpvClient::new(config, network_manager, storage_manager, wallet).await.unwrap(); // Should connect faster due to saved peers + let token = CancellationToken::new(); + let cancel = token.clone(); + let run_client = client.clone(); let start = tokio::time::Instant::now(); - client.start().await.unwrap(); + let handle = tokio::spawn(async move { run_client.run(token).await }); // Wait for connection but with shorter timeout time::sleep(Duration::from_secs(3)).await; @@ -119,7 +130,8 @@ async fn test_peer_persistence() { let elapsed = start.elapsed(); println!("Connected to {} peers in {:?} (using saved peers)", peer_count, elapsed); - client.stop().await.unwrap(); + cancel.cancel(); + let _ = handle.await; } } diff --git a/dash-spv/tests/wallet_integration_test.rs b/dash-spv/tests/wallet_integration_test.rs index 58c6d3be3..2ed64dd5b 100644 --- a/dash-spv/tests/wallet_integration_test.rs +++ b/dash-spv/tests/wallet_integration_test.rs @@ -3,8 +3,10 @@ //! These tests validate end-to-end wallet operations through the SPVWalletManager. use std::sync::Arc; +use std::time::Duration; use tempfile::TempDir; use tokio::sync::RwLock; +use tokio_util::sync::CancellationToken; use dash_spv::network::PeerNetworkManager; use dash_spv::storage::DiskStorageManager; @@ -42,23 +44,27 @@ async fn test_spv_client_creation() { } #[tokio::test] -async fn test_spv_client_start_stop() { - // Test starting and stopping the client +async fn test_spv_client_run_stop() { let client = create_test_client().await; - // Start the client - client.start().await.unwrap(); + let token = CancellationToken::new(); + let cancel = token.clone(); - // Verify client is running - let running = client.is_running().await; - assert!(running); + let run_client = client.clone(); + let handle = tokio::spawn(async move { run_client.run(token).await }); - // Stop the client - client.stop().await.unwrap(); + tokio::time::timeout(Duration::from_secs(5), async { + while !client.is_running().await { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("client failed to start"); - // Verify client is stopped - let running = client.is_running().await; - assert!(!running); + cancel.cancel(); + handle.await.unwrap().unwrap(); + + assert!(!client.is_running().await); } #[tokio::test] From cbe7cade2f2e069f92af8bf5e9d44528db9a0712 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 12 Feb 2026 20:03:27 +0100 Subject: [PATCH 2/2] feat: report/propagate client start/run failures We currently just silently ignore any failure inside the sync coordinator or `monitor_network` thread. This PR propagates them and also adds a FFI callback structure which can be set to receive notifications about errors inside the SPV client. --- dash-spv-ffi/FFI_API.md | 38 +++++- dash-spv-ffi/include/dash_spv_ffi.h | 38 ++++++ dash-spv-ffi/src/callbacks.rs | 42 ++++++ dash-spv-ffi/src/client.rs | 49 ++++++- .../tests/unit/test_client_lifecycle.rs | 120 ++++++++++++++++++ dash-spv/src/client/sync_coordinator.rs | 40 ++++-- dash-spv/src/sync/sync_coordinator.rs | 2 + 7 files changed, 311 insertions(+), 18 deletions(-) diff --git a/dash-spv-ffi/FFI_API.md b/dash-spv-ffi/FFI_API.md index 95ef02125..1aac61d18 100644 --- a/dash-spv-ffi/FFI_API.md +++ b/dash-spv-ffi/FFI_API.md @@ -4,7 +4,7 @@ This document provides a comprehensive reference for all FFI (Foreign Function I **Auto-generated**: This documentation is automatically generated from the source code. Do not edit manually. -**Total Functions**: 48 +**Total Functions**: 50 ## Table of Contents @@ -94,12 +94,14 @@ Functions: 2 ### Event Callbacks -Functions: 4 +Functions: 6 | Function | Description | Module | |----------|-------------|--------| +| `dash_spv_ffi_client_clear_client_error_callback` | Clear the client error callback | client | | `dash_spv_ffi_client_clear_network_event_callbacks` | Clear network event callbacks | client | | `dash_spv_ffi_client_clear_progress_callback` | Clear progress callback | client | +| `dash_spv_ffi_client_set_client_error_callback` | Set a callback for fatal client errors (start failure, sync thread crash) | client | | `dash_spv_ffi_client_set_network_event_callbacks` | Set network event callbacks for push-based event notifications | client | | `dash_spv_ffi_client_set_progress_callback` | Set progress callback for sync progress updates | client | @@ -609,6 +611,22 @@ This function is unsafe because: - The caller must ensure all pointers are valid ### Event Callbacks - Detailed +#### `dash_spv_ffi_client_clear_client_error_callback` + +```c +dash_spv_ffi_client_clear_client_error_callback(client: *mut FFIDashSpvClient,) -> i32 +``` + +**Description:** +Clear the client error callback. # Safety - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. + +**Safety:** +- `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. + +**Module:** `client` + +--- + #### `dash_spv_ffi_client_clear_network_event_callbacks` ```c @@ -641,6 +659,22 @@ Clear progress callback. # Safety - `client` must be a valid, non-null pointer --- +#### `dash_spv_ffi_client_set_client_error_callback` + +```c +dash_spv_ffi_client_set_client_error_callback(client: *mut FFIDashSpvClient, callback: FFIClientErrorCallback,) -> i32 +``` + +**Description:** +Set a callback for fatal client errors (start failure, sync thread crash). # Safety - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. - The `callback` struct and its `user_data` must remain valid until the callback is cleared. - The callback must be thread-safe as it may be called from a background thread. + +**Safety:** +- `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. - The `callback` struct and its `user_data` must remain valid until the callback is cleared. - The callback must be thread-safe as it may be called from a background thread. + +**Module:** `client` + +--- + #### `dash_spv_ffi_client_set_network_event_callbacks` ```c diff --git a/dash-spv-ffi/include/dash_spv_ffi.h b/dash-spv-ffi/include/dash_spv_ffi.h index b9599e917..cc2e75e36 100644 --- a/dash-spv-ffi/include/dash_spv_ffi.h +++ b/dash-spv-ffi/include/dash_spv_ffi.h @@ -434,6 +434,23 @@ typedef struct FFIProgressCallback { void *user_data; } FFIProgressCallback; +/** + * Callback for fatal client errors (e.g. start failure, monitor thread crash). + * + * The `error` string pointer is borrowed and only valid for the duration + * of the callback. Callers must copy the string if they need to retain it + * after the callback returns. + */ +typedef void (*OnClientErrorCallback)(const char *error, void *user_data); + +/** + * Client error callback configuration. + */ +typedef struct FFIClientErrorCallback { + OnClientErrorCallback on_error; + void *user_data; +} FFIClientErrorCallback; + /** * FFIResult type for error handling */ @@ -684,6 +701,27 @@ int32_t dash_spv_ffi_client_set_progress_callback(struct FFIDashSpvClient *clien */ int32_t dash_spv_ffi_client_clear_progress_callback(struct FFIDashSpvClient *client) ; +/** + * Set a callback for fatal client errors (start failure, sync thread crash). + * + * # Safety + * - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. + * - The `callback` struct and its `user_data` must remain valid until the callback is cleared. + * - The callback must be thread-safe as it may be called from a background thread. + */ + +int32_t dash_spv_ffi_client_set_client_error_callback(struct FFIDashSpvClient *client, + struct FFIClientErrorCallback callback) +; + +/** + * Clear the client error callback. + * + * # Safety + * - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. + */ + int32_t dash_spv_ffi_client_clear_client_error_callback(struct FFIDashSpvClient *client) ; + struct FFIClientConfig *dash_spv_ffi_config_new(FFINetwork network) ; struct FFIClientConfig *dash_spv_ffi_config_mainnet(void) ; diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index 39960a4a5..12cc90184 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -573,6 +573,48 @@ impl Default for FFIWalletEventCallbacks { } } +// ============================================================================ +// FFIClientErrorCallback - Fatal client-level errors +// ============================================================================ + +/// Callback for fatal client errors (e.g. start failure, monitor thread crash). +/// +/// The `error` string pointer is borrowed and only valid for the duration +/// of the callback. Callers must copy the string if they need to retain it +/// after the callback returns. +pub type OnClientErrorCallback = + Option; + +/// Client error callback configuration. +#[repr(C)] +#[derive(Clone)] +pub struct FFIClientErrorCallback { + pub on_error: OnClientErrorCallback, + pub user_data: *mut c_void, +} + +unsafe impl Send for FFIClientErrorCallback {} +unsafe impl Sync for FFIClientErrorCallback {} + +impl Default for FFIClientErrorCallback { + fn default() -> Self { + Self { + on_error: None, + user_data: std::ptr::null_mut(), + } + } +} + +impl FFIClientErrorCallback { + /// Dispatch a client error to the callback. + pub fn dispatch(&self, error: &str) { + if let Some(cb) = self.on_error { + let c_error = CString::new(error).unwrap_or_default(); + cb(c_error.as_ptr(), self.user_data); + } + } +} + impl FFIWalletEventCallbacks { /// Dispatch a WalletEvent to the appropriate callback. pub fn dispatch(&self, event: &key_wallet_manager::WalletEvent) { diff --git a/dash-spv-ffi/src/client.rs b/dash-spv-ffi/src/client.rs index 2c0ddbdb9..bd750ee84 100644 --- a/dash-spv-ffi/src/client.rs +++ b/dash-spv-ffi/src/client.rs @@ -1,7 +1,7 @@ use crate::{ - null_check, set_last_error, FFIClientConfig, FFIErrorCode, FFINetworkEventCallbacks, - FFIProgressCallback, FFISyncEventCallbacks, FFISyncProgress, FFIWalletEventCallbacks, - FFIWalletManager, + null_check, set_last_error, FFIClientConfig, FFIClientErrorCallback, FFIErrorCode, + FFINetworkEventCallbacks, FFIProgressCallback, FFISyncEventCallbacks, FFISyncProgress, + FFIWalletEventCallbacks, FFIWalletManager, }; // Import wallet types from key-wallet-ffi use key_wallet_ffi::FFIWalletManager as KeyWalletFFIWalletManager; @@ -120,6 +120,7 @@ pub struct FFIDashSpvClient { network_event_callbacks: Arc>>, wallet_event_callbacks: Arc>>, progress_callback: Arc>>, + client_error_callback: Arc>>, } /// Create a new SPV client and return an opaque pointer. @@ -179,6 +180,7 @@ pub unsafe extern "C" fn dash_spv_ffi_client_new( network_event_callbacks: Arc::new(Mutex::new(None)), wallet_event_callbacks: Arc::new(Mutex::new(None)), progress_callback: Arc::new(Mutex::new(None)), + client_error_callback: Arc::new(Mutex::new(None)), }; Box::into_raw(Box::new(ffi_client)) } @@ -351,13 +353,17 @@ pub unsafe extern "C" fn dash_spv_ffi_client_run(client: *mut FFIDashSpvClient) )); } - // Spawn the sync monitoring task + let error_callback = client.client_error_callback.clone(); let spv_client = client.inner.clone(); tasks.push(client.runtime.spawn(async move { tracing::debug!("Sync task: starting run"); if let Err(e) = spv_client.run(shutdown_token).await { tracing::error!("Sync task: error: {}", e); + let cb = error_callback.lock().unwrap().clone(); + if let Some(ref cb) = cb { + cb.dispatch(&e.to_string()); + } } tracing::debug!("Sync task: exiting"); @@ -720,3 +726,38 @@ pub unsafe extern "C" fn dash_spv_ffi_client_clear_progress_callback( FFIErrorCode::Success as i32 } + +/// Set a callback for fatal client errors (start failure, sync thread crash). +/// +/// # Safety +/// - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. +/// - The `callback` struct and its `user_data` must remain valid until the callback is cleared. +/// - The callback must be thread-safe as it may be called from a background thread. +#[no_mangle] +pub unsafe extern "C" fn dash_spv_ffi_client_set_client_error_callback( + client: *mut FFIDashSpvClient, + callback: FFIClientErrorCallback, +) -> i32 { + null_check!(client); + + let client = &(*client); + *client.client_error_callback.lock().unwrap() = Some(callback); + + FFIErrorCode::Success as i32 +} + +/// Clear the client error callback. +/// +/// # Safety +/// - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. +#[no_mangle] +pub unsafe extern "C" fn dash_spv_ffi_client_clear_client_error_callback( + client: *mut FFIDashSpvClient, +) -> i32 { + null_check!(client); + + let client = &(*client); + *client.client_error_callback.lock().unwrap() = None; + + FFIErrorCode::Success as i32 +} diff --git a/dash-spv-ffi/tests/unit/test_client_lifecycle.rs b/dash-spv-ffi/tests/unit/test_client_lifecycle.rs index 6bda25e8f..ceef717ec 100644 --- a/dash-spv-ffi/tests/unit/test_client_lifecycle.rs +++ b/dash-spv-ffi/tests/unit/test_client_lifecycle.rs @@ -9,6 +9,8 @@ mod tests { use key_wallet_ffi::FFINetwork; use serial_test::serial; use std::ffi::CString; + use std::sync::mpsc; + use std::sync::{Arc as StdArc, Mutex as StdMutex}; use std::thread; use std::time::Duration; use tempfile::TempDir; @@ -209,6 +211,124 @@ mod tests { } } + #[test] + #[serial] + fn test_client_error_callback_fires_on_start_failure() { + let (tx, rx) = mpsc::channel::(); + let tx_ptr = Box::into_raw(Box::new(tx)); + + extern "C" fn on_error( + error: *const std::os::raw::c_char, + user_data: *mut std::os::raw::c_void, + ) { + let tx = unsafe { &*(user_data as *const mpsc::Sender) }; + let error_str = unsafe { std::ffi::CStr::from_ptr(error) }.to_str().unwrap().to_owned(); + let _ = tx.send(error_str); + } + + unsafe { + let (config, _temp_dir) = create_test_config_with_dir(); + let client = dash_spv_ffi_client_new(config); + assert!(!client.is_null()); + + let callback = FFIClientErrorCallback { + on_error: Some(on_error), + user_data: tx_ptr as *mut std::os::raw::c_void, + }; + let result = dash_spv_ffi_client_set_client_error_callback(client, callback); + assert_eq!(result, FFIErrorCode::Success as i32); + + // Call run() twice — the second run's sync thread will call + // start() on the already-running client, triggering "already running" + let run_result = dash_spv_ffi_client_run(client); + assert_eq!(run_result, FFIErrorCode::Success as i32); + + // Brief wait for the first run's sync thread to complete start() + thread::sleep(Duration::from_millis(200)); + + let _run_result2 = dash_spv_ffi_client_run(client); + + // Wait for the error callback to fire (with timeout) + let error_msg = rx + .recv_timeout(Duration::from_secs(5)) + .expect("Error callback should have been called on start failure"); + assert!( + error_msg.contains("already running"), + "Expected 'already running' error, got: {}", + error_msg + ); + + dash_spv_ffi_client_stop(client); + + // Free the sender only after stop has joined all threads, + // so no background thread can call on_error with a dangling user_data. + drop(Box::from_raw(tx_ptr)); + + dash_spv_ffi_client_destroy(client); + dash_spv_ffi_config_destroy(config); + } + } + + #[test] + #[serial] + fn test_client_error_callback_dispatch() { + let error_store: StdArc>> = StdArc::new(StdMutex::new(None)); + let error_store_raw = StdArc::into_raw(error_store.clone()); + + extern "C" fn on_error( + error: *const std::os::raw::c_char, + user_data: *mut std::os::raw::c_void, + ) { + assert!(!error.is_null()); + let store = unsafe { StdArc::from_raw(user_data as *const StdMutex>) }; + let error_str = unsafe { std::ffi::CStr::from_ptr(error) }.to_str().unwrap().to_owned(); + *store.lock().unwrap() = Some(error_str); + let _ = StdArc::into_raw(store); + } + + let callback = FFIClientErrorCallback { + on_error: Some(on_error), + user_data: error_store_raw as *mut std::os::raw::c_void, + }; + + callback.dispatch("test error message"); + + let received = error_store.lock().unwrap(); + assert_eq!(received.as_deref(), Some("test error message")); + drop(received); + + unsafe { drop(StdArc::from_raw(error_store_raw)) }; + } + + #[test] + #[serial] + fn test_client_error_callback_null_client() { + unsafe { + let callback = FFIClientErrorCallback { + on_error: None, + user_data: std::ptr::null_mut(), + }; + + assert_eq!( + dash_spv_ffi_client_set_client_error_callback(std::ptr::null_mut(), callback), + FFIErrorCode::NullPointer as i32 + ); + + assert_eq!( + dash_spv_ffi_client_clear_client_error_callback(std::ptr::null_mut()), + FFIErrorCode::NullPointer as i32 + ); + } + } + + #[test] + #[serial] + fn test_client_error_callback_no_callback_set() { + // Dispatch with no callback set should not panic + let callback = FFIClientErrorCallback::default(); + callback.dispatch("should not panic"); + } + #[test] #[serial] fn test_client_repeated_creation_destruction() { diff --git a/dash-spv/src/client/sync_coordinator.rs b/dash-spv/src/client/sync_coordinator.rs index 42ac541a0..da1fe444d 100644 --- a/dash-spv/src/client/sync_coordinator.rs +++ b/dash-spv/src/client/sync_coordinator.rs @@ -30,32 +30,48 @@ impl DashSpvClient { - tracing::info!("Sync progress:{}", *progress_updates.borrow()); + let error = tokio::select! { + result = progress_updates.changed() => { + match result { + Ok(()) => { + tracing::info!("Sync progress: {}", *progress_updates.borrow()); + None + } + Err(_) => { + tracing::warn!("Progress channel closed."); + break None + } + } } _ = sync_coordinator_tick_interval.tick() => { - // Tick the sync coordinator to aggregate progress - if let Err(e) = self.sync_coordinator.lock().await.tick().await { - tracing::warn!("Sync coordinator tick error: {}", e); - } + self.sync_coordinator.lock().await.tick().await.err().map(Into::into) } _ = token.cancelled() => { tracing::debug!("DashSpvClient run loop cancelled"); - break + break None } + }; + + if error.is_some() { + break error; } - } + }; + + // Always stop the client + let stop_result = self.stop().await; - self.stop().await + match error { + Some(e) => Err(e), + None => stop_result, + } } } diff --git a/dash-spv/src/sync/sync_coordinator.rs b/dash-spv/src/sync/sync_coordinator.rs index c32fb3cfa..d1f78b106 100644 --- a/dash-spv/src/sync/sync_coordinator.rs +++ b/dash-spv/src/sync/sync_coordinator.rs @@ -236,9 +236,11 @@ where } Ok(Err(e)) => { tracing::error!("Manager task failed: {}", e); + return Err(e); } Err(e) => { tracing::error!("Manager task panicked: {}", e); + return Err(SyncError::InvalidState(format!("Manager task panicked: {}", e))); } } }