Repository navigation
fix(blockvalidation): check difficulty of every catch-up header before fetching blocks - #2084
Merged
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: The catch-up header precheck now calls
The cost matches the CHANGELOG: about 300 extra trunk reads per catch-up for the ~147 headers whose adjustment window reaches below the ancestor. Headers above that window are judged from the in-memory map alone. The CHANGELOG entry sits under One reviewer thread is still open. It is non-blocking and asks for a DAA fixture that uses |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-11-02 13:40 UTC |
freemans13
approved these changes
Nov 2, 2026
freemans13
left a comment
freemans13
left a comment
Collaborator
There was a problem hiding this comment.
Approving. The precheck now runs every catch-up header through the same calculator full-block validation uses, so the first ~147 headers of a fork, pre-DAA headers and no-adjustment chains are no longer waved through to the subtree prewarm. I read the diff on 0875f49, the moved branchSource, and CalcNextWorkRequiredFrom end to end, including the historical and testnet min-difficulty paths.
What I checked:
- Parity with full-block validation. Both call sites in
BlockValidation.go(lines 2259 and 3670) pass the candidate's own timestamp ascurrentBlockTime, and the precheck passesint64(h.Timestamp). So the testnet 2x-spacing rule fires on the same input in both places. - The fresh
Difficultyper run. ItslastBlockHashandlastTestnetTargetcaches start empty, so nothing carries over between peers.blockassembly_difficultyCachedefaults to true, so the testnet walk-back reuseslastTestnetTargetwithin a run rather than re-walking up to 2016 headers for each one. - Error routing. A wrong
nBitsis malicious; a failed anchor read or calculator error is aServiceErrorand the peer is not charged. The unlinked-runProcessingErrorcannot be reached after step 8, and is the safe answer if it ever is. - CHANGELOG entry sits under
## [Unreleased].
I ran go test -race -tags testtxmetacache ./services/blockvalidation/ ./services/legacy/netsync/ ./services/blockchain/ on 0875f49 locally: 3735 passed across the three packages.
One non-blocking note inline about which trunk the tests read through. I did not run the mutation check you describe, and I did not exercise the change on a live node.
| if tSettings.ChainCfgParams.NoDifficultyAdjustment { | ||
| // Returns a NetworkPeerMaliciousError if any header's nBits differ from the requirement, a | ||
| // ServiceError if the requirement cannot be computed, or nil if every header is correct. | ||
| // Failing closed matters: anything this lets through goes on to have its blocks fetched. |
Collaborator
There was a problem hiding this comment.
Non-blocking. Production passes u.blockchainClient as trunk, but every DAA case in catchup_difficulty_test.go passes the sqlitememory store directly. The end-to-end test (TestCatchup_HeaderWithWrongDifficultyIsRejectedBeforeAnyBodyFetch) does go through NewLocalClient, but it runs on regtest, where CalcNextWorkRequiredFrom returns the parent's bits before reading any ancestry. So no test reads a fork's GetSuitableBlock / GetHashOfAncestorBlock / GetBlockHeaders through a client.
The risk is small: legacy netsync already hands the blockchain client to the same branch source as its trunk (services/legacy/netsync/manager.go:4508), so that path runs live today. If you want it pinned here too, running one of the DAA fixtures with blockchain.NewLocalClient(store) as the trunk would close it.
…e fetching blocks Catch-up's header precheck (step 9.5) recomputed the DAA from the fetched headers alone, so it skipped every header whose 144-block window reached below the common ancestor - the first ~147 of any fork - as well as pre-DAA headers and chains with no difficulty adjustment. A short fork mined against a target the peer chose, including one header on top of our tip, passed into step 10, where the subtree prewarm downloads block data before any block is validated. The precheck now uses blockchain.Difficulty.CalcNextWorkRequiredFrom, the calculator full-block validation uses, over a header source in which the fetched headers answer for themselves and the blockchain client answers for the ancestor and everything below it. Every header is judged, and the check fails closed: a wrong nBits is a malicious peer, and a requirement that cannot be computed stops catch-up without blaming the peer. The header source is the legacy header cache's branchSource, moved to services/blockchain as NewBranchSource so both callers share it. ComputeTarget was exported only for the old in-memory precheck and is folded back into Difficulty.computeTarget. Stub-client catch-up fixtures now declare NoDifficultyAdjustment, since the precheck reads ancestry the stub does not have, and the Sybil fixture is anchored on a 207fffff chain so its honest chain repeats its anchor's bits.
icellan
force-pushed
the
claude-issue-4582
branch
from
November 2, 2026 13:24
0875f49 to
6bb3332
Compare
|
oskarszoon
approved these changes
Nov 2, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Background
Catch-up's header precheck (step 9.5,
validateHeaderChainDifficulty) recomputed the DAA from the fetched headers alone. A header was checked only when its whole 144-block window, plus the median lookback, was inside the fetched run, so the precheck skipped:Chain work is only compared for deep forks, so a short fork mined against a target the peer chose could get past it, including a single header on top of our tip. Under the default
pessimisticbootstrap mode, step 10 then prewarms subtree data before any block is validated, so the node downloaded block data for headers whose difficulty was never checked. Full-block validation already enforces nBits unconditionally. Catch-up has to do the same before it fetches anything.Change
validateHeaderChainDifficultynow callsblockchain.Difficulty.CalcNextWorkRequiredFrom, the same calculator full-block validation uses. It reads ancestry through a header source: the fetched headers answer for themselves, and the blockchain client answers for the common ancestor and everything below it. Windows that reach below the ancestor are read from the store. Pre-DAA retargets, EDA, testnet minimum difficulty, STN and no-adjustment chains follow the same rules as full-block validation. The hand-written in-memory DAA is gone.nBitsis reported as a malicious peer (invalid_difficulty) before any body is requested. If the expected bits can't be computed, catch-up stops with aServiceErrorand the peer isn't blamed. This matches the direct path.branchSourcemoves toservices/blockchain/branch_source.goasNewBranchSource, and both callers now use it. netsync's behaviour doesn't change.ComputeTargetis folded back. It was exported only for the old in-memory precheck, so it goes back intoDifficulty.computeTarget.catchup.goandBlockValidation.gothat described the skip are updated. There's a CHANGELOG entry under "Not breaking, but worth knowing".Tests
catchup_difficulty_test.gois rewritten against real sqlitememory stores. The oracle is the store-backedCalcNextWorkRequired, which reads ancestry through SQL rather than through the branch source. Cases:TestCatchup_HeaderWithWrongDifficultyIsRejectedBeforeAnyBodyFetchrunscatchup()against a real store. It asserts that the peer is charged as malicious and is asked for nothing but headers.checkpoint_prefix_nbits_test.goused to assert, as a precondition, that the precheck let this bad header through. It now asserts that both the precheck and the body check reject it.NoDifficultyAdjustment, because the precheck now reads ancestry the stubs don't have. Only the difficulty calculator reads that flag.Verification
go test -race -tags testtxmetacache ./services/blockvalidation/... ./services/legacy/netsync/passes.-run 'Difficult|CalcNext|ComputeTarget' ./services/blockchain/passes, and so did the full blockchain package before the final simplification.go vetand the catch-up tests pass.golangci-lintv2.13.2 (the CI pin) reports nothing.Notes
catchup.goin common, in different places.