execution, node: test RPC state across unwind phases

[yperbasis]

Part of #21860. Complements the lower-level cache tests in #23005 with running-node coverage for the RPC/unwind failure class in #22463.

Why

The missing safety net was the complete interaction between HTTP RPC requests and an Engine API reorg:

• three tagged requests bind to old head A;
• forkchoice unwinds to ancestor B;
• newly bound latest requests observe B through overlay publication, durable commit, and database fallback;
• the delayed A requests may finish from their valid MVCC views, but must not refill StateCache with dead-fork data.

Timing sleeps cannot define these windows reliably, so the test pauses execution at synchronous lifecycle boundaries.

Scenario

sequenceDiagram
    participant CL as Mock CL
    participant FCU as ExecModule FCU
    participant OldRPC as RPC views bound to A
    participant Overlay as SharedDomains overlay
    participant DB as MDBX
    participant NewRPC as newly bound RPC

    CL->>FCU: forkchoiceUpdated(A)
    FCU->>Overlay: publish A
    OldRPC->>OldRPC: bind tagged views and pause before reading
    FCU->>DB: commit A
    FCU->>Overlay: clear A

    CL->>FCU: forkchoiceUpdated(B)
    FCU->>FCU: unwind to B
    Note over FCU,DB: committed A remains visible until B is published
    FCU->>Overlay: publish B
    NewRPC->>Overlay: assert B before commit
    FCU->>DB: commit B
    NewRPC->>Overlay: assert B after commit
    FCU->>Overlay: clear B
    NewRPC->>DB: assert durable B

    OldRPC-->>OldRPC: finish from the old A views

    CL->>FCU: forkchoiceUpdated(C)
    FCU->>Overlay: publish transaction-free C
    NewRPC->>Overlay: repeat canonical reads through C
    Note over NewRPC,Overlay: untouched keys probe StateCache for stale A refill
    FCU->>Overlay: clear C

Loading

The B overlay publication is the visibility switch. Before it, newly bound requests see committed A; from publication onward, they must consistently see B. The final C step is an explicitly transaction-free child of B, built through testing_buildBlockV1 with a non-nil empty transaction list. This bypasses the txpool, so unwound transactions cannot enter C and its fresh overlay cannot answer the probed keys from its own writes. Repeated reads therefore exercise the shared StateCache and expose any stale A refill.

Assertions

Boundary	What the test proves
Tagged A RPC views bound	Exactly the delayed storage, code, and nonce requests are pinned before their first read
B unwind complete	Replacement state is still private; committed A remains visible
B overlay published	Dead-fork storage is absent while retained account, code, and nonce remain
B committed and overlay cleared	The same result survives commit and MDBX fallback
Delayed A views released	Old values may return, but cannot repopulate canonical cache entries
C transaction-free overlay cache probes	Repeated reads return B-equivalent state, proving the shared cache was not poisoned by A

Each lifecycle assertion uses a separate persistent StateChurn contract, so an earlier RPC cache fill cannot mask a later path. Their selected slot is absent at B and written only on A, exercising removal of dead-fork storage while preserving the account. A fifth contract exists only on A, exercising complete account and code removal. The generated A storage value is required to be non-zero, so the distinction cannot silently collapse. Non-zero value restoration remains covered by the broader StateChurn suite and #23005.

Design and production impact

• StateTransitionObserver exposes five inline boundaries: RPC view bound, unwind complete, overlay published, commit complete, and overlay cleared.
• The delayed calls use a dedicated tagged RPC client. The RPC-view barrier accepts only matching server-side peer contexts, so unrelated cache views cannot consume its slots.
• Publication, commit, and clear observations are paired with the FCU that actually published the overlay. An FCU response is not treated as proof that deferred teardown has finished.
• The observer is optional and nil in production. RPC view observation is added by an engineapitester cache decorator, leaving the production Cache.View path unchanged.
• The real overlay event bus remains drop-tolerant; it is not reused as a blocking test barrier.
• Node shutdown drains detached forkchoice work before closing the database, preventing an in-flight transaction from blocking DB shutdown.
• The test uses real HTTP eth_getStorageAt, eth_getCode, and eth_getTransactionCount calls and real Engine API forkchoice updates. Channels establish ordering; timeouts only bound failures.
• The testing RPC namespace is enabled only for this engineapitester fixture so C can bypass the txpool; production RPC configuration is unchanged.
• The obsolete rpc/rpchelper: stale cached pending block pins "pending" reads to an old height, blocking new txns with "nonce too low" #22299 latest-nonce workaround is removed; rpc/rpchelper: invalidate cached pending block once the chain moves past it #22326 now supplies the normal pending-nonce behavior.

Relationship and scope

