perf(blockchain/sql): on_main_chain fast path for GetBlockHeaders / GetBlockHeaderIDs#817
Merged
oskarszoon merged 5 commits intobsv-blockchain:mainfrom May 7, 2026
Conversation
…etBlockHeaderIDs
Replace the recursive CTE walk with a backward index scan over
idx_on_main_chain_height when the start hash is on the main chain. Falls
back to the existing CTE for fork tips, unknown hashes, or while a
main-chain rebuild is in flight, so the CTE remains authoritative.
EXPLAIN ANALYZE on a 80k-block testnet DB (35 MB, fully cached):
N=100 CTE 0.554 ms / 604 buffer hits
fast 0.186 ms / 118 buffer hits -> 2.97x, -80% I/O
N=5288 CTE 17.4 ms / 31729 buffer hits
fast 2.875 ms / 4728 buffer hits -> 6.05x, -85% I/O
Speedup grows with N because the CTE pays one index lookup per ancestor
plus a final sort, while the fast path is a single ordered index scan
(idx_on_main_chain_height already exists).
On a production-sized DB (~1.27M blocks) the CTE pays additional cold-
buffer cost; expected real-world speedup is 10-20x for the catchup-
sized batches observed in pg_stat_statements during legacy IBD.
Sync hot path always feeds parent-of-incoming-block as the start hash,
which is virtually always on_main_chain, so the fast path hits >99% of
the time during normal sync.
Same hybrid pattern used in GetLatestBlockHeaderFromBlockLocator. Same
TOCTOU caveats: the on_main_chain probe and the main query are non-
atomic, but the store's single-writer model bounds staleness to one call
and the CTE fallback is self-healing.
Contributor
|
🤖 Claude Code Review Status: Complete All previously identified issues have been resolved by the author: History:
Current Review:
The implementation correctly handles the TOCTOU window between probe and main query as documented and acceptable under the single-writer model. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-07 08:13 UTC |
…tra-query TOCTOU The fast path previously evaluated `SELECT height FROM blocks WHERE hash = $1` twice within the same SELECT (once for the `<=` upper bound and once for the `> ... - $N` lower bound). Even under the single-writer model, that race is within a single call rather than between calls. Resolve the start-block height in the existing probe (which already runs once per call), and bind it as a literal parameter to the main query. Two wins: 1. Eliminates the intra-query race entirely — the upper and lower bounds are guaranteed to come from the same height value. 2. Restores optimal plan choice. The CTE form (WITH start_block) caused the planner to pick `idx_height` with a Filter at small N (~3x slower than the subquery form). With the height bound as a literal parameter, the planner uses the `idx_on_main_chain_height` partial index for a clean range scan. EXPLAIN ANALYZE on 80k-block testnet DB: N=100 literal-bound: 0.158 ms / 107 buffer hits N=5288 literal-bound: 2.546 ms / 4714 buffer hits Same plan shape and timings as the original subquery-twice form, with the race removed. Addresses Claude Code Review feedback on PR bsv-blockchain#817.
4 tasks
… package and func docs The package- and function-level comments still described only the recursive CTE path. Update them to reflect the hybrid approach now in place: cache → on_main_chain fast path → CTE fallback. Behaviour unchanged. Addresses Claude Code Review minor feedback on PR bsv-blockchain#817.
The previous doc-update commit used 7-space indentation under bullet points; gofmt expects 5 spaces (one tab plus the bullet glyph alignment). Apply gofmt -w. No semantic change. Fixes the golangci-lint `gci` failure on PR bsv-blockchain#817 CI.
ordishs
approved these changes
May 6, 2026
freemans13
approved these changes
May 6, 2026
…lectors
Add explicit named tests for the hybrid query selector introduced in
GetBlockHeaders and GetBlockHeaderIDs:
1. FastPath: assert fast path and CTE return identical headers/IDs for a
main-chain start block (correctness invariant across both paths).
2. ForkTipFallback: assert CTE is used for a fork-tip start block and
returns the fork's own ancestor chain, not the main-chain blocks at
the same heights.
3. CTEWhenRebuilding: assert correct results are returned via the CTE
fallback while mainChainRebuilding > 0.
4. UnknownHashReturnsEmpty: assert empty result with nil error when the
start hash does not exist in the store.
5. BuildQuery_FastPathQuery: unit-test buildGetBlockHeadersQuery and
buildGetBlockHeaderIDsQuery — confirm the returned query contains
"on_main_chain = true" and not "WITH RECURSIVE" for an on-chain block
with mainChainRebuilding == 0.
6. BuildQuery_CTEQuery: confirm the CTE form ("WITH RECURSIVE") is
returned when mainChainRebuilding > 0 or the start block is a fork tip.
Additional integration tests: ModelForkReturnsOnlyForkAncestors,
ModelForkTipIDs, FastPathCacheNotPoisoned.
|
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
4 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
Replace the recursive CTE walk with a backward index scan over
idx_on_main_chain_heightwhen the start hash is on the main chain. Falls back to the existing CTE for fork tips, unknown hashes, or while a main-chain rebuild is in flight — same hybrid pattern used inGetLatestBlockHeaderFromBlockLocator.This closes a remediation gap: most other chain-walk queries already use
on_main_chain(GetBlockByHeight,GetBlockHeadersByHeight,GetBlocksByHeight,GetLastNBlocks,CheckBlockIsInCurrentChain, etc.) butGetBlockHeadersandGetBlockHeaderIDsstill ran the recursive CTE on every call.Why
During legacy sync,
pg_stat_statementson a fresh testnet node showed the heavy CTE inGetBlockHeaderswas the top time-consuming query — 47 calls × 14.18 ms avg = 666 ms total over 6 minutes of sync, returning ~5,288 rows per call when block-validation requested catchup batches.GetBlockHeaders(parent_hash, N)is on the per-block hot path: every legacy sync block triggers it for MTP, ancestor fetch, and fork checks.Measurements
EXPLAIN ANALYZE on a 80k-block testnet DB (35 MB, fully cached in shared_buffers):
Speedup: 2.97× at N=100, 6.05× at N=5288. I/O reduction: 5–7× at both sizes.
Speedup grows with N because the CTE pays one index lookup per ancestor plus a final sort, while the fast path is a single ordered index scan (
idx_on_main_chain_heightpartial index already exists).On a production-sized DB (~1.27M blocks) the CTE additionally pays cold-buffer cost on parent_id walks; expected real-world speedup is 10–20× for the catchup-sized batches observed in
pg_stat_statements.Hit rate
The sync hot path always passes parent-of-incoming-block as the start hash, which is virtually always
on_main_chain = true. Fork tips and unknown hashes fall through to the CTE — preserving correctness for reorg and validation paths.Correctness
Same hybrid pattern, same guards as
GetLatestBlockHeaderFromBlockLocator:mainChainRebuilding.Load() == 0first; if a rebuild is in flight, take the CTE (authoritative path).SELECT on_main_chain FROM blocks WHERE hash = $1; if false / NULL / error, take the CTE.WHERE on_main_chain = true AND height ∈ [tip-$N, tip].TOCTOU between the probe and the main query is bounded by the store's single-writer model: at worst one call returns slightly-stale data, the next call sees the updated
mainChainRebuildingguard and takes the CTE. Acceptable and self-healing.Test plan
go test ./stores/blockchain/sql/...— 475 tests passgo test ./services/blockchain/...— pass (25.4s)go test ./services/blockvalidation/...— pass (164.5s)go vet ./stores/blockchain/sql/...— cleanTestSQLGetBlockHeaderscovers the CTE-fallback path (block2Altis on a fork → fast path probe returnson_main_chain=false→ falls back to CTE)Related follow-ups (not in this PR)
Other quick wins identified during the same investigation, deferred to separate PRs:
GetBlockHeaders(hash, 11)call entirely.bufio.NewReaderSizeinsubtreevalidation— observed 16 GB cumulative allocation in 47 min uptime, reducible to near zero withsync.Pool.scheduled_blob_deletions— currently one INSERT per row; multi-row VALUES list saves ~50× round-trips per block.swiss.NewMap[outpoint]in blockvalidation — observed 224 GB cumulative allocation, dominant heap churn driving GC pressure.These don't share files or test infrastructure with the SQL store change so they belong in separate PRs.