refactor: migrate received_filter_heights to tokio::Mutex and add find_available_header_at_or_before helper #141
Conversation
- Introduced `find_available_header_at_or_before` method to encapsulate the logic for scanning backward to find the nearest available block header in storage. - Replaced repetitive header scanning logic in multiple locations with calls to the new method, improving code clarity and maintainability. - Enhanced error handling and logging during header retrieval to provide better insights into the syncing process.
- Updated `received_filter_heights` and `processing_thread_requests` to use `tokio::sync::Mutex` instead of `std::sync::Mutex` for better async support. - Adjusted related locking mechanisms in `FilterSyncManager`, `SpvStats`, and various test files to accommodate the new async locking. - Enhanced test files to reflect the changes in synchronization primitives, ensuring compatibility with async operations.
|
Warning Rate limit exceeded@PastaPastaPasta has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughMutex types were migrated from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant FilterSyncManager as FilterSyncManager
participant Storage
participant Network
participant Heights as received_filter_heights\n(tokio::Mutex)
App->>FilterSyncManager: start_sync()
FilterSyncManager->>Storage: find_available_header_at_or_before(height)
alt header found
Storage-->>FilterSyncManager: (block_hash, height)
FilterSyncManager->>Network: request_filter_batch(start..end)
Network-->>FilterSyncManager: filters
FilterSyncManager->>+Heights: lock().await
Heights-->>-FilterSyncManager: guard
FilterSyncManager->>Heights: insert(new heights)
else none found
FilterSyncManager-->>App: return storage error / abort
end
FilterSyncManager-->>App: result
sequenceDiagram
autonumber
participant UI as StatusDisplay
participant Stats as SpvStats
participant Heights as received_filter_heights\n(tokio::Mutex)
UI->>Stats: read (filters_received)
UI-->>UI: capture filters_received
UI->>+Heights: lock().await
Heights-->>-UI: guard
UI->>UI: compute last_synced = max(heights)
UI-->>UI: render SyncProgress using captured values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 (2)
dash-spv/src/sync/filters.rs (2)
576-609: Don’t silently stop on storage gaps; return an errorOn “no available headers found,” the code sets syncing_filter_headers = false and returns Ok(false), masking a storage inconsistency. Prefer returning a Storage error (matches recovery path behavior).
- self.syncing_filter_headers = false; - return Ok(false); + self.syncing_filter_headers = false; + return Err(SyncError::Storage(format!( + "No available headers found between {} and {} while selecting next batch stop hash", + min_height, + next_batch_end_height + )));
941-962: Incorrectly rejects genesis-only stateGuarding on storage_tip_index > 0 prevents starting sync when only the genesis header exists. Allow index 0.
- // Get the stop hash (tip of headers) - 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())); - }; + // Get the stop hash (tip of headers), including genesis at index 0 + 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();
🧹 Nitpick comments (5)
dash-spv/src/types.rs (1)
590-593: Approve — default initialization is correct; serde skip prevents (de)serialization issues. Consider adding a type alias for Arc<tokio::sync::Mutex<std::collections::HashSet>> and migrating other constructors.
Occurrences to update: dash-spv/src/types.rs, dash-spv/src/sync/filters.rs, dash-spv/src/sync/sequential/mod.rs, plus tests under dash-spv/tests.dash-spv/tests/edge_case_filter_sync_test.rs (1)
56-58: Use RwLock for read-dominant access to sent_messages (optional)Tests mostly read sent_messages; RwLock reduces contention vs Mutex.
Apply:
-use tokio::sync::Mutex; +use tokio::sync::RwLock; - sent_messages: Arc<Mutex<Vec<NetworkMessage>>>, + sent_messages: Arc<RwLock<Vec<NetworkMessage>>>, @@ - self.sent_messages: Arc::new(Mutex::new(Vec::new())), + self.sent_messages: Arc::new(RwLock::new(Vec::new())), @@ - async fn get_sent_messages(&self) -> Vec<NetworkMessage> { - self.sent_messages.lock().await.clone() - } + async fn get_sent_messages(&self) -> Vec<NetworkMessage> { + self.sent_messages.read().await.clone() + } @@ - self.sent_messages.lock().await.push(message); + self.sent_messages.write().await.push(message);Also applies to: 75-77, 185-186, 289-290
dash-spv/src/sync/filters.rs (3)
2605-2609: Consider returning Option or making this asyncUsing try_lock() may undercount during contention; returning 0 can mislead UIs. Optional: make this async and lock().await.
- pub fn get_received_filter_count(&self) -> u32 { - match self.received_filter_heights.try_lock() { - Ok(heights) => heights.len() as u32, - Err(_) => 0, - } - } + pub async fn get_received_filter_count(&self) -> u32 { + self.received_filter_heights.lock().await.len() as u32 + }
2850-2852: Reduce lock hold time while clearing heights (optional)Avoid holding a stats write-lock while awaiting the heights mutex.
- // Clear the received heights tracking for a fresh start - let mut heights = stats_lock.received_filter_heights.lock().await; - heights.clear(); + // Clear the received heights tracking for a fresh start + let heights_arc = stats_lock.received_filter_heights.clone(); + drop(stats_lock); + let mut heights = heights_arc.lock().await; + heights.clear();
3035-3038: try_lock fallbacks can hide gaps/timeoutsReturning empty/false on lock contention may under-report state. Consider async variants or capturing a snapshot elsewhere.
Also applies to: 3072-3075, 3099-3102, 3447-3450
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
dash-spv-ffi/tests/unit/test_type_conversions.rs(1 hunks)dash-spv/src/client/status_display.rs(1 hunks)dash-spv/src/sync/filters.rs(20 hunks)dash-spv/src/sync/sequential/mod.rs(1 hunks)dash-spv/src/types.rs(2 hunks)dash-spv/tests/block_download_test.rs(1 hunks)dash-spv/tests/cfheader_gap_test.rs(1 hunks)dash-spv/tests/edge_case_filter_sync_test.rs(5 hunks)dash-spv/tests/filter_header_verification_test.rs(1 hunks)dash-spv/tests/simple_gap_test.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/tests/block_download_test.rsdash-spv/src/types.rsdash-spv/tests/cfheader_gap_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/sync/sequential/mod.rsdash-spv/src/client/status_display.rsdash-spv/tests/simple_gap_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/filters.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Place integration tests under the
tests/directory
Files:
dash-spv/tests/block_download_test.rsdash-spv/tests/cfheader_gap_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_gap_test.rsdash-spv/tests/edge_case_filter_sync_test.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/tests/block_download_test.rsdash-spv/src/types.rsdash-spv/tests/cfheader_gap_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/sync/sequential/mod.rsdash-spv/src/client/status_display.rsdash-spv/tests/simple_gap_test.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/filters.rs
{dash-spv,rpc-integration-test}/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide integration tests for network operations
Files:
dash-spv/tests/block_download_test.rsdash-spv/tests/cfheader_gap_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_gap_test.rsdash-spv/tests/edge_case_filter_sync_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic
Files:
dash-spv/tests/block_download_test.rsdash-spv/tests/cfheader_gap_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_gap_test.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/tests/edge_case_filter_sync_test.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/types.rsdash-spv/src/sync/sequential/mod.rsdash-spv/src/client/status_display.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_type_conversions.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_type_conversions.rs
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Prefer async using tokio where applicable
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Use async/await throughout on the Tokio runtime
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#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.
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Prefer async using tokio where applicable
Applied to files:
dash-spv/tests/block_download_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_gap_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash-spv/tests/block_download_test.rsdash-spv/tests/cfheader_gap_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_gap_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv/tests/block_download_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_gap_test.rsdash-spv-ffi/tests/unit/test_type_conversions.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/tests/cfheader_gap_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/tests/filter_header_verification_test.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#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/tests/edge_case_filter_sync_test.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/tests/edge_case_filter_sync_test.rs
🧬 Code graph analysis (4)
dash-spv/src/types.rs (3)
dash-spv/src/sync/filters.rs (1)
new(237-271)dash-spv/src/sync/sequential/mod.rs (1)
new(86-113)dash-spv/src/client/mod.rs (1)
new(174-237)
dash-spv-ffi/tests/unit/test_type_conversions.rs (2)
dash-spv/src/sync/filters.rs (1)
new(237-271)dash-spv/src/sync/sequential/mod.rs (1)
new(86-113)
dash-spv/tests/edge_case_filter_sync_test.rs (1)
dash-spv/tests/block_download_test.rs (1)
get_sent_messages(46-48)
dash-spv/src/sync/filters.rs (2)
dash-spv/src/client/mod.rs (3)
storage(2452-2454)new(174-237)start(240-404)dash-spv/src/sync/sequential/mod.rs (1)
new(86-113)
⏰ 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). (15)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: SPV Components Tests
- GitHub Check: Core Components Tests
🔇 Additional comments (11)
dash-spv/tests/block_download_test.rs (1)
11-12: Consistent switch to Tokio Mutex in tests.Imports match usage (received_heights). Nothing else to change.
dash-spv-ffi/tests/unit/test_type_conversions.rs (1)
212-214: Test updated to Tokio Mutex; safe as no locking occurs.This keeps FFI tests in sync with core types. No action required.
dash-spv/tests/simple_gap_test.rs (1)
4-5: Tokio Mutex import is consistent with FilterSyncManager::new signature.Looks good.
dash-spv/tests/cfheader_gap_test.rs (1)
9-10: Approve: Tokio mutex migration verified. All occurrences of received_filter_heights are std::sync::Arc<tokio::sync::Mutex<_>>; locking is done with .lock().await or try_lock(), and no blocking .lock() calls were found for that field. Unrelated std::sync::Mutex uses remain in FFI/tests and test-utils (expected).dash-spv/tests/filter_header_verification_test.rs (1)
35-36: Tokio mutex switch LGTMUsing Arc + tokio::sync::Mutex here aligns with the async migration across the codebase.
dash-spv/src/sync/filters.rs (5)
77-82: Tokio mutex migration LGTMprocessing_thread_requests and received_filter_heights now use Arc<tokio::sync::Mutex<_>>; consistent with async usage.
105-162: Backward header-scan helper looks correctLoop bounds, conversion via header_abs_to_storage_index, and saturating_sub are safe and avoid underflow.
1701-1711: Minor: hold heights lock briefly (already fine)This periodic log acquires the lock; scope is tight. Keep as-is.
2429-2447: Tokio lock on processing_thread_requests is correctAwaited lock with remove() is the right pattern here.
2616-2621: Unify API: use tokio::sync::Mutex in spawn_filter_processor signatureReplace the Arc-wrapped std::sync::Mutex with tokio::sync::Mutex to match the module's async mutex usage.
Location: dash-spv/src/sync/filters.rs:2615-2621
- pub fn spawn_filter_processor( - _network_message_sender: mpsc::Sender<NetworkMessage>, - _processing_thread_requests: std::sync::Arc< - std::sync::Mutex<std::collections::HashSet<BlockHash>>, - >, + pub fn spawn_filter_processor( + _network_message_sender: mpsc::Sender<NetworkMessage>, + _processing_thread_requests: std::sync::Arc< + tokio::sync::Mutex<std::collections::HashSet<BlockHash>>, + >,rg found only the function definition in this repo (no call sites). Verify and update any external callers, workspace crates, tests, or CI that construct/pass Arc<std::sync::Mutex<...>> before merging.
dash-spv/src/sync/sequential/mod.rs (1)
88-90: Approve — switch received_filter_heights to tokio::sync::Mutex; verify no remaining std::sync::Mutex<HashSet> usagesChange aligns with async locking; I couldn't confirm absence of Arc<std::sync::Mutex<HashSet>> from the prior run — re-run these checks locally:
#!/bin/bash # run from repo root rg -nP --type=rust 'Arc<\s*std::sync::Mutex\s*<\s*(?:std::collections::)?HashSet<u32>\s*>\s*>' -C2 || true rg -n --type=rust 'std::sync::Mutex' -C2 || true rg -n --type=rust 'Arc<\s*tokio::sync::Mutex' -C2 || true
- Improved the sync progress method by cloning the inner heights handle and copying necessary counters without holding the RwLock, enhancing performance. - Updated the calculation of the last synced filter height to avoid holding the RwLock guard, ensuring better concurrency and responsiveness in the status display.
- Created a new type alias `SharedFilterHeights` for the mutex-protected set of filter heights, enhancing code clarity and reducing redundancy. - Updated references in `SpvStats`, `FilterSyncManager`, and `SequentialSyncManager` to use the new type alias, streamlining the codebase.
PastaPastaPasta
changed the title
- Updated the error handling in the filter header synchronization process to return a more informative `SyncError` when no available headers are found between specified heights. - This change enhances the clarity of error reporting, aiding in debugging and improving the overall robustness of the synchronization logic.
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 (3)
dash-spv/src/sync/filters.rs (3)
595-606: Don’t swallow a storage inconsistency; return an error instead of Ok(false)Stopping sync quietly on “no available headers” hides corruption and prevents outer recovery. Propagate a Storage error.
Apply this diff:
- self.syncing_filter_headers = false; - return Ok(false); + self.syncing_filter_headers = false; + return Err(SyncError::Storage(format!( + "No available headers found between {} and {} - storage appears to have gaps", + min_height, + next_batch_end_height + )));
3137-3153: Fix absolute vs storage index mix‑up in CFHeader gap checkYou’re comparing a block header storage index to an absolute filter height, which misreports gaps when sync_base_height > 0.
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 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);
2613-2617: Unify to tokio::sync::Mutex in spawn_filter_processor signatureField processing_thread_requests is Arc<tokio::sync::Mutex<_>>, but this API still takes std::sync::Mutex. This prevents callers from wiring the same handle and is inconsistent with the rest of the refactor.
Apply this diff:
- pub fn spawn_filter_processor( + pub fn spawn_filter_processor( _network_message_sender: mpsc::Sender<NetworkMessage>, - _processing_thread_requests: std::sync::Arc< - std::sync::Mutex<std::collections::HashSet<BlockHash>>, - >, + _processing_thread_requests: std::sync::Arc< + tokio::sync::Mutex<std::collections::HashSet<BlockHash>>, + >, stats: std::sync::Arc<tokio::sync::RwLock<crate::types::SpvStats>>, ) -> FilterNotificationSender {Also update any call sites accordingly.
🧹 Nitpick comments (2)
dash-spv/src/sync/filters.rs (2)
1834-1845: Remove unused network param from handle_request_timeout (or use it)The mutable network argument (as &mut dyn NetworkManager) isn’t used; you queue retries and send later. Drop the param to simplify the API and avoid trait‑object mismatch with generic N.
Apply this diff and fix the single call site:
- async fn handle_request_timeout( + async fn handle_request_timeout( &mut self, range: (u32, u32), - _network: &mut dyn NetworkManager, storage: &S, ) -> SyncResult<()> {And in check_filter_request_timeouts:
- self.handle_request_timeout(range, network, storage).await?; + self.handle_request_timeout(range, storage).await?;
2602-2605: Best‑effort count via try_lock may under‑report; consider documented semantics or fallbackReturning 0 on contention can confuse UIs. Either document “best‑effort (may be 0 under contention)” or block briefly (lock with a short timeout) to avoid misleading zeros.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dash-spv/src/sync/filters.rs(21 hunks)dash-spv/src/sync/sequential/mod.rs(2 hunks)dash-spv/src/types.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/src/types.rs
- dash-spv/src/sync/sequential/mod.rs
🧰 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 (2)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Prefer async using tokio where applicable
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Use async/await throughout on the Tokio runtime
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#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.
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#134
File: dash-spv/src/network/discovery.rs:14-14
Timestamp: 2025-09-03T17:45:58.984Z
Learning: In hickory_resolver 0.25.0+, TokioAsyncResolver is deprecated in favor of TokioResolver. When migrating from trust_dns_resolver to hickory_resolver, the correct type to use is TokioResolver, not TokioAsyncResolver.
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/src/sync/filters.rs
🧬 Code graph analysis (1)
dash-spv/src/sync/filters.rs (2)
dash-spv/src/client/mod.rs (3)
storage(2452-2454)new(174-237)start(240-404)dash-spv/src/sync/sequential/mod.rs (1)
new(86-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: SPV Components Tests
🔇 Additional comments (5)
dash-spv/src/sync/filters.rs (5)
105-162: Helper looks solid; inclusive bounds and checkpoint-aware index mapping are correctGood backward scan with saturating_sub and base‑aware index conversion. Nice diagnostics too.
746-778: Recovery path uses the new scan helper correctlyGood: bounded backward scan and consistent erroring when nothing is found.
1002-1026: Initial request fallback is correct; prefer inclusive scan (you did) and explicit errors (you did)This mirrors the recovery logic well. 👍
2847-2849: Lock ordering is OK; keep it consistentYou hold stats.write() then heights.lock().await. Elsewhere (record_filter_received_at_height) you read stats then lock heights. Good consistent ordering; please keep this invariant to avoid deadlocks.
2426-2443: Async mutex usage is correct — ensure producers use tokio::Mutex and share the same ArcConsumer awaits a tokio::Mutex; confirm spawn_filter_processor (producer) also uses Arc<tokio::Mutex<...>> and is passed the same Arc so requests inserted by the processor are visible here.
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/src/sync/filters.rs (1)
3141-3154: Fix unit mismatch in CFHeader gap check (storage index vs absolute height).get_tip_height() returns a header storage index, but you subtract it directly from a filter header height (absolute). This can suppress real gaps and block auto-restart.
Apply this diff:
- let block_height = storage + let block_tip_index = storage .get_tip_height() .await .map_err(|e| SyncError::Storage(format!("Failed to get block tip: {}", e)))? .unwrap_or(0); + // Convert storage index to absolute blockchain height + let block_height = self.header_storage_to_abs_height(block_tip_index); - let filter_height = storage + let filter_height = storage .get_filter_tip_height()Also applies to: 3153-3166
🧹 Nitpick comments (2)
dash-spv/src/sync/filters.rs (2)
1702-1712: Use try_lock for periodic metrics to avoid extra await on hot path.This logging block shouldn’t block if the mutex is held.
Apply this diff:
- { - let guard = self.received_filter_heights.lock().await; - if guard.len() % 1000 == 0 { - tracing::info!( - "Filter sync state: {} filters received, {} active requests, {} pending requests", - guard.len(), - self.active_filter_requests.len(), - self.pending_filter_requests.len() - ); - } - } + if let Ok(guard) = self.received_filter_heights.try_lock() { + if guard.len() % 1000 == 0 { + tracing::info!( + "Filter sync state: {} filters received, {} active requests, {} pending requests", + guard.len(), + self.active_filter_requests.len(), + self.pending_filter_requests.len() + ); + } + }
1826-1830: Remove unused NetworkManager param from timeout handler.Not used; passing a trait object is unnecessary. Simplify signature and call site.
Apply this diff:
- for range in timed_out_requests { - self.handle_request_timeout(range, network, storage).await?; - } + for range in timed_out_requests { + self.handle_request_timeout(range, storage).await?; + }- 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<()> {Also applies to: 1838-1843
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/sync/filters.rs(21 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 (2)
📓 Common learnings
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#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.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Prefer async using tokio where applicable
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Use async/await throughout on the Tokio runtime
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/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). (13)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha256)
- 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 (dash_deserialize_amount)
🔇 Additional comments (5)
dash-spv/src/sync/filters.rs (5)
105-162: Header back-scan helper looks correct and robust.Good use of saturating arithmetic, inclusive bounds, and informative tracing.
573-613: Nice fallback to nearest available header via the helper.The recoveries for missing stop headers and timeout paths are clean and avoid oversize requests.
Also applies to: 751-758, 1006-1015
1730-1737: Async lock usage for received_filter_heights is sound.Bounds-checked loop and single insert path look good.
Also applies to: 1750-1756
2430-2448: Tokio mutex adoption for processing_thread_requests is applied correctly here.The fast-path removal with .remove() is appropriate.
2606-2609: Returning 0 on try_lock failure: confirm UI semantics.If the mutex is contended, reported count drops to 0. Ensure dashboards/UX won’t interpret this as reset.
- Changed the locking mechanism for `processing_thread_requests` from `std::sync::Mutex` to `tokio::sync::Mutex` to enhance async compatibility. - Updated the locking logic in the `spawn_filter_processor` and related methods to use async await syntax, improving performance and responsiveness in the filter processing workflow.
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 (3)
dash-spv/src/sync/filters.rs (3)
2042-2050: Bug: using storage index where absolute height is required (breaks lookups with non‑zero base).find_height_for_block_hash expects absolute heights, but you pass the storage tip index. Convert to absolute first.
Apply this diff in both helpers:
- 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 header_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(header_tip_index);Also applies to: 2091-2099
1032-1035: Use Storage errors for storage failures (not Validation).Incorrect error kind harms observability and policy handling. These branches are all storage I/O failures.
- return Err(SyncError::Validation(format!( + return Err(SyncError::Storage(format!( "Failed to get initial batch stop header at height {}: {}", batch_end_height, e )));- return Err(SyncError::Validation(format!( + return Err(SyncError::Storage(format!( "Failed to get tip header: {}", e )));- return Err(SyncError::Validation(format!( + return Err(SyncError::Storage(format!( "Failed to get tip header during recovery: {}", e )));Also applies to: 672-677, 828-832
553-620: Deduplicate stop‑hash resolution to avoid divergence.The backward-scan fallback is implemented three times with slight variations. Centralize into a small helper (e.g., resolve_stop_hash(end_abs_height, min_abs_height, storage)) to reduce bugs like the error-kind inconsistency fixed above.
I can draft the helper and wire it into these call sites if you want.
Also applies to: 734-791, 1006-1030
♻️ Duplicate comments (2)
dash-spv/src/sync/filters.rs (2)
77-82: Consistency achieved: tokio::Mutex everywhere for processing_thread_requests.This resolves prior type friction between struct and API. LGTM.
Also applies to: 2616-2621
2845-2847: Don’t await a tokio::Mutex while holding a tokio::RwLock write guard (deadlock/latency risk).Extract the Arc, drop the write guard, then await the mutex.
- // Clear the received heights tracking for a fresh start - let mut heights = stats_lock.received_filter_heights.lock().await; - heights.clear(); + // Clear received heights without awaiting while holding the write guard + let received = stats_lock.received_filter_heights.clone(); + drop(stats_lock); + let mut heights = received.lock().await; + heights.clear();
🧹 Nitpick comments (4)
dash-spv/src/sync/filters.rs (4)
105-162: Header scan helper looks good; consider log throttling.Scanning on sparse storage can emit many warn/debug lines. Add a simple throttle or downgrade repeated missing-header logs to trace.
1702-1712: Tiny logging nit: avoid logging at zero and hold the lock briefly.Guard against the initial 0 case to prevent a noisy log on first insert.
- { - let guard = self.received_filter_heights.lock().await; - if guard.len() % 1000 == 0 { + { + let guard = self.received_filter_heights.lock().await; + let len = guard.len(); + drop(guard); + if len != 0 && len % 1000 == 0 { tracing::info!( "Filter sync state: {} filters received, {} active requests, {} pending requests", - guard.len(), + len, self.active_filter_requests.len(), self.pending_filter_requests.len() ); } }
1730-1737: Reduce lock contention when checking request completion.mark_filter_received calls is_request_complete per range; each call reacquires the mutex. Pass a snapshot of heights into the check or prefetch once and reuse for all ranges.
Also applies to: 1689-1693
2606-2609: try_lock() may underreport received count. Verify call sites tolerate 0 when contended.If this feeds user‑visible progress, consider a non‑blocking cached gauge or an async getter that awaits the lock.
Would you like an async get_received_filter_count_async() that awaits the lock and is used only in non‑hot paths?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv/src/sync/filters.rs(25 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 (3)
📓 Common learnings
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#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.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Use async/await throughout on the Tokio runtime
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Prefer async using tokio where applicable
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#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/filters.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/src/sync/filters.rs
🧬 Code graph analysis (1)
dash-spv/src/sync/filters.rs (2)
dash-spv/src/client/mod.rs (2)
storage(2452-2454)new(174-237)dash-spv/src/sync/sequential/mod.rs (1)
new(86-113)
⏰ 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). (17)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_amount)
- 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: Strict Warnings and Clippy Checks
- GitHub Check: Key Wallet Components Tests
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- Updated the locking logic in the filter synchronization process to clone the `received_filter_heights` before awaiting the mutex, ensuring the `RwLock` is released promptly. - This change enhances concurrency and responsiveness during filter sync operations, aligning with recent async improvements in the codebase.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests