Skip to content

simulators/ethereum/engine: wait for sent tx to be pending before building payload#1561

Open
yperbasis wants to merge 1 commit into
ethereum:masterfrom
yperbasis:fix/engine-wait-tx-pending
Open

simulators/ethereum/engine: wait for sent tx to be pending before building payload#1561
yperbasis wants to merge 1 commit into
ethereum:masterfrom
yperbasis:fix/engine-wait-tx-pending

Conversation

@yperbasis

Copy link
Copy Markdown
Member

Summary

Makes the Invalid Missing Ancestor Syncing ReOrg, Transaction* engine tests
deterministic instead of relying on a timing window. These tests intermittently
fail during setup with:

FAIL: Unable to customize payload: no transactions available for modification

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 ProduceSingleBlock the harness sends a transaction from the
OnPayloadProducerSelected hook, then after a fixed PayloadProductionClientDelay
(1s) calls getPayload. SendTransaction returns as soon as the tx is admitted
to 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;
GenerateInvalidPayload then has nothing to modify and the test aborts during
setup, 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 the
transaction as pending in its pool (TransactionByHashisPending) before
returning. 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.TransactionByHash and
PendingTransactionCount, which were panic("NOT IMPLEMENTED") stubs — the
in-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.go is untouched, so this composes with #1462 if both land, and removes
the 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:

Build Iterations Failures
master (bba71e0) 15 1Unable to customize payload: no transactions available for modification
this branch 60 0

That 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 ./..., gofmt clean in simulators/ethereum/engine.
  • Repeated local runs against erigon: flake reproduced on master (1/15),
    gone on this branch (0/60).

Fixes #1351

…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky "Invalid Missing Ancestor Syncing ReOrg" tests fail when payload contains no transactions

1 participant