Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a comprehensive Foreign Function Interface (FFI) layer for the Dash SPV client, adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant C_App as C Application
participant FFI as dash-spv-ffi (C API)
participant RustClient as DashSpvClient (Rust)
participant Network as Dash Network
C_App->>FFI: Create FFIClientConfig
C_App->>FFI: Set config options (data dir, peers, etc.)
C_App->>FFI: Create FFIDashSpvClient(config)
C_App->>FFI: Start client
FFI->>RustClient: Initialize and start
RustClient->>Network: Connect and sync
C_App->>FFI: Sync to tip (with callbacks)
FFI->>RustClient: Sync blockchain
RustClient->>Network: Download blocks/filters
RustClient-->>FFI: Report progress/events
FFI-->>C_App: Invoke progress/completion callbacks
C_App->>FFI: Add/remove watch items
FFI->>RustClient: Update watchlist
C_App->>FFI: Query balances, UTXOs, transactions
FFI->>RustClient: Fetch wallet data
FFI-->>C_App: Return results
C_App->>FFI: Stop and destroy client/config
FFI->>RustClient: Shutdown and cleanup
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 33
🔭 Outside diff range comments (5)
dash-spv/src/network/message_handler.rs (1)
60-70:ChainLock/InstantLockmessages never counted
MessageStatscontainschainlock_messagesandinstantlock_messages, yet thematchskips these variants. This silently under-reports statistics and makes the enum entries inMessageHandleResultmisleading.NetworkMessage::MnListDiff(diff) => { self.stats.masternode_diff_messages += 1; MessageHandleResult::MasternodeDiff(diff) } + NetworkMessage::ChainLock(cl) => { + self.stats.chainlock_messages += 1; + MessageHandleResult::ChainLock(cl) + } + NetworkMessage::InstantLock(il) => { + self.stats.instantlock_messages += 1; + MessageHandleResult::InstantLock(il) + }(Requires the two variants to exist in
dashcore::network::message::*.)Without this addition the counters stay at zero.
dash-spv/src/client/wallet_utils.rs (1)
190-204: Hold the lock once when syncing many watch items
sync_watch_items_with_walletacquires a read lock for every item, thrashing the async lock and needlessly increasing contention.- for item in watch_items.iter() { - if let WatchItem::Address { address, .. } = item { - let wallet = self.wallet.read().await; - … - } - } + let mut wallet = self.wallet.write().await; + for item in watch_items { + if let WatchItem::Address { address, .. } = item { + if let Err(e) = wallet.add_watched_address(address.clone()).await { + tracing::warn!("Failed to sync address {} with wallet: {}", address, e); + } else { + synced_count += 1; + } + } + }This cuts the number of await points from N to 1 and materially improves throughput during large syncs.
dash-spv/src/client/block_processor.rs (1)
291-306:wallet.read().awaitfollowed bywallet.add_utxo()risks self-deadlock
add_utxois a mutating call executed while a read-lock onwalletis still held.
IfWallet::add_utxotries to acquire an internal write lock (common pattern) this can dead-lock on Tokio’sRwLock.Extract the wallet instance outside of the read guard:
-let wallet = self.wallet.read().await; -if let Err(e) = wallet.add_utxo(utxo).await { +let wallet = Arc::clone(&self.wallet); +drop(self.wallet.read().await); // release outer read lock first +if let Err(e) = wallet.write().await.add_utxo(utxo).await {or expose a non-locking
add_utxoAPI that takes&self.dash-spv/src/client/mod.rs (1)
655-666: Duplicate timeout check blocks – unnecessary extra workThe same
last_timeout_checkguard appears twice directly one after the other.
The second block is effectively dead code (always false) or, if intervals diverge later, doubles the work.-// First block (keep this one) if last_timeout_check.elapsed() >= timeout_check_interval { let _ = self .sync_manager .check_sync_timeouts(&mut *self.storage, &mut *self.network) .await; } - -// Second block (remove) -if last_timeout_check.elapsed() >= timeout_check_interval { - // Request timeout handling … - last_timeout_check = Instant::now(); -} +last_timeout_check = Instant::now();dash-spv/src/storage/mod.rs (1)
22-27:StorageManageralready exposesas_any_mut; the extraAsAnyMuttrait is duplicate
StorageManagerdefinesfn as_any_mut(&mut self). The helper trait below provides the same method, leading to API duplication and possible confusion:pub trait AsAnyMut { … } // duplicate impl<T: 'static> AsAnyMut for T { … }Unless other crates rely on
AsAnyMut, remove it and its impl, or makeStorageManager: AsAnyMutto avoid redundancy.
♻️ Duplicate comments (3)
dash-spv/src/storage/disk.rs (1)
565-573: Same high-cost cloning for filter segmentsApply the same fix in
evict_oldest_filter_segmentto avoid double memory amplification.dash-spv-ffi/src/callbacks.rs (1)
80-100: Same use-after-free issue in event callbacks.The
FFIEventCallbacksmethods have the same use-after-free vulnerability withCStringas mentioned above.dash-spv-ffi/src/config.rs (1)
146-156: Function accepts relay parameter but doesn't use it.Same issue as
set_user_agent- the function accepts a parameter but doesn't use it because the functionality isn't supported inClientConfig.
🧹 Nitpick comments (42)
dash-spv-ffi/build.rs (1)
5-18: Addcargo:rerun-if-changedhints and replace bareunwraps in build-scriptThe build-script is executed every
cargo build; without rerun hints Cargo will rebuild the crate even when nothing relevant changed, slowing incremental builds.
Additionally, bareunwrap()s turn accidental I/O or env-var failures into opaque panics – anexpectwith context is preferable.fn main() { - let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap(); + let crate_dir = env::var("CARGO_MANIFEST_DIR") + .expect("CARGO_MANIFEST_DIR env-var must be set by Cargo"); + + // Re-run the build-script only when sources or config change + println!("cargo:rerun-if-changed=build.rs"); + println!("cargo:rerun-if-changed=cbindgen.toml"); + println!("cargo:rerun-if-changed=src"); - let output_path = PathBuf::from(&crate_dir).join("include"); + let output_path = PathBuf::from(&crate_dir).join("include"); - std::fs::create_dir_all(&output_path).unwrap(); + std::fs::create_dir_all(&output_path) + .expect("Failed to create FFI header output directory");dash-spv/src/network/message_handler.rs (1)
132-136: Dead enum variants – inventory types never returned
MessageHandleResult::{Inventory, GetData}are defined but the handler always wraps those messages inUnhandled(...). Either drop the unused variants or return them here to avoid API confusion.dash-spv/src/network/handshake.rs (1)
54-58: Lift time-out constants to module scope
HANDSHAKE_TIMEOUTandMESSAGE_POLL_INTERVALare re-created on every call. Moving them toconstdefinitions at module level avoids redundant allocations and documents their purpose more clearly.- const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(10); - const MESSAGE_POLL_INTERVAL: Duration = Duration::from_millis(100); +const HANDSHAKE_TIMEOUT: tokio::time::Duration = tokio::time::Duration::from_secs(10); +const MESSAGE_POLL_INTERVAL: tokio::time::Duration = tokio::time::Duration::from_millis(100);(Adjust imports accordingly.)
dash-spv/src/network/peer.rs (1)
68-79: Avoid unnecessary allocations when querying peer data
connected_peers,connected_countandbest_heightiterate three times. Minor, but using a single iterator (or caching results) would reduce traversal cost for large peer sets.- pub fn connected_count(&self) -> usize { - self.peers.values().filter(|p| p.connected).count() - } + pub fn connected_count(&self) -> usize { + self.peers.values().fold(0, |acc, p| acc + p.connected as usize) + }Purely optional – current code is acceptable for typical peer counts.
dash-spv/examples/filter_sync.rs (1)
13-20: Handle potential network mismatch withoutunwrap()Using
.require_network(...).unwrap()will crash the example on a simple user error. Returning a descriptive error keeps the sample beginner-friendly.- let watch_address = Address::<dashcore::address::NetworkUnchecked>::from_str( - "Xan9iCVe1q5jYRDZ4VSMCtBjq2VyQA3Dge", - )?; + let watch_address = Address::<dashcore::address::NetworkUnchecked>::from_str( + "Xan9iCVe1q5jYRDZ4VSMCtBjq2VyQA3Dge", + )? + .require_network(Network::Dash) + .map_err(|e| format!("Address not on Dash network: {e}"))?; - .watch_address(watch_address.clone().require_network(Network::Dash).unwrap()) + .watch_address(watch_address.clone())dash-spv/src/client/config.rs (2)
260-266: Duplicate peer entry in default mainnet seed list
"127.0.0.1:9999"appears twice.
Connecting twice to the same address wastes one outbound slot and slightly skews peer-selection metrics.- "149.28.22.65:9999".parse().unwrap(), // masternode.io seed - "127.0.0.1:9999".parse().unwrap(), + "149.28.22.65:9999".parse().unwrap(), // masternode.io seedIf the duplication is intentional (e.g. for prioritisation during local testing), add a short comment to avoid confusion.
160-170: Mixed builder styles make the API inconsistent
add_peer(&mut self) -> &mut Selfmutates in-place while the otherwith_*helpers takeselfby value and return a new instance.
This asymmetry hampers fluent chaining:// can't chain cleanly – lifetime of `&mut self` conflicts ClientConfig::regtest().add_peer(addr).with_log_level("debug");Consider aligning on one style (builder-pattern by-value is more common):
-pub fn add_peer(mut self, address: SocketAddr) -> Self { - self.peers.push(address); - self -} +pub fn add_peer(mut self, address: SocketAddr) -> Self { + self.peers.push(address); + self +}…and update call sites accordingly.
Also applies to: 203-213
dash-spv/src/network/tests.rs (1)
12-44: Hard-coding everyClientConfigfield inflates maintenance overheadThe test re-specifies 30-ish fields; each new field added to
ClientConfigwill break compilation until every test is updated.Prefer starting from an existing preset and overriding what the test cares about:
let temp_dir = TempDir::new().unwrap(); let mut cfg = ClientConfig::regtest() .with_storage_path(temp_dir.path().into()) .with_validation_mode(ValidationMode::Basic); cfg.enable_filters = false; cfg.enable_masternodes = false; cfg.max_peers = 3;This keeps the test resilient to future config changes and highlights only the behaviour under test.
dash-spv/src/network/pool.rs (1)
74-76: Minor lock-duration optimisation (optional)
self.connections.read().await.iter()...holds the read-lock for the entire collection/clone operation.
Taking the lock into a local variable first clarifies intent and can marginally improve clarity:let connections = self.connections.read().await; connections .iter() .map(|(addr, conn)| (*addr, conn.clone())) .collect()Not critical, just a readability micro-tweak.
dash-spv-ffi/examples/basic_usage.c (2)
3-4: Prefer system-wide include path for generated headerUsing a relative path (
"../include/dash_spv_ffi.h") breaks once the library is installed system-wide or when the build directory layout changes.
Recommend:-#include "../include/dash_spv_ffi.h" +#include <dash_spv_ffi.h>Adjust your build system (
pkg-config, CMakefind_package, etc.) accordingly.
19-24: Hard-coded data directory may fail on non-Unix targets
"/tmp/dash-spv-test"is Unix-specific and writable only on some systems. Consider:const char* home = getenv("HOME"); char datadir[PATH_MAX]; snprintf(datadir, sizeof(datadir), "%s/.dash-spv-test", home ? home : "."); dash_spv_ffi_config_set_data_dir(config, datadir);Or expose the path via CLI/ENV for portability.
dash-spv-ffi/cbindgen.toml (1)
14-15: Function names are left un-prefixed – potential global symbol clashes
[fn] prefix = ""means exported functions keep their Rust names, while types get thedash_spv_ffi_prefix.
In mixed-FFI environments this easily collides with other libraries.Consider:
-[fn] -prefix = "" +# Apply the same library prefix to functions +[fn] +prefix = "dash_spv_ffi_"Regenerate the header to keep the public surface coherent.
Also applies to: 22-25
dash-spv/src/network/addrv2.rs (1)
98-100: Minor clone allocation can be avoided
choose_multiple(...).cloned().collect()allocates a newVecand clones everyAddrV2Message.
If zero-copy is desired, returnVec<&AddrV2Message>(or small-vec) and clone only when serialising. Not critical, just a note.Also applies to: 111-113
dash-spv-ffi/tests/test_wallet.rs (1)
124-126: Heap-allocating solely fordestroyinflates the test’s complexity
Box::into_raw(Box::new(ffi_utxo))allocates a fresh copy that never existed in production code just to calldash_spv_ffi_utxo_destroy.
Instead, expose (or add) a dedicated helper likedash_spv_ffi_utxo_from_rustinside the FFI crate for tests, or skipdestroy– the stack copy will be dropped automatically.dash-spv-ffi/tests/c_tests/Makefile (1)
33-41: `echo "\n…"`` does not print a newline in POSIX sh
echodoes not interpret\nunless the implementation supports-e.
The loop therefore prints the two characters\nliterally, polluting test logs.- echo "\nRunning $$test:"; + printf '\nRunning %s:\n' "$$test";dash-spv-ffi/tests/test_error.rs (1)
8-14: Two different “clear error” APIs cause ambiguityThe test calls both
clear_last_error()and the FFI symboldash_spv_ffi_clear_error().
If these diverge in behaviour or implementation (e.g., one is#[inline]Rust-side, the other true FFI), the test may mask bugs.
Stick to one public API to avoid confusion.dash-spv/src/storage/memory.rs (1)
250-262:get_utxos_for_addressclones into temp vector – prefer pre-allocationWhen an address has many outpoints, repeated
pushreallocations hurt performance.-if let Some(outpoints) = self.utxo_address_index.get(address) { - for outpoint in outpoints { - if let Some(utxo) = self.utxos.get(outpoint) { - utxos.push(utxo.clone()); - } - } -} +if let Some(outpoints) = self.utxo_address_index.get(address) { + utxos.reserve(outpoints.len()); + for outpoint in outpoints { + if let Some(utxo) = self.utxos.get(outpoint) { + utxos.push(utxo.clone()); + } + } +}dash-spv-ffi/tests/README.md (2)
42-56: Add language identifier to fenced code blocks
bashcommands are already fenced, but the Rust block in “Rust Tests” is missing thebashidentifier on the opening back-ticks. That prevents correct syntax highlighting on GitHub.-```bash +# Run all Rust tests -cargo test -p dash-spv-ffi +```bash +# Run all Rust tests +cargo test -p dash-spv-ffi
75-87: Clarify coverage list duplicates “Integration”Bullet 8 (“Integration”) partially overlaps items 1-7 and might confuse readers because integration tests are in their own top-level section. Consider rephrasing or removing to avoid redundancy.
dash-spv-ffi/tests/unit/test_configuration.rs (3)
9-22: Function name doesn’t match its purpose
test_config_with_invalid_networkexercises valid networks only. Rename to reflect intent or add a branch that actually feeds an invalid enum value and assertsInvalidArgument.
31-35: Path length may exceed OS limits
"x".repeat(4000)can overflowPATH_MAXon Windows/macOS and fail sporadically. Either:
- Skip on non-Linux targets, or
- Cap to a conservative 250–300 chars.
- let long_path = format!("/tmp/{}", "x".repeat(4000)); + #[cfg(target_os = "windows")] + let long_path = format!("C:\\temp\\{}", "x".repeat(240)); + #[cfg(not(target_os = "windows"))] + let long_path = format!("/tmp/{}", "x".repeat(4000));
97-103: Thousand-peer loop inflates test timeAdding 1000 peers does not increase coverage compared to ~50 and lengthens CI. Consider reducing or marking the test as
#[ignore]for default runs.dash-spv-ffi/tests/test_config.rs (2)
109-116: Validate success path after setting invalid peerYou assert
InvalidArgument(good) but don’t clear the thread-local error state afterwards. Subsequent FFI calls may return stale error messages. Calldash_spv_ffi_clear_last_error()or equivalent after this assertion if available.
125-133: Missing round-trip verification foruser_agentThe test sets the user agent but never reads it back. Add a getter (or expose one) to ensure the string is stored correctly.
dash-spv-ffi/tests/test_utils.rs (1)
65-69: Environment variable persists across tests
test_enable_test_modesetsDASH_SPV_TEST_MODEglobally but never restores its previous value. This may influence later tests. Save the old value and reset in adropguard orfinallyblock.dash-spv/src/client/wallet_utils.rs (1)
117-120: Returning0on balance failure may hide real problems
safe_get_wallet_balancelogs the error but returns a zero balance, which can silently propagate incorrect financial data.
Propagate the error upward (or wrap it in a dedicatedFallbackBalanceenum) unless you have a strong UX reason to mask it.dash-spv/src/network/persist.rs (1)
84-88: Avoid relying onDebugoutput for network identity
format!("{:?}", self.network)is human-oriented and could change (e.g. addingMainnet).
Persist the canonicalNetworkstring (dash,testnet, …) or a numeric discriminant instead; this makes on-disk compatibility explicit and future-proof.dash-spv-ffi/src/lib.rs (2)
20-47: Avoid#[path = …]in published crates; move unit-test modules intotests/insteadThe explicit
#[path = "../tests/unit/…"]attributes couple the library to a workspace-relative layout.
When the crate is published on crates.io or consumed outside this workspace the paths will be invalid andcargo testwill fail.Suggested minimal change (inside this file):
-#[cfg(test)] -#[path = "../tests/unit/test_type_conversions.rs"] -mod test_type_conversions; +#[cfg(test)] +mod test_type_conversions;…and move the file to
dash-spv-ffi/tests/test_type_conversions.rs(same for the other test modules).This keeps the crate self-contained and follows the conventional cargo test layout.
9-15: Very broadpub usere-exports may pollute the top-level namespace
pub use callbacks::*;(etc.) re-exports everything from all sub-modules. For FFI symbols that might be fine, but it also re-exports every helper, private constant, and internal type, increasing the public API surface and breaking encapsulation.Consider creating a curated prelude or re-exporting only the C-compatible symbols that are meant to be stable.
dash-spv/src/client/block_processor.rs (1)
296-299: Mis-ordered log arguments repeattxidand omitvin
"💸 TX {} input {}:{} …", txid, txid, vinprints the txid twice and never prints the input index.-tracing::info!("💸 TX {} input {}:{} spending UTXO …", txid, txid, vin, …); +tracing::info!("💸 TX {} input {}:{} spending UTXO …", txid, vin, txid, …);(Or reorder the placeholders – first
txid, thenvin.)dash-spv-ffi/tests/unit/test_type_conversions.rs (1)
58-70:FFIArray::newreturns an owned struct – boxing theninto_rawis redundantWrapping the struct in a
Boxjust to obtain a raw pointer increases allocation and obscures intent.Instead:
let empty_array = FFIArray::new::<u32>(vec![]); unsafe { dash_spv_ffi_array_destroy(&empty_array as *const _ as *mut _); }(or expose a dedicated
destroy_arrayhelper that takesFFIArrayby value).dash-spv/src/network/multi_peer.rs (1)
13-15: New dashcore imports look good, but avoid magic numbers for service flags.Importing
ServiceFlags,NetworkMessage, andNetworkis necessary for the added functionality. Consider replacingServiceFlags::from(1)with a named constant (e.g.,ServiceFlags::NODE_NETWORK) for clarity.dash-spv/src/client/mod.rs (1)
654-658: Minor: Prefermatches!(...)instead of nestedif letfor readabilityThe nested
if letchain that checksprepare_sync’s result could be flattened:if matches!( self.sync_manager .header_sync_mut() .prepare_sync(&mut *self.storage) .await, Ok(_) ) { … }dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
135-157:test_client_resource_cleanupdoesn’t assert anything – the test can never failKeeping only comments and a
sleepmeans this test will pass regardless of leaks.
Either:
- Measure thread count before/after with
thread::scope+"procfs"/parking_lot_core::park::num_threads(), or- Drop the test and rely on
valgrind/miriin CI.Otherwise the test adds runtime with zero coverage.
dash-spv/src/lib.rs (1)
60-68: Minor polish: avoid re-export duplication & keep groups sorted
Networkis re-exported twice (once indirectly byclientand once here).
While harmless, removing the duplicate keeps the public prelude tidy.-pub use dashcore::{Address, BlockHash, Network, OutPoint, ScriptBuf}; +pub use dashcore::{Address, BlockHash, OutPoint, ScriptBuf, Network};Also consider grouping re-exports alphabetically for quick scanning.
dash-spv-ffi/tests/integration/test_cross_language.rs (1)
20-55: Header-generation test silently passes whencbindgenis missingIf
cbindgenis absent the test prints a message and still succeeds,
masking broken CI environments. Either mark the test#[ignore]when the
binary is unavailable, or turn the absence into a failure:let output = Command::new("cbindgen").current_dir(&crate_dir) ... .output() .expect("failed to spawn cbindgen"); assert!( output.status.success(), "cbindgen failed: {}", String::from_utf8_lossy(&output.stderr) );dash-spv-ffi/tests/c_tests/test_basic.c (1)
248-277: Callbackuser_datais shared but typed inconsistently
progress_callbackandcompletion_callbackboth treatuser_dataas
int*, yet you pass the address of different variables
(progress_called/completion_called). Passing a single int works here
only by accident; consider separate fields or a struct holding both flags to
avoid accidental overwrite.dash-spv-ffi/src/utils.rs (1)
29-31: Guarantee a stable pointer fordash_spv_ffi_version
concat!(..., "\0").as_ptr()returns a pointer each time the function is
called; in practice it is stable, but defining astaticmakes the guarantee
explicit and avoids any future refactor surprises.#[no_mangle] pub extern "C" fn dash_spv_ffi_version() -> *const c_char { - concat!(env!("CARGO_PKG_VERSION"), "\0").as_ptr() as *const c_char + static VERSION: &[u8] = concat!(env!("CARGO_PKG_VERSION"), "\0").as_bytes(); + VERSION.as_ptr() as *const c_char }dash-spv-ffi/tests/integration/test_full_workflow.rs (1)
249-249: Use a valid test transaction for more realistic testing.The current test transaction hex appears to be invalid. Consider using a properly formatted transaction to ensure the test exercises the actual transaction validation logic.
-// Create a test transaction (this would normally come from wallet) -// For testing, we'll use a minimal transaction hex -let test_tx_hex = "01000000000100000000000000001976a914000000000000000000000000000000000000000088ac00000000"; +// Create a valid test transaction for regtest +// This is a properly formatted coinbase transaction +let test_tx_hex = "0100000001000000000000000000000000000000000000000000000000000000000000000000ffffffff0704ffff001d0104ffffffff0100f2052a0100000043410467b1c13e1e4b0c9b314e24bf31d661e7e9e7f3e7e6e7b8e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7e7ac00000000";dash-spv-ffi/src/client.rs (1)
544-545: Consider reducing API surface to match implementation.Many functions return "Not implemented" errors. Consider either:
- Removing these functions from the public API until implemented
- Clearly documenting which functions are not yet available
- Adding a roadmap for implementation
This would improve the developer experience by setting clear expectations about available functionality.
Also applies to: 592-593, 661-662, 730-731, 868-869, 941-942, 972-973
dash-spv-ffi/include/dash_spv_ffi.h (1)
6-23: Prefix enum constants to avoid global name clashes.Identifiers like
None,Basic,Full,Address, etc. are extremely generic in the C global namespace and risk collisions.
Consider namespacing, e.g.FFI_VALIDATION_NONE,FFI_WATCH_ADDRESS, …dash-spv-ffi/tests/unit/test_error_handling.rs (1)
1-2: Redundant#[cfg(test)]wrapperFiles under
tests/are already compiled exclusively for tests; the extra#[cfg(test)] mod tests { ... }indirection just adds noise and an unnecessary module layer.
Consider dropping the wrapper and placing the test functions at the crate root:-#[cfg(test)] -mod tests { - // … -} +// tests live at crate root in integration-test crates +// …
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
Cargo.toml(1 hunks)dash-network-ffi/src/lib.rs(2 hunks)dash-network/src/lib.rs(2 hunks)dash-spv-ffi/Cargo.toml(1 hunks)dash-spv-ffi/README.md(1 hunks)dash-spv-ffi/build.rs(1 hunks)dash-spv-ffi/cbindgen.toml(1 hunks)dash-spv-ffi/examples/basic_usage.c(1 hunks)dash-spv-ffi/include/dash_spv_ffi.h(1 hunks)dash-spv-ffi/src/callbacks.rs(1 hunks)dash-spv-ffi/src/client.rs(1 hunks)dash-spv-ffi/src/config.rs(1 hunks)dash-spv-ffi/src/error.rs(1 hunks)dash-spv-ffi/src/lib.rs(1 hunks)dash-spv-ffi/src/types.rs(1 hunks)dash-spv-ffi/src/utils.rs(1 hunks)dash-spv-ffi/src/wallet.rs(1 hunks)dash-spv-ffi/tests/README.md(1 hunks)dash-spv-ffi/tests/c_tests/Makefile(1 hunks)dash-spv-ffi/tests/c_tests/test_advanced.c(1 hunks)dash-spv-ffi/tests/c_tests/test_basic.c(1 hunks)dash-spv-ffi/tests/c_tests/test_integration.c(1 hunks)dash-spv-ffi/tests/integration/mod.rs(1 hunks)dash-spv-ffi/tests/integration/test_cross_language.rs(1 hunks)dash-spv-ffi/tests/integration/test_full_workflow.rs(1 hunks)dash-spv-ffi/tests/performance/mod.rs(1 hunks)dash-spv-ffi/tests/performance/test_benchmarks.rs(1 hunks)dash-spv-ffi/tests/security/mod.rs(1 hunks)dash-spv-ffi/tests/security/test_security.rs(1 hunks)dash-spv-ffi/tests/test_client.rs(1 hunks)dash-spv-ffi/tests/test_config.rs(1 hunks)dash-spv-ffi/tests/test_error.rs(1 hunks)dash-spv-ffi/tests/test_types.rs(1 hunks)dash-spv-ffi/tests/test_utils.rs(1 hunks)dash-spv-ffi/tests/test_wallet.rs(1 hunks)dash-spv-ffi/tests/unit/test_async_operations.rs(1 hunks)dash-spv-ffi/tests/unit/test_client_lifecycle.rs(1 hunks)dash-spv-ffi/tests/unit/test_configuration.rs(1 hunks)dash-spv-ffi/tests/unit/test_error_handling.rs(1 hunks)dash-spv-ffi/tests/unit/test_memory_management.rs(1 hunks)dash-spv-ffi/tests/unit/test_type_conversions.rs(1 hunks)dash-spv-ffi/tests/unit/test_wallet_operations.rs(1 hunks)dash-spv/examples/filter_sync.rs(2 hunks)dash-spv/examples/simple_sync.rs(3 hunks)dash-spv/src/client/block_processor.rs(6 hunks)dash-spv/src/client/config.rs(2 hunks)dash-spv/src/client/consistency.rs(3 hunks)dash-spv/src/client/filter_sync.rs(2 hunks)dash-spv/src/client/message_handler.rs(11 hunks)dash-spv/src/client/mod.rs(53 hunks)dash-spv/src/client/status_display.rs(5 hunks)dash-spv/src/client/wallet_utils.rs(3 hunks)dash-spv/src/client/watch_manager.rs(2 hunks)dash-spv/src/error.rs(6 hunks)dash-spv/src/lib.rs(3 hunks)dash-spv/src/main.rs(10 hunks)dash-spv/src/network/addrv2.rs(5 hunks)dash-spv/src/network/connection.rs(7 hunks)dash-spv/src/network/constants.rs(2 hunks)dash-spv/src/network/discovery.rs(5 hunks)dash-spv/src/network/handshake.rs(7 hunks)dash-spv/src/network/message_handler.rs(5 hunks)dash-spv/src/network/mod.rs(4 hunks)dash-spv/src/network/multi_peer.rs(24 hunks)dash-spv/src/network/peer.rs(4 hunks)dash-spv/src/network/persist.rs(5 hunks)dash-spv/src/network/pool.rs(5 hunks)dash-spv/src/network/tests.rs(2 hunks)dash-spv/src/storage/disk.rs(23 hunks)dash-spv/src/storage/memory.rs(5 hunks)dash-spv/src/storage/mod.rs(3 hunks)dash-spv/src/storage/types.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (14)
dash-spv/examples/simple_sync.rs (1)
dash-spv/src/lib.rs (1)
init_logging(80-98)
dash-spv-ffi/tests/integration/mod.rs (1)
dash-spv-ffi/tests/c_tests/test_integration.c (1)
test_full_workflow(51-142)
dash-network-ffi/src/lib.rs (1)
dash-network/src/lib.rs (2)
magic(55-63)from_magic(33-42)
dash-spv/src/client/config.rs (2)
dash-spv/src/client/mod.rs (3)
new(193-262)network(381-383)network(1318-1321)dash-spv/src/types.rs (1)
address(243-248)
dash-spv-ffi/tests/test_types.rs (1)
dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)
dash-spv-ffi/tests/unit/test_configuration.rs (3)
dash/src/blockdata/transaction/outpoint.rs (1)
is_null(107-109)dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)dash-spv/src/validation/mod.rs (1)
mode(85-87)
dash-network/src/lib.rs (1)
dash-network-ffi/src/lib.rs (2)
magic(72-74)from_magic(64-70)
dash-spv-ffi/tests/test_config.rs (2)
dash/src/blockdata/transaction/outpoint.rs (1)
is_null(107-109)dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)
dash-spv-ffi/tests/performance/test_benchmarks.rs (2)
dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)dash-spv-ffi/src/error.rs (1)
set_last_error(25-30)
dash-spv-ffi/src/lib.rs (2)
dash-spv-ffi/tests/test_client.rs (1)
test_client_lifecycle(73-85)dash-spv-ffi/tests/c_tests/test_advanced.c (2)
test_wallet_operations(21-67)test_memory_management(205-230)
dash-spv-ffi/tests/test_utils.rs (2)
dash/src/blockdata/transaction/outpoint.rs (1)
is_null(107-109)dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)
dash-spv-ffi/src/config.rs (4)
dash-spv-ffi/src/error.rs (1)
set_last_error(25-30)dash-spv/src/validation/mod.rs (1)
mode(85-87)dash-spv/src/client/config.rs (2)
mainnet(145-147)testnet(150-152)dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)
dash-spv-ffi/src/wallet.rs (3)
dash-spv-ffi/src/error.rs (1)
set_last_error(25-30)dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)dash-spv/src/types.rs (1)
address(243-248)
dash-spv-ffi/src/client.rs (4)
dash-spv-ffi/src/error.rs (1)
set_last_error(25-30)dash-spv/src/types.rs (1)
address(243-248)dash-spv-ffi/src/types.rs (1)
from_ptr(19-24)dash/src/blockdata/transaction/mod.rs (1)
txid(208-223)
🪛 LanguageTool
dash-spv-ffi/tests/README.md
[uncategorized] ~100-~100: Loose punctuation mark.
Context: ...s ## Test Dependencies - serial_test: Ensures tests run serially to avoid con...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 checkmake (0.2.2)
dash-spv-ffi/tests/c_tests/Makefile
[warning] 33-33: Target body for "test" exceeds allowed length of 5 (8).
(maxbodylength)
🪛 Gitleaks (8.26.0)
dash-spv-ffi/tests/security/test_security.rs
373-373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (44)
Cargo.toml (1)
2-2: Approve: FFI crate added to workspace members.The new
dash-spv-ffientry correctly integrates the FFI crate into the workspace.dash-spv/src/storage/types.rs (2)
3-4: Approve: Import reordered.Moving
use std::collections::HashMapbelowserdeimports aligns with the established grouping conventions.
11-14: Approve: Trailing whitespace cleanup.Removal of extraneous blank/trailing spaces streamlines the formatting without affecting functionality.
Also applies to: 24-29, 33-43, 67-67
dash-spv-ffi/tests/security/mod.rs (1)
1-1: Approve: Security test module included.Adding
mod test_security;brings the new security tests into the suite as intended.dash-spv/src/network/constants.rs (2)
31-31: Approve: Flattened TESTNET_DNS_SEEDS formatting.Condensing the testnet DNS seeds into one line is purely stylistic and keeps the constant concise.
44-44: Approve: New MESSAGE_RECEIVE_TIMEOUT constant.The addition of
MESSAGE_RECEIVE_TIMEOUTmatches the existing timeout pattern and fills a previous gap.dash-spv/examples/simple_sync.rs (3)
3-3: Approve: Imports reordered.Listing
init_loggingfirst, followed byClientConfigandDashSpvClient, improves readability by grouping by responsibility.
12-13: Approve: Comment trailing-space cleanup.Removed unnecessary spaces after chained method comments to maintain consistency.
39-39: Approve: Trailing newline trimmed.Cleanup of the final closing brace line preserves file cleanliness.
dash-network-ffi/src/lib.rs (2)
125-127: Align FFINetworkInfo::magictest for Regtest
Updated the expected magic value to0xDCB7C1FCto match coredash-networkmappings.
133-136: Align FFIfrom_magictest for Regtest
Now correctly expectsfrom_magic(0xDCB7C1FC)to succeed for Regtest.dash-network/src/lib.rs (3)
39-39: Align Regtest magic bytes infrom_magicmapping
Updated the Regtest arm to0xDCB7C1FCfor consistency with the FFI layer.
115-118: Update expected magic number intest_network_magic
Reflects the new Regtest magic value0xDCB7C1FC.
123-126: Update expected value intest_network_from_magic
Now assertsNetwork::from_magic(0xDCB7C1FC)yieldsSome(Network::Regtest).dash-spv-ffi/tests/performance/mod.rs (1)
1-1: Verify performance benchmarks module is available
Ensure thatdash-spv-ffi/tests/performance/test_benchmarks.rsexists.#!/bin/bash # Check for the presence of the benchmarks module file fd --glob test_benchmarks.rs dash-spv-ffi/tests/performancedash-spv-ffi/tests/integration/mod.rs (1)
1-2: Verify integration test modules
Confirm thattest_full_workflow.rsandtest_cross_language.rsexist underdash-spv-ffi/tests/integration.#!/bin/bash # Check for the presence of integration test files fd --glob test_full_workflow.rs dash-spv-ffi/tests/integration fd --glob test_cross_language.rs dash-spv-ffi/tests/integrationdash-spv/src/error.rs (1)
11-11: Whitespace-only changes (trailing space removal)
No semantic modifications; formatting-only edits.Also applies to: 14-14, 17-17, 20-20, 23-23, 32-32, 34-34, 39-39, 42-42, 48-48, 58-58, 64-64, 72-72, 132-132
dash-spv/src/network/discovery.rs (1)
18-25: Whitespace-only changes – looks goodNo functional impact; readability is slightly improved.
dash-spv/src/client/status_display.rs (1)
120-129: Good refactor – async call chain is now easier to readSplitting the
.awaitchain over multiple lines improves readability without altering behaviour.dash-spv-ffi/README.md (1)
21-24: Header output path in README disagrees withMakefileREADME says the header is emitted to
include/dash_spv_ffi.h, but the Makefile generates it in the project root.
Readers following the README will get a “file not found” error when compiling C code.Please adjust either the Makefile or the documentation so both point to the same location.
dash-spv-ffi/tests/unit/test_configuration.rs (1)
38-42: Possible type mismatch inFFIString::from_ptrusage
from_ptr(seesrc/types.rs) expects a raw*const c_char, but hereretrieved.ptris wrapped in anFFIString. Confirm the signature:let path_str = FFIString::from_ptr(retrieved.ptr)?;If
from_ptrinstead expectsFFIString, compilation will fail. Adjust call or provide a dedicated helper.dash-spv-ffi/tests/test_utils.rs (1)
31-37: Potential memory leak fordash_spv_ffi_versionIf
dash_spv_ffi_versionallocates (vs. returning a static), the pointer is never freed. Providedash_spv_ffi_string_destroyor document that the string is static.-let version_ptr = dash_spv_ffi_version(); +let version_ptr = dash_spv_ffi_version(); +// If non-static, remember to free: +// dash_spv_ffi_string_destroy(version_ptr);dash-spv-ffi/Cargo.toml (1)
19-21: Reduce Tokio feature set to trim the FFI artefact
tokio = { features = ["full"] }pulls in time, fs, macros, sync, tracing, etc.
If the FFI layer only needs the runtime and I/O primitives, consider a narrower set (e.g.["rt-multi-thread", "macros", "io-util"]).
This shaves several MB off the staticlib and speeds up link time.Please double-check which Tokio sub-crates are actually referenced from
dash-spv-ffi.
If most of them are unused, downsizing is a quick binary-size win.dash-spv-ffi/tests/test_types.rs (1)
52-59: Good validation ofFFIArray– nice jobThe test exercises construction, slice access, and correct destruction which are the major foot-guns for an FFI vector wrapper.
No issues spotted here.dash-spv/src/client/block_processor.rs (1)
185-192:new_outpoints_to_watchis populated but never consumedThe vector is built for every block yet is not used afterwards, wasting memory and code complexity.
Either remove the collection or forward it to the component that handles dynamic watch-lists.
dash-spv/src/network/multi_peer.rs (2)
17-18: Import cleanup is correct.The new imports of
ClientConfig, aliasedSpvError as Error,NetworkManager, andTcpConnectionare required and appropriately grouped.Also applies to: 24-24
172-174: These formatting changes (multiline.await) only affect style and do not alter behavior.dash-spv/src/network/mod.rs (1)
21-21: Add import forNetworkMessageis necessary.Importing
NetworkMessagehere ensures the trait signatures that reference it compile correctly.dash-spv/src/client/mod.rs (2)
481-484: Potential busy loop whencfheader_gap_check_interval_secs == 0
Duration::from_secs(0)makeslast_filter_gap_check.elapsed() >= intervaltrue every iteration, hammering the gap-check code.Validate the config and enforce
>= 1(or treat0as “disabled”).
240-245: Wallet is wired to an in-memory placeholder – persistence is silently lost
placeholder_storageis always a freshMemoryStorageManager, meaning theWalletnever uses the realstorageselected just above (which may be a disk backend).
Balances, UTXOs and address metadata written by the wallet will therefore disappear between restarts, defeating the whole purpose ofenable_persistence.-let placeholder_storage = Arc::new(RwLock::new( - crate::storage::MemoryStorageManager::new().await.map_err(|e| SpvError::Storage(e))?, -)); -let wallet = Arc::new(RwLock::new(crate::wallet::Wallet::new(placeholder_storage))); +// Re-use the same storage backend that the rest of the client uses +let wallet_storage = Arc::new(RwLock::new(self.storage.as_any_mut())); // or clone/Arc around Box +let wallet = Arc::new(RwLock::new(crate::wallet::Wallet::new(wallet_storage)));Depending on
Wallet::new’s signature you may need to wrap the existingstoragein anArc<RwLock<…>>or expose aclone_box()on the trait. In any case, both components must share the same backend.Likely an incorrect or invalid review comment.
dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
167-214: Unsynchronised FFI calls from multiple threads – verify thread-safety contract
dash_spv_ffi_client_get_*are invoked concurrently on the same raw pointer guarded only by a RustMutex.
If the underlying C API is not explicitly documented as thread-safe, this can cause UB.Consider wrapping the FFI client in a safe Rust type that enforces Send/Sync based on the C-side guarantees, or gate the test behind a
cfg(feature="unsafe-concurrency").dash-spv/src/main.rs (1)
41-42: CLI requires clap ≥4 (ArgAction::Append) – confirm workspace version
ArgAction::Appendexists since clap v4. EnsureCargo.tomlpinsclap = "4"or users building with older lockfiles will break.dash-spv-ffi/tests/unit/test_wallet_operations.rs (1)
293-322: Raw client pointer is used from multiple threads without any proven Send/Sync contract
FFIDashSpvClientis an opaque C handle; unless the underlying library
explicitly documents thread-safety, handing the same pointer to several
threads may lead to races inside the Rust core or in C callbacks.Please double-check the FFI docs and consider:
- providing a thread-safe wrapper on the Rust side, or
- serialising FFI calls with a
Mutexguard kept for the entire call.dash-spv-ffi/tests/integration/test_cross_language.rs (1)
239-243: Pointer arithmetic uses the wrong integer type – does not compile
offsetexpects anisize. Cast explicitly:-let mut len = 0; -while *c_str.offset(len) != 0 { - len += 1; -} -assert_eq!(len as usize, test_str.len()); +let mut len: isize = 0; +while *c_str.offset(len) != 0 { + len += 1; +} +assert_eq!(len as usize, test_str.len());Likely an incorrect or invalid review comment.
dash-spv/src/client/message_handler.rs (1)
50-263: Looks good – no functional changes.
Pure re-formatting; behaviour remains intact.dash-spv/src/client/filter_sync.rs (1)
46-165: Formatting-only diff acknowledged.
Implementation unchanged; nothing to action.dash-spv-ffi/tests/c_tests/test_advanced.c (1)
170-199: Thread-safety of shared client is unclear.
test_concurrent_accessdrives the sameFFIDashSpvClientfrom four
threads. Unless the FFI explicitly guarantees internal synchronisation, this
is undefined behaviour in C and UB in Rust. Consider:
- Documenting the client as thread-safe and using
Send + Syncbehind the
handle, or- Creating one client per thread for the test.
Tagging for verification with the library authors.
dash-spv-ffi/tests/security/test_security.rs (1)
370-378: Hex constant is a test vector, not a leaked key – safe to ignore.dash-spv-ffi/src/error.rs (1)
48-59: To locate theSpvErrordefinition and verify whether it includes aWalletvariant, let’s search across the codebase with ripgrep:#!/bin/bash # Find the enum definition of SpvError and inspect its variants rg -n "enum SpvError" -A 20This will show where
SpvErroris declared and list the variants so we can confirm ifWalletexists.dash-spv-ffi/tests/c_tests/test_integration.c (2)
37-42: Fix potential null pointer dereference in printfSimilar to the block callback, the ternary operator should ensure a valid string is always passed to
printf.- printf("Transaction %s: confirmed=%d\n", txid ? txid : "null", confirmed); + printf("Transaction %s: confirmed=%d\n", txid ? txid : "(null)", confirmed);Likely an incorrect or invalid review comment.
30-35: Fix potential null pointer dereference in printfThe ternary operator should return a string literal when
hashis null, not the null pointer itself. Passing null toprintfwith%sis undefined behavior.- printf("New block at height %u: %s\n", height, hash ? hash : "null"); + printf("New block at height %u: %s\n", height, hash ? hash : "(null)");Likely an incorrect or invalid review comment.
dash-spv-ffi/src/types.rs (2)
207-215: ```shell
#!/bin/bashShow the definition of FFIArray and its methods in types.rs
rg -A5 "struct FFIArray" dash-spv-ffi/src/types.rs
rg -A10 "impl FFIArray" dash-spv-ffi/src/types.rs--- `47-57`: Here’s a more reliable search to locate the `Network` enum and list its variants: ```shell #!/bin/bash # Search for the definition of the `Network` enum to list all variants rg -n '^enum\s+Network' -A10dash-spv-ffi/tests/unit/test_async_operations.rs (1)
240-274: Stack-allocated callback context passed across threads
ReentrantDatais allocated on the test stack and its pointer is handed to the async FFI operation.
If the FFI implementation invokes the callback on a background thread after the test function returns (or if the test panics), that pointer becomes dangling, leading to UAF.Safer patterns:
- Box the data and leak it, reclaiming it in the completion callback, or
- Extend the lifetime by putting it in an
Arcand clone/drop appropriately.-let reentrant_data = ReentrantData { /* … */ }; -let callbacks = FFICallbacks { - // … - user_data: &reentrant_data as *const _ as *mut c_void, -}; +let reentrant_data = Box::new(ReentrantData { /* … */ }); +let callbacks = FFICallbacks { + // … + user_data: Box::into_raw(reentrant_data) as *mut c_void, +}; // free in completion callback once no longer needed
|
@CodeRabbit rereview |
|
Tip For best results, initiate chat on the files or code changes. @QuantumExplorer I'll provide a comprehensive re-review of this substantial FFI implementation. This is an impressive piece of work that adds a complete C interface to the Dash SPV client. 🎯 Overall AssessmentThis PR introduces a well-structured FFI layer with comprehensive testing, documentation, and proper C bindings. The implementation demonstrates good understanding of FFI best practices and memory safety concerns. ✅ StrengthsArchitecture & Design
Memory Management
Error Handling
Testing Excellence
🔧 Technical ImplementationFFI Best Practices// Excellent example of proper FFI design
extern "C" int dash_spv_ffi_client_start(FFIDashSpvClient* client) {
null_check!(client, FFIErrorCode::NullPointer as i32);
// Safe conversion and error handling
}Build System Integration
🛡️ Security ConsiderationsInput Validation
Memory Safety
📋 Minor ObservationsDocumentationThe README and examples are comprehensive, but consider adding:
API DesignThe callback system is well-designed, but consider:
TestingExcellent test coverage, but could benefit from:
🔄 Breaking ChangesThe Regtest network magic bytes change ( 🎉 Recommendations
📊 Code Quality Metrics
🚀 ConclusionThis is a high-quality implementation that significantly enhances the Dash SPV client's accessibility. The FFI layer is well-designed, thoroughly tested, and follows best practices for memory safety and error handling. The comprehensive testing suite and documentation make this a production-ready addition. Recommendation: APPROVE ✅ This FFI implementation represents a substantial and valuable addition to the codebase that will enable broader ecosystem integration while maintaining the security and reliability standards of the core library. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor
Chores