feat(katana): special block hash registry contract #2894
Conversation
WalkthroughOhayo, sensei! The pull request introduces changes to the block mining process in the Katana core backend. A new function Changes
Possibly related PRs
🪧 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: 1
🧹 Nitpick comments (2)
crates/katana/core/src/backend/mod.rs (2)
102-103: Implement the dedicated class for the special contractOhayo, sensei! The TODO comment suggests creating a dedicated class for the block hash registry contract. Implementing this will improve code organization and maintainability.
119-119: Define special contract address0x1as a constantOhayo, sensei! Consider defining the special contract address
0x1as a constant to improve readability and ease future maintenance.For example:
const BLOCK_HASH_REGISTRY_ADDRESS: ContractAddress = address!("0x1");Then, use
BLOCK_HASH_REGISTRY_ADDRESSin your code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/core/src/backend/mod.rs(4 hunks)
🔇 Additional comments (3)
crates/katana/core/src/backend/mod.rs (3)
14-14: Import 'address' macroOhayo, sensei! The addition of
addressto the imports is appropriate, as it's used in the updated code.
52-52: Makeexecution_outputmutableOhayo, sensei! Making
execution_outputmutable enables modification of state updates withindo_mine_block, which is necessary for the new functionality.
71-75: Callupdate_block_hash_registry_contractappropriatelyOhayo, sensei! The invocation of
update_block_hash_registry_contractwith the correct parameters enhances the block mining process by updating the block hash registry.
| .block_hash_by_num(block_number)? | ||
| .expect("qed; missing block hash"); |
There was a problem hiding this comment.
Avoid panicking on missing block hash
Ohayo, sensei! Using .expect("qed; missing block hash") may cause a panic if the block hash is missing. It's safer to handle this case by returning an error to enhance robustness.
Proposed change:
- .expect("qed; missing block hash");
+ .ok_or_else(|| BlockProductionError::MissingBlockHash(block_number))?;You'll need to define BlockProductionError::MissingBlockHash(u64) accordingly.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/katana/core/src/backend/mod.rs (1)
113-117:⚠️ Potential issueAvoid panicking on missing block hash
Ohayo again, sensei! Using
.expect("qed; missing block hash")may cause a panic if the block hash is missing.- .expect("qed; missing block hash"); + .ok_or_else(|| BlockProductionError::MissingBlockHash(block_number))?;
🧹 Nitpick comments (3)
crates/katana/core/src/backend/mod.rs (3)
102-104: Consider implementing the TODO as a separate contract classOhayo, sensei! The TODO comment suggests creating a dedicated class for this contract. This would improve modularity and make the contract's behavior more maintainable.
Would you like me to help design the contract class structure?
109-109: Extract STORED_BLOCK_HASH_BUFFER as a module-level constantConsider moving this magic number to a module-level constant with documentation explaining its significance.
+/// The number of blocks to wait before storing a block hash in the registry +const BLOCK_HASH_REGISTRY_BUFFER: u64 = 10; + fn update_block_hash_registry_contract( &self, state_updates: &mut StateUpdates, block_number: BlockNumber, ) -> Result<(), BlockProductionError> { - const STORED_BLOCK_HASH_BUFFER: u64 = 10;
119-120: Consider extracting the registry contract address as a constantThe hardcoded address
"0x1"should be extracted as a named constant for better maintainability and documentation.+/// The address of the special block hash registry contract +const BLOCK_HASH_REGISTRY_ADDRESS: ContractAddress = address!("0x1"); + - let storages = state_updates.storage_updates.entry(address!("0x1")).or_default(); + let storages = state_updates.storage_updates.entry(BLOCK_HASH_REGISTRY_ADDRESS).or_default();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/core/src/backend/mod.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/katana/core/src/backend/mod.rs (2)
6-6: LGTM! Import changes are well-aligned with new functionalityThe addition of
BlockNumber,ContractAddress, andFelttypes enhances type safety for the block hash registry implementation.Also applies to: 14-14
52-52: LGTM! Block mining flow properly integrates block hash registry updatesThe changes correctly integrate the special block hash registry updates into the block mining process, maintaining proper error handling.
Also applies to: 71-76
kariy
force-pushed
the
katana/block-hash-registry
branch
from
061e6b7 to
775ddf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/core/src/backend/mod.rs (2)
110-110: Document the STORED_BLOCK_HASH_BUFFER constantThe buffer size of 10 blocks should be documented with an explanation of its significance and any network-specific requirements.
+ /// Number of blocks to wait before storing a block hash in the registry. + /// This buffer ensures finality before recording block hashes. const STORED_BLOCK_HASH_BUFFER: u64 = 10;
105-130: Add comprehensive function documentationOhayo, sensei! This critical function lacks documentation explaining its purpose, behavior, and relationship to the StarkNet state architecture.
Add this documentation:
+ /// Updates the special block hash registry contract at address 0x1. + /// + /// This function implements the block hash storage mechanism as described in + /// https://docs.starknet.io/architecture-and-concepts/network-architecture/starknet-state/#address_0x1 + /// + /// # Arguments + /// * `state_updates` - Mutable reference to state updates where the block hash will be stored + /// * `block_number` - The number of the current block being processed + /// + /// # Returns + /// * `Ok(())` if the update was successful + /// * `Err(BlockProductionError)` if there was an error accessing block history fn update_block_hash_registry_contract(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/katana/core/src/backend/mod.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (5)
crates/katana/core/src/backend/mod.rs (5)
6-7: LGTM! Imports are well-organizedOhayo, sensei! The new imports are correctly added to support the block hash registry functionality.
Also applies to: 15-15
53-53: LGTM! Block hash registry update is properly integratedOhayo, sensei! The changes to
do_mine_blockare well-structured:
- Mutability change allows for state updates modification
- Block hash registry update is performed at the correct point in the block mining process
Also applies to: 72-77
103-104: Consider implementing a dedicated contract classOhayo, sensei! The TODO comment suggests this functionality should be moved to a dedicated class. This would improve modularity and testability.
Would you like me to help create a dedicated contract class for this functionality?
105-130: Add tests for block hash registry updatesOhayo, sensei! The block hash registry functionality needs comprehensive tests to verify:
- Correct storage of block hashes after the buffer period
- Handling of missing block hashes
- Integration with the block mining process
Would you like me to help create a test suite for this functionality?
116-124: Review the BlockHash::ZERO fallback mechanismThe current implementation falls back to BlockHash::ZERO for missing block hashes, which needs careful consideration:
- This is marked as a temporary solution (TODO: Fix quick!)
- Could potentially cause issues if these zero hashes are relied upon by other contracts
Run this script to check for potential consumers of these block hashes:
✅ Verification successful
Ohayo sensei! Here's my analysis of the BlockHash::ZERO fallback mechanism:
BlockHash::ZERO fallback is safe for temporary use
The fallback mechanism is used only for system-level chain organization and doesn't impact contract execution or state validity. No contract-level dependencies on specific block hash values were found in the codebase.
However, implementing proper historical block hash retrieval should be prioritized as indicated by the TODO comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to contract address 0x1 and block hash usage rg -A 5 "0x1|BlockHash::ZERO" # Search for storage reads from this contract ast-grep --pattern 'storage_read($contract, $_)'Length of output: 68859
Script:
#!/bin/bash # Search for block hash related function calls and storage patterns rg -A 5 "get_block_hash|block_hash|BlockHash" --type rust # Search for block hash validation patterns ast-grep --pattern 'if $block_hash == $_' # Search for block hash storage patterns ast-grep --pattern 'storage.get_block_hash($_, $_)'Length of output: 130783
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2894 +/- ##
==========================================
+ Coverage 55.83% 55.85% +0.01%
==========================================
Files 449 449
Lines 57700 57730 +30
==========================================
+ Hits 32219 32246 +27
- Misses 25481 25484 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/katana/core/src/backend/mod.rs(4 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
283-288: LGTM! Special handling for system contract.Ohayo, sensei! The implementation correctly handles the special system contract at address
0x1by returningClassHash::ZERO. This aligns with the Starknet architecture documentation.
314-321: LGTM! Consistent special handling.The implementation correctly skips the class hash check for the special system contract at address
0x1, maintaining consistency with the previous handling.crates/katana/core/src/backend/mod.rs (2)
53-53: LGTM! Mutable parameter for state updates.The change to make
execution_outputmutable is necessary to support the block hash registry updates.
72-77: LGTM! Block hash registry integration.The integration of
update_block_hash_registry_contractis placed correctly after transaction processing but before block commitment.
| // TODO: create a dedicated struct for this contract. | ||
| // https://docs.starknet.io/architecture-and-concepts/network-architecture/starknet-state/#address_0x1 | ||
| fn update_block_hash_registry_contract( | ||
| &self, | ||
| state_updates: &mut StateUpdates, | ||
| block_number: BlockNumber, | ||
| ) -> Result<(), BlockProductionError> { | ||
| const STORED_BLOCK_HASH_BUFFER: u64 = 10; | ||
|
|
||
| if block_number >= STORED_BLOCK_HASH_BUFFER { | ||
| let block_number = block_number - STORED_BLOCK_HASH_BUFFER; | ||
| let block_hash = self.blockchain.provider().block_hash_by_num(block_number)?; | ||
|
|
||
| // When in forked mode, we might not have the older block hash in the database. This | ||
| // could be the case where the `block_number - STORED_BLOCK_HASH_BUFFER` is | ||
| // earlier than the forked block, which right now, Katana doesn't | ||
| // yet have the ability to fetch older blocks on the database level. So, we default to | ||
| // `BlockHash::ZERO` in this case. | ||
| // | ||
| // TODO: Fix quick! | ||
| let block_hash = block_hash.unwrap_or(BlockHash::ZERO); | ||
|
|
||
| let storages = state_updates.storage_updates.entry(address!("0x1")).or_default(); | ||
| storages.insert(block_number.into(), block_hash); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling improvements.
Ohayo, sensei! While the implementation is generally good, there are a few suggestions:
- The unwrap on line 123 could be handled more gracefully.
- The TODO comment on line 122 should be more descriptive.
Apply this diff to improve error handling:
- // TODO: Fix quick!
- let block_hash = block_hash.unwrap_or(BlockHash::ZERO);
+ // TODO: Implement historical block fetching for forked mode to avoid defaulting to ZERO
+ let block_hash = block_hash.unwrap_or_else(|| {
+ tracing::warn!(
+ target: LOG_TARGET,
+ block_number = %block_number,
+ "Block hash not found for historical block in forked mode, defaulting to ZERO"
+ );
+ BlockHash::ZERO
+ });Additionally, consider adding a constant for the special contract address:
+ /// Special system contract address for block hash registry
+ const BLOCK_HASH_REGISTRY_ADDRESS: ContractAddress = address!("0x1");
+
fn update_block_hash_registry_contract(Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/storage/provider/tests/fixtures.rs (1)
72-73: Ohayo, sensei! LGTM! Source of test fixture updates.The contract addresses have been updated to use more distinctive test values that are commonly used in testing scenarios.
Consider adding a comment explaining why these specific numbers were chosen, as they are common "leet speak" testing values:
+// Using distinctive calculator-style numbers for better test readability let address_1 = address!("1337"); // "LEET" let address_2 = address!("80085"); // "BOOBS"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/katana/storage/provider/tests/contract.rs(2 hunks)crates/katana/storage/provider/tests/fixtures.rs(1 hunks)crates/katana/storage/provider/tests/storage.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/katana/storage/provider/tests/contract.rs (2)
46-47: LGTM! Test vectors updated consistently.The contract addresses have been updated to use the new test fixtures consistently.
98-122: LGTM! Historical test cases aligned with the new contract addresses.The historical test cases have been properly updated to maintain consistency with the new contract addresses and their expected states across different blocks.
crates/katana/storage/provider/tests/storage.rs (2)
42-46: LGTM! Latest storage test vectors updated consistently.The storage test vectors have been properly updated to use the new contract addresses while maintaining the same logical test patterns.
98-129: LGTM! Historical storage test vectors properly aligned.The historical storage test cases have been systematically updated across all blocks (0, 1, 4, 5) while preserving the original test logic and progression of storage values.
https://docs.starknet.io/architecture-and-concepts/network-architecture/starknet-state/#address_0x1
Summary by CodeRabbit
New Features
Improvements
0x1contract address in API methods to improve error handling and control flow.Tests