avoid re-downloading filters during SyncPhase::DownloadingFilters #240
Conversation
WalkthroughAdds an async StorageManager method Changes
Sequence DiagramsequenceDiagram
participant DF as DownloadingFilters
participant SM as StorageManager
participant FH as FilterHeaders
rect rgb(248,249,255)
Note over DF,FH: Initialize resume logic
end
DF->>DF: Read filter_header_tip
alt no headers (filter_header_tip == 0)
DF->>DF: Transition to next phase (no headers)
else headers present
DF->>SM: get_stored_filter_height()
SM-->>DF: Option<u32> stored_height
alt stored_height exists
DF->>DF: start_height = stored_height + 1
else none
DF->>DF: start_height = max(1, sync_base)
end
DF->>DF: Calculate count = filter_header_tip - start_height + 1
alt start_height > filter_header_tip
DF->>DF: Transition to next phase (complete)
else
DF->>FH: Download filters [start_height .. start_height+count-1]
FH-->>DF: Filters received
DF->>DF: Update phase progress/total_filters
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dash-spv/src/sync/phase_execution.rs (1)
152-157: Guard clause for missing filter headers is appropriate.The early return when
filter_header_tip <= 0prevents unnecessary processing. Note that sincefilter_header_tipisu32, this is equivalent to== 0; consider using== 0for clarity, though functionally identical.- if filter_header_tip <= 0 { + if filter_header_tip == 0 {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dash-spv/src/storage/disk/state.rs(1 hunks)dash-spv/src/storage/memory.rs(1 hunks)dash-spv/src/storage/mod.rs(1 hunks)dash-spv/src/sync/phase_execution.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/sync/phase_execution.rsdash-spv/src/storage/disk/state.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use trait-based abstractions for core components likeNetworkManagerandStorageManagerto enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles withcargo check --all-features
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/sync/phase_execution.rsdash-spv/src/storage/disk/state.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Storage module should provide abstraction via
StorageManagertrait with concrete implementations forMemoryStorageManagerandDiskStorageManager
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/storage/disk/state.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Synchronization should use
SyncManagerfor phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Files:
dash-spv/src/sync/phase_execution.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Storage should use segmented storage with headers stored in 10,000-header segments with index files, separate storage for filter headers and compact block filters
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/storage/disk/state.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/storage/disk/state.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rsdash-spv/src/storage/disk/state.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/memory.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Applied to files:
dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/sync/phase_execution.rs
🧬 Code graph analysis (4)
dash-spv/src/storage/mod.rs (2)
dash-spv/src/storage/disk/state.rs (1)
get_stored_filter_height(556-558)dash-spv/src/storage/memory.rs (1)
get_stored_filter_height(261-268)
dash-spv/src/storage/memory.rs (2)
dash-spv/src/storage/disk/state.rs (1)
get_stored_filter_height(556-558)dash-spv/src/storage/mod.rs (1)
get_stored_filter_height(132-132)
dash-spv/src/sync/phase_execution.rs (2)
dash-spv/src/client/core.rs (2)
storage(171-173)network(166-168)dash-spv/src/sync/headers/manager.rs (2)
storage(346-346)get_sync_base_height(1035-1037)
dash-spv/src/storage/disk/state.rs (2)
dash-spv/src/storage/memory.rs (1)
get_stored_filter_height(261-268)dash-spv/src/storage/mod.rs (1)
get_stored_filter_height(132-132)
🔇 Additional comments (6)
dash-spv/src/storage/mod.rs (1)
130-133: Well-designed trait method addition.The new
get_stored_filter_heightmethod follows the existing trait patterns with appropriate documentation that clearly distinguishes it fromget_filter_tip_height. The signature using&selfcorrectly reflects the read-only nature of this operation.dash-spv/src/storage/memory.rs (1)
261-268: LGTM!The implementation correctly returns the maximum key from the filters HashMap, which stores absolute blockchain heights. The idiomatic use of
.keys().max().copied()is clean and appropriate for in-memory storage.dash-spv/src/sync/phase_execution.rs (3)
165-189: Resume logic correctly calculates start height from stored filters.The implementation properly:
- Reads the last stored filter height from storage
- Resumes from
stored_height + 1if filters exist- Falls back to
sync_base_height.max(1)for fresh startsThis achieves the PR objective of avoiding re-downloads.
191-201: Correctly handles already-synced case.When
start_height > filter_header_tip, all filters are already downloaded. The transition to the next phase with appropriate logging prevents redundant work.
203-229: Filter download initiation with proper count calculation.The count calculation
filter_header_tip - start_height + 1correctly computes the inclusive range. The phase'stotal_filtersis updated to reflect the actual remaining count rather than the full range, which is appropriate for progress tracking during resumed syncs.dash-spv/src/storage/disk/state.rs (1)
556-558: Implement missingget_stored_filter_heightmethod in DiskStorageManager.The trait impl delegates to
Self::get_stored_filter_height(self).await, but this inherent method does not exist in DiskStorageManager. The MemoryStorageManager has a working implementation; DiskStorageManager must provide the corresponding disk-based implementation that scans the filter storage directory and returns the height of the highest stored filter.⛔ Skipped due to learnings
Learnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: dash-spv/CLAUDE.md:0-0 Timestamp: 2025-12-04T15:51:59.632Z Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`Learnt from: DCG-Claude Repo: dashpay/rust-dashcore PR: 0 File: :0-0 Timestamp: 2025-06-26T15:54:02.509Z Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.Learnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: dash-spv/CLAUDE.md:0-0 Timestamp: 2025-12-04T15:51:59.632Z Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations
ZocoLini
force-pushed
the
fix/filter-sync-from-last-stored
branch
from
c5ae92c to
b6cdf1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dash-spv/src/storage/disk/filters.rs (1)
208-210: Consider async metadata check for better async hygiene.The synchronous
path.is_file()call can block the executor thread. While metadata checks are typically fast, it's better practice in async contexts to use async file operations.Apply this diff to use async metadata checking:
let path = entry.path(); - // Skip if not a file - if !path.is_file() { - continue; - } + // Skip if not a file (use async metadata check) + let metadata = match tokio::fs::metadata(&path).await { + Ok(m) => m, + Err(_) => continue, + }; + if !metadata.is_file() { + continue; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dash-spv/src/storage/disk/filters.rs(1 hunks)dash-spv/src/storage/disk/state.rs(1 hunks)dash-spv/src/storage/memory.rs(1 hunks)dash-spv/src/storage/mod.rs(1 hunks)dash-spv/src/sync/phase_execution.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- dash-spv/src/storage/memory.rs
- dash-spv/src/storage/mod.rs
- dash-spv/src/storage/disk/state.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/sync/phase_execution.rsdash-spv/src/storage/disk/filters.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use trait-based abstractions for core components likeNetworkManagerandStorageManagerto enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles withcargo check --all-features
Files:
dash-spv/src/sync/phase_execution.rsdash-spv/src/storage/disk/filters.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Synchronization should use
SyncManagerfor phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Files:
dash-spv/src/sync/phase_execution.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Storage module should provide abstraction via
StorageManagertrait with concrete implementations forMemoryStorageManagerandDiskStorageManager
Files:
dash-spv/src/storage/disk/filters.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Storage should use segmented storage with headers stored in 10,000-header segments with index files, separate storage for filter headers and compact block filters
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Applied to files:
dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`
Applied to files:
dash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/src/storage/disk/filters.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations
Applied to files:
dash-spv/src/storage/disk/filters.rs
🧬 Code graph analysis (2)
dash-spv/src/sync/phase_execution.rs (2)
dash-spv/src/client/core.rs (2)
storage(171-173)network(166-168)dash-spv/src/sync/headers/manager.rs (2)
storage(346-346)get_sync_base_height(1035-1037)
dash-spv/src/storage/disk/filters.rs (3)
dash-spv/src/storage/disk/state.rs (1)
get_stored_filter_height(556-558)dash-spv/src/storage/memory.rs (1)
get_stored_filter_height(261-268)dash-spv/src/storage/mod.rs (1)
get_stored_filter_height(132-132)
🔇 Additional comments (4)
dash-spv/src/storage/disk/filters.rs (1)
185-246: LGTM! Resume logic correctly scans for stored filters.The implementation correctly:
- Returns
Nonewhen no filters directory exists- Filters out segment files by checking for purely numeric filenames
- Handles edge cases (non-files, non-.dat, parse errors)
- Includes a reasonable sanity check for suspiciously high heights
The defensive error handling on line 227 (despite the comment noting it can't fail) is good practice.
dash-spv/src/sync/phase_execution.rs (3)
152-157: LGTM! Good defensive check for missing filter headers.The early return when
filter_header_tip == 0prevents attempting to download filters when no filter headers are available, avoiding unnecessary work and potential errors.
165-174: LGTM! Proper storage integration with error handling.The call to
get_stored_filter_heightis correctly wrapped with error handling and includes helpful logging for debugging the resume logic.
176-230: LGTM! Resume logic correctly avoids re-downloading filters.The implementation correctly:
- Resumes from
stored_height + 1when filters exist, avoiding re-downloads- Falls back to
sync_base_height(minimum 1) when no filters are stored- Handles the edge case where all filters are already downloaded by transitioning to the next phase
- Calculates the inclusive range correctly:
count = filter_header_tip - start_height + 1- Updates the phase's
total_filtersto reflect the actual work needed- Provides comprehensive logging at each decision point
The use of
max(1)on line 186 correctly ensures the start height is never below the genesis block.
|
Waiting #244 |
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Fix extracted from #189.
This fix aims to avoid re-downloading filters by starting from the last stored
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.