cl: reuse one converter between consensus and execution withdrawals

[lystopad]

Split out of #23105, which grew too large to review in one piece. This is the mechanical, behaviour-neutral part.

The conversion between cltypes.Withdrawal and types.Withdrawal was hand-written at each call site. cl/cltypes/withdrawal.go already had a private singular converter, so this exports a plural one and uses it in the two places that built the list by hand: the payload-attributes emitter in the forkchoice stage, and cacheExecutionBody.

No behaviour change. Both sites produce the same slice as before, including whether it comes back nil — cacheExecutionBody keeps returning nil for an empty withdrawals list rather than an empty slice.

Part of a series splitting #23105 into reviewable units. The remaining parts follow separately; this one stands alone and depends on nothing else in the series.

[Copilot]

Pull request overview

This PR refactors Caplin’s consensus↔execution withdrawal conversion to reuse a single shared converter, avoiding duplicated hand-written mapping at call sites while keeping behavior unchanged (including nil vs empty-slice semantics where noted).

Changes:

• Export cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals as the shared conversion helper.
• Update forkchoice’s payload-attributes emission to use the shared converter.
• Update cacheExecutionBody to use the shared converter while preserving nil withdrawals for empty lists.

Reviewed changes

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

File	Description
cl/phase1/stages/forkchoice.go	Switch payload-attributes withdrawals construction to the shared converter.
cl/cltypes/withdrawal.go	Export plural consensus→execution withdrawals converter.
cl/beacon/handler/block_production.go	Reuse shared converter when caching execution payload withdrawals (preserving nil behavior for empty lists).

---

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

cl/beacon/handler/block_production.go:2600

• This introduces an extra allocation + extra pass by first materializing consensusWithdrawals and then allocating ret inside ConvertConsensusWithdrawalsToExecutionWithdrawals. Since payload.Withdrawals is already an iterable list, consider adding an exported converter that works directly from the SSZ list (or exporting the singular converter) so ws can be filled in one pass without the intermediate slice, while still keeping a single conversion implementation.

	if payload.Withdrawals != nil && payload.Withdrawals.Len() > 0 {
		consensusWithdrawals := make([]*cltypes.Withdrawal, payload.Withdrawals.Len())
		payload.Withdrawals.Range(func(idx int, w *cltypes.Withdrawal, _ int) bool {
			consensusWithdrawals[idx] = w
			return true
		})
		ws = cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals)

[Copilot]

Pull request overview

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

Suppressed comments (1)

cl/cltypes/withdrawal.go:91

• This claims the helper is the single crossing point, but eth1_block.go still builds execution-withdrawal lists through the singular converter. Since that invariant is already false, describe the function's behavior instead.

// ConvertConsensusWithdrawalsToExecutionWithdrawals is the single crossing point between the two
// representations, so callers building the same list cannot drift apart.

[yperbasis]

No blocking correctness findings. The conversion preserves field values, order, pointer independence, and both call sites’ nil/empty behavior.

One low-severity issue: the new doc comment claims this helper is the “single crossing point,” but three manual conversions remain, plus internal singular conversions in eth1_block.go. Since this PR stands alone, the comment should document the actual behavior instead—especially that nil input returns a non-nil empty slice.

Reviewed head 362e2e3f. CI has 129 successful checks, with 6 skipped; the PR is mergeable.

[lystopad]

You are right, and the count is worse than the comment suggested: three hand-written conversions remain in block_production.go (the Gloas paths, which move in a later PR of this series) plus the two singular conversions in eth1_block.go. Calling this the single crossing point described where the series ends up, not where this PR leaves things.

Reworded to describe the behaviour instead:

// ConvertConsensusWithdrawalsToExecutionWithdrawals converts a withdrawal list to its execution
// representation, in order and with no shared pointers. The result is never nil, which matters
// because the execution layer rejects a nil list and an empty one under opposite conditions.

Added cl/cltypes/withdrawal_test.go to pin each of those claims rather than only assert them — order and values, no pointer sharing with the source, and never-nil for both nil and empty input. The nil case is the one you singled out, and it is load-bearing: checkWithdrawalsPresence rejects a nil list after Shanghai and a non-nil one before it, so the difference is not cosmetic.

Head is now 9577312c37, rebased onto 6043164024.