From 51b648e4c0b84a12488a495f67f09a2057e66154 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 15:22:17 +0200 Subject: [PATCH 1/2] txnprovider/txpool: wake a caller that goes away while waiting for a block best parks in a sync.Cond, which has no notion of a context: the wait ends when a new block broadcasts the condition, or when the node shuts down. A caller that gave up in the meantime therefore stays parked, holding whatever it brought with it, until one of those happens. While the chain is stalled that is never, which is when it matters most. Cancelling now broadcasts, so the caller wakes and returns. The broadcast takes the pool lock, which is what keeps it from landing between the cancellation check and the wait, where a wakeup would be lost. Reachable once anything cancels a live build: discarding an evicted payload builder does, which is why this is a prerequisite for that change rather than part of it. --- txnprovider/txpool/pool.go | 17 +++++++ txnprovider/txpool/pool_best_lock_test.go | 60 +++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/txnprovider/txpool/pool.go b/txnprovider/txpool/pool.go index b7fba981023..06d89ea32b5 100644 --- a/txnprovider/txpool/pool.go +++ b/txnprovider/txpool/pool.go @@ -725,6 +725,23 @@ func (p *TxPool) best(ctx context.Context, n int, txns *TxnsRlp, onTopOf uint64, availableGas mdgas.FullMdGas, yielded mapset.Set[[32]byte], availableRlpSpace int) (bool, int, error) { + // sync.Cond has no notion of a context, so a caller that goes away while parked below would + // sleep until the next block broadcast the condition, or forever while the chain is stalled. + // Broadcasting on cancellation wakes it; every waiter rechecks its own condition anyway. The + // broadcast takes the lock so it cannot land between the check below and the wait, which is the + // window where a wakeup would be lost. + waitDone := make(chan struct{}) + defer close(waitDone) + go func() { + select { + case <-ctx.Done(): + p.lock.Lock() + p.lastSeenCond.Broadcast() + p.lock.Unlock() + case <-waitDone: + } + }() + p.lock.Lock() for last := p.lastSeenBlock.Load(); last < onTopOf; last = p.lastSeenBlock.Load() { select { diff --git a/txnprovider/txpool/pool_best_lock_test.go b/txnprovider/txpool/pool_best_lock_test.go index b71ea6bce1e..d3b0458b76e 100644 --- a/txnprovider/txpool/pool_best_lock_test.go +++ b/txnprovider/txpool/pool_best_lock_test.go @@ -17,13 +17,17 @@ package txpool import ( + "bytes" "context" + "strings" "sync" "testing" + "time" mapset "github.com/deckarep/golang-set/v2" "github.com/stretchr/testify/require" + "github.com/erigontech/erigon/common/log/v3" mdgas "github.com/erigontech/erigon/execution/protocol/mdgas" ) @@ -44,3 +48,59 @@ func TestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlock(t *testing.T) { require.True(t, p.lock.TryLock(), "best returned holding the pool lock") p.lock.Unlock() } + +// waitingBuffer records what the pool logged, so a test can tell when a caller has reached the +// "Waiting for block" trace and is therefore about to park in the condition. +type waitingBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (w *waitingBuffer) Write(p []byte) (int, error) { + w.mu.Lock() + defer w.mu.Unlock() + return w.buf.Write(p) +} + +func (w *waitingBuffer) contains(s string) bool { + w.mu.Lock() + defer w.mu.Unlock() + return strings.Contains(w.buf.String(), s) +} + +func TestBestReturnsWhenItsCallerGoesAwayWhileWaitingForABlock(t *testing.T) { + logs := &waitingBuffer{} + logger := log.New() + logger.SetHandler(log.StreamHandler(logs, log.LogfmtFormat())) + + lock := &sync.Mutex{} + p := &TxPool{ + lock: lock, + lastSeenCond: sync.NewCond(lock), + logger: logger, + pending: NewPendingSubPool(PendingSubPool, 1), + baseFee: NewSubPool(BaseFeeSubPool, 1), + queued: NewSubPool(QueuedSubPool, 1), + } + ctx, cancel := context.WithCancel(t.Context()) + + returned := make(chan error, 1) + go func() { + _, _, err := p.best(ctx, 1, &TxnsRlp{}, 1, mdgas.FullMdGas{}, mapset.NewSet[[32]byte](), 0) + returned <- err + }() + + // Cancel only once it is parked, so this exercises the wait rather than the check before it. + require.Eventually(t, func() bool { return logs.contains("Waiting for block") }, 5*time.Second, time.Millisecond) + cancel() + + // Nothing else wakes it: no block arrives and the pool is not shutting down. A wait that cannot + // observe its caller leaves the builder that made this request holding a read view until one of + // those happens, which on a stalled chain is never. + select { + case err := <-returned: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(5 * time.Second): + t.Fatal("best never returned; only a new block would have woken it") + } +} From 570d0d7a9f120e1993948ab732e38a0cd1eed89a Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Mon, 17 Aug 2026 16:30:28 +0200 Subject: [PATCH 2/2] txnprovider/txpool: join the cancellation watcher before returning Closing its stop channel only made one of the watcher's two cases ready. With the context already ended it could pick the other, and take the pool lock after the call it belongs to had returned. Waiting for it to finish keeps it scoped to the call, as described. The wait is registered before the lock is taken, so it runs after the lock is released. --- txnprovider/txpool/pool.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/txnprovider/txpool/pool.go b/txnprovider/txpool/pool.go index 06d89ea32b5..956c0f8b820 100644 --- a/txnprovider/txpool/pool.go +++ b/txnprovider/txpool/pool.go @@ -730,17 +730,24 @@ func (p *TxPool) best(ctx context.Context, n int, txns *TxnsRlp, onTopOf uint64, // Broadcasting on cancellation wakes it; every waiter rechecks its own condition anyway. The // broadcast takes the lock so it cannot land between the check below and the wait, which is the // window where a wakeup would be lost. - waitDone := make(chan struct{}) - defer close(waitDone) + stopWatching, watcherDone := make(chan struct{}), make(chan struct{}) go func() { + defer close(watcherDone) select { case <-ctx.Done(): p.lock.Lock() p.lastSeenCond.Broadcast() p.lock.Unlock() - case <-waitDone: + case <-stopWatching: } }() + // Joined rather than merely told to stop: with the context already ended the watcher can pick + // either case, and one that picked cancellation would otherwise take the lock after this call + // had returned. Registered before the lock is taken, so it runs after the lock is released. + defer func() { + close(stopWatching) + <-watcherDone + }() p.lock.Lock() for last := p.lastSeenBlock.Load(); last < onTopOf; last = p.lastSeenBlock.Load() {