execution: key builders by payload timestamp and give them a lifecycle

[lystopad]

Split out of #23105, which grew too large to review in one piece. The payload-preparation work there depends on this, but every problem below is reachable on main today, so it stands alone.

Deduplication remembered only the last request

lastParameters held a single set of parameters, so any interleaved request for a different timestamp destroyed the deduplication for the first one. A repeated request then started a second builder for a payload already being built, and the first kept running unreachable.

Builders are now kept by the payload timestamp they are for, next to an immutable copy of the parameters they were created with. The copy matters: the caller owns the withdrawals slice and the pointer fields it passed, and could otherwise mutate them and change what a later comparison sees.

A failed builder was reused forever

BlockBuilder latches its error, so once a build failed every later request that deduplicated onto it got that error back — spending the slot waiting for a payload that could never arrive. A failed builder is now treated as absent and dropped when its error surfaces.

Being stopped is deliberately not failure: a stopped builder still holds the payload it was stopped for, which is exactly what a repeated request is asking for.

Eviction did not bound a builder's lifetime

This is the problem in #23101. Setting the interrupt flag is not enough, because Builder.Build ran its database read view and its transaction provider on the node-lifetime context, and the flag is not read until whichever of those is blocking returns on its own — up to most of a slot. An evicted builder therefore left the map while its goroutine and read view stayed alive.

A builder now answers two different requests:

• Interrupt — return the block you have so far. This is how a payload is collected and how the maximum build time is enforced; both want the payload.
• Discard — the payload is not wanted, release what you hold. This cancels the context the build runs under, so a read view or a transaction provider blocked on it returns at once.

Eviction discards. BlockBuilderFunc takes a context, NewBlockBuilder derives a per-builder cancellable one, and Build uses it for BeginTemporalRo, NewSharedDomains, createBlock and execBlock — which is how it reaches ProvideTxns. Builder.ctx is removed rather than left beside it.

Two deliberate limits:

• eviction stays asynchronous, so this is not a hard bound on live builders at any instant. It changes the window from "until the blocking call gives up on its own" to "as fast as a cancelled call returns". Making eviction wait would block AssembleBlock while holding the module semaphore.
• nothing added is timed or fork-specific. The build context carries no deadline of its own, and the only duration in play remains buildDuration, derived from the chain's slot length — Gnosis and Chiado need no new constants, and a fork that changes slot timing needs no change here.

#23101 is assigned to @yperbasis, so this does not close it — please retarget or close it as you see fit.

Cancellation

Both entry points check for a cancelled caller before acting, so an expired request reports why it stopped instead of looking like contention, which callers retry. GetAssembledBlock reads cancellation from the returned error rather than the ambient context, which could otherwise change between Stop returning and the check.

Tests

Deduplication and lifetime are covered directly: builders kept apart by timestamp, a superseded builder still packing and still retrievable by an id already handed out, a collected payload handed back to a repeated request rather than starting a second builder, a failed builder not reused and dropped once its error surfaces, a caller cancelled while Stop is waiting leaving the builder collectable, and an evicted builder blocked in its provider actually completing rather than merely having a flag set.

Part of a series splitting #23105.

[Copilot]

Pull request overview

This PR refactors ExecModule’s payload-builder deduplication to key builders by payload timestamp (instead of a single “last parameters” record) and introduces clearer builder lifecycle handling (failed builders treated as absent, evicted builders are now cancelled). This supports upcoming payload-preparation work by making repeated FCU / getPayload sequences reliably reuse the intended builder and by preventing orphaned builder goroutines.

Changes:

• Track builders by payload timestamp + immutable parameter snapshot (builderEntry, buildersByTimestamp) to fix deduplication across interleaved timestamps and prevent caller-owned parameter mutation from affecting comparisons.
• Cancel builders on eviction and drop failed builders when their error is surfaced, avoiding permanent reuse of latched failures.
• Add unit tests for dedup/lifecycle rules and for BlockBuilder.Failed() semantics across running/stopped/errored/completed cases.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
execution/execmodule/exec_module.go	Switch builder storage to builderEntry and add timestamp index.
execution/execmodule/block_building.go	Implement timestamp-keyed dedup, parameter cloning, eviction cancellation, and cancellation prechecks.
execution/execmodule/block_building_internal_test.go	Add tests for timestamp dedup + lifecycle behaviors and parameter ownership.
execution/builder/block_builder.go	Add Cancel() and Failed() helpers; have Stop() delegate to Cancel().
execution/builder/block_builder_test.go	Add unit tests for Failed() behavior across builder states.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lystopad]

Good catch, and it was inconsistent: the code already declined to drop the builder on a context error but still reported it as a build failure. A caller that gave up now returns its own error without logging or dropping — the builder is still running and may still be collected. Only a real build failure is reported and dropped. Added TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUp to pin it.

[lystopad]

Right, the docstring overclaimed — it only inspects the latched error, not the result. Reworded to say it reports whether the builder has finished and ended in an error.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

execution/execmodule/block_building.go:212

• The decision to keep or drop a builder on error is keyed off ctx.Err(), but ctx.Err() can become non-nil even when Stop returned a builder failure (e.g., if the context is canceled/deadlines at roughly the same time the builder finishes with an error, or if both ctx.Done() and b.done are ready and Stop selects b.done). In that case the failed builder won’t be dropped, and its latched error can keep being served to retries — reintroducing the “failed builder reused forever” behavior.

Key this branch off the returned err being a context cancellation/deadline error, not the context’s current state.

		if ctx.Err() != nil {
			return AssembledBlockResult{}, err
		}

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

execution/execmodule/block_building_internal_test.go:280

• This test cancels the context before calling GetAssembledBlock, so it returns from the initial ctx.Err() check and never exercises the new ctx.Err() branch after BlockBuilder.Stop has begun waiting. Add a case that calls GetAssembledBlock with a live context, blocks builder completion, cancels while Stop is waiting, and verifies that the entry is preserved and the context error is returned.

	ctx, cancel := context.WithCancel(t.Context())
	cancel()
	_, err = module.GetAssembledBlock(ctx, result.PayloadID)
	require.ErrorIs(t, err, context.Canceled)

execution/execmodule/block_building.go:209

• This branch is reached only after entry.builder.Stop(ctx) has called Cancel, so the builder has already been interrupted; it is not guaranteed to keep running. Reword the comment to explain that the entry is retained because a caller timeout does not determine the builder's eventual result.

		// A caller that gave up says nothing about the builder, which keeps running and may still
		// be collected. Only a builder that actually failed is reported and dropped, so its latched
		// error stops being handed to every retry.

execution/execmodule/block_building_internal_test.go:113

• These deletions remove two live builders from the cleanup's map without signaling them, so their goroutines remain running after the test until their watchdogs fire. Stop or cancel both retained builders before deleting their entries.

This issue also appears on line 277 of the same file.

	delete(module.builders, firstID)
	delete(module.builders, secondID)

[domiwei]

The timestamp indexing, parameter snapshot, and failed-builder reuse changes look sound in the focused tests. I found one lifecycle gap and two test gaps that are worth addressing before treating #23101 as fixed.

I ran go test ./execution/builder ./execution/execmodule -count=1 and focused -race tests successfully. Those runs do not exercise cancellation of transitive blocking work or the mid-Stop cancellation window described inline.

Minor repository-standard note: the new Failed doc comment has three sentences; agents.md requires source comments to be one sentence, rarely two. It can be reduced to “Failed reports whether the completed builder has a latched error.”

[domiwei]

[P1] Propagate eviction cancellation into blocking builder work

Cancel only sets the atomic interrupt flag. The real Builder.Build holds a temporal read transaction and SharedDomains, then calls ProvideTxns with the node-lifetime b.ctx; for example, the Shutter provider may wait for up to a slot before returning. The interrupt is not checked until after that call, so an evicted entry can disappear from the map while its goroutine/read view remains active. Repeated distinct requests can therefore accumulate more active builders than MaxBuilders during the blocking window—the resource-lifetime condition from #23101 is still not bounded.

Please give each builder cancellable work ownership that reaches ProvideTxns/other blocking calls, and test eviction with a provider that blocks until its context is canceled. The test should observe builder completion/resource release, not only interrupt.Load().

[lystopad]

Fixed properly rather than nominally. Cancel only set the atomic flag, and Build ran the read view and the transaction provider on the node-lifetime context, so the flag was not read until whatever was blocking returned on its own.

A builder now answers two different requests. Interrupting asks for the block it has so far - that is how a payload is collected, and how the maximum build time is enforced, and both want the payload. Discarding says the payload is not wanted at all and cancels the context the build runs under, so a read view or a provider blocked on it returns at once. Eviction discards.

