fix(katana): invalid trie path conversion #2844
Conversation
kariy
force-pushed
the
katana/trie-db-snapshot
branch
from
656097d to
d76c5ae
Compare
WalkthroughOhayo, sensei! This pull request introduces a comprehensive set of changes across the Katana blockchain infrastructure, focusing on enhancing state management, trie structures, and proof generation capabilities. The modifications span multiple crates, introducing new traits, refactoring existing implementations, and adding support for storage proofs and historical state retrieval. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RPCServer
participant StateProvider
participant TrieDatabase
participant StorageProofGenerator
Client->>RPCServer: Request Storage Proof
RPCServer->>StateProvider: Retrieve Historical State
StateProvider->>TrieDatabase: Access Trie Data
TrieDatabase->>StorageProofGenerator: Generate Multi-Proof
StorageProofGenerator-->>RPCServer: Return Proof
RPCServer-->>Client: Respond with Storage Proof
Possibly Related PRs
Suggested Reviewers
Sensei, the changes represent a significant evolution in Katana's state management and proof generation capabilities! 🚀🔍 🪧 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
🔭 Outside diff range comments (1)
crates/katana/rpc/rpc/Cargo.toml (1)
Ohayo sensei! Found one non-workspace dependency that needs attention!
While most dependencies correctly use workspace versions, I spotted one direct version specification:
- In
katana-rpc-types/Cargo.toml:serde_json_pythonic = "0.1.2"is directly specified instead of using workspace versionConsider moving this dependency to the workspace level for better version management and consistency with the project's dependency strategy.
Note: All other dependencies in the RPC crates properly use the workspace versions as indicated by the
.workspace = truespecification, which is great for maintainability!🔗 Analysis chain
Line range hint
19-34: Ohayo sensei! The dependency additions look well-structured!The new dependencies enhance the RPC capabilities with metrics, serialization, and blockchain functionality. All dependencies correctly use workspace versions.
Let's verify the version compatibility:
The previous command might have failed because the path was too specific. Let's try a broader search to verify the dependency versions and ensure there are no conflicts between workspace and local specifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential version conflicts in workspace dependencies # Look for any direct version specifications that might conflict with workspace versions rg -g "Cargo.toml" "version\s*=\s*\"" "crates/katana/rpc/"Length of output: 55
Script:
#!/bin/bash # Let's check the workspace Cargo.toml for the versions of these dependencies cat Cargo.toml | grep -A 1 "^\[workspace.dependencies\]" # Also check if there are any direct version specifications in the RPC crates fd Cargo.toml -e toml crates/katana/rpc | xargs catLength of output: 3786
🧹 Nitpick comments (51)
crates/katana/trie/src/storages.rs (1)
14-30: Ohayo sensei! Check for consistent references.The
multiproofmethod takes&mut selfbut only reads from the storage keys. Consider switching it to&selffor improved clarity and concurrency safety:- pub fn multiproof(&mut self, address: ContractAddress, storage_keys: Vec<StorageKey>) -> MultiProof { + pub fn multiproof(&self, address: ContractAddress, storage_keys: Vec<StorageKey>) -> MultiProof {crates/katana/trie/src/contracts.rs (2)
26-29: Ohayo sensei! Remove debug prints before production.Line 28 uses
dbg!(&keys), which can clutter logs. Consider removing or converting to trace-level logs.-dbg!(&keys); +// dbg!(&keys); // remove or use a logging macro if needed
33-44: Ohayo sensei! Keep consistent withBonsaiPersistentDatabaseusage.When the database also implements
BonsaiPersistentDatabase<CommitId>, error handling oninsertandcommitis currently absent. Consider returning aResultto manage DB or I/O errors gracefully.crates/katana/trie/src/classes.rs (1)
37-51: Ohayo sensei! Consider non-mutable borrowing formultiproof.Similar to
StoragesTrie, if no writes occur, prefer&selfover&mut self. This fosters safer concurrency usage.crates/katana/storage/provider/src/traits/state.rs (2)
14-22: Ohayo sensei! Synchronized root retrieval is well-structured.Using
Poseidon::hash_arrayfor the final root is aligned with the StarkNet docs. For large sets of tries or potential concurrency, ensure no performance pitfalls: hashing large arrays repeatedly might need caching.
24-25: Ohayo sensei!classes_rootcan be documented further.Document the nature of
Feltreturned, especially if the user expects a default root for an empty classes trie.crates/katana/storage/db/src/models/trie.rs (2)
13-22: Ohayo sensei! Consolidate error checks incompress.
compressdirectly concatenates data. Ifkey.encode()orvalue.compress()fail for some reason, consider returningResultto handle unexpected edge cases.
91-93: Ohayo sensei!Invalid trie database key typeerror check is good.The explicit
expect(...)is acceptable here, but you might unify error handling with the approach used for partial buffers.crates/katana/storage/provider/src/providers/db/trie.rs (1)
25-40: Ohayo sensei, thetrie_insert_declared_classesimplementation looks solid!
The approach of batching the updates inside a single transaction (self.0.update(...)) is cleaner. Here are a few observations:
trie.insert(*class_hash, *compiled_hash)assumes insertion cannot fail. Consider verifying error handling if the underlying function can fail in edge cases.- The final
Ok(trie.root())is straightforward but ensure that external callers handle potential indexing or hashing collisions gracefully.crates/katana/trie/src/lib.rs (2)
10-19: Ohayo sensei, good job exposing these new modules and re-exports!
Publicly re-exporting them keeps the code more organized and clarifies the crate’s surface area for downstream users.
71-84: Ohayo sensei, theinsertandcommitmethods are clear!
Beware of repeated commits in short succession, as it may hinder performance. A robust batching mechanism or throttling might be beneficial in the future.crates/katana/executor/src/abstraction/mod.rs (1)
209-225: Ohayo sensei, solidStateProofProviderimplementation!
Forwarding multiproof calls to the underlying provider is a clean design. You might consider logging or caching proof data if repeated proofs become a performance bottleneck.crates/katana/rpc/rpc-types/src/trie.rs (4)
66-77: Ohayo sensei,GetStorageProofResponseis a clear aggregator of proof data.
Keep an eye on backward compatibility if you alter the shape of these results in subsequent versions.
79-82: Ohayo sensei,ClassesProoforganizes nodes well.
Potentially keep a higher-level summary for large proofs to allow partial verification.
99-102: Ohayo sensei,ContractStorageProofsgrouping is fine.
If you plan to expand storage proof details, consider a more flexible structure or nested representation.
191-195: Ohayo sensei,path_to_felteffectively reconstructs the Felt from bits.
As withfelt_to_path, verifying edge cases at length boundaries is advisable.crates/katana/storage/db/src/trie/snapshot.rs (2)
15-23: Ohayo sensei,SnapshotTrieDbis well-defined for read-only snapshots!
The extension withsnapshot_idin the struct is intuitive. Make sure references toPhantomData<Tb>are consistent across the code to avoid lifetime confusion.
25-33: Ohayo sensei,Debugimplementation looks neat.
Excludingsnapshot_idfrom debug logs might be an intentional choice, but consider including it for easier troubleshooting.crates/katana/core/src/backend/mod.rs (1)
166-166: Ohayo, sensei! Renamingtrie_providertoproviderclarifies the purpose.Shorter, more intuitive naming fosters better readability.
bin/katana/src/cli/init/mod.rs (3)
121-200: Ohayo, sensei! The interactivepromptmethod is well-designed, but watch out for error handling.Validation of user input is good. Additional logging of parse failures could help debugging.
289-293: Ohayo, sensei!get_flattened_classfunction is short, sweet, and handles JSON parsing well.Error context messages might help if JSON deserialization fails.
303-319: Ohayo, sensei! The config path logic is seamlessly integrated with the user’s OS.Check if
dirs::config_local_dir()can fail in exotic environments, adding graceful fallback might be beneficial.crates/katana/storage/provider/src/providers/db/state.rs (2)
194-207: Ohayo, sensei! Exposing classes and contracts root is a clean design.We might consider combining them into a single state root if used together often.
411-414: Ohayo, sensei! Documentingrecent_change_from_blockhelps clarify the block ranking logic.This function is easy to misread, so additional examples in doc comments are always welcome.
crates/katana/storage/provider/src/providers/fork/state.rs (4)
105-124: Ohayo, sensei! The default multiproof stubs forForkedStateDbcan be placeholders for future expansions.We might later link these to an actual trie if needed.
Would you like sample code for a minimal multiproof integration?
126-134: Ohayo, sensei! Returning zero for roots is acceptable for a partial/forked environment, but clarifying this in docs might help.
177-196: Ohayo, sensei! TheLatestStateProvidermultiproof stubs maintain consistency withForkedStateDb.Again, doc comments explaining the intended future usage would be helpful.
301-309: Ohayo, sensei! The zero root inForkedSnapshotis consistent.Proper documentation ensures future maintainers understand why it’s always zero.
crates/katana/storage/db/src/trie/mod.rs (1)
30-30: Ohayo sensei! Consider clarifying the_phantomusage.
Excessive reliance on the_phantomfield might be improved by using a more explicit zero-sized struct or typed marker to better communicate intent.Also applies to: 34-35
crates/katana/storage/db/src/tables.rs (2)
180-189: Ohayo sensei! Watch out for table count expansions.
IncreasingNUM_TABLESto 33 and adding new trie variants is fine, but keep an eye on collisions or macro expansions as the project grows.
250-269: Ohayo sensei! Nice modular approach for trie tables.
SeparatingClassesTrie,ContractsTrie, andStoragesTrieimproves clarity. Adding doc comments or usage examples could further help new contributors.crates/katana/storage/provider/src/providers/db/mod.rs (1)
Line range hint
1132-1218: Ohayo sensei! Solid implementation ofget_proofs.
Verify boundary checks remain comprehensive. If the block doesn't exist, ensure the flow gracefully handles the error to match other provider calls.crates/katana/rpc/rpc/src/starknet/mod.rs (1)
1132-1218: Ohayo sensei! Great addition ofget_proofsin RPC.
Consider adding logs or telemetry around large proof requests for stronger observability. This can help quickly isolate performance issues if the request loads become heavy.bin/katana/src/cli/mod.rs (1)
36-37: Ohayo sensei, clarity in subcommand naming.
Using#[command(about = ...)]forInithelps users quickly understand the subcommand. The short description is succinct and direct.crates/katana/executor/src/implementation/noop.rs (1)
173-197: Ohayo sensei!
All multiproof methods return defaultkatana_trie::MultiProofinstances. This is convenient for a “no-op” scenario. However, sensei, if you ever decide to extend them to produce real proofs for testing or debugging, be sure to confirm that the rest of the code handles non-default data properly.crates/katana/rpc/rpc/tests/proofs.rs (1)
73-158: Ohayo sensei!
Theclasses_proofstest thoroughly checks separate class declarations and aggregated proofs, ensuring correctness. The logic looks good. One small note: capturing edge cases, such as no declared classes or repeated classes, can further harden the test coverage.crates/katana/storage/db/src/abstraction/transaction.rs (1)
94-128: Ohayo sensei!
DbTxMutRefis a straightforward extension for mutable references. Similar concurrency or ownership caveats may apply. If any multi-threaded usage arises, ensure these references remain sound.
[architecure_advice → NOTE: There's a small correction, the tag in the instructions is "architecture_advice" (not spelled incorrectly).]crates/katana/rpc/rpc-api/src/starknet.rs (1)
188-199: Ohayo sensei!
Theget_storage_proofmethod is an important extension for advanced state verification, giving sensei-level insight into the node’s state. The logic signature seems consistent. For future expansions, consider clear error responses in case any sub-proofs are unsupported.crates/katana/rpc/rpc/src/starknet/read.rs (1)
270-282: Ohayo sensei! Check for block existence and error handling.
This newly introducedget_storage_proofmethod is a valuable addition. However, consider verifying that the requestedblock_idactually exists and properly handling any potential error conditions (e.g., invalid block references). Also, ensure the method is covered in unit tests to maintain reliability for clients.crates/katana/storage/provider/tests/block.rs (2)
32-32: Ohayo sensei!#[ignore]annotation is understandable but keep an eye on coverage.
You are ignoring trie computation tests for forked mode. This might reduce test coverage. Consider partial tests or mocks so we don’t forget to revisit later.
49-49: Ohayo sensei! Another ignored test spotted.
As with the previous comment, consider partial coverage or a future plan to implement forked-mode trie computations.crates/katana/cli/src/options.rs (1)
111-116: Ohayo sensei! Introducingmax_proof_keysto the CLI is an excellent config extension.
This option makes the node more flexible for storage-proof usage. Be sure to document any performance implications for large values.crates/katana/storage/provider/src/providers/fork/mod.rs (1)
413-423: Ohayo sensei! Commented-outStateRootProviderblock.
Marking these lines as deprecated or removed is consistent with the shift towardTrieWriter. If you intend to remove them entirely, feel free to clean them up.crates/katana/executor/src/implementation/blockifier/state.rs (2)
241-267: Ohayo sensei! The new multiproof methods are left unimplemented.
It's great to see the groundwork for multiproof functionality. However, returningunimplemented!may block downstream usage. Consider providing a placeholder or partial implementation, possibly returning an empty proof or an error that is more descriptive than “not supported” to guide future contributors.
269-277: Ohayo sensei! The new state root methods are also left unimplemented.
If calculating these roots is indeed not supported, consider documenting the reasoning to inform readers about constraints or future plans. This can help maintainers quickly assess the feasibility of implementing them later.crates/katana/storage/provider/src/providers/fork/backend.rs (2)
665-688: Ohayo sensei! The multiproof methods remain unimplemented here.
For external contributors and testers, returning an explicit error or a descriptive message may help them understand that this feature is not currently available in forked mode. You may also want to raise a dedicated issue for multiproof functionality if it's planned.
690-698: Ohayo sensei! Theclasses_rootandcontracts_rootmethods are likewise unimplemented.
Even though this is forked mode, providing more context in the error (for example,Err(ProviderError::UnsupportedFeature(...))) could reduce confusion for integrators.crates/katana/runner/macro/Cargo.toml (1)
11-13: Ohayo sensei! Transitioning to workspace-level dependencies fosters a more unified versioning approach.
All references now resolve to the workspace version, improving consistency across the project. Just be sure to carefully review the workspace’s Cargo.toml to confirm that the correct versions and features are set.bin/katana/Cargo.toml (1)
22-24: Consider using workspace versions for consistency.Ohayo sensei! The
dirs = "5.0.1"dependency is version-pinned while most other dependencies use workspace versioning. Consider moving this to workspace versioning for better maintenance.crates/katana/storage/db/Cargo.toml (1)
37-39: Good addition of testing frameworks, but consider workspace versioning.Ohayo sensei! The addition of testing frameworks is excellent for improving code quality. However:
- Consider moving
proptest = "1.6.0"to workspace versioning for consistency with other dependencies.- The move of
starknetto dev-dependencies is a good separation of concerns.crates/katana/rpc/rpc/tests/test_data/test_sierra_contract.json (1)
Line range hint
1-7: Ohayo sensei! Consider adding debug information for better debugging experience.The debug information arrays are empty, which could make debugging more challenging. Consider populating:
- type_names
- libfunc_names
- user_func_names
🛑 Comments failed to post (10)
crates/katana/trie/src/storages.rs (1)
32-48: 🛠️ Refactor suggestion
Ohayo sensei! Encourage standardized error handling.
Insert errors succeed silently. If insertion or commits can fail, ensure you bubble those up via
Resultor logging. Currently, both methods return()instead of aResult, which hides potential DB layer issues.crates/katana/trie/src/classes.rs (1)
53-65: 🛠️ Refactor suggestion
Ohayo sensei! Evaluate error propagation for
insertandcommit.Current code does not reflect potential DB errors. A
Resulttype would allow capturing them. This is recommended for robust system design.crates/katana/storage/db/src/models/trie.rs (2)
24-35: 🛠️ Refactor suggestion
Ohayo sensei! Prefer descriptive error messages.
panic!("empty buffer")at line 88 might be replaced by a formalCodecErroror a specialized error type to avoid abrupt terminations in production.
86-88: 🛠️ Refactor suggestion
Ohayo sensei! Avoid panic on partial buffers.
Instead of
panic!, prefer returning aCodecError. That way, upper layers can manage or retry gracefully.crates/katana/storage/db/src/trie/snapshot.rs (1)
59-123: 🛠️ Refactor suggestion
Ohayo sensei, the
BonsaiDatabaseimplementation suitably forbids modifications in a snapshot.
Key details:
unimplemented!ortodo!for mutating methods is consistent with read-only semantics.- The
getfunction’s logic is robust, though an additional check for missing entries in the history might be warranted.- Consider implementing
containsorget_by_prefixif necessary for future expansions.crates/katana/core/src/backend/mod.rs (1)
255-263: 🛠️ Refactor suggestion
Ohayo, sensei! Careful about error handling with
expect.In production code, it might be beneficial to handle errors more gracefully instead of panicking.
bin/katana/src/cli/init/mod.rs (1)
202-281: 🛠️ Refactor suggestion
Ohayo, sensei! The
init_core_contractfunction is a big plus for asynchronous contract deployment.Consider removing or refactoring the
expect("should exist")calls in a production environment, to avoid occasional panics.crates/katana/storage/db/src/trie/mod.rs (2)
148-190: 🛠️ Refactor suggestion
Ohayo sensei! Unimplemented methods in
BonsaiDatabasetrait.
There are severaltodo!()andunimplemented!()macros, which can cause panics. Recommend returning a descriptiveErrin the interim or implementing partial features.
321-363:
⚠️ Potential issueOhayo sensei! Missing implementation of merge functionality.
Themergemethod isunimplemented!()and could block usage scenarios that require re-computation of tries across multiple snapshots.crates/katana/contracts/Makefile (1)
14-22:
⚠️ Potential issueOhayo! The Makefile needs some love, sensei!
The current structure has several issues that need attention:
- Using the same target
$(BUILD_DIR)for multiple rules can cause conflicts with the account contract build- The build process could fail silently if the piltover directory is not properly initialized
Here's a suggested improvement:
-## ---- Piltover contracts +## ---- Piltover contracts +PILTOVER_DIR := ./piltover +PILTOVER_TARGET := $(BUILD_DIR)/$(CLASS_NAME) -ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX) -CLASS_NAME := appchain_core_contract.json +PILTOVER_ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX) +PILTOVER_CLASS_NAME := appchain_core_contract.json -$(BUILD_DIR): ./piltover/src/* - cd piltover && scarb build - mv target/dev/$(ORIGINAL_CLASS_NAME) $(BUILD_DIR)/$(CLASS_NAME) +$(PILTOVER_TARGET): $(PILTOVER_DIR)/src/* + @if [ ! -d "$(PILTOVER_DIR)" ]; then \ + echo "Error: Piltover directory not found. Run 'git submodule update --init'"; \ + exit 1; \ + fi + cd $(PILTOVER_DIR) && scarb build + mv $(PILTOVER_DIR)/target/dev/$(PILTOVER_ORIGINAL_CLASS_NAME) $(PILTOVER_TARGET) + +.PHONY: build-piltover clean-piltover + +build-piltover: $(PILTOVER_TARGET) + +clean-piltover: + rm -f $(PILTOVER_TARGET)📝 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 contracts PILTOVER_DIR := ./piltover PILTOVER_TARGET := $(BUILD_DIR)/$(CLASS_NAME) PILTOVER_ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX) PILTOVER_CLASS_NAME := appchain_core_contract.json $(PILTOVER_TARGET): $(PILTOVER_DIR)/src/* @if [ ! -d "$(PILTOVER_DIR)" ]; then \ echo "Error: Piltover directory not found. Run 'git submodule update --init'"; \ exit 1; \ fi cd $(PILTOVER_DIR) && scarb build mv $(PILTOVER_DIR)/target/dev/$(PILTOVER_ORIGINAL_CLASS_NAME) $(PILTOVER_TARGET) .PHONY: build-piltover clean-piltover build-piltover: $(PILTOVER_TARGET) clean-piltover: rm -f $(PILTOVER_TARGET)
kariy
force-pushed
the
katana/trie-db-snapshot
branch
from
d76c5ae to
443734a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## katana/dev #2844 +/- ##
=============================================
Coverage ? 55.71%
=============================================
Files ? 446
Lines ? 57447
Branches ? 0
=============================================
Hits ? 32005
Misses ? 25442
Partials ? 0 ☔ View full report in Codecov by Sentry. |
We didn't preserve the
Pathsemantics when it is being converted from RPC types to internal types frombonsai-trie.