Skip to content

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Nov 25, 2025

Just drops lots of unused code.

Summary by CodeRabbit

  • Revert
    • Removed header synchronization, chainlock validation, validation engine/state, sequential request controller, progress tracking, and error-recovery subsystems.
    • Simplified sequential sync by removing hybrid masternode sync strategy and related APIs.
    • Removed high-level sync state and related public sync components.
  • Tests
    • Deleted multiple unit/integration test suites covering validation, headers, recovery, and error handling.
  • Documentation
    • Minor module documentation update.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

Walkthrough

This pull request deletes multiple synchronization subsystems and related tests from the dash-spv crate, removing modules for chainlock validation, header sync, validation engines/state, sequential request control/recovery/progress, and associated public types and tests.

Changes

Cohort / File(s) Change Summary
Core sync module removals
dash-spv/src/sync/chainlock_validation.rs, dash-spv/src/sync/headers.rs, dash-spv/src/sync/validation.rs, dash-spv/src/sync/validation_state.rs
Entire modules deleted, removing chain lock validation (including cache/stats), header synchronization orchestration, validation engine with caching/stats, and snapshot-based validation state management with rollback and listeners.
Sync module structure updates
dash-spv/src/sync/mod.rs
Removed public module exports (chainlock_validation, discovery, headers, state, validation, validation_state), re-exports (HeaderSyncManager, SyncState), SyncComponent enum, and test module validation_test.
Sequential sync subsystem removals
dash-spv/src/sync/sequential/progress.rs, dash-spv/src/sync/sequential/recovery.rs, dash-spv/src/sync/sequential/request_control.rs
Entire modules deleted: overall progress tracking (ETA/weights), error recovery (strategies, checkpoints, backoff, stats), and phase-aware request controller (queuing, rate limits, timeouts, stats).
Sequential sync module structure
dash-spv/src/sync/sequential/mod.rs
Removed public submodules progress, recovery, request_control and re-export of RequestController.
Sequential sync lifecycle/manager updates
dash-spv/src/sync/sequential/lifecycle.rs, dash-spv/src/sync/sequential/manager.rs
Removed request_controller field/import/initialization, removed reset_to_idle() and clear_pending_requests() call, removed phase_history() and get_quorum_manager() accessors and some test-only methods.
Sync phases refinement
dash-spv/src/sync/sequential/phases.rs, dash-spv/src/sync/sequential/transitions.rs
Removed HybridSyncStrategy and hybrid-related fields/logic from DownloadingMnList phase, eliminated elapsed-time accessor and hybrid progress formatting; removed explicit sync_strategy: None initializer in transitions.
Discovery module pruning
dash-spv/src/sync/discovery.rs
Removed simplified QRInfoRequest struct and its constructor, plus related imports/documentation.
Test file removals
dash-spv/src/sync/validation_test.rs, dash-spv/tests/header_sync_test.rs, dash-spv/tests/error_handling_test.rs, dash-spv/tests/error_recovery_integration_test.rs
Deleted multiple test modules and test cases, including validation integration tests, header sync manager tests, comprehensive error handling tests with mocks, and error-recovery integration tests.
Documentation update
dash-spv/src/client/mod.rs
Removed progress.rs reference from the module structure documentation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Files requiring extra attention:
    • Core deletions in sync (headers, validation, validation_state, chainlock_validation) for dangling references elsewhere.
    • Sequential layer changes: removal of request control, recovery, and progress; verify lifecycle/manager integrations.
    • Tests removed across multiple suites — assess coverage gaps and CI impact.

Poem

🐰 I hopped through code with nimble paws,

Found syncing nests and cleared the halls,
Chains and headers, caches gone to rest,
Requests and recoveries un-dressed,
A quiet burrow — time for carrots and naps. 🥕

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 PR title accurately describes the primary change: removal of unused synchronization code across multiple modules and subsystems.
Docstring Coverage ✅ Passed Docstring coverage is 90.91% 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
  • Commit unit tests in branch chore/unused-sync

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32a8d98 and aef4a0b.

