erigontech / erigontech/erigon

txnprovider/txpool: make the best() block wait natively cancellable

Open
#23,355 1 comment 0 reactions 1 assignee Claimed by @yperbasis View on GitHub
tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

Follow-up to #23343, from @yperbasis's review there.

## Background

#23343 made a caller parked in `best` observable to cancellation: a watcher goroutine broadcasts `lastSeenCond` when the context ends, and `best` joins that watcher before returning. The join is needed for correctness — without it the watcher can take `p.lock` after the call has already returned.

## Problem

That unconditional join changes how a cancelled caller returns. Before #23343 it returned without touching `p.lock` again; now its return needs the watcher to win `p.lock` first. Three consequences:

1. A caller cancelled at its deadline cannot return until the current lock holder finishes — for example a long `OnNewBlock` or `processRemoteTxns` pass.
2. Cancellation can no longer break a stuck cycle it used to break. `best` can reach `isBlockNumBasedForkActivated` → `p._chainDB.BeginRo(context.Background())` while holding `p.lock` (Bor, before the fork flag latches). If that uncancellable `BeginRo` waits on the read-txn limiter while the slots are held by callers that keep a chain-DB read txn open across `best`, and whose cancelled `best` calls sit in this join, nothing moves.
3. A panic raised while the lock is held unwinds into the blocking join. If the context is cancelled at that moment, the watcher waits forever for the lock and the runtime never gets to print the panic.

## Why this needs an owner now

None of it is reachable in tree today: no give-up path cancels the context `best` receives. `BlockBuilder.Stop` only sets the interrupt atomic, which `best` never reads, and `Builder.Build` passes the node-lifetime context into `ProvideTxns`. #23272 wires a cancellable context through for the payload builder, so all three become live when it merges. This should land with or before #23272.

## Proposed fix

Replace `lastSeenCond` with a `chan struct{}` that `OnNewBlock` and shutdown close and swap under `p.lock`. The wait becomes `select { case <-signal: case <-ctx.Done(): }` — natively cancellable — and the watcher, the join and all three consequences above disappear. `lastSeenCond` has a single `Wait` site, so the swap is contained.

## Worth folding in

- Use `context.AfterFunc` rather than the hand-rolled watcher, and arm it only after the `lastSeenBlock` check fails, so callers that never park do no extra work. Both are superseded if the redesign above lands, and worth doing if it does not.
- `pool_best_lock_test.go`: add `defer cancel()` so a failing `require.Eventually` cannot leave a parked goroutine behind, and consider `testing/synctest` to pin the parked state instead of polling for the "Waiting for block" trace text.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.