Skip to content

fix(executor): map L2 gas correctly#160

Merged
kariy merged 1 commit intomainfrom
fix/l2-gas
Jul 7, 2025
Merged

fix(executor): map L2 gas correctly#160
kariy merged 1 commit intomainfrom
fix/l2-gas

Conversation

@kariy
Copy link
Member

@kariy kariy commented Jul 7, 2025

Currently, we're not passing the L2 gas prices from the primitive Header to blockifier's BlockContext because we initially didn't have support for L2 gas prices - not until #19. The L2 gas prices parameter is introduced in Starknet RPC spec 0.8 - which only recently we have support for. And with the recent addition of Starknet gas price oracle in #152, we can finally sample the L2 gas prices correctly instead of implicitly using the L1 gas prices.

@claude
Copy link
Contributor

claude bot commented Jul 7, 2025

Claude finished @kariy's task —— View job


PR Review Complete ✅

Todo List:

  • Read and analyze the changed file
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Provide constructive feedback
  • Include Marcus Aurelius quote

Code Review Feedback

This PR addresses a critical bug in L2 gas price mapping within the Starknet VM processor. The fix is well-implemented and necessary for correct gas pricing.

Code Quality and Best Practices

  • Clean implementation: The code follows consistent patterns with proper variable naming (eth_l2_gas_price, strk_l2_gas_price)
  • Proper error handling: Maintains the existing unwrap_or(NonzeroGasPrice::MIN) pattern for safe gas price handling
  • Code organization: Variables are logically grouped and well-ordered
  • Comment cleanup: Appropriately removes TODO comments that are now resolved

Bug Fix Analysis

The original code had a significant bug where L2 gas prices were incorrectly mapped to L1 gas prices:

// Before (incorrect):
l2_gas_price: eth_l1_gas_price,  // Wrong value used!

// After (correct):
l2_gas_price: eth_l2_gas_price,  // Proper L2 gas price

This fix ensures that:

  • L2 operations are priced correctly using actual L2 gas prices
  • Gas calculations align with Starknet v0.13.4+ specifications
  • The conversion from Cairo steps to L2 gas (line 129-130) works with accurate price data

Performance Considerations

  • Minimal overhead: The changes don't introduce additional computational complexity
  • Efficient data access: Direct field access from header maintains performance
  • No allocations: The fix reuses existing patterns without extra memory allocations

Security Concerns

  • Safe gas price handling: Maintains the defensive NonzeroGasPrice::MIN fallback
  • No new attack vectors: The fix doesn't introduce security vulnerabilities
  • Maintains bounds: Gas price extraction preserves existing safety checks

⚠️ Test Coverage Recommendations

While this is a solid fix, consider adding tests for:

  1. Gas price mapping accuracy: Verify L2 gas prices are correctly extracted from headers
  2. Edge cases: Test behavior when header gas prices are zero or invalid
  3. Integration tests: Ensure transaction execution uses correct L2 gas prices

📍 Code References

  • Gas price extraction: crates/executor/src/implementation/blockifier/mod.rs:148-151
  • L2 gas assignment: crates/executor/src/implementation/blockifier/mod.rs:176,181
  • Related L2 gas context: crates/executor/src/implementation/blockifier/mod.rs:120-130

Overall Assessment

This is a high-quality bug fix that resolves an important issue in L2 gas price handling. The implementation is correct, follows existing patterns, and maintains code quality standards. The fix is critical for proper Starknet L2 operation pricing and aligns with the v0.13.4+ gas model.

Recommendation: ✅ Approve - This fix should be merged promptly as it addresses a functional correctness issue.


"Remember that very little disturbs him who has formed the habit of confining himself to the present moment."
                                                        - Marcus Aurelius, Meditations

@github-actions
Copy link

github-actions bot commented Jul 7, 2025

Binary size report 📊

Branch Size
main 64.08MiB
fix/l2-gas 64.08MiB ( +0.01% )

@codecov
Copy link

codecov bot commented Jul 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.29%. Comparing base (9bde0ae) to head (986e608).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
- Coverage   73.32%   72.29%   -1.03%     
==========================================
  Files         209      219      +10     
  Lines       23132    24120     +988     
==========================================
+ Hits        16961    17438     +477     
- Misses       6171     6682     +511     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kariy kariy merged commit c74cd72 into main Jul 7, 2025
12 of 13 checks passed
@kariy kariy deleted the fix/l2-gas branch July 7, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant