triedb/pathdb: trie-node lookup index for O(1) diff-layer resolution#2312
triedb/pathdb: trie-node lookup index for O(1) diff-layer resolution#2312lucca30 wants to merge 3 commits into
Conversation
Extend the PR #30971 lookup structure (accounts/storages flat state) with a trie-node index keyed by owner+path, maintained in addLayer/removeLayer alongside the existing maps. reader.Node now resolves the owning layer via nodeTip instead of walking the diff-layer chain newest->oldest (up to TriesInMemory=128 map probes, each taking a per-layer RLock). Stale-layer reads fall back to the traversal path, mirroring AccountRLP/Storage, and the bor clean-cache hash-mismatch self-heal path is preserved. The index is derived in-memory from the layer stack (newLookup on open/cap0, addLayer on add, removeLayer on flatten) and is not persisted, so it rebuilds correctly from the journal on restart. It reuses the same tree.lock that the flat-state index already takes, fitting the parallel-execution locking model. No upstream equivalent exists: geth indexes flat state only and still resolves trie nodes via the recursive parent walk. Steady-state microbench (128 diff layers, node at max depth): walk ~2200 ns/op -> index ~88 ns/op (O(depth) -> O(1), ~25x). Tests (lookup_node_test.go): hand-verified tips (account + storage owner, top/middle/bottom, post-cap, stale); exhaustive index-resolve vs legacy-walk byte-equality across rewrites/deletion/resurrection/side-branch, re-checked after a partial cap; concurrent readers racing add/cap (green under -race).
Split the lookup-index resolution and errSnapshotStale fallback out of reader.Node into a resolveNode helper, so Node's body returns to the develop shape (hash validation + clean-cache self-heal) and the new resolution path is isolated and documented on its own.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2312 +/- ##
===========================================
+ Coverage 54.05% 54.07% +0.01%
===========================================
Files 907 907
Lines 161982 162087 +105
===========================================
+ Hits 87564 87650 +86
- Misses 69000 69030 +30
+ Partials 5418 5407 -11
... and 20 files with indirect coverage changes
🚀 New features to boost your workflow:
|
- Extract removeNodeEntry helper from removeNodes, dropping its cognitive complexity from 18 to 11 (under the 15 gate). - Add tests for the previously-uncovered defensive branches: lookupNode's missing-layer guard, removeNodes' account/storage not-found guards, and reader.resolveNode (happy path + stale-state resolution error). Raises patch coverage 85%->98% and kills the two surviving diffguard mutants (layertree lookupNode nil-layer, lookup removeNodes storage not-found). The remaining survivors are report-only observability branches.
|
@claude review |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
There was a problem hiding this comment.
No bugs found by either pass, but I'm deferring rather than approving. This PR touches triedb/pathdb's trie-node resolution path (state-critical, hot in parallel block execution), and the author has explicitly kept it in draft pending a dedicated mainnet validation experiment on current develop before considering it ready — that gating experiment hasn't landed yet, so a human should make the merge call once it does.
What was reviewed: the new index's concurrency (addNodes/removeNodes run alongside the existing account/storage goroutines under the pre-existing tree.lock write-lock, writing disjoint maps); the interaction between partial cap() flattening and the index (verified the flattened layer's own mutation-history entries are correctly dropped while lookups for that exact state still resolve via the disk-layer base fallback); and reader.resolveNode's stale-fallback path, which mirrors the existing AccountRLP/Storage precedent. Also confirmed the removeNodes error being swallowed by cap()'s clearDiff closure is pre-existing behavior on the account/storage paths, not something newly introduced here.
Extended reasoning...
This is a focused but consensus/state-critical change to triedb/pathdb, adding an O(1) mutation-history index for trie-node resolution that mirrors the existing flat-state (account/storage) lookup index from #30971. It touches layertree.go, lookup.go, and reader.go — specifically the read path used by every trie node fetch during block execution.
No security risks in the traditional sense (no auth/crypto/RPC surface), but correctness risk is real: if the index ever resolved a node to the wrong diff layer, a node could compute a wrong state root, which is a consensus-split-class bug. I traced the concurrency model (goroutines in addLayer/removeLayer write disjoint maps under the existing tree.lock), the interaction between cap()'s partial-flatten and full-commit paths and index rebuilding, and the stale-fallback logic in reader.resolveNode, and found nothing incorrect — the design closely mirrors the already-proven flat-state index and the new tests (equivalence test against the legacy walk, a race-covered concurrent stress test) are appropriately targeted.
That said, the author explicitly marked this PR as kept in draft pending a dedicated mainnet experiment isolating this exact change before it's considered ready, separate from the campaign build it was validated alongside. That's the author's own bar for mergeability, and it hasn't been cleared yet in this PR's lifecycle. Combined with the change sitting on a hot, state-correctness-critical path, I think this warrants a human sign-off rather than a shadow approval, regardless of the clean bug-hunt result.



