Repository navigation
Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call - #4318
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4318 +/- ##
==========================================
- Coverage 67.49% 66.39% -1.11%
==========================================
Files 2181 2060 -121
Lines 167798 155785 -12013
==========================================
- Hits 113251 103427 -9824
+ Misses 54537 52348 -2189
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview The Tests are flipped from “panic propagates to baseapp” to frame-level OOG (including a real Reviewed by Cursor Bugbot for commit 7cad64a. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
The PR adds a recovery step (execute) at the one point every dynamic-gas precompile call passes through. A gas-meter panic (sdk.ErrorOutOfGas or sdk.ErrorGasOverflow) becomes vm.ErrOutOfGas with zero remaining gas, and every other panic is raised again. The outer error handler now passes vm.ErrOutOfGas through unchanged instead of rewriting it as a revert. The legacy/v67 copies match the live code, no non-test precompile code returns vm.ErrOutOfGas or makes a nested evm.Call (so the new passthrough cannot catch an unrelated error), and the new call-frame and msg-server tests check the receipt, gas charged and rollback behaviour, so I found nothing that needs changing.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
|
Successfully created backport PR for |
## Summary - Bump `version.json` from `v6.7.0-rc2` to `v6.7.0-rc3` to cut the third `v6.7` release candidate. Contents since rc2: #4334, #4315, #4313, plus the rc3 changelog update (#4335, via its `release/v6.7` backport). Merge after that backport so the `v6.7.0-rc3` tag includes the updated changelog. - #4334 (backport of #4318) is `app-hash-breaking` with no height gate, so a network that already ran the v6.7 upgrade on rc1 or rc2 has to move all of its validators to rc3 together. - As on rc1 and rc2, the tagging ruleset stops `uci-release-publish` from creating the tag on merge, so push `v6.7.0-rc3` on the merge commit by hand; that push publishes the release and runs GoReleaser. - #4315 fixes the arm64 static build that failed GoReleaser on the rc1 and rc2 tag pushes, so rc3 should be the first `v6.7` release candidate with binaries attached. ## Test plan - [x] `git diff --check` Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Masih H. Derkani <m@derkani.org>
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
An EVM call into a dynamic-gas precompile whose executor exhausts its Cosmos gas meter (json's per-byte parse charge, p256's fixed verify charge, staking and the distribution dispatch, which unlike bank or wasmd have no method-level
recover) currently lets thesdk.ErrorOutOfGaspanic escapeRunAndCalculateGas.msgServer.EVMTransactionre-panics, baseapp turns it into Cosmos code 11, the msg-server state is rolled back so no receipt is written, and EndBlock synthesises the ante-failure receipt with zerogasUsedand zeroeffectiveGasPricewhile the ante handler has already chargedgasLimit × price. Fixes PLT-1318.DynamicGasPrecompilenow runs the executor through a newexecutestep that recovers onlysdk.ErrorOutOfGasandsdk.ErrorGasOverflowand reports them as(nil, 0, vm.ErrOutOfGas); every other panic (OCC aborts, store failures, programming errors) is re-raised unchanged. go-ethereum'sCallthen treats the frame like any other out-of-gas call: the snapshot is reverted, the frame's gas is consumed, the outer execution continues, and the normalWriteReceiptpath records status 0 with the real gas used and effective gas price, refunding any unused remainder. The outerRunAndCalculateGasdeferred handler keeps rewriting precompile errors tovm.ErrExecutionRevertedbut leavesvm.ErrOutOfGasintact and does not record it as a StateDB precompile error, so the receipt'sVmErrorreads a singleout of gasrather than reverted. The recovery is deliberately placed at the single choke point rather than in each precompile, and the p256 comment and test that pinned the old propagating behaviour are updated.This changes execution results, so it is app-hash breaking and is intended to ship in v6.7.0 itself (
CustomPrecompileshas no height gate, so it cannot land in a later v6.7.x patch). The archivedprecompiles/common/legacy/v67andprecompiles/p256/legacy/v67snapshots are patched by hand to match, sincescripts/bump_versiondoes not refresh an already-archived tag anddebug_trace*at v6.7 heights will run that code once v6.8 is appended;legacy/v66keeps the propagating behaviour. Any further v6.7-bound change toprecompiles/commonhas to be mirrored intolegacy/v67by hand for the same reason. Giga already falls back to v2 for every precompile address so the two executors stay in agreement. Reviewers should scrutinise that state written by a partially-executed executor is fully discarded by the EVM snapshot revert and that events from the failed frame are dropped (they are, since the inner event manager is never flushed on error). Validated by the inverted unit tests inprecompiles/commonandprecompiles/p256, a nested-frame test that drives the precompile through a realvm.EVM.Calland asserts storage writes and events from the exhausted frame are rolled back, and a new msg-server testTestEVMTransactionPrecompileOutOfGasthat drives a real json precompile call to out-of-gas and asserts a nil msg error,gasUsed == gasLimit, the sender debited exactlygasUsed × price, and a status-0 receipt with the correct effective gas price; that test panics onmainwithout the fix.