chore: prep for Amsterdam fork#3352
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesThe Engine API integration now supports fork-aware method selection across Prague, Osaka, and Amsterdam. Payload envelopes preserve raw Amsterdam fields, execution forwards slot numbers and execution requests, RPC fallbacks are cached, tracing reports resolved methods, and unit/E2E tests plus documentation cover the updated behavior. Engine API fork support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Execution
participant EngineRPCClient
participant EngineAPI
Execution->>EngineRPCClient: Submit forkchoice with payload attributes
EngineRPCClient->>EngineAPI: Select forkchoiceUpdated version
EngineAPI-->>EngineRPCClient: Return payload ID
Execution->>EngineRPCClient: Request payload
EngineRPCClient->>EngineAPI: Select getPayload version
EngineAPI-->>EngineRPCClient: Return Amsterdam payload envelope
Execution->>EngineRPCClient: Submit payload and execution requests
EngineRPCClient->>EngineAPI: Select newPayload version
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Claude finished @tac0turtle's task in 4m 11s —— View job Code Review: prep for Amsterdam fork
OverviewThis PR adds Amsterdam fork support to ev-node's Engine API layer. The approach is sound: a new Three issues need attention before this lands in production: one silent correctness bug (missing JSON tag), one compile-time error in the E2E test, and one missing TODO on a temporary derivation rule. Critical1.
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3352 +/- ##
==========================================
- Coverage 62.36% 62.32% -0.04%
==========================================
Files 120 120
Lines 13425 13425
==========================================
- Hits 8372 8367 -5
- Misses 4114 4119 +5
Partials 939 939
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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 (1)
execution/evm/engine_rpc_client.go (1)
147-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap NewPayload errors with the selected method.
Compute the method once and include it in both preparation and RPC errors.
Proposed fix
func (e *engineRPCClient) NewPayload(ctx context.Context, payload *EnginePayloadEnvelope, blobHashes []string, parentBeaconBlockRoot string, executionRequests [][]byte) (*engine.PayloadStatusV1, error) { + method := newPayloadMethod(payload) payloadParam, err := payload.executionPayloadParam() if err != nil { - return nil, err + return nil, fmt.Errorf("prepare %s payload: %w", method, err) } @@ - err = e.client.CallContext(ctx, &result, newPayloadMethod(payload), payloadParam, blobHashes, parentBeaconBlockRoot, requests) + err = e.client.CallContext(ctx, &result, method, payloadParam, blobHashes, parentBeaconBlockRoot, requests) if err != nil { - return nil, err + return nil, fmt.Errorf("%s failed: %w", method, err) }As per coding guidelines, “Wrap errors with context using fmt.Errorf in Go code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/evm/engine_rpc_client.go` around lines 147 - 165, Update NewPayload to compute the selected method from newPayloadMethod(payload) once before payload preparation. Wrap both executionPayloadParam preparation errors and client.CallContext RPC errors with fmt.Errorf, including the selected method as context, while preserving the existing return behavior and avoiding a second method computation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/api/engine-api.md`:
- Around line 121-123: Update engine_getPayloadV6 response examples to include
the executionRequests field, label the final array in engine_newPayloadV5
examples, and mirror the documentation changes in
docs/reference/api/engine-api.md (121-123 and 169-175) and
docs/ev-reth/engine-api.md (140-144). In execution/evm/README.md (62-64), note
that ev-node forwards both executionPayload and executionRequests.
---
Nitpick comments:
In `@execution/evm/engine_rpc_client.go`:
- Around line 147-165: Update NewPayload to compute the selected method from
newPayloadMethod(payload) once before payload preparation. Wrap both
executionPayloadParam preparation errors and client.CallContext RPC errors with
fmt.Errorf, including the selected method as context, while preserving the
existing return behavior and avoiding a second method computation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 241c8f25-5c6d-4f62-9a12-b089aab44040
📒 Files selected for processing (13)
docs/ev-reth/engine-api.mddocs/reference/api/engine-api.mdexecution/evm/README.mdexecution/evm/engine_payload.goexecution/evm/engine_rpc_client.goexecution/evm/engine_rpc_client_test.goexecution/evm/engine_rpc_tracing.goexecution/evm/execution.goexecution/evm/execution_payload_attrs_test.goexecution/evm/execution_reconcile_test.goexecution/evm/proposer_test.goexecution/evm/test/engine_fork_upgrade_test.goexecution/evm/test/test_helpers.go
| "excessBlobGas": "0x0", | ||
| "slotNumber": "0x...", | ||
| "blockAccessList": [] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n## File sizes\n'
wc -l docs/reference/api/engine-api.md docs/ev-reth/engine-api.md execution/evm/README.md
printf '\n## Relevant excerpts: docs/reference/api/engine-api.md\n'
sed -n '100,190p' docs/reference/api/engine-api.md
printf '\n## Relevant excerpts: docs/ev-reth/engine-api.md\n'
sed -n '120,170p' docs/ev-reth/engine-api.md
printf '\n## Relevant excerpts: execution/evm/README.md\n'
sed -n '40,90p' execution/evm/README.md
printf '\n## Search executionRequests across docs\n'
rg -n "executionRequests|blockAccessList|slotNumber|engine_forkchoiceUpdatedV4|engine_newPayloadV5" docs execution -SRepository: evstack/ev-node
Length of output: 13672
🌐 Web query:
Ethereum executionRequests Engine API Prague newPayloadV5 forkchoiceUpdatedV4 executionRequests docs
💡 Result:
In the Ethereum Prague upgrade, the Engine API introduced support for execution requests, a mechanism defined by EIP-7685 that allows the execution layer to trigger protocol-level operations [1][2]. These requests are communicated between the execution layer and consensus layer through updated Engine API methods [1]. Key Engine API changes for Prague include: 1. New Method Versions: The Prague upgrade introduced engine_newPayloadV4 and engine_getPayloadV4 to support the inclusion and validation of execution requests [1][3][2]. 2. Execution Requests Parameter: In engine_newPayloadV4, an executionRequests parameter was added [1][2]. This parameter is an array of byte arrays, where each element represents a request [1][2]. Each request is formatted as a byte array beginning with a single-byte request type, followed by the specific request data [2]. 3. Validation and Ordering: Requests must be ordered by their request type in ascending order [2]. The execution layer computes a commitment to these requests (the requestsRoot), which is included in the block header and validated against the data provided via the Engine API [1][4][2]. Regarding the specific methods mentioned in your query: - newPayloadV5 and forkchoiceUpdatedV4: These represent later versions of the Engine API methods introduced in subsequent upgrades (e.g., the Amsterdam fork) to support additional features like block access lists and further protocol evolutions [5][6]. They are not the methods used for the initial Prague execution requests implementation, which utilized V4 versions [1][2]. - Documentation: Official specifications for these changes are maintained in the ethereum/execution-apis repository, specifically within the prague.md and related specification documents [7][8][6]. In summary, while Prague established the foundation for execution requests using V4 engine methods, V5 (newPayloadV5) and V4 (forkchoiceUpdatedV4) methods represent subsequent API evolutions required for later network upgrades [5][6].
Citations:
- 1: https://deepwiki.com/ethereum/execution-apis/3.3.4-prague-(execution-requests)
- 2: https://deepwiki.com/ethereum/execution-apis/5.5-execution-payloads-and-attributes
- 3: Added engine_getPayloadV4 and engine_newPayloadV4 for Prague (EIP-7002) ethereum/execution-apis#528
- 4: engine: Make execution requests a sidecar, take 2 ethereum/execution-apis#591
- 5: chore: prep for Amsterdam fork #3352
- 6: https://github.com/jiayaoqijia/eth2030/blob/master/docs/DESIGN.md
- 7: https://github.com/ethereum/execution-apis/blob/main/src/engine/prague.md
- 8: https://github.com/ethereum/execution-apis/blob/869b7f062830ba51a7fd8a51dfa4678c6d36b6ec/src/engine/prague.md
🏁 Script executed:
set -euo pipefail
printf 'docs/reference/api/engine-api.md\n'
grep -n "executionRequests\|blockAccessList\|slotNumber" -n docs/reference/api/engine-api.md || true
printf '\ndocs/ev-reth/engine-api.md\n'
grep -n "executionRequests\|blockAccessList\|slotNumber" -n docs/ev-reth/engine-api.md || true
printf '\nexecution/evm/README.md\n'
grep -n "executionRequests\|executionPayload" -n execution/evm/README.md || trueRepository: evstack/ev-node
Length of output: 1320
Document executionRequests in the Engine API docs
engine_getPayloadV6 responses omit executionRequests, and the engine_newPayloadV5 examples leave the last array unlabeled. Add the field/label in both Engine API docs and note in execution/evm/README.md that ev-node forwards both executionPayload and executionRequests.
📍 Affects 3 files
docs/reference/api/engine-api.md#L121-L123(this comment)docs/reference/api/engine-api.md#L169-L175docs/ev-reth/engine-api.md#L140-L144execution/evm/README.md#L62-L64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/api/engine-api.md` around lines 121 - 123, Update
engine_getPayloadV6 response examples to include the executionRequests field,
label the final array in engine_newPayloadV5 examples, and mirror the
documentation changes in docs/reference/api/engine-api.md (121-123 and 169-175)
and docs/ev-reth/engine-api.md (140-144). In execution/evm/README.md (62-64),
note that ev-node forwards both executionPayload and executionRequests.
Source: MCP tools
1 participant
Overview
Summary by CodeRabbit
New Features
slotNumberandblockAccessList) end-to-end.Bug Fixes / Improvements
Documentation