Skip to content

chore: replaced wildcard with exhaustive SyncPhase match#239

Merged
xdustinface merged 1 commit intov0.41-devfrom
chore/sync-phase-exhaustive-match
Dec 11, 2025
Merged

chore: replaced wildcard with exhaustive SyncPhase match#239
xdustinface merged 1 commit intov0.41-devfrom
chore/sync-phase-exhaustive-match

Conversation

@ZocoLini
Copy link
Collaborator

@ZocoLini ZocoLini commented Dec 4, 2025

With his change we force everybody to update the matches if a new SyncPhase is created

Summary by CodeRabbit

  • Refactor

    • Internal sync-phase handling made explicit and more exhaustive to improve maintainability and future-proof behavior. No functional or user-visible changes.
  • Chores

    • Clarified progress and timeout handling paths internally; behavior remains unchanged for end users.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

Walkthrough

Replaced wildcard (_) match arms with explicit SyncPhase variant matches across sync modules, making no-op and timeout/recovery branches enumerate Idle, DownloadingFilters, DownloadingBlocks, and FullySynced explicitly while preserving existing behavior.

Changes

Cohort / File(s) Summary
Explicit SyncPhase pattern matching
dash-spv/src/sync/phase_execution.rs, dash-spv/src/sync/phases.rs
Replaced catch-all (_) match arms with explicit SyncPhase variant branches in: execute_current_phase, check_timeout, recover_from_timeout (phase_execution.rs) and last_progress_time, update_progress, progress computation (phases.rs). Behavior unchanged; no-op/timeouts/recovery now enumerate Idle, DownloadingFilters, DownloadingBlocks, FullySynced explicitly for exhaustiveness and future-proofing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect each updated match to ensure all current SyncPhase variants are covered and intentions for no-op branches are preserved.
  • Verify progress computation changes return the intended zeroed PhaseProgress for inactive phases.
  • Confirm no unreachable code or logic regressions introduced by more explicit matching.

Poem

I nibble on patterns, tidy and spry,
Wildcards replaced — now each branch meets my eye.
Explicit and neat, no surprises remain,
Hop into clarity, code clean as a grain.
🐇✨

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 summarizes the main change: replacing wildcard patterns with exhaustive SyncPhase matching for improved code maintainability.
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
  • Commit unit tests in branch chore/sync-phase-exhaustive-match

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

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

500-511: Clarify comment for semantic accuracy.

The comment "For other phases, we'll need phase-specific recovery" groups Idle and FullySynced (which are inactive and don't need recovery) with DownloadingFilters and DownloadingBlocks (which may need future recovery logic). Consider clarifying:

-            | SyncPhase::FullySynced {
-                ..
-            } => {
-                // For other phases, we'll need phase-specific recovery
+            | SyncPhase::FullySynced {
+                ..
+            } => {
+                // Idle/FullySynced don't need recovery; DownloadingFilters/DownloadingBlocks need future implementation
             }

Note: DownloadingFilters has extensive custom timeout handling in check_timeout() (lines 349-442) which executes before this method is called, so the lack of recovery here may be intentional.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1035c62 and 82f10d8.

📒 Files selected for processing (2)
  • dash-spv/src/sync/sequential/phase_execution.rs (3 hunks)
  • dash-spv/src/sync/sequential/phases.rs (2 hunks)
🧰 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/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.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/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.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/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
Learning: Implement sequential sync with each phase completing before the next begins to ensure consistency and simplify error recovery
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 asyncThrowingStream for blockchain synchronization with streaming API (syncProgressStream()) instead of callback-based approach when possible
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
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/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.rs
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
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/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.rs
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
Learning: Implement sequential sync with each phase completing before the next begins to ensure consistency and simplify error recovery

Applied to files:

  • dash-spv/src/sync/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.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/**/address_pool/**/*.rs : Implement staged gap limit management using `GapLimitStage` structure with tracking of last_used_index and used_indices HashSet to enable efficient address discovery without loading entire address chains into memory

Applied to files:

  • dash-spv/src/sync/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.rs
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
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/src/sync/sequential/phase_execution.rs
  • dash-spv/src/sync/sequential/phases.rs
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
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/src/sync/sequential/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
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/src/sync/sequential/phase_execution.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.

Applied to files:

  • dash-spv/src/sync/sequential/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.617Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.617Z
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/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). (20)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: Key Wallet Components Tests
  • GitHub Check: Core Components Tests
  • GitHub Check: SPV Components Tests
  • GitHub Check: Pre-commit (ubuntu-latest)
  • GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (4)
dash-spv/src/sync/sequential/phases.rs (2)

173-176: LGTM! Exhaustive pattern matching for inactive phases.

The explicit handling of Idle and FullySynced correctly returns None for phases that have no progress tracking. This change ensures the compiler will flag any future SyncPhase additions that aren't explicitly handled.


204-207: LGTM! Consistent exhaustive pattern for progress updates.

The explicit no-op for Idle and FullySynced is correct since these phases lack a last_progress field. This aligns with the exhaustive matching goal of the PR.

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

199-204: LGTM! Exhaustive handling for inactive phases.

The explicit pattern correctly handles Idle and FullySynced as no-ops in the execution phase. This maintains the existing behavior while ensuring compile-time exhaustiveness checks.


443-451: LGTM! Exhaustive timeout check patterns.

The explicit handling groups Idle, FullySynced, and DownloadingBlocks appropriately. Note that DownloadingBlocks currently has minimal implementation (as indicated by line 196's comment "For now, we'll complete the sync"), so the no-op timeout handling is consistent.

@ZocoLini ZocoLini force-pushed the chore/sync-phase-exhaustive-match branch from 82f10d8 to 61bbb3a Compare December 10, 2025 20:49
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

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

430-439: Inconsistent wildcard usage defeats PR objective.

The progress() method still uses a wildcard catch-all at Line 430, which is inconsistent with the PR's stated goal of exhaustive matching. When new SyncPhase variants are added, this wildcard will silently catch them without forcing explicit consideration.

Apply this diff to make the match exhaustive:

-            _ => PhaseProgress {
+            SyncPhase::Idle | SyncPhase::FullySynced { .. } => PhaseProgress {
                 phase_name: self.name(),
                 items_completed: 0,
                 items_total: None,
                 percentage: 0.0,
                 rate: 0.0,
                 eta: None,
                 elapsed: Duration::from_secs(0),
             },
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82f10d8 and 61bbb3a.

📒 Files selected for processing (2)
  • dash-spv/src/sync/phase_execution.rs (3 hunks)
  • dash-spv/src/sync/phases.rs (2 hunks)
🧰 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/phases.rs
  • dash-spv/src/sync/phase_execution.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/phases.rs
  • dash-spv/src/sync/phase_execution.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/phases.rs
  • dash-spv/src/sync/phase_execution.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Implement sequential sync with each phase completing before the next begins to ensure consistency and simplify error recovery
📚 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/phases.rs
  • dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase with tokio runtime for all asynchronous operations

Applied to files:

  • dash-spv/src/sync/phases.rs
  • dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with `cargo check --all-features`

Applied to files:

  • dash-spv/src/sync/phases.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/sync/phases.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/**/transaction_checking/**/*.rs : In SPV sync integration, use `wallet_info.check_transaction()` with appropriate `TransactionContext` and update wallet state atomically only when `is_relevant` flag is true

Applied to files:

  • dash-spv/src/sync/phases.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/src/sync/phases.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: Implement sequential sync with each phase completing before the next begins to ensure consistency and simplify error recovery

Applied to files:

  • dash-spv/src/sync/phases.rs
  • dash-spv/src/sync/phase_execution.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). (20)
  • GitHub Check: SPV Components Tests
  • GitHub Check: Core Components Tests
  • GitHub Check: RPC Tests (stable, true)
  • GitHub Check: fuzz (dash_deserialize_address)
  • GitHub Check: fuzz (hashes_sha256)
  • GitHub Check: fuzz (hashes_sha1)
  • GitHub Check: fuzz (hashes_ripemd160)
  • GitHub Check: fuzz (dash_deserialize_script)
  • GitHub Check: fuzz (dash_deserialize_witness)
  • GitHub Check: fuzz (hashes_cbor)
  • GitHub Check: fuzz (dash_deserialize_block)
  • GitHub Check: fuzz (dash_outpoint_string)
  • GitHub Check: fuzz (dash_deserialize_amount)
  • GitHub Check: fuzz (hashes_sha512)
  • GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
  • GitHub Check: fuzz (hashes_json)
  • GitHub Check: fuzz (hashes_sha512_256)
  • GitHub Check: fuzz (dash_deser_net_msg)
  • GitHub Check: Pre-commit (ubuntu-latest)
  • GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (5)
dash-spv/src/sync/phases.rs (2)

173-176: Exhaustive match improves maintainability.

The explicit match for Idle and FullySynced correctly replaces the wildcard. This ensures that any future SyncPhase variants must be explicitly considered here, preventing unintended fallthrough.


204-207: Exhaustive match improves maintainability.

The explicit no-op for Idle and FullySynced correctly replaces the wildcard, ensuring compile-time checks when new variants are added.

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

188-193: Exhaustive match improves maintainability.

The explicit no-op for Idle and FullySynced correctly replaces the wildcard. These phases have no execution logic by design, and the explicit match ensures future variants are handled.


428-436: Exhaustive match improves maintainability.

The explicit no-op for Idle, FullySynced, and DownloadingBlocks correctly replaces the wildcard. The DownloadingBlocks phase currently has no timeout handling, which aligns with its minimal implementation in execute_current_phase (Lines 179-186).


481-490: Exhaustive match improves maintainability.

The explicit listing of phases without recovery logic correctly replaces the wildcard. Note that DownloadingFilters recovery is handled separately in check_timeout (Lines 334-427), which explains why it falls through here.

@ZocoLini ZocoLini force-pushed the chore/sync-phase-exhaustive-match branch from 61bbb3a to 6b78c6d Compare December 10, 2025 20:56
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

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

430-441: Idle/FullySynced PhaseProgress remains neutral; consider special‑casing FullySynced (optional)

The new explicit arm for SyncPhase::Idle | SyncPhase::FullySynced { .. } returns a neutral PhaseProgress (0 items, 0%, zero elapsed), which matches the former wildcard behavior and is safe. If, in the future, consumers would benefit from FullySynced reporting percentage = 100.0 and a real elapsed based on total_sync_time, you could split that variant out and compute a more meaningful summary—purely as a UX improvement, not a functional bug.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61bbb3a and 6b78c6d.

📒 Files selected for processing (2)
  • dash-spv/src/sync/phase_execution.rs (3 hunks)
  • dash-spv/src/sync/phases.rs (3 hunks)
🧰 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/phases.rs
  • dash-spv/src/sync/phase_execution.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/phases.rs
  • dash-spv/src/sync/phase_execution.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/phases.rs
  • dash-spv/src/sync/phase_execution.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Synchronization should use `SyncManager` for phase-based coordination: Phase 1 (Headers), Phase 2 (Masternode List), Phase 3 (Filter Headers), Phase 4 (Filters), Phase 5 (Blocks)
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Implement sequential sync with each phase completing before the next begins to ensure consistency and simplify error recovery
📚 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/phases.rs
  • dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase with tokio runtime for all asynchronous operations

Applied to files:

  • dash-spv/src/sync/phases.rs
  • dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/**/*.rs : Use Minimum Rust Version (MSRV) 1.89 and ensure all code compiles with `cargo check --all-features`

Applied to files:

  • dash-spv/src/sync/phases.rs
  • dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Implement sequential sync with each phase completing before the next begins to ensure consistency and simplify error recovery

Applied to files:

  • dash-spv/src/sync/phases.rs
  • dash-spv/src/sync/phase_execution.rs
📚 Learning: 2025-12-04T15:51:59.632Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-04T15:51:59.632Z
Learning: Applies to dash-spv/src/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/src/sync/phases.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/src/sync/phases.rs
🔇 Additional comments (4)
dash-spv/src/sync/phases.rs (1)

173-176: Explicit Idle/FullySynced handling in timeout/progress updates looks correct

The new explicit SyncPhase::Idle | SyncPhase::FullySynced { .. } arms in last_progress_time (Lines [173]-[176]) and update_progress (Lines [204]-[207]) preserve the previous _ behavior (no timeout source and no-op updates) while making the match exhaustive. This cleanly meets the PR goal of forcing updates when new SyncPhase variants are added.

Also applies to: 204-207

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

188-193: No-op handling for Idle/FullySynced in execute_current_phase is appropriate

The explicit SyncPhase::Idle | SyncPhase::FullySynced { .. } => { /* nothing */ } arm makes it clear we do not run any execution logic when not actively syncing, while replacing the previous wildcard. This keeps behavior unchanged and enforces compile-time exhaustiveness for new phases.


428-436: Explicit Idle/FullySynced/DownloadingBlocks branch in check_timeout matches prior behavior

Treating Idle, FullySynced, and DownloadingBlocks as “no phase-specific timeout checks” in check_timeout preserves the old wildcard semantics while making the match exhaustive. Generic phase timeouts are still driven by last_progress_time(), so nothing regresses here; any future phase additions will now force an explicit decision.


481-490: Exhaustive non-recovery branch in recover_from_timeout is consistent

The explicit grouping of Idle, DownloadingFilters, DownloadingBlocks, and FullySynced into the “no phase-specific recovery yet” arm maintains the previous catch-all behavior while ensuring new SyncPhase variants can’t silently fall through. Filter-specific recovery remains handled in check_timeout, so this change is consistent with the existing design.

@xdustinface xdustinface merged commit ab2e851 into v0.41-dev Dec 11, 2025
24 checks passed
@ZocoLini ZocoLini deleted the chore/sync-phase-exhaustive-match branch December 11, 2025 01:42
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