fix(mempool): tests for transaction invalidation across v2/giga (CON-375) - #4072
fix(mempool): tests for transaction invalidation across v2/giga (CON-375)#4072shemnon wants to merge 3 commits into
Conversation
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.
PR SummaryMedium Risk Overview
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. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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— thetxStoreproperties 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 ofrequiredBalanceover all ready nonces. TheevmTx.requiredBalancegodoc 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
insertsame-nonce replacement, but nottxStore.ShouldReject, which got the samecanReplacechange and readsrequiredBalancefrom the LRU cache rather than frombyNonce. A divergence between the two would silently drop valid replacements before CheckTx; a small test exercisingShouldRejectfor 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 |
There was a problem hiding this comment.
[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.
| 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) { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
…/con-375-review-preexisting-mempool-invalidation-behavior-for-v66
Summary
status=0, gasUsed=0stub receipt instead of staying pending.readyCostaccumulator toevmAccountand threads it through the readiness-advance loop, the same-nonce replacement check, andshouldReject, so all three agree on what's actually affordable.testEnv.readyTxs()'s test oracle intx_test.go, which had the identical bug baked in, and adds two targeted regression tests.Test plan
go test ./sei-tendermint/internal/mempool/...-- new regression tests fail against the pre-fix code (verified via temporary stash), pass with the fixgo test ./app/...-- CON-411 V2 integration test passesgo test ./giga/tests/...-- CON-411 Giga-differential test passes (V2/Giga byte-identical results)gofmt -s/goimportsclean on all changed filesLinear: CON-375, CON-411