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.