fix: dash spv syncing #140
Conversation
WalkthroughAdds a FilterHeadersProgress SpvEvent and FFI callback; forwards FilterHeadersProgress events through the FFI client; makes storage and filter-sync checkpoint-aware with storage-index ↔ absolute-height translations; forwards CFilter messages to the block processor; and adds extensive filter-sync tracking, queueing, and retry APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SPV as SPV Client
participant Event as SpvEvent
participant FFI as FFI Client
participant CB as C Callback
rect rgb(235,245,255)
note over SPV: compute absolute heights and percentage on change
SPV->>Event: Emit FilterHeadersProgress(filter_h, header_h, pct)
end
Event-->>FFI: Deliver FilterHeadersProgress
FFI->>FFI: call_filter_headers_progress(filter_h, header_h, pct)
alt callback set
FFI->>CB: on_filter_headers_progress(filter_h, header_h, pct, user_data)
else not set
FFI-->>FFI: no-op
end
sequenceDiagram
autonumber
participant Peer as Network Peer
participant MsgH as MessageHandler
participant BlockP as Block Processor
participant Wallet as Wallet Matcher
participant Event as SpvEvent
Peer-->>MsgH: CFilter(block_hash, filter)
rect rgb(240,255,240)
MsgH->>BlockP: ProcessCompactFilter(block_hash, filter, response_tx)
BlockP->>Wallet: Match filter -> emit CompactFilterMatched
Wallet-->>Event: CompactFilterMatched(...)
end
note over MsgH,BlockP: Forward CFilter to block processor instead of error-only path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
dash-spv/src/sync/filters.rs (2)
954-976: Stop-hash lookup incorrectly rejects tip at storage index 0Guarding on
storage_tip_index > 0prevents using a valid tip at index 0 (genesis-only or first-post-checkpoint state). Queryget_header(storage_tip_index)unconditionally and error only if it’s None.- let stop_hash = if storage_tip_index > 0 { - // Use storage_tip_height directly since get_header expects storage height - storage - .get_header(storage_tip_index) - .await - .map_err(|e| { - SyncError::Storage(format!( - "Failed to get stop header at storage height {} (blockchain height {}): {}", - storage_tip_index, header_tip_height, e - )) - })? - .ok_or_else(|| { - SyncError::Storage(format!( - "Stop header not found at storage height {} (blockchain height {})", - storage_tip_index, header_tip_height - )) - })? - .block_hash() - } else { - return Err(SyncError::Storage("No headers available for filter sync".to_string())); - }; + let stop_hash = storage + .get_header(storage_tip_index) + .await + .map_err(|e| { + SyncError::Storage(format!( + "Failed to get stop header at storage height {} (blockchain height {}): {}", + storage_tip_index, header_tip_height, e + )) + })? + .ok_or_else(|| { + SyncError::Storage(format!( + "Stop header not found at storage height {} (blockchain height {})", + storage_tip_index, header_tip_height + )) + })? + .block_hash();
1580-1598: Do not coerce “below-checkpoint” heights to storage index 0Using
None => 0forheader_abs_to_storage_index(...)can yield an incorrect stop hash (e.g., whenend == sync_base_height, whose header isn’t in header storage). Fail fast with a clear error instead.- let storage_height = match self.header_abs_to_storage_index(batch_end) { - Some(v) => v, - None => 0, - }; + let storage_height = self + .header_abs_to_storage_index(batch_end) + .ok_or_else(|| SyncError::Storage(format!( + "batch_end {} is below checkpoint base {}", + batch_end, self.sync_base_height + )))?;- let storage_height = match self.header_abs_to_storage_index(end) { - Some(v) => v, - None => 0, - }; + let storage_height = self.header_abs_to_storage_index(end).ok_or_else(|| { + SyncError::Storage(format!("retry end {} is below checkpoint base {}", end, self.sync_base_height)) + })?;- let storage_height = match self.header_abs_to_storage_index(end) { - Some(v) => v, - None => 0, - }; + let storage_height = self.header_abs_to_storage_index(end).ok_or_else(|| { + SyncError::Storage(format!("range end {} is below checkpoint base {}", end, self.sync_base_height)) + })?;- let batch_storage_height = - match self.header_abs_to_storage_index(batch_end) { - Some(v) => v, - None => 0, - }; + let batch_storage_height = self.header_abs_to_storage_index(batch_end) + .ok_or_else(|| SyncError::Storage(format!( + "batch end {} is below checkpoint base {}", + batch_end, self.sync_base_height + )))?;Also applies to: 1909-1912, 3353-3356, 3392-3396
dash-spv-ffi/tests/unit/test_async_operations.rs (1)
1-665: Add a unit test that exercises FilterHeadersProgress callbackGuideline requires a unit test per new FFI function. Add a minimal test that sets
on_filter_headers_progressand invokes the Rust-side bridge (no network). Example:#[test] #[serial] fn test_filter_headers_progress_callback_invoked() { use std::sync::atomic::{AtomicU32, Ordering}; let called = std::sync::Arc::new(AtomicU32::new(0)); struct ProgData { cnt: std::sync::Arc<AtomicU32> } extern "C" fn on_fhp(filter_h: u32, header_h: u32, pct: f64, user: *mut c_void) { assert!(pct >= 0.0 && pct <= 100.0); assert!(filter_h <= header_h); let d = unsafe { &*(user as *const ProgData) }; d.cnt.fetch_add(1, Ordering::SeqCst); } let data = ProgData { cnt: called.clone() }; // Build callbacks with only the new progress callback set let cbs = FFIEventCallbacks { on_block: None, on_transaction: None, on_balance_update: None, on_mempool_transaction_added: None, on_mempool_transaction_confirmed: None, on_mempool_transaction_removed: None, on_compact_filter_matched: None, on_wallet_transaction: None, on_filter_headers_progress: Some(on_fhp), user_data: &data as *const _ as *mut c_void, }; // Call the bridge directly (no client/network needed) // adjust path/name if different: dash_spv_ffi::callbacks::call_filter_headers_progress(&cbs, 150, 300, 50.0); assert_eq!(called.load(Ordering::SeqCst), 1); }If the bridge lives under a different module path, I can adapt the snippet.
dash-spv-ffi/include/dash_spv_ffi.h (1)
230-233: Declare or #include the FFINetwork typedef in dash-spv-ffi/include/dash_spv_ffi.hVerification: the provided script returned "FFINetwork typedef not found" for that header. Several public APIs in this file accept FFINetwork; without its typedef the header cannot be used standalone and will break C/Swift builds. Add the FFINetwork typedef or include the header that defines it.
Affected: dash-spv-ffi/include/dash_spv_ffi.h — lines 230–233 (also applies to 243–247, 257–261, 270–273).
dash-spv/src/storage/memory.rs (1)
325-327: get_header_height_by_hash must be standardized to return BLOCKCHAIN (absolute) heights — memory currently returns a storage index.memory::get_header_height_by_hash returns the raw header_hash_index (dash-spv/src/storage/memory.rs:325–327). disk/storage tests and many callers expect checkpoint-aware blockchain heights (disk tests around dash-spv/src/storage/disk.rs:2047–2059). filters.rs contains an ad‑hoc conversion for storage→blockchain (dash-spv/src/sync/filters.rs:2065–2069) but several call sites treat the result as absolute (e.g., dash-spv/src/sync/sequential/mod.rs:1949–1954, dash-spv/src/sync/masternodes.rs:192–196, dash-spv/src/chain/reorg.rs:375–381, dash-spv/src/client/message_handler.rs:526–531).
Action: either (A) make get_header_height_by_hash consistently return blockchain (absolute) heights (update memory.rs to apply the checkpoint/base offset), or (B) explicitly document and enforce “storage index” semantics everywhere and centralize conversions. Align implementations and callers accordingly.
🧹 Nitpick comments (8)
dash-spv/src/sync/filters.rs (2)
989-1075: Batch stop-hash backward scan: good safeguards; consider clamping scan lower-bound at checkpointLogic is solid. Minor improvement: clamp
min_heighttoself.sync_base_heightin checkpoint mode to avoid scanning into pre-base space.- let min_height = self.current_sync_height; // Don't go below where we are + let mut min_height = self.current_sync_height; + if self.sync_base_height > 0 { + min_height = min_height.max(self.sync_base_height); + }
2849-2869: Stability window hardcoded to 3sLooks fine; consider making this configurable via
ClientConfigif you see false positives/negatives in the wild.dash-spv/src/client/message_handler.rs (1)
168-185: Forwarding CFilter to the block processor: verify no double-processing side effects.We now send CFilter to both the sequential sync manager and the block processor. Ensure the block processor’s compact-filter handling is idempotent and won’t emit duplicate CompactFilterMatched events when the sync manager also inspects filters internally.
If duplication appears, consider gating the extra forward behind a config flag or an internal “already matched” check in the block processor.
dash-spv-ffi/src/callbacks.rs (1)
393-411: Reduce log noise for high-frequency progress callbacks.Filter headers progress can fire often; INFO may spam logs in apps. Suggest DEBUG for “calling/completed” lines.
- tracing::info!( + tracing::debug!( "📊 Calling filter headers progress callback: filter_height={}, header_height={}, pct={:.2}", filter_height, header_height, percentage ); ... - tracing::info!("✅ Filter headers progress callback completed"); + tracing::debug!("✅ Filter headers progress callback completed");dash-spv-ffi/src/client.rs (1)
730-732: Unused sync_running flag.The AtomicBool is set to false but never read; dead state. Remove it or integrate it to stop the progress thread early.
- // Shared flag to coordinate internal threads during sync - let sync_running = Arc::new(AtomicBool::new(true)); ... - // Signal background handlers to stop - sync_running_for_join.store(false, Ordering::Relaxed);Also applies to: 808-810
dash-spv/src/storage/memory.rs (3)
152-176: Range-to-index translation is correct; consider documenting end-exclusivity.Logic for clamping and converting absolute Range (end-exclusive) to indices is correct. Add a brief doc comment to avoid misuse.
178-184: Minor: early return on empty slice.You can return early when start_idx >= self.filter_headers.len() to avoid slicing math.
Apply this minimal tweak:
- if start_idx > self.filter_headers.len() { + if start_idx >= self.filter_headers.len() { return Ok(Vec::new()); }
59-71: DRY: factor out a small helper to fetch (base, has_base).You repeat sync-state probing in three places. Consider a private helper to reduce duplication and branch drift.
Also applies to: 144-151, 203-211
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
dash-spv-ffi/include/dash_spv_ffi.h(2 hunks)dash-spv-ffi/src/callbacks.rs(4 hunks)dash-spv-ffi/src/client.rs(6 hunks)dash-spv-ffi/tests/integration/test_full_workflow.rs(2 hunks)dash-spv-ffi/tests/test_event_callbacks.rs(2 hunks)dash-spv-ffi/tests/unit/test_async_operations.rs(1 hunks)dash-spv/src/client/message_handler.rs(1 hunks)dash-spv/src/client/mod.rs(5 hunks)dash-spv/src/storage/memory.rs(3 hunks)dash-spv/src/sync/filters.rs(28 hunks)dash-spv/src/types.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/types.rsdash-spv/src/storage/memory.rsdash-spv/src/client/mod.rsdash-spv/src/client/message_handler.rsdash-spv/src/sync/filters.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/types.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv/src/storage/memory.rsdash-spv/src/client/mod.rsdash-spv-ffi/tests/integration/test_full_workflow.rsdash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rsdash-spv/src/client/message_handler.rsdash-spv/src/sync/filters.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/types.rsdash-spv/src/storage/memory.rsdash-spv/src/client/mod.rsdash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rsdash-spv/src/client/message_handler.rsdash-spv/src/sync/filters.rs
dash-spv-ffi/tests/unit/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
Add a corresponding Rust unit test for each new FFI function
Files:
dash-spv-ffi/tests/unit/test_async_operations.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/tests/integration/test_full_workflow.rsdash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic
Files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/integration/test_full_workflow.rsdash-spv-ffi/tests/test_event_callbacks.rs
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/tests/integration/test_full_workflow.rsdash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rs
dash-spv-ffi/include/dash_spv_ffi.h
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
The public C header is generated; ensure it reflects FFI changes after builds
Files:
dash-spv-ffi/include/dash_spv_ffi.h
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Provide both
MemoryStorageManagerandDiskStorageManagerbehind theStorageManagertrait
Files:
dash-spv/src/storage/memory.rs
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: FFI functions must be declared with #[no_mangle] extern "C"
Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Accept input strings from C as *const c_char (borrowed; do not free inputs)
Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Add cbindgen annotations for complex types so headers are generated correctly
Files:
dash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rs
dash-spv-ffi/src/callbacks.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
Use C function pointers for async progress/completion callbacks; callbacks may be invoked from any thread
Files:
dash-spv-ffi/src/callbacks.rs
🧠 Learnings (9)
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/src/types.rsdash-spv/src/client/mod.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/callbacks.rs : Use C function pointers for async progress/completion callbacks; callbacks may be invoked from any thread
Applied to files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/tests/test_event_callbacks.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv-ffi/tests/unit/test_async_operations.rsdash-spv-ffi/tests/integration/test_full_workflow.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
Applied to files:
dash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h : After updating dash-spv-ffi, synchronize the generated header into the Swift SDK using ./sync-headers.sh so Sources/DashSPVFFI/include/dash_spv_ffi.h stays up to date
Applied to files:
dash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/client.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv/src/storage/memory.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/client/mod.rsdash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/src/callbacks.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types so headers are generated correctly
Applied to files:
dash-spv-ffi/src/callbacks.rs
🧬 Code graph analysis (3)
dash-spv/src/storage/memory.rs (2)
dash-spv/src/storage/disk.rs (3)
new(210-324)get_filter_header(1378-1413)get_filter_tip_height(1415-1417)dash-spv/src/storage/mod.rs (2)
get_filter_header(130-130)get_filter_tip_height(133-133)
dash-spv/src/client/mod.rs (3)
dash-spv/src/storage/memory.rs (1)
stats(263-323)dash-spv/src/storage/disk.rs (1)
stats(1592-1623)dash-spv/src/network/peer.rs (2)
stats(122-128)connected_peers(69-71)
dash-spv-ffi/src/client.rs (3)
dash-spv-ffi/tests/test_event_callbacks.rs (1)
new(29-45)dash-spv/src/client/message_handler.rs (1)
new(36-60)dash-spv/src/client/mod.rs (1)
new(174-237)
⏰ 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). (6)
- GitHub Check: Integration Tests (stable, 22.1.3)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: SPV Components Tests
- GitHub Check: Key Wallet Components Tests
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (19)
dash-spv/src/sync/filters.rs (4)
1569-1587: Queue builder end-hash resolution should fail if header missing at computed indexRight now this path errors nicely; good. Ensure the same strictness is applied symmetrically in all other range builders (see earlier comment replacing
None => 0).
1104-1119: Start-height validation LGTMRejecting
start_height == 0for GetCFHeaders is correct per protocol semantics.
3234-3249: Progress “gap” uses filter header tip vs last synced filter heightThis reads consistent. If
last_synced_filter_heightis absolute, ensure all producers set absolute heights (ties back to the height-space consistency comment).
1792-1809: get_header_height_by_hash returns absolute (blockchain) heights — remove the erroneous storage→absolute conversionsConfirmed: storage implementations and tests treat the reverse index as blockchain heights (memory: src/storage/memory.rs:325–327; disk tests comment at src/storage/disk.rs:2046–2058). Do not call header_storage_to_abs_height on values returned by get_header_height_by_hash — instead treat them as absolute heights. Fix example: remove the conversion at src/sync/filters.rs:2068–2074 (and any similar sites where get_header_height_by_hash is converted).
Likely an incorrect or invalid review comment.
dash-spv-ffi/tests/unit/test_async_operations.rs (1)
593-604: New callback field initialization LGTM
on_filter_headers_progress: Nonekeeps tests forward‑compatible with the extended API.dash-spv/src/types.rs (1)
788-804: New SpvEvent::FilterHeadersProgress variant looks goodAbsolute heights + clamped percentage is sensible. Ensure all event match sites are updated; CI should catch exhaustiveness.
dash-spv-ffi/tests/test_event_callbacks.rs (1)
166-178: FFI callbacks updated to include on_filter_headers_progressAdditive change initialized to None keeps existing tests stable.
Also applies to: 260-272
dash-spv-ffi/tests/integration/test_full_workflow.rs (1)
190-201: Integration callbacks extended with new optional fieldsLGTM; fields set to None preserve behavior.
dash-spv-ffi/src/callbacks.rs (1)
138-141: FFI surface extension looks good.Type alias, struct field, and Default init are consistent and cbindgen-friendly.
Also applies to: 152-154, 170-185
dash-spv-ffi/src/client.rs (2)
233-242: Event wiring for FilterHeadersProgress: LGTM.Correctly maps SpvEvent to the new FFI callback with the expected argument order.
1042-1045: Good: visibility into whether the new callback is set.This aids integration debugging without affecting behavior.
dash-spv/src/client/mod.rs (4)
731-734: De-dupe guard for FilterHeadersProgress events: nice touch.Tracking last emitted heights to avoid redundant events is appropriate.
869-889: Header rate computation: confirm units under checkpoint mode.You measure headers/sec using storage tip (index) deltas while user-facing heights include sync_base_height + tip. That’s fine; just ensure any UI consuming headers_per_second understands it’s relative to storage growth (not absolute height).
939-966: FilterHeadersProgress percentage and absolute-height handling: LGTM.
- Uses absolute header height = sync_base_height + storage_tip
- Guards div-by-zero; clamps to 100%; emits only on change
2413-2419: Debug stats log is useful and low-noise.No functional impact; helps diagnose sync visibility.
dash-spv/src/storage/memory.rs (4)
59-71: Checkpoint offset semantics: confirm +1 vs. base alignment.You compute abs_offset = sync_base_height + 1 for headers (log-only). Elsewhere (client/disk), absolute-height math may use base + storage_index. Please confirm the intended convention so header/log heights align across modules.
73-73: Header duplicate handling and absolute-height logs look good.Indexing and O(1) hash reverse index are correct; logs include both storage_index and computed absolute height.
Also applies to: 78-85, 93-101
144-151: Good: absolute-to-storage mapping base detection.Deriving the base from sync_state and gating on synced_from_checkpoint is sound and matches disk storage behavior.
203-211: Tip height calculation (absolute) looks correct.Returns base + len - 1 and guards empty case. Matches the absolute-height contract for filters.
| typedef void (*FilterHeadersProgressCallback)(uint32_t filter_height, | ||
| uint32_t header_height, | ||
| double percentage, | ||
| void *user_data); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New FilterHeadersProgress callback shape and placement look correct.
Signature matches Rust (u32, u32, f64, void*), and appending the field at the end of FFIEventCallbacks preserves ABI for rebuilt consumers.
Please also sync the generated header into the Swift SDK to keep it in lockstep:
Also applies to: 166-167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Swift header is in sync with generated one
GEN="dash-spv-ffi/include/dash_spv_ffi.h"
SWIFT="swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h"
if [[ -f "$GEN" && -f "$SWIFT" ]]; then
if cmp -s "$GEN" "$SWIFT"; then
echo "Headers are in sync ✅"
else
echo "Headers differ ❌"; diff -u "$SWIFT" "$GEN" | sed -n '1,200p'
echo "Run ./sync-headers.sh to update."
exit 1
fi
else
echo "One or both header paths missing. Please check repository layout."
exit 1
fiLength of output: 1048
Sync Swift header with generated header (run ./sync-headers.sh)
dash-spv-ffi/include/dash_spv_ffi.h adds the FilterHeadersProgress typedef and the on_filter_headers_progress field; swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h is out of sync — run ./sync-headers.sh to update. Signature matches Rust (u32, u32, f64, void*) and appending the field at the end of FFIEventCallbacks preserves ABI.
🤖 Prompt for AI Agents
In dash-spv-ffi/include/dash_spv_ffi.h around lines 152-156 the new typedef
FilterHeadersProgressCallback and the on_filter_headers_progress callback field
were added but the Swift copy
swift-dash-core-sdk/Sources/DashSPVFFI/include/dash_spv_ffi.h is out of sync;
run ./sync-headers.sh to regenerate the Swift header so it includes the typedef
(uint32_t, uint32_t, double, void*) and append the on_filter_headers_progress
field at the end of the FFIEventCallbacks struct to preserve ABI compatibility.
| async fn get_filter_header(&self, height: u32) -> StorageResult<Option<FilterHeader>> { | ||
| // Filter headers are stored starting from height 0 in the vector | ||
| Ok(self.filter_headers.get(height as usize).copied()) | ||
| // Map blockchain (absolute) height to storage index relative to checkpoint base | ||
| let base = match self.load_sync_state().await { | ||
| Ok(Some(state)) if state.synced_from_checkpoint && state.sync_base_height > 0 => { | ||
| state.sync_base_height | ||
| } | ||
| _ => 0u32, | ||
| }; | ||
|
|
||
| let idx = height.saturating_sub(base) as usize; | ||
| Ok(self.filter_headers.get(idx).copied()) | ||
| } |
There was a problem hiding this comment.
Bug: incorrect mapping for heights below checkpoint base (possible wrong filter header).
Using saturating_sub returns index 0 for requests where height < base, yielding a wrong header instead of None (disk impl returns None with a warning). Fix to match disk semantics.
Apply this diff:
async fn get_filter_header(&self, height: u32) -> StorageResult<Option<FilterHeader>> {
// Map blockchain (absolute) height to storage index relative to checkpoint base
let base = match self.load_sync_state().await {
Ok(Some(state)) if state.synced_from_checkpoint && state.sync_base_height > 0 => {
state.sync_base_height
}
_ => 0u32,
};
-
- let idx = height.saturating_sub(base) as usize;
- Ok(self.filter_headers.get(idx).copied())
+ if base > 0 && height < base {
+ tracing::warn!(
+ "Attempting to get filter header at height {} below sync_base_height {}",
+ height,
+ base
+ );
+ return Ok(None);
+ }
+ let idx = if base > 0 { (height - base) as usize } else { height as usize };
+ Ok(self.filter_headers.get(idx).copied())
}📝 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.
| async fn get_filter_header(&self, height: u32) -> StorageResult<Option<FilterHeader>> { | |
| // Filter headers are stored starting from height 0 in the vector | |
| Ok(self.filter_headers.get(height as usize).copied()) | |
| // Map blockchain (absolute) height to storage index relative to checkpoint base | |
| let base = match self.load_sync_state().await { | |
| Ok(Some(state)) if state.synced_from_checkpoint && state.sync_base_height > 0 => { | |
| state.sync_base_height | |
| } | |
| _ => 0u32, | |
| }; | |
| let idx = height.saturating_sub(base) as usize; | |
| Ok(self.filter_headers.get(idx).copied()) | |
| } | |
| async fn get_filter_header(&self, height: u32) -> StorageResult<Option<FilterHeader>> { | |
| // Map blockchain (absolute) height to storage index relative to checkpoint base | |
| let base = match self.load_sync_state().await { | |
| Ok(Some(state)) if state.synced_from_checkpoint && state.sync_base_height > 0 => { | |
| state.sync_base_height | |
| } | |
| _ => 0u32, | |
| }; | |
| if base > 0 && height < base { | |
| tracing::warn!( | |
| "Attempting to get filter header at height {} below sync_base_height {}", | |
| height, | |
| base | |
| ); | |
| return Ok(None); | |
| } | |
| let idx = if base > 0 { (height - base) as usize } else { height as usize }; | |
| Ok(self.filter_headers.get(idx).copied()) | |
| } |
🤖 Prompt for AI Agents
In dash-spv/src/storage/memory.rs around lines 186 to 197, the current use of
saturating_sub maps any height < base to index 0 which returns an incorrect
FilterHeader; change the logic to detect if height < base and return Ok(None)
(and optionally log or warn about the out-of-range request) to match the disk
implementation semantics; only compute idx = (height - base) as usize and index
into filter_headers when height >= base and idx < filter_headers.len(),
otherwise return Ok(None).
…ith checkpoint sync - Adjusted absolute height calculations in MemoryStorageManager to be base-inclusive. - Updated comments for clarity on how storage indexing relates to absolute blockchain height. - Ensured consistency in height retrieval methods to align with DiskStorage behavior.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/sync/filters.rs (1)
3190-3217: Unit mismatch: comparing block header STORAGE index with filter header ABSOLUTE height.This underreports/overreports gaps under checkpoints. Convert block tip index to absolute before diffing.
- let block_height = storage - .get_tip_height() - .await - .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? - .unwrap_or(0); + let block_storage_tip = storage + .get_tip_height() + .await + .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? + .unwrap_or(0); + let block_height = self.header_storage_to_abs_height(block_storage_tip); @@ - let gap_size = block_height.saturating_sub(filter_height); + let gap_size = block_height.saturating_sub(filter_height);
♻️ Duplicate comments (1)
dash-spv/src/storage/memory.rs (1)
186-197: Bug: incorrect mapping for heights below checkpoint base (may return wrong header).Using saturating_sub maps any height < base to index 0, returning the first stored header instead of None. This diverges from DiskStorage semantics and breaks overlap detection.
- let idx = height.saturating_sub(base) as usize; - Ok(self.filter_headers.get(idx).copied()) + if base > 0 && height < base { + tracing::warn!( + "Attempting to get filter header at height {} below sync_base_height {}", + height, + base + ); + return Ok(None); + } + let idx = if base > 0 { (height - base) as usize } else { height as usize }; + Ok(self.filter_headers.get(idx).copied())
🧹 Nitpick comments (5)
dash-spv/src/storage/memory.rs (1)
144-184: Guard against start_idx > end_idx to avoid slice panics.A caller can pass an empty or inverted range causing start_idx > end_idx. Add a fast return to keep this safe.
- if start_idx > self.filter_headers.len() { + if start_idx > self.filter_headers.len() { return Ok(Vec::new()); } - let end_idx = end_idx.min(self.filter_headers.len()); - Ok(self.filter_headers[start_idx..end_idx].to_vec()) + let end_idx = end_idx.min(self.filter_headers.len()); + if start_idx >= end_idx { + return Ok(Vec::new()); + } + Ok(self.filter_headers[start_idx..end_idx].to_vec())dash-spv/src/sync/filters.rs (4)
693-861: Timeout recovery path mostly mirrors the main flow; watch the same 0-fallback bug elsewhere.The main tip-handling branch is fine. Ensure any future absolute→storage conversions don’t fall back to 0 silently.
1103-1111: Minor: consider allowing genesis-only scenarios in request_filter_headers callers.You already guard start_height != 0. Ensure callers never pass 0 (download_filter_header_for_block could, for genesis). If that’s intentional, add a short comment there.
693-707: SYNC_TIMEOUT_SECONDS is short (5s).If mainnet peers lag, consider making it configurable. Not a blocker.
693-861: Optional: expose knobs for max retries and timeouts via ClientConfig.Defaults are good; configurability helps ops tune on varied networks.
Also applies to: 1886-1956, 3220-3492
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dash-spv/src/storage/memory.rs(4 hunks)dash-spv/src/sync/filters.rs(28 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/storage/memory.rsdash-spv/src/sync/filters.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Provide both
MemoryStorageManagerandDiskStorageManagerbehind theStorageManagertrait
Files:
dash-spv/src/storage/memory.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/storage/memory.rsdash-spv/src/sync/filters.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/storage/memory.rsdash-spv/src/sync/filters.rs
🧠 Learnings (2)
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv/src/storage/memory.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/sync/filters.rs
🧬 Code graph analysis (1)
dash-spv/src/storage/memory.rs (2)
dash-spv/src/storage/disk.rs (3)
new(210-324)get_filter_header(1378-1413)get_filter_tip_height(1415-1417)dash-spv/src/storage/mod.rs (2)
get_filter_header(130-130)get_filter_tip_height(133-133)
⏰ 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). (20)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: Integration Tests (stable, 22.1.3)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: Core Components Tests
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: SPV Components Tests
- GitHub Check: Key Wallet Components Tests
🔇 Additional comments (22)
dash-spv/src/storage/memory.rs (3)
59-71: Checkpoint-aware absolute-height offset looks correct.Deriving abs_offset from sync_base_height once per batch is clean and keeps logs and mappings coherent with checkpoint mode.
203-211: Tip height computation aligns with absolute-height semantics.Returning base + len - 1 in checkpoint mode matches DiskStorage behavior.
325-341: Reverse-index returns absolute height as expected.Mapping stored storage_index to base + index keeps callers consistent across backends.
dash-spv/src/sync/filters.rs (19)
23-28: Batch size set to 1999 is a sensible safety margin under Dash Core’s 2000 limit.
116-175: Header tip height conversion to absolute is correct.Converting storage tip index via header_storage_to_abs_height avoids unit mix-ups later.
1112-1119: Validation and diagnostics for GetCFHeaders look good.Rejecting start_height == 0 and logging both header/filter storage indices aid debugging.
498-517: Next-batch stop-hash resolution is robust.Good fallback to current_sync_height when end falls at/before base.
220-238: Index/height conversion helpers are clean and consistent.Base-inclusive convention is well documented and applied.
1241-1277: Checkpoint overlap handling is pragmatic.Synthesizing a connection point at base-1 avoids false negatives when peers start at/around the checkpoint.
950-989: Startup flow is solid; initial batch stop-hash search includes safe backward scan.Error messages are descriptive; avoids oversize requests at the tip.
Also applies to: 990-1089
2058-2079: Height lookup uses reverse index first — good.Range checking on absolute height avoids unnecessary scans.
173-176: Diagnostics are excellent.The added context in debug logs will materially speed incident forensics.
Also applies to: 294-362
3528-3548: Range merging respects MAX_FILTER_REQUEST_SIZE — nice.Keeps retries efficient without violating protocol limits.
2877-2891: Stats helpers are straightforward and thread-safe.No blocking or unnecessary cloning on the hot path.
Also applies to: 2956-2965, 2983-3002
1718-1771: Active-request completion check is fine; “dummy completion” is a neat trigger.Ensures queue keeps flowing without busy-waiting.
2626-2639: Clear filter-sync state without touching received heights — sensible default.
1886-1956: Timeout handler uses per-range retry counts and prioritizes retries.After fixing the storage-index fallback, this logic is solid.
1444-1454: Good: err out when batch_end below base (no silent fallback).This is the correct pattern to use elsewhere too.
869-949: Start conditions and peer capability checks are correct.Early exit when no COMPACT_FILTERS peers prevents wasted work.
2144-2166: Non-blocking placeholder for wallet match-check path is fine.Clear TODO and safe behavior.
2626-2651: is_syncing_filters returns true while queue/active exist — good UX.
1718-1731: Lock usage on received_filter_heights is minimal and safe.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/sync/filters.rs (1)
3177-3201: Unit mix-up: comparing header storage index vs. absolute height in gap check.get_tip_height() returns a storage index; filter tip is absolute. Convert storage index to absolute before computing gap or you may spuriously trigger/skip restarts.
- let block_height = storage - .get_tip_height() - .await - .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? - .unwrap_or(0); + let block_tip_index = storage + .get_tip_height() + .await + .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? + .unwrap_or(0); + let block_height = self.header_storage_to_abs_height(block_tip_index);
♻️ Duplicate comments (1)
dash-spv/src/sync/filters.rs (1)
501-513: Eliminate unwrap_or_default() fallbacks when converting abs→storage indices (can misaddress stop_hash / retries).Defaulting to 0 (or silently substituting) risks targeting the wrong header under checkpoints. Fail fast or explicitly skip the batch/retry. Applies to next-batch stop hash, tip/prev lookups, queue build, and retry paths.
Patch set:
@@ - let storage_height = self - .header_abs_to_storage_index(next_batch_end_height) - .unwrap_or_else(|| { - tracing::warn!( - "next_batch_end_height {} is at or before checkpoint base {}", - next_batch_end_height, - self.sync_base_height - ); - // Fallback to current_sync_height - self.header_abs_to_storage_index(self.current_sync_height) - .unwrap_or_default() - }); + let storage_height = self + .header_abs_to_storage_index(next_batch_end_height) + .or_else(|| { + tracing::warn!( + "next_batch_end_height {} is at or before checkpoint base {}", + next_batch_end_height, + self.sync_base_height + ); + // Try current_sync_height, but never default to 0 + self.header_abs_to_storage_index(self.current_sync_height) + }) + .ok_or_else(|| { + SyncError::Storage(format!( + "Cannot resolve storage index for next_batch_end {} or current_sync_height {} (below checkpoint base {})", + next_batch_end_height, self.current_sync_height, self.sync_base_height + )) + })?; @@ - let tip_storage_height = - self.header_abs_to_storage_index(header_tip_height).unwrap_or_default(); + let tip_storage_height = self + .header_abs_to_storage_index(header_tip_height) + .ok_or_else(|| SyncError::Storage("tip height below checkpoint base".to_string()))?; @@ - let prev_storage_height = self - .header_abs_to_storage_index(header_tip_height - 1) - .unwrap_or_default(); + let prev_storage_height = self + .header_abs_to_storage_index(header_tip_height - 1) + .ok_or_else(|| SyncError::Storage("prev tip height below checkpoint base".to_string()))?;@@ - let storage_height = self.header_abs_to_storage_index(batch_end).unwrap_or_default(); + let storage_height = self + .header_abs_to_storage_index(batch_end) + .ok_or_else(|| SyncError::Storage("batch_end below checkpoint base".to_string()))?;@@ - let storage_height = self.header_abs_to_storage_index(end).unwrap_or_default(); + let storage_height = match self.header_abs_to_storage_index(end) { + Some(v) => v, + None => { + tracing::warn!( + "Cannot retry timed out range {}-{}: end below checkpoint base", + start, end + ); + return Ok(()); + } + };@@ - let storage_height = self.header_abs_to_storage_index(end).unwrap_or_default(); + let Some(storage_height) = self.header_abs_to_storage_index(end) else { + tracing::warn!( + "Skipping retry for {}-{}: end below checkpoint base", + start, end + ); + continue; + }; @@ - let batch_storage_height = - self.header_abs_to_storage_index(batch_end).unwrap_or_default(); + let Some(batch_storage_height) = + self.header_abs_to_storage_index(batch_end) + else { + tracing::warn!( + "Skipping sub-batch {}-{}: end below checkpoint base", + current_start, batch_end + ); + current_start = batch_end + 1; + continue; + };Also applies to: 615-627, 1569-1587, 1897-1919, 3336-3340, 3372-3390
🧹 Nitpick comments (4)
dash-spv/src/sync/filters.rs (4)
1874-1879: Remove unused _network param and trait-object mismatch for consistency.Simplifies API; call sites already don’t use it.
- async fn handle_request_timeout( - &mut self, - range: (u32, u32), - _network: &mut dyn NetworkManager, - storage: &S, - ) -> SyncResult<()> { + async fn handle_request_timeout( + &mut self, + range: (u32, u32), + storage: &S, + ) -> SyncResult<()> { @@ - self.handle_request_timeout(range, network, storage).await?; + self.handle_request_timeout(range, storage).await?;Also applies to: 1863-1866
29-33: Consider making flow-control constants configurable (via ClientConfig).Hard-coding concurrency/timeouts makes tuning harder across networks and peers. Non-blocking.
Want me to thread these through ClientConfig and add sane defaults?
1562-1627: Queue-build logging is helpful but might be noisy on large syncs.If logs get chatty in production, gate detailed per-batch logs behind trace.
2620-2623: Minor: clarify that received_filter_heights is cumulative across sessions.Small doc note to avoid confusion when operators expect a fresh count after reset().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/sync/filters.rs(28 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/sync/filters.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/sync/filters.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/sync/filters.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
⏰ 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). (20)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
- GitHub Check: Key Wallet Components Tests
- GitHub Check: SPV Components Tests
🔇 Additional comments (6)
dash-spv/src/sync/filters.rs (6)
984-987: Good: explicit error on batch_end below checkpoint base.Using ok_or_else(...) here avoids silent index 0; consistent with safer semantics elsewhere.
1434-1444: Good: stop-hash resolution for filter batches now errors instead of defaulting.This prevents misaddressed GetCFilters requests under checkpoints.
1103-1109: Nice diagnostics.Logging both header and filter storage indices clarifies checkpoint math during debugging.
2050-2063: Contract comment helps.Noting that get_header_height_by_hash returns absolute height avoids future unit mistakes.
2823-2859: Stability check logic is clean.Debouncing tip movement before declaring completion is sound.
1-1: Repo-wide sanity check for lingering unsafe fallbacks — verification inconclusiveThe supplied script produced no output; I cannot confirm absence of
header_abs_to_storage_index(...).unwrap_or_default/unwrap_or(0)or match arms mappingNone => 0. Re-run the check and paste the full command output so I can verify.
- Replaced `unwrap_or_default()` with `ok_or_else()` to provide more informative error messages when blockchain heights are at or below the checkpoint base. - Enhanced validation for various height calculations to ensure consistency and clarity in error reporting.
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 (6)
dash-spv/src/sync/filters.rs (6)
518-566: The “scan backwards to nearest available header” pattern is duplicated. Extract a helper.Avoid triplicated loops; a single find_available_header_at_or_before(abs_height, min_abs_height, storage) -> Option<(BlockHash,u32)> would simplify call sites and reduce future bugs.
Also applies to: 737-765, 1017-1076
2091-2096: Bug: mixing storage index with absolute height when resolving block height.get_tip_height() returns a storage index (relative to the checkpoint base). You use it as an absolute end_height. Convert first to absolute height to keep find_height_for_block_hash bounds correct.
Apply this diff:
- let header_tip_height = storage - .get_tip_height() - .await - .map_err(|e| SyncError::Storage(format!("Failed to get header tip height: {}", e)))? - .unwrap_or(0); + let storage_tip_index = storage + .get_tip_height() + .await + .map_err(|e| SyncError::Storage(format!("Failed to get header tip height: {}", e)))? + .unwrap_or(0); + let header_tip_height = self.header_storage_to_abs_height(storage_tip_index);
2141-2146: Bug: same unit-mismatch in download_and_check_filter.Convert storage tip index to absolute height before passing it as range end.
Apply this diff:
- let header_tip_height = storage - .get_tip_height() - .await - .map_err(|e| SyncError::Storage(format!("Failed to get header tip height: {}", e)))? - .unwrap_or(0); + let storage_tip_index = storage + .get_tip_height() + .await + .map_err(|e| SyncError::Storage(format!("Failed to get header tip height: {}", e)))? + .unwrap_or(0); + let header_tip_height = self.header_storage_to_abs_height(storage_tip_index);
3195-3206: Bug: gap check compares different units (header storage index vs absolute filter height).block_height is a storage index; filter_height is an absolute height. gap_size is therefore wrong and may trigger/skip restarts incorrectly.
Apply this diff:
- let block_height = storage - .get_tip_height() - .await - .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? - .unwrap_or(0); + let storage_tip_index = storage + .get_tip_height() + .await + .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? + .unwrap_or(0); + let block_height = self.header_storage_to_abs_height(storage_tip_index); @@ - let gap_size = block_height.saturating_sub(filter_height); + let gap_size = block_height.saturating_sub(filter_height);Note: Only block_height needed fixing; the gap_size line stays the same but now operates on consistent units.
Also applies to: 3207-3211
1759-1768: Flow-control bookkeeping is O(ranges × size). Consider tracking per-range counts.is_request_complete scans every height in every active range. Keep a remaining_count per active range and decrement on receipt to cut work on large syncs.
Also applies to: 1819-1830, 2531-2547
1711-1713: Use tokio::sync::Mutex in async paths instead of std::sync::Mutex.These locks are taken on async tasks; std::sync::Mutex can block the runtime. Swap to tokio::sync::Mutex for received_filter_heights and processing_thread_requests.
Also applies to: 1748-1757, 3079-3084, 3154-3162
♻️ Duplicate comments (1)
dash-spv/src/sync/filters.rs (1)
501-509: Nice: removed “None ⇒ 0” fallbacks; now fail fast with explicit errors.These ok_or_else(...) checks prevent misaddressed requests and bad retries when below the checkpoint base.
Also applies to: 611-617, 628-635, 724-728, 992-999, 1023-1027, 1065-1073, 1442-1452, 1578-1583, 1910-1915, 3353-3360, 3395-3402
🧹 Nitpick comments (4)
dash-spv/src/sync/filters.rs (4)
294-362: Consider trimming/structuring repeated debug logs.Four nearly identical tracing::debug! blocks differ only in optional hashes. Factor into a small helper to cut duplication and reduce log churn.
1886-1892: Minor: drop unused network parameter and dyn trait in handle_request_timeout.The method never uses network. Remove it and the dyn indirection to tighten the API.
Apply this diff (signature and call site):
- async fn handle_request_timeout( - &mut self, - range: (u32, u32), - _network: &mut dyn NetworkManager, - storage: &S, - ) -> SyncResult<()> { + async fn handle_request_timeout( + &mut self, + range: (u32, u32), + storage: &S, + ) -> SyncResult<()> {And:
- self.handle_request_timeout(range, network, storage).await?; + self.handle_request_timeout(range, storage).await?;Also applies to: 1979-2015
2886-2906: Lock ordering consistency for stats and inner heights.record_filter_received_at_height drops the RwLock before taking the inner Mutex (good). start_filter_sync_tracking locks both at once. Align to the same order (drop stats_lock before locking heights) to avoid future deadlocks if other code inverts the order.
Also applies to: 2937-2953
106-109: Edge-case note: calculate_batch_start_height when hashes.len() == 0.Upstream callers guard against empty batches, but the function itself will compute 0 via saturating_sub(len-1). Consider a debug_assert! or a brief doc comment to encode the precondition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/sync/filters.rs(28 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/sync/filters.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/sync/filters.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/sync/filters.rs
🧠 Learnings (1)
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/sync/filters.rs
⏰ 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). (20)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Key Wallet Components Tests
- GitHub Check: Core Components Tests
🔇 Additional comments (10)
dash-spv/src/sync/filters.rs (10)
22-23: Batch size 1999 for CFHeaders is correct.Staying under the 2000 limit avoids off-by-one overlaps. Good choice.
116-175: Good: header storage-index → absolute-height translation and diagnostics.Using header_storage_to_abs_height avoids unit mixups, and the richer debug logs will help triage peers sending odd ranges.
220-229: Solid base-inclusive mapping (abs ⇄ storage).This Option-returning conversion is the right place to fence heights at/under the checkpoint base.
230-238: LGTM: storage-index → absolute-height helper.Keeps all callers consistent on units.
240-249: LGTM: filter abs → storage mapping mirrors header mapping.Consistent semantics reduce footguns across the module.
366-372: Benign checkpoint-overlap handling is sound.The explicit allowance for base/base+1 avoids false alarms during startup around the checkpoint.
709-717: Good: normalize storage tip index to absolute height before comparisons.Prevents mixing units when determining batch and tip logic.
Also applies to: 896-905, 2027-2033
1110-1118: Nice request validation and contextual logging.Rejecting start_height == 0 and logging both header/filter storage indices makes triage easier.
3510-3554: Merging/splitting ranges respects the 1000-filter cap.Good guardrails; this will prevent invalid GetCFilters.
1-3570: Run formatting, clippy and unit-normalization audit locally — sandbox run failedSandbox execution failed with: "Could not locate working directory: no /proc/self/exe available". Re-run from the repository root (or on CI) and paste outputs:
- rustup show active-toolchain || true
- cargo fmt --all -- --check
- cargo clippy --all-targets --all-features -- -D warnings
- rg -n --type=rust -C3 '.get_tip_height().*?await' | rg -n -v 'header_storage_to_abs_height|storage_tip_index|_index' || true
If the rg output shows raw get_tip_height() usages that are treated as blockchain heights, convert them using header_storage_to_abs_height before use and fix any formatting/clippy failures.
Summary by CodeRabbit
New Features
Improvements
Tests