[r3.6] txnprovider/txpool: release the pool lock and wake a caller that gives up waiting - #23360
Merged
AskAlexSharov merged 4 commits intoAug 19, 2026
Merged
Conversation
… for a block (#23333) `TxPool.best` takes the pool lock and then waits for the block it was asked to build on top of: ```go p.lock.Lock() for last := p.lastSeenBlock.Load(); last < onTopOf; last = p.lastSeenBlock.Load() { select { case <-ctx.Done(): return false, 0, ctx.Err() // <- still holding p.lock default: } p.lastSeenCond.Wait() } ... p.lock.Unlock() ``` A caller that goes away while waiting returns from inside the loop without releasing the lock. The only unlock is past the loop, so the lock stays held for the life of the process and every later pool operation blocks behind it — `OnNewBlock`, `ProvideTxns`, `AddLocalTxns`, and shutdown. It does not recover on its own: the thing that would let the wait finish is a block update, and that needs the same lock. ### Why now Today this is reachable only at shutdown, because the only caller that cancels this context is the one shutting the node down, which is why it has not been noticed. It stops being shutdown-only as soon as anything cancels a live build. #23272 gives each payload builder a cancellable context so that discarding an evicted one actually releases its resources, and a builder waiting here is then cancelled during normal operation — a routine eviction would deadlock the pool. @yperbasis found it while reviewing that PR and suggested taking it separately, which is what this is. ### Scope Only the missing unlock. Two things I deliberately did not change: - cancelling the context does not wake a goroutine parked in `lastSeenCond.Wait()`, so an evicted builder still returns at the next block rather than immediately. Making the wait cancellable is a larger change and a separate question from the lock being leaked. - the ordering comment below the loop, about `p.lock` and the `poolDB` read-transaction limiter, is untouched. `TestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlock` covers it, and fails on the current code with "best returned holding the pool lock". Note: `make lint` reports `db/seg/decompress.go:199: field residencyOnce is unused`, which is pre-existing on `main` — I confirmed it with this change stashed. `golangci-lint` is clean for `txnprovider/txpool/...`. (cherry picked from commit a497fb7)
…block (#23343) Second prerequisite for #23272, after #23333. Raised by @yperbasis reviewing that PR. `TxPool.best` parks in `p.lastSeenCond.Wait()` until the block it was asked to build on top of arrives. `sync.Cond` has no notion of a context: the wait ends when a new block broadcasts the condition (`pool.go:361`) or when the node shuts down. Cancelling the caller does nothing. So a caller that has given up stays parked, holding its read transaction and `SharedDomains`, until a block arrives. **While the chain is stalled that is never** — which is exactly when builders accumulate and when releasing them matters. #23333 stopped a cancelled caller leaving the pool lock held. This is the other half: making the wait notice at all. ### The fix Cancellation broadcasts the condition, so the parked caller wakes, sees its context, and returns. Waiters re-check their own condition on waking regardless, so an extra broadcast is harmless. The broadcast takes the pool lock. That is load-bearing rather than incidental: without it the broadcast could land between the cancellation check and `Wait()`, and the wakeup would be lost — the caller would sleep on exactly as before. Holding the lock means the broadcast can only happen before the check, where the check sees it, or after `Wait()` has released the lock, where the broadcast reaches it. The watcher goroutine is scoped to the call and exits with it. ### Test `TestBestReturnsWhenItsCallerGoesAwayWhileWaitingForABlock` cancels only once the caller has reached the "Waiting for block" trace, so it exercises the wait rather than the check in front of it. Nothing else wakes it: no block arrives, nothing shuts down. Against the current code it fails with `best never returned; only a new block would have woken it`. ### Why separate It is a `txnprovider/txpool` change, and #23272 is an `execution/` change that is large already. Same reasoning as #23333, which @yperbasis suggested splitting out for the same reason. Note: `make lintci` reports `db/seg/decompress.go:199: field residencyOnce is unused`, which is darwin-only — that field's user is `residency_gate_linux.go`, so CI does not see it. `golangci-lint` is clean for `txnprovider/txpool/...`. (cherry picked from commit 85aa63e)
yperbasis
approved these changes
Aug 18, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 18, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 18, 2026
AskAlexSharov
enabled auto-merge
August 18, 2026 16:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #23333 and #23343 to release/3.6.
Both fix the same wait loop in
TxPool.best, and they ship together because #23343 does not apply without #23333: the test file it touches is created by #23333.The first is the one worth having on a release branch on its own merits.
bestreturnedctx.Err()from inside the wait loop while still holdingp.lock, so a caller that gave up leaked the pool lock and every later pool operation blocked behind it. Verified present on this branch before the pick.The second makes a caller parked in
sync.Cond.Waitobservable to cancellation at all: without it, a caller that goes away sleeps until the next block, or forever while the chain is stalled.r3.6 notes
Straight cherry-picks, no adaptations.
BlockBuilder.Stop()takes no context on this branch, so nothing here depends on #22835 or on #23289.Known follow-up, tracked in #23355 and not specific to this backport: the watcher join means a cancelled caller's return now waits for
p.lock. Not reachable on this branch either, for the same reason it is not reachable on main — no in-tree give-up path cancels the contextbestreceives.Verification
go test ./txnprovider/txpool/ -run TestBest -race -count=200green.test timed out, blocked on the mutex); dropping the watcher join failsTestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlock.make lintciclean, 0 issues.