fix(flat): harden the warmer resolution protocol (#12951 follow-ups) - #12980
fix(flat): harden the warmer resolution protocol (#12951 follow-ups)#12980kamilchodola wants to merge 1 commit into
Conversation
- Memoize a warmer-owned child into its parent's slot only once it is resolved, so live traversals never reach an unverified placeholder through the shared node graph instead of the SnapshotBundle gate. - Write the RLP (and mark persisted) before decoding, matching the stock resolve order; an undecodable read is then kept and not re-read. - Keep the structural hash-only-Unknown predicate in TrieNodeCache.Add alongside the warmer-owned check.
|
Claude finished @kamilchodola's task in 9m 58s —— View job Review: hardening the warmer resolution protocol
No Critical, High, or Medium findings. All three changes hold up under the checks below; four Low-severity notes, two posted inline. What I verified (rather than took on trust)1. The child-memoization gate (
2. Write-RLP-before-decode ( The new state — an unresolved warmer node that has RLP — doesn't leak anywhere:
3. The placeholder gate (
4. Tests
Low-severity notes
Caveat on validation
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| // A hash-only Unknown node is not authoritative; the persistence writers skip the same shape. | ||
| private static bool IsPlaceholder(TrieNode node) => node.NodeType == NodeType.Unknown && node.FullRlp.Length == 0; |
There was a problem hiding this comment.
Low (DRY). This is now the 7th copy of the same predicate in this assembly — PersistedSnapshotBuilder.cs:88,109, PersistedSnapshotUtils.cs:54,64,123,133 all spell out kv.Value.FullRlp.Length == 0 && kv.Value.NodeType == NodeType.Unknown, and PersistenceManager.cs:520-527,545-552 write the same test as a nested if. Since you're introducing a named helper anyway, consider making it internal static (e.g. on a TrieNodeExtensions/FlatTrieNodes helper) and reusing it at those sites, so the "skip the hash-only placeholder shape" rule has one definition.
Not blocking — the repo rule also says don't over-extract trivial one-liners, so this is a judgment call. But a 7-way duplicated invariant is the case where the named helper pays for itself.
| data = childOrRef = child; | ||
| childOrRef = child; | ||
| // An unresolved warmer placeholder is not authoritative, so it stays out of the shared node graph. | ||
| if (!child.IsWarmerOwned || child.IsWarmerResolved) data = child; |
There was a problem hiding this comment.
Low (comment precision). The gate is correct, but it only covers the case 160: hash-reference branch — the default: (inline child) branch a few lines below still does data = childOrRef = child unconditionally, and CreateInlineChild propagates MarkWarmerOwned() from a warmer-owned parent (line 1488-1490). So an unresolved warmer-owned inline child can still be memoized into a shared parent slot, which reads as an oversight next to a comment that says unresolved warmer placeholders "stay out of the shared node graph".
It's safe as written — an inline child's bytes come out of the parent's own RLP, it has no Keccak (so VerifyWarmerOwnedRlp is trivially true), and any traversal would build the identical node — so there is nothing to isolate. Worth one clause acknowledging that (e.g. "…inline children carry the parent's own bytes and need no isolation") so the asymmetry between the two branches is deliberate on its face.
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
1 participant
Changes
Follow-ups from @wurdum's review of #12951 (hardening of the warmer resolution protocol; no change to the design).
ResolveChildWithChildPath(both copies) memoizes a warmer-owned child into its parent's slot only once it is resolved. Live traversals (GetNew/SetNew) take children straight from the parent's_nodeDatawithout going throughSnapshotBundle's gate, and the live storage root is aliased into the warm-up tree (FlatStorageTree.cs:59), so this was a second door into a live read. Exposure was nil by lifecycle (the bundle is per block, so a mid-block warmer miss is persistence-backed and the path-keyed read is current); this makes it nil by construction.ResolveUnknownNode. Verification has already bound the bytes to the requested hash, so this is free; an undecodable read is then kept and not re-read from RocksDB.Unknownpredicate inTrieNodeCache.Addalongside the warmer-owned check. No producer writes that shape into the transient today (PatriciaTree.Commitgates onFullRlp.Length >= 32), but the persistence writers skip it defensively in six places and the cost is one check on the background populator thread.Not taken from the review, with reasons on the threads: a
_warmerResolveFailedMask(mismatches are rare and confined to lagging jobs, and a terminal failed state would make the voidResolveWarmerOwnedNodereturn silently through the slot door); movingIsPersistedafter verification (TrieNode(NodeType.Unknown, hash)already sets it in the constructor, so the flag is set by construction for every hash-onlyUnknownnode onmastertoo); marking the live storage root warmer-owned (pre-existing, exposure nil per-block, and it would push the live tree's own root through the CAS protocol — separate issue).Validation
Nethermind.Trie.Test477 passed / 12 skipped;Nethermind.State.Flat.Test993 passed / 10 skipped (the 14TearDown: IOException … arena_0000.binfailures are the known Windows arena file-lock artefact).49f897aee4(same diff for the touched files), against the fix(flat): isolate warmer misses and safely publish verified reads #12951 image as baseline: EXPB realblocks A/B amd64 (baseline first) / arm64 (this branch first),run_count=3, and Sync Master Validation Flat mainnet + gnosis. Draft until those are green.Types of changes
Testing
Requires testing
If yes, did you write tests?
Notes on testing
New tests: an unresolved warmer placeholder is re-looked-up rather than memoized, and memoized once resolved (
TrieNodeTests); an undecodable warmer read is kept and the resolver is called once; a hash-onlyUnknownnode in the transient is not promoted (SnapshotBundleWarmerTests).Documentation
Requires documentation update
Requires explanation in Release Notes