feat: additional ffi methods for chain tip #145
Conversation
WalkthroughAdds FFI APIs for tip queries, storage clearing, and sync-state reset; introduces sync-with-progress and progress object destruction; switches the FFI client storage backend from in-memory to disk with a storage-path fallback; adds corresponding async methods in core client; extends C/Swift headers and FFI docs and refines cancel-sync behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant FFI as FFI Layer
participant Client as DashSpvClient
participant Chain as ChainState
Caller->>FFI: dash_spv_ffi_client_get_tip_height(client, out_height)
FFI->>Client: tip_height()
Client->>Chain: read tip height
Chain-->>Client: u32
Client-->>FFI: status + writes out_height
FFI-->>Caller: return i32
sequenceDiagram
autonumber
actor Caller
participant FFI as FFI Layer
participant Client as DashSpvClient
participant Storage as DiskStorageManager
Caller->>FFI: dash_spv_ffi_client_clear_storage(client)
FFI->>Client: clear_storage()
Client->>Storage: clear all persisted data
Storage-->>Client: Result
Client-->>FFI: map Result -> i32
FFI-->>Caller: return i32
sequenceDiagram
autonumber
actor Caller
participant FFI as FFI Layer
participant Client as DashSpvClient
participant Monitor as monitor_network (async)
participant Callbacks as progress/completion callbacks
Caller->>FFI: dash_spv_ffi_client_sync_to_tip_with_progress(...)
FFI->>Client: start sync task (spawn)
Client->>Monitor: monitor_network (async) -> emits progress
Monitor-->>FFI: progress events
FFI-->>Callbacks: invoke progress_callback(...)
alt Cancellation requested
Caller->>FFI: dash_spv_ffi_client_cancel_sync()
FFI->>Client: signal shutdown + join_active_threads
Client->>Monitor: AbortHandle -> cancels monitor
Client-->>FFI: call completion_callback(false, err_msg)
else Completed normally
Monitor-->>FFI: completion
FFI-->>Callbacks: completion_callback(true, NULL)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
dash-spv-ffi/include/dash_spv_ffi.h (1)
424-456: New tip/clear FFI surface looks consistent with impl; add one doc noteAPIs match the Rust impl and pointer safety notes are correct. Suggest documenting that clear_storage attempts to stop the client before wiping to avoid surprises.
/** * Clear all persisted SPV storage (headers, filters, metadata, sync state). * * # Safety * - `client` must be a valid, non-null pointer. + * - This call will attempt to stop the client before clearing to avoid races with in‑flight writes. */ int32_t dash_spv_ffi_client_clear_storage(struct FFIDashSpvClient *client) ;dash-spv-ffi/FFI_API.md (2)
1019-1034: Callout that clear_storage stops the clientDoc the stop-before-clear behavior to align with FFI impl and set user expectations.
**Description:** -Clear all persisted SPV storage (headers, filters, metadata, sync state). # Safety - `client` must be a valid, non-null pointer. +Clear all persisted SPV storage (headers, filters, metadata, sync state). +This call attempts to stop the client first to avoid races with in‑flight writes. +# Safety - `client` must be a valid, non-null pointer.
1051-1081: Clarify hash byte order and buffer sizeAdd an explicit note that out_hash must be exactly 32 bytes and uses internal (little‑endian) byte order as returned by dashcore hashes.
**Description:** -Get the current chain tip hash (32 bytes) if available. # Safety - `client` must be a valid, non-null pointer. - `out_hash` must be a valid pointer to a 32-byte buffer. +Get the current chain tip hash (32 bytes) if available. +Bytes are copied exactly as stored (32‑byte hash array). Caller must provide a buffer of exactly 32 bytes. +# Safety - `client` must be a valid, non-null pointer. - `out_hash` must be a valid pointer to a 32-byte buffer.dash-spv/src/client/mod.rs (2)
109-114: clear_storage: consider “stopped” preconditionMethod itself is fine; recommend documenting that it should be called when the client is stopped (FFI does this), or guard here if called directly.
/// Clear all persisted storage (headers, filters, state, sync state). pub async fn clear_storage(&mut self) -> Result<()> { + // Recommended: only call when not running to avoid interfering tasks. let mut storage = self.storage.lock().await; storage.clear().await.map_err(SpvError::Storage) }
115-119: clear_sync_state: same “stopped” caveatSafe as-is; add a note mirroring clear_storage about preferred usage when stopped.
dash-spv-ffi/src/client.rs (2)
149-162: Temp-dir fallback for storage_path: add an env/OS overrideReasonable fallback; consider honoring an env var (e.g., DASH_SPV_DATA_DIR) and/or platform-appropriate dirs on iOS/Android.
- let storage_path = client_config.storage_path.clone().unwrap_or_else(|| { + let storage_path = client_config.storage_path.clone().unwrap_or_else(|| { + if let Ok(p) = std::env::var("DASH_SPV_DATA_DIR") { + return std::path::PathBuf::from(p); + } let mut path = std::env::temp_dir(); path.push("dash-spv"); path.push(format!("{:?}", client_config.network).to_lowercase()); tracing::warn!( "dash-spv FFI config missing storage path, falling back to temp dir {:?}", path ); path });
1159-1191: clear_sync_state while running can surprise callersSafe but may race with periodic saves; consider documenting “call when stopped” or perform a best‑effort pause similar to clear_storage.
pub unsafe extern "C" fn dash_spv_ffi_client_clear_sync_state( client: *mut FFIDashSpvClient, ) -> i32 { @@ - let res = spv_client.clear_sync_state().await; + // Optional: pause client to avoid concurrent writes of sync_state.json + // let _ = spv_client.stop().await; + let res = spv_client.clear_sync_state().await;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dash-spv-ffi/FFI_API.md(7 hunks)dash-spv-ffi/include/dash_spv_ffi.h(1 hunks)dash-spv-ffi/src/client.rs(4 hunks)dash-spv/src/client/mod.rs(1 hunks)swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/client/mod.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rs
dash-spv-ffi/include/dash_spv_ffi.h
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
The public C header is generated; ensure it reflects FFI changes after builds
Files:
dash-spv-ffi/include/dash_spv_ffi.h
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
dash-spv-ffi/include/dash_spv_ffi.hswift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.hdash-spv-ffi/src/client.rs
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h
📄 CodeRabbit inference engine (swift-dash-core-sdk/CLAUDE.md)
After updating dash-spv-ffi, synchronize the generated header into the Swift SDK using ./sync-headers.sh so Sources/DashSPVFFI/include/dash_spv_ffi.h stays up to date
Files:
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h
swift-dash-core-sdk/**
📄 CodeRabbit inference engine (AGENTS.md)
Mobile SDK code resides in swift-dash-core-sdk/
Files:
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: FFI functions must be declared with #[no_mangle] extern "C"
Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Accept input strings from C as *const c_char (borrowed; do not free inputs)
Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Add cbindgen annotations for complex types so headers are generated correctly
Files:
dash-spv-ffi/src/client.rs
🧠 Learnings (18)
📓 Common learnings
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
📚 Learning: 2025-08-16T04:15:57.225Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : When accessing SPV functionality, use DashSDK public APIs instead of direct client access; add public wrapper methods to DashSDK if needed
Applied to files:
dash-spv/src/client/mod.rsdash-spv-ffi/include/dash_spv_ffi.hswift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.hdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Applied to files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
Applied to files:
dash-spv-ffi/include/dash_spv_ffi.hswift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.hdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h : After updating dash-spv-ffi, synchronize the generated header into the Swift SDK using ./sync-headers.sh so Sources/DashSPVFFI/include/dash_spv_ffi.h stays up to date
Applied to files:
dash-spv-ffi/include/dash_spv_ffi.hswift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.hdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift} : Be careful with memory management at FFI boundaries (C/Swift interop)
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : FFI functions must be declared with #[no_mangle] extern "C"
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
dash-spv-ffi/FFI_API.md
🧬 Code graph analysis (4)
dash-spv/src/client/mod.rs (4)
dash-spv/src/types.rs (2)
tip_hash(241-243)tip_height(229-238)dash-spv/src/storage/memory.rs (1)
clear_sync_state(392-397)dash-spv/src/storage/disk.rs (1)
clear_sync_state(1697-1704)dash-spv/src/storage/mod.rs (1)
clear_sync_state(188-188)
dash-spv-ffi/include/dash_spv_ffi.h (1)
dash-spv-ffi/src/client.rs (4)
dash_spv_ffi_client_get_tip_hash(1018-1063)dash_spv_ffi_client_get_tip_height(1071-1110)dash_spv_ffi_client_clear_storage(1117-1152)dash_spv_ffi_client_clear_sync_state(1159-1191)
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h (1)
dash-spv-ffi/src/client.rs (4)
dash_spv_ffi_client_get_tip_hash(1018-1063)dash_spv_ffi_client_get_tip_height(1071-1110)dash_spv_ffi_client_clear_storage(1117-1152)dash_spv_ffi_client_clear_sync_state(1159-1191)
dash-spv-ffi/src/client.rs (5)
dash-spv/src/client/mod.rs (4)
storage(2397-2399)network(432-434)network(1299-1302)new(198-265)dash-spv/tests/reverse_index_test.rs (1)
storage(61-61)dash-spv/src/network/multi_peer.rs (1)
new(77-127)dash-spv/src/storage/disk.rs (1)
new(210-324)dash-spv-ffi/src/error.rs (2)
set_last_error(26-31)from(53-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
🔇 Additional comments (10)
dash-spv/src/client/mod.rs (2)
97-101: tip_hash: simple, correct, non-blockingRead-lock on state and Option return is appropriate.
103-107: tip_height: correct aggregation with checkpoint baseMatches ChainState::tip_height semantics.
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h (2)
152-167: New progress callback wiring looks goodSignature matches emitted event (filter_header_height, header_height, percentage) and is added to FFIEventCallbacks.
424-456: Vendored header matches generated header — no action required.
dash-spv-ffi/include/dash_spv_ffi.h and swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h are identical (same size and SHA256 checksum).dash-spv-ffi/src/client.rs (6)
8-8: Switch to DiskStorageManager: good moveEnables persistence and aligns with downstream client usage.
106-112: Type alias updated to DiskStorageManagerAlias change is coherent with the new storage backend.
1012-1063: get_tip_hash: pointer checks and copy are correctNull checks, take/put pattern, and 32‑byte copy are solid.
1071-1110: get_tip_height: LGTMNull checks and out param write are correct.
1117-1152: clear_storage: graceful stop before wipeGood precaution; matches header/doc suggestion.
11-11: Keep import: use dash_spv::Hashdash-spv re-exports Hash via
pub use dashcore::hashes::Hash;(dash-spv/src/lib.rs:82), souse dash_spv::Hash;is valid — keep as-is.Likely an incorrect or invalid review comment.
| **Auto-generated**: This documentation is automatically generated from the source code. Do not edit manually. | ||
|
|
||
| **Total Functions**: 64 | ||
| **Total Functions**: 68 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify function count matches headers
Ensure “Total Functions: 68” reflects the current header set.
🏁 Script executed:
#!/bin/bash
# Compare API doc function count vs generated C header
COUNT_HEADER=$(rg -nP '^\s*(?:int32_t|bool|void|struct [A-Za-z_]\w*\s*\*)\s+dash_spv_ffi_\w+\s*\(' dash-spv-ffi/include/dash_spv_ffi.h | wc -l)
COUNT_DOC=$(rg -n '^\*\*Total Functions\*\*:\s*\d+' dash-spv-ffi/FFI_API.md | sed -E 's/.*: *([0-9]+)/\1/')
echo "Header: $COUNT_HEADER, Doc: $COUNT_DOC"
test "$COUNT_HEADER" = "$COUNT_DOC"Length of output: 19
Mismatch — update FFI API count or header
dash-spv-ffi/FFI_API.md (line 7) shows "Total Functions: 68" but dash-spv-ffi/include/dash_spv_ffi.h currently exposes 51 dash_spv_ffi_* functions. Update the doc to 51 or regenerate/commit the header to include the missing 17 functions.
🤖 Prompt for AI Agents
In dash-spv-ffi/FFI_API.md around line 7, the "Total Functions: 68" header is
inconsistent with dash-spv-ffi/include/dash_spv_ffi.h which currently exposes 51
dash_spv_ffi_* functions; either update the markdown count to "Total Functions:
51" or regenerate/commit the header to actually expose the missing 17 functions
so the two stay in sync — choose the correct source of truth (prefer
regenerating the header if the codebase should have 68 functions, otherwise
update the doc), then modify FFI_API.md line 7 to the correct number and commit
the change (or update the header file and run the generator, add/commit the new
header).
There was a problem hiding this comment.
Actionable comments posted: 0
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/Cargo.toml (1)
1-12: Add rust-version = "1.89" (MSRV) to workspace or crate Cargo.tomlVerified: root Cargo.toml and dash-spv-ffi/Cargo.toml are missing rust-version. Preferred: add to workspace Cargo.toml; otherwise add to dash-spv-ffi/Cargo.toml.
[package] name = "dash-spv-ffi" version = { workspace = true } edition = "2021" +rust-version = "1.89"
♻️ Duplicate comments (1)
dash-spv-ffi/FFI_API.md (1)
7-7: Sync API count: please re-verify header vs doc totalPast mismatch reappeared intermittently. Recheck that “Total Functions: 68” matches the generated C header count and update one or the other accordingly.
Run:
#!/bin/bash # Compare API doc function count vs generated C header set -euo pipefail HEADER="dash-spv-ffi/include/dash_spv_ffi.h" DOC="dash-spv-ffi/FFI_API.md" COUNT_HEADER=$(rg -nP '^\s*(?:int32_t|bool|void|struct [A-Za-z_]\w*\s*\*|FFIResult|FFIArray|FFIString)\s+dash_spv_ffi_\w+\s*\(' "$HEADER" | wc -l) COUNT_DOC=$(rg -n '^\*\*Total Functions\*\*:\s*\d+' "$DOC" | sed -E 's/.*: *([0-9]+)/\1/') printf "Header: %s, Doc: %s\n" "$COUNT_HEADER" "$COUNT_DOC" test "$COUNT_HEADER" = "$COUNT_DOC" # Also assert new prototypes are present in the header required=( dash_spv_ffi_client_get_tip_hash dash_spv_ffi_client_get_tip_height dash_spv_ffi_client_clear_storage dash_spv_ffi_client_clear_sync_state ) for sym in "${required[@]}"; do rg -nP "\b${sym}\s*\(" "$HEADER" >/dev/null || { echo "Missing: $sym"; exit 1; } done echo "OK"
🧹 Nitpick comments (11)
dash-spv-ffi/FFI_API.md (4)
639-640: Clarify cancel semantics (doc only clears sync callbacks, not event callbacks)The implementation only unregisters sync progress/completion callbacks; event callbacks remain set. Reword to avoid implying that all callbacks are cleared.
Apply:
-Cancels the sync operation. This stops the SPV client, clears callbacks, and joins active threads so the sync operation halts immediately. # Safety The client pointer must be valid and non-null. # Returns Returns 0 on success, or an error code on failure. +Cancels the sync operation. This stops the SPV client, unregisters sync progress/completion callbacks, and waits for active sync threads to exit (typically within ~100ms). Event callbacks remain registered. # Safety The client pointer must be valid and non-null. # Returns Returns 0 on success, or an error code on failure.
1025-1032: Document that clear_storage stops the client and requires restartThe code stops the client before wiping storage to avoid races. Make that explicit to prevent surprises.
-Clear all persisted SPV storage (headers, filters, metadata, sync state). # Safety - `client` must be a valid, non-null pointer. +Clear all persisted SPV storage (headers, filters, metadata, sync state). The client will be stopped if running; call `dash_spv_ffi_client_start` again to resume. # Safety - `client` must be a valid, non-null pointer.
1052-1061: Specify hash byte order (endianness) for out_hashWithout this, C/Swift callers risk reversing bytes incorrectly.
-Get the current chain tip hash (32 bytes) if available. # Safety - `client` must be a valid, non-null pointer. - `out_hash` must be a valid pointer to a 32-byte buffer. +Get the current chain tip hash as 32 raw bytes in internal (little‑endian) order as produced by dashcore::BlockHash (not display/reversed order). # Safety - `client` must be a valid, non-null pointer. - `out_hash` must be a valid pointer to a 32-byte buffer.
1067-1078: Mention “unknown tip” case for heightCallers should know the return path when the chain isn’t initialized.
-Get the current chain tip height (absolute). # Safety - `client` must be a valid, non-null pointer. - `out_height` must be a valid, non-null pointer. +Get the current chain tip height (absolute). If the tip is not yet known, an error code is returned and `out_height` is not written. # Safety - `client` must be a valid, non-null pointer. - `out_height` must be a valid, non-null pointer.dash-spv-ffi/src/client.rs (6)
156-169: Avoid temp directory as default persistent storageFalling back to
std::env::temp_dir()risks data loss (OS may purge). Prefer an app data dir (e.g., usingdirectories/dirs-next) or require the caller to setstorage_path. At minimum, support an override env var.Example minimal tweak:
- let storage_path = client_config.storage_path.clone().unwrap_or_else(|| { - let mut path = std::env::temp_dir(); + let storage_path = client_config.storage_path.clone().unwrap_or_else(|| { + if let Ok(p) = std::env::var("DASH_SPV_DATA_DIR") { + return std::path::PathBuf::from(p); + } + let mut path = std::env::temp_dir(); path.push("dash-spv"); path.push(format!("{:?}", client_config.network).to_lowercase()); tracing::warn!( "dash-spv FFI config missing storage path, falling back to temp dir {:?}", path ); path });
217-228: JoinHandle accumulation (possible growth over repeated syncs)Handles for progress/sync threads are pushed into
active_threadsand only joined on stop/cancel/destroy. Repeated syncs can grow this vec even after threads exit.Options:
- Track only long‑lived listener threads; don’t store short‑lived sync/progress handles (drop them).
- Or spawn a lightweight reaper that
join()s finished handles and removes them from the vec.- Or switch to tokio tasks and
JoinSeton the runtime to manage lifecycle deterministically.Also applies to: 846-848, 941-943
389-425: Cancel/stop clears only sync callbacks (not event callbacks) — align code or docs
stop_client_internalunregisterssync_callbacksbut leavesevent_callbacksintact. The doc originally implied “clears callbacks” broadly. Either also clearevent_callbackshere or (preferable) keep as-is and ensure the docs state that event callbacks persist across cancel/stop.Would you like me to update the doc phrasing (already suggested) and add a brief rustdoc comment here to cement the contract?
1089-1105: Return a more precise error for “no tip yet”
Ok(None)maps toStorageError. Consider returning a dedicated code (e.g., NotFound/NoData) to distinguish “not initialized” from genuine storage failures.If
FFIErrorCodehas a NotFound/NoData variant, we can switch:- Ok(None) => { - set_last_error("No tip hash available"); - FFIErrorCode::StorageError as i32 - } + Ok(None) => { + set_last_error("No tip hash available"); + FFIErrorCode::NotFound as i32 + }Confirm the variant name before changing.
798-844: FFIDetailedSyncProgress lifetime: only valid during callbackYou allocate a boxed progress and pass a pointer; it’s dropped right after the callback. This is fine, but callers must treat the pointer as ephemeral. Add an explicit note in FFI_API.md under
dash_spv_ffi_client_sync_to_tip_with_progressthat the progress pointer is only valid for the duration of the call and must not be retained.I can add the doc line if you want.
1165-1183: Stopping before clear_storage is great — also reset shutdown flag deterministicallyYou stop the client before clearing. Consider setting
shutdown_signaltrue around this op to ensure listeners exit quickly, then leave it false on return (similar to cancel path), avoiding any window where event thread keeps polling while storage is being wiped.dash-spv-ffi/Cargo.toml (1)
24-25: Duplicate env_logger dependency
env_logger = "0.10"appears in both dependencies and dev-dependencies. Keep one (usually dev-dependencies unless used in the library).-[dependencies] -… -env_logger = "0.10" +[dependencies] +… [dev-dependencies] tempfile = "3.8" serial_test = "3.0" -env_logger = "0.10" +env_logger = "0.10"Also applies to: 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dash-spv-ffi/Cargo.toml(1 hunks)dash-spv-ffi/FFI_API.md(8 hunks)dash-spv-ffi/include/dash_spv_ffi.h(2 hunks)dash-spv-ffi/src/client.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv-ffi/include/dash_spv_ffi.h
🧰 Additional context used
📓 Path-based instructions (6)
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.mddash-spv-ffi/src/client.rs
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
MSRV is 1.89; set and respect rust-version = "1.89" in Cargo.toml
Files:
dash-spv-ffi/Cargo.toml
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: FFI functions must be declared with #[no_mangle] extern "C"
Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Accept input strings from C as *const c_char (borrowed; do not free inputs)
Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Add cbindgen annotations for complex types so headers are generated correctly
Files:
dash-spv-ffi/src/client.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv-ffi/src/client.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
dash-spv-ffi/src/client.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv-ffi/src/client.rs
🧠 Learnings (19)
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
Applied to files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/Cargo.toml : Set MSRV to 1.89 using the `rust-version` field and keep the code compatible with Rust 1.89
Applied to files:
dash-spv-ffi/Cargo.toml
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Applied to files:
dash-spv-ffi/Cargo.toml
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : FFI functions must be declared with #[no_mangle] extern "C"
Applied to files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.mddash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.mddash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv-ffi/Cargo.tomldash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : Enforce Rust formatting via `cargo fmt --check` on all Rust source files
Applied to files:
dash-spv-ffi/Cargo.toml
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h : After updating dash-spv-ffi, synchronize the generated header into the Swift SDK using ./sync-headers.sh so Sources/DashSPVFFI/include/dash_spv_ffi.h stays up to date
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift} : Be careful with memory management at FFI boundaries (C/Swift interop)
Applied to files:
dash-spv-ffi/FFI_API.mddash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv-ffi/FFI_API.mddash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/callbacks.rs : Use C function pointers for async progress/completion callbacks; callbacks may be invoked from any thread
Applied to files:
dash-spv-ffi/src/client.rs
🧬 Code graph analysis (1)
dash-spv-ffi/src/client.rs (3)
dash-spv/src/client/mod.rs (5)
storage(2397-2399)wallet(129-131)network(432-434)network(1299-1302)new(198-265)dash-spv/src/network/multi_peer.rs (2)
new(77-127)clone(968-991)dash-spv/src/storage/disk.rs (1)
new(210-324)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Integration Tests (stable, 22.1.3)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Key Wallet Components Tests
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_witness)
🔇 Additional comments (1)
dash-spv-ffi/Cargo.toml (1)
26-26: futures added — confirm no newer MSRV-bound featuresUsing
AbortHandle/Abortablefromfutures0.3 is fine with MSRV 1.89; just ensure nofutures-utilfeatures sneak in that raise MSRV. CI should guard via clippy + MSRV.
Summary by CodeRabbit
New Features
Refactor
Documentation