📒 Files selected for processing (20)
  • dash-spv/src/client/mod.rs (0 hunks)
  • dash-spv/src/sync/chainlock_validation.rs (0 hunks)
  • dash-spv/src/sync/discovery.rs (0 hunks)
  • dash-spv/src/sync/headers.rs (0 hunks)
  • dash-spv/src/sync/mod.rs (0 hunks)
  • dash-spv/src/sync/sequential/lifecycle.rs (0 hunks)
  • dash-spv/src/sync/sequential/manager.rs (0 hunks)
  • dash-spv/src/sync/sequential/mod.rs (0 hunks)
  • dash-spv/src/sync/sequential/phases.rs (1 hunks)
  • dash-spv/src/sync/sequential/progress.rs (0 hunks)
  • dash-spv/src/sync/sequential/recovery.rs (0 hunks)
  • dash-spv/src/sync/sequential/request_control.rs (0 hunks)
  • dash-spv/src/sync/sequential/transitions.rs (0 hunks)
  • dash-spv/src/sync/state.rs (0 hunks)
  • dash-spv/src/sync/validation.rs (0 hunks)
  • dash-spv/src/sync/validation_state.rs (0 hunks)
  • dash-spv/src/sync/validation_test.rs (0 hunks)
  • dash-spv/tests/error_handling_test.rs (0 hunks)
  • dash-spv/tests/error_recovery_integration_test.rs (0 hunks)
  • dash-spv/tests/header_sync_test.rs (0 hunks)
💤 Files with no reviewable changes (19)
  • dash-spv/src/sync/discovery.rs
  • dash-spv/src/client/mod.rs
  • dash-spv/src/sync/validation_test.rs
  • dash-spv/tests/error_handling_test.rs
  • dash-spv/tests/error_recovery_integration_test.rs
  • dash-spv/tests/header_sync_test.rs
  • dash-spv/src/sync/headers.rs
  • dash-spv/src/sync/validation.rs
  • dash-spv/src/sync/state.rs
  • dash-spv/src/sync/sequential/transitions.rs
  • dash-spv/src/sync/sequential/manager.rs
  • dash-spv/src/sync/chainlock_validation.rs
  • dash-spv/src/sync/sequential/lifecycle.rs
  • dash-spv/src/sync/sequential/recovery.rs
  • dash-spv/src/sync/sequential/progress.rs
  • dash-spv/src/sync/mod.rs
  • dash-spv/src/sync/sequential/mod.rs
  • dash-spv/src/sync/validation_state.rs
  • dash-spv/src/sync/sequential/request_control.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 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/sequential/phases.rs
dash-spv/src/**/*.rs

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

dash-spv/src/**/*.rs: Use trait-based abstractions for swappable implementations (e.g., NetworkManager, StorageManager)
Use async/await throughout the codebase with tokio runtime
Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) for error handling
Use trait objects (Arc<dyn StorageManager>, Arc<dyn NetworkManager>) for runtime polymorphism
Use Tokio channels for inter-component message passing
Add comprehensive unit tests in-module for individual components

Files:

  • dash-spv/src/sync/sequential/phases.rs
dash-spv/src/sync/**/*.rs

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

dash-spv/src/sync/**/*.rs: Use sequential phase-based synchronization with SequentialSyncManager for organized sync phases
Use state machines with SyncState enum to drive synchronization flow and state transitions

Files:

  • dash-spv/src/sync/sequential/phases.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use sequential phase-based synchronization with `SequentialSyncManager` for organized sync phases
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/**/*.rs : Add comprehensive unit tests in-module for individual components
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/tests/**/*.rs : Add integration tests in the `tests/` directory for comprehensive test suites
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/tests/**/*.rs : Add real network tests with live Dash Core node integration that gracefully handle node unavailability
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.311Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use state machines with `SyncState` enum to drive synchronization flow and state transitions
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: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/types.rs : Define common data structures in `types.rs` including `SyncProgress`, `ValidationMode`, `WatchItem`, etc.
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/**/*.rs : Use async/await throughout the codebase with tokio runtime
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/**/*.rs : Use trait-based abstractions for swappable implementations (e.g., `NetworkManager`, `StorageManager`)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Maintain modular validation with configurable validation modes (None/Basic/Full) in `ValidationMode`
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use sequential phase-based synchronization with `SequentialSyncManager` for organized sync phases

Applied to files:

  • dash-spv/src/sync/sequential/phases.rs
📚 Learning: 2025-12-01T08:00:17.311Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.311Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use state machines with `SyncState` enum to drive synchronization flow and state transitions

Applied to files:

  • dash-spv/src/sync/sequential/phases.rs
📚 Learning: 2025-12-01T08:00:17.310Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:17.310Z
Learning: Applies to dash-spv/src/types.rs : Define common data structures in `types.rs` including `SyncProgress`, `ValidationMode`, `WatchItem`, etc.

Applied to files:

  • dash-spv/src/sync/sequential/phases.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: SPV Components Tests
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: Pre-commit (ubuntu-latest)
  • GitHub Check: Pre-commit (macos-latest)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (1)
dash-spv/src/sync/sequential/phases.rs (1)

119-143: Masternode list phase naming and progress logic look consistent.

The simplified name() for DownloadingMnList and the request/height-based progress calculation (with sane guards against division by zero and saturating arithmetic) are coherent and align with a non-hybrid sync model.

Also applies to: 269-311


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: 0

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/sequential/phases.rs (1)

46-51: Remove orphaned requests_total and requests_completed fields.

Verification confirms these fields are never mutated anywhere in the codebase—only initialized to 0 in dash-spv/src/sync/sequential/transitions.rs:222-223. The conditional logic in dash-spv/src/sync/sequential/phases.rs:278-296 that references these fields checks if requests_total > 0 and if requests_completed < requests_total, but both conditions are always false since the fields remain 0. This creates dead code branches that will never execute.

In contrast, diffs_processed is actively incremented in dash-spv/src/sync/sequential/message_handlers.rs:421,487 and properly used in progress calculations, so it should remain.

Remove the unused fields and simplify the progress calculation logic to eliminate dead code paths.

🧹 Nitpick comments (1)
dash-spv/src/sync/sequential/phases.rs (1)

278-286: Consider simplifying progress calculation if request tracking is no longer used.

The progress calculation includes both request-based (lines 278-280) and height-based fallback logic (lines 280-286). If the request fields (requests_total, requests_completed) are no longer actively maintained after removing the hybrid sync strategy, the request-based path may never execute, and the fallback becomes the primary logic. Consider simplifying this to use only height-based progress if request tracking is obsolete.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 939d95f and c003297.

📒 Files selected for processing (11)
  • dash-spv/src/client/mod.rs (0 hunks)
  • dash-spv/src/sync/sequential/lifecycle.rs (0 hunks)
  • dash-spv/src/sync/sequential/manager.rs (0 hunks)
  • dash-spv/src/sync/sequential/mod.rs (0 hunks)
  • dash-spv/src/sync/sequential/phases.rs (1 hunks)
  • dash-spv/src/sync/sequential/progress.rs (0 hunks)
  • dash-spv/src/sync/sequential/recovery.rs (0 hunks)
  • dash-spv/src/sync/sequential/request_control.rs (0 hunks)
  • dash-spv/src/sync/sequential/transitions.rs (0 hunks)
  • dash-spv/tests/error_handling_test.rs (0 hunks)
  • dash-spv/tests/error_recovery_integration_test.rs (0 hunks)
💤 Files with no reviewable changes (10)
  • dash-spv/src/sync/sequential/mod.rs
  • dash-spv/src/sync/sequential/transitions.rs
  • dash-spv/tests/error_recovery_integration_test.rs
  • dash-spv/tests/error_handling_test.rs
  • dash-spv/src/sync/sequential/lifecycle.rs
  • dash-spv/src/sync/sequential/manager.rs
  • dash-spv/src/sync/sequential/progress.rs
  • dash-spv/src/client/mod.rs
  • dash-spv/src/sync/sequential/recovery.rs
  • dash-spv/src/sync/sequential/request_control.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (1)
dash-spv/src/sync/sequential/phases.rs (1)

129-129: LGTM! Phase name simplified correctly.

The static label "Downloading Masternode Lists" is clear and aligns with the removal of hybrid-mode differentiation described in the PR.

@xdustinface xdustinface force-pushed the chore/unused-sync branch 2 times, most recently from 3253371 to 5bb43d9 Compare November 26, 2025 05:54
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 (2)
dash-spv/src/sync/filters/download.rs (2)

6-12: Align module docs with current responsibilities

The feature list still mentions “Individual filter header downloads for blocks”, but this module now focuses on queuing filter requests, requesting CFILTERs, verifying against CFHeaders, and storing headers; there’s no dedicated per-block header download helper here anymore. Consider updating or trimming this bullet so the docs accurately reflect what this module now does.


298-318: Deduplicate overlapping-headers log and fix stale comment

The “Received overlapping filter headers…” message is logged twice under the same condition (lines 298–304 and 313–318), which just doubles log noise. Also, the earlier comment still refers to verify_filter_header_chain, while the actual code uses handle_overlapping_headers. Suggest merging these two if current_filter_tip >= start_height && start_height > 0 blocks into one and updating the comment to describe handle_overlapping_headers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c003297 and 5bb43d9.

📒 Files selected for processing (37)
  • dash-spv/src/client/config.rs (0 hunks)
  • dash-spv/src/client/config_test.rs (0 hunks)
  • dash-spv/src/client/filter_sync.rs (1 hunks)
  • dash-spv/src/client/mod.rs (0 hunks)
  • dash-spv/src/network/tests.rs (0 hunks)
  • dash-spv/src/sync/chainlock_validation.rs (0 hunks)
  • dash-spv/src/sync/discovery.rs (0 hunks)
  • dash-spv/src/sync/filters/download.rs (1 hunks)
  • dash-spv/src/sync/filters/gaps.rs (0 hunks)
  • dash-spv/src/sync/filters/headers.rs (1 hunks)
  • dash-spv/src/sync/filters/manager.rs (0 hunks)
  • dash-spv/src/sync/filters/matching.rs (0 hunks)
  • dash-spv/src/sync/filters/mod.rs (0 hunks)
  • dash-spv/src/sync/filters/requests.rs (0 hunks)
  • dash-spv/src/sync/filters/retry.rs (2 hunks)
  • dash-spv/src/sync/filters/stats.rs (1 hunks)
  • dash-spv/src/sync/headers.rs (0 hunks)
  • dash-spv/src/sync/mod.rs (0 hunks)
  • dash-spv/src/sync/sequential/lifecycle.rs (0 hunks)
  • dash-spv/src/sync/sequential/manager.rs (0 hunks)
  • dash-spv/src/sync/sequential/mod.rs (0 hunks)
  • dash-spv/src/sync/sequential/phase_execution.rs (3 hunks)
  • dash-spv/src/sync/sequential/phases.rs (1 hunks)
  • dash-spv/src/sync/sequential/progress.rs (0 hunks)
  • dash-spv/src/sync/sequential/recovery.rs (0 hunks)
  • dash-spv/src/sync/sequential/request_control.rs (0 hunks)
  • dash-spv/src/sync/sequential/transitions.rs (0 hunks)
  • dash-spv/src/sync/state.rs (0 hunks)
  • dash-spv/src/sync/validation.rs (0 hunks)
  • dash-spv/src/sync/validation_state.rs (0 hunks)
  • dash-spv/src/sync/validation_test.rs (0 hunks)
  • dash-spv/tests/cfheader_gap_test.rs (0 hunks)
  • dash-spv/tests/edge_case_filter_sync_test.rs (0 hunks)
  • dash-spv/tests/error_handling_test.rs (0 hunks)
  • dash-spv/tests/error_recovery_integration_test.rs (0 hunks)
  • dash-spv/tests/header_sync_test.rs (0 hunks)
  • dash-spv/tests/simple_gap_test.rs (0 hunks)
