Skip to content

test: harden the block-improvement wait against empty payloads - #12859

Draft
Marchhill wants to merge 1 commit into
masterfrom
marc/harden-wait-for-improved-block
Draft

test: harden the block-improvement wait against empty payloads#12859
Marchhill wants to merge 1 commit into
masterfrom
marc/harden-wait-for-improved-block

Conversation

@Marchhill

Copy link
Copy Markdown
Contributor

Changes

MergeTestBlockchain.WaitForImprovedBlock matched only on the parent hash. PayloadPreparationService publishes an initial empty block for a parent and then improves it, so the first BlockImproved event satisfied the wait regardless of whether the tx pool had yet made the submitted transactions selectable. TxPool head processing runs on a background channel (_headBlocksChannel / ProcessNewHeads), so its account/nonce view lags the forkchoiceUpdated that advanced the head — engine_getPayload* could therefore hand back the empty block.

That is what made EngineModuleTests.Witness_includes_bytecode_and_storage_proof_for_a_called_contract flaky on Run Nethermind.Merge.Plugin.Test [no-intrinsics]: when the transaction missed its block, the called contract had no code and witness.Codes lacked the runtime bytecode. DOTNET_EnableHWIntrinsic=0 slows keccak/ecrecover on the sender-resolution and pool-insert path, widening the window — which is why only the [no-intrinsics] variant surfaced it.

  • Add an additive minTransactions parameter to WaitForImprovedBlock. The default of 0 leaves all ~20 existing call sites behaviourally unchanged.
  • Thread the submitted transaction count through BuildAmsterdamPayload, and assert the retrieved payload actually carries them so a regression fails at the cause rather than in a downstream witness assertion.
  • Apply the same requirement to the two V6 call sites that submit transactions and then assert on block hash, receipts root, or block contents — they carried the identical latent race.
  • Add a regression test pinning the contract: an empty improvement must not satisfy minTransactions.

Improvements are monotonic — ImproveBlock seeds each new context with the previous best block — so observing an improvement with N transactions guarantees the retrieved payload keeps at least N.

Types of changes

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

The flake would not reproduce directly on a fast dev box, so the race was demonstrated with a throwaway probe rather than assumed:

  • Pre-fix, deterministic. With a transaction becoming pool-visible just after the first improvement, the parent-hash-only wait completed and engine_getPayloadV6 returned a 0-transaction payload — the exact CI symptom.
  • Post-fix, deterministic. With minTransactions: 1 the wait provably did not complete on the empty improvement (improvementWait.IsCompleted was false) and completed only once an improvement carried the transaction; the payload then had 1 transaction.
  • Positive control. Neutralising the new predicate makes the committed regression test fail with an empty improvement must not satisfy minTransactions / Expected: False / But was: True, confirming it is not vacuous.
  • Full assembly, DOTNET_EnableHWIntrinsic=0: 1683 passed, 0 failed, 2 skipped — matching the counts from the green CI re-run.
  • Stress, DOTNET_EnableHWIntrinsic=0 plus 10 busy loops on 12 cores (load average ~65) to widen the window: 12/12 consecutive green runs of the witness and V6 suites.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

Test-infrastructure only; no production code is touched. Unrelated to the separate ExpiryQueue_ReleasesStorageAfterCumulativeChurn flake in Nethermind.TxPool.Test.

WaitForImprovedBlock matched only on the parent hash. The payload service
publishes an initial empty block for a parent and improves it afterwards, so
the first BlockImproved event satisfied the wait regardless of whether the tx
pool had yet made the submitted transactions selectable — TxPool head
processing runs on a background channel, so its account/nonce view lags the
forkchoiceUpdated that advanced the head. engine_getPayload* could then return
the empty block, and Witness_includes_bytecode_and_storage_proof_for_a_called_contract
failed intermittently because the called contract had no code in the witness.

Add an additive minTransactions parameter (default 0 keeps every existing call
site unchanged) and thread the submitted transaction count through
BuildAmsterdamPayload. Improvements are monotonic — each context is seeded with
the previous best block — so observing an improvement with N transactions
guarantees the retrieved payload keeps at least N.

The two V6 call sites that submit transactions and then assert on block hash,
receipts root, or block contents carried the same latent race, so they now
require their transaction counts too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant