cl/beacon: resolve payload withdrawals in one place

[lystopad]

Split out of #23105, which grew too large to review in one piece.

What this does

produceBeaconBody chose between three withdrawal sources inline — a Gloas head whose payload was revealed, a Gloas head whose payload was not, and everything before Gloas — each with its own hand-written conversion loop, and then assembled the payload attributes around whichever it picked.

The choice is now a method that names what it selects between, and the attributes come from a single version-aware constructor. What is left inline is the genuinely fork-specific part: the two fields only Gloas sends. The three conversion loops go through the shared converter from #23271, which are the call sites @yperbasis pointed out as still hand-written when reviewing that PR.

Behaviour change

The refactor itself is behaviour-neutral, but the constructor also fixes two cases where the old inline construction built a request the chosen wire version cannot express:

• the parent beacon block root is omitted below Deneb — Capella and Bellatrix dispatch to forkchoiceUpdatedV2/V1, and validatePayloadAttributesPreFCU rejects a non-nil parent root there with InvalidPayloadAttributesErr;
• withdrawals are omitted below Capella — Bellatrix dispatches to forkchoiceUpdatedV1, which has no withdrawals field, and the expectation is a non-nil empty slice, so one was being sent.

Both only bite on the engine transport, and both made the request fail rather than be ignored.

Withdrawal source equivalence

The routing is otherwise unchanged, including the details that are easy to lose in a refactor:

• a FULL Gloas head reads from the state copy with the parent payload applied, not from the head state;
• an EMPTY Gloas head reads the expectation the state already cached, and does not compute a fresh one;
• before Gloas the expectation is computed from the head state;
• the resulting slice keeps its nil-ness in every case.

TestExpectedWithdrawalsReadsTheRightSourcePerFork gives the two Gloas states different withdrawal outcomes, so it fails if the source selection is removed rather than passing either way. TestPayloadAttributesOmitFieldsTheChosenVersionCannotCarry pins the two gates across every fork.

Part of a series splitting #23105.

[lystopad]

Rebased onto main now that #23271 has merged, and corrected one thing from @yperbasis's review of #23105 (point 6) that applies directly here.

I had payloadAttributes send the parent beacon block root unconditionally, on the reasoning that the execution layer decides what to do with these fields from the payload timestamp. That reasoning does not survive a versioned wire format: validatePayloadAttributesPreFCU rejects a non-nil parent beacon block root below Deneb with InvalidPayloadAttributesErr, so Capella maps to forkchoiceUpdatedV2 and the request fails outright rather than being ignored. It is now gated at Deneb, with a version table pinning every fork.

Withdrawals stay unconditional, which matches what production sent before this series.

Also updated for the merge: the shared converter is now on main, so the three hand-written conversion loops here go through it — the ones you counted when reviewing #23271.

Head is 0c0d8af7a6.

[Copilot]

Pull request overview

Centralizes withdrawal resolution and payload-attribute construction in beacon block production.

Changes:

• Extracts fork-specific withdrawal source selection.
• Reuses the shared withdrawal converter.
• Adds tests for source routing and versioned attributes.

Reviewed changes

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

File	Description
cl/beacon/handler/block_production.go	Adds withdrawal and payload-attribute helpers.
cl/beacon/handler/block_production_test.go	Tests withdrawal routing and parent-root handling.

Suppressed comments (2)

cl/beacon/handler/block_production.go:252

• This is not behavior-neutral as the PR description claims: the previous production path always populated ParentBeaconBlockRoot, whereas Bellatrix and Capella now receive nil, and the new test explicitly pins that change. Please either restore the old behavior or document this Engine API compatibility fix and its pre-Deneb impact in the PR description.

	if version.AfterOrEqual(clparams.DenebVersion) {
		attrs.ParentBeaconBlockRoot = parentRoot
	}

cl/beacon/handler/block_production_test.go:767

• require.Empty accepts both nil and non-nil empty slices, so this does not pin the nil-ness that the PR identifies as execution-layer-significant. Assert nil explicitly; that also verifies this branch did not compute a fresh non-nil expectation.

	withdrawals, err = a.expectedWithdrawals(gloasState, nil, clparams.GloasVersion, 0)
	require.NoError(t, err)
	require.Empty(t, withdrawals)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lystopad]

Both fixed. The stale block was my own leftover: the replacement was inserted above the old one instead of over it, so the file carried two doc comments describing opposite behaviour. Removed.

[lystopad]

Correct, and this is the second weak test of mine in this series, so thank you for checking. Two fresh states both produce an empty expectation, so reading the wrong one passed just as well.

The state copy carrying the parent payload now holds a pending builder withdrawal that the head state does not, and the assertion is on that withdrawal rather than on non-nilness. Reverting the source selection makes it fail; it is no longer satisfiable by reading either state.

[yperbasis]

Requesting changes for two issues:

1. payloadAttributes assigns Withdrawals for Bellatrix. expectedWithdrawals produces a non-nil empty slice, while Bellatrix dispatches engine_forkchoiceUpdatedV1; V1 rejects any non-nil withdrawals. Gate this field at Capella or later and extend the table test to require nil for Bellatrix. The old inline construction also had this problem, but the new version-aware constructor and Bellatrix test should not preserve the invalid request shape.

2. TestExpectedWithdrawalsReadsTheRightSourcePerFork does not prove that the FULL path reads withParentPayload. Both states are fresh and produce the same empty expectation; the test still passes if the source selection is removed. Give the states different withdrawal outcomes and assert the exact result from withParentPayload.

Minor follow-ups: remove the contradictory duplicate payloadAttributes documentation, and update the PR description because the parent-root change means the PR is no longer behavior-neutral.

[yperbasis]

Requesting changes for one remaining issue:

payloadAttributes still assigns Withdrawals for Bellatrix. expectedWithdrawals returns a non-nil empty slice there, while ForkChoiceUpdate dispatches Bellatrix to engine_forkchoiceUpdatedV1. V1 does not carry withdrawals, and strict execution clients reject the request instead of returning a payload ID. Gate this field at Capella or later and extend the table test to require nil for Bellatrix.

The FULL Gloas state-source test and the duplicate documentation are fixed.

Minor follow-up: the PR description still says "No behaviour change", although the parent beacon block root is now intentionally omitted below Deneb.

[lystopad]

Fixed in 3e6e2f9b8e — you are right, and it is the same class as the parent root: Bellatrix dispatches to forkchoiceUpdatedV1, which has no withdrawals field, and expectedWithdrawals returns a non-nil empty slice there, so one was being sent. Gated at Capella, and the table test now asserts both fields per fork rather than only the parent root.

The PR description is updated too. You are right that it was wrong to still call this behaviour-neutral: the refactor is, but the constructor now also omits two fields the chosen wire version cannot express, and both of those made the request fail rather than be ignored. That is spelled out as its own section now.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[lystopad]

You are right that nil does not remove the key: PayloadAttributes has no omitempty, so V1 and V2 still receive withdrawals and parentBeaconBlockRoot as null. @yperbasis reached the same point and settled the practical half of it — the engine API schemas do not forbid extra properties, and execution clients validate decoded values rather than key presence, so a null reads as absent. Geth's V1 rejects a non-nil withdrawals list, which is what this PR stops sending, and accepts nil.

So this is a wording problem rather than a defect, and the wording is fixed: the field is left unpopulated, not omitted.

Making the keys genuinely absent is a separate change in engine_types, and not a simple one — bare omitempty on Withdrawals would be wrong, because V2 and later require the empty list to be present on the wire. omitzero is the right tool, in its own PR.

[yperbasis]

Approving.

Non-blocking follow-ups surfaced by the re-review:

1. Pre-existing: at the Gloas fork boundary the isPreGloasParent case leaves the FULL-state copy unset, so expectedWithdrawals returns the empty payload_expected_withdrawals installed by UpgradeToGloas. processWithdrawalsGloas does not early-return there (latest_block_hash == bid.block_hash after the upgrade) and computes a fresh expectation, so ProcessExecutionPayloadEnvelope rejects the proposer's own envelope ("withdrawals root mismatch with expected") and the boundary slot loses its payload. The new helper makes the fix small — route the boundary through the fresh-compute source. Please take it as the next PR in the series, with a boundary test.

2. Pre-existing on a touched line: random comes from GetRandaoMixes(a.ethClock.GetCurrentEpoch()) — the wall-clock epoch, not the target slot's. A request served after the wall clock crosses into the next epoch reads a not-yet-reset mix, and the block then fails its own randao check in ProcessBlock. Use state.Epoch(baseState), like emitNextPaylodAttributesEvent does.

3. Consider passing slotNumber/targetGasLimit into payloadAttributes as well, so one function owns the versioned schema, and assert TargetGasLimit non-nil on the Gloas row plus value pass-through in the table test — today deleting the attrs.TargetGasLimit assignment passes the whole suite while every Gloas proposal would get -38003.

4. Wording nit, which also addresses the open Copilot comment: nil fields serialize as null keys, not absent ones, so "not populated" is more exact than "left out". This is not a functional defect — the engine API schemas do not forbid extra properties, and ELs validate decoded values only (Geth's V1 rejects non-nil withdrawals, so the old non-nil empty slice failed there and nil passes). If we ever want absent keys, bare omitempty on Withdrawals would be wrong since V2+ requires the empty list on the wire; omitzero, in a separate engine_types PR.

[lystopad]

Thanks for the approval. I took the two points that are about this PR's own code and left the rest for follow-ups, in 28073af646.

Point 3 — one owner for the versioned schema. Taken, and it was a real gap rather than a tidiness one: the slot number and target gas limit were assigned by the caller after the constructor, so the constructor did not actually own the schema and the table test could not observe them. Deleting either assignment left the whole suite green while every Gloas proposal would have been rejected with -38003. Both are now parameters, and the test asserts the values arrive rather than only that the fields are non-nil — dropping TargetGasLimit now fails the Gloas row.

Point 4 — wording. Fixed, and it also answers the open Copilot thread: nil does not remove the key, so "left unpopulated" rather than "omitted". Agreed this is not a functional defect, for the reason you give — clients validate decoded values, and Geth's V1 rejects a non-nil withdrawals list, which is exactly what this stops sending. I have noted on that thread that making keys genuinely absent needs omitzero in engine_types, not bare omitempty, since V2+ requires the empty list on the wire.

Points 1 and 2 — separate, and I would rather not fold them in. Both are pre-existing and neither is in this PR's diff:

• the Gloas fork boundary leaving the FULL-state copy unset, so the proposer's own envelope is rejected and the boundary slot loses its payload. Agreed the new helper makes the fix small; I will take it as the next PR in the series with a boundary test, as you suggest.
• random coming from the wall-clock epoch rather than the target slot's, which fails the block's own randao check when a request is served across an epoch boundary. That one looks like it deserves its own PR and a backport assessment, since it can affect any proposal served late in an epoch, not only Gloas.

Say if you would rather see either of them here instead.