Repository navigation
feat(evmonly): add eth_estimateGas via existing libraries - #4325
Conversation
Calls gasestimator.Estimate directly instead of hand-rolling the search (the approach in #4312, which had known correctness bugs), the same way the mainline EVM module's eth_estimateGas already works. - Executor.EstimateGas opens one state snapshot for the whole search and builds the minimal Header/ChainContext/Engine adapter gasestimator needs - threads a new EvmEstimateGas backend primitive through evmOnlyApplication, Proxy, and Environment, alongside EvmCall - guards against gasestimator's cancellation blind spot (a probe cut off at JUMP/JUMPI reads back as a successful halt) and a BLOBBASEFEE panic from an unset BlobBaseFee, both found in review - tests every context-reading opcode in the 0x30-0x4a range for panics, not just BLOBBASEFEE
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview The executor builds minimal header/chain/engine adapters for the estimator, applies the same RPC behavior matches Reviewed by Cursor Bugbot for commit 2031950. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Clean change: eth_estimateGas calls go-ethereum's gasestimator.Estimate over a single snapshot, and the wiring from app to Proxy to Environment matches the existing EvmCall path. It handles the BLOBBASEFEE nil panic and the missed-cancellation case, and the tests cover both. There is one small suggestion about how the error is reported when the caller cancels.
Findings: 0 blocking | 1 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4325 +/- ##
==========================================
- Coverage 67.50% 66.41% -1.10%
==========================================
Files 2181 2062 -119
Lines 167848 155891 -11957
==========================================
- Hits 113306 103531 -9775
+ Misses 54532 52350 -2182
Partials 10 10
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 using default effort 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 59d9bae. Configure here.
Refer to it generically as the standard gasestimator library instead.
seidroid: estimateCtx.Err() is also non-nil when the caller's own context is cancelled (e.g. a disconnected client), which previously returned the "exceeded timeout" message instead of context.Canceled. Check the caller's context first so callers and metrics can tell a real timeout from a cancellation. Also drops a stateDB.Error() check that Cursor flagged as looking at the wrong object: gasestimator.Estimate's run already checks every probe's own copy and returns that as a real error before any success path is reachable, so the outer stateDB can never be in an error state once Estimate returns nil. Confirmed dead code, not a live bug.
wen-coding: - note the OpenView-outside-the-lock race in currentExecutionContext - add BLOCKHASH to the context-opcode panic test - trim the EstimateGas godoc's dangling reference to the unmerged hand-rolled search Also passes over every comment this change added: shorter, what not why/how, and drops a couple of stale references to a test that no longer exists under that name.
* 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) ...

Summary
eth_estimateGasto the EVM-only Giga RPC by calling the standardgasestimator.Estimatelibrary function directly, the same function the mainline (Cosmos-backed) EVM module'seth_estimateGasalready uses, instead of the hand-rolled bisection search in Add eth_estimateGas to the EVM-only Giga RPC #4312 (which had known correctness bugs and was not merged).Executor.EstimateGas(giga/evmonly/estimate.go) opens one state snapshot for the whole search —gasestimator.Estimateholds it across every probe rather than reopening state per probe — and builds the minimal*types.Header/core.ChainContext/ consensusEngineadaptergasestimatorneeds, reproducingbuildBlockContext's existing semantics (zero Coinbase, parent-hash-onlyBLOCKHASH).EvmEstimateGasbackend primitive throughevmOnlyApplication→Proxy→Environment, mirroring the existingEvmCallwiring.gasestimator.Estimatenever checks whether a probe's EVM was cancelled; the vendored interpreter only samples cancellation atJUMP/JUMPIand clears it to a normal halt, so a probe cut off by the search's deadline could read back as a successful low estimate. Now checked explicitly afterEstimatereturns.BLOBBASEFEEpanicked (nil*uint256.Intpush) because the header carries noExcessBlobGasand a non-blob call leavesBlobGasFeeCapnil;gasestimator's panic recovery turned that into a permanent "not enough gas". Fixed by defaultingBlobGasFeeCapto zero on a copy of the message, matching the literal zerobuildBlockContextalready uses foreth_call.0x30-0x4arange (not justBLOBBASEFEE) to guard against the same class of bug recurring.Test plan
go build ./...gofmt -s -l/goimports -lclean on all touched filesgo test ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/... ./sei-tendermint/internal/proxy/... ./sei-tendermint/internal/rpc/core/...— all green