From d577aaceb3873cf76498a1931b7798ee8d2965e8 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 13:23:45 +0200 Subject: [PATCH 1/7] execution, cl/phase1: stop discarding a payload that is already built Follow-up to #23273, which made this reachable: BlockBuilder.Stop selected on the caller's context and on the finished payload at once, so when both were ready Go chose between them at random and about half the time returned a cancellation while holding a complete block. The caller had passed context.Background() before, so the race could not fire; now a validator client that times out can lose a proposal that was ready. The finished payload wins. A caller that gave up is also not a build failure, and was being reported as one. It now returns without an error-level record. The busy sentinel moves next to the Busy field it reports, so set_head.go's identically worded error shares its identity instead of only its wording. The hand-rolled cancellable sleep becomes common.Sleep, and the contention that caused a wait is kept in the error rather than replaced by the bare context error. --- .../execution_client_direct.go | 19 +++--- .../execution_client_direct_test.go | 21 ++++--- execution/builder/block_builder.go | 10 +++- execution/builder/block_builder_test.go | 59 +++++++++++++++++++ execution/execmodule/block_building.go | 5 ++ .../execmodule/chainreader/chain_reader.go | 9 +-- execution/execmodule/interface.go | 5 ++ execution/execmodule/set_head.go | 2 +- 8 files changed, 97 insertions(+), 33 deletions(-) create mode 100644 execution/builder/block_builder_test.go diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index b020f5ec7ab..670f3192030 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -164,28 +164,23 @@ func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, ) for attempt := range attempts { if ctxErr := ctx.Err(); ctxErr != nil { + if err != nil { + return 0, fmt.Errorf("%w (last attempt: %w)", ctxErr, err) + } return 0, ctxErr } if id, err = assemble(ctx); err == nil { return id, nil } - if !errors.Is(err, chainreader.ErrExecutionBusy) { + if !errors.Is(err, execmodule.ErrBusy) { return 0, err } if attempt+1 == attempts { break } - timer := time.NewTimer(delay) - select { - case <-ctx.Done(): - if !timer.Stop() { - select { - case <-timer.C: - default: - } - } - return 0, ctx.Err() - case <-timer.C: + if sleepErr := common.Sleep(ctx, delay); sleepErr != nil { + // Keep the contention that caused the wait: it is the reason the caller ran out of time. + return 0, fmt.Errorf("%w (last attempt: %w)", sleepErr, err) } } return 0, err diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go index 6f2af144871..68162e84924 100644 --- a/cl/phase1/execution_client/execution_client_direct_test.go +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -24,7 +24,7 @@ import ( "github.com/stretchr/testify/require" - "github.com/erigontech/erigon/execution/execmodule/chainreader" + "github.com/erigontech/erigon/execution/execmodule" ) func TestRetryAssembleBlockReturnsFirstSuccess(t *testing.T) { @@ -32,7 +32,7 @@ func TestRetryAssembleBlockReturnsFirstSuccess(t *testing.T) { id, err := retryAssembleBlock(t.Context(), 3, time.Millisecond, func(context.Context) (uint64, error) { calls++ if calls < 3 { - return 0, chainreader.ErrExecutionBusy + return 0, execmodule.ErrBusy } return 7, nil }) @@ -45,13 +45,11 @@ func TestRetryAssembleBlockReturnsFirstSuccess(t *testing.T) { func TestRetryAssembleBlockStopsOnRejection(t *testing.T) { rejected := errors.New("withdrawals before shanghai") calls := 0 - _, err := retryAssembleBlock(t.Context(), 30, time.Hour, func(context.Context) (uint64, error) { + _, err := retryAssembleBlock(t.Context(), 30, time.Second, func(context.Context) (uint64, error) { calls++ return 0, rejected }) - // Only contention settles by waiting; a rejection answers the same way however often it is - // asked, so retrying it just burns the slot. require.ErrorIs(t, err, rejected) require.Equal(t, 1, calls) } @@ -60,23 +58,24 @@ func TestRetryAssembleBlockGivesUpAfterAttempts(t *testing.T) { calls := 0 _, err := retryAssembleBlock(t.Context(), 2, time.Millisecond, func(context.Context) (uint64, error) { calls++ - return 0, chainreader.ErrExecutionBusy + return 0, execmodule.ErrBusy }) - require.ErrorIs(t, err, chainreader.ErrExecutionBusy) + require.ErrorIs(t, err, execmodule.ErrBusy) require.Equal(t, 2, calls) } func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) calls := 0 - _, err := retryAssembleBlock(ctx, 30, time.Hour, func(context.Context) (uint64, error) { + _, err := retryAssembleBlock(ctx, 30, time.Second, func(context.Context) (uint64, error) { calls++ cancel() - return 0, chainreader.ErrExecutionBusy + return 0, execmodule.ErrBusy }) require.ErrorIs(t, err, context.Canceled) + require.ErrorIs(t, err, execmodule.ErrBusy, "the contention that caused the wait must survive in the error") require.Equal(t, 1, calls) } @@ -84,9 +83,9 @@ func TestRetryAssembleBlockDoesNotStartWithCanceledContext(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() calls := 0 - _, err := retryAssembleBlock(ctx, 30, time.Hour, func(context.Context) (uint64, error) { + _, err := retryAssembleBlock(ctx, 30, time.Second, func(context.Context) (uint64, error) { calls++ - return 0, chainreader.ErrExecutionBusy + return 0, execmodule.ErrBusy }) require.ErrorIs(t, err, context.Canceled) diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index e77019a08c2..64214e2df2b 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -91,10 +91,16 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, error) { b.interrupt.Store(true) + // A payload that is already built wins over an expired caller. Selecting on both at once + // would pick between them at random and sometimes throw the block away. select { - case <-ctx.Done(): - return nil, ctx.Err() case <-b.done: + default: + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-b.done: + } } b.mu.Lock() diff --git a/execution/builder/block_builder_test.go b/execution/builder/block_builder_test.go new file mode 100644 index 00000000000..cc8ebbc06ab --- /dev/null +++ b/execution/builder/block_builder_test.go @@ -0,0 +1,59 @@ +// 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 ( + "context" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/execution/types" +) + +func TestBlockBuilderStopPrefersAFinishedPayloadOverAnExpiredCaller(t *testing.T) { + t.Parallel() + + built := make(chan struct{}) + b := NewBlockBuilder(func(_ *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) { + for !interrupt.Load() { + time.Sleep(time.Millisecond) + } + defer close(built) + return &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, nil + }, &Parameters{}, time.Minute) + + // Both the payload and the deadline are ready before Stop is called. Selecting over the two at + // once would discard a block that is already built about half the time. + b.interrupt.Store(true) + <-built + // Join once with a live context so the result is latched before the race below is exercised. + first, err := b.Stop(t.Context()) + require.NoError(t, err) + require.NotNil(t, first) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + for range 50 { + result, err := b.Stop(ctx) + require.NoError(t, err) + require.NotNil(t, result) + } +} diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index ef1d94cddaf..f29b7a08702 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -18,6 +18,7 @@ package execmodule import ( "context" + "errors" "reflect" "time" @@ -129,6 +130,10 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := bldr.Stop(ctx) if err != nil { + // A caller that gave up says nothing about the builder, which keeps running. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return AssembledBlockResult{}, err + } e.logger.Error("Failed to build PoS block", "err", err) return AssembledBlockResult{}, err } diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index cd2082c09f5..99d6e928f05 100644 --- a/execution/execmodule/chainreader/chain_reader.go +++ b/execution/execmodule/chainreader/chain_reader.go @@ -18,7 +18,6 @@ package chainreader import ( "context" - "errors" "fmt" "math/big" "time" @@ -279,10 +278,6 @@ func (c ChainReaderWriterEth1) HasBlock(ctx context.Context, hash common.Hash) ( return c.executionModule.HasBlock(ctx, &hash, nil) } -// ErrExecutionBusy reports that the execution module was already occupied, which settles on its -// 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") - func (c ChainReaderWriterEth1) AssembleBlock(ctx context.Context, baseHash common.Hash, attributes *engine_types.PayloadAttributes) (id uint64, err error) { params := &builder.Parameters{ ParentHash: baseHash, @@ -299,7 +294,7 @@ func (c ChainReaderWriterEth1) AssembleBlock(ctx context.Context, baseHash commo return 0, err } if result.Busy { - return 0, ErrExecutionBusy + return 0, execmodule.ErrBusy } return result.PayloadID, nil } @@ -310,7 +305,7 @@ func (c ChainReaderWriterEth1) GetAssembledBlock(ctx context.Context, id uint64) return nil, nil, nil, nil, err } if result.Busy { - return nil, nil, nil, nil, ErrExecutionBusy + return nil, nil, nil, nil, execmodule.ErrBusy } if result.Block == nil { return nil, nil, nil, nil, nil diff --git a/execution/execmodule/interface.go b/execution/execmodule/interface.go index 044d53b7472..2e5c5265f7f 100644 --- a/execution/execmodule/interface.go +++ b/execution/execmodule/interface.go @@ -18,6 +18,7 @@ package execmodule import ( "context" + "errors" "fmt" "github.com/holiman/uint256" @@ -91,6 +92,10 @@ type AssembleBlockResult struct { PayloadID uint64 } +// ErrBusy reports that the execution module was already occupied. It settles on its own, unlike a +// rejection, which returns the same answer however many times it is asked. +var ErrBusy = errors.New("execution module is busy") + // AssembledBlockResult is the native return type for GetAssembledBlock. type AssembledBlockResult struct { // Busy is true when the builder has not finished yet. diff --git a/execution/execmodule/set_head.go b/execution/execmodule/set_head.go index 9f88c56c9f7..608766185f2 100644 --- a/execution/execmodule/set_head.go +++ b/execution/execmodule/set_head.go @@ -54,7 +54,7 @@ func (e *ExecModule) SetHead(ctx context.Context, targetBlock uint64) error { acquireCtx, acquireCancel := context.WithTimeout(ctx, 5*time.Second) defer acquireCancel() if err := e.semaphore.Acquire(acquireCtx, 1); err != nil { - return fmt.Errorf("execution module is busy: %w", err) + return fmt.Errorf("%w: %w", ErrBusy, err) } defer e.semaphore.Release(1) From ea228714eb89402d5a809dd5b7d4dd3c0adce723 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 13:48:17 +0200 Subject: [PATCH 2/7] execution: address review A caller that goes away while waiting for the semaphore is not a busy module, and saying so would invite a retry with nothing to wait for. Only the local timeout reports ErrBusy. The conflation predates this change but was harmless while the error was untyped. Correct a comment that claimed a cancelled caller leaves the builder running: Stop interrupts it either way, and the point is only that this is not a build failure. --- execution/execmodule/block_building.go | 2 +- execution/execmodule/set_head.go | 5 +++ .../execmodule/set_head_internal_test.go | 39 +++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 execution/execmodule/set_head_internal_test.go diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index f29b7a08702..e745bf4bd86 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -130,7 +130,7 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := bldr.Stop(ctx) if err != nil { - // A caller that gave up says nothing about the builder, which keeps running. + // The caller gave up waiting; nothing about the build itself went wrong. if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return AssembledBlockResult{}, err } diff --git a/execution/execmodule/set_head.go b/execution/execmodule/set_head.go index 608766185f2..a8bac195302 100644 --- a/execution/execmodule/set_head.go +++ b/execution/execmodule/set_head.go @@ -54,6 +54,11 @@ func (e *ExecModule) SetHead(ctx context.Context, targetBlock uint64) error { acquireCtx, acquireCancel := context.WithTimeout(ctx, 5*time.Second) defer acquireCancel() if err := e.semaphore.Acquire(acquireCtx, 1); err != nil { + // Only the local timeout means the module was occupied. A caller that went away says + // nothing about it, and reporting that as busy would invite a pointless retry. + if ctx.Err() != nil { + return err + } return fmt.Errorf("%w: %w", ErrBusy, err) } defer e.semaphore.Release(1) diff --git a/execution/execmodule/set_head_internal_test.go b/execution/execmodule/set_head_internal_test.go new file mode 100644 index 00000000000..37aae9af7b0 --- /dev/null +++ b/execution/execmodule/set_head_internal_test.go @@ -0,0 +1,39 @@ +// 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 execmodule + +import ( + "context" + "testing" + + "golang.org/x/sync/semaphore" + + "github.com/stretchr/testify/require" +) + +func TestSetHeadReportsBusyOnlyWhenTheModuleIsOccupied(t *testing.T) { + module := &ExecModule{semaphore: semaphore.NewWeighted(1)} + require.NoError(t, module.semaphore.Acquire(t.Context(), 1)) + + // The caller went away. Nothing is known about the module, so calling it busy would invite a + // retry that has nothing to wait for. + ctx, cancel := context.WithCancel(t.Context()) + cancel() + err := module.SetHead(ctx, 1) + require.ErrorIs(t, err, context.Canceled) + require.NotErrorIs(t, err, ErrBusy) +} From 54ab79d5e6e12e44293ddd7aaceff31ba47a59fd Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 15:53:45 +0200 Subject: [PATCH 3/7] execution: close the remaining window in the payload priority check Probing before the select only narrowed the race: the payload can land while the select is choosing, and both channels are then ready again. Re-check after cancellation is chosen, so a payload that has landed is never traded for it. The regression test closes the completion channel from the context's own Done call, which is read as the select sets itself up, so the interleaving happens on every attempt rather than being a few nanoseconds wide. --- execution/builder/block_builder.go | 26 +++++++++++---- execution/builder/block_builder_test.go | 44 +++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 64214e2df2b..a51facc0963 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -91,15 +91,17 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, error) { b.interrupt.Store(true) - // A payload that is already built wins over an expired caller. Selecting on both at once - // would pick between them at random and sometimes throw the block away. - select { - case <-b.done: - default: + // A payload that has landed wins over an expired caller, however close together the two + // arrive: selecting on both at once would pick between them at random and throw the block + // away. The second check matters as much as the first, because the payload can land while + // the select below is choosing. + if !b.finished() { select { - case <-ctx.Done(): - return nil, ctx.Err() case <-b.done: + case <-ctx.Done(): + if !b.finished() { + return nil, ctx.Err() + } } } @@ -108,6 +110,16 @@ func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, erro return b.result, b.err } +// finished reports whether the build goroutine has stored its outcome. +func (b *BlockBuilder) finished() bool { + select { + case <-b.done: + return true + default: + return false + } +} + 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 index cc8ebbc06ab..c22520dd488 100644 --- a/execution/builder/block_builder_test.go +++ b/execution/builder/block_builder_test.go @@ -18,6 +18,7 @@ package builder import ( "context" + "sync" "sync/atomic" "testing" "time" @@ -57,3 +58,46 @@ func TestBlockBuilderStopPrefersAFinishedPayloadOverAnExpiredCaller(t *testing.T require.NotNil(t, result) } } + +// completeOnDoneCheck closes the builder's completion channel the first time the context's Done +// channel is read, which is when a select is setting itself up. It makes the payload land between +// the priority probe and the select's choice, an interleaving that is otherwise a few nanoseconds +// wide. +type completeOnDoneCheck struct { + context.Context + cancelled chan struct{} + complete func() + once sync.Once +} + +func (c *completeOnDoneCheck) Done() <-chan struct{} { + c.once.Do(c.complete) + return c.cancelled +} + +func (c *completeOnDoneCheck) Err() error { return context.Canceled } + +func TestBlockBuilderStopKeepsAPayloadThatLandsWhileTheCallerGivesUp(t *testing.T) { + t.Parallel() + + // Both channels are ready by the time the select chooses, so it picks between them at random. + // Preferring the payload has to hold on every attempt, not most of them. + for range 300 { + done := make(chan struct{}) + cancelled := make(chan struct{}) + close(cancelled) + b := &BlockBuilder{ + done: done, + result: &types.BlockWithReceipts{Block: types.NewBlock(&types.Header{}, nil, nil, nil, nil)}, + } + ctx := &completeOnDoneCheck{ + Context: t.Context(), + cancelled: cancelled, + complete: func() { close(done) }, + } + + result, err := b.Stop(ctx) + require.NoError(t, err) + require.NotNil(t, result) + } +} From c8596320724c9f39d1dd59ae03371ecc4a796f17 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Fri, 14 Aug 2026 16:28:18 +0200 Subject: [PATCH 4/7] execution: latch which deadline ran out instead of asking afterwards Reading the caller's context after Acquire returns misreads a caller that went away just after the local timeout fired, which is the case that says the module was occupied. The wait now carries its own cause, recorded at the moment it ran out. --- execution/execmodule/set_head.go | 14 ++++++++------ execution/execmodule/set_head_internal_test.go | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/execution/execmodule/set_head.go b/execution/execmodule/set_head.go index a8bac195302..57dc33f1b20 100644 --- a/execution/execmodule/set_head.go +++ b/execution/execmodule/set_head.go @@ -18,6 +18,7 @@ package execmodule import ( "context" + "errors" "fmt" "time" @@ -51,15 +52,16 @@ func getLatestBlockNumber(tx kv.Tx) (uint64, error) { // SetHead rewinds the local chain to the specified block number by unwinding // all staged sync stages. This is the core implementation used by debug_setHead. func (e *ExecModule) SetHead(ctx context.Context, targetBlock uint64) error { - acquireCtx, acquireCancel := context.WithTimeout(ctx, 5*time.Second) + // The cause records which deadline ran out at the moment it did. Reading the caller's context + // afterwards would misread a caller that went away just after the local timeout fired, and + // only the local timeout says the module was occupied. + acquireCtx, acquireCancel := context.WithTimeoutCause(ctx, 5*time.Second, ErrBusy) defer acquireCancel() if err := e.semaphore.Acquire(acquireCtx, 1); err != nil { - // Only the local timeout means the module was occupied. A caller that went away says - // nothing about it, and reporting that as busy would invite a pointless retry. - if ctx.Err() != nil { - return err + if cause := context.Cause(acquireCtx); errors.Is(cause, ErrBusy) { + return fmt.Errorf("%w: %w", ErrBusy, err) } - return fmt.Errorf("%w: %w", ErrBusy, err) + return err } defer e.semaphore.Release(1) diff --git a/execution/execmodule/set_head_internal_test.go b/execution/execmodule/set_head_internal_test.go index 37aae9af7b0..54abf37217b 100644 --- a/execution/execmodule/set_head_internal_test.go +++ b/execution/execmodule/set_head_internal_test.go @@ -19,6 +19,7 @@ package execmodule import ( "context" "testing" + "time" "golang.org/x/sync/semaphore" @@ -37,3 +38,18 @@ func TestSetHeadReportsBusyOnlyWhenTheModuleIsOccupied(t *testing.T) { require.ErrorIs(t, err, context.Canceled) require.NotErrorIs(t, err, ErrBusy) } + +func TestSetHeadReportsBusyWhenItsOwnWaitRunsOut(t *testing.T) { + module := &ExecModule{semaphore: semaphore.NewWeighted(1)} + require.NoError(t, module.semaphore.Acquire(t.Context(), 1)) + + // The wait is bounded by a deadline of its own, and reaching it is what says the module was + // occupied. A caller cancelled just after that must not turn it into something else. + ctx, cancel := context.WithCancel(t.Context()) + acquireCtx, acquireCancel := context.WithTimeoutCause(ctx, time.Nanosecond, ErrBusy) + defer acquireCancel() + require.Error(t, module.semaphore.Acquire(acquireCtx, 1)) + cancel() + + require.ErrorIs(t, context.Cause(acquireCtx), ErrBusy) +} From 79e067325c9f43b28daa8ad03a8d3f59a2bbf7b5 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 10:59:48 +0200 Subject: [PATCH 5/7] execution, cl/phase1: address review The assemble retry lost the caller's cancellation when it landed on the final attempt: there was no wait left to notice it, so how the caller was classified depended on which retry it died on. The last attempt is now checked directly, keeping both causes. SetHead classified on the exported sentinel, which a caller can supply as its own cancellation cause and have contention reported for its own timeout. The marker is private now, and the wait's length is a field so the busy path can be reached through SetHead itself rather than reconstructed beside it. The busy sentinel keeps an alias where it used to be defined, so callers that referred to it still compile. Also: the Busy field described the builder still working rather than the module being occupied; the retry test could not tell a cancellable wait from a plain sleep; and the comment on the payload priority check credited the shortcut rather than the check that does the work. --- .../execution_client_direct.go | 20 ++++++--- .../execution_client_direct_test.go | 20 +++++++++ execution/builder/block_builder.go | 8 ++-- .../execmodule/chainreader/chain_reader.go | 6 +++ execution/execmodule/exec_module.go | 3 ++ execution/execmodule/interface.go | 3 +- execution/execmodule/set_head.go | 24 +++++++--- .../execmodule/set_head_internal_test.go | 44 ++++++++++++------- 8 files changed, 94 insertions(+), 34 deletions(-) diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index 670f3192030..922199a9398 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -158,16 +158,21 @@ func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, if attempts <= 0 { return 0, errors.New("assemble block requires at least one attempt") } + // A caller that ran out of time keeps the contention that used it up: that is why the slot was + // lost, and the bare context error does not say so. + ranOut := func(ctxErr, last error) error { + if last == nil { + return ctxErr + } + return fmt.Errorf("%w (last attempt: %w)", ctxErr, last) + } var ( id uint64 err error ) for attempt := range attempts { if ctxErr := ctx.Err(); ctxErr != nil { - if err != nil { - return 0, fmt.Errorf("%w (last attempt: %w)", ctxErr, err) - } - return 0, ctxErr + return 0, ranOut(ctxErr, err) } if id, err = assemble(ctx); err == nil { return id, nil @@ -179,10 +184,13 @@ func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, break } if sleepErr := common.Sleep(ctx, delay); sleepErr != nil { - // Keep the contention that caused the wait: it is the reason the caller ran out of time. - return 0, fmt.Errorf("%w (last attempt: %w)", sleepErr, err) + return 0, ranOut(sleepErr, err) } } + // The last attempt can itself have used up the caller, and there is no wait left to notice it. + if ctxErr := ctx.Err(); ctxErr != nil { + return 0, ranOut(ctxErr, err) + } return 0, err } diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go index 68162e84924..a3584d20dc8 100644 --- a/cl/phase1/execution_client/execution_client_direct_test.go +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -68,12 +68,16 @@ func TestRetryAssembleBlockGivesUpAfterAttempts(t *testing.T) { func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) calls := 0 + started := time.Now() _, err := retryAssembleBlock(ctx, 30, time.Second, func(context.Context) (uint64, error) { calls++ cancel() return 0, execmodule.ErrBusy }) + // Returning well inside the backoff is the property: a plain sleep would serve the same error + // a second later, and the test could not tell the two apart. + require.Less(t, time.Since(started), 500*time.Millisecond) require.ErrorIs(t, err, context.Canceled) require.ErrorIs(t, err, execmodule.ErrBusy, "the contention that caused the wait must survive in the error") require.Equal(t, 1, calls) @@ -98,3 +102,19 @@ func TestRetryAssembleBlockRejectsNoAttempts(t *testing.T) { }) require.EqualError(t, err, "assemble block requires at least one attempt") } + +func TestRetryAssembleBlockKeepsCancellationOnTheFinalAttempt(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + calls := 0 + _, err := retryAssembleBlock(ctx, 1, time.Second, func(context.Context) (uint64, error) { + calls++ + cancel() + return 0, execmodule.ErrBusy + }) + + // With no attempts left there is no wait to notice the cancellation, so the last attempt has to + // be checked directly. Otherwise how the caller is classified depends on which retry it died on. + require.Equal(t, 1, calls) + require.ErrorIs(t, err, context.Canceled) + require.ErrorIs(t, err, execmodule.ErrBusy) +} diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index a51facc0963..1ab6537222e 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -91,10 +91,10 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, error) { b.interrupt.Store(true) - // A payload that has landed wins over an expired caller, however close together the two - // arrive: selecting on both at once would pick between them at random and throw the block - // away. The second check matters as much as the first, because the payload can land while - // the select below is choosing. + // A payload that has landed wins over an expired caller, even when both are ready at the same + // time: selecting on both at once would pick between them at random and throw the block away. + // The check after cancellation is the one that prevents that, because the payload can land + // while the select is choosing; the one before is only a shortcut. if !b.finished() { select { case <-b.done: diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index 99d6e928f05..48a65e4db86 100644 --- a/execution/execmodule/chainreader/chain_reader.go +++ b/execution/execmodule/chainreader/chain_reader.go @@ -278,6 +278,12 @@ func (c ChainReaderWriterEth1) HasBlock(ctx context.Context, hash common.Hash) ( return c.executionModule.HasBlock(ctx, &hash, nil) } +// ErrExecutionBusy is where this signal used to be defined. The identity now lives next to the +// field it reports, and this stays so callers that referred to it keep compiling. +// +// Deprecated: use execmodule.ErrBusy. +var ErrExecutionBusy = execmodule.ErrBusy + 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 6d771fa4001..e9e8bd4d1f3 100644 --- a/execution/execmodule/exec_module.go +++ b/execution/execmodule/exec_module.go @@ -194,6 +194,9 @@ type ExecModule struct { logger log.Logger // Block building + // acquireTimeout bounds how long SetHead waits for the module; zero means the default. + acquireTimeout time.Duration + nextPayloadId uint64 lastParameters *builder.Parameters builderFunc builder.BlockBuilderFunc diff --git a/execution/execmodule/interface.go b/execution/execmodule/interface.go index 2e5c5265f7f..a501acb6e65 100644 --- a/execution/execmodule/interface.go +++ b/execution/execmodule/interface.go @@ -98,7 +98,8 @@ var ErrBusy = errors.New("execution module is busy") // 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 // Block holds the assembled block with receipts and requests. // Nil when Busy is true or when no builder was found for the payload ID. diff --git a/execution/execmodule/set_head.go b/execution/execmodule/set_head.go index 57dc33f1b20..9c9242257f3 100644 --- a/execution/execmodule/set_head.go +++ b/execution/execmodule/set_head.go @@ -49,19 +49,29 @@ func getLatestBlockNumber(tx kv.Tx) (uint64, error) { return blockNum, nil } +const defaultAcquireTimeout = 5 * time.Second + +// errAcquireTimedOut marks this wait running out. It stays private so that a caller cancelling with +// a cause of its own cannot be mistaken for the module being occupied. +var errAcquireTimedOut = errors.New("timed out waiting for the execution module") + // SetHead rewinds the local chain to the specified block number by unwinding // all staged sync stages. This is the core implementation used by debug_setHead. func (e *ExecModule) SetHead(ctx context.Context, targetBlock uint64) error { - // The cause records which deadline ran out at the moment it did. Reading the caller's context - // afterwards would misread a caller that went away just after the local timeout fired, and - // only the local timeout says the module was occupied. - acquireCtx, acquireCancel := context.WithTimeoutCause(ctx, 5*time.Second, ErrBusy) + // The cause records which deadline ran out at the moment it did, so the classification does not + // depend on what the caller's context looks like afterwards. The marker is private: a caller + // cancelling with ErrBusy as its own cause must not have contention reported for its timeout. + acquireTimeout := e.acquireTimeout + if acquireTimeout == 0 { + acquireTimeout = defaultAcquireTimeout + } + acquireCtx, acquireCancel := context.WithTimeoutCause(ctx, acquireTimeout, errAcquireTimedOut) defer acquireCancel() if err := e.semaphore.Acquire(acquireCtx, 1); err != nil { - if cause := context.Cause(acquireCtx); errors.Is(cause, ErrBusy) { - return fmt.Errorf("%w: %w", ErrBusy, err) + if errors.Is(context.Cause(acquireCtx), errAcquireTimedOut) { + return fmt.Errorf("set head: %w: %v", ErrBusy, err) } - return err + return fmt.Errorf("set head: %w", err) } defer e.semaphore.Release(1) diff --git a/execution/execmodule/set_head_internal_test.go b/execution/execmodule/set_head_internal_test.go index 54abf37217b..023bfef0db3 100644 --- a/execution/execmodule/set_head_internal_test.go +++ b/execution/execmodule/set_head_internal_test.go @@ -26,30 +26,42 @@ import ( "github.com/stretchr/testify/require" ) -func TestSetHeadReportsBusyOnlyWhenTheModuleIsOccupied(t *testing.T) { - module := &ExecModule{semaphore: semaphore.NewWeighted(1)} +// occupiedModule returns a module whose semaphore is already taken, so SetHead cannot get past the +// wait, with a wait short enough to run in a test. +func occupiedModule(t *testing.T) *ExecModule { + t.Helper() + module := &ExecModule{semaphore: semaphore.NewWeighted(1), acquireTimeout: time.Millisecond} require.NoError(t, module.semaphore.Acquire(t.Context(), 1)) + return module +} + +func TestSetHeadReportsBusyWhenItsOwnWaitRunsOut(t *testing.T) { + err := occupiedModule(t).SetHead(t.Context(), 1) - // The caller went away. Nothing is known about the module, so calling it busy would invite a - // retry that has nothing to wait for. + // Reaching the wait's own deadline is what says the module was occupied. + require.ErrorIs(t, err, ErrBusy) +} + +func TestSetHeadDoesNotReportBusyWhenTheCallerGivesUp(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() - err := module.SetHead(ctx, 1) + + err := occupiedModule(t).SetHead(ctx, 1) + + // Nothing is known about the module, so calling it busy would invite a retry with nothing to + // wait for. require.ErrorIs(t, err, context.Canceled) require.NotErrorIs(t, err, ErrBusy) } -func TestSetHeadReportsBusyWhenItsOwnWaitRunsOut(t *testing.T) { - module := &ExecModule{semaphore: semaphore.NewWeighted(1)} - require.NoError(t, module.semaphore.Acquire(t.Context(), 1)) +func TestSetHeadDoesNotReportBusyForACallerCancelledWithThatCause(t *testing.T) { + // A caller may cancel with any cause it likes, including this package's own sentinel. That says + // nothing about the module, so the marker classified on has to be one no caller can supply. + ctx, cancel := context.WithCancelCause(t.Context()) + cancel(ErrBusy) - // The wait is bounded by a deadline of its own, and reaching it is what says the module was - // occupied. A caller cancelled just after that must not turn it into something else. - ctx, cancel := context.WithCancel(t.Context()) - acquireCtx, acquireCancel := context.WithTimeoutCause(ctx, time.Nanosecond, ErrBusy) - defer acquireCancel() - require.Error(t, module.semaphore.Acquire(acquireCtx, 1)) - cancel() + err := occupiedModule(t).SetHead(ctx, 1) - require.ErrorIs(t, context.Cause(acquireCtx), ErrBusy) + require.ErrorIs(t, err, context.Canceled) + require.NotErrorIs(t, err, ErrBusy) } From 10e4c7cbacbf8d7bcd4f0f241d2de3091e188238 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 13:32:50 +0200 Subject: [PATCH 6/7] execution: drop a redundant inner error from the busy report The wait can only have failed on its own deadline in that branch, which is what ErrBusy already says, so the inner error added the deadline back as unmatchable text. --- execution/execmodule/set_head.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/execution/execmodule/set_head.go b/execution/execmodule/set_head.go index 9c9242257f3..9871357dd62 100644 --- a/execution/execmodule/set_head.go +++ b/execution/execmodule/set_head.go @@ -69,7 +69,7 @@ func (e *ExecModule) SetHead(ctx context.Context, targetBlock uint64) error { defer acquireCancel() if err := e.semaphore.Acquire(acquireCtx, 1); err != nil { if errors.Is(context.Cause(acquireCtx), errAcquireTimedOut) { - return fmt.Errorf("set head: %w: %v", ErrBusy, err) + return fmt.Errorf("set head: %w", ErrBusy) } return fmt.Errorf("set head: %w", err) } From 517b7bff83a452864477f24706ef241f83ce7a41 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 16:54:45 +0200 Subject: [PATCH 7/7] execution: let Stop say whether it gave up, rather than leaving it to be guessed A build can fail with a context error of its own - a transaction provider timing out, a shutdown reaching the read view - so the error's shape does not say whether the caller gave up or the build failed. Reading it that way skipped the record for a real failure. Stop tags its own give-up branch instead, and that is what the caller checks. The GetAssembledBlock method doc described Busy as the builder not having finished, which the field doc had already been corrected away from. The file no longer contradicts itself. Three tests that pinned outcomes without pinning the mechanism now do both: Stop giving up rather than failing, a caller whose own deadline is shorter than the module's wait, and a backoff that aborts rather than sleeping through cancellation. Each fails against the substitution it is there to rule out. ErrBusy documents the convention it is used under, since it stays matchable inside a cancelled caller's error but must not on its own trigger a retry for a caller that has gone. --- .../execution_client_direct.go | 4 +++- .../execution_client_direct_test.go | 9 ++++---- execution/builder/block_builder.go | 8 ++++++- execution/builder/block_builder_test.go | 22 +++++++++++++++++++ execution/execmodule/block_building.go | 6 +++-- execution/execmodule/interface.go | 7 +++++- .../execmodule/set_head_internal_test.go | 15 +++++++++++++ 7 files changed, 62 insertions(+), 9 deletions(-) diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index 922199a9398..37f6741838d 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -159,7 +159,9 @@ func retryAssembleBlock(ctx context.Context, attempts int, delay time.Duration, return 0, errors.New("assemble block requires at least one attempt") } // A caller that ran out of time keeps the contention that used it up: that is why the slot was - // lost, and the bare context error does not say so. + // lost, and the bare context error does not say so. One line rather than errors.Join, because + // this ends up in a log record. Kept on one line rather than joined, since + // this ends up in a log record. ranOut := func(ctxErr, last error) error { if last == nil { return ctxErr diff --git a/cl/phase1/execution_client/execution_client_direct_test.go b/cl/phase1/execution_client/execution_client_direct_test.go index a3584d20dc8..be50e1dd07f 100644 --- a/cl/phase1/execution_client/execution_client_direct_test.go +++ b/cl/phase1/execution_client/execution_client_direct_test.go @@ -69,15 +69,16 @@ func TestRetryAssembleBlockStopsWhenContextIsCanceled(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) calls := 0 started := time.Now() - _, err := retryAssembleBlock(ctx, 30, time.Second, func(context.Context) (uint64, error) { + _, err := retryAssembleBlock(ctx, 30, time.Minute, func(context.Context) (uint64, error) { calls++ cancel() return 0, execmodule.ErrBusy }) - // Returning well inside the backoff is the property: a plain sleep would serve the same error - // a second later, and the test could not tell the two apart. - require.Less(t, time.Since(started), 500*time.Millisecond) + // Returning well inside the backoff is the property. The bound is far from both ends: a sleep + // that ignored the context would take a minute, and a loaded runner has no trouble with ten + // seconds. + require.Less(t, time.Since(started), 10*time.Second) require.ErrorIs(t, err, context.Canceled) require.ErrorIs(t, err, execmodule.ErrBusy, "the contention that caused the wait must survive in the error") require.Equal(t, 1, calls) diff --git a/execution/builder/block_builder.go b/execution/builder/block_builder.go index 1ab6537222e..289d3d035ba 100644 --- a/execution/builder/block_builder.go +++ b/execution/builder/block_builder.go @@ -18,6 +18,7 @@ package builder import ( "context" + "errors" "fmt" "sync" "sync/atomic" @@ -28,6 +29,11 @@ import ( "github.com/erigontech/erigon/execution/types" ) +// ErrStopAbandoned reports that Stop gave up waiting, not that the build failed. The build carries +// on and its payload may still be collected, so a caller cannot read this as a failure - and cannot +// tell the two apart from the context error alone, because a build can fail with one of its own. +var ErrStopAbandoned = errors.New("stopped waiting for the block builder") + type BlockBuilderFunc func(param *Parameters, interrupt *atomic.Bool) (*types.BlockWithReceipts, error) // BlockBuilder wraps a goroutine that builds Proof-of-Stake payloads (PoS "mining") @@ -100,7 +106,7 @@ func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, erro case <-b.done: case <-ctx.Done(): if !b.finished() { - return nil, ctx.Err() + return nil, fmt.Errorf("%w: %w", ErrStopAbandoned, ctx.Err()) } } } diff --git a/execution/builder/block_builder_test.go b/execution/builder/block_builder_test.go index c22520dd488..5362bb0e9be 100644 --- a/execution/builder/block_builder_test.go +++ b/execution/builder/block_builder_test.go @@ -18,6 +18,7 @@ package builder import ( "context" + "errors" "sync" "sync/atomic" "testing" @@ -101,3 +102,24 @@ func TestBlockBuilderStopKeepsAPayloadThatLandsWhileTheCallerGivesUp(t *testing. require.NotNil(t, result) } } + +func TestBlockBuilderStopReportsGivingUpRatherThanFailing(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) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + result, err := b.Stop(ctx) + + // The build has not finished, so there is nothing to hand back - but it has not failed either, + // and a caller that cannot tell the difference reports a healthy node as broken. + require.Nil(t, result) + require.ErrorIs(t, err, ErrStopAbandoned) + require.ErrorIs(t, err, context.Canceled) +} diff --git a/execution/execmodule/block_building.go b/execution/execmodule/block_building.go index e745bf4bd86..e933b4f7dee 100644 --- a/execution/execmodule/block_building.go +++ b/execution/execmodule/block_building.go @@ -130,8 +130,10 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := bldr.Stop(ctx) if err != nil { - // The caller gave up waiting; nothing about the build itself went wrong. - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // Only Stop can say whether it gave up waiting or the build failed. A build failure can + // carry a context error of its own - a transaction provider timing out, a shutdown reaching + // the read view - so the error shape does not distinguish them. + if errors.Is(err, builder.ErrStopAbandoned) { return AssembledBlockResult{}, err } e.logger.Error("Failed to build PoS block", "err", err) diff --git a/execution/execmodule/interface.go b/execution/execmodule/interface.go index a501acb6e65..c8a99388240 100644 --- a/execution/execmodule/interface.go +++ b/execution/execmodule/interface.go @@ -94,6 +94,10 @@ type AssembleBlockResult struct { // ErrBusy reports that the execution module was already occupied. It settles on its own, unlike a // rejection, which returns the same answer however many times it is asked. +// +// It stays matchable inside an error that also reports a cancelled caller, so the contention which +// used up that caller's time is not lost. Anything keyed on it to decide whether to retry has to +// check the context first: there is nothing to retry for a caller that has gone. var ErrBusy = errors.New("execution module is busy") // AssembledBlockResult is the native return type for GetAssembledBlock. @@ -157,7 +161,8 @@ type ExecutionModule interface { AssembleBlock(ctx context.Context, params *builder.Parameters) (AssembleBlockResult, error) // GetAssembledBlock retrieves the block that was assembled under the - // given payloadID. The result is Busy when the builder has not finished. + // given payloadID. The result is Busy when the module was already occupied; otherwise the + // call waits for the builder to finish. GetAssembledBlock(ctx context.Context, payloadID uint64) (AssembledBlockResult, error) // --- Header / body queries -------------------------------------------- diff --git a/execution/execmodule/set_head_internal_test.go b/execution/execmodule/set_head_internal_test.go index 023bfef0db3..9cd37a19ea7 100644 --- a/execution/execmodule/set_head_internal_test.go +++ b/execution/execmodule/set_head_internal_test.go @@ -65,3 +65,18 @@ func TestSetHeadDoesNotReportBusyForACallerCancelledWithThatCause(t *testing.T) require.ErrorIs(t, err, context.Canceled) require.NotErrorIs(t, err, ErrBusy) } + +func TestSetHeadDoesNotReportBusyForACallerWithAShorterDeadline(t *testing.T) { + module := &ExecModule{semaphore: semaphore.NewWeighted(1), acquireTimeout: time.Hour} + require.NoError(t, module.semaphore.Acquire(t.Context(), 1)) + + // The caller's own deadline expires long before this wait would, so what ran out is the + // caller's patience, not the module's. Classifying on the error's shape rather than on which + // deadline was reached reports the module as occupied on no evidence at all. + ctx, cancel := context.WithTimeout(t.Context(), time.Millisecond) + defer cancel() + + err := module.SetHead(ctx, 1) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.NotErrorIs(t, err, ErrBusy) +}