Skip to content

fix: Stop loading headers twice into ChainState::headers #258

Merged
PastaPastaPasta merged 2 commits intodashpay:v0.41-devfrom
xdustinface:fix/header-loading
Dec 10, 2025
Merged

fix: Stop loading headers twice into ChainState::headers #258
PastaPastaPasta merged 2 commits intodashpay:v0.41-devfrom
xdustinface:fix/header-loading

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Dec 10, 2025

We currently load the headers twice into the ChainState::headers vector in HeaderSyncManager::load_headers_from_storage (if we have a tip height during startup):

  • First within storage.load_chain_state
  • And then within load_headers_from_storage itself further down

This leads to invalid tip height during sync and then to failing checkpoint checks and a stuck sync:

  • During initial checkpoint sync because we load the checkpoint and then after initialize_genesis_block we run into load_headers_from_storage since we have a tip height in this case. Thats not a problem in the initial genesis sync because we there not run into load_headers_from_storage.
  • When reloading after a restart its a problem for genesis and checkpoint sync.

The logic of reloading the headers in DashSpvClient::start in case of initial checkpoint sync should probably also be tweaked to not run into a reload for the initial sync but at least with this PR its not a issue anymore just a redundancy.

See the test i added in 8e94e50 which fails on v0.41-dev but passes here.

Before

2025-12-04T16:05:36.717779Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:05:36.727966Z  INFO ✅ Loaded 2 headers into HeaderSyncManager in 0.00s (1129 headers/sec)
....

2025-12-05T04:31:15.980982Z ERROR Sequential sync manager error handling message: Validation error: Block at height 2300000 does not match checkpoint

After

2025-12-04T16:09:37.959349Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:09:37.966703Z  INFO ✅ Loaded 1 headers into HeaderSyncManager in 0.00s (862 headers/sec)

Summary by CodeRabbit

  • Refactor

    • Simplified header loading by replacing incremental batch processing with a single-step restoration from persisted chain state; progress reporting simplified and final tip height logged.
  • Tests

    • Added tests that validate loading headers and chain state from disk using deterministic test data and temporary storage.
  • Chores

    • Added a test-only dependency to support the new test cases.

✏️ Tip: You can customize this high-level summary in your review settings.

Results on current `v0.41-dev`

---- test_load_headers_from_storage::genesis_1_block stdout ----

thread 'test_load_headers_from_storage::genesis_1_block' (8727544) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 0
 right: 1

---- test_load_headers_from_storage::checkpoint_1_block stdout ----

thread 'test_load_headers_from_storage::checkpoint_1_block' (8727541) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 1
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test_load_headers_from_storage::checkpoint_60000_blocks stdout ----

thread 'test_load_headers_from_storage::checkpoint_60000_blocks' (8727542) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 60000
 right: 120000

---- test_load_headers_from_storage::genesis_60000_blocks stdout ----

thread 'test_load_headers_from_storage::genesis_60000_blocks' (8727545) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 59999
 right: 60000
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

Walkthrough

Restores in-memory header chain from a persisted chain state instead of batch height-range loading, adds a dev-dependency test-case = "3.3", and introduces a disk-backed test that validates loading headers from storage via HeaderSyncManager.

Changes

Cohort / File(s) Summary
Dev Dependencies
dash-spv/Cargo.toml
Adds test-case = "3.3" under [dev-dependencies].
Header Loading Refactor
dash-spv/src/sync/headers/manager.rs
load_headers_from_storage() now restores the in-memory chain from a stored chain state (initializes timing/counters earlier, sets total_headers_synced from stored tip height, simplifies logging, and returns loaded count from the restored state); removes prior batch-based loading loop and per-batch progress logic.
Storage-backed Test
dash-spv/tests/header_sync_test.rs
Adds test_load_headers_from_storage using DiskStorageManager, TempDir, deterministic header chain generation, and HeaderSyncManager to store a chain state and assert loaded header counts and restored tip height match expectations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Inspect manager.rs for correct atomic replacement of in-memory chain state and proper updates to cached_sync_base_height and total_headers_synced.
  • Verify removal of batch loop doesn't change callers' expectations about returned counts or progress semantics.
  • Check header_sync_test.rs setup: deterministic chain generation, correct storage writes, and reliable temp dir handling.

Poem

🐰 I tunneled to disk where headers lay,

Restored the chain in one bright day,
With test-case hopping at my side,
TempDir warmed the path I tried,
A rabbit smiles — the sync's okay.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main change: removing duplicate header loading into ChainState::headers, which is the core issue addressed in the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
dash-spv/tests/header_sync_test.rs (1)

346-382: Consider adding assertions for sync_base_height preservation.

The test validates that header counts match but doesn't verify that sync_base_height is correctly preserved during load. Since the PR addresses checkpoint sync issues, explicitly asserting sync_base_height consistency would strengthen the test.

Add this assertion after line 378:

assert_eq!(
    cs.sync_base_height, 
    sync_base_height, 
    "sync_base_height should be preserved"
);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d182ab and 072ada5.

📒 Files selected for processing (3)
  • dash-spv/Cargo.toml (1 hunks)
  • dash-spv/src/sync/headers/manager.rs (1 hunks)
  • dash-spv/tests/header_sync_test.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code

**/*.rs: Each crate keeps sources in src/; unit tests live alongside code with #[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code with rustfmt (see rustfmt.toml); run cargo fmt --all before commits
Lint Rust code with clippy; avoid unwrap()/expect() in library code; use error types (e.g., thiserror)
Use snake_case for function and variable names in Rust
Use UpperCamelCase for types and traits in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async via tokio where 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 -- --ignored flag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code

Files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
dash-spv/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/**/*.rs: Use trait-based abstractions for core components like NetworkManager and StorageManager to enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with cargo check --all-features

Files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
dash-spv/src/sync/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Synchronization should use SyncManager for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)

Files:

  • dash-spv/src/sync/headers/manager.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 Rust

Integration tests use tests/ directory (e.g., rpc-integration-test)

Files:

  • dash-spv/tests/header_sync_test.rs
dash-spv/tests/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Integration tests should be organized in tests/ directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration

Files:

  • dash-spv/tests/header_sync_test.rs
🧠 Learnings (22)
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase with tokio runtime for all asynchronous operations

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/Cargo.toml
📚 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/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with `cargo check --all-features`

Applied to files:

  • dash-spv/Cargo.toml
📚 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/Cargo.toml
📚 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/Cargo.toml
  • dash-spv/tests/header_sync_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: 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/Cargo.toml
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should be organized in `tests/` directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration

Applied to files:

  • dash-spv/Cargo.toml
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-08-21T04:45:50.436Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.436Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.

Applied to files:

  • dash-spv/Cargo.toml
📚 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/Cargo.toml
📚 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/Cargo.toml
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)

Applied to files:

  • dash-spv/Cargo.toml
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Keep test vectors updated with Dash Core, monitor for new DIPs affecting wallet structure, maintain backward compatibility for serialized wallets, and update derivation paths for Platform features

Applied to files:

  • dash-spv/Cargo.toml
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/error.rs : Maintain domain-specific error types in `error.rs` with variants for `NetworkError`, `StorageError`, `SyncError`, `ValidationError`, and `SpvError` (top-level wrapper)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/tests/**/*.rs : Integration tests use `tests/` directory (e.g., `rpc-integration-test`)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Validation module should support configurable validation modes (`ValidationMode::None`, `ValidationMode::Basic`, `ValidationMode::Full`) with header validation, ChainLock, and InstantLock verification

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/client/**/*.rs : Client module should provide high-level API through `DashSpvClient` and configuration via `ClientConfig`

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/network/**/*.rs : Network module should handle TCP connections, handshake management, message routing, and peer management following Dash protocol implementation

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/wallet/**/*.rs : Wallet module should provide UTXO tracking via `Utxo` struct, balance calculation with confirmation states, and transaction processing via `TransactionProcessor`

Applied to files:

  • dash-spv/tests/header_sync_test.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: Core Components Tests
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (2)
dash-spv/tests/header_sync_test.rs (1)

8-9: LGTM!

The new imports for DiskStorageManager, HeaderSyncManager, ReorgConfig, test_case, and TempDir are appropriate for the parameterized storage loading test.

Also applies to: 17-17, 19-19

dash-spv/src/sync/headers/manager.rs (1)

104-136: Verify tip_height calculation consistency with sync_base_height.

The refactored method now relies on stored_chain_state.tip_height() (Line 117) and assigns it directly to self.total_headers_synced (Line 124). However, update_cached_from_state_snapshot calculates this value as:

sync_base_height.saturating_add(headers_len).saturating_sub(1)

Confirm whether ChainState::tip_height() returns the absolute blockchain height (accounting for sync_base_height) or a relative height. If it returns a relative height within stored headers, Line 124 may need adjustment to match the formula used elsewhere.

We currently load the headers twice into the header vector in `HeaderSyncManager::load_headers_from_storage`:

- First1. within `storage.load_chain_state`
- And then within `load_headers_from_storage` itself further down

```
2025-12-04T16:05:36.717779Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:05:36.727966Z  INFO ✅ Loaded 2 headers into HeaderSyncManager in 0.00s (1129 headers/sec)
....

2025-12-05T04:31:15.980982Z ERROR Sequential sync manager error handling message: Validation error: Block at height 2300000 does not match checkpoint
```

```
2025-12-04T16:09:37.959349Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:09:37.966703Z  INFO ✅ Loaded 1 headers into HeaderSyncManager in 0.00s (862 headers/sec)
´´´
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
dash-spv/tests/header_sync_test.rs (1)

339-344: Fix duplicated parameters for checkpoint_0_blocks test case

"checkpoint_0_blocks" currently uses (0, 0), identical to "genesis_0_blocks", so it never exercises a non‑zero sync_base_height checkpoint scenario. This was already flagged in a previous review and still applies.

To actually cover checkpoint sync with zero headers, update sync_base_height (e.g., to the checkpoint base used elsewhere):

-#[test_case(0, 0 ; "checkpoint_0_blocks")]
+#[test_case(170000, 0 ; "checkpoint_0_blocks")]
🧹 Nitpick comments (1)
dash-spv/tests/header_sync_test.rs (1)

345-379: Verify load_headers_from_storage storage mutability and consider minor test refinements

This regression test is valuable for guarding against double‑loading headers from storage across different sync_base_height / header_count combinations.

Two follow‑ups to consider:

  1. Storage mutability for load_headers_from_storage
    Based on prior learnings, StorageManager trait methods generally take &mut self. If HeaderSyncManager::load_headers_from_storage is declared as taking &mut S: StorageManager, this call should likely be:

  • let loaded_count =
  •    header_sync.load_headers_from_storage(&storage).await.expect("Failed to load headers");
    
  • let loaded_count = header_sync
  •    .load_headers_from_storage(&mut storage)
    
  •    .await
    
  •    .expect("Failed to load headers");
    
    
    Please double‑check the method signature and adjust if needed so the test compiles cleanly.  
    Based on learnings, `StorageManager` uses `&mut self` methods.
    
    
  1. Optional: tighten assertions / comments
    • The comment // Setup: Create storage with 100 headers is now stale since header_count is parameterized; updating or dropping it would avoid confusion.
    • If ChainState exposes a tip or sync_base_height–relative invariant (e.g., via an accessor), asserting that alongside headers.len() would further guard against regressions in tip calculation, which this PR is targeting.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 072ada5 and df7628b.

📒 Files selected for processing (2)
  • dash-spv/src/sync/headers/manager.rs (1 hunks)
  • dash-spv/tests/header_sync_test.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • dash-spv/src/sync/headers/manager.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code

**/*.rs: Each crate keeps sources in src/; unit tests live alongside code with #[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code with rustfmt (see rustfmt.toml); run cargo fmt --all before commits
Lint Rust code with clippy; avoid unwrap()/expect() in library code; use error types (e.g., thiserror)
Use snake_case for function and variable names in Rust
Use UpperCamelCase for types and traits in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async via tokio where 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 -- --ignored flag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code

Files:

  • dash-spv/tests/header_sync_test.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 Rust

Integration tests use tests/ directory (e.g., rpc-integration-test)

Files:

  • dash-spv/tests/header_sync_test.rs
dash-spv/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/**/*.rs: Use trait-based abstractions for core components like NetworkManager and StorageManager to enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with cargo check --all-features

Files:

  • dash-spv/tests/header_sync_test.rs
dash-spv/tests/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Integration tests should be organized in tests/ directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration

Files:

  • dash-spv/tests/header_sync_test.rs
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Cover critical parsing, networking, SPV, and wallet flows with tests; add regression tests for fixes; consider property tests with `proptest`
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should be organized in `tests/` directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations

Applied to files:

  • dash-spv/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/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Validation module should support configurable validation modes (`ValidationMode::None`, `ValidationMode::Basic`, `ValidationMode::Full`) with header validation, ChainLock, and InstantLock verification

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/error.rs : Maintain domain-specific error types in `error.rs` with variants for `NetworkError`, `StorageError`, `SyncError`, `ValidationError`, and `SpvError` (top-level wrapper)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/tests/**/*.rs : Integration tests use `tests/` directory (e.g., `rpc-integration-test`)

Applied to files:

  • dash-spv/tests/header_sync_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/header_sync_test.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Test both mainnet and testnet configurations

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/client/**/*.rs : Client module should provide high-level API through `DashSpvClient` and configuration via `ClientConfig`

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/network/**/*.rs : Network module should handle TCP connections, handshake management, message routing, and peer management following Dash protocol implementation

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)

Applied to files:

  • dash-spv/tests/header_sync_test.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/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Never panic in library code; use `Result<T>` for all fallible operations with custom `Error` type variants and provide context in error messages

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Use `?` operator for error propagation in Rust code to maintain clean error handling patterns

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation}/**/*.rs : Cache intermediate derivation results and use hardened derivation only when required; batch derive child keys when possible to optimize derivation performance

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Core Components Tests
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: SPV Components Tests
  • GitHub Check: fuzz (dash_deserialize_script)
🔇 Additional comments (1)
dash-spv/tests/header_sync_test.rs (1)

5-11: New imports correctly reflect DiskStorage-based regression test

Bringing in DiskStorageManager, HeaderSyncManager, ReorgConfig, TempDir, and test_case matches the new storage-backed header loading test; usage is consistent and looks good.

Also applies to: 18-19

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
dash-spv/tests/header_sync_test.rs (1)

342-342: Use consistent checkpoint height across test cases.

Line 342 uses sync_base_height = 100, while line 343 uses 170000 for checkpoint tests. For consistency and to properly test checkpoint sync behavior, all checkpoint test cases should use the same checkpoint base height.

Apply this diff to align with the other checkpoint tests:

-#[test_case(100, 0 ; "checkpoint_0_blocks")]
+#[test_case(170000, 0 ; "checkpoint_0_blocks")]
🧹 Nitpick comments (2)
dash-spv/tests/header_sync_test.rs (1)

346-379: Consider adding tip height validation.

The test validates that the loaded header count matches the stored count, but doesn't verify that the tip height calculation is correct after loading. Since the PR objectives mention fixing "incorrect tip height during sync," it would strengthen the test to also assert the tip height.

Add this assertion after line 378:

 assert_eq!(loaded_count as usize, header_count, "Loaded count mismatch");
 assert_eq!(header_count, cs.headers.len(), "Chain state count mismatch");
+
+let expected_tip = if header_count == 0 {
+    sync_base_height
+} else {
+    sync_base_height + header_count as u32 - 1
+};
+assert_eq!(cs.tip_height(), expected_tip, "Tip height mismatch");
dash-spv/src/sync/headers/manager.rs (1)

109-122: Document why storage errors are silently ignored.

The code uses if let Ok(Some(...)) which silently ignores errors from storage.load_chain_state(). While this might be acceptable for initialization (falling back to an empty state), the coding guidelines recommend propagating errors appropriately. Consider either propagating the error or adding a comment explaining why errors are intentionally ignored.

Option 1: Add documentation explaining the behavior:

+    // Silently ignore storage errors and start with empty state if loading fails.
+    // This allows initialization to proceed even if storage is corrupted or unavailable.
     if let Ok(Some(stored_chain_state)) = storage.load_chain_state().await {

Option 2: Log errors while continuing with empty state:

-    if let Ok(Some(stored_chain_state)) = storage.load_chain_state().await {
+    match storage.load_chain_state().await {
+        Ok(Some(stored_chain_state)) => {
         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
         {
             loaded_count = stored_chain_state.headers.len();
             tip_height = stored_chain_state.tip_height();
             self.cached_sync_base_height = stored_chain_state.sync_base_height;
             let mut cs = self.chain_state.write().await;
             *cs = stored_chain_state;
         }
+        }
+        Ok(None) => {
+            tracing::debug!("No stored chain state found, starting with empty state");
+        }
+        Err(e) => {
+            tracing::warn!("Failed to load chain state from storage: {}. Starting with empty state.", e);
+        }
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df7628b and ee659df.

📒 Files selected for processing (2)
  • dash-spv/src/sync/headers/manager.rs (1 hunks)
  • dash-spv/tests/header_sync_test.rs (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code

**/*.rs: Each crate keeps sources in src/; unit tests live alongside code with #[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code with rustfmt (see rustfmt.toml); run cargo fmt --all before commits
Lint Rust code with clippy; avoid unwrap()/expect() in library code; use error types (e.g., thiserror)
Use snake_case for function and variable names in Rust
Use UpperCamelCase for types and traits in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async via tokio where 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 -- --ignored flag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code

Files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
dash-spv/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/**/*.rs: Use trait-based abstractions for core components like NetworkManager and StorageManager to enable swappable implementations
Use async/await throughout the codebase with tokio runtime for all asynchronous operations
Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with cargo check --all-features

Files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
dash-spv/src/sync/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Synchronization should use SyncManager for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)

Files:

  • dash-spv/src/sync/headers/manager.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 Rust

Integration tests use tests/ directory (e.g., rpc-integration-test)

Files:

  • dash-spv/tests/header_sync_test.rs
dash-spv/tests/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Integration tests should be organized in tests/ directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration

Files:

  • dash-spv/tests/header_sync_test.rs
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for core components like `NetworkManager` and `StorageManager` to enable swappable implementations

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Storage module should provide abstraction via `StorageManager` trait with concrete implementations for `MemoryStorageManager` and `DiskStorageManager`

Applied to files:

  • dash-spv/src/sync/headers/manager.rs
  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should be organized in `tests/` directory with comprehensive test suites for network layer, storage layer, header sync, and real network integration

Applied to files:

  • dash-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/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/error.rs : Maintain domain-specific error types in `error.rs` with variants for `NetworkError`, `StorageError`, `SyncError`, `ValidationError`, and `SpvError` (top-level wrapper)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Validation module should support configurable validation modes (`ValidationMode::None`, `ValidationMode::Basic`, `ValidationMode::Full`) with header validation, ChainLock, and InstantLock verification

Applied to files:

  • dash-spv/tests/header_sync_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/header_sync_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: Run `./sync-headers.sh` to copy updated headers to Swift SDK after rebuilding dash-spv-ffi

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Test both mainnet and testnet configurations

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic test vectors with fixed seeds and known test vectors in unit tests for key derivation, mnemonic generation, and address generation to ensure reproducibility

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/client/**/*.rs : Client module should provide high-level API through `DashSpvClient` and configuration via `ClientConfig`

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/network/**/*.rs : Network module should handle TCP connections, handshake management, message routing, and peer management following Dash protocol implementation

Applied to files:

  • dash-spv/tests/header_sync_test.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/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Never panic in library code; use `Result<T>` for all fallible operations with custom `Error` type variants and provide context in error messages

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{wallet,account,managed_account}/**/*.rs : Use BTreeMap for ordered data structures (accounts, transactions) and HashMap for quick lookups (address to index mapping) to optimize data structure performance

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation,account}/**/*.rs : Use `ExtendedPubKey` and `ExtendedPrivKey` from the BIP32 implementation for key derivation; avoid direct private key exposure or serialization in logs

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/*.rs : Use `?` operator for error propagation in Rust code to maintain clean error handling patterns

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{account,managed_account}/**/*.rs : Separate immutable and mutable concerns: Use `Account` for immutable identity information (keys, derivation paths) and `ManagedAccount` for mutable state (address pools, metadata, balances)

Applied to files:

  • dash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/{bip32,derivation}/**/*.rs : Cache intermediate derivation results and use hardened derivation only when required; batch derive child keys when possible to optimize derivation performance

Applied to files:

  • dash-spv/tests/header_sync_test.rs
🧬 Code graph analysis (2)
dash-spv/src/sync/headers/manager.rs (3)
dash-spv/src/types.rs (1)
  • tip_height (333-342)
dash-spv/src/client/core.rs (2)
  • tip_height (198-201)
  • storage (169-171)
dash-spv/src/storage/memory.rs (1)
  • sync_base_height (45-50)
dash-spv/tests/header_sync_test.rs (2)
dash-spv/src/sync/headers/manager.rs (1)
  • storage (263-263)
dash-spv/tests/reverse_index_test.rs (1)
  • storage (61-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
  • GitHub Check: Key Wallet Components Tests
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: SPV Components Tests
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (1)
dash-spv/src/sync/headers/manager.rs (1)

105-135: LGTM: Refactored loading logic eliminates redundant double-load.

The refactored logic correctly restores headers from the persisted chain state instead of batch-loading from storage, which eliminates the redundant double-load issue mentioned in the PR objectives. The implementation properly updates total_headers_synced, cached_sync_base_height, and the in-memory chain state, and includes helpful logging with tip height information.

@PastaPastaPasta PastaPastaPasta merged commit 1439955 into dashpay:v0.41-dev Dec 10, 2025
24 checks passed
pauldelucia pushed a commit that referenced this pull request Dec 12, 2025
* Add test

Results on current `v0.41-dev`

---- test_load_headers_from_storage::genesis_1_block stdout ----

thread 'test_load_headers_from_storage::genesis_1_block' (8727544) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 0
 right: 1

---- test_load_headers_from_storage::checkpoint_1_block stdout ----

thread 'test_load_headers_from_storage::checkpoint_1_block' (8727541) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 1
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test_load_headers_from_storage::checkpoint_60000_blocks stdout ----

thread 'test_load_headers_from_storage::checkpoint_60000_blocks' (8727542) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 60000
 right: 120000

---- test_load_headers_from_storage::genesis_60000_blocks stdout ----

thread 'test_load_headers_from_storage::genesis_60000_blocks' (8727545) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 59999
 right: 60000

* Stop loading headers twice into `ChainState::headers`

We currently load the headers twice into the header vector in `HeaderSyncManager::load_headers_from_storage`:

- First1. within `storage.load_chain_state`
- And then within `load_headers_from_storage` itself further down

```
2025-12-04T16:05:36.717779Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:05:36.727966Z  INFO ✅ Loaded 2 headers into HeaderSyncManager in 0.00s (1129 headers/sec)
....

2025-12-05T04:31:15.980982Z ERROR Sequential sync manager error handling message: Validation error: Block at height 2300000 does not match checkpoint
```

```
2025-12-04T16:09:37.959349Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:09:37.966703Z  INFO ✅ Loaded 1 headers into HeaderSyncManager in 0.00s (862 headers/sec)
´´´
pauldelucia pushed a commit that referenced this pull request Dec 12, 2025
* Add test

Results on current `v0.41-dev`

---- test_load_headers_from_storage::genesis_1_block stdout ----

thread 'test_load_headers_from_storage::genesis_1_block' (8727544) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 0
 right: 1

---- test_load_headers_from_storage::checkpoint_1_block stdout ----

thread 'test_load_headers_from_storage::checkpoint_1_block' (8727541) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 1
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test_load_headers_from_storage::checkpoint_60000_blocks stdout ----

thread 'test_load_headers_from_storage::checkpoint_60000_blocks' (8727542) panicked at dash-spv/tests/header_sync_test.rs:381:5:
assertion `left == right` failed: Chain state count mismatch
  left: 60000
 right: 120000

---- test_load_headers_from_storage::genesis_60000_blocks stdout ----

thread 'test_load_headers_from_storage::genesis_60000_blocks' (8727545) panicked at dash-spv/tests/header_sync_test.rs:380:5:
assertion `left == right` failed: Loaded count mismatch
  left: 59999
 right: 60000

* Stop loading headers twice into `ChainState::headers`

We currently load the headers twice into the header vector in `HeaderSyncManager::load_headers_from_storage`:

- First1. within `storage.load_chain_state`
- And then within `load_headers_from_storage` itself further down

```
2025-12-04T16:05:36.717779Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:05:36.727966Z  INFO ✅ Loaded 2 headers into HeaderSyncManager in 0.00s (1129 headers/sec)
....

2025-12-05T04:31:15.980982Z ERROR Sequential sync manager error handling message: Validation error: Block at height 2300000 does not match checkpoint
```

```
2025-12-04T16:09:37.959349Z  INFO 🚀 Starting sync from checkpoint at height 1900000 instead of genesis (requested start height: 2200000)
2025-12-04T16:09:37.966703Z  INFO ✅ Loaded 1 headers into HeaderSyncManager in 0.00s (862 headers/sec)
´´´
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants