Remove headers from chain state #292
Conversation
📝 WalkthroughWalkthroughRemoves in-memory per-height headers and related ChainState APIs; shifts header/tip access to storage-first async calls, changes StorageManager get_tip_height signature and adds start/stored-length accessors; updates sync, client, chainlock, FFI, segment cache, and tests to use storage lookups and simplified error/default handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / SyncManager
participant HdrSync as HeaderSyncManager
participant Storage as StorageLayer
participant Net as Network/Peer
participant ChainLock as ChainLockMgr
rect rgb(240,248,255)
note left of Client: Storage-first async header/tip access
Client->>Storage: get_tip_height() -> Option<u32>
Storage-->>Client: Option<u32>
alt tip exists
Client->>Storage: get_header(height) -> Option<Header>
Storage-->>Client: Option<Header>
else tip missing
Storage-->>Client: None
end
Client->>HdrSync: prepare/start sync (pass Storage)
HdrSync->>Net: request_headers(base_hash)
Net-->>HdrSync: headers message
HdrSync->>Storage: store_headers(...)
Storage-->>HdrSync: ack
HdrSync->>Client: update_chain_state_cache(sync_base_height)
end
rect rgb(255,250,240)
note right of ChainLock: ChainLock validation via storage
ChainLock->>Storage: get_header(block_height)
Storage-->>ChainLock: Option<Header> / Err -> StorageError
alt header found
ChainLock->>ChainLock: compare block_hash, proceed with MN checks
else missing
ChainLock-->>ChainLock: warn about future block, continue/abort per logic
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
ZocoLini
force-pushed
the
remove-headers-from-chain-state
branch
2 times, most recently
from
fb75d92 to
12c9304
Compare
ZocoLini
force-pushed
the
remove-headers-from-chain-state
branch
from
12c9304 to
bc544a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/storage/segments.rs (1)
230-233: Consider clearingstart_heightinclear_in_memory.The
clear_in_memorymethod clearstip_heightbut doesn't clear the newly addedstart_heightfield. This could leave stale data if the cache is cleared and reused.🔎 Proposed fix
pub fn clear_in_memory(&mut self) { self.segments.clear(); self.tip_height = None; + self.start_height = None; }
🧹 Nitpick comments (6)
dash-spv/src/storage/mod.rs (2)
84-88: Approve signature changes; consider adding documentation for new methods.The change to
get_tip_height()returningOption<u32>directly aligns with the PR objective that this operation cannot fail once storage is built. The newget_start_height()andget_stored_headers_len()methods are useful additions for the storage-first architecture.Consider adding doc comments for the new methods to match the documentation style of other trait methods (e.g.,
/// Get the starting height of stored headers.).🔎 Suggested documentation
/// Get the current tip blockchain height. async fn get_tip_height(&self) -> Option<u32>; + /// Get the starting height of stored headers. async fn get_start_height(&self) -> Option<u32>; + /// Get the count of stored headers. async fn get_stored_headers_len(&self) -> u32;
100-100: Consider consistency withget_tip_heightsignature.
get_filter_tip_heightstill returnsStorageResult<Option<u32>>whileget_tip_heightwas changed toOption<u32>. If filter tip height access has similar in-memory characteristics after initialization, consider aligning the signatures for consistency. Otherwise, a brief doc comment explaining why this method can fail would be helpful.dash-spv/tests/storage_consistency_test.rs (1)
409-409: Minor inconsistency:.unwrap()differs from pattern used elsewhere.This line uses
.unwrap()while other parts of this test file useif let Some(...). Since headers were stored before this line, the unwrap is safe, but it's slightly inconsistent with the defensive pattern used throughout the rest of this test file.🔎 Optional: Align with the file's defensive pattern
- let tip_height = storage.get_tip_height().await.unwrap(); + let tip_height = storage.get_tip_height().await.expect("tip should exist after storing headers");Or use the
if let Some(...)pattern consistent with other tests in this file.dash-spv/tests/header_sync_test.rs (1)
341-341: Consider usingexpect()with a descriptive message.While
.unwrap()is acceptable in tests, using.expect("Headers were stored, tip should exist")would provide clearer diagnostics if this ever fails.🔎 Suggested improvement
- let tip_height = storage.get_tip_height().await.unwrap(); + let tip_height = storage.get_tip_height().await.expect("Headers were stored, tip should exist");dash-spv/src/sync/headers/manager.rs (2)
98-112: Consider propagating storage errors.Similar to the issue in
manager.rs, this method changed from returningSyncResult<u32>to returning(). Ifstorage.load_chain_state()fails (theErrcase), the error is silently ignored and the method continues with the existing (potentially stale) chain state.This could lead to sync proceeding with incorrect state if storage is corrupted or unavailable.
Consider at minimum adding a warning log when storage loading fails:
🔎 Proposed improvement
pub async fn load_headers_from_storage(&mut self, storage: &S) { // First, try to load the persisted chain state which may contain sync_base_height if let Ok(Some(stored_chain_state)) = storage.load_chain_state().await { tracing::info!( "Loaded chain state from storage with sync_base_height: {}", stored_chain_state.sync_base_height, ); // Update our chain state with the loaded one { self.cached_sync_base_height = stored_chain_state.sync_base_height; let mut cs = self.chain_state.write().await; *cs = stored_chain_state; } + } else { + tracing::warn!("Failed to load chain state from storage or no state found"); } }
657-665: Consider extracting repeated checkpoint header retrieval.This code segment (lines 657-665) is nearly identical to lines 497-505, retrieving the checkpoint header unconditionally. This duplication:
- Makes the code harder to maintain
- Repeats the same unconditional retrieval issue noted earlier
🔎 Proposed refactor
Consider extracting this into a helper method:
async fn get_checkpoint_header(&self, storage: &S) -> SyncResult<Option<BlockHeader>> { if !self.is_synced_from_checkpoint() { return Ok(None); } let first_height = storage .get_start_height() .await .ok_or(SyncError::Storage("Failed to get start height".to_string()))?; let checkpoint_header = storage .get_header(first_height) .await .map_err(|e| SyncError::Storage(format!("Failed to get checkpoint header: {}", e)))?; Ok(checkpoint_header) }This would centralize the logic and make the conditional behavior explicit.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
dash-spv-ffi/src/types.rs(0 hunks)dash-spv-ffi/tests/unit/test_type_conversions.rs(0 hunks)dash-spv/src/chain/chainlock_manager.rs(1 hunks)dash-spv/src/client/core.rs(1 hunks)dash-spv/src/client/lifecycle.rs(6 hunks)dash-spv/src/client/progress.rs(1 hunks)dash-spv/src/client/status_display.rs(1 hunks)dash-spv/src/client/sync_coordinator.rs(3 hunks)dash-spv/src/storage/mod.rs(1 hunks)dash-spv/src/storage/segments.rs(5 hunks)dash-spv/src/storage/state.rs(2 hunks)dash-spv/src/sync/filters/headers.rs(4 hunks)dash-spv/src/sync/filters/retry.rs(1 hunks)dash-spv/src/sync/headers/manager.rs(20 hunks)dash-spv/src/sync/manager.rs(5 hunks)dash-spv/src/sync/masternodes/manager.rs(7 hunks)dash-spv/src/sync/message_handlers.rs(1 hunks)dash-spv/src/sync/phase_execution.rs(1 hunks)dash-spv/src/sync/transitions.rs(3 hunks)dash-spv/src/types.rs(1 hunks)dash-spv/tests/edge_case_filter_sync_test.rs(1 hunks)dash-spv/tests/filter_header_verification_test.rs(2 hunks)dash-spv/tests/header_sync_test.rs(10 hunks)dash-spv/tests/integration_real_node_test.rs(1 hunks)dash-spv/tests/rollback_test.rs(2 hunks)dash-spv/tests/segmented_storage_debug.rs(1 hunks)dash-spv/tests/segmented_storage_test.rs(6 hunks)dash-spv/tests/simple_header_test.rs(1 hunks)dash-spv/tests/simple_segmented_test.rs(2 hunks)dash-spv/tests/storage_consistency_test.rs(15 hunks)dash-spv/tests/storage_test.rs(1 hunks)
💤 Files with no reviewable changes (2)
- dash-spv-ffi/src/types.rs
- dash-spv-ffi/tests/unit/test_type_conversions.rs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/chain/chainlock_manager.rsdash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/src/client/status_display.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/tests/simple_header_test.rsdash-spv/src/sync/phase_execution.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/storage/state.rsdash-spv/src/storage/segments.rsdash-spv/src/sync/filters/headers.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/types.rsdash-spv/src/sync/headers/manager.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/src/client/status_display.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/tests/simple_header_test.rsdash-spv/src/sync/phase_execution.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/storage/state.rsdash-spv/src/storage/segments.rsdash-spv/src/sync/filters/headers.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/types.rsdash-spv/src/sync/headers/manager.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Files:
dash-spv/src/sync/transitions.rsdash-spv/src/sync/message_handlers.rsdash-spv/src/sync/filters/retry.rsdash-spv/src/sync/phase_execution.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/sync/manager.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/headers/manager.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/state.rsdash-spv/src/storage/segments.rs
dash-spv/src/client/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Files:
dash-spv/src/client/core.rsdash-spv/src/client/status_display.rsdash-spv/src/client/progress.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/lifecycle.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Write integration tests for network operations in Rust
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate in RustIntegration tests use
tests/directory (e.g.,rpc-integration-test)
Files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/tests/**/*.rs: Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rs
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Prepare support for future Dash upgrades including Schnorr/Taproot support, descriptor wallets, multi-signature account types, and Lightning Network payment channels
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Enable mempool tracking on SPV connection using the appropriate strategy (.fetchAll, .bloomFilter, .selective)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.swift : Use actors for state management in SPVClient to ensure thread safety
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
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-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/src/sync/phase_execution.rsdash-spv/src/storage/state.rsdash-spv/src/sync/filters/headers.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/header_sync_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/types.rsdash-spv/src/sync/headers/manager.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/chain/chainlock_manager.rsdash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/src/client/status_display.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/rollback_test.rsdash-spv/tests/storage_test.rsdash-spv/src/client/lifecycle.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv/src/chain/chainlock_manager.rsdash-spv/tests/simple_segmented_test.rsdash-spv/src/client/status_display.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/types.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/chain/chainlock_manager.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/src/sync/phase_execution.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/storage/state.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/storage_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/manager.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/src/client/status_display.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/filters/headers.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/tests/integration_real_node_test.rsdash-spv/src/client/status_display.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/filters/headers.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/rollback_test.rsdash-spv/src/client/lifecycle.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/sync/transitions.rsdash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/tests/simple_segmented_test.rsdash-spv/src/client/status_display.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rsdash-spv/src/sync/phase_execution.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/storage/state.rsdash-spv/src/storage/segments.rsdash-spv/src/sync/filters/headers.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/src/sync/manager.rsdash-spv/src/client/sync_coordinator.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/types.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/src/sync/transitions.rsdash-spv/src/client/progress.rsdash-spv/src/sync/filters/retry.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/src/sync/transitions.rsdash-spv/src/sync/masternodes/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/tests/integration_real_node_test.rsdash-spv/src/client/status_display.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/tests/header_sync_test.rsdash-spv/src/types.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/client/core.rsdash-spv/src/sync/message_handlers.rsdash-spv/src/client/status_display.rsdash-spv/src/sync/manager.rsdash-spv/tests/rollback_test.rsdash-spv/src/sync/masternodes/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/src/client/core.rsdash-spv/src/client/status_display.rsdash-spv/src/client/progress.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/headers/manager.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/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/client/lifecycle.rsdash-spv/src/types.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/storage_test.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: When making changes to Rust FFI in dash-spv-ffi, rebuild with `cargo build --release` and run Swift tests to verify integration
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.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/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/tests/simple_segmented_test.rsdash-spv/tests/simple_header_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rs
📚 Learning: 2025-06-26T15:52:45.327Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:52:45.327Z
Learning: In Rust, calling `headers.last().unwrap()` or `headers[headers.len() - 1]` is only safe if the vector is guaranteed non-empty; placing such code outside a tightly-coupled empty check is fragile and risks panics if future refactoring bypasses the check. Prefer pattern matching or moving logic inside the non-empty branch for robustness.
Applied to files:
dash-spv/tests/simple_segmented_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/filters/headers.rsdash-spv/tests/rollback_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Run `./sync-headers.sh` to copy updated headers to Swift SDK after rebuilding dash-spv-ffi
Applied to files:
dash-spv/src/sync/filters/headers.rsdash-spv/tests/header_sync_test.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-12-01T08:01:00.652Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:01:00.652Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : Use DashSDK's public methods, not direct SPVClient access, when accessing SPV functionality in Swift code
Applied to files:
dash-spv/src/client/sync_coordinator.rsdash-spv/src/client/lifecycle.rsdash-spv/src/types.rs
📚 Learning: 2025-02-25T06:19:32.230Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 51
File: dash/src/sml/masternode_list_entry/hash.rs:7-12
Timestamp: 2025-02-25T06:19:32.230Z
Learning: The `consensus_encode` method on `MasternodeListEntry` writing to a `Vec` buffer cannot fail, so using `.expect()` is appropriate rather than propagating the error with the `?` operator.
Applied to files:
dash-spv/src/sync/masternodes/manager.rs
🧬 Code graph analysis (23)
dash-spv/src/chain/chainlock_manager.rs (2)
dash-spv/src/types.rs (1)
header(67-69)dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/src/storage/mod.rs (1)
dash-spv/src/storage/state.rs (3)
get_tip_height(400-402)get_start_height(404-406)get_stored_headers_len(408-423)
dash-spv/src/client/core.rs (2)
dash-spv/src/storage/segments.rs (1)
tip_height(409-411)dash-spv/src/types.rs (1)
header(67-69)
dash-spv/src/sync/message_handlers.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/integration_real_node_test.rs (3)
dash-spv/src/client/core.rs (1)
storage(168-170)dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)dash-spv/src/storage/state.rs (1)
get_tip_height(400-402)
dash-spv/tests/simple_segmented_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/src/client/status_display.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/filter_header_verification_test.rs (2)
dash-spv/src/storage/segments.rs (1)
tip_height(409-411)dash-spv/src/client/core.rs (2)
tip_height(201-203)storage(168-170)
dash-spv/src/client/progress.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/src/sync/filters/retry.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/simple_header_test.rs (3)
dash-spv/src/client/core.rs (1)
storage(168-170)dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)dash-spv/src/storage/state.rs (1)
get_tip_height(400-402)
dash-spv/src/sync/phase_execution.rs (1)
dash-spv/src/client/core.rs (2)
network(163-165)storage(168-170)
dash-spv/tests/segmented_storage_debug.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/storage_consistency_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/segmented_storage_test.rs (2)
dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)dash-spv/src/storage/state.rs (1)
get_tip_height(400-402)
dash-spv/src/sync/filters/headers.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/edge_case_filter_sync_test.rs (2)
dash-spv/src/storage/segments.rs (1)
tip_height(409-411)dash-spv/src/client/core.rs (2)
tip_height(201-203)storage(168-170)
dash-spv/src/client/sync_coordinator.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/tests/header_sync_test.rs (5)
dash-spv/src/client/core.rs (2)
storage(168-170)tip_height(201-203)dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)dash-spv/src/storage/state.rs (1)
get_tip_height(400-402)dash-spv/src/storage/segments.rs (1)
tip_height(409-411)dash-spv/src/storage/headers.rs (1)
headers(21-21)
dash-spv/tests/storage_test.rs (1)
dash-spv/src/client/core.rs (1)
storage(168-170)
dash-spv/src/client/lifecycle.rs (2)
dash-spv/src/client/core.rs (2)
storage(168-170)tip_height(201-203)dash-spv/src/storage/segments.rs (1)
tip_height(409-411)
dash-spv/src/sync/masternodes/manager.rs (2)
dash-spv/src/storage/segments.rs (1)
tip_height(409-411)dash-spv/src/client/core.rs (2)
tip_height(201-203)storage(168-170)
dash-spv/src/sync/headers/manager.rs (4)
dash-spv/src/sync/manager.rs (1)
load_headers_from_storage(141-144)dash-spv/src/storage/mod.rs (2)
get_stored_headers_len(88-88)get_tip_height(84-84)dash-spv/src/storage/state.rs (2)
get_stored_headers_len(408-423)get_tip_height(400-402)dash-spv/src/storage/segments.rs (1)
tip_height(409-411)
⏰ 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). (16)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (ubuntu-latest)
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. |
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dash-spv/src/sync/manager.rs (2)
142-144: Silently discards initialization errors.The method signature changed from returning
SyncResult<u32>to(), removing error propagation. Based on the relevant code snippet, the delegatedheader_sync.load_headers_from_storage(storage)loads chain state from storage and updates internal state, but any errors during this process are now silently ignored. While this may be acceptable for initialization (the sync will proceed from defaults if load fails), consider adding explicit error logging or documenting this behavior.Run the following script to verify error handling in the delegated method:
#!/bin/bash # Description: Check error handling in HeaderSync::load_headers_from_storage # Find the implementation ast-grep --pattern 'impl $_ HeaderSyncManager$_ { $$$ pub async fn load_headers_from_storage($$$) { $$$ } $$$ }'
383-385: Document the 0 return value semantics.The method returns 0 via
unwrap_or(0)whenstorage.get_tip_height()returnsNone. While this is consistent with the codebase and reasonable for sync initialization, the method documentation should clarify that 0 represents both "no headers synced" and "at genesis height" to prevent future confusion.📝 Suggested documentation
- /// Get the actual blockchain height from storage height, accounting for checkpoints + /// Get the actual blockchain height from storage. + /// + /// Returns 0 if no tip height is present in storage (no headers synced) + /// or when the blockchain is at genesis height. This behavior is intentional + /// for initializing sync state. pub(super) async fn get_blockchain_height_from_storage(&self, storage: &S) -> u32 { storage.get_tip_height().await.unwrap_or(0) }
🧹 Nitpick comments (1)
dash-spv/src/storage/state.rs (1)
368-384: Consider simplifying the nested range check logic.The nested
if-letblocks with early returns are correct but verbose. Consider refactoring for improved readability:🔎 Proposed refactor
async fn get_header(&self, height: u32) -> StorageResult<Option<BlockHeader>> { - if let Some(tip_height) = self.get_tip_height().await { - if height > tip_height { - return Ok(None); - } - } else { + let tip_height = match self.get_tip_height().await { + Some(h) => h, + None => return Ok(None), + }; + + let start_height = match self.get_start_height().await { + Some(h) => h, + None => return Ok(None), + }; + + if height > tip_height || height < start_height { return Ok(None); } - - if let Some(start_height) = self.get_start_height().await { - if height < start_height { - return Ok(None); - } - } else { - return Ok(None); - } - + Ok(self.block_headers.write().await.get_items(height..height + 1).await?.first().copied()) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dash-spv-ffi/src/types.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rsdash-spv/src/types.rsdash-spv/tests/segmented_storage_test.rs
💤 Files with no reviewable changes (2)
- dash-spv-ffi/src/types.rs
- dash-spv/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv-ffi/tests/unit/test_type_conversions.rs
- dash-spv/src/client/core.rs
🧰 Additional context used
📓 Path-based instructions (8)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/tests/**/*.rs: Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Files:
dash-spv/tests/segmented_storage_test.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Never hardcode network parameters, addresses, or keys
Files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types (thiserror) and propagate errors appropriately
Use tokio runtime for async operations
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality
Use secure random number generation for keys
Never log or expose private keys
Code must target Rust 1.89 minimum supported version (MSRV)
Format code using cargo fmt
Pass clippy linting without warnings
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
Files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Create integration tests for network operations
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
dash-spv/tests/segmented_storage_test.rs
**/{dash-network,dash-spv,key-wallet}/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use async/await for async operations in network and wallet modules
Files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Files:
dash-spv/src/sync/manager.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/state.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
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.
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/tests/segmented_storage_test.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/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.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/tests/segmented_storage_test.rsdash-spv/src/sync/manager.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/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/tests/segmented_storage_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/tests/segmented_storage_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/tests/segmented_storage_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-22T17:59:37.849Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T17:59:37.849Z
Learning: Applies to **/{dash-network,dash-spv,key-wallet}/**/*.rs : Use async/await for async operations in network and wallet modules
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-06-26T15:52:45.327Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:52:45.327Z
Learning: In Rust, calling `headers.last().unwrap()` or `headers[headers.len() - 1]` is only safe if the vector is guaranteed non-empty; placing such code outside a tightly-coupled empty check is fragile and risks panics if future refactoring bypasses the check. Prefer pattern matching or moving logic inside the non-empty branch for robustness.
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/sync/manager.rsdash-spv/src/storage/state.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/src/sync/manager.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/sync/manager.rs
🧬 Code graph analysis (2)
dash-spv/tests/segmented_storage_test.rs (3)
dash-spv/src/client/core.rs (1)
storage(168-170)dash-spv/src/storage/state.rs (1)
get_tip_height(387-389)dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)
dash-spv/src/sync/manager.rs (1)
dash-spv/src/sync/headers/manager.rs (1)
load_headers_from_storage(98-112)
🔇 Additional comments (3)
dash-spv/src/storage/state.rs (1)
395-410: LGTM!The
get_stored_headers_len()implementation correctly calculates the inclusive range length usingend_height - start_height + 1. The early returns for missing heights are appropriate.dash-spv/src/sync/manager.rs (2)
255-268: LGTM!The simplified implementation correctly handles the new
get_tip_height()API that returnsOption<u32>directly. The pattern matching properly handles bothNoneandSome(height)cases.
360-361: Signature simplified to match storage-first architecture.The removal of the
headers_lenparameter aligns with the PR's goal of removing headers fromChainStateand delegating to storage-based access.
ZocoLini
force-pushed
the
remove-headers-from-chain-state
branch
from
c5679fd to
29654ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dash-spv/src/sync/manager.rs (2)
142-144: Past concern remains: error handling removed.The method signature change from
SyncResult<u32>to()removes error propagation, as previously flagged. If silent failure during initialization is intentional, consider adding explicit logging within the delegatedheader_sync.load_headers_from_storage()to aid debugging.
383-384: Past concern remains: return value semantics unclear.As previously noted, the method returns
0both when storage has no tip (no headers synced) and when the blockchain is at genesis. Consider adding a doc comment to clarify this semantic choice for future maintainers.
🧹 Nitpick comments (1)
dash-spv/src/client/core.rs (1)
192-197: Consider simplifying the double-? pattern for clarity.The implementation correctly fetches the tip hash from storage, but the pattern
.ok()??on line 195 may be hard to read. Consider usingmap_error a more explicit match for clarity:- let tip_height = storage.get_tip_height().await?; - let header = storage.get_header(tip_height).await.ok()??; - - Some(header.block_hash()) + let tip_height = storage.get_tip_height().await?; + let header = storage + .get_header(tip_height) + .await + .map_err(|_| None) + .ok()??; + Some(header.block_hash())Or more explicitly:
let tip_height = storage.get_tip_height().await?; match storage.get_header(tip_height).await { Ok(Some(header)) => Some(header.block_hash()), _ => None, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dash-spv-ffi/src/types.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rsdash-spv/src/types.rsdash-spv/tests/segmented_storage_test.rs
💤 Files with no reviewable changes (2)
- dash-spv-ffi/src/types.rs
- dash-spv/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/tests/segmented_storage_test.rs
🧰 Additional context used
📓 Path-based instructions (10)
dash-spv-ffi/tests/unit/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
Add corresponding unit tests in
tests/unit/for each new FFI function
Files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Never hardcode network parameters, addresses, or keys
Files:
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types (thiserror) and propagate errors appropriately
Use tokio runtime for async operations
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality
Use secure random number generation for keys
Never log or expose private keys
Code must target Rust 1.89 minimum supported version (MSRV)
Format code using cargo fmt
Pass clippy linting without warnings
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
Files:
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rs
**/*-ffi/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*-ffi/**/*.rs: Exercise careful handling at FFI boundaries for memory safety
Be careful with FFI memory management
Files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Create integration tests for network operations
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rs
dash-spv/src/client/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Files:
dash-spv/src/client/core.rs
**/{dash-network,dash-spv,key-wallet}/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use async/await for async operations in network and wallet modules
Files:
dash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/state.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Files:
dash-spv/src/sync/manager.rs
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.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 : Add cbindgen annotations for complex types in FFI functions
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.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 : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.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 : All FFI types must have corresponding `_destroy()` functions for explicit memory management
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.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 : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.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 `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.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/client/core.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/src/client/core.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/client/core.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/client/core.rsdash-spv/src/sync/manager.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/client/core.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/client/core.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-22T17:59:37.849Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T17:59:37.849Z
Learning: Applies to **/{dash-network,dash-spv,key-wallet}/**/*.rs : Use async/await for async operations in network and wallet modules
Applied to files:
dash-spv/src/client/core.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/src/client/core.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/src/sync/manager.rs
📚 Learning: 2025-06-26T15:52:45.327Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:52:45.327Z
Learning: In Rust, calling `headers.last().unwrap()` or `headers[headers.len() - 1]` is only safe if the vector is guaranteed non-empty; placing such code outside a tightly-coupled empty check is fragile and risks panics if future refactoring bypasses the check. Prefer pattern matching or moving logic inside the non-empty branch for robustness.
Applied to files:
dash-spv/src/sync/manager.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/sync/manager.rs
🧬 Code graph analysis (2)
dash-spv/src/client/core.rs (2)
dash-spv/src/storage/segments.rs (1)
tip_height(409-411)dash-spv/src/types.rs (1)
header(67-69)
dash-spv/src/sync/manager.rs (1)
dash-spv/src/sync/headers/manager.rs (1)
load_headers_from_storage(98-112)
🔇 Additional comments (10)
dash-spv-ffi/tests/unit/test_type_conversions.rs (1)
163-185: LGTM! Test properly updated for ChainState API change.The removal of the
headersfield initialization andheader_heightassertion correctly aligns with the PR objective of moving headers to storage-first access. The test maintains its purpose of validatingNone-value conversions for the remainingChainStatefields.dash-spv/src/storage/state.rs (5)
49-72: LGTM: ChainState deserialization correctly omits headers field.The removal of header loading logic from
load_chain_statealigns with the PR objective to move headers to storage-backed access. The JSON deserialization correctly reconstructsChainStatewith all necessary fields except the removedheadersfield.
367-382: Good defensive boundary checks added.The pre-flight validation against
tip_heightandstart_heightprevents out-of-bounds header requests and provides clear semantics: returningNonewhen the requested height falls outside the stored range or when storage has no headers.
386-389: LGTM: Signature simplified per PR design.The removal of
StorageResultwrapping aligns with the PR objective that this method "cannot fail once the StorageManager is built." The directOption<u32>return simplifies caller error handling.
390-409: LGTM: New storage accessors correctly implemented.Both
get_start_heightandget_stored_headers_lenare correctly implemented:
get_start_heightprovides the storage-backed start heightget_stored_headers_lencorrectly computes the inclusive range length with the formulaend_height - start_height + 1- Early returns for
Nonecases prevent computation on incomplete storage state
640-640: Good test coverage for new API.The test correctly validates that
get_stored_headers_len()returns the expected count after storing headers at a checkpoint height.dash-spv/src/client/core.rs (1)
202-202: LGTM: Correctly defaults to 0 for missing tip height.The
unwrap_or(0)pattern is appropriate here, representing either genesis height or the absence of stored headers. This aligns with the storage-first access pattern of the PR.dash-spv/src/sync/manager.rs (3)
224-224: LGTM: Storage parameter correctly threaded through.The addition of the
storageparameter torequest_headersenables storage-backed header synchronization, consistent with the PR's storage-first architecture.
255-255: LGTM: Simplified tip height retrieval.The direct use of
Option<u32>fromstorage.get_tip_height()correctly aligns with the simplified storage API, removing unnecessary error wrapping.
360-361: LGTM: Signature simplified to remove headers_len.Removing the
headers_lenparameter aligns with moving header ownership to storage. The cache update now only needs thesync_base_height, as header length can be queried from storage when needed.
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. |
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dash-spv/src/sync/manager.rs (2)
142-144: Error handling issue already flagged in previous review.This change was previously identified as problematic. The method signature changed to return
(), silently discarding any errors fromheader_sync.load_headers_from_storage(storage). As noted in the prior review, either restore error propagation by returning a Result type, or add explicit error logging if silent failure is the intended behavior.
374-376: Documentation need already flagged in previous review.This change was previously identified as needing better documentation. The
unwrap_or(0)pattern returns0for both "no tip in storage" and "genesis height" without distinguishing between these cases. As noted in the prior review, the method should document that a return value of0has dual semantics for clarity to future maintainers.
🧹 Nitpick comments (3)
dash-spv/tests/simple_header_test.rs (1)
101-101: Consider consistent Option handling style.Line 60 uses direct
Nonecomparison while line 101 uses.expect(). Consider using a consistent pattern throughout the test for clarity, such as:
unwrap_or(0)for defaultingexpect("message")for assertions that should always succeed- Direct
Some(value)matching for exact checksdash-spv/tests/header_sync_test.rs (1)
341-341: Consider safer Option handling in test.Line 341 uses
unwrap()afterget_tip_height(), which could panic if the result isNone. While acceptable in tests, consider usingexpect("message")for clearer failure diagnostics:🔎 Suggested improvement
- let tip_height = storage.get_tip_height().await.unwrap(); + let tip_height = storage.get_tip_height().await.expect("Tip height should exist after storing headers");dash-spv/src/sync/manager.rs (1)
351-352: LGTM! API simplified by removing redundant parameter.The removal of the
headers_lenparameter simplifies the API and aligns with the PR's goal of eliminating in-memory header tracking. The delegation toheader_sync.update_cached_from_state_snapshotis clean and reduces the coupling between chain state updates and header count tracking.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/client/lifecycle.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/state.rsdash-spv/src/sync/manager.rsdash-spv/src/sync/message_handlers.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- dash-spv/src/sync/message_handlers.rs
- dash-spv/tests/integration_real_node_test.rs
- dash-spv-ffi/tests/unit/test_type_conversions.rs
🧰 Additional context used
📓 Path-based instructions (8)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/tests/header_sync_test.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/state.rsdash-spv/tests/simple_header_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/tests/**/*.rs: Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/tests/header_sync_test.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/state.rsdash-spv/tests/simple_header_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/segments.rsdash-spv/src/storage/state.rs
dash-spv/src/client/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Files:
dash-spv/src/client/lifecycle.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Files:
dash-spv/src/sync/manager.rs
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
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.
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/state.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/storage/segments.rsdash-spv/src/storage/state.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.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/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/storage/state.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-06-26T15:52:45.327Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:52:45.327Z
Learning: In Rust, calling `headers.last().unwrap()` or `headers[headers.len() - 1]` is only safe if the vector is guaranteed non-empty; placing such code outside a tightly-coupled empty check is fragile and risks panics if future refactoring bypasses the check. Prefer pattern matching or moving logic inside the non-empty branch for robustness.
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/sync/manager.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/state.rsdash-spv/tests/simple_header_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.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/tests/simple_header_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/src/client/lifecycle.rsdash-spv/src/sync/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/src/client/lifecycle.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/client/lifecycle.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/src/client/lifecycle.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/client/lifecycle.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/sync/manager.rs
🧬 Code graph analysis (5)
dash-spv/tests/header_sync_test.rs (5)
dash-spv/src/client/core.rs (2)
storage(168-170)tip_height(201-203)dash-spv/src/storage/state.rs (1)
get_tip_height(352-354)dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)dash-spv/src/storage/segments.rs (1)
tip_height(392-394)dash-spv/src/storage/headers.rs (1)
headers(21-21)
dash-spv/src/storage/state.rs (3)
dash-spv/src/storage/segments.rs (2)
tip_height(392-394)start_height(397-399)dash-spv/src/client/core.rs (2)
tip_height(201-203)storage(168-170)dash-spv/src/storage/mod.rs (3)
get_tip_height(84-84)get_start_height(86-86)get_stored_headers_len(88-88)
dash-spv/tests/simple_header_test.rs (2)
dash-spv/src/storage/state.rs (1)
get_tip_height(352-354)dash-spv/src/storage/mod.rs (1)
get_tip_height(84-84)
dash-spv/src/client/lifecycle.rs (2)
dash-spv/src/client/core.rs (2)
storage(168-170)tip_height(201-203)dash-spv/src/storage/segments.rs (1)
tip_height(392-394)
dash-spv/src/sync/manager.rs (1)
dash-spv/src/sync/headers/manager.rs (1)
load_headers_from_storage(98-112)
⏰ 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_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (ubuntu-latest)
🔇 Additional comments (16)
dash-spv/src/storage/segments.rs (4)
78-78: LGTM! New field properly initialized.The
start_heightfield addition mirrors the existingtip_heightpattern and is correctly initialized toNoneinload_or_new.Also applies to: 137-137
363-366: LGTM! Start height tracking correctly uses minimum.The
start_heightupdate instore_items_at_heightcorrectly usesmin()to track the earliest stored height, mirroring themax()pattern used fortip_height.
396-399: LGTM! Accessor follows established pattern.The new
start_height()accessor matches the existingtip_height()accessor pattern.
143-176: Metadata loading correctly handles segment gaps for determining start/tip heights.The min/max segment ID logic works as intended even with missing intermediate segments. Using
min_seg_id.first_valid_offset()andmax_seg_id.last_valid_offset()independently correctly identifies the valid height range regardless of gaps. However, there's no explicit validation or documentation stating whether gaps are expected/allowed, andget_items()will panic in debug mode (line 558 assertion) when retrieving data across missing segments. Consider adding a comment clarifying segment continuity expectations or documenting that gaps in segment files represent sparse storage.dash-spv/tests/simple_header_test.rs (1)
60-60: LGTM! Test updated for new storage API.The assertion correctly adapts to
get_tip_height()now returningOption<u32>directly instead ofResult<Option<u32>, Error>.dash-spv/src/client/lifecycle.rs (3)
397-397: LGTM! Genesis verification correctly uses new API.The genesis verification correctly adapts to the new
get_tip_height()signature returningOption<u32>.
330-349: No flush/commit issue exists here—update_chain_state_cachedoesn't read from storage.
update_chain_state_cacheonly updates the in-memory cachedsync_base_heightvalue (viaupdate_cached_from_state_snapshot); it doesn't read headers or state from storage. The sequence is correct: headers and chain state are stored, then the sync manager's in-memory cache is updated with the same height value. No storage visibility guarantees are needed.
169-177: Add error logging for failed header loads from storage.The
load_headers_from_storagemethod handles missing headers gracefully but silently ignores errors when headers are corrupted or I/O fails. Add a log statement in the error case to ensure data corruption or read failures are visible.dash-spv/tests/header_sync_test.rs (2)
33-33: LGTM! Test assertions correctly updated for new API.All
get_tip_height()assertions have been properly updated to handle the newOption<u32>return type, comparing directly againstNoneorSome(value).Also applies to: 60-60, 70-70, 105-116, 143-143, 148-148, 153-153, 194-194, 232-232
366-395: Remove this comment; the stated concern is inaccurate.The
ChainState::new_for_networkimplementation indash-spv/src/types.rs(lines 276–290) does not push a genesis header. It initializes the masternode engine and setssync_base_height = 0, but contains nostate.headers.push(genesis_header)call. The test's header storage logic viastorage.store_headers(&headers)is not misaligned with any undone genesis header push, as no such push exists in the current code.dash-spv/src/storage/state.rs (4)
47-70: LGTM! Chain state loading simplified correctly.The removal of header loading from
load_chain_statealigns with the PR objective of removing headers from chain state. The method now only loads scalar state fields from JSON.
333-348: Boundary checks correctly prevent out-of-range access.The added boundary checks ensure
get_headerreturnsNonefor heights outside the stored range[start_height, tip_height]. The early returns when tip_height or start_height isNoneare also correct.
605-605: LGTM! Test validates new length calculation.The test correctly asserts that
get_stored_headers_lenmatches the number of headers stored.
360-375: Theget_stored_headers_lenformula is correct and validated by the existing testtest_checkpoint_storage_indexingin the test module. The test creates 100 headers at checkpoint height 1,100,000 and asserts thatget_stored_headers_len()returns 100, confirming that the calculationend_height - start_height + 1works as expected (1,100,099 − 1,100,000 + 1 = 100).dash-spv/src/sync/manager.rs (2)
215-215: LGTM! Storage parameter correctly threaded through.The addition of the
storageparameter torequest_headersaligns with the PR's objective to shift from in-memory header caching to storage-first access. Error propagation is correctly preserved with the?operator.
246-246: The infallibility assumption forget_tip_heightis sound and correctly applied.The method returns
Option<u32>directly because it performs a read-only operation on in-memory state (reading from aRwLock<BlockHeaderChain>), which cannot fail with I/O errors or storage corruption. The operation is guaranteed to complete without returning aResulterror type. The distinction betweenget_tip_height(infallible read) andget_header(fallible lookup) is correct and properly motivated by their different operational guarantees.
|
|
||
| // Step 3: Process the Entire Validated Batch | ||
|
|
||
| // Checkpoint Validation: Perform in-memory security check against checkpoints |
There was a problem hiding this comment.
Why did you remove the checkpoint validation?
There was a problem hiding this comment.
I am rolling it back, I don't remember the reason but I think I had one at some point
| #[test_case(170000, 1 ; "checkpoint_1_block")] | ||
| #[test_case(12345, 60000 ; "checkpoint_60000_blocks")] | ||
| #[tokio::test] | ||
| async fn test_load_headers_from_storage(sync_base_height: u32, header_count: usize) { |
There was a problem hiding this comment.
Why was this test removed?
There was a problem hiding this comment.
Bcs what this is testing is that all headers are stored in the ChainState, then the ChainState stored in the Storage making the headers persist as Segments. Then the test asks the HeaderSyncManager for the headers stored as Segments and compares the length with the length of the headers in the ChainState. Since ChainState no longer has a copy of the headers this test doesn't make sense
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
I am making some changes following your review. About the methods you suggested, I can add them in this PR or save it for late. Since you are rewriting the sync I wouldn't add nothing new here or in the future that is needed by the current sync logic
| #[test_case(170000, 1 ; "checkpoint_1_block")] | ||
| #[test_case(12345, 60000 ; "checkpoint_60000_blocks")] | ||
| #[tokio::test] | ||
| async fn test_load_headers_from_storage(sync_base_height: u32, header_count: usize) { |
There was a problem hiding this comment.
Bcs what this is testing is that all headers are stored in the ChainState, then the ChainState stored in the Storage making the headers persist as Segments. Then the test asks the HeaderSyncManager for the headers stored as Segments and compares the length with the length of the headers in the ChainState. Since ChainState no longer has a copy of the headers this test doesn't make sense
There was a problem hiding this comment.
Actionable comments posted: 1
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/headers/manager.rs (1)
541-560: Redundant genesis header check.Lines 541-546 retrieve the genesis header from storage, then lines 548-555 check again if the genesis exists. This double-check is redundant since we're already inside the
Some(genesis_header)branch from line 541. The inner check at lines 548-554 will always returnSomeand the store operation will never execute.This appears to be dead code that should be removed.
🔎 Suggested fix
if let Some(genesis_header) = storage.get_header(0).await.map_err(|e| { SyncError::Storage(format!( "Error trying to get genesis block from storage: {}", e )) })? { - // Store genesis in storage if not already there - if storage - .get_header(0) - .await - .map_err(|e| { - SyncError::Storage(format!("Failed to check genesis: {}", e)) - })? - .is_none() - { - tracing::info!("Storing genesis block in storage"); - storage.store_headers(&[genesis_header]).await.map_err(|e| { - SyncError::Storage(format!("Failed to store genesis: {}", e)) - })?; - } - let genesis_hash = genesis_header.block_hash(); tracing::info!("Starting from genesis block: {}", genesis_hash); Some(genesis_hash)If the intent was to store genesis when it doesn't exist, that logic should be outside the
Somebranch.
♻️ Duplicate comments (2)
dash-spv/src/sync/headers/manager.rs (2)
515-523: Move checkpoint header retrieval inside conditional branches.Lines 515-523 unconditionally retrieve
start_heightandcheckpoint_header, but these values are only used within conditional blocks that checkis_synced_from_checkpoint()(lines 528 and 596). If not syncing from a checkpoint, this code will error with "Failed to get start height" despite not needing those values.This is the same issue flagged in previous reviews. Move the retrieval to lines 528-529 and 596-597 where the values are actually used.
🔎 Suggested refactor
Remove lines 515-523 and retrieve the checkpoint header only where needed:
At line 528:
- if self.is_synced_from_checkpoint() && storage.get_stored_headers_len().await > 0 { - let checkpoint_hash = checkpoint_header.block_hash(); + if self.is_synced_from_checkpoint() && storage.get_stored_headers_len().await > 0 { + let first_height = storage.get_start_height().await + .ok_or(SyncError::Storage("Failed to get start height".to_string()))?; + let checkpoint_header = storage.get_header(first_height).await + .map_err(|e| SyncError::Storage(format!("Failed to get first header: {}", e)))? + .ok_or(SyncError::Storage("Storage didn't return first header".to_string()))?; + let checkpoint_hash = checkpoint_header.block_hash();Similarly at line 596.
675-683: Same issue: Move checkpoint header retrieval inside conditional branch.This has the identical problem as
prepare_sync(lines 515-523). Lines 675-683 unconditionally retrievestart_heightandcheckpoint_header, but these are only used at lines 690-697 within a conditional checkingis_synced_from_checkpoint(). This will cause unnecessary errors when not syncing from a checkpoint.🔎 Suggested refactor
Move the retrieval inside the conditional at line 688:
if self.is_synced_from_checkpoint() { // Use the checkpoint hash from chain state if storage.get_stored_headers_len().await > 0 { + let first_height = storage.get_start_height().await + .ok_or(SyncError::Storage("Failed to get start height".to_string()))?; + let checkpoint_header = storage.get_header(first_height).await + .map_err(|e| SyncError::Storage(format!("Failed to get first header: {}", e)))? + .ok_or(SyncError::Storage("Storage didn't return first header".to_string()))?; let checkpoint_hash = checkpoint_header.block_hash();
🧹 Nitpick comments (1)
dash-spv/src/sync/headers/manager.rs (1)
845-846: Consider:unwrap_or(0)conflates "no headers" with "genesis height".The
unwrap_or(0)at line 846 makes it impossible for callers to distinguish between:
Some(0): Genesis block at height 0 (valid state)None: No headers stored (empty state)Both map to 0. If this distinction matters for callers, consider returning
Option<u32>directly or changing the method name toget_chain_height_or_defaultto make the behavior explicit.Note: The past review comment suggested returning
Result<Option<u32>>, but sinceget_tip_height()now returnsOption<u32>(no error path per PR objectives), there's no error state being masked here—only the ambiguity between empty and genesis.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dash-spv/src/storage/state.rsdash-spv/src/sync/headers/manager.rsdash-spv/tests/storage_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/tests/storage_test.rs
🧰 Additional context used
📓 Path-based instructions (4)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/storage/state.rsdash-spv/src/sync/headers/manager.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/state.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/src/storage/state.rsdash-spv/src/sync/headers/manager.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Files:
dash-spv/src/sync/headers/manager.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/storage/state.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/storage/state.rsdash-spv/src/sync/headers/manager.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/state.rsdash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-06-26T15:48:36.342Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:48:36.342Z
Learning: In Rust FFI code, unwrap() must not be used on CString::new, especially for error messages or untrusted input, as it can panic if the string contains null bytes. Instead, use unwrap_or_else with a fallback to a known-safe, hardcoded string to prevent panics across FFI boundaries.
Applied to files:
dash-spv/src/storage/state.rs
📚 Learning: 2025-06-26T15:51:03.126Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:51:03.126Z
Learning: In Rust network code, using unwrap() on SystemTime::now().duration_since(UNIX_EPOCH) can panic if the system clock is before UNIX_EPOCH. Use unwrap_or with a safe fallback (e.g., Duration::from_secs(0)) to prevent panics in edge cases.
Applied to files:
dash-spv/src/storage/state.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to src/**/*.rs : Avoid `unwrap()` and `expect()` in library code; use proper error types (e.g., `thiserror`)
Applied to files:
dash-spv/src/storage/state.rs
📚 Learning: 2025-02-27T05:39:16.767Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 56
File: dash/src/sml/masternode_list_engine/message_request_verification.rs:91-91
Timestamp: 2025-02-27T05:39:16.767Z
Learning: In the Dash Rust codebase, unwrap() on try_into() for byte slices from hash types like QuorumSigningRequestId is safe because hash types guarantee fixed-size byte arrays (32 bytes), ensuring the slice operations and conversions will never fail.
Applied to files:
dash-spv/src/storage/state.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/sync/headers/manager.rs
📚 Learning: 2025-06-26T15:52:45.327Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:52:45.327Z
Learning: In Rust, calling `headers.last().unwrap()` or `headers[headers.len() - 1]` is only safe if the vector is guaranteed non-empty; placing such code outside a tightly-coupled empty check is fragile and risks panics if future refactoring bypasses the check. Prefer pattern matching or moving logic inside the non-empty branch for robustness.
Applied to files:
dash-spv/src/sync/headers/manager.rs
🧬 Code graph analysis (1)
dash-spv/src/storage/state.rs (5)
dash-spv/src/network/handshake.rs (1)
state(297-299)dash-spv/src/storage/segments.rs (2)
tip_height(392-394)start_height(397-399)dash-spv/src/client/core.rs (2)
tip_height(201-203)storage(168-170)dash-spv/src/storage/mod.rs (3)
get_tip_height(84-84)get_start_height(86-86)get_stored_headers_len(88-88)dash-spv/src/storage/headers.rs (1)
headers(21-21)
⏰ 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). (11)
- GitHub Check: No-std Checks
- GitHub Check: Documentation
- GitHub Check: Security Audit
- GitHub Check: Verify Test Execution
- GitHub Check: MSRV Check (1.89)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Thread Sanitizer
- GitHub Check: Address Sanitizer
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (windows-latest)
🔇 Additional comments (9)
dash-spv/src/storage/state.rs (6)
18-73: LGTM: Chain state persistence correctly excludes headers.The changes appropriately remove header persistence and loading from chain state management, aligning with the PR objective to remove the
headersfield fromChainState.
332-342: LGTM: Boundary checks properly guard header access.The addition of boundary checks using
is_none_orcorrectly prevents out-of-range access and follows modern Rust idioms.
344-346: LGTM: Signature change aligns with PR objectives.The simplified return type
Option<u32>(removing theStorageResultwrapper) correctly reflects that this operation cannot fail once theStorageManageris built, as noted in the PR objectives.
348-350: LGTM: New accessor method.The
get_start_heightmethod provides necessary access to the start height for callers, consistent with the new storage-centric approach.
352-367: LGTM: Length computation is correct and clear.The method correctly computes the number of stored headers as an inclusive range
[start_height, end_height], returning 0 when either boundary is absent. The explicit early-return pattern clearly communicates the logic.
597-597: LGTM: Test validates new method.The assertion appropriately verifies that
get_stored_headers_len()returns the correct count after storing headers.dash-spv/src/sync/headers/manager.rs (3)
100-114: LGTM: Simplified signature reflects side-effect pattern.Changing the return type to
()is appropriate since this method updates internal state via side effects rather than returning computed values.
277-316: LGTM: Conditional checkpoint retrieval is correct.The checkpoint header retrieval at lines 284-296 is properly guarded by the conditional check at line 283, ensuring it only executes when syncing from a checkpoint with stored headers.
417-430: LGTM: Headers2 initialization properly handles storage errors.The error handling for retrieving the tip header from storage is appropriate, with descriptive error messages.
| // Update chain tip manager with the last header in the batch | ||
| if let Some(last_header) = headers.last() { | ||
| let final_height = self.chain_state.read().await.get_height(); | ||
| let final_height = storage.get_tip_height().await.unwrap_or(0); |
There was a problem hiding this comment.
Consider error handling instead of defaulting to 0.
After successfully storing headers in the batch (line 236), get_tip_height() returning None at line 248 would indicate an inconsistent storage state. Rather than silently defaulting to 0, this should be treated as an error since headers were just stored.
🔎 Suggested improvement
- let final_height = storage.get_tip_height().await.unwrap_or(0);
+ let final_height = storage.get_tip_height().await
+ .ok_or_else(|| SyncError::InvalidState("No tip height after storing headers".to_string()))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let final_height = storage.get_tip_height().await.unwrap_or(0); | |
| let final_height = storage.get_tip_height().await | |
| .ok_or_else(|| SyncError::InvalidState("No tip height after storing headers".to_string()))?; |
Removed the headers field from ChainState. Also modified a StorageManager trait method return type removing a the possibility of error (bcs it cannot fail once the StorageManager is built), that added a little bit of noise to this PR, more than expected.
All tests pass and I was able to execute example simple_sync reaching height 2_391_372 in 15 min (more or less).
There is only one thing, one piece of logic that I didn't replace.
In
dash-spv/src/types.rsline 303, inside methodChainState::new_for_networkwe can find this:I just dropped the push of the genesis_block without adding it to the storage expecting a future issue during sync or while executing the tests but that never happened. I guess we are sync the genesis block from the network instead of having it hardcoded there but I am not sure and make me wonder. Since it works and theorically this is somethign we can ask the network for I am not wasting time here unless someone knows something that I don't. I want to continue updating ChainState
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.