feat(katana): revert blockifier bump
#2979
Conversation
kariy
changed the title
blockifier bump
WalkthroughOhayo, sensei! This pull request introduces several interconnected changes across the Katana project, focusing on dependency updates, gas price handling, and transaction processing. The modifications span multiple crates, including core executor, primitives, and RPC-related components. Key updates involve switching from specific commit references to newer commit references for dependencies, refining gas price calculations to ensure non-zero values, and updating entry point type representations to streamline the conversion processes. Changes
Sequence DiagramsequenceDiagram
participant Executor
participant StateReader
participant TransactionPool
participant BlockInfo
Executor->>StateReader: Request compiled contract class
StateReader-->>Executor: Return contract class
Executor->>TransactionPool: Validate transaction
TransactionPool->>BlockInfo: Check gas prices
BlockInfo-->>TransactionPool: Provide non-zero gas prices
TransactionPool-->>Executor: Transaction validation result
The sequence diagram illustrates the updated flow of transaction execution, highlighting the changes in gas price handling and contract class retrieval across different components of the Katana system. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (1)
crates/katana/executor/src/implementation/blockifier/state.rs (1)
53-54: Consider implementing the TODO comment for cache management.The TODO comment suggests implementing class compilation caching at the executor level. This could improve performance by avoiding redundant compilations.
Would you like me to help implement a caching mechanism for compiled classes at the executor level?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/katana/cairo/Cargo.toml(1 hunks)crates/katana/core/src/backend/gas_oracle.rs(1 hunks)crates/katana/core/tests/backend.rs(1 hunks)crates/katana/executor/Cargo.toml(1 hunks)crates/katana/executor/src/implementation/blockifier/error.rs(2 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(4 hunks)crates/katana/executor/src/implementation/blockifier/state.rs(5 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(18 hunks)crates/katana/pool/src/validation/stateful.rs(2 hunks)crates/katana/primitives/src/block.rs(2 hunks)crates/katana/primitives/src/conversion/rpc.rs(2 hunks)crates/katana/rpc/rpc-types/src/class.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (26)
crates/katana/cairo/Cargo.toml (1)
21-21: Verify the compatibility of starknet_api branch with Cairo 2.9.2.Ohayo, sensei! The dependency has been updated to use a specific branch targeting blockifier v0.8.0-rc3 with Cairo 2.9.1, but the PR description mentions Cairo 2.9.2.
Let's verify the compatibility:
✅ Verification successful
Branch content is compatible with Cairo 2.9.2 despite the misleading name
Ohayo, sensei! While the branch name suggests Cairo 2.9.1, I've verified that the actual dependencies in the branch's Cargo.toml are using Cairo 2.9.2, so it's fully compatible! 🎉
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the Cairo version in the specified branch curl -s "https://raw.githubusercontent.com/dojoengine/sequencer/blockifier/v0.8.0-rc3_cairo-2.9.1/Cargo.toml" | grep -A 5 "cairo-lang"Length of output: 372
crates/katana/executor/Cargo.toml (1)
18-18: Ensure consistent branch references across dependencies.Ohayo again, sensei! The blockifier dependency is now using the same branch as starknet_api, which is good for consistency.
crates/katana/primitives/src/conversion/rpc.rs (2)
8-8: LGTM! Import changes align with the downgrade.The import changes correctly reflect the move from EntryPointV0 to EntryPoint.
37-37: Verify function signature changes are consistent.The changes to
to_rpc_entry_pointsandcollect_entry_pointssignatures align with the downgrade.Let's check for any missed references:
Also applies to: 40-40
✅ Verification successful
Ohayo sensei! Function signatures are consistent across the codebase ✨
The changes to both
to_rpc_entry_pointsandcollect_entry_pointsare properly aligned, with consistent usage ofEntryPointtype throughout the conversion flow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining EntryPointV0 references rg "EntryPointV0" -A 2Length of output: 22
Script:
#!/bin/bash # Search for the specific functions and their usage rg "to_rpc_entry_points|collect_entry_points" -A 3 # Also search for EntryPoint type usage ast-grep --pattern 'EntryPoint'Length of output: 1933
crates/katana/rpc/rpc-types/src/class.rs (1)
10-10: LGTM! Type changes are consistent with the downgrade.The changes to use EntryPoint instead of EntryPointV0 in both the imports and struct definition are consistent with the changes in other files.
Also applies to: 129-129
crates/katana/executor/src/implementation/blockifier/utils.rs (6)
2-44: Ohayo sensei, your newly introduced imports look consistent!
They cleanly manage references forNonZeroU128,BlockInfo,GasPrices,ExecutionMode, and the rest. Everything aligns well with the subsequent usage below.
66-120: Ohayo sensei, nice addition ofsimulation_flags!
Extractingvalidate,charge_fee, andnonce_checkfromsimulation_flagsis logical. Please ensure there's adequate test coverage for these scenarios so no corner case is missed.
154-177: Ohayo sensei, verify resource limiting logic.
The TODO note about settinglimit_steps_by_resourcesto false if fees are disabled is key. Confirm that fee-less modes won't trigger resource constraints inadvertently.Sensei, let me know if you'd like a quick script to locate all references to
limit_steps_by_resourcesso we can confirm there's no oversight.
Line range hint
182-373: Ohayo sensei, transaction conversion logic is well-structured!
Distinct handling forInvokeTx,DeployAccountTx,DeclareTx, andL1HandlerTxlooks consistent. Keepingonly_query: falseuniform across cases is a nice touch.
Line range hint
395-529: Ohayo sensei, good fallback for zero gas prices.
Usingunwrap_or(NonZeroU128::new(1).unwrap())protects against invalid zero values. If you foresee dynamic fallback needs, ensure it’s documented and tested.
Line range hint
536-807: Ohayo sensei, well-rounded helper functions and thorough tests!
get_fee_type_from_txneatly categorizes fee types, and the expansions into_classensure correct class info for fee calculations. The nested event test approach is commendable.crates/katana/core/tests/backend.rs (1)
39-39: Ohayo sensei, switching fromGasOracle::zerotoGasOracle::sampled_starknetis a solid move!
This ensures realistic non-zero gas prices during tests, mirroring the production environment more closely.crates/katana/executor/src/implementation/blockifier/error.rs (1)
2-2: Ohayo sensei, enhanced error reporting withformat_panic_data.
Generating more detailed info forExecutionFailedeases debugging. Ensure the underlyingerror_datanever includes sensitive data in case it's displayed publicly.Also applies to: 35-36
crates/katana/pool/src/validation/stateful.rs (1)
169-170: LGTM! Transaction type handling has been updated.The changes correctly adapt to the new transaction type variants from the blockifier downgrade.
Also applies to: 181-181
crates/katana/executor/src/implementation/blockifier/mod.rs (4)
8-14: LGTM! Import changes reflect the new gas price handling.The imports have been updated to use
NonZeroU128and simplified block-related imports.
105-111: Ohayo, sensei! Gas price handling has been improved.The changes ensure that gas prices are never zero by defaulting to 1, which aligns with the blockifier's behavior.
113-115: Consider addressing the TODO comments.These TODOs might need attention for better state management.
Would you like me to help implement functions for altering the block context?
123-126: LGTM! Gas price assignments are consistent.The gas price assignments in both the block info and block environment are now consistent.
Also applies to: 231-244
crates/katana/core/src/backend/gas_oracle.rs (1)
68-68: LGTM! Gas prices now default to 1.The change ensures consistency with the blockifier's behavior of requiring non-zero gas prices.
crates/katana/primitives/src/block.rs (2)
57-58: LGTM! Documentation clarifies gas price requirements.The TODO comment effectively explains the rationale for non-zero gas prices.
Also applies to: 60-60
78-82: LGTM! Default implementation ensures non-zero gas prices.The implementation aligns with blockifier's requirements by defaulting to 1 instead of 0.
crates/katana/executor/src/implementation/blockifier/state.rs (5)
49-62: Ohayo! The method signature change aligns with the blockifier downgrade.The change from
get_compiled_classtoget_compiled_contract_classwith the new return typeContractClassreflects the downgrade to blockifier v0.8.0-rc3. The implementation correctly handles error cases and maintains proper error propagation.
220-224: LGTM! Consistent implementation in CachedState.The
CachedStateimplementation correctly delegates to the inner state, maintaining consistency with the updated method signature.
353-369: Test cases properly validate the new contract class behavior, sensei!The test cases have been updated to use the new method name and verify both regular and legacy class handling. The assertions correctly validate the contract class conversion and compilation.
425-432: Test setup properly handles class conversion.The test setup correctly uses
utils::to_classfollowed bycontract_class()to match the new implementation's behavior.
545-545: Error case testing is properly updated.The test for non-existent data correctly verifies the error case with the new method name.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2979 +/- ##
==========================================
- Coverage 57.04% 57.01% -0.03%
==========================================
Files 425 425
Lines 56301 56185 -116
==========================================
- Hits 32115 32033 -82
+ Misses 24186 24152 -34 ☔ View full report in Codecov by Sentry. |
This is essentially branch
blockifier:blockifier/v0.8.0-rc3(what we're using prior to #2879) but with Cairo dependencies bumped to version2.9.2.The current version of
blockifierthat we're using, includes extra stuff that are not expected in the v0.13.3 version. This may results in different computed fees for some transactions when they are compared with thesnosCairo program.Reason for this downgrade is to ensure transactions and fees are being computed similarly in both
snosandkatana. But mainly insnosas it performs the computation twice - (1) computed usingblockifierwhere the outputs are used as the 'predicted' values for the Cairo program run, and (2) the Cairo program also execute the transactions again and will