Skip to content

fix(mempool): tests for transaction invalidation across v2/giga (CON-375) - #4072

Open
shemnon wants to merge 3 commits into
mainfrom
shemnon/con-375-review-preexisting-mempool-invalidation-behavior-for-v66
Open

fix(mempool): tests for transaction invalidation across v2/giga (CON-375)#4072
shemnon wants to merge 3 commits into
mainfrom
shemnon/con-375-review-preexisting-mempool-invalidation-behavior-for-v66

Conversation

@shemnon

@shemnon shemnon commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The mempool checked each queued nonce's cost against the same static per-account balance snapshot instead of a running total, so two txs from one account could both look individually affordable even though their combined cost exceeded the real balance. The second would then fail ante's real balance check at inclusion and land on-chain as a status=0, gasUsed=0 stub receipt instead of staying pending.
  • Adds a readyCost accumulator to evmAccount and threads it through the readiness-advance loop, the same-nonce replacement check, and shouldReject, so all three agree on what's actually affordable.
  • Fixes testEnv.readyTxs()'s test oracle in tx_test.go, which had the identical bug baked in, and adds two targeted regression tests.
  • Adds an EIP-7702 differential test (CON-411): a delegate/drain/undelegate sequence reproduces the same stub-receipt symptom via a completely different mechanism (execution-time SELFBALANCE-triggered balance loss, invisible to any admission-time cost estimate). Confirms Giga has no independent code path for this failure class -- any fee/nonce/balance validation failure makes it unconditionally fall back to V2 -- and that the fallback produces byte-identical results.

Test plan

  • go test ./sei-tendermint/internal/mempool/... -- new regression tests fail against the pre-fix code (verified via temporary stash), pass with the fix
  • go test ./app/... -- CON-411 V2 integration test passes
  • go test ./giga/tests/... -- CON-411 Giga-differential test passes (V2/Giga byte-identical results)
  • gofmt -s / goimports clean on all changed files
  • Manually reproduced the original bug end-to-end against a local v6.6.2 and HEAD docker cluster before the fix, and confirmed the fix corrects it (tx2 stays pending, un-hangs correctly once funded)

Linear: CON-375, CON-411

Two queued nonces for the same EVM account were each checked against
the same static balance snapshot instead of a running total, letting
the mempool mark both ready even when their combined cost exceeded
the account's real balance. The second tx would then fail ante's real
balance check at inclusion, landing on-chain as a status=0/gasUsed=0
stub receipt instead of staying pending.

- Add a readyCost accumulator to evmAccount and use it in the
  readiness-advance loop, same-nonce replacement check, and
  shouldReject, so all three agree on what's actually affordable.
- Fix testEnv.readyTxs()'s oracle in tx_test.go, which had the
  identical bug, and add two targeted regression tests.
- Add an EIP-7702 differential test (CON-411): a delegate/drain/
  undelegate sequence produces the same stub receipt via a completely
  different mechanism (SELFBALANCE-triggered execution-time balance
  loss, invisible to any admission-time cost estimate). Confirms Giga
  has no independent code path for this failure class -- it
  unconditionally falls back to V2 for any fee/nonce/balance
  validation failure -- and that the fallback produces byte-identical
  results to V2.
@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes mempool inclusion/readiness for all multi-nonce EVM senders; wrong accounting could block valid txs or still admit unaffordable ones, but behavior is covered by new unit and integration tests.

Overview
Fixes EVM mempool readiness so multiple queued nonces from one account are checked against cumulative upfront cost, not the same static balance for each tx. That mismatch let a second tx look ready when only the first was affordable, so it could be included and fail ante with the status=0, gasUsed=0 stub receipt instead of staying pending.

evmAccount now tracks readyCost and uses hasSufficientBalance, canReplace, commitReady, and replaceReady in the readiness-advance loop, same-nonce replacement, and shouldReject; readyCost is cleared on compact. The tx_test oracle matches this behavior, with two regression tests for advance and replace.

Adds EIP-7702 integration coverage (CON-411): deploy → delegate → drain balance → self-sponsored undelegate with no funds. Asserts V2 behavior (failed ante, stub receipt, nonce bump, delegation unchanged) and a Giga vs V2 differential so batch fallback yields identical results.

Reviewed by Cursor Bugbot for commit b02ed0c. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 2, 2026, 9:14 PM

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.25%. Comparing base (7273a43) to head (b02ed0c).

Files with missing lines Patch % Lines
sei-tendermint/internal/mempool/tx.go 82.60% 2 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4072      +/-   ##
==========================================
- Coverage   61.30%   60.25%   -1.06%     
==========================================
  Files        2178     2055     -123     
  Lines      190788   176580   -14208     
