feat: expose filters #189
feat: expose filters #189
Conversation
WalkthroughAdds FFI APIs and types to load compact filters and query per-height matched wallet IDs; refactors compact-filter matching to return Vec<[u8;32]> of matching wallet IDs, persists matches into ChainState, and exposes storage APIs to bulk-load filters within bounded ranges (max 10,000). Changes
sequenceDiagram
autonumber
participant FFI as FFI Client
participant Sync as Sync/MessageHandler
participant Storage as StorageManager
participant Wallet as WalletManager/Wallets
participant Chain as ChainState
FFI->>Sync: load_filters(start..end)
Sync->>Storage: load_filters(range) (validate <=10k)
Storage-->>Sync: Vec<(height, filter_data)>
loop per filter
Sync->>Wallet: check_filter_for_matches(filter_data)
Wallet-->>Sync: Vec<wallet_id> (maybe empty)
alt matches not empty
Sync->>Chain: record_filter_matches(height, wallet_ids)
Chain-->>Sync: ack
end
end
Sync-->>FFI: return FFIFilterMatches (owns data)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
dash-spv/src/storage/mod.rs (1)
130-133: Filter height/query APIs match backing implementationsThe new
get_stored_filter_heightandload_filtersmethods, including the 10,000-block range cap and “skip missing filters” semantics, line up with both disk and memory implementations and give a clear abstraction for batch filter access.As an optional refinement, you might briefly note that the
Range<u32>end is exclusive (e.g., “heights in[range.start, range.end)”) in the docs to make the boundary behavior explicit for callers.Also applies to: 152-159
dash-spv/tests/storage_test.rs (1)
319-352: Prefer matching onStorageError::InvalidInputover error string textThe limit checks in
test_load_filters_range_limitcurrently assert onto_string()contents. That works, but it’s a bit brittle if the error message wording changes.You can make the test more robust by matching the concrete error variant and only optionally asserting on the message, e.g.:
-use dash_spv::storage::{MemoryStorageManager, StorageManager}; +use dash_spv::storage::{MemoryStorageManager, StorageManager}; +use dash_spv::error::StorageError; - let result = storage.load_filters(0..10_001).await; - assert!(result.is_err(), "Should reject range exceeding 10,000 blocks"); - let error_msg = result.unwrap_err().to_string(); - assert!( - error_msg.contains("exceeds maximum") && error_msg.contains("10000"), - "Error message should mention the limit" - ); + let result = storage.load_filters(0..10_001).await; + match result { + Err(StorageError::InvalidInput(msg)) => { + assert!( + msg.contains("exceeds maximum") && msg.contains("10000"), + "Error message should mention the limit" + ); + } + other => panic!("Expected StorageError::InvalidInput, got {:?}", other), + }This keeps the semantic check while tying the assertion to the intended error variant rather than its string formatting.
dash-spv/src/sync/sequential/message_handlers.rs (1)
607-612: Filter persistence and match recording logic looks correct; minor clone can be avoidedPersisting the CFilter after header verification and then recording
matched_wallet_idsintoChainStategives you durable filter data plus per-height wallet match tracking, which fits the newfilter_matchesmodel and the surrounding sync/FFI changes. Error mapping toSyncError::Storageis consistent with existing patterns.One small refinement: you currently clone
matched_wallet_idsjust to pass ownership intorecord_filter_matches:chain_state.record_filter_matches(height, matched_wallet_ids.clone());Since you only use the vector afterward for its length in logs, you could avoid the clone by capturing the count first and then moving the vector:
let match_count = matched_wallet_ids.len(); // ... chain_state.record_filter_matches(height, matched_wallet_ids); // use match_count for logging belowThis keeps behavior identical while avoiding an extra allocation/copy on each matching filter.
Also applies to: 613-621, 625-655, 656-661
key-wallet-manager/src/wallet_interface.rs (1)
35-40: Trait docs and return type look good; consider a WalletId alias for clarityThe new documentation and
Vec<[u8; 32]>return type clearly define the semantics (empty = no download, non-empty = download those wallet IDs) and align with the rest of the PR.Given that this
Vec<[u8; 32]>shape now appears in several places (here, chain state, filter matching), consider introducing a dedicated alias or newtype (e.g.type WalletId = [u8; 32];) and usingVec<WalletId>in the trait and downstream code to improve readability and reduce potential for mismatched array lengths.Also applies to: 41-47
dash-spv/src/sync/sequential/phase_execution.rs (1)
153-186: Consider clampingstart_heightwithsync_base_heightto avoid redundant filter downloadsThe resume logic from
stored_filter_heightis sensible, but there are a couple of edge cases worth tightening:
- If
stored_filter_height < self.header_sync.get_sync_base_height(), you’ll resume below the sync base and re-download filters you’ll never use. Clamping tomax(stored_height + 1, sync_base_height.max(1))would avoid this.- If
stored_filter_height > filter_header_tip, the current code will log that all filters are downloaded and skip the phase, but silently accepts an inconsistent state (stored past tip). Emitting a warning (or falling back to a safer strategy) could make diagnosing storage corruption easier.Refactoring the
start_heightcalculation to incorporatesync_base_heightin theSome(stored_height)branch would address the first point without changing the main happy-path behavior.Also applies to: 188-200
dash-spv/src/storage/disk/filters.rs (1)
263-289: Range-bounded bulk filter loading is correct; consider de-duplicatingMAX_RANGEThe implementation:
- Uses
saturating_subto avoid underflow,- Enforces a 10,000-block range cap, and
- Skips missing filters while returning
(height, filter)pairs.For maintainability, you might optionally extract
MAX_RANGEto a shared constant used by both disk and memory implementations to prevent divergence over time.dash-spv/src/types.rs (1)
428-448: Consider adding deduplication logic to prevent duplicate wallet IDs.Both
record_filter_matchandrecord_filter_matchescan insert duplicate wallet IDs at the same height if called multiple times. This could lead to memory bloat and incorrect counting.If deduplication is needed, consider using a
BTreeSetor checking for existence before inserting:pub fn record_filter_match(&mut self, height: u32, wallet_id: [u8; 32]) { - self.filter_matches.entry(height).or_insert_with(Vec::new).push(wallet_id); + let vec = self.filter_matches.entry(height).or_insert_with(Vec::new); + if !vec.contains(&wallet_id) { + vec.push(wallet_id); + } }Alternatively, change the type to
BTreeMap<u32, BTreeSet<[u8; 32]>>if order doesn't matter:-pub filter_matches: BTreeMap<u32, Vec<[u8; 32]>>, +pub filter_matches: BTreeMap<u32, BTreeSet<[u8; 32]>>,dash-spv-ffi/src/types.rs (1)
609-628: Memory management is correct, but note the logic duplication.The destroy function properly frees nested allocations: first each filter's data (lines 614-622), then the filters array itself (line 625). However, the filter data deallocation logic is duplicated from
dash_spv_ffi_compact_filter_destroy(lines 595-597).This follows the existing pattern in the codebase (similar to
FFIUnconfirmedTransaction), so it's consistent, but if the FFICompactFilter structure changes, both functions must be updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
dash-spv-ffi/include/dash_spv_ffi.h(5 hunks)dash-spv-ffi/src/client.rs(2 hunks)dash-spv-ffi/src/types.rs(1 hunks)dash-spv-ffi/tests/unit/test_client_lifecycle.rs(1 hunks)dash-spv-ffi/tests/unit/test_type_conversions.rs(1 hunks)dash-spv/src/client/block_processor.rs(2 hunks)dash-spv/src/client/block_processor_test.rs(2 hunks)dash-spv/src/error.rs(2 hunks)dash-spv/src/storage/disk/filters.rs(3 hunks)dash-spv/src/storage/disk/state.rs(3 hunks)dash-spv/src/storage/memory.rs(2 hunks)dash-spv/src/storage/mod.rs(2 hunks)dash-spv/src/storage/sync_state.rs(4 hunks)dash-spv/src/sync/filters/matching.rs(1 hunks)dash-spv/src/sync/sequential/message_handlers.rs(2 hunks)dash-spv/src/sync/sequential/phase_execution.rs(1 hunks)dash-spv/src/types.rs(4 hunks)dash-spv/tests/storage_test.rs(1 hunks)key-wallet-manager/src/wallet_interface.rs(1 hunks)key-wallet-manager/src/wallet_manager/process_block.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/tests/unit/test_type_conversions.rsdash-spv/src/storage/mod.rsdash-spv/src/sync/sequential/message_handlers.rsdash-spv-ffi/tests/unit/test_client_lifecycle.rsdash-spv/src/sync/sequential/phase_execution.rsdash-spv/src/storage/disk/filters.rsdash-spv/src/types.rsdash-spv/src/sync/filters/matching.rsdash-spv/src/storage/memory.rsdash-spv/src/client/block_processor.rsdash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-spv/src/client/block_processor_test.rsdash-spv-ffi/src/client.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/sync_state.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/error.rsdash-spv/src/storage/mod.rsdash-spv/tests/storage_test.rsdash-spv/src/storage/memory.rsdash-spv-ffi/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/sync_state.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
key-wallet-manager/src/wallet_manager/process_block.rsdash-spv/src/client/block_processor_test.rs
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
dash-spv-ffi/src/types.rsdash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T15:46:56.854Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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/include/dash_spv_ffi.h
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 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-02-25T06:13:52.858Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 51
File: dash/src/sml/masternode_list/scores_for_quorum.rs:50-73
Timestamp: 2025-02-25T06:13:52.858Z
Learning: ScoreHash is a cryptographic hash in the rust-dashcore library, and therefore does not need collision handling when used as a key in collections like BTreeMap due to the extremely low probability of collisions.
Applied to files:
dash-spv/src/storage/disk/state.rsdash-spv/src/storage/sync_state.rs
🧬 Code graph analysis (12)
dash-spv/src/storage/mod.rs (3)
dash-spv/src/storage/disk/filters.rs (2)
get_stored_filter_height(191-243)load_filters(269-289)dash-spv/src/storage/disk/state.rs (2)
get_stored_filter_height(550-552)load_filters(578-580)dash-spv/src/storage/memory.rs (2)
get_stored_filter_height(261-268)load_filters(297-315)
dash-spv/src/sync/sequential/message_handlers.rs (3)
dash-spv/src/client/core.rs (2)
storage(171-173)chain_state(206-209)dash-spv/src/storage/disk/manager.rs (1)
new(79-129)dash-spv/src/sync/sequential/lifecycle.rs (1)
new(31-62)
dash-spv-ffi/tests/unit/test_client_lifecycle.rs (2)
dash-spv-ffi/src/client.rs (4)
dash_spv_ffi_client_new(140-210)dash_spv_ffi_client_get_transaction_count(1790-1817)dash_spv_ffi_client_destroy(1417-1455)dash_spv_ffi_client_get_blocks_with_transactions_count(1835-1873)dash-spv-ffi/src/config.rs (1)
dash_spv_ffi_config_destroy(345-354)
key-wallet-manager/src/wallet_manager/process_block.rs (2)
key-wallet-manager/src/wallet_manager/mod.rs (2)
new(70-76)new(107-114)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
wallet_id(30-30)wallet_id(126-128)
dash-spv/src/sync/sequential/phase_execution.rs (1)
dash-spv/src/sync/headers_with_reorg.rs (1)
get_sync_base_height(1119-1121)
dash-spv/tests/storage_test.rs (1)
dash-spv/src/storage/memory.rs (1)
new(30-42)
dash-spv/src/storage/disk/filters.rs (4)
dash-spv/src/storage/disk/state.rs (2)
get_stored_filter_height(550-552)load_filters(578-580)dash-spv/src/storage/memory.rs (3)
get_stored_filter_height(261-268)load_filters(297-315)new(30-42)dash-spv/src/storage/mod.rs (2)
get_stored_filter_height(132-132)load_filters(158-158)dash-spv/src/storage/disk/manager.rs (1)
new(79-129)
dash-spv/src/storage/memory.rs (3)
dash-spv/src/storage/disk/filters.rs (2)
get_stored_filter_height(191-243)load_filters(269-289)dash-spv/src/storage/disk/state.rs (2)
get_stored_filter_height(550-552)load_filters(578-580)dash-spv/src/storage/mod.rs (2)
get_stored_filter_height(132-132)load_filters(158-158)
key-wallet-manager/src/wallet_interface.rs (2)
dash-spv/src/client/block_processor_test.rs (2)
check_compact_filter(66-74)check_compact_filter(289-297)key-wallet-manager/src/wallet_manager/process_block.rs (1)
check_compact_filter(87-123)
dash-spv-ffi/include/dash_spv_ffi.h (2)
dash-spv-ffi/src/client.rs (4)
dash_spv_ffi_client_load_filters(1310-1385)dash_spv_ffi_client_get_filter_matched_heights(1697-1772)dash_spv_ffi_client_get_transaction_count(1790-1817)dash_spv_ffi_client_get_blocks_with_transactions_count(1835-1873)dash-spv-ffi/src/types.rs (4)
dash_spv_ffi_compact_filter_destroy(592-599)dash_spv_ffi_compact_filters_destroy(609-628)dash_spv_ffi_filter_match_entry_destroy(663-674)dash_spv_ffi_filter_matches_destroy(684-701)
dash-spv-ffi/src/client.rs (2)
dash-spv-ffi/src/error.rs (1)
set_last_error(26-31)dash-spv/src/client/core.rs (1)
wallet(161-163)
dash-spv/src/storage/disk/state.rs (3)
dash-spv/src/storage/memory.rs (2)
get_stored_filter_height(261-268)load_filters(297-315)dash-spv/src/storage/disk/filters.rs (2)
get_stored_filter_height(191-243)load_filters(269-289)dash-spv/src/storage/mod.rs (2)
get_stored_filter_height(132-132)load_filters(158-158)
⏰ 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: update-docs-comment
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Core Components Tests
- GitHub Check: Strict Warnings and Clippy Checks
🔇 Additional comments (26)
dash-spv-ffi/tests/unit/test_type_conversions.rs (1)
165-176: Initialize newfilter_matchesfield consistentlySetting
filter_matchesto an emptyBTreeMapkeeps the test’sChainStateconstruction aligned with the updated struct and avoids uninitialized field issues. Looks correct and consistent with the new persistence model.dash-spv/src/error.rs (1)
114-116:StorageError::InvalidInputvariant is correctly integratedThe new
InvalidInput(String)variant and its explicit arm in the manualCloneimplementation are wired consistently with the existing string-based variants, so downstream storage code can distinguish invalid input cleanly without changing existing behavior.Also applies to: 118-130
dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
245-300: Nice coverage for zero-count and null-client FFI behaviorThese tests validate the expected 0 counts for fresh wallets and ensure the new FFI query functions behave safely when passed a null client pointer, aligning with the
null_check!behavior inclient.rs. The lifecycle (create → query → destroy) mirrors existing patterns, so this fits cleanly into the test suite.dash-spv/tests/storage_test.rs (1)
288-317: Filter range behavior is well covered
test_load_filters_rangedoes a good job exercising the intended semantics ofload_filters: skipping missing heights, confirming end-exclusive ranges (101..103 only returns 101 even though 103 exists), and returning an empty vector when no filters are present in the range. This aligns with the MemoryStorageManager implementation.dash-spv/src/client/block_processor.rs (1)
180-190: Compact filter handling correctly adapted to per-wallet matchesUsing
matched_wallet_idsand derivinghas_matchespreserves the existing boolean contract for callers while enriching internal logging with the number of matching wallets. The control flow and locking remain consistent with prior behavior.Also applies to: 195-205
dash-spv/src/sync/filters/matching.rs (1)
54-60: Filter–wallet integration cleanly exposes per-wallet matches
check_filter_for_matchesnow correctly wraps the wallet’scheck_compact_filter, returning theVec<[u8; 32]>of wallet IDs and logging only when matches exist. This aligns with the updatedWalletInterfaceand the broader move toward per-wallet filter match tracking.Also applies to: 61-69
dash-spv/src/storage/disk/filters.rs (2)
8-8: ImportingStorageErrorfor range validation is appropriateThe added import is necessary for the new
load_filtersrange validation and is consistent with existing error handling in this module.
185-243: Directory-based stored filter height detection looks robustThe scan correctly:
- Ignores non-files and non-
.datentries.- Restricts to purely numeric filenames and caps heights at 2,000,000 with a warning.
- Returns
Nonewhen no valid filter files are present.This aligns with the documented intent to reflect actually persisted filters rather than headers.
key-wallet-manager/src/wallet_manager/process_block.rs (1)
87-123: Per-wallet compact filter matching and caching behavior looks correctThe new implementation:
- Evaluates each wallet independently using its
monitored_addresses(network),- Builds per-wallet script pubkeys and calls
filter.match_any(..).unwrap_or(false),- Collects wallet IDs with hits, and
- Caches
any_matchas a boolean infilter_matches, preserving the prior “any wallet matched” semantics.This cleanly exposes matched wallet IDs while keeping the existing cache contract.
dash-spv/src/storage/memory.rs (2)
261-268: In-memory stored filter height computation is simple and correctUsing
self.filters.keys().max().copied()to derive the highest stored filter height (orNonewhen empty) matches the intended semantics and is consistent with how filters are stored in this manager.
297-315: In-memory bulk filter loading correctly mirrors disk behaviorThe range-bounded
load_filters:
- Enforces the same 10,000-block maximum range as the disk implementation,
- Handles inverted ranges safely via
saturating_sub,- Iterates the range and returns
(height, filter.clone())only for existing entries.This keeps memory and disk storage backends behaviorally aligned.
dash-spv/src/client/block_processor_test.rs (2)
66-74: MockWallet’scheck_compact_filteradaptation matches new APIReturning a non-empty
Vec<[u8; 32]>with a fixed test wallet ID for all filters is a straightforward way to signal “matched” in tests under the new signature.
289-297: NonMatchingWallet correctly models the “no match” caseReturning an empty
Vec<[u8; 32]>cleanly represents a non-matching filter for tests and aligns with the updatedWalletInterfacecontract.dash-spv/src/storage/disk/state.rs (3)
5-5:Rangeimport is appropriate for new trait method signatureThe added
use std::ops::Range;matches the newload_filtersmethod signature and keeps the implementation concise.
550-552: Trait forwarding forget_stored_filter_heightis consistentDelegating
StorageManager::get_stored_filter_heighttoDiskStorageManager::get_stored_filter_heightmatches the existing pattern used throughout this impl and correctly exposes the disk-level helper.
578-580:load_filtersdelegation cleanly exposes disk-backed bulk loadingForwarding
load_filtersthrough theStorageManagerimpl ensures callers can use the new disk bulk filter loading API via the trait without duplicating logic.dash-spv/src/types.rs (3)
285-287: Good choice of data structure.Using
BTreeMap<u32, Vec<[u8; 32]>>enables efficient range queries via therange()method, which is exactly whatget_filter_matched_heightsleverages. The 32-byte fixed arrays for wallet IDs align with FFI best practices.Based on learnings
461-480: LGTM! Good range validation and error handling.The 10,000 block limit prevents abuse, and the error messages are clear. The use of
saturating_subprevents underflow, andBTreeMap::range()provides efficient extraction.Note: Line 477 clones each Vec, which is necessary for thread safety across the FFI boundary but could be expensive for ranges with many matches. Given the 10K block limit, this is acceptable.
1163-1245: Excellent test coverage!The tests comprehensively validate:
- Single and bulk filter match recording
- Range queries with hits and misses
- Boundary conditions (exactly 10,000 blocks passes, 10,001 fails)
- Empty and partially overlapping ranges
dash-spv-ffi/src/types.rs (2)
554-599: LGTM! Memory management is correct.The
FFICompactFilterstruct and its destroy function properly handle ownership transfer. The use ofVec::from_raw_parts(data, data_len, data_len)is correct because the Vec is forgotten immediately after creation (seedash-spv-ffi/src/client.rs:1356-1359), ensuring len equals capacity.
630-701: LGTM! FFIFilterMatches memory management mirrors FFICompactFilters pattern.The destroy functions correctly handle the two-level allocation:
- Array of entries (line 698)
- Each entry's wallet_ids array (lines 688-696)
The logic properly reconstructs and drops
Vec<[u8; 32]>with matching len/capacity, which is safe given how these are created indash-spv-ffi/src/client.rs:1742-1757.dash-spv-ffi/src/client.rs (3)
1310-1385: LGTM! Well-designed for concurrent access.Lines 1335-1344: The function gets a storage reference without taking the client mutex, allowing other operations to proceed concurrently. This is excellent for responsiveness during sync.
The memory transfer pattern (lines 1354-1370) correctly uses
std::mem::forgetto transfer ownership to the FFI caller, who must calldash_spv_ffi_compact_filters_destroy.
1697-1772: LGTM! Consistent pattern with proper ownership transfer.The function follows the same concurrent-access pattern as
load_filters, obtaining chain state without blocking other operations (lines 1722-1731). The BTreeMap-to-FFI conversion correctly transfers ownership viastd::mem::forget(lines 1742-1757).
1790-1873: LGTM! Both count functions are well-implemented.
get_transaction_count(lines 1790-1817): Simple and correct, returns the persisted transaction history length.
get_blocks_with_transactions_count(lines 1835-1873): Properly counts unique block heights using a HashSet (lines 1854-1859), filtering for confirmed transactions only. The warning logs (lines 1810, 1864-1866) help with debugging.dash-spv-ffi/include/dash_spv_ffi.h (2)
112-749: LGTM! C declarations correctly match Rust implementations.The auto-generated header properly declares all new FFI types and functions:
- Struct layouts match the Rust definitions
- Function signatures correctly translate Rust types to C types
- Memory management requirements are clearly documented (e.g., lines 116-118, 558-560, 687-688)
- Safety requirements are explicit (e.g., lines 576-577, 704-705)
1156-1198: LGTM! Destroy function declarations are comprehensive and well-documented.All four destroy functions are properly declared with clear safety documentation emphasizing:
- Pointer validity requirements
- One-time-only calling constraint
- Risk of use-after-free
This matches the Rust implementations and follows FFI best practices.
dash-spv/src/storage/sync_state.rs
Outdated
| use dashcore::{BlockHash, Network}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::collections::BTreeMap; |
There was a problem hiding this comment.
🧩 Analysis chain
Adding filter_matches without a default or version bump likely breaks loading existing sync state
FilterSyncState now has a non-optional filter_matches: BTreeMap<u32, Vec<[u8; 32]>> field, but:
- The struct still derives
Deserializewith no#[serde(default)]on the new field, and SYNC_STATE_VERSIONremains2, so older on-disk states with version<= 2and nofilter_matchesfield will fail to deserialize at the serde layer before your version checks run.
Unless this format was introduced fresh with version 2 and no prior persisted data exists, this is a breaking change for existing users.
Two options to make this safe:
- Add a serde default for
filter_matches(backward compatible, keeps version number):
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct FilterSyncState {
@@
- /// Filter matches: height -> Vec of wallet IDs (32-byte arrays) that matched.
- /// This tracks which wallet IDs had transactions in blocks with matching compact filters.
- pub filter_matches: BTreeMap<u32, Vec<[u8; 32]>>,
+ /// Filter matches: height -> Vec of wallet IDs (32-byte arrays) that matched.
+ /// This tracks which wallet IDs had transactions in blocks with matching compact filters.
+ #[serde(default)]
+ pub filter_matches: BTreeMap<u32, Vec<[u8; 32]>>,- Or bump
SYNC_STATE_VERSIONand handle older versions explicitly in your loading path (e.g., treating missingfilter_matchesasBTreeMap::new()), which is heavier but makes the format change explicit.
Given the comment on SYNC_STATE_VERSION, option (1) is likely the least invasive way to preserve deserialization of existing state while still taking advantage of the new field.
Also applies to: 114-120, 187-193, 373-379
🏁 Script executed:
#!/bin/bash
# Examine the FilterSyncState struct definition and SYNC_STATE_VERSION
cd /tmp/repo 2>/dev/null || true
# Show the struct definition around lines 114-120
echo "=== FilterSyncState struct definition (lines 100-130) ==="
sed -n '100,130p' src/storage/sync_state.rs 2>/dev/null || echo "File not found"
# Show SYNC_STATE_VERSION definition
echo -e "\n=== SYNC_STATE_VERSION definition ==="
rg -n 'SYNC_STATE_VERSION' src/storage/sync_state.rs -A2 -B2 2>/dev/null || echo "Not found"
# Check for any serde attributes on filter_matches field
echo -e "\n=== Full FilterSyncState struct with all attributes ==="
ast-grep --pattern $'pub struct FilterSyncState {
$$$
}' 2>/dev/null || rg -n 'pub struct FilterSyncState' src/storage/sync_state.rs -A30 2>/dev/null
# Check for version handling/migration logic
echo -e "\n=== Version handling in deserialization ==="
rg -n 'SYNC_STATE_VERSION\|migrate\|version' src/storage/sync_state.rs -C3 2>/dev/null | head -50Length of output: 1458
🏁 Script executed:
#!/bin/bash
# Show struct definition with derive macros (lines 100-125)
echo "=== FilterSyncState struct with derives (lines 100-125) ==="
sed -n '100,125p' dash-spv/src/storage/sync_state.rs
# Search for SYNC_STATE_VERSION in entire dash-spv/src directory
echo -e "\n=== SYNC_STATE_VERSION search ==="
rg -n 'SYNC_STATE_VERSION' dash-spv/src/ -A3 -B1
# Check for any version constant or const definitions
echo -e "\n=== Version-related constants ==="
rg -n 'const.*VERSION\|static.*VERSION' dash-spv/src/ | head -20
# Look for deserialization/loading functions
echo -e "\n=== Deserialization/load functions ==="
rg -n 'fn.*load\|fn.*deserialize' dash-spv/src/storage/sync_state.rs -A2Length of output: 2735
🏁 Script executed:
#!/bin/bash
# Find PersistentSyncState struct and its fields
echo "=== PersistentSyncState struct definition ==="
rg -n 'pub struct PersistentSyncState' dash-spv/src/storage/sync_state.rs -A20
# Look for any custom Serialize/Deserialize implementations
echo -e "\n=== Custom serialize/deserialize implementations ==="
rg -n 'impl.*Serialize\|impl.*Deserialize' dash-spv/src/storage/sync_state.rs -A5
# Check how state is loaded from disk (file I/O)
echo -e "\n=== File loading and deserialization ==="
rg -n 'from_file\|load\|read\|deserialize' dash-spv/src/storage/ -B2 -A5 | head -80Length of output: 757
🏁 Script executed:
#!/bin/bash
# Search for where PersistentSyncState is actually loaded/deserialized from storage
echo "=== PersistentSyncState loading/parsing ==="
rg -n 'PersistentSyncState' dash-spv/src/storage/ -B3 -A3 | grep -E '(from_|deserialize|serde_json|bincode|read)' -A3 -B3
# Check the storage module to see how files are loaded
echo -e "\n=== Storage module structure ==="
ls -la dash-spv/src/storage/ 2>/dev/null | head -20
# Search for file I/O operations on sync state files
echo -e "\n=== Sync state file operations ==="
rg -n 'sync.*state|state.*file' dash-spv/src/storage/ -i | head -30Length of output: 4852
Add #[serde(default)] to filter_matches field or bump SYNC_STATE_VERSION
The concern is valid and confirmed. When sync state is loaded from disk using serde_json::from_str() at dash-spv/src/storage/disk/state.rs:168, the new filter_matches field in FilterSyncState lacks a default deserializer. Existing persisted state files (version 2) that lack this field will fail to deserialize before the version check at line 253 can run.
Choose one solution:
- Add
#[serde(default)]tofilter_matches(preserves backward compatibility, keeps version 2):
/// Filter matches: height -> Vec of wallet IDs (32-byte arrays) that matched.
/// This tracks which wallet IDs had transactions in blocks with matching compact filters.
+ #[serde(default)]
pub filter_matches: BTreeMap<u32, Vec<[u8; 32]>>,- Or bump
SYNC_STATE_VERSIONto 3 and add migration logic in the loading path to handle missingfilter_matchesfrom older states (makes format change explicit).
Option 1 is the least invasive fix.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dash-spv/src/storage/sync_state.rs around line 5, the new FilterSyncState
struct's filter_matches field lacks a serde default which causes deserialization
of older (version 2) on-disk state to fail; add #[serde(default)] to the
filter_matches field declaration so missing field deserializes to an empty
BTreeMap (no format bump), ensuring the struct implements Default or uses
BTreeMap::new() as the default value; alternatively, if you prefer an explicit
format change, bump SYNC_STATE_VERSION to 3 and add migration handling in the
loading path, but the recommended fix is to add #[serde(default)] to
filter_matches for backward compatibility.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-ffi/include/key_wallet_ffi.h (1)
896-900: Documentation inconsistency: wrong enum type referenced.The doc comment at line 899 says
"Returns FFINetwork::NoNetworks"but the function returnsFFINetworks, and the correct value isNO_NETWORKS. Same issue appears at lines 953 and 1009 forbls_account_get_networkandeddsa_account_get_network.Since this file is auto-generated by cbindgen, the fix should be in the Rust source doc comments.
🧹 Nitpick comments (2)
dash-spv-ffi/include/dash_spv_ffi.h (2)
713-731: Doc wording for transaction count is slightly misleadingThe function name and summary (“Get the total count of transactions across all wallets”) are clear, but the sentence:
“Use this to show how many blocks contained relevant transactions for the user's wallets.”
describes what the blocks‑with‑transactions metric does, not transaction count. I’d suggest tightening the doc to avoid confusion with
dash_spv_ffi_client_get_blocks_with_transactions_count.Example rewording:
- * not the ephemeral sync statistics. Use this to show how many - * blocks contained relevant transactions for the user's wallets. + * not the ephemeral sync statistics. Use this to show how many + * wallet-related transactions have been seen across all wallets.
1156-1199: Destroy helpers for new types match existing FFI patternsThe four destroy functions (
dash_spv_ffi_compact_filter_destroy,dash_spv_ffi_compact_filters_destroy,dash_spv_ffi_filter_match_entry_destroy,dash_spv_ffi_filter_matches_destroy) match the crate’s pattern of providing explicit free functions for heap-owned FFI structs. The docs make the single‑ownership requirement clear (“must not be used after this function” / “only be called once per allocation”). Just ensure the high-level docs/examples clearly indicate which destroy function to call for objects returned by each API (e.g., arrays from*_client_load_filters/*_get_filter_matched_heightsshould only go through the array-level destroy to avoid accidental double frees).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dash-spv-ffi/include/dash_spv_ffi.h(7 hunks)key-wallet-ffi/include/key_wallet_ffi.h(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 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:
key-wallet-ffi/include/key_wallet_ffi.hdash-spv-ffi/include/dash_spv_ffi.h
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
key-wallet-ffi/include/key_wallet_ffi.hdash-spv-ffi/include/dash_spv_ffi.h
📚 Learning: 2025-06-26T15:46:56.854Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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:
key-wallet-ffi/include/key_wallet_ffi.hdash-spv-ffi/include/dash_spv_ffi.h
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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.h
⏰ 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). (8)
- GitHub Check: update-docs-comment
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Key Wallet Components Tests
🔇 Additional comments (12)
dash-spv-ffi/include/dash_spv_ffi.h (7)
6-6: cbindgen version bump is fineUpdating the generated-header marker to
cbindgen:0.29.2is consistent with a regenerated FFI surface; no issues here.
32-35: Explicit mempool strategy discriminants preserve ABIReintroducing
FFIMempoolStrategyhere with explicit numeric values (FetchAll = 0,BloomFilter = 1) keeps the C ABI stable for existing callers and resolves any forward-declaration ordering issues with the config APIs.
112-153: Compact filter structs and ownership semantics look correctThe
FFICompactFilter/FFICompactFilterslayout (height +uint8_t *data+uintptr_t data_len, plus array-of-filters) is FFI-safe and matches the existingFFIArray/usizepattern. The docs clearly direct callers to free results fromdash_spv_ffi_client_load_filtersviadash_spv_ffi_compact_filters_destroy, anddash_spv_ffi_compact_filter_destroyis available for single-filter allocations. I don’t see ABI or ownership issues here.
216-252: Filter match structs follow the established 32‑byte ID pattern
FFIFilterMatchEntryandFFIFilterMatchesmirror the compact-filter pattern and useuint8_t (*wallet_ids)[32]for fixed-size 32‑byte identifiers, which aligns with the existing practice of passing binary IDs as fixed arrays instead of strings. The docs also make it clear thatdash_spv_ffi_filter_matches_destroyis responsible for freeing both the entries array and each entry’swallet_ids. Based on learnings, this is consistent with how txids/block hashes are handled.
555-583: Range-based filter loading API is well-specified; confirm inclusive/exclusive expectationsThe
dash_spv_ffi_client_load_filtersAPI is clearly documented:[start_height, end_height)semantics, 10 000‑block max range, null on error with details viadash_spv_ffi_get_last_error, and ownership transferred to the caller viadash_spv_ffi_compact_filters_destroy. Functionally and ABI‑wise this looks sound; just ensure the Rust implementation enforces the same inclusive/exclusive bounds and range limit so C callers don’t hit off‑by‑one surprises.
683-711: Filter match query API pairs cleanly with the new match structs
dash_spv_ffi_client_get_filter_matched_heightsmirrorsclient_load_filtersin range semantics, error handling, and ownership rules, and returns the newFFIFilterMatcheswrapper. This is a coherent extension of the FFI surface; as with filters, just confirm the Rust side enforces the documented[start_height, end_height)range and 10 000‑block limit.
732-749: Blocks-with-transactions count API is consistent and useful
dash_spv_ffi_client_get_blocks_with_transactions_countrounds out the metrics nicely: it returns a persistent count of unique block heights with relevant transactions and documents the “0 if client not initialized or wallet not available” behavior. Signature and semantics are consistent withdash_spv_ffi_client_get_transaction_count.key-wallet-ffi/include/key_wallet_ffi.h (5)
27-35: LGTM: Well-designed single-network enum.The
FFINetworkenum correctly uses sequential values for single network selection, complementing the bit-flagFFINetworksenum for multi-network scenarios.
97-107: LGTM: Correct bit flag implementation.The
FFINetworksenum properly uses powers of 2 for individual flags, andALL_NETWORKS = 15correctly represents the bitwise OR of all network flags (1|2|4|8).
128-144: LGTM: Clear address pool type enumeration.The enum appropriately uses sequential values for mutually exclusive pool types, with good documentation for each variant.
184-208: LGTM: Account creation options properly defined.The enum provides clear, mutually exclusive options for wallet account creation with helpful documentation for each variant.
814-818: API consistency between FFINetwork and FFINetworks is correctly implemented.The Rust implementation confirms the review comment's claims:
wallet_create_from_mnemonic_with_optionsacceptsFFINetworks(bit flags) for multi-network wallet configurationwallet_get_accountacceptsFFINetwork(single value) for network-specific operations- Proper
From/TryFromtrait implementations exist inkey-wallet-ffi/src/types.rsfor bidirectional conversion betweenNetwork,FFINetwork, andFFINetworksThe API design correctly distinguishes between wallet-level multi-network configuration and operation-level single-network targeting.
There was a problem hiding this comment.
Changes found:
- WalletInterface::check_compact_filter now returns a vector of 32-byte wallet IDs that matched the filter. This method is used by FilterSyncManager<S, N>::check_filter_for_matches that returns the vector of wallet IDs for later being stored in ChainState.filter_matches field. The function dash_spv_ffi_client_get_filter_matched_heights calls ChainState::get_filter_matched_heights to extract data for the previous mentioned field.
- the function dash_spv_ffi_client_load_filters makes use of the new StorageManager::load_filters method. All the structs that implement the trait were updated to implement it.
- The new method StorageManager::get_stored_filter_height doesn't look like it has any relation with the filters exposition. Is more an optimization that allows downloading from the last stored filter during SyncPhase::DownloadingFilters
- The FilterSyncState::filter_matches doesn't look like it is being used
- Missing
get_stored_filter_height,load_filtersimplementation in MockStorageManager (The file where this issue exists is no longer in the v0.41-dev branch)
It took my sometime but here is what I found. Almost everything has something to do with the filters exposition. Made some notes on stuff that doesn't make sense to me and the only change I found that has nothing to do with what this PR tries to add is the method StorageManager::get_stored_filter_height used to avoid downloading all the filters in each sync.
@QuantumExplorer if this looks good to you I will push the changes suggested
dash-spv/src/types.rs
Outdated
| /// # Parameters | ||
| /// - `height`: The blockchain height where the filter matched | ||
| /// - `wallet_id`: The 32-byte wallet identifier that matched | ||
| pub fn record_filter_match(&mut self, height: u32, wallet_id: [u8; 32]) { |
There was a problem hiding this comment.
This method is only tested, never used
| .unwrap_or_else(|| crate::types::ChainState::new()); | ||
|
|
||
| // Record the filter matches | ||
| chain_state.record_filter_matches(height, matched_wallet_ids.clone()); |
There was a problem hiding this comment.
This clone is unnecessary
dash-spv/src/storage/sync_state.rs
Outdated
| pub matched_heights: Vec<u32>, | ||
| /// Filter matches: height -> Vec of wallet IDs (32-byte arrays) that matched. | ||
| /// This tracks which wallet IDs had transactions in blocks with matching compact filters. | ||
| pub filter_matches: BTreeMap<u32, Vec<[u8; 32]>>, |
There was a problem hiding this comment.
Couldn't find any use of this
…ck SyncPhase::DownloadingFilters phase execution logic
…_matches optimized
…andle_cfilter_message
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dash-spv/src/storage/disk/state.rs(2 hunks)dash-spv/src/storage/memory.rs(1 hunks)dash-spv/src/storage/mod.rs(1 hunks)dash-spv/src/storage/sync_state.rs(0 hunks)dash-spv/src/sync/sequential/message_handlers.rs(2 hunks)dash-spv/src/sync/sequential/phase_execution.rs(1 hunks)dash-spv/src/types.rs(4 hunks)
💤 Files with no reviewable changes (1)
- dash-spv/src/storage/sync_state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/sync/sequential/phase_execution.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/storage/mod.rsdash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
dash-spv/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/**/*.rs: Use trait-based abstractions for swappable implementations (e.g.,NetworkManager,StorageManager)
Use async/await throughout the codebase with tokio runtime
Use domain-specific error types (NetworkError,StorageError,SyncError,ValidationError,SpvError) for error handling
Use trait objects (Arc<dyn StorageManager>,Arc<dyn NetworkManager>) for runtime polymorphism
Use Tokio channels for inter-component message passing
Add comprehensive unit tests in-module for individual components
Files:
dash-spv/src/storage/mod.rsdash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/storage/**/*.rs: ImplementStorageManagertrait with segmented storage: headers in 10,000-header segments with index files, filters in separate storage, and state persistence in dedicated paths
Use segmented storage with headers in 10,000-header chunks stored with index files for efficient retrieval
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/sync/**/*.rs: Use sequential phase-based synchronization withSequentialSyncManagerfor organized sync phases
Use state machines withSyncStateenum to drive synchronization flow and state transitions
Files:
dash-spv/src/sync/sequential/message_handlers.rs
dash-spv/src/types.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Define common data structures in
types.rsincludingSyncProgress,ValidationMode,WatchItem, etc.
Files:
dash-spv/src/types.rs
🧠 Learnings (21)
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Implement `StorageManager` trait with segmented storage: headers in 10,000-header segments with index files, filters in separate storage, and state persistence in dedicated paths
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Use segmented storage with headers in 10,000-header chunks stored with index files for efficient retrieval
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use trait objects (`Arc<dyn StorageManager>`, `Arc<dyn NetworkManager>`) for runtime polymorphism
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use trait-based abstractions for swappable implementations (e.g., `NetworkManager`, `StorageManager`)
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use async/await throughout the codebase with tokio runtime
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/disk/state.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use sequential phase-based synchronization with `SequentialSyncManager` for organized sync phases
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use state machines with `SyncState` enum to drive synchronization flow and state transitions
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:01:00.652Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Enable mempool tracking on SPV connection using the appropriate strategy (.fetchAll, .bloomFilter, .selective)
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rsdash-spv/src/storage/disk/state.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Add comprehensive unit tests in-module for individual components
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/tests/**/*.rs : Add integration tests in the `tests/` directory for comprehensive test suites
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/types.rs : Define common data structures in `types.rs` including `SyncProgress`, `ValidationMode`, `WatchItem`, etc.
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use domain-specific error types (`NetworkError`, `StorageError`, `SyncError`, `ValidationError`, `SpvError`) for error handling
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-02-25T06:13:52.858Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 51
File: dash/src/sml/masternode_list/scores_for_quorum.rs:50-73
Timestamp: 2025-02-25T06:13:52.858Z
Learning: ScoreHash is a cryptographic hash in the rust-dashcore library, and therefore does not need collision handling when used as a key in collections like BTreeMap due to the extremely low probability of collisions.
Applied to files:
dash-spv/src/storage/disk/state.rs
🧬 Code graph analysis (3)
dash-spv/src/sync/sequential/message_handlers.rs (1)
dash-spv/src/storage/disk/manager.rs (1)
new(79-129)
dash-spv/src/storage/disk/state.rs (3)
dash-spv/src/storage/memory.rs (1)
load_filters(288-306)dash-spv/src/storage/mod.rs (1)
load_filters(154-154)dash-spv/src/storage/disk/filters.rs (1)
load_filters(269-289)
dash-spv/src/storage/memory.rs (2)
dash-spv/src/storage/mod.rs (1)
load_filters(154-154)dash-spv/src/storage/disk/filters.rs (1)
load_filters(269-289)
⏰ 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_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Core Components Tests
- GitHub Check: Key Wallet Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (8)
dash-spv/src/types.rs (4)
18-18: LGTM! Well-chosen data structure for ordered filter matches.Using
BTreeMap<u32, Vec<[u8; 32]>>forfilter_matchesis appropriate as it provides efficient range-based queries (used inget_filter_matched_heights) and maintains height ordering naturally.Also applies to: 284-287
428-443: LGTM! Efficient batch recording with proper merge handling.The method correctly handles both new entries and merging with existing wallet IDs. The early return for empty
wallet_idsavoids unnecessary BTreeMap operations.
445-475: LGTM! Range query with appropriate size limit.The 10,000 block limit is consistent with the segmented storage architecture. The use of
BTreeMap::rangeprovides efficient O(log n + m) lookup where m is the number of entries in the range.
1154-1223: LGTM! Comprehensive test coverage for the new filter match API.The tests cover critical scenarios including bulk recording, range queries, the 10,000-block limit enforcement, and edge cases (empty ranges, partial overlaps). Well done!
dash-spv/src/sync/sequential/message_handlers.rs (1)
607-625: LGTM! Correct ordering of filter persistence and match detection.Storing the filter to disk (lines 607-611) before checking for matches ensures the filter data is persisted regardless of match results. The transition from boolean to
Vec<[u8; 32]>for wallet IDs enables fine-grained tracking.dash-spv/src/storage/mod.rs (1)
148-154: LGTM! Well-documented trait method with appropriate constraints.The
load_filtersmethod signature and documentation are clear. The 10,000 block limit is consistent with the segmented storage architecture documented in the coding guidelines.dash-spv/src/storage/disk/state.rs (1)
5-5: LGTM! Straightforward trait method delegation.The
Rangeimport and trait method implementation follow the established pattern in this file of delegating to inherent implementation methods.Also applies to: 574-576
dash-spv/src/storage/memory.rs (1)
288-306: LGTM! Consistent implementation with proper range validation.The implementation correctly:
- Enforces the 10,000 block limit using
saturating_sub- Returns
InvalidInputerror for oversized ranges- Skips missing filters as documented
- Matches the behavior of the disk storage implementation
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 (1)
dash-spv-ffi/FFI_API.md (1)
1393-1412: Add new FFI types to the Type Definitions section.The Type Definitions section should include the new filter-related types introduced in this PR to maintain complete documentation coverage.
Add the following types to the "Core Types" subsection after the existing types:
- `FFIEventCallbacks` - Event callback structure - `CoreSDKHandle` - Platform SDK integration handle +- `FFICompactFilter` - Compact filter with height and data +- `FFICompactFilters` - Array of compact filters with count +- `FFIFilterMatchEntry` - Filter match entry with height and wallet IDs +- `FFIFilterMatches` - Array of filter match entries with count
🧹 Nitpick comments (4)
dash-spv-ffi/FFI_API.md (2)
1414-1423: Update Memory Management section to document new filter types.The Memory Management section should reflect the new types and their lifecycle, particularly the range-query limits mentioned in the function documentation.
Add a new subsection after the "Important Rules" to document filter-specific lifecycle requirements:
5. **Shared Ownership**: `dash_spv_ffi_client_get_wallet_manager()` returns `FFIWalletManager*` that must be released with `dash_spv_ffi_wallet_manager_free()` + +### Filter API Usage + +1. **Range Limits**: Filter queries (`dash_spv_ffi_client_load_filters` and `dash_spv_ffi_client_get_filter_matched_heights`) are limited to 10,000-block ranges +2. **Filter Cleanup**: `FFICompactFilters` must be freed with `dash_spv_ffi_compact_filters_destroy()` +3. **Match Cleanup**: `FFIFilterMatches` must be freed with `dash_spv_ffi_filter_matches_destroy()`
1424-1474: Add example usage for new filter and transaction APIs.The Usage Examples section should demonstrate the new filter loading and query capabilities introduced in this PR.
Add a new example section after the Event Callbacks example:
dash_spv_ffi_client_set_event_callbacks(client, callbacks);Then append:
+ +### Loading and Querying Filters + +```c +// Load compact filters for a range +FFICompactFilters* filters = dash_spv_ffi_client_load_filters(client, 100, 1100); +if (filters != NULL) { + for (size_t i = 0; i < filters->count; i++) { + printf("Filter at height %u, data length: %zu\n", + filters->filters[i].height, + filters->filters[i].data_len); + } + dash_spv_ffi_compact_filters_destroy(filters); +} + +// Get filter-matched heights with wallet IDs +FFIFilterMatches* matches = dash_spv_ffi_client_get_filter_matched_heights(client, 100, 1100); +if (matches != NULL) { + for (size_t i = 0; i < matches->count; i++) { + printf("Height %u matched %zu wallet(s)\n", + matches->entries[i].height, + matches->entries[i].wallet_ids_count); + } + dash_spv_ffi_filter_matches_destroy(matches); +} + +// Get transaction statistics +usize tx_count = dash_spv_ffi_client_get_transaction_count(client); +usize block_count = dash_spv_ffi_client_get_blocks_with_transactions_count(client); +printf("Transactions: %zu across %zu blocks\n", tx_count, block_count); +```key-wallet-manager/src/wallet_manager/mod.rs (1)
91-94: UseWalletIdalias infilter_matches(and consider caching policy).You already define
pub type WalletId = [u8; 32];above; here you hard‑code[u8; 32]instead of usingWalletId. For consistency and easier refactors, it would be clearer to declare this as:filter_matches: BTreeMap<Network, BTreeMap<BlockHash, Vec<WalletId>>>,Also, you currently cache an entry even when the matched list is empty. If filters are processed for large ranges of heights, you may want to consider skipping insertion for the empty case to bound the cache size over long runs (the miss path just recomputes once). Not urgent, but worth thinking about given this is per‑block state.
key-wallet-manager/src/wallet_manager/process_block.rs (1)
87-132: Align return/cached types withWalletIdalias.
check_compact_filterreturnsVec<[u8; 32]>and pushes*wallet_idvalues, whileWalletManageralready definesWalletId = [u8; 32]. For clarity and to keep types in sync, it would be preferable to use the alias consistently here, e.g.:async fn check_compact_filter( &mut self, filter: &BlockFilter, block_hash: &BlockHash, network: Network, ) -> Vec<WalletId> { // ... let mut matched_wallet_ids: Vec<WalletId> = Vec::new(); // ... }and update the
WalletInterfacetrait / callers accordingly if feasible. This keeps the concept of “wallet id” explicit throughout the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dash-spv-ffi/FFI_API.md(9 hunks)key-wallet-manager/src/wallet_manager/mod.rs(1 hunks)key-wallet-manager/src/wallet_manager/process_block.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
key-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rs
🧠 Learnings (35)
📓 Common learnings
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Enable mempool tracking on SPV connection using the appropriate strategy (.fetchAll, .bloomFilter, .selective)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Implement `StorageManager` trait with segmented storage: headers in 10,000-header segments with index files, filters in separate storage, and state persistence in dedicated paths
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Clearly separate watch-only wallets from full wallets using the wallet type system; never attempt signing operations on watch-only accounts and validate external signatures match expected pubkeys
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation}/**/*.rs : Cache intermediate derivation results and use hardened derivation only when required; batch derive child keys when possible to optimize derivation performance
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Support multiple `KeySource` variants (Private, Public, NoKeySource) in address pool initialization to enable both full wallets and watch-only wallets with the same interface
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Use strongly typed enum-based system for `AccountType` with specific variants (Standard, Identity, Masternode, etc.) to provide compile-time safety for different account purposes
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Implement `StorageManager` trait with segmented storage: headers in 10,000-header segments with index files, filters in separate storage, and state persistence in dedicated paths
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
key-wallet-manager/src/wallet_manager/mod.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account,address_pool}/**/*.rs : Always validate network consistency: use the account's network type when creating addresses and assert that operations use the expected network to prevent mainnet/testnet mixing
Applied to files:
key-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true
Applied to files:
key-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : Use `TransactionRouter::classify_transaction()` to determine transaction type and `get_relevant_account_types()` to avoid checking all accounts for every transaction; route CoinJoin and other specialized transactions only to relevant account types
Applied to files:
key-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing
Applied to files:
key-wallet-manager/src/wallet_manager/process_block.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Use `?` operator for error propagation in Rust code to maintain clean error handling patterns
Applied to files:
key-wallet-manager/src/wallet_manager/process_block.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types in FFI functions
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/c_tests/**/*.{c,h} : Add corresponding C tests in `tests/c_tests/` for each new FFI function
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : All FFI types must have corresponding `_destroy()` functions for explicit memory management
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: When making changes to Rust FFI in dash-spv-ffi, rebuild with `cargo build --release` and run Swift tests to verify integration
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:01:00.652Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Use DashSDK's public methods, not direct SPVClient access, when accessing SPV functionality in Swift code
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: When debugging FFI issues, check `dash_spv_ffi_get_last_error()` for error details
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use domain-specific error types (`NetworkError`, `StorageError`, `SyncError`, `ValidationError`, `SpvError`) for error handling
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Add comprehensive unit tests in-module for individual components
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/tests/**/*.rs : Add integration tests in the `tests/` directory for comprehensive test suites
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv-ffi/FFI_API.md
🧬 Code graph analysis (1)
key-wallet-manager/src/wallet_manager/process_block.rs (1)
key-wallet-manager/src/wallet_manager/mod.rs (2)
new(70-76)new(107-114)
🪛 LanguageTool
dash-spv-ffi/FFI_API.md
[grammar] ~1117-~1117: Use a hyphen to join words.
Context: ...Matches ``` Description: Get filter matched heights with wallet IDs in a giv...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (3)
dash-spv-ffi/FFI_API.md (3)
99-100: LGTM!The new transaction count functions (
dash_spv_ffi_client_get_transaction_countanddash_spv_ffi_client_get_blocks_with_transactions_count) are well-documented with clear parameter descriptions and safety annotations. The distinction between persisted metrics and ephemeral sync statistics is appropriately explained.
1113-1124: LGTM!The filter loading and matched-heights query functions are comprehensively documented with:
- Clear range-limit constraints (10,000 blocks max)
- Explicit caller ownership and cleanup requirements
- Proper safety annotations and error handling guidance
- Return value semantics
Also applies to: 1193-1204
1238-1310: LGTM!All four destroy functions for the new filter types follow the established memory-management pattern consistently. Safety documentation is clear and comprehensive for each destruction operation.
| ``` | ||
| **Description:** | ||
| Get filter matched heights with wallet IDs in a given range. Returns an `FFIFilterMatches` struct containing all heights where filters matched and the wallet IDs that matched at each height. The caller must free the result using `dash_spv_ffi_filter_matches_destroy`. # Parameters - `client`: Valid pointer to an FFIDashSpvClient - `start_height`: Starting block height (inclusive) - `end_height`: Ending block height (exclusive) # Limits - Maximum range size: 10,000 blocks - If `end_height - start_height > 10000`, an error is returned # Returns - Non-null pointer to FFIFilterMatches on success - Null pointer on error (check `dash_spv_ffi_get_last_error`) # Safety - `client` must be a valid, non-null pointer - Caller must call `dash_spv_ffi_filter_matches_destroy` on the returned pointer |
There was a problem hiding this comment.
Fix hyphenation in description.
Line 1117 uses "filter matched heights" which should be hyphenated for grammatical correctness.
-Get filter matched heights with wallet IDs in a given range.
+Get filter-matched heights with wallet IDs in a given range.📝 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.
| Get filter matched heights with wallet IDs in a given range. Returns an `FFIFilterMatches` struct containing all heights where filters matched and the wallet IDs that matched at each height. The caller must free the result using `dash_spv_ffi_filter_matches_destroy`. # Parameters - `client`: Valid pointer to an FFIDashSpvClient - `start_height`: Starting block height (inclusive) - `end_height`: Ending block height (exclusive) # Limits - Maximum range size: 10,000 blocks - If `end_height - start_height > 10000`, an error is returned # Returns - Non-null pointer to FFIFilterMatches on success - Null pointer on error (check `dash_spv_ffi_get_last_error`) # Safety - `client` must be a valid, non-null pointer - Caller must call `dash_spv_ffi_filter_matches_destroy` on the returned pointer | |
| Get filter-matched heights with wallet IDs in a given range. Returns an `FFIFilterMatches` struct containing all heights where filters matched and the wallet IDs that matched at each height. The caller must free the result using `dash_spv_ffi_filter_matches_destroy`. # Parameters - `client`: Valid pointer to an FFIDashSpvClient - `start_height`: Starting block height (inclusive) - `end_height`: Ending block height (exclusive) # Limits - Maximum range size: 10,000 blocks - If `end_height - start_height > 10000`, an error is returned # Returns - Non-null pointer to FFIFilterMatches on success - Null pointer on error (check `dash_spv_ffi_get_last_error`) # Safety - `client` must be a valid, non-null pointer - Caller must call `dash_spv_ffi_filter_matches_destroy` on the returned pointer |
🧰 Tools
🪛 LanguageTool
[grammar] ~1117-~1117: Use a hyphen to join words.
Context: ...Matches ``` Description: Get filter matched heights with wallet IDs in a giv...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In dash-spv-ffi/FFI_API.md around line 1117, the phrase "Get filter matched
heights" should be corrected to use a hyphenated compound adjective; change it
to "Get filter-matched heights with wallet IDs in a given range." so the
description reads grammatically correct while preserving the rest of the
sentence and documentation details.
| // Collect script pubkeys for this specific wallet | ||
| let script_bytes: Vec<Vec<u8>> = | ||
| monitored.iter().map(|addr| addr.script_pubkey().as_bytes().to_vec()).collect(); | ||
|
|
||
| // Check if this wallet's addresses match the filter | ||
| let hit = filter | ||
| .match_any(block_hash, &mut script_bytes.iter().map(|s| s.as_slice())) | ||
| .unwrap_or(false) | ||
| }; | ||
| .unwrap_or(false); | ||
|
|
||
| if hit { | ||
| matched_wallet_ids.push(*wallet_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
ls -la | head -20Repository: dashpay/rust-dashcore
Length of output: 1252
🏁 Script executed:
find . -name "*.rs" -path "*/wallet*" -type f | head -20Repository: dashpay/rust-dashcore
Length of output: 944
🏁 Script executed:
rg "let script_bytes.*Vec.*Vec" --max-count 5Repository: dashpay/rust-dashcore
Length of output: 164
🏁 Script executed:
rg "filter\.match_any" -A 2 -B 2Repository: dashpay/rust-dashcore
Length of output: 342
🏁 Script executed:
cat -n key-wallet-manager/src/wallet_manager/process_block.rs | sed -n '100,140p'Repository: dashpay/rust-dashcore
Length of output: 1777
🏁 Script executed:
cat -n key-wallet-manager/src/wallet_manager/process_block.rs | sed -n '1,50p'Repository: dashpay/rust-dashcore
Length of output: 2115
🏁 Script executed:
cat -n key-wallet-manager/src/wallet_manager/process_block.rs | grep -E "fn|check_compact" -A 5 | head -40Repository: dashpay/rust-dashcore
Length of output: 1613
🏁 Script executed:
rg "check_compact_filter" -B 2 -A 15Repository: dashpay/rust-dashcore
Length of output: 18962
🏁 Script executed:
rg "match_any" -B 5 -A 5 --type rsRepository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
fd "bip158.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 235
🏁 Script executed:
rg "impl.*BlockFilter" -A 20 dash/src/bip158.rsRepository: dashpay/rust-dashcore
Length of output: 2544
🏁 Script executed:
rg "fn match_any" -A 5 --all-typesRepository: dashpay/rust-dashcore
Length of output: 478
🏁 Script executed:
cat dash/src/bip158.rs | grep -A 30 "fn match_any"Repository: dashpay/rust-dashcore
Length of output: 3317
🏁 Script executed:
cat -n dash/src/bip158.rs | sed -n '1,100p'Repository: dashpay/rust-dashcore
Length of output: 3946
🏁 Script executed:
rg "check_compact_filter" key-wallet-manager/src/wallet_interface.rs -B 2 -A 6Repository: dashpay/rust-dashcore
Length of output: 379
🏁 Script executed:
rg "check_compact_filter" key-wallet-manager/tests/ -A 3 -B 1Repository: dashpay/rust-dashcore
Length of output: 2124
🏁 Script executed:
rg "tracing::|log::|error!|warn!" key-wallet-manager/src/wallet_manager/ | head -10Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
cat -n key-wallet-manager/src/wallet_manager/process_block.rs | tail -50Repository: dashpay/rust-dashcore
Length of output: 2270
Handle match_any errors explicitly instead of treating them as "no match".
filter.match_any(...).unwrap_or(false) silently converts any error—such as IO errors from corrupted or malformed filter data—into a "no wallets matched" result. The subsequent cache insert at lines 125-129 then permanently stores this empty result for that (network, block_hash) pair, causing the wallet manager to silently miss relevant blocks without any indication of failure.
To preserve system reliability, either:
- Skip caching when
match_anyreturnsErr(_), allowing a retry on the next call - Add logging when errors occur so failures are observable
- Consider changing the return type to
Result<Vec<[u8; 32]>, Error>(requires trait update) to let callers decide how to handle filter errors
🤖 Prompt for AI Agents
In key-wallet-manager/src/wallet_manager/process_block.rs around lines 111 to
123, replace the current unwrap_or(false) behavior on filter.match_any so errors
are not treated as "no match" and cached forever: call match_any and handle the
Result explicitly — on Ok(true) push the wallet_id as before; on Ok(false)
continue normally; on Err(e) log the error with context (network, block_hash,
wallet_id) and DO NOT include this wallet in the matched list and DO NOT write a
cache entry for this (network, block_hash) so the check can be retried later
(alternatively propagate the error by returning Result and update call sites if
you prefer upstream handling). Ensure the cache insert code only runs when there
was no Err from match_any.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dash-spv/src/sync/sequential/message_handlers.rs (1)
641-656: ChainState per-match I/O concern remains unaddressed.Loading and saving ChainState for every filter match (lines 644-656) could cause performance issues during bulk filter sync. Additionally,
unwrap_or_else(|| ChainState::new())on line 648 masks storage errors by silently creating a fresh state.Consider:
- Batching filter match recording and persisting periodically (e.g., every N matches or at phase completion)
- Propagating the storage error rather than silently creating a new ChainState
- let mut chain_state = storage - .load_chain_state() - .await - .map_err(|e| SyncError::Storage(format!("Failed to load chain state: {}", e)))? - .unwrap_or_else(|| crate::types::ChainState::new()); + let mut chain_state = storage + .load_chain_state() + .await + .map_err(|e| SyncError::Storage(format!("Failed to load chain state: {}", e)))? + .ok_or_else(|| SyncError::Storage("Chain state not found".to_string()))?;dash-spv-ffi/FFI_API.md (1)
1116-1117: Hyphenate “filter-matched heights” in the description.The phrase “Get filter matched heights…” should be hyphenated as “filter-matched heights” to read as a compound adjective in the description.
-Get filter matched heights with wallet IDs in a given range. +Get filter-matched heights with wallet IDs in a given range.
🧹 Nitpick comments (3)
dash-spv/src/types.rs (1)
428-443: Consider deduplicating wallet IDs on record.If a filter at the same height is processed multiple times (e.g., due to retries), duplicates will accumulate since
extendappends without checking for existing entries. If this is a concern for memory or query accuracy:pub fn record_filter_matches(&mut self, height: u32, wallet_ids: Vec<[u8; 32]>) { if wallet_ids.is_empty() { return; } if let Some(ids) = self.filter_matches.get_mut(&height) { - ids.extend(wallet_ids); + for id in wallet_ids { + if !ids.contains(&id) { + ids.push(id); + } + } } else { self.filter_matches.insert(height, wallet_ids); } }Alternatively, use a
HashSetinternally if uniqueness is required.dash-spv-ffi/src/client.rs (2)
1669-1767: Avoid holding the client mutex acrosschain_state().awaitinget_filter_matched_heights.
dash_spv_ffi_client_get_filter_matched_heightscurrently keeps aMutexGuardoninnerwhile awaitingclient.chain_state().await. This is generally discouraged in async Rust because it:
- Holds a blocking
std::sync::Mutexacross an.await, potentially stalling any other FFI call that needs the same mutex while the future is pending.- Diverges from the established “take, await, then put back” pattern used elsewhere in this file.
Functionality and FFI ownership transfer for
FFIFilterMatchesandFFIFilterMatchEntryotherwise look correct.A more consistent pattern would be:
pub unsafe extern "C" fn dash_spv_ffi_client_get_filter_matched_heights( client: *mut FFIDashSpvClient, start_height: u32, end_height: u32, ) -> *mut crate::types::FFIFilterMatches { @@ - let result = client.runtime.block_on(async { - // Get chain state without taking the client - let chain_state = { - let guard = inner.lock().unwrap(); - match guard.as_ref() { - Some(client) => client.chain_state().await, - None => { - set_last_error("Client not initialized"); - return None; - } - } - }; + let result = client.runtime.block_on(async { + // Take the client out of the mutex while we await. + let mut spv_client = { + let mut guard = inner.lock().unwrap(); + match guard.take() { + Some(c) => c, + None => { + set_last_error("Client not initialized"); + return None; + } + } + }; + + let chain_state = spv_client.chain_state().await; + + // Put the client back before doing further work. + let mut guard = inner.lock().unwrap(); + *guard = Some(spv_client); @@ - // Get filter matches in range - works even during sync - let matches_result = chain_state.get_filter_matched_heights(start_height..end_height); + // Get filter matches in range. + let matches_result = chain_state.get_filter_matched_heights(start_height..end_height);This keeps the blocking mutex out of the async wait while preserving existing error reporting and FFI conversion semantics. Please double-check against how
DashSpvClientis used during long‑running sync to ensure this doesn’t conflict with your intended concurrency model.
1814-1867: Refine async + mutex usage inget_blocks_with_transactions_count.Functionally this looks good: it derives a set of unique confirmed block heights from the wallet’s transaction history and returns the count, matching the documentation.
As with
get_transaction_count, though, it keeps astd::sync::MutexGuardoninneracrosswallet.read().await, which can serialize any other caller needing the same mutex while that async lock is pending.You can follow the same pattern of cloning the wallet handle before awaiting:
pub unsafe extern "C" fn dash_spv_ffi_client_get_blocks_with_transactions_count( client: *mut FFIDashSpvClient, ) -> usize { @@ - let result = client.runtime.block_on(async { - // Get wallet without taking the client - let guard = inner.lock().unwrap(); - match guard.as_ref() { - Some(spv_client) => { - // Access wallet and get unique block heights - let wallet = spv_client.wallet(); - let wallet_guard = wallet.read().await; - let tx_history = wallet_guard.transaction_history(); - - // Count unique block heights (confirmed transactions only) - let mut unique_heights = std::collections::HashSet::new(); - for tx in tx_history { - if let Some(height) = tx.height { - unique_heights.insert(height); - } - } - - unique_heights.len() - } - None => { - tracing::warn!( - "Client not initialized when querying blocks with transactions count" - ); - 0 - } - } - }); + let result = client.runtime.block_on(async { + // Clone wallet handle without holding the mutex across .await + let wallet = { + let guard = inner.lock().unwrap(); + match guard.as_ref() { + Some(spv_client) => spv_client.wallet().clone(), + None => { + tracing::warn!( + "Client not initialized when querying blocks with transactions count" + ); + return 0; + } + } + }; + + let wallet_guard = wallet.read().await; + let tx_history = wallet_guard.transaction_history(); + + // Count unique block heights (confirmed transactions only) + let mut unique_heights = std::collections::HashSet::new(); + for tx in tx_history { + if let Some(height) = tx.height { + unique_heights.insert(height); + } + } + + unique_heights.len() + });This keeps the FFI interface the same while making the async locking behaviour friendlier to other callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dash-spv-ffi/FFI_API.md(9 hunks)dash-spv-ffi/src/client.rs(2 hunks)dash-spv/src/error.rs(2 hunks)dash-spv/src/storage/disk/state.rs(2 hunks)dash-spv/src/sync/sequential/message_handlers.rs(2 hunks)dash-spv/src/sync/sequential/phase_execution.rs(1 hunks)dash-spv/src/types.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- dash-spv/src/sync/sequential/phase_execution.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/src/error.rs
- dash-spv/src/storage/disk/state.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv-ffi/src/client.rsdash-spv/src/types.rs
dash-spv/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/**/*.rs: Use trait-based abstractions for swappable implementations (e.g.,NetworkManager,StorageManager)
Use async/await throughout the codebase with tokio runtime
Use domain-specific error types (NetworkError,StorageError,SyncError,ValidationError,SpvError) for error handling
Use trait objects (Arc<dyn StorageManager>,Arc<dyn NetworkManager>) for runtime polymorphism
Use Tokio channels for inter-component message passing
Add comprehensive unit tests in-module for individual components
Files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/sync/**/*.rs: Use sequential phase-based synchronization withSequentialSyncManagerfor organized sync phases
Use state machines withSyncStateenum to drive synchronization flow and state transitions
Files:
dash-spv/src/sync/sequential/message_handlers.rs
**/*-ffi/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Careful handling of memory safety at FFI boundaries in Rust code
Files:
dash-spv-ffi/src/client.rs
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: Use#[no_mangle] extern "C"attribute when implementing new FFI functions in Rust
All FFI types must have corresponding_destroy()functions for explicit memory management
Rust strings must be returned as*const c_charwith caller responsibility to free usingdash_string_free
Input strings in FFI functions are*const c_char(borrowed, not freed by C caller)
Add cbindgen annotations for complex types in FFI functions
Use thread-local storage for error propagation viadash_spv_ffi_get_last_error()function
Files:
dash-spv-ffi/src/client.rs
dash-spv/src/types.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Define common data structures in
types.rsincludingSyncProgress,ValidationMode,WatchItem, etc.
Files:
dash-spv/src/types.rs
🧠 Learnings (32)
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Implement `StorageManager` trait with segmented storage: headers in 10,000-header segments with index files, filters in separate storage, and state persistence in dedicated paths
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv-ffi/src/client.rsdash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Implement wallet functionality with UTXO tracking, balance calculation with confirmation states, and transaction processing
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv-ffi/src/client.rsdash-spv/src/types.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv-ffi/src/client.rsdash-spv/src/types.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use state machines with `SyncState` enum to drive synchronization flow and state transitions
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rs
📚 Learning: 2025-12-01T08:01:00.652Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Enable mempool tracking on SPV connection using the appropriate strategy (.fetchAll, .bloomFilter, .selective)
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use sequential phase-based synchronization with `SequentialSyncManager` for organized sync phases
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/managed_account/**/*.rs : Implement atomic state updates in ManagedAccount: when processing transactions, update address usage state, UTXO set, transaction history, and balance calculations together to maintain consistency
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rsdash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Use segmented storage with headers in 10,000-header chunks stored with index files for efficient retrieval
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/sync/sequential/message_handlers.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types in FFI functions
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Add comprehensive unit tests in-module for individual components
Applied to files:
dash-spv-ffi/src/client.rsdash-spv/src/types.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/tests/**/*.rs : Add integration tests in the `tests/` directory for comprehensive test suites
Applied to files:
dash-spv/src/types.rsdash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/types.rs : Define common data structures in `types.rs` including `SyncProgress`, `ValidationMode`, `WatchItem`, etc.
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use trait-based abstractions for swappable implementations (e.g., `NetworkManager`, `StorageManager`)
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use trait objects (`Arc<dyn StorageManager>`, `Arc<dyn NetworkManager>`) for runtime polymorphism
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:00:17.379Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.379Z
Learning: Applies to dash-spv/src/**/*.rs : Use domain-specific error types (`NetworkError`, `StorageError`, `SyncError`, `ValidationError`, `SpvError`) for error handling
Applied to files:
dash-spv/src/types.rs
📚 Learning: 2025-12-01T08:01:00.652Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Use DashSDK's public methods, not direct SPVClient access, when accessing SPV functionality in Swift code
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : All FFI types must have corresponding `_destroy()` functions for explicit memory management
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/c_tests/**/*.{c,h} : Add corresponding C tests in `tests/c_tests/` for each new FFI function
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: When debugging FFI issues, check `dash_spv_ffi_get_last_error()` for error details
Applied to files:
dash-spv-ffi/FFI_API.md
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
dash-spv-ffi/FFI_API.md
🧬 Code graph analysis (2)
dash-spv/src/sync/sequential/message_handlers.rs (2)
dash-spv/src/storage/disk/state.rs (2)
stats(400-431)stats(601-603)dash-spv/src/storage/disk/manager.rs (1)
new(79-129)
dash-spv-ffi/src/client.rs (3)
dash-spv-ffi/src/error.rs (1)
set_last_error(26-31)dash-spv/src/client/core.rs (2)
chain_state(206-209)wallet(161-163)dash-spv/src/client/status_display.rs (1)
chain_state(146-149)
🪛 LanguageTool
dash-spv-ffi/FFI_API.md
[grammar] ~1117-~1117: Use a hyphen to join words.
Context: ...Matches ``` Description: Get filter matched heights with wallet IDs in a giv...
(QB_NEW_EN_HYPHEN)
⏰ 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). (5)
- GitHub Check: Key Wallet Components Tests
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Pre-commit (ubuntu-latest)
🔇 Additional comments (8)
dash-spv/src/sync/sequential/message_handlers.rs (3)
614-618: Filter persistence before evaluation is the right approach.Storing the filter to disk before evaluating matches ensures data is persisted even if processing fails later. This aligns with the segmented storage pattern for filters.
620-632: API change to return matched wallet IDs is well-integrated.The shift from boolean to
Vec<[u8; 32]>enables tracking which specific wallets matched, supporting richer diagnostics and the new ChainState persistence. The length check on line 634 correctly replaces the previous boolean condition.
658-669: Improved logging provides useful diagnostic context.Including the matched wallet count and height in logs aids debugging filter sync issues. The log levels (debug for ChainState recording, info for block request) are appropriate.
dash-spv/src/types.rs (3)
284-288: Filter matches field uses appropriate data structure.
BTreeMap<u32, Vec<[u8; 32]>>provides ordered iteration by height and efficient range queries, which aligns well with theget_filter_matched_heightsmethod. The documentation clearly explains the purpose.Consider adding
filter_matchesto theDebugimpl (lines 543-555) for completeness, though omitting it is reasonable if the map could be large.
456-475: Range query implementation is efficient and well-bounded.The 10,000 block limit aligns with the storage segmentation pattern (headers in 10,000-header segments per coding guidelines). Using
BTreeMap::range()provides efficient O(log n + k) iteration. Thesaturating_subprevents underflow ifrange.start > range.end.
1153-1223: Unit tests provide good coverage of the new filter match API.Tests cover bulk recording, range queries, range limit enforcement, and edge cases (empty/partial ranges). The test names follow the descriptive naming convention per coding guidelines.
Note: The past review comment "method only tested, never used" is now resolved—
record_filter_matchesis called inmessage_handlers.rsline 651.dash-spv-ffi/FFI_API.md (1)
7-7: Docs for new filter and count APIs look consistent with the Rust FFI surface.The updated function counts, table rows, and detailed sections for the new filter-loading, filter-match, and transaction/block-count APIs correctly mirror the
extern "C"signatures, range limits, and destroy semantics fromdash-spv-ffi/src/client.rsanddash-spv-ffi/src/types.rs. Nothing to change here.Also applies to: 94-101, 143-165, 817-847, 1110-1123, 1190-1203, 1238-1267, 1280-1309
dash-spv-ffi/src/client.rs (1)
1281-1379: Filter-loading FFI looks sound and matches ownership expectations.The
dash_spv_ffi_client_load_filtersimplementation correctly:
- Validates the height range with a hard limit of 10,000 blocks.
- Avoids holding the client mutex across
awaitby cloning the storage handle first.- Transfers ownership of filter byte buffers and the
FFICompactFilterarray to FFI viaas_mut_ptr+mem::forget, relying on the corresponding destroy helpers.No changes needed here.
If you want an extra safety check, you can grep for the definitions of
FFICompactFilter,FFICompactFilters, anddash_spv_ffi_compact_filters_destroyto confirm the destroy path reconstructs and frees both the outer array and each filter’sdatabuffer symmetrically.
| /// Get the total count of transactions across all wallets. | ||
| /// | ||
| /// This returns the persisted transaction count from the wallet, | ||
| /// not the ephemeral sync statistics. Use this to show how many | ||
| /// blocks contained relevant transactions for the user's wallets. | ||
| /// | ||
| /// # Parameters | ||
| /// - `client`: Valid pointer to an FFIDashSpvClient | ||
| /// | ||
| /// # Returns | ||
| /// - Transaction count (0 or higher) | ||
| /// - Returns 0 if client not initialized or wallet not available | ||
| /// | ||
| /// # Safety | ||
| /// - `client` must be a valid, non-null pointer | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn dash_spv_ffi_client_get_transaction_count( | ||
| client: *mut FFIDashSpvClient, | ||
| ) -> usize { | ||
| null_check!(client, 0); | ||
|
|
||
| let client = &(*client); | ||
| let inner = client.inner.clone(); | ||
|
|
||
| let result = client.runtime.block_on(async { | ||
| // Get wallet without taking the client | ||
| let guard = inner.lock().unwrap(); | ||
| match guard.as_ref() { | ||
| Some(spv_client) => { | ||
| // Access wallet and get transaction count | ||
| let wallet = spv_client.wallet(); | ||
| let wallet_guard = wallet.read().await; | ||
| let tx_history = wallet_guard.transaction_history(); | ||
| tx_history.len() | ||
| } | ||
| None => { | ||
| tracing::warn!("Client not initialized when querying transaction count"); | ||
| 0 | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| result | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
cd /tmp && find . -name "client.rs" -path "*/dash-spv-ffi/src/*" 2>/dev/null | head -5Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Check the repository structure
git ls-files | grep -E "dash-spv-ffi.*client\.rs" | head -5Repository: dashpay/rust-dashcore
Length of output: 125
🏁 Script executed:
# Get a broader view of the repository structure
git ls-files | head -20Repository: dashpay/rust-dashcore
Length of output: 378
🏁 Script executed:
# Read the specific lines from the file
sed -n '1750,1820p' dash-spv-ffi/src/client.rs | cat -nRepository: dashpay/rust-dashcore
Length of output: 2848
🏁 Script executed:
# Also search for get_filter_matched_heights to understand the similar pattern mentioned
rg "get_filter_matched_heights" dash-spv-ffi/src/client.rs -B 5 -A 20Repository: dashpay/rust-dashcore
Length of output: 1909
🏁 Script executed:
# Check the wallet type and its structure to understand if cloning is feasible
rg "fn wallet\(" dash-spv-ffi/src/client.rs -B 2 -A 5Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Find the wallet() method definition
rg "fn wallet\(" dash-spv-ffi/src/ -B 2 -A 8Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Check the FFIDashSpvClient structure and inner type
rg "struct FFIDashSpvClient" dash-spv-ffi/src/ -B 2 -A 10Repository: dashpay/rust-dashcore
Length of output: 958
🏁 Script executed:
# Look at the full get_filter_matched_heights implementation to compare pattern
sed -n '1700,1750p' dash-spv-ffi/src/client.rs | cat -nRepository: dashpay/rust-dashcore
Length of output: 2285
🏁 Script executed:
# Let's check the InnerClient structure to understand wallet() return type
rg "struct InnerClient" dash-spv-ffi/src/ -A 10Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Also check what wallet() returns to confirm it's cloneable
rg "fn wallet\(&self\)" dash-spv-ffi/src/ -B 2 -A 5 | head -30Repository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Verify the wallet type more thoroughly
rg "wallet:" dash-spv-ffi/src/client.rs | head -5Repository: dashpay/rust-dashcore
Length of output: 204
🏁 Script executed:
# Double-check by looking at the exact code structure of both functions side-by-side
echo "=== get_filter_matched_heights pattern (CORRECT) ===" && \
sed -n '1716,1728p' dash-spv-ffi/src/client.rs && \
echo "" && \
echo "=== get_transaction_count pattern (PROBLEMATIC) ===" && \
sed -n '1789,1801p' dash-spv-ffi/src/client.rsRepository: dashpay/rust-dashcore
Length of output: 1130
Fix documentation and avoid holding client mutex across async operation in get_transaction_count.
-
The doc comment at line 24 incorrectly states "Use this to show how many blocks contained relevant transactions"—this function returns transaction count, not block count. That description applies to the separate
get_blocks_with_transactionsfunction. Change it to: "Use this to show how many transactions have been recorded for the user's wallets." -
The mutex guard on
inneris held acrosswallet.read().awaitat line 51. Unlikeget_filter_matched_heightsin the same file (which scopes the guard in a block and releases it before.await), this pattern unnecessarily blocks other callers waiting for the same mutex. Scope the guard to release it before awaiting:
let result = client.runtime.block_on(async {
- // Get wallet without taking the client
- let guard = inner.lock().unwrap();
- match guard.as_ref() {
- Some(spv_client) => {
- // Access wallet and get transaction count
- let wallet = spv_client.wallet();
- let wallet_guard = wallet.read().await;
- let tx_history = wallet_guard.transaction_history();
- tx_history.len()
- }
- None => {
- tracing::warn!("Client not initialized when querying transaction count");
- 0
- }
- }
+ // Clone wallet handle without holding the mutex across .await
+ let wallet = {
+ let guard = inner.lock().unwrap();
+ match guard.as_ref() {
+ Some(spv_client) => spv_client.wallet().clone(),
+ None => {
+ tracing::warn!("Client not initialized when querying transaction count");
+ return 0;
+ }
+ }
+ };
+
+ let wallet_guard = wallet.read().await;
+ let tx_history = wallet_guard.transaction_history();
+ tx_history.len()
});Committable suggestion skipped: line range outside the PR's diff.
|
Need to fix the following clippy errors: |
dash-spv/src/storage/disk/filters.rs
Outdated
| Ok(*self.cached_filter_tip_height.read().await) | ||
| } | ||
|
|
||
| /// Get the highest stored compact filter height by scanning the filters directory. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dash-spv/src/storage/disk/filters.rs (1)
220-228: Consider parallel loading for better performance.The sequential file I/O works correctly but could be slow for large ranges (up to 10,000 reads). Consider using
futures::stream::iterwithbuffer_unorderedortokio::spawnto parallelize filter loading.Example optimization:
use futures::stream::{self, StreamExt}; let tasks = range.map(|height| async move { self.load_filter(height).await.map(|opt| opt.map(|filter| (height, filter))) }); let results: Vec<_> = stream::iter(tasks) .buffer_unordered(50) // Load up to 50 filters concurrently .collect() .await; let mut result = Vec::new(); for res in results { if let Some((height, filter)) = res? { result.push((height, filter)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/storage/disk/filters.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/storage/disk/filters.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use trait-based abstractions for core components likeNetworkManagerandStorageManagerto enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles withcargo check --all-features
Files:
dash-spv/src/storage/disk/filters.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Storage module should provide abstraction via
StorageManagertrait with concrete implementations forMemoryStorageManagerandDiskStorageManager
Files:
dash-spv/src/storage/disk/filters.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 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
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/address_pool/**/*.rs : Pre-generate addresses in batches (typically 20-100) and store in address pools; only derive on-demand when pool is exhausted to optimize performance and memory usage
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation}/**/*.rs : Cache intermediate derivation results and use hardened derivation only when required; batch derive child keys when possible to optimize derivation performance
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*ffi*.rs : For FFI bindings, expose safe C interfaces with appropriate memory management and use #[no_mangle] for public FFI functions
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Storage should use segmented storage with headers stored in 10,000-header segments with index files, separate storage for filter headers and compact block filters
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`
Applied to files:
dash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations
Applied to files:
dash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/error.rs : Maintain domain-specific error types in `error.rs` with variants for `NetworkError`, `StorageError`, `SyncError`, `ValidationError`, and `SpvError` (top-level wrapper)
Applied to files:
dash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Storage should use segmented storage with headers stored in 10,000-header segments with index files, separate storage for filter headers and compact block filters
Applied to files:
dash-spv/src/storage/disk/filters.rs
🧬 Code graph analysis (1)
dash-spv/src/storage/disk/filters.rs (3)
dash-spv/src/storage/mod.rs (1)
load_filters(154-154)dash-spv/src/storage/memory.rs (2)
load_filters(288-306)new(30-42)dash-spv/src/storage/disk/state.rs (1)
load_filters(581-583)
🔇 Additional comments (3)
dash-spv/src/storage/disk/filters.rs (3)
8-8: LGTM: Import addition supports new error handling.The
StorageErrorimport is correctly added to support the range validation error in the newload_filtersmethod.
203-209: LGTM: Clear documentation and consistent signature.The method signature correctly implements the
StorageManagertrait, and the documentation clearly explains the behavior including the range limit and missing filter handling.
210-218: LGTM: Robust range validation with clear error messaging.The range validation logic is correct and consistent with the
MemoryStorageManagerimplementation. The use ofsaturating_subprevents underflow, and the error message clearly communicates the constraint.
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.