chore: prep for Amsterdam fork

[tac0turtle]

Overview

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b5f12d12-dc3d-4de5-a69c-f393211200bf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch marko/reth_2.3

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @tac0turtle's task in 4m 11s —— View job

---

Code Review: prep for Amsterdam fork

• Gather context (AGENTS.md, PR diff)
• Review execution/evm/engine_payload.go (new file)
• Review execution/evm/engine_rpc_client.go (major changes)
• Review execution/evm/execution.go
• Review tracing, tests, and docs
• Post final review

---

Overview

This PR adds Amsterdam fork support to ev-node's Engine API layer. The approach is sound: a new EnginePayloadEnvelope wrapper preserves blockAccessList through the JSON round-trip (the critical correctness concern), the RPC client auto-detects and caches the supported method version with fallback, and the slotNumber attribute is always included but stripped for V3 calls. Test coverage for the version-selection and fallback logic is thorough.

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.

---

Critical

1. engine_payload.go — Missing json:"blobsBundle" tag on both marshal and unmarshal structs

In both the UnmarshalJSON and MarshalJSON local struct definitions, BlobsBundle has no JSON tag:

// UnmarshalJSON (line 28-33)
type executionPayloadEnvelope struct {
    ExecutionPayload json.RawMessage `json:"executionPayload"`
    BlockValue       *hexutil.Big    `json:"blockValue"`
    BlobsBundle      *engine.BlobsBundle          // ← missing tag!
    ...
}

// MarshalJSON (line 72-77) — same issue

Go will use the field name BlobsBundle (PascalCase) as the JSON key, but the Engine API uses "blobsBundle" (camelCase). This silently drops blob bundle data on every Engine API call — blobs will never be deserialized from EL responses, and will never be included in payloads sent to the EL.

Fix: Add `json:"blobsBundle"` to both struct definitions.

Fix this →

---

2. test/engine_fork_upgrade_test.go line 103 — Type mismatch in E2E test

stateRoot, err := executionClient.InitChain(...)     // stateRoot: []byte
...
for _, step := range steps {
    t.Run(step.name, func(t *testing.T) {
        nextStateRoot, err := executionClient.ExecuteTxs(..., stateRoot)
        require.NoError(t, err)
        stateRoot = nextStateRoot   // ← compile error: ExecuteResult != []byte
    })
}

InitChain returns ([]byte, error), so stateRoot is []byte. ExecuteTxs returns (execution.ExecuteResult, error) — a struct with UpdatedStateRoot []byte and NextProposerAddress []byte. The assignment stateRoot = nextStateRoot is a type error that will be caught when building with -tags evm.

Fix:

nextResult, err := executionClient.ExecuteTxs(ctx, nil, step.height, time.Unix(int64(step.timestamp), 0), stateRoot)
require.NoError(t, err)
stateRoot = nextResult.UpdatedStateRoot

Fix this →

---

Medium

3. execution.go line 1081 — deriveSlotNumber needs a TODO

The ams_update.md design doc explicitly calls out: "The slot number should come from an explicit chain rule … this should be agreed with the ev-node/ev-reth chain semantics before enabling the fork." There is no in-code reminder:

func (c *EngineClient) deriveSlotNumber(blockHeight uint64) uint64 {
    return blockHeight   // ← no TODO, no comment
}

A future reader won't know this is provisional. Add a comment:

// TODO: slot derivation rule must be agreed with ev-reth chain semantics
// before setting amsterdamTime in any chainspec. Currently uses blockHeight
// as a placeholder.
func (c *EngineClient) deriveSlotNumber(blockHeight uint64) uint64 {
    return blockHeight
}

4. execution.go line 142 — Unreachable return

retryWithBackoffOnPayloadStatus has a final return at line 142 that is unreachable. The loop already returns at line 131 when attempt >= maxRetries:

