Repository navigation
fix(blockvalidation,subtreevalidation): reject forks below a passed checkpoint and keep older-era side blocks out of block assembly - #2069
Conversation
…heckpoint and feed block assembly only from tip blocks A side block is validated at its own height, so below Genesis or Chronicle its transactions get the older script rules. Block assembly then mines them at tip+1 without re-checking scripts, and peers holding the stored verdict skip the re-check too. - ValidateBlockWithOptions rejects a block whose height is below a checkpoint the tip has already reached (bitcoin-sv's bad-fork-prior-to-checkpoint). Checkpoints above the tip do not count, so initial sync is unaffected. The tip is only read for blocks at or below the highest checkpoint. - CheckBlockSubtrees and the legacy CheckSubtree path add a block's transactions to block assembly only in RUNNING and only when the block's parent is the current tip. Fixes bitcoin-sv/teranode#4940
|
🤖 Claude Code Review Status: Complete Current Review:
No correctness issues found. The latest commit only merges
Notes (not blocking):
|
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-28 11:34 UTC |
… parent Feeding block assembly only from blocks that extend the tip stranded the txs of a same-height competitor that loses: they are stored unmined but never reach the template, a resubmission hits ErrTxExists and is not re-added, and a child arriving later can enter the template without its parent. Feed instead when the block's height and tip+1 fall under the same transaction rules (no BIP65, BIP66, CSV, UAHF, Genesis or Chronicle activation between them). That still keeps an older-era side block out of the template, which is what issue 4940 needs. Also: - drop the default GetBestBlockHeader mock from setupTestServer; testify serves the first match, so it shadowed every test's own expectation. Tests that need a tip call mockBestBlockHeader. - give the checkpoint call-site fixture an unreached checkpoint with a hash of its own, and say so in its comment. - correct the CHANGELOG entry and the mirror comment in the validator.
icellan
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Approving. Both rules are correct, and I found nothing blocking. I read head 1b29e02.
Fork rule. ForkBelowCheckpoint only counts checkpoints the tip has reached, so initial sync is unaffected. It cannot catch our own main-chain blocks: an already-stored valid block returns early at the top of ValidateBlockWithOptions, before this check runs. Revalidating an invalid-marked block below a passed checkpoint now fails, which matches bitcoin-sv.
Era rule. sameTxRuleEra covers every activation height that services/validator, model and services/subtreevalidation read: BIP65, BIP66, CSV, UAHF, Genesis and Chronicle. BIP34 affects only the coinbase, and the DAA height affects only difficulty, so leaving both out is right. I also checked that the legacy route passes a real BlockHeight (Client.go:141). Without it the era check would silently turn feeding off.
One non-blocking test gap, inline. The existing bot comment about the stale "separate, broader change" sentence above the checkpoint-hash check still applies at this head.
What I ran. go test -tags testtxmetacache on the new and touched tests in ./model/, ./services/blockvalidation/ and ./services/subtreevalidation/: 123 passed. I did not run -race, the full packages or lint. I did not test the reorg route, which the PR description says it leaves open (bitcoin-sv/teranode#4769).
ordishs
left a comment
ordishs
left a comment
There was a problem hiding this comment.
The fork rule matches bitcoin-sv's bad-fork-prior-to-checkpoint. Checkpoints above the tip do not count, so initial sync is unaffected. The script-era gate closes the direct-fork route into block assembly. No blocking issues found. Tests pass on ./model/, ./services/blockvalidation/ and ./services/subtreevalidation/.
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The new checkpoint and assembly gates leave reachable cases where a below-checkpoint fork is treated as valid or a transaction enters a template under rules different from those used to validate it.
- P1 —
services/blockvalidation/BlockValidation.go:2116: The checkpoint decision reads the tip before subtree work. Another block can advance the tip past the checkpoint before this block reachesAddBlock, which then stores a below-checkpoint fork. Catchup can also return success for an already-stored valid hash before reaching this check. Enforce the fork rule at a state-coordinated storage boundary and cover the stored-hash path. - P1 —
services/subtreevalidation/check_block_subtrees.go:422:sameTxRuleEramisses BDK's P2SH and DAA script-flag activations and checks UAHF one block early. Equal activation flags also do not make a transaction final at both candidate heights or parent median times. Compare the mandatory rules and finality for the actual mining context before feeding assembly. - P1 —
services/subtreevalidation/check_block_subtrees.go:411: The tip is sampled once before subtree loading and validation. If it crosses Genesis or Chronicle before transactions reach assembly, an older-era block can still feed the new-era template. Tie the feed decision to the assembler's current tip era or revalidate queued transactions when that era changes.
# Conflicts: # CHANGELOG.md
|
…rom BDK flags and re-check the checkpoint fork rule before storing - validator.SameTxRules compares BDK's consensus script flags at both heights instead of a list of chaincfg heights. The list missed P2SH, put UAHF and DAA one block early, and did not match BDK's STN heights. BIP68 still follows ChainCfgParams.CSVHeight. - feedsBlockAssembly also requires the block not to be above tip+1, so a height locktime final at the block is final at tip+1. - rejectForkBelowCheckpoint runs before subtree work and again just before AddBlock, in case the tip passes a checkpoint while the block validates. - The fork-below-checkpoint test asserts AddBlock is not called; the stale "separate, broader change" comment is gone.
…he non-optimistic store, under its own context The optimistic path stores the block before any subtree work, right after the first check, so a second check there covers no window. On the normal path the re-check runs after the subtree work, so it must not fail on a cancelled parent context, like the store that follows it.
|
@oskarszoon, point by point. Fixes are in the latest two commits. P1-1, checkpoint check reads the tip before subtree work ( On the already-stored hash: I don't think that's a gap. A stored valid block below a passed checkpoint was accepted before the tip reached the checkpoint, so it is either a main-chain ancestor or a fork that was valid when it arrived. bitcoin-sv applies P1-2, era rule misses P2SH/DAA and has UAHF off by one ( Finality: the feed now also requires P1-3, tip crossing Genesis/Chronicle mid-validation. Not changing this here. Block assembly has no era awareness at all. The same one-block window exists on main for every policy-validated mempool tx in flight when the tip crosses an activation height. Re-validating queued txs when the era changes is a block assembly feature, and it would cover mempool txs too, not just this gate. Both activations are already behind mainnet and testnet, so the window only applies to regtest/STN. I'd file it as a follow-up rather than hold this PR for it. |
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The new pre-store checkpoint recheck and BDK flag comparison address parts of the prior review, but the checkpoint rule can still be bypassed by concurrent storage or an already-stored hash, and the assembly feed can still admit transactions invalid for the current mining context.
- P1 —
services/blockvalidation/BlockValidation.go:2824: The second checkpoint check still reads the tip before a separateAddBlockcall. A concurrent block can pass the checkpoint between those operations; the optimistic path has only the first check. The existing-valid return at line 1966 also skips the rule for a stored hash reached during catchup. Enforce checkpoint eligibility with the storage decision and on the stored-hash path. - P1 —
services/validator/same_tx_rules.go:28: Equal script flags andblockHeight <= tip+1do not establish time-based finality on different branches. A side-block transaction can be final at its parent MTP and non-final for the best tip's next block, yet enter assembly without a finality recheck. Check finality against the assembler's current candidate context. - P1 —
services/subtreevalidation/check_block_subtrees.go:412: The assembly-feed decision uses one tip snapshot before subtree loading and validation. If the tip crosses Genesis or Chronicle before the transactions are admitted, older-era transactions can still reach the newer-era template. Recheck at assembly admission or discard queued transactions when the tip changes era.
Fixes bitcoin-sv/teranode#4940
Problem
A side block is validated at its own height. Below Genesis (or Chronicle) its transactions get the older script rules. When the node is RUNNING those transactions go to block assembly, which mines them at tip+1 without re-checking scripts. Peers that already hold the stored verdict skip the re-check as well. A cheap fork deep below the tip can therefore put transactions into our template that the rest of the network rejects.
Change
ValidateBlockWithOptions, right after the exact checkpoint hash check). A block whose height is below a checkpoint the tip has already reached is rejected asBlockInvalidwith reasonbad-fork-prior-to-checkpoint, the same rule as bitcoin-sv. Checkpoints above the tip don't count, so initial sync is unaffected. The tip is read only for blocks at or below the highest checkpoint, so blocks above it cost no extra call. On the non-optimistic path the rule runs again just before the block is stored, under its own context, in case the tip passes a checkpoint while the subtrees validate. The rule itself ismodel.ForkBelowCheckpoint, applied byrejectForkBelowCheckpoint.feedsBlockAssembly, used byCheckBlockSubtreesand the legacyCheckSubtreepath). In RUNNING, a block's transactions go to block assembly only when the block is not above tip+1 and both heights fall under the same transaction rules (validator.SameTxRules): BDK computes the same consensus script flags at both, and no BIP68 activation (CSVHeight) lies between them. The flags come from BDK, not a list of chaincfg heights: BDK enables P2SH at a height chaincfg does not carry, UAHF and DAA one block after their fork heights, and its STN BIP65/66 heights differ from chaincfg. A same-era block that does not extend the tip still feeds, so the txs of a competitor that loses stay in the template. A first version fed only blocks extending the tip, which stranded them: a resubmission hitsErrTxExistsand is not re-added, and a child arriving later could enter the template without its parent.Behaviour changes
GetBestBlockHeadercall; on the legacy path it is one per subtree. Below the highest checkpoint the fork rule adds one more per block, two on the non-optimistic path.Not addressed
CheckSubtree(Server.go) still sends transactions to block assembly in every FSM state. Nothing in production calls it; it is reachable only through the internal gRPC API.Tests
TestForkBelowCheckpoint: truth table for the rule.TestValidateBlock_RejectsForkBelowCheckpoint: a side block below a passed checkpoint is rejected before its subtrees are checked and is not stored.TestValidateBlock_RejectsForkBelowCheckpointPassedDuringValidation: the tip passes the checkpoint while the subtrees validate; the block is rejected and not stored.TestSameTxRules: truth table from BDK's probed flag boundaries (regtest P2SH/Genesis/Chronicle/CSV/BIP66/BIP65, mainnet P2SH and UAHF/DAA at fork+1).TestSameTxRules_BIP68FollowsCSVHeightpins the BIP68 term.TestCheckBlockSubtrees_BlockAssemblyFeedFollowsScriptEra: a below-Genesis side block does not feed block assembly (the issue); a same-era competitor whose parent is not the tip does; a block above tip+1 does not.setupTestServerregisters no default tip: testify serves the first match, so a default shadowed every test's own expectation. Tests that only need a tip callmockBestBlockHeader.TestCheckpointValidationCallSitesthe checkpoint sits above the tip, unreached, so the difficulty gate it tests is still reached.go test -tags testtxmetacachepasses on./model/,./services/blockvalidation/,./services/subtreevalidation/and./services/validator/; the first three also with-race.golangci-lintreports nothing new.