feat(katana): return contract storage root in RPC proof response - #2846
Conversation
Katana Trie and State Management EnhancementWalkthroughOhayo, sensei! This pull request introduces a comprehensive refactoring of Katana's state management and trie infrastructure. The changes consolidate various state-related traits, introduce new multiproof and root retrieval mechanisms, and enhance the overall flexibility of the blockchain's state handling. Key modifications include unifying trie writers, adding state proof providers, and implementing more robust methods for managing contract, class, and storage tries. Changes
Sequence DiagramsequenceDiagram
participant StateProvider
participant TrieWriter
participant ClassesTrie
participant ContractsTrie
participant StoragesTrie
StateProvider->>TrieWriter: trie_insert_declared_classes
TrieWriter->>ClassesTrie: insert
ClassesTrie-->>TrieWriter: commit
StateProvider->>TrieWriter: trie_insert_contract_updates
TrieWriter->>ContractsTrie: insert
ContractsTrie-->>TrieWriter: commit
TrieWriter->>StoragesTrie: insert
StoragesTrie-->>TrieWriter: commit
Possibly Related PRs
Sensei, the changes represent a significant evolution in our state management approach, providing more granular and flexible handling of blockchain state tries! 🚀🔧 🪧 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 10
🧹 Nitpick comments (50)
crates/katana/rpc/rpc-types-builder/src/state_update.rs (1)
38-38: Ohayo sensei! Carefully Check the Expect Message.Although the line is blank, note that any preceding
.expect("should exist if block exists")message is helpful. Please ensure this message remains relevant for debugging.crates/katana/storage/provider/tests/block.rs (2)
32-32: Ohayo sensei! Having this test ignored is understandable given the unsupported trie computation.It might be good to briefly mention or reference in the code when you expect to remove the
#[ignore]so future devs know if there's any plan to enable or fix it.
49-49: Ohayo sensei! Same note regarding the ignored test here.Consider adding comments on progress or a roadmap to remove this ignore, ensuring clarity for new contributors.
crates/katana/storage/db/src/trie/snapshot.rs (5)
15-23: Ohayo sensei! Consider including a doc comment for this struct.
Providing a short high-level description will help others quickly understand the purpose ofSnapshotTrieDb.
25-33: Ohayo sensei! Include the snapshot ID in debug output.
Right now, we only formattx. Addingsnapshot_idmight help pinpoint which snapshot is being debugged.fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("SnapshotTrieDb") - .field("tx", &self.tx) + .field("tx", &self.tx) + .field("snapshot_id", &self.snapshot_id) .finish() }
67-67: Ohayo sensei! Add a doc comment forcreate_batch.
Even if it's empty, a comment clarifying why we return an empty batch will aid understanding.
92-97: Ohayo sensei! “todo!” comment is fine, but be mindful of usage.
A specialized error or a more descriptive placeholder may better communicate the feature’s status.
116-118: Ohayo sensei! “todo!” approach forcontains.
Consider clarifying timeline or relevant references to complete this method, for better context.crates/katana/trie/src/classes.rs (2)
37-51: Ohayo sensei! Validate input sets
When generating a multiproof for potentially largeclass_hashes, consider validating or logging the size to avoid performance issues or out-of-memory scenarios.
53-65: Ohayo sensei! Graceful error handling might help
insertandcommitunwrap potential errors, which can panic. A gentleResultapproach with descriptive error messages could make debugging easier.- self.trie.insert(Self::BONSAI_IDENTIFIER, hash, value) + if let Err(err) = self.trie.insert(Self::BONSAI_IDENTIFIER, hash, value) { + // handle or propagate error here + }crates/katana/storage/provider/src/traits/state.rs (1)
24-31: Ohayo sensei! Great addition of dedicated trie root fetch methods
classes_root,contracts_root, andstorage_rootclarify the retrieval of each domain's root. Keep an eye on potential performance overhead if these calls are frequently repeated.crates/katana/storage/provider/src/providers/db/trie.rs (2)
26-41: Ohayo sensei! Partitioning class insertion logic is cleaner
trie_insert_declared_classesnicely separates class-hash updates from contract updates. Ensure that any partial insertion failures roll back or handle errors gracefully.
49-108: Ohayo sensei! Consider concurrency aspects
storagesandcontractstries are updated in a loop. If parallel updates or multi-threading is introduced, a synchronization approach might be needed. Currently, single-thread usage is fine.crates/katana/trie/src/lib.rs (1)
Line range hint
96-108: Ohayo sensei!anyhow::Resultforcompute_merkle_root
Usinganyhow::Resultis flexible for error chaining but rest assured the overhead is acceptable. If performance is critical, a specialized error type might be more efficient.crates/katana/executor/src/abstraction/mod.rs (1)
227-243: Ohayo sensei!StateRootProviderintegration
By implementingclasses_root,contracts_root,storage_root, andstate_root,StateProviderDbunifies all root retrieval logic. Beware that repeated root calls might be expensive if they perform complex I/O.crates/katana/storage/db/src/abstraction/transaction.rs (2)
94-129: Ohayo sensei, consider unifying trait definitions
We see thatDbTxMutRefduplicates thetype Cursor<T>andtype DupCursor<T>. This duplication is a recognized TODO. Consolidating them in a single location or using generic constraints might reduce boilerplate.
151-180: Ohayo sensei, add a specialized unit test
Though the code works for references, adding targeted tests to verify the behavior of each method in both read-only and read-write contexts ensures coverage and prevents regressions. Would you like me to draft some test stubs?crates/katana/rpc/rpc-types/src/trie.rs (2)
66-106: Ohayo sensei, strongly typed proofs
DefiningGetStorageProofResponseand the associated proof structures is a good approach. This ensures clarity in RPC responses. Consider adding a short doc block explaining usage ofclasses_proofvs.contracts_proof.
108-129: Ohayo sensei, simpler wrappers
NodeWithHashandNodesare well-defined. The customDerefandDerefMutimprove ergonomics. Keep an eye on memory overhead for large proofs.crates/katana/core/src/backend/mod.rs (1)
255-263: Ohayo sensei, consider better error handling
Using.expect("failed to update...")might panic in production. A more robust approach is to propagate the error or provide tailored logging.- .expect("failed to update class trie"); + .map_err(|e| anyhow!("Class trie update failed: {e:?}"))?;bin/katana/src/cli/init/mod.rs (2)
88-120: Ohayo sensei, structuringInitArgs::execute
Theexecutemethod properly constructs a runtime and organizes the flow. Consider logging progress steps to inform users about configuration steps.
202-281: Ohayo sensei, multi-step settlement contract deployment
init_core_contractis well-structured. Using a spinner is a nice UX detail. You might want more robust error messages if a step fails (e.g., class declare vs. contract deploy).crates/katana/storage/provider/src/providers/db/state.rs (2)
165-193: Ohayo sensei! Validate empty input in multiproof methods.
The multiproof logic is correct overall. However, consider returning an empty proof or an explicit error if the input vectors for classes/contracts/storage are empty, to avoid potential edge cases in downstream code.
387-423: Ohayo sensei! The historical state root retrieval is aligned with project requirements.
As with the multiproof coverage, substitutingexpect("should exist")with a more robust fallback approach might improve resilience for partial states or incomplete blocks.crates/katana/storage/db/src/trie/mod.rs (6)
29-33: Ohayo sensei!TrieDbFactorystruct is a useful abstraction.
Consider verifying if&'a PhantomData<()>is fully needed or if a simpler lifetime management suffices.
34-47: Ohayo sensei! TheTrieDbFactoryimpl is clear.
Adding stronger error handling aroundhistorical(block)might help avoidSome(...)proxies if the data doesn’t exist.
49-80: Ohayo sensei!GlobalTrieis well-structured for latest tries.
It might help to unify repeatedkatana_trie::XxxTrie::new(TrieDb::new(...))calls via a small helper function to reduce code duplication.
82-121: Ohayo sensei!HistoricalGlobalTriecleanly extends functionality.
Guardingexpect("should exist")with a more informative error for uninitialized or partial historical data would bolster reliability.
209-239: Ohayo sensei!TrieDbMutintroduces write caching.
Storing writes inwrite_cacheis good for snapshot logic but be mindful of potential memory usage if many entries accumulate.
Line range hint
243-327: Ohayo sensei! Write operations are well-limited.
remove_by_prefixusage is robust. Consider bounding the iteration in large databases or using indexes if performance becomes an issue.crates/katana/storage/provider/src/providers/fork/state.rs (4)
105-124: Ohayo sensei! Stubbed multiproof is acceptable.
However, returning a defaultMultiProofmay prematurely signal success in external callers. Consider returning an explicit unsupported or placeholder status to reduce confusion.
181-200: Ohayo sensei! Same stubbing approach inLatestStateProvider.
Alternatively, implement partial proof logic or warn that these are placeholders so consumers don’t rely on them incorrectly.
288-307: Ohayo sensei! Similar multiproof stubbing inForkedSnapshot.
Everything is consistent with the approach inForkedStateDb. Just ensure you unify them if or when real proofs become available.
[minimum_revision_needed: none | final decision is up to you]
309-321: Ohayo sensei! The stubbedStateRootProviderforForkedSnapshot.
Again, returningFelt::ZEROcan be ambiguous, especially for real usage. Consider a distinct sentinel or clarifying doc comment.crates/katana/storage/db/src/tables.rs (1)
250-269: Ohayo sensei! The new trie tables store high-level data consistently.
Might be worth adding doc comments explaining how these tries differ from one another for new developers.crates/katana/trie/src/storages.rs (1)
16-28: Ohayo sensei! Consider adding a test helper formultiproof.While
rootandmultiproofare straightforward, having a dedicated test function that exercisesmultiproofwith various edge cases would strengthen reliability. This is especially beneficial for surface-level bugs in hashing or key conversions.crates/katana/trie/src/contracts.rs (1)
17-34: Ohayo sensei! Good introduction ofBONSAI_IDENTIFIER.Embedding an identifier for the contract mapping inside the trie logic is a fine architectural choice, providing clear scoping of keys. Consider adding in-code references for any extended usage or potential expansions.
crates/saya/core/src/blockchain/mod.rs (1)
9-9: Ohayo sensei! Nice consolidation of state traits.
IntroducingStateFactoryProvideraligns with the refactor that moves away fromStateRootProvider. This approach appears cleaner, but verify that all dependent modules can still derive the state root without disruptions.crates/katana/executor/src/implementation/noop.rs (1)
173-197: Ohayo sensei! The newStateProofProviderimplementation is looking good.These multiproof stubs returning an empty default proof are a straightforward start. Consider adding tests or logs if you expand them so that future senseis can better troubleshoot real proofs.
crates/katana/storage/provider/src/providers/fork/mod.rs (2)
602-603: Ohayo sensei! Implementingtrie_insert_declared_classesis a step toward consolidating class updates. For now, it returns a dummyFelt::ZERO, but you might want to confirm how it should reflect real trie manipulations in future iterations.
613-613: Ohayo sensei! Implementingtrie_insert_contract_updatessimilarly returnsFelt::ZERO. In a future iteration, consider mixing in the logic for actual updates to the fork's trie data.crates/katana/executor/src/implementation/blockifier/state.rs (2)
250-256: Ohayo sensei!
Unimplementedcontract_multiproofSimilar to
class_multiproof, this method remains unimplemented. If the system eventually needs to return multiproof data, schedule a follow-up to provide the actual logic.
258-266: Ohayo sensei!
Unimplementedstorage_multiproofAgain, the same pattern arises: unimplemented method. If this is intentional, consider returning a more descriptive error for clarity.
crates/katana/storage/provider/src/providers/fork/backend.rs (3)
679-687: Ohayo sensei!
Unimplementedstorage_multiproofIdentical pattern: no actual logic for storage proofs. Provide a short docstring or error message to clarify this limitation if you see fit.
695-697: Ohayo sensei!
Unimplementedcontracts_rootSame approach; if you opt to keep placeholders, add notes to reduce confusion for future maintainers.
699-701: Ohayo sensei!
Unimplementedstorage_rootNo functionality indicated. Same comment applies: consider a descriptive error or a
TODO.bin/katana/Cargo.toml (1)
22-24: Consider using workspace versions for consistency, sensei!The
dirsandinquiredependencies use fixed versions while most other dependencies use workspace versions. Consider moving these to workspace-level version management for better maintainability.-dirs = "5.0.1" +dirs.workspace = true -inquire = "0.7.5" +inquire.workspace = truecrates/katana/rpc/rpc/Cargo.toml (1)
Line range hint
20-34: Ohayo! The RPC dependency additions look comprehensive, sensei!The new dependencies enhance the RPC capabilities with proper metrics (
dojo-metrics,metrics), error handling (thiserror), and HTTP server infrastructure (tower,tower-http). This provides a solid foundation for the new storage proof functionality.Consider adding metrics for:
- Storage proof generation time
- Proof size statistics
- Cache hit/miss rates for frequently accessed storage slots
crates/katana/storage/db/src/models/trie.rs (1)
8-11: Ohayo! Consider adding documentation for the new struct, sensei.The struct looks well-designed, but adding documentation would help explain its purpose and usage patterns.
Add documentation like this:
+/// Represents a historical entry in the trie database +/// containing the key and its associated value at a point in time. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct TrieHistoryEntry { pub key: TrieDatabaseKey, pub value: TrieDatabaseValue, }crates/katana/rpc/rpc/tests/test_data/test_sierra_contract.json (1)
Line range hint
1-892: Consider adding debug information for better test observability.The contract is well-structured but lacks debug information in the
sierra_program_debug_infosection, which could be helpful for testing and debugging.Consider adding type names, libfunc names, and user func names in the debug info section to improve test observability.
🛑 Comments failed to post (10)
crates/katana/trie/src/lib.rs (1)
56-70: 🛠️ Refactor suggestion
Ohayo sensei! Panics on
rootorget_multi_proof
Returningexpect(...)can cause abrupt panics if the database is unavailable. Consider returningProviderResultto capture these errors gracefully.crates/katana/storage/provider/src/providers/db/state.rs (1)
348-385: 🛠️ Refactor suggestion
Ohayo sensei! Storing and retrieving historical multiproofs could benefit from checks.
This implementation forHistoricalStateProvideris fine, but consider logging or returning a clear error ifhistorical(self.block_number)fails or is absent rather than callingexpect("should exist").crates/katana/storage/db/src/trie/mod.rs (1)
328-370: 🛠️ Refactor suggestion
Ohayo sensei! Snapshotting changes is crucial.
Check if any concurrency concerns arise when multiple threads callsnapshotsimultaneously (e.g., mismatch inwrite_cache).crates/katana/storage/provider/src/providers/fork/state.rs (2)
126-138: 🛠️ Refactor suggestion
Ohayo sensei! The root provider functions returning zero are placeholders.
If zero indicates "no data," a separate error or optional type might be clearer for external integrators.
202-214: 🛠️ Refactor suggestion
Ohayo sensei! Returning zero for
classes_root,contracts_root, andstorage_root.
Be careful: if an aggregator merges root data but sees0, it might misinterpret the entire state as empty.crates/katana/rpc/rpc/src/starknet/mod.rs (1)
1132-1219: 🛠️ Refactor suggestion
Ohayo sensei!
get_proofsis a fantastic addition, but handle edge cases more explicitly.
- Consider returning partial proofs if some classes/contracts aren’t found.
- The line that calls
expect("...")in block or header retrieval might be replaced with a custom error to avoid panics.- Confirm the sum of keys used matches expected concurrency patterns to avoid performance hits.
crates/katana/contracts/Makefile (2)
16-17:
⚠️ Potential issueOhayo! Variable redefinition needs attention, sensei!
The
ORIGINAL_CLASS_NAMEvariable is being redefined which could lead to conflicts with the default account section. Consider using unique variable names for different targets.-ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX) +PILTOVER_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.PILTOVER_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX) CLASS_NAME := appchain_core_contract.json
19-21: 🛠️ Refactor suggestion
Build directory dependency might need refinement.
The
$(BUILD_DIR)target is defined multiple times with different prerequisites, which could lead to unexpected behavior. Consider using separate target names.-$(BUILD_DIR): ./piltover/src/* +build-piltover: ./piltover/src/* + mkdir -p $(BUILD_DIR) cd piltover && scarb build mv target/dev/$(ORIGINAL_CLASS_NAME) $(BUILD_DIR)/$(CLASS_NAME)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.build-piltover: ./piltover/src/* mkdir -p $(BUILD_DIR) cd piltover && scarb build mv target/dev/$(ORIGINAL_CLASS_NAME) $(BUILD_DIR)/$(CLASS_NAME)crates/katana/storage/db/src/models/trie.rs (2)
24-35:
⚠️ Potential issueAdd proper buffer length validation in Decompress implementation.
The current implementation could panic if the buffer is too short after key decoding.
Apply this diff to add proper validation:
impl Decompress for TrieHistoryEntry { fn decompress<B: AsRef<[u8]>>(bytes: B) -> Result<Self, CodecError> { let bytes = bytes.as_ref(); let key = TrieDatabaseKey::decode(bytes)?; // first byte is the key type, second byte is the actual key length let key_bytes_length = 1 + 1 + key.key.len(); + if bytes.len() <= key_bytes_length { + return Err(CodecError::InvalidData("Buffer too short for value".into())); + } let value = TrieDatabaseValue::decompress(&bytes[key_bytes_length..])?; Ok(Self { key, value }) } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.impl Decompress for TrieHistoryEntry { fn decompress<B: AsRef<[u8]>>(bytes: B) -> Result<Self, CodecError> { let bytes = bytes.as_ref(); let key = TrieDatabaseKey::decode(bytes)?; // first byte is the key type, second byte is the actual key length let key_bytes_length = 1 + 1 + key.key.len(); if bytes.len() <= key_bytes_length { return Err(CodecError::InvalidData("Buffer too short for value".into())); } let value = TrieDatabaseValue::decompress(&bytes[key_bytes_length..])?; Ok(Self { key, value }) } }
86-97:
⚠️ Potential issueReplace panics with proper error handling in decode method.
The current implementation uses panic which is not ideal for a decode operation.
Apply this diff to use proper error handling:
- if bytes.len() < 2 { - // Need at least type and length bytes - panic!("empty buffer") - } + if bytes.len() < 2 { + return Err(CodecError::InvalidData("Buffer too short for type and length".into())); + } - let r#type = - TrieDatabaseKeyType::try_from(bytes[0]).expect("Invalid trie database key type"); + let r#type = TrieDatabaseKeyType::try_from(bytes[0]) + .map_err(|e| CodecError::InvalidData(format!("Invalid key type: {}", e.0)))?; let key_len = bytes[1] as usize; - if bytes.len() < 2 + key_len { - panic!("Buffer too short for key length"); - } + if bytes.len() < 2 + key_len { + return Err(CodecError::InvalidData("Buffer too short for key data".into())); + }Committable suggestion skipped: line range outside the PR's diff.
1 participant
the storage root is required to compute the contract state hash (needed for the proofs verification). the rpc specs doesn't include the storage root in the response object, but considering that the specs version is still on draft for now, we can tolerate deviating from it to simplify the SNOS integration
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores