ChainState fiedls removal #289
Conversation
WalkthroughRemoved fork detection, reorganization management, the synchronous in-memory ChainStorage implementation, and their associated tests and public exports across the chain and storage modules. Multiple structs, traits, methods, and test files were deleted. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
dash-spv/**/*.rs📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Files:
dash-spv/src/sync/**/*.rs📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-12-16T09:03:55.811ZApplied to files:
📚 Learning: 2025-12-16T09:03:55.811ZApplied to files:
📚 Learning: 2025-12-01T08:01:18.174ZApplied to files:
📚 Learning: 2025-12-16T09:03:55.811ZApplied to files:
⏰ 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). (5)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
dash-spv/src/chain/fork_detector.rs (3)
1-4: Module documentation is stale.The docstring states this module "detects when incoming headers create a fork," but the detection logic (
check_header) has been removed. Update the documentation to reflect that this is now a fork storage/query utility rather than a detection module.🔎 Suggested documentation update
-//! Fork detection logic for identifying blockchain forks +//! Fork storage and query utilities for blockchain forks //! -//! This module detects when incoming headers create a fork in the blockchain -//! rather than extending the current chain tip. +//! This module provides storage and query operations for known blockchain forks.
57-68: Remove unusedForkDetectionResultenum.This enum is defined and publicly exported but no method in
ForkDetectorreturns it, and it is not used anywhere in the codebase. Remove it if no external code depends on it.
17-24: Remove unusedmax_forksparameter or implement fork limit enforcement.The
max_forksparameter is validated but never stored or used. The struct contains only aforksHashMap with no correspondingmax_forksfield. Either remove the parameter entirely if fork limits are not needed, or store the limit and enforce it when adding forks.Additionally,
ForkDetectionResultenum is defined but never used anywhere in the codebase and should be removed unless future implementations require it.
🧹 Nitpick comments (2)
dash-spv/src/chain/mod.rs (1)
1-8: Consider updating module documentation to reflect simplified functionality.The module docstring describes comprehensive chain management features (fork detection, reorganization, etc.), but with the removal of
ReorgManagerand simplification ofForkDetector, some of these capabilities have been reduced. Consider revising the documentation to match current functionality.dash-spv/src/chain/fork_detector.rs (1)
74-79: Test validates unused functionality.This test verifies the
max_forks == 0error, but sincemax_forksis never stored or enforced, this validation has no practical effect. If the parameter is removed per the earlier suggestion, this test should also be removed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dash-spv/src/chain/fork_detector.rs(1 hunks)dash-spv/src/chain/fork_detector_test.rs(0 hunks)dash-spv/src/chain/mod.rs(1 hunks)dash-spv/src/chain/reorg.rs(0 hunks)dash-spv/src/chain/reorg_test.rs(0 hunks)dash-spv/src/storage/mod.rs(0 hunks)dash-spv/src/storage/sync_storage.rs(0 hunks)
💤 Files with no reviewable changes (5)
- dash-spv/src/storage/mod.rs
- dash-spv/src/chain/fork_detector_test.rs
- dash-spv/src/chain/reorg_test.rs
- dash-spv/src/storage/sync_storage.rs
- dash-spv/src/chain/reorg.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/mod.rsdash-spv/src/chain/fork_detector.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/mod.rsdash-spv/src/chain/fork_detector.rs
🧠 Learnings (16)
📚 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/src/chain/mod.rsdash-spv/src/chain/fork_detector.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/chain/mod.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/chain/mod.rsdash-spv/src/chain/fork_detector.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/src/chain/mod.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/src/chain/mod.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/chain/mod.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/mod.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/chain/mod.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/src/chain/mod.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/mod.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/src/chain/mod.rsdash-spv/src/chain/fork_detector.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/chain/fork_detector.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/src/chain/fork_detector.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/src/chain/fork_detector.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/src/chain/fork_detector.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 : Write integration tests for network operations in Rust
Applied to files:
dash-spv/src/chain/fork_detector.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). (19)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Core Components Tests
- GitHub Check: Key Wallet Components Tests
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: SPV Components Tests
🔇 Additional comments (1)
dash-spv/src/chain/mod.rs (1)
29-29: LGTM - API surface reduction is consistent with PR objectives.The removal of
ReorgManagerfrom public exports aligns with the stated goal of removing ChainStorage and related code.ReorgEventremains available for external consumers who need to react to reorganization events.
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. |
ZocoLini
changed the title
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv/src/sync/headers/manager.rs (2)
1-1: Update file documentation to reflect fork detection removal.The file header comment mentions "fork detection and reorganization handling," but the
ForkDetectorhas been removed from this module as part of this PR. Update the documentation to accurately reflect the current functionality.🔎 Proposed fix
-//! Header synchronization with fork detection and reorganization handling. +//! Header synchronization with reorganization handling.
284-286: Remove outdated comment about fork detection.This comment suggests that fork detection is "temporarily disabled" and needs to be implemented in production. However, per the PR objectives, fork detection has been permanently removed from the codebase. This comment should be removed to avoid confusing future maintainers.
🔎 Proposed fix
- // Note: Fork detection is temporarily disabled for batch processing - // In a production implementation, we would need to handle fork detection - // at the batch level or in a separate phase -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dash-spv/src/chain/fork_detector.rs(0 hunks)dash-spv/src/chain/mod.rs(0 hunks)dash-spv/src/chain/reorg.rs(0 hunks)dash-spv/src/sync/headers/manager.rs(1 hunks)
💤 Files with no reviewable changes (3)
- dash-spv/src/chain/reorg.rs
- dash-spv/src/chain/mod.rs
- dash-spv/src/chain/fork_detector.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/src/sync/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/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 (5)
📓 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: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
📚 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/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/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/headers/manager.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/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/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/src/sync/headers/manager.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). (5)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: SPV Components Tests
- GitHub Check: Core Components Tests
🔇 Additional comments (2)
dash-spv/src/sync/headers/manager.rs (2)
838-844: LGTM!The
reset_pending_requestsmethod correctly no longer resets anyForkDetectorstate, which is consistent with its removal from the struct.
10-10: VerifyForkDetectorremoval doesn't break dependent code.The import change correctly removes
ForkDetectorfrom the chain module imports. Ensure that no other parts of the codebase depend onForkDetectorbeing available throughHeaderSyncManager.Run the following script to verify that
ForkDetectoris not referenced elsewhere in the codebase:#!/bin/bash # Description: Search for any remaining references to ForkDetector in the codebase # Search for ForkDetector usage (excluding removed files mentioned in PR) rg -nP '\bForkDetector\b' \ -g '!dash-spv/src/chain/fork_detector.rs' \ -g '!dash-spv/src/chain/fork_detector_test.rs' \ -C3
Was working on removing headers from
ChainStatebcs we should use theDiskStorageManagerfor that and found thisChainStoragebeing part of the code where those headers where being used. It turns out that the method whereChainState.headersandChainStoragewhere being used was only tested so I drop the hole thing and related code that become unused after that so I can continue working on theChainStatefields removalSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.