execution, cl/phase1: stop discarding a payload that is already built

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

cl/phase1/execution_client/execution_client_direct_test.go

@@ -24,15 +24,15 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	"github.com/erigontech/erigon/execution/execmodule/chainreader"
+	"github.com/erigontech/erigon/execution/execmodule"
 )
 
 func TestRetryAssembleBlockReturnsFirstSuccess(t *testing.T) {
 	calls := 0
 	id, err := retryAssembleBlock(t.Context(), 3, time.Millisecond, func(context.Context) (uint64, error) {
 		calls++
 		if calls < 3 {
-			return 0, 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,33 +58,34 @@ 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)
 }
 
 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)

execution/builder/block_builder.go

@@ -91,17 +91,35 @@ 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, 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 <-b.done:
+		case <-ctx.Done():
+			if !b.finished() {
+				return nil, ctx.Err()
+			}
+		}
 	}
 
 	b.mu.Lock()
 	defer b.mu.Unlock()
 	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()

execution/builder/block_builder_test.go

@@ -0,0 +1,103 @@
+// 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 <http://www.gnu.org/licenses/>.
+
+package builder
+
+import (
+	"context"
+	"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)
+	}
+}

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 {
+		// 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
+		}
 		e.logger.Error("Failed to build PoS block", "err", err)
 		return AssembledBlockResult{}, err
 	}

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

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.

execution/execmodule/set_head.go

@@ -18,6 +18,7 @@ package execmodule
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -51,10 +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 {
-		return fmt.Errorf("execution module is busy: %w", err)
+		if cause := context.Cause(acquireCtx); errors.Is(cause, ErrBusy) {
+			return fmt.Errorf("%w: %w", ErrBusy, err)
+		}
+		return err
 	}
 	defer e.semaphore.Release(1)