evm: rewrite eth_estimateGas for panic safety and performance #2424
Conversation
… SDB context in successive estimation calls.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRefactors EVM gas-estimation and context keys: introduces typed context keys ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPC as EstimateGas
participant ProbeSDB as Probe StateDB
participant Prober as Probe Runner
participant VM as EVM
Client->>GRPC: EthCallRequest (with simulation flag)
GRPC->>ProbeSDB: init fresh SDB (isolated gas meter, fresh nonce)
GRPC->>Prober: run probe at hi
Prober->>VM: Execute(tx @ hi)
alt panic classified OOG
VM-->>Prober: panic (OOG)
Prober->>GRPC: treat as OOG -> adjust hi/lo
else VM error/revert
VM-->>Prober: VM error -> return error to client
else success
VM-->>Prober: success -> set lo/gasUsed
end
rect rgb(235,240,255)
Note over GRPC,Prober: Binary search loop (skew mid toward lo, stop when relative tolerance met)
loop binary search
GRPC->>Prober: run with mid (skewed)
Prober->>VM: Execute(tx @ mid)
alt OOG or exec failure
VM-->>Prober: failure -> hi = mid
else success
VM-->>Prober: success -> lo = mid
end
end
end
GRPC-->>Client: EstimateGasResponse (final estimate)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is an excellent and comprehensive rewrite of eth_estimateGas. The changes significantly improve panic safety, performance, and maintainability, aligning the implementation more closely with go-ethereum. The introduction of RecoverOutOfGasPanic and the gas-free context for probes are particularly strong improvements. My review includes a critical fix for a potential integer overflow in the new binary search logic and a suggestion to improve the clarity of the gas limit determination.
| // Determine the highest gas limit can be used during the estimation. | ||
| // Start with block gas limit | ||
| params := rootCtx.ConsensusParams() | ||
| if params != nil && params.Block != nil && params.Block.MaxGas > 0 { | ||
| hi = uint64(params.Block.MaxGas) | ||
| } else { | ||
| // Fallback to gasCap if block params not available | ||
| hi = req.GasCap | ||
| } | ||
|
|
||
| // Override with user-provided gas limit if it's valid | ||
| if args.Gas != nil && uint64(*args.Gas) >= gethparams.TxGas { | ||
| hi = uint64(*args.Gas) | ||
| } else { | ||
| // Query block gas limit | ||
| params := ctx.ConsensusParams() | ||
| if params != nil && params.Block != nil && params.Block.MaxGas > 0 { | ||
| hi = uint64(params.Block.MaxGas) | ||
| } else { | ||
| hi = req.GasCap | ||
| } | ||
| } | ||
|
|
||
| // TODO: Recap the highest gas limit with account's available balance. | ||
| // Recap the highest gas allowance with specified gascap. | ||
| if req.GasCap != 0 && hi > req.GasCap { | ||
| hi = req.GasCap | ||
| } |
There was a problem hiding this comment.
The logic to determine the upper bound hi is functionally correct, but its structure could be clearer. It currently initializes hi from the block/request gas cap, then potentially overrides it with args.Gas, and finally caps it again with req.GasCap.
A more direct structure that first checks for args.Gas and then falls back to other limits would improve readability and maintainability, aligning more closely with the previous implementation and standard practice.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/changelog-check.yml(1 hunks)x/evm/const.go(1 hunks)x/evm/evm.go(1 hunks)x/evm/evmstate/grpc_query.go(9 hunks)x/evm/evmstate/keeper.go(1 hunks)x/evm/evmstate/keeper_test.go(3 hunks)x/evm/evmtest/tx.go(2 hunks)x/evm/msg.go(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
📚 Learning: 2024-12-06T16:10:34.174Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Applied to files:
x/evm/const.gox/evm/msg.gox/evm/evmtest/tx.gox/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/msg.gox/evm/evmtest/tx.gox/evm/evmstate/grpc_query.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/msg.gox/evm/evmtest/tx.go
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Applied to files:
x/evm/msg.gox/evm/evmtest/tx.gox/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-10T08:13:40.130Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2379
File: x/evm/keeper/erc20.go:103-112
Timestamp: 2025-09-10T08:13:40.130Z
Learning: ERC20 utility functions in x/evm/keeper/erc20.go (Transfer, Mint, Burn) should use COMMIT_READONLY rather than COMMIT_ETH_TX because they are designed as building blocks for larger Ethereum transactions, particularly in precompiles where multiple operations happen with a persistent *vm.EVM object and vm.StateDB. The commit should happen at the transaction level, not at each individual ERC20 operation level.
Applied to files:
x/evm/evmtest/tx.go
📚 Learning: 2025-09-10T08:13:32.673Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2379
File: x/evm/keeper/erc20.go:75-78
Timestamp: 2025-09-10T08:13:32.673Z
Learning: ERC20 utility functions in x/evm/keeper/erc20.go (like Mint, Transfer, Burn) should use COMMIT_READONLY instead of COMMIT_ETH_TX because they are designed as building blocks for larger Ethereum transactions, particularly within precompiles where multiple operations happen with a persistent *vm.EVM object and vm.StateDB. The commit decision should happen at the transaction level, not at each individual ERC20 operation.
Applied to files:
x/evm/evmtest/tx.go
📚 Learning: 2024-11-04T20:38:45.909Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Applied to files:
x/evm/evmtest/tx.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns only `*vm.EVM` without an error value.
Applied to files:
x/evm/evmtest/tx.gox/evm/evmstate/keeper_test.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmstate/grpc_query.gox/evm/evmstate/keeper_test.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmstate/grpc_query.gox/evm/evmstate/keeper_test.go
📚 Learning: 2025-09-16T00:11:38.994Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:80-90
Timestamp: 2025-09-16T00:11:38.994Z
Learning: The `WithLastErrApplyEvmMsg` method on sdk.Context in the Nibiru codebase is implemented as an in-place setter that mutates the context directly, unlike standard Cosmos SDK With* methods which return new contexts.
Applied to files:
x/evm/evmstate/grpc_query.gox/evm/evmstate/keeper.go
🧬 Code graph analysis (5)
x/evm/msg.go (2)
x/evm/tx.pb.go (3)
MsgEthereumTxResponse(333-348)MsgEthereumTxResponse(352-352)MsgEthereumTxResponse(353-355)api/eth/evm/v1/tx.pulsar.go (3)
MsgEthereumTxResponse(9205-9224)MsgEthereumTxResponse(9239-9239)MsgEthereumTxResponse(9242-9244)
x/evm/evmtest/tx.go (1)
x/evm/const.go (1)
GasEstimateZeroToleranceCtxKey(43-43)
x/evm/evmstate/grpc_query.go (5)
x/evm/const.go (2)
SimulationCtxKey(42-42)GasEstimateZeroToleranceCtxKey(43-43)x/evm/evm.go (1)
RecoverOutOfGasPanic(123-135)eth/gas_limit.go (1)
NewInfiniteGasMeterWithLimit(42-47)x/evm/errors.go (1)
NewRevertError(60-71)x/evm/msg.go (2)
GasEstimateErrorRatioTolerance(518-518)BinSearch(521-558)
x/evm/evmstate/keeper_test.go (1)
x/evm/const.go (1)
SimulationCtxKey(42-42)
x/evm/evmstate/keeper.go (1)
x/evm/const.go (1)
SimulationCtxKey(42-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: heavy-tests
- GitHub Check: Cursor Bugbot
- GitHub Check: proto-gen-rs
- GitHub Check: build
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: e2e-evm
🔇 Additional comments (1)
.github/workflows/changelog-check.yml (1)
1-1: Clean workflow name simplification.The renaming from "Check changelog update" to "Changelog" improves clarity without affecting workflow behavior or logic.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/evm/evmstate/grpc_query.go (1)
357-375: Consider restructuring the upper bound logic for clarity.The logic is functionally correct but involves setting
hifrom block max gas, potentially overriding it withargs.Gas, and then capping it again withreq.GasCap. This creates a back-and-forth that can be hard to follow.Consider a more direct structure:
// Determine the absolute maximum gas limit var absoluteMax uint64 params := rootCtx.ConsensusParams() if params != nil && params.Block != nil && params.Block.MaxGas > 0 { absoluteMax = uint64(params.Block.MaxGas) } else { absoluteMax = req.GasCap } // Start with user-provided gas if valid, otherwise use the absolute maximum hi := absoluteMax if args.Gas != nil && uint64(*args.Gas) >= gethparams.TxGas { hi = min(uint64(*args.Gas), absoluteMax) } // Apply final cap with GasCap if req.GasCap != 0 && hi > req.GasCap { hi = req.GasCap }This makes the precedence clearer: absolute max → user preference → final cap.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
evm-e2e/test/eth_queries.test.ts(1 hunks)x/evm/const.go(1 hunks)x/evm/evmstate/grpc_query.go(9 hunks)x/evm/evmstate/keeper.go(1 hunks)x/evm/evmstate/keeper_test.go(3 hunks)x/evm/evmtest/tx.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- x/evm/const.go
- x/evm/evmtest/tx.go
- x/evm/evmstate/keeper_test.go
- x/evm/evmstate/keeper.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
evm-e2e/test/eth_queries.test.tsx/evm/evmstate/grpc_query.go
📚 Learning: 2024-12-06T16:10:34.174Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-11-04T20:38:45.909Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:11:38.994Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:80-90
Timestamp: 2025-09-16T00:11:38.994Z
Learning: The `WithLastErrApplyEvmMsg` method on sdk.Context in the Nibiru codebase is implemented as an in-place setter that mutates the context directly, unlike standard Cosmos SDK With* methods which return new contexts.
Applied to files:
x/evm/evmstate/grpc_query.go
🧬 Code graph analysis (2)
evm-e2e/test/eth_queries.test.ts (1)
evm-e2e/test/utils.ts (1)
INTRINSIC_TX_GAS(29-29)
x/evm/evmstate/grpc_query.go (5)
x/evm/const.go (2)
CtxKeyEvmSimulation(42-42)CtxKeyGasEstimateZeroTolerance(43-43)x/evm/evm.go (1)
RecoverOutOfGasPanic(123-135)eth/gas_limit.go (1)
NewInfiniteGasMeterWithLimit(42-47)x/evm/errors.go (1)
NewRevertError(60-71)x/evm/msg.go (2)
GasEstimateErrorRatioTolerance(518-518)BinSearch(521-558)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Cursor Bugbot
- GitHub Check: heavy-tests
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: proto-gen-rs
- GitHub Check: e2e-evm
- GitHub Check: lint
🔇 Additional comments (6)
evm-e2e/test/eth_queries.test.ts (1)
23-32: LGTM! Tolerance-based assertion aligns with the new gas estimation flow.The updated assertion correctly validates that the estimated gas for a simple value transfer is within 5% of the intrinsic gas cost. This aligns with the PR's introduction of tolerance-based gas estimation (
GasEstimateErrorRatioTolerance = 0.015in the backend).x/evm/evmstate/grpc_query.go (5)
267-267: LGTM! Consistent context key migration.The migration from
SimulationContextKeytoevm.CtxKeyEvmSimulationis applied consistently across all query methods (EthCall, EstimateGas, TraceTx, TraceCall, TraceBlock), aligning with the context key definitions inx/evm/const.go.Also applies to: 321-321, 506-506, 603-603, 693-693
383-451: LGTM! Well-structured executable function with proper panic handling.The
executablefunction correctly:
- Recovers SDK/EVM out-of-gas panics and treats them as VM failures (allowing binary search to continue)
- Propagates unexpected panics as contextual errors
- Creates a fresh StateDB for each probe with infinite gas meter and zero KV gas costs (preventing store-gas panics)
- Handles account nonce increment to mirror the ante handler behavior
- Treats
core.ErrIntrinsicGasas a VM failure that requires raising the gas limit- Distinguishes consensus errors (abort search) from VM errors (continue search)
453-469: LGTM! Base case execution correctly validates and seeds the lower bound.The base case logic properly:
- Executes at the highest allowable gas limit to validate the transaction
- Returns immediately on consensus errors
- Distinguishes between different failure modes:
- Execution reverts → return decoded revert error
- Non-OOG VM errors → return the VM error
- OOG at max limit → return "gas required exceeds allowance"
This approach efficiently validates the transaction and provides clear error messages.
472-476: LGTM! Underflow protection correctly implemented.The code now properly handles the case where
result.GasUsed == 0by settinglo = 0instead of attempting to subtract one (which would underflow tomath.MaxUint64). This addresses the past review comment and prevents corruption of the binary search.
479-486: LGTM! Tolerance-based binary search enables performance improvements.The tolerance handling correctly:
- Defaults to
evm.GasEstimateErrorRatioTolerance(~1.5%) for production use- Allows tests to set
CtxKeyGasEstimateZeroTolerancefor exact results- Passes the tolerance to the
BinSearchfunction which implements skewed bisectionThis aligns with the PR's goal of reducing average binary-search iterations by ~60% while maintaining accuracy.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
x/evm/evmstate/grpc_query.go (1)
357-375: Upper bound determination logic is confusing.The logic initializes
hifrom the block gas limit orreq.GasCap, then potentially overrides it withargs.Gas, and finally caps it again withreq.GasCap. A clearer structure would checkargs.Gasfirst, then fall back to block limit, then apply the cap. This makes the logic flow more intuitive and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/evm/evm.go(0 hunks)x/evm/evmstate/grpc_query.go(9 hunks)
💤 Files with no reviewable changes (1)
- x/evm/evm.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
📚 Learning: 2024-12-06T16:10:34.174Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-11-04T20:38:45.909Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:11:38.994Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:80-90
Timestamp: 2025-09-16T00:11:38.994Z
Learning: The `WithLastErrApplyEvmMsg` method on sdk.Context in the Nibiru codebase is implemented as an in-place setter that mutates the context directly, unlike standard Cosmos SDK With* methods which return new contexts.
Applied to files:
x/evm/evmstate/grpc_query.go
🧬 Code graph analysis (1)
x/evm/evmstate/grpc_query.go (4)
x/evm/const.go (2)
CtxKeyEvmSimulation(42-42)CtxKeyGasEstimateZeroTolerance(43-43)eth/gas_limit.go (1)
NewInfiniteGasMeterWithLimit(42-47)x/evm/errors.go (1)
NewRevertError(60-71)x/evm/msg.go (2)
GasEstimateErrorRatioTolerance(518-518)BinSearch(521-558)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: e2e-evm
- GitHub Check: build
- GitHub Check: proto-gen-rs
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: heavy-tests
🔇 Additional comments (4)
x/evm/evmstate/grpc_query.go (4)
267-267: LGTM! Context key refactoring is consistent.The replacement of
SimulationContextKeywithevm.CtxKeyEvmSimulationis applied consistently across EthCall, TraceTx, TraceCall, and TraceBlock. This aligns with the new typed context keys introduced inx/evm/const.go.Also applies to: 521-521, 618-618, 708-708
468-491: Good optimization: base case seeds the lower bound.Executing at the maximum permissible gas first validates the transaction and provides an actual
GasUsedvalue to seed a smarter lower bound for the binary search. The underflow protection at lines 487-491 correctly handles the edge case whereGasUsed == 0.This optimization aligns with the PR objective to reduce average binary-search iterations by ~60%.
430-454: Excellent panic-safety design.Each probe constructs a fresh StateDB with:
InfiniteGasMeterWithLimit(evmMsg.GasLimit)to prevent SDK gas panics- Zero
KVGasConfigandTransientKVGasConfigto eliminate store-gas consumptionAccount nonce handling correctly mirrors the ante-handler behavior by incrementing to
nonce + 1, ensuring consistency with transaction execution. This design isolates each probe from store-gas panics while maintaining correct state semantics.Based on learnings: The pattern of assigning a new gas meter to the context is intentional and acceptable to isolate gas consumption.
494-501: Binary search with tolerance improves performance.The implementation uses
evm.GasEstimateErrorRatioTolerance(1.5%) for early exit, with an override path viaevm.CtxKeyGasEstimateZeroTolerancefor test scenarios requiring exact values. The skewed bisection logic inevm.BinSearch(capping midpoint tolo*2) further optimizes convergence toward the actual gas requirement.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/evm/evmstate/grpc_query.go (1)
469-469: Fix typo in comment."extermely" should be "extremely".
🧹 Nitpick comments (2)
x/evm/evmstate/grpc_query.go (2)
357-375: Consider restructuring upper bound logic for clarity.The current logic initializes
hifrom block gas limit, then potentially overrides it withargs.Gas, and finally caps it withreq.GasCap. While functionally correct, this multi-step assignment could be more straightforward.Consider checking
args.Gasfirst (if valid and within bounds), then falling back to the minimum of block gas andreq.GasCap. This would make the precedence more explicit and reduce the back-and-forth assignments.Based on the previous review comment, would you like me to generate a clearer implementation that checks
args.Gasfirst before falling back to block/cap limits?
399-399: Fix misleading comment.The comment states "Non-OOG panics are not handled here," but they are actually handled in this else branch by setting
oog=falseandperrto a descriptive error.Apply this diff to clarify the comment:
- } else { - // Non-OOG panics are not handled here + } else { + // Handle non-OOG panics oog, perr = false, fmt.Errorf( `unexpected panic in eth_estimateGas { gas: %d }: %v`, gas, panicInfo) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/evm/evmstate/grpc_query.go(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
📚 Learning: 2024-12-06T16:10:34.174Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-11-04T20:38:45.909Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:11:38.994Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:80-90
Timestamp: 2025-09-16T00:11:38.994Z
Learning: The `WithLastErrApplyEvmMsg` method on sdk.Context in the Nibiru codebase is implemented as an in-place setter that mutates the context directly, unlike standard Cosmos SDK With* methods which return new contexts.
Applied to files:
x/evm/evmstate/grpc_query.go
🧬 Code graph analysis (1)
x/evm/evmstate/grpc_query.go (4)
x/evm/const.go (2)
CtxKeyEvmSimulation(42-42)CtxKeyGasEstimateZeroTolerance(43-43)eth/gas_limit.go (1)
NewInfiniteGasMeterWithLimit(42-47)x/evm/errors.go (1)
NewRevertError(60-71)x/evm/msg.go (2)
GasEstimateErrorRatioTolerance(518-518)BinSearch(521-558)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: heavy-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: e2e-evm
- GitHub Check: proto-gen-rs
- GitHub Check: build
🔇 Additional comments (7)
x/evm/evmstate/grpc_query.go (7)
267-267: LGTM! Context key updates are consistent.The migration to
evm.CtxKeyEvmSimulationis applied consistently across EthCall, TraceTx, TraceCall, and TraceBlock paths, aligning with the new typed context key pattern introduced in this PR.Also applies to: 522-522, 619-619, 709-709
313-322: LGTM! Simplified API surface.The new
EstimateGasmethod signature removes theCallTypeabstraction and provides a cleaner public API. The initialization ofrootCtxwith the simulation flag is correct.
413-428: LGTM! Message reconstruction is correct.The pattern of reconstructing
evmMsgwith only theGasLimitchanged is clear and correct. The inline comment on line 418 helpfully marks the modified field.
430-455: LGTM! Gas-free simulation context is correctly configured.The initialization creates an isolated probe environment:
- Infinite gas meter with limit prevents SDK store-gas panics
- Zero KV gas configs eliminate state access costs
- Nonce increment matches ante-handler semantics
This aligns with the PR's panic-safety objectives. Based on learnings from the codebase, assigning a new gas meter to the context is an acceptable pattern for ensuring operations don't affect the parent context's gas consumption.
457-467: LGTM! Error handling semantics are correct.The special treatment of
core.ErrIntrinsicGasas a VM failure (allowing the search to continue) aligns with the PR's stated error semantics. Other errors properly abort the search.
488-492: LGTM! Underflow protection correctly implemented.The check for
result.GasUsed > 0before subtracting prevents the underflow issue that was flagged in previous reviews. WhenGasUsedis zero,lois correctly set to 0 instead of wrapping tomath.MaxUint64.
494-502: LGTM! Binary search configuration is correct.The tolerance-based early exit (defaulting to 1.5% per PR objectives) with support for zero-tolerance via context flag is correctly implemented. The call to
evm.BinSearchwith the new signature aligns with the refactored binary search logic mentioned in the PR objectives.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
justfile (2)
61-61: Consider parameterizing or dynamically determining the version.The version
v2.7.0is hardcoded. Consider making this a parameter or determining it dynamically using git tags.Apply this diff to make the version a parameter with a default:
+# Generates CHANGELOG-UNRELEASED.md based on commits and pull requests. +# Usage: just gen-changelog [last-version] -gen-changelog: +gen-changelog LAST_VER="v2.7.0": #!/usr/bin/env bash source contrib/bashlib.sh which_ok cargo if ! which_ok git-cliff; then echo "Installing git-cliff with cargo" cargo install git-cliff fi which_ok git-cliff - LAST_VER="v2.7.0" start_branch="$(git branch --show-current)"
63-65: Add safety check before switching branches.Switching branches without checking for uncommitted changes could fail or leave the repository in an unexpected state. Add a check or use
git stashto handle uncommitted changes.Apply this diff to add a safety check:
start_branch="$(git branch --show-current)" + # Ensure we're on a clean working tree or stash changes + if ! git diff-index --quiet HEAD --; then + log_info "Uncommitted changes detected. Stashing..." + git stash push -m "gen-changelog: auto-stash before branch switch" + STASHED=1 + fi git checkout main git-cliff "$LAST_VER.." -o CHANGELOG-UNRELEASED.md git checkout "$start_branch" + if [ "${STASHED:-0}" -eq 1 ]; then + log_info "Restoring stashed changes..." + git stash pop + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/changelog-check.yml(0 hunks)CHANGELOG-UNRELEASED.md(1 hunks)cliff.toml(1 hunks)justfile(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/changelog-check.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
🪛 LanguageTool
CHANGELOG-UNRELEASED.md
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ing using gocovmerge; bring README more up to date in [#2416](https://github.com/NibiruCha...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~29-~29: The official name of this software platform is spelled with a capital “H”.
Context: ...3ed1821105)) by @Unique-Divine * feat(.github/pr-title-lint): Enable "/", capital let...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Cursor Bugbot
- GitHub Check: proto-gen-rs
- GitHub Check: build
- GitHub Check: e2e-evm
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: heavy-tests
| if result.GasUsed > 0 { | ||
| lo = result.GasUsed - 1 | ||
| } else { | ||
| lo = 0 | ||
| } |
There was a problem hiding this comment.
this is great, it could transform binary search into a couple of tries only
There was a problem hiding this comment.
Yeah, it's likely the first run will get the value neeeded
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2424 +/- ##
=======================================
Coverage 59.26% 59.26%
=======================================
Files 359 359
Lines 24205 24216 +11
=======================================
+ Hits 14344 14352 +8
Misses 8635 8635
- Partials 1226 1229 +3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
x/evm/evmstate/grpc_query.go (1)
488-492: Prevent potential infinite loop when GasUsed is zero.If
result.GasUsedis zero,lois set to 0. When passed toevm.BinSearch, the skew logic (mid = lo * 2) would setmid = 0, causingexecutable(0)to fail repeatedly with the same state (lo=0, hi=hi), resulting in an infinite loop.While
GasUsed=0is extremely unlikely in practice (EVM always consumes at least 21000 intrinsic gas), defensive programming suggests ensuringlo >= 1orlo >= gethparams.TxGasto avoid this edge case.Apply this diff to ensure a minimum lower bound:
// Set smart lower bound based on actual gas used if result.GasUsed > 0 { lo = result.GasUsed - 1 } else { - lo = 0 + // Ensure lo is at least 1 to avoid infinite loop in BinSearch skew logic + lo = 1 }Or more conservatively, set it to the intrinsic gas cost:
// Set smart lower bound based on actual gas used if result.GasUsed > 0 { lo = result.GasUsed - 1 } else { - lo = 0 + // Fallback to intrinsic gas cost if GasUsed is unexpectedly zero + lo = gethparams.TxGas - 1 }
🧹 Nitpick comments (2)
justfile (2)
63-63: Consider updating main branch before generating changelog.The script checks out main but doesn't ensure it's up-to-date with the remote. This could result in an incomplete or stale changelog if local main is behind origin/main.
Add a fetch and pull before checking out:
LAST_VER="v2.7.0" start_branch="$(git branch --show-current)" + git fetch origin main git checkout main + git pull origin main git-cliff "$LAST_VER.." -o CHANGELOG-UNRELEASED.md
61-61: Consider making the last version configurable.The
LAST_VERvalue is hardcoded and will require manual updates for each release. Consider making it a parameter or detecting it automatically.Option 1 - Make it a recipe parameter with a default:
-gen-changelog: +gen-changelog LAST_VER="v2.7.0": #!/usr/bin/env bash source contrib/bashlib.sh which_ok cargo if ! which_ok git-cliff; then echo "Installing git-cliff with cargo" cargo install git-cliff fi which_ok git-cliff - LAST_VER="v2.7.0" start_branch="$(git branch --show-current)"Option 2 - Auto-detect the latest tag:
- LAST_VER="v2.7.0" + LAST_VER="$(git describe --tags --abbrev=0)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cliff.toml(1 hunks)justfile(1 hunks)x/evm/evmstate/grpc_query.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cliff.toml
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2424
File: x/evm/msg.go:536-545
Timestamp: 2025-11-03T17:12:06.110Z
Learning: In the BinSearch function used for gas estimation in x/evm/msg.go, the lower bound `lo` is always initialized to at least the intrinsic gas cost (21000), never zero. This ensures that the skew logic `mid = lo * 2` always produces a valid positive midpoint value.
📚 Learning: 2025-09-16T00:14:44.279Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:92-97
Timestamp: 2025-09-16T00:14:44.279Z
Learning: In the EthereumTx function in x/evm/keeper/msg_server.go, the existing pattern of checking `if evmResp != nil` before calling SafeConsumeGas is correct because the control flow already handles the nil case through an early return when there's a consensus error (applyErr != nil && evmResp == nil).
Applied to files:
justfilex/evm/evmstate/grpc_query.go
📚 Learning: 2024-12-06T16:10:34.174Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension.go:0-0
Timestamp: 2024-12-06T16:10:34.174Z
Learning: In `x/evm/keeper/bank_extension.go`, assigning a new `GasMeter` to the context within the `NibiruBankKeeper.ForceGasInvariant` function is intentional and acceptable. This ensures that subsequent operations do not affect the parent `ctx`'s gas consumption.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-11-03T17:12:06.110Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2424
File: x/evm/msg.go:536-545
Timestamp: 2025-11-03T17:12:06.110Z
Learning: In the BinSearch function used for gas estimation in x/evm/msg.go, the lower bound `lo` is always initialized to at least the intrinsic gas cost (21000), never zero. This ensures that the skew logic `mid = lo * 2` always produces a valid positive midpoint value.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-10-13T05:48:51.771Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2076
File: x/evm/keeper/msg_server.go:115-116
Timestamp: 2024-10-13T05:48:51.771Z
Learning: In the `ApplyEvmTx` function within `x/evm/keeper/msg_server.go`, a zero value for `weiPerGas` is acceptable as it signifies that gas is free. There is no need to check for zero before performing the gas refund in this context.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-12-06T11:54:38.275Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2119
File: x/evm/keeper/bank_extension_test.go:67-71
Timestamp: 2024-12-06T11:54:38.275Z
Learning: When writing tests in `x/evm/keeper/bank_extension_test.go`, prefer comparing gas consumption values as formatted strings using `fmt.Sprintf("%d", value)` instead of numeric values directly in `s.Equalf`. This avoids hexadecimal representations that are hard to read in shell outputs.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2024-11-04T20:38:45.909Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.909Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:27.341Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/evmmodule/genesis_test.go:51-51
Timestamp: 2025-01-17T17:32:27.341Z
Learning: The `NewEVM()` method in TestDeps returns `(*vm.EVM, *statedb.StateDB)` without an error value. The second return value is often discarded when only the EVM object is needed.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-01-17T17:32:33.412Z
Learnt from: k-yang
Repo: NibiruChain/nibiru PR: 2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
Applied to files:
x/evm/evmstate/grpc_query.go
📚 Learning: 2025-09-16T00:11:38.994Z
Learnt from: Unique-Divine
Repo: NibiruChain/nibiru PR: 2385
File: x/evm/keeper/msg_server.go:80-90
Timestamp: 2025-09-16T00:11:38.994Z
Learning: The `WithLastErrApplyEvmMsg` method on sdk.Context in the Nibiru codebase is implemented as an in-place setter that mutates the context directly, unlike standard Cosmos SDK With* methods which return new contexts.
Applied to files:
x/evm/evmstate/grpc_query.go
🧬 Code graph analysis (1)
x/evm/evmstate/grpc_query.go (5)
x/evm/const.go (2)
CtxKeyEvmSimulation(42-42)CtxKeyGasEstimateZeroTolerance(43-43)x/evm/query.pb.go (6)
EthCallRequest(711-720)EthCallRequest(724-724)EthCallRequest(725-727)EstimateGasResponse(784-787)EstimateGasResponse(791-791)EstimateGasResponse(792-794)eth/gas_limit.go (1)
NewInfiniteGasMeterWithLimit(42-47)x/evm/errors.go (1)
NewRevertError(60-71)x/evm/msg.go (2)
GasEstimateErrorRatioTolerance(518-518)BinSearch(521-558)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: proto-gen-rs
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: heavy-tests
- GitHub Check: e2e-evm
| git checkout main | ||
| git-cliff "$LAST_VER.." -o CHANGELOG-UNRELEASED.md | ||
| git checkout "$start_branch" |
There was a problem hiding this comment.
Add error handling for git checkout commands.
The git checkout commands lack error handling. If git checkout main fails (e.g., due to uncommitted changes or missing branch), the script continues and runs git-cliff on the wrong branch. If git checkout "$start_branch" fails, the user is left on the main branch unexpectedly.
Apply this diff to add error handling:
- git checkout main
- git-cliff "$LAST_VER.." -o CHANGELOG-UNRELEASED.md
- git checkout "$start_branch"
+ git checkout main || { log_error "Failed to checkout main branch"; exit 1; }
+ git-cliff "$LAST_VER.." -o CHANGELOG-UNRELEASED.md || { log_error "git-cliff failed"; git checkout "$start_branch"; exit 1; }
+ git checkout "$start_branch" || { log_error "Failed to return to branch $start_branch"; exit 1; }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In justfile around lines 63-65, the two git checkout commands lack error
handling; ensure each git checkout is checked and the script aborts or restores
state on failure: after running git checkout main, verify it succeeded and if
not print a clear error and exit so git-cliff won't run on the wrong branch;
after running git checkout "$start_branch", verify it succeeded and if not print
an error and either attempt to return to the previous branch (if known) or exit
with a non-zero status so the user is not left on main unexpectedly.
evm: rewrite eth_estimateGas for panic safety and performance
OOG panics as normal VM errors.
eliminating SDK store-gas panics.
Purpose:
Outcome
eth_estimateGasis now:Summary
This PR rewrites Nibiru’s
eth_estimateGasto be panic-safe,deterministic, and aligned with go-ethereum’s semantics, while cutting
average binary-search iterations by ~60%.
1. Unified gas estimation
Removed
CallTypeenum andEstimateGasForEvmCallType; logic is now consolidated in a singleEstimateGasmethod.Always executes under a consistent simulation context (
rootCtx) with:This guarantees no store-gas panics during estimation.
2. Robust panic handling
Replaced
HandleOutOfGasPanic(*error)with functionalRecoverOutOfGasPanic(context string) (bool, error).Classifies panics:
"out of gas"→ VM error (vmError=true), continue search."unexpected panic in eth_estimateGas").Prevents crashes that previously bubbled up to gRPC middleware.
3. Simplified probe semantics
evmMsg.GasLimitchanges per iteration (evmMsg.GasLimit = gas).nonce + 1).4. Performance improvements
Execute once at max gas; set
lo = result.GasUsed - 1.Cap midpoint to
lo * 2when standard midpoint overshoots typical usage.Stop when
(hi - lo) / hi < 1.5%; wallets already apply 20–25% buffers.5. Error semantics
core.ErrIntrinsicGastreated as VM failure, not hard error."gas required exceeds allowance (<cap>)".Behavioral outcomes
(isOog, err)classifierReferences
eth/gasestimator/gasestimator.golines 123-191Note
Rewrites
eth_estimateGasfor panic safety and faster convergence, updates context keys and binsearch logic, adjusts tests, and adds git-cliff changelog generation tooling.Query/EstimateGas(removedCallTypepath) and rewrote logic to run probes on gas-free contexts with panic classification (treat OOG as VM failure).BinSearch(GasEstimateErrorRatioTolerance, midpoint skew), and cap/nonce handling fixes.CtxKeyEvmSimulationandCtxKeyGasEstimateZeroTolerance; removed old panic helper andCallType.CtxKeyEvmSimulationinEthCall,TraceTx,TraceCall, andTraceBlock.eth_estimateGasassertion inevm-e2e/test/eth_queries.test.ts.evmtest.GenerateEthTxMsgAndSigner.git-cliffconfig (cliff.toml) andjust gen-changelog; generateCHANGELOG-UNRELEASED.md..github/workflows/changelog-check.yml.Written by Cursor Bugbot for commit dd6db35. This will update automatically on new commits. Configure here.