BlockBuilderFunc takes a context, NewBlockBuilder derives a per-builder cancellable one, and Build uses it for BeginTemporalRo, NewSharedDomains, createBlock and execBlock, which is how it reaches ProvideTxns. Builder.ctx is gone rather than left beside it, so there is no second context to drift.

TestEvictionReleasesABuilderBlockedOnItsProvider does what you asked: a provider that blocks until its context is cancelled, and the assertion is that the goroutine finishes, not that a flag flipped. With Discard reduced back to flag-only it fails with "evicted builder was never released" after the full timeout.

One thing worth being explicit about: this does not make eviction synchronous, so it does not put a hard bound on live builders at any instant. It changes the window from "until the provider gives up on its own" to "as fast as a cancelled call returns". Making eviction wait would block AssembleBlock while holding the module semaphore, which seemed worse than the problem.

[domiwei]

[P1] Exercise cancellation after Stop has begun

This context is canceled before GetAssembledBlock is called, so the function returns from its initial ctx.Err() check. It never acquires the semaphore, calls Stop, or reaches the new ctx.Err() classification at lines 205–211. A focused coverage run confirms the remainder of GetAssembledBlock executes zero times in this test.

Please synchronize after Stop has set the builder interrupt but before done closes, cancel the caller then, and verify the subsequent same-ID GetAssembledBlock and same-parameters AssembleBlock behavior.

[lystopad]

You were right, and coverage proved it: cancelling before the call returned at the entry check and exercised none of the classification.

Replaced with TestGetAssembledBlockKeepsBuilderWhenTheCallerGivesUpMidStop, which cancels while Stop is already waiting - the window a caller-side timeout actually lands in. It asserts the entry and its timestamp index survive, and that a later collection on the same id still returns the payload. Verified it fails when cancellation no longer spares the builder.

[domiwei]

[P2] Stop these builders before removing the cleanup handles

first and second still loop on interrupt.Load() here. The registered cleanup only visits entries still present in module.builders, so deleting these IDs makes both goroutines unreachable by cleanup and they outlive the test until their minute-long watchdogs fire. Retain independent builder handles for cleanup, or stop/cancel them before deleting the entries.

[lystopad]

Fixed. Both are discarded before their entries are removed, so cleanup is not the only thing that could have reached them.

[yperbasis]

Requesting changes for three lifecycle issues:

• P1: evictOldBuilders only sets the atomic interrupt flag. Builder.Build continues using its node-lifetime context for database and transaction-provider work, so a provider blocked in ProvideTxns can retain the read view after its entry is evicted. Repeated distinct requests can therefore leave more than MaxBuilders active. Please propagate per-builder cancellation into blocking work and make the regression test observe completion, not only the flag.
• P2: GetAssembledBlock classifies the returned error using ctx.Err after Stop returns. The context can become cancelled after Stop returned the builder error but before this check, leaving a failed entry and its timestamp index in place. Use the Stop outcome or the builder failed state. The new cancellation test cancels before the call, so it exits at the initial context check and does not cover this branch.
• P2: TestAssembleBlockKeepsBuildersApartByTimestamp deletes firstID and secondID before cleanup can stop them. Both goroutines then remain alive until their watchdogs fire. Stop them before deletion or retain separate handles for cleanup.

Focused package and race tests pass; these lifecycle cases are not covered by those passing runs.

[lystopad]

@domiwei @yperbasis — all three lifecycle issues addressed in 11ace21996, and you were right that the eviction one was not really fixed.

Eviction

Cancel set an atomic flag, but Build ran its read view and its transaction provider on the node-lifetime context, and the flag is not read until whatever is blocking returns on its own. So an evicted builder left the map with its goroutine and read view still alive.

A builder now answers two different requests:

• Interrupt — give me the block you have so far. This is how a payload is collected and how the maximum build time is enforced; both want the payload.
• Discard — the payload is not wanted, release what you hold. This cancels the context the build runs under, so a read view or a provider blocked on it returns at once.

Eviction discards. BlockBuilderFunc takes a context, NewBlockBuilder derives a per-builder cancellable one, and Build uses it for BeginTemporalRo, NewSharedDomains, createBlock and execBlock — which is how it reaches ProvideTxns. That plumbing already existed and was being handed the wrong context. Builder.ctx is removed rather than left beside it.

Two things I want to be explicit about, because neither is free:

• This does not make eviction synchronous, so it is not a hard bound on live builders at any instant. It changes the window from "until the provider gives up on its own" to "as fast as a cancelled call returns". Making eviction wait would block AssembleBlock while holding the module semaphore, which looked worse than the problem.
• Nothing added is timed or fork-specific. The build context carries no deadline of its own, and the only duration in play is still buildDuration, derived from the chain's slot length — so Gnosis and Chiado's shorter slots need no new constants, and a future fork changing slot timing needs no change here.

The two test gaps

The cancellation test did exit at the entry check, as your coverage run showed. Replaced with one that cancels while Stop is already waiting, asserting the entry and its timestamp index survive and that a later collection still returns the payload.

GetAssembledBlock also now reads a cancelled caller from the returned error rather than the ambient context, so the two cannot drift between Stop returning and the check.

The two builders in TestAssembleBlockKeepsBuildersApartByTimestamp are discarded before their entries are removed.

Not in this PR

Failed's docstring is one sentence now.

I left out the suggestion to key deduplication on (timestamp, parent hash). It is a good idea and would remove most of the supersede handling, but it is a different concern from lifetime, and this series exists because mixing concerns made the original PR unreviewable. Happy to do it next.

Every new test here was checked against the previous behaviour rather than assumed — including the eviction one, which fails with "evicted builder was never released" if Discard goes back to setting only the flag.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

execution/builder/block_builder.go:73

• When Discard cancels buildCtx, the real builder returns context.Canceled from its database/provider work, and this branch logs the expected eviction as “Failed to build a block” at warning level. Distinct FCU requests that trigger eviction can therefore generate misleading warning logs. Suppress the warning when the build context itself was canceled; genuine build errors occur while that context is still active.

		result, err = build(buildCtx, param, &builder.interrupt)

[lystopad]

Real, and it is a data race rather than only a semantic problem — confirmed under -race. staticTxnProvider clears s.txns and flips s.done from the build goroutine while DeepEqual reads the same fields, and the comparison's answer also changes as the build progresses.

A request carrying a CustomTxnProvider is now never treated as the same request as another. That is not just avoiding the read: a provider that hands its transactions over once and then returns nil is not something two builds can share, so deduplicating onto a builder created with a different provider instance was wrong regardless.

TestAssembleBlockNeverReusesABuilderWithACustomProvider keeps a provider being consumed while a second identical request arrives. With the provider back inside the comparison it fails under -race with WARNING: DATA RACE; without it, clean.

Worth noting the race predates this PR — main compares e.lastParameters the same way — but this is the right place to fix it since the comparison is being rewritten here.

[lystopad]

Also took the suppressed comment about the eviction warning: discarding a builder makes its database and provider work return a cancellation, which NewBlockBuilder then reported as Failed to build a block at warning level. Evictions are expected, so that path logs at debug now and only a genuine failure warns. That one is a direct consequence of Discard actually cancelling, so it belongs here.

Head is 42f8b47665. go test -race is clean across execution/builder and execution/execmodule.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[lystopad]

Good catch, and I confirmed the example: txnprovider/shutter/pool.go wraps its parent-block wait timeout as "issue while waiting for parent block %d: %w" over context.DeadlineExceeded. Inspecting the error therefore kept a genuinely failed builder and served that latched error to every later retry of the slot, which is precisely what this PR exists to stop.

Rather than add a status to Stop, the check now asks the builder: Failed() is already there and means finished with an error. A caller that gave up leaves a builder that has not finished, so it is kept and stays collectable; a build that failed on its own is dropped whatever its error wraps.

TestGetAssembledBlockDropsABuildThatFailedWithAContextError pins it with a provider-style wrapped DeadlineExceeded, and fails against the previous errors.Is check.

[yperbasis]

Reviewed at 60aa6ea. Findings by severity; the two blocking ones are what stops approval.

Blocking

• txnprovider/txpool/pool.go:732 — evicting a builder can deadlock the whole txpool. best returns ctx.Err() from the waiting loop while still holding p.lock (locked at line 728; the only unlock is after the loop at line 749). Before this PR the build ctx was node-lifetime, so this path could fire only at shutdown. Now eviction → Discard cancels a live build's ctx inside ProvideTxns, so an evicted builder waiting in that loop returns with the lock held — OnNewBlock, ProvideTxns, AddLocalTxns and shutdown then block forever. Please unlock before that return, in this PR or in a small prerequisite PR.

• execution/builder/block_builder.go:92 — the watchdog never discards. On max build time it sets interrupt and then blocks forever in Stop(context.Background()). A build stuck in a wait that ignores interrupt (e.g. the txpool cond-wait above) keeps its goroutines, MDBX read txn and SharedDomains pinned until count-based eviction, which needs 127 newer builders — days on a quiet node, blocking MDBX page reclaim the whole time. Suggest Stop with a grace-period ctx, then discard() on timeout. Only safe together with the txpool fix above.

Medium

• execution/execmodule/block_building.go:123 — eviction can remove the current holder of a timestamp, and a test asserts the opposite. evictOldBuilders goes strictly by lowest id and does not protect the builder indexed in buildersByTimestamp, but the test message says "the current builder for a timestamp must survive eviction" (block_building_internal_test.go:128) — and that test itself evicts the current holder for timestamp 101. Either skip the current index holder in the eviction loop or fix the message. Skipping also protects a CL's proposal-target id, which the new dedup keeps current for longer than main did.

• execution/execmodule/chainreader/chain_reader.go:315 — Caplin cannot see that a payload was dropped. After dropBuilder removes a failed builder, the first poll returns the latched error, but every later poll gets (nil, ..., nil) with a nil error, which pollAssembledPayload treats as "still building" — it spins silently for the rest of the production window. The engine-api path returns UnknownPayload for the same state; please return an error (or a sentinel) here too.

• The new execmodule tests race a real ~3s watchdog. Timestamp 100 is far in the past, so buildDuration takes the slot/4 floor (3s with chain.Config{}), and assertions like block_building_internal_test.go:111 require interrupt to still be false. A ≥3s stall on a loaded CI runner flips the flag and fails the test. Use near-future timestamps or pass the duration explicitly like block_builder_test.go does.

Minor / non-blocking

• BlockBuilder.Stop: when ctx.Done() and b.done are both ready, the select picks randomly, so a caller can get ctx.Err() although a finished payload is latched. A non-blocking preference for <-b.done makes it deterministic.
• AssembleBlock does not check e.bacgroundCtx, so during shutdown it still registers builders whose buildCtx is already cancelled at creation and hands their ids out. Minor now that the Failed() path drops them on the first getPayload. (Also: bacgroundCtx is misspelled.)
• cloneBuilderParameters mirrors builder.Parameters from another package; the next reference-typed field added to Parameters will compile and silently shallow-copy. Suggest (*Parameters).Copy() next to the struct, sharing the withdrawals copy with Block.Copy.
• evictOldBuilders re-implements dropBuilder inline; fold Discard into dropBuilder (a no-op on a finished builder) and call it from the eviction loop.
• AssembleBlock still writes previousID into the caller's params.PayloadId (line 160) only so the comparison ignores that field; zeroing PayloadId on copies inside sameBuildRequest would remove the caller-visible mutation.
• Registry state: builderEntry.timestamp duplicates entry.params.Timestamp; buildersByTimestamp is initialized lazily instead of in the constructor; the nil-entry guards exist only because tests pad the map with nils.
• Tests: a dozen near-identical ExecModule literals and repeated spin-stub builders — a newTestModule(t, builderFunc) helper would cut most of it.
• Several test comments repeat the full rationale already stated at the canonical site (dedup / drop / supersede); short pointers would age better.

[lystopad]

On the first blocking finding — the txpool deadlock — I took your suggestion of a separate prerequisite PR: #23333.

It is the missing unlock only. I confirmed the shape you described: best holds p.lock across the wait loop and the cancellation branch returns from inside it, so the lock is never released, and the block update that would let the wait finish needs that same lock. Nothing recovers.

Two things I left alone there, both worth being explicit about:

• cancelling the context does not wake a goroutine parked in lastSeenCond.Wait(). So with txnprovider/txpool: release the pool lock when a caller stops waiting for a block #23333 alone, a discarded builder still returns at the next block rather than at once — no deadlock, but not prompt either. Making that wait cancellable is a bigger change and a separate question from the lock being leaked, so I would rather not fold it in.
• consequently the watchdog change here still needs care even once txnprovider/txpool: release the pool lock when a caller stops waiting for a block #23333 lands: Stop would block until the next block wake-up rather than forever, which is better but not bounded by anything this PR controls.

I will come back to this PR's own findings — the watchdog, the eviction removing a current timestamp holder, Caplin not seeing a dropped payload, and the tests racing the ~3s watchdog floor — once #23333 is settled, since the watchdog fix depends on it.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[yperbasis]

Reviewed at 60aa6ea5 on top of current main. #23333 fixes the missing txpool unlock, but two blocking lifecycle problems remain.

• P1: eviction still cannot promptly release the real txpool provider. BlockBuilder.Discard cancels the build context, but TxPool.best can be parked in lastSeenCond.Wait(). That wait does not observe context cancellation; it wakes only when another block or shutdown broadcasts the condition. An evicted builder can therefore keep its goroutine, read transaction, and SharedDomains alive until the next broadcast, potentially indefinitely while the chain is stalled. Please make this wait context-aware and cover eviction with the real txpool path, requiring completion without an external broadcast.

• P1: the maximum-build watchdog can still wait forever. On timeout it calls Stop(context.Background()), which sets the interrupt flag and then waits without a deadline. A build blocked outside an interrupt check, including the txpool wait above, defeats maxBuildTime and keeps its resources pinned. Please give Stop a bounded grace period and call Discard if it expires, with a regression test using a build blocked solely on its context.

I verified the virtual merge with current main: focused builder and execmodule tests, their race tests, the txpool lock-release test, and git diff --check all pass. Diagnostic tests reproduce both lifecycle gaps.

[lystopad]

Both blocking findings and the three medium ones are in a5c33c430f, now that #23333 has merged.

Watchdog. Reaching the maximum build time asked for the block and then waited forever. It now gets a short grace period to hand it over and is discarded if it will not. The grace is sized against how often the build loop looks at the interrupt flag rather than against the slot: a build that is cooperating answers within one of those polls, and one that is not never will. TestBlockBuilderReleasesABuildThatIgnoresTheDeadline uses a provider that only honours its context, and hangs for the full timeout against the previous code; TestBlockBuilderStillHandsOverAPayloadWhenItsBudgetRunsOut checks the ordinary case still yields its block rather than being thrown away.

Eviction taking a live proposal target. Fixed the behaviour rather than the message, as you suggested. Anything a timestamp still resolves to is skipped whatever its age; if everything remaining is current the count is left above the bound, which seemed the safer way to be wrong. TestEvictionSparesTheBuilderATimestampStillPointsAt puts the current holder at the lowest id so eviction would take it first.

Worth flagging: my first attempt at that test passed with the guard removed, because the lowest id in the existing scenario happened to be a superseded builder. The test above is the one that actually discriminates.

Caplin not seeing a dropped payload. AssembledBlockResult carries Unknown, and the chain reader turns it into ErrUnknownPayload. I deliberately did not change the engine path: assembled.Block == nil still maps to UnknownPayloadErr there, so that contract is untouched. Making the poll loop stop early on the sentinel belongs with the poll loop, which is in cl/beacon/handler and is what #23274 is editing; I would rather not have two PRs in that function at once.

Tests racing the watchdog. They share one fixture now, with timestamps far enough ahead that buildDuration gives a budget measured in slots instead of taking its floor.

Also taken: the parameters copy moved next to the struct it copies, with a field-count test that fails when a field is added; the payload id is no longer written into the caller's parameters just to make a comparison match; eviction and dropping share one path; and the timestamp index is built with the module.

Not taken, deliberately: the bacgroundCtx spelling. It is 25 sites across files this PR does not otherwise touch, and it would bury the change here — better as its own one-line-per-site commit. The Stop select preference you mention is fixed in #23289 rather than duplicated here.

make lintci clean, go test -race green across execution/..., and each new test checked against the behaviour it replaces.

[lystopad]

@yperbasis — you reviewed 60aa6ea5, which is one push behind. Taking your two P1s in turn:

The watchdog waiting forever is fixed in a5c33c430f, pushed before your review landed. It gets a bounded grace period to hand the block over and is discarded if it will not, and TestBlockBuilderReleasesABuildThatIgnoresTheDeadline uses a build blocked solely on its context, which is the regression test you asked for. The grace is sized against how often the build loop reads the interrupt flag rather than against the slot, so it does not add a chain-dependent constant.

Eviction not promptly releasing the real txpool provider is #23343, opened just now as a second prerequisite alongside #23333.

You are right that I had left this as a limitation rather than a fix — I noted on #23333 and again here that a discarded builder returns at the next block rather than at once, and treated that as acceptable. It is not: "returns at the next block" is fine on a healthy chain and useless on a stalled one, and a stalled chain is when builders pile up. Thank you for making it blocking.

The fix broadcasts the condition on cancellation, taking the pool lock so the broadcast cannot land between the check and the wait, where it would be lost. Its test 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, and nothing else wakes it — no block, no shutdown. Against current main it fails with best never returned.

I kept it out of this PR for the same reason as #23333: it is a txnprovider/txpool change and this one is an execution/ change that is already large.

On your ask for eviction covered through the real txpool path end to end: #23343 covers the wait itself, which is the part that was broken. Wiring a real TxPool into an ExecModule eviction test needs a pool database and sender machinery that this package's tests do not currently stand up, so I have not built that here. Say if you want it and I will do it as its own change rather than half of one.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[yperbasis]

Re-reviewed a5c33c430f after reading the complete PR conversation. The watchdog now has a bounded stop path, but three blockers remain.

1. [P1] Preserve the builder-cache bound when protecting a live proposal. isCurrentFor is true for the newest builder at every distinct timestamp, and successful entries never leave buildersByTimestamp. With normal one-builder-per-slot traffic, once the cache reaches MaxBuilders, evictOldBuilders skips every entry and each later build grows both maps without bound. A focused regression left 131 entries after eviction with MaxBuilders == 128. Please distinguish genuinely live proposal targets from completed or expired timestamp holders and add a distinct-timestamp sequence test that asserts the bound.

2. [P2] Propagate Unknown through the Caplin adapter. chainreader.GetAssembledBlock declares ErrUnknownPayload but never checks result.Unknown; it reaches the result.Block == nil branch and returns a nil payload with a nil error. A focused adapter test reproduced this. Please return ErrUnknownPayload before the nil-block branch and cover that boundary in a test.

3. [P1] Land #23343 before this PR. The current merge result still has a real txpool caller parked in lastSeenCond.Wait() after its context is cancelled, so Discard cannot promptly release that provider on a stalled chain. The combined #23272 + #23343 tree passes the targeted normal and race tests, but this PR alone does not contain the wakeup fix.

The prior watchdog finding is resolved on this head.

[lystopad]

All three addressed in 584f859e41, and the first one was a bad call on my part rather than an oversight.

[P1] The cache bound. You are right, and I had reasoned about this and got it wrong. My comment said the count is "left above the bound if everything is current, which is the safer of the two ways to be wrong" — as though that were a rare corner. It is the normal steady state: one builder per slot, each with its own timestamp, and a successful entry stays indexed, so every entry qualified and eviction skipped all of them. Unbounded growth in both maps, not an edge case. I reproduced your figure: 136 entries against a bound of 128.

Being indexed is no longer sufficient. Only a slot that has not passed can still be waited on, and there are at most a couple of those at any moment, so the bound holds. The timestamp is compared in seconds rather than as a time.Time, so an implausible one wraps into "not live" instead of overflowing into the past. TestEvictionKeepsTheBuilderCacheBounded walks a distinct-timestamp sequence past MaxBuilders and asserts both maps; it fails at 136 against the previous head. TestEvictionSparesTheBuilderATimestampStillPointsAt still covers the protection itself.

[P2] Unknown never propagated. Straightforwardly my bug: the sentinel was declared and the result never consulted, so it did nothing at all. Worse, the evidence was in front of me — I grepped for it, saw only the declaration and no use, and moved on. Fixed, with chain_reader_test.go covering all three boundaries: unknown, busy, and a builder that simply has nothing yet.

[P1] Ordering. Agreed, and #23343 is open for exactly that. This PR needs it to land first; on its own the wakeup fix is absent and Discard cannot promptly release a provider parked in lastSeenCond.Wait().

One consequence worth noting for review: the liveness window made me correct the test timestamps too. They had been set an hour ahead to keep the max-build-time watchdog from firing mid-test, which now also put them outside the window a proposal can be waiting in. They sit ten seconds ahead instead, which satisfies both.

make lintci clean apart from the darwin-only db/seg artefact, go test -race green across execution/....