fix(payload): prevent parallel prewarming from dropping transactions - #7351
Open
teyrebaz33 wants to merge 1 commit into
Open
fix(payload): prevent parallel prewarming from dropping transactions#7351teyrebaz33 wants to merge 1 commit into
teyrebaz33 wants to merge 1 commit into
Conversation
BestTransactionsPrewarming's coordinator eagerly calls advance() up to 2x the thread count during startup. In parallel mode, a worker's Some(tx) result is only sent once prewarm_transaction finishes (asynchronously), while the coordinator's exhaustion sentinel (None) is sent synchronously the moment the source iterator returns None. For a source smaller than the eager-fill batch, the coordinator could reach the exhaustion branch and send None before an earlier, still in-flight worker sent its Some(tx). Since messages are delivered in send order, the consumer could observe the premature None first and stop, silently dropping a valid, still-pending transaction. The transaction remained known to the pool (visible via eth_getTransactionByHash) but was never included by that or any subsequent payload, since every payload build hits the same race. Fixes this by tracking how many parallel workers are currently in flight. The exhaustion sentinel is only sent once the source is exhausted AND no workers are in flight. Workers now report completion via a distinct WorkerDone command (rather than the existing Advance, which also represents a consumer pulling the next item), so the coordinator can tell the two apart and decide whether it's safe to finalize. The sequential (non-parallel) path is unchanged. Adds a regression test that deterministically reproduces the original timing: saturates every worker thread with a blocking sleep before submitting a source smaller than the eager-fill batch, forcing the real transactions' prewarm workers to queue behind the busy threads -- mirroring a slow worker finishing after the coordinator has already observed exhaustion. Verified this test fails reliably against the pre-fix logic and passes reliably (15+ consecutive runs) against the fix. Originally diagnosed by @Osraka in tempoxyz#7284, including root cause and a narrow fix direction; this implements and verifies that fix along with a regression test.
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.
Closes #7284
Problem
With parallel payload building enabled (
--builder.parallel, currently hidden/experimental),BestTransactionsPrewarming's coordinator eagerly callsadvance()up to2 * num_threadstimes during startup. In parallel mode, a worker'sSome(tx)result is only sent ontransactions_txonceprewarm_transactionfinishes (asynchronously), while the coordinator's exhaustion sentinel (None) is sent synchronously the moment the source iterator returnsNone.For a source smaller than the eager-fill batch (e.g. a single pending transaction), the coordinator can reach the exhaustion branch and send
Nonebefore an earlier, still in-flight worker has sent itsSome(tx). Since messages are delivered in send order, the consumer can observe the prematureNonefirst and stop -- silently dropping a valid, still-pending transaction. The transaction remains known to the pool (eth_getTransactionByHashstill returns it) but is never included by that or any subsequent payload, since every payload build hits the same race.Fix
Tracks how many parallel prewarm workers are currently in flight (
in_flight: usizeon the coordinator context). The exhaustion sentinel is only sent once the source is exhausted and no workers are in flight. Workers now report completion via a newWorkerDonecommand, distinct from the existingAdvance(which also represents a consumer pulling the next item), so the coordinator can tell the two apart and decide whether it's safe to finalize. The sequential (non-parallel) path is unchanged.Tests
Added a regression test that deterministically reproduces the original timing: saturates every worker thread with a blocking sleep before submitting a source smaller than the eager-fill batch, forcing the two real transactions' prewarm workers to queue behind the busy threads -- mirroring a slow worker finishing after the coordinator has already observed exhaustion. Verified this test fails reliably against the pre-fix logic and passes reliably (15+ consecutive runs) against the fix. Full
cargo test -p tempo-payload-builder prewarming,cargo clippy -p tempo-payload-builder --lib, andcargo fmt --checkall clean.Note: while iterating I also noticed the pre-existing
prewarming_does_not_use_shared_worker_state_slottest has some intrinsic flakiness (~15-20%) fully independent of this change (reproducible in isolation on current main). CI'sretries = 2on the default profile absorbs this in practice; flagging it here for visibility rather than trying to fix an unrelated test as part of this PR.Scope note
This didn't reach a full end-to-end reproduction (real node, real RPC transaction,
advance_block()) matching the exact repro in #7284, since parallel payload building is currently a hidden/in-development flag (--builder.parallel, with an explicit "should not be used" warning) not exposed by the existing e2e test harness's node-construction path. The included test instead reproduces the exact race deterministically at the coordination-logic level, which is where the root cause lives.Thanks to @Osraka for the original diagnosis, reproduction, and fix direction in #7284 -- this implements and verifies that approach.