diff --git a/cl/phase1/execution_client/execution_client_direct.go b/cl/phase1/execution_client/execution_client_direct.go index b020f5ec7ab..37f6741838d 100644 --- a/cl/phase1/execution_client/execution_client_direct.go +++ b/cl/phase1/execution_client/execution_client_direct.go @@ -158,36 +158,41 @@ 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. 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 + } + return fmt.Errorf("%w (last attempt: %w)", ctxErr, last) + } var ( id uint64 err error ) for attempt := range attempts { if ctxErr := ctx.Err(); ctxErr != nil { - return 0, ctxErr + return 0, ranOut(ctxErr, err) } 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 { + 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 6f2af144871..be50e1dd07f 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,29 @@ 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) { + started := time.Now() + _, err := retryAssembleBlock(ctx, 30, time.Minute, func(context.Context) (uint64, error) { calls++ cancel() - return 0, chainreader.ErrExecutionBusy + return 0, execmodule.ErrBusy }) + // 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) } @@ -84,9 +88,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) @@ -99,3 +103,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 e77019a08c2..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") @@ -91,10 +97,18 @@ func NewBlockBuilder(build BlockBuilderFunc, param *Parameters, maxBuildTime tim func (b *BlockBuilder) Stop(ctx context.Context) (*types.BlockWithReceipts, error) { b.interrupt.Store(true) - select { - case <-ctx.Done(): - return nil, ctx.Err() - case <-b.done: + // 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: + case <-ctx.Done(): + if !b.finished() { + return nil, fmt.Errorf("%w: %w", ErrStopAbandoned, ctx.Err()) + } + } } b.mu.Lock() @@ -102,6 +116,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 new file mode 100644 index 00000000000..5362bb0e9be --- /dev/null +++ b/execution/builder/block_builder_test.go @@ -0,0 +1,125 @@ +// 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" + "errors" + "sync" + "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) + } +} + +// 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) + } +} + +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 ef1d94cddaf..e933b4f7dee 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,12 @@ func (e *ExecModule) GetAssembledBlock(ctx context.Context, payloadID uint64) (A } blockWithReceipts, err := bldr.Stop(ctx) if err != nil { + // 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) return AssembledBlockResult{}, err } diff --git a/execution/execmodule/chainreader/chain_reader.go b/execution/execmodule/chainreader/chain_reader.go index cd2082c09f5..48a65e4db86 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,9 +278,11 @@ 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") +// 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{ @@ -299,7 +300,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 +311,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/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 044d53b7472..c8a99388240 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,9 +92,18 @@ 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. +// +// 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. 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. @@ -151,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.go b/execution/execmodule/set_head.go index 9f88c56c9f7..9871357dd62 100644 --- a/execution/execmodule/set_head.go +++ b/execution/execmodule/set_head.go @@ -18,6 +18,7 @@ package execmodule import ( "context" + "errors" "fmt" "time" @@ -48,13 +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 { - acquireCtx, acquireCancel := context.WithTimeout(ctx, 5*time.Second) + // 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 { - return fmt.Errorf("execution module is busy: %w", err) + if errors.Is(context.Cause(acquireCtx), errAcquireTimedOut) { + return fmt.Errorf("set head: %w", ErrBusy) + } + 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 new file mode 100644 index 00000000000..9cd37a19ea7 --- /dev/null +++ b/execution/execmodule/set_head_internal_test.go @@ -0,0 +1,82 @@ +// 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" + "time" + + "golang.org/x/sync/semaphore" + + "github.com/stretchr/testify/require" +) + +// 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) + + // 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 := 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 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) + + err := occupiedModule(t).SetHead(ctx, 1) + + 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) +}