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

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
 }
 

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,39 @@ 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)
 }
 
 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)
+}

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,17 +97,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, 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()
 	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,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 <http://www.gnu.org/licenses/>.
+
+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)
+}

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
 	}