if attempt >= maxRetries {
    return fmt.Errorf("max retries (%d) exceeded for %s: %w", maxRetries, operation, err)  // line 131
}
// ... select/backoff ...
}
// This return is never reached:
return fmt.Errorf("max retries (%d) exceeded for %s", maxRetries, operation)  // line 142

Remove the dead return at line 142.

5. engine_rpc_client.go line 82 — Cross-version coupling should be documented

When FCU V4 succeeds, the code immediately advances payloadVersion to V6:

if version == engineForkchoiceVersionV4 {
    e.payloadVersion.Store(uint32(enginePayloadVersionV6))
}

This works for Amsterdam (FCU V4 ↔ getPayload V6) but encodes an implicit fork-version coupling. If future forks decouple these version numbers, this will silently use the wrong getPayload version before the first failure. A comment stating the invariant being relied on would help future maintainers.

6. test/test_helpers.go lines 205-227 — Duplicated decodeSecret / getAuthToken

These two functions are verbatim copies of the unexported functions in execution/evm/execution.go. Since the test package (package test) can't import unexported symbols from package evm, the duplication is currently unavoidable. But consider exposing them via an internal package or a _test.go file in package evm to avoid two implementations drifting out of sync.

---

Minor

7. ams_update.md at repo root

Design/planning documents in the repo root clutter the project structure and age poorly. Consider moving this to docs/ or deleting it after the PR is merged (the checklist at the bottom is now implemented).

8. rawExecutionPayloadHasField — two JSON parses for one check

hasAmsterdamFields calls rawExecutionPayloadHasField twice, each time fully unmarshaling the JSON:

return rawExecutionPayloadHasField(e.RawExecutionPayload, "slotNumber") ||
    rawExecutionPayloadHasField(e.RawExecutionPayload, "blockAccessList")

This is fine for correctness but wastes one parse when the first check fails. Consolidate into a single unmarshal that checks both keys:

func rawPayloadHasAmsterdamFields(payload json.RawMessage) bool {
    if len(payload) == 0 { return false }
    var fields map[string]json.RawMessage
    if err := json.Unmarshal(payload, &fields); err != nil { return false }
    _, hasSlot := fields["slotNumber"]
    _, hasBAL  := fields["blockAccessList"]
    return hasSlot || hasBAL
}

9. execution.go — time.After in retry loop

time.After allocates a timer that won't be reclaimed by the GC until it fires, even if the context is cancelled first. Prefer:

timer := time.NewTimer(backoff)
select {
case <-ctx.Done():
    timer.Stop()
    return ...
case <-timer.C:
    backoff *= 2
}

This is pre-existing, but it's worth fixing since the retry logic is newly refactored.

---

Positive Notes

• EnginePayloadEnvelope design is correct — keeping RawExecutionPayload as json.RawMessage and only decoding what ev-node needs is exactly the right approach to avoid dropping future EL fields (blockAccessList, etc.).
• Fallback/cache logic in engine_rpc_client.go is clean and atomic-safe. The version starts at V4 (Prague default) and only advances on actual server negotiation.
• forkchoiceArgsForMethod correctly strips slotNumber for V3 and enforces its presence for V4, so the attributes map in execution.go can always include it without needing fork-awareness at the call site.
• Test coverage for the version-selection FSM is excellent — Prague, Osaka, Amsterdam, fork upgrade, non-fork errors, and BAL passthrough are all covered.
• Tracing correctly records the resolved method name after the call (post-fallback), not the attempted version.

---

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	⏩ skipped	Jun 22, 2026, 9:22 AM

[github-actions]

PR Preview Action v1.8.1
🚀 View preview at https://evstack.github.io/docs-preview/pr-3352/
Built to branch main at 2026-06-22 09:23 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.71%. Comparing base (29e8542) to head (e7df142).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3352   +/-   ##
=======================================
  Coverage   60.70%   60.71%           
=======================================
  Files         127      127           
  Lines       13781    13781           
=======================================
+ Hits         8366     8367    +1     
+ Misses       4502     4501    -1     
  Partials      913      913           

Flag	Coverage Δ
combined	60.71% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.