fix(katana): unbound call execution from the block context limit #3026
Conversation
|
Ohayo, sensei! Here's the updated breakdown of the changes: WalkthroughThis pull request updates the workspace configuration by adding a new member and introduces a new package for miscellaneous contracts. It adds a new Cairo module with a StarkNet contract, updates the executor to support cloning of call entities, and enhances contract call execution with dynamic gas management. The changes also remove a deprecated function from the utils module and update test fixtures for broader limits. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant E as Executor
participant CS as CachedState
participant BC as BlockContext
participant CEP as CallEntryPoint
C->>E: execute_call(request, state, block_context, max_gas)
E->>CS: Initialize CachedState from state
E->>BC: Pass block context and gas info
E->>CEP: Prepare CallEntryPoint with calldata and selector
CEP-->>E: Return execution info (CallInfo)
E-->>C: Return result data from execute_call
Possibly related PRs
Hope that helps, sensei! 🪧 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
Documentation and Community
|
![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
🧹 Nitpick comments (4)
crates/katana/executor/src/implementation/blockifier/call.rs (2)
31-69: Ohayo sensei, watch out for potential overflow issues when casting tousize.On certain 32-bit architectures, converting
max_gasfromu64tousizemight lead to overflow. Consider adding a safety check or capping the gas tousize::MAXbefore creating theRunResources.fn execute_call_inner<S: StateReader>( request: EntryPointCall, state: &mut CachedState<S>, block_context: &BlockContext, max_gas: u64, ) -> EntryPointExecutionResult<CallInfo> { ... - ctx.vm_run_resources = RunResources::new(max_gas as usize); + let run_gas = if max_gas as usize as u64 == max_gas { + max_gas as usize + } else { + usize::MAX + }; + ctx.vm_run_resources = RunResources::new(run_gas); ... }
71-162: Ohayo sensei, consider parameterizing your test cases.Your
max_stepstest is thorough, but you could reduce repetition by adapting a table-driven approach to test different iteration values, improving readability.crates/katana/executor/src/abstraction/mod.rs (1)
106-106: Ohayo sensei, derivingClonemay be handy but ensure it's necessary.If
EntryPointCallgrows large in future, cloning might be expensive. Consider referencing slices in downstream calls when possible.crates/katana/executor/src/implementation/blockifier/mod.rs (1)
332-332: Consider making the gas limit configurable.The hard-coded gas limit of 1_000_000_000 should ideally be configurable through the environment or configuration settings.
Consider introducing a configuration parameter for this value:
- let retdata = call::execute_call(call, state, block_context, 1_000_000_000)?; + let retdata = call::execute_call(call, state, block_context, self.cfg.max_call_gas)?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/katana/contracts/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/katana/contracts/Scarb.toml(1 hunks)crates/katana/contracts/misc/Scarb.toml(1 hunks)crates/katana/contracts/misc/src/lib.cairo(1 hunks)crates/katana/executor/src/abstraction/mod.rs(1 hunks)crates/katana/executor/src/implementation/blockifier/call.rs(1 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(2 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(3 hunks)crates/katana/executor/tests/fixtures/call_test.json(1 hunks)crates/katana/executor/tests/fixtures/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/contracts/misc/Scarb.toml
🔇 Additional comments (10)
crates/katana/executor/src/implementation/blockifier/call.rs (1)
19-29: Ohayo sensei, nice approach to returning the contract call results!The API is concise and straightforward. The direct usage of the returned retdata vector is helpful for immediate consumption in higher-level code.
crates/katana/contracts/misc/src/lib.cairo (1)
1-17: Ohayo sensei, your bounded loop logic is straightforward!This simple counter increment loop cleanly demonstrates controllable step consumption for testing. No issues found.
crates/katana/executor/tests/fixtures/mod.rs (1)
241-243: Ohayo! Consider the implications of using MAX values in test configuration.Setting these limits to their maximum values could potentially mask issues that would occur with more realistic limits in production environments.
Could you confirm if these changes are intentional for testing purposes only? We should ensure that production configurations maintain appropriate bounds.
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
5-5: LGTM! New module organization looks clean, sensei.Good separation of concerns by moving call-related functionality to its own module.
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
7-7: Clean import organization, sensei!The import cleanup and reorganization improves code maintainability.
Also applies to: 14-14, 19-19
crates/katana/contracts/Scarb.toml (1)
2-2: LGTM! Workspace member addition looks good.The addition of the "misc" member follows the standard workspace structure.
crates/katana/executor/tests/fixtures/call_test.json (4)
358-364: Ohayo! The contract interface looks clean and focused, sensei!The
bounded_callfunction has a clear purpose with appropriate parameter type and view mutability.
19-22: Excellent error handling implementation, sensei!The program includes comprehensive error messages for critical scenarios:
- Arithmetic overflow protection
- Deserialization safety
- Gas exhaustion handling
247-249: Robust gas management implementation, sensei!The program includes comprehensive gas operations:
- Granular gas withdrawal
- Full gas withdrawal
- Built-in cost tracking
Also applies to: 299-299, 301-301
347-356: Clean entry points configuration, sensei!The configuration is minimal yet complete with a properly defined external function selector.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3026 +/- ##
==========================================
+ Coverage 56.23% 56.34% +0.10%
==========================================
Files 436 437 +1
Lines 58829 58891 +62
==========================================
+ Hits 33083 33181 +98
+ Misses 25746 25710 -36 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/executor/src/implementation/blockifier/call.rs (2)
69-69: Ohayo sensei, consider protective casting!
Usingmax_gas as usizecould be problematic ifmax_gasexceeds the platform’susize::MAX. A more defensive approach—such asusize::try_from(max_gas)—may prevent overflow.- ctx.vm_run_resources = RunResources::new(max_gas as usize); + let bounded_gas = usize::try_from(max_gas) + .unwrap_or_else(|_| usize::max_value()); + ctx.vm_run_resources = RunResources::new(bounded_gas);
73-166: Ohayo sensei, comprehensive test coverage—consider boundary tests!
Themax_stepstest suite thoroughly checks various gas limits. As an extra safeguard, you might include edge cases like extremely small “0 gas” or extremely large gas settings if relevant for your application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/executor/src/implementation/blockifier/call.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
crates/katana/executor/src/implementation/blockifier/call.rs (2)
19-29: Ohayo sensei, this public interface looks well-structured!
Theexecute_callfunction neatly delegates implementation details toexecute_call_inner, keeping the interface clean.
31-43: Ohayo sensei, cautiously verify default fields!
When using..Default::default()inCallEntryPoint, ensure all defaulted fields (likeclass_hash,entry_point_type, etc.) behave correctly. If any defaulted field must be explicitly set, missing it may cause unexpected behavior downstream.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/executor/src/implementation/blockifier/call.rs (2)
31-71: Consider adding documentation for the inner function, sensei.While the implementation is solid, adding documentation for
execute_call_innerwould help explain:
- The relationship between
max_gasand block context limits- The manual override of run resources
- The significance of
limit_steps_by_resourcesThe implementation correctly handles gas limits and execution context. The comments explaining the block context relationship are particularly helpful.
90-164: Excellent test coverage, sensei! Consider a few enhancements.The test implementation thoroughly verifies gas consumption and limits. A few suggestions to make it even better:
- Consider extracting magic numbers into named constants for better readability:
- let max_gas_1 = 1_000_000; + const LOW_GAS_LIMIT: u64 = 1_000_000; + let max_gas_1 = LOW_GAS_LIMIT;
- Add test cases for edge cases:
- Zero gas limit
- Maximum possible gas limit
- Gas limit exactly at block context limit
The test implementation effectively validates:
- Gas consumption tracking
- Resource exhaustion handling
- Block context independence
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/executor/src/implementation/blockifier/call.rs(1 hunks)
🔇 Additional comments (1)
crates/katana/executor/src/implementation/blockifier/call.rs (1)
19-29: Ohayo! The public interface looks clean, sensei!The
execute_callfunction provides a clean interface for contract calls with proper state management and error handling.
Summary by CodeRabbit
New Features
Refactor & Cleanup
Tests