Skip to content

Commit 77c7800

Browse files
committed
execution: trim over-long comments to the load-bearing why
Address the #22154 review's comment-length notes: condense the BranchCache doc, the reset_stages branchCache-clear note, and the ValidateChain SetParent note to their load-bearing invariants. The design-doc detail (concurrency walkthrough, responsibility split, disk counters, two-role breakdown, cherry- pick note) moves out of source per the comment policy. No behaviour change.
1 parent 0950d78 commit 77c7800

3 files changed

Lines changed: 19 additions & 142 deletions

File tree

execution/commitment/branch_cache.go

Lines changed: 11 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -46,115 +46,18 @@ func isCommitmentStateKey(prefix []byte) bool {
4646
return bytes.Equal(prefix, KeyCommitmentState)
4747
}
4848

49-
// BranchCache stores commitment-trie branch data:
49+
// BranchCache stores commitment-trie branch data: a bounded LRU tail plus a
50+
// single never-evicted slot for the root branch (a length-0 / no-key prefix).
51+
// Aggregator-scope (one instance per Domain), pulled via BranchCacheProvider and
52+
// plumbed to the trie through InitializeTrieAndUpdates. It is a passive store —
53+
// the trie walker/encoder drive all reads and writes; the cache never fetches
54+
// state itself.
5055
//
51-
// - Bounded LRU tail with configurable capacity (eviction is well-defined,
52-
// suitable for long-lived caching across many Process calls without
53-
// unbounded memory growth).
54-
// - Single pinned slot for the root branch (always hottest, always present
55-
// once populated, never subject to LRU eviction). Compact prefix of
56-
// length 0 (or single-byte "no-key" form) targets this slot.
57-
//
58-
// Lifetime: aggregator-scope (one instance per Domain). SharedDomains
59-
// pulls the instance via BranchCacheProvider on the AggregatorRoTx;
60-
// commitment-context plumbs it through to the trie via
61-
// InitializeTrieAndUpdates. The previous WarmupCache type (per-Process,
62-
// duplicating account/storage/branch caching above this layer) was
63-
// deleted in the WarmupCache consolidation; BranchCache is now the
64-
// single branch cache.
65-
//
66-
// # Responsibility split (architectural)
67-
//
68-
// The cache is a passive store. Reads and writes are driven by the
69-
// trie walker / encoder; the cache itself never reaches into the
70-
// underlying state.
71-
//
72-
// - BranchCache: passive store of branch bytes.
73-
// Doesn't fetch anything.
74-
// - Branch warmer (warmuper.go): narrow scope — pre-fetches
75-
// *branches* along touched-key paths via SD.GetLatest. No
76-
// account/storage prefetch — that conflated branch warm-up with
77-
// leaf-data fetch. If a fold needs leaf data the trie walker
78-
// fetches it directly (or it's already in Updates / memoized as
79-
// stateHash).
80-
// - Trie walker, block-processing path: receives Updates from the
81-
// executor, folds them. Memoized stateHashes serve siblings; new
82-
// values come from Updates. Doesn't reach into leaf data via
83-
// prefetch.
84-
// - Trie walker, witness / proof generation path: walks the trie
85-
// structure and *needs* to fetch state to materialize the proof.
86-
// This is the walker's responsibility — it drives its own reads
87-
// against SD. If that path turns out to be cold-bound on real
88-
// workloads it may indicate a need for separate account / storage
89-
// caches (the `add_execution_context_with_caches` work has a
90-
// reference design for these). Treat that as a separate concern
91-
// from this BranchCache — different scope, different lifetime,
92-
// different invalidation. Do not regrow the branch warmer's
93-
// scope to cover it.
94-
//
95-
// The disk_sto / disk_acc counters on the [commitment][cache-fp] log
96-
// line surface any fall-through where the trie compute reaches the
97-
// underlying ctx.Account / ctx.Storage paths. On block-processing
98-
// workloads they should remain zero; non-zero values signal a
99-
// memoization gap or a missing walker-side prefetch.
100-
//
101-
// # Concurrency contract — caller invariants
102-
//
103-
// Internally, the LRU tail is thread-safe (hashicorp/golang-lru/v2) and
104-
// the pinned root slot is an atomic.Pointer. So any combination of
105-
// concurrent Get / Put / Invalidate is mechanically safe — no panics, no
106-
// torn reads. But "mechanically safe" is NOT the same as "logically
107-
// consistent across writers." The cache is designed to be used under one
108-
// caller invariant:
109-
//
110-
// - Single writer per prefix at any moment. The cache does not coordinate
111-
// concurrent writes to the same key — last-Put-wins semantics, with no
112-
// guarantee that the winning value is the one the application wanted.
113-
//
114-
// # Concurrency contract — how the existing concurrent trie satisfies it
115-
//
116-
// The current ConcurrentPatriciaHashed (parallel commitment calculator)
117-
// satisfies that invariant by construction:
118-
//
119-
// - Mounts partition the prefix space by FIRST NIBBLE. Mount N's
120-
// encoder only writes branches whose key starts with [0x0N ...].
121-
// Different mounts therefore never write to the same prefix.
122-
// (See hex_concurrent_patricia_hashed.go: NewConcurrentPatriciaHashed
123-
// creates 16 mounts via SpawnSubTrie; each mount has its own HPH,
124-
// own BranchEncoder, own PatriciaContext / roTx.)
125-
//
126-
// - Root branch (prefix [0x00]) is written by the single root fold
127-
// that runs SEQUENTIALLY after errgroup.Wait() in ParallelHashSort.
128-
// One writer for the pinned root slot.
129-
//
130-
// - Mount→root grid roll-up is mutex-protected via
131-
// ConcurrentPatriciaHashed.rootMu — but that updates IN-MEMORY grid
132-
// cells, not the cache. The cache only sees the eventual root
133-
// branch when the post-Wait root fold encodes it.
134-
//
135-
// # Concurrency contract — what future parallel fold work must preserve
136-
//
137-
// A future parallel tree-reduce fold would change the picture: the parent
138-
// fold (incl. root) would no longer be a single post-Wait sequential pass.
139-
// Multiple goroutines would compute parent branches in parallel as their
140-
// children complete. This MUST not violate "single writer per prefix" —
141-
// any future Stage F design needs an explicit per-prefix coordination layer
142-
// (atomic counter on parent "children remaining"; only the last-decrementer
143-
// writes the parent). That coordination belongs at the orchestrator layer;
144-
// the cache itself does NOT add per-prefix locking because that would be
145-
// wasted work for the current architecture.
146-
//
147-
// If you are implementing parallel fold (or any other architecture that
148-
// breaks the "single writer per prefix" invariant), do NOT relax the
149-
// invariant by adding internal locking to the cache. Add the
150-
// coordination at the orchestrator layer where the partitioning logic
151-
// lives. The cache stays simple; the orchestrator owns the discipline.
152-
//
153-
// Likewise if you change the prefix partitioning (e.g. by-second-nibble
154-
// mounts, depth-based partitioning, anything other than first-nibble),
155-
// re-validate that distinct workers continue to write disjoint prefix
156-
// spaces. Re-read the partitioning code in
157-
// hex_concurrent_patricia_hashed.go and confirm.
56+
// Concurrency: the LRU tail and the atomic-pointer root slot make any mix of
57+
// concurrent Get/Put/Invalidate mechanically safe, but the cache does not
58+
// coordinate writers — callers must ensure a single writer per prefix
59+
// (last-Put-wins otherwise); add any such coordination at the orchestrator, not
60+
// by locking the cache.
15861
type BranchCache struct {
15962
// Root tier — single slot for the root branch (always hottest, always
16063
// present). Atomic-pointer access so no lock is needed for the hot

execution/execmodule/exec_module.go

Lines changed: 6 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -550,36 +550,12 @@ func (e *ExecModule) ValidateChain(ctx context.Context, blockHash common.Hash, b
550550
}
551551
var tx kv.TemporalRwTx = doms.BlockOverlay()
552552

553-
// Chain whenever a currentContext exists, not only when head-extending
554-
// (header.ParentHash == head): fork-payload caching needs the parent link
555-
// too — see the two-role breakdown below.
556-
//
557-
// Chain the validation SD to the latest in-memory canonical generation:
558-
// e.currentContext when present, otherwise the newest in-flight commit
559-
// generation (gate item 2 — the prior FCU cleared currentContext and
560-
// handed its SD to the background commit).
561-
//
562-
// The parent link serves two roles:
563-
//
564-
// 1. Head-extending payloads read the canonical generation's
565-
// not-yet-committed domain state instead of stale MDBX.
566-
//
567-
// 2. Fork payloads: unwindToCommonCanonical below must build an unwind
568-
// set, and the diffsets of the canonical blocks it unwinds live in
569-
// the canonical generation's pastChangesAccumulator — reachable only
570-
// through this parent link (GetDiffset chains to the parent). Without
571-
// it the unwind silently runs with no unwind set, leaving the
572-
// BranchCache unmasked and corrupting the computed root.
573-
//
574-
// For a fork payload the parent does NOT shadow the unwound base: once
575-
// unwindToCommonCanonical has run, doms.mem.unwindChangeset holds every
576-
// key the unwound canonical blocks touched, and TemporalMemBatch.getLatest
577-
// resolves those from the unwind set before ever consulting the parent.
578-
//
579-
// Cherry-pick note: the upstream commit also chained to e.latestGen()
580-
// (the gate-2 in-flight commit generation) when currentContext is nil;
581-
// that generation chain is not on this branch, so currentContext is the
582-
// only canonical generation here.
553+
// Chain the validation SD to the canonical generation (e.currentContext) for
554+
// any payload with a parent, not just head-extending ones: head-extending
555+
// payloads read its not-yet-committed domain state instead of stale MDBX, and
556+
// fork payloads reach the canonical generation's pastChangesAccumulator (via
557+
// GetDiffset's parent chain) to build the unwind set — without the link the
558+
// unwind runs empty, leaving the BranchCache unmasked and corrupting the root.
583559
if e.currentContext != nil {
584560
doms.SetParent(e.currentContext)
585561
}

execution/stagedsync/rawdbreset/reset_stages.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -188,10 +188,8 @@ func ResetExec(ctx context.Context, db kv.TemporalRwDB) (err error) {
188188
}
189189

190190
// Wiping the commitment table leaves the aggregator's in-memory branchCache
191-
// referencing trie nodes that no longer exist on disk. A subsequent from-0
192-
// re-exec then reads those stale nodes when computing block 0's commitment
193-
// and produces a wrong trie root under parallel exec.
194-
// Drop the cache so it repopulates from the freshly-wiped table.
191+
// pointing at now-deleted trie nodes; drop it so a from-0 re-exec repopulates
192+
// from the wiped table instead of computing a wrong root off stale nodes.
195193
branchCacheCleared := false
196194
if hasAgg, ok := db.(dbstate.HasAgg); ok {
197195
if agg, ok := hasAgg.Agg().(*dbstate.Aggregator); ok {

0 commit comments

Comments
 (0)