From a7d8cc357e9b7cf36c54de67238118a7d9fa70de Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 7 Aug 2026 17:33:35 +0200 Subject: [PATCH 01/20] cl/beacon: prime the execution layer before a slot this node proposes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caplin only sends payload attributes when the validator client asks for a block, at the start of the proposal slot. The execution layer then has to pack the block from scratch, so Caplin waits until a publication margin before the attestation deadline to collect it. The block is finished late in the slot and leaves little room for signing and gossip. 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 early in the slot instead. Preparation runs only when the next proposer is a validator the client has registered a fee recipient for, which leaves nodes that do not propose untouched. Production compares the payload id it gets back against the primed one and falls back to its previous, later schedule whenever they differ — a reorg, a late block, a changed fee recipient, or an execution layer that was busy — so the worst case is the behaviour it has today. --- cl/beacon/handler/block_production.go | 19 +- cl/beacon/handler/block_production_test.go | 57 +++++- cl/beacon/handler/handler.go | 3 + cl/beacon/handler/payload_preparation.go | 207 +++++++++++++++++++++ cmd/caplin/caplin1/run.go | 1 + 5 files changed, 278 insertions(+), 9 deletions(-) create mode 100644 cl/beacon/handler/payload_preparation.go diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index 5533cf7b1ec..0e6efa7f038 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -208,11 +208,18 @@ func attestationDue(cfg *clparams.BeaconChainConfig, stateVersion clparams.State return time.Duration(cfg.AttestationDueMs(stateVersion.AfterOrEqual(clparams.GloasVersion))) * time.Millisecond } -// computeBlockBuilderWindow returns when to first poll for the assembled payload and when to stop, -// reserving a publication margin before the attestation deadline (see payloadPublicationDivisor). -func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion) blockBuilderWindow { +// computeBlockBuilderWindow returns when to first poll for the assembled payload and when to stop. +// +// Without a primed builder the execution layer only starts packing when the block is requested, so +// polling stops a publication margin before the attestation deadline to give it most of the slot +// (see payloadPublicationDivisor). A primed builder has been packing since before the slot, so the +// payload can be taken early instead, leaving the rest of the margin for signing and gossip. +func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion, prepared bool) blockBuilderWindow { due := attestationDue(cfg, stateVersion) grabBy := slotStart.Add(due - due/payloadPublicationDivisor) + if prepared { + grabBy = slotStart.Add(due / payloadPublicationDivisor) + } firstGetAt := grabBy.Add(-minPayloadPollingWindow) if firstGetAt.Before(now) { firstGetAt = now @@ -1063,7 +1070,11 @@ func (a *ApiHandler) produceBeaconBody( return } slotStart := a.ethClock.GetSlotTime(targetSlot) - buildWindow := computeBlockBuilderWindow(builderStartedAt, slotStart, a.beaconChainCfg, stateVersion) + // An identical payload id means the execution layer kept the builder primed before the slot, + // so the payload is already packed and does not need the rest of the slot to fill. + prepared := a.preparedPayload.matches(targetSlot, idBytes) + log.Info("BlockProduction: payload preparation", "slot", targetSlot, "prepared", prepared) + buildWindow := computeBlockBuilderWindow(builderStartedAt, slotStart, a.beaconChainCfg, stateVersion, prepared) payload, bundles, requestsBundle, blockValue, ok := pollAssembledPayload(ctx, buildWindow, retryTime, func() (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { return a.engine.GetAssembledBlock(ctx, idBytes, stateVersion) }) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 6f6951bc734..5d004cdebe2 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -59,7 +59,7 @@ func TestBlockBuilderWindowPreGloas(t *testing.T) { slotStart := time.Unix(100, 0) now := slotStart - window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.ElectraVersion) + window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.ElectraVersion, false) // Attestation deadline is 4s; polling stops a quarter of it (1s) earlier, at 3s. require.Equal(t, slotStart.Add(3*time.Second).Add(-minPayloadPollingWindow), window.firstGetAt) @@ -74,7 +74,7 @@ func TestBlockBuilderWindowGloas(t *testing.T) { slotStart := time.Unix(100, 0) now := slotStart - window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.GloasVersion) + window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.GloasVersion, false) // Attestation deadline is 3s; polling stops a quarter of it (750ms) earlier, at 2.25s. require.Equal(t, slotStart.Add(2250*time.Millisecond).Add(-minPayloadPollingWindow), window.firstGetAt) @@ -327,7 +327,7 @@ func TestBlockBuilderWindowLateStartKeepsPublicationMargin(t *testing.T) { slotStart := time.Unix(100, 0) now := slotStart.Add(2950 * time.Millisecond) - window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.ElectraVersion) + window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.ElectraVersion, false) // A late request clamps the first poll up to now but still stops at 3s, preserving the margin. require.Equal(t, now, window.firstGetAt) @@ -342,7 +342,7 @@ func TestBlockBuilderWindowLateRequestGrabsImmediately(t *testing.T) { slotStart := time.Unix(100, 0) now := slotStart.Add(5 * time.Second) - window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.GloasVersion) + window := computeBlockBuilderWindow(now, slotStart, cfg, clparams.GloasVersion, false) require.Equal(t, now, window.firstGetAt) require.Equal(t, now, window.pollUntil) @@ -365,7 +365,7 @@ func TestBlockBuilderWindowReservesPublicationMargin(t *testing.T) { {"gloas", clparams.GloasVersion, 3 * time.Second, 2250 * time.Millisecond}, } { t.Run(tc.name, func(t *testing.T) { - window := computeBlockBuilderWindow(slotStart, slotStart, cfg, tc.version) + window := computeBlockBuilderWindow(slotStart, slotStart, cfg, tc.version, false) require.Equal(t, slotStart.Add(tc.wantPollUntil), window.pollUntil) require.True(t, window.pollUntil.Before(slotStart.Add(tc.deadline)), "polling must stop before the attestation deadline to leave publication margin") @@ -732,3 +732,50 @@ func TestCaplinBlockProductionGlamsterdamSlotNumber(t *testing.T) { require.Equal(t, hexutil.Uint64(targetSlot), *spy.lastAttributes.SlotNumber, "SlotNumber should equal the target slot") } + +func TestBlockBuilderWindowTakesPreparedPayloadEarly(t *testing.T) { + cfg := &clparams.BeaconChainConfig{ + SecondsPerSlot: 12, + IntervalsPerSlot: 3, + } + slotStart := time.Unix(100, 0) + + // 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. + prepared := computeBlockBuilderWindow(slotStart, slotStart, cfg, clparams.ElectraVersion, true) + require.Equal(t, slotStart.Add(time.Second), prepared.pollUntil) + + // Without a primed builder nothing changes: the execution layer still needs most of the slot. + unprepared := computeBlockBuilderWindow(slotStart, slotStart, cfg, clparams.ElectraVersion, false) + require.Equal(t, slotStart.Add(3*time.Second), unprepared.pollUntil) + + require.True(t, prepared.pollUntil.Before(unprepared.pollUntil)) +} + +func TestPreparedPayloadMatchesOnlyTheSamePrime(t *testing.T) { + var p preparedPayload + id := []byte{1, 2, 3, 4, 5, 6, 7, 8} + + require.False(t, p.matches(10, id), "nothing primed yet") + + p.set(10, id) + require.True(t, p.matches(10, id)) + + // A different payload id means the execution layer started a fresh build — a reorg, a late + // block, or a changed fee recipient — so the warm builder is gone. + require.False(t, p.matches(10, []byte{9, 9, 9, 9, 9, 9, 9, 9})) + require.False(t, p.matches(11, id), "primed for another slot") + require.False(t, p.matches(10, nil), "no id from the execution layer") +} + +func TestPreparedPayloadCopiesTheID(t *testing.T) { + var p preparedPayload + id := []byte{1, 2, 3, 4, 5, 6, 7, 8} + + p.set(10, id) + id[0] = 0xff + + // The caller's buffer must not be able to invalidate, or forge, a later match. + require.True(t, p.matches(10, []byte{1, 2, 3, 4, 5, 6, 7, 8})) + require.False(t, p.matches(10, id)) +} diff --git a/cl/beacon/handler/handler.go b/cl/beacon/handler/handler.go index 1f15ca474cb..c2d1c7bd892 100644 --- a/cl/beacon/handler/handler.go +++ b/cl/beacon/handler/handler.go @@ -106,6 +106,9 @@ type ApiHandler struct { routerCfg *beacon_router_configuration.RouterConfiguration logger log.Logger + // preparedPayload tracks the payload primed ahead of a slot this node proposes. + preparedPayload preparedPayload + // Validator data structures validatorParams *validator_params.ValidatorParams blobBundles *lru.Cache[common.Bytes48, BlobBundle] // Keep recent bundled blobs from the execution layer. diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go new file mode 100644 index 00000000000..29af1fcae4f --- /dev/null +++ b/cl/beacon/handler/payload_preparation.go @@ -0,0 +1,207 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package handler + +import ( + "bytes" + "context" + "errors" + "sync" + "time" + + "github.com/erigontech/erigon/cl/clparams" + "github.com/erigontech/erigon/cl/phase1/core/state" + "github.com/erigontech/erigon/cl/transition" + "github.com/erigontech/erigon/common" + "github.com/erigontech/erigon/common/hexutil" + "github.com/erigontech/erigon/common/log/v3" + "github.com/erigontech/erigon/execution/engineapi/engine_types" + "github.com/erigontech/erigon/execution/types" +) + +var ( + errNodeSyncing = errors.New("node is syncing") + errNotOurProposal = errors.New("next slot is not proposed by a registered validator") + errNoPayloadID = errors.New("execution layer returned no payload id") +) + +// preparedPayload records the payload id the execution layer returned for a slot this node primed +// ahead of time. Block production compares the id its own forkchoice update returns against this +// record: an equal id means the execution layer recognised the request as a repeat and has been +// packing transactions since the prime, so the payload is worth taking early. Anything else — a +// reorg, a late block, a changed fee recipient, an execution layer that was busy — yields a +// different id and leaves production on its usual later schedule. +type preparedPayload struct { + mu sync.Mutex + slot uint64 + payloadID []byte +} + +func (p *preparedPayload) set(slot uint64, payloadID []byte) { + p.mu.Lock() + defer p.mu.Unlock() + p.slot, p.payloadID = slot, bytes.Clone(payloadID) +} + +func (p *preparedPayload) matches(slot uint64, payloadID []byte) bool { + p.mu.Lock() + defer p.mu.Unlock() + return len(payloadID) > 0 && p.slot == slot && bytes.Equal(p.payloadID, payloadID) +} + +// StartPayloadPreparation primes the execution layer for slots this node is due to propose, so the +// payload is already packed when the validator client asks for a block instead of being built from +// scratch inside the proposal slot. +func (a *ApiHandler) StartPayloadPreparation(ctx context.Context) { + go a.preparePayloadLoop(ctx) +} + +func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { + // A quarter-slot tick lands well inside every slot without assuming when in the slot the head + // arrives; preparation is skipped unless the next slot is ours, so the cost is a proposer + // lookup on a state we already hold. + ticker := time.NewTicker(time.Duration(a.beaconChainCfg.SecondsPerSlot) * time.Second / 4) + defer ticker.Stop() + + var lastPrepared uint64 + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + } + + targetSlot := a.ethClock.GetCurrentSlot() + 1 + if targetSlot <= lastPrepared { + continue + } + if a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch).AfterOrEqual(clparams.GloasVersion) { + // [Gloas:EIP7732] builders gossip bids instead; the engine is not primed this way. + continue + } + if err := a.preparePayloadFor(ctx, targetSlot); err != nil { + log.Debug("PayloadPreparation: skipped", "slot", targetSlot, "err", err) + continue + } + lastPrepared = targetSlot + } +} + +// preparePayloadFor sends the forkchoice update for targetSlot ahead of the slot itself. It returns +// an error, rather than logging loudly, whenever there is simply nothing to do — the node is +// syncing, the slot belongs to someone else, or the validator client has not registered a fee +// recipient yet — because block production falls back to building inside the slot in every one of +// those cases. +func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) error { + baseBlockRoot := a.syncedData.HeadRoot() + if baseBlockRoot == (common.Hash{}) { + return errNodeSyncing + } + + var proposerIndex uint64 + if err := a.syncedData.ViewHeadState(func(headState *state.CachingBeaconState) error { + var err error + proposerIndex, err = headState.GetBeaconProposerIndexForSlot(targetSlot) + return err + }); err != nil { + return err + } + // Only our own proposals are worth priming, and a fee recipient we do not yet know would build + // a payload that block production could not reuse anyway. + feeRecipient, ok := a.validatorParams.GetFeeRecipient(proposerIndex) + if !ok { + return errNotOurProposal + } + + var baseState *state.CachingBeaconState + if err := a.syncedData.ViewHeadState(func(headState *state.CachingBeaconState) error { + var err error + baseState, err = headState.Copy() + return err + }); err != nil { + return err + } + if err := transition.DefaultMachine.ProcessSlots(baseState, targetSlot); err != nil { + return err + } + + head, safeHash, finalizedHash, attrs, err := a.preparedForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) + if err != nil { + return err + } + stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) + payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalizedHash, safeHash, head, attrs, stateVersion) + if err != nil { + return err + } + if len(payloadID) == 0 { + return errNoPayloadID + } + + a.preparedPayload.set(targetSlot, payloadID) + log.Info("PayloadPreparation: primed execution layer", "slot", targetSlot, "proposer", proposerIndex) + return nil +} + +// 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 +// nothing else — production simply builds inside the slot as before — but it is silent, so +// PayloadPreparation logs whether the primed id was still valid at production time. +func (a *ApiHandler) preparedForkChoiceInputs( + baseState *state.CachingBeaconState, + baseBlockRoot common.Hash, + targetSlot uint64, + feeRecipient common.Address, +) (head, safeHash, finalizedHash common.Hash, attrs *engine_types.PayloadAttributes, err error) { + head = baseState.LatestExecutionPayloadHeader().BlockHash + + finalizedHash = a.forkchoiceStore.GetFinalizedExecutionHash(baseState.FinalizedCheckpoint().Root) + if finalizedHash == (common.Hash{}) { + finalizedHash = head + } + safeHash = a.forkchoiceStore.GetFinalizedExecutionHash(baseState.CurrentJustifiedCheckpoint().Root) + if safeHash == (common.Hash{}) { + safeHash = head + } + + epoch := targetSlot / a.beaconChainCfg.SlotsPerEpoch + clWithdrawals, err := state.GetExpectedWithdrawals(baseState, epoch) + if err != nil { + return head, safeHash, finalizedHash, nil, err + } + withdrawals := make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals)) + for _, w := range clWithdrawals.Withdrawals { + withdrawals = append(withdrawals, &types.Withdrawal{ + Index: w.Index, + Amount: w.Amount, + Validator: w.Validator, + Address: w.Address, + }) + } + + attrs = &engine_types.PayloadAttributes{ + Timestamp: hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), + PrevRandao: baseState.GetRandaoMixes(epoch), + SuggestedFeeRecipient: feeRecipient, + Withdrawals: withdrawals, + ParentBeaconBlockRoot: &baseBlockRoot, + } + return head, safeHash, finalizedHash, attrs, nil +} diff --git a/cmd/caplin/caplin1/run.go b/cmd/caplin/caplin1/run.go index a744357bc8e..91aa5ac5bfc 100644 --- a/cmd/caplin/caplin1/run.go +++ b/cmd/caplin/caplin1/run.go @@ -623,6 +623,7 @@ func RunCaplinService(ctx context.Context, engine execution_client.ExecutionEngi payloadAttestationService, proposerPreferencesService, ) + apiHandler.StartPayloadPreparation(ctx) go beacon.ListenAndServe(&beacon.LayeredBeaconHandler{ ArchiveApi: apiHandler, }, config.BeaconAPIRouter) From 6d86abf0dc5aaa3fc77a2b82fc7b494bc078ac95 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 7 Aug 2026 18:30:15 +0200 Subject: [PATCH 02/20] cl/beacon: address review on payload preparation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep a primed record per slot rather than one at a time: consecutive proposals let the prime for the later slot evict the record production was about to look up, losing the warm builder and reporting prepared as false when it was not. Read the head root, the proposer and the state under one view of the head state. Reading them separately let a head update in between pair a parent beacon block root with a different state, priming a builder that production could never match. Checking the fee recipient inside the same view also skips the state copy when the slot is not ours. Log only unexpected failures. Most ticks land on a slot somebody else proposes, and reporting those drowned out the ones worth seeing. Skip preparation when the head sits beyond the proposer lookahead, where the proposer index has to be reshuffled from the seed — far too costly to repeat every tick on a large validator set. --- cl/beacon/handler/block_production_test.go | 17 ++++ cl/beacon/handler/payload_preparation.go | 91 +++++++++++++++------- 2 files changed, 82 insertions(+), 26 deletions(-) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 5d004cdebe2..7ce7974f9dc 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -768,6 +768,23 @@ func TestPreparedPayloadMatchesOnlyTheSamePrime(t *testing.T) { require.False(t, p.matches(10, nil), "no id from the execution layer") } +func TestPreparedPayloadKeepsConsecutiveSlots(t *testing.T) { + var p preparedPayload + first := []byte{1, 1, 1, 1, 1, 1, 1, 1} + second := []byte{2, 2, 2, 2, 2, 2, 2, 2} + + // Consecutive proposals: priming slot 11 must not evict slot 10, whose block may still be + // in production. + p.set(10, first) + p.set(11, second) + require.True(t, p.matches(10, first)) + require.True(t, p.matches(11, second)) + + // Records old enough that they can no longer be produced are dropped, so the map is bounded. + p.set(10+preparedPayloadRetainSlots+1, []byte{3, 3, 3, 3, 3, 3, 3, 3}) + require.False(t, p.matches(10, first)) +} + func TestPreparedPayloadCopiesTheID(t *testing.T) { var p preparedPayload id := []byte{1, 2, 3, 4, 5, 6, 7, 8} diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 29af1fcae4f..c1e89e1e47f 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -23,6 +23,7 @@ import ( "sync" "time" + "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/clparams" "github.com/erigontech/erigon/cl/phase1/core/state" "github.com/erigontech/erigon/cl/transition" @@ -37,30 +38,45 @@ var ( errNodeSyncing = errors.New("node is syncing") errNotOurProposal = errors.New("next slot is not proposed by a registered validator") errNoPayloadID = errors.New("execution layer returned no payload id") + errHeadTooFarBack = errors.New("head state is too far behind the slot to prepare") ) +// preparedPayloadRetainSlots keeps a primed record alive past the slot it was primed for, so +// priming the next slot cannot evict the record for a proposal that is still being produced. +const preparedPayloadRetainSlots = 2 + // preparedPayload records the payload id the execution layer returned for a slot this node primed // ahead of time. Block production compares the id its own forkchoice update returns against this // record: an equal id means the execution layer recognised the request as a repeat and has been // packing transactions since the prime, so the payload is worth taking early. Anything else — a // reorg, a late block, a changed fee recipient, an execution layer that was busy — yields a // different id and leaves production on its usual later schedule. +// Records are kept per slot: consecutive proposals would otherwise let the prime for the later slot +// evict the one production is about to look up. type preparedPayload struct { - mu sync.Mutex - slot uint64 - payloadID []byte + mu sync.Mutex + payloads map[uint64][]byte } func (p *preparedPayload) set(slot uint64, payloadID []byte) { p.mu.Lock() defer p.mu.Unlock() - p.slot, p.payloadID = slot, bytes.Clone(payloadID) + if p.payloads == nil { + p.payloads = map[uint64][]byte{} + } + // Slots this far back can no longer be produced, so dropping them bounds the map. + for recorded := range p.payloads { + if recorded+preparedPayloadRetainSlots < slot { + delete(p.payloads, recorded) + } + } + p.payloads[slot] = bytes.Clone(payloadID) } func (p *preparedPayload) matches(slot uint64, payloadID []byte) bool { p.mu.Lock() defer p.mu.Unlock() - return len(payloadID) > 0 && p.slot == slot && bytes.Equal(p.payloadID, payloadID) + return len(payloadID) > 0 && bytes.Equal(p.payloads[slot], payloadID) } // StartPayloadPreparation primes the execution layer for slots this node is due to propose, so the @@ -94,47 +110,70 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { continue } if err := a.preparePayloadFor(ctx, targetSlot); err != nil { - log.Debug("PayloadPreparation: skipped", "slot", targetSlot, "err", err) + // 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) + } continue } lastPrepared = targetSlot } } +// isExpectedPreparationSkip reports whether there was simply nothing to prepare, as opposed to a +// failure worth reporting. +func isExpectedPreparationSkip(err error) bool { + return errors.Is(err, errNotOurProposal) || + errors.Is(err, errNodeSyncing) || + errors.Is(err, errHeadTooFarBack) || + errors.Is(err, synced_data.ErrNotSynced) +} + // preparePayloadFor sends the forkchoice update for targetSlot ahead of the slot itself. It returns // an error, rather than logging loudly, whenever there is simply nothing to do — the node is // syncing, the slot belongs to someone else, or the validator client has not registered a fee // recipient yet — because block production falls back to building inside the slot in every one of // those cases. func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) error { - baseBlockRoot := a.syncedData.HeadRoot() - if baseBlockRoot == (common.Hash{}) { - return errNodeSyncing - } - - var proposerIndex uint64 + var ( + baseBlockRoot common.Hash + proposerIndex uint64 + feeRecipient common.Address + baseState *state.CachingBeaconState + ) + // Root, proposer and state all come from one view of the head. Reading them separately would + // let a head update in between pair a parent beacon block root with a different state, priming + // a builder that production can never match. if err := a.syncedData.ViewHeadState(func(headState *state.CachingBeaconState) error { - var err error - proposerIndex, err = headState.GetBeaconProposerIndexForSlot(targetSlot) - return err - }); err != nil { - return err - } - // Only our own proposals are worth priming, and a fee recipient we do not yet know would build - // a payload that block production could not reuse anyway. - feeRecipient, ok := a.validatorParams.GetFeeRecipient(proposerIndex) - if !ok { - return errNotOurProposal - } + baseBlockRoot = a.syncedData.HeadRoot() + if baseBlockRoot == (common.Hash{}) { + return errNodeSyncing + } + // Beyond the proposer lookahead the index has to be reshuffled from the seed, which is far + // too costly to repeat every tick on a large validator set. + slotsPerEpoch := a.beaconChainCfg.SlotsPerEpoch + if targetSlot/slotsPerEpoch > headState.Slot()/slotsPerEpoch+a.beaconChainCfg.MinSeedLookahead { + return errHeadTooFarBack + } - var baseState *state.CachingBeaconState - if err := a.syncedData.ViewHeadState(func(headState *state.CachingBeaconState) error { var err error + if proposerIndex, err = headState.GetBeaconProposerIndexForSlot(targetSlot); err != nil { + return err + } + // Only our own proposals are worth priming, and a fee recipient we do not yet know would + // build a payload that block production could not reuse anyway. Checked before the state + // copy, which is the expensive part. + var ok bool + if feeRecipient, ok = a.validatorParams.GetFeeRecipient(proposerIndex); !ok { + return errNotOurProposal + } baseState, err = headState.Copy() return err }); err != nil { return err } + if err := transition.DefaultMachine.ProcessSlots(baseState, targetSlot); err != nil { return err } From bfb96eac5b23b7cae358db19003f12e36a5b2144 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 7 Aug 2026 19:16:36 +0200 Subject: [PATCH 03/20] cl/beacon: log once that payload preparation is running Preparation is silent on a node that rarely proposes, so a loop that never started is indistinguishable from one with nothing to do. Say once at startup that it is watching, and at what cadence. --- cl/beacon/handler/payload_preparation.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index c1e89e1e47f..f0fc5f83779 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -90,7 +90,11 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { // A quarter-slot tick lands well inside every slot without assuming when in the slot the head // arrives; preparation is skipped unless the next slot is ours, so the cost is a proposer // lookup on a state we already hold. - ticker := time.NewTicker(time.Duration(a.beaconChainCfg.SecondsPerSlot) * time.Second / 4) + tick := time.Duration(a.beaconChainCfg.SecondsPerSlot) * time.Second / 4 + // Preparation is silent on a node that rarely proposes, so say once that it is running: + // otherwise a loop that never started looks exactly like one with nothing to do. + log.Info("PayloadPreparation: watching for proposals", "every", tick) + ticker := time.NewTicker(tick) defer ticker.Stop() var lastPrepared uint64 From 7c73858939b62b89b9b128c7b1b257c5af2a9e74 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 7 Aug 2026 21:31:34 +0200 Subject: [PATCH 04/20] cl/beacon: prime again when the head moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preparation primed a slot once and never revisited it. When the previous slot's block arrives late the head moves after that, leaving the execution layer warming a builder on a parent that is no longer the head; production then finds a different payload id and falls back to building inside the slot. That is precisely the case where the proposal is most at risk, so priming once misses the slots that need it most. Track the head the slot was primed on and prime again whenever it changes. Observed on a mainnet proposal: the previous slot's block landed 3.4s late, moving the head more than eight seconds before the proposal — ample time to prime again, which the loop had no way to notice. --- cl/beacon/handler/block_production_test.go | 18 +++++++++++ cl/beacon/handler/payload_preparation.go | 36 ++++++++++++++-------- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 7ce7974f9dc..a4e4a3d39ad 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -796,3 +796,21 @@ func TestPreparedPayloadCopiesTheID(t *testing.T) { require.True(t, p.matches(10, []byte{1, 2, 3, 4, 5, 6, 7, 8})) require.False(t, p.matches(10, id)) } + +func TestShouldPrepareAgainWhenTheHeadMoves(t *testing.T) { + headA := common.Hash{0xaa} + headB := common.Hash{0xbb} + + // Nothing primed yet. + require.True(t, shouldPrepare(10, 0, headA, common.Hash{})) + + // Already primed this slot on this head: nothing to do. + require.False(t, shouldPrepare(10, 10, headA, headA)) + + // The previous slot's block arrived late and moved the head, so the warm builder is on a parent + // that is no longer the head — prime again rather than wait for the next slot. + require.True(t, shouldPrepare(10, 10, headB, headA)) + + // A new target slot always needs priming. + require.True(t, shouldPrepare(11, 10, headA, headA)) +} diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index f0fc5f83779..772cdfa5e08 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -97,7 +97,10 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { ticker := time.NewTicker(tick) defer ticker.Stop() - var lastPrepared uint64 + var ( + primedSlot uint64 + primedHead common.Hash + ) for { select { case <-ctx.Done(): @@ -106,14 +109,15 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { } targetSlot := a.ethClock.GetCurrentSlot() + 1 - if targetSlot <= lastPrepared { + if !shouldPrepare(targetSlot, primedSlot, a.syncedData.HeadRoot(), primedHead) { continue } if a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch).AfterOrEqual(clparams.GloasVersion) { // [Gloas:EIP7732] builders gossip bids instead; the engine is not primed this way. continue } - if err := a.preparePayloadFor(ctx, targetSlot); err != nil { + head, err := a.preparePayloadFor(ctx, targetSlot) + if err != nil { // Most ticks land on a slot somebody else proposes; logging those would drown out the // failures worth seeing. if !isExpectedPreparationSkip(err) { @@ -121,10 +125,18 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { } continue } - lastPrepared = targetSlot + primedSlot, primedHead = targetSlot, head } } +// shouldPrepare reports whether the target slot still needs priming. Priming once per slot is not +// enough: when the previous slot's block arrives late the head moves after we have already primed, +// and the execution layer is left warming a builder on a parent that is no longer the head. That is +// exactly the case where the proposal is most at risk, so prime again whenever the head changes. +func shouldPrepare(targetSlot, primedSlot uint64, head, primedHead common.Hash) bool { + return targetSlot != primedSlot || head != primedHead +} + // isExpectedPreparationSkip reports whether there was simply nothing to prepare, as opposed to a // failure worth reporting. func isExpectedPreparationSkip(err error) bool { @@ -139,7 +151,7 @@ func isExpectedPreparationSkip(err error) bool { // syncing, the slot belongs to someone else, or the validator client has not registered a fee // recipient yet — because block production falls back to building inside the slot in every one of // those cases. -func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) error { +func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) (common.Hash, error) { var ( baseBlockRoot common.Hash proposerIndex uint64 @@ -175,29 +187,29 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) e baseState, err = headState.Copy() return err }); err != nil { - return err + return common.Hash{}, err } if err := transition.DefaultMachine.ProcessSlots(baseState, targetSlot); err != nil { - return err + return common.Hash{}, err } head, safeHash, finalizedHash, attrs, err := a.preparedForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) if err != nil { - return err + return common.Hash{}, err } stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalizedHash, safeHash, head, attrs, stateVersion) if err != nil { - return err + return common.Hash{}, err } if len(payloadID) == 0 { - return errNoPayloadID + return common.Hash{}, errNoPayloadID } a.preparedPayload.set(targetSlot, payloadID) - log.Info("PayloadPreparation: primed execution layer", "slot", targetSlot, "proposer", proposerIndex) - return nil + log.Info("PayloadPreparation: primed execution layer", "slot", targetSlot, "proposer", proposerIndex, "head", baseBlockRoot) + return baseBlockRoot, nil } // preparedForkChoiceInputs assembles the forkchoice-update arguments for building targetSlot on top From 174a3ca75be27a5a9d02f50321af24c0b15e5e14 Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 19:33:13 +0800 Subject: [PATCH 05/20] cl/beacon, execution: harden payload preparation --- cl/beacon/handler/block_production.go | 15 +++- cl/beacon/handler/block_production_test.go | 55 ++++++++---- cl/beacon/handler/payload_preparation.go | 47 ++++------ execution/builder/block_builder.go | 6 +- execution/execmodule/block_building.go | 20 ++++- .../block_building_internal_test.go | 89 +++++++++++++++++++ execution/execmodule/exec_module.go | 10 ++- 7 files changed, 187 insertions(+), 55 deletions(-) diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index 0e6efa7f038..edd66ae4f29 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -234,6 +234,11 @@ func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconCha } } +func preparedPayloadMinimumAge(cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion) time.Duration { + due := attestationDue(cfg, stateVersion) + return max(due-2*due/payloadPublicationDivisor, 0) +} + func shouldRetryGetPayload(now, deadline time.Time) bool { return now.Before(deadline) } @@ -1070,9 +1075,13 @@ func (a *ApiHandler) produceBeaconBody( return } slotStart := a.ethClock.GetSlotTime(targetSlot) - // An identical payload id means the execution layer kept the builder primed before the slot, - // so the payload is already packed and does not need the rest of the slot to fill. - prepared := a.preparedPayload.matches(targetSlot, idBytes) + // Early collection requires both the same builder and enough pre-slot packing time. + prepared := a.preparedPayload.matches( + targetSlot, + idBytes, + time.Now(), + preparedPayloadMinimumAge(a.beaconChainCfg, stateVersion), + ) log.Info("BlockProduction: payload preparation", "slot", targetSlot, "prepared", prepared) buildWindow := computeBlockBuilderWindow(builderStartedAt, slotStart, a.beaconChainCfg, stateVersion, prepared) payload, bundles, requestsBundle, blockValue, ok := pollAssembledPayload(ctx, buildWindow, retryTime, func() (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index a4e4a3d39ad..b1734beee46 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -755,46 +755,71 @@ func TestBlockBuilderWindowTakesPreparedPayloadEarly(t *testing.T) { func TestPreparedPayloadMatchesOnlyTheSamePrime(t *testing.T) { var p preparedPayload id := []byte{1, 2, 3, 4, 5, 6, 7, 8} + now := time.Unix(100, 0) - require.False(t, p.matches(10, id), "nothing primed yet") + require.False(t, p.matches(10, id, now, 0), "nothing primed yet") - p.set(10, id) - require.True(t, p.matches(10, id)) + p.set(10, id, now) + require.True(t, p.matches(10, id, now, 0)) // A different payload id means the execution layer started a fresh build — a reorg, a late // block, or a changed fee recipient — so the warm builder is gone. - require.False(t, p.matches(10, []byte{9, 9, 9, 9, 9, 9, 9, 9})) - require.False(t, p.matches(11, id), "primed for another slot") - require.False(t, p.matches(10, nil), "no id from the execution layer") + require.False(t, p.matches(10, []byte{9, 9, 9, 9, 9, 9, 9, 9}, now, 0)) + require.False(t, p.matches(11, id, now, 0), "primed for another slot") + require.False(t, p.matches(10, nil, now, 0), "no id from the execution layer") +} + +func TestPreparedPayloadRequiresMinimumWarmup(t *testing.T) { + var p preparedPayload + id := []byte{1, 2, 3, 4, 5, 6, 7, 8} + now := time.Unix(100, 0) + minAge := 2 * time.Second + + p.set(10, id, now.Add(-minAge+time.Nanosecond)) + require.False(t, p.matches(10, id, now, minAge)) + + p.set(10, id, now.Add(-minAge)) + require.True(t, p.matches(10, id, now, minAge)) +} + +func TestPreparedPayloadMinimumWarmupPreservesBuildTime(t *testing.T) { + cfg := &clparams.BeaconChainConfig{ + SecondsPerSlot: 12, + IntervalsPerSlot: 3, + } + + require.Equal(t, 2*time.Second, preparedPayloadMinimumAge(cfg, clparams.ElectraVersion)) } func TestPreparedPayloadKeepsConsecutiveSlots(t *testing.T) { var p preparedPayload first := []byte{1, 1, 1, 1, 1, 1, 1, 1} second := []byte{2, 2, 2, 2, 2, 2, 2, 2} + now := time.Unix(100, 0) // Consecutive proposals: priming slot 11 must not evict slot 10, whose block may still be // in production. - p.set(10, first) - p.set(11, second) - require.True(t, p.matches(10, first)) - require.True(t, p.matches(11, second)) + p.set(10, first, now) + p.set(11, second, now) + require.True(t, p.matches(10, first, now, 0)) + require.True(t, p.matches(11, second, now, 0)) // Records old enough that they can no longer be produced are dropped, so the map is bounded. - p.set(10+preparedPayloadRetainSlots+1, []byte{3, 3, 3, 3, 3, 3, 3, 3}) - require.False(t, p.matches(10, first)) + p.set(10+preparedPayloadRetainSlots+1, []byte{3, 3, 3, 3, 3, 3, 3, 3}, now) + require.False(t, p.matches(10, first, now, 0)) } func TestPreparedPayloadCopiesTheID(t *testing.T) { var p preparedPayload id := []byte{1, 2, 3, 4, 5, 6, 7, 8} + now := time.Unix(100, 0) - p.set(10, id) + p.set(10, id, now) id[0] = 0xff // The caller's buffer must not be able to invalidate, or forge, a later match. - require.True(t, p.matches(10, []byte{1, 2, 3, 4, 5, 6, 7, 8})) - require.False(t, p.matches(10, id)) + require.True(t, p.matches(10, []byte{1, 2, 3, 4, 5, 6, 7, 8}, now, 0)) + require.False(t, p.matches(10, id, now, 0)) } func TestShouldPrepareAgainWhenTheHeadMoves(t *testing.T) { diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 772cdfa5e08..cc64dddbd66 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -45,24 +45,21 @@ var ( // priming the next slot cannot evict the record for a proposal that is still being produced. const preparedPayloadRetainSlots = 2 -// preparedPayload records the payload id the execution layer returned for a slot this node primed -// ahead of time. Block production compares the id its own forkchoice update returns against this -// record: an equal id means the execution layer recognised the request as a repeat and has been -// packing transactions since the prime, so the payload is worth taking early. Anything else — a -// reorg, a late block, a changed fee recipient, an execution layer that was busy — yields a -// different id and leaves production on its usual later schedule. -// Records are kept per slot: consecutive proposals would otherwise let the prime for the later slot -// evict the one production is about to look up. +type preparedPayloadRecord struct { + id []byte + primedAt time.Time +} + type preparedPayload struct { mu sync.Mutex - payloads map[uint64][]byte + payloads map[uint64]preparedPayloadRecord } -func (p *preparedPayload) set(slot uint64, payloadID []byte) { +func (p *preparedPayload) set(slot uint64, payloadID []byte, primedAt time.Time) { p.mu.Lock() defer p.mu.Unlock() if p.payloads == nil { - p.payloads = map[uint64][]byte{} + p.payloads = map[uint64]preparedPayloadRecord{} } // Slots this far back can no longer be produced, so dropping them bounds the map. for recorded := range p.payloads { @@ -70,13 +67,14 @@ func (p *preparedPayload) set(slot uint64, payloadID []byte) { delete(p.payloads, recorded) } } - p.payloads[slot] = bytes.Clone(payloadID) + p.payloads[slot] = preparedPayloadRecord{id: bytes.Clone(payloadID), primedAt: primedAt} } -func (p *preparedPayload) matches(slot uint64, payloadID []byte) bool { +func (p *preparedPayload) matches(slot uint64, payloadID []byte, now time.Time, minAge time.Duration) bool { p.mu.Lock() defer p.mu.Unlock() - return len(payloadID) > 0 && bytes.Equal(p.payloads[slot], payloadID) + record, ok := p.payloads[slot] + return ok && len(payloadID) > 0 && bytes.Equal(record.id, payloadID) && now.Sub(record.primedAt) >= minAge } // StartPayloadPreparation primes the execution layer for slots this node is due to propose, so the @@ -129,10 +127,7 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { } } -// shouldPrepare reports whether the target slot still needs priming. Priming once per slot is not -// enough: when the previous slot's block arrives late the head moves after we have already primed, -// and the execution layer is left warming a builder on a parent that is no longer the head. That is -// exactly the case where the proposal is most at risk, so prime again whenever the head changes. +// shouldPrepare requires a fresh builder after the target slot or its parent head changes. func shouldPrepare(targetSlot, primedSlot uint64, head, primedHead common.Hash) bool { return targetSlot != primedSlot || head != primedHead } @@ -146,11 +141,7 @@ func isExpectedPreparationSkip(err error) bool { errors.Is(err, synced_data.ErrNotSynced) } -// preparePayloadFor sends the forkchoice update for targetSlot ahead of the slot itself. It returns -// an error, rather than logging loudly, whenever there is simply nothing to do — the node is -// syncing, the slot belongs to someone else, or the validator client has not registered a fee -// recipient yet — because block production falls back to building inside the slot in every one of -// those cases. +// preparePayloadFor sends the forkchoice update for targetSlot ahead of the slot itself. func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) (common.Hash, error) { var ( baseBlockRoot common.Hash @@ -207,18 +198,12 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( return common.Hash{}, errNoPayloadID } - a.preparedPayload.set(targetSlot, payloadID) + a.preparedPayload.set(targetSlot, payloadID, time.Now()) log.Info("PayloadPreparation: primed execution layer", "slot", targetSlot, "proposer", proposerIndex, "head", baseBlockRoot) return baseBlockRoot, nil } -// 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 -// nothing else — production simply builds inside the slot as before — but it is silent, so -// PayloadPreparation logs whether the primed id was still valid at production time. +// preparedForkChoiceInputs mirrors the pre-Gloas production inputs so the execution layer can reuse the builder. func (a *ApiHandler) preparedForkChoiceInputs( baseState *state.CachingBeaconState, baseBlockRoot common.Hash, diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index e77019a08c2..6fca1ce5919 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -89,7 +89,7 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim } func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, error) { - b.interrupt.Store(true) + b.Cancel() select { case <-ctx.Done(): @@ -102,6 +102,10 @@ func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, erro return b.result, b.err } +func (b *BlockBuilder) Cancel() { + b.interrupt.Store(true) +} + func (b *BlockBuilder) Block() *types.Block { b.mu.Lock() defer b.mu.Unlock() diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index ef1d94cddaf..427a4dd0e79 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -65,7 +65,16 @@ func (e *ExecModule) evictOldBuilders() { // remove old builders so that at most MaxBuilders - 1 remain for i := 0; i <= len(e.builders)-engine_helpers.MaxBuilders; i++ { - delete(e.builders, ids[i]) + id := ids[i] + if old := e.builders[id]; old != nil { + old.Cancel() + } + delete(e.builders, id) + for timestamp, builderID := range e.buildersByTimestamp { + if builderID == id { + delete(e.buildersByTimestamp, timestamp) + } + } } } @@ -87,6 +96,11 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete return AssembleBlockResult{PayloadID: e.lastParameters.PayloadId}, nil } } + if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { + if previous := e.builders[previousID]; previous != nil { + previous.Cancel() + } + } // Initiate payload building e.evictOldBuilders() @@ -96,6 +110,10 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete e.lastParameters = params e.builders[e.nextPayloadId] = builder.NewBlockBuilder(e.builderFunc, params, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())) + if e.buildersByTimestamp == nil { + e.buildersByTimestamp = make(map[uint64]uint64) + } + e.buildersByTimestamp[params.Timestamp] = e.nextPayloadId e.logger.Info("[ForkChoiceUpdated] BlockBuilder added", "payload", e.nextPayloadId) return AssembleBlockResult{PayloadID: e.nextPayloadId}, nil diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index b48479af2e6..d8c4b3b79c7 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -17,13 +17,102 @@ package execmodule import ( + "context" + "errors" "math" + "sync/atomic" "testing" "time" + "golang.org/x/sync/semaphore" + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/common" + "github.com/erigontech/erigon/common/log/v3" + "github.com/erigontech/erigon/execution/builder" + "github.com/erigontech/erigon/execution/chain" + "github.com/erigontech/erigon/execution/engineapi/engine_helpers" + "github.com/erigontech/erigon/execution/types" ) +func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { + type runningBuilder struct { + id uint64 + interrupt *atomic.Bool + } + started := make(chan runningBuilder, 4) + module := &ExecModule{ + semaphore: semaphore.NewWeighted(1), + config: &chain.Config{}, + logger: log.Root(), + builders: map[uint64]*builder.BlockBuilder{}, + builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- runningBuilder{id: params.PayloadId, interrupt: interrupt} + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }, + } + t.Cleanup(func() { + for _, blockBuilder := range module.builders { + if blockBuilder != nil { + _, _ = blockBuilder.Stop(context.Background()) + } + } + }) + + waitStarted := func() runningBuilder { + t.Helper() + select { + case running := <-started: + return running + case <-time.After(time.Second): + t.Fatal("builder did not start") + return runningBuilder{} + } + } + assemble := func(timestamp uint64, parent common.Hash) (uint64, runningBuilder) { + t.Helper() + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: timestamp, ParentHash: parent}) + require.NoError(t, err) + require.False(t, result.Busy) + return result.PayloadID, waitStarted() + } + + firstID, first := assemble(100, common.Hash{0x01}) + adjacentID, adjacent := assemble(101, common.Hash{0x02}) + require.NotEqual(t, firstID, adjacentID) + require.False(t, first.interrupt.Load(), "a builder for another target timestamp must stay alive") + + secondID, second := assemble(100, common.Hash{0x03}) + require.NotEqual(t, firstID, secondID) + require.Eventually(t, first.interrupt.Load, time.Second, time.Millisecond) + require.False(t, adjacent.interrupt.Load(), "superseding timestamp 100 must not cancel timestamp 101") + require.False(t, second.interrupt.Load()) + + thirdID, third := assemble(100, common.Hash{0x04}) + require.NotEqual(t, secondID, thirdID) + require.Eventually(t, second.interrupt.Load, time.Second, time.Millisecond) + require.False(t, adjacent.interrupt.Load()) + require.False(t, third.interrupt.Load()) + + duplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x04}}) + require.NoError(t, err) + require.Equal(t, thirdID, duplicate.PayloadID) + require.False(t, third.interrupt.Load(), "a duplicate request must keep its builder alive") + + delete(module.builders, firstID) + for id := thirdID + 1; len(module.builders) < engine_helpers.MaxBuilders; id++ { + module.builders[id] = nil + } + module.evictOldBuilders() + require.Eventually(t, adjacent.interrupt.Load, time.Second, time.Millisecond) + require.NotContains(t, module.builders, adjacentID) + require.NotContains(t, module.buildersByTimestamp, uint64(101)) +} + func TestBuildDuration(t *testing.T) { const ethereum, gnosis = uint64(12), uint64(5) slotStart := time.Unix(1_700_000_000, 0) diff --git a/execution/execmodule/exec_module.go b/execution/execmodule/exec_module.go index edd986a4984..f3d1a7c7818 100644 --- a/execution/execmodule/exec_module.go +++ b/execution/execmodule/exec_module.go @@ -194,10 +194,11 @@ type ExecModule struct { logger log.Logger // Block building - nextPayloadId uint64 - lastParameters *builder.Parameters - builderFunc builder.BlockBuilderFunc - builders map[uint64]*builder.BlockBuilder + nextPayloadId uint64 + lastParameters *builder.Parameters + builderFunc builder.BlockBuilderFunc + builders map[uint64]*builder.BlockBuilder + buildersByTimestamp map[uint64]uint64 // Changes accumulator hook *stageloop.Hook @@ -267,6 +268,7 @@ func NewExecModule( forkValidator: forkValidator, pipelineExecutor: pipelineExecutor, builders: make(map[uint64]*builder.BlockBuilder), + buildersByTimestamp: make(map[uint64]uint64), builderFunc: builderFunc, config: config, semaphore: semaphore.NewWeighted(1), From 21c9644cc82f12863762faeb5a7abfc8f4b81808 Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 19:59:09 +0800 Subject: [PATCH 06/20] cl/beacon: gate payload preparation by fork and locality --- cl/beacon/handler/block_production.go | 60 +++++++++++++------ cl/beacon/handler/block_production_test.go | 67 +++++++++++++++++++++- cl/beacon/handler/payload_preparation.go | 61 ++++++++++++-------- execution/execmodule/block_building.go | 9 +-- 4 files changed, 147 insertions(+), 50 deletions(-) diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index edd66ae4f29..04f6678e049 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -208,12 +208,7 @@ func attestationDue(cfg *clparams.BeaconChainConfig, stateVersion clparams.State return time.Duration(cfg.AttestationDueMs(stateVersion.AfterOrEqual(clparams.GloasVersion))) * time.Millisecond } -// computeBlockBuilderWindow returns when to first poll for the assembled payload and when to stop. -// -// Without a primed builder the execution layer only starts packing when the block is requested, so -// polling stops a publication margin before the attestation deadline to give it most of the slot -// (see payloadPublicationDivisor). A primed builder has been packing since before the slot, so the -// payload can be taken early instead, leaving the rest of the margin for signing and gossip. +// computeBlockBuilderWindow uses the earlier collection window only for a sufficiently warmed builder. func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion, prepared bool) blockBuilderWindow { due := attestationDue(cfg, stateVersion) grabBy := slotStart.Add(due - due/payloadPublicationDivisor) @@ -239,6 +234,33 @@ func preparedPayloadMinimumAge(cfg *clparams.BeaconChainConfig, stateVersion clp return max(due-2*due/payloadPublicationDivisor, 0) } +func payloadAttributesForVersion( + version clparams.StateVersion, + timestamp hexutil.Uint64, + prevRandao common.Hash, + feeRecipient common.Address, + withdrawals []*types.Withdrawal, + parentRoot *common.Hash, + slotNumber, targetGasLimit *hexutil.Uint64, +) *engine_types.PayloadAttributes { + attrs := &engine_types.PayloadAttributes{ + Timestamp: timestamp, + PrevRandao: prevRandao, + SuggestedFeeRecipient: feeRecipient, + } + if version.AfterOrEqual(clparams.CapellaVersion) { + attrs.Withdrawals = withdrawals + } + if version.AfterOrEqual(clparams.DenebVersion) { + attrs.ParentBeaconBlockRoot = parentRoot + } + if version.AfterOrEqual(clparams.GloasVersion) { + attrs.SlotNumber = slotNumber + attrs.TargetGasLimit = targetGasLimit + } + return attrs +} + func shouldRetryGetPayload(now, deadline time.Time) bool { return now.Before(deadline) } @@ -1045,18 +1067,21 @@ func (a *ApiHandler) produceBeaconBody( } } - attrs := &engine_types.PayloadAttributes{ - Timestamp: hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), - PrevRandao: random, - SuggestedFeeRecipient: feeRecipient, - Withdrawals: withdrawals, - ParentBeaconBlockRoot: (*common.Hash)(&blockRoot), - } + var slotNumber *hexutil.Uint64 if stateVersion.AfterOrEqual(clparams.GloasVersion) { sn := hexutil.Uint64(targetSlot) - attrs.SlotNumber = &sn - attrs.TargetGasLimit = targetGasLimit + slotNumber = &sn } + attrs := payloadAttributesForVersion( + stateVersion, + hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), + random, + feeRecipient, + withdrawals, + (*common.Hash)(&blockRoot), + slotNumber, + targetGasLimit, + ) builderStartedAt := time.Now() idBytes, err := a.engine.ForkChoiceUpdate( ctx, @@ -1075,8 +1100,9 @@ func (a *ApiHandler) produceBeaconBody( return } slotStart := a.ethClock.GetSlotTime(targetSlot) - // Early collection requires both the same builder and enough pre-slot packing time. - prepared := a.preparedPayload.matches( + prepared := canUsePreparedPayload( + &a.preparedPayload, + a.engine.SupportInsertion(), targetSlot, idBytes, time.Now(), diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index b1734beee46..062e05b126c 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -740,8 +740,7 @@ func TestBlockBuilderWindowTakesPreparedPayloadEarly(t *testing.T) { } slotStart := time.Unix(100, 0) - // 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. + // A primed payload is taken one quarter of the attestation deadline into the slot. prepared := computeBlockBuilderWindow(slotStart, slotStart, cfg, clparams.ElectraVersion, true) require.Equal(t, slotStart.Add(time.Second), prepared.pollUntil) @@ -791,6 +790,70 @@ func TestPreparedPayloadMinimumWarmupPreservesBuildTime(t *testing.T) { require.Equal(t, 2*time.Second, preparedPayloadMinimumAge(cfg, clparams.ElectraVersion)) } +func TestPreparedPayloadRequiresBuilderContinuity(t *testing.T) { + var p preparedPayload + id := []byte{1, 2, 3, 4, 5, 6, 7, 8} + now := time.Unix(100, 0) + p.set(10, id, now.Add(-3*time.Second)) + + require.True(t, canUsePreparedPayload(&p, true, 10, id, now, 2*time.Second)) + require.False(t, canUsePreparedPayload(&p, false, 10, id, now, 2*time.Second)) +} + +func TestShouldPreparePayloadVersion(t *testing.T) { + for _, tc := range []struct { + version clparams.StateVersion + want bool + }{ + {clparams.Phase0Version, false}, + {clparams.AltairVersion, false}, + {clparams.BellatrixVersion, true}, + {clparams.CapellaVersion, true}, + {clparams.DenebVersion, true}, + {clparams.FuluVersion, true}, + {clparams.GloasVersion, false}, + } { + require.Equal(t, tc.want, shouldPreparePayloadVersion(tc.version), tc.version.String()) + } +} + +func TestPayloadAttributesForkFields(t *testing.T) { + root := common.Hash{0xaa} + slot := hexutil.Uint64(10) + gasLimit := hexutil.Uint64(30_000_000) + withdrawals := []*types.Withdrawal{{Index: 1}} + + for _, tc := range []struct { + version clparams.StateVersion + wantWithdrawals bool + wantParentRoot bool + wantGloasFields bool + }{ + {clparams.BellatrixVersion, false, false, false}, + {clparams.CapellaVersion, true, false, false}, + {clparams.DenebVersion, true, true, false}, + {clparams.FuluVersion, true, true, false}, + {clparams.GloasVersion, true, true, true}, + } { + t.Run(tc.version.String(), func(t *testing.T) { + attrs := payloadAttributesForVersion( + tc.version, + 1, + common.Hash{0xbb}, + common.Address{0xcc}, + withdrawals, + &root, + &slot, + &gasLimit, + ) + require.Equal(t, tc.wantWithdrawals, attrs.Withdrawals != nil) + require.Equal(t, tc.wantParentRoot, attrs.ParentBeaconBlockRoot != nil) + require.Equal(t, tc.wantGloasFields, attrs.SlotNumber != nil) + require.Equal(t, tc.wantGloasFields, attrs.TargetGasLimit != nil) + }) + } +} + func TestPreparedPayloadKeepsConsecutiveSlots(t *testing.T) { var p preparedPayload first := []byte{1, 1, 1, 1, 1, 1, 1, 1} diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index cc64dddbd66..12b3558c78a 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -77,6 +77,10 @@ func (p *preparedPayload) matches(slot uint64, payloadID []byte, now time.Time, return ok && len(payloadID) > 0 && bytes.Equal(record.id, payloadID) && now.Sub(record.primedAt) >= minAge } +func canUsePreparedPayload(p *preparedPayload, builderContinuity bool, slot uint64, payloadID []byte, now time.Time, minAge time.Duration) bool { + return builderContinuity && p.matches(slot, payloadID, now, minAge) +} + // StartPayloadPreparation primes the execution layer for slots this node is due to propose, so the // payload is already packed when the validator client asks for a block instead of being built from // scratch inside the proposal slot. @@ -110,8 +114,8 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if !shouldPrepare(targetSlot, primedSlot, a.syncedData.HeadRoot(), primedHead) { continue } - if a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch).AfterOrEqual(clparams.GloasVersion) { - // [Gloas:EIP7732] builders gossip bids instead; the engine is not primed this way. + stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) + if !shouldPreparePayloadVersion(stateVersion) { continue } head, err := a.preparePayloadFor(ctx, targetSlot) @@ -132,6 +136,10 @@ func shouldPrepare(targetSlot, primedSlot uint64, head, primedHead common.Hash) return targetSlot != primedSlot || head != primedHead } +func shouldPreparePayloadVersion(version clparams.StateVersion) bool { + return version.AfterOrEqual(clparams.BellatrixVersion) && version.Before(clparams.GloasVersion) +} + // isExpectedPreparationSkip reports whether there was simply nothing to prepare, as opposed to a // failure worth reporting. func isExpectedPreparationSkip(err error) bool { @@ -185,11 +193,11 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( return common.Hash{}, err } - head, safeHash, finalizedHash, attrs, err := a.preparedForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) + stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) + head, safeHash, finalizedHash, attrs, err := a.preparedForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient, stateVersion) if err != nil { return common.Hash{}, err } - stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalizedHash, safeHash, head, attrs, stateVersion) if err != nil { return common.Hash{}, err @@ -209,6 +217,7 @@ func (a *ApiHandler) preparedForkChoiceInputs( baseBlockRoot common.Hash, targetSlot uint64, feeRecipient common.Address, + stateVersion clparams.StateVersion, ) (head, safeHash, finalizedHash common.Hash, attrs *engine_types.PayloadAttributes, err error) { head = baseState.LatestExecutionPayloadHeader().BlockHash @@ -222,26 +231,32 @@ func (a *ApiHandler) preparedForkChoiceInputs( } epoch := targetSlot / a.beaconChainCfg.SlotsPerEpoch - clWithdrawals, err := state.GetExpectedWithdrawals(baseState, epoch) - if err != nil { - return head, safeHash, finalizedHash, nil, err - } - withdrawals := make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals)) - for _, w := range clWithdrawals.Withdrawals { - withdrawals = append(withdrawals, &types.Withdrawal{ - Index: w.Index, - Amount: w.Amount, - Validator: w.Validator, - Address: w.Address, - }) + var withdrawals []*types.Withdrawal + if stateVersion.AfterOrEqual(clparams.CapellaVersion) { + clWithdrawals, err := state.GetExpectedWithdrawals(baseState, epoch) + if err != nil { + return head, safeHash, finalizedHash, nil, err + } + withdrawals = make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals)) + for _, w := range clWithdrawals.Withdrawals { + withdrawals = append(withdrawals, &types.Withdrawal{ + Index: w.Index, + Amount: w.Amount, + Validator: w.Validator, + Address: w.Address, + }) + } } - attrs = &engine_types.PayloadAttributes{ - Timestamp: hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), - PrevRandao: baseState.GetRandaoMixes(epoch), - SuggestedFeeRecipient: feeRecipient, - Withdrawals: withdrawals, - ParentBeaconBlockRoot: &baseBlockRoot, - } + attrs = payloadAttributesForVersion( + stateVersion, + hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), + baseState.GetRandaoMixes(epoch), + feeRecipient, + withdrawals, + &baseBlockRoot, + nil, + nil, + ) return head, safeHash, finalizedHash, attrs, nil } diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 427a4dd0e79..f144fc6c23f 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -40,14 +40,7 @@ func (e *ExecModule) checkWithdrawalsPresence(time uint64, withdrawals []*types. return nil } -// buildDuration returns how long a payload builder may run before it stops itself. -// -// A consensus layer may send payload attributes well ahead of the slot the payload is for and then -// call getPayload against the cached payload id, without sending fresh attributes. A builder that -// stops before that slot hands back a payload missing every transaction that arrived in between, so -// build until shortly into the target slot, derived from the payload's own timestamp. The floor -// leaves a late request no worse off than a fixed budget; the cap stops an implausible timestamp -// from pinning a builder and its resources. +// buildDuration spans the target slot for early requests while bounding late and implausibly future requests. func buildDuration(payloadTimestamp uint64, now time.Time, secondsPerSlot uint64) time.Duration { slot := time.Duration(secondsPerSlot) * time.Second // Reject beyond the cap horizon before converting: a large enough timestamp overflows From e4e8af2b3e2710e5c0a27d74a307f4d67b6f5aeb Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 20:04:29 +0800 Subject: [PATCH 07/20] cl/beacon: skip payload preparation for remote engines --- cl/beacon/handler/block_production_test.go | 14 ++++++++++++++ cl/beacon/handler/payload_preparation.go | 3 +++ 2 files changed, 17 insertions(+) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 062e05b126c..60b0902cf61 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -800,6 +800,20 @@ func TestPreparedPayloadRequiresBuilderContinuity(t *testing.T) { require.False(t, canUsePreparedPayload(&p, false, 10, id, now, 2*time.Second)) } +func TestStartPayloadPreparationSkipsRemoteEngine(t *testing.T) { + ctrl := gomock.NewController(t) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().SupportInsertion().Return(false) + handler := &ApiHandler{ + engine: engine, + beaconChainCfg: &clparams.BeaconChainConfig{SecondsPerSlot: 12}, + } + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + handler.StartPayloadPreparation(ctx) +} + func TestShouldPreparePayloadVersion(t *testing.T) { for _, tc := range []struct { version clparams.StateVersion diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 12b3558c78a..6b5528da86f 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -85,6 +85,9 @@ func canUsePreparedPayload(p *preparedPayload, builderContinuity bool, slot uint // payload is already packed when the validator client asks for a block instead of being built from // scratch inside the proposal slot. func (a *ApiHandler) StartPayloadPreparation(ctx context.Context) { + if !a.engine.SupportInsertion() { + return + } go a.preparePayloadLoop(ctx) } From 0ebcb744930b7d99f2804b1823456ab75bc687d9 Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 20:08:27 +0800 Subject: [PATCH 08/20] cl/beacon: handle absent execution engine --- cl/beacon/handler/block_production_test.go | 21 +++++++++++++++++++++ cl/beacon/handler/payload_preparation.go | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 60b0902cf61..79baecec27f 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -814,6 +814,27 @@ func TestStartPayloadPreparationSkipsRemoteEngine(t *testing.T) { handler.StartPayloadPreparation(ctx) } +func TestStartPayloadPreparationSkipsNilEngine(t *testing.T) { + handler := &ApiHandler{} + require.NotPanics(t, func() { + handler.StartPayloadPreparation(t.Context()) + }) +} + +func TestStartPayloadPreparationStartsLocalEngine(t *testing.T) { + ctrl := gomock.NewController(t) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().SupportInsertion().Return(true) + handler := &ApiHandler{ + engine: engine, + beaconChainCfg: &clparams.BeaconChainConfig{SecondsPerSlot: 12}, + } + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + handler.StartPayloadPreparation(ctx) +} + func TestShouldPreparePayloadVersion(t *testing.T) { for _, tc := range []struct { version clparams.StateVersion diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 6b5528da86f..3670c9e6b61 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -85,7 +85,7 @@ func canUsePreparedPayload(p *preparedPayload, builderContinuity bool, slot uint // payload is already packed when the validator client asks for a block instead of being built from // scratch inside the proposal slot. func (a *ApiHandler) StartPayloadPreparation(ctx context.Context) { - if !a.engine.SupportInsertion() { + if a.engine == nil || !a.engine.SupportInsertion() { return } go a.preparePayloadLoop(ctx) From 818d4ce38484ddf2f41e4fb0e0dbece248cea1ea Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 21:01:53 +0800 Subject: [PATCH 09/20] execution: preserve exact builders by timestamp --- execution/execmodule/block_building.go | 53 +++++++++-- .../block_building_internal_test.go | 87 +++++++++++++++++++ execution/execmodule/exec_module.go | 3 +- 3 files changed, 133 insertions(+), 10 deletions(-) diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index f144fc6c23f..3d36978e5bb 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -17,6 +17,7 @@ package execmodule import ( + "bytes" "context" "reflect" "time" @@ -53,6 +54,36 @@ func buildDuration(payloadTimestamp uint64, now time.Time, secondsPerSlot uint64 return min(max(d, slot/4), 2*slot) } +func cloneBuilderParameters(params *builder.Parameters) *builder.Parameters { + if params == nil { + return nil + } + cloned := *params + cloned.ExtraData = bytes.Clone(params.ExtraData) + if params.Withdrawals != nil { + cloned.Withdrawals = make([]*types.Withdrawal, len(params.Withdrawals)) + for i, withdrawal := range params.Withdrawals { + if withdrawal != nil { + copy := *withdrawal + cloned.Withdrawals[i] = © + } + } + } + if params.ParentBeaconBlockRoot != nil { + copy := *params.ParentBeaconBlockRoot + cloned.ParentBeaconBlockRoot = © + } + if params.SlotNumber != nil { + copy := *params.SlotNumber + cloned.SlotNumber = © + } + if params.TargetGasLimit != nil { + copy := *params.TargetGasLimit + cloned.TargetGasLimit = © + } + return &cloned +} + func (e *ExecModule) evictOldBuilders() { ids := common.SortedKeys(e.builders) @@ -63,6 +94,7 @@ func (e *ExecModule) evictOldBuilders() { old.Cancel() } delete(e.builders, id) + delete(e.builderParameters, id) for timestamp, builderID := range e.buildersByTimestamp { if builderID == id { delete(e.buildersByTimestamp, timestamp) @@ -81,15 +113,14 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete return AssembleBlockResult{}, err } - // First check if we're already building a block with the requested parameters - if e.lastParameters != nil { - params.PayloadId = e.lastParameters.PayloadId - if reflect.DeepEqual(e.lastParameters, params) { + if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { + candidate := cloneBuilderParameters(params) + candidate.PayloadId = previousID + params.PayloadId = previousID + if reflect.DeepEqual(e.builderParameters[previousID], candidate) { e.logger.Info("[ForkChoiceUpdated] duplicate build request") - return AssembleBlockResult{PayloadID: e.lastParameters.PayloadId}, nil + return AssembleBlockResult{PayloadID: previousID}, nil } - } - if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { if previous := e.builders[previousID]; previous != nil { previous.Cancel() } @@ -100,13 +131,17 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete e.nextPayloadId++ params.PayloadId = e.nextPayloadId - e.lastParameters = params + ownedParams := cloneBuilderParameters(params) - e.builders[e.nextPayloadId] = builder.NewBlockBuilder(e.builderFunc, params, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())) + e.builders[e.nextPayloadId] = builder.NewBlockBuilder(e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())) if e.buildersByTimestamp == nil { e.buildersByTimestamp = make(map[uint64]uint64) } + if e.builderParameters == nil { + e.builderParameters = make(map[uint64]*builder.Parameters) + } e.buildersByTimestamp[params.Timestamp] = e.nextPayloadId + e.builderParameters[e.nextPayloadId] = ownedParams e.logger.Info("[ForkChoiceUpdated] BlockBuilder added", "payload", e.nextPayloadId) return AssembleBlockResult{PayloadID: e.nextPayloadId}, nil diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index d8c4b3b79c7..610bb3b4742 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -86,6 +86,12 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { require.NotEqual(t, firstID, adjacentID) require.False(t, first.interrupt.Load(), "a builder for another target timestamp must stay alive") + firstDuplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + require.Equal(t, firstID, firstDuplicate.PayloadID) + require.False(t, first.interrupt.Load(), "an exact earlier-timestamp builder must stay alive") + require.False(t, adjacent.interrupt.Load()) + secondID, second := assemble(100, common.Hash{0x03}) require.NotEqual(t, firstID, secondID) require.Eventually(t, first.interrupt.Load, time.Second, time.Millisecond) @@ -111,6 +117,87 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { require.Eventually(t, adjacent.interrupt.Load, time.Second, time.Millisecond) require.NotContains(t, module.builders, adjacentID) require.NotContains(t, module.buildersByTimestamp, uint64(101)) + require.NotContains(t, module.builderParameters, adjacentID) +} + +func TestAssembleBlockOwnsParameters(t *testing.T) { + type observedParameters struct { + parentRoot common.Hash + extraData byte + } + readParameters := make(chan struct{}) + observed := make(chan observedParameters, 1) + module := &ExecModule{ + semaphore: semaphore.NewWeighted(1), + config: &chain.Config{}, + logger: log.Root(), + builders: map[uint64]*builder.BlockBuilder{}, + builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + <-readParameters + observed <- observedParameters{parentRoot: *params.ParentBeaconBlockRoot, extraData: params.ExtraData[0]} + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }, + } + root := common.Hash{0xaa} + params := &builder.Parameters{ + Timestamp: 100, + ParentHash: common.Hash{0x01}, + ParentBeaconBlockRoot: &root, + ExtraData: []byte{0xbb}, + } + result, err := module.AssembleBlock(t.Context(), params) + require.NoError(t, err) + require.False(t, result.Busy) + + root[0] = 0xcc + params.ExtraData[0] = 0xdd + close(readParameters) + require.Equal(t, observedParameters{parentRoot: common.Hash{0xaa}, extraData: 0xbb}, <-observed) + + duplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{ + Timestamp: 100, + ParentHash: common.Hash{0x01}, + ParentBeaconBlockRoot: &common.Hash{0xaa}, + ExtraData: []byte{0xbb}, + }) + require.NoError(t, err) + require.Equal(t, result.PayloadID, duplicate.PayloadID) + _, _ = module.builders[result.PayloadID].Stop(context.Background()) +} + +func TestCloneBuilderParametersPreservesRepresentations(t *testing.T) { + require.Nil(t, cloneBuilderParameters(nil)) + + empty := cloneBuilderParameters(&builder.Parameters{Withdrawals: []*types.Withdrawal{}, ExtraData: []byte{}}) + require.NotNil(t, empty.Withdrawals) + require.NotNil(t, empty.ExtraData) + + root := common.Hash{0x01} + slot := uint64(2) + gasLimit := uint64(3) + params := &builder.Parameters{ + Withdrawals: []*types.Withdrawal{nil, {Index: 4}}, + ParentBeaconBlockRoot: &root, + SlotNumber: &slot, + TargetGasLimit: &gasLimit, + ExtraData: []byte{5}, + } + cloned := cloneBuilderParameters(params) + params.Withdrawals[1].Index = 40 + root[0] = 10 + slot = 20 + gasLimit = 30 + params.ExtraData[0] = 50 + + require.Nil(t, cloned.Withdrawals[0]) + require.Equal(t, uint64(4), cloned.Withdrawals[1].Index) + require.Equal(t, common.Hash{0x01}, *cloned.ParentBeaconBlockRoot) + require.Equal(t, uint64(2), *cloned.SlotNumber) + require.Equal(t, uint64(3), *cloned.TargetGasLimit) + require.Equal(t, byte(5), cloned.ExtraData[0]) } func TestBuildDuration(t *testing.T) { diff --git a/execution/execmodule/exec_module.go b/execution/execmodule/exec_module.go index f3d1a7c7818..8aeb6508f94 100644 --- a/execution/execmodule/exec_module.go +++ b/execution/execmodule/exec_module.go @@ -195,10 +195,10 @@ type ExecModule struct { logger log.Logger // Block building nextPayloadId uint64 - lastParameters *builder.Parameters builderFunc builder.BlockBuilderFunc builders map[uint64]*builder.BlockBuilder buildersByTimestamp map[uint64]uint64 + builderParameters map[uint64]*builder.Parameters // Changes accumulator hook *stageloop.Hook @@ -269,6 +269,7 @@ func NewExecModule( pipelineExecutor: pipelineExecutor, builders: make(map[uint64]*builder.BlockBuilder), buildersByTimestamp: make(map[uint64]uint64), + builderParameters: make(map[uint64]*builder.Parameters), builderFunc: builderFunc, config: config, semaphore: semaphore.NewWeighted(1), From 60d9b644568aa2cbe02859f1843596d3ffbf9550 Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 21:21:47 +0800 Subject: [PATCH 10/20] cl/beacon: cover payload preparation wiring --- cl/beacon/handler/block_production_test.go | 83 ++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 79baecec27f..756c827c81d 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -33,10 +33,13 @@ import ( "github.com/erigontech/erigon/cl/beacon/beaconhttp" builder_mock "github.com/erigontech/erigon/cl/beacon/builder/mock_services" + "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/clparams" "github.com/erigontech/erigon/cl/cltypes" "github.com/erigontech/erigon/cl/cltypes/solid" + "github.com/erigontech/erigon/cl/phase1/core/state" "github.com/erigontech/erigon/cl/phase1/execution_client" + "github.com/erigontech/erigon/cl/transition" "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/hexutil" "github.com/erigontech/erigon/common/log/v3" @@ -835,6 +838,86 @@ func TestStartPayloadPreparationStartsLocalEngine(t *testing.T) { handler.StartPayloadPreparation(ctx) } +func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, _, fcu, validatorParams := setupTestingHandler(t, clparams.CapellaVersion, log.Root(), true) + config := *handler.beaconChainCfg + targetEpoch := postState.Slot()/config.SlotsPerEpoch + 1 + targetSlot := targetEpoch * config.SlotsPerEpoch + config.DenebForkEpoch = targetEpoch + config.InitializeForkSchedule() + + headState := state.New(&config) + require.NoError(t, postState.CopyInto(headState)) + headState.SetFinalizedCheckpoint(solid.Checkpoint{Epoch: targetEpoch - 2, Root: common.Hash{0x31}}) + headState.SetCurrentJustifiedCheckpoint(solid.Checkpoint{Epoch: targetEpoch - 1, Root: common.Hash{0x32}}) + currentEpoch := headState.Slot() / config.SlotsPerEpoch + headState.SetRandaoMixAt(int(currentEpoch%config.EpochsPerHistoricalVector), common.Hash{0x51}) + headState.SetRandaoMixAt(int(targetEpoch%config.EpochsPerHistoricalVector), common.Hash{0x52}) + baseBlockRoot := common.Hash{0x41} + syncedData := synced_data.NewSyncedDataManager(&config, true) + require.NoError(t, syncedData.OnHeadStateWithBlockRoot(headState, baseBlockRoot)) + handler.beaconChainCfg = &config + handler.syncedData = syncedData + + proposerIndex, err := headState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + + feeRecipient := common.Address{0x11} + validatorParams.SetFeeRecipient(proposerIndex, feeRecipient) + + advancedState, err := headState.Copy() + require.NoError(t, err) + require.NoError(t, transition.DefaultMachine.ProcessSlots(advancedState, targetSlot)) + require.Equal(t, clparams.CapellaVersion, headState.Version()) + require.Equal(t, clparams.DenebVersion, advancedState.Version()) + require.NotEqual(t, headState.GetRandaoMixes(targetEpoch), advancedState.GetRandaoMixes(targetEpoch)) + finalizedRoot := advancedState.FinalizedCheckpoint().Root + justifiedRoot := advancedState.CurrentJustifiedCheckpoint().Root + require.NotEqual(t, finalizedRoot, justifiedRoot) + expectedFinalized := common.Hash{0x21} + expectedSafe := common.Hash{0x22} + fcu.Eth1Hashes[finalizedRoot] = expectedFinalized + fcu.Eth1Hashes[justifiedRoot] = expectedSafe + require.NotEqual(t, expectedFinalized, expectedSafe) + + version := handler.beaconChainCfg.GetCurrentStateVersion(targetSlot / handler.beaconChainCfg.SlotsPerEpoch) + require.Equal(t, clparams.DenebVersion, version) + expectedWithdrawals, err := state.GetExpectedWithdrawals(advancedState, targetSlot/handler.beaconChainCfg.SlotsPerEpoch) + require.NoError(t, err) + + payloadID := []byte{1, 2, 3, 4, 5, 6, 7, 8} + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, finalized, safe, head common.Hash, attrs *engine_types.PayloadAttributes, gotVersion clparams.StateVersion) ([]byte, error) { + require.Equal(t, expectedFinalized, finalized) + require.Equal(t, expectedSafe, safe) + require.Equal(t, advancedState.LatestExecutionPayloadHeader().BlockHash, head) + require.Equal(t, version, gotVersion) + require.Equal(t, hexutil.Uint64(state.ComputeTimestampAtSlot(advancedState, targetSlot)), attrs.Timestamp) + require.Equal(t, common.Hash(advancedState.GetRandaoMixes(targetSlot/handler.beaconChainCfg.SlotsPerEpoch)), attrs.PrevRandao) + require.Equal(t, feeRecipient, attrs.SuggestedFeeRecipient) + require.Equal(t, &baseBlockRoot, attrs.ParentBeaconBlockRoot) + require.Nil(t, attrs.SlotNumber) + require.Nil(t, attrs.TargetGasLimit) + require.NotNil(t, attrs.Withdrawals) + require.Len(t, attrs.Withdrawals, len(expectedWithdrawals.Withdrawals)) + for i, withdrawal := range expectedWithdrawals.Withdrawals { + require.Equal(t, withdrawal.Index, attrs.Withdrawals[i].Index) + require.Equal(t, withdrawal.Amount, attrs.Withdrawals[i].Amount) + require.Equal(t, withdrawal.Validator, attrs.Withdrawals[i].Validator) + require.Equal(t, withdrawal.Address, attrs.Withdrawals[i].Address) + } + return payloadID, nil + }) + handler.engine = engine + + primedHead, err := handler.preparePayloadFor(t.Context(), targetSlot) + require.NoError(t, err) + require.Equal(t, baseBlockRoot, primedHead) + require.True(t, handler.preparedPayload.matches(targetSlot, payloadID, time.Now(), 0)) +} + func TestShouldPreparePayloadVersion(t *testing.T) { for _, tc := range []struct { version clparams.StateVersion From 779487c4fd7f0fed1ce5cfa4bb4ea957e332e6b2 Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 23:32:12 +0800 Subject: [PATCH 11/20] cl/beacon: harden payload preparation lifecycle --- cl/beacon/handler/block_production.go | 72 +++----- cl/beacon/handler/block_production_test.go | 172 +++++++++++++++++- cl/beacon/handler/payload_preparation.go | 105 +++++++---- .../execution_client_direct.go | 45 ++++- .../execution_client_direct_test.go | 74 ++++++++ .../execmodule/chainreader/chain_reader.go | 4 +- 6 files changed, 378 insertions(+), 94 deletions(-) create mode 100644 cl/phase1/execution_client/execution_client_direct_test.go diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index 04f6678e049..cdddc6f476b 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -211,15 +211,14 @@ func attestationDue(cfg *clparams.BeaconChainConfig, stateVersion clparams.State // computeBlockBuilderWindow uses the earlier collection window only for a sufficiently warmed builder. func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion, prepared bool) blockBuilderWindow { due := attestationDue(cfg, stateVersion) - grabBy := slotStart.Add(due - due/payloadPublicationDivisor) + pollUntil := slotStart.Add(due - due/payloadPublicationDivisor) + firstGetAt := pollUntil.Add(-minPayloadPollingWindow) if prepared { - grabBy = slotStart.Add(due / payloadPublicationDivisor) + firstGetAt = slotStart.Add(due / payloadPublicationDivisor).Add(-minPayloadPollingWindow) } - firstGetAt := grabBy.Add(-minPayloadPollingWindow) if firstGetAt.Before(now) { firstGetAt = now } - pollUntil := grabBy if pollUntil.Before(firstGetAt) { pollUntil = firstGetAt } @@ -507,10 +506,6 @@ func (a *ApiHandler) GetEthV3ValidatorBlock( }); err != nil { return nil, err } - - if err != nil { - return nil, err - } if baseState == nil { return nil, beaconhttp.NewEndpointError( http.StatusNotFound, @@ -987,8 +982,8 @@ func (a *ApiHandler) produceBeaconBody( } } } - currEpoch := a.ethClock.GetCurrentEpoch() - random := baseState.GetRandaoMixes(currEpoch) + targetEpoch := targetSlot / a.beaconChainCfg.SlotsPerEpoch + random := baseState.GetRandaoMixes(targetEpoch) var executionPayload *cltypes.Eth1Block var executionValue uint64 @@ -1010,6 +1005,16 @@ func (a *ApiHandler) produceBeaconBody( }() retryTime := 10 * time.Millisecond feeRecipient, _ := a.validatorParams.GetFeeRecipient(proposerIndex) + fcuHead, fcuSafeHash, fcuFinalizedHash := head, safeHash, finalizedHash + var attrs *engine_types.PayloadAttributes + if stateVersion.Before(clparams.GloasVersion) { + var err error + fcuHead, fcuSafeHash, fcuFinalizedHash, attrs, err = a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient, stateVersion) + if err != nil { + log.Error("BlockProduction: build forkchoice inputs failed", "err", err) + return + } + } var withdrawals []*types.Withdrawal switch { case gloasWithdrawalsState != nil: @@ -1046,48 +1051,29 @@ func (a *ApiHandler) produceBeaconBody( }) } } - default: - // Pre-GLOAS: compute withdrawals normally - clWithdrawals, err := state.GetExpectedWithdrawals( - baseState, - targetSlot/a.beaconChainCfg.SlotsPerEpoch, - ) - if err != nil { - log.Error("BlockProduction: GetExpectedWithdrawals failed", "err", err) - return - } - withdrawals = make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals)) - for _, w := range clWithdrawals.Withdrawals { - withdrawals = append(withdrawals, &types.Withdrawal{ - Index: w.Index, - Amount: w.Amount, - Validator: w.Validator, - Address: w.Address, - }) - } } - var slotNumber *hexutil.Uint64 if stateVersion.AfterOrEqual(clparams.GloasVersion) { + var slotNumber *hexutil.Uint64 sn := hexutil.Uint64(targetSlot) slotNumber = &sn + attrs = payloadAttributesForVersion( + stateVersion, + hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), + random, + feeRecipient, + withdrawals, + (*common.Hash)(&blockRoot), + slotNumber, + targetGasLimit, + ) } - attrs := payloadAttributesForVersion( - stateVersion, - hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), - random, - feeRecipient, - withdrawals, - (*common.Hash)(&blockRoot), - slotNumber, - targetGasLimit, - ) builderStartedAt := time.Now() idBytes, err := a.engine.ForkChoiceUpdate( ctx, - finalizedHash, - safeHash, - head, + fcuFinalizedHash, + fcuSafeHash, + fcuHead, attrs, stateVersion, ) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 756c827c81d..57021a154b9 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -31,15 +31,18 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "github.com/erigontech/erigon/cl/beacon/beacon_router_configuration" "github.com/erigontech/erigon/cl/beacon/beaconhttp" builder_mock "github.com/erigontech/erigon/cl/beacon/builder/mock_services" "github.com/erigontech/erigon/cl/beacon/synced_data" + sync_mock_services "github.com/erigontech/erigon/cl/beacon/synced_data/mock_services" "github.com/erigontech/erigon/cl/clparams" "github.com/erigontech/erigon/cl/cltypes" "github.com/erigontech/erigon/cl/cltypes/solid" "github.com/erigontech/erigon/cl/phase1/core/state" "github.com/erigontech/erigon/cl/phase1/execution_client" "github.com/erigontech/erigon/cl/transition" + "github.com/erigontech/erigon/cl/utils/eth_clock" "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/hexutil" "github.com/erigontech/erigon/common/log/v3" @@ -745,13 +748,19 @@ func TestBlockBuilderWindowTakesPreparedPayloadEarly(t *testing.T) { // A primed payload is taken one quarter of the attestation deadline into the slot. prepared := computeBlockBuilderWindow(slotStart, slotStart, cfg, clparams.ElectraVersion, true) - require.Equal(t, slotStart.Add(time.Second), prepared.pollUntil) + require.Equal(t, slotStart.Add(time.Second).Add(-minPayloadPollingWindow), prepared.firstGetAt) + require.Equal(t, slotStart.Add(3*time.Second), prepared.pollUntil) // Without a primed builder nothing changes: the execution layer still needs most of the slot. unprepared := computeBlockBuilderWindow(slotStart, slotStart, cfg, clparams.ElectraVersion, false) + require.Equal(t, slotStart.Add(3*time.Second).Add(-minPayloadPollingWindow), unprepared.firstGetAt) require.Equal(t, slotStart.Add(3*time.Second), unprepared.pollUntil) - require.True(t, prepared.pollUntil.Before(unprepared.pollUntil)) + require.True(t, prepared.firstGetAt.Before(unprepared.firstGetAt)) + + late := computeBlockBuilderWindow(slotStart.Add(2*time.Second), slotStart, cfg, clparams.ElectraVersion, true) + require.Equal(t, slotStart.Add(2*time.Second), late.firstGetAt) + require.Equal(t, slotStart.Add(3*time.Second), late.pollUntil) } func TestPreparedPayloadMatchesOnlyTheSamePrime(t *testing.T) { @@ -809,6 +818,7 @@ func TestStartPayloadPreparationSkipsRemoteEngine(t *testing.T) { engine.EXPECT().SupportInsertion().Return(false) handler := &ApiHandler{ engine: engine, + routerCfg: &beacon_router_configuration.RouterConfiguration{Validator: true}, beaconChainCfg: &clparams.BeaconChainConfig{SecondsPerSlot: 12}, } ctx, cancel := context.WithCancel(t.Context()) @@ -817,6 +827,19 @@ func TestStartPayloadPreparationSkipsRemoteEngine(t *testing.T) { handler.StartPayloadPreparation(ctx) } +func TestStartPayloadPreparationSkipsWithoutValidatorAPI(t *testing.T) { + ctrl := gomock.NewController(t) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().SupportInsertion().Times(0) + handler := &ApiHandler{ + engine: engine, + routerCfg: &beacon_router_configuration.RouterConfiguration{Validator: false}, + beaconChainCfg: &clparams.BeaconChainConfig{SecondsPerSlot: 12}, + } + + handler.StartPayloadPreparation(t.Context()) +} + func TestStartPayloadPreparationSkipsNilEngine(t *testing.T) { handler := &ApiHandler{} require.NotPanics(t, func() { @@ -830,6 +853,7 @@ func TestStartPayloadPreparationStartsLocalEngine(t *testing.T) { engine.EXPECT().SupportInsertion().Return(true) handler := &ApiHandler{ engine: engine, + routerCfg: &beacon_router_configuration.RouterConfiguration{Validator: true}, beaconChainCfg: &clparams.BeaconChainConfig{SecondsPerSlot: 12}, } ctx, cancel := context.WithCancel(t.Context()) @@ -838,6 +862,35 @@ func TestStartPayloadPreparationStartsLocalEngine(t *testing.T) { handler.StartPayloadPreparation(ctx) } +func TestPreparePayloadLoopRunsImmediatelyWithSlotDeadline(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, _, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), true) + targetSlot := postState.Slot() + 1 + proposerIndex, err := postState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) + + slotStart := time.Now().Add(time.Minute) + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetCurrentSlot().Return(targetSlot - 1) + clock.EXPECT().GetSlotTime(targetSlot).Return(slotStart) + handler.ethClock = clock + + ctx, cancel := context.WithCancel(t.Context()) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(callCtx context.Context, _, _, _ common.Hash, _ *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { + deadline, ok := callCtx.Deadline() + require.True(t, ok) + require.Equal(t, slotStart, deadline) + cancel() + return []byte{1, 2, 3, 4, 5, 6, 7, 8}, nil + }) + handler.engine = engine + + handler.preparePayloadLoop(ctx) +} + func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { ctrl := gomock.NewController(t) _, _, _, _, postState, handler, _, _, fcu, validatorParams := setupTestingHandler(t, clparams.CapellaVersion, log.Root(), true) @@ -918,6 +971,121 @@ func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { require.True(t, handler.preparedPayload.matches(targetSlot, payloadID, time.Now(), 0)) } +func TestPreparePayloadForRejectsChangedHeadBeforeForkChoiceUpdate(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, syncedData, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), false) + targetSlot := postState.Slot() + 1 + proposerIndex, err := postState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) + + baseBlockRoot := common.Hash{0x41} + changedBlockRoot := common.Hash{0x42} + syncedDataMock := syncedData.(*sync_mock_services.MockSyncedData) + gomock.InOrder( + syncedDataMock.EXPECT().ViewHeadStateWithIdentity(gomock.Any()). + DoAndReturn(func(view synced_data.ViewHeadStateWithIdentityFn) error { + return view(postState, baseBlockRoot, postState.Slot()) + }), + syncedDataMock.EXPECT().HeadRoot().Return(changedBlockRoot), + ) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + handler.engine = engine + + _, err = handler.preparePayloadFor(t.Context(), targetSlot) + require.ErrorIs(t, err, errPreparationHeadChanged) + require.False(t, handler.preparedPayload.matches(targetSlot, []byte{1}, time.Now(), 0)) +} + +func TestPreparePayloadForUsesPostEpochProposer(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, syncedData, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), false) + config := *handler.beaconChainCfg + targetEpoch := postState.Slot()/config.SlotsPerEpoch + 1 + targetSlot := targetEpoch * config.SlotsPerEpoch + config.FuluForkEpoch = targetEpoch + 10 + config.GloasForkEpoch = targetEpoch + 11 + config.InitializeForkSchedule() + handler.beaconChainCfg = &config + + headState := state.New(&config) + require.NoError(t, postState.CopyInto(headState)) + headState.SetSlot(targetSlot - 1) + for i := 0; i < headState.ValidatorLength(); i += 2 { + headState.SetEffectiveBalanceForValidatorAtIndex(i, 0) + require.NoError(t, headState.SetValidatorBalance(i, config.MaxEffectiveBalanceElectra)) + } + + mixPosition := (targetEpoch + config.EpochsPerHistoricalVector - config.MinSeedLookahead - 1) % config.EpochsPerHistoricalVector + var oldProposer, newProposer uint64 + found := false + for nonce := byte(0); ; nonce++ { + headState.SetRandaoMixAt(int(mixPosition), common.Hash{nonce}) + var err error + oldProposer, err = headState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + + advanced, err := headState.Copy() + require.NoError(t, err) + require.NoError(t, transition.DefaultMachine.ProcessSlots(advanced, targetSlot)) + newProposer, err = advanced.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + if oldProposer != newProposer { + found = true + break + } + if nonce == 255 { + break + } + } + require.True(t, found, "fixture must expose a proposer change across epoch processing") + validatorParams.SetFeeRecipient(newProposer, common.Address{0x11}) + + baseBlockRoot := common.Hash{0x41} + syncedDataMock := syncedData.(*sync_mock_services.MockSyncedData) + syncedDataMock.EXPECT().ViewHeadStateWithIdentity(gomock.Any()). + DoAndReturn(func(view synced_data.ViewHeadStateWithIdentityFn) error { + return view(headState, baseBlockRoot, headState.Slot()) + }) + syncedDataMock.EXPECT().HeadRoot().Return(baseBlockRoot) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, _, _, _ common.Hash, attrs *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { + require.Equal(t, common.Address{0x11}, attrs.SuggestedFeeRecipient) + return []byte{1, 2, 3, 4, 5, 6, 7, 8}, nil + }) + handler.engine = engine + + _, err := handler.preparePayloadFor(t.Context(), targetSlot) + require.NoError(t, err) +} + +func TestProduceBeaconBodyTakesRootAndStateFromOneView(t *testing.T) { + _, _, _, _, postState, _, _, syncedData, _, _ := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), false) + expectedRoot := common.Hash{0x41} + syncedDataMock := syncedData.(*sync_mock_services.MockSyncedData) + syncedDataMock.EXPECT().ViewHeadStateWithIdentity(gomock.Any()). + DoAndReturn(func(view synced_data.ViewHeadStateWithIdentityFn) error { + return view(postState, expectedRoot, postState.Slot()) + }) + + var ( + gotRoot common.Hash + copied *state.CachingBeaconState + ) + require.NoError(t, syncedData.ViewHeadStateWithIdentity( + func(headState *state.CachingBeaconState, root common.Hash, _ uint64) error { + var err error + gotRoot = root + copied, err = headState.Copy() + return err + })) + require.Equal(t, expectedRoot, gotRoot) + require.NotSame(t, postState, copied) + require.Equal(t, postState.Slot(), copied.Slot()) +} + func TestShouldPreparePayloadVersion(t *testing.T) { for _, tc := range []struct { version clparams.StateVersion diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 3670c9e6b61..5606b1f9c60 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -35,10 +35,11 @@ import ( ) var ( - errNodeSyncing = errors.New("node is syncing") - errNotOurProposal = errors.New("next slot is not proposed by a registered validator") - errNoPayloadID = errors.New("execution layer returned no payload id") - errHeadTooFarBack = errors.New("head state is too far behind the slot to prepare") + errNodeSyncing = errors.New("node is syncing") + errNotOurProposal = errors.New("next slot is not proposed by a registered validator") + errNoPayloadID = errors.New("execution layer returned no payload id") + errHeadTooFarBack = errors.New("head state is too far behind the slot to prepare") + errPreparationHeadChanged = errors.New("head changed while preparing payload") ) // preparedPayloadRetainSlots keeps a primed record alive past the slot it was primed for, so @@ -85,7 +86,7 @@ func canUsePreparedPayload(p *preparedPayload, builderContinuity bool, slot uint // payload is already packed when the validator client asks for a block instead of being built from // scratch inside the proposal slot. func (a *ApiHandler) StartPayloadPreparation(ctx context.Context) { - if a.engine == nil || !a.engine.SupportInsertion() { + if a.routerCfg == nil || !a.routerCfg.Validator || a.engine == nil || !a.engine.SupportInsertion() { return } go a.preparePayloadLoop(ctx) @@ -103,30 +104,43 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { defer ticker.Stop() var ( - primedSlot uint64 - primedHead common.Hash + primedSlot uint64 + primedHead common.Hash + lastFailureLog time.Time ) - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: + for immediate := true; ; immediate = false { + if immediate { + select { + case <-ctx.Done(): + return + default: + } + } else { + select { + case <-ctx.Done(): + return + case <-ticker.C: + } } targetSlot := a.ethClock.GetCurrentSlot() + 1 + stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) + if stateVersion.AfterOrEqual(clparams.GloasVersion) { + return + } if !shouldPrepare(targetSlot, primedSlot, a.syncedData.HeadRoot(), primedHead) { continue } - stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) if !shouldPreparePayloadVersion(stateVersion) { continue } - head, err := a.preparePayloadFor(ctx, targetSlot) + prepareCtx, cancel := context.WithDeadline(ctx, a.ethClock.GetSlotTime(targetSlot)) + head, err := a.preparePayloadFor(prepareCtx, targetSlot) + cancel() 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) + if !isExpectedPreparationSkip(err) && time.Since(lastFailureLog) >= time.Minute { + log.Warn("PayloadPreparation: failed", "slot", targetSlot, "err", err) + lastFailureLog = time.Now() } continue } @@ -148,26 +162,26 @@ func shouldPreparePayloadVersion(version clparams.StateVersion) bool { func isExpectedPreparationSkip(err error) bool { return errors.Is(err, errNotOurProposal) || errors.Is(err, errNodeSyncing) || + errors.Is(err, errNoPayloadID) || errors.Is(err, errHeadTooFarBack) || + errors.Is(err, errPreparationHeadChanged) || errors.Is(err, synced_data.ErrNotSynced) } // preparePayloadFor sends the forkchoice update for targetSlot ahead of the slot itself. func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) (common.Hash, error) { var ( - baseBlockRoot common.Hash - proposerIndex uint64 - feeRecipient common.Address - baseState *state.CachingBeaconState + baseBlockRoot common.Hash + proposerIndex uint64 + feeRecipient common.Address + baseState *state.CachingBeaconState + lookupAfterAdvance bool ) // Root, proposer and state all come from one view of the head. Reading them separately would // let a head update in between pair a parent beacon block root with a different state, priming // a builder that production can never match. - if err := a.syncedData.ViewHeadState(func(headState *state.CachingBeaconState) error { - baseBlockRoot = a.syncedData.HeadRoot() - if baseBlockRoot == (common.Hash{}) { - return errNodeSyncing - } + if err := a.syncedData.ViewHeadStateWithIdentity(func(headState *state.CachingBeaconState, root common.Hash, _ uint64) error { + baseBlockRoot = root // Beyond the proposer lookahead the index has to be reshuffled from the seed, which is far // too costly to repeat every tick on a large validator set. slotsPerEpoch := a.beaconChainCfg.SlotsPerEpoch @@ -175,16 +189,16 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( return errHeadTooFarBack } + lookupAfterAdvance = targetSlot/slotsPerEpoch > headState.Slot()/slotsPerEpoch var err error - if proposerIndex, err = headState.GetBeaconProposerIndexForSlot(targetSlot); err != nil { - return err - } - // Only our own proposals are worth priming, and a fee recipient we do not yet know would - // build a payload that block production could not reuse anyway. Checked before the state - // copy, which is the expensive part. - var ok bool - if feeRecipient, ok = a.validatorParams.GetFeeRecipient(proposerIndex); !ok { - return errNotOurProposal + if !lookupAfterAdvance { + if proposerIndex, err = headState.GetBeaconProposerIndexForSlot(targetSlot); err != nil { + return err + } + var ok bool + if feeRecipient, ok = a.validatorParams.GetFeeRecipient(proposerIndex); !ok { + return errNotOurProposal + } } baseState, err = headState.Copy() return err @@ -195,12 +209,25 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( if err := transition.DefaultMachine.ProcessSlots(baseState, targetSlot); err != nil { return common.Hash{}, err } + if lookupAfterAdvance { + var err error + if proposerIndex, err = baseState.GetBeaconProposerIndexForSlot(targetSlot); err != nil { + return common.Hash{}, err + } + var ok bool + if feeRecipient, ok = a.validatorParams.GetFeeRecipient(proposerIndex); !ok { + return common.Hash{}, errNotOurProposal + } + } stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) - head, safeHash, finalizedHash, attrs, err := a.preparedForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient, stateVersion) + head, safeHash, finalizedHash, attrs, err := a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient, stateVersion) if err != nil { return common.Hash{}, err } + if a.syncedData.HeadRoot() != baseBlockRoot { + return common.Hash{}, errPreparationHeadChanged + } payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalizedHash, safeHash, head, attrs, stateVersion) if err != nil { return common.Hash{}, err @@ -214,8 +241,8 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( return baseBlockRoot, nil } -// preparedForkChoiceInputs mirrors the pre-Gloas production inputs so the execution layer can reuse the builder. -func (a *ApiHandler) preparedForkChoiceInputs( +// preGloasForkChoiceInputs builds the shared preparation and production inputs. +func (a *ApiHandler) preGloasForkChoiceInputs( baseState *state.CachingBeaconState, baseBlockRoot common.Hash, targetSlot uint64, diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index ab5e5a9290f..4230493e910 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -144,14 +144,9 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized // fork choice commits). This is common in single-process dev mode // where the CL and EL share the same process. idBytes := make([]byte, 8) - var id uint64 - for range 30 { - id, err = cc.chainRW.AssembleBlock(head, attr) - if err == nil { - break - } - time.Sleep(200 * time.Millisecond) - } + id, err := retryAssembleBlock(ctx, 30, 200*time.Millisecond, func(ctx context.Context) (uint64, error) { + return cc.chainRW.AssembleBlock(ctx, head, attr) + }) if err != nil { return nil, err } @@ -159,6 +154,40 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized return idBytes, nil } +func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, assemble func(context.Context) (uint64, error)) (uint64, error) { + if attempts <= 0 { + return 0, errors.New("assemble block requires at least one attempt") + } + var ( + id uint64 + err error + ) + for attempt := range attempts { + if ctxErr := ctx.Err(); ctxErr != nil { + return 0, ctxErr + } + if id, err = assemble(ctx); err == nil { + return id, nil + } + if attempt+1 == attempts { + break + } + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + return 0, ctx.Err() + case <-timer.C: + } + } + return 0, err +} + func (cc *ExecutionClientDirect) SupportInsertion() bool { return true } diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go new file mode 100644 index 00000000000..7863ecb476e --- /dev/null +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -0,0 +1,74 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package execution_client + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + calls := 0 + _, err := retryAssembleBlock(ctx, 30, time.Hour, func(context.Context) (uint64, error) { + calls++ + cancel() + return 0, errors.New("busy") + }) + + require.ErrorIs(t, err, context.Canceled) + require.Equal(t, 1, calls) +} + +func TestRetryAssembleBlockDoesNotStartWithCanceledContext(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + calls := 0 + _, err := retryAssembleBlock(ctx, 30, time.Hour, func(context.Context) (uint64, error) { + calls++ + return 0, errors.New("busy") + }) + + require.ErrorIs(t, err, context.Canceled) + require.Zero(t, calls) +} + +func TestRetryAssembleBlockReturnsFirstSuccess(t *testing.T) { + calls := 0 + id, err := retryAssembleBlock(t.Context(), 3, time.Millisecond, func(context.Context) (uint64, error) { + calls++ + if calls < 3 { + return 0, errors.New("busy") + } + return 7, nil + }) + + require.NoError(t, err) + require.Equal(t, uint64(7), id) + require.Equal(t, 3, calls) +} + +func TestRetryAssembleBlockRejectsNoAttempts(t *testing.T) { + _, err := retryAssembleBlock(t.Context(), 0, time.Millisecond, func(context.Context) (uint64, error) { + return 1, nil + }) + require.EqualError(t, err, "assemble block requires at least one attempt") +} diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index 6d9122aa278..88231210e9e 100644 --- a/execution/execmodule/chainreader/chain_reader.go +++ b/execution/execmodule/chainreader/chain_reader.go @@ -279,7 +279,7 @@ func (c ChainReaderWriterEth1) HasBlock(ctx context.Context, hash common.Hash) ( return c.executionModule.HasBlock(ctx, &hash, nil) } -func (c ChainReaderWriterEth1) AssembleBlock(baseHash common.Hash, attributes *engine_types.PayloadAttributes) (id uint64, err error) { +func (c ChainReaderWriterEth1) AssembleBlock(ctx context.Context, baseHash common.Hash, attributes *engine_types.PayloadAttributes) (id uint64, err error) { params := &builder.Parameters{ ParentHash: baseHash, Timestamp: uint64(attributes.Timestamp), @@ -290,7 +290,7 @@ func (c ChainReaderWriterEth1) AssembleBlock(baseHash common.Hash, attributes *e TargetGasLimit: (*uint64)(attributes.TargetGasLimit), ParentBeaconBlockRoot: attributes.ParentBeaconBlockRoot, } - result, err := c.executionModule.AssembleBlock(context.Background(), params) + result, err := c.executionModule.AssembleBlock(ctx, params) if err != nil { return 0, err } From f808980e7e373bf72ba8e103fea1f304d86c2fdb Mon Sep 17 00:00:00 2001 From: kewei Date: Mon, 10 Aug 2026 23:49:29 +0800 Subject: [PATCH 12/20] execution, cl/beacon: close preparation cancellation races --- cl/beacon/handler/block_production_test.go | 17 +++++++++++ execution/execmodule/block_building.go | 3 ++ .../block_building_internal_test.go | 30 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 57021a154b9..8a033ee7568 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -27,6 +27,7 @@ import ( "testing" "time" + "github.com/go-chi/chi/v5" "github.com/holiman/uint256" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -1086,6 +1087,22 @@ func TestProduceBeaconBodyTakesRootAndStateFromOneView(t *testing.T) { require.Equal(t, postState.Slot(), copied.Slot()) } +func TestGetEthV3ValidatorBlockMapsNotSyncedToServiceUnavailable(t *testing.T) { + _, _, _, _, _, handler, _, syncedData, _, _ := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), false) + syncedDataMock := syncedData.(*sync_mock_services.MockSyncedData) + syncedDataMock.EXPECT().ViewHeadStateWithIdentity(gomock.Any()).Return(synced_data.ErrNotSynced) + + req := httptest.NewRequest(http.MethodGet, "/?randao_reveal="+hexutil.Encode(make([]byte, 96)), nil) + routeCtx := chi.NewRouteContext() + routeCtx.URLParams.Add("slot", "1") + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, routeCtx)) + + _, err := handler.GetEthV3ValidatorBlock(httptest.NewRecorder(), req) + var endpointErr *beaconhttp.EndpointError + require.ErrorAs(t, err, &endpointErr) + require.Equal(t, http.StatusServiceUnavailable, endpointErr.Code) +} + func TestShouldPreparePayloadVersion(t *testing.T) { for _, tc := range []struct { version clparams.StateVersion diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 3d36978e5bb..4e9c32214a0 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -108,6 +108,9 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete return AssembleBlockResult{Busy: true}, nil } defer e.semaphore.Release(1) + if err := ctx.Err(); err != nil { + return AssembleBlockResult{}, err + } if err := e.checkWithdrawalsPresence(params.Timestamp, params.Withdrawals); err != nil { return AssembleBlockResult{}, err diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index 610bb3b4742..eca786915d6 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -168,6 +168,36 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { _, _ = module.builders[result.PayloadID].Stop(context.Background()) } +func TestAssembleBlockCanceledContextDoesNotSupersedeBuilder(t *testing.T) { + started := make(chan *atomic.Bool, 1) + module := &ExecModule{ + semaphore: semaphore.NewWeighted(1), + config: &chain.Config{}, + logger: log.Root(), + builders: map[uint64]*builder.BlockBuilder{}, + builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- interrupt + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }, + } + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + interrupt := <-started + t.Cleanup(func() { + _, _ = module.builders[result.PayloadID].Stop(context.Background()) + }) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, err = module.AssembleBlock(ctx, &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x02}}) + require.ErrorIs(t, err, context.Canceled) + require.False(t, interrupt.Load()) + require.Equal(t, result.PayloadID, module.buildersByTimestamp[100]) +} + func TestCloneBuilderParametersPreservesRepresentations(t *testing.T) { require.Nil(t, cloneBuilderParameters(nil)) From c7bac8a5ab463a5b4044dc520c6d0d5d4bf032ab Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Tue, 11 Aug 2026 12:31:48 +0200 Subject: [PATCH 13/20] cl/beacon: drop the unused node-syncing sentinel Payload preparation and block production now take head state and identity from one locked view, which reports a missing identity as ErrNotSynced. Nothing returns the local sentinel any more. --- cl/beacon/handler/payload_preparation.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 5606b1f9c60..f9ce2cca23e 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -35,7 +35,6 @@ import ( ) var ( - errNodeSyncing = errors.New("node is syncing") errNotOurProposal = errors.New("next slot is not proposed by a registered validator") errNoPayloadID = errors.New("execution layer returned no payload id") errHeadTooFarBack = errors.New("head state is too far behind the slot to prepare") @@ -161,7 +160,6 @@ func shouldPreparePayloadVersion(version clparams.StateVersion) bool { // failure worth reporting. func isExpectedPreparationSkip(err error) bool { return errors.Is(err, errNotOurProposal) || - errors.Is(err, errNodeSyncing) || errors.Is(err, errNoPayloadID) || errors.Is(err, errHeadTooFarBack) || errors.Is(err, errPreparationHeadChanged) || From 18bc33615ee162ff09db46f1ec056ed5a7afbe35 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Tue, 11 Aug 2026 16:19:31 +0200 Subject: [PATCH 14/20] execution: stop serving a superseded payload id Cancelling the previous builder for a timestamp froze it where it stood while leaving its id in the map, so getPayload on that id returned whatever had been packed at cancel time - near-empty when the supersede came early. Drop the entry so a superseded id reads as unknown instead. --- execution/execmodule/block_building.go | 5 +++ .../block_building_internal_test.go | 41 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 4e9c32214a0..962219e6204 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -127,6 +127,11 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete if previous := e.builders[previousID]; previous != nil { previous.Cancel() } + // Cancel freezes a builder where it stands, so a superseded id must stop being + // retrievable: otherwise GetAssembledBlock hands back whatever it had packed at + // that instant, which is near-empty when the supersede came early. + delete(e.builders, previousID) + delete(e.builderParameters, previousID) } // Initiate payload building diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index eca786915d6..be39cb4692e 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -120,6 +120,47 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { require.NotContains(t, module.builderParameters, adjacentID) } +func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { + started := make(chan struct{}, 4) + release := make(chan struct{}) + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builder.BlockBuilder{}, + builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- struct{}{} + <-release + return nil, nil + }, + } + t.Cleanup(func() { + close(release) + for _, blockBuilder := range module.builders { + if blockBuilder != nil { + _, _ = blockBuilder.Stop(context.Background()) + } + } + }) + + first, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + <-started + + second, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x02}}) + require.NoError(t, err) + require.NotEqual(t, first.PayloadID, second.PayloadID) + + // A cancelled builder still holds whatever it had packed; serving that would mean + // proposing a near-empty block, so the superseded id must read as unknown instead. + require.NotContains(t, module.builders, first.PayloadID) + require.NotContains(t, module.builderParameters, first.PayloadID) + + assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) + require.NoError(t, err) + require.Nil(t, assembled.Block) +} + func TestAssembleBlockOwnsParameters(t *testing.T) { type observedParameters struct { parentRoot common.Hash From 15fdf6fe8f8d821c71f33725f0484165f96ce7bd Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Tue, 11 Aug 2026 16:19:31 +0200 Subject: [PATCH 15/20] cl/phase1: do not assemble on a head the execution layer has not adopted A busy forkchoice update leaves the execution layer on the previous head. Building anyway produced a payload on the wrong parent, and the timestamp dedup then pinned that dead id for the rest of the slot, failing every later getPayload. Only attribute-bearing updates are affected; plain head updates are unchanged. --- cl/phase1/execution_client/execution_client_direct.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index 4230493e910..9f12762cc71 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -126,6 +126,9 @@ func (cc *ExecutionClientDirect) NewPayload( return PayloadStatusNone, errors.New("unexpected status") } +// ErrForkChoiceUpdateBusy reports that the execution layer never adopted the requested head. +var ErrForkChoiceUpdateBusy = errors.New("execution layer busy, forkchoice not adopted") + func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized, safe, head common.Hash, attr *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { status, _, _, err := cc.chainRW.UpdateForkChoice(ctx, head, safe, finalized) if err != nil { @@ -140,6 +143,12 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized if attr == nil { return nil, nil } + // A busy update means the execution layer never adopted this head. Assembling anyway + // builds on the wrong parent, and the timestamp dedup then pins that dead payload id for + // the rest of the slot, so every later getPayload fails. + if status == execmodule.ExecutionStatusBusy { + return nil, ErrForkChoiceUpdateBusy + } // Retry AssembleBlock if the EL is busy (semaphore contention with // fork choice commits). This is common in single-process dev mode // where the CL and EL share the same process. From 849d918b2a36dc1aea33a93fee773ba4051c4277 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Tue, 11 Aug 2026 16:19:31 +0200 Subject: [PATCH 16/20] cl/beacon: prime against the selected head, not the memoized one Fork choice publishes the selected head before the execution layer is notified, while HeadRoot only advances once the head state has been copied. Reading the latter let a tick send attributes for a head the execution layer had already moved past, unwinding the block it just executed right before this node proposes. --- cl/beacon/handler/block_production_test.go | 5 +++-- cl/beacon/handler/payload_preparation.go | 19 +++++++++++++++++-- cl/beacon/handler/utils_test.go | 10 ++++++++-- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 8a033ee7568..21227166f80 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -911,6 +911,7 @@ func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { baseBlockRoot := common.Hash{0x41} syncedData := synced_data.NewSyncedDataManager(&config, true) require.NoError(t, syncedData.OnHeadStateWithBlockRoot(headState, baseBlockRoot)) + syncedData.OnSelectedHead(baseBlockRoot, headState.Slot()) handler.beaconChainCfg = &config handler.syncedData = syncedData @@ -988,7 +989,7 @@ func TestPreparePayloadForRejectsChangedHeadBeforeForkChoiceUpdate(t *testing.T) DoAndReturn(func(view synced_data.ViewHeadStateWithIdentityFn) error { return view(postState, baseBlockRoot, postState.Slot()) }), - syncedDataMock.EXPECT().HeadRoot().Return(changedBlockRoot), + syncedDataMock.EXPECT().SelectedHead().Return(changedBlockRoot, postState.Slot(), true), ) engine := execution_client.NewMockExecutionEngine(ctrl) engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) @@ -1049,7 +1050,7 @@ func TestPreparePayloadForUsesPostEpochProposer(t *testing.T) { DoAndReturn(func(view synced_data.ViewHeadStateWithIdentityFn) error { return view(headState, baseBlockRoot, headState.Slot()) }) - syncedDataMock.EXPECT().HeadRoot().Return(baseBlockRoot) + syncedDataMock.EXPECT().SelectedHead().Return(baseBlockRoot, headState.Slot(), true) engine := execution_client.NewMockExecutionEngine(ctrl) engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). DoAndReturn(func(_ context.Context, _, _, _ common.Hash, attrs *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index f9ce2cca23e..56fa715002b 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -26,6 +26,7 @@ import ( "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/clparams" "github.com/erigontech/erigon/cl/phase1/core/state" + "github.com/erigontech/erigon/cl/phase1/execution_client" "github.com/erigontech/erigon/cl/transition" "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/hexutil" @@ -127,7 +128,18 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if stateVersion.AfterOrEqual(clparams.GloasVersion) { return } - if !shouldPrepare(targetSlot, primedSlot, a.syncedData.HeadRoot(), primedHead) { + selectedRoot, _, selected := a.syncedData.SelectedHead() + if !selected { + continue + } + // Fork choice publishes the selected head before the execution layer is notified and + // before the memoized state catches up. Priming while those disagree would send + // attributes for a head the execution layer has already moved past, unwinding the + // block it just executed right before this node proposes. + if selectedRoot != a.syncedData.HeadRoot() { + continue + } + if !shouldPrepare(targetSlot, primedSlot, selectedRoot, primedHead) { continue } if !shouldPreparePayloadVersion(stateVersion) { @@ -163,6 +175,9 @@ func isExpectedPreparationSkip(err error) bool { errors.Is(err, errNoPayloadID) || errors.Is(err, errHeadTooFarBack) || errors.Is(err, errPreparationHeadChanged) || + errors.Is(err, execution_client.ErrForkChoiceUpdateBusy) || + errors.Is(err, context.DeadlineExceeded) || + errors.Is(err, context.Canceled) || errors.Is(err, synced_data.ErrNotSynced) } @@ -223,7 +238,7 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( if err != nil { return common.Hash{}, err } - if a.syncedData.HeadRoot() != baseBlockRoot { + if selectedRoot, _, selected := a.syncedData.SelectedHead(); !selected || selectedRoot != baseBlockRoot { return common.Hash{}, errPreparationHeadChanged } payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalizedHash, safeHash, head, attrs, stateVersion) diff --git a/cl/beacon/handler/utils_test.go b/cl/beacon/handler/utils_test.go index 71657e54917..ae00500217e 100644 --- a/cl/beacon/handler/utils_test.go +++ b/cl/beacon/handler/utils_test.go @@ -87,8 +87,14 @@ func setupTestingHandler(t *testing.T, v clparams.StateVersion, logger log.Logge bcfg.InitializeForkSchedule() if useRealSyncDataMgr { - syncedData = synced_data.NewSyncedDataManager(&bcfg, true) - syncedData.OnHeadState(postState) + manager := synced_data.NewSyncedDataManager(&bcfg, true) + manager.OnHeadState(postState) + // Fork choice publishes the selected head before the state is memoized, so a synced + // fixture has to expose both for handlers that read head identity. + headRoot, err := postState.BlockRoot() + require.NoError(t, err) + manager.OnSelectedHead(headRoot, postState.Slot()) + syncedData = manager } else { syncedData = sync_mock_services.NewMockSyncedData(ctrl) } From cfba475d5054f934c790471e6345bff29866310c Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Tue, 11 Aug 2026 16:50:40 +0200 Subject: [PATCH 17/20] execution, cl/beacon: address P2/P3 review follow-ups - bound how far ahead a slot may be primed, so a pre-genesis clamp cannot freeze a builder hours before its slot and have production reuse it - check cancellation before reporting contention, so an expired request no longer masquerades as Busy and get retried - log an unregistered fee recipient in production instead of silently building to the zero address - derive the prepared warm-up from the two grab offsets so the window and the minimum age cannot drift apart - keep one authority for the fork after which preparation retires - fold builder parameters into the builder entry, dropping a third parallel map and making eviction drop its timestamp index without scanning - compare build parameters directly instead of deep-cloning to feed DeepEqual - restore the why to the buildDuration docstring --- cl/beacon/handler/block_production.go | 25 ++++++-- cl/beacon/handler/block_production_test.go | 27 ++++++++- cl/beacon/handler/payload_preparation.go | 24 +++++++- execution/execmodule/block_building.go | 60 +++++++++++-------- .../block_building_internal_test.go | 28 ++++----- execution/execmodule/exec_module.go | 6 +- 6 files changed, 120 insertions(+), 50 deletions(-) diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index cdddc6f476b..81e07a5bc61 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -211,10 +211,10 @@ func attestationDue(cfg *clparams.BeaconChainConfig, stateVersion clparams.State // computeBlockBuilderWindow uses the earlier collection window only for a sufficiently warmed builder. func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion, prepared bool) blockBuilderWindow { due := attestationDue(cfg, stateVersion) - pollUntil := slotStart.Add(due - due/payloadPublicationDivisor) + pollUntil := slotStart.Add(unpreparedGrabOffset(due)) firstGetAt := pollUntil.Add(-minPayloadPollingWindow) if prepared { - firstGetAt = slotStart.Add(due / payloadPublicationDivisor).Add(-minPayloadPollingWindow) + firstGetAt = slotStart.Add(preparedGrabOffset(due)).Add(-minPayloadPollingWindow) } if firstGetAt.Before(now) { firstGetAt = now @@ -228,9 +228,20 @@ func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconCha } } +// unpreparedGrabOffset is when production starts polling for a payload it did not prime, and +// preparedGrabOffset is when it may start for one it did. Their difference is the warm-up a +// primed builder must already have, so the two paths give a builder the same total build time. +func unpreparedGrabOffset(due time.Duration) time.Duration { + return due - due/payloadPublicationDivisor +} + +func preparedGrabOffset(due time.Duration) time.Duration { + return due / payloadPublicationDivisor +} + func preparedPayloadMinimumAge(cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion) time.Duration { due := attestationDue(cfg, stateVersion) - return max(due-2*due/payloadPublicationDivisor, 0) + return max(unpreparedGrabOffset(due)-preparedGrabOffset(due), 0) } func payloadAttributesForVersion( @@ -1004,7 +1015,13 @@ func (a *ApiHandler) produceBeaconBody( log.Info("BlockProduction: ForkChoiceUpdate&GetPayload took", "duration", time.Since(start)) }() retryTime := 10 * time.Millisecond - feeRecipient, _ := a.validatorParams.GetFeeRecipient(proposerIndex) + feeRecipient, registered := a.validatorParams.GetFeeRecipient(proposerIndex) + if !registered { + // Preparation treats an unregistered proposer as someone else's slot; production + // still builds, so make the zero-address fallback visible rather than silent. + log.Warn("BlockProduction: no fee recipient registered for proposer, using zero address", + "proposer", proposerIndex) + } fcuHead, fcuSafeHash, fcuFinalizedHash := head, safeHash, finalizedHash var attrs *engine_types.PayloadAttributes if stateVersion.Before(clparams.GloasVersion) { diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index 21227166f80..e09f674ce26 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -871,7 +871,7 @@ func TestPreparePayloadLoopRunsImmediatelyWithSlotDeadline(t *testing.T) { require.NoError(t, err) validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) - slotStart := time.Now().Add(time.Minute) + slotStart := time.Now().Add(6 * time.Second) clock := eth_clock.NewMockEthereumClock(ctrl) clock.EXPECT().GetCurrentSlot().Return(targetSlot - 1) clock.EXPECT().GetSlotTime(targetSlot).Return(slotStart) @@ -892,6 +892,31 @@ func TestPreparePayloadLoopRunsImmediatelyWithSlotDeadline(t *testing.T) { handler.preparePayloadLoop(ctx) } +func TestPreparePayloadLoopSkipsSlotsTooFarAhead(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, _, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), true) + targetSlot := postState.Slot() + 1 + proposerIndex, err := postState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) + + // Before genesis the current slot clamps to zero, so the next slot can be hours out. + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetCurrentSlot().Return(targetSlot - 1).AnyTimes() + clock.EXPECT().GetSlotTime(targetSlot).Return(time.Now().Add(time.Hour)).AnyTimes() + handler.ethClock = clock + + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().SupportInsertion().Return(true) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + handler.engine = engine + + ctx, cancel := context.WithTimeout(t.Context(), 250*time.Millisecond) + defer cancel() + handler.StartPayloadPreparation(ctx) + <-ctx.Done() +} + func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { ctrl := gomock.NewController(t) _, _, _, _, postState, handler, _, _, fcu, validatorParams := setupTestingHandler(t, clparams.CapellaVersion, log.Root(), true) diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 56fa715002b..225a53d03a3 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -125,9 +125,16 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { targetSlot := a.ethClock.GetCurrentSlot() + 1 stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) - if stateVersion.AfterOrEqual(clparams.GloasVersion) { + if preparationRetired(stateVersion) { return } + // Before genesis the current slot clamps to zero, so the next slot can be arbitrarily + // far off. A builder primed that early freezes at its own cap long before the slot and + // production would then reuse the stale payload, so leave those slots unprimed. + slotStart := a.ethClock.GetSlotTime(targetSlot) + if time.Until(slotStart) > maxPreparationLead(a.beaconChainCfg) { + continue + } selectedRoot, _, selected := a.syncedData.SelectedHead() if !selected { continue @@ -145,7 +152,7 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if !shouldPreparePayloadVersion(stateVersion) { continue } - prepareCtx, cancel := context.WithDeadline(ctx, a.ethClock.GetSlotTime(targetSlot)) + prepareCtx, cancel := context.WithDeadline(ctx, slotStart) head, err := a.preparePayloadFor(prepareCtx, targetSlot) cancel() if err != nil { @@ -164,8 +171,19 @@ func shouldPrepare(targetSlot, primedSlot uint64, head, primedHead common.Hash) return targetSlot != primedSlot || head != primedHead } +// maxPreparationLead bounds how far ahead of a slot priming is worthwhile. +func maxPreparationLead(cfg *clparams.BeaconChainConfig) time.Duration { + return 2 * time.Duration(cfg.SecondsPerSlot) * time.Second +} + +// preparationRetired is the single authority for the fork after which builders gossip bids +// instead of being primed, so the loop and the per-slot check cannot drift apart. +func preparationRetired(version clparams.StateVersion) bool { + return version.AfterOrEqual(clparams.GloasVersion) +} + func shouldPreparePayloadVersion(version clparams.StateVersion) bool { - return version.AfterOrEqual(clparams.BellatrixVersion) && version.Before(clparams.GloasVersion) + return version.AfterOrEqual(clparams.BellatrixVersion) && !preparationRetired(version) } // isExpectedPreparationSkip reports whether there was simply nothing to prepare, as opposed to a diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 962219e6204..87f079e5251 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -41,7 +41,10 @@ func (e *ExecModule) checkWithdrawalsPresence(time uint64, withdrawals []*types. return nil } -// buildDuration spans the target slot for early requests while bounding late and implausibly future requests. +// buildDuration spans the target slot for early requests while bounding late and implausibly +// future requests. A consensus layer may send attributes well ahead of the slot and then call +// getPayload against the cached id without sending fresh ones, so a builder that stopped before +// the slot would hand back a payload missing every transaction that arrived in between. func buildDuration(payloadTimestamp uint64, now time.Time, secondsPerSlot uint64) time.Duration { slot := time.Duration(secondsPerSlot) * time.Second // Reject beyond the cap horizon before converting: a large enough timestamp overflows @@ -84,6 +87,14 @@ func cloneBuilderParameters(params *builder.Parameters) *builder.Parameters { return &cloned } +// builderEntry keeps a builder with the parameters and timestamp it was created for, so the +// three cannot drift apart and eviction can drop the timestamp index without scanning it. +type builderEntry struct { + builder *builder.BlockBuilder + params *builder.Parameters + timestamp uint64 +} + func (e *ExecModule) evictOldBuilders() { ids := common.SortedKeys(e.builders) @@ -91,47 +102,45 @@ func (e *ExecModule) evictOldBuilders() { for i := 0; i <= len(e.builders)-engine_helpers.MaxBuilders; i++ { id := ids[i] if old := e.builders[id]; old != nil { - old.Cancel() - } - delete(e.builders, id) - delete(e.builderParameters, id) - for timestamp, builderID := range e.buildersByTimestamp { - if builderID == id { - delete(e.buildersByTimestamp, timestamp) + if old.builder != nil { + old.builder.Cancel() + } + if e.buildersByTimestamp[old.timestamp] == id { + delete(e.buildersByTimestamp, old.timestamp) } } + delete(e.builders, id) } } func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Parameters) (AssembleBlockResult, error) { + // Cancellation is checked first so an expired request reports why it stopped instead of + // masquerading as contention, which callers retry. + if err := ctx.Err(); err != nil { + return AssembleBlockResult{}, err + } if !e.semaphore.TryAcquire(1) { return AssembleBlockResult{Busy: true}, nil } defer e.semaphore.Release(1) - if err := ctx.Err(); err != nil { - return AssembleBlockResult{}, err - } if err := e.checkWithdrawalsPresence(params.Timestamp, params.Withdrawals); err != nil { return AssembleBlockResult{}, err } if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { - candidate := cloneBuilderParameters(params) - candidate.PayloadId = previousID params.PayloadId = previousID - if reflect.DeepEqual(e.builderParameters[previousID], candidate) { + if previous := e.builders[previousID]; previous != nil && reflect.DeepEqual(previous.params, params) { e.logger.Info("[ForkChoiceUpdated] duplicate build request") return AssembleBlockResult{PayloadID: previousID}, nil } - if previous := e.builders[previousID]; previous != nil { - previous.Cancel() + if previous := e.builders[previousID]; previous != nil && previous.builder != nil { + previous.builder.Cancel() } // Cancel freezes a builder where it stands, so a superseded id must stop being // retrievable: otherwise GetAssembledBlock hands back whatever it had packed at // that instant, which is near-empty when the supersede came early. delete(e.builders, previousID) - delete(e.builderParameters, previousID) } // Initiate payload building @@ -141,15 +150,15 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete params.PayloadId = e.nextPayloadId ownedParams := cloneBuilderParameters(params) - e.builders[e.nextPayloadId] = builder.NewBlockBuilder(e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())) if e.buildersByTimestamp == nil { e.buildersByTimestamp = make(map[uint64]uint64) } - if e.builderParameters == nil { - e.builderParameters = make(map[uint64]*builder.Parameters) + e.builders[e.nextPayloadId] = &builderEntry{ + builder: builder.NewBlockBuilder(e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())), + params: ownedParams, + timestamp: params.Timestamp, } e.buildersByTimestamp[params.Timestamp] = e.nextPayloadId - e.builderParameters[e.nextPayloadId] = ownedParams e.logger.Info("[ForkChoiceUpdated] BlockBuilder added", "payload", e.nextPayloadId) return AssembleBlockResult{PayloadID: e.nextPayloadId}, nil @@ -172,16 +181,19 @@ func blockValue(br *types.BlockWithReceipts, baseFee *uint256.Int) *uint256.Int } func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (AssembledBlockResult, error) { + if err := ctx.Err(); err != nil { + return AssembledBlockResult{}, err + } if !e.semaphore.TryAcquire(1) { return AssembledBlockResult{Busy: true}, nil } defer e.semaphore.Release(1) - bldr, ok := e.builders[payloadID] - if !ok { + entry, ok := e.builders[payloadID] + if !ok || entry.builder == nil { return AssembledBlockResult{}, nil } - blockWithReceipts, err := bldr.Stop(ctx) + blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { e.logger.Error("Failed to build PoS block", "err", err) return AssembledBlockResult{}, err diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index be39cb4692e..fc2610eca5e 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -46,7 +46,7 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { semaphore: semaphore.NewWeighted(1), config: &chain.Config{}, logger: log.Root(), - builders: map[uint64]*builder.BlockBuilder{}, + builders: map[uint64]*builderEntry{}, builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- runningBuilder{id: params.PayloadId, interrupt: interrupt} for !interrupt.Load() { @@ -56,9 +56,9 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { }, } t.Cleanup(func() { - for _, blockBuilder := range module.builders { - if blockBuilder != nil { - _, _ = blockBuilder.Stop(context.Background()) + for _, entry := range module.builders { + if entry != nil && entry.builder != nil { + _, _ = entry.builder.Stop(context.Background()) } } }) @@ -117,7 +117,7 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { require.Eventually(t, adjacent.interrupt.Load, time.Second, time.Millisecond) require.NotContains(t, module.builders, adjacentID) require.NotContains(t, module.buildersByTimestamp, uint64(101)) - require.NotContains(t, module.builderParameters, adjacentID) + require.NotContains(t, module.builders, adjacentID) } func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { @@ -127,7 +127,7 @@ func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { logger: log.Root(), config: &chain.Config{}, semaphore: semaphore.NewWeighted(1), - builders: map[uint64]*builder.BlockBuilder{}, + builders: map[uint64]*builderEntry{}, builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- struct{}{} <-release @@ -136,9 +136,9 @@ func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { } t.Cleanup(func() { close(release) - for _, blockBuilder := range module.builders { - if blockBuilder != nil { - _, _ = blockBuilder.Stop(context.Background()) + for _, entry := range module.builders { + if entry != nil && entry.builder != nil { + _, _ = entry.builder.Stop(context.Background()) } } }) @@ -154,7 +154,7 @@ func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { // A cancelled builder still holds whatever it had packed; serving that would mean // proposing a near-empty block, so the superseded id must read as unknown instead. require.NotContains(t, module.builders, first.PayloadID) - require.NotContains(t, module.builderParameters, first.PayloadID) + require.NotContains(t, module.builders, first.PayloadID) assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) require.NoError(t, err) @@ -172,7 +172,7 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { semaphore: semaphore.NewWeighted(1), config: &chain.Config{}, logger: log.Root(), - builders: map[uint64]*builder.BlockBuilder{}, + builders: map[uint64]*builderEntry{}, builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { <-readParameters observed <- observedParameters{parentRoot: *params.ParentBeaconBlockRoot, extraData: params.ExtraData[0]} @@ -206,7 +206,7 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { }) require.NoError(t, err) require.Equal(t, result.PayloadID, duplicate.PayloadID) - _, _ = module.builders[result.PayloadID].Stop(context.Background()) + _, _ = module.builders[result.PayloadID].builder.Stop(context.Background()) } func TestAssembleBlockCanceledContextDoesNotSupersedeBuilder(t *testing.T) { @@ -215,7 +215,7 @@ func TestAssembleBlockCanceledContextDoesNotSupersedeBuilder(t *testing.T) { semaphore: semaphore.NewWeighted(1), config: &chain.Config{}, logger: log.Root(), - builders: map[uint64]*builder.BlockBuilder{}, + builders: map[uint64]*builderEntry{}, builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- interrupt for !interrupt.Load() { @@ -228,7 +228,7 @@ func TestAssembleBlockCanceledContextDoesNotSupersedeBuilder(t *testing.T) { require.NoError(t, err) interrupt := <-started t.Cleanup(func() { - _, _ = module.builders[result.PayloadID].Stop(context.Background()) + _, _ = module.builders[result.PayloadID].builder.Stop(context.Background()) }) ctx, cancel := context.WithCancel(t.Context()) diff --git a/execution/execmodule/exec_module.go b/execution/execmodule/exec_module.go index 8aeb6508f94..f7c8f2f7d34 100644 --- a/execution/execmodule/exec_module.go +++ b/execution/execmodule/exec_module.go @@ -196,9 +196,8 @@ type ExecModule struct { // Block building nextPayloadId uint64 builderFunc builder.BlockBuilderFunc - builders map[uint64]*builder.BlockBuilder + builders map[uint64]*builderEntry buildersByTimestamp map[uint64]uint64 - builderParameters map[uint64]*builder.Parameters // Changes accumulator hook *stageloop.Hook @@ -267,9 +266,8 @@ func NewExecModule( logger: logger, forkValidator: forkValidator, pipelineExecutor: pipelineExecutor, - builders: make(map[uint64]*builder.BlockBuilder), + builders: make(map[uint64]*builderEntry), buildersByTimestamp: make(map[uint64]uint64), - builderParameters: make(map[uint64]*builder.Parameters), builderFunc: builderFunc, config: config, semaphore: semaphore.NewWeighted(1), From 0c7de4487bc633ee05551232de79a55ab224f0cc Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Tue, 11 Aug 2026 17:48:47 +0200 Subject: [PATCH 18/20] cl/phase1: wait for a busy forkchoice update instead of abandoning the slot Erroring on Busy made production drop the payload entirely, where the assemble retry had previously ridden out the contention - the update continues on the module's own context, so a later attempt sees it settle. Retry it under the caller's context, and require an adopted head before assembling so MissingSegment and TooFarAway no longer fall through and pin an unservable payload id. --- cl/beacon/handler/payload_preparation.go | 2 +- .../execution_client_direct.go | 54 ++++++++++++++--- .../execution_client_direct_test.go | 59 +++++++++++++++++++ 3 files changed, 107 insertions(+), 8 deletions(-) diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 225a53d03a3..17140577b3c 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -193,7 +193,7 @@ func isExpectedPreparationSkip(err error) bool { errors.Is(err, errNoPayloadID) || errors.Is(err, errHeadTooFarBack) || errors.Is(err, errPreparationHeadChanged) || - errors.Is(err, execution_client.ErrForkChoiceUpdateBusy) || + errors.Is(err, execution_client.ErrForkChoiceNotAdopted) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) || errors.Is(err, synced_data.ErrNotSynced) diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index 9f12762cc71..90772c26be6 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -126,8 +126,14 @@ func (cc *ExecutionClientDirect) NewPayload( return PayloadStatusNone, errors.New("unexpected status") } -// ErrForkChoiceUpdateBusy reports that the execution layer never adopted the requested head. -var ErrForkChoiceUpdateBusy = errors.New("execution layer busy, forkchoice not adopted") +// ErrForkChoiceNotAdopted reports that the execution layer did not adopt the requested head, so +// there is nothing to build on. +var ErrForkChoiceNotAdopted = errors.New("execution layer did not adopt forkchoice head") + +const ( + forkChoiceBusyAttempts = 30 + forkChoiceBusyDelay = 200 * time.Millisecond +) func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized, safe, head common.Hash, attr *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { status, _, _, err := cc.chainRW.UpdateForkChoice(ctx, head, safe, finalized) @@ -143,11 +149,20 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized if attr == nil { return nil, nil } - // A busy update means the execution layer never adopted this head. Assembling anyway - // builds on the wrong parent, and the timestamp dedup then pins that dead payload id for - // the rest of the slot, so every later getPayload fails. - if status == execmodule.ExecutionStatusBusy { - return nil, ErrForkChoiceUpdateBusy + // Only assemble once the execution layer has actually adopted the head. Building on a head it + // never took pins a payload id that can never be served, because the timestamp dedup then hands + // that dead id back for the rest of the slot. Busy is transient - the update continues on the + // module's own context - so wait for it to settle rather than giving up on the slot. + status, err = awaitForkChoiceAdopted(ctx, status, forkChoiceBusyAttempts, forkChoiceBusyDelay, + func(ctx context.Context) (execmodule.ExecutionStatus, error) { + retried, _, _, retryErr := cc.chainRW.UpdateForkChoice(ctx, head, safe, finalized) + return retried, retryErr + }) + if err != nil { + return nil, err + } + if status != execmodule.ExecutionStatusSuccess { + return nil, fmt.Errorf("%w: status %d", ErrForkChoiceNotAdopted, status) } // Retry AssembleBlock if the EL is busy (semaphore contention with // fork choice commits). This is common in single-process dev mode @@ -163,6 +178,31 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized return idBytes, nil } +// awaitForkChoiceAdopted retries while the execution layer reports contention. The update itself +// continues on the module's own context, so a later attempt observes it settle instead of starting +// fresh work; giving up immediately would abandon a proposal over a transient condition. +func awaitForkChoiceAdopted( + ctx context.Context, + status execmodule.ExecutionStatus, + attempts int, + delay time.Duration, + update func(context.Context) (execmodule.ExecutionStatus, error), +) (execmodule.ExecutionStatus, error) { + for attempt := 0; status == execmodule.ExecutionStatusBusy && attempt < attempts; attempt++ { + select { + case <-ctx.Done(): + return status, ctx.Err() + case <-time.After(delay): + } + retried, err := update(ctx) + if err != nil { + return status, fmt.Errorf("execution Client RPC failed to retrieve ForkChoiceUpdate response, err: %w", err) + } + status = retried + } + return status, nil +} + func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, assemble func(context.Context) (uint64, error)) (uint64, error) { if attempts <= 0 { return 0, errors.New("assemble block requires at least one attempt") diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go index 7863ecb476e..c5413d0ad5b 100644 --- a/cl/phase1/execution_client/execution_client_direct_test.go +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -23,6 +23,8 @@ import ( "time" "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/execution/execmodule" ) func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { @@ -72,3 +74,60 @@ func TestRetryAssembleBlockRejectsNoAttempts(t *testing.T) { }) require.EqualError(t, err, "assemble block requires at least one attempt") } + +func TestAwaitForkChoiceAdoptedPassesThroughSettledStatus(t *testing.T) { + calls := 0 + status, err := awaitForkChoiceAdopted(t.Context(), execmodule.ExecutionStatusSuccess, 30, time.Hour, + func(context.Context) (execmodule.ExecutionStatus, error) { + calls++ + return execmodule.ExecutionStatusSuccess, nil + }) + + require.NoError(t, err) + require.Equal(t, execmodule.ExecutionStatusSuccess, status) + require.Zero(t, calls, "a settled status must not be re-sent") +} + +func TestAwaitForkChoiceAdoptedWaitsForBusyToSettle(t *testing.T) { + calls := 0 + status, err := awaitForkChoiceAdopted(t.Context(), execmodule.ExecutionStatusBusy, 30, time.Millisecond, + func(context.Context) (execmodule.ExecutionStatus, error) { + calls++ + if calls < 3 { + return execmodule.ExecutionStatusBusy, nil + } + return execmodule.ExecutionStatusSuccess, nil + }) + + require.NoError(t, err) + require.Equal(t, execmodule.ExecutionStatusSuccess, status) + require.Equal(t, 3, calls) +} + +func TestAwaitForkChoiceAdoptedGivesUpAfterAttempts(t *testing.T) { + calls := 0 + status, err := awaitForkChoiceAdopted(t.Context(), execmodule.ExecutionStatusBusy, 2, time.Millisecond, + func(context.Context) (execmodule.ExecutionStatus, error) { + calls++ + return execmodule.ExecutionStatusBusy, nil + }) + + // Still Busy, so the caller reports the head was never adopted rather than assembling on it. + require.NoError(t, err) + require.Equal(t, execmodule.ExecutionStatusBusy, status) + require.Equal(t, 2, calls) +} + +func TestAwaitForkChoiceAdoptedStopsWhenContextIsCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + calls := 0 + _, err := awaitForkChoiceAdopted(ctx, execmodule.ExecutionStatusBusy, 30, time.Hour, + func(context.Context) (execmodule.ExecutionStatus, error) { + calls++ + return execmodule.ExecutionStatusSuccess, nil + }) + + require.ErrorIs(t, err, context.Canceled) + require.Zero(t, calls) +} From 0a970ff40eae8104912509e853b103f6cac303c7 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Wed, 12 Aug 2026 22:55:54 +0200 Subject: [PATCH 19/20] cl/beacon, cl/phase1, execution: address the second consolidated review P1 - Treat a cancelled or failed builder as absent when deduplicating by timestamp. A builder that died latches its error, so reusing its id spent the slot waiting for a payload that could never arrive, with no way to re-prime while slot and head were unchanged. - Stop deleting a superseded payload id. The id-matching production already performs makes the deletion redundant, while dropping the entry turned a getPayload from any other consumer into an unknown-payload error where the builder used to stay retrievable. - Stop retrying a busy forkchoice update inside the execution client, which re-asserted the head it had captured for up to six seconds. Busy is now reported as ErrForkChoiceBusy and the caller decides: preparation skips and re-primes on the next tick with a freshly read head, production retries its own pinned head only until the payload has to be collected. P2 - Stand preparation down while a block is being produced, so priming the next slot cannot hold the execution layer's semaphore across this slot's collection window. - Remember the not-ours verdict per slot and head, so the last slot of an epoch no longer pays a state copy and a full epoch transition on all four ticks. - Require a minimum usable lead before priming, which also removes the case where the work ran under an already expired deadline. - Send withdrawals and the parent beacon block root for every fork. The execution layer decides what they mean from the payload timestamp, so gating them on the consensus fork put the two sides on different oracles; retryAssembleBlock now also stops on a rejection instead of retrying a permanent error thirty times. - Thread the caller's context into GetAssembledBlock on the caplin path. - State the revenue tradeoff the earlier collection window makes. P3 - Report a failing poll once with a final count instead of once per attempt. - Guard a nil builder entry consistently with the sibling sites. - Bound the preparation lead at one slot, which is all a live chain offers. - Reduce payloadAttributes to the fields every fork sends, share the safe/finalized fallback between preparation and production, and route the withdrawal conversion through one exported converter. --- cl/beacon/handler/block_production.go | 140 +++++++++-------- cl/beacon/handler/block_production_test.go | 144 +++++++++++++----- cl/beacon/handler/handler.go | 4 + cl/beacon/handler/payload_preparation.go | 106 +++++++------ cl/cltypes/withdrawal.go | 10 ++ .../execution_client_direct.go | 74 ++++----- .../execution_client_direct_test.go | 88 ++++------- .../execution_client_engine.go | 2 +- cl/phase1/stages/forkchoice.go | 12 +- execution/builder/block_builder.go | 17 +++ execution/builder/block_builder_test.go | 79 ++++++++++ execution/execmodule/block_building.go | 38 +++-- .../block_building_internal_test.go | 92 ++++++++--- .../execmodule/chainreader/chain_reader.go | 12 +- execution/execmodule/exec_module_test.go | 2 +- 15 files changed, 520 insertions(+), 300 deletions(-) create mode 100644 execution/builder/block_builder_test.go diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index 81e07a5bc61..6db7d633368 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -46,6 +46,7 @@ import ( "github.com/erigontech/erigon/cl/gossip" "github.com/erigontech/erigon/cl/persistence/beacon_indicies" "github.com/erigontech/erigon/cl/phase1/core/state" + "github.com/erigontech/erigon/cl/phase1/execution_client" "github.com/erigontech/erigon/cl/phase1/forkchoice" "github.com/erigontech/erigon/cl/phase1/network/subnets" "github.com/erigontech/erigon/cl/pool" @@ -208,7 +209,10 @@ func attestationDue(cfg *clparams.BeaconChainConfig, stateVersion clparams.State return time.Duration(cfg.AttestationDueMs(stateVersion.AfterOrEqual(clparams.GloasVersion))) * time.Millisecond } -// computeBlockBuilderWindow uses the earlier collection window only for a sufficiently warmed builder. +// computeBlockBuilderWindow uses the earlier collection window only for a sufficiently warmed +// builder. Collecting stops the builder, so the earlier window trades the transactions that would +// have arrived during the rest of the slot for the time it takes to sign, publish and gossip the +// block before attesters vote on it. func computeBlockBuilderWindow(now, slotStart time.Time, cfg *clparams.BeaconChainConfig, stateVersion clparams.StateVersion, prepared bool) blockBuilderWindow { due := attestationDue(cfg, stateVersion) pollUntil := slotStart.Add(unpreparedGrabOffset(due)) @@ -244,31 +248,50 @@ func preparedPayloadMinimumAge(cfg *clparams.BeaconChainConfig, stateVersion clp return max(unpreparedGrabOffset(due)-preparedGrabOffset(due), 0) } -func payloadAttributesForVersion( - version clparams.StateVersion, +const forkChoiceBusyRetryDelay = 100 * time.Millisecond + +// forkChoiceUpdateForProposal keeps asking while the execution layer reports contention, but only +// until the payload would have to be collected: an id obtained after that is no use to this slot. +// The head is not re-read because a proposal is committed to the parent it is being built on. +func (a *ApiHandler) forkChoiceUpdateForProposal( + ctx context.Context, + targetSlot uint64, + finalized, safe, head common.Hash, + attrs *engine_types.PayloadAttributes, + stateVersion clparams.StateVersion, +) ([]byte, error) { + retryUntil := a.ethClock.GetSlotTime(targetSlot).Add(unpreparedGrabOffset(attestationDue(a.beaconChainCfg, stateVersion))) + for { + idBytes, err := a.engine.ForkChoiceUpdate(ctx, finalized, safe, head, attrs, stateVersion) + if !errors.Is(err, execution_client.ErrForkChoiceBusy) || !time.Now().Before(retryUntil) { + return idBytes, err + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(forkChoiceBusyRetryDelay): + } + } +} + +// payloadAttributes builds the attributes preparation and production must agree on. Withdrawals and +// the parent beacon block root are set for every fork because the execution layer decides what to do +// with them from the payload timestamp; gating them on the consensus fork instead puts the two sides +// on different oracles, which disagree on any chain where the forks are not aligned. +func payloadAttributes( timestamp hexutil.Uint64, prevRandao common.Hash, feeRecipient common.Address, withdrawals []*types.Withdrawal, parentRoot *common.Hash, - slotNumber, targetGasLimit *hexutil.Uint64, ) *engine_types.PayloadAttributes { - attrs := &engine_types.PayloadAttributes{ + return &engine_types.PayloadAttributes{ Timestamp: timestamp, PrevRandao: prevRandao, SuggestedFeeRecipient: feeRecipient, + Withdrawals: withdrawals, + ParentBeaconBlockRoot: parentRoot, } - if version.AfterOrEqual(clparams.CapellaVersion) { - attrs.Withdrawals = withdrawals - } - if version.AfterOrEqual(clparams.DenebVersion) { - attrs.ParentBeaconBlockRoot = parentRoot - } - if version.AfterOrEqual(clparams.GloasVersion) { - attrs.SlotNumber = slotNumber - attrs.TargetGasLimit = targetGasLimit - } - return attrs } func shouldRetryGetPayload(now, deadline time.Time) bool { @@ -296,11 +319,26 @@ func pollAssembledPayload( defer deadlineTimer.Stop() retryTicker := time.NewTicker(retryTime) defer retryTicker.Stop() + // A slot that fails fails on every poll of the window, so report the first and then a count + // rather than hundreds of copies of one line. + var ( + failures int + lastErr error + ) + defer func() { + if failures > 1 { + log.Error("BlockProduction: payload polling kept failing", "attempts", failures, "err", lastErr) + } + }() for { // Grab at least once, even past the deadline, so a late produce request still gets a payload. payload, bundles, requestsBundle, blockValue, err := get() if err != nil { - log.Error("BlockProduction: Failed to get payload", "err", err) + failures++ + lastErr = err + if failures == 1 { + log.Error("BlockProduction: Failed to get payload", "err", err) + } } else if payload != nil { return payload, bundles, requestsBundle, blockValue, true } @@ -903,6 +941,9 @@ func (a *ApiHandler) produceBeaconBody( baseBlockSlot, ) } + a.proposalsInFlight.Add(1) + defer a.proposalsInFlight.Add(-1) + var wg sync.WaitGroup stateVersion := a.beaconChainCfg.GetCurrentStateVersion( targetSlot / a.beaconChainCfg.SlotsPerEpoch, @@ -964,14 +1005,7 @@ func (a *ApiHandler) produceBeaconBody( head = baseState.GetLatestBlockHash() } } - finalizedHash := a.forkchoiceStore.GetFinalizedExecutionHash(baseState.FinalizedCheckpoint().Root) - if finalizedHash == (common.Hash{}) { - finalizedHash = head - } - safeHash := a.forkchoiceStore.GetFinalizedExecutionHash(baseState.CurrentJustifiedCheckpoint().Root) - if safeHash == (common.Hash{}) { - safeHash = head - } + safeHash, finalizedHash := a.executionCheckpointHashes(baseState, head) proposerIndex, err := baseState.GetBeaconProposerIndexForSlot(targetSlot) if err != nil { return nil, 0, err @@ -1026,7 +1060,7 @@ func (a *ApiHandler) produceBeaconBody( var attrs *engine_types.PayloadAttributes if stateVersion.Before(clparams.GloasVersion) { var err error - fcuHead, fcuSafeHash, fcuFinalizedHash, attrs, err = a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient, stateVersion) + fcuHead, fcuSafeHash, fcuFinalizedHash, attrs, err = a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) if err != nil { log.Error("BlockProduction: build forkchoice inputs failed", "err", err) return @@ -1044,56 +1078,33 @@ func (a *ApiHandler) produceBeaconBody( log.Error("BlockProduction: GetExpectedWithdrawals (FULL) failed", "err", err) return } - withdrawals = make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals)) - for _, w := range clWithdrawals.Withdrawals { - withdrawals = append(withdrawals, &types.Withdrawal{ - Index: w.Index, - Amount: w.Amount, - Validator: w.Validator, - Address: w.Address, - }) - } + withdrawals = cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(clWithdrawals.Withdrawals) case stateVersion >= clparams.GloasVersion && gloasWithdrawalsState == nil: // GLOAS EMPTY: use cached payload_expected_withdrawals from state cachedWithdrawals := baseState.GetPayloadExpectedWithdrawals() if cachedWithdrawals != nil { - withdrawals = make([]*types.Withdrawal, 0, cachedWithdrawals.Len()) - for i := 0; i < cachedWithdrawals.Len(); i++ { - w := cachedWithdrawals.Get(i) - withdrawals = append(withdrawals, &types.Withdrawal{ - Index: w.Index, - Amount: w.Amount, - Validator: w.Validator, - Address: w.Address, - }) + consensusWithdrawals := make([]*cltypes.Withdrawal, cachedWithdrawals.Len()) + for i := range consensusWithdrawals { + consensusWithdrawals[i] = cachedWithdrawals.Get(i) } + withdrawals = cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals) } } if stateVersion.AfterOrEqual(clparams.GloasVersion) { - var slotNumber *hexutil.Uint64 - sn := hexutil.Uint64(targetSlot) - slotNumber = &sn - attrs = payloadAttributesForVersion( - stateVersion, + attrs = payloadAttributes( hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), random, feeRecipient, withdrawals, (*common.Hash)(&blockRoot), - slotNumber, - targetGasLimit, ) + slotNumber := hexutil.Uint64(targetSlot) + attrs.SlotNumber = &slotNumber + attrs.TargetGasLimit = targetGasLimit } builderStartedAt := time.Now() - idBytes, err := a.engine.ForkChoiceUpdate( - ctx, - fcuFinalizedHash, - fcuSafeHash, - fcuHead, - attrs, - stateVersion, - ) + idBytes, err := a.forkChoiceUpdateForProposal(ctx, targetSlot, fcuFinalizedHash, fcuSafeHash, fcuHead, attrs, stateVersion) if err != nil { log.Error("BlockProduction: Failed to get payload id", "err", err) return @@ -2641,15 +2652,12 @@ func (a *ApiHandler) cacheExecutionBody(payload *cltypes.Eth1Block) { } var ws []*types.Withdrawal if payload.Withdrawals != nil { - payload.Withdrawals.Range(func(idx int, w *cltypes.Withdrawal, total int) bool { - ws = append(ws, &types.Withdrawal{ - Index: w.Index, - Validator: w.Validator, - Address: w.Address, - Amount: w.Amount, - }) + consensusWithdrawals := make([]*cltypes.Withdrawal, 0, payload.Withdrawals.Len()) + payload.Withdrawals.Range(func(_ int, w *cltypes.Withdrawal, _ int) bool { + consensusWithdrawals = append(consensusWithdrawals, w) return true }) + ws = cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals) } a.blockReader.CacheBlockBody(payload.BlockNumber, rawTxs, ws) } diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index e09f674ce26..c595c10cfc6 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -917,6 +917,93 @@ func TestPreparePayloadLoopSkipsSlotsTooFarAhead(t *testing.T) { <-ctx.Done() } +func TestPreparePayloadLoopStandsOffWhileProducing(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, _, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), true) + targetSlot := postState.Slot() + 1 + proposerIndex, err := postState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) + + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetCurrentSlot().Return(targetSlot - 1).AnyTimes() + clock.EXPECT().GetSlotTime(gomock.Any()).Times(0) + handler.ethClock = clock + + // Priming would contend with the block being produced for the execution layer's single slot. + handler.proposalsInFlight.Add(1) + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + handler.engine = engine + + ctx, cancel := context.WithTimeout(t.Context(), 250*time.Millisecond) + defer cancel() + handler.preparePayloadLoop(ctx) +} + +func TestPreparePayloadLoopSkipsSlotsAboutToStart(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, _, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), true) + targetSlot := postState.Slot() + 1 + proposerIndex, err := postState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) + + // Too close to the slot for the prime to ever reach the age production demands of it, so the + // state copy and the forkchoice update would both be spent for nothing. + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetCurrentSlot().Return(targetSlot - 1).AnyTimes() + clock.EXPECT().GetSlotTime(targetSlot).Return(time.Now().Add(200 * time.Millisecond)).AnyTimes() + handler.ethClock = clock + + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + handler.engine = engine + + ctx, cancel := context.WithTimeout(t.Context(), 250*time.Millisecond) + defer cancel() + handler.preparePayloadLoop(ctx) +} + +func TestForkChoiceUpdateForProposalWaitsOutContention(t *testing.T) { + ctrl := gomock.NewController(t) + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetSlotTime(uint64(10)).Return(time.Now()) + + calls := 0 + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(context.Context, common.Hash, common.Hash, common.Hash, *engine_types.PayloadAttributes, clparams.StateVersion) ([]byte, error) { + calls++ + if calls < 3 { + return nil, execution_client.ErrForkChoiceBusy + } + return []byte{1, 2, 3, 4, 5, 6, 7, 8}, nil + }).Times(3) + + a := &ApiHandler{engine: engine, ethClock: clock, beaconChainCfg: &clparams.MainnetBeaconConfig} + id, err := a.forkChoiceUpdateForProposal(t.Context(), 10, common.Hash{}, common.Hash{}, common.Hash{}, &engine_types.PayloadAttributes{}, clparams.ElectraVersion) + + require.NoError(t, err) + require.Len(t, id, 8) +} + +func TestForkChoiceUpdateForProposalStopsOnceTheWindowClosed(t *testing.T) { + ctrl := gomock.NewController(t) + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetSlotTime(uint64(10)).Return(time.Now().Add(-time.Minute)) + + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, execution_client.ErrForkChoiceBusy).Times(1) + + // Past the point the payload has to be collected, another attempt cannot help this slot. + a := &ApiHandler{engine: engine, ethClock: clock, beaconChainCfg: &clparams.MainnetBeaconConfig} + _, err := a.forkChoiceUpdateForProposal(t.Context(), 10, common.Hash{}, common.Hash{}, common.Hash{}, &engine_types.PayloadAttributes{}, clparams.ElectraVersion) + + require.ErrorIs(t, err, execution_client.ErrForkChoiceBusy) +} + func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { ctrl := gomock.NewController(t) _, _, _, _, postState, handler, _, _, fcu, validatorParams := setupTestingHandler(t, clparams.CapellaVersion, log.Root(), true) @@ -1146,41 +1233,19 @@ func TestShouldPreparePayloadVersion(t *testing.T) { } } -func TestPayloadAttributesForkFields(t *testing.T) { +func TestPayloadAttributesCarryEveryFieldTheExecutionLayerGates(t *testing.T) { root := common.Hash{0xaa} - slot := hexutil.Uint64(10) - gasLimit := hexutil.Uint64(30_000_000) withdrawals := []*types.Withdrawal{{Index: 1}} - for _, tc := range []struct { - version clparams.StateVersion - wantWithdrawals bool - wantParentRoot bool - wantGloasFields bool - }{ - {clparams.BellatrixVersion, false, false, false}, - {clparams.CapellaVersion, true, false, false}, - {clparams.DenebVersion, true, true, false}, - {clparams.FuluVersion, true, true, false}, - {clparams.GloasVersion, true, true, true}, - } { - t.Run(tc.version.String(), func(t *testing.T) { - attrs := payloadAttributesForVersion( - tc.version, - 1, - common.Hash{0xbb}, - common.Address{0xcc}, - withdrawals, - &root, - &slot, - &gasLimit, - ) - require.Equal(t, tc.wantWithdrawals, attrs.Withdrawals != nil) - require.Equal(t, tc.wantParentRoot, attrs.ParentBeaconBlockRoot != nil) - require.Equal(t, tc.wantGloasFields, attrs.SlotNumber != nil) - require.Equal(t, tc.wantGloasFields, attrs.TargetGasLimit != nil) - }) - } + attrs := payloadAttributes(1, common.Hash{0xbb}, common.Address{0xcc}, withdrawals, &root) + + // The execution layer decides from the payload timestamp what these mean, and rejects a request + // that disagrees with it. Withholding them on a consensus-fork rule puts the two on separate + // oracles, so both are sent whatever the fork. + require.Equal(t, withdrawals, attrs.Withdrawals) + require.Equal(t, &root, attrs.ParentBeaconBlockRoot) + require.Nil(t, attrs.SlotNumber, "only a Gloas proposal knows its slot number") + require.Nil(t, attrs.TargetGasLimit) } func TestPreparedPayloadKeepsConsecutiveSlots(t *testing.T) { @@ -1214,20 +1279,21 @@ func TestPreparedPayloadCopiesTheID(t *testing.T) { require.False(t, p.matches(10, id, now, 0)) } -func TestShouldPrepareAgainWhenTheHeadMoves(t *testing.T) { +func TestSlotHeadMatchesOnlyTheExactPairing(t *testing.T) { headA := common.Hash{0xaa} headB := common.Hash{0xbb} + settled := slotHead{slot: 10, head: headA} - // Nothing primed yet. - require.True(t, shouldPrepare(10, 0, headA, common.Hash{})) + // Nothing recorded yet matches nothing. + require.False(t, slotHead{}.is(10, headA)) - // Already primed this slot on this head: nothing to do. - require.False(t, shouldPrepare(10, 10, headA, headA)) + // Already settled for this slot on this head: nothing to do. + require.True(t, settled.is(10, headA)) // The previous slot's block arrived late and moved the head, so the warm builder is on a parent // that is no longer the head — prime again rather than wait for the next slot. - require.True(t, shouldPrepare(10, 10, headB, headA)) + require.False(t, settled.is(10, headB)) // A new target slot always needs priming. - require.True(t, shouldPrepare(11, 10, headA, headA)) + require.False(t, settled.is(11, headA)) } diff --git a/cl/beacon/handler/handler.go b/cl/beacon/handler/handler.go index c2d1c7bd892..11b3261df9c 100644 --- a/cl/beacon/handler/handler.go +++ b/cl/beacon/handler/handler.go @@ -108,6 +108,10 @@ type ApiHandler struct { // preparedPayload tracks the payload primed ahead of a slot this node proposes. preparedPayload preparedPayload + // proposalsInFlight counts block productions running on this node, so preparation can stand + // off. Both go through the execution layer's weight-one semaphore, and a prime holding it + // across the collection window turns a fully built payload into a missed slot. + proposalsInFlight atomic.Int64 // Validator data structures validatorParams *validator_params.ValidatorParams diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 17140577b3c..66adab2c422 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -25,6 +25,7 @@ import ( "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/clparams" + "github.com/erigontech/erigon/cl/cltypes" "github.com/erigontech/erigon/cl/phase1/core/state" "github.com/erigontech/erigon/cl/phase1/execution_client" "github.com/erigontech/erigon/cl/transition" @@ -32,7 +33,6 @@ import ( "github.com/erigontech/erigon/common/hexutil" "github.com/erigontech/erigon/common/log/v3" "github.com/erigontech/erigon/execution/engineapi/engine_types" - "github.com/erigontech/erigon/execution/types" ) var ( @@ -104,8 +104,8 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { defer ticker.Stop() var ( - primedSlot uint64 - primedHead common.Hash + primed slotHead + notOurs slotHead lastFailureLog time.Time ) for immediate := true; ; immediate = false { @@ -128,11 +128,21 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if preparationRetired(stateVersion) { return } - // Before genesis the current slot clamps to zero, so the next slot can be arbitrarily - // far off. A builder primed that early freezes at its own cap long before the slot and - // production would then reuse the stale payload, so leave those slots unprimed. + if !shouldPreparePayloadVersion(stateVersion) { + continue + } + // On consecutive proposals the prime for the next slot lands inside this one, where it + // would compete for the execution layer with the block being produced right now. + if a.proposalsInFlight.Load() > 0 { + continue + } + // Before genesis the current slot clamps to zero, so the next slot can be arbitrarily far + // off; a builder primed that early hits its own cap before the slot even starts. Too late + // and the prime can never reach the age production demands of it, so the state copy and the + // forkchoice update would both be spent for nothing. slotStart := a.ethClock.GetSlotTime(targetSlot) - if time.Until(slotStart) > maxPreparationLead(a.beaconChainCfg) { + lead := time.Until(slotStart) + if lead > maxPreparationLead(a.beaconChainCfg) || lead < preparedPayloadMinimumAge(a.beaconChainCfg, stateVersion) { continue } selectedRoot, _, selected := a.syncedData.SelectedHead() @@ -146,34 +156,43 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if selectedRoot != a.syncedData.HeadRoot() { continue } - if !shouldPrepare(targetSlot, primedSlot, selectedRoot, primedHead) { - continue - } - if !shouldPreparePayloadVersion(stateVersion) { + // Both verdicts hold until the slot or its parent head moves. Re-deriving the proposer costs + // a state copy, and an epoch transition on top when the slot is in the next epoch. + if primed.is(targetSlot, selectedRoot) || notOurs.is(targetSlot, selectedRoot) { continue } prepareCtx, cancel := context.WithDeadline(ctx, slotStart) head, err := a.preparePayloadFor(prepareCtx, targetSlot) cancel() if err != nil { + if errors.Is(err, errNotOurProposal) { + notOurs = slotHead{slot: targetSlot, head: selectedRoot} + } if !isExpectedPreparationSkip(err) && time.Since(lastFailureLog) >= time.Minute { log.Warn("PayloadPreparation: failed", "slot", targetSlot, "err", err) lastFailureLog = time.Now() } continue } - primedSlot, primedHead = targetSlot, head + primed = slotHead{slot: targetSlot, head: head} } } -// shouldPrepare requires a fresh builder after the target slot or its parent head changes. -func shouldPrepare(targetSlot, primedSlot uint64, head, primedHead common.Hash) bool { - return targetSlot != primedSlot || head != primedHead +// slotHead records work already settled for a target slot on a given head, so a later tick can skip +// repeating it. A zero value matches nothing reachable: slot zero is never primed. +type slotHead struct { + slot uint64 + head common.Hash } -// maxPreparationLead bounds how far ahead of a slot priming is worthwhile. +func (s slotHead) is(slot uint64, head common.Hash) bool { + return s.slot == slot && s.head == head +} + +// maxPreparationLead bounds how far ahead of a slot priming is worthwhile. One slot is all a live +// chain ever offers, since preparation only ever targets the slot after the current one. func maxPreparationLead(cfg *clparams.BeaconChainConfig) time.Duration { - return 2 * time.Duration(cfg.SecondsPerSlot) * time.Second + return time.Duration(cfg.SecondsPerSlot) * time.Second } // preparationRetired is the single authority for the fork after which builders gossip bids @@ -194,6 +213,7 @@ func isExpectedPreparationSkip(err error) bool { errors.Is(err, errHeadTooFarBack) || errors.Is(err, errPreparationHeadChanged) || errors.Is(err, execution_client.ErrForkChoiceNotAdopted) || + errors.Is(err, execution_client.ErrForkChoiceBusy) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) || errors.Is(err, synced_data.ErrNotSynced) @@ -252,7 +272,7 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( } stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) - head, safeHash, finalizedHash, attrs, err := a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient, stateVersion) + head, safeHash, finalizedHash, attrs, err := a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) if err != nil { return common.Hash{}, err } @@ -272,16 +292,9 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( return baseBlockRoot, nil } -// preGloasForkChoiceInputs builds the shared preparation and production inputs. -func (a *ApiHandler) preGloasForkChoiceInputs( - baseState *state.CachingBeaconState, - baseBlockRoot common.Hash, - targetSlot uint64, - feeRecipient common.Address, - stateVersion clparams.StateVersion, -) (head, safeHash, finalizedHash common.Hash, attrs *engine_types.PayloadAttributes, err error) { - head = baseState.LatestExecutionPayloadHeader().BlockHash - +// executionCheckpointHashes resolves the safe and finalized execution hashes, falling back to the +// head for a checkpoint whose execution block this node has not seen yet. +func (a *ApiHandler) executionCheckpointHashes(baseState *state.CachingBeaconState, head common.Hash) (safeHash, finalizedHash common.Hash) { finalizedHash = a.forkchoiceStore.GetFinalizedExecutionHash(baseState.FinalizedCheckpoint().Root) if finalizedHash == (common.Hash{}) { finalizedHash = head @@ -290,34 +303,31 @@ func (a *ApiHandler) preGloasForkChoiceInputs( if safeHash == (common.Hash{}) { safeHash = head } + return safeHash, finalizedHash +} + +// preGloasForkChoiceInputs builds the shared preparation and production inputs. +func (a *ApiHandler) preGloasForkChoiceInputs( + baseState *state.CachingBeaconState, + baseBlockRoot common.Hash, + targetSlot uint64, + feeRecipient common.Address, +) (head, safeHash, finalizedHash common.Hash, attrs *engine_types.PayloadAttributes, err error) { + head = baseState.LatestExecutionPayloadHeader().BlockHash + safeHash, finalizedHash = a.executionCheckpointHashes(baseState, head) epoch := targetSlot / a.beaconChainCfg.SlotsPerEpoch - var withdrawals []*types.Withdrawal - if stateVersion.AfterOrEqual(clparams.CapellaVersion) { - clWithdrawals, err := state.GetExpectedWithdrawals(baseState, epoch) - if err != nil { - return head, safeHash, finalizedHash, nil, err - } - withdrawals = make([]*types.Withdrawal, 0, len(clWithdrawals.Withdrawals)) - for _, w := range clWithdrawals.Withdrawals { - withdrawals = append(withdrawals, &types.Withdrawal{ - Index: w.Index, - Amount: w.Amount, - Validator: w.Validator, - Address: w.Address, - }) - } + clWithdrawals, err := state.GetExpectedWithdrawals(baseState, epoch) + if err != nil { + return head, safeHash, finalizedHash, nil, err } - attrs = payloadAttributesForVersion( - stateVersion, + attrs = payloadAttributes( hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), baseState.GetRandaoMixes(epoch), feeRecipient, - withdrawals, + cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(clWithdrawals.Withdrawals), &baseBlockRoot, - nil, - nil, ) return head, safeHash, finalizedHash, attrs, nil } diff --git a/cl/cltypes/withdrawal.go b/cl/cltypes/withdrawal.go index 40447af3601..d20bdb7cb71 100644 --- a/cl/cltypes/withdrawal.go +++ b/cl/cltypes/withdrawal.go @@ -87,6 +87,16 @@ func convertExecutionWithdrawalsToConsensusWithdrawals(executionWithdrawal []*ty return ret } +// ConvertConsensusWithdrawalsToExecutionWithdrawals is the single crossing point between the two +// representations, so payload attributes built on different code paths cannot drift apart. +func ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals []*Withdrawal) []*types.Withdrawal { + ret := make([]*types.Withdrawal, len(consensusWithdrawals)) + for i, w := range consensusWithdrawals { + ret[i] = convertConsensusWithdrawalToExecutionWithdrawal(w) + } + return ret +} + // ExpectedWithdrawals represents the expected withdrawals for a beacon state type ExpectedWithdrawals struct { Withdrawals []*Withdrawal `json:"withdrawals"` diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index 90772c26be6..78de8f7266a 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -29,6 +29,7 @@ import ( "github.com/erigontech/erigon/cl/monitor" "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/hexutil" + "github.com/erigontech/erigon/common/log/v3" "github.com/erigontech/erigon/execution/engineapi/engine_types" "github.com/erigontech/erigon/execution/execmodule" "github.com/erigontech/erigon/execution/execmodule/chainreader" @@ -130,10 +131,24 @@ func (cc *ExecutionClientDirect) NewPayload( // there is nothing to build on. var ErrForkChoiceNotAdopted = errors.New("execution layer did not adopt forkchoice head") -const ( - forkChoiceBusyAttempts = 30 - forkChoiceBusyDelay = 200 * time.Millisecond -) +// ErrForkChoiceBusy reports contention rather than rejection: the update continues on the module's +// own context. Retrying is the caller's decision because only the caller knows whether the head it +// asked for is still the one it wants. +var ErrForkChoiceBusy = errors.New("execution layer busy with a forkchoice update") + +// forkChoiceStatusError classifies a status reached with payload attributes attached. Only an +// adopted head is safe to build on: a builder ignores the parent it was asked for and packs on top +// of whatever the execution tip really is. +func forkChoiceStatusError(status execmodule.ExecutionStatus) error { + switch status { + case execmodule.ExecutionStatusSuccess: + return nil + case execmodule.ExecutionStatusBusy: + return ErrForkChoiceBusy + default: + return fmt.Errorf("%w: status %d", ErrForkChoiceNotAdopted, status) + } +} func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized, safe, head common.Hash, attr *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { status, _, _, err := cc.chainRW.UpdateForkChoice(ctx, head, safe, finalized) @@ -147,23 +162,14 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized return nil, errors.New("bad block as forkchoice") } if attr == nil { + if status == execmodule.ExecutionStatusBusy { + log.Warn("[ForkChoiceUpdated] execution layer was busy, head not applied", "head", head) + } return nil, nil } - // Only assemble once the execution layer has actually adopted the head. Building on a head it - // never took pins a payload id that can never be served, because the timestamp dedup then hands - // that dead id back for the rest of the slot. Busy is transient - the update continues on the - // module's own context - so wait for it to settle rather than giving up on the slot. - status, err = awaitForkChoiceAdopted(ctx, status, forkChoiceBusyAttempts, forkChoiceBusyDelay, - func(ctx context.Context) (execmodule.ExecutionStatus, error) { - retried, _, _, retryErr := cc.chainRW.UpdateForkChoice(ctx, head, safe, finalized) - return retried, retryErr - }) - if err != nil { + if err := forkChoiceStatusError(status); err != nil { return nil, err } - if status != execmodule.ExecutionStatusSuccess { - return nil, fmt.Errorf("%w: status %d", ErrForkChoiceNotAdopted, status) - } // Retry AssembleBlock if the EL is busy (semaphore contention with // fork choice commits). This is common in single-process dev mode // where the CL and EL share the same process. @@ -178,31 +184,6 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized return idBytes, nil } -// awaitForkChoiceAdopted retries while the execution layer reports contention. The update itself -// continues on the module's own context, so a later attempt observes it settle instead of starting -// fresh work; giving up immediately would abandon a proposal over a transient condition. -func awaitForkChoiceAdopted( - ctx context.Context, - status execmodule.ExecutionStatus, - attempts int, - delay time.Duration, - update func(context.Context) (execmodule.ExecutionStatus, error), -) (execmodule.ExecutionStatus, error) { - for attempt := 0; status == execmodule.ExecutionStatusBusy && attempt < attempts; attempt++ { - select { - case <-ctx.Done(): - return status, ctx.Err() - case <-time.After(delay): - } - retried, err := update(ctx) - if err != nil { - return status, fmt.Errorf("execution Client RPC failed to retrieve ForkChoiceUpdate response, err: %w", err) - } - status = retried - } - return status, nil -} - func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, assemble func(context.Context) (uint64, error)) (uint64, error) { if attempts <= 0 { return 0, errors.New("assemble block requires at least one attempt") @@ -218,6 +199,11 @@ func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, if id, err = assemble(ctx); err == nil { return id, nil } + // Only contention settles by waiting. A rejected request returns the same answer however + // many times it is asked, and retrying it burns the slot instead of reporting it. + if !errors.Is(err, chainreader.ErrExecutionBusy) { + return 0, err + } if attempt+1 == attempts { break } @@ -280,8 +266,8 @@ func (cc *ExecutionClientDirect) HasBlock(ctx context.Context, hash common.Hash) return cc.chainRW.HasBlock(ctx, hash) } -func (cc *ExecutionClientDirect) GetAssembledBlock(_ context.Context, idBytes []byte, _ clparams.StateVersion) (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { - return cc.chainRW.GetAssembledBlock(binary.LittleEndian.Uint64(idBytes)) +func (cc *ExecutionClientDirect) GetAssembledBlock(ctx context.Context, idBytes []byte, _ clparams.StateVersion) (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { + return cc.chainRW.GetAssembledBlock(ctx, binary.LittleEndian.Uint64(idBytes)) } func (cc *ExecutionClientDirect) HasGapInSnapshots(ctx context.Context) bool { diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go index c5413d0ad5b..95dcbc5d8c4 100644 --- a/cl/phase1/execution_client/execution_client_direct_test.go +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/require" "github.com/erigontech/erigon/execution/execmodule" + "github.com/erigontech/erigon/execution/execmodule/chainreader" ) func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { @@ -33,7 +34,7 @@ func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { _, err := retryAssembleBlock(ctx, 30, time.Hour, func(context.Context) (uint64, error) { calls++ cancel() - return 0, errors.New("busy") + return 0, chainreader.ErrExecutionBusy }) require.ErrorIs(t, err, context.Canceled) @@ -46,19 +47,32 @@ func TestRetryAssembleBlockDoesNotStartWithCanceledContext(t *testing.T) { calls := 0 _, err := retryAssembleBlock(ctx, 30, time.Hour, func(context.Context) (uint64, error) { calls++ - return 0, errors.New("busy") + return 0, chainreader.ErrExecutionBusy }) require.ErrorIs(t, err, context.Canceled) require.Zero(t, calls) } +func TestRetryAssembleBlockStopsOnPermanentError(t *testing.T) { + rejected := errors.New("withdrawals before shanghai") + calls := 0 + _, err := retryAssembleBlock(t.Context(), 30, time.Hour, func(context.Context) (uint64, error) { + calls++ + return 0, rejected + }) + + // A rejection answers the same way however often it is asked, so retrying it only burns the slot. + require.ErrorIs(t, err, rejected) + require.Equal(t, 1, calls) +} + func TestRetryAssembleBlockReturnsFirstSuccess(t *testing.T) { calls := 0 id, err := retryAssembleBlock(t.Context(), 3, time.Millisecond, func(context.Context) (uint64, error) { calls++ if calls < 3 { - return 0, errors.New("busy") + return 0, chainreader.ErrExecutionBusy } return 7, nil }) @@ -75,59 +89,19 @@ func TestRetryAssembleBlockRejectsNoAttempts(t *testing.T) { require.EqualError(t, err, "assemble block requires at least one attempt") } -func TestAwaitForkChoiceAdoptedPassesThroughSettledStatus(t *testing.T) { - calls := 0 - status, err := awaitForkChoiceAdopted(t.Context(), execmodule.ExecutionStatusSuccess, 30, time.Hour, - func(context.Context) (execmodule.ExecutionStatus, error) { - calls++ - return execmodule.ExecutionStatusSuccess, nil - }) - - require.NoError(t, err) - require.Equal(t, execmodule.ExecutionStatusSuccess, status) - require.Zero(t, calls, "a settled status must not be re-sent") -} - -func TestAwaitForkChoiceAdoptedWaitsForBusyToSettle(t *testing.T) { - calls := 0 - status, err := awaitForkChoiceAdopted(t.Context(), execmodule.ExecutionStatusBusy, 30, time.Millisecond, - func(context.Context) (execmodule.ExecutionStatus, error) { - calls++ - if calls < 3 { - return execmodule.ExecutionStatusBusy, nil - } - return execmodule.ExecutionStatusSuccess, nil - }) - - require.NoError(t, err) - require.Equal(t, execmodule.ExecutionStatusSuccess, status) - require.Equal(t, 3, calls) -} - -func TestAwaitForkChoiceAdoptedGivesUpAfterAttempts(t *testing.T) { - calls := 0 - status, err := awaitForkChoiceAdopted(t.Context(), execmodule.ExecutionStatusBusy, 2, time.Millisecond, - func(context.Context) (execmodule.ExecutionStatus, error) { - calls++ - return execmodule.ExecutionStatusBusy, nil - }) - - // Still Busy, so the caller reports the head was never adopted rather than assembling on it. - require.NoError(t, err) - require.Equal(t, execmodule.ExecutionStatusBusy, status) - require.Equal(t, 2, calls) -} - -func TestAwaitForkChoiceAdoptedStopsWhenContextIsCanceled(t *testing.T) { - ctx, cancel := context.WithCancel(t.Context()) - cancel() - calls := 0 - _, err := awaitForkChoiceAdopted(ctx, execmodule.ExecutionStatusBusy, 30, time.Hour, - func(context.Context) (execmodule.ExecutionStatus, error) { - calls++ - return execmodule.ExecutionStatusSuccess, nil +func TestForkChoiceStatusErrors(t *testing.T) { + for _, tc := range []struct { + name string + status execmodule.ExecutionStatus + want error + }{ + {"busy is contention, not rejection", execmodule.ExecutionStatusBusy, ErrForkChoiceBusy}, + {"too far away", execmodule.ExecutionStatusTooFarAway, ErrForkChoiceNotAdopted}, + {"missing segment", execmodule.ExecutionStatusMissingSegment, ErrForkChoiceNotAdopted}, + } { + t.Run(tc.name, func(t *testing.T) { + require.ErrorIs(t, forkChoiceStatusError(tc.status), tc.want) }) - - require.ErrorIs(t, err, context.Canceled) - require.Zero(t, calls) + } + require.NoError(t, forkChoiceStatusError(execmodule.ExecutionStatusSuccess)) } diff --git a/cl/phase1/execution_client/execution_client_engine.go b/cl/phase1/execution_client/execution_client_engine.go index d3ef2ad26cc..3715a335f41 100644 --- a/cl/phase1/execution_client/execution_client_engine.go +++ b/cl/phase1/execution_client/execution_client_engine.go @@ -320,7 +320,7 @@ func (cc *ExecutionClientEngine) HasBlock(ctx context.Context, hash common.Hash) func (cc *ExecutionClientEngine) GetAssembledBlock(ctx context.Context, id []byte, version clparams.StateVersion) (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { if cc.isLocal() { - return cc.chainRW.GetAssembledBlock(binary.LittleEndian.Uint64(id)) + return cc.chainRW.GetAssembledBlock(ctx, binary.LittleEndian.Uint64(id)) } // GetPayload versions advance with the response fields introduced by each fork. diff --git a/cl/phase1/stages/forkchoice.go b/cl/phase1/stages/forkchoice.go index abfd0a9b6f7..b8651cbe1e2 100644 --- a/cl/phase1/stages/forkchoice.go +++ b/cl/phase1/stages/forkchoice.go @@ -14,6 +14,7 @@ import ( "github.com/erigontech/erigon/cl/beacon/beaconevents" "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/clparams" + "github.com/erigontech/erigon/cl/cltypes" "github.com/erigontech/erigon/cl/monitor" "github.com/erigontech/erigon/cl/monitor/shuffling_metrics" "github.com/erigontech/erigon/cl/persistence/beacon_indicies" @@ -30,7 +31,6 @@ import ( "github.com/erigontech/erigon/db/datadir" "github.com/erigontech/erigon/db/kv" "github.com/erigontech/erigon/execution/engineapi/engine_types" - "github.com/erigontech/erigon/execution/types" ) // computeAndNotifyServicesOfNewForkChoice calculates the new head of the fork choice and notifies relevant services. @@ -251,19 +251,11 @@ func emitNextPaylodAttributesEvent(cfg *Cfg, headSlot uint64, headRoot common.Ha log.Warn("failed to get proposer index", "err", err) return err } - withdrawals := []*types.Withdrawal{} expWithdrawals, err := state.GetExpectedWithdrawals(s, epoch) if err != nil { return err } - for _, w := range expWithdrawals.Withdrawals { - withdrawals = append(withdrawals, &types.Withdrawal{ - Amount: w.Amount, - Index: w.Index, - Validator: w.Validator, - Address: w.Address, - }) - } + withdrawals := cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(expWithdrawals.Withdrawals) payloadAttributes := engine_types.PayloadAttributes{ Timestamp: hexutil.Uint64(headPayloadHeader.Time + cfg.beaconCfg.SecondsPerSlot), PrevRandao: randaoMix, diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 6fca1ce5919..836b420621c 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -106,6 +106,23 @@ func (b *BlockBuilder) Cancel() { b.interrupt.Store(true) } +// Stale reports whether the builder can no longer improve on what it holds, because it was +// cancelled or finished with an error. A builder that filled its block is not stale: it simply +// has nothing left to add. +func (b *BlockBuilder) Stale() bool { + if b.interrupt.Load() { + return true + } + select { + case <-b.done: + default: + return false + } + b.mu.Lock() + defer b.mu.Unlock() + return b.err != nil +} + func (b *BlockBuilder) Block() *types.Block { b.mu.Lock() defer b.mu.Unlock() diff --git a/execution/builder/block_builder_test.go b/execution/builder/block_builder_test.go new file mode 100644 index 00000000000..f44150f8d32 --- /dev/null +++ b/execution/builder/block_builder_test.go @@ -0,0 +1,79 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package builder + +import ( + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/execution/types" +) + +func TestBlockBuilderRunningIsNotStale(t *testing.T) { + t.Parallel() + + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + <-release + return nil, errors.New("builder stopped") + }, &Parameters{}, time.Minute) + + require.Never(t, b.Stale, 50*time.Millisecond, 5*time.Millisecond) +} + +func TestBlockBuilderIsStaleOnceCancelled(t *testing.T) { + t.Parallel() + + b := NewBlockBuilder(func(_ *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }, &Parameters{}, time.Minute) + + b.Cancel() + require.True(t, b.Stale()) +} + +func TestBlockBuilderIsStaleOnceItFails(t *testing.T) { + t.Parallel() + + b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + return nil, errors.New("build failed") + }, &Parameters{}, time.Minute) + + require.Eventually(t, b.Stale, time.Second, time.Millisecond) +} + +func TestBlockBuilderStaysReusableOnceItFillsTheBlock(t *testing.T) { + t.Parallel() + + built := make(chan struct{}) + b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + defer close(built) + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }, &Parameters{}, time.Minute) + + <-built + // A builder that ran out of room holds a complete payload, so its id is still worth reusing. + require.Never(t, b.Stale, 50*time.Millisecond, 5*time.Millisecond) +} diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 87f079e5251..e32e5dd281b 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -95,6 +95,13 @@ type builderEntry struct { timestamp uint64 } +func (e *ExecModule) dropBuilder(id uint64, entry *builderEntry) { + if e.buildersByTimestamp[entry.timestamp] == id { + delete(e.buildersByTimestamp, entry.timestamp) + } + delete(e.builders, id) +} + func (e *ExecModule) evictOldBuilders() { ids := common.SortedKeys(e.builders) @@ -129,18 +136,22 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete } if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { - params.PayloadId = previousID - if previous := e.builders[previousID]; previous != nil && reflect.DeepEqual(previous.params, params) { - e.logger.Info("[ForkChoiceUpdated] duplicate build request") - return AssembleBlockResult{PayloadID: previousID}, nil - } - if previous := e.builders[previousID]; previous != nil && previous.builder != nil { + // A stale builder can never grow its payload, so reusing its id would spend the slot + // waiting on a block that will not arrive. Take a fresh builder instead and leave the + // old id answerable for whoever already holds it. + if previous := e.builders[previousID]; previous != nil && previous.builder != nil && !previous.builder.Stale() { + params.PayloadId = previousID + if reflect.DeepEqual(previous.params, params) { + e.logger.Info("[ForkChoiceUpdated] duplicate build request") + return AssembleBlockResult{PayloadID: previousID}, nil + } + // Superseding must not outlive the caller that asked for it: cancelling on behalf of + // an expired request would freeze a builder the next request is already waiting on. + if err := ctx.Err(); err != nil { + return AssembleBlockResult{}, err + } previous.builder.Cancel() } - // Cancel freezes a builder where it stands, so a superseded id must stop being - // retrievable: otherwise GetAssembledBlock hands back whatever it had packed at - // that instant, which is near-empty when the supersede came early. - delete(e.builders, previousID) } // Initiate payload building @@ -190,11 +201,16 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A defer e.semaphore.Release(1) entry, ok := e.builders[payloadID] - if !ok || entry.builder == nil { + if !ok || entry == nil || entry.builder == nil { return AssembledBlockResult{}, nil } blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { + // A failed builder latches its error, so keeping the entry would hand the same failure to + // every retry. A caller whose own context expired says nothing about the builder. + if ctx.Err() == nil { + e.dropBuilder(payloadID, entry) + } e.logger.Error("Failed to build PoS block", "err", err) return AssembledBlockResult{}, err } diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index fc2610eca5e..d0f1d332088 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -120,28 +120,21 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { require.NotContains(t, module.builders, adjacentID) } -func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { +func TestSupersededPayloadIDStaysRetrievable(t *testing.T) { started := make(chan struct{}, 4) - release := make(chan struct{}) module := &ExecModule{ logger: log.Root(), config: &chain.Config{}, semaphore: semaphore.NewWeighted(1), builders: map[uint64]*builderEntry{}, - builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- struct{}{} - <-release - return nil, nil + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil }, } - t.Cleanup(func() { - close(release) - for _, entry := range module.builders { - if entry != nil && entry.builder != nil { - _, _ = entry.builder.Stop(context.Background()) - } - } - }) first, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) require.NoError(t, err) @@ -150,15 +143,76 @@ func TestSupersededPayloadIDStopsBeingRetrievable(t *testing.T) { second, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x02}}) require.NoError(t, err) require.NotEqual(t, first.PayloadID, second.PayloadID) + <-started - // A cancelled builder still holds whatever it had packed; serving that would mean - // proposing a near-empty block, so the superseded id must read as unknown instead. - require.NotContains(t, module.builders, first.PayloadID) - require.NotContains(t, module.builders, first.PayloadID) - + // Superseding hands the timestamp to a fresh builder, but an id already returned to a caller + // stays answerable: dropping it would turn a getPayload into an unknown-payload error. assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) require.NoError(t, err) - require.Nil(t, assembled.Block) + require.NotNil(t, assembled.Block) + + _, _ = module.builders[second.PayloadID].builder.Stop(context.Background()) +} + +func TestAssembleBlockDoesNotReuseFailedBuilder(t *testing.T) { + var failNext atomic.Bool + failNext.Store(true) + started := make(chan struct{}, 4) + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- struct{}{} + if failNext.Swap(false) { + return nil, errors.New("build failed") + } + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }, + } + + params := func() *builder.Parameters { + return &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}} + } + first, err := module.AssembleBlock(t.Context(), params()) + require.NoError(t, err) + <-started + require.Eventually(t, module.builders[first.PayloadID].builder.Stale, time.Second, time.Millisecond) + + // Identical parameters would normally dedup onto the same id. A builder that already died + // latches its error, so reusing it would spend the slot on a payload that can never arrive. + second, err := module.AssembleBlock(t.Context(), params()) + require.NoError(t, err) + require.NotEqual(t, first.PayloadID, second.PayloadID) + <-started + + _, _ = module.builders[second.PayloadID].builder.Stop(context.Background()) +} + +func TestGetAssembledBlockDropsFailedBuilder(t *testing.T) { + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + builderFunc: func(_ *builder.Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + return nil, errors.New("build failed") + }, + } + + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + + _, err = module.GetAssembledBlock(t.Context(), result.PayloadID) + require.Error(t, err) + + // The error is latched, so leaving the entry in place would keep serving it to every retry. + require.NotContains(t, module.builders, result.PayloadID) + require.NotContains(t, module.buildersByTimestamp, uint64(100)) } func TestAssembleBlockOwnsParameters(t *testing.T) { diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index 88231210e9e..162a7da5c8a 100644 --- a/execution/execmodule/chainreader/chain_reader.go +++ b/execution/execmodule/chainreader/chain_reader.go @@ -279,6 +279,10 @@ func (c ChainReaderWriterEth1) HasBlock(ctx context.Context, hash common.Hash) ( return c.executionModule.HasBlock(ctx, &hash, nil) } +// ErrExecutionBusy separates contention, which settles on its own, from a rejection that will +// return the same answer however many times it is asked. +var ErrExecutionBusy = errors.New("execution data is still syncing") + func (c ChainReaderWriterEth1) AssembleBlock(ctx context.Context, baseHash common.Hash, attributes *engine_types.PayloadAttributes) (id uint64, err error) { params := &builder.Parameters{ ParentHash: baseHash, @@ -295,18 +299,18 @@ func (c ChainReaderWriterEth1) AssembleBlock(ctx context.Context, baseHash commo return 0, err } if result.Busy { - return 0, errors.New("execution data is still syncing") + return 0, ErrExecutionBusy } return result.PayloadID, nil } -func (c ChainReaderWriterEth1) GetAssembledBlock(id uint64) (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { - result, err := c.executionModule.GetAssembledBlock(context.Background(), id) +func (c ChainReaderWriterEth1) GetAssembledBlock(ctx context.Context, id uint64) (*cltypes.Eth1Block, *engine_types.BlobsBundle, *typesproto.RequestsBundle, *big.Int, error) { + result, err := c.executionModule.GetAssembledBlock(ctx, id) if err != nil { return nil, nil, nil, nil, err } if result.Busy { - return nil, nil, nil, nil, errors.New("execution data is still syncing") + return nil, nil, nil, nil, ErrExecutionBusy } if result.Block == nil { return nil, nil, nil, nil, nil diff --git a/execution/execmodule/exec_module_test.go b/execution/execmodule/exec_module_test.go index 76c4272935f..b1120584f2a 100644 --- a/execution/execmodule/exec_module_test.go +++ b/execution/execmodule/exec_module_test.go @@ -1276,7 +1276,7 @@ func TestAssembleBlockWithWithdrawalRequest(t *testing.T) { time.Hour, ) - eth1Block, blobsBundle, requestsBundle, blockValue, err := chainRW.GetAssembledBlock(payloadId) + eth1Block, blobsBundle, requestsBundle, blockValue, err := chainRW.GetAssembledBlock(ctx, payloadId) require.NoError(t, err) require.NotNil(t, eth1Block, "Eth1Block should not be nil") require.NotNil(t, blobsBundle, "BlobsBundle should not be nil") From 8a351e377f82f82ecdf2cdc7daf58648c283afb0 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Thu, 13 Aug 2026 15:06:20 +0200 Subject: [PATCH 20/20] cl/beacon, cl/phase1, cl/validator, execution: address the third consolidated review High - Stop reading a collected builder as dead. Cancellation is not failure: a stopped builder holds the payload it was stopped for, and a byte-identical repeat has to be handed that payload rather than starting a fresh build whose first grab lands near-empty. Only a builder that finished with an error is passed over, so Stale becomes Failed. - Leave a superseded builder running to its own deadline, as before this PR. The timestamp index already moves to the replacement, so nothing reaches the old one by deduplication, while an id already handed out keeps answering with a payload that is still growing. - Bound acquiring a payload id by the moment the payload has to be collected, so neither the contention retries nor the assemble retry below them can eat the publication margin, and keep one attempt past that bound so a late request is answered rather than failed. Medium - Start preparation at Capella. Payload attributes always carry withdrawals, which the execution layer rejects before Shanghai, so priming earlier could only ever fail once a minute. - Key the primed and not-ours verdicts to a validator registration generation, so a registration arriving after the loop started - or a fee recipient changing under a prime already sent - is not ignored until the head moves. - Re-check the lead after the state copy and the epoch transition, which are slow enough to have spent it, instead of recording a prime production would reject on age. - Skip everything before the first registration, so a non-validating node never reaches the copy. - Retry a busy forkchoice update inside the prime rather than dropping the whole attempt: the head view, the copy and the transition are far more expensive to repeat than the update. The head is re-checked before every attempt. - Replace a test that named produceBeaconBody but only exercised its own mock. Low - Only report a polling window that never produced anything. - Describe Busy as what it is: the execution layer either declined the update or is still running it, and from the consensus layer the two are indistinguishable. - Treat execution contention during a prime as an expected skip. - Branch once per fork when building forkchoice inputs, so production cannot derive its own variant of the values preparation primed with. - State each rationale once at its canonical site. --- cl/beacon/handler/block_production.go | 97 +++++++++++-------- cl/beacon/handler/block_production_test.go | 83 ++++++++++------ cl/beacon/handler/payload_preparation.go | 83 +++++++++++----- .../execution_client_direct.go | 11 +-- .../execution_client_direct_test.go | 1 - .../validator_params/validator_params.go | 15 ++- execution/builder/block_builder.go | 11 +-- execution/builder/block_builder_test.go | 22 +++-- execution/execmodule/block_building.go | 22 ++--- .../block_building_internal_test.go | 79 +++++++++++---- 10 files changed, 276 insertions(+), 148 deletions(-) diff --git a/cl/beacon/handler/block_production.go b/cl/beacon/handler/block_production.go index 6db7d633368..8fd483500d0 100644 --- a/cl/beacon/handler/block_production.go +++ b/cl/beacon/handler/block_production.go @@ -250,9 +250,11 @@ func preparedPayloadMinimumAge(cfg *clparams.BeaconChainConfig, stateVersion clp const forkChoiceBusyRetryDelay = 100 * time.Millisecond -// forkChoiceUpdateForProposal keeps asking while the execution layer reports contention, but only -// until the payload would have to be collected: an id obtained after that is no use to this slot. -// The head is not re-read because a proposal is committed to the parent it is being built on. +// forkChoiceUpdateForProposal bounds acquiring a payload id by the moment the payload has to be +// collected, so neither the retries here nor the assemble retry below them can eat the margin the +// block needs to reach attesters. One attempt always runs past that bound, because failing a +// proposal outright is worse than answering late. The head is not re-read between attempts: a +// proposal is committed to the parent it is being built on. func (a *ApiHandler) forkChoiceUpdateForProposal( ctx context.Context, targetSlot uint64, @@ -260,11 +262,19 @@ func (a *ApiHandler) forkChoiceUpdateForProposal( attrs *engine_types.PayloadAttributes, stateVersion clparams.StateVersion, ) ([]byte, error) { - retryUntil := a.ethClock.GetSlotTime(targetSlot).Add(unpreparedGrabOffset(attestationDue(a.beaconChainCfg, stateVersion))) - for { - idBytes, err := a.engine.ForkChoiceUpdate(ctx, finalized, safe, head, attrs, stateVersion) - if !errors.Is(err, execution_client.ErrForkChoiceBusy) || !time.Now().Before(retryUntil) { - return idBytes, err + collectAt := a.ethClock.GetSlotTime(targetSlot).Add(unpreparedGrabOffset(attestationDue(a.beaconChainCfg, stateVersion))) + boundedCtx, cancel := context.WithDeadline(ctx, collectAt) + defer cancel() + + for time.Now().Before(collectAt) { + idBytes, err := a.engine.ForkChoiceUpdate(boundedCtx, finalized, safe, head, attrs, stateVersion) + switch { + case err == nil: + return idBytes, nil + case ctx.Err() != nil: + return nil, ctx.Err() + case !errors.Is(err, execution_client.ErrForkChoiceBusy) && !errors.Is(err, context.DeadlineExceeded): + return nil, err } select { case <-ctx.Done(): @@ -272,6 +282,7 @@ func (a *ApiHandler) forkChoiceUpdateForProposal( case <-time.After(forkChoiceBusyRetryDelay): } } + return a.engine.ForkChoiceUpdate(ctx, finalized, safe, head, attrs, stateVersion) } // payloadAttributes builds the attributes preparation and production must agree on. Withdrawals and @@ -294,6 +305,27 @@ func payloadAttributes( } } +// gloasWithdrawals reads the withdrawals for a Gloas proposal from the state copy carrying the +// parent payload when the head is FULL, and from the cached expectation when it is EMPTY. +func gloasWithdrawals(baseState, withParentPayload *state.CachingBeaconState, epoch uint64) ([]*types.Withdrawal, error) { + if withParentPayload != nil { + clWithdrawals, err := state.GetExpectedWithdrawals(withParentPayload, epoch) + if err != nil { + return nil, err + } + return cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(clWithdrawals.Withdrawals), nil + } + cached := baseState.GetPayloadExpectedWithdrawals() + if cached == nil { + return nil, nil + } + consensusWithdrawals := make([]*cltypes.Withdrawal, cached.Len()) + for i := range consensusWithdrawals { + consensusWithdrawals[i] = cached.Get(i) + } + return cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals), nil +} + func shouldRetryGetPayload(now, deadline time.Time) bool { return now.Before(deadline) } @@ -319,14 +351,16 @@ func pollAssembledPayload( defer deadlineTimer.Stop() retryTicker := time.NewTicker(retryTime) defer retryTicker.Stop() - // A slot that fails fails on every poll of the window, so report the first and then a count - // rather than hundreds of copies of one line. + // A slot that fails tends to fail on every poll of the window, so report the first and then a + // count rather than hundreds of copies of one line. Contention that clears is not worth an + // alert, so the summary is only for a window that never produced anything. var ( - failures int - lastErr error + failures int + lastErr error + collected bool ) defer func() { - if failures > 1 { + if failures > 1 && !collected { log.Error("BlockProduction: payload polling kept failing", "attempts", failures, "err", lastErr) } }() @@ -340,6 +374,7 @@ func pollAssembledPayload( log.Error("BlockProduction: Failed to get payload", "err", err) } } else if payload != nil { + collected = true return payload, bundles, requestsBundle, blockValue, true } select { @@ -1056,8 +1091,12 @@ func (a *ApiHandler) produceBeaconBody( log.Warn("BlockProduction: no fee recipient registered for proposer, using zero address", "proposer", proposerIndex) } - fcuHead, fcuSafeHash, fcuFinalizedHash := head, safeHash, finalizedHash - var attrs *engine_types.PayloadAttributes + // One branch per fork: the pre-Gloas inputs are the same ones preparation builds, and the + // prepared-id match depends on production not deriving its own variant of them. + var ( + fcuHead, fcuSafeHash, fcuFinalizedHash common.Hash + attrs *engine_types.PayloadAttributes + ) if stateVersion.Before(clparams.GloasVersion) { var err error fcuHead, fcuSafeHash, fcuFinalizedHash, attrs, err = a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) @@ -1065,33 +1104,13 @@ func (a *ApiHandler) produceBeaconBody( log.Error("BlockProduction: build forkchoice inputs failed", "err", err) return } - } - var withdrawals []*types.Withdrawal - switch { - case gloasWithdrawalsState != nil: - // GLOAS FULL: compute withdrawals from the state copy with parent payload applied - clWithdrawals, err := state.GetExpectedWithdrawals( - gloasWithdrawalsState, - targetSlot/a.beaconChainCfg.SlotsPerEpoch, - ) + } else { + fcuHead, fcuSafeHash, fcuFinalizedHash = head, safeHash, finalizedHash + withdrawals, err := gloasWithdrawals(baseState, gloasWithdrawalsState, targetSlot/a.beaconChainCfg.SlotsPerEpoch) if err != nil { - log.Error("BlockProduction: GetExpectedWithdrawals (FULL) failed", "err", err) + log.Error("BlockProduction: GetExpectedWithdrawals failed", "err", err) return } - withdrawals = cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(clWithdrawals.Withdrawals) - case stateVersion >= clparams.GloasVersion && gloasWithdrawalsState == nil: - // GLOAS EMPTY: use cached payload_expected_withdrawals from state - cachedWithdrawals := baseState.GetPayloadExpectedWithdrawals() - if cachedWithdrawals != nil { - consensusWithdrawals := make([]*cltypes.Withdrawal, cachedWithdrawals.Len()) - for i := range consensusWithdrawals { - consensusWithdrawals[i] = cachedWithdrawals.Get(i) - } - withdrawals = cltypes.ConvertConsensusWithdrawalsToExecutionWithdrawals(consensusWithdrawals) - } - } - - if stateVersion.AfterOrEqual(clparams.GloasVersion) { attrs = payloadAttributes( hexutil.Uint64(state.ComputeTimestampAtSlot(baseState, targetSlot)), random, diff --git a/cl/beacon/handler/block_production_test.go b/cl/beacon/handler/block_production_test.go index c595c10cfc6..07f09113f0b 100644 --- a/cl/beacon/handler/block_production_test.go +++ b/cl/beacon/handler/block_production_test.go @@ -874,7 +874,7 @@ func TestPreparePayloadLoopRunsImmediatelyWithSlotDeadline(t *testing.T) { slotStart := time.Now().Add(6 * time.Second) clock := eth_clock.NewMockEthereumClock(ctrl) clock.EXPECT().GetCurrentSlot().Return(targetSlot - 1) - clock.EXPECT().GetSlotTime(targetSlot).Return(slotStart) + clock.EXPECT().GetSlotTime(targetSlot).Return(slotStart).AnyTimes() handler.ethClock = clock ctx, cancel := context.WithCancel(t.Context()) @@ -1079,6 +1079,10 @@ func TestPreparePayloadForSendsCompleteForkChoiceUpdate(t *testing.T) { }) handler.engine = engine + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetSlotTime(gomock.Any()).Return(time.Now().Add(6 * time.Second)).AnyTimes() + handler.ethClock = clock + primedHead, err := handler.preparePayloadFor(t.Context(), targetSlot) require.NoError(t, err) require.Equal(t, baseBlockRoot, primedHead) @@ -1107,6 +1111,10 @@ func TestPreparePayloadForRejectsChangedHeadBeforeForkChoiceUpdate(t *testing.T) engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) handler.engine = engine + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetSlotTime(gomock.Any()).Return(time.Now().Add(6 * time.Second)).AnyTimes() + handler.ethClock = clock + _, err = handler.preparePayloadFor(t.Context(), targetSlot) require.ErrorIs(t, err, errPreparationHeadChanged) require.False(t, handler.preparedPayload.matches(targetSlot, []byte{1}, time.Now(), 0)) @@ -1171,33 +1179,49 @@ func TestPreparePayloadForUsesPostEpochProposer(t *testing.T) { }) handler.engine = engine + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetSlotTime(gomock.Any()).Return(time.Now().Add(6 * time.Second)).AnyTimes() + handler.ethClock = clock + _, err := handler.preparePayloadFor(t.Context(), targetSlot) require.NoError(t, err) } -func TestProduceBeaconBodyTakesRootAndStateFromOneView(t *testing.T) { - _, _, _, _, postState, _, _, syncedData, _, _ := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), false) - expectedRoot := common.Hash{0x41} +func TestPreparePayloadForPairsRootAndStateFromOneView(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, postState, handler, _, syncedData, _, validatorParams := setupTestingHandler(t, clparams.ElectraVersion, log.Root(), false) + targetSlot := postState.Slot() + 1 + proposerIndex, err := postState.GetBeaconProposerIndexForSlot(targetSlot) + require.NoError(t, err) + validatorParams.SetFeeRecipient(proposerIndex, common.Address{0x11}) + + // The root the view hands over is the only one preparation may use. Reading it separately would + // let a head update in between pair a parent beacon block root with a different state, priming a + // builder production can never match. + viewRoot := common.Hash{0x41} syncedDataMock := syncedData.(*sync_mock_services.MockSyncedData) syncedDataMock.EXPECT().ViewHeadStateWithIdentity(gomock.Any()). DoAndReturn(func(view synced_data.ViewHeadStateWithIdentityFn) error { - return view(postState, expectedRoot, postState.Slot()) + return view(postState, viewRoot, postState.Slot()) }) + syncedDataMock.EXPECT().SelectedHead().Return(viewRoot, postState.Slot(), true) - var ( - gotRoot common.Hash - copied *state.CachingBeaconState - ) - require.NoError(t, syncedData.ViewHeadStateWithIdentity( - func(headState *state.CachingBeaconState, root common.Hash, _ uint64) error { - var err error - gotRoot = root - copied, err = headState.Copy() - return err - })) - require.Equal(t, expectedRoot, gotRoot) - require.NotSame(t, postState, copied) - require.Equal(t, postState.Slot(), copied.Slot()) + clock := eth_clock.NewMockEthereumClock(ctrl) + clock.EXPECT().GetSlotTime(gomock.Any()).Return(time.Now().Add(6 * time.Second)).AnyTimes() + handler.ethClock = clock + + engine := execution_client.NewMockExecutionEngine(ctrl) + engine.EXPECT().ForkChoiceUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, _, _, _ common.Hash, attrs *engine_types.PayloadAttributes, _ clparams.StateVersion) ([]byte, error) { + require.NotNil(t, attrs.ParentBeaconBlockRoot) + require.Equal(t, viewRoot, *attrs.ParentBeaconBlockRoot) + return []byte{1, 2, 3, 4, 5, 6, 7, 8}, nil + }) + handler.engine = engine + + primedHead, err := handler.preparePayloadFor(t.Context(), targetSlot) + require.NoError(t, err) + require.Equal(t, viewRoot, primedHead) } func TestGetEthV3ValidatorBlockMapsNotSyncedToServiceUnavailable(t *testing.T) { @@ -1223,7 +1247,7 @@ func TestShouldPreparePayloadVersion(t *testing.T) { }{ {clparams.Phase0Version, false}, {clparams.AltairVersion, false}, - {clparams.BellatrixVersion, true}, + {clparams.BellatrixVersion, false}, {clparams.CapellaVersion, true}, {clparams.DenebVersion, true}, {clparams.FuluVersion, true}, @@ -1239,9 +1263,6 @@ func TestPayloadAttributesCarryEveryFieldTheExecutionLayerGates(t *testing.T) { attrs := payloadAttributes(1, common.Hash{0xbb}, common.Address{0xcc}, withdrawals, &root) - // The execution layer decides from the payload timestamp what these mean, and rejects a request - // that disagrees with it. Withholding them on a consensus-fork rule puts the two on separate - // oracles, so both are sent whatever the fork. require.Equal(t, withdrawals, attrs.Withdrawals) require.Equal(t, &root, attrs.ParentBeaconBlockRoot) require.Nil(t, attrs.SlotNumber, "only a Gloas proposal knows its slot number") @@ -1282,18 +1303,22 @@ func TestPreparedPayloadCopiesTheID(t *testing.T) { func TestSlotHeadMatchesOnlyTheExactPairing(t *testing.T) { headA := common.Hash{0xaa} headB := common.Hash{0xbb} - settled := slotHead{slot: 10, head: headA} + settled := slotHead{slot: 10, head: headA, generation: 3} // Nothing recorded yet matches nothing. - require.False(t, slotHead{}.is(10, headA)) + require.NotEqual(t, settled, slotHead{}) - // Already settled for this slot on this head: nothing to do. - require.True(t, settled.is(10, headA)) + // Already settled for this slot, on this head, under these registrations: nothing to do. + require.Equal(t, settled, slotHead{slot: 10, head: headA, generation: 3}) // The previous slot's block arrived late and moved the head, so the warm builder is on a parent // that is no longer the head — prime again rather than wait for the next slot. - require.False(t, settled.is(10, headB)) + require.NotEqual(t, settled, slotHead{slot: 10, head: headB, generation: 3}) // A new target slot always needs priming. - require.False(t, settled.is(11, headA)) + require.NotEqual(t, settled, slotHead{slot: 11, head: headA, generation: 3}) + + // A registration arriving late can make a slot ours after it was ruled out, or change the fee + // recipient a prime already went out with. + require.NotEqual(t, settled, slotHead{slot: 10, head: headA, generation: 4}) } diff --git a/cl/beacon/handler/payload_preparation.go b/cl/beacon/handler/payload_preparation.go index 66adab2c422..8c93899fa64 100644 --- a/cl/beacon/handler/payload_preparation.go +++ b/cl/beacon/handler/payload_preparation.go @@ -33,6 +33,7 @@ import ( "github.com/erigontech/erigon/common/hexutil" "github.com/erigontech/erigon/common/log/v3" "github.com/erigontech/erigon/execution/engineapi/engine_types" + "github.com/erigontech/erigon/execution/execmodule/chainreader" ) var ( @@ -40,6 +41,7 @@ var ( errNoPayloadID = errors.New("execution layer returned no payload id") errHeadTooFarBack = errors.New("head state is too far behind the slot to prepare") errPreparationHeadChanged = errors.New("head changed while preparing payload") + errPreparationTooLate = errors.New("slot is too close to prime a payload production would use") ) // preparedPayloadRetainSlots keeps a primed record alive past the slot it was primed for, so @@ -131,15 +133,19 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if !shouldPreparePayloadVersion(stateVersion) { continue } - // On consecutive proposals the prime for the next slot lands inside this one, where it - // would compete for the execution layer with the block being produced right now. + // On consecutive proposals the prime for the next slot lands inside this one. if a.proposalsInFlight.Load() > 0 { continue } + // Nothing is registered, so nothing here can be ours. Checking first keeps a non-validating + // node off the state copy entirely. + generation := a.validatorParams.Generation() + if generation == 0 { + continue + } // Before genesis the current slot clamps to zero, so the next slot can be arbitrarily far // off; a builder primed that early hits its own cap before the slot even starts. Too late - // and the prime can never reach the age production demands of it, so the state copy and the - // forkchoice update would both be spent for nothing. + // and the prime can never reach the age production demands of it. slotStart := a.ethClock.GetSlotTime(targetSlot) lead := time.Until(slotStart) if lead > maxPreparationLead(a.beaconChainCfg) || lead < preparedPayloadMinimumAge(a.beaconChainCfg, stateVersion) { @@ -156,9 +162,11 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { if selectedRoot != a.syncedData.HeadRoot() { continue } - // Both verdicts hold until the slot or its parent head moves. Re-deriving the proposer costs - // a state copy, and an epoch transition on top when the slot is in the next epoch. - if primed.is(targetSlot, selectedRoot) || notOurs.is(targetSlot, selectedRoot) { + // Both verdicts survive only while the slot, its parent head and the registrations they + // were taken under all hold. Re-deriving the proposer costs a state copy, and an epoch + // transition on top when the slot is in the next epoch. + settled := slotHead{slot: targetSlot, head: selectedRoot, generation: generation} + if primed == settled || notOurs == settled { continue } prepareCtx, cancel := context.WithDeadline(ctx, slotStart) @@ -166,7 +174,7 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { cancel() if err != nil { if errors.Is(err, errNotOurProposal) { - notOurs = slotHead{slot: targetSlot, head: selectedRoot} + notOurs = settled } if !isExpectedPreparationSkip(err) && time.Since(lastFailureLog) >= time.Minute { log.Warn("PayloadPreparation: failed", "slot", targetSlot, "err", err) @@ -174,19 +182,17 @@ func (a *ApiHandler) preparePayloadLoop(ctx context.Context) { } continue } - primed = slotHead{slot: targetSlot, head: head} + primed = slotHead{slot: targetSlot, head: head, generation: generation} } } -// slotHead records work already settled for a target slot on a given head, so a later tick can skip -// repeating it. A zero value matches nothing reachable: slot zero is never primed. +// slotHead records work already settled for a target slot, on a given head, under a given set of +// validator registrations. A zero value matches nothing reachable: preparation never runs before +// the first registration arrives. type slotHead struct { - slot uint64 - head common.Hash -} - -func (s slotHead) is(slot uint64, head common.Hash) bool { - return s.slot == slot && s.head == head + slot uint64 + head common.Hash + generation uint64 } // maxPreparationLead bounds how far ahead of a slot priming is worthwhile. One slot is all a live @@ -201,8 +207,10 @@ func preparationRetired(version clparams.StateVersion) bool { return version.AfterOrEqual(clparams.GloasVersion) } +// shouldPreparePayloadVersion starts at Capella because payload attributes always carry withdrawals, +// which the execution layer rejects before Shanghai: priming earlier could only ever fail. func shouldPreparePayloadVersion(version clparams.StateVersion) bool { - return version.AfterOrEqual(clparams.BellatrixVersion) && !preparationRetired(version) + return version.AfterOrEqual(clparams.CapellaVersion) && !preparationRetired(version) } // isExpectedPreparationSkip reports whether there was simply nothing to prepare, as opposed to a @@ -212,8 +220,10 @@ func isExpectedPreparationSkip(err error) bool { errors.Is(err, errNoPayloadID) || errors.Is(err, errHeadTooFarBack) || errors.Is(err, errPreparationHeadChanged) || + errors.Is(err, errPreparationTooLate) || errors.Is(err, execution_client.ErrForkChoiceNotAdopted) || errors.Is(err, execution_client.ErrForkChoiceBusy) || + errors.Is(err, chainreader.ErrExecutionBusy) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) || errors.Is(err, synced_data.ErrNotSynced) @@ -272,14 +282,16 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( } stateVersion := a.beaconChainCfg.GetCurrentStateVersion(targetSlot / a.beaconChainCfg.SlotsPerEpoch) + // The lead was measured before the copy and the epoch transition, which are slow enough to have + // spent it. Stop rather than record a prime production would reject on age. + if time.Until(a.ethClock.GetSlotTime(targetSlot)) < preparedPayloadMinimumAge(a.beaconChainCfg, stateVersion) { + return common.Hash{}, errPreparationTooLate + } head, safeHash, finalizedHash, attrs, err := a.preGloasForkChoiceInputs(baseState, baseBlockRoot, targetSlot, feeRecipient) if err != nil { return common.Hash{}, err } - if selectedRoot, _, selected := a.syncedData.SelectedHead(); !selected || selectedRoot != baseBlockRoot { - return common.Hash{}, errPreparationHeadChanged - } - payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalizedHash, safeHash, head, attrs, stateVersion) + payloadID, err := a.forkChoiceUpdateForPreparation(ctx, baseBlockRoot, finalizedHash, safeHash, head, attrs, stateVersion) if err != nil { return common.Hash{}, err } @@ -292,6 +304,33 @@ func (a *ApiHandler) preparePayloadFor(ctx context.Context, targetSlot uint64) ( return baseBlockRoot, nil } +// forkChoiceUpdateForPreparation retries contention within the prime, whose context already ends at +// the slot it is for. Everything upstream of it — the head view, the state copy, the epoch +// transition — is far more expensive to repeat than the update itself. The head is re-checked before +// every attempt so a retry can never assert one fork choice has already left behind. +func (a *ApiHandler) forkChoiceUpdateForPreparation( + ctx context.Context, + baseBlockRoot common.Hash, + finalized, safe, head common.Hash, + attrs *engine_types.PayloadAttributes, + stateVersion clparams.StateVersion, +) ([]byte, error) { + for { + if selectedRoot, _, selected := a.syncedData.SelectedHead(); !selected || selectedRoot != baseBlockRoot { + return nil, errPreparationHeadChanged + } + payloadID, err := a.engine.ForkChoiceUpdate(ctx, finalized, safe, head, attrs, stateVersion) + if !errors.Is(err, execution_client.ErrForkChoiceBusy) { + return payloadID, err + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(forkChoiceBusyRetryDelay): + } + } +} + // executionCheckpointHashes resolves the safe and finalized execution hashes, falling back to the // head for a checkpoint whose execution block this node has not seen yet. func (a *ApiHandler) executionCheckpointHashes(baseState *state.CachingBeaconState, head common.Hash) (safeHash, finalizedHash common.Hash) { diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index 78de8f7266a..0f8984f4d96 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -131,9 +131,10 @@ func (cc *ExecutionClientDirect) NewPayload( // there is nothing to build on. var ErrForkChoiceNotAdopted = errors.New("execution layer did not adopt forkchoice head") -// ErrForkChoiceBusy reports contention rather than rejection: the update continues on the module's -// own context. Retrying is the caller's decision because only the caller knows whether the head it -// asked for is still the one it wants. +// ErrForkChoiceBusy reports contention rather than rejection. The execution layer either declined +// the update outright or is still running it in the background; the two are indistinguishable from +// here, and in both cases only a later attempt settles it. Retrying is the caller's decision, +// because only the caller knows whether the head it asked for is still the one it wants. var ErrForkChoiceBusy = errors.New("execution layer busy with a forkchoice update") // forkChoiceStatusError classifies a status reached with payload attributes attached. Only an @@ -163,7 +164,7 @@ func (cc *ExecutionClientDirect) ForkChoiceUpdate(ctx context.Context, finalized } if attr == nil { if status == execmodule.ExecutionStatusBusy { - log.Warn("[ForkChoiceUpdated] execution layer was busy, head not applied", "head", head) + log.Debug("[ForkChoiceUpdated] execution layer busy, head may not have been applied", "head", head) } return nil, nil } @@ -199,8 +200,6 @@ func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, if id, err = assemble(ctx); err == nil { return id, nil } - // Only contention settles by waiting. A rejected request returns the same answer however - // many times it is asked, and retrying it burns the slot instead of reporting it. if !errors.Is(err, chainreader.ErrExecutionBusy) { return 0, err } diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go index 95dcbc5d8c4..7556411f58a 100644 --- a/cl/phase1/execution_client/execution_client_direct_test.go +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -62,7 +62,6 @@ func TestRetryAssembleBlockStopsOnPermanentError(t *testing.T) { return 0, rejected }) - // A rejection answers the same way however often it is asked, so retrying it only burns the slot. require.ErrorIs(t, err, rejected) require.Equal(t, 1, calls) } diff --git a/cl/validator/validator_params/validator_params.go b/cl/validator/validator_params/validator_params.go index f422714b8bf..3a063721bab 100644 --- a/cl/validator/validator_params/validator_params.go +++ b/cl/validator/validator_params/validator_params.go @@ -18,12 +18,14 @@ package validator_params import ( "sync" + "sync/atomic" "github.com/erigontech/erigon/common" ) type ValidatorParams struct { feeRecipients sync.Map + generation atomic.Uint64 } func NewValidatorParams() *ValidatorParams { @@ -31,7 +33,18 @@ func NewValidatorParams() *ValidatorParams { } func (vp *ValidatorParams) SetFeeRecipient(validatorIndex uint64, feeRecipient common.Address) { - vp.feeRecipients.Store(validatorIndex, feeRecipient) + // Validator clients re-register unchanged on a schedule, so only a real change counts: a + // generation that moved on every call would be useless to anyone caching a lookup. + if previous, loaded := vp.feeRecipients.Swap(validatorIndex, feeRecipient); !loaded || previous.(common.Address) != feeRecipient { + vp.generation.Add(1) + } +} + +// Generation changes whenever a registration is added or its fee recipient changes, and is zero +// until the first one arrives. A consumer caching a lookup can compare it to find out whether a +// later registration could have changed the answer. +func (vp *ValidatorParams) Generation() uint64 { + return vp.generation.Load() } func (vp *ValidatorParams) GetFeeRecipient(validatorIndex uint64) (common.Address, bool) { diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 836b420621c..2deb3b43897 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -106,13 +106,10 @@ func (b *BlockBuilder) Cancel() { b.interrupt.Store(true) } -// Stale reports whether the builder can no longer improve on what it holds, because it was -// cancelled or finished with an error. A builder that filled its block is not stale: it simply -// has nothing left to add. -func (b *BlockBuilder) Stale() bool { - if b.interrupt.Load() { - return true - } +// Failed reports whether the builder finished without producing anything. The error is latched, so +// a caller that would otherwise reuse this builder has to treat it as absent. Being cancelled is +// not failure: a stopped builder still holds the payload it was stopped for. +func (b *BlockBuilder) Failed() bool { select { case <-b.done: default: diff --git a/execution/builder/block_builder_test.go b/execution/builder/block_builder_test.go index f44150f8d32..e0bf5aa3470 100644 --- a/execution/builder/block_builder_test.go +++ b/execution/builder/block_builder_test.go @@ -27,7 +27,7 @@ import ( "github.com/erigontech/erigon/execution/types" ) -func TestBlockBuilderRunningIsNotStale(t *testing.T) { +func TestBlockBuilderRunningHasNotFailed(t *testing.T) { t.Parallel() release := make(chan struct{}) @@ -37,31 +37,35 @@ func TestBlockBuilderRunningIsNotStale(t *testing.T) { return nil, errors.New("builder stopped") }, &Parameters{}, time.Minute) - require.Never(t, b.Stale, 50*time.Millisecond, 5*time.Millisecond) + require.Never(t, b.Failed, 50*time.Millisecond, 5*time.Millisecond) } -func TestBlockBuilderIsStaleOnceCancelled(t *testing.T) { +func TestBlockBuilderStoppedForItsPayloadHasNotFailed(t *testing.T) { t.Parallel() b := NewBlockBuilder(func(_ *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { for !interrupt.Load() { time.Sleep(time.Millisecond) } - return nil, errors.New("builder stopped") + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil }, &Parameters{}, time.Minute) - b.Cancel() - require.True(t, b.Stale()) + _, err := b.Stop(t.Context()) + require.NoError(t, err) + + // Collecting the payload is what a proposal does. Reading that as failure would make a repeated + // request rebuild from scratch instead of being handed the block that was just built. + require.False(t, b.Failed()) } -func TestBlockBuilderIsStaleOnceItFails(t *testing.T) { +func TestBlockBuilderHasFailedOnceItErrors(t *testing.T) { t.Parallel() b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { return nil, errors.New("build failed") }, &Parameters{}, time.Minute) - require.Eventually(t, b.Stale, time.Second, time.Millisecond) + require.Eventually(t, b.Failed, time.Second, time.Millisecond) } func TestBlockBuilderStaysReusableOnceItFillsTheBlock(t *testing.T) { @@ -75,5 +79,5 @@ func TestBlockBuilderStaysReusableOnceItFillsTheBlock(t *testing.T) { <-built // A builder that ran out of room holds a complete payload, so its id is still worth reusing. - require.Never(t, b.Stale, 50*time.Millisecond, 5*time.Millisecond) + require.Never(t, b.Failed, 50*time.Millisecond, 5*time.Millisecond) } diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index e32e5dd281b..7eb38e84d67 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -135,26 +135,20 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete return AssembleBlockResult{}, err } + // A stopped builder is still worth reusing: it holds the payload it was stopped for, which is + // exactly what a repeated request is asking for. Only a failed one has to be passed over. if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { - // A stale builder can never grow its payload, so reusing its id would spend the slot - // waiting on a block that will not arrive. Take a fresh builder instead and leave the - // old id answerable for whoever already holds it. - if previous := e.builders[previousID]; previous != nil && previous.builder != nil && !previous.builder.Stale() { + if previous := e.builders[previousID]; previous != nil && previous.builder != nil && !previous.builder.Failed() { params.PayloadId = previousID if reflect.DeepEqual(previous.params, params) { e.logger.Info("[ForkChoiceUpdated] duplicate build request") return AssembleBlockResult{PayloadID: previousID}, nil } - // Superseding must not outlive the caller that asked for it: cancelling on behalf of - // an expired request would freeze a builder the next request is already waiting on. - if err := ctx.Err(); err != nil { - return AssembleBlockResult{}, err - } - previous.builder.Cancel() } } - - // Initiate payload building + // A superseded builder keeps running to its own deadline. The timestamp index moves to the new + // one, so nothing reaches it by dedup, while an id already handed out goes on answering with a + // payload that is still growing. e.evictOldBuilders() e.nextPayloadId++ @@ -206,8 +200,8 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { - // A failed builder latches its error, so keeping the entry would hand the same failure to - // every retry. A caller whose own context expired says nothing about the builder. + // Keeping a failed entry would hand the same latched error to every retry. A caller whose + // own context expired says nothing about the builder. if ctx.Err() == nil { e.dropBuilder(payloadID, entry) } diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index d0f1d332088..7d85b5eca16 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -36,7 +36,7 @@ import ( "github.com/erigontech/erigon/execution/types" ) -func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { +func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { type runningBuilder struct { id uint64 interrupt *atomic.Bool @@ -84,32 +84,33 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { firstID, first := assemble(100, common.Hash{0x01}) adjacentID, adjacent := assemble(101, common.Hash{0x02}) require.NotEqual(t, firstID, adjacentID) - require.False(t, first.interrupt.Load(), "a builder for another target timestamp must stay alive") firstDuplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) require.NoError(t, err) require.Equal(t, firstID, firstDuplicate.PayloadID) - require.False(t, first.interrupt.Load(), "an exact earlier-timestamp builder must stay alive") - require.False(t, adjacent.interrupt.Load()) + // Superseding hands the timestamp to a new builder and leaves the old ones running, so only the + // index moves. Timestamp 101 is a different proposal and is untouched throughout. secondID, second := assemble(100, common.Hash{0x03}) require.NotEqual(t, firstID, secondID) - require.Eventually(t, first.interrupt.Load, time.Second, time.Millisecond) - require.False(t, adjacent.interrupt.Load(), "superseding timestamp 100 must not cancel timestamp 101") - require.False(t, second.interrupt.Load()) + require.Equal(t, secondID, module.buildersByTimestamp[100]) + require.Equal(t, adjacentID, module.buildersByTimestamp[101]) thirdID, third := assemble(100, common.Hash{0x04}) require.NotEqual(t, secondID, thirdID) - require.Eventually(t, second.interrupt.Load, time.Second, time.Millisecond) - require.False(t, adjacent.interrupt.Load()) - require.False(t, third.interrupt.Load()) + require.Equal(t, thirdID, module.buildersByTimestamp[100]) duplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x04}}) require.NoError(t, err) require.Equal(t, thirdID, duplicate.PayloadID) - require.False(t, third.interrupt.Load(), "a duplicate request must keep its builder alive") + for _, running := range []runningBuilder{first, second, third, adjacent} { + require.False(t, running.interrupt.Load(), "builder %d must still be packing", running.id) + } + + // Eviction is where a builder is actually stopped, and it takes the timestamp index with it. delete(module.builders, firstID) + delete(module.builders, secondID) for id := thirdID + 1; len(module.builders) < engine_helpers.MaxBuilders; id++ { module.builders[id] = nil } @@ -117,18 +118,18 @@ func TestAssembleBlockSupersedesBuilderForSameTimestamp(t *testing.T) { require.Eventually(t, adjacent.interrupt.Load, time.Second, time.Millisecond) require.NotContains(t, module.builders, adjacentID) require.NotContains(t, module.buildersByTimestamp, uint64(101)) - require.NotContains(t, module.builders, adjacentID) + require.False(t, third.interrupt.Load(), "the current builder for a timestamp must survive eviction") } -func TestSupersededPayloadIDStaysRetrievable(t *testing.T) { - started := make(chan struct{}, 4) +func TestSupersededBuilderKeepsPackingAndStaysRetrievable(t *testing.T) { + started := make(chan *atomic.Bool, 4) module := &ExecModule{ logger: log.Root(), config: &chain.Config{}, semaphore: semaphore.NewWeighted(1), builders: map[uint64]*builderEntry{}, builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { - started <- struct{}{} + started <- interrupt for !interrupt.Load() { time.Sleep(time.Millisecond) } @@ -138,20 +139,58 @@ func TestSupersededPayloadIDStaysRetrievable(t *testing.T) { first, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) require.NoError(t, err) - <-started + firstInterrupt := <-started second, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x02}}) require.NoError(t, err) require.NotEqual(t, first.PayloadID, second.PayloadID) + secondInterrupt := <-started + + // The timestamp index moves to the new builder, so nothing reaches the old one by dedup. It is + // left running: freezing it would answer an id already handed out with a near-empty payload. + require.Equal(t, second.PayloadID, module.buildersByTimestamp[100]) + require.False(t, firstInterrupt.Load()) + + assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) + require.NoError(t, err) + require.NotNil(t, assembled.Block) + + secondInterrupt.Store(true) +} + +func TestCollectedPayloadIsHandedBackToARepeatedRequest(t *testing.T) { + started := make(chan struct{}, 4) + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- struct{}{} + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }, + } + + params := func() *builder.Parameters { + return &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}} + } + first, err := module.AssembleBlock(t.Context(), params()) + require.NoError(t, err) <-started - // Superseding hands the timestamp to a fresh builder, but an id already returned to a caller - // stays answerable: dropping it would turn a getPayload into an unknown-payload error. assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) require.NoError(t, err) require.NotNil(t, assembled.Block) - _, _ = module.builders[second.PayloadID].builder.Stop(context.Background()) + // Collecting stops the builder. A repeated request must still be handed that payload: rebuilding + // from scratch this late means the next grab takes a near-empty block. + repeat, err := module.AssembleBlock(t.Context(), params()) + require.NoError(t, err) + require.Equal(t, first.PayloadID, repeat.PayloadID) + require.Empty(t, started, "a repeated request must not start a second builder") } func TestAssembleBlockDoesNotReuseFailedBuilder(t *testing.T) { @@ -181,7 +220,7 @@ func TestAssembleBlockDoesNotReuseFailedBuilder(t *testing.T) { first, err := module.AssembleBlock(t.Context(), params()) require.NoError(t, err) <-started - require.Eventually(t, module.builders[first.PayloadID].builder.Stale, time.Second, time.Millisecond) + require.Eventually(t, module.builders[first.PayloadID].builder.Failed, time.Second, time.Millisecond) // Identical parameters would normally dedup onto the same id. A builder that already died // latches its error, so reusing it would spend the slot on a payload that can never arrive.