evmrpc: include reverts in *ExcludeTraceFail discriminator (CON-296)#3450
evmrpc: include reverts in *ExcludeTraceFail discriminator (CON-296)#3450wen-coding wants to merge 1 commit into
Conversation
isPanicOrSyntheticTx mapped Status==0 to "exclude", which conflates two different things: ante failures (tx never reached the VM) and in-VM failures like reverts (tx ran and produced a real trace). Per the README's Tracing Failure Management Endpoints section, the target is the first set only — txs "included in blocks but not executed." Reverts have a trace and should stay in. Receipt fields carry enough signal to discriminate. WriteReceipt populates EffectiveGasPrice from msg.GasPrice for any tx that reached the VM; the ante-deferred receipt path in abci.go leaves it zero. isReceiptFromAnteError already encodes that check and is already used by filterTransactions, so block and tx *ExcludeTraceFail now agree on what they filter. The existing test was anchored to TestPanicTxHash, whose receipt had EffectiveGasPrice=1000000 — i.e., a revert mislabelled as a panic. Aligned the fixture with its name (real ante-failure receipt) and replaced the single assertion with a table that pins the discriminator across ante failure / synthetic / revert. The "revert is included" case is what catches future regressions in this direction. PR #3402 introduced the over-strict mapping; it hasn't been released yet, so no integrator is depending on the current behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Improves correctness across upgrades and hardens edge cases. The discriminator now evaluates Updates fixtures and expands test coverage. Test receipts are adjusted to represent true ante failures (including a pre- Reviewed by Cursor Bugbot for commit baab70c. 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 #3450 +/- ##
==========================================
- Coverage 59.29% 59.26% -0.04%
==========================================
Files 2125 2120 -5
Lines 175629 175168 -461
==========================================
- Hits 104144 103818 -326
+ Misses 62404 62298 -106
+ Partials 9081 9052 -29
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fa94814. Configure here.
Cursor Bugbot caught a real divergence on PR #3450 between filterTransactions and isPanicOrSyntheticTx. isReceiptFromAnteError branches on ctx.ClosestUpgradeName() — pre-v5.8.0 receipts have EffectiveGasPrice=0 but empty VmError, and only the pre-v5.8.0 branch catches them. filterTransactions passes ctxProvider(block.Block.Height), which picks the correct branch for old blocks. isPanicOrSyntheticTx was passing ctxProvider(LatestCtxHeight), so on a current chain it always landed on the post-v5.8.0 branch and let pre-v5.8.0 ante failures slip through. Pull the block-height context off receipt.BlockNumber and pass that to isReceiptFromAnteError — same call site shape as filterTransactions. Pinned the behavior with a new test fixture (TestPreV580AnteTxHash: EffectiveGasPrice=0, no VmError, block 99) and extended the test ctxProvider so block 99 returns a pre-v5.8.0 upgrade name and LatestCtxHeight returns a post-v5.8.0 one. semver.Compare("", "v5.8.0") returns -1, so without the explicit LatestCtxUpgradeName the buggy LatestCtxHeight path would coincidentally pass — the test fixture makes the divergence visible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
baab70c to
fa94814
Compare

Summary
isPanicOrSyntheticTxexcluded anyStatus==0receipt from the*ExcludeTraceFailendpoints, which conflates two cases:evmrpc/README.md's Tracing Failure Management Endpoints section describes the target as the first set only — txs "included in blocks but not executed." Reverts have a real trace; they should stay in. The pre-#3402 trace-based path also kept them (filtered ontrace.Error, which is empty for reverts).Discriminator
Receipt fields are sufficient.
WriteReceipt(x/evm/keeper/receipt.go) setsEffectiveGasPrice = msg.GasPricefor any tx that reached the VM. The ante-deferred receipt path (x/evm/keeper/abci.go) writes a stub receipt withEffectiveGasPrice = 0.isReceiptFromAnteErroralready encodes this check, and it's the same helperfilterTransactionsuses for block endpoints — so block and tx*ExcludeTraceFailnow agree on what they filter.Test coverage
The existing single-case test was anchored to
TestPanicTxHash, whose receipt hadEffectiveGasPrice: 1000000— by the correct discriminator that's a revert, mislabelled as a panic. The test passed only because the over-strict mapping happened to exclude it for the wrong reason.Aligned the fixture with its name (real ante-failure receipt:
EffectiveGasPrice=0,VmError="nonce too high") and replaced the single assertion with a table pinning the matrix:TestPanicTxHash(now ante)EffectiveGasPrice=0, nonce-errorVmErrorTestSyntheticTxHashTxType=ShellEVMTxTypeTestNonPanicTxHash(revert)Status=0,EffectiveGasPrice>0The "revert is included" case is the one that catches future regressions in this direction.
Backward compat
#3402 introduced the over-strict mapping (May 2026). It's on
mainonly —git branch -r --contains eb17459e5 | grep release/v6returns nothing — so no released version exposes the over-strict behavior. This fix lands before #3402 reaches any release branch.Test plan
gofmt -s -lcleango build ./evmrpc/...cleango test ./evmrpc/ -count=1passes (22s)-count=100revert_is_includedfails with the fix reverted ("Should not be: transaction is panic tx"), passes with the fix🤖 Generated with Claude Code