💤 Files with no reviewable changes (30)
  • dash-spv/src/sync/filters/mod.rs
  • dash-spv/src/sync/filters/requests.rs
  • dash-spv/src/client/config_test.rs
  • dash-spv/src/sync/sequential/transitions.rs
  • dash-spv/src/sync/validation_test.rs
  • dash-spv/tests/simple_gap_test.rs
  • dash-spv/src/sync/filters/gaps.rs
  • dash-spv/tests/edge_case_filter_sync_test.rs
  • dash-spv/tests/error_recovery_integration_test.rs
  • dash-spv/src/network/tests.rs
  • dash-spv/src/sync/sequential/mod.rs
  • dash-spv/tests/error_handling_test.rs
  • dash-spv/src/client/config.rs
  • dash-spv/tests/cfheader_gap_test.rs
  • dash-spv/src/sync/headers.rs
  • dash-spv/tests/header_sync_test.rs
  • dash-spv/src/sync/discovery.rs
  • dash-spv/src/sync/sequential/lifecycle.rs
  • dash-spv/src/sync/chainlock_validation.rs
  • dash-spv/src/sync/sequential/progress.rs
  • dash-spv/src/sync/mod.rs
  • dash-spv/src/sync/state.rs
  • dash-spv/src/sync/validation_state.rs
  • dash-spv/src/client/mod.rs
  • dash-spv/src/sync/validation.rs
  • dash-spv/src/sync/sequential/manager.rs
  • dash-spv/src/sync/filters/matching.rs
  • dash-spv/src/sync/sequential/recovery.rs
  • dash-spv/src/sync/sequential/request_control.rs
  • dash-spv/src/sync/filters/manager.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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/filters/retry.rs
  • dash-spv/src/sync/filters/headers.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.

Applied to files:

  • dash-spv/src/sync/filters/stats.rs
  • dash-spv/src/sync/filters/download.rs
  • dash-spv/src/sync/filters/headers.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/sync/filters/headers.rs
🧬 Code graph analysis (1)
dash-spv/src/sync/sequential/phase_execution.rs (2)
dash-spv/src/client/core.rs (2)
  • network (166-168)
  • storage (171-173)
dash-spv/src/client/queries.rs (1)
  • network (46-49)
⏰ 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_sha256)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: Strict Warnings and Clippy Checks
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: Core Components Tests
  • GitHub Check: Key Wallet Components Tests
🔇 Additional comments (8)
dash-spv/src/sync/sequential/phases.rs (1)

127-130: Simplified DownloadingMnList name arm is correct and consistent

The name() match arm for SyncPhase::DownloadingMnList now just matches { .. } and returns "Downloading Masternode Lists". This cleanly reflects the variant after removing hybrid strategy fields and keeps the enum match exhaustive with no behavioral regressions.

dash-spv/src/sync/filters/retry.rs (1)

166-168: Simplified condition looks correct.

The guard now only checks syncing_filter_headers, which is appropriate after removing the flow-control toggle. This aligns with the PR's goal of simplifying the sync subsystem.

dash-spv/src/sync/sequential/phase_execution.rs (3)

115-130: Consolidated flow-control entry point and early transition handling.

The code now always uses start_sync_headers_with_flow_control and properly handles the case where sync doesn't start by transitioning to the next phase immediately. This is clean and maintains correct control flow.


318-322: Unified CFHeaders timeout handling.

The timeout check now consistently uses check_cfheader_request_timeouts for the DownloadingCFHeaders phase. This simplification aligns with the removal of the flow-control configuration toggle.


470-474: Recovery path mirrors timeout handling.

The recovery logic for DownloadingCFHeaders now matches the timeout check by using check_cfheader_request_timeouts, ensuring consistent behavior.

dash-spv/src/client/filter_sync.rs (1)

164-170: Correctly updated to match simplified get_flow_control_status API.

The destructuring and logging are now consistent with the updated return type (usize, usize) from get_flow_control_status(). The log message clearly communicates the queue status.

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

144-156: Clean delegation to unified flow-control handler.

The handle_cfheaders_message method now directly delegates to handle_cfheaders_with_flow_control, eliminating any branching based on configuration. This is a clean simplification that consolidates CFHeaders processing to a single code path.

dash-spv/src/sync/filters/download.rs (1)

98-102: Log message update matches the single remaining sync path

Removing the explicit “with flow control” wording from this info log makes sense now that this is the only filter-sync path; the queued/active counts are still present and useful for debugging. From my side this change looks good.

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

@xdustinface xdustinface merged commit c59518d into v0.41-dev Dec 1, 2025
38 of 40 checks passed
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.

3 participants