simulators/ethereum/engine: wait for sent tx to be pending before building payload#1561
Open
yperbasis wants to merge 1 commit into
Open
simulators/ethereum/engine: wait for sent tx to be pending before building payload#1561yperbasis wants to merge 1 commit into
yperbasis wants to merge 1 commit into
Conversation
…lding payload
The "Invalid Missing Ancestor Syncing ReOrg, Transaction*, EmptyTxs=False"
tests intermittently fail during setup with:
FAIL: Unable to customize payload: no transactions available for modification
Root cause (ethereum#1351): in ProduceSingleBlock the harness sends a tx from the
OnPayloadProducerSelected hook, then after a fixed 1s delay calls getPayload.
SendTransaction returns as soon as the tx is admitted to the pool, but
promotion into the pending view the builder reads is asynchronous, so the
builder occasionally snapshots an empty pool and returns a payload with no
transactions. GenerateInvalidPayload then has nothing to modify.
Instead of widening the fixed delay, make the send deterministic: after a
successful SendTransaction, wait until the producer reports the tx as pending
in its pool (via TransactionByHash) before returning. This guarantees the
subsequent build sees the tx.
Also implement GethNode.TransactionByHash and PendingTransactionCount, which
were panicking "NOT IMPLEMENTED" stubs — the in-process geth node is the
producer in these tests, so the wait must work against it.
Fixes ethereum#1351
3 tasks
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.
Summary
Makes the
Invalid Missing Ancestor Syncing ReOrg, Transaction*engine testsdeterministic instead of relying on a timing window. These tests intermittently
fail during setup with:
Fixes #1351. This is an alternative to #1462 (which widens the fixed delay): it
closes the race at its source rather than making it less likely, so it does not
add latency to every produced block.
Root cause
In
ProduceSingleBlockthe harness sends a transaction from theOnPayloadProducerSelectedhook, then after a fixedPayloadProductionClientDelay(1s) calls
getPayload.SendTransactionreturns as soon as the tx is admittedto the pool, but the producer promotes it into the pending view the payload
builder reads asynchronously. With only a 1s budget the builder occasionally
snapshots an empty pending set and returns a payload with zero transactions;
GenerateInvalidPayloadthen has nothing to modify and the test aborts duringsetup, before the client under test is ever exercised. The flake has been
observed against geth, erigon and reth (see #1351), confirming it is harness-side.
Fix
After a successful
SendTransaction, block until the producer reports thetransaction as pending in its pool (
TransactionByHash→isPending) beforereturning. The subsequent build is then guaranteed to see it. The wait is
best-effort and bounded (10s), so it degrades to the previous behavior rather
than introducing a new hard failure if a client never reports the tx as pending.
This required implementing
GethNode.TransactionByHashandPendingTransactionCount, which werepanic("NOT IMPLEMENTED")stubs — thein-process geth node is the payload producer in these tests, so the wait has to
work against it. (This is the "more targeted" alternative noted in #1462; the
panicking stubs are why it needed a bit more than a one-line poll.)
clmock.gois untouched, so this composes with #1462 if both land, and removesthe need for the blanket delay increase.
Verification
Reproduced and fixed locally with hive against erigon (pre-built
erigontech/erigon:main-latest), repeatedly running the affected family(
engine-api/Invalid Missing Ancestor Syncing ReOrg, Transaction*— 10 tests:{Signature, Nonce, Gas, GasPrice, Value}×CanonicalReOrg={false, true}) at--sim.parallelism=10:master(bba71e0)Unable to customize payload: no transactions available for modificationThat is 660 green test executions on the patched branch. At the observed baseline
rate, the probability of seeing zero failures in 60 iterations by chance is ~1.6%.
Test plan
go build ./...,go vet ./...,gofmtclean insimulators/ethereum/engine.master(1/15),gone on this branch (0/60).
Fixes #1351