execution/engineapi: validate getPayload fork boundaries and blob bundle counts

[yperbasis]

Summary

• reject engine_getPayloadV1 for Shanghai-and-later payloads while keeping engine_getPayloadV2 valid on both sides of the Shanghai boundary
• validate blob bundle counts on every bundle-bearing endpoint:
  • BlobsBundleV1 from engine_getPayloadV3/V4 (Cancun/Prague) has equal commitment, blob, and proof counts
  • BlobsBundleV2 from engine_getPayloadV5/V6 (Osaka/Amsterdam) has equal commitment/blob counts and CELLS_PER_EXT_BLOB cell proofs per blob
• rely on bundle construction's non-nil initialized result instead of an unreachable nil fallback
• correct the GetPayloadV4 and GetPayloadV6 comments to match the response structures defined by the specifications

Why

Erigon payload IDs are monotonically allocated identifiers; they do not encode the Engine API version used to start the build. getPayload must therefore validate the built payload's timestamp against the requested endpoint. V2 intentionally spans Paris and Shanghai, whereas V1 and V3–V6 must stay within their respective fork windows.

Every response that carries a blob bundle must satisfy the count invariant of its bundle version. BlobsBundleV1 requires equal commitment, blob, and proof counts. BlobsBundleV2 requires equal commitment/blob counts and CELLS_PER_EXT_BLOB proofs per blob. BlobsBundleFromTransactions always returns a non-nil bundle with initialized slices, including for an empty bundle.

Testing

• go test ./execution/engineapi/... -count=1
• make lint (repeated)
• make erigon integration

[Copilot]

Pull request overview

This PR tightens EngineServer.getPayload fork-version invariants so the selected Engine API payload schema cannot cross fork boundaries (now including Paris↔Shanghai), and extends blobs-bundle structural validation to cover Amsterdam (and later) payload versions.

Changes:

• Enforce Shanghai-era schema matching in getPayload (reject Paris/Shanghai mismatches via timestamp↔version checks).
• Apply blobs bundle length validation for Amsterdam+ versions by broadening the guard from == Fulu to >= Fulu.
• Add regression tests covering Paris/Shanghai mismatches and invalid Amsterdam blobs bundle structure.

Reviewed changes

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

File	Description
execution/engineapi/engine_server.go	Adds Shanghai fork boundary enforcement in getPayload and ensures blobs-bundle validation runs for newer payload versions.
execution/engineapi/engine_server_getpayload_test.go	Adds regression tests for fork/schema mismatches and Amsterdam blobs-bundle structural validation; refactors server construction helper to accept custom configs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

[AskAlexSharov]

Reviewed at e2978c8. Both changes look correct to me, and I verified the two claims that carry the most risk.

The dead nil-fill removal is safe. GetPayloadResponse has exactly one construction site (assembledBlockToPayloadResponse), and it always sets BlobsBundle from BlobsBundleFromTransactions, which returns either an error or a non-nil bundle with all three slices initialized to empty. So payload.BlobsBundle cannot be nil at the check, and dropping the fill does not add a nil deref.

The widened version >= FuluVersion cannot fire on a pre-Osaka payload. The fork gate above already rejects !IsOsaka(ts) && version >= FuluVersion, so reaching the bundle check with version >= Fulu implies IsOsaka(ts). The widening only adds Amsterdam, as intended.

The new tests really guard the fix. I reverted both production hunks in a scratch worktree and re-ran: TestGetPayloadV1RejectsShanghaiPayload and TestGetPayloadV6RejectsInvalidBlobsBundle both fail, and pass again with the hunks restored. TestGetPayloadV2AcceptsParisPayload stays green either way, which is right — it guards against over-rejection, not under-rejection.

Three things worth addressing:

1. The Shanghai clause is deliberately asymmetric and nothing says so. Every other fork appears as a pair — !IsX && version >= X plus IsX && version < X. Shanghai gets only the second half, because engine_getPayloadV2 must keep serving Paris payloads. A future contributor completing the pattern would add !IsShanghai(ts) && version >= CapellaVersion and silently break V2 on the Paris side. TestGetPayloadV2AcceptsParisPayload would catch it, but a one-line comment on that clause would save the debugging, and it fits the "non-obvious invariant the types don't enforce" case in the comment policy.

2. BlobsBundleV1 is still unvalidated. The PR title says "validate ... blob bundles", but the length check only runs for Osaka and later. Deneb/Electra (getPayloadV3/V4) get no check at all — I grepped, there is no equivalent validation elsewhere for the V1 bundle, where the invariant is len(commitments) == len(blobs) == len(proofs). Either extend the check to version >= DenebVersion with the per-version proof ratio, or say in the description that V1 bundles stay unvalidated, so the gap is explicit rather than looking covered.

3. Missing the complement test. The risk introduced by a new rejection rule is over-rejection. That is covered for V2/Paris but not for V1/Paris. TestGetPayloadV1AcceptsParisPayload on parisShanghaiChainConfig() with minimalPayloadBlock(99, nil) would pin that V1 still works below the Shanghai boundary, and it is a few lines with the helpers already added here.

Nits, take or leave:

• GetPayloadV6's doc now says "returning ExecutionPayloadV4". Worth a second check against the Amsterdam spec — the old "ExecutionPayloadV6" was certainly wrong since no such type exists, so this is an improvement either way.
• minimalPayloadBlock builds Shanghai-and-later payloads with nil withdrawals. Fine for these tests since getPayload does not inspect them, but it means the V1-rejection test would still pass even if the payload were malformed for other reasons.
• Still marked draft — is it ready for review?

[yperbasis]

Thanks for the thorough review — all three points are addressed:

Point 1 — landed in 49d90d1 (crossed with your review in flight): the gate now carries a comment stating that Shanghai, unlike later forks, does not require an exact version match because engine_getPayloadV2 serves both Paris and Shanghai payloads.

Point 2 — a7dedba extends the count check to every version that returns a bundle: len(commitments) == len(blobs) == len(proofs) for BlobsBundleV1 (Deneb, Electra; cancun.md item 4), and CELLS_PER_EXT_BLOB cell proofs per blob for BlobsBundleV2 (Fulu, Gloas; osaka.md item 3). TestGetPayloadV3RejectsInvalidBlobsBundle pins the new lower bound (it was red before the production change), and TestGetPayloadV4AcceptsValidBlobsBundle guards against applying the cell-proof ratio below Fulu. The cryptographic sub-assertions (verify_blob_kzg_proof_batch, verify_cell_kzg_proof_batch) remain guaranteed by construction — blob transactions are KZG-verified at txpool admission — the same stance the Fulu-only check already took.

Point 3 — cf0ef24 adds TestGetPayloadV1AcceptsParisPayload. You were right about the gap: with the IsShanghai guard removed from the new clause, only this test fails across the whole getPayload matrix.

On the nits: amsterdam.md says "This method is updated to return the new ExecutionPayloadV4 structure" (ExecutionPayloadV3 plus blockAccessList), so the GetPayloadV6 doc comment is correct as written. Agreed on minimalPayloadBlock with nil withdrawals — these tests pin the fork gate, which reads only header.Time, so fork-complete fixtures would add coupling without extra signal. And the PR is no longer a draft — it is ready for review.

[Copilot]

Pull request overview

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