feat(katana): global compiled class cache #3004
Conversation
WalkthroughOhayo, sensei! This pull request refactors various aspects of Katana’s state management and contract class handling. It removes the dependency on katana-trie in the executor package and updates state initialization and caching in the blockifier module. Several methods related to state proofs, state roots, and compiled class retrieval have been removed or migrated to new traits. Additionally, changes in import paths, error handling, and test enhancements further streamline the codebase for improved modularity and clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as StarknetVMProcessor
participant CS as CachedState
participant SP as StateProviderDb
participant Cache as COMPILED_CLASS_CACHE
VM->>CS: Call simulate_with(...)
CS->>SP: Initialize new StateProviderDb (with state & defaults)
SP-->>CS: Return processed state
CS->>Cache: Retrieve compiled class if needed
Cache-->>CS: Return contract class info
CS-->>VM: Return updated state
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (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
🔭 Outside diff range comments (1)
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
584-611: 🛠️ Refactor suggestionClass conversion logic needs more robust error handling.
The TODO comment indicates uncertainty about error handling in class conversions. Consider implementing a custom error type for better error handling and documentation.
+#[derive(Debug, thiserror::Error)] +pub enum ClassConversionError { + #[error("Invalid legacy class: {0}")] + InvalidLegacy(#[from] ProgramError), + #[error("Invalid sierra class: {0}")] + InvalidSierra(#[from] ProgramError), } -pub fn to_class_info(class: class::CompiledClass) -> Result<ClassInfo, ProgramError> { +pub fn to_class_info(class: class::CompiledClass) -> Result<ClassInfo, ClassConversionError> {
🧹 Nitpick comments (5)
crates/katana/executor/src/abstraction/mod.rs (1)
10-10: Consider avoiding wildcard imports.
pub use crate::error::*;might inadvertently bring unwanted items into scope. If you prefer more explicit imports, consider selectively importing only the needed items.crates/katana/executor/src/implementation/blockifier/state.rs (2)
33-45: Monitor potential lock contention.
Using a single Arc<Mutex<...>> for both cached state and declared classes is convenient, but multiple threads might compete for the same lock. Consider an RwLock or finer-grained locks if scalability becomes a concern.
123-124: Empty trait impl placeholders.
StateProofProviderandStateRootProviderare currently empty. If they’re intended for future functionality, consider adding a TODO or doc comment for clarity.crates/katana/pool/src/validation/stateful.rs (1)
91-92: Consider adding error handling for state initialization.The state initialization could fail if there are issues with the cache. Consider wrapping this in a Result to handle potential errors gracefully.
- let cached_state = - CachedState::new(StateProviderDb::new(state, COMPILED_CLASS_CACHE.clone())); + let cached_state = CachedState::new(StateProviderDb::new(state, COMPILED_CLASS_CACHE.clone())) + .map_err(|e| Error::new("Failed to initialize cached state", e))?;crates/katana/executor/src/implementation/blockifier/mod.rs (1)
170-170: Watch out for potential deadlock scenarios.Multiple places in the code acquire locks on the state. Consider documenting the lock ordering to prevent potential deadlocks, especially in the simulate_with and execute_transactions methods.
Consider adding a comment documenting the lock acquisition order:
// Lock ordering: // 1. state.inner // 2. cached_state // Always acquire locks in this order to prevent deadlocksAlso applies to: 199-199, 214-214, 329-330
📜 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 (19)
crates/katana/executor/Cargo.toml(0 hunks)crates/katana/executor/benches/execution.rs(2 hunks)crates/katana/executor/src/abstraction/mod.rs(1 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(7 hunks)crates/katana/executor/src/implementation/blockifier/state.rs(3 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(6 hunks)crates/katana/executor/src/implementation/noop.rs(2 hunks)crates/katana/executor/src/lib.rs(1 hunks)crates/katana/executor/tests/executor.rs(1 hunks)crates/katana/node/Cargo.toml(2 hunks)crates/katana/pool/src/validation/stateful.rs(2 hunks)crates/katana/storage/provider/src/error.rs(2 hunks)crates/katana/storage/provider/src/providers/db/state.rs(0 hunks)crates/katana/storage/provider/src/providers/fork/backend.rs(1 hunks)crates/katana/storage/provider/src/providers/fork/state.rs(4 hunks)crates/katana/storage/provider/src/providers/in_memory/state.rs(1 hunks)crates/katana/storage/provider/src/traits/contract.rs(3 hunks)crates/katana/storage/provider/src/traits/state.rs(3 hunks)crates/katana/storage/provider/tests/class.rs(1 hunks)
💤 Files with no reviewable changes (2)
- crates/katana/executor/Cargo.toml
- crates/katana/storage/provider/src/providers/db/state.rs
✅ Files skipped from review due to trivial changes (2)
- crates/katana/storage/provider/tests/class.rs
- crates/katana/executor/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: build
- GitHub Check: clippy
- GitHub Check: ensure-wasm
🔇 Additional comments (29)
crates/katana/storage/provider/src/traits/contract.rs (2)
7-7: Ohayo! Nice simplification of the trait bounds, sensei!Removing
Send + Syncbounds fromContractClassProvidermakes the trait more flexible and easier to implement for various types.
37-48: Excellent trait design with extension pattern!The new
ContractClassProviderExttrait elegantly separates the compiled class functionality into an extension trait with a default implementation. This approach:
- Maintains backward compatibility
- Allows for custom implementations when needed
- Provides sensible default behavior
crates/katana/storage/provider/src/traits/state.rs (2)
26-28: Smart default implementations for state root methods!The default implementations returning appropriate errors make it clear when functionality is not supported while reducing boilerplate in implementations.
Also applies to: 31-33, 36-39
98-116: Well-structured error handling for state proofs!The default implementations for state proof methods follow a consistent pattern and provide clear error messages. This is a good example of the fail-fast principle.
crates/katana/executor/src/implementation/noop.rs (1)
168-169: Clean and concise noop implementations!Taking advantage of the default implementations from the traits makes the noop provider much cleaner and easier to maintain.
crates/katana/storage/provider/src/error.rs (2)
99-103: Well-defined error variants for state operations!The new error variants
StateProofNotSupportedandStateRootNotFoundprovide clear and specific error messages for these operations.
119-119: Arigato for fixing the typo, sensei!Fixed typo in error message from "soemthing" to "Something".
crates/katana/storage/provider/src/providers/in_memory/state.rs (1)
141-154: Ohayo! The changes look good, sensei!The removal of the
compiled_classmethod from theContractClassProviderimplementation aligns with the PR's objective of implementing a global compiled class cache. The remaining functionality for handling class hashes and contract classes is preserved.crates/katana/storage/provider/src/providers/fork/state.rs (1)
98-99: Ohayo! The changes look good, sensei!The empty trait implementations for
StateProofProviderandStateRootProviderclearly indicate that these features are not supported in forked mode. This is a clean way to handle unsupported functionality.Also applies to: 138-139
crates/katana/executor/tests/executor.rs (1)
15-15: Ohayo! The changes look good, sensei!The addition of the
ContractClassProviderExtimport shows that the test has been properly updated to use the new trait for accessing compiled class functionality.crates/katana/storage/provider/src/providers/fork/backend.rs (1)
576-631: Ohayo! The changes look good, sensei!The
ContractClassProviderimplementation has been cleanly updated to remove thecompiled_classmethod while retaining the essentialcompiled_class_hash_of_class_hashfunctionality. The extensive test coverage ensures that request deduplication continues to work correctly.crates/katana/node/Cargo.toml (2)
16-16: Ohayo! The katana-provider addition looks great, sensei!The addition of katana-provider as a workspace dependency aligns well with the global compiled class cache objectives.
55-55: Verify the full-node feature functionality, sensei!The removal of katana-provider from the full-node feature suggests it's now required in the base configuration. Please ensure this change doesn't affect the experimental full-node mode's behavior.
Run this script to check for any remaining references to katana-provider in full-node related code:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for katana-provider usage in full-node related files rg -l "katana.?provider" "src/full/"Length of output: 86
Ohayo, sensei! It looks like the initial script didn't yield any results because the "src/full/" directory isn’t present. I went ahead and conducted a global search for any occurrences of "katana-provider" to verify its removal from the experimental full‑node configuration.
Please run this script to search the entire repository:
Once you verify that there are no stray references and that the removal is intentional, please also run your integration tests or manually exercise the experimental full-node mode to ensure its runtime behavior remains correct.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for any references to katana-provider in the repository rg -l "katana-provider" .Length of output: 591
Ohayo, sensei! The global search shows that
katana-provideris still referenced broadly, but the specific removal from thefull-nodefeature incrates/katana/node/Cargo.tomlseems intentional. However, since the search in thesrc/full/directory didn’t yield results (likely because that directory doesn't exist), we should verify the feature configuration in theCargo.tomlfiles.Let's dive deeper into the
crates/katana/node/Cargo.tomlfile to inspect the full-node feature definition and see if it’s still logically consistent despite removing thekatana-provider. Could you please execute the following script to output the contents of the feature configuration in that file?Once we confirm that the dependency on
katana-providerhas been intentionally removed and the related modules are correctly factored as base dependencies, we can be sure the experimental full-node mode remains intact.
🏁 Script executed:
[run_scripts] #!/bin/bash # Output the feature definitions from the crates/katana/node/Cargo.toml file rg -A 10 "\[features\]" crates/katana/node/Cargo.tomlLength of output: 456
Ohayo, sensei! The feature configuration in
crates/katana/node/Cargo.tomlclearly shows that the experimentalfull-nodefeature now only depends on"dep:katana-feeder-gateway"and"dep:tokio", with no explicit reference tokatana-provider. This confirms that the removal was intentional, and that thekatana-providershould now be part of the base configuration for both full‑node and non‑full node modes.I recommend running your integration tests or manual runtime verification on the full‑node mode to ensure everything behaves as expected.
crates/katana/executor/src/abstraction/mod.rs (2)
8-8: Ohayo sensei! No concerns with the new import.
This streamlined import forContractAddressandFeltlooks consistent with the rest of the codebase.
12-12: Explicit reference toExecutorErroris clean.
The updated type alias referencingcrate::error::ExecutorErroris clearer and ensures the error type is easy to trace.crates/katana/executor/src/implementation/blockifier/state.rs (9)
2-2: Ohayo sensei! Good job refining the imports.
These newly added imports (e.g.,Deref,BlockifierContractClass, etc.) neatly align with the updated trait implementations below.Also applies to: 5-5, 11-11, 14-14, 18-18
22-25: Lifetime'ausage looks fine.
DefiningCachedState<'a>with an internalArc<Mutex<...>>is valid for the lifetime you introduce. Just ensure external references don't outlive'a.
28-31: Straightforward internal struct definition.
CachedStateInner<'a>properly bundles the cached state and declared classes. No immediate issues spotted.
48-72: Implementation forContractClassProvideris solid.
Lock usage, valid checks, and fallback to the underlyingcached_stateare consistent. Error handling also seems straightforward.
75-121: ConsistentStateProvidermethods.
All logic for class hashes, nonces, and storage checks is clearly handled. The early return for non-deployed contracts is a good safeguard.
126-130: StateProviderDb struct is a clean wrapper.
Wrapping the provider and cache logic is a neat approach. The lifetime annotation'alines up with your design.
132-139:Derefimplementation is straightforward.
No hidden complexities. This approach makes yourBox<dyn StateProvider>usage more ergonomic.
140-147: Constructor forStateProviderDbis fine.
AcceptingBox<dyn StateProvider + 'a>and a compiled class cache feels flexible and consistent.
149-222:StateReaderimplementation handles caching nicely.
Good use oftrace!to log cache hits/misses. The fallback to your provider on cache misses is logical. Overall, the pattern is well-structured.crates/katana/executor/benches/execution.rs (2)
6-7: Ohayo sensei! Updated imports match the new module structure.
Switching tokatana_executor::implementation::blockifier::state::StateProviderDbis consistent with the recent refactor. No issues spotted.
49-49: InstantiatingStateProviderDbwith a default cache is clear.
CachedState::new(StateProviderDb::new(state, Default::default()))nicely leverages the default for your compiled class cache. This ensures a clean setup for benchmarks.crates/katana/pool/src/validation/stateful.rs (1)
12-12: Ohayo sensei! New imports look good but let's verify the order.The imports are logically grouped and follow Rust's convention of separating external and internal imports.
Also applies to: 16-17
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
36-37: Global cache implementation looks solid!The use of LazyLock with Arc provides thread-safe initialization and access to the cache. Good choice for a global compiled class cache.
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
613-623: Clean implementation of class conversion!The to_class function provides a clear separation between legacy and new class formats. Good pattern matching and error propagation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3004 +/- ##
==========================================
- Coverage 56.99% 56.41% -0.58%
==========================================
Files 431 431
Lines 57151 57162 +11
==========================================
- Hits 32572 32247 -325
- Misses 24579 24915 +336 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Refactor/Chores
NoopStateProviderby removing unused methods and dependencies.Bug Fixes
Tests