fix(evm): make conditional balance check free in gas cost #2114
Conversation
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2114 +/- ##
=======================================
Coverage 64.67% 64.68%
=======================================
Files 273 273
Lines 21605 21610 +5
=======================================
+ Hits 13973 13978 +5
Misses 6655 6655
Partials 977 977
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
109-109: Consider improving the changelog entry format and detail.While the entry correctly captures the change, consider:
- Indenting the line to match other entries in the section
- Adding more context about the impact, e.g.:
-- [#2114](https://github.com/NibiruChain/nibiru/pull/2114) - fix(evm): make gas cost zero in conditional bank keeper flow + - [#2114](https://github.com/NibiruChain/nibiru/pull/2114) - fix(evm): make gas cost zero in conditional bank keeper flow to improve consensus reliability by eliminating gas calculation discrepancies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/evm/keeper/bank_extension.go(2 hunks)
🔇 Additional comments (5)
x/evm/keeper/bank_extension.go (4)
4-4: LGTM: Import addition aligns with PR objectives
The addition of storetypes import is necessary for implementing zero-cost gas configuration.
89-92: LGTM: Proper gas config restoration pattern
The implementation correctly caches and restores the original gas configuration using defer, ensuring cleanup even in error cases.
94-95: Verify thread safety of context modifications
While making the balance check free in gas cost addresses the consensus failure, we should verify that modifying the context's gas config doesn't introduce race conditions during parallel execution of ApplyEvmMsg.
#!/bin/bash
# Search for other places where KVGasConfig is modified to ensure no conflicts
rg -A 3 "WithKVGasConfig|KVGasConfig\(\)" --type go
# Look for parallel execution patterns
ast-grep --pattern 'go func() { $$$ ApplyEvmMsg $$$ }'88-95: Architecture Impact: Verify gas accounting across transaction types
The zero-cost balance check affects all coin management operations that involve EVM balances. While this change resolves the immediate consensus issue, we should verify that:
- Gas accounting remains accurate for complex transactions involving multiple coin operations
- The total gas used still properly reflects the computational cost of transactions
✅ Verification successful
Based on the search results, I can now provide a final response:
Gas accounting is correctly handled for EVM balance operations
The zero-cost balance check in SyncStateDBWithAccount is safe because:
- It's only triggered for EVM denom balance changes, controlled by
findEtherBalanceChangeFromCoins - The gas cost elimination is properly scoped within a deferred context restoration
- The overall transaction gas accounting is maintained through
ApplyEvmMsgwhich:- Tracks intrinsic gas costs
- Handles gas refunds according to EIP-3529
- Properly syncs gas usage with the block gas meter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas accounting patterns in transaction handling
rg -A 5 "GasMeter|ConsumeGas|gasUsed" --type go
# Look for transaction types that might be affected
ast-grep --pattern 'type $_Tx struct { $$$ }'
Length of output: 46838
Script:
#!/bin/bash
# Search for gas accounting in EVM transactions and state sync
rg -A 5 "SyncStateDBWithAccount|ApplyEvmMsg|ApplyMessage" --type go
# Look for gas consumption in bank operations
rg -A 5 "MintCoins|BurnCoins|SendCoins.*SyncStateDBWithAccount" --type go
Length of output: 24427
CHANGELOG.md (1)
109-109: LGTM! Changelog entry accurately reflects the PR purpose.
The changelog entry "fix(evm): make gas cost zero in conditional bank keeper flow" properly documents the change to make conditional balance check free in gas cost, which aligns with the PR objectives to address consensus failures related to gas calculations.
Purpose / Abstract
We recently experienced another consensus failure on devnet-3. It happened again with the gas_used value in the last_results block. It's likely due to parallel calls to
ApplyEvmMsgthrashing the StateDB pointer on the NibiruBankKeeper.In order to ensure that all nodes calculate the same
gas_usedvalue, we make the conditional path cost zero gas, which shouldn't be a major issue because there's only a tiny db read in that path.Summary by CodeRabbit
New Features
Bug Fixes
Documentation