refactor: fix last clippy warnings in dash_spv #131
Conversation
WalkthroughTests were refactored to prefer by-value/header Copy semantics, modernized assertions, reduced clones, and added lint suppressions and small helpers. The FFI crate received a major client lifecycle and event/callback surface expansion and an internal refactor to avoid holding locks across await points. Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host (FFI caller)
participant FFIC as FFIDashSpvClient
participant Inner as InnerClient (async)
participant Listener as EventListener Thread
participant Registry as CallbackRegistry
Host->>FFIC: dash_spv_ffi_client_new(config)
FFIC->>Inner: construct client (spawn runtime)
Host->>FFIC: dash_spv_ffi_client_start()
FFIC->>Inner: start() -- (take guard, perform async outside lock)
FFIC->>Listener: start_event_listener()
loop event loop
Listener->>Inner: recv event
Inner-->>Listener: Event(...)
Listener->>Registry: forward event
Registry->>Host: invoke callback(s)
end
Host->>FFIC: dash_spv_ffi_client_stop()/dash_spv_ffi_client_destroy()
FFIC->>Inner: stop() -- (take guard, stop async, reinstate)
Listener->>FFIC: shutdown signal (thread exit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (4)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
dash-spv/tests/smart_fetch_integration_test.rs (2)
20-25: Fix overlap check: use AND, not ORTo assert window overlaps [1000,1100], require both conditions.
-assert!(window.mining_end >= 1000 || window.mining_start <= 1100); +assert!(window.mining_end >= 1000 && window.mining_start <= 1100);
182-188: Same overlap check issue here: use ANDEnsure windows truly intersect [120,144].
-assert!(window.mining_end >= 120 || window.mining_start <= 144); +assert!(window.mining_end >= 120 && window.mining_start <= 144);dash-spv/src/chain/fork_detector_test.rs (3)
232-248: Type inference clash: nonce expression needs u32; fork_height is usize.
fork_heightdrivesthread_id/itousize(forheader_at_height), butcreate_test_headerwantsu32fornonce. Cast the nonce expression tou32(consistent with earlier tests usingi as u32).- let fork_header = create_test_header( - fork_point_header.block_hash(), - 1000 + thread_id * 100 + i, - ); + let fork_header = create_test_header( + fork_point_header.block_hash(), + (1000 + thread_id * 100 + i) as u32, + );
287-301: Fix moved value used after move (genesis).
genesisis moved at Line 290 then used at Line 299. Capture hash before the move.- let genesis = genesis_block(Network::Dash).header; - storage.store_header(&genesis, 0).expect("Failed to store genesis"); - chain_state.add_header(genesis); + let genesis = genesis_block(Network::Dash).header; + let genesis_hash = genesis.block_hash(); + storage.store_header(&genesis, 0).expect("Failed to store genesis"); + chain_state.add_header(genesis); @@ - let future_header = create_test_header_with_time(genesis.block_hash(), 3, u32::MAX); + let future_header = create_test_header_with_time(genesis_hash, 3, u32::MAX);
355-366: Fix moved value used after move (genesis).- let genesis = genesis_block(Network::Dash).header; - storage.store_header(&genesis, 0).expect("Failed to store genesis"); - chain_state.add_header(genesis); + let genesis = genesis_block(Network::Dash).header; + let genesis_hash = genesis.block_hash(); + storage.store_header(&genesis, 0).expect("Failed to store genesis"); + chain_state.add_header(genesis); @@ - let header1 = create_test_header(genesis.block_hash(), 1); + let header1 = create_test_header(genesis_hash, 1);
🧹 Nitpick comments (15)
dash-spv/tests/reverse_index_test.rs (2)
24-24: Nit: remove redundant cast for consistencyi is already u32; align with Line 76.
- assert_eq!(height, Some(i as u32), "Height mismatch for header {}", i); + assert_eq!(height, Some(i), "Height mismatch for header {}", i);
54-54: Nit: remove redundant cast for consistencySame as Line 24.
- assert_eq!(height, Some(i as u32), "Height mismatch for header {}", i); + assert_eq!(height, Some(i), "Height mismatch for header {}", i);dash-spv/src/client/message_handler.rs (1)
36-48: Optional: consider bundling ctor argsA small Context/Deps struct (grouping storage, network, stats, channels) would drop the need for the allow and tighten the API.
dash-spv/src/chain/checkpoints.rs (1)
428-455: Optional: replace many params with a builder/params structA CheckpointParams builder would improve readability and avoid the lint altogether.
dash-spv/src/chain/chainlock_test.rs (1)
100-102: Preserve error context in panic.Using unwrap_or_else is fine for clippy, but the current closure drops the actual error value. Include it to aid debugging.
- .unwrap_or_else(|_| { - panic!("ChainLock at height {} should process successfully", height) - }); + .unwrap_or_else(|e| { + panic!("ChainLock at height {} should process successfully: {e}", height) + });dash-spv/tests/header_sync_test.rs (1)
140-140: Keep the error value in the panic for easier triage.Same behavior, but don’t discard Err(e).
- .unwrap_or_else(|_| panic!("Failed to store batch {}-{}", batch_start, batch_end)); + .unwrap_or_else(|e| panic!("Failed to store batch {}-{}: {e}", batch_start, batch_end));dash-spv/src/validation/headers_edge_test.rs (1)
301-301: Duplicate header without cloning — confirm Copy semanticsUsing
vec![header, header]relies onBlockHeader: Copy. If that's guaranteed in this crate, great. If not, this won’t compile; revert to cloning just for this test.Optional:
vec![header; 2]also reads well and is concise ifClone/Copyis implemented.dash-spv/src/chain/chain_tip.rs (2)
275-275: Pass-by-value header in test — LGTM; precompute hash to avoid reuse-of-moved var if neededIf
BlockHeaderever stops beingCopy, precomputing the hash avoids use-after-move in tests.Apply:
- // The extend operation should succeed - let result = manager.extend_tip(&tip1_hash, new_header, new_work); + // The extend operation should succeed + let new_tip_hash = new_header.block_hash(); + let result = manager.extend_tip(&tip1_hash, new_header, new_work); @@ - // The new tip should exist - let new_tip_hash = new_header.block_hash(); + // The new tip should exist
318-319: Same here — consider precomputing hashKeeps the pattern consistent and robust against future API/type changes.
- let result = manager.extend_tip(&tip1_hash, new_header, new_work); + let new_hash = new_header.block_hash(); + let result = manager.extend_tip(&tip1_hash, new_header, new_work); @@ - assert!(manager.get_tip(&new_header.block_hash()).is_some()); + assert!(manager.get_tip(&new_hash).is_some());dash-spv/src/chain/reorg.rs (2)
552-552: Avoid borrow-and-move of the same value in a single callSlight refactor improves readability and prevents borrow-after-move pitfalls if
BlockHeaderever isn’tCopy.- let tip = ChainTip::new(genesis, 0, ChainWork::from_header(&genesis)); + let work = ChainWork::from_header(&genesis); + let tip = ChainTip::new(genesis, 0, work);
576-576: Repeat the borrow/move avoidance patternMirror the earlier refactor for consistency.
- let tip = ChainTip::new(genesis, 100, ChainWork::from_header(&genesis)); + let work = ChainWork::from_header(&genesis); + let tip = ChainTip::new(genesis, 100, work);dash-spv/src/chain/orphan_pool_test.rs (2)
173-175: Reliance on Header: Copy semantics — please confirmThis pushes header then reuses it, which requires Header to be Copy. If that ever changes upstream, this will fail. If you want to avoid that coupling, store only hashes in headers and compare by hash in assertions.
Possible tweak (keeps intent, removes Copy reliance):
for i in 0..5 { let header = create_test_header(parent, i); - headers.push(header); - pool.add_orphan(header); + headers.push(header.block_hash()); + pool.add_orphan(header); } @@ - let orphan_set: HashSet<_> = orphans.iter().map(|h| h.block_hash()).collect(); - let header_set: HashSet<_> = headers.iter().map(|h| h.block_hash()).collect(); + let orphan_set: HashSet<_> = orphans.iter().map(|h| h.block_hash()).collect(); + let header_set: HashSet<_> = headers.iter().copied().collect();
352-352: Optional: compare by hash to decouple from Header equality/CopyThis assertion currently depends on Header being Copy + Eq. Comparing by hash avoids that.
-assert_eq!(removed.header, header); +assert_eq!(removed.header.block_hash(), hash);Also applies to: 362-362
dash-spv/tests/smart_fetch_integration_test.rs (1)
141-149: Counting blocks inside window is correct; minor nit on saturating mathCurrent logic is fine. Optional: use u64 and saturating ops to avoid any potential underflow/overflow if types change.
- if window_end >= window_start { - blocks_with_smart_fetch += (window_end - window_start + 1) as usize; - } + if window_end >= window_start { + let span = (u64::from(window_end) - u64::from(window_start) + 1) as usize; + blocks_with_smart_fetch += span; + }dash-spv/tests/storage_consistency_test.rs (1)
666-666: Type inference here is okay, but make segment_size explicitly u32 to avoid future surprises.Today it infers to
u32due to usage; an explicit annotation reduces cognitive load and prevents accidental drift if surrounding code changes.Example (outside the changed lines):
let segment_size: u32 = 50_000;Also applies to: 679-680, 695-695
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
dash-spv/src/bloom/tests.rs(2 hunks)dash-spv/src/chain/chain_tip.rs(2 hunks)dash-spv/src/chain/chainlock_test.rs(1 hunks)dash-spv/src/chain/checkpoints.rs(1 hunks)dash-spv/src/chain/fork_detector.rs(3 hunks)dash-spv/src/chain/fork_detector_test.rs(13 hunks)dash-spv/src/chain/orphan_pool.rs(4 hunks)dash-spv/src/chain/orphan_pool_test.rs(4 hunks)dash-spv/src/chain/reorg.rs(1 hunks)dash-spv/src/chain/reorg_test.rs(1 hunks)dash-spv/src/client/config_test.rs(8 hunks)dash-spv/src/client/message_handler.rs(1 hunks)dash-spv/src/network/mock.rs(1 hunks)dash-spv/src/network/tests.rs(3 hunks)dash-spv/src/storage/disk.rs(1 hunks)dash-spv/src/sync/chainlock_validation.rs(1 hunks)dash-spv/src/sync/validation_test.rs(3 hunks)dash-spv/src/validation/headers_edge_test.rs(1 hunks)dash-spv/tests/error_recovery_integration_test.rs(2 hunks)dash-spv/tests/error_types_test.rs(1 hunks)dash-spv/tests/header_sync_test.rs(1 hunks)dash-spv/tests/instantsend_integration_test.rs(3 hunks)dash-spv/tests/integration_real_node_test.rs(0 hunks)dash-spv/tests/reverse_index_test.rs(1 hunks)dash-spv/tests/smart_fetch_integration_test.rs(3 hunks)dash-spv/tests/storage_consistency_test.rs(3 hunks)dash-spv/tests/storage_test.rs(4 hunks)dash-spv/tests/transaction_calculation_test.rs(1 hunks)
💤 Files with no reviewable changes (1)
- dash-spv/tests/integration_real_node_test.rs
🧰 Additional context used
📓 Path-based instructions (8)
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/network/mock.rsdash-spv/src/chain/checkpoints.rsdash-spv/src/client/message_handler.rsdash-spv/src/storage/disk.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/sync/validation_test.rsdash-spv/src/sync/chainlock_validation.rsdash-spv/src/chain/chain_tip.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/src/client/config_test.rsdash-spv/src/chain/chainlock_test.rsdash-spv/src/network/tests.rsdash-spv/tests/header_sync_test.rsdash-spv/src/validation/headers_edge_test.rsdash-spv/tests/error_types_test.rsdash-spv/src/bloom/tests.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/src/chain/fork_detector.rsdash-spv/src/chain/orphan_pool.rsdash-spv/src/chain/reorg.rsdash-spv/src/chain/fork_detector_test.rsdash-spv/tests/smart_fetch_integration_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.rsdash-spv/src/chain/reorg_test.rsdash-spv/src/chain/orphan_pool_test.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement DNS-first peer discovery with the specified seeds, immediate startup (no initial discovery delay), and exclusive peer mode when explicit peers are provided
Files:
dash-spv/src/network/mock.rsdash-spv/src/network/tests.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/network/mock.rsdash-spv/src/chain/checkpoints.rsdash-spv/src/client/message_handler.rsdash-spv/src/storage/disk.rsdash-spv/tests/reverse_index_test.rsdash-spv/src/sync/validation_test.rsdash-spv/src/sync/chainlock_validation.rsdash-spv/src/chain/chain_tip.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/src/client/config_test.rsdash-spv/src/chain/chainlock_test.rsdash-spv/src/network/tests.rsdash-spv/tests/header_sync_test.rsdash-spv/src/validation/headers_edge_test.rsdash-spv/tests/error_types_test.rsdash-spv/src/bloom/tests.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/src/chain/fork_detector.rsdash-spv/src/chain/orphan_pool.rsdash-spv/src/chain/reorg.rsdash-spv/src/chain/fork_detector_test.rsdash-spv/tests/smart_fetch_integration_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.rsdash-spv/src/chain/reorg_test.rsdash-spv/src/chain/orphan_pool_test.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/network/mock.rsdash-spv/src/chain/checkpoints.rsdash-spv/src/client/message_handler.rsdash-spv/src/storage/disk.rsdash-spv/src/sync/validation_test.rsdash-spv/src/sync/chainlock_validation.rsdash-spv/src/chain/chain_tip.rsdash-spv/src/client/config_test.rsdash-spv/src/chain/chainlock_test.rsdash-spv/src/network/tests.rsdash-spv/src/validation/headers_edge_test.rsdash-spv/src/bloom/tests.rsdash-spv/src/chain/fork_detector.rsdash-spv/src/chain/orphan_pool.rsdash-spv/src/chain/reorg.rsdash-spv/src/chain/fork_detector_test.rsdash-spv/src/chain/reorg_test.rsdash-spv/src/chain/orphan_pool_test.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Provide both
MemoryStorageManagerandDiskStorageManagerbehind theStorageManagertrait
Files:
dash-spv/src/storage/disk.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Place integration tests under the
tests/directory
Files:
dash-spv/tests/reverse_index_test.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/error_types_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/smart_fetch_integration_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.rs
{dash-spv,rpc-integration-test}/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide integration tests for network operations
Files:
dash-spv/tests/reverse_index_test.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/error_types_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/smart_fetch_integration_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic
Files:
dash-spv/tests/reverse_index_test.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/error_types_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/smart_fetch_integration_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.rs
🧠 Learnings (21)
📓 Common learnings
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`
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
📚 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/chain/checkpoints.rsdash-spv/src/client/message_handler.rsdash-spv/src/storage/disk.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/src/network/tests.rsdash-spv/tests/header_sync_test.rsdash-spv/src/bloom/tests.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.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/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash-spv/src/sync/validation_test.rsdash-spv/tests/transaction_calculation_test.rsdash-spv/src/client/config_test.rsdash-spv/src/network/tests.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/src/chain/fork_detector_test.rsdash-spv/tests/smart_fetch_integration_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.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-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/tests/transaction_calculation_test.rsdash-spv/src/client/config_test.rsdash-spv/src/network/tests.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/error_types_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/storage_test.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/tests/transaction_calculation_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.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/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Applied to files:
dash-spv/src/chain/chainlock_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.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/tests/**/*.rs : Place integration tests under the `tests/` directory
Applied to files:
dash-spv/src/network/tests.rsdash-spv/tests/error_types_test.rsdash-spv/src/bloom/tests.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/error_recovery_integration_test.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Applied to files:
dash-spv/src/network/tests.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/tests/error_types_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_recovery_integration_test.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/**/*.rs : Use the custom Error type; propagate errors with the ? operator and provide contextual messages
Applied to files:
dash-spv/tests/error_types_test.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/src/**/*.rs : Place unit tests alongside code with #[cfg(test)]
Applied to files:
dash-spv/src/bloom/tests.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/tests/instantsend_integration_test.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/tests/**/*.rs : Include integration tests that cover full wallet workflows
Applied to files:
dash-spv/tests/instantsend_integration_test.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/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Applied to files:
dash-spv/tests/instantsend_integration_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/error_recovery_integration_test.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/tests/address_tests.rs : Place address generation and validation tests in tests/address_tests.rs
Applied to files:
dash-spv/tests/instantsend_integration_test.rsdash-spv/tests/error_recovery_integration_test.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/**/*.rs : Always validate network consistency; never mix mainnet and testnet operations
Applied to files:
dash-spv/tests/instantsend_integration_test.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/address_pool/**/*.rs : Pre-generate addresses in bounded batches (typically 20–100) and store them in pools
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Keep test vectors deterministic
Applied to files:
dash-spv/tests/error_recovery_integration_test.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/tests/**/*.rs : Use deterministic testing with known vectors (fixed seeds) in Rust tests
Applied to files:
dash-spv/tests/error_recovery_integration_test.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/tests/mnemonic_tests.rs : Place mnemonic generation and validation tests in tests/mnemonic_tests.rs
Applied to files:
dash-spv/tests/error_recovery_integration_test.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/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv/tests/storage_test.rs
🧬 Code graph analysis (7)
dash-spv/src/sync/validation_test.rs (2)
dash-spv/src/sync/chainlock_validation.rs (1)
default(34-43)dash-spv/src/sync/validation.rs (1)
default(45-56)
dash-spv/src/network/tests.rs (3)
dash-spv/src/network/message_handler.rs (2)
stats(116-118)default(14-16)dash-spv/src/network/pool.rs (1)
default(145-147)dash-spv/src/network/mod.rs (1)
get_dsq_preference(179-181)
dash-spv/tests/storage_consistency_test.rs (2)
dash-spv/src/types.rs (1)
tip_height(226-235)dash-spv/tests/segmented_storage_test.rs (1)
create_test_header(14-23)
dash-spv/src/chain/fork_detector.rs (1)
dash-spv/src/chain/fork_detector_test.rs (1)
create_test_header(14-20)
dash-spv/src/chain/reorg.rs (3)
dash-spv/src/chain/chain_tip.rs (1)
new(27-35)dash-spv/src/chain/chain_work.rs (1)
from_header(26-29)dash-spv/src/chain/reorg_test.rs (1)
test_max_reorg_depth(61-92)
dash-spv/src/chain/fork_detector_test.rs (1)
dash-spv/src/chain/fork_detector.rs (3)
create_test_header(244-249)get_forks(193-195)get_fork(198-200)
dash-spv/src/chain/orphan_pool_test.rs (1)
dash-spv/src/chain/orphan_pool.rs (1)
contains(133-135)
⏰ 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). (14)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (38)
dash-spv/tests/reverse_index_test.rs (1)
76-76: LGTM: unnecessary cast removedi is inferred as u32 here, so dropping the cast is correct.
dash-spv/src/storage/disk.rs (1)
52-52: Scoped Clippy allow is fineTargeted #[allow(clippy::enum_variant_names)] on WorkerNotification is acceptable to keep Clippy clean without broad allowances.
dash-spv/src/client/message_handler.rs (1)
35-35: Scoped Clippy allow is fineUsing #[allow(clippy::too_many_arguments)] on new() is reasonable here.
dash-spv/src/chain/checkpoints.rs (1)
427-427: Scoped Clippy allow is fineAllowing too_many_arguments on this internal helper used to build static data is pragmatic.
dash-spv/src/network/mock.rs (1)
98-102: Default impl mirrors new()Simple, correct Default delegating to new(); helps tests use ..Default::default().
dash-spv/tests/error_recovery_integration_test.rs (2)
258-263: Good change: use array literal to avoid heap allocs.Switching from Vec to a fixed-size array here is clearer and allocation-free. Iteration semantics remain the same.
393-398: Likewise: scenarios as an array reads cleaner and is zero-allocation.No behavior change; this should also please clippy.
dash-spv/src/chain/reorg_test.rs (1)
13-13: Nice: prefer Copy over Clone for BlockHeader.This removes an unnecessary clone and keeps intent clear. No downsides.
dash-spv/tests/transaction_calculation_test.rs (2)
88-89: Good: use unsigned_abs for Amount::from_sat.unsigned_abs avoids intermediate casts and is MSRV-safe (stabilized in Rust 1.51.0), which is well below the project’s MSRV 1.89.
If you want to double-check, see Rust 1.51.0 notes stating “unsigned_abs is now implemented for all signed integer types.” (releases.rs)
93-94: Same here — consistent use of unsigned_abs looks good.No behavior change; improves clarity and clamps type correctly to u64 for Amount.
dash-spv/src/sync/chainlock_validation.rs (1)
347-350: Test config init via struct literal — LGTMCleaner than mutating a default; keeps the test immutable and clear.
dash-spv/src/chain/reorg.rs (2)
560-560: Headers vec construction — LGTMUsing the same
genesisby value here is fine given currentCopysemantics.Confirm
BlockHeader: Copyremains true across MSRV 1.89 to keep tests future-proof.
567-567: Assertion style improvement — LGTM
assert!(!result.unwrap())reads better than comparing tofalse.dash-spv/src/chain/orphan_pool.rs (1)
257-257: By-value header usage in tests — LGTMRemoves unnecessary clones and aligns with
BlockHeader: Copyacross the test suite.If
BlockHeaderever dropsCopy, restore.clone()in these call sites to keep tests compiling.Also applies to: 272-272, 303-305, 320-322
dash-spv/tests/error_types_test.rs (1)
336-336: LGTM: use of io::Error::other for clarityDirect constructor improves readability; behavior unchanged.
dash-spv/src/chain/orphan_pool_test.rs (2)
75-78: Good move to by-value addsPassing headers by value removes needless clones and keeps tests idiomatic.
154-156: Iterator-based membership checks read betterSwitch to iter().take()/skip() matches contains(&BlockHash) API and avoids indexing.
Also applies to: 159-161
dash-spv/src/bloom/tests.rs (2)
4-4: Allowing clippy::module_inception in tests is fineReasonable suppression for a test-only module.
671-677: Direct struct literal for config: clearer intentInitializing desired fields up front is cleaner than mutating after Default.
dash-spv/src/sync/validation_test.rs (3)
20-25: Explicit ClientConfig literal reads wellClearer than mutate-after-default; fits PR goal.
185-189: Explicit ValidationConfig is fineMatches defaults but documents intent for retries in-test.
227-231: Explicit ChainLockValidationConfig cache sizeGood: expresses the perf test’s requirement without side effects.
dash-spv/src/chain/fork_detector.rs (1)
260-260: By-value header usage in tests removes needless clones — good cleanup.Passing
BlockHeaderby value here is clear and efficient (relies onCopy), and aligns with the PR objective of eliminating redundantclone()s.Also applies to: 269-269, 311-311, 315-315
dash-spv/src/network/tests.rs (2)
110-113: Struct-literal init with ..Default is concise and clear.Keeps tests expressive without mutating a default value.
219-219: Assert style matches Clippy’s bool_assert_comparisons guidance.Switching to
assert!/!avoids lint noise.Also applies to: 224-224, 227-227
dash-spv/tests/storage_test.rs (1)
34-38: Iteration tweaks and slice args look good; fewer allocations and clearer intent.
- Using
enumerate().take(5)reads well givencreate_test_headers(5).- Switching
store_filter(.., &[...])avoids temporary Vecs.Also applies to: 114-118, 195-195, 232-234
dash-spv/tests/instantsend_integration_test.rs (1)
24-24: InstantLock construction simplification and gating are fine.Inline return plus a deterministic non-zero dummy signature is appropriate for the currently ignored test. The file-level cfg keeps this dormant until the API stabilizes.
Also applies to: 32-33, 82-90
dash-spv/src/client/config_test.rs (8)
131-134: Good switch to struct-literal init with ..Default.Clearer intent and no unintended mutations.
143-146: Good: direct construction for invalid filter_checkpoint_interval.
155-158: Good: direct construction for invalid max_peers.
167-170: Good: direct construction for invalid max_concurrent_filter_requests.
179-183: Good: mempool tracking enabled explicitly for this assertion.
192-196: Good: invalid mempool_timeout explicitly expressed.
205-210: Good: selective strategy + recent_send_window_secs=0 clearly targets the validation path.
259-262: Good: wallet_creation_time exercised via struct-literal init.dash-spv/src/chain/fork_detector_test.rs (3)
152-152: Nice assertion style cleanup.Range check via (100..=104).contains(&n) is clearer.
332-334: Iterator-based removal looks good.Using
iter().take(3)avoids indexing and is clearer.
340-341: Iterator-based lookup check looks good.
| chain_state.add_header(checkpoint_header); | ||
|
|
||
| // Add more headers building on checkpoint | ||
| let mut prev_hash = checkpoint_header.block_hash(); | ||
| for i in 1..5 { | ||
| let header = create_test_header(prev_hash, 1000 + i); | ||
| storage.store_header(&header, 1000 + i).expect("Failed to store header"); | ||
| chain_state.add_header(header.clone()); | ||
| chain_state.add_header(header); | ||
| prev_hash = header.block_hash(); |
There was a problem hiding this comment.
Fix moved value used after move (checkpoint_header and loop header).
checkpoint_header is moved into chain_state.add_header(checkpoint_header) (Line 41) and then used again (Lines 44, 55). Same pattern in the loop: header is moved (Line 48) and then used (Line 49). Record hashes before moving and (for the pre-checkpoint storage) store before the move.
Apply:
- let checkpoint_header = create_test_header(BlockHash::from([0u8; 32]), 1000);
- storage.store_header(&checkpoint_header, 1000).expect("Failed to store checkpoint");
- chain_state.add_header(checkpoint_header);
+ let checkpoint_header = create_test_header(BlockHash::from([0u8; 32]), 1000);
+ let checkpoint_hash = checkpoint_header.block_hash();
+ storage.store_header(&checkpoint_header, 1000).expect("Failed to store checkpoint");
+ // Also index it at a lower height to simulate pre-checkpoint storage
+ storage.store_header(&checkpoint_header, 500).expect("Failed to store at height 500");
+ chain_state.add_header(checkpoint_header);
@@
- let mut prev_hash = checkpoint_header.block_hash();
+ let mut prev_hash = checkpoint_hash;
@@
- storage.store_header(&checkpoint_header, 500).expect("Failed to store at height 500");
+ // already stored above
@@
- for i in 1..5 {
- let header = create_test_header(prev_hash, 1000 + i);
- storage.store_header(&header, 1000 + i).expect("Failed to store header");
- chain_state.add_header(header);
- prev_hash = header.block_hash();
- }
+ for i in 1..5 {
+ let header = create_test_header(prev_hash, 1000 + i);
+ let next_hash = header.block_hash();
+ storage.store_header(&header, 1000 + i).expect("Failed to store header");
+ chain_state.add_header(header);
+ prev_hash = next_hash;
+ }Also applies to: 55-56
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 41 to 49 (and also
apply same fix at lines 55-56), checkpoint_header and header are being moved
into chain_state.add_header(...) then used afterwards; to fix, capture the
needed values (e.g., let checkpoint_hash = checkpoint_header.block_hash())
before calling add_header and use those captured hashes when needed later, and
for the loop capture header_hash (or store to storage) before moving header into
add_header — alternatively call storage.store_header(...) before calling
chain_state.add_header(header) so you don't use a moved value afterwards; apply
the same pattern at the other occurrence (lines 55-56).
| let genesis = genesis_block(Network::Dash).header; | ||
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | ||
| chain_state.add_header(genesis.clone()); | ||
| chain_state.add_header(genesis); | ||
|
|
||
| // Build main chain | ||
| let mut main_chain_tip = genesis.block_hash(); |
There was a problem hiding this comment.
Fix moved value used after move (genesis).
genesis is moved into add_header(genesis) (Line 73) and then used at Line 76. Capture the hash before moving.
- let genesis = genesis_block(Network::Dash).header;
- storage.store_header(&genesis, 0).expect("Failed to store genesis");
- chain_state.add_header(genesis);
+ let genesis = genesis_block(Network::Dash).header;
+ let genesis_hash = genesis.block_hash();
+ storage.store_header(&genesis, 0).expect("Failed to store genesis");
+ chain_state.add_header(genesis);
@@
- let mut main_chain_tip = genesis.block_hash();
+ let mut main_chain_tip = genesis_hash;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis.clone()); | |
| chain_state.add_header(genesis); | |
| // Build main chain | |
| let mut main_chain_tip = genesis.block_hash(); | |
| let genesis = genesis_block(Network::Dash).header; | |
| let genesis_hash = genesis.block_hash(); | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis); | |
| // Build main chain | |
| let mut main_chain_tip = genesis_hash; |
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 71 to 76, the variable
`genesis` is moved into `chain_state.add_header(genesis)` and then reused at
line 76; compute its block hash before the move and use that stored hash
afterwards. Concretely, call `let genesis_hash = genesis.block_hash();` (or
otherwise clone/extract the hash) immediately after creating `genesis`, keep
using `genesis` for `add_header(...)`, and use `genesis_hash` when you need the
tip/hash later.
| let header = create_test_header(main_chain_tip, i); | ||
| storage.store_header(&header, i).expect("Failed to store header"); | ||
| chain_state.add_header(header.clone()); | ||
| chain_state.add_header(header); | ||
| main_chain_tip = header.block_hash(); | ||
| } |
There was a problem hiding this comment.
Fix moved value used after move in the loop (header).
header is moved on Line 81 and then used on Line 81/82. Save the hash first.
- let header = create_test_header(main_chain_tip, i);
- storage.store_header(&header, i).expect("Failed to store header");
- chain_state.add_header(header);
- main_chain_tip = header.block_hash();
+ let header = create_test_header(main_chain_tip, i);
+ let next_hash = header.block_hash();
+ storage.store_header(&header, i).expect("Failed to store header");
+ chain_state.add_header(header);
+ main_chain_tip = next_hash;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let header = create_test_header(main_chain_tip, i); | |
| storage.store_header(&header, i).expect("Failed to store header"); | |
| chain_state.add_header(header.clone()); | |
| chain_state.add_header(header); | |
| main_chain_tip = header.block_hash(); | |
| } | |
| let header = create_test_header(main_chain_tip, i); | |
| let next_hash = header.block_hash(); | |
| storage.store_header(&header, i).expect("Failed to store header"); | |
| chain_state.add_header(header); | |
| main_chain_tip = next_hash; |
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 78 to 82, the variable
`header` is moved by `chain_state.add_header(header)` and then
`header.block_hash()` is called afterwards; capture the header's block hash into
a local variable before calling `add_header` (e.g., let hash =
header.block_hash();) and then use that saved `hash` for `main_chain_tip` (and
any other subsequent uses) to avoid using a moved value.
| let genesis = genesis_block(Network::Dash).header; | ||
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | ||
| chain_state.add_header(genesis.clone()); | ||
| chain_state.add_header(genesis); | ||
|
|
||
| // Build main chain past genesis | ||
| let header1 = create_test_header(genesis.block_hash(), 1); | ||
| storage.store_header(&header1, 1).expect("Failed to store header"); | ||
| chain_state.add_header(header1.clone()); | ||
| chain_state.add_header(header1); | ||
|
|
There was a problem hiding this comment.
Fix moved value used after move (genesis used to build header1).
genesis is moved at Line 125 then used at Line 128. Store the hash first.
- let genesis = genesis_block(Network::Dash).header;
- storage.store_header(&genesis, 0).expect("Failed to store genesis");
- chain_state.add_header(genesis);
+ let genesis = genesis_block(Network::Dash).header;
+ let genesis_hash = genesis.block_hash();
+ storage.store_header(&genesis, 0).expect("Failed to store genesis");
+ chain_state.add_header(genesis);
@@
- let header1 = create_test_header(genesis.block_hash(), 1);
+ let header1 = create_test_header(genesis_hash, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis.clone()); | |
| chain_state.add_header(genesis); | |
| // Build main chain past genesis | |
| let header1 = create_test_header(genesis.block_hash(), 1); | |
| storage.store_header(&header1, 1).expect("Failed to store header"); | |
| chain_state.add_header(header1.clone()); | |
| chain_state.add_header(header1); | |
| let genesis = genesis_block(Network::Dash).header; | |
| let genesis_hash = genesis.block_hash(); | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis); | |
| // Build main chain past genesis | |
| let header1 = create_test_header(genesis_hash, 1); | |
| storage.store_header(&header1, 1).expect("Failed to store header"); | |
| chain_state.add_header(header1); |
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 123 to 131, genesis is
moved into store_header/add_header and then its block_hash() is called to build
header1; compute and store the genesis hash before the move (e.g. let
genesis_hash = genesis.block_hash();) and then use genesis_hash when calling
create_test_header(...) so you don't use genesis after it has been moved.
| let genesis = genesis_block(Network::Dash).header; | ||
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | ||
| chain_state.add_header(genesis.clone()); | ||
| chain_state.add_header(genesis); | ||
|
|
||
| // Build main chain past genesis | ||
| let header1 = create_test_header(genesis.block_hash(), 1); | ||
| storage.store_header(&header1, 1).expect("Failed to store header"); | ||
| chain_state.add_header(header1.clone()); | ||
| chain_state.add_header(header1); | ||
|
|
There was a problem hiding this comment.
Fix moved value used after move (genesis used to build header1).
Same pattern: capture hash before moving.
- let genesis = genesis_block(Network::Dash).header;
- storage.store_header(&genesis, 0).expect("Failed to store genesis");
- chain_state.add_header(genesis);
+ let genesis = genesis_block(Network::Dash).header;
+ let genesis_hash = genesis.block_hash();
+ storage.store_header(&genesis, 0).expect("Failed to store genesis");
+ chain_state.add_header(genesis);
@@
- let header1 = create_test_header(genesis.block_hash(), 1);
+ let header1 = create_test_header(genesis_hash, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis.clone()); | |
| chain_state.add_header(genesis); | |
| // Build main chain past genesis | |
| let header1 = create_test_header(genesis.block_hash(), 1); | |
| storage.store_header(&header1, 1).expect("Failed to store header"); | |
| chain_state.add_header(header1.clone()); | |
| chain_state.add_header(header1); | |
| let genesis = genesis_block(Network::Dash).header; | |
| let genesis_hash = genesis.block_hash(); | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis); | |
| // Build main chain past genesis | |
| let header1 = create_test_header(genesis_hash, 1); | |
| storage.store_header(&header1, 1).expect("Failed to store header"); | |
| chain_state.add_header(header1); |
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 162 to 170, the test
moves `genesis` when calling `chain_state.add_header(genesis)` and then attempts
to use it to build `header1`, causing a use-after-move; fix by capturing the
genesis block hash into a separate variable (e.g., `let genesis_hash =
genesis.block_hash();`) before the move and then use `genesis_hash` when
creating `header1` so `genesis` can still be moved into `add_header`.
| let genesis = genesis_block(Network::Dash).header; | ||
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | ||
| chain_state.lock().unwrap().add_header(genesis.clone()); | ||
| chain_state.lock().unwrap().add_header(genesis); | ||
|
|
||
| // Build a base chain | ||
| let mut prev_hash = genesis.block_hash(); | ||
| for i in 1..20 { | ||
| let header = create_test_header(prev_hash, i); | ||
| storage.store_header(&header, i).expect("Failed to store header"); | ||
| chain_state.lock().unwrap().add_header(header.clone()); | ||
| chain_state.lock().unwrap().add_header(header); | ||
| prev_hash = header.block_hash(); | ||
| } |
There was a problem hiding this comment.
Two fixes: moved value after move (genesis) and moved header inside loop.
- Capture
genesis_hashbefore moving. - In the loop, record
next_hashbefore movingheader.
- let genesis = genesis_block(Network::Dash).header;
- storage.store_header(&genesis, 0).expect("Failed to store genesis");
- chain_state.lock().unwrap().add_header(genesis);
+ let genesis = genesis_block(Network::Dash).header;
+ let genesis_hash = genesis.block_hash();
+ storage.store_header(&genesis, 0).expect("Failed to store genesis");
+ chain_state.lock().unwrap().add_header(genesis);
@@
- let mut prev_hash = genesis.block_hash();
+ let mut prev_hash = genesis_hash;
@@
- let header = create_test_header(prev_hash, i);
- storage.store_header(&header, i).expect("Failed to store header");
- chain_state.lock().unwrap().add_header(header);
- prev_hash = header.block_hash();
+ let header = create_test_header(prev_hash, i);
+ let next_hash = header.block_hash();
+ storage.store_header(&header, i).expect("Failed to store header");
+ chain_state.lock().unwrap().add_header(header);
+ prev_hash = next_hash;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.lock().unwrap().add_header(genesis.clone()); | |
| chain_state.lock().unwrap().add_header(genesis); | |
| // Build a base chain | |
| let mut prev_hash = genesis.block_hash(); | |
| for i in 1..20 { | |
| let header = create_test_header(prev_hash, i); | |
| storage.store_header(&header, i).expect("Failed to store header"); | |
| chain_state.lock().unwrap().add_header(header.clone()); | |
| chain_state.lock().unwrap().add_header(header); | |
| prev_hash = header.block_hash(); | |
| } | |
| let genesis = genesis_block(Network::Dash).header; | |
| let genesis_hash = genesis.block_hash(); | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.lock().unwrap().add_header(genesis); | |
| // Build a base chain | |
| let mut prev_hash = genesis_hash; | |
| for i in 1..20 { | |
| let header = create_test_header(prev_hash, i); | |
| let next_hash = header.block_hash(); | |
| storage.store_header(&header, i).expect("Failed to store header"); | |
| chain_state.lock().unwrap().add_header(header); | |
| prev_hash = next_hash; | |
| } |
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 208 to 219, the test
moves `genesis` into store_header/add_header then later needs its hash and
similarly moves `header` inside the loop before using its block_hash; fix by
capturing genesis_hash = genesis.block_hash() (or genesis.header.block_hash()
depending on context) before calling store_header/add_header, and inside the
loop compute let next_hash = header.block_hash() (or header.header.block_hash())
before moving `header` into storage/chain_state so you use the saved hash
afterward rather than using a moved value.
| let genesis = genesis_block(Network::Dash).header; | ||
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | ||
| chain_state.add_header(genesis.clone()); | ||
| chain_state.add_header(genesis); | ||
|
|
||
| // Build main chain past genesis | ||
| let header1 = create_test_header(genesis.block_hash(), 1); | ||
| storage.store_header(&header1, 1).expect("Failed to store header"); | ||
| chain_state.add_header(header1.clone()); | ||
| chain_state.add_header(header1); | ||
|
|
There was a problem hiding this comment.
Fix moved value used after move (genesis).
- let genesis = genesis_block(Network::Dash).header;
- storage.store_header(&genesis, 0).expect("Failed to store genesis");
- chain_state.add_header(genesis);
+ let genesis = genesis_block(Network::Dash).header;
+ let genesis_hash = genesis.block_hash();
+ storage.store_header(&genesis, 0).expect("Failed to store genesis");
+ chain_state.add_header(genesis);
@@
- let header1 = create_test_header(genesis.block_hash(), 1);
+ let header1 = create_test_header(genesis_hash, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis.clone()); | |
| chain_state.add_header(genesis); | |
| // Build main chain past genesis | |
| let header1 = create_test_header(genesis.block_hash(), 1); | |
| storage.store_header(&header1, 1).expect("Failed to store header"); | |
| chain_state.add_header(header1.clone()); | |
| chain_state.add_header(header1); | |
| let genesis = genesis_block(Network::Dash).header; | |
| let genesis_hash = genesis.block_hash(); | |
| storage.store_header(&genesis, 0).expect("Failed to store genesis"); | |
| chain_state.add_header(genesis); | |
| // Build main chain past genesis | |
| let header1 = create_test_header(genesis_hash, 1); | |
| storage.store_header(&header1, 1).expect("Failed to store header"); | |
| chain_state.add_header(header1); |
🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 311 to 319, the test
moves `genesis` into `chain_state.add_header(genesis)` and then tries to use
`genesis.block_hash()` afterwards causing a "use after move" error; fix it by
computing and storing the genesis hash (e.g. let genesis_hash =
genesis.block_hash()) before calling `chain_state.add_header(genesis)` and then
use `genesis_hash` when creating `header1`, or alternatively clone `genesis`
(e.g. pass `genesis.clone()` to add_header) if cloning is acceptable.
| let genesis = genesis_block(Network::Dash).header; | ||
| chain_state.add_header(genesis.clone()); | ||
| chain_state.add_header(genesis); | ||
|
|
||
| let header1 = create_test_header(genesis.block_hash(), 1); | ||
| chain_state.add_header(header1.clone()); | ||
| chain_state.add_header(header1); | ||
|
|
||
| let header2 = create_test_header(header1.block_hash(), 2); | ||
| chain_state.add_header(header2.clone()); | ||
| chain_state.add_header(header2); | ||
|
|
||
| // Try to extend from header1 (in chain state but not storage) |
There was a problem hiding this comment.
Fix moved value used after move (genesis, header1).
Both genesis and then header1 are moved and reused later. Capture hashes before moving.
- let genesis = genesis_block(Network::Dash).header;
- chain_state.add_header(genesis);
+ let genesis = genesis_block(Network::Dash).header;
+ let genesis_hash = genesis.block_hash();
+ chain_state.add_header(genesis);
@@
- let header1 = create_test_header(genesis.block_hash(), 1);
- chain_state.add_header(header1);
+ let header1 = create_test_header(genesis_hash, 1);
+ let header1_hash = header1.block_hash();
+ chain_state.add_header(header1);
@@
- let header2 = create_test_header(header1.block_hash(), 2);
- chain_state.add_header(header2);
+ let header2 = create_test_header(header1_hash, 2);
+ let header2_hash = header2.block_hash();
+ chain_state.add_header(header2);
@@
- let header3 = create_test_header(header1.block_hash(), 3);
+ let header3 = create_test_header(header1_hash, 3);🤖 Prompt for AI Agents
In dash-spv/src/chain/fork_detector_test.rs around lines 376 to 385, genesis and
header1 are moved into chain_state.add_header(...) and then their .block_hash()
is called later, causing use-after-move; capture the needed hashes before the
values are moved by assigning let genesis_hash = genesis.block_hash(); and let
header1_hash = header1.block_hash(); (or similarly named variables) and then use
those stored hashes in subsequent calls instead of calling .block_hash() on the
moved values.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
dash-spv-ffi/src/types.rs (1)
290-299: Avoid unchecked Layout; handle invalid align/size gracefully.Use
Layout::from_size_alignto prevent UB ifelem_alignis malformed. Fall back to no-op dealloc when layout is invalid.- // Deallocate the vector buffer using the original layout - use std::alloc::{dealloc, Layout}; - let size = (*arr).elem_size.saturating_mul((*arr).capacity); - if size > 0 && (*arr).elem_align.is_power_of_two() && (*arr).elem_align > 0 { - // Safety: elem_size/elem_align were recorded from the original Vec<T> - let layout = Layout::from_size_align_unchecked(size, (*arr).elem_align); - unsafe { dealloc((*arr).data as *mut u8, layout) }; - } + use std::alloc::{dealloc, Layout}; + let size = (*arr).elem_size.saturating_mul((*arr).capacity); + if let Ok(layout) = Layout::from_size_align(size, (*arr).elem_align) { + if size > 0 { + unsafe { dealloc((*arr).data as *mut u8, layout) }; + } + } // else: skip dealloc to avoid UB; memory was likely not allocated as recordeddash-spv-ffi/src/client.rs (2)
196-241: Potential deadlock: don’t hold event_callbacks mutex while invoking user callbacks.You lock event_callbacks, then call callbacks.call_* which may synchronously enter user code. If user code re-enters FFI (e.g., set_event_callbacks/stop), it can deadlock on the same mutex.
Snapshot the function pointers and user_data, drop the lock, then invoke:
- let callbacks = event_callbacks.lock().unwrap(); + // Snapshot callbacks and drop the lock before invoking user code + let callbacks = { + let cb = event_callbacks.lock().unwrap(); + cb.clone() // Derive Clone (and Copy) for FFIEventCallbacks + };If FFIEventCallbacks isn’t Clone+Copy yet, derive it where defined:
- pub struct FFIEventCallbacks { ... } + #[derive(Clone, Copy, Default)] + pub struct FFIEventCallbacks { ... }Alternatively, extract individual fn pointers + user_data into locals under the lock and then call them outside the lock. I can wire this fully if you prefer the latter approach.
1194-1215: FFI txid as C string: prefer binary [u8; 32] per FFI guidance.Hex parsing on the FFI boundary is slower and error-prone. Per learnings, txids should be passed as fixed-size byte arrays in FFI.
Add a binary variant and deprecate the string-based one:
#[no_mangle] pub unsafe extern "C" fn dash_spv_ffi_client_record_send_bytes( client: *mut FFIDashSpvClient, txid: *const [u8; 32], ) -> i32 { null_check!(client); null_check!(txid); let txid = dashcore::Txid::from_byte_array(*txid); // or from_slice if preferred // then reuse the existing body (lock/take/reinsert) calling record_transaction_send(txid) }Keep the existing function for backwards compatibility, but encourage callers to use the binary API in docs and headers.
dash-spv-ffi/include/dash_spv_ffi.h (2)
49-49: Fix invalid typedef for FFIClientConfig (compile-breaker).
typedef ClientConfig FFIClientConfig;references an undefinedClientConfigin this header. Forward-declare the struct to makeFFIClientConfig*usable from C, or adjust cbindgen to emit a proper opaque typedef.Apply this header-side fix (ensure generator emits the equivalent):
-typedef ClientConfig FFIClientConfig; +typedef struct ClientConfig FFIClientConfig;
1-5: Sync generated FFI header with Swift SDKswift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h is out of sync with dash-spv-ffi/include/dash_spv_ffi.h (missing imports and function declarations). Re-run
cbindgenand./sync-headers.shto regenerate and update the Swift SDK header.
🧹 Nitpick comments (35)
test-utils/src/helpers.rs (2)
39-41: Good addition; minor DRY nit.Consider delegating to
len()to keep a single locking path.Apply within this hunk:
- pub fn is_empty(&self) -> bool { - self.data.lock().unwrap().is_empty() - } + pub fn is_empty(&self) -> bool { + self.len() == 0 + }
93-97: Default impl is fine; could be derived to reduce boilerplate.Current impl is correct. Optionally replace with
#[derive(Default)]on the struct and drop this block.Apply in this hunk:
-impl Default for ErrorInjector { - fn default() -> Self { - Self::new() - } -}And elsewhere, update the struct declaration:
#[derive(Default)] pub struct ErrorInjector { should_fail: Arc<Mutex<bool>>, fail_count: Arc<Mutex<usize>>, }dash-spv-ffi/tests/unit/test_type_conversions.rs (1)
131-137: Optional: iterate by value and compare enums directly (avoid repr casts)Slightly cleaner and more idiomatic; also removes the need for explicit derefs and integer casts. If
FFINetwork: PartialEq, consider:- for (ffi_net, dash_net) in networks.iter() { - let converted: dashcore::Network = (*ffi_net).into(); - assert_eq!(converted, *dash_net); - - let back: FFINetwork = (*dash_net).into(); - assert_eq!(back as i32, *ffi_net as i32); - } + for (ffi_net, dash_net) in networks { + let converted: dashcore::Network = ffi_net.into(); + assert_eq!(converted, dash_net); + + let back: FFINetwork = dash_net.into(); + assert_eq!(back, ffi_net); + }If
FFINetworklacksPartialEq, keep the cast-based equality but still iterate by value:- for (ffi_net, dash_net) in networks.iter() { + for (ffi_net, dash_net) in networks { let converted: dashcore::Network = ffi_net.into(); - assert_eq!(converted, *dash_net); + assert_eq!(converted, dash_net); - let back: FFINetwork = dash_net.into(); - assert_eq!(back as i32, *ffi_net as i32); + let back: FFINetwork = dash_net.into(); + assert_eq!(back as i32, ffi_net as i32); }.github/workflows/rust.yml (1)
226-237: Keep multi-Dash Core coverage in the matrix (or justify narrowing).Reducing to a single
dashversionrisks missing regressions across supported cores. Consider restoring the previous dual-version run (or add a nightly/weekly matrix) to retain coverage.matrix: - rust: [stable] - dashversion: ["22.1.3"] + rust: [stable] + dashversion: ["22.0.0", "22.1.3"]dash-spv-ffi/src/types.rs (1)
341-364: String-array destroy path looks correct; minor nit: document Box ownership assumption.You correctly free each boxed FFIString then the buffer. Add one line noting elements must come from
Box::into_raw./// - Each element pointer must be valid or null; non-null entries are freed. /// - The memory referenced by `arr.data` must not be used after this call. +/// - Each element must have been allocated via `Box::into_raw(FFIString)`.dash-spv-ffi/tests/test_event_callbacks.rs (1)
240-240: Same note for c"info".Confirm MSRV or switch to CString as above.
- dash_spv_ffi_init_logging(c"info".as_ptr()); + let level = CString::new("info").unwrap(); + dash_spv_ffi_init_logging(level.as_ptr());dash-spv-ffi/src/client.rs (11)
134-145: Runtime sizing: consider making worker threads configurable.Hard-coding 4 threads may be suboptimal across platforms. Read a setting from FFIClientConfig (or env) with a sane default.
- .worker_threads(4) // Use 4 threads for better performance on iOS + .worker_threads(config.runtime_threads().unwrap_or(4))If FFIClientConfig lacks this, I can add a field plus cbindgen annotations. Want a patch?
149-164: Storage backend is fixed to MemoryStorageManager — verify intent.This discards any persistent storage choice present in config and makes FFI clients ephemeral. If not intended (e.g., on mobile), select storage based on config.
Possible sketch:
- let storage = dash_spv::storage::MemoryStorageManager::new().await; + let storage = match client_config.storage_backend { + StorageBackend::Memory => dash_spv::storage::MemoryStorageManager::new().await.map(Storage::Memory), + StorageBackend::Sled { ref path } => dash_spv::storage::SledStorageManager::new(path).await.map(Storage::Sled), + }?;(Adjust to actual types/enums in dash-spv.)
282-301: Normalize mempool callbacks to pass typed hashes.Currently txid and block_hash appear to be forwarded as Strings. For consistency and to avoid hex parsing on the C side, parse to dashcore::{Txid, BlockHash} and pass 32-byte arrays in FFI shims (matches how on_transaction uses a [u8;32] in tests).
Example:
- callbacks.call_mempool_transaction_added(txid, amount, addresses, is_instant_send); + if let Ok(tx) = txid.parse::<dashcore::Txid>() { + callbacks.call_mempool_transaction_added(&tx, amount, addresses, is_instant_send); + } else { + tracing::error!("Failed to parse mempool txid: {}", txid); + }Apply similar parsing for confirmed/removed paths.
390-430: Start: event listener lifecycle should be idempotent.Consider tracking a boolean (e.g., listener_started: AtomicBool) to avoid accidental multiple spawns, and to start the listener if callbacks are set post-start but before first listener spawn.
+ listener_started: AtomicBool, ... - client.start_event_listener(); + if !client.listener_started.swap(true, Ordering::Relaxed) { + client.start_event_listener(); + }Initialize to false in new().
592-667: Test sync helper: make delay configurable or shorter under CI.Sleeping 10s can slow CI. Consider env-driven or parameterized shorter waits with a retry loop.
- tokio::time::sleep(Duration::from_secs(10)).await; + let secs = std::env::var("DASH_SPV_TEST_SYNC_WAIT_SECS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(5); + tokio::time::sleep(Duration::from_secs(secs)).await;
692-844: Detailed progress: pointer lifetime to FFIDetailedSyncProgress is ephemeral. Document clearly in headers.You Box the progress and pass ffi_progress.as_ref() to the callback. That pointer is only valid during the callback. Ensure cbindgen doc comments in the public header state that the pointer must not be retained.
I can add the doc comment in Rust and regenerate headers if desired.
859-897: Cancel sync: clears callbacks and stops client.Works as a coarse cancel. Consider adding an abort handle/cancellation token to actually terminate monitor_network(), then join progress thread for deterministic shutdown.
904-937: Getters returning owned pointers with matching destroy fns — good.Both getters return heap-allocated FFI types and you provided dash_spv_ffi_sync_progress_destroy and dash_spv_ffi_spv_stats_destroy. Consider adding unit tests that round-trip allocate/destroy to guard against regressions.
Also applies to: 944-977
1012-1040: Setting callbacks post-start: optionally (re)start listener if not yet spawned.Right now this only logs. If the listener hasn’t been started (e.g., callbacks set before start()), consider deferring a spawn until start() or starting here if receiver is still available.
1047-1085: Destroy: small cleanup nit.Reinserting spv_client after stop is unnecessary since the entire FFIDashSpvClient is being dropped. You can just drop it. Not a blocker.
- let _ = spv_client.stop().await; - let mut guard = client.inner.lock().unwrap(); - *guard = Some(spv_client); + let _ = spv_client.stop().await; + // Drop spv_client here
1111-1144: Rescan stub: return NotImplemented is fine; wire up when API lands.Consider feature-gating or documenting this in the C header. I can help implement once dash-spv exposes a rescan API.
dash-spv-ffi/include/dash_spv_ffi.h (12)
239-246: Document error reporting contract for client_new.If
dash_spv_ffi_client_newcan fail, specify whether it returns NULL and how to obtain the error (dash_spv_ffi_get_last_error). Keeps C/Swift callers from guessing.
366-373: Add free note to get_sync_progress docs.Mention that the returned pointer must be freed with
dash_spv_ffi_sync_progress_destroy.
374-381: Add free note to get_stats docs.Mention that the returned pointer must be freed with
dash_spv_ffi_spv_stats_destroy.
390-398: Callout: callbacks are invoked from any thread.Explicitly state that all event callbacks may be invoked from arbitrary threads and must be reentrant/non-blocking. This mirrors existing async callback guidance.
- * # Safety - * - `client` must be a valid, non-null pointer. + * # Safety + * - `client` must be a valid, non-null pointer. + * - Callbacks may be invoked from any thread; they must be thread-safe and avoid long blocking.
399-406: LGTM: destroy semantics.Accepts null and frees resources; matches Rust impl. Consider adding “pointer must not be used after this call.”
423-431: Mark rescan API as experimental or return a stable NotImplemented code.Docs say “not yet implemented.” Consider returning a dedicated
FFIErrorCode::NotImplementedand marking the function experimental to prevent accidental reliance.
432-440: Clarify preconditions for enabling mempool tracking.Specify whether the client must be started and if the call is idempotent when reapplying the same strategy.
441-448: Specify txid encoding for record_send.Document expected
txidformat (32-byte big-endian vs. little-endian, hex casing/length). Prevents subtle mismatches.- * - `client` and `txid` must be valid, non-null pointers. + * - `client` and `txid` must be valid, non-null pointers. + * - `txid` must be a 64-char lowercase hex string of the tx hash (big-endian).
751-756: Clarify array_destroy behavior (does not free the FFIArray struct).Make it explicit that only
arr->datais freed and theFFIArraywrapper is not. Optionally specify whether fields are zeroed.- * - The memory referenced by `arr.data` must not be used after this call. + * - Frees only `arr->data`; the FFIArray struct itself is not freed. + * - The memory referenced by `arr->data` must not be used after this call.
765-771: LGTM: string_array_destroy safety notes.Notes are clear. Consider adding that the
FFIArraywrapper is not freed here either (to mirrorarray_destroy).
465-470: Unify wallet-manager docs and reference wallet_manager_free
Consolidate the duplicated documentation fordash_spv_ffi_client_get_wallet_managerinto a single block and update its Safety section to instruct users to callwallet_manager_free(as defined in key-wallet-ffi) to destroy the manager.
810-817: Document accepted log levels and idempotency indash_spv_ffi_init_logging
Specify the allowedlevelvalues (trace,debug,info,warn,error), the default behavior whenlevel == NULL, and confirm whether calling this function multiple times is safe (idempotent).dash-spv-ffi/tests/unit/test_error_handling.rs (6)
3-3: Confirm import path: integration tests typically shouldn't useuse crate::*In files under tests/,
craterefers to the test crate, not the library. If this compiles due to a re-export, all good; otherwise switch to the library crate name (likelydash_spv_ffi).Apply if needed:
- use crate::*; + use dash_spv_ffi::*;
86-91: Simplify assertion by comparing the whole string directlyNo need to assert length and per-char equality; a direct equality check is clearer and cheaper.
- // Should handle long strings without truncation - assert_eq!(error_str.len(), 10000); - assert!(error_str.chars().all(|c| c == 'X')); + // Should handle long strings without truncation + assert_eq!(error_str, long_error);
96-131: Broaden coverage: include Wallet error mapping (and Unknown if applicable)You assert numeric values and mappings for several variants. Consider adding a case for
SpvError::Wallet(_)to validateFFIErrorCode::WalletErrormapping. IfSpvErrorhas an “other/unknown” catch-all, add that too forFFIErrorCode::Unknown.If you share the available
WalletErrorvariants, I can provide a concrete diff.
159-174: Clean up global error state at end of testThis test sets a global error but doesn’t clear it. Clearing avoids cross-test leakage if additional non-serial tests are added later.
unsafe { // Test with config functions let result = dash_spv_ffi_config_set_data_dir(std::ptr::null_mut(), std::ptr::null()); assert_eq!(result, FFIErrorCode::NullPointer as i32); // Check error was set let error_ptr = dash_spv_ffi_get_last_error(); assert!(!error_ptr.is_null()); let error_str = CStr::from_ptr(error_ptr).to_str().unwrap(); assert_eq!(error_str, "Null pointer provided"); } + dash_spv_ffi_clear_error();
213-218: Assert non-null before dereferencing error pointerBe consistent and defensive by asserting the pointer is non-null before
CStr::from_ptr.set_last_error("Error\nwith\nnewlines"); let error_ptr = dash_spv_ffi_get_last_error(); + assert!(!error_ptr.is_null()); unsafe { let error_str = CStr::from_ptr(error_ptr).to_str().unwrap(); assert_eq!(error_str, "Error\nwith\nnewlines"); } // Test error with tabs set_last_error("Error\twith\ttabs"); let error_ptr = dash_spv_ffi_get_last_error(); + assert!(!error_ptr.is_null()); unsafe { let error_str = CStr::from_ptr(error_ptr).to_str().unwrap(); assert_eq!(error_str, "Error\twith\ttabs"); } // Test error with quotes set_last_error("Error with \"quotes\" and 'apostrophes'"); let error_ptr = dash_spv_ffi_get_last_error(); + assert!(!error_ptr.is_null()); unsafe { let error_str = CStr::from_ptr(error_ptr).to_str().unwrap(); assert_eq!(error_str, "Error with \"quotes\" and 'apostrophes'"); }Also applies to: 221-226, 229-235
32-69: Clarify pointer‐lifetime semantics ofdash_spv_ffi_get_last_error()and optionally harden the test
dash_spv_ffi_get_last_error()returns a pointer into aCStringstored in a globalMutex<Option<CString>>; each call toset_last_error()(orclear_last_error()) replaces and drops the previous buffer, so the returned pointer remains valid only until the next such call on any thread.- In
test_concurrent_error_handling, the initialBarrier+sleepmakes it unlikely that aset_last_error()happens after you callget_last_error(), but to guard against future scheduling changes, immediately copy the C string into a Rust‐ownedString:if !error_ptr.is_null() { let s = unsafe { CStr::from_ptr(error_ptr).to_string_lossy().into_owned() }; assert!(s.contains("Error from thread")); }
- If you need stronger concurrency guarantees, expose an owned‐getter variant (e.g.
dash_spv_ffi_get_last_error_owned()) that allocates a fresh buffer the caller can free.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
.github/workflows/rust.yml(1 hunks)dash-spv-ffi/include/dash_spv_ffi.h(7 hunks)dash-spv-ffi/src/callbacks.rs(1 hunks)dash-spv-ffi/src/client.rs(27 hunks)dash-spv-ffi/src/platform_integration.rs(1 hunks)dash-spv-ffi/src/types.rs(5 hunks)dash-spv-ffi/src/utils.rs(1 hunks)dash-spv-ffi/tests/test_event_callbacks.rs(2 hunks)dash-spv-ffi/tests/test_types.rs(1 hunks)dash-spv-ffi/tests/unit/test_async_operations.rs(1 hunks)dash-spv-ffi/tests/unit/test_error_handling.rs(1 hunks)dash-spv-ffi/tests/unit/test_type_conversions.rs(1 hunks)test-utils/src/builders.rs(1 hunks)test-utils/src/helpers.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- dash-spv-ffi/src/utils.rs
🧰 Additional context used
📓 Path-based instructions (10)
dash-spv-ffi/tests/unit/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
Add a corresponding Rust unit test for each new FFI function
Files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.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/tests/unit/test_async_operations.rstest-utils/src/builders.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rstest-utils/src/helpers.rsdash-spv-ffi/src/platform_integration.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-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/tests/unit/test_async_operations.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/src/platform_integration.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/client.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic
Files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/tests/test_event_callbacks.rs
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/src/platform_integration.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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:
test-utils/src/builders.rstest-utils/src/helpers.rsdash-spv-ffi/src/platform_integration.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/src/client.rs
test-utils/**
📄 CodeRabbit inference engine (AGENTS.md)
Shared test utilities are kept in test-utils/
Files:
test-utils/src/builders.rstest-utils/src/helpers.rs
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/platform_integration.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/src/client.rs
dash-spv-ffi/src/callbacks.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
Use C function pointers for async progress/completion callbacks; callbacks may be invoked from any thread
Files:
dash-spv-ffi/src/callbacks.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
🧠 Learnings (26)
📓 Common learnings
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`
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
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
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
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.
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*)
📚 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-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/tests/test_event_callbacks.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/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/tests/unit/test_async_operations.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
.github/workflows/rust.ymldash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_type_conversions.rs
📚 Learning: 2025-08-21T04:45:50.436Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.436Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Applied to files:
.github/workflows/rust.yml
📚 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/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.h
📚 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/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/src/platform_integration.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/tests/test_types.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/types.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/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/tests/unit/test_error_handling.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 : FFI functions must be declared with #[no_mangle] extern "C"
Applied to files:
dash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/tests/**/*.rs : Place integration tests under the `tests/` directory
Applied to files:
dash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.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 : Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/tests/unit/test_type_conversions.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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 : Accept input strings from C as *const c_char (borrowed; do not free inputs)
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/ffi/**/*.rs : Expose safe C FFI with #[no_mangle] extern "C" functions only for vetted APIs
Applied to files:
dash-spv-ffi/src/callbacks.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 : Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Applied to files:
dash-spv-ffi/tests/test_event_callbacks.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-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/c_tests/**/*.c : In C code, free all strings returned by the FFI using dash_string_free
Applied to files:
dash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.h
📚 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.h
📚 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-ffi/include/dash_spv_ffi.hdash-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/include/dash_spv_ffi.hdash-spv-ffi/src/client.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
dash-spv-ffi/include/dash_spv_ffi.h
📚 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-06-26T15:46:56.854Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:46:56.854Z
Learning: Transaction IDs (txids) 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/src/client.rs
🧬 Code graph analysis (4)
dash-spv-ffi/tests/test_event_callbacks.rs (1)
dash-spv-ffi/src/utils.rs (1)
dash_spv_ffi_init_logging(11-31)
dash-spv-ffi/src/types.rs (1)
dash-spv/src/types.rs (2)
new(181-183)new(877-897)
dash-spv-ffi/include/dash_spv_ffi.h (2)
dash-spv-ffi/src/client.rs (10)
dash_spv_ffi_client_start(395-430)dash_spv_ffi_client_get_sync_progress(904-937)dash_spv_ffi_client_get_stats(944-977)dash_spv_ffi_client_is_filter_sync_available(984-1005)dash_spv_ffi_client_set_event_callbacks(1012-1040)dash_spv_ffi_client_destroy(1047-1085)dash_spv_ffi_sync_progress_destroy(1092-1096)dash_spv_ffi_spv_stats_destroy(1103-1107)dash_spv_ffi_client_rescan_blockchain(1116-1144)dash_spv_ffi_client_enable_mempool_tracking(1151-1187)dash-spv-ffi/src/types.rs (1)
dash_spv_ffi_string_destroy(307-311)
dash-spv-ffi/src/client.rs (2)
dash-spv/src/client/mod.rs (5)
wallet(105-107)network(418-420)network(1249-1252)storage(2409-2411)new(174-237)key-wallet/src/managed_account/transaction_record.rs (1)
amount(130-132)
⏰ 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). (19)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
🔇 Additional comments (21)
test-utils/src/builders.rs (1)
139-145: Field init shorthand is correct and idiomatic.Switching to
valueshorthand is a no-op semantically and aligns with clippy style. LGTM.dash-spv-ffi/tests/unit/test_type_conversions.rs (1)
131-137: Clone-on-copy removed — clippy fix looks goodSwitching from
.clone().into()to(*val).into()correctly avoids redundant clones onCopyenums and matches the PR goal of clearing clippy warnings.dash-spv-ffi/src/callbacks.rs (1)
341-352: Scoped lint suppression is appropriate here.Allowing
too_many_argumentsonly oncall_wallet_transactionkeeps clippy clean without broad disables. No functional change.dash-spv-ffi/src/platform_integration.rs (1)
236-240: Placeholder mainnet activation height needs a real value or source.The comment explicitly says “placeholder - needs verification”. This propagates incorrect values through the FFI.
Would you like me to wire this from dashcore network params (if exposed) or gate it behind a cfg/test-only fallback?
dash-spv-ffi/src/types.rs (2)
24-27: Good Safety docs for from_ptr.Clear constraints; returning Err on null is fine for call sites distinguishing “no string” vs. empty.
242-244: User agent mapping is safer and avoids extra allocation.
as_deref().unwrap_or("")is the right choice here.dash-spv-ffi/tests/unit/test_async_operations.rs (1)
147-150: LGTM: clearer range check.
(0.0..=100.0).contains(&last_progress)reads better and avoids double comparisons.dash-spv-ffi/tests/test_types.rs (1)
103-105: Clippy-friendly boolean assertions — good change.Switching to assert!/assert!(!...) removes clippy::bool_assert_comparison noise and reads better.
dash-spv-ffi/tests/test_event_callbacks.rs (1)
140-140: Remove CString fallback: C-string literals supported on MSRV
c-string literals (c"…"; edition 2021+) were stabilized in Rust 1.77 and are available on stable Rust 1.89 without feature flags, soc"debug".as_ptr()is fully supported. (blog.rust-lang.org, github.com)dash-spv-ffi/src/client.rs (6)
104-121: SharedClient alias and inner Option pattern look good.Using Arc<Mutex<Option<...>>> with take()/reinsert avoids holding locks across awaits and prevents re-entrancy issues within async boundaries.
238-241: Type consistency for txid/addresses passed to callbacks.You parse txid to dashcore::Txid for the general transaction callback (good). For wallet_transaction, amount is forwarded as-is (i64). Confirm the FFI wallet_transaction callback under key-wallet-ffi expects i64 and that addresses encoding matches (UTF-8 C string).
Also applies to: 250-259
351-388: Config update path looks correct.Lock-taking pattern avoids await while holding the mutex and properly reinserts the client. Good error propagation.
492-577: sync_to_tip simple: solid callback cleanup and reinsertion.Good: unregister on both Ok/Err and avoid holding locks across awaits.
1151-1187: Enable mempool tracking: pattern matches other methods — OK.Error handling and reinsertion look correct.
1263-1288: Wallet manager handle: ensure matching destroy and cbindgen annotations.Confirm key-wallet-ffi exposes a wallet_manager_free (or similar) and that cbindgen exports the opaque type properly for C/Swift. Also ensure runtime lifetime assumptions are documented since you embed an Arc.
dash-spv-ffi/include/dash_spv_ffi.h (5)
382-389: LGTM on filter-sync availability API.Signature matches implementation and safety note is adequate.
407-414: LGTM: FFISyncProgress destroy.No concerns.
415-422: LGTM: FFISpvStats destroy.No concerns.
744-750: LGTM: FFIString destroy safety.Null-checked and ownership documented.
259-266: Thedash_spv_ffi_client_update_configsymbol is defined in Rust and matches the C header signature:
- Header snippet (dash-spv-ffi/include/dash_spv_ffi.h):
/** * Update the client configuration. * * # Safety * - `client` must be a valid, non-null pointer to a created client. * - `config` must be a valid pointer to a properly initialized config object. */ int32_t dash_spv_ffi_client_update_config(struct FFIDashSpvClient *client, struct FFIDashSpvConfig *config);- Rust implementation (dash-spv-ffi/src/client.rs):
#[no_mangle] pub unsafe extern "C" fn dash_spv_ffi_client_update_config( client: *mut FFIDashSpvClient, config: *mut FFIDashSpvConfig, ) -> i32 { // ... implementation ... }No further action required.
dash-spv-ffi/tests/unit/test_error_handling.rs (1)
126-127: Switch tostd::io::Error::other— LGTMThis addresses the Clippy lint and preserves the intended mapping to
FFIErrorCode::RuntimeError. Good change.
| /** | ||
| * Stop the SPV client. | ||
| * | ||
| * # Safety | ||
| * - `client` must be a valid, non-null pointer to a created client. | ||
| */ | ||
| int32_t dash_spv_ffi_client_stop(struct FFIDashSpvClient *client); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify start/stop idempotency and thread-safety.
State whether multiple calls to start/stop are safe/no-ops and on which threads they may be invoked.
🏁 Script executed:
#!/bin/bash
rg -n 'extern "C"\s*fn\s+dash_spv_ffi_client_stop' dash-spv-ffi/srcLength of output: 190
🏁 Script executed:
#!/bin/bash
# Show the implementations of start and stop to inspect idempotency and thread-safety
sed -n '370,480p' dash-spv-ffi/src/client.rsLength of output: 3348
🏁 Script executed:
#!/bin/bash
rg -n -C3 "fn start_event_listener" dash-spv-ffi/srcLength of output: 535
Clarify start/stop idempotency and thread-safety
- Document that each call to dash_spv_ffi_client_start/stop always invokes the underlying SpvClient.start()/stop() (no internal no-op) and will return an error if the client isn’t in the expected state.
- Note that both functions lock the internal Mutex and call runtime.block_on, making them safe to call from any host thread—but they must not be invoked from within an active Tokio context (to avoid deadlock).
Apply these clarifications to the safety comments in dash-spv-ffi/include/dash_spv_ffi.h (lines 267–274).
🤖 Prompt for AI Agents
In dash-spv-ffi/include/dash_spv_ffi.h around lines 267 to 274, update the
safety comment for dash_spv_ffi_client_stop (and similarly for
dash_spv_ffi_client_start wherever documented) to state that each call always
invokes the underlying SpvClient.start()/stop() (no internal no-op) and will
return an error if the client isn’t in the expected state; also document that
both functions acquire the internal Mutex and use runtime.block_on, so they are
safe to call from any host thread but must not be called from within an active
Tokio runtime/thread (to avoid deadlock).
| // Convert quorum type and hash for engine lookup (infallible) | ||
| let llmq_type: LLMQType = (quorum_type as u8).into(); | ||
| let quorum_hash = QuorumHash::from_byte_array(hash_array); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restoring input validation for quorum_type avoids silent truncation.
Casting u32 -> u8 and using into() can accept out-of-range values, changing an argument error into a “not found” later. Prefer explicit validation and return InvalidArgument on bad inputs.
- // Convert quorum type and hash for engine lookup (infallible)
- let llmq_type: LLMQType = (quorum_type as u8).into();
+ // Convert quorum type and hash for engine lookup with validation
+ let llmq_type: LLMQType = match LLMQType::try_from(quorum_type as u8) {
+ Ok(t) => t,
+ Err(_) => {
+ return FFIResult::error(
+ FFIErrorCode::InvalidArgument,
+ &format!("Invalid quorum_type: {}", quorum_type),
+ );
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Convert quorum type and hash for engine lookup (infallible) | |
| let llmq_type: LLMQType = (quorum_type as u8).into(); | |
| let quorum_hash = QuorumHash::from_byte_array(hash_array); | |
| // Convert quorum type and hash for engine lookup with validation | |
| let llmq_type: LLMQType = match LLMQType::try_from(quorum_type as u8) { | |
| Ok(t) => t, | |
| Err(_) => { | |
| return FFIResult::error( | |
| FFIErrorCode::InvalidArgument, | |
| &format!("Invalid quorum_type: {}", quorum_type), | |
| ); | |
| } | |
| }; | |
| let quorum_hash = QuorumHash::from_byte_array(hash_array); |
This refactoring aims to streamline the testing process and improve maintainability of the test suite.
Summary by CodeRabbit
New Features
Refactor
Tests
Style