What
Adds a per-node mutation-history index to pathdb so
reader.Noderesolves theowning layer in O(1) instead of walking the diff-layer chain newest→oldest
(up to
TriesInMemory = 128map probes, each taking a per-layerRLock).This mirrors the flat-state lookup index that #30971 already added for
accounts/storages — but for trie nodes, which currently have none. The new
accountNodes/storageNodesmaps live in the existinglookupstruct and aremaintained in the same
addLayer/removeLayerfunctions, so trie-node reads nowuse the same fast resolution path (
nodeTip→lookupNode) that account andstorage reads already do.
Why
Under the current code every
reader.Nodecall walks the diff stack from theentry layer down, probing each layer's node set and taking that layer's
RLockat every hop. With a full 128-layer stack a node that lives near thebottom costs ~128 map probes per read. The flat-state reader was already freed
from this walk by #30971; trie-node reads were left behind.
This is on the hot path of parallel block execution (many goroutines resolving
trie nodes concurrently), so removing the per-read walk cuts CPU on every state
access.
How
lookup.accountNodes/lookup.storageNodes: mutation history keyed byowner+path, value = ordered list of diff-layer state roots that rewrote the
node (same shape as
accounts/storages).lookup.nodeTip: reverse-scans the history for the newest entry that is thequeried state or an ancestor of it; falls back to the disk layer, or returns
zero (stale) — identical semantics to
accountTip/storageTip.layerTree.lookupNode: resolves the guaranteed-containing layer undertree.lock.RLock, mirroringlookupAccount/lookupStorage.reader.Node: resolves vialookupNode, reads the node at depth 0, and onthe narrow disk-layer-went-stale window falls back to the legacy walk — exactly
as
AccountRLP/Storagedo. The existing bor clean-cache hash-mismatchself-heal path is preserved unchanged.
addNodes/removeNodeshelpers invokedfrom
addLayer/removeLayer, so it rides along on every mutation path: layeradd, partial flatten (
cap), full commit (cap 0→ freshnewLookup), andjournal replay on restart (
newLookuprebuilds from the reconstructed stack).The index is derived in-memory and never persisted, so a restart rebuilds
it from the journal exactly like the flat-state index.
Correctness
The index is maintained by the same
addLayer/removeLayer/newLookupfunctions as the proven flat-state index, so it inherits the identical set of
maintenance points — there is no new mutation path that could leave it stale.
nodeTipreuses the descendant/stale logic ofaccountTip.Tests added in
lookup_node_test.go:TestNodeLookup— hand-verified tips (account trie + a storage owner),probing top/middle/bottom of the stack, after a
cap, and stale states.TestNodeLookupEquivalence— for every observable(owner, path, state),asserts the index resolution returns byte-identical data to the legacy
newest→oldest walk, across a stack with re-writes at the same path, a deletion,
a resurrection, two storage owners, and a side branch; re-checked after a
partial cap. (Scoped to live states: dangling post-cap side-branch layers
diverge from the naive walk exactly as
lookupAccount/lookupStoragealreadydo — inherent to the lookup-index design.)
TestNodeLookupConcurrent— readers resolving nodes while the stack ismutated by concurrent
add/cap; green under-race.The full
triedb/pathdbsuite passes under-race.Measured on a mainnet node
Validated on a bor node replaying mainnet state (16 vCPU). A before/after CPU
profile shows node resolution effectively leave the profile:
with a clean tip figure of exec 2.42 ms/Mgas. That profile was taken with the
in-memory diff stack full (sync/catch-up regime), which is exactly where the
newest→oldest walk is deepest and most expensive — i.e. the upper end of the win.
We're happy to share the profiles/methodology.
Benchmark (per-read cost isolated)
Single node resolution over a 128-diff-layer stack, node written at max depth
(
lookup_node_test.go):~25× on a full-depth resolve; the walk is ~26 ns/hop so the break-even is ~3–4
layers deep and the index is constant thereafter. At a shallow tip the per-read
win is correspondingly smaller — the mainnet 29.8% figure above reflects the
deeper catch-up regime. The primary win is CPU/latency (O(
TriesInMemory) →O(1)); the index reuses the existing
tree.lock, so it fits the current lockingmodel rather than changing it.
Cost
The index is held in memory only — two maps in the existing
lookupstructrecording, per mutated trie-node path across the live diff layers, the state
roots that rewrote it. Same shape and order of magnitude as the flat-state index
in #30971, bounded by
TriesInMemory, shrinks on flatten, and rebuilt from thejournal on restart (never persisted). Maintenance is O(nodes-in-layer) per
add/flatten, parallel to the existing account/storage index updates and off the
read path.