perf(blockchain): main-chain fast-path for block locator builder (#1021)#1023
Merged
Conversation
…ator equivalence test
Also fix pre-existing prealloc lint finding in server_test.go (flagged because the file is in the changed set).
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: No issues found. The implementation is well-structured with comprehensive tests and defensive fallback logic. Highlights:
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-06-04 07:38 UTC |
ordishs
approved these changes
Jun 2, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Approving — well-scoped, well-tested perf change with a conservative correctness posture (fall back whenever the fast path can't be proven equivalent). Built the changed packages and ran the locator + store unit tests locally, all green.
Correctness — solid
- Fast-path equivalence is sound: the main chain is linear, so the
on_main_chain=trueblock at heighth <= startHeightis the unique ancestor ofstartHashath, identical to the CTE walk. Verified byTest_getBlockLocator_FastPathMatchesWalkandTestMainChainBlockHashesByHeights_AgreesWithCTEWalk. - Ceiling derived from the start hash's own height rather than trusting the caller —
Test...CeilingFromHashHeightpins this. Good defensive design. - Fallback coverage is complete (fork tip, mid-rebuild, unknown hash, empty inputs, incomplete/short result, collapsed duplicate heights) and closely mirrors the existing
GetLatestBlockHeaderFromBlockLocatorpattern. - No injection risk — both branches fully parameterized.
Comments (non-blocking)
uint8cast replaces the safe-conversion helper (Server.go:3167,maxEntries = uint8(blockHeaderHeight) + 1). Functionally safe (guarded by<= 12) but diverges from the codebase'ssafeconversionconvention and is exactly theuint32->uint8narrowinggosecG115 flags. Please confirm gosec config; sincemaxEntriesis only a capacity hint, computing it as anintwould sidestep it entirely.- TOCTOU note dropped. The reference
GetLatestBlockHeaderFromBlockLocatordocuments the non-atomic gap between the rebuild-guard check and the main query (bounded/self-healing under the single-writer model). The same race exists inMainChainBlockHashesByHeightsbut isn't commented — worth porting the paragraph for parity. - Preflight swallows all DB errors as "not on main chain." Acceptable and documented, but there's no signal distinguishing an expected fork-tip fallback from an unhealthy DB silently disabling the fast path. Consider a trace attribute / debug counter on the fallback reason.
- Nit: genesis being
on_main_chain=trueis the load-bearing assumption behind the benchmark numbers (otherwise every build silently falls back — still correct, just un-optimized). Covered by the rebuild walking to height 0; flagging as the key assumption.
Tests — strong
Golden height schedules, fast-vs-walk equivalence, all fallback branches, hash-derived ceiling, PostgreSQL testcontainer parity, and an EXPLAIN test that pins the index path with enable_seqscan=off. The cold benchmark honestly clears responseCache outside the timer and documents why, keeping the 57x claim credible. Minor smell: benchLocatorHeights duplicates computeLocatorHeights to avoid a service->store import — commented and benchmark-only, fine as-is.
…gnitive complexity
Rename MainChainBlockHashesByHeights{,_test}.go to snake_case per
docs/references/codingConventions.md, and split the store method into
mainChainHeightsQuery + scanHeightHashes helpers to drop SonarQube
cognitive complexity from 19 to below the 15 threshold.
|
freemans13
approved these changes
Jun 4, 2026
freemans13
left a comment
Collaborator
There was a problem hiding this comment.
Approve. Well-scoped main-chain fast path that faithfully mirrors the existing GetLatestBlockHeaderFromBlockLocator/GetBlockHeaders pattern, with byte-identical output guaranteed by an exhaustive fallback (fork tip, mid-rebuild, unknown hash, missing/incomplete result).
Verified locally: go build + go vet clean on both packages; targeted unit tests (computeLocatorHeights, FastPathMatchesWalk, MainChainBlockHashesByHeights*) pass. Worked through the hash-derived ceiling in both H_caller>H_actual and H_caller<H_actual directions — consistent with the walk in both.
Non-blocking nit: the real-store equivalence tests (sqlitememory/Postgres) request only heights {3,2,1}/{2,1}; since every production locator ends at genesis (0), adding height 0 to one real-store assertion would guard against a silent always-fallback regression if genesis were ever not flagged on_main_chain.
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
Adds a main-chain fast path to the block locator builder (
getBlockLocator), closing #1021.Previously, building a locator from the tip issued ~12–21 sequential recursive-CTE calls (
GetBlockInChainByHeightHashper entry), a cumulative O(chain-height)parent_idtraversal to produce ~33 hashes. For a start hash on the main chain this is now a single indexed query.computeLocatorHeights— the height schedule extracted as pure arithmetic (no DB), unit-tested against golden sequences including the height-skip boundary.MainChainBlockHashesByHeights(new store method) — fetches the main-chain block hash at every locator height in one query overidx_on_main_chain_height. Gated by anon_main_chainpreflight and themainChainRebuildingguard; the height ceiling is derived from the start hash's own height (not trusted from the caller). Mirrors the existingGetLatestBlockHeaderFromBlockLocatorpattern.Performance
Cold benchmark (5000-deep chain, in-memory SQLite, no network), fast path vs per-height CTE walk:
~57× faster, ~15× fewer bytes, ~11× fewer allocs. The gap widens with chain depth and with real PostgreSQL network round-trips (12–21 round-trips collapse to one).
Test Plan
computeLocatorHeightsgolden test (heights 0, 1, 12, 13, 255, 1000)EXPLAINconfirmsidx_on_main_chain_height(planner pinned viaenable_seqscan=off); fork-tip fallbackgo build ./...,go vet,-race,golangci-lintclean