feat(katana): compute block commitments #2609
Conversation
WalkthroughOhayo, sensei! This pull request introduces multiple changes across various files in the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 12
🧹 Outside diff range and nitpick comments (7)
crates/katana/primitives/src/state.rs (1)
33-48: Add documentation for the len method, sensei!While the implementation is correct, it would benefit from documentation explaining what this length represents and its significance in the context of state updates.
Consider adding documentation like this:
+ /// Returns the total number of state updates across all categories: + /// - Deployed contracts + /// - Replaced classes + /// - Declared classes + /// - Deprecated declared classes + /// - Nonce updates + /// - Storage updates (sum of all storage entries for each contract) #[allow(clippy::len_without_is_empty)] pub fn len(&self) -> usize {crates/saya/provider/src/rpc/mod.rs (2)
Line range hint
93-107: Consider grouping commitment-related fields together.Ohayo! The
state_diff_lengthis added among other commitment-related fields. For better code organization and maintainability, consider grouping all commitment-related fields together:
- state_diff_commitment
- state_diff_length
- events_commitment
- receipts_commitment
- transactions_commitment
Apply this diff to improve field organization:
header: Header { events_count: 0, - state_diff_length: 0, transaction_count: 0, + // Commitment-related fields + state_diff_length: 0, + state_diff_commitment: Felt::ZERO, events_commitment: Felt::ZERO, receipts_commitment: Felt::ZERO, - state_diff_commitment: Felt::ZERO, transactions_commitment: Felt::ZERO,
Line range hint
147-148: Consider computing state_diff_length from serialized state updates.Ohayo sensei! The
serialized_state_updateobtained fromstate_diff_to_feltscould be used to compute thestate_diff_length. This would provide an accurate length instead of the current placeholder value.Consider updating the
fetch_blockmethod to include this computation:// Add this field to the Header initialization state_diff_length: serialized_state_update.len() as u64,Also applies to: 182-183
crates/katana/primitives/src/receipt.rs (2)
180-189: Clean and well-structured implementation, sensei!The struct design follows Rust best practices with appropriate trait derivations. The use of
DerefandAsRefallows for ergonomic access to the innerReceiptmethods.Consider adding a doc comment for the struct itself explaining its purpose and relationship to the block commitment computation:
/// Associates a transaction hash with its receipt, used for computing block commitments. /// This combination allows for efficient verification of transaction execution results. pub struct ReceiptWithTxHash {
220-244: Efficient implementation of messages hash computation!The implementation is well-optimized with pre-allocated capacity and effective use of iterators.
Consider extracting the payload hash computation into a separate method for better readability:
fn compute_payload_hash(payload: &[Felt]) -> Felt { let len = Felt::from(payload.len()); let payload = iter::once(len) .chain(payload.iter().cloned()) .collect::<Vec<Felt>>(); hash::Poseidon::hash_array(&payload) }This would make the main method more concise:
let payload_hash = compute_payload_hash(&msg.payload);crates/katana/primitives/src/chain_spec.rs (1)
375-375: Consider enhancing test coverage forstate_diff_length.While the test includes the new field, it doesn't verify scenarios where
state_diff_lengthmight have non-zero values. Consider adding test cases that:
- Verify the relationship between state updates and
state_diff_length- Cover edge cases where state differences might affect this value
crates/katana/core/src/backend/mod.rs (1)
225-230: Optimizeevent_hashFunction ScopeOhayo, sensei! The
event_hashfunction is defined withincompute_event_commitment. If this utility function could be useful elsewhere, consider moving it to a broader scope for reusability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
crates/katana/core/Cargo.toml(2 hunks)crates/katana/core/src/backend/mod.rs(3 hunks)crates/katana/core/src/backend/storage.rs(3 hunks)crates/katana/primitives/src/block.rs(5 hunks)crates/katana/primitives/src/chain_spec.rs(2 hunks)crates/katana/primitives/src/receipt.rs(2 hunks)crates/katana/primitives/src/state.rs(3 hunks)crates/katana/storage/db/src/version.rs(2 hunks)crates/saya/provider/src/rpc/mod.rs(1 hunks)
🔇 Additional comments (16)
crates/katana/core/Cargo.toml (1)
16-16: Ohayo! The new dependencies align well with block commitment requirements.
The addition of katana-trie and starknet-types-core makes perfect sense for computing block commitments, sensei! The trie implementation will be crucial for managing state roots, while the StarkNet types ensure proper typing of blockchain-specific data structures.
Let's verify these dependencies are properly configured in the workspace:
Also applies to: 31-31
✅ Verification successful
Ohayo! The dependencies are properly configured in the workspace, sensei!
The verification shows that:
katana-trieis correctly defined in the workspace dependencies with a path tocrates/katana/trie- Both dependencies are properly referenced across multiple Cargo.toml files in the katana ecosystem
- The workspace structure supports these dependencies, ensuring consistent versioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workspace configuration for new dependencies
# Test 1: Check if dependencies exist in workspace
rg -A 1 "katana-trie\W|starknet-types-core\W" Cargo.toml
# Test 2: Verify version specifications in workspace
rg -A 1 '\[workspace\.dependencies\]' -A 50 Cargo.toml | rg "katana-trie|starknet-types-core"
Length of output: 692
Script:
#!/bin/bash
# Let's fix the command and verify workspace dependencies properly
# Check workspace dependencies section
rg '^\[workspace\.dependencies\]' -A 50 Cargo.toml
# Check all references to these dependencies across Cargo.toml files
fd Cargo.toml -x rg -l "katana-trie|starknet-types-core" {}
Length of output: 2485
crates/katana/storage/db/src/version.rs (1)
84-84: LGTM! Test updated correctly.
The test assertion has been properly updated to match the new database version.
crates/katana/primitives/src/state.rs (2)
2-5: Ohayo! The new imports look good, sensei!
The added imports are well-chosen and properly utilized in the new functionality.
90-104: Consider adding validation for the state elements, sensei!
The elements vector construction assumes all inputs are valid. Consider adding validation to ensure the total size doesn't exceed any protocol limits.
Let's check if there are any size limits defined in the codebase:
crates/saya/provider/src/rpc/mod.rs (1)
93-93: Ohayo sensei! Verify the placeholder initialization of state_diff_length.
The state_diff_length is initialized to 0, which appears to be a placeholder. Since this field is related to block commitments (as indicated by the PR title), we should ensure it's properly computed based on the state diff size.
Let's check how this field is used in other parts of the codebase:
✅ Verification successful
Ohayo sensei! The initialization of state_diff_length to 0 is correct.
The codebase shows that state_diff_length is properly computed in katana/core/src/backend/mod.rs as:
let state_diff_length = self.state_updates.len() as u32;The zero initialization in the review is just for the default/genesis block cases, which is consistent with other similar files in the codebase. This is verified by:
- Genesis block initialization in
chain_spec.rs - Default block implementation in
block.rs - Proper computation during block creation in
backend/mod.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of state_diff_length to understand the computation
rg "state_diff_length" -A 5
Length of output: 6264
crates/katana/primitives/src/receipt.rs (3)
1-11: Ohayo! The imports look good, sensei!
The new imports are well-organized and necessary for implementing the receipt hash computation functionality.
192-194: Simple and effective constructor implementation!
201-218: Please clarify the gas consumption implementation, sensei.
A few points need verification:
- Line 214 uses
Felt::ZEROfor L2 gas consumption - is this intentional or a placeholder? - Line 216 has a commented out
l1_data_gas- should this be included in the hash computation?
✅ Verification successful
Let me gather more information about the gas consumption implementation.
Let me check one more aspect about the fee structure.
Let me try one final search to understand the fee structure.
Ohayo! Here's what I found about the gas implementation, sensei!
The current implementation shows intentional design choices:
-
Using
Felt::ZEROfor L2 gas consumption is intentional as shown by the transaction handling code, where L2 gas is tracked separately throughResourceBoundsstructure. -
The commented out
l1_data_gasline should remain commented out as it's not part of the current fee computation model. The codebase consistently uses a two-part gas model with:gas_consumedfor actual consumptionl2_gasbounds for resource limits
The hash computation correctly reflects the current gas model implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for L2 gas consumption implementations in other files
rg -A 5 "l2_gas|gas_consumption"
Length of output: 11751
Script:
#!/bin/bash
# Search for receipt fee implementation
rg -A 10 "impl.*Fee" crates/katana/primitives/src/fee.rs
# Search for gas consumption related fields in receipt
rg -A 5 "gas_consumed|l1_data_gas" crates/katana/primitives/src/receipt.rs
Length of output: 424
Script:
#!/bin/bash
# Search for Fee struct definition and its fields
ast-grep --pattern 'struct Fee {
$$$
}'
# Check for any gas consumption related tests
rg -A 5 "test.*gas" crates/katana/primitives/src/
Length of output: 95
Script:
#!/bin/bash
# Search for Fee struct definition with regular grep
rg -A 10 "pub struct Fee" crates/katana/primitives/src/fee.rs
# Look for any gas-related constants
rg "const.*gas|ZERO" crates/katana/primitives/src/
Length of output: 3283
crates/katana/core/src/backend/storage.rs (2)
70-71: Implementation mirrors trait bounds correctly, sensei!
The generic implementation properly includes the new trait bounds, maintaining consistency with the trait definition.
23-23: Ohayo! The trait bounds addition looks good, sensei!
The addition of ClassTrieWriter and ContractTrieWriter traits enhances the database interface to support block commitment computations.
Let's verify that all existing implementations of the Database trait implement these new traits:
Also applies to: 47-48
✅ Verification successful
Ohayo! All implementations are properly aligned, sensei!
The verification shows that all necessary trait implementations are in place:
DbProvider<Db>implements bothClassTrieWriterandContractTrieWriterBlockchainProvider<Db>andForkedProvideralso implement both traits- The trait bounds addition is safe as all database implementations have corresponding trait implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing Database implementations
rg "impl.*Database.*for" --type rust
# Search for ClassTrieWriter implementations
rg "impl.*ClassTrieWriter.*for" --type rust
# Search for ContractTrieWriter implementations
rg "impl.*ContractTrieWriter.*for" --type rust
Length of output: 3494
crates/katana/primitives/src/chain_spec.rs (1)
56-56: Ohayo sensei! Please document the purpose of state_diff_length.
The newly added field is initialized to 0 without any explanation of its purpose or why this default value is appropriate.
Could you verify if the hardcoded 0 value is correct for all cases? Consider adding documentation to explain:
- The purpose of this field
- Why 0 is an appropriate default value
- When this value should be updated
crates/katana/core/src/backend/mod.rs (1)
180-185:
Handle Potential Errors When Computing Commitments
Ohayo, sensei! The use of .unwrap() when computing roots and commitments may cause panics if an error occurs. It's advisable to handle these potential errors gracefully to enhance robustness.
Modify the commitment computations to propagate errors:
-pub fn commit(self) -> SealedBlock {
+pub fn commit(self) -> Result<SealedBlock, SomeErrorType> {
// ...
- let transactions_commitment = self.compute_transaction_commitment();
- let events_commitment = self.compute_event_commitment();
- let receipts_commitment = self.compute_receipt_commitment();
- let state_diff_commitment = self.compute_state_diff_commitment();
+ let transactions_commitment = self.compute_transaction_commitment()?;
+ let events_commitment = self.compute_event_commitment()?;
+ let receipts_commitment = self.compute_receipt_commitment()?;
+ let state_diff_commitment = self.compute_state_diff_commitment()?;
// ...
- SealedBlock { hash, header, body: self.transactions }
+ Ok(SealedBlock { hash, header, body: self.transactions })
}Update the return types and handle errors appropriately throughout the code.
Likely invalid or redundant comment.
crates/katana/primitives/src/block.rs (4)
1-2: Ohayo sensei! Adding necessary imports enhances functionality.
The additions of cairo_short_string_to_felt and short_string are essential for the new hashing mechanism.
13-16: Ohayo sensei! The type aliases for BlockNumber and BlockHash improve code clarity.
Defining BlockNumber and BlockHash as type aliases enhances readability and maintainability of the codebase.
326-337: Ohayo sensei! Excellent work on adding comprehensive tests for concat_counts.
These tests cover various scenarios and ensure the correctness of the concat_counts function.
93-94: Ensure state_diff_length is properly initialized, sensei.
Since state_diff_length is a new field in the Header struct, please verify that all instances where Header is instantiated are updated to include this field to prevent uninitialized data issues.
Run the following script to find Header instantiations missing state_diff_length:
✅ Verification successful
These scripts will help us:
- Find direct struct instantiations using AST pattern matching
- Locate constructor method usage
- Understand the Header struct definition
- Find test files that might contain Header instantiations
This will give us a more comprehensive view of how Header is being used throughout the codebase.
All Header instantiations properly initialize state_diff_length, sensei!
After reviewing the codebase, all instances of Header struct initialization properly handle the state_diff_length field through one of these approaches:
- Explicit initialization (e.g., in
chain_spec.rs:56,rpc/mod.rs:93) - Using
Default::default()spread operator - Copying from existing headers
The field is properly handled in all key locations:
- Genesis block creation
- Block construction in the backend
- Test utilities
- RPC implementations
No uninitialized instances were found, so the implementation is safe and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all places where Header is instantiated without `state_diff_length`.
# Search for `Header {` patterns and check if `state_diff_length` is included.
rg -A20 'Header\s*\{[^}]*\}' | grep -B20 'Header\s*\{' | grep -v 'state_diff_length'
Length of output: 103
Script:
#!/bin/bash
# Let's try a more robust search approach using ast-grep to find Header struct instantiations
ast-grep --pattern 'Header {
$$$
}'
# Also search for Header::new or similar constructor patterns
rg -A 10 'Header::(new|default)'
Length of output: 14407
Script:
#!/bin/bash
# Let's also check where Header struct is defined to understand its construction patterns
rg -A 20 'struct Header'
Length of output: 1444
Script:
#!/bin/bash
# Search for test files that might contain Header instantiations
fd -e rs test | xargs rg -l 'Header'
Length of output: 36
| @@ -5,7 +5,7 @@ use std::mem; | |||
| use std::path::{Path, PathBuf}; | |||
|
|
|||
| /// Current version of the database. | |||
| pub const CURRENT_DB_VERSION: u32 = 3; | |||
| pub const CURRENT_DB_VERSION: u32 = 4; | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! The database version change needs documentation and migration strategy.
The version bump from 3 to 4 appears to be a breaking change without any documented migration path. From the code analysis:
- The version check is enforced via
check_db_version()function - No database upgrade/migration logic was found
- No changelog entries exist documenting the changes
Please:
- Add a comment explaining what changed in v4
- Document the migration strategy for existing databases
- Consider adding a CHANGELOG.md to track database version changes
Example comment format:
/// Current version of the database.
/// v4: Added block commitment storage (YYYY-MM-DD)
pub const CURRENT_DB_VERSION: u32 = 4;🔗 Analysis chain
Ohayo sensei! Please document the database version upgrade.
The database version increment from 3 to 4 is a breaking change that requires additional context. Consider:
- Adding a comment explaining what changed in v4
- Documenting the migration strategy for existing databases
- Updating the changelog if applicable
This helps maintainers and users understand the impact of the version bump and how to handle database upgrades.
Let's check for any migration-related code:
Let's check for any migration-related code in the storage module and database version changes:
Let's check for any database migration code or upgrade logic:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for migration-related code or documentation
rg -i "migration|upgrade.*database|db.*upgrade"
Length of output: 172469
Script:
#!/bin/bash
# Search for migration-related code in the storage module
rg -g "!target" -g "!*.json" "migration|upgrade.*database|db.*upgrade" crates/katana/storage/
# Check for version-related files and code
fd "version" crates/katana/storage/ --exec cat {}
Length of output: 3111
Script:
#!/bin/bash
# Search for any database upgrade or migration logic in the codebase
rg -g "!target" -g "!*.json" "fn.*upgrade.*db|migrate.*database|upgrade.*version" crates/katana/storage/
# Look for any changelog or upgrade documentation
fd "CHANGELOG|UPGRADE" crates/katana/storage/ --exec cat {}
Length of output: 170
| pub fn compute_state_diff_hash(states: StateUpdates) -> Felt { | ||
| let replaced_classes_len = states.replaced_classes.len(); | ||
| let deployed_contracts_len = states.deployed_contracts.len(); | ||
| let updated_contracts_len = Felt::from(deployed_contracts_len + replaced_classes_len); | ||
| // flatten the updated contracts into a single list of Felt values | ||
| let updated_contracts = states.deployed_contracts.into_iter().chain(states.replaced_classes); | ||
| let updated_contracts = updated_contracts.flat_map(|(addr, hash)| vec![addr.into(), hash]); | ||
|
|
||
| let declared_classes = states.declared_classes; | ||
| let declared_classes_len = Felt::from(declared_classes.len()); | ||
| let declared_classes = declared_classes.into_iter().flat_map(|e| vec![e.0, e.1]); | ||
|
|
||
| let deprecated_declared_classes = states.deprecated_declared_classes; | ||
| let deprecated_declared_classes_len = Felt::from(deprecated_declared_classes.len()); | ||
|
|
||
| let storage_updates = states.storage_updates; | ||
| let storage_updates_len = Felt::from(storage_updates.len()); | ||
| let storage_updates = storage_updates.into_iter().flat_map(|update| { | ||
| let address = Felt::from(update.0); | ||
| let storage_entries_len = Felt::from(update.1.len()); | ||
| let storage_entries = update.1.into_iter().flat_map(|entries| vec![entries.0, entries.1]); | ||
| iter::once(address).chain(iter::once(storage_entries_len)).chain(storage_entries) | ||
| }); | ||
|
|
||
| let nonce_updates = states.nonce_updates; | ||
| let nonces_len = Felt::from(nonce_updates.len()); | ||
| let nonce_updates = nonce_updates.into_iter().flat_map(|nonce| vec![nonce.0.into(), nonce.1]); | ||
|
|
||
| let magic = short_string!("STARKNET_STATE_DIFF0"); | ||
| let elements: Vec<Felt> = iter::once(magic) | ||
| .chain(iter::once(updated_contracts_len)) | ||
| .chain(updated_contracts) | ||
| .chain(iter::once(declared_classes_len)) | ||
| .chain(declared_classes) | ||
| .chain(iter::once(deprecated_declared_classes_len)) | ||
| .chain(deprecated_declared_classes) | ||
| .chain(iter::once(Felt::ONE)) | ||
| .chain(iter::once(Felt::ZERO)) | ||
| .chain(iter::once(storage_updates_len)) | ||
| .chain(storage_updates) | ||
| .chain(iter::once(nonces_len)) | ||
| .chain(nonce_updates) | ||
| .collect(); | ||
|
|
||
| hash::Poseidon::hash_array(&elements) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving readability of the state diff hash computation, sensei!
While the implementation is functionally correct, there are several opportunities for improvement:
- Extract the magic string to a named constant:
+const STATE_DIFF_MAGIC: Felt = short_string!("STARKNET_STATE_DIFF0");- Break down the complex chain operations into smaller, documented functions:
impl StateUpdates {
fn flatten_updated_contracts(&self) -> impl Iterator<Item = Felt> {
self.deployed_contracts
.iter()
.chain(&self.replaced_classes)
.flat_map(|(addr, hash)| vec![(*addr).into(), *hash])
}
fn flatten_storage_updates(&self) -> impl Iterator<Item = Felt> {
self.storage_updates.iter().flat_map(|(addr, updates)| {
let address = Felt::from(*addr);
let entries_len = Felt::from(updates.len());
let entries = updates
.iter()
.flat_map(|(k, v)| vec![*k, *v]);
iter::once(address)
.chain(iter::once(entries_len))
.chain(entries)
})
}
}This would make the main function more readable and easier to maintain.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2609 +/- ##
==========================================
+ Coverage 54.91% 55.63% +0.71%
==========================================
Files 393 393
Lines 48328 48580 +252
==========================================
+ Hits 26539 27026 +487
+ Misses 21789 21554 -235 ☔ View full report in Codecov by Sentry. |
ref #2607
resolves #1027
Summary by CodeRabbit
New Features
katana-corepackage.state_diff_lengthfield to various structures for improved block and state management.ReceiptWithTxHashstruct to associate transaction hashes with receipts.Bug Fixes
Documentation
Refactor