perf(blockchain/sql): fast-path GetBlockInChainByHeightHash on on_main_chain (#1018)#1022
Merged
Merged
Conversation
…n_chain (bsv-blockchain#1018) GetBlockInChainByHeightHash unconditionally walked the parent_id chain via the recursive ChainBlocks CTE even when startHash sits on the canonical chain. Add the same on_main_chain fast path the other locator/height queries already use: preflight whether startHash is on the main chain and, when it is (and no main-chain rebuild is in progress), replace the recursive walk with a single WHERE on_main_chain = true AND height = $1 index lookup on idx_on_main_chain_height. The fast path keeps the AND height <= startHash.height bound so it stays exactly equivalent to the CTE (no row returned when height > startHash.height). Fork-tip starts (on_main_chain = false) and reorg windows (mainChainRebuilding > 0) fall back to the recursive CTE, preserving fork-aware behaviour. GetForkedBlockHeaders keeps its CTE: its "all blocks NOT in the ancestor set of startHash" semantic has no equivalent on_main_chain predicate; documented inline. Adds an equivalence test asserting the fast path, the CTE, and GetBlockByHeight agree across all heights for an on-main start, plus the rebuilding-gate and height-boundary edges.
7 tasks
Contributor
|
🤖 Claude Code Review Status: Complete Summary: No issues found. This is a well-crafted performance optimization with solid correctness guarantees. Key Strengths:
Verification per AGENTS.md: The PR states verification commands were run ( |
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-06-02 12:11 UTC |
ordishs
approved these changes
Jun 2, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Approve. Clean, conservative read-path optimization behind a correct preflight guard, mirroring the established GetLatestHeaderFromBlockLocator pattern line-for-line.
- Path selection is sound: fast path only when
!rebuilding && startOnMain; CTE fallback in every ambiguous case (fork-tip start, rebuilding window, DB error, unknown hash). - Parameter binding ($1=height, $2=hash) is consistent across both branches.
- CTE equivalence verified at the edges (height == / > startHash.height, unknown hash).
- TOCTOU correctly bounded by
mainChainRebuildingand self-healing under the single-writer model. - Good test: equivalence across every height + edges, dynamic tip discovery, and cache flush between branches so the CTE path isn't masked. Passes
-racelocally. - Correctly leaves
GetForkedBlockHeadersand the 6 fork-aware sites on the CTE, with clear inline justification.
Optional follow-ups (non-blocking): assert BlockNotFound type on the height>tip edge instead of a bare error.
freemans13
approved these changes
Jun 2, 2026
freemans13
left a comment
Collaborator
There was a problem hiding this comment.
Approve. Focused, well-documented performance change that completes #1018.
Correctness verified:
- Fast-path/CTE equivalence holds: outside a rebuild window there is exactly one on_main_chain=true block per height, so for an on-main startHash the ancestor at any lower height is that block. The height <= startHash.height bound preserves the CTE's walk-from-seed semantics, including the no-row-above-seed edge.
- Fork-tip and reorg windows correctly fall back to the CTE via the startOnMain preflight and the mainChainRebuilding guard. Pattern matches the already-merged GetLatestHeaderFromBlockLocator fast path line-for-line.
- The on_main_chain column and mainChainRebuilding guard the fast path depends on are maintained unconditionally (independent of blockchain_use_in_memory_chain_check), so behaviour is identical with the in-memory chain check on or off.
- GetForkedBlockHeaders correctly left on the CTE — its complement-of-ancestor-set semantic has no on_main_chain equivalent; good inline rationale.
Test coverage is solid: the new equivalence test asserts fast path == forced-CTE == GetBlockByHeight across every height plus the height edges, discovers the tip dynamically, and clears the response cache between branches. Fork-tip fallback is exercised by the pre-existing test's losing fork.
Minor non-blocking nits: preflight uses startHash.CloneBytes() where startHash[:] would avoid the alloc (locator path uses the slice form); fast-path calls now make one extra indexed round-trip for the preflight (still a clear win over the recursive CTE).
freemans13
added a commit
to freemans13/teranode
that referenced
this pull request
Jun 2, 2026
…tegration Merge stu/blockvalidation-inmemory-checkold (ca7ef31) — off-chain prefetch for checkOldBlockIDs plus the upstream/main it carried (bsv-blockchain#1005, bsv-blockchain#993, bsv-blockchain#1022). Conflict resolutions: - services/legacy/netsync/manager.go: kept integration's gap-watchdog self-heal (requestBlocksFromTip / maybeResyncOnMissingParent / missingParentResyncInterval); the incoming side had no content there. - stores/utxo/aerospike/batch_create.go: pass nil for the new GetBinsToStore arena param (main added *bt.Arena; integration-only BatchCreate caller predates it). nil = non-arena route, output-equivalent per create_arena_test.
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.
Closes #1018 (for the remaining sites).
What
8 of the 10 sites in #1018 already had the
on_main_chainfast path merged. This finishes the list:GetBlockInChainByHeightHash— added the fast path. Because the function is parameterized by astartHashthat may be a fork tip, it uses the preflight variant (same asGetLatestBlockHeaderFromBlockLocator), not the plainmainChainRebuilding-only gate: it checks whetherstartHashis itselfon_main_chainand only then replaces the recursiveChainBlockswalk with a singleWHERE on_main_chain = true AND height = $1lookup onidx_on_main_chain_height. Fork-tip starts and reorg windows (mainChainRebuilding > 0) fall back to the CTE.AND height <= startHash.heightso it is exactly equivalent to the CTE (no row whenheight > startHash.height).GetForkedBlockHeaders— intentionally kept on the CTE. Its semantic is "all blocks NOT in the ancestor set ofstartHash", which has no equivalenton_main_chainpredicate (NOT on_main_chaindrops the main-chain ancestors when the start is a main tip;on_main_chain = falseis wrong when the start is a fork tip). Documented inline. It also has no production callers outside the store.The other 6
WITH RECURSIVEsites in the package (GetSuitableBlock,GetBlockHeadersFromTill,CheckBlockIsAncestorOfBlock,GetBlockHeadersFromOldest,GetHashOfAncestorBlock,GetBlocks) are not in the #1018 list and are genuinely fork-aware — left unchanged.Correctness
The fast path is only taken when
startHashis on the main chain, so the ancestor at any lower height is also on the main chain by definition. ThemainChainRebuildingreference counter brackets every window whereon_main_chainis transiently inconsistent (StoreBlock,InvalidateBlock,RevalidateBlock,rebuildOnMainChainFlag); when it is non-zero the CTE path is forced. The preflight/main-query TOCTOU is bounded and self-healing under the store's single-writer model (same reasoning as the existing locator fast path).Tests
Added
TestSQL_GetBlockInChainByHeightHash_FastPathEquivalence: asserts the fast path, the CTE (forced via the rebuilding flag), andGetBlockByHeightall agree across every height for an on-main start, plus theheight == startHash.heightandheight > startHash.heightedges. The pre-existing fork-tip test already covers the CTE fallback.go build ./stores/blockchain/... ./services/blockchain/...— okgo vet/staticcheck/golangci-lint— cleango test ./stores/blockchain/sql/— okgo test -race ./stores/blockchain/sql/— okNote on #1019
#1019 (drop 4 indexes) was investigated alongside this. Conclusion: do not drop them — all four have real readers; the "0 scans" was a measurement-window artefact. Details posted on that issue.