-
Notifications
You must be signed in to change notification settings - Fork 7
chore: Drop unused sync code #208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis 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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (20)
💤 Files with no reviewable changes (19)
🧰 Additional context used📓 Path-based instructions (3)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
dash-spv/src/**/*.rs📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Files:
dash-spv/src/sync/**/*.rs📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-12-01T08:00:17.310ZApplied to files:
📚 Learning: 2025-12-01T08:00:17.311ZApplied to files:
📚 Learning: 2025-12-01T08:00:17.310ZApplied 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). (13)
🔇 Additional comments (1)
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 |
There was a problem hiding this 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 orphanedrequests_totalandrequests_completedfields.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 indash-spv/src/sync/sequential/phases.rs:278-296that references these fields checksif requests_total > 0andif 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_processedis actively incremented indash-spv/src/sync/sequential/message_handlers.rs:421,487and 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
📒 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.
3253371 to
5bb43d9
Compare
There was a problem hiding this 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 responsibilitiesThe 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 commentThe “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 useshandle_overlapping_headers. Suggest merging these twoif current_filter_tip >= start_height && start_height > 0blocks into one and updating the comment to describehandle_overlapping_headers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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.rsdash-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.rsdash-spv/src/sync/filters/download.rsdash-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: SimplifiedDownloadingMnListname arm is correct and consistentThe
name()match arm forSyncPhase::DownloadingMnListnow 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_controland 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_timeoutsfor theDownloadingCFHeadersphase. This simplification aligns with the removal of the flow-control configuration toggle.
470-474: Recovery path mirrors timeout handling.The recovery logic for
DownloadingCFHeadersnow matches the timeout check by usingcheck_cfheader_request_timeouts, ensuring consistent behavior.dash-spv/src/client/filter_sync.rs (1)
164-170: Correctly updated to match simplifiedget_flow_control_statusAPI.The destructuring and logging are now consistent with the updated return type
(usize, usize)fromget_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_messagemethod now directly delegates tohandle_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 pathRemoving 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.
c31a565 to
c003297
Compare
c003297 to
32a8d98
Compare
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
32a8d98 to
aef4a0b
Compare
Just drops lots of unused code.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.