feat: auto-detect Engine API GetPayload version for Osaka fork#3113
feat: auto-detect Engine API GetPayload version for Osaka fork#3113tac0turtle merged 4 commits 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 2m 3s —— View job Code Review: Auto-detect Engine API GetPayload version for Osaka fork
SummaryThis is a well-scoped, clean implementation. The fallback-and-cache pattern is correct, the atomic flag provides safe lock-free coordination, and the test coverage is thorough. A few observations below. Architecture & Code Quality
The implementation is minimal and correct. The One subtle point: the expression Concurrent fork transition safety: Two simultaneous calls during the transition (both reading One-way transition assumption:
The Error HandlingThe error messages are well-formed and include method name and payload ID for diagnosability:
One subtle observability gap: when the fallback succeeds (V4→V5 transition call), the tracing span records TestingThe tests in
One gap: there is no direct test asserting Minor: the Performance
Minor Nits
Overall AssessmentThe implementation is correct, thread-safe, and well-tested. The approach of using an |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds negotiated Engine API selection to the Engine RPC client: it prefers a cached method (V4 or V5), calls that method, falls back on Engine "Unsupported fork" (-38005) errors to the alternate method, caches the successful method, and exposes the resolved method via GetPayloadMethod for tracing. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EngineRPCClient
participant Engine_V4 as Engine API V4
participant Engine_V5 as Engine API V5
Client->>EngineRPCClient: GetPayload(id)
alt useV5 == false (or unset)
EngineRPCClient->>Engine_V4: engine_getPayloadV4(id)
Engine_V4-->>EngineRPCClient: error (-38005) "Unsupported fork"
EngineRPCClient->>Engine_V5: engine_getPayloadV5(id)
Engine_V5-->>EngineRPCClient: Payload
EngineRPCClient->>EngineRPCClient: set useV5 = true
else useV5 == true
EngineRPCClient->>Engine_V5: engine_getPayloadV5(id)
Engine_V5-->>EngineRPCClient: Payload
end
EngineRPCClient-->>Client: Payload or wrapped error (includes method + id)
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.90% 60.95% +0.05%
==========================================
Files 113 113
Lines 11617 11617
==========================================
+ Hits 7075 7081 +6
+ Misses 3743 3738 -5
+ Partials 799 798 -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.
![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: 954b703 | Previous: c449847 | Ratio |
|---|---|---|---|
SpamoorSmoke - BlockExecutor.CreateBlock (avg) |
4.144171779141105 us |
2.4564564564564564 us |
1.69 |
This comment was automatically generated by workflow using github-action-benchmark.
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
Looks mostly good, I think we need to update it to make the actual method that is configured to be called exposed in some way so that the tracing wrapper reports the correct method call in the span.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
execution/evm/engine_rpc_client.go (1)
50-77: Minor observability gap on failed fallback attempts.When the fallback method also fails (line 71),
GetPayloadMethod()will return the primary method name (sinceuseV5isn't updated on failure), but the trace won't reflect that both methods were attempted. This could be confusing when debugging failed fork transitions.Consider: Is the detailed error message (which includes both method names) sufficient for debugging, or should the traced method also indicate a fallback was attempted?
🤖 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 50 - 77, GetPayload currently tries a primary RPC method then falls back to an alternate but does not record that a fallback was attempted when the fallback also fails; update engineRPCClient.GetPayload to record the fallback attempt in observability (e.g., attach a span tag/log field) before returning the final error so traces reflect both getPayloadV4Method and getPayloadV5Method were tried; reference useV5, getPayloadV4Method/getPayloadV5Method and the error branch where the altMethod CallContext fails to add the trace/log annotation indicating a fallback was attempted (do not change useV5 on failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@execution/evm/engine_rpc_client.go`:
- Around line 50-77: GetPayload currently tries a primary RPC method then falls
back to an alternate but does not record that a fallback was attempted when the
fallback also fails; update engineRPCClient.GetPayload to record the fallback
attempt in observability (e.g., attach a span tag/log field) before returning
the final error so traces reflect both getPayloadV4Method and getPayloadV5Method
were tried; reference useV5, getPayloadV4Method/getPayloadV5Method and the error
branch where the altMethod CallContext fails to add the trace/log annotation
indicating a fallback was attempted (do not change useV5 on failure).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
execution/evm/engine_rpc_client.goexecution/evm/engine_rpc_client_test.goexecution/evm/engine_rpc_tracing.go
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/evm/engine_rpc_client_test.go
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
LGTM, one nit on naming 👍
2 participants
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