Skip to content

cl/beacon: prime the execution layer before a slot this node proposes - #23105

Draft
lystopad wants to merge 20 commits into
mainfrom
feature/lystopad/caplin-prepare-payload
Draft

cl/beacon: prime the execution layer before a slot this node proposes#23105
lystopad wants to merge 20 commits into
mainfrom
feature/lystopad/caplin-prepare-payload

Conversation

@lystopad

@lystopad lystopad commented Aug 7, 2026

Copy link
Copy Markdown
Member

Builds on #23098 (merged).

Problem

Caplin sends payload attributes only when the validator client asks for a block, at the start of the proposal slot. The execution layer therefore starts packing from scratch inside the slot, and Caplin compensates by waiting until a publication margin before the attestation deadline to collect the payload — attestationDeadline - attestationDeadline/4, so ~3s into a 12s slot.

The block is then finished ~3s in, leaving ~1s to cover consensus processing, delivery to the validator client, signing, publishing and gossip. On a mainnet validator with a remote validator client that was not enough: a blob-carrying block was published 27ms before the attestation deadline and was re-orged out.

Change

Send the forkchoice update ahead of slots this node is due to propose. The execution layer recognises the repeat request at production time and keeps the builder it already warmed, so the payload is packed by the time the validator client asks and can be collected a quarter of the way into the slot instead of three quarters. The builder gets the same packing time; it just starts and finishes earlier, which returns most of the margin to publication.

Preparation tracks the head it primed on and primes again when that changes. Priming once is not enough: when the previous slot's block arrives late the head moves afterwards, leaving the execution layer warming a builder on a parent that is no longer the head — and that is exactly when the proposal is most at risk.

No new configuration. Preparation runs only when the next slot's proposer is a validator registered via prepare_beacon_proposer, so nodes that do not propose never prepare, never send an extra forkchoice update and never start a builder. It is limited to an in-process Erigon execution layer because the optimisation relies on this PR's extended local builder lifetime; standalone Caplin using a remote Engine API keeps the existing production schedule and sends no preparation forkchoice updates.

Safety

Production compares the payload id its own forkchoice update returns against the primed one. They match only when the execution layer kept the warm builder; any divergence — a reorg, a late block, a changed fee recipient, a busy execution layer — yields a different id and production falls back to its current, later schedule. The worst case is today's behaviour, not a thin block.

Scoped to forks before Gloas, mirroring the existing early return in emitNextPaylodAttributesEvent, since ePBS builders gossip bids instead.

Testing

Unit tests cover the early collection window, the unchanged window without a primed builder, re-priming when the head moves, fork-specific payload attributes, nil/remote/local execution-engine startup, builder replacement and eviction, and the primed-payload record rejecting a wrong id, a wrong slot, an absent id, insufficient warm-up and a mutated caller buffer.

Run on a mainnet validator. Five proposals, all prepared=true, two of which re-primed after the previous slot's block arrived late and moved the head.

Blocks were published between t+1.5s and t+2.3s into the slot, so the worst case left 1.7s of margin against the 4s attestation deadline — against 27ms on the re-orged block described above. All were finalised. They ranged up to 91% gas usage and 13 blobs, so collecting earlier is not producing thin blocks. Block import latency, goroutine count and attestation behaviour were unchanged over 35h of running.

The two slowest were the two fullest blocks: GetAssembledBlock blocks until the builder finishes finalising, which costs roughly a second on a near-full block versus about a tenth of that on a light one. That is outside the scope of this change but worth knowing.

@lystopad lystopad self-assigned this Aug 7, 2026
@lystopad
lystopad requested a review from taratorio August 7, 2026 15:34
@lystopad lystopad added the Caplin Caplin: Consensus Layer, Beacon API label Aug 7, 2026
@lystopad
lystopad requested a review from Copilot August 7, 2026 15:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a “lookahead” path in Caplin to prime the execution layer one slot before this node is due to propose, so the payload builder can start packing earlier and the block can be collected earlier in-slot (pre-Gloas), improving publication margin.

Changes:

  • Start a background payload-preparation loop in Caplin and record the primed payload ID per slot.
  • Use the recorded primed payload ID during block production to shift the payload polling/collection window earlier when the builder was successfully warmed.
  • Add unit tests for the prepared/unprepared polling window and for prepared-payload ID matching behavior.

Reviewed changes

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

Show a summary per file
File Description
cmd/caplin/caplin1/run.go Starts the payload preparation loop when the beacon API handler is created.
cl/beacon/handler/payload_preparation.go New background priming loop + primed payload ID tracking and FCU argument assembly.
cl/beacon/handler/handler.go Adds preparedPayload state to ApiHandler.
cl/beacon/handler/block_production.go Uses primed payload ID to decide whether to collect payload early; updates window calculation.
cl/beacon/handler/block_production_test.go Updates existing window tests and adds tests for prepared behavior and ID copying/matching.

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

Comment thread cl/beacon/handler/payload_preparation.go
Comment thread cl/beacon/handler/payload_preparation.go Outdated
Comment thread cl/beacon/handler/payload_preparation.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

cl/beacon/handler/payload_preparation.go:121

  • preparePayloadLoop waits for the first ticker tick before attempting any preparation. Because time.Ticker does not tick immediately, a restart close to a proposal slot can miss priming the very next slot (the main optimisation target). Also, once the chain reaches Gloas, the loop keeps waking every tick even though priming is permanently disabled for all future slots; it can return instead.
	for {
		select {
		case <-ctx.Done():
			return
		case <-ticker.C:

cl/beacon/handler/payload_preparation.go:203

  • preparedForkChoiceInputs duplicates the pre-Gloas ForkChoiceUpdate argument derivation that already exists in produceBeaconBody (head/safe/finalized/withdrawals/attrs). This duplication is easy to let drift over time, and any divergence silently disables the warm-builder optimisation.
// preparedForkChoiceInputs assembles the forkchoice-update arguments for building targetSlot on top
// of baseState, mirroring the pre-Gloas path in produceBeaconBody.
//
// The two must derive byte-identical arguments: the execution layer keeps the builder it already
// warmed only when it recognises the request as a repeat. Divergence costs that warm builder and

@lystopad
lystopad force-pushed the feature/lystopad/caplin-prepare-payload branch from e5f1908 to 856fdc5 Compare August 7, 2026 18:57
@lystopad
lystopad marked this pull request as draft August 7, 2026 20:56
@lystopad

lystopad commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Converting to draft while a follow-up fix is validated on a live mainnet validator. Please hold off reviewing until it is pushed.

Running this branch on a mainnet proposer surfaced a gap in the preparation loop: it primes a slot once and never revisits it. When the previous slot's block arrives late the head moves after the prime, leaving the execution layer warming a builder on a parent that is no longer the head. Production then gets a different payload id, reports prepared=false, and falls back to building inside the slot.

Observed on slot 14942071: the previous slot's block landed 3.404s into its slot, moving the head more than eight seconds before the proposal — ample time to prime again, which the loop had no way to notice. The proposal fell back to the pre-change schedule and was re-orged.

Behaviour is safe either way, since a stale prime only costs the optimisation. But on a chain where blocks routinely arrive over a second into their slot, priming once misses a large share of proposals — including the late-block slots where the margin matters most.

The fix tracks the head a slot was primed on and primes again whenever it changes. It is running on a mainnet validator now; I will push it here once it has proposed enough blocks to show the intended timing, rather than land it on reasoning alone.

@lystopad
lystopad force-pushed the feature/lystopad/caplin-prepare-payload branch from 856fdc5 to 77a40a7 Compare August 9, 2026 07:02
@lystopad
lystopad marked this pull request as ready for review August 9, 2026 07:02
@lystopad
lystopad requested a lite review from Copilot August 9, 2026 07:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

cl/beacon/handler/payload_preparation.go:127

  • Unexpected errors from preparePayloadFor are currently logged at Debug with message "skipped", which makes real priming failures (e.g. Engine API errors) easy to miss at default log levels and harder to diagnose in production. Consider treating non-expected errors as warnings (and wording as "failed") while still keeping expected skips silent.
		if err != nil {
			// Most ticks land on a slot somebody else proposes; logging those would drown out the
			// failures worth seeing.
			if !isExpectedPreparationSkip(err) {
				log.Debug("PayloadPreparation: skipped", "slot", targetSlot, "err", err)

cl/beacon/handler/block_production_test.go:744

  • The comment says the prepared payload is taken "a quarter of the way into the slot", but the code bases the timing on a fraction of the attestation deadline (attestationDue/payloadPublicationDivisor). With the test config (12s slot, 4s attestation due), this is 1s into the slot (not 1/4 of the slot). Updating the wording avoids misleading future readers.
	// A builder primed before the slot has already packed the payload, so it is taken a quarter of
	// the way into the slot rather than at the publication margin, leaving the rest for gossip.

@yperbasis
yperbasis requested a balanced review from Copilot August 10, 2026 08:21
@yperbasis yperbasis added this to the 3.7.0 milestone Aug 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

cl/beacon/handler/block_production.go:1072

  • A matching payload ID does not prove the builder received the intended warm-up time. If the first successful prime happens shortly before the slot (for example after late proposer registration or earlier busy responses), this still sets prepared=true and moves collection from t+3s to t+1s, giving the builder less packing time than the existing path and potentially producing a thin block. Record when the payload was primed and use the early window only after it has warmed for at least the difference between the normal and prepared deadlines; otherwise keep the normal window.
		prepared := a.preparedPayload.matches(targetSlot, idBytes)

@domiwei domiwei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review found two medium timing/lifecycle gaps that can make the optimization's worst case worse than today's behavior.

Comment thread cl/beacon/handler/block_production.go Outdated
Comment thread cl/beacon/handler/payload_preparation.go Outdated
@domiwei
domiwei requested a review from mh0lt as a code owner August 10, 2026 11:33
@domiwei
domiwei requested a balanced review from Copilot August 10, 2026 12:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

execution/execmodule/block_building.go:95

  • An exact builder for this timestamp may still be live even when it is not lastParameters. For example, after priming consecutive slots N and N+1, production for N reaches this branch, cancels N's warmed builder, and creates a new payload ID, so the early window is lost. Keep parameters per timestamp/builder and return the existing ID when they match; cancel only when the parameters for that timestamp differ.
	if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok {
		if previous := e.builders[previousID]; previous != nil {
			previous.Cancel()
		}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

execution/execmodule/chainreader/chain_reader.go:284

  • This sentinel represents any weight-one semaphore contention, including an FCU or another payload request; it does not mean execution data is syncing. The current text will therefore misdiagnose routine contention in the new retry and polling logs. Use a generic busy message.
var ErrExecutionBusy = errors.New("execution data is still syncing")

@domiwei

domiwei commented Aug 13, 2026

Copy link
Copy Markdown
Member

Reviewed the current head 8a351e377f. I agree with the motivation, but I don't think this is ready to merge yet. The original focused change was 5 files / +278 -9; the cumulative PR is now 19 files / +1920 -162 and crosses Caplin scheduling, forkchoice ordering, direct and remote execution clients, builder lifecycle, validator registration state, Gloas production, and shared withdrawal conversion.

There are three blocking correctness concerns:

  1. Preparation can retain the only native EL semaphore past its slot deadline. preparePayloadFor gives the preparation FCU a context ending at slotStart, but ExecModule.UpdateForkChoice transfers admitted work to bacgroundCtx. If preparation wins the semaphore shortly before the slot, its caller can time out while the underlying FCU/reorg/flush continues holding capacity into the proposal window. proposalsInFlight only prevents a preparation that has not yet entered; it cannot protect the reverse ordering. The existing test covers production-first, but not preparation-admitted-first followed by a healthy proposal.

  2. The selected-head check is still check-then-act. forkChoiceUpdateForPreparation reads SelectedHead() and then separately enters engine.ForkChoiceUpdate. A reachable ordering is: preparation checks A; canonical FCU(B) completes; preparation then acquires the native semaphore and successfully sends FCU(A). A post-call recheck would avoid recording the stale payload, but would not undo the EL mutation. The current changed-head test covers B being visible before the check, not this reverse interleaving.

  3. The proposal deadline wrapper changes the remote-engine path even though preparation is local-only. forkChoiceUpdateForProposal treats err == nil as success. ExecutionClientEngine.ForkChoiceUpdate converts its recognized deadline RPC error to (nil, nil), so at the boundary the wrapper returns an empty ID and skips its intended final outer-context attempt. A slow remote EL can therefore lose the proposal instead of receiving the documented last attempt.

I would require deterministic tests across the real ownership boundaries for all three cases: native FCU admission/background ownership vs proposal, head A -> canonical B -> stale preparation A, and remote deadline normalization -> final attempt.

Suggested PR split

I would not split mechanically by the current review-fix commits, because several commits mix required fixes with unrelated cleanup. I suggest rebuilding/squashing into these units:

PR 1: pre-Gloas local payload preparation

Keep the coherent feature and the correctness dependencies it cannot work without:

  • preparation scheduler, validator/local-engine/fork gates;
  • slot + head + validator-registration-generation tracking and re-priming;
  • prepared payload ID/age matching and the earlier collection window;
  • one shared pre-Gloas FCU-input builder so preparation and production produce identical attributes;
  • target-epoch RANDAO and checkpoint fallback needed for that equivalence;
  • exact builder reuse by timestamp + immutable full parameters;
  • failed/collected/superseded builder lifecycle required to preserve IDs already handed out;
  • caller-owned Busy/deadline handling, after the three ordering/progress problems above are fixed.

This PR should have an explicit non-goal: no Gloas production refactor and no behavior change for remote Engine API deployments except where independently justified and tested.

PR 2: shared withdrawal conversion cleanup

  • export/reuse ConvertConsensusWithdrawalsToExecutionWithdrawals;
  • migrate the payload-attributes emitter and cacheExecutionBody call sites;
  • keep this mechanical and behavior-neutral.

The converter may still be introduced privately in PR 1 if needed for preparation/production equivalence; unrelated call-site migration belongs here.

PR 3: Gloas production refactor

  • gloasWithdrawals;
  • Gloas branch restructuring in produceBeaconBody;
  • shared payload-attribute construction as it applies to Gloas;
  • before/exact/after-Gloas equivalence tests.

The PR body says preparation is scoped to forks before Gloas, so carrying this production-only Gloas rewrite in the feature PR expands both the blast radius and the review matrix without enabling the optimization.

PR 4: observability and small cleanup

  • payload polling log coalescing;
  • zero-fee-recipient warning;
  • startup watcher log;
  • unused sentinel/comment cleanup.

These may be worthwhile, but none is required for the preparation invariant and they make regressions harder to attribute.

One smaller standards note: several new source comments are three or four sentences and narrate call sequences/review history. The repository policy asks for one sentence, rarely two, stating the invariant; the detailed rationale already fits well in the PR description.

So my recommendation is: fix the three proposal-safety issues, trim PR 1 to the coherent pre-Gloas/local feature, and move the Gloas/converter-callsite/logging work into follow-ups. That should make both the concurrency proof and the eventual revert/bisect surface much clearer.

lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so the mechanical cleanup can be reviewed on its own.

The conversion between cltypes.Withdrawal and types.Withdrawal was hand-written at each
call site. Export the existing converter and use it, so the lists cannot drift apart.

No behaviour change: both call sites produce the same slice, including its nil-ness.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so it can be reviewed on its own. The preparation work there leans on
this, but every problem below is reachable today.

A single lastParameters field remembered only the most recent request, so any interleaved
request for a different timestamp destroyed the deduplication for the first: a repeated
request then started a second builder for a payload already being built. Builders are now
kept by the timestamp they are for, alongside an immutable copy of the parameters they were
created with, so a caller cannot mutate the slice it passed and change what a later
comparison sees.

A builder that failed latched its error and was handed back forever, spending the slot
waiting on a payload that could never arrive. It is now treated as absent, and dropped when
its error surfaces. Being stopped is not failure: a stopped builder still holds the payload
it was stopped for, which is exactly what a repeated request is asking for.

Eviction dropped builders from the map without stopping them, so the goroutine kept running
with no way to reach it. It now cancels on the way out, which is the problem described in
issue #23101. Both entry points check for a cancelled caller before acting, so an expired
request reports why it stopped rather than looking like contention that callers retry.
lystopad added a commit that referenced this pull request Aug 14, 2026
…nd the caller's context

Split out of #23105 so it can be reviewed on its own.

AssembleBlock and GetAssembledBlock reported contention as a bare error reading "execution
data is still syncing", which is neither what happened nor distinguishable from a rejection.
Both now return ErrExecutionBusy, and the assemble retry only waits on that: a rejection such
as mismatched withdrawals used to be retried thirty times over six seconds before surfacing,
which spends the slot instead of reporting it.

The retry also ran with no regard for the caller, sleeping through a cancelled context for
the full six seconds. It is now a helper that checks the context before every attempt and
waits on it rather than on a bare sleep, and the chain reader takes the caller's context
instead of substituting context.Background().
lystopad added a commit that referenced this pull request Aug 14, 2026
…e recipient at all

Split out of #23105 so it can be reviewed on its own.

A slot whose payload never arrives fails on every poll of the collection window, at the
retry cadence, so one failure produced hundreds of copies of the same line. Report the first
and then a count, and only for a window that produced nothing: contention that clears is not
worth an alert.

Production falls back to the zero address when no fee recipient is registered for the
proposer, which gives that block's fees away. Say so rather than leave it to be discovered
from the produced block.
@lystopad

Copy link
Copy Markdown
Member Author

@domiwei @yperbasis — agreed, this is too large to review as one piece. Splitting it, roughly along the lines domiwei proposed. Four parts are open now, each off current main and independent of the others:

Still to come, in order:

  • the Gloas production refactor (gloasWithdrawals, single branch per fork, shared attribute construction);
  • payload preparation itself, once the three ordering problems from domiwei's last review are fixed and tested: semaphore ownership across the slot boundary, the check-then-act on the selected head, and the remote-engine deadline normalisation defeating the guaranteed final attempt.

On that last set — domiwei is right on all three, including that my earlier answer about the head check was wrong. The payload-id comparison protects which payload gets proposed, but it does not undo the execution-layer mutation, so losing that race is a correctness problem rather than a lost optimisation.

This PR stays open as the reference until the series lands; it is not intended to merge in this form. Nothing here is a rush — I would rather land small pieces than argue about a 19-file diff.

lystopad added a commit that referenced this pull request Aug 14, 2026
…e recipient at all

Split out of #23105 so it can be reviewed on its own.

A slot whose payload never arrives fails on every poll of the collection window, at the
retry cadence, so one failure produced hundreds of copies of the same line. Report the first
and then a count, and only for a window that produced nothing: contention that clears is not
worth an alert.

Production falls back to the zero address when no fee recipient is registered for the
proposer, which gives that block's fees away. Say so rather than leave it to be discovered
from the produced block.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so it can be reviewed on its own. The preparation work there leans on
this, but every problem below is reachable today.

A single lastParameters field remembered only the most recent request, so any interleaved
request for a different timestamp destroyed the deduplication for the first: a repeated
request then started a second builder for a payload already being built. Builders are now
kept by the timestamp they are for, alongside an immutable copy of the parameters they were
created with, so a caller cannot mutate the slice it passed and change what a later
comparison sees.

A builder that failed latched its error and was handed back forever, spending the slot
waiting on a payload that could never arrive. It is now treated as absent, and dropped when
its error surfaces. Being stopped is not failure: a stopped builder still holds the payload
it was stopped for, which is exactly what a repeated request is asking for.

Eviction dropped builders from the map without stopping them, so the goroutine kept running
with no way to reach it. It now cancels on the way out, which is the problem described in
issue #23101. Both entry points check for a cancelled caller before acting, so an expired
request reports why it stopped rather than looking like contention that callers retry.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so the mechanical cleanup can be reviewed on its own.

The conversion between cltypes.Withdrawal and types.Withdrawal was hand-written at each
call site. Export the existing converter and use it, so the lists cannot drift apart.

No behaviour change: both call sites produce the same slice, including its nil-ness.
lystopad added a commit that referenced this pull request Aug 14, 2026
…nd the caller's context

Split out of #23105 so it can be reviewed on its own.

AssembleBlock and GetAssembledBlock reported contention as a bare error reading "execution
data is still syncing", which is neither what happened nor distinguishable from a rejection.
Both now return ErrExecutionBusy, and the assemble retry only waits on that: a rejection such
as mismatched withdrawals used to be retried thirty times over six seconds before surfacing,
which spends the slot instead of reporting it.

The retry also ran with no regard for the caller, sleeping through a cancelled context for
the full six seconds. It is now a helper that checks the context before every attempt and
waits on it rather than on a bare sleep, and the chain reader takes the caller's context
instead of substituting context.Background().

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined findings from two review passes on the current head, categorized by severity.

High

  1. cl/beacon/handler/block_production.go:285forkChoiceUpdateForProposal's contention tolerance ends at collectAt: the promised past-bound attempt is a single shot that fails in microseconds while the semaphore is held (TryAcquire → Busy), and any FCU slower than the 1s fcuTimeout is converted to the same Busy (execution/execmodule/forkchoice.go:136-140). Under contention (e.g. a late previous-slot block holding a long FCU commit) the pre-PR path published a late block; this path returns HTTP 500 and the slot is missed, even when a fully built primed payload sits in the EL. The 100ms backoff also sleeps once more after the bound has passed, delaying the final attempt. Suggest a bounded busy-retry past collectAt (or a fallback to the primed payload id), and skipping the sleep once the bound is reached.

  2. cl/beacon/handler/payload_preparation.go:137 — the proposalsInFlight standoff cannot stop a prime that is already running, and the counter increments only inside produceBeaconBody (block_production.go:979), after ~1s of state work. On consecutive proposals the prime for slot S+1 busy-retries its FCU until slot start (the loop at payload_preparation.go:318-331 re-checks only the head, and block S does not exist yet, so the head never changes), contending with slot S's production on the weight-1 semaphore for the whole slot. If production then slips past collectAt, the poll window collapses to a single GetAssembledBlock, and one Busy misses the proposal. Suggest incrementing the counter at GetEthV3ValidatorBlock entry, re-checking it inside the prep FCU retry loop (or making the in-flight prime cancellable), and giving the collapsed window one bounded retry.

Medium

  1. cl/beacon/handler/payload_preparation.go:319 — the head re-check before the prep FCU is check-then-act. A canonical FCU for a head selected right after the check returns Busy, which the nil-attributes path swallows as success (cl/phase1/execution_client/execution_client_direct.go:166), while the prep FCU keeps the semaphore until cleanup on the module background context, beyond any caller deadline (execution/execmodule/forkchoice.go:117-131). The CL then reports the new head while the EL is still on the old one, and the adoption lands inside the proposal critical path (see point 1). The swallow predates this PR, but the PR makes long attribute-carrying holds routine right before our proposal slots. Suggest retry/latest-wins ordering for canonical FCUs, or serializing preparation with head publication.

  2. cl/beacon/handler/block_production.go:272case err == nil treats an empty payload id as terminal success. Over the engine transport, SYNCING with a null id comes back as ([]byte{}, nil) (execution_client_engine.go:195-205, plus the errContextExceeded(nil, nil) path at :196), so exactly the transient-busy signal bypasses the retry loop; preparation maps the same signal to errNoPayloadID and retries. Treat len(idBytes) == 0 && err == nil as retryable.

  3. cl/beacon/handler/payload_preparation.go:76primedAt semantics break the intended packing floor in three ways. minAge is derived as unpreparedGrabOffset − preparedGrabOffset (block_production.go:246), i.e. "a prepared payload gets at least normal-path packing time", but matches measures age at the moment the production FCU returns, so a delayed FCU admits a ~2.0s-packed payload against a ~3s floor. The too-late check (payload_preparation.go:287) discards a completed prime without sending the now-free FCU, while the slow-FCU path still records sub-minAge primes. And set overwrites primedAt when a re-prime dedups onto the same still-running builder, understating its warm-up. Anchoring the age test at slot start (and keeping the older primedAt when the id is unchanged) settles all three.

  4. cl/beacon/handler/block_production.go:292payloadAttributes always sets ParentBeaconBlockRoot, but the engine transport is versioned: Capella maps to forkchoiceUpdatedV2 (execution_client_engine.go:187-188), and a non-nil parent root below Deneb is rejected per spec (execution/engineapi/engine_server.go:207-211). Engine-mode Capella priming therefore always fails and warns once a minute. Version-gate the root (nil below Deneb); that also repairs pre-existing engine-mode Capella production. The "different oracles" comment does not hold on a versioned wire format.

  5. cl/beacon/handler/payload_preparation.go:253 — carried over from the previous round: for epoch-crossing target slots the proposer lookup still costs a full state copy under the synced-data read lock plus an epoch transition, usually to conclude "not ours". On Fulu+ GetBeaconProposerIndexForSlot answers next-epoch slots O(1) from the EIP-7917 lookahead, whose validity bound is exactly the errHeadTooFarBack guard already applied. Gate: lookupAfterAdvance = crossesEpoch && version < Fulu.

  6. execution/execmodule/block_building.go:140 — the dedup index keyed on timestamp alone remembers only the latest builder per timestamp, so a head flip A→B→A within a slot mints a cold third builder while the warm A-builder still runs and holds the wanted payload — prepared=false in exactly the churn slots the feature targets. Keying on (timestamp, parent hash) returns the warm builder in both cases and removes most of the supersede machinery.

Low

  1. execution/execmodule/block_building.go:60 — attribute drift disables the feature silently: reflect.DeepEqual against cloneBuilderParameters has no exhaustiveness guard, and matches folds its failure causes into a bare bool. A reflect.NumField pin test plus a reason-tagged log or counter makes drift visible.

  2. cl/beacon/handler/block_production.go:1138canUsePreparedPayload uses engine.SupportInsertion() as a stand-in for "payload ids come from the same builder index". The two capabilities coincide today but are independent; name the real precondition (builder continuity).

  3. cl/phase1/execution_client/execution_client_direct.go:209 — the context-aware sleep is hand-rolled three times (here, and the time.After selects in block_production.go:279 and payload_preparation.go:329); common.Sleep(ctx, d) is a one-line replacement.

  4. execution/execmodule/block_building.go:92builderEntry.timestamp duplicates params.Timestamp, and the nil-guards plus the lazy map init guard states production never creates. Read params.Timestamp directly and build test fixtures through AssembleBlock.

  5. cl/beacon/handler/block_production_test.go:1123 — the test brute-forces up to 256 randao nonces, each iteration paying a full state copy plus an epoch transition. Pin the discovered nonce as a fixture constant and keep the require.NotEqual guard.

  6. Comment policy: the stopped-builder rationale appears in full both at execution/execmodule/block_building.go:138 and in Failed's docstring (execution/builder/block_builder.go:111); payload_preparation.go:158-161 narrates a step-by-step failure scenario; several comments exceed B2 English ("masquerading as contention", the pollAssembledPayload preamble, the ErrForkChoiceBusy doc). Trim per CLAUDE.md.

@lystopad
lystopad marked this pull request as draft August 14, 2026 08:44
auto-merge was automatically disabled August 14, 2026 08:44

Pull request was converted to draft

lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so the mechanical cleanup can be reviewed on its own.

The conversion between cltypes.Withdrawal and types.Withdrawal was hand-written at each
call site. Export the existing converter and use it, so the lists cannot drift apart.

No behaviour change: both call sites produce the same slice, including its nil-ness.
Sahil-4555 pushed a commit to Sahil-4555/erigon that referenced this pull request Aug 14, 2026
…nd the caller's context (erigontech#23273)

Split out of erigontech#23105, which grew too large to review in one piece.
Independent of the other parts of that series.

### Contention was indistinguishable from rejection

`AssembleBlock` and `GetAssembledBlock` reported a busy execution module
as `errors.New("execution data is still syncing")`. That message is
inaccurate — it is weight-one semaphore contention with a forkchoice
update or another payload request, not syncing — and being an untyped
error, callers could not tell it apart from a real rejection.

Both now return `chainreader.ErrExecutionBusy`, and the assemble retry
waits only on that. Previously a permanent rejection — mismatched
withdrawals, for instance — was retried thirty times across six seconds
before surfacing, which spends the proposal slot rather than reporting
the problem.

### The retry ignored its caller

The loop was `for range 30 { ...; time.Sleep(200 * time.Millisecond) }`,
with no context check anywhere. A cancelled caller still waited the full
six seconds. It is now an extracted helper that checks the context
before each attempt and waits on it rather than on a bare sleep.

While there, `ChainReaderWriterEth1.AssembleBlock` and
`GetAssembledBlock` take the caller's context instead of substituting
`context.Background()`, so the deadline a caller sets actually reaches
the execution module.

### Tests

The helper is covered directly: first success, stopping on a rejection,
exhausting attempts on contention, cancellation mid-flight and before
the first attempt, and the zero-attempts guard.

Part of a series splitting erigontech#23105.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so it can be reviewed on its own. The preparation work there leans on
this, but every problem below is reachable today.

A single lastParameters field remembered only the most recent request, so any interleaved
request for a different timestamp destroyed the deduplication for the first: a repeated
request then started a second builder for a payload already being built. Builders are now
kept by the timestamp they are for, alongside an immutable copy of the parameters they were
created with, so a caller cannot mutate the slice it passed and change what a later
comparison sees.

A builder that failed latched its error and was handed back forever, spending the slot
waiting on a payload that could never arrive. It is now treated as absent, and dropped when
its error surfaces. Being stopped is not failure: a stopped builder still holds the payload
it was stopped for, which is exactly what a repeated request is asking for.

Eviction dropped builders from the map without stopping them, so the goroutine kept running
with no way to reach it. It now cancels on the way out, which is the problem described in
issue #23101. Both entry points check for a cancelled caller before acting, so an expired
request reports why it stopped rather than looking like contention that callers retry.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so it can be reviewed on its own. No behaviour change.

produceBeaconBody chose between three withdrawal sources inline, each with its own
hand-written conversion loop, and then assembled the attributes around the result. The choice
is now a method that names what it selects between, and the attributes come from a single
constructor. That leaves the fork-specific part of production as the two fields only Gloas
sends.

The three conversion loops go through the shared converter, which is what the doc comment on
that converter has been describing.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so it can be reviewed on its own. The preparation work there leans on
this, but every problem below is reachable today.

A single lastParameters field remembered only the most recent request, so any interleaved
request for a different timestamp destroyed the deduplication for the first: a repeated
request then started a second builder for a payload already being built. Builders are now
kept by the timestamp they are for, alongside an immutable copy of the parameters they were
created with, so a caller cannot mutate the slice it passed and change what a later
comparison sees.

A builder that failed latched its error and was handed back forever, spending the slot
waiting on a payload that could never arrive. It is now treated as absent, and dropped when
its error surfaces. Being stopped is not failure: a stopped builder still holds the payload
it was stopped for, which is exactly what a repeated request is asking for.

Eviction dropped builders from the map without stopping them, so the goroutine kept running
with no way to reach it. It now cancels on the way out, which is the problem described in
issue #23101. Both entry points check for a cancelled caller before acting, so an expired
request reports why it stopped rather than looking like contention that callers retry.
pull Bot pushed a commit to Dustin4444/erigon that referenced this pull request Aug 14, 2026
…rigontech#23271)

Split out of erigontech#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 erigontech#23105 into reviewable units. The remaining
parts follow separately; this one stands alone and depends on nothing
else in the series.
lystopad added a commit that referenced this pull request Aug 14, 2026
Split out of #23105 so it can be reviewed on its own. No behaviour change.

produceBeaconBody chose between three withdrawal sources inline, each with its own
hand-written conversion loop, and then assembled the attributes around the result. The choice
is now a method that names what it selects between, and the attributes come from a single
constructor. That leaves the fork-specific part of production as the two fields only Gloas
sends.

The three conversion loops go through the shared converter, which is what the doc comment on
that converter has been describing.
lystopad added a commit that referenced this pull request Aug 14, 2026
…e recipient at all

Split out of #23105 so it can be reviewed on its own.

A slot whose payload never arrives fails on every poll of the collection window, at the
retry cadence, so one failure produced hundreds of copies of the same line. Report the first
and then a count, and only for a window that produced nothing: contention that clears is not
worth an alert.

Production falls back to the zero address when no fee recipient is registered for the
proposer, which gives that block's fees away. Say so rather than leave it to be discovered
from the produced block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Caplin Caplin: Consensus Layer, Beacon API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants