feat: auto-detect Engine API GetPayload version for Osaka fork#3113
feat: auto-detect Engine API GetPayload version for Osaka fork#3113tac0turtle wants to merge 1 commit intomainfrom
Conversation
GetPayload now automatically selects between engine_getPayloadV4 (Prague) and engine_getPayloadV5 (Osaka) by caching the last successful version and retrying with the alternative on "Unsupported fork" errors (code -38005). This handles Prague chains, Osaka-at-genesis chains, and time-based Prague-to-Osaka upgrades with zero configuration. At most one extra RPC call occurs at the fork transition point. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @tac0turtle's task in 1m 46s —— View job Code ReviewThe implementation is clean, well-motivated, and uses the right concurrency primitive ( Bug: Stale trace attribute in
|
| Category | Status |
|---|---|
| Code quality | ✅ Clean, idiomatic Go |
| Concurrency safety | ✅ atomic.Bool correct (minor doc caveat) |
| Error handling | ✅ Proper errors.As usage |
| Testing | |
| Tracing | ❌ Stale method attribute post-upgrade |
| Performance | ✅ At most one extra RPC call per transition (sequential) |
The tracing bug is the most impactful issue. The rest are improvements to consider before merging.
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
📝 WalkthroughWalkthroughThe changes introduce dynamic version negotiation to the Engine RPC client. When the getPayload method receives an "Unsupported fork" error from one API version, it automatically switches to the alternate version and caches the successful choice for future calls, reducing redundant fallback attempts during network operations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EngineRPCClient
participant Engine_V4 as Engine API V4
participant Engine_V5 as Engine API V5
Client->>EngineRPCClient: GetPayload()
alt useV5 flag not set (initial)
EngineRPCClient->>Engine_V4: engine_getPayloadV4
Engine_V4-->>EngineRPCClient: Unsupported fork error (-38005)
EngineRPCClient->>Engine_V5: engine_getPayloadV5
Engine_V5-->>EngineRPCClient: Payload + set useV5=true
else useV5 flag already set
EngineRPCClient->>Engine_V5: engine_getPayloadV5
Engine_V5-->>EngineRPCClient: Payload
end
EngineRPCClient-->>Client: Payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3113 +/- ##
==========================================
- Coverage 60.97% 60.93% -0.04%
==========================================
Files 113 113
Lines 11617 11617
==========================================
- Hits 7083 7079 -4
- Misses 3736 3739 +3
- Partials 798 799 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
execution/evm/engine_rpc_client.go (1)
52-64: Wrap RPC errors with call context to improve diagnosability.The current returns lose method-level context when failures bubble up. Wrapping here will make operational debugging much easier.
♻️ Proposed patch
import ( "context" "errors" + "fmt" "sync/atomic" @@ err := e.client.CallContext(ctx, &result, method, payloadID) if err == nil { return &result, nil } if !isUnsupportedForkErr(err) { - return nil, err + return nil, fmt.Errorf("%s failed for payload %x: %w", method, payloadID, err) } @@ err = e.client.CallContext(ctx, &result, altMethod, payloadID) if err != nil { - return nil, err + return nil, fmt.Errorf("%s failed after %s unsupported fork for payload %x: %w", altMethod, method, payloadID, err) }As per coding guidelines "Wrap errors with context using fmt.Errorf".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/engine_rpc_client.go` around lines 52 - 64, The RPC CallContext error returns currently lose method context; when returning errors from the calls to e.client.CallContext (both the primary call using method and the fallback using altMethod) wrap the original error using fmt.Errorf to include the method name and payloadID (e.g. fmt.Errorf("engine rpc %s payload %v: %w", method, payloadID, err)), and do the same for the fallback call (include altMethod in the message); update returns in the block around isUnsupportedForkErr(err) and the subsequent fallback so errors preserve call context for diagnosability.execution/evm/engine_rpc_client_test.go (1)
96-215: Consider table-driving the GetPayload scenario tests.These three scenario tests share setup/teardown and method-tracking logic; table-driven structure would reduce duplication and make new fork-transition cases easier to add.
As per coding guidelines "Use table-driven tests where appropriate".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/engine_rpc_client_test.go` around lines 96 - 215, The three tests TestGetPayload_PragueChain_UsesV4, TestGetPayload_OsakaChain_FallsBackToV5 and TestGetPayload_ForkUpgrade_SwitchesV4ToV5 duplicate setup/teardown and call-tracking; refactor them into a single table-driven test that iterates over cases (name, server behavior closure for fakeEngineServer, expected call sequences before/after cache or upgrade, and any state transitions like toggling osakaActive), reuse NewEngineRPCClient and client.GetPayload(ctx, engine.PayloadID{}) in a loop, and assert calledMethods for each subcase; keep unique identifiers (fakeEngineServer, NewEngineRPCClient, GetPayload, calledMethods, osakaActive) so each case can provide the server callback and expected sequences, reducing duplication and making future fork scenarios easier to add.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/evm/engine_rpc_client_test.go`:
- Around line 172-215: The test TestGetPayload_ForkUpgrade_SwitchesV4ToV5
currently only asserts success but not that the client cached the switch to V5;
instrument the fakeEngineServer handler with counters (e.g., v4Calls, v5Calls)
or a boolean flag to record how many times engine_getPayloadV4 and
engine_getPayloadV5 were invoked, call client.GetPayload as already written,
then after the first post-upgrade call assert that v4Calls == 1 and v5Calls >= 1
(first fallback used V5), and after the subsequent call assert v4Calls remains 1
and v5Calls increased (or v4Calls did not increase) to prove the client cached
and directly used V5; reference the test function
TestGetPayload_ForkUpgrade_SwitchesV4ToV5, the fakeEngineServer handler, and
client.GetPayload when adding these assertions.
---
Nitpick comments:
In `@execution/evm/engine_rpc_client_test.go`:
- Around line 96-215: The three tests TestGetPayload_PragueChain_UsesV4,
TestGetPayload_OsakaChain_FallsBackToV5 and
TestGetPayload_ForkUpgrade_SwitchesV4ToV5 duplicate setup/teardown and
call-tracking; refactor them into a single table-driven test that iterates over
cases (name, server behavior closure for fakeEngineServer, expected call
sequences before/after cache or upgrade, and any state transitions like toggling
osakaActive), reuse NewEngineRPCClient and client.GetPayload(ctx,
engine.PayloadID{}) in a loop, and assert calledMethods for each subcase; keep
unique identifiers (fakeEngineServer, NewEngineRPCClient, GetPayload,
calledMethods, osakaActive) so each case can provide the server callback and
expected sequences, reducing duplication and making future fork scenarios easier
to add.
In `@execution/evm/engine_rpc_client.go`:
- Around line 52-64: The RPC CallContext error returns currently lose method
context; when returning errors from the calls to e.client.CallContext (both the
primary call using method and the fallback using altMethod) wrap the original
error using fmt.Errorf to include the method name and payloadID (e.g.
fmt.Errorf("engine rpc %s payload %v: %w", method, payloadID, err)), and do the
same for the fallback call (include altMethod in the message); update returns in
the block around isUnsupportedForkErr(err) and the subsequent fallback so errors
preserve call context for diagnosability.
| func TestGetPayload_ForkUpgrade_SwitchesV4ToV5(t *testing.T) { | ||
| var mu sync.Mutex | ||
| osakaActive := false | ||
|
|
||
| srv := fakeEngineServer(t, func(method string) (string, int, string) { | ||
| mu.Lock() | ||
| active := osakaActive | ||
| mu.Unlock() | ||
|
|
||
| if active { | ||
| // Post-Osaka: V5 works, V4 rejected | ||
| if method == "engine_getPayloadV5" { | ||
| return minimalPayloadEnvelopeJSON, 0, "" | ||
| } | ||
| return "", -38005, "Unsupported fork" | ||
| } | ||
| // Pre-Osaka: V4 works, V5 rejected | ||
| if method == "engine_getPayloadV4" { | ||
| return minimalPayloadEnvelopeJSON, 0, "" | ||
| } | ||
| return "", -38005, "Unsupported fork" | ||
| }) | ||
| defer srv.Close() | ||
|
|
||
| client := NewEngineRPCClient(dialTestServer(t, srv.URL)) | ||
| ctx := context.Background() | ||
|
|
||
| // Pre-upgrade: V4 works. | ||
| _, err := client.GetPayload(ctx, engine.PayloadID{}) | ||
| require.NoError(t, err) | ||
|
|
||
| // Simulate fork activation. | ||
| mu.Lock() | ||
| osakaActive = true | ||
| mu.Unlock() | ||
|
|
||
| // First post-upgrade call: V4 fails, falls back to V5, caches. | ||
| _, err = client.GetPayload(ctx, engine.PayloadID{}) | ||
| require.NoError(t, err) | ||
|
|
||
| // Subsequent calls: V5 directly. | ||
| _, err = client.GetPayload(ctx, engine.PayloadID{}) | ||
| require.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Fork-upgrade test misses the cache-behavior assertion.
This test can still pass even if post-upgrade calls keep retrying V4 first; it currently validates success, not cached-method selection.
✅ Suggested assertion enhancement
func TestGetPayload_ForkUpgrade_SwitchesV4ToV5(t *testing.T) {
var mu sync.Mutex
osakaActive := false
+ var calledMethods []string
srv := fakeEngineServer(t, func(method string) (string, int, string) {
mu.Lock()
+ calledMethods = append(calledMethods, method)
active := osakaActive
mu.Unlock()
@@
_, err := client.GetPayload(ctx, engine.PayloadID{})
require.NoError(t, err)
+ mu.Lock()
+ assert.Equal(t, []string{"engine_getPayloadV4"}, calledMethods)
+ calledMethods = nil
+ mu.Unlock()
@@
_, err = client.GetPayload(ctx, engine.PayloadID{})
require.NoError(t, err)
+ mu.Lock()
+ assert.Equal(t, []string{"engine_getPayloadV4", "engine_getPayloadV5"}, calledMethods)
+ calledMethods = nil
+ mu.Unlock()
// Subsequent calls: V5 directly.
_, err = client.GetPayload(ctx, engine.PayloadID{})
require.NoError(t, err)
+ mu.Lock()
+ assert.Equal(t, []string{"engine_getPayloadV5"}, calledMethods)
+ mu.Unlock()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/evm/engine_rpc_client_test.go` around lines 172 - 215, The test
TestGetPayload_ForkUpgrade_SwitchesV4ToV5 currently only asserts success but not
that the client cached the switch to V5; instrument the fakeEngineServer handler
with counters (e.g., v4Calls, v5Calls) or a boolean flag to record how many
times engine_getPayloadV4 and engine_getPayloadV5 were invoked, call
client.GetPayload as already written, then after the first post-upgrade call
assert that v4Calls == 1 and v5Calls >= 1 (first fallback used V5), and after
the subsequent call assert v4Calls remains 1 and v5Calls increased (or v4Calls
did not increase) to prove the client cached and directly used V5; reference the
test function TestGetPayload_ForkUpgrade_SwitchesV4ToV5, the fakeEngineServer
handler, and client.GetPayload when adding these assertions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Spamoor Trace Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: ce75825 | Previous: c449847 | Ratio |
|---|---|---|---|
SpamoorSmoke - Batch.SetHeight (avg) |
13.424698795180722 us |
8.335329341317365 us |
1.61 |
SpamoorSmoke - BlockExecutor.CreateBlock (avg) |
4.484939759036145 us |
2.4564564564564564 us |
1.83 |
SpamoorSmoke - BlockExecutor.RetrieveBatch (avg) |
43.8222891566265 us |
28.236526946107784 us |
1.55 |
SpamoorSmoke - ForcedInclusionRetriever.RetrieveForcedIncludedTxs (avg) |
7.286144578313253 us |
4.023952095808383 us |
1.81 |
SpamoorSmoke - Sequencer.GetNextBatch (avg) |
33.725903614457835 us |
21.377245508982035 us |
1.58 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
GetPayload now automatically selects between engine_getPayloadV4 (Prague) and engine_getPayloadV5 (Osaka) by caching the last successful version and retrying with the alternative on "Unsupported fork" errors (code -38005).
This handles Prague chains, Osaka-at-genesis chains, and time-based Prague-to-Osaka upgrades with zero configuration. At most one extra RPC call occurs at the fork transition point.
Overview
Summary by CodeRabbit
New Features
Tests