#23005 tests cache admission and commit/publication interleavings directly. This PR tests the surrounding HTTP RPC, Engine API, overlay, commit, and fallback wiring. It intentionally does not duplicate every internal midpoint.

Coverage is a deterministic in-process full node. Crash windows, randomized reorg schedules, real-CL/Kurtosis runs, and the exact DB-commit/cache-publication midpoint remain separate #21860 layers.

Review order

1. execution/execmodule/state_transition.go and forkchoice.go: boundary semantics and placement.
2. node/eth/backend.go and execution/engineapi/engineapitester: full-node wiring, test-only RPC/cache hooks, empty-payload construction, and shutdown draining.
3. execution/engineapi/engine_api_rpc_unwind_test.go: reorg scenario and cache-refill assertions.
4. engine_api_state_churn_reorg_test.go: shared churn helper and pending-nonce path.

[Copilot]

Pull request overview

Adds deterministic node-level coverage for RPC state-cache correctness across forkchoice unwind phases.

Changes:

• Adds synchronous state-transition observers and test wiring.
• Tests RPC reads across overlay, commit, database fallback, and stale-view completion.
• Ensures shutdown fully drains detached execution work.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
node/eth/backend.go	Wires observers, cache decorators, and execution draining.
execution/execmodule/state_transition.go	Defines lifecycle observation points.
execution/execmodule/forkchoice.go	Emits observations at unwind and publication boundaries.
execution/execmodule/execmoduletester/exec_module_tester.go	Exposes observer configuration to tests.
execution/execmodule/execmoduletester/exec_module_tester_test.go	Verifies lifecycle observations.
execution/execmodule/exec_module.go	Adds construction options, cache constructor, and draining.
execution/execmodule/exec_module_internal_test.go	Tests execution draining.
execution/engineapi/engineapitester/state_cache.go	Observes RPC cache-view binding.
execution/engineapi/engineapitester/state_cache_test.go	Verifies synchronous view observation.
execution/engineapi/engineapitester/engine_api_tester.go	Wires instrumentation into full-node tests.
execution/engineapi/engine_api_state_churn_reorg_test.go	Extracts the shared churn helper.
execution/engineapi/engine_api_rpc_unwind_test.go	Adds the end-to-end unwind regression test.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[AskAlexSharov]

Code review of the current head. Nice test — the lifecycle barriers are much better than sleeps. Findings below, ordered by severity.

1. Ethereum.Stop() lost its shutdown bound — node/eth/backend.go:1474

Drain() is WaitIdle(context.Background()), replacing a 5s-bounded WaitIdle. e.backgroundCtx is the ctx passed to eth.New, not sentryCtx, so sentryCancel() does not cancel it. A background FCU sitting in runPostForkchoice → agg.CollateAndPrune (file collation, minutes, not promptly cancellable) now blocks the whole remainder of Stop(), and there is no log line while it waits. Every other wait in Stop() is bounded (privateAPI 1s, read-ahead 5s, KZG 30s) — this one should be too, with a warning on expiry.

2. Drain()'s precondition is not met by Stop() — execution/execmodule/exec_module.go:333

The docstring says "Callers must stop producers first". Stop() only signals the engine-API/JSON-RPC servers via sentryCancel() and never joins them, so a forkchoiceUpdated already in the accept queue can start a new updateForkChoice after Drain() returns, open a BeginTemporalRo, and hang chainDB.Close() in waitTxsAllDoneOnClose — the exact hang Drain exists to prevent.

3. observe() treats ctx.Done() as an implicit release — execution/engineapi/engine_api_rpc_unwind_test.go:350

select {
case <-proceed:
case <-ctx.Done():
}

FCU observations get the FCU ctx, the RPC observation the HTTP request ctx. If either is cancelled (client disconnect, teardown, t.Context() cancellation after an earlier failure) the barrier lifts silently: the FCU proceeds to commit/clear while the main goroutine is still between wait() and release(), so the boundary assertions race against real state changes and can pass for the wrong reason. Better to record that the release came from cancellation and fail the test.

4. RPCViewBound holds are keyed only by point + count — execution/engineapi/engine_api_rpc_unwind_test.go:132

rpcViewObserverCache wraps the cache shared by the whole embedded rpcdaemon. Any other View caller (filters, eth_call, a background subscription) consumes one of the three slots; the hold then hits 0 and is deleted before all three delayed requests have blocked. The unblocked one binds at the wrong lifecycle point and the "delayed A view must not refill StateCache" assertions become vacuous while still green. Consider keying the hold on something request-identifying, or asserting the observed count is exactly 3 at release time.

5. TestStateTransitionObserver asserts an invariant the code does not hold — execution/execmodule/execmoduletester/exec_module_tester_test.go:78-79

published == commitComplete == cleared is not guaranteed:

• dispatchNotificationsFromOverlay returns (true, err) when Dispatch fails after PublishOverlay → published+cleared, no commit.
• dispatcher == nil / accum == nil / overlay == nil return (false, nil) but CommitComplete still fires later → commit without publish.
• The isDomainAheadOfBlocks branch (forkchoice.go:505) calls currentContext.Commit and returns without emitting CommitComplete.
• unwindIfNeeded returning a non-nil result emits UnwindComplete and then returns with no publish/commit/clear.

Any of these turns the test red for a reason unrelated to the observer contract.

6. Removing the #22299 nonce pinning leaves the failure outside the retry — execution/engineapi/engine_api_state_churn_reorg_test.go:298

require.Eventually around churn.Poke only retries submission errors. The failure mode the pinning guarded is a stale, too-high pending nonce: Poke returns err == nil, the retry exits on the first attempt, the txn stays queued, BuildCanonicalBlock mines without it, and VerifyTxnsInclusion fails hard with no retry. Worth confirming #22326 actually removes that window, or moving VerifyTxnsInclusion inside the Eventually.

7. expectedStorage can legitimately be zero — execution/engineapi/engine_api_rpc_unwind_test.go:98

stateChurnPokeValue is keccak(...) % 3. With stateChurnSeed = 0 it is 1 today, but nothing pins that: if it ever hashes to 0, assertContractRPCStatePresentWithStorage degenerates into assertContractRPCStatePresentWithZeroStorage and every pre/post-reorg storage distinction the test exists to prove disappears — silently green. A require.NotZero(t, expectedStorage) costs one line. (This is why commit 1's firstNonZeroStateChurnSeed() existed; the constant replaced it without the guard.)

8. RPC client leak + unchecked assertion — execution/engineapi/engine_api_rpc_unwind_test.go:100

http.DefaultTransport.(*http.Transport) panics if anything in the test binary replaces DefaultTransport. Separately, the defer rpcClient.Close() dropped in the second commit was not replaced — only transport.CloseIdleConnections is registered, so the client itself is never closed.

9. Altitude — WithRPCStateCacheDecorator in the production constructor

eth.New now accepts a func(kvcache.Cache) kvcache.Cache that can swap the RPC state cache for anything, purely so one integration test can count View calls. WithStateTransitionObserver on NewExecModule is the narrower, already-present seam; the RPC side could be observed by having the test drive kvcache.Cache directly rather than opening a general decorator hook in node/eth.

Points 1 and 2 are the only ones that affect production behaviour; the rest are test robustness.

[yperbasis]

@AskAlexSharov Thanks for the detailed review. I pushed 2510fc0 with the test hardening that I agree is needed:

• Point 4: the delayed storage, code, and nonce calls now use a dedicated RPC client with a unique user-agent. The RPCViewBound hold filters the server-side peer context, so unrelated cache views cannot consume its three slots. A focused test covers matched and unmatched observations.
• Point 7: the test now requires expectedStorage to be non-zero before using it as the pre-unwind value.
• Point 8: the unchecked http.DefaultTransport.(*http.Transport) assertion is gone, while idle HTTP connections are still cleaned up. I did not add rpcClient.Close() because rpc.Client.Close intentionally returns immediately for HTTP clients.

I do not think the remaining points require changes in this PR:

• Points 1 and 2 rely on the exec module having a different background context. The ctx passed to NewExecModule is the same context stored as s.sentryCtx; Stop() cancels it first, stops the private API and engine, waits for background components, and only then drains execution. The unbounded drain is intentional: continuing into chainDB.Close() while execution still owns a transaction would restore the shutdown race this change prevents. A timeout could be useful for diagnostics, but it cannot safely permit shutdown to continue.
• Point 3: ctx.Done() is the teardown escape path. A canceled RPC or FCU cannot make a clean run pass because every asynchronous result is awaited and required to have no error; cancellation of t.Context() also means the test has already failed. Waiting only for an explicit release could instead wedge cleanup after an earlier failure.
• Point 5: TestStateTransitionObserver drives one successful FCU path. Its equality checks are scoped to that path, not a claim that every early-return or error path emits the same events.
• Point 6: fd0a084 (rpc/rpchelper: invalidate cached pending block once the chain moves past it #22326) is an ancestor of this branch. Its root-cause validation explicitly ran the churn test with the nonce workaround removed and fixed the stale pending-block nonce that caused rpc/rpchelper: stale cached pending block pins "pending" reads to an old height, blocking new txns with "nonce too low" #22299.
• Point 9: driving the cache directly would stop exercising the RPC-to-cache binding that this integration test is meant to protect. The decorator is inert unless explicitly supplied and wraps only the RPC cache created by eth.New.

Validation on the updated head: 10 consecutive normal runs, 3 consecutive race-detector runs, repeated clean lint, and successful erigon plus integration builds.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.