==========================================
- Hits       116968   106390   -10578     
+ Misses      62796    60371    -2425     
+ Partials    11024     9819    -1205     
Flag Coverage Δ
sei-chain-pr 66.57% <82.60%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-tendermint/internal/mempool/tx.go 91.39% <82.60%> (-0.68%) ⬇️

... and 205 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cumulative readyCost accounting in evmAccount is sound — every removal path funnels through compact, which resets nextNonce/readyCost and re-derives readiness, and the overflow/underflow guards are correct. The main gaps are on the test side: the new Giga differential test under-funds the authority so its final tx fails for lack of funds rather than because drain() emptied the balance, and the txStore properties doc still states the old per-tx readiness rule.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • [suggestion] sei-tendermint/internal/mempool/tx.go:188-189 — the txStore properties block still documents the pre-fix invariant ("tx is ready if all txs with lower nonces are ready or executed AND balance >= tx.requiredBalance"). Readiness is now cumulative: the balance must cover the sum of requiredBalance over all ready nonces. The evmTx.requiredBalance godoc at line 77 ("the sender balance threshold for this EVM tx to become ready") is stale for the same reason — it is now a contribution to a per-account running total, not a standalone threshold.
  • [suggestion] The new regression tests cover the readiness-advance loop and the insert same-nonce replacement, but not txStore.ShouldReject, which got the same canReplace change and reads requiredBalance from the LRU cache rather than from byNonce. A divergence between the two would silently drop valid replacements before CheckTx; a small test exercising ShouldReject for a cached-but-unaffordable replacement would pin that path down.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

// Three sponsor txs at up to 500,000 gas * 100 gwei each need up to 4.5e16
// wei; fund generously above that.
fundAccount(t, tCtx, sponsor.AccountAddress, big.NewInt(1_000_000_000_000_000_000)) // gas money only
fundAccount(t, tCtx, authority.AccountAddress, big.NewInt(1_000_000_000_000_000)) // the balance drain() will empty

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The authority is funded with 1e15 wei, but tx3's upfront balance check is gas * gasFeeCap + value = 200,000 × 100 gwei = 2e16 wei (see the V2 BuyGas equivalent at app/app.go:2954). The authority therefore cannot afford the undelegate regardless of whether drain() ran, so require.NotEqual(t, uint32(0), v2Results[3].Code, ...) passes even if tx2 is deleted — the test doesn't actually exercise the CON-411 execution-time balance-loss mechanism it documents at line 122 ("Balance is now empty, so the fee check fails").

The V2-only test in app/eip7702_selfbalance_drain_test.go funds the authority with 1_000_000 usei (1e18 wei), which is well above the 2e16 threshold, so its premise does hold. Suggest matching it here (e.g. big.NewInt(1_000_000_000_000_000_000)) so the failure is genuinely drain-induced.

Separately, the comment on line 59 says three sponsor txs at 500,000 gas × 100 gwei need "up to 4.5e16 wei"; that product is 5e16 per tx, so 1.5e17 total. The 1e18 funding still covers it, but the arithmetic in the comment is off.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

increased to 1e18

oldReady := oldEvm.nonce < account.nextNonce
// If the old tx is ready but the new tx is not, then reject the new tx.
if oldReady && account.balance.Cmp(&evm.requiredBalance) < 0 {
if oldReady && !account.canReplace(&oldEvm.requiredBalance, &evm.requiredBalance) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Worth calling out as a deliberate behavior change: a higher-priority replacement for an already-ready nonce is now rejected whenever trailing ready nonces have consumed the budget, even when the replacement alone is affordable. That makes fee-bumping the lowest queued nonce fail with errSameNonce for any account with a later ready tx — a common speed-up flow, since raising the gas price raises requiredBalance.

The strictly-correct outcome would be to accept the higher-priority replacement and demote the trailing nonces back to pending (which is what the next compact would derive anyway), rather than refusing the replacement. Rejecting is the safe conservative choice for this fix, but it isn't mentioned in the PR description and the adjacent comment ("If the old tx is ready but the new tx is not, then reject the new tx") no longer describes what the condition tests — it now conflates "the new tx is unaffordable" with "the new tx plus already-committed successors is unaffordable". At minimum, update the comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exposing a different seam outside the scope of this bug, area where behavior is intended to change. Hence there is no need to test.

@shemnon shemnon changed the title fix(mempool): track cumulative committed cost per account (CON-375) fix(mempool): tests for transaction invalidation across v2/giga (CON-375) Sep 1, 2026
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