fix(legacy): gate quickValidation on checkpoints, not FSM state#847
Merged
oskarszoon merged 3 commits intoMay 12, 2026
Merged
Conversation
quickValidationMode in prepareSubtrees was gated on FSMStateLEGACYSYNCING, so CATCHINGBLOCKS (catchup-driven re-syncs from block 0 or after a fork) fell back to the slow path: full subtree re-validation via subtreeValidation, no pre-allocated block ID, and per-UTXO SetMinedMulti through setTxMinedStatus. The trust property that justifies the fast path is checkpoint coverage plus PoW, not the FSM state that happens to be active. Replace the FSM lookup with a check against the highest hard-coded checkpoint, reusing the helper added in bsv-blockchain#844 (blockchain.HighestCheckpointHeight). Blocks at/below the highest checkpoint take the fast path regardless of FSM state; blocks above it always take the regular validation path. Drops the IsFSMCurrentState RPC from the per-block hot path. Updates the buildAddBlockOpts doc block in blockvalidation to reflect the broader scope.
Contributor
|
🤖 Claude Code Review Status: Complete SummaryThis PR correctly replaces FSM-state-based gating with checkpoint-based gating for the quick validation path. The implementation is well-tested and documented. The changes align with the security model established in PR #844. Findings: No issues found. The PR demonstrates careful engineering:
The implementation correctly handles edge cases (nil chainParams, no-checkpoint networks) and the security rationale is sound. |
…sed scope Aligns docs/topics/services/legacy.md with the code change in the parent commit — quick validation now engages for any block at/below the highest hard-coded checkpoint, regardless of whether the FSM is in LEGACYSYNCING or CATCHINGBLOCKS.
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-12 12:10 UTC |
icellan
requested changes
May 12, 2026
handle_block.go does not import services/legacy/blockchain, so there is no name collision to disambiguate. The alias was carried over from manager.go out of habit; reverting to the package's natural name keeps the import simpler. Manager.go still needs the alias because it imports both packages.
icellan
approved these changes
May 12, 2026
|
ordishs
approved these changes
May 12, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — clean swap of the FSM-state gate for a checkpoint-coverage gate. Trust model is sound (PoW + checkpoint linkage), pre-storage double-spend check still runs via checkParentExistsOnChain, and removing the per-block IsFSMCurrentState RPC is a nice bonus.
One nit (non-blocking):
handle_block_test.go:790 — mainnetHighest is derived as chaincfg.MainNetParams.Checkpoints[len-1].Height, which assumes the slice is sorted ascending. The helper under test (HighestCheckpointHeight) deliberately does not make that assumption. If go-chaincfg ever ships an unordered list, the "one above highest" sub-case would silently become "not above highest" and pass spuriously.
Suggest mirroring the helper:
mainnetHighest := blockchain.HighestCheckpointHeight(chaincfg.MainNetParams.Checkpoints)Happy to land as-is if you'd rather not churn.
This was referenced May 14, 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
3 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.
Summary
prepareSubtreesin legacy netsync only enabledquickValidationModewhen the FSM was inLEGACYSYNCING.CATCHINGBLOCKS(catchup-driven sync from block 0, or after a fork) fell back to the slow path: full subtree re-validation throughsubtreeValidation, no pre-allocated block ID, and per-UTXOSetMinedMultiviasetTxMinedStatus.HasMetTargetDifficultycheck, is what makes the block canonical.blockchain.HighestCheckpointHeight). Blocks at/below the highest checkpoint take the fast path regardless of FSM state; blocks above it always take the regular validation path.IsFSMCurrentStateRPC from the per-block hot path. Updates thebuildAddBlockOptsdoc block inblockvalidationto describe the broader scope.Why this is safe
The fast path skips two things: subtree re-validation through
subtreeValidation, andsetTxMinedStatus' post-storage double-spend cross-check. Both are redundant when:block.Valid()has already run pre-storage double-spend detection viacheckParentExistsOnChainbeforeAddBlock.This is the same invariant #844 uses to gate the FSM-RUN transition. The checkpoint list is bumped by updating go-chaincfg; the gate moves forward with it.
Trade-offs / follow-ups
Test plan
go build ./...go vet ./services/legacy/netsync/... ./services/blockvalidation/...go test -race -short ./services/legacy/netsync/— 49 passed including newTestSyncManager_quickValidationAllowed(6 sub-cases: nil chainParams, regtest, mainnet height 0, equal-to-highest, one-above-highest)go test -race -short ./services/blockvalidation/[validateTransactionsLegacyMode] created utxoslog line)