From 9a294a7d61fafa03b852dd079367433131b7251e Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 08:21:03 +0200 Subject: [PATCH 1/7] execution: key builders by payload timestamp and give them a lifecycle Split out of #23105 so it can be reviewed on its own. The preparation work there leans on this, but every problem below is reachable today. A single lastParameters field remembered only the most recent request, so any interleaved request for a different timestamp destroyed the deduplication for the first: a repeated request then started a second builder for a payload already being built. Builders are now kept by the timestamp they are for, alongside an immutable copy of the parameters they were created with, so a caller cannot mutate the slice it passed and change what a later comparison sees. A builder that failed latched its error and was handed back forever, spending the slot waiting on a payload that could never arrive. It is now treated as absent, and dropped when its error surfaces. Being stopped is not failure: a stopped builder still holds the payload it was stopped for, which is exactly what a repeated request is asking for. Eviction dropped builders from the map without stopping them, so the goroutine kept running with no way to reach it. It now cancels on the way out, which is the problem described in issue #23101. Both entry points check for a cancelled caller before acting, so an expired request reports why it stopped rather than looking like contention that callers retry. --- execution/builder/block_builder.go | 20 +- execution/builder/block_builder_test.go | 83 +++++ execution/execmodule/block_building.go | 108 +++++- .../block_building_internal_test.go | 340 ++++++++++++++++++ execution/execmodule/exec_module.go | 10 +- 5 files changed, 541 insertions(+), 20 deletions(-) create mode 100644 execution/builder/block_builder_test.go diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index e77019a08c2..2deb3b43897 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,24 @@ func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, erro return b.result, b.err } +func (b *BlockBuilder) Cancel() { + b.interrupt.Store(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: + 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..e0bf5aa3470 --- /dev/null +++ b/execution/builder/block_builder_test.go @@ -0,0 +1,83 @@ +// 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 TestBlockBuilderRunningHasNotFailed(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.Failed, 50*time.Millisecond, 5*time.Millisecond) +} + +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 &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }, &Parameters{}, time.Minute) + + _, 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 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.Failed, 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.Failed, 50*time.Millisecond, 5*time.Millisecond) +} diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index ef1d94cddaf..121005915ab 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -17,6 +17,7 @@ package execmodule import ( + "bytes" "context" "reflect" "time" @@ -60,16 +61,75 @@ 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 +} + +// 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) 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) // 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 { + 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 } @@ -79,23 +139,35 @@ 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) { - e.logger.Info("[ForkChoiceUpdated] duplicate build request") - return AssembleBlockResult{PayloadID: e.lastParameters.PayloadId}, nil + // 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 { + 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 + } } } - - // 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++ 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())) + if e.buildersByTimestamp == nil { + e.buildersByTimestamp = make(map[uint64]uint64) + } + 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.logger.Info("[ForkChoiceUpdated] BlockBuilder added", "payload", e.nextPayloadId) return AssembleBlockResult{PayloadID: e.nextPayloadId}, nil @@ -118,17 +190,25 @@ 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 == nil || entry.builder == nil { return AssembledBlockResult{}, nil } - blockWithReceipts, err := bldr.Stop(ctx) + blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { + // 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) + } 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 b48479af2e6..7d85b5eca16 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -17,13 +17,353 @@ 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 TestAssembleBlockKeepsBuildersApartByTimestamp(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]*builderEntry{}, + 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 _, entry := range module.builders { + if entry != nil && entry.builder != nil { + _, _ = entry.builder.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) + + firstDuplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + require.Equal(t, firstID, firstDuplicate.PayloadID) + + // 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.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.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) + + 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 + } + 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)) + require.False(t, third.interrupt.Load(), "the current builder for a timestamp must survive eviction") +} + +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 <- interrupt + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }, + } + + first, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + 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 + + assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) + require.NoError(t, err) + require.NotNil(t, assembled.Block) + + // 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) { + 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.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. + 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) { + 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]*builderEntry{}, + 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].builder.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]*builderEntry{}, + 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].builder.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)) + + 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) { 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 6d771fa4001..292b7b70df7 100644 --- a/execution/execmodule/exec_module.go +++ b/execution/execmodule/exec_module.go @@ -194,10 +194,10 @@ type ExecModule struct { logger log.Logger // Block building - nextPayloadId uint64 - lastParameters *builder.Parameters - builderFunc builder.BlockBuilderFunc - builders map[uint64]*builder.BlockBuilder + nextPayloadId uint64 + builderFunc builder.BlockBuilderFunc + builders map[uint64]*builderEntry + buildersByTimestamp map[uint64]uint64 // Changes accumulator hook *stageloop.Hook @@ -266,7 +266,7 @@ func NewExecModule( logger: logger, forkValidator: forkValidator, pipelineExecutor: pipelineExecutor, - builders: make(map[uint64]*builder.BlockBuilder), + builders: make(map[uint64]*builderEntry), builderFunc: builderFunc, config: config, semaphore: semaphore.NewWeighted(1), From 2f0fe0403a7659996e1b2a7e4c5b0c37a7cf8a74 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 08:56:46 +0200 Subject: [PATCH 2/7] execution: address review A caller that gave up is not a build failure: return its own error without reporting it or dropping a builder that is still running and may still be collected. Correct the Failed docstring to say what it checks. --- execution/builder/block_builder.go | 2 +- execution/execmodule/block_building.go | 10 +++--- .../block_building_internal_test.go | 33 +++++++++++++++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 2deb3b43897..8e78f96919f 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -106,7 +106,7 @@ func (b *BlockBuilder) Cancel() { b.interrupt.Store(true) } -// Failed reports whether the builder finished without producing anything. The error is latched, so +// Failed reports whether the builder has finished and ended in an error. That 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 { diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 121005915ab..c9ecb47655f 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -204,11 +204,13 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { - // 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) + // A caller that gave up says nothing about the builder, which keeps running and may still + // be collected. Only a builder that actually failed is reported and dropped, so its latched + // error stops being handed to every retry. + if ctx.Err() != nil { + return AssembledBlockResult{}, err } + 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 7d85b5eca16..e3a4315c5ad 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -254,6 +254,39 @@ func TestGetAssembledBlockDropsFailedBuilder(t *testing.T) { require.NotContains(t, module.buildersByTimestamp, uint64(100)) } +func TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUp(t *testing.T) { + started := make(chan struct{}, 1) + 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 + }, + } + + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + <-started + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, err = module.GetAssembledBlock(ctx, result.PayloadID) + require.ErrorIs(t, err, context.Canceled) + + // The caller gave up; the builder did not. Dropping it here would lose a payload that is still + // on its way, and reporting it as a build failure would misattribute the timeout. + require.Contains(t, module.builders, result.PayloadID) + require.Equal(t, result.PayloadID, module.buildersByTimestamp[100]) + + _, _ = module.builders[result.PayloadID].builder.Stop(context.Background()) +} + func TestAssembleBlockOwnsParameters(t *testing.T) { type observedParameters struct { parentRoot common.Hash From bbca601614be6b8882565002b8abe4a068fceea3 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 14:20:36 +0200 Subject: [PATCH 3/7] execution: give each builder a cancellable lifetime Addresses the eviction gap in #23101 rather than only appearing to. Cancel set an atomic flag, but Builder.Build ran its database read view and its transaction provider on the node-lifetime context, and the flag is not read until those return. A provider can wait most of a slot, so an evicted builder left the map while its goroutine and read view stayed alive, and repeated distinct requests could hold more of them than MaxBuilders allows. A builder now answers two distinct requests. Interrupting asks for the block it has so far, which is how a payload is collected and how the maximum build time is enforced; both still want the payload. Discarding says the payload is not wanted at all and cancels the context the build runs under, so a read view or a provider blocked on it returns at once instead of waiting out its own deadline. Eviction discards. Nothing here is timed or fork-specific: the build context carries no deadline of its own, and the existing budget still derives from the chain's slot length. GetAssembledBlock reads a cancelled caller from the returned error rather than from the ambient context, which could otherwise change between Stop returning and the check. --- execution/builder/block_builder.go | 32 ++-- execution/builder/block_builder_test.go | 9 +- execution/builder/builder.go | 16 +- execution/builder/builder_test.go | 3 +- execution/execmodule/block_building.go | 14 +- .../block_building_internal_test.go | 167 +++++++++++++----- .../execmoduletester/exec_module_tester.go | 1 - node/eth/backend.go | 1 - 8 files changed, 162 insertions(+), 81 deletions(-) diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 8e78f96919f..a624869ebfe 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -28,19 +28,26 @@ import ( "github.com/erigontech/erigon/execution/types" ) -type BlockBuilderFunc func(param *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) +// BlockBuilderFunc builds a payload. Its context ends when the payload is discarded, so anything +// that can block - opening a read view, waiting on a transaction provider - has to honour it. +type BlockBuilderFunc func(ctx context.Context, param *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) -// BlockBuilder wraps a goroutine that builds Proof-of-Stake payloads (PoS "mining") +// BlockBuilder wraps a goroutine that builds Proof-of-Stake payloads (PoS "mining"). +// +// It answers to two different requests. Interrupting asks for the block it has so far, which is how +// a payload is collected. Discarding says the payload is not wanted at all, and cancels the work. type BlockBuilder struct { interrupt atomic.Bool + discard context.CancelFunc mu sync.Mutex done chan struct{} result *types.BlockWithReceipts err error } -func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime time.Duration) *BlockBuilder { - builder := &BlockBuilder{done: make(chan struct{})} +func NewBlockBuilder(ctx context.Context, build BlockBuilderFunc, param *Parameters, maxBuildTime time.Duration) *BlockBuilder { + buildCtx, discard := context.WithCancel(ctx) + builder := &BlockBuilder{done: make(chan struct{}), discard: discard} go func() { var result *types.BlockWithReceipts @@ -58,11 +65,12 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim builder.err = err builder.mu.Unlock() close(builder.done) + discard() }() log.Info("Building block...") t := time.Now() - result, err = build(param, &builder.interrupt) + result, err = build(buildCtx, param, &builder.interrupt) if err != nil { log.Warn("Failed to build a block", "err", err) } else { @@ -89,7 +97,7 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim } func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, error) { - b.Cancel() + b.interrupt.Store(true) select { case <-ctx.Done(): @@ -102,13 +110,17 @@ func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, erro return b.result, b.err } -func (b *BlockBuilder) Cancel() { +// Discard abandons the build and releases what it holds. A read view or a transaction provider +// blocked on the builder's context returns at once instead of waiting out its own deadline, which +// is the difference between an evicted builder freeing its resources now and freeing them a slot +// from now. +func (b *BlockBuilder) Discard() { b.interrupt.Store(true) + b.discard() } -// Failed reports whether the builder has finished and ended in an error. That 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. +// Failed reports whether the builder has finished and ended in an error, which a caller looking to +// reuse it has to read as absent because that error is latched. func (b *BlockBuilder) Failed() bool { select { case <-b.done: diff --git a/execution/builder/block_builder_test.go b/execution/builder/block_builder_test.go index e0bf5aa3470..5b2a1b8ca73 100644 --- a/execution/builder/block_builder_test.go +++ b/execution/builder/block_builder_test.go @@ -17,6 +17,7 @@ package builder import ( + "context" "errors" "sync/atomic" "testing" @@ -32,7 +33,7 @@ func TestBlockBuilderRunningHasNotFailed(t *testing.T) { release := make(chan struct{}) t.Cleanup(func() { close(release) }) - b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + b := NewBlockBuilder(t.Context(), func(_ context.Context, _ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { <-release return nil, errors.New("builder stopped") }, &Parameters{}, time.Minute) @@ -43,7 +44,7 @@ func TestBlockBuilderRunningHasNotFailed(t *testing.T) { func TestBlockBuilderStoppedForItsPayloadHasNotFailed(t *testing.T) { t.Parallel() - b := NewBlockBuilder(func(_ *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + b := NewBlockBuilder(t.Context(), func(_ context.Context, _ *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { for !interrupt.Load() { time.Sleep(time.Millisecond) } @@ -61,7 +62,7 @@ func TestBlockBuilderStoppedForItsPayloadHasNotFailed(t *testing.T) { func TestBlockBuilderHasFailedOnceItErrors(t *testing.T) { t.Parallel() - b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + b := NewBlockBuilder(t.Context(), func(_ context.Context, _ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { return nil, errors.New("build failed") }, &Parameters{}, time.Minute) @@ -72,7 +73,7 @@ func TestBlockBuilderStaysReusableOnceItFillsTheBlock(t *testing.T) { t.Parallel() built := make(chan struct{}) - b := NewBlockBuilder(func(_ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + b := NewBlockBuilder(t.Context(), func(_ context.Context, _ *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) diff --git a/execution/builder/builder.go b/execution/builder/builder.go index 5150348b674..95faef9a7b6 100644 --- a/execution/builder/builder.go +++ b/execution/builder/builder.go @@ -45,7 +45,6 @@ type SDProvider func() *execctx.SharedDomains // without staged-sync machinery. Its Build method satisfies BlockBuilderFunc and can // be passed directly to ExecModule. type Builder struct { - ctx context.Context db kv.TemporalRoDB pendingBlockCh chan *types.Block builderCfg *buildercfg.BuilderConfig @@ -64,7 +63,6 @@ type Builder struct { } func NewBuilder( - ctx context.Context, db kv.TemporalRoDB, builderCfg *buildercfg.BuilderConfig, chainConfig *chain.Config, @@ -81,7 +79,6 @@ func NewBuilder( logger log.Logger, ) *Builder { return &Builder{ - ctx: ctx, db: db, pendingBlockCh: make(chan *types.Block, 1), builderCfg: builderCfg, @@ -107,7 +104,10 @@ func (b *Builder) PendingBlockCh() chan *types.Block { } // Build satisfies BlockBuilderFunc. Pass b.Build directly to ExecModule. -func (b *Builder) Build(param *Parameters, interrupt *atomic.Bool) (result *types.BlockWithReceipts, err error) { +// +// Everything that can block runs under ctx, so discarding the payload releases the read view and +// unblocks the transaction provider instead of leaving them to finish on their own. +func (b *Builder) Build(ctx context.Context, param *Parameters, interrupt *atomic.Bool) (result *types.BlockWithReceipts, err error) { defer func() { if rec := recover(); rec != nil { err = fmt.Errorf("%+v, trace: %s", rec, dbg.Stack()) @@ -124,7 +124,7 @@ func (b *Builder) Build(param *Parameters, interrupt *atomic.Bool) (result *type BuiltBlock: &exec.AssembledBlock{}, } - tx, err := b.db.BeginTemporalRo(b.ctx) + tx, err := b.db.BeginTemporalRo(ctx) if err != nil { return nil, err } @@ -145,7 +145,7 @@ func (b *Builder) Build(param *Parameters, interrupt *atomic.Bool) (result *type } } - sd, err := execctx.NewSharedDomains(b.ctx, compositeTx, b.logger, execctx.WithoutDeferredBranchUpdates(), execctx.WithoutSharedBranchCache()) + sd, err := execctx.NewSharedDomains(ctx, compositeTx, b.logger, execctx.WithoutDeferredBranchUpdates(), execctx.WithoutSharedBranchCache()) if err != nil { return nil, err } @@ -172,10 +172,10 @@ func (b *Builder) Build(param *Parameters, interrupt *atomic.Bool) (result *type execCfg := StageBuilderExecCfg(state, b.notifier, b.chainConfig, b.engine, b.vmConfig, b.tmpdir, interrupt, param.PayloadId, txnProvider, b.blockReader) finishCfg := StageBuilderFinishCfg(b.chainConfig, b.engine, state, b.sealCancel, b.blockReader, b.latestBlockBuiltStore) - if err := createBlock(b.ctx, sd, compositeTx, executionAt, createCfg, b.logger); err != nil { + if err := createBlock(ctx, sd, compositeTx, executionAt, createCfg, b.logger); err != nil { return nil, err } - if err := execBlock(b.ctx, sd, compositeTx, executionAt, execCfg, b.executeBlockCfg, b.logger); err != nil { + if err := execBlock(ctx, sd, compositeTx, executionAt, execCfg, b.executeBlockCfg, b.logger); err != nil { return nil, err } if err := finishBlock(compositeTx, finishCfg, b.logger); err != nil { diff --git a/execution/builder/builder_test.go b/execution/builder/builder_test.go index f7c758522fe..87a74eb129a 100644 --- a/execution/builder/builder_test.go +++ b/execution/builder/builder_test.go @@ -48,14 +48,13 @@ func TestBuilder_Build_DBError(t *testing.T) { want := errors.New("db open failed") b := &Builder{ - ctx: context.Background(), db: &errDB{err: want}, builderCfg: &buildercfg.BuilderConfig{}, pendingBlockCh: make(chan *types.Block, 1), logger: log.New(), } - _, err := b.Build(&Parameters{}, &atomic.Bool{}) + _, err := b.Build(t.Context(), &Parameters{}, &atomic.Bool{}) require.ErrorIs(t, err, want) } diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index c9ecb47655f..8d273773d64 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -19,6 +19,7 @@ package execmodule import ( "bytes" "context" + "errors" "reflect" "time" @@ -114,7 +115,7 @@ func (e *ExecModule) evictOldBuilders() { id := ids[i] if old := e.builders[id]; old != nil { if old.builder != nil { - old.builder.Cancel() + old.builder.Discard() } if e.buildersByTimestamp[old.timestamp] == id { delete(e.buildersByTimestamp, old.timestamp) @@ -163,7 +164,7 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete e.buildersByTimestamp = make(map[uint64]uint64) } e.builders[e.nextPayloadId] = &builderEntry{ - builder: builder.NewBlockBuilder(e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())), + builder: builder.NewBlockBuilder(e.bacgroundCtx, e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())), params: ownedParams, timestamp: params.Timestamp, } @@ -204,12 +205,13 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { - // A caller that gave up says nothing about the builder, which keeps running and may still - // be collected. Only a builder that actually failed is reported and dropped, so its latched - // error stops being handed to every retry. - if ctx.Err() != nil { + // The caller gave up waiting; nothing about the build itself went wrong. Reading that from + // the returned error rather than the ambient context keeps the two from drifting apart + // between Stop returning and this check. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return AssembledBlockResult{}, err } + // Keeping a failed entry would hand its latched error to every retry. 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 e3a4315c5ad..3fbafb5a5e4 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -43,11 +43,12 @@ func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { } started := make(chan runningBuilder, 4) module := &ExecModule{ - semaphore: semaphore.NewWeighted(1), - config: &chain.Config{}, - logger: log.Root(), - builders: map[uint64]*builderEntry{}, - builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + semaphore: semaphore.NewWeighted(1), + config: &chain.Config{}, + logger: log.Root(), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- runningBuilder{id: params.PayloadId, interrupt: interrupt} for !interrupt.Load() { time.Sleep(time.Millisecond) @@ -109,6 +110,10 @@ func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { } // Eviction is where a builder is actually stopped, and it takes the timestamp index with it. + // Removing entries puts them beyond the registered cleanup, so release them here instead of + // leaving two goroutines running until their watchdogs fire. + module.builders[firstID].builder.Discard() + module.builders[secondID].builder.Discard() delete(module.builders, firstID) delete(module.builders, secondID) for id := thirdID + 1; len(module.builders) < engine_helpers.MaxBuilders; id++ { @@ -121,14 +126,62 @@ func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { require.False(t, third.interrupt.Load(), "the current builder for a timestamp must survive eviction") } +func TestEvictionReleasesABuilderBlockedOnItsProvider(t *testing.T) { + // A transaction provider can wait for most of a slot before returning, and the interrupt flag + // is not read until it does. Only cancelling the build reaches it. + entered := make(chan struct{}, 1) + released := make(chan error, 1) + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(ctx context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + entered <- struct{}{} + select { + case <-ctx.Done(): + released <- ctx.Err() + return nil, ctx.Err() + case <-time.After(time.Minute): + released <- errors.New("provider was never released") + return nil, errors.New("provider was never released") + } + }, + } + + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + <-entered + + entry := module.builders[result.PayloadID] + require.NotNil(t, entry) + for id := result.PayloadID + 1; len(module.builders) < engine_helpers.MaxBuilders; id++ { + module.builders[id] = nil + } + module.evictOldBuilders() + require.NotContains(t, module.builders, result.PayloadID) + + // Observing the goroutine finish is the point: an evicted builder that merely has its flag set + // keeps its read view open until whatever it is blocked on gives up on its own. + select { + case err := <-released: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(5 * time.Second): + t.Fatal("evicted builder was never released") + } + require.Eventually(t, func() bool { return entry.builder.Failed() }, 5*time.Second, time.Millisecond) +} + 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) { + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- interrupt for !interrupt.Load() { time.Sleep(time.Millisecond) @@ -161,11 +214,12 @@ func TestSupersededBuilderKeepsPackingAndStaysRetrievable(t *testing.T) { 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) { + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- struct{}{} for !interrupt.Load() { time.Sleep(time.Millisecond) @@ -198,11 +252,12 @@ func TestAssembleBlockDoesNotReuseFailedBuilder(t *testing.T) { 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) { + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- struct{}{} if failNext.Swap(false) { return nil, errors.New("build failed") @@ -234,11 +289,12 @@ func TestAssembleBlockDoesNotReuseFailedBuilder(t *testing.T) { 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) { + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, _ *builder.Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { return nil, errors.New("build failed") }, } @@ -254,37 +310,48 @@ func TestGetAssembledBlockDropsFailedBuilder(t *testing.T) { require.NotContains(t, module.buildersByTimestamp, uint64(100)) } -func TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUp(t *testing.T) { - started := make(chan struct{}, 1) +func TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUpMidStop(t *testing.T) { + interrupted := make(chan struct{}) + release := make(chan struct{}) 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{}{} + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { for !interrupt.Load() { time.Sleep(time.Millisecond) } + close(interrupted) + <-release return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil }, } result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) require.NoError(t, err) - <-started + // Cancel while Stop is already waiting, which is the window a caller-side timeout actually + // lands in. Cancelling beforehand returns at the entry check and exercises none of this. ctx, cancel := context.WithCancel(t.Context()) + collected := make(chan error, 1) + go func() { + _, collectErr := module.GetAssembledBlock(ctx, result.PayloadID) + collected <- collectErr + }() + <-interrupted cancel() - _, err = module.GetAssembledBlock(ctx, result.PayloadID) - require.ErrorIs(t, err, context.Canceled) + require.ErrorIs(t, <-collected, context.Canceled) - // The caller gave up; the builder did not. Dropping it here would lose a payload that is still - // on its way, and reporting it as a build failure would misattribute the timeout. + // The builder was not dropped, so the payload it goes on to produce is still reachable. require.Contains(t, module.builders, result.PayloadID) require.Equal(t, result.PayloadID, module.buildersByTimestamp[100]) - _, _ = module.builders[result.PayloadID].builder.Stop(context.Background()) + close(release) + assembled, err := module.GetAssembledBlock(t.Context(), result.PayloadID) + require.NoError(t, err) + require.NotNil(t, assembled.Block) } func TestAssembleBlockOwnsParameters(t *testing.T) { @@ -295,11 +362,12 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { readParameters := make(chan struct{}) observed := make(chan observedParameters, 1) module := &ExecModule{ - semaphore: semaphore.NewWeighted(1), - config: &chain.Config{}, - logger: log.Root(), - builders: map[uint64]*builderEntry{}, - builderFunc: func(params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + semaphore: semaphore.NewWeighted(1), + config: &chain.Config{}, + logger: log.Root(), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { <-readParameters observed <- observedParameters{parentRoot: *params.ParentBeaconBlockRoot, extraData: params.ExtraData[0]} for !interrupt.Load() { @@ -338,11 +406,12 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { 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]*builderEntry{}, - builderFunc: func(_ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + semaphore: semaphore.NewWeighted(1), + config: &chain.Config{}, + logger: log.Root(), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { started <- interrupt for !interrupt.Load() { time.Sleep(time.Millisecond) diff --git a/execution/execmodule/execmoduletester/exec_module_tester.go b/execution/execmodule/execmoduletester/exec_module_tester.go index 2ec702c496e..bd16a3dc994 100644 --- a/execution/execmodule/execmoduletester/exec_module_tester.go +++ b/execution/execmodule/execmoduletester/exec_module_tester.go @@ -682,7 +682,6 @@ func New(tb testing.TB, opts ...Option) *ExecModuleTester { readAheader := exec.NewBlockReadAheader() blkBuilder := builder.NewBuilder( - mock.Ctx, mock.DB, &cfg.Builder, mock.ChainConfig, diff --git a/node/eth/backend.go b/node/eth/backend.go index 449485bd0db..e1924cee58d 100644 --- a/node/eth/backend.go +++ b/node/eth/backend.go @@ -807,7 +807,6 @@ func New(ctx context.Context, stack *node.Node, config *ethconfig.Config, logger } blkBuilder := builder.NewBuilder( - backend.sentryCtx, backend.chainDB, &config.Builder, backend.chainConfig, From 503e1518f37e4495add7e8b501d0ade1f739e897 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 15:30:56 +0200 Subject: [PATCH 4/7] execution: keep the transaction provider out of the request comparison reflect.DeepEqual descended into CustomTxnProvider, which the running build mutates: the testing namespace's provider clears its transaction list and flips a flag from the build goroutine, so the comparison read fields another goroutine was writing, and its answer changed as the build progressed. A request carrying a provider is now never treated as the same request, which is also what a provider that hands its transactions over once implies. Discarding a builder makes its work return a cancellation, which was reported as a failed build. An eviction is expected, so it is no longer a warning. --- execution/builder/block_builder.go | 6 +- execution/execmodule/block_building.go | 16 ++++- .../block_building_internal_test.go | 61 +++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index a624869ebfe..5b396e61e00 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -72,7 +72,11 @@ func NewBlockBuilder(ctx context.Context, build BlockBuilderFunc, param *Paramet t := time.Now() result, err = build(buildCtx, param, &builder.interrupt) if err != nil { - log.Warn("Failed to build a block", "err", err) + if buildCtx.Err() != nil { + log.Debug("Block builder discarded", "err", err) + } else { + log.Warn("Failed to build a block", "err", err) + } } else { block := result.Block log.Info("Built block", "hash", block.Hash(), "height", block.NumberU64(), "txs", len(block.Transactions()), "executionRequests", len(result.Requests), "gasUsedPct", 100*float64(block.GasUsed())/float64(block.GasLimit()), "time", time.Since(t)) diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 8d273773d64..504264c649b 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -92,6 +92,20 @@ func cloneBuilderParameters(params *builder.Parameters) *builder.Parameters { return &cloned } +// sameBuildRequest reports whether a request is asking for the payload another one is already +// building. A custom transaction provider is never treated as the same request: it is stateful and +// hands its transactions over once, so a second request carrying one is not asking for what the +// first is building, and comparing the provider itself would read fields the running build writes. +func sameBuildRequest(previous, current *builder.Parameters) bool { + if previous == nil || current == nil { + return false + } + if previous.CustomTxnProvider != nil || current.CustomTxnProvider != nil { + return false + } + return reflect.DeepEqual(previous, current) +} + // 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 { @@ -145,7 +159,7 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete if previousID, ok := e.buildersByTimestamp[params.Timestamp]; ok { if previous := e.builders[previousID]; previous != nil && previous.builder != nil && !previous.builder.Failed() { params.PayloadId = previousID - if reflect.DeepEqual(previous.params, params) { + if sameBuildRequest(previous.params, params) { e.logger.Info("[ForkChoiceUpdated] duplicate build request") return AssembleBlockResult{PayloadID: previousID}, nil } diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index 3fbafb5a5e4..5e94a7ee397 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -34,6 +34,7 @@ import ( "github.com/erigontech/erigon/execution/chain" "github.com/erigontech/erigon/execution/engineapi/engine_helpers" "github.com/erigontech/erigon/execution/types" + "github.com/erigontech/erigon/txnprovider" ) func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { @@ -354,6 +355,66 @@ func TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUpMidStop(t *testing.T) require.NotNil(t, assembled.Block) } +// mutableTxnProvider stands in for the stateful providers the testing namespace supplies: it hands +// its transactions over once and clears them, from the build goroutine. +type mutableTxnProvider struct { + txns []types.Transaction + done atomic.Bool +} + +func (m *mutableTxnProvider) ProvideTxns(context.Context, ...txnprovider.ProvideOption) ([]types.Transaction, error) { + if !m.done.CompareAndSwap(false, true) { + return nil, nil + } + txns := m.txns + m.txns = nil + return txns, nil +} + +func TestAssembleBlockNeverReusesABuilderWithACustomProvider(t *testing.T) { + started := make(chan struct{}, 4) + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(ctx context.Context, params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- struct{}{} + // Keep the provider busy for the whole test, which is when a comparison would read it. + for !interrupt.Load() && ctx.Err() == nil { + if params.CustomTxnProvider != nil { + _, _ = params.CustomTxnProvider.ProvideTxns(ctx) + } + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }, + } + + withProvider := func() *builder.Parameters { + return &builder.Parameters{ + Timestamp: 100, + ParentHash: common.Hash{0x01}, + CustomTxnProvider: &mutableTxnProvider{txns: []types.Transaction{}}, + } + } + first, err := module.AssembleBlock(t.Context(), withProvider()) + require.NoError(t, err) + <-started + + // The provider is single-shot and mutates as it runs, so a second request carrying one is not + // asking for what the first is building, and its fields must never be compared. + second, err := module.AssembleBlock(t.Context(), withProvider()) + require.NoError(t, err) + require.NotEqual(t, first.PayloadID, second.PayloadID) + <-started + + for _, entry := range module.builders { + entry.builder.Discard() + } +} + func TestAssembleBlockOwnsParameters(t *testing.T) { type observedParameters struct { parentRoot common.Hash From d40805156b8970886cb67f2e9188ec4b1289fb90 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 16:15:11 +0200 Subject: [PATCH 5/7] execution: ask the builder whether the build failed, rather than reading the error Stop reports the caller's wait expiring and the build's own failure through the same error, and a build can fail with a context error of its own: the Shutter provider wraps one when its parent-block wait runs out. Inspecting the error therefore kept a genuinely failed builder and served its latched error to every later retry of the slot, which is the case this change exists to prevent. The builder knows which happened, so ask it. --- execution/execmodule/block_building.go | 10 +++---- .../block_building_internal_test.go | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 504264c649b..a8ce4392831 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -19,7 +19,6 @@ package execmodule import ( "bytes" "context" - "errors" "reflect" "time" @@ -219,10 +218,11 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { - // The caller gave up waiting; nothing about the build itself went wrong. Reading that from - // the returned error rather than the ambient context keeps the two from drifting apart - // between Stop returning and this check. - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // Stop reports the caller's wait expiring and the build's own failure through the same + // error, and a build can fail with a context error of its own - a transaction provider + // giving up, say. Only the builder knows which happened, so ask it rather than guess from + // the error: a caller that gave up leaves a builder still worth collecting. + if !entry.builder.Failed() { return AssembledBlockResult{}, err } // Keeping a failed entry would hand its latched error to every retry. diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index 5e94a7ee397..1da8c27da6e 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -19,6 +19,7 @@ package execmodule import ( "context" "errors" + "fmt" "math" "sync/atomic" "testing" @@ -573,3 +574,28 @@ func TestBuildDurationCapsOverflowingTimestamp(t *testing.T) { // the floor instead of the cap. require.Equal(t, 24*time.Second, buildDuration(math.MaxUint64, now, 12)) } + +func TestGetAssembledBlockDropsABuildThatFailedWithAContextError(t *testing.T) { + module := &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + bacgroundCtx: t.Context(), + builderFunc: func(context.Context, *builder.Parameters, *atomic.Bool) (*types.BlockWithReceipts, error) { + // A transaction provider that gives up reports its own context error, which is a failed + // build rather than a caller that stopped waiting. + return nil, fmt.Errorf("issue while waiting for parent block: %w", context.DeadlineExceeded) + }, + } + + 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.ErrorIs(t, err, context.DeadlineExceeded) + + // Keeping it would serve that latched error to every later retry of the same slot. + require.NotContains(t, module.builders, result.PayloadID) + require.NotContains(t, module.buildersByTimestamp, uint64(100)) +} From a5c33c430f1d41606cf3c663f5176501b96809e9 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 15:12:56 +0200 Subject: [PATCH 6/7] execution: bound a build that will not stop, and keep eviction off a live proposal Reaching the maximum build time asked for the block and then waited on the answer forever. A build parked in something that never reads the interrupt flag - a transaction provider waiting on a block, say - held its read view until the builder count forced it out, which on a quiet node is a very long time. It is given a short grace period to hand the block over, and discarded if it will not. Only safe now that a cancelled caller no longer leaves the pool lock held. Eviction went strictly by age, which could take the builder a timestamp still resolves to - the one a proposal for that timestamp is waiting on. Age says nothing about that, so those are skipped, and the count is left above the bound if everything is current, which is the safer of the two ways to be wrong. An id with no builder behind it read as an ordinary empty result, indistinguishable from a build still running, so a caller polled it for the rest of the slot. It says so now. Also from review: the parameters copy moves next to the struct it copies, with a test that fails when a field is added to it; the payload id is no longer written into the caller's parameters just to be compared; eviction and dropping share one path; the timestamp index is built with the module rather than on first use; and the tests share one fixture with timestamps that keep the real build watchdog from firing in the middle of them. --- execution/builder/block_builder.go | 19 +- execution/builder/block_builder_test.go | 41 ++ execution/builder/parameters.go | 35 ++ execution/builder/parameters_test.go | 72 +++ execution/execmodule/block_building.go | 106 ++--- .../block_building_internal_test.go | 420 ++++++++---------- .../execmodule/chainreader/chain_reader.go | 4 + execution/execmodule/exec_module.go | 1 + execution/execmodule/interface.go | 8 +- 9 files changed, 409 insertions(+), 297 deletions(-) create mode 100644 execution/builder/parameters_test.go diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 5b396e61e00..aa9a0b610b1 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -28,6 +28,11 @@ import ( "github.com/erigontech/erigon/execution/types" ) +// buildStopGrace is how long a builder asked to stop is given to hand its block over. It is sized +// against how often the build loop looks at the interrupt flag, not against the slot: a build that +// is cooperating answers within one of those polls, and one that is not never will. +const buildStopGrace = 500 * time.Millisecond + // BlockBuilderFunc builds a payload. Its context ends when the payload is discarded, so anything // that can block - opening a read view, waiting on a transaction provider - has to honour it. type BlockBuilderFunc func(ctx context.Context, param *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) @@ -88,13 +93,19 @@ func NewBlockBuilder(ctx context.Context, build BlockBuilderFunc, param *Paramet defer timer.Stop() select { case <-timer.C: - log.Warn("Stopping block builder due to max build time exceeded") - _, _ = builder.Stop(context.Background()) - log.Debug("Stopped block builder due to max build time exceeded") - return case <-builder.done: return } + // Ask for the block it has, which is what the budget was for, but do not wait on it + // indefinitely: a build parked somewhere that never reads the flag would hold its read view + // until the builder count forced it out, which on a quiet node is a very long time. + log.Warn("Stopping block builder due to max build time exceeded") + graceCtx, cancelGrace := context.WithTimeout(ctx, buildStopGrace) + defer cancelGrace() + if _, err := builder.Stop(graceCtx); err != nil { + builder.Discard() + } + log.Debug("Stopped block builder due to max build time exceeded") }() return builder diff --git a/execution/builder/block_builder_test.go b/execution/builder/block_builder_test.go index 5b2a1b8ca73..ebd634d47eb 100644 --- a/execution/builder/block_builder_test.go +++ b/execution/builder/block_builder_test.go @@ -82,3 +82,44 @@ func TestBlockBuilderStaysReusableOnceItFillsTheBlock(t *testing.T) { // A builder that ran out of room holds a complete payload, so its id is still worth reusing. require.Never(t, b.Failed, 50*time.Millisecond, 5*time.Millisecond) } + +func TestBlockBuilderReleasesABuildThatIgnoresTheDeadline(t *testing.T) { + t.Parallel() + + // A build parked in something that never reads the interrupt flag - a transaction provider + // waiting on a block, say - would hold its read view until the builder count forced it out. + released := make(chan error, 1) + b := NewBlockBuilder(t.Context(), func(ctx context.Context, _ *Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { + select { + case <-ctx.Done(): + released <- ctx.Err() + case <-time.After(time.Minute): + released <- errors.New("build was never released") + } + return nil, errors.New("builder stopped") + }, &Parameters{}, time.Millisecond) + + select { + case err := <-released: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(10 * time.Second): + t.Fatal("build outlived its budget without being released") + } + require.Eventually(t, b.Failed, 5*time.Second, time.Millisecond) +} + +func TestBlockBuilderStillHandsOverAPayloadWhenItsBudgetRunsOut(t *testing.T) { + t.Parallel() + + // Reaching the budget asks for the block it has, which is what the budget is for. Only a build + // that will not answer is discarded. + b := NewBlockBuilder(t.Context(), func(_ context.Context, _ *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }, &Parameters{}, time.Millisecond) + + require.Eventually(t, func() bool { return b.Block() != nil }, 5*time.Second, time.Millisecond) + require.False(t, b.Failed()) +} diff --git a/execution/builder/parameters.go b/execution/builder/parameters.go index 10d063fd37d..351cf93c9a4 100644 --- a/execution/builder/parameters.go +++ b/execution/builder/parameters.go @@ -17,6 +17,8 @@ package builder import ( + "bytes" + "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/execution/types" "github.com/erigontech/erigon/txnprovider" @@ -40,3 +42,36 @@ type Parameters struct { // ExtraData overrides the builder's configured extra data when non-nil. ExtraData []byte } + +// Copy returns parameters that no longer share anything mutable with the receiver, so a caller +// cannot change what a builder was asked for after the fact. Reference-typed fields added to +// Parameters have to be handled here; TestParametersCopyCoversEveryField fails if one is not. +func (p *Parameters) Copy() *Parameters { + if p == nil { + return nil + } + copied := *p + copied.ExtraData = bytes.Clone(p.ExtraData) + if p.Withdrawals != nil { + copied.Withdrawals = make([]*types.Withdrawal, len(p.Withdrawals)) + for i, withdrawal := range p.Withdrawals { + if withdrawal != nil { + w := *withdrawal + copied.Withdrawals[i] = &w + } + } + } + if p.ParentBeaconBlockRoot != nil { + root := *p.ParentBeaconBlockRoot + copied.ParentBeaconBlockRoot = &root + } + if p.SlotNumber != nil { + slot := *p.SlotNumber + copied.SlotNumber = &slot + } + if p.TargetGasLimit != nil { + limit := *p.TargetGasLimit + copied.TargetGasLimit = &limit + } + return &copied +} diff --git a/execution/builder/parameters_test.go b/execution/builder/parameters_test.go new file mode 100644 index 00000000000..80270049c4f --- /dev/null +++ b/execution/builder/parameters_test.go @@ -0,0 +1,72 @@ +// 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 ( + "reflect" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/common" + "github.com/erigontech/erigon/execution/types" +) + +func TestParametersCopyKeepsNothingShared(t *testing.T) { + t.Parallel() + + require.Nil(t, (*Parameters)(nil).Copy()) + + // An empty slice is not the same request as an absent one, so the distinction has to survive. + empty := (&Parameters{Withdrawals: []*types.Withdrawal{}, ExtraData: []byte{}}).Copy() + require.NotNil(t, empty.Withdrawals) + require.NotNil(t, empty.ExtraData) + + root := common.Hash{0x01} + slot := uint64(2) + gasLimit := uint64(3) + params := &Parameters{ + Withdrawals: []*types.Withdrawal{nil, {Index: 4}}, + ParentBeaconBlockRoot: &root, + SlotNumber: &slot, + TargetGasLimit: &gasLimit, + ExtraData: []byte{5}, + } + copied := params.Copy() + + params.Withdrawals[1].Index = 40 + root[0] = 10 + slot = 20 + gasLimit = 30 + params.ExtraData[0] = 50 + + require.Nil(t, copied.Withdrawals[0]) + require.Equal(t, uint64(4), copied.Withdrawals[1].Index) + require.Equal(t, common.Hash{0x01}, *copied.ParentBeaconBlockRoot) + require.Equal(t, uint64(2), *copied.SlotNumber) + require.Equal(t, uint64(3), *copied.TargetGasLimit) + require.Equal(t, byte(5), copied.ExtraData[0]) +} + +func TestParametersCopyCoversEveryField(t *testing.T) { + t.Parallel() + + // Copy has to be revisited whenever a reference-typed field is added, and nothing else will say + // so: a shallow copy of a new slice or pointer compiles and silently shares it. + require.Equal(t, 11, reflect.TypeFor[Parameters]().NumField(), + "Parameters gained or lost a field; check whether Copy has to copy it") +} diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index a8ce4392831..10e5741beed 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -17,7 +17,6 @@ package execmodule import ( - "bytes" "context" "reflect" "time" @@ -61,36 +60,6 @@ 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 -} - // sameBuildRequest reports whether a request is asking for the payload another one is already // building. A custom transaction provider is never treated as the same request: it is stateful and // hands its transactions over once, so a second request carrying one is not asking for what the @@ -102,39 +71,59 @@ func sameBuildRequest(previous, current *builder.Parameters) bool { if previous.CustomTxnProvider != nil || current.CustomTxnProvider != nil { return false } - return reflect.DeepEqual(previous, current) + // The payload id is this module's to assign, so it is not part of the question, and comparing + // copies keeps the caller's parameters out of it. + withoutID := func(p *builder.Parameters) builder.Parameters { + stripped := *p + stripped.PayloadId = 0 + return stripped + } + previousWithoutID, currentWithoutID := withoutID(previous), withoutID(current) + return reflect.DeepEqual(&previousWithoutID, ¤tWithoutID) } -// 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. +// builderEntry keeps a builder with the immutable parameters it was created for, so the two cannot +// drift apart and eviction can drop the timestamp index without scanning it. type builderEntry struct { - builder *builder.BlockBuilder - params *builder.Parameters - timestamp uint64 + builder *builder.BlockBuilder + params *builder.Parameters } +// isCurrentFor reports whether this entry is the one its timestamp resolves to, which is the one a +// proposal for that timestamp is waiting on. +func (e *ExecModule) isCurrentFor(id uint64, entry *builderEntry) bool { + return entry != nil && entry.params != nil && e.buildersByTimestamp[entry.params.Timestamp] == id +} + +// dropBuilder removes a builder and releases whatever it is holding. func (e *ExecModule) dropBuilder(id uint64, entry *builderEntry) { - if e.buildersByTimestamp[entry.timestamp] == id { - delete(e.buildersByTimestamp, entry.timestamp) + if entry != nil { + if e.isCurrentFor(id, entry) { + delete(e.buildersByTimestamp, entry.params.Timestamp) + } + if entry.builder != nil { + entry.builder.Discard() + } } delete(e.builders, id) } +// evictOldBuilders drops the oldest builders so that at most MaxBuilders - 1 remain, skipping any +// that a timestamp still resolves to: those are what a proposal is waiting on, and being old by id +// says nothing about that. If every remaining builder is current the count is left above the bound, +// which is the safer of the two ways to be wrong. func (e *ExecModule) evictOldBuilders() { - ids := common.SortedKeys(e.builders) - - // remove old builders so that at most MaxBuilders - 1 remain - for i := 0; i <= len(e.builders)-engine_helpers.MaxBuilders; i++ { - id := ids[i] - if old := e.builders[id]; old != nil { - if old.builder != nil { - old.builder.Discard() - } - if e.buildersByTimestamp[old.timestamp] == id { - delete(e.buildersByTimestamp, old.timestamp) - } + remaining := len(e.builders) - engine_helpers.MaxBuilders + 1 + for _, id := range common.SortedKeys(e.builders) { + if remaining <= 0 { + return + } + entry := e.builders[id] + if e.isCurrentFor(id, entry) { + continue } - delete(e.builders, id) + e.dropBuilder(id, entry) + remaining-- } } @@ -157,7 +146,6 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete // 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 { if previous := e.builders[previousID]; previous != nil && previous.builder != nil && !previous.builder.Failed() { - params.PayloadId = previousID if sameBuildRequest(previous.params, params) { e.logger.Info("[ForkChoiceUpdated] duplicate build request") return AssembleBlockResult{PayloadID: previousID}, nil @@ -171,15 +159,11 @@ func (e *ExecModule) AssembleBlock(ctx context.Context, params *builder.Paramete e.nextPayloadId++ params.PayloadId = e.nextPayloadId - ownedParams := cloneBuilderParameters(params) + ownedParams := params.Copy() - if e.buildersByTimestamp == nil { - e.buildersByTimestamp = make(map[uint64]uint64) - } e.builders[e.nextPayloadId] = &builderEntry{ - builder: builder.NewBlockBuilder(e.bacgroundCtx, e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())), - params: ownedParams, - timestamp: params.Timestamp, + builder: builder.NewBlockBuilder(e.bacgroundCtx, e.builderFunc, ownedParams, buildDuration(params.Timestamp, time.Now(), e.config.SecondsPerSlot())), + params: ownedParams, } e.buildersByTimestamp[params.Timestamp] = e.nextPayloadId e.logger.Info("[ForkChoiceUpdated] BlockBuilder added", "payload", e.nextPayloadId) @@ -214,7 +198,7 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A entry, ok := e.builders[payloadID] if !ok || entry == nil || entry.builder == nil { - return AssembledBlockResult{}, nil + return AssembledBlockResult{Unknown: true}, nil } blockWithReceipts, err := entry.builder.Stop(ctx) if err != nil { diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index 1da8c27da6e..8c112043411 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -38,26 +38,41 @@ import ( "github.com/erigontech/erigon/txnprovider" ) +// newTestModule builds a module whose builders run builderFunc. Every test needs the same fields, +// and getting the config or the context wrong changes the builder's own deadline silently. +func newTestModule(t *testing.T, builderFunc builder.BlockBuilderFunc) *ExecModule { + t.Helper() + return &ExecModule{ + logger: log.Root(), + config: &chain.Config{}, + semaphore: semaphore.NewWeighted(1), + builders: map[uint64]*builderEntry{}, + buildersByTimestamp: map[uint64]uint64{}, + bacgroundCtx: t.Context(), + builderFunc: builderFunc, + } +} + +// testTimestamp is far enough ahead that buildDuration gives a builder a budget measured in slots. +// A timestamp in the past takes the floor instead, which is seconds, and the max-build-time +// watchdog then fires in the middle of a test. +func testTimestamp(offset uint64) uint64 { + return uint64(time.Now().Add(time.Hour).Unix()) + offset +} + func TestAssembleBlockKeepsBuildersApartByTimestamp(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]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, 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") - }, - } + module := newTestModule(t, func(_ context.Context, 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 _, entry := range module.builders { if entry != nil && entry.builder != nil { @@ -84,26 +99,26 @@ func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { return result.PayloadID, waitStarted() } - firstID, first := assemble(100, common.Hash{0x01}) - adjacentID, adjacent := assemble(101, common.Hash{0x02}) + firstID, first := assemble(testTimestamp(0), common.Hash{0x01}) + adjacentID, adjacent := assemble(testTimestamp(1), common.Hash{0x02}) require.NotEqual(t, firstID, adjacentID) - firstDuplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + firstDuplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) require.Equal(t, firstID, firstDuplicate.PayloadID) // 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}) + secondID, second := assemble(testTimestamp(0), common.Hash{0x03}) require.NotEqual(t, firstID, secondID) - require.Equal(t, secondID, module.buildersByTimestamp[100]) - require.Equal(t, adjacentID, module.buildersByTimestamp[101]) + require.Equal(t, secondID, module.buildersByTimestamp[testTimestamp(0)]) + require.Equal(t, adjacentID, module.buildersByTimestamp[testTimestamp(1)]) - thirdID, third := assemble(100, common.Hash{0x04}) + thirdID, third := assemble(testTimestamp(0), common.Hash{0x04}) require.NotEqual(t, secondID, thirdID) - require.Equal(t, thirdID, module.buildersByTimestamp[100]) + require.Equal(t, thirdID, module.buildersByTimestamp[testTimestamp(0)]) - duplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x04}}) + duplicate, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x04}}) require.NoError(t, err) require.Equal(t, thirdID, duplicate.PayloadID) @@ -111,21 +126,22 @@ func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { 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. - // Removing entries puts them beyond the registered cleanup, so release them here instead of - // leaving two goroutines running until their watchdogs fire. - module.builders[firstID].builder.Discard() - module.builders[secondID].builder.Discard() - delete(module.builders, firstID) - delete(module.builders, secondID) + // Eviction is where a builder is actually stopped, and it goes by age. What a timestamp still + // resolves to is exempt whatever its age, because that is what a proposal for that timestamp is + // waiting on: first and second have been superseded, third and adjacent have not. 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)) - require.False(t, third.interrupt.Load(), "the current builder for a timestamp must survive eviction") + + require.NotContains(t, module.builders, firstID) + require.Eventually(t, first.interrupt.Load, time.Second, time.Millisecond) + require.Contains(t, module.builders, adjacentID, "the current builder for a timestamp must survive eviction") + require.Contains(t, module.builders, thirdID) + require.False(t, adjacent.interrupt.Load()) + require.False(t, third.interrupt.Load()) + require.Equal(t, thirdID, module.buildersByTimestamp[testTimestamp(0)]) + require.Equal(t, adjacentID, module.buildersByTimestamp[testTimestamp(1)]) } func TestEvictionReleasesABuilderBlockedOnItsProvider(t *testing.T) { @@ -133,36 +149,34 @@ func TestEvictionReleasesABuilderBlockedOnItsProvider(t *testing.T) { // is not read until it does. Only cancelling the build reaches it. entered := make(chan struct{}, 1) released := make(chan error, 1) - module := &ExecModule{ - logger: log.Root(), - config: &chain.Config{}, - semaphore: semaphore.NewWeighted(1), - builders: map[uint64]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(ctx context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { - entered <- struct{}{} - select { - case <-ctx.Done(): - released <- ctx.Err() - return nil, ctx.Err() - case <-time.After(time.Minute): - released <- errors.New("provider was never released") - return nil, errors.New("provider was never released") - } - }, - } + module := newTestModule(t, func(ctx context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + entered <- struct{}{} + select { + case <-ctx.Done(): + released <- ctx.Err() + return nil, ctx.Err() + case <-time.After(time.Minute): + released <- errors.New("provider was never released") + return nil, errors.New("provider was never released") + } + }) - result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + blocked, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) <-entered - entry := module.builders[result.PayloadID] + // Superseded, so eviction is allowed to take it: what a timestamp still resolves to is exempt. + _, err = module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x02}}) + require.NoError(t, err) + <-entered + + entry := module.builders[blocked.PayloadID] require.NotNil(t, entry) - for id := result.PayloadID + 1; len(module.builders) < engine_helpers.MaxBuilders; id++ { + for id := uint64(len(module.builders)) + 100; len(module.builders) < engine_helpers.MaxBuilders; id++ { module.builders[id] = nil } module.evictOldBuilders() - require.NotContains(t, module.builders, result.PayloadID) + require.NotContains(t, module.builders, blocked.PayloadID) // Observing the goroutine finish is the point: an evicted builder that merely has its flag set // keeps its read view open until whatever it is blocked on gives up on its own. @@ -177,33 +191,26 @@ func TestEvictionReleasesABuilderBlockedOnItsProvider(t *testing.T) { 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{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { - started <- interrupt - for !interrupt.Load() { - time.Sleep(time.Millisecond) - } - return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil - }, - } + module := newTestModule(t, func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- interrupt + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }) - first, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + first, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) firstInterrupt := <-started - second, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x02}}) + second, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), 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.Equal(t, second.PayloadID, module.buildersByTimestamp[testTimestamp(0)]) require.False(t, firstInterrupt.Load()) assembled, err := module.GetAssembledBlock(t.Context(), first.PayloadID) @@ -215,23 +222,16 @@ func TestSupersededBuilderKeepsPackingAndStaysRetrievable(t *testing.T) { 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{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, _ *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 - }, - } + module := newTestModule(t, func(_ context.Context, _ *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}} + return &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}} } first, err := module.AssembleBlock(t.Context(), params()) require.NoError(t, err) @@ -253,26 +253,19 @@ 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{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, _ *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") - }, - } + module := newTestModule(t, func(_ context.Context, _ *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}} + return &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}} } first, err := module.AssembleBlock(t.Context(), params()) require.NoError(t, err) @@ -290,18 +283,11 @@ func TestAssembleBlockDoesNotReuseFailedBuilder(t *testing.T) { } func TestGetAssembledBlockDropsFailedBuilder(t *testing.T) { - module := &ExecModule{ - logger: log.Root(), - config: &chain.Config{}, - semaphore: semaphore.NewWeighted(1), - builders: map[uint64]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, _ *builder.Parameters, _ *atomic.Bool) (*types.BlockWithReceipts, error) { - return nil, errors.New("build failed") - }, - } + module := newTestModule(t, func(_ context.Context, _ *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}}) + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) _, err = module.GetAssembledBlock(t.Context(), result.PayloadID) @@ -309,29 +295,22 @@ func TestGetAssembledBlockDropsFailedBuilder(t *testing.T) { // 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)) + require.NotContains(t, module.buildersByTimestamp, testTimestamp(0)) } func TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUpMidStop(t *testing.T) { interrupted := make(chan struct{}) release := make(chan struct{}) - module := &ExecModule{ - logger: log.Root(), - config: &chain.Config{}, - semaphore: semaphore.NewWeighted(1), - builders: map[uint64]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { - for !interrupt.Load() { - time.Sleep(time.Millisecond) - } - close(interrupted) - <-release - return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil - }, - } + module := newTestModule(t, func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + close(interrupted) + <-release + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }) - result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) // Cancel while Stop is already waiting, which is the window a caller-side timeout actually @@ -348,7 +327,7 @@ func TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUpMidStop(t *testing.T) // The builder was not dropped, so the payload it goes on to produce is still reachable. require.Contains(t, module.builders, result.PayloadID) - require.Equal(t, result.PayloadID, module.buildersByTimestamp[100]) + require.Equal(t, result.PayloadID, module.buildersByTimestamp[testTimestamp(0)]) close(release) assembled, err := module.GetAssembledBlock(t.Context(), result.PayloadID) @@ -374,24 +353,17 @@ func (m *mutableTxnProvider) ProvideTxns(context.Context, ...txnprovider.Provide func TestAssembleBlockNeverReusesABuilderWithACustomProvider(t *testing.T) { started := make(chan struct{}, 4) - module := &ExecModule{ - logger: log.Root(), - config: &chain.Config{}, - semaphore: semaphore.NewWeighted(1), - builders: map[uint64]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(ctx context.Context, params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { - started <- struct{}{} - // Keep the provider busy for the whole test, which is when a comparison would read it. - for !interrupt.Load() && ctx.Err() == nil { - if params.CustomTxnProvider != nil { - _, _ = params.CustomTxnProvider.ProvideTxns(ctx) - } - time.Sleep(time.Millisecond) + module := newTestModule(t, func(ctx context.Context, params *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- struct{}{} + // Keep the provider busy for the whole test, which is when a comparison would read it. + for !interrupt.Load() && ctx.Err() == nil { + if params.CustomTxnProvider != nil { + _, _ = params.CustomTxnProvider.ProvideTxns(ctx) } - return nil, errors.New("builder stopped") - }, - } + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }) withProvider := func() *builder.Parameters { return &builder.Parameters{ @@ -423,21 +395,14 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { } readParameters := make(chan struct{}) observed := make(chan observedParameters, 1) - module := &ExecModule{ - semaphore: semaphore.NewWeighted(1), - config: &chain.Config{}, - logger: log.Root(), - builders: map[uint64]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, 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") - }, - } + module := newTestModule(t, func(_ context.Context, 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, @@ -467,21 +432,14 @@ func TestAssembleBlockOwnsParameters(t *testing.T) { 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]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(_ context.Context, _ *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}}) + module := newTestModule(t, func(_ context.Context, _ *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: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) interrupt := <-started t.Cleanup(func() { @@ -490,42 +448,10 @@ func TestAssembleBlockCanceledContextDoesNotSupersedeBuilder(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() - _, err = module.AssembleBlock(ctx, &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x02}}) + _, err = module.AssembleBlock(ctx, &builder.Parameters{Timestamp: testTimestamp(0), 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)) - - 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]) + require.Equal(t, result.PayloadID, module.buildersByTimestamp[testTimestamp(0)]) } func TestBuildDuration(t *testing.T) { @@ -576,20 +502,13 @@ func TestBuildDurationCapsOverflowingTimestamp(t *testing.T) { } func TestGetAssembledBlockDropsABuildThatFailedWithAContextError(t *testing.T) { - module := &ExecModule{ - logger: log.Root(), - config: &chain.Config{}, - semaphore: semaphore.NewWeighted(1), - builders: map[uint64]*builderEntry{}, - bacgroundCtx: t.Context(), - builderFunc: func(context.Context, *builder.Parameters, *atomic.Bool) (*types.BlockWithReceipts, error) { - // A transaction provider that gives up reports its own context error, which is a failed - // build rather than a caller that stopped waiting. - return nil, fmt.Errorf("issue while waiting for parent block: %w", context.DeadlineExceeded) - }, - } + module := newTestModule(t, func(context.Context, *builder.Parameters, *atomic.Bool) (*types.BlockWithReceipts, error) { + // A transaction provider that gives up reports its own context error, which is a failed + // build rather than a caller that stopped waiting. + return nil, fmt.Errorf("issue while waiting for parent block: %w", context.DeadlineExceeded) + }) - result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: 100, ParentHash: common.Hash{0x01}}) + result, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) require.NoError(t, err) _, err = module.GetAssembledBlock(t.Context(), result.PayloadID) @@ -597,5 +516,46 @@ func TestGetAssembledBlockDropsABuildThatFailedWithAContextError(t *testing.T) { // Keeping it would serve that latched error to every later retry of the same slot. require.NotContains(t, module.builders, result.PayloadID) - require.NotContains(t, module.buildersByTimestamp, uint64(100)) + require.NotContains(t, module.buildersByTimestamp, testTimestamp(0)) +} + +func TestGetAssembledBlockSaysWhenAPayloadIdIsUnknown(t *testing.T) { + module := newTestModule(t, func(context.Context, *builder.Parameters, *atomic.Bool) (*types.BlockWithReceipts, error) { + return nil, errors.New("builder stopped") + }) + + // An id with no builder behind it can never produce anything. Reporting it as an ordinary empty + // result leaves a caller polling it for the rest of the slot. + assembled, err := module.GetAssembledBlock(t.Context(), 404) + require.NoError(t, err) + require.True(t, assembled.Unknown) + require.Nil(t, assembled.Block) +} + +func TestEvictionSparesTheBuilderATimestampStillPointsAt(t *testing.T) { + started := make(chan struct{}, 1) + module := newTestModule(t, func(_ context.Context, _ *builder.Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + started <- struct{}{} + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + return nil, errors.New("builder stopped") + }) + + current, err := module.AssembleBlock(t.Context(), &builder.Parameters{Timestamp: testTimestamp(0), ParentHash: common.Hash{0x01}}) + require.NoError(t, err) + <-started + + // It is the oldest by id, so eviction would take it first, and it is also what a proposal for + // that timestamp is waiting on. Age says nothing about that. + for id := current.PayloadID + 1; len(module.builders) < engine_helpers.MaxBuilders+1; id++ { + module.builders[id] = nil + } + module.evictOldBuilders() + + require.Contains(t, module.builders, current.PayloadID) + require.Equal(t, current.PayloadID, module.buildersByTimestamp[testTimestamp(0)]) + require.False(t, module.builders[current.PayloadID].builder.Failed()) + + module.builders[current.PayloadID].builder.Discard() } diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index cd2082c09f5..2f7d303e09a 100644 --- a/execution/execmodule/chainreader/chain_reader.go +++ b/execution/execmodule/chainreader/chain_reader.go @@ -283,6 +283,10 @@ func (c ChainReaderWriterEth1) HasBlock(ctx context.Context, hash common.Hash) ( // own, as opposed to a rejection that returns the same answer however many times it is asked. var ErrExecutionBusy = errors.New("execution module is busy") +// ErrUnknownPayload reports that no builder is held for a payload id, so nothing will ever arrive +// for it. Without it a caller polling that id cannot tell it from a build still running. +var ErrUnknownPayload = errors.New("unknown payload id") + func (c ChainReaderWriterEth1) AssembleBlock(ctx context.Context, baseHash common.Hash, attributes *engine_types.PayloadAttributes) (id uint64, err error) { params := &builder.Parameters{ ParentHash: baseHash, diff --git a/execution/execmodule/exec_module.go b/execution/execmodule/exec_module.go index 292b7b70df7..0ef2ba3e824 100644 --- a/execution/execmodule/exec_module.go +++ b/execution/execmodule/exec_module.go @@ -267,6 +267,7 @@ func NewExecModule( forkValidator: forkValidator, pipelineExecutor: pipelineExecutor, builders: make(map[uint64]*builderEntry), + buildersByTimestamp: make(map[uint64]uint64), builderFunc: builderFunc, config: config, semaphore: semaphore.NewWeighted(1), diff --git a/execution/execmodule/interface.go b/execution/execmodule/interface.go index 044d53b7472..25f993b7b4f 100644 --- a/execution/execmodule/interface.go +++ b/execution/execmodule/interface.go @@ -93,10 +93,14 @@ type AssembleBlockResult struct { // AssembledBlockResult is the native return type for GetAssembledBlock. type AssembledBlockResult struct { - // Busy is true when the builder has not finished yet. + // Busy is true when the module was already occupied, not when the builder is still working: + // otherwise the call waits for the builder to finish. Busy bool + // Unknown is true when no builder is held for the payload id, so nothing will ever arrive for + // it. A caller polling that id cannot otherwise tell it from a build still running. + Unknown bool // Block holds the assembled block with receipts and requests. - // Nil when Busy is true or when no builder was found for the payload ID. + // Nil when Busy or Unknown is true, or when the builder produced nothing. Block *types.BlockWithReceipts BlockValue *uint256.Int } From 584f859e41d8cd5d2fb441901ab9a968cf28d78f Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 15:59:49 +0200 Subject: [PATCH 7/7] execution: keep the builder cache bounded while protecting a live proposal Skipping every entry a timestamp resolved to was not a rare case: ordinary traffic is one builder per slot, each with its own timestamp, and a successful entry stays indexed. So every entry qualified, eviction skipped all of them, and both maps grew for the life of the process. Only a slot that has not passed can still be waited on, so that is what is protected now. There are at most a couple of those at any moment, which leaves the bound intact. An implausible timestamp is compared in seconds rather than as a time, so it wraps into "not live" instead of overflowing into the past. The unknown payload id never reached the consensus layer: the sentinel was declared but the result was not consulted, so an id with no builder behind it still came back as an ordinary empty result. --- execution/execmodule/block_building.go | 37 +++++++++--- .../block_building_internal_test.go | 29 ++++++++-- .../execmodule/chainreader/chain_reader.go | 3 + .../chainreader/chain_reader_test.go | 56 +++++++++++++++++++ 4 files changed, 112 insertions(+), 13 deletions(-) create mode 100644 execution/execmodule/chainreader/chain_reader_test.go diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index 10e5741beed..ad2ab981120 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -89,16 +89,32 @@ type builderEntry struct { params *builder.Parameters } -// isCurrentFor reports whether this entry is the one its timestamp resolves to, which is the one a -// proposal for that timestamp is waiting on. -func (e *ExecModule) isCurrentFor(id uint64, entry *builderEntry) bool { +// isIndexedFor reports whether the timestamp index still resolves to this entry, which is what has +// to be cleaned up when the entry goes. +func (e *ExecModule) isIndexedFor(id uint64, entry *builderEntry) bool { return entry != nil && entry.params != nil && e.buildersByTimestamp[entry.params.Timestamp] == id } +// isLiveProposalTarget reports whether an entry is what a proposal is still waiting on: the builder +// its timestamp resolves to, for a slot that has not passed. Being indexed is not enough on its own, +// because every slot has its own timestamp and successful entries stay indexed: protecting all of +// them would mean never evicting anything. +func (e *ExecModule) isLiveProposalTarget(id uint64, entry *builderEntry, now time.Time) bool { + if !e.isIndexedFor(id, entry) { + return false + } + // Compared as seconds rather than times, so an implausible timestamp wraps into "not live" + // instead of overflowing into the past. + nowSeconds := uint64(max(now.Unix(), 0)) + slotSeconds := e.config.SecondsPerSlot() + timestamp := entry.params.Timestamp + return timestamp+slotSeconds > nowSeconds && timestamp <= nowSeconds+2*slotSeconds +} + // dropBuilder removes a builder and releases whatever it is holding. func (e *ExecModule) dropBuilder(id uint64, entry *builderEntry) { if entry != nil { - if e.isCurrentFor(id, entry) { + if e.isIndexedFor(id, entry) { delete(e.buildersByTimestamp, entry.params.Timestamp) } if entry.builder != nil { @@ -108,18 +124,21 @@ func (e *ExecModule) dropBuilder(id uint64, entry *builderEntry) { delete(e.builders, id) } -// evictOldBuilders drops the oldest builders so that at most MaxBuilders - 1 remain, skipping any -// that a timestamp still resolves to: those are what a proposal is waiting on, and being old by id -// says nothing about that. If every remaining builder is current the count is left above the bound, -// which is the safer of the two ways to be wrong. +// evictOldBuilders drops the oldest builders so that at most MaxBuilders - 1 remain, skipping only +// those a proposal is still waiting on. Being old by id says nothing about that, and there are only +// ever a couple of live targets, so the bound holds. func (e *ExecModule) evictOldBuilders() { remaining := len(e.builders) - engine_helpers.MaxBuilders + 1 + if remaining <= 0 { + return + } + now := time.Now() for _, id := range common.SortedKeys(e.builders) { if remaining <= 0 { return } entry := e.builders[id] - if e.isCurrentFor(id, entry) { + if e.isLiveProposalTarget(id, entry, now) { continue } e.dropBuilder(id, entry) diff --git a/execution/execmodule/block_building_internal_test.go b/execution/execmodule/block_building_internal_test.go index 8c112043411..5eceecfb683 100644 --- a/execution/execmodule/block_building_internal_test.go +++ b/execution/execmodule/block_building_internal_test.go @@ -53,11 +53,11 @@ func newTestModule(t *testing.T, builderFunc builder.BlockBuilderFunc) *ExecModu } } -// testTimestamp is far enough ahead that buildDuration gives a builder a budget measured in slots. -// A timestamp in the past takes the floor instead, which is seconds, and the max-build-time -// watchdog then fires in the middle of a test. +// testTimestamp is a slot that has not happened yet but is close enough to be one a proposal could +// be waiting for. Far enough ahead that buildDuration gives a budget measured in slots, so the +// max-build-time watchdog cannot fire mid-test, and near enough that the slot still counts as live. func testTimestamp(offset uint64) uint64 { - return uint64(time.Now().Add(time.Hour).Unix()) + offset + return uint64(time.Now().Add(10*time.Second).Unix()) + offset } func TestAssembleBlockKeepsBuildersApartByTimestamp(t *testing.T) { @@ -559,3 +559,24 @@ func TestEvictionSparesTheBuilderATimestampStillPointsAt(t *testing.T) { module.builders[current.PayloadID].builder.Discard() } + +func TestEvictionKeepsTheBuilderCacheBounded(t *testing.T) { + module := newTestModule(t, func(context.Context, *builder.Parameters, *atomic.Bool) (*types.BlockWithReceipts, error) { + return nil, errors.New("builder stopped") + }) + + // Ordinary traffic is one builder per slot, so every entry is the one its own timestamp + // resolves to. If being indexed were enough to protect an entry, nothing would ever be evicted + // and both maps would grow for the life of the process. + past := uint64(time.Now().Add(-time.Hour).Unix()) + for i := range uint64(engine_helpers.MaxBuilders + 8) { + _, err := module.AssembleBlock(t.Context(), &builder.Parameters{ + Timestamp: past + i, + ParentHash: common.Hash{byte(i), byte(i >> 8)}, + }) + require.NoError(t, err) + } + + require.LessOrEqual(t, len(module.builders), engine_helpers.MaxBuilders) + require.LessOrEqual(t, len(module.buildersByTimestamp), engine_helpers.MaxBuilders) +} diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index 2f7d303e09a..37fb13f20e4 100644 --- a/execution/execmodule/chainreader/chain_reader.go +++ b/execution/execmodule/chainreader/chain_reader.go @@ -316,6 +316,9 @@ func (c ChainReaderWriterEth1) GetAssembledBlock(ctx context.Context, id uint64) if result.Busy { return nil, nil, nil, nil, ErrExecutionBusy } + if result.Unknown { + return nil, nil, nil, nil, ErrUnknownPayload + } if result.Block == nil { return nil, nil, nil, nil, nil } diff --git a/execution/execmodule/chainreader/chain_reader_test.go b/execution/execmodule/chainreader/chain_reader_test.go new file mode 100644 index 00000000000..ba870fd4134 --- /dev/null +++ b/execution/execmodule/chainreader/chain_reader_test.go @@ -0,0 +1,56 @@ +// 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 chainreader + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/execution/execmodule" +) + +// assembledBlockStub answers GetAssembledBlock with a fixed result and panics on anything else, so +// a test can drive the one boundary it cares about. +type assembledBlockStub struct { + execmodule.ExecutionModule + result execmodule.AssembledBlockResult +} + +func (s assembledBlockStub) GetAssembledBlock(context.Context, uint64) (execmodule.AssembledBlockResult, error) { + return s.result, nil +} + +func TestGetAssembledBlockDistinguishesAnUnknownIdFromAnEmptyOne(t *testing.T) { + unknown := ChainReaderWriterEth1{executionModule: assembledBlockStub{result: execmodule.AssembledBlockResult{Unknown: true}}} + _, _, _, _, err := unknown.GetAssembledBlock(t.Context(), 1) + + // Nothing will ever arrive for an id with no builder behind it. Reporting that as an ordinary + // empty result leaves a caller polling it for the rest of the slot. + require.ErrorIs(t, err, ErrUnknownPayload) + + busy := ChainReaderWriterEth1{executionModule: assembledBlockStub{result: execmodule.AssembledBlockResult{Busy: true}}} + _, _, _, _, err = busy.GetAssembledBlock(t.Context(), 1) + require.ErrorIs(t, err, ErrExecutionBusy) + + // A builder that simply has nothing yet is neither: the caller should keep waiting. + building := ChainReaderWriterEth1{executionModule: assembledBlockStub{}} + block, _, _, _, err := building.GetAssembledBlock(t.Context(), 1) + require.NoError(t, err) + require.Nil(t, block) +}