Repository navigation
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4312 +/- ##
==========================================
- Coverage 67.54% 66.45% -1.09%
==========================================
Files 2181 2062 -119
Lines 167832 155885 -11947
==========================================
- Hits 113359 103597 -9762
+ Misses 54463 52278 -2185
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
c201c68 to
965c679
Compare
…e-gas' into masih/1790168306-evmonly-estimate-gas
PR SummaryMedium Risk Overview Executor ( RPC ( Stack wiring: Reviewed by Cursor Bugbot for commit 1a94fd7. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This adds a well-tested eth_estimateGas to the EVM-only Giga RPC, modelled on go-ethereum's gas estimator. I found no blocking problems. Two suggestions: the block gas limit is skipped when the caller omits gas, and the search's probes don't share one state snapshot.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Each probe runs through
backend.EvmCall, which uses whatever committed state and block context are latest when it runs. The balance read inestimateUpperBoundworks the same way. If a block commits during the search, the passing and failing bounds come from different states, so the result can be a limit that never succeeded against any single state. go-ethereum avoids this by running the whole estimate against one state. Consider fixing the state/height once perEstimateGascall if the backend can do it, or at least document the limitation. (Also raised by Codex.) - 1 suggestion(s)/nit(s) flagged inline on specific lines.
shemnon
left a comment
shemnon
left a comment
There was a problem hiding this comment.
Requesting changes. v2 calls geth's estimator; this copies it, and the copy is already drifting. Reuse gasestimator.Estimate / export.DoEstimateGas, or say exactly why this backend cannot.
| // of gas against the current committed state. Like eth_call it creates no | ||
| // transaction and persists no state change; a call that reverts even at the | ||
| // highest allowed gas returns the revert error rather than an estimate. | ||
| func (api *callAPI) EstimateGas(ctx context.Context, args export.TransactionArgs, block *ethrpc.BlockNumberOrHash) (hexutil.Uint64, error) { |
There was a problem hiding this comment.
v2's eth_estimateGas is export.DoEstimateGas, which calls gasestimator.Estimate. "DoEstimateGas needs an ethapi.Backend" is not yet a reason to copy the search: Estimate takes a core.Message and gasestimator.Options, not that backend. If EvmCall cannot supply the State, Header, and Chain those options need, say so. Otherwise call it.
The copy is already off geth. execute there only treats ErrIntrinsicGas as "raise the limit" and returns every other error; this treats ErrFloorDataGas as raisable too.
There was a problem hiding this comment.
Done in 1a94fd7: the search is gone. Executor.EstimateGas (giga/evmonly/estimate.go) opens one committed store view, wraps it in a nativeStateDB and calls gasestimator.Estimate with a *types.Header and a minimal core.ChainContext built from the same BlockContext Call uses (a test asserts NUMBER/TIMESTAMP/PREVRANDAO/GASLIMIT/BASEFEE/COINBASE/BLOCKHASH parity). The RPC layer only validates the selector, fills defaults, caps by defaultCallGasCap and maps the estimator error.
| return 0, err | ||
| } | ||
| if failed { | ||
| if len(result.Revert()) > 0 { |
There was a problem hiding this comment.
A bare revert() has empty revert data. Geth and v2 still return code 3 with data: "0x" because they key off ErrExecutionReverted. This builds revertError only when result.Revert() is non-empty, so the same call is a plain -32000 execution reverted with no data. Match geth: code 3 whenever the error is ErrExecutionReverted.
There was a problem hiding this comment.
Done in 1a94fd7: vm.ErrExecutionReverted from the estimator now maps to the code-3 revertError regardless of data length, so a bare revert() returns {code: 3, data: "0x"}; covered at the API level and over a real ethrpc handler.
| func (api *callAPI) executeWithGas(ctx context.Context, msg *core.Message, gas uint64) (failed bool, result *core.ExecutionResult, err error) { | ||
| attempt := *msg | ||
| attempt.GasLimit = gas | ||
| result, err = api.backend.EvmCall(ctx, &attempt) |
There was a problem hiding this comment.
Add a TODO for the block overrides geth applies in BlockOverrides.Apply: number, difficulty, time, gasLimit, feeRecipient, prevRandao, baseFeePerGas, blobBaseFee. Geth rejects beaconRoot and withdrawals on this method, so leave those out.
There was a problem hiding this comment.
Done in 1a94fd7: TODO on EstimateGas listing number, difficulty, time, gasLimit, feeRecipient, prevRandao, baseFeePerGas and blobBaseFee.
| ethrpc "github.com/ethereum/go-ethereum/rpc" | ||
| ) | ||
|
|
||
| // estimateGasErrorRatio is the relative gap between the highest failing and |
There was a problem hiding this comment.
The comments restate the code. Keep the geth match and the zero-gas CallDefaults trick. Drop the search narration (63/64, geometric mid, "one execution settles it").
There was a problem hiding this comment.
Done in 1a94fd7: the search and its comments are gone with the swap to gasestimator.Estimate; the zero-gas CallDefaults comment is the one that remains.
|
Swapped to |
…ol#4325) ## Summary - Adds `eth_estimateGas` to the EVM-only Giga RPC by calling the standard `gasestimator.Estimate` library function directly, the same function the mainline (Cosmos-backed) EVM module's `eth_estimateGas` already uses, instead of the hand-rolled bisection search in sei-protocol#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.Estimate` holds it across every probe rather than reopening state per probe — and builds the minimal `*types.Header` / `core.ChainContext` / consensus `Engine` adapter `gasestimator` needs, reproducing `buildBlockContext`'s existing semantics (zero Coinbase, parent-hash-only `BLOCKHASH`). - Threads a new `EvmEstimateGas` backend primitive through `evmOnlyApplication` → `Proxy` → `Environment`, mirroring the existing `EvmCall` wiring. - Fixes two bugs found in review before merge: - `gasestimator.Estimate` never checks whether a probe's EVM was cancelled; the vendored interpreter only samples cancellation at `JUMP`/`JUMPI` and 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 after `Estimate` returns. - `BLOBBASEFEE` panicked (nil `*uint256.Int` push) because the header carries no `ExcessBlobGas` and a non-blob call leaves `BlobGasFeeCap` nil; `gasestimator`'s panic recovery turned that into a permanent "not enough gas". Fixed by defaulting `BlobGasFeeCap` to zero on a copy of the message, matching the literal zero `buildBlockContext` already uses for `eth_call`. - Adds a table-driven test covering every context-reading opcode in the `0x30`-`0x4a` range (not just `BLOBBASEFEE`) to guard against the same class of bug recurring. ## Test plan - [x] `go build ./...` - [x] `gofmt -s -l` / `goimports -l` clean on all touched files - [x] `go test ./giga/evmonly/... ./sei-tendermint/internal/evmonlyapp/... ./sei-tendermint/internal/proxy/... ./sei-tendermint/internal/rpc/core/...` — all green - [x] Manually reverted each fix and confirmed the corresponding new test fails, then restored and confirmed it passes
2 participants
The EVM-only Giga RPC serves
eth_callbut noteth_estimateGas, sosei-load,cast send, ethers and viem all fail before they can submit a transaction unless the caller hard-codes a gas limit.Executor.EstimateGasingiga/evmonlyopens one committed state-store view and hands it to go-ethereum'sgasestimator.Estimate, which copies thatnativeStateDBfor every probe, so the whole search reads a single snapshot and inherits geth's plain-transfer short-circuit,UsedGas-based lower bound, optimistic guess, balance/fee-cap capping and 1.5% error ratio. The estimator only needs a*types.Headerand acore.ChainContext, both built from the sameBlockContextCalluses; the one intentional gap isBLOBBASEFEE, which is the EIP-4844 minimum rather than the context's value because a header cannot express it directly. It is plumbed throughevmOnlyApplication,Proxy, the RPCEnvironmentand theBackendinterface as an optional capability likeEvmCall, andcallAPI.EstimateGasonly validates the block selector, fills call defaults with a zero gas so an omittedgasbounds by the block gas limit, caps bydefaultCallGasCap, and mapsvm.ErrExecutionReverted(with or without data) to the same code-3revertErroraseth_call.Read-only and consensus-neutral; the only behaviour change is that
eth_estimateGasnow exists (theTestHandlerRejectsUnknownMethodsprobe moved toeth_getStorageAt). Executor tests deploy bytecode into an in-memory store and assert plain-transfer cost, convergence againstCall, block-limit and cap bounds, revert with and without data, block-context parity withCall, no state mutation, timeout and cancellation; the RPC tests assert the selector, gas and error mapping against a stub backend and the JSON-RPC shape over a realethrpchandler.