You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(dash-spv): validate block headers as they are received - #1304
Block headers were validated only when a finished segment was stored. A batch that did not match a segment reached fork_height unchecked, so a single header that did not even meet its own bits could make the client truncate everything above a stored block.
Changes
handle_headers_pipeline validates every batch once, before routing it: continuity, PoW and DGW difficulty. An invalid batch bans its peer and returns the error.
The ancestors come from storage when the parent is stored, which covers the tip and forks, or else from the segment whose tip it extends: the checkpoint the segment starts at plus its last buffered headers (HeadersPipeline::segment_ancestors). Segments that are not stored yet keep their whole buffer, so nothing new is tracked.
store_headers only stores.
A batch whose parent is neither stored nor a segment tip is not validated, as before nothing follows from it.
Tests
test_received_headers_are_validated: valid and invalid headers on the tip and on a fork. Invalid ones are not stored, raise no ForkDetected, and ban the peer.
test_segment_ancestors: checkpoint plus buffered headers, a segment above a checkpoint it does not start at, and an unknown hash.
Issues
Closes#1193. Covers the block-header half of #1290 and the unvalidated fork of #1103, and stops an invalid batch from entering a segment (#1194). The rest of those stays open.
Bots — coderabbitai skipped after its own rate limit
Self-review — post /self-reviewed
Build green
Approvals
dash-spv (dash-spv/src/sync/block_headers/manager.rs, dash-spv/src/sync/block_headers/pipeline.rs, dash-spv/src/sync/block_headers/segment_state.rs) — approved by xdustinface
When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.
Summary by CodeRabbit
Bug Fixes
Improved block header synchronization when headers extend the current chain or branch from an existing point, including around checkpoints.
Invalid headers with a known chain attachment are rejected, and peers that submit them are restricted. This helps prevent invalid headers from affecting the chain tip or creating a fork.
Headers were validated only when a finished segment was stored, so a fork
was detected and followed on headers nobody had checked. Validate every
batch once in `handle_headers_pipeline`, against the stored block or the
segment tip it extends, and ban the peer that sent an invalid one.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
Reviewing files that changed from the base of the PR and between 45c80c0 and 8030985.
Files selected for processing: 3
dash-spv/src/sync/block_headers/manager.rs
dash-spv/src/sync/block_headers/pipeline.rs
dash-spv/src/sync/block_headers/segment_state.rs
Walkthrough
The block-header manager now resolves ancestry for incoming batches and validates batches with a known attachment point before pipeline routing. Validation failures result in a peer ban. The headers pipeline can provide recent ancestors for a matching segment.
The manager loads ancestors from stored headers or a matching pipeline segment. The pipeline returns the segment height and its recent ancestor headers, including checkpoint headers when the segment starts at the relevant checkpoint. Tests cover ancestor selection and unmatched segment hashes.
Validate batches before routing dash-spv/src/sync/block_headers/manager.rs
The manager validates batches with a known attachment point before pipeline routing. It bans the peer and returns the error on validation failure. store_headers now stores an already validated batch. Tests cover valid and invalid tip extensions and forks.
dashpay/rust-dashcore#1290 — This PR validates block-header batches before routing and bans peers when validation fails; the CFHeaders portion described by the issue is outside this change.
Possibly related PRs
dashpay/rust-dashcore#921 — It adds downward header synchronization and batch validation in the same manager and pipeline.
dashpay/rust-dashcore#1223 — It changes the header validator and difficulty functions used to validate received batches.
dashpay/rust-dashcore#902 — It changes the network and pipeline architecture used by the same block-header components.
Suggested reviewers:xdustinface
Merge Risk: Moderate · previous review 45c80
Header validation near a checkpoint-backed storage start can accept a fork whose difficulty bits are wrong, and the peer is not banned. This is a bounded window, but it weakens the validation this PR is meant to add. Prepend the checkpoint history before merging, or explicitly accept the gap.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: validating block headers when they are received.
Linked Issues check
[#1193] Objective: validate a branch (continuity, PoW, difficulty) before ForkDetected, using stored ancestors below the fork point, and ignore the branch and penalize the peer on failure. Evidence: i…
Out of Scope Changes check
All changed code serves the validate-before-fork objective of #1193. manager.rs: the ancestors and attach_point helpers, the validation call in handle_headers_pipeline with peer ban, the reduced store…
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check
Explanation
[#1193] Objective: validate a branch (continuity, PoW, difficulty) before ForkDetected, using stored ancestors below the fork point, and ignore the branch and penalize the peer on failure. Evidence: in manager.rs, handle_headers_pipeline now calls attach_point(headers) before pipeline.receive_headers. attach_point finds the parent by hash in storage and loads ancestors with ancestors(height), which reads the stored range from height - (DGW_PAST_BLOCKS - 1) to height. BlockHeaderValidator::validate runs on those ancestors. On error, the code calls network.ban_peer(peer) and returns Err, so the batch never reaches pipeline routing or ForkDetected. store_headers now only stores, so headers are no longer validated after fork detection. test_received_headers_are_validated covers valid and invalid headers on the tip and on a fork. It asserts no ForkDetected on failure, no storage change, and a ban of the peer. Segment-tip batches use HeadersPipeline::segment_ancestors (checkpoint plus last DGW_PAST_BLOCKS buffered headers). Pipeline receive_headers buffers only batches whose prev hash matches a segment tip, so every buffered header passes through attach_point before storage. Residual uncertainty, not part of the #1193 verdict: segment_ancestors takes the first segment whose tip matches, without the complete and non-tip skip that receive_headers applies. Whether this changes results at a checkpoint boundary depends on SegmentState::matches and checkpoint header counts, which were not verified. #1290 and #1103 are not directly linked, and their remaining scope is not required here.
Full details: Out of Scope Changes check
Explanation
All changed code serves the validate-before-fork objective of #1193. manager.rs: the ancestors and attach_point helpers, the validation call in handle_headers_pipeline with peer ban, the reduced store_headers, the removed #[cfg(test)] on the Header import (needed because non-test code now uses Header), and test_received_headers_are_validated. pipeline.rs: segment_ancestors and its use of DGW_PAST_BLOCKS, the unconditional Header import (same reason), and test_segment_ancestors, which covers the new helper. No unrelated behavior, files, or refactors appear in the diff summary or in the two file diffs inspected. The PR description's claim of partial coverage for #1290 and #1103 does not add out-of-scope code.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 2 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Reviewing files that changed from the base of the PR and between 2900e46 and 45c80c0.
Files selected for processing: 2
dash-spv/src/sync/block_headers/manager.rs
dash-spv/src/sync/block_headers/pipeline.rs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bot-review-skippedA required review bot did not report; it was skipped by the window or by a person.waiting-self-reviewWaiting for the author to post /self-reviewed
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
the
waiting-bots
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Problem
Block headers were validated only when a finished segment was stored. A batch that did not match a segment reached
fork_heightunchecked, so a single header that did not even meet its ownbitscould make the client truncate everything above a stored block.Changes
handle_headers_pipelinevalidates every batch once, before routing it: continuity, PoW and DGW difficulty. An invalid batch bans its peer and returns the error.HeadersPipeline::segment_ancestors). Segments that are not stored yet keep their whole buffer, so nothing new is tracked.store_headersonly stores.A batch whose parent is neither stored nor a segment tip is not validated, as before nothing follows from it.
Tests
test_received_headers_are_validated: valid and invalid headers on the tip and on a fork. Invalid ones are not stored, raise noForkDetected, and ban the peer.test_segment_ancestors: checkpoint plus buffered headers, a segment above a checkpoint it does not start at, and an unknown hash.Issues
Closes #1193. Covers the block-header half of #1290 and the unvalidated fork of #1103, and stops an invalid batch from entering a segment (#1194). The rest of those stays open.
🤖 Generated with Claude Code
PR Hygiene ·
8030985/self-revieweddash-spv(dash-spv/src/sync/block_headers/manager.rs,dash-spv/src/sync/block_headers/pipeline.rs,dash-spv/src/sync/block_headers/segment_state.rs) — approved by xdustinfaceWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit