commitment: add persistent branch cache across Process() calls

[mh0lt]

Summary

• Adds a persistent LRU branch cache (65k entries, ~13MB) to HexPatriciaHashed that survives across Process() calls
• Three-level read path in branchFromCacheOrDB: warmup cache → persistent cache → DB read (populate on miss)
• Invalidation on all three fold() write paths (delete, propagate, branch) for both deferred and non-deferred modes
• pbh/pbm (persistent branch hit/miss) counters in commitment metrics and exec3 committed log line
• Cache propagated to ConcurrentPatriciaHashed subtries via SetBranchCache

Motivation

The warmup cache is ephemeral — created per Process() call and discarded after. After a flush cycle, the first block sees ~62% warmup cache hit rate as it starts cold. The persistent cache retains root-zone branch data across batches, eliminating post-flush cold starts.

Test plan

• go test ./execution/commitment/... -short passes
• make lint clean
• Binary builds and runs on mainnet parallel node
• pbh/pbm metrics visible in committed log lines
• CI

🤖 Generated with Claude Code

[awskii]

this cache can be implementation behind PatriciaContext because concurrent trie requires multiple contexts (one per subtrie) and behind this iface it can be seamlessly integrated.

[awskii]

when this cache is invalidated? what if we do unwind? what if it gets incoherent with what we have in state?

[yperbasis]

Issues

1. BranchCache.Get returns a reference, not a copy

Put defensively copies data, but Get returns the cached slice directly. If any consumer writes to the returned []byte, it corrupts the cache entry. The warmup cache's GetBranch may have the same pattern, but
the persistent cache has a much longer lifetime, making this riskier. Consider either documenting the "read-only" contract or returning a copy in Get.

2. Duplicate accessor on HexPatriciaHashed

func (hph *HexPatriciaHashed) BranchCache() *BranchCache { return hph.branchCache }
func (hph *HexPatriciaHashed) GetBranchCache() *BranchCache { return hph.branchCache }

GetBranchCache() satisfies the Trie interface. BranchCache() is an extra unexported-style accessor doing the same thing. Remove one.

3. Prefetcher ignores parent context lifecycle

NewBranchPrefetcher creates its context from context.Background(). The ctx passed to StartBranchPrefetcher is used for the trie context factory (DB connections) but not the prefetcher's goroutine lifecycle. If
the parent context is cancelled, the prefetcher keeps running until Stop() is called explicitly. This works because StopBranchPrefetcher() is called in Close() and before ComputeCommitment, but it's fragile —
any new code path that skips Stop() leaks goroutines.

Deriving from the caller's context (context.WithCancel(ctx) instead of context.Background()) would make it fail-safe.

4. return data, err where err is guaranteed nil

In hex_patricia_hashed.go branchFromCacheOrDB, after the early if err != nil { return nil, err }, the final return data, err always has err == nil. Should be return data, nil for clarity.

5. PriorityQueue fix is correct but could leak blocked Add() callers

The fix correctly eliminates the close/send race. After Drain() nils q.resultCh, any Add() caller that captured the old channel reference and is blocked on resultCh <- item will remain blocked until ctx.Done()
fires. This is fine if callers always have a bounded context, but worth a comment noting that Add() relies on context cancellation for cleanup after the queue is drained.

6. No test for BranchPrefetcher

BranchCache has thorough tests. The prefetcher (background workers, Submit/Stop lifecycle, SubmitPlainKey hashing) has none. It's the most concurrency-sensitive new code.

7. Dead flag ExperimentalConcurrentCommitment

Still referenced in 6 files (backtester.go, flags.go, default_flags.go, statecfg.go, etc.). The PR description says "cleaned up in a follow-up" — fine, but it should actually happen.

[awskii]

better keep cache behind patriciaContext and not bloating commitment

[mh0lt]

Review feedback addressed

All 7 items from @yperbasis have been fixed in commit bd59f01:

1. ✅ BranchCache.Get returns defensive copy
2. ✅ Duplicate BranchCache() accessor removed — only GetBranchCache() remains
3. ✅ BranchPrefetcher derives from parent context (context.WithCancel(parentCtx))
4. ✅ return data, nil clarity fix
5. ✅ PriorityQueue.Add cleanup documented
6. ✅ BranchPrefetcher tests added (lifecycle, context cancellation, SubmitPlainKey)
7. ✅ ExperimentalConcurrentCommitment dead flag removed from all 6 files

Rebased onto current main, lint clean, tests pass.

@awskii architectural feedback

The request to move BranchCache behind PatriciaContext is tracked as a follow-up: #20218

The key correctness concern (cache invalidation on Reset/unwind) is already addressed — cache is cleared on Reset() in commit 95c0b82.

🤖 Generated with Claude Code

[yperbasis]

Issues

1. TOCTOU race in Submit/SubmitPlainKey vs Stop — send on closed channel

branch_prefetcher.go:
func (p *BranchPrefetcher) Submit(hashedKey []byte) {
if !p.started.Load() || p.closed.Load() { // (A) check
return
}
// ... window ...
select {
case p.work <- key: // (C) panics if channel closed between A and C
default:
}
}
func (p *BranchPrefetcher) Stop() {
if p.closed.Swap(true) { return } // (B) set closed
close(p.work) // closes channel
}

If another goroutine calls Stop() between (A) and (C), p.work is closed and the send panics. The select/default only protects against blocking, not against sending on a closed channel.

Practical risk is low because StopBranchPrefetcher nils out the onHashedKey callback before calling Stop(), and in the current execution model TouchPlainKey and StopBranchPrefetcher don't run concurrently. But
this is fragile — a simple defer recover() or draining through context cancellation (don't close the channel, let workers exit via ctx.Done()) would be more robust.

2. bufferedTrieContext.PutBranch doesn't copy data/prevData

func (b *bufferedTrieContext) PutBranch(prefix, data, prevData []byte) error {
key := string(prefix)
b.writes[key] = data // stores reference, no copy
...
b.prev[key] = prevData // stores reference, no copy
return nil
}

If BranchEncoder.CollectUpdate passes a slice backed by a reused encoder buffer, subsequent encodes during the same goroutine could corrupt previously buffered entries. The non-buffered TrieContext.PutBranch
writes through to the MemBatch (which copies), so this wasn't an issue before.

Verify that EncodeBranch always returns a freshly allocated slice. If it does, this is fine. If there's any buffer reuse, add slices.Clone(data) / slices.Clone(prevData).

3. onHashedKey read/write is not synchronized

// Writer (any goroutine):
func (t *Updates) SetOnHashedKey(fn func([]byte)) { t.onHashedKey = fn }

// Reader (execution goroutine):
if t.onHashedKey != nil { t.onHashedKey(hashedKey) }

This is a data race under the Go race detector if SetOnHashedKey(nil) and TouchPlainKey ever execute on different goroutines. Currently they don't overlap in practice, but an atomic.Pointer or a mutex would
make it race-free.

4. EnableWarmupCache(!isApplyingBlocks) — undocumented behavioral change

exec3.go changes from always-on warmup cache to disabled during initial sync:
// Before:
doms.EnableWarmupCache(true)
// After:
doms.EnableWarmupCache(!isApplyingBlocks)

This is a separate optimization from the persistent cache addition. During isApplyingBlocks (initial sync), the ephemeral warmup cache is now disabled, relying entirely on the persistent cache + prefetcher.
This should be called out explicitly in the PR description since it changes sync-time behavior independent of the cache feature.

5. bufferedTrieContext.flush iterates map (non-deterministic order)

func (b *bufferedTrieContext) flush(target PatriciaContext) error {
for key, data := range b.writes {
target.PutBranch([]byte(key), data, b.prev[key])
}
...
}

If PutBranch ordering matters for state history (e.g., parent branches before children for correct prevData chain), non-deterministic map iteration could produce inconsistent results across runs. Within a
single subtrie's writes this is likely fine since each key is independent, but worth a comment explaining why ordering doesn't matter here.

Minor

• Lost observability: missAccount, missStorage, missBranch warmup cache metrics are removed and replaced only with pCacheBranchHit/pCacheBranchMiss. Warmup cache effectiveness for accounts/storage is no longer
  tracked.
• Mock Variant() in test: mockPatriciaContext implements Variant() but PatriciaContext doesn't require it — dead code in the test.
• BranchCache struct layout: The mu sync.RWMutex sits between the pinned arrays and branches *maphash.LRU. On hot paths, mu and branches could share a cache line with t4, causing false sharing between
  pinned-tier and LRU-tier operations. Consider placing mu and branches before the arrays or adding padding. Likely negligible in practice given the RWMutex acquisition cost dominates.

Verdict

The core design — three-level read path, invalidation on all fold write paths, cache cleared on Reset — is correct. The bufferedTrieContext properly isolates per-goroutine writes during ParallelHashSort. The
issues above are hardening/robustness concerns (closed-channel panic, missing copies, data race under detector) rather than fundamental correctness bugs in the current execution model. Items 1–3 should be
addressed before merge; items 4–5 are judgment calls.

[mh0lt]

CI Status After Rebase (2026-03-30)

Rebased onto latest main (ef63408ad1), 11 commits, no conflicts. CI still failing — these are real code bugs, not stale-base issues (main is fully green).

Nil Pointer Dereferences in Tests

TestCheckStateVerify — db/integrity
TestAggregator_SqueezeCommitment — db/state (+ merge index out of range [5] with length 5)
TestSharedDomain_Unwind — db/state/execctx
TestAggregatorV3_BuildFiles_WithReorgDepth — db/state

The BranchCache changes likely introduce fields that aren't initialised in test setup paths. The merge index out of range [5] with length 5 suggests the cache interferes with merge/squeeze file operations.

Also Failing

• All hive engine tests (api, cancun, exchange-capabilities, withdrawals) + rpc/compat
• Race tests: core-rpc, execution-other, execution-eest-blockchain/devnet, execution-tests
• Unit tests: ubuntu, macos, windows
• Kurtosis pectra, benchmarks

Context

We need this PR for the parallel executor — commitment is currently ~50% of batch wall-clock time during initial sync (pprof shows 39% CPU in ComputeCommitment → followAndUpdate → unfold → domain reads). Execution alone runs at 467-499 mgas/s but overall throughput is ~210 mgas/s due to commitment I/O.

🤖 Generated with Claude Code

[yperbasis]

CI is red

[Giulio2002]

I dont believe this gives you significant improvement as I have already tested this in the past. also please, if you want to do this, use the stateCache commitment cache. it is much cleaner and it is only a 4 lines diff which is currently disabled. the only reason why stateCache is enabled, is not for performance, but for whenever we merge files (many page faults).

hit/misses is the wrong metric. look at performance gains. you can test this using Milen's tool

[mh0lt]

Closed as likely this redundant work now.