feat: implement floor gas + calldata gas and improve gas calculation#825
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Code Review: Correct and Comprehensive Gas Costs ImplementationNote: This action was performed by Claude AI assistant, not @roninjin10 or @fucory ✅ Overall AssessmentThis PR correctly implements gas cost calculations for calldata and floor gas (EIP-7623), addressing critical gaps in the EVM implementation. The changes align well with Ethereum specifications and follow the codebase conventions in CLAUDE.md. 📊 Code Quality & Best PracticesStrengths:
Minor improvements:
🐛 Potential Issues
⚡ Performance ConsiderationsPositive:
Optimization opportunity:
🔐 Security AnalysisCritical fixes:
Security considerations verified:
✅ Test CoverageExcellent test coverage including:
Test quality:
📝 Additional Notes
🎯 RecommendationAPPROVE - This is a well-implemented, thoroughly tested fix for critical gas calculation issues. The code is production-ready pending successful CI/CD validation. 🔧 Pre-merge checklist:
Great work on this comprehensive fix! The attention to detail in handling hardfork transitions and edge cases is commendable. |
07b77de to
7bc9d17
Compare
Code ReviewThis PR implements EIP-7623 (Floor Gas) and improves the gas calculation logic. Here's my detailed review: ✅ Strengths
🔍 Observations & Suggestions
|
bab9c6c to
692f787
Compare
7bc9d17 to
7d35df5
Compare
roninjin10
left a comment
There was a problem hiding this comment.
Is the places where it looks like a bug to me a bug? What test is this pr making pass or should this be adding a test?
| if (!comptime config.disable_gas_checks) { | ||
| result.gas_left = gas_limit - floor_gas_cost; | ||
| } |
There was a problem hiding this comment.
this doesn't look right to me
There was a problem hiding this comment.
what is the issue here? purpose is if we didn't consume at least floor_gas we deduct it from the original gas
| // EIP-7623 (Prague): Ensure at least floor_gas_cost is consumed | ||
| if (gas_consumed < floor_gas_cost) { |
There was a problem hiding this comment.
nit using std.math.max would be more readable
| if (gas_limit < initial_gas_cost) return CallResult.failure(self.getCallArenaAllocator(), 0) catch unreachable; | ||
| if (gas_limit < floor_gas_cost) return CallResult.failure(self.getCallArenaAllocator(), 0) catch unreachable; |
There was a problem hiding this comment.
This doesn't look right to me we still need to deal with not underflowing. Might be a good idea to remove disable gas cost as an option right now seems to be randomly implemented without much thought and too much vibes everywhere
This makes some execution specs correctly deduct balance, I don't remember which ones but some cancun state tests |
1dc65d4
into
09-24-spec-runner-small-fixes-and-better-debugging
…825) ## Description Implements EIP-7623 (Floor Gas) and refactors gas calculation logic for transactions. This PR adds support for calculating the minimum gas a transaction must consume based on calldata tokens, which is required for the Prague hardfork. The implementation includes: 1. Adding EIP-7623 to the Prague hardfork EIP list 2. Implementing token-based calldata gas calculation 3. Adding floor gas cost calculation for Prague 4. Refactoring intrinsic gas calculation to be more modular 5. Ensuring transactions consume at least the floor gas amount The PR also adds comprehensive tests for all the new gas calculation functions, including edge cases and regression tests. ## Type of Change - [x] 🎉 New feature (non-breaking change which adds functionality) - [ ] ✅ Test additions or updates ## Testing - [x] `zig build test` passes - [x] `zig build` completes successfully - [x] All existing tests pass - [x] Added new tests for changes (if applicable) ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my own code - [x] I have commented my code where necessary - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes

Description
Implements EIP-7623 (Floor Gas) and refactors gas calculation logic for transactions. This PR adds support for calculating the minimum gas a transaction must consume based on calldata tokens, which is required for the Prague hardfork. The implementation includes:
The PR also adds comprehensive tests for all the new gas calculation functions, including edge cases and regression tests.
Type of Change
Testing
zig build testpasseszig buildcompletes successfullyChecklist