test(sae): Address legacypool race#5292
Merged
StephenButtolph merged 9 commits intomasterfrom Apr 27, 2026
Merged
Conversation
2d03433 to
194ad23
Compare
alarso16
commented
Apr 16, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces flakes in SAE VM tests caused by a known race in libevm’s legacy txpool nonce tracking during concurrent block execution/reorg signaling.
Changes:
- Mark
TestWorstCaseas explicitly flaky (opt-in via env var) and move it into a Bazel flaky-only test target. - Ensure receipt-related tests wait for block execution before asserting/using execution-dependent state.
- Document the “pending tx” race caveat on helper methods that wait for mempool pending state.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vms/saevm/sae/worstcase_test.go |
Makes TestWorstCase opt-in flaky and removes CLI-flag plumbing in favor of fixed parameters. |
vms/saevm/sae/vm_test.go |
Removes flag usage from TestMain; adds warnings about pending-state wait helpers under concurrent execution. |
vms/saevm/sae/rpc_test.go |
Waits for blocks to finish executing before continuing receipt-based assertions. |
vms/saevm/sae/accept_block_test.go |
Unifies flaky gating under SAEVM_TEST_FLAKY. |
vms/saevm/sae/BUILD.bazel |
Moves flaky tests into a dedicated flaky_test Bazel target and excludes them from the main test target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8e69e95 to
8b20881
Compare
8b20881 to
fb6db1f
Compare
JonathanOppenheimer
approved these changes
Apr 16, 2026
Contributor
JonathanOppenheimer
left a comment
There was a problem hiding this comment.
This look pretty good to me. You clearly did your research.
StephenButtolph
approved these changes
Apr 27, 2026
alarso16
commented
Apr 27, 2026
Contributor
Author
alarso16
left a comment
There was a problem hiding this comment.
Symbolic approval of Stephen's edits
JonathanOppenheimer
approved these changes
Apr 27, 2026
Contributor
JonathanOppenheimer
left a comment
There was a problem hiding this comment.
Restoring the fuzz testing is good IMO.
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.
Why this should be merged
There is a bug in the legacypool that's not worth fixing. Specifically, at the end of block execution, an event is sent to the TxPool to indicate a reorg. This is expected to update nonces so that transactions already in the mempool are either evicted or promoted using this new data. However, the internal nonce tracking happens after the promotions, so if there is already a pending tx for some account in the pool that is still executable, a dependent queued transaction will be incorrectly gapped. If the pending tx is ever repriced or executed (and maybe some other cases), the queued transaction can later be moved to pending.
This is a rather complicated problem, and still exists upstream in go-ethereum. There is no simple way to inject a couple lines in libevm to address this, so we should probably just deal with it here. Most importantly, this doesn't significantly affect any production processes, since the transaction will just be included in a later block. It only affects testing, because we want to rely on adding transactions to a block.
How this works
This race requires a few conditions:
This is pretty specific, so there's several ways that we can in general fix this. I only know of two flakes of this manner, fixed in this PR. The first flake,
TestWorstCase, relies on execution in the background, so we must just mark it as flaky. The second flake is easily avoided by not issuing transactions while a block is executing.I manually reviewed all tests in
sae/saevm, and if there were any blocks that expected multiple transactions from a single account, I ensured that they are either listed above, or are the first block created (aka race avoided)How this was tested
Lots and lots of debugging... if the flakes come back, I'm happy to further investigate
Need to be documented in RELEASES.md?
No