fix(evm): non-deterministic gas usage #2110
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
x/evm/keeper/grpc_query.go (1)
Line range hint
391-459: Consider adding gas usage validation testsThe gas estimation logic is complex and critical for consensus. To prevent future regressions and ensure deterministic gas usage:
- Consider adding specific test cases that verify gas usage remains consistent across multiple estimation calls
- Add test cases that verify gas estimation behavior when StateDB is modified during readonly queries
Would you like me to help create comprehensive test cases for validating gas estimation determinism?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/evm/keeper/grpc_query.go(1 hunks)x/evm/keeper/msg_server.go(1 hunks)
🔇 Additional comments (3)
x/evm/keeper/grpc_query.go (2)
457-457: LGTM: Error message typo fix
The typo fix in the error message from "Estimgate" to "Estimate" improves clarity and readability.
Line range hint 446-459: Verify gas estimation determinism
This error handling block is crucial for gas estimation. While the typo fix is good, we should verify that the gas estimation remains deterministic across multiple calls, especially after eth_estimateGas endpoints, as mentioned in the PR objectives.
Let's verify the gas estimation determinism:
✅ Verification successful
Let me gather more information about the gas estimation implementation to verify its determinism.
Let me search for more specific details about the gas estimation implementation.
Let me check one final aspect about the binary search implementation and state handling.
Based on the gathered information, I can now provide a final response about the gas estimation determinism.
Gas estimation is deterministic and safe ✅
The implementation ensures deterministic gas estimation through:
- Usage of
CacheContext()for state isolation during estimation - Binary search implementation that consistently converges on the minimum required gas
- No state persistence during estimation (all operations are performed in an isolated context)
- Proper error handling for common cases like
ErrExecutionRevertedandErrOutOfGas
The error handling block in question correctly handles VM errors without affecting the deterministic nature of the estimation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential state modifications during gas estimation that could affect determinism
# Look for state modifications in estimation context
ast-grep --pattern 'func $_(ctx $_, req *evm.EthCallRequest) $_ {
$$$
tmpCtx, _ := ctx.CacheContext()
$$$
}'
# Search for StateDB modifications during estimation
rg -A 5 "eth_estimateGas|EstimateGas"
Length of output: 24688
Script:
#!/bin/bash
# Search for the core gas estimation logic and state modifications
# Look for cache context usage and state modifications during estimation
# Search for the core estimation implementation
ast-grep --pattern 'func $_(ctx $_, req *evm.EthCallRequest) $_ {
$$$
tmpCtx, _ := ctx.CacheContext()
$$$
stateDB := $$$
$$$
}'
# Look for state modifications during estimation
rg -A 5 "CacheContext|CommitCache" x/evm/keeper/
# Check if there are any state persistence during estimation
rg -A 5 "Commit|Finalize" x/evm/keeper/grpc_query.go
Length of output: 956
Script:
#!/bin/bash
# Search for the binary search implementation in gas estimation
rg -A 15 "binary search" x/evm/keeper/grpc_query.go
# Look for state modifications and cache context usage in gas estimation
rg -B 5 -A 15 "EstimateGasForEvmCallType" x/evm/keeper/grpc_query.go
# Check if there are any state persistence calls in the estimation flow
ast-grep --pattern 'func $_(ctx $_, req *evm.EthCallRequest) $_ {
$$$
result := $$$
$$$
stateDB.$$$Commit()
$$$
}'
Length of output: 2395
Script:
#!/bin/bash
# Look for the binary search implementation and state handling
rg -B 10 -A 20 "BinSearch" x/evm/keeper/grpc_query.go
# Check the implementation of executable function
ast-grep --pattern 'executable := func(gas uint64) (bool, *evm.MsgEthereumTxResponse, error) {
$$$
}'
# Look for any state persistence in the execution path
rg -A 10 "ApplyMessage.*false" x/evm/keeper/
Length of output: 1248
x/evm/keeper/msg_server.go (1)
262-267: Correctly restoring StateDB to prevent side effects
The addition of saving the old StateDB reference and restoring it after execution ensures that the BankKeeper's StateDB is not persistently modified during ApplyEvmMsg. This prevents unintended side effects, such as non-deterministic gas usage in subsequent transactions caused by residual state changes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
107-107: Enhance the changelog entry for PR #2110 to better reflect the changes.The current entry could be improved to better explain the issue being fixed and provide more context. Consider updating it to:
-- [#2110](https://github.com/NibiruChain/nibiru/pull/2110) - fix(evm): Restore StateDB to its state prior to ApplyEvmMsg call to ensure deterministic gas usage. This fixes an issue where the StateDB pointer field in NibiruBankKeeper was being updated during readonly query endpoints like eth_estimateGas, leading to non-deterministic gas usage in subsequent transactions. +- [#2110](https://github.com/NibiruChain/nibiru/pull/2110) - fix(evm): Restore StateDB to its state prior to ApplyEvmMsg call to ensure deterministic gas usage. This fixes an issue introduced in #2095 where the StateDB pointer field in NibiruBankKeeper was being updated during readonly query endpoints like eth_estimateGas, leading to non-deterministic gas usage in subsequent transactions due to conditional program flow paths resulting from a nil check. The fix was verified by measuring gas usage consistency before and after eth_estimateGas calls.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2110 +/- ##
=======================================
Coverage 64.66% 64.67%
=======================================
Files 273 273
Lines 21600 21605 +5
=======================================
+ Hits 13968 13973 +5
Misses 6655 6655
Partials 977 977
|
Purpose / Abstract
eth_estimateGasendpoint to trigger a BankKeeper.StateDB pointer updatev2.0.0-rc.12), the gas_used is different. On this branch, the gas_used is identical.See https://www.notion.so/nibiru/2024-11-07-testnet-1-consensus-failure-137d2597a03c8064a399eec8f24614aa?pvs=4 for more details
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation