fix(flat): isolate warmer misses and safely publish verified reads - #12951
Conversation
…cture #12877 restored the warmer's negative cache by writing the miss placeholder (NodeType.Unknown, empty RLP) into the shared _transientResource.Nodes and tried to keep it safe with an IsPlaceholder predicate at every live-read site and at the shared-cache promotion. That coexistence is the defect: the same placeholder instance sits in the cache that live reads consult, so a warmer resolve racing a live read reopens the InvalidStateRoot window #12793 closed (reproduced on Flat sync at live head, mainnet and gnosis; HalfPath unaffected). Remove the coexistence instead of guarding it. Warmer misses now go to a dedicated TransientResource.MissNodes cache that no live read ever consults and that TrieNodeCache.Add never promotes. Nodes therefore holds only real nodes, so the IsPlaceholder guards and the predicate are dead code and are dropped. The negative cache is preserved (a repeated warmer miss is still served without re-probing persistence), so the #12793 perf recovery stands.
…iant Reword the XML doc to claim only what is verified — no live read consults MissNodes and TrieNodeCache.Add never promotes it — instead of the stronger 'a stale miss can never reach the state-root computation', which does not hold: the placeholder also escapes as the warmer return value and is later resolved by a live read through persistence (by hash, correct by construction, matching #12793 and pre-dating this PR).
…sient Isolating the warmer's negative cache in `MissNodes` is what makes #12924 correct, but it also strands the warmer's work. A warmer miss means the node is absent from the in-memory structures, not from persistence: the warmer returns a placeholder and the traversal resolves it through `ReadOnlySnapshotBundle.TryLoadStateRlp`, i.e. a real persistence read. That placeholder lives in `MissNodes`, which neither live reads nor `TrieNodeCache.Add` consult, so every node the warmer pulls off disk is fetched, decoded and then discarded at block end while the live path repeats the same read. Publish what the warmer read: a private `TrieNode` built from the RLP, decoded before it is stored in `TransientResource.Nodes`, never handed to the warmer. The block's live reads and the cross-block `TrieNodeCache` reuse the read; no reader can observe a node mid-resolution, which is the hazard that made publishing the warmer's own instance unsafe (`IsPlaceholder` was a predicate over `NodeType`/`FullRlp` while another thread wrote both). Persistence read count is unchanged: the publish is a side effect of the read the warmer already performs, not an extra one.
|
Claude finished @kamilchodola's task in 10m 42s —— View job Review: publish the trie warmer's persistence reads into the transient
The diagnosis in the description is right and well-argued: CriticalThe published node's
The needed invariant — a warmer persistence read at
That yields a stale subtree → wrong storage root → I want to be clear about my confidence: steps 1–4 are each read off the code, but I have not run this, and the queued Sync Master Validation Medium
Low
Things I checked and found fine
Not doneI could not run Agreed with the draft framing — the two gates you've queued (Sync Master Validation |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
…d node Review catch on the previous commit, and a real defect in it. `ReadOnlySnapshotBundle.TryLoadStateRlp`/`TryLoadStorageRlp` take a `hash` but never use it: the read is keyed by path alone. Publishing those bytes as `new TrieNode(NodeType.Unknown, hash, rlp)` therefore *stamps* the requested hash onto whatever node currently sits at that path, and the warmer can ask for a (path, hash) pair the pinned persistence view does not hold - it reaches newer pairs through the cross-block `TrieNodeCache`, and on a multi-block branch through a post-commit storage root. Every reader-side guard (`ChildCache.TryGet`, `TrieNodeCache.TryGet`, `FindCachedOrUnknown`'s `NodeHashMismatchException`) compares against the node's claimed `Keccak`, so all of them would be satisfied by construction and a stale node would be served to live reads and promoted cross-block: `InvalidStateRoot`, the symptom this whole chain keeps producing. Verify the binding instead of asserting it - publish only when the RLP hashes to the requested hash - and state the invariant in the `<remarks>`. Also from the review: - `TryResolveNode` instead of `ResolveNode`, so a publish-side decode failure cannot change the outcome or exception type of the read it piggybacks on. - Pick the `ReadOnlySpan<byte>` ctor explicitly so overload resolution cannot silently flip between copying and aliasing the caller's array. - Document the costs honestly: a Keccak per published node, the RLP copy, the dual `MissNodes`/`Nodes` slot and its effect on the regrow decision, and the `GetOrAdd`-replaces-on-mismatch interaction with a racing trie commit. - Tests now drive the real warmer adapter path rather than calling the bundle method directly, dropping an assertion that held trivially, and cover the mismatch case: RLP for node A returned at the path where the warmer asks for node B must publish nothing. Verified non-vacuous - removing the hash guard fails both cases.
kamilchodola
changed the title
|
@claude re-review |
|
Claude finished @kamilchodola's task in 9m 30s —— View job Re-review:
|
| # | Item | Status |
|---|---|---|
| Critical | hash asserted, not verified |
Fixed. SnapshotBundle.cs:468 — if (ValueKeccak.Compute(rlp) != hash) return; before anything is published. ValueHash256 != Hash256? resolves to the value comparison at Hash256.cs:103, not reference equality. Regression test present and I confirmed it is non-vacuous (below). |
| Medium | decode failure changes the read's outcome | Fixed, and better than asked. TryResolveNode + bail on false (:473). The residual worry — DecodeRlp throwing something outside the catch (RlpException) — is now unreachable rather than merely handled: the hash check runs first, so the bytes are the canonical encoding of a real node. |
| Medium | GetOrAdd can evict the authoritative node |
Acknowledged with rationale in <remarks>, with the correct reasoning (evicted node stays authoritative in _changedStateNodes/_snapshots, so it costs a re-read). I agree ChildCache has no add-if-absent and adding one isn't worth it unverified. |
| Low | RLP copy / silent overload flip | Explicit cast added. Keeping the copy is the right call — your point about the same byte[] also going back to the warmer via WriteRlp is stronger than my SafeReturn argument, because it makes the aliasing invariant span two independently-lived nodes. |
| Low | vacuous assertion / bypassed call path | Fixed. Both tests now drive warmed.TryResolveNode(WarmerResolver(...)), so StateTrieStoreAdapter.cs:55/110 are actually exercised. |
Your extra point about TryFindStateNodeInPersistence consulting the cross-block TrieNodeCache first is right, and it does kill state-only publishing: the cache holds nodes promoted from previous blocks' transients, so the warmer can resolve a newer parent from the cache and then ask pinned persistence for a child at (path, newHash) — a single-block, state-trie instance of the same mismatch. Option 1 was the correct choice.
New checks this round
The isolation commits (6da4345, b772b09) removed TrieNodeCache.IsPlaceholder and its two call sites. Since that deletes a guard, I traced whether anything other than the warmer can still put a placeholder into Nodes:
- Every writer to
Nodesis nowUpdateState/StorageNode(commit),GetOrAddState/StorageNodeat:220/:351(warmer persistence hits), and:483/:487(publish). Placeholders go only toMissNodes. - The deleted doc claimed "the trie warmer's negative cache and the trie commit path both produce it" — the commit-path half was wrong.
PatriciaTree.cs:271-278callsResolveKey→Seal()→ commits only whenFullRlp.Length >= 32, so a committed node always has a realNodeTypeand ≥32 bytes of RLP. The guard was warmer-only, and removing it is safe. - Isolation is a strict improvement on the base, beyond correctness: on
mastera warmer miss didNodes.GetOrAdd(placeholder), which evicts a real node from that direct-mapped slot for live readers. That no longer happens. - Published nodes never reach the DB.
Nodesis not a snapshot source (CollectAndApplySnapshotbuilds from_currentPooledContent;TransientResource's own summary states it is "not committed intoIFlatDbManageras part of aSnapshot"). So the worst case for a publishing bug is bounded at in-memory cache poisoning — which is what the hash check now closes. TrieNodeCache.Add(retired)runs whileFlatDbManager.PopulateTrieNodeCachestill holds the owner lease, andCommitsets_pausePrewarmer = truewithout draining outstanding warmups, so an in-flight publish can write into the retired transient concurrently withAdd's shard scan.ChildCache's 16-byte tuple is written non-atomically, so a torn read can file a node under the wrong bucket — butTrieNodeCache.TryGetverifiesKeccak == hash, so the outcome is a cache miss, not corruption. This race is unchanged frommaster(the warmer already wrote toNodesat:220/:351); not a finding against this PR, noting it so it isn't mistaken for one later.- Test non-vacuity, checked by hand rather than assumed: in
Warmer_does_not_publish_a_node_the_rlp_does_not_hash_to, dropping the guard makesNodeshold a node withKeccak == requestedHash, so the live read returnsNodeType.Leafand both assertions fail.EncodedLeaf()produces 37 bytes (0xc0+36: 3-byte hex-prefix key + 33-byte value), so it is genuinely hash-referenced rather than inlined, as its summary claims.Warmer_miss_does_not_reach_live_reads_or_trie_node_cachewould also pass onmaster— it pins the new structure (fails ifMissNodesis dropped without restoringIsPlaceholder), which is the right guard to have. - Also fine:
IsPersistedon the published node (TrieNode.cs:356-359), soPrunePersistedRecursively(1)behaves; the publish's own lease/ABA round-trip and unconditionalfinallyrelease;TreePath nodePath = pathso the keying path isn't the oneTryResolveNodemutates; theFlatTestHelpers.MakeBundleoverload and<inheritdoc>.
Findings
Medium — MissNodes doubles the transient node-array footprint, and GetSize() can't see it. TransientResource.cs:42 · Fix this →
ChildCache allocates all 256 shards in its constructor, so a Nodes capacity of ~400k entries (≈6.4 MB) is now matched by an equally-sized MissNodes — paid per TransientResource, in every Usage pool, including read-only/RPC scopes whose MissNodes stays empty. Separately, GetSize() (line 76) still reports only Nodes.Capacity, so if MissNodes.Reset() regrows past Nodes, the next pool-created resource silently constructs it smaller than the workload demonstrated it needs — degrading exactly the negative-cache hit rate this PR exists to protect. The <remarks> price the slot accounting but not this. Suggested one-line fix inline.
Low — the cost accounting undersells the node duplication. SnapshotBundle.cs:425
"one TrieNode" reads as if the published copy is the only node-level cost, but the MissNodes entry stops being a placeholder the moment the warmer resolves it — TryResolveNode does WriteRlp + DecodeRlp in place (TrieNode.cs:467-476). So a missed path retains two fully-materialised copies until Reset, where master retained one. Deliberate price of not sharing the instance, and I'd keep the design; just worth naming, and worth reading allocation/GC off the pending A/B (the EventPipe sidecar already has it) rather than wall-clock alone.
On the validation gate
Flagging without re-litigating, since the PR body says it plainly: the only EXPB comment on this PR (32506853474) compared against a cached master baseline and produced no realblocks metrics — the payload set that matters most for a storage-layer change — and the +2.2% AVG / −5.5% P99 figure quoted in the body is from the isolation-only ancestor, not this tree. The ValueKeccak.Compute now sits on the path the change exists to make cheaper. My read is that it should still win (sub-µs Keccak against the RocksDB read it prevents), but that's a prediction, not a measurement, and your own fallback (memoise path → rlp at the RLP layer, no hash claim needed, no cross-block promotion) only becomes comparable once there's a same-session A/B. Likewise the correctness gate: the sync evidence in the body predates the hash check, so it validated a tree that had the defect. Neither is a code objection — both are the gates I'd want green before merge, and both are yours to call.
Not done
dotnet build and dotnet test were both refused in this environment, so everything above is static analysis; I did not reproduce your 14/14 SnapshotBundleWarmerTests result. I did verify by hand the things a compiler would have caught: the ValueHash256/Hash256 operator exists, TrieNodeFactory is public with a (ReadOnlySpan<byte>, CappedArray<byte>) overload that accepts [0x3, 0x4] / new byte[32], Nethermind.State.Flat.Test has InternalsVisibleTo for StateTrieStoreWarmerAdapter, and the new using Nethermind.Trie.Pruning; in SnapshotBundle.cs doesn't collide with the fully-qualified Nethermind.Trie.Pruning.Metrics references already in that file.
Verdict: the correctness objection from last round is properly resolved, and I found nothing new at Critical or High. The one Medium is a resource-cost question, not a correctness one.
· branch perf/flat-warmer-publish-resolved
benaadams
left a comment
benaadams
left a comment
There was a problem hiding this comment.
Three remaining findings from the local review.
|
|
||
| start.Set(); | ||
| bool firstLoadStarted = loadStarted.Wait(BailOutTimeout); | ||
| allowLoad.Set(); |
There was a problem hiding this comment.
Medium — the concurrency test never exercises a live read while resolution is in flight. It releases the blocked load immediately, so a regression that publishes the resolved bit before RLP/decode completion could still pass this test while exposing the mutable shared node to block processing. Before allowLoad.Set(), perform the matching live state/storage lookup and assert that it is not warmed and remains Unknown.
There was a problem hiding this comment.
Added in 58bd117: the test now does the matching live state/storage lookup while the load is parked and asserts it is not warmed and stays Unknown.
|
|
||
| [TestCase(false)] | ||
| [TestCase(true)] | ||
| public void Unresolved_warmer_miss_does_not_reach_trie_node_cache(bool storage) |
There was a problem hiding this comment.
Low — the storage: false case duplicates the preceding Unresolved_warmer_miss_is_not_promoted scenario. It covers the same state-cache behavior and adds stronger live-read assertions. Per the test-duplication rule, remove the preceding test or fold its explicit cache assertion into this parameterized case.
There was a problem hiding this comment.
Removed Unresolved_warmer_miss_is_not_promoted in 58bd117; the parameterized case covers it.
| using (Assert.EnterMultipleScope()) | ||
| { | ||
| Assert.That(live.NodeType, Is.EqualTo(NodeType.Leaf)); | ||
| Assert.That(live, Is.SameAs(warmed)); |
There was a problem hiding this comment.
Low — the PR description no longer matches the final implementation. It still describes MissNodes and a private published node that is never shared with the warmer, but this assertion confirms that live readers intentionally reuse the warmer instance and only retirement detaches it. Please update the description to the final resolving/resolved protocol; the targeted run is also 21 tests, not 14.
There was a problem hiding this comment.
PR description rewritten to the resolving/resolved protocol that is actually in the tree, with the validation section pointing at the runs on this tree.
|
@claude Please check if the changes made in this PR are actually solving the performance degradation problem OR there was some different bottleneck fixed which obfuscate perf loses by initial changes. Check the history of related PRs. |
|
Claude finished @kamilchodola's task in 12m 31s —— View job Does this PR actually solve the degradation, or is something else masking it?
Short answer: no, it does not. On the only clean measurement available, this tree is ~1.6% slower than master on flat/realblocks, and the reported delta improving from +4.52% to +1.61% over today's commits is mostly the master baseline drifting upward, not the branch getting faster. And yes — the historical "negative cache" framing mis-attributes the bottleneck: the money was never in the miss short-circuit, it was in reusing/promoting the warmer's resolved node, which is the same mechanism that has produced 1. What the measurements actually sayThe one trustworthy datapoint is your own dispatched A/B — run 32733971643:
AVG +1.61%, and the ranges do not overlap — the branch's best run (19.77) is worse than master's worst (19.71). Median +0.6%, P90 +2.7%, P95 +1.7%, P99 ≈ flat (−0.3%). So it is a small, consistent, body-of-the-distribution regression, not a tail artifact. Now the per-commit
Read the branch column, not the delta column: 20.37 → 20.09 → 20.05 → 20.03 → 20.21. The branch has been flat within rig noise all day. The headline moving from +4.52% to +1.61% is almost entirely the cached master aggregate rising 19.49 → 19.89. Every measurement of this branch, on both methodologies, puts it above master. Best estimate of the real cost: +0.3–0.5 ms/block, ~1.6–2.5% on flat/realblocks. (Superblocks −1.84% and fusaka −0.32% in the PR comment are single runs against a cached baseline, and superblocks P99 swings ±34% run to run on this rig — I would not read a sign off either.) 2. The history: the bottleneck was mis-labelled
That pair of rows is the decisive experiment and it is already in your own history: a fully working, isolated negative cache is worth ≈ 0. Nearly all of #12877's gain came from (b)+(c) — the warmer's resolved node being reused by live reads and promoted cross-block. Which is exactly the shared-instance retention that fails the sync gate. Perf and the defect are two faces of one mechanism, so "restore the negative cache" was never going to buy the regression back, and #12924 measuring +2.2% with a perfect negative cache confirms it. This PR (in its current, rewritten form) does restore (b) and (c) — that is what 3. Where the residual cost is, in the codeThe design pays new CPU on precisely the path it exists to accelerate:
4. So: was a different bottleneck fixed, masking the loss?Two distinct answers, both yes:
5. What would settle it
6. Incidental, on this tree
Bottom line: the degradation is not recovered by this tree, and on the evidence it cannot be recovered by any variant that keeps the warmer's node private — the gain lives in shared reuse + cross-block promotion, which is the same thing that broke consensus twice. This PR re-enables that mechanism behind a resolution protocol and a drain barrier, and the protocol's cost currently eats the gain. I would not merge it as a performance change on these numbers; if it is worth merging it is as a correctness change over master (which still carries #12877's predicate-guarded shared placeholder), and that case needs a Sync Master Validation run that actually starts. |
A path-keyed persistence read can answer a warmer read with another version of the node at that path, so the RLP hash check is expected to fail while the warmer runs ahead of or behind the live reads. Resolve warm-up traversal with TryResolveNode so such a node just ends the traversal unresolved and unpublished, instead of throwing out to the warmer processor as an ERROR.
|
@claude re-review - check if anything outstanding is needed to be done before merging |
|
Claude finished @kamilchodola's task in 7m 43s —— View job Pre-merge check: what's still outstanding
The big news since my last pass: the correctness gate is now green on this exact tree. Everything else outstanding is small. Nothing new at Critical or High. The gate that mattered is greenSync Master Validation 32775387528 ran on Worth stating plainly because the PR body still hedges ("No new live-sync validation against the current Protocol re-derivationI re-checked publication safety from scratch, since shared-instance reuse is precisely what broke twice before:
OutstandingFixed by me and pushed — Code Lint was red. Medium — the concurrency test never exercises a live read mid-resolution. Inline · @benaadams raised this and it's still open. Low — Low — duplicate test. Low — the PR description now describes a design that isn't in the tree. Third round for this one, and it matters more than usual because the body's validation section is reporting a different design's results:
Not a blocker, restating for the record: the perf case is unchanged from yesterday's analysis. The latest comparison on this tree has realblocks at +1.82% AVG / +6.72% P99 against a cached VerdictNo Critical, no High. The correctness argument is sound and — for the first time in this chain — backed by a green flat mainnet + gnosis sync on the tree being merged. Before merge I'd want: the mid-resolution live-read assertion, a re-run of |
Left over from the removed PublishWarmedNode; every Metrics reference in the file is fully qualified, so it fails Code Lint with IDE0005. Co-authored-by: Kamil Chodoła <43241881+kamilchodola@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lves Also drop the test subsumed by its parameterized successor and trim the comments this PR added to one-liners.
|
@claude re-review |
|
Claude finished @kamilchodola's task in 8m 7s —— View job Re-review:
|
| Item | Status |
|---|---|
Critical: hash asserted, not verified |
Fixed — VerifyWarmerOwnedRlp (TrieNode.cs:691) before DecodeRlp, and again independently at promotion (TrieNodeCache.cs:143) |
| Medium: decode failure changes the read's outcome | Fixed — TryResolveWarmerOwnedNode returns false; PatriciaTree.cs:997 treats that as end-of-warm-up |
Medium: GetOrAdd can evict the authoritative node |
Acknowledged with rationale |
Medium: MissNodes footprint / GetSize() blind spot |
Moot — MissNodes no longer exists; the redesign removed the second ChildCache entirely |
| Medium: concurrency test never reads mid-resolution | Fixed in 58bd117, and non-vacuously — see below |
Low: duplicate Unresolved_warmer_miss_is_not_promoted |
Deleted |
| Low: description described a different design | Rewritten to the resolving/resolved protocol |
Code Lint red on IDE0005 |
Green — Code Lint 32824759261 |
On the new assertion: I checked it bites rather than passes by construction. At the point it runs, the first resolver is parked inside Load() holding _warmerResolvingMask, _warmerResolvedMask is clear, and _rlpArray is still null. FindStateNodeOrUnknown therefore fails the !IsWarmerOwned || IsWarmerResolved gate, DoFindStateNodeExternal misses (the stub only answers TryLoadStateRlp, not the decoded-snapshot lookup), and the caller gets a fresh Unknown. Drop the gate, or set resolved before DecodeRlp/WriteRlp, and Is.Not.SameAs(warmed) fails immediately. That is exactly the invariant the design rests on.
Protocol re-derivation (done fresh, not carried over)
- Flag algebra is safe. Every mutator of
_blockAndFlags—IsPersisted,IsBoundaryProofNode,Seal,MarkWarmerOwned,TryAcquireWarmerResolution,CompleteWarmerResolution— is a CAS retry loop over the whole byte, so the three new bits cannot be clobbered by a concurrentIsPersisted = true. The only raw writes are in constructors, before publication. - Ordering is release/acquire.
DecodeRlp→WriteRlp(seqlock) →CompleteWarmerResolution(Interlocked.CompareExchange, full fence); readers gate onVolatile.Read. No reader can see a decodedNodeTypewith a null_rlpArray. - A failed verification leaves the node clean.
WriteRlpruns only afterVerifyWarmerOwnedRlppasses, so a stale read leaves_rlpArraynull andresolvedclear — the node stays a placeholder rather than a half-materialised one, and a later resolver retries from scratch. - The resolving spin terminates.
CompleteWarmerResolutionis in afinally, so the bit cannot leak; a failed resolution clears resolving without setting resolved, so a spinner retries rather than parking. WaitForExclusiveLeasecannot hang at the call site.FlatDbManager.PopulateTrieNodeCache(:188-192) holds a lease when it callsAdd, so_leases >= 1on entry and the wait is for stale warmer readers to drain — each of which releases in afinallyaround a bounded find.SpinWait.SpinOnceescalates toThread.Yield/Sleep, so it is not a hard spin on an oversubscribed box.ThrowInvalidKeccakis unreachable on the production warmer path. The only warmer entries areFlatWorldStateScope.cs:368andFlatStorageTree.cs:129, bothWarmUpPath→TryResolveNode; live readers only reach resolved warmer nodes (which return early fromTryAcquireWarmerResolution); and the unresolved warmer-owned nodes they can reach areCreateInlineChildproducts with a nullKeccak, where verification short-circuits. See the inline comment.DoWarmUpPath's child walk does not reintroduce the throw.GetChildWithChildPathreturns the child without resolving it (TrieNode.cs:966-1003); resolution happens at the top of the next loop iteration, throughTryResolveNode. So a stale child ends the warm-up rather than escapingWarmUpPath'scatch (TrieException) { …; throw; }.
Findings
Low — TrieNodeCache.cs:145 the keccak is null branch is unreachable, and it's the one path that would skip verification. Inline Every warmer-owned node in Nodes comes from CreateWarmerUnknownNode(hash); inline children (the only other MarkWarmerOwned caller) live in a parent's _nodeData and never enter Nodes. If it were reached it would promote unverified bytes under a null Keccak, which TryGet (:91) can never match — occupying a bucket, evicting its previous occupant, and charging _shardMemoryUsages for something permanently unreachable. Collapsing it to if (keccak is null || …) return null; removes code and makes "re-verified before promotion" unconditional.
Low — TrieNode.cs:528 the comment says the condition is benign; the helper it labels throws. Inline The tolerance lives in TryResolveWarmerOwnedNode and PatriciaTree.cs:997, not here. coding-style.md: a comment that contradicts the code is worse than none.
Low (informational, no action) — a permanently stale path re-reads persistence on every warmer visit. A node that fails verification stays warmer-owned and unresolved in Nodes, so the next WarmUpStateNode hit returns it and TryResolveNode re-issues the persistence read. The negative cache absorbs misses, not stale hits. On master this didn't happen only because the stale bytes were accepted — i.e. it's the bug's cost, correctly paid. Bounded by how often the flat DB genuinely holds another version at a warmed path, which is the multi-block-branch case; worth a glance in the A/B rather than a change.
Low (informational) — DoWarmUpPath is shared, so hash/HalfPath layouts also change. ResolveNode → TryResolveNode means a genuinely missing node during prewarm now silently ends the traversal instead of raising an enhanced TrieException. Warm-up is best-effort so I think this is the right default, but it does remove the only diagnostic signal a non-flat store had for a missing node during prewarm.
On merging
- Correctness: settled, and now backed by a green flat mainnet + gnosis sync on the exact production code.
masterstill carries fix(flat): restore the trie warmer negative cache without poisoning live reads #12877's predicate-guarded shared placeholder resolved from an unverified path-keyed read; this closes it. - Performance: unchanged from my 16:21 analysis and still the open question. The body reports −0.4% / −1.6% / −1.3% realblocks and 0.0% fusaka across four same-session A/Bs in both orderings — that's a genuinely better evidence base than the cached-baseline tables, and it reads as parity-to-slightly-positive rather than the +1.6% I measured on the older tree. Parity is a fine result for a correctness fix; I'd still leave the "Optimization" box as a judgement call rather than a claim the numbers compel.
- CI: lint, formatting, spell check, Trivy, dependency review, build tools and stateless tests are green on HEAD.
Build solutions,Nethermind tests (Flat DB),Nethermind extra test variants,Nethermind/Ethereum tests,Integration tests (E2E)andSync PR Gate (Hoodi)are still running — worth confirming green before merge, since the last two rounds each had one unexplained red (Nethermind.Synchronization.Test, thenNethermind.Blockchain.Test) that never got classified.
Not done
dotnet build and dotnet test are refused in this job, so all of the above is static analysis; I did not reproduce the 20/20 SnapshotBundleWarmerTests or 475 Nethermind.Trie.Test results. Compile-level facts I checked by hand instead: Nethermind.Trie.Test already has InternalsVisibleTo (TrieNode.cs:22) so the new MarkWarmerOwned usage in StaleWarmerTrieStore resolves; RefCountingLease.Single == 1 matching WaitForExclusiveLease's comparison; and the TrieNode(NodeType, Hash256, CappedArray<byte>) overload setting IsPersisted for NodeType.Unknown.
Verdict: no Critical, High, or Medium. Two cosmetic Lows, neither blocking. My prior not-mergeable was on a Medium that the redesign has since made moot — I'd merge this on the green sync run once the pending test jobs land, treating it as the correctness fix it is.
· branch perf/flat-warmer-publish-resolved
| Hash256? keccak = source.Keccak; | ||
| if (keccak is not null && ValueKeccak.Compute(fullRlp.AsSpan()) != keccak) return null; | ||
|
|
||
| TrieNode detached = keccak is null | ||
| ? new TrieNode(NodeType.Unknown, fullRlp) | ||
| : new TrieNode(NodeType.Unknown, keccak, fullRlp); |
There was a problem hiding this comment.
Low — the keccak is null branch is unreachable, and it is the one path here that would skip verification.
Every warmer-owned node that can appear in transientResource.Nodes comes from SnapshotBundle.CreateWarmerUnknownNode(hash), which is called only with a non-null hash from the four warmer find sites (SnapshotBundle.cs:193, :219, :327, :349). MarkWarmerOwned is otherwise reached only from CreateInlineChild, and inline children live inside a parent's _nodeData — they are never inserted into Nodes. So source.Keccak is non-null for every source that reaches this helper.
If it were reachable it would be the wrong behaviour twice over: the keccak is not null && guard on line 143 short-circuits, so unverified bytes get promoted; and the resulting node has Keccak == null, which TryGet (line 91, maybeNode.Keccak == hash) can never match — so it would occupy a bucket, evict whatever was there, and be charged to _shardMemoryUsages while being permanently unreachable.
Per the AGENTS.md preference for removing code over adding it, I'd collapse it to the invariant:
| Hash256? keccak = source.Keccak; | |
| if (keccak is not null && ValueKeccak.Compute(fullRlp.AsSpan()) != keccak) return null; | |
| TrieNode detached = keccak is null | |
| ? new TrieNode(NodeType.Unknown, fullRlp) | |
| : new TrieNode(NodeType.Unknown, keccak, fullRlp); | |
| Hash256? keccak = source.Keccak; | |
| if (keccak is null || ValueKeccak.Compute(fullRlp.AsSpan()) != keccak) return null; | |
| TrieNode detached = new(NodeType.Unknown, keccak, fullRlp); |
That also makes the "re-verified before promotion" claim unconditional, which is what the PR description says it is.
(The two catches below are likewise defensive-only — TryResolveNode on a non-warmer node already swallows RlpException internally, and the bytes are hash-verified before they get here — but they're cheap and I'd leave them.)
| // A path-keyed store serving another version of the node is staleness, not corruption. | ||
| [DoesNotReturn, StackTraceHidden] | ||
| void ThrowInvalidKeccak(in TreePath nodePath) => throw new TrieNodeException( |
There was a problem hiding this comment.
Low — the comment says the condition is benign, the code it labels throws.
.agents/rules/coding-style.md: "a comment that contradicts the code is worse than no comment". As written this reads as an explanation of why the mismatch is tolerated, sitting directly on the helper that raises TrieNodeException for it. The tolerance actually lives in the other overload (TryResolveWarmerOwnedNode at :616, if (!VerifyWarmerOwnedRlp(rlp)) return false;) and in PatriciaTree.cs:997.
Worth noting while you're here that this throw is unreachable on the production warmer path, which is why the comment reads oddly:
- the only warmer entry points are
FlatWorldStateScope.cs:368andFlatStorageTree.cs:129, bothWarmUpPath→DoWarmUpPath→TryResolveNode, neverResolveNode; - a live reader can only obtain a warmer-owned node with a
KeccakonceIsWarmerResolvedis set (SnapshotBundle.cs:170/:301), andTryAcquireWarmerResolutionreturnsfalsefor a resolved node, soResolveWarmerOwnedNodereturns before reaching the check; - the warmer-owned nodes a live reader can reach unresolved are
CreateInlineChildproducts, whoseKeccakis null — soVerifyWarmerOwnedRlpshort-circuits totrue.
I'm not asking you to delete the branch (ResolveNode is public and symmetry with TryResolveNode is reasonable), just to reword so the comment describes what the throw means rather than why it doesn't matter — e.g. "The RLP does not hash to the requested node; the tolerant path is TryResolveNode, which reports this as a miss."
…lution A live reader that reaches a warmer-owned node through a shared parent's child slot waits for the warmer's in-flight persistence read; SpinWait's Sleep(1) escalation would add up to a millisecond of tail latency there, so spin and yield only. Also cover the retirement drain with a regression test and drop a vacuous assertion.
wurdum
left a comment
wurdum
left a comment
There was a problem hiding this comment.
Out of the changed files scope:
The warm-up storage tree is rooted at the live tree's RootRef, which the live adapter created, so it is never warmer-owned. DoWarmUpPath does no FindCachedOrUnknown re-lookup at depth 0, so the root resolves through the plain TryResolveNode branch with no ValueKeccak.Compute check while TryLoadStorageRlp ignores hash. That writes possibly-stale RLP into the live storage root — the failure this PR fixes, on the one node the verification cannot reach.
// The warm-up traversal resolves this node through a path-keyed reader; require hash verification.
// src/Nethermind/Nethermind.State.Flat/ScopeProvider/FlatStorageTree.cs:59
_warmupStorageTree.RootRef = _tree.RootRef;
_warmupStorageTree.RootRef?.MarkWarmerOwned();
|
@wurdum on the out-of-scope root note: agreed it is the one node the verification cannot reach, but it is pre-existing ( |
svlachakis
left a comment
svlachakis
left a comment
There was a problem hiding this comment.
Ran a six-reviewer pass. Starting with what matters most: the publication protocol is sound, and this
is not #12877 again. That was traced independently twice, not assumed. _nodeData and _rlpArray are
both written before CompleteWarmerResolution's Interlocked.CompareExchange, readers Volatile.Read
the flag before touching either, so no reader can observe the resolved bit ahead of the data. The CAS
cannot grant two owners, and every co-resident writer (IsPersisted, Seal, MarkWarmerOwned) is a
full-byte CAS loop so no flag is lost. _blockAndFlags packs no block number despite the name, so bits
3-5 were genuinely free. The shared RLP array is a fresh allocation that is never pooled and never
mutated in place. A failed verification leaves _nodeData and _rlpArray both null, so there is no
half-decoded state. #12877 shared a mutable placeholder behind a predicate; this shares an
immutable-once-published node behind a verified flag, and the difference holds up.
The problem is that the PR's stated reason for existing does not survive checking.
The base already retained resolved warmer nodes
pr-12951-base's live gate is !TrieNodeCache.IsPlaceholder(node), which accepts any warmer node with
NodeType != Unknown, and its Add promoted that same object into the shared cache. Only unresolved
placeholders were dropped, and this PR drops those too. So against the tree that was benchmarked, this
does not restore retention - it adds a verification tax to retention that already worked.
Where the measured deltas plausibly come from instead, both unrelated to retention:
DoWarmUpPathno longer throws per unresolvable path. On base every warmer traversal reaching a node
absent from persistence threw, unwound, and hitEnhanceException, which formats the hex key and root
hash into a message and rethrows - roughly 10-30 us each. Nodes absent from persistence are normal
while persistence lags the head. At ~500 such paths per block that is 5-15 ms of warmer CPU removed.- The live gate got cheaper.
IsPlaceholderreadnode.FullRlp, i.e. a seqlock with two volatile reads
and a possible spin; the new gate is one or two plain byte loads, on every trie node of every live
state read.
On the numbers themselves: the spread across the three realblocks deltas (1.2 pp) is the same order as
the deltas (0.4-1.6%), so the data is its own noise estimate; three same-sign results at n=3 is p=0.125;
fusaka at 0.0% is consistent with no effect. And it cannot recover the +2-3% #12924 cost, because the
baseline is master rather than #12924 - that comparison was not run.
Four things I would want fixed
1. SpinOnce(sleep1Threshold: -1) in TryAcquireWarmerResolution. The -1 disables the escalation to
Thread.Sleep(1), so the loop degrades to Thread.Yield()/Sleep(0) forever. On Linux sched_yield
returns immediately when nothing of equal priority is runnable, so this is a 100% CPU busy-wait for the
full duration of a pread, on up to N-1 cores, with no timeout and no fallback. It is the only -1 in
the change - ReadRlp, WriteRlp and WaitForExclusiveLease all use plain SpinOnce(). It will not
appear in a warm A/B and will appear on a cold or IO-contended node as newPayload tail latency plus a CPU
floor that scales with the warmer worker count.
2. Block processing can reach an unresolved warmer node without passing the gate. The gate at
SnapshotBundle.cs:170/:301 covers the path-keyed transient lookup only. The warmer writes unresolved
warmer-owned children into _nodeData[i] of shared nodes via ResolveChildWithChildPath, and
DoWarmUpPath passes keepChildRef: true so UnresolveChild does not clear the slot. Block processing
legitimately takes the parent - the new gate permits it, and Resolved_warmer_node_is_reused_by_live_reads_and_promoted_detached
asserts it - descends to the child, and gets it with no gate. It then calls the throwing
ResolveNode, which lands in ResolveWarmerOwnedNode. So the block thread can both spin on warmer I/O
and take ThrowInvalidKeccak, which TrieWarmer.HandleJob catches for the warmer and nothing catches
here. Still better than base, which decoded the mismatched RLP silently - but claim 3 in the description
should not be read as "block processing never touches an unresolved warmer node", because it does.
3. A failed verification is not memoised, and the multiplier is per warmed key. On failure FullRlp is
never written, the resolved bit stays clear, and the node stays in the transient under the requested
hash. The next warmer traversal gets the same instance back, re-enters TryResolveWarmerOwnedNode,
re-issues the flat-DB read and recomputes the Keccak. Base decoded the stale bytes once and
NodeType != Unknown short-circuited every later visit. Level-1 state nodes are traversed by every warmed
key, so this moves from once-per-node to once-per-warmed-key - order 100-200x on shallow nodes. Bit
0b0100_0000 is free; a sticky unresolvable flag that TryAcquireWarmerResolution checks is about five
lines, and the transient is per-block so the negative result expires on its own.
4. The test that guards the #12877 failure mode parks at the wrong point.
Concurrent_owned_warmer_resolution_loads_once blocks inside TryLoadStateRlp, i.e. before
VerifyWarmerOwnedRlp, before DecodeRlp, before WriteRlp. At that instant the node is
NodeType.Unknown with empty RLP, which is exactly what the old IsPlaceholder predicate already
excluded - revert the gate to !IsPlaceholder(node) and the test still passes. The window the new gate
actually closes is later: DecodeRlp assigns _nodeData, so NodeType stops being Unknown while
FullRlp is still empty and the resolved bit is still clear. That is the #12877 shape and nothing parks
there. A test that puts a decoded-but-unpublished warmer node in the transient and asserts
FindStateNodeOrUnknown does not return it would fail under the old predicate and pass under the new
one, which is the revert sensitivity currently missing.
Smaller
- Single-flight does not hold across instances.
ChildCache.GetOrAddis check-then-act, so two warmer
threads warming accounts with a shared prefix create two distinct placeholders for the same path and
both issue a persistence read. The flag protocol only serialises threads that already share an
instance. The concurrency tests hand all four tasks the same instance, so they never exercise it.
_count++on the same path is a non-atomic read-modify-write. WaitForExclusiveLeasedrains lookups, not resolutions - the lease is released in
FindStateNodeOrUnknownForTrieWarmer'sfinally, andTryResolveNoderuns after that. So promotion
findsIsWarmerResolved == falseand silently drops exactly the coldest, slowest nodes, which is the
retention this PR is for. The name also promises exclusivity it cannot provide: nothing stops a new
lease one instruction later.- The
TrieNodeCache.cs:145keccak is nullbranch is unreachable (warmer-owned nodes in the transient
all come fromCreateWarmerUnknownNode(hash), which cannot have a null hash on a sealed node), and it
is the one branch that would publish without verification.if (keccak is null) return null;fails
closed instead. - The comment at
TrieNode.cs:530says a path-keyed store serving another version is staleness rather
than corruption, sitting on the[DoesNotReturn]helper that throws. The benign handling is 90 lines
away at theTryResolvetwin. - The description's reason for the
DoWarmUpPathchange is unfounded:TrieWarmer.HandleJobalready
caught bothTrieNodeExceptionandNodeHashMismatchExceptionwith empty bodies, so nothing that
previously surfaced is now hidden. The change is right; the justification is not. - The gate that stands between the warmer and block processing lost its name and its documentation.
!TrieNodeCache.IsPlaceholder(node)carried an XML doc spelling out why such a node must neither enter
the shared cache nor satisfy a live read; it is now an unnamed boolean duplicated at two call sites,
reading_blockAndFlagstwice. And the ordering rule the whole protocol rests on - resolved bit last,
acquire-read first - is documented nowhere, whileReadRlpandWriteRlpin the same file both carry
explicit seqlock and ARM64 notes.
One design question worth answering before this lands
The root fact is a contract violation in the flat layer: ITrieNodeResolver.TryLoadRlp(path, hash, flags)
promises the RLP of that hash, and ReadOnlySnapshotBundle.TryLoadStateRlp never reads hash. This PR
repairs that by lifting hash verification into TrieNode, behind a new per-node flag, plus an
InternalsVisibleTo grant. Verifying in StateTrieStoreWarmerAdapter.TryLoadRlp and
StorageTrieStoreWarmerAdapter.TryLoadRlp instead is about three lines, computes the same Keccak the
same number of times, and the existing TryResolveNode already implements "failed verification leaves it
unresolved" exactly. Gating live reads on the already-public NodeType != NodeType.Unknown would also
avoid creating the decoded-but-unpublished window in the first place, since the standard resolver writes
RLP before the decode while the new warmer copy inverts that order.
That would take most of the 223 lines out of TrieNode.cs - a type every layout, sync healing, snap
serving and the visitor go through - and leave the flat-specific concern in the flat assembly. What it
gives up is single-flight, which per the point above does not currently work across instances anyway, and
which the A/B cannot separate from the two mechanisms that are actually producing the numbers.
5 participants
Changes
Supersedes #12924. Flat-layout trie warmer: keep reusing what the warmer resolves, without ever sharing a mutable node with block processing.
Unknownplaceholder in the transientNodes(SnapshotBundle.CreateWarmerUnknownNode). It resolves through a resolving/resolved flag protocol onTrieNode(_blockAndFlags): one resolver loads the RLP, verifiesKeccak(rlp) == requested hash, decodes, then publishes the resolved bit; other resolvers wait. A failed verification leaves it unresolved.SnapshotBundle.FindStateNodeOrUnknown/FindStorageNodeOrUnknowngate); otherwise they fall through to the snapshots/persistence as before.TrieNodeCache.Addwaits for in-flight warmer reads to drain (TransientResource.WaitForExclusiveLease) and promotes a detached, re-verified copy of each resolved warmer node (sharing the RLP array, not the object).PatriciaTree.DoWarmUpPathtreats an unresolvable node as the end of the warm-up. Flat persistence is path-keyed, so a warmer read can legitimately return another version of the node at that path; that is staleness, not an error, and it must not reach the log as one.Why
ReadOnlySnapshotBundle.TryLoadStateRlp/TryFindStateNodesignore the requested hash, so anything published from a warmer read must be hash-verified first.InvalidStateRooton flat mainnet. fix(flat): isolate the trie warmer negative cache (InvalidStateRoot on Flat live-head sync) #12924 isolated misses instead → correct, but +2–3% AVG because every warmer-resolved node was discarded at block end. This PR restores that retention behind an explicit publication protocol.Validation
run_count=3per arm, both orderings:SnapshotBundleWarmerTests20/20,Nethermind.Trie.Test475 passed.Types of changes
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Regression tests cover: stale (other-version) RLP is not resolved, not served to live reads and not promoted; invalid RLP likewise; a live read during an in-flight resolution stays
Unknown; concurrent resolvers load once; promotion yields a detached copy;WarmUpPathdoes not throw on a stale path-keyed read.Documentation
Requires documentation update
Requires explanation in Release Notes