rpc/jsonrpc: keep request commitment reads off shared BranchCache - #22198
rpc/jsonrpc: keep request commitment reads off shared BranchCache#22198yperbasis wants to merge 22 commits into
Conversation
Fixes #22152. A payload build runs outside the exec-module semaphore on its own read snapshot, and its SharedDomains auto-attached the aggregator-lifetime BranchCache, so the build's commitment fold read-filled the shared cache with branch bytes from that snapshot. A build outliving head progression (max-build-time self-stop; CLs pre-request payloads every slot) then wrote stale branches which, once the fresher entries were absent (LRU eviction, or the fill landing after the FCU flush refresh under last-Put-wins), every subsequent commitment on the node folded: the node computed self-consistently wrong trie roots, rejected other nodes' valid blocks and built blocks they reject — two identically-built nodes disagreeing at the tip, on both serial and parallel executors. Detach the builder's SharedDomains from the cache: builds neither populate it nor read entries newer than their snapshot; canonical validation/FCU SDs keep the warm cache. DetachBranchCache existed for exactly this hazard but had no callers. The regression test reproduces the poisoning deterministically: a real build parked via CustomTxnProvider pins a block-1 snapshot while the chain advances two blocks, the cache is cleared to model eviction, the released fold repopulates it stale, and the next canonical block — folding a branch row the stale fill covers — must validate. Before the fix it fails with the production signature (wrong trie root); recipient addresses are ground so the poisoned row is exactly the one that block folds and no per-block actor refreshes it in between.
There was a problem hiding this comment.
Pull request overview
This PR prevents payload building (which can run on a stale read snapshot, outside the exec-module semaphore) from interacting with the shared aggregator-lifetime commitment BranchCache, avoiding stale read-fills that can poison the cache and lead to wrong trie roots / consensus splits (Fixes #22152).
Changes:
- Detach the shared
BranchCachefor builderSharedDomainsso payload builds read commitment branches directly from their own snapshot and never populate the shared cache. - Update
SharedDomains.DetachBranchCachedocumentation to explicitly call out the payload builder concurrency hazard. - Add an end-to-end regression test that deterministically reproduces stale-snapshot cache poisoning and asserts canonical validation remains correct.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
execution/execmodule/exec_module_branchcache_test.go |
Adds a deterministic regression test covering stale-snapshot cache poisoning via a parked payload build. |
execution/builder/builder.go |
Detaches BranchCache in Builder.Build to prevent shared-cache reads/writes from stale builder snapshots. |
db/state/execctx/domain_shared.go |
Clarifies DetachBranchCache documentation to include the payload builder as a required caller. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…option, applied to builds and getProof
- WithoutBranchCache SharedDomains option replaces DetachBranchCache:
detachment is constructional (covers the constructor's SeekCommitment
window, which used to run attached) and cannot be dropped by a refactor.
Applied to the builder SD, the build's filterSd, and the getProof/
getWitness SDs — eth_getProof("latest") read-fills the shared cache from
an unsynchronized RPC snapshot, the same class as the builder.
- WithSequentialCommitment for payload builds: the parallel trie's
per-worker contexts open fresh transactions at the current head, not the
build's snapshot (#22209 tracks snapshot-pinned worker views).
- Test: the junk build's txn inclusion is now asserted, which exposed that
the hand-built txn lacked a recovered sender and was silently filtered
out (SetSender applied, as the txpool does). New
TestWithoutBranchCacheNeverTouchesSharedCache pins the option contract.
clearSharedBranchCache clears via AggTx directly instead of constructing
a SharedDomains. Issue URL and incident narration dropped from comments.
yperbasis
changed the title
A pre-loop build error would leave the test blocked on gate.entered until the package timeout; fail fast with a message instead.
…side SharedDomains RPC request SDs run outside the exec-module semaphore; keep them all off the shared BranchCache uniformly, as getProof/getWitness already are.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rpc/jsonrpc/eth_call.go:477
getProofallocates a per-requestSharedDomainsbut never closes it.SharedDomains.Close()is used in adjacent RPC paths (e.g.getWitness) to release the membatch/commitment context and flush request metrics; leaving it open will leak resources pereth_getProofcall.
domains, err := execctx.NewSharedDomains(ctx, tx, log.New(), execctx.WithoutDeferredBranchUpdates(), execctx.WithoutBranchCache(), execctx.WithSequentialCommitment())
if err != nil {
return nil, err
}
sdCtx := domains.GetCommitmentContext()
`getProof` creates a `SharedDomains` for its proof computation and never closes it on any path, leaking the SD's mem batch and commitment trie context on every `eth_getProof` call. The sibling `getWitness` already does `defer domains.Close()`. Found during the review of erigontech#22198. No red test: a missing `defer Close` has no practical unit-level repro (the leak only shows as memory growth across many calls), so this is a mechanical consistency fix mirroring the adjacent `getWitness` pattern.
awskii
left a comment
awskii
left a comment
There was a problem hiding this comment.
Reviewed the full diff plus callers and enclosing functions. The core fix looks correct and well-scoped: WithoutBranchCache() is nil-safe with the right zero-value default, the DetachBranchCache removal is faithful (no remaining callers), and I couldn't find a missed apply-site — every NewSharedDomains that reads commitment branches concurrently with head progression is detached, and the ones left attached run under the exec semaphore or offline (integrity/squeeze/genesis/backtester CLIs). No blocker.
Two optional nudges, both low priority:
Test isolation is assumed, not checked — execution/execmodule/exec_module_branchcache_test.go. The whole reproduction rests on the addrC block (i=2) leaving the a+b branch row untouched, enforced only by grindRecipients' hard-coded forbidden-nibble set. Nothing asserts the row is actually unchanged across that block. It's provably safe today (Amsterdam is logs/header-only, no new per-block state writer), so this isn't a bug — but reading the a+b row and require.Equal-ing it before/after block 2 would make the mechanism self-checking instead of relying on the forbidden set staying exhaustive.
Read-path SD option triple is copy-pasted 8× — WithoutDeferredBranchUpdates() + WithoutBranchCache() + WithSequentialCommitment() appears verbatim at 8 NewSharedDomains sites across 4 packages (builder.go, eth_call.go getProof/getWitness, eth_simulation.go, debug_execution_witness.go, receipts_generator.go ×2, rpchelper/commitment.go). A small execctx constructor for the read-only/one-shot case would DRY it and make the cache-safe path the default. The nearby integrity and testing SDs already carry inconsistent subsets (they omit WithoutBranchCache — fine since they're offline/non-commitment, but it shows the incantation is easy to get subtly wrong).
The follow-ups you list (#22209, #22211) already cover the deeper items I'd otherwise raise (parallel folding for builds; PutIfAbsent read-fill discipline for the opt-out attach polarity), so nothing to add there.
…ng test Assert the A+B depth-1 commitment branch row is rewritten by block 2 and left untouched by block 3, pinning the head-1 vs head-3 staleness the repro depends on instead of trusting grindRecipients' forbidden-nibble set to stay exhaustive.
|
@awskii thanks for the thorough pass — especially confirming the apply-site partition and the nil-safe zero-value default. Test isolation — done in 1181e4f. The test now reads the A+B depth-1 commitment branch row (the one Read-path option triple — I'd rather keep the sites explicit for now. As you noted, the options already appear in inconsistent subsets ( |
yperbasis
changed the title
awskii
left a comment
awskii
left a comment
There was a problem hiding this comment.
Two points inline, neither blocking.
Copilot's earlier getProof-never-closes comment is stale — eth_call.go:489 defers domains.Close().
| // WithoutSharedBranchCache keeps commitment reads within the transaction snapshot. | ||
| // WithoutSharedBranchCache disables the aggregator-scoped commitment branch cache | ||
| // and its adaptive pin controller. Cache entries are not view-bound, so callers | ||
| // whose reads can overlap cache writes from another transaction must pass this option. |
There was a problem hiding this comment.
This rule isn't followed by two request-owned SharedDomains in rpc/jsonrpc/receipts:
receipts_generator.go:333—calculatePostState && postState.CommitmentHistoryreceipts_generator.go:553— pre-Byzantium slow path,opts.CommitmentHistoryEnabled
Both pass WithoutDeferredBranchUpdates(), WithSequentialCommitment() on the live RPC temporal tx — the pre-PR option pair of the four sites fixed here, minus WithoutSharedBranchCache() — so sd.branchCache is attached at domain_shared.go:392.
SetHistoryStateReader lands after construction, so the constructor's own SeekCommitment (domain_shared.go:404) reads through getLatestMetered, which consults the cache at :1488 and read-fills at :1533. An RO request tx pinned at a lagging view, serving a pre-Byzantium receipt on a node with commitment history, publishes its KeyCommitmentState under its own step/txN into an aggregator-lifetime cache.
The installed history reader bypasses sd for branch reads afterwards, so today it's the one construction-time entry — but the consume direction is the class this PR closes, and a later latest-reader on those sites brings it back whole.
Scope lists only #22533 and #22211. Deliberate, or missed? Either move newSnapshotCommitmentDomains to rpc/rpchelper (both packages already import it) and route both sites through it, or name the issue in Scope.
| ) | ||
|
|
||
| func newSnapshotCommitmentDomains(ctx context.Context, tx kv.TemporalTx, logger log.Logger) (*execctx.SharedDomains, error) { | ||
| return execctx.NewSharedDomains(ctx, tx, logger, execctx.WithoutDeferredBranchUpdates(), execctx.WithoutSharedBranchCache(), execctx.WithSequentialCommitment()) |
There was a problem hiding this comment.
NewSharedDomains returns a non-nil *SharedDomains alongside its error on the SeekCommitment and ErrBehindCommitment paths (domain_shared.go:404-417) — its own comment there says "sd is fully initialized". This helper forwards that pair unchanged, and all four callers return nil, err with defer domains.Close() only after the check, so a persistent state/index error leaks the mem batch's ETL collectors and the commitment trie once per failed request. IsDomainAheadOfBlocks (domain_shared.go:227) closes a non-nil result before it looks at err, which is the constructor contract.
The leak predates this file, but the helper is now the single choke point where it can be closed:
sd, err := execctx.NewSharedDomains(ctx, tx, logger, ...)
if err != nil {
if sd != nil {
sd.Close()
}
return nil, err
}
return sd, nil
Problem
BranchCacheis aggregator-scoped, and its entries are not bound to a request's temporal transaction or file view. A request-owned commitment lookup that reaches this cache can consume a branch from a newer view or publish a stale read-fill from an older view. Either mixes commitment data across snapshots; a stale fill can later be consumed by canonical execution and produce a wrong root.The commitment domains used by
eth_getProof,eth_simulateV1,debug_executionWitness, andeth_getWitness/eth_getTxWitnessare request-owned and must keep their commitment reads within the request view.Canonical execution-witness collapse filtering has an additional view boundary: commitment computation writes modified branches to request-local memory, while unchanged branches must come from its installed snapshot reader. Reading transaction-latest state for an unchanged branch can use a child count produced by later blocks and incorrectly retain or discard a collapse sibling.
Change
The four RPC paths construct their
SharedDomainsthrough one snapshot-isolated helper. It selects sequential commitment, applies branch updates eagerly, and passesWithoutSharedBranchCache(). This disables shared commitment-cache reads, read-fills, flush publication, and the cache's adaptive pin controller for those domains.Execution-witness collapse filtering reads a complete post-compute commitment view. It checks local and parent memory first, then uses the installed snapshot reader for an unchanged branch. It rejects configurations that cannot provide that view: no installed reader, a history-mode reader that suppresses branch writes, pending deferred branch updates, or a staged-unwind bound on the fallback read. There is no transaction-latest fallback.
State and code caches are unchanged. Serialized canonical execution keeps using the shared
BranchCache.Scope
This PR isolates request commitment branch caching and execution-witness child-count reads. It does not change block/tag resolution or transaction selection. The separate
eth_getProoftwo-transaction snapshot skew is handled by #22533. A class-level write discipline forBranchCacheremains tracked by #22211.Performance
The request domains give up shared commitment-cache hits to prevent cross-view reads and fills. The history-backed
eth_getWitnesspath already routes its branch reads through installed readers, so using the common isolated constructor there primarily enforces the request-view invariant and prevents call-site drift.Execution-witness child-count lookups check the existing local/parent memory layers before using the installed reader. Other
SharedDomainsusers and caches are unaffected.Tests
TestGetProofIgnoresSharedBranchCacheandTestSimulateV1IgnoresSharedBranchCachepoison every persisted commitment branch in the shared cache, invoke the corresponding RPC, and verify both the result and the untouched poison entries. Each test fails without cache isolation because commitment calculation tries to decode a poisoned branch.TestSnapshotCommitmentDomainsIgnoreSharedBranchCacheverifies that the common request-domain constructor reads a persisted branch from the request transaction while leaving a poisoned shared-cache entry untouched.TestBranchChildCountReadsPostComputeViewverifies that changed branches use memory and unchanged branches use the installed reader.TestBranchChildCountRejectsIncompleteComputedViewcovers missing and history-mode readers, pending deferred updates, and staged-unwind fallback bounds.Verification
go test ./execution/commitment/commitmentdb -count=1go test ./rpc/jsonrpc -count=1make lintmake erigon integrationRelates to #22211.