txnprovider/txpool: release the pool lock when a caller stops waiting for a block - #23333
Merged
Conversation
… for a block best takes the pool lock and then waits for the block it was asked to build on top of. A caller that goes away in the meantime returned from inside that loop without releasing it, so the lock stayed held and every later pool operation blocked, including the block updates that would have let the wait finish. Nothing recovers from that on its own. Reachable today only at shutdown, because the only caller that cancels is the one shutting the node down. It stops being shutdown-only as soon as anything cancels a live build, which is what discarding an evicted payload builder does.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a mutex leak in transaction selection when a waiting caller cancels.
Changes:
- Unlocks the pool before returning on context cancellation.
- Adds regression coverage verifying the lock remains usable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
txnprovider/txpool/pool.go |
Releases the pool lock on cancellation. |
txnprovider/txpool/pool_best_lock_test.go |
Tests cancellation and lock release. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
yperbasis
approved these changes
Aug 17, 2026
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
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.
TxPool.besttakes the pool lock and then waits for the block it was asked to build on top of: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:
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.p.lockand thepoolDBread-transaction limiter, is untouched.TestBestReleasesTheLockWhenTheCallerGivesUpWaitingForABlockcovers it, and fails on the current code with "best returned holding the pool lock".Note:
make lintreportsdb/seg/decompress.go:199: field residencyOnce is unused, which is pre-existing onmain— I confirmed it with this change stashed.golangci-lintis clean fortxnprovider/txpool/....