feat(katana): commitment fields in block header #2560
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes across multiple files, primarily focusing on the restructuring of block-related types and methods. Key enhancements include the addition of a 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: 7
🧹 Outside diff range and nitpick comments (9)
crates/katana/executor/tests/fixtures/mod.rs (2)
106-106: Ohayo again, sensei! Consistent rename, but let's consider an optimization.The rename to
protocol_versionis consistent with the earlier change, which is great for maintainability. However, sinceprotocol_versionis likely an immutable value, we might not need to clone it for each block.Consider removing the
.clone()call ifprotocol_versionis an immutable type like&stror a simple integer. If it's a more complex type that requires ownership, then the current implementation is correct.
156-156: Ohayo once more, sensei! Consistent renaming, but let's address a small inconsistency.Great job on consistently renaming
versiontoprotocol_versionacross all blocks. This improves the overall clarity of the code. However, I noticed a small inconsistency:
- In the second block (line 156), we're using
protocol_version.clone()- In the third block (line 189), we're using
protocol_versiondirectlyLet's make this consistent across all blocks. If cloning is necessary, use it for all blocks. If not, remove it from all blocks. This will depend on whether
protocol_versionneeds to be an owned value in thePartialHeaderstruct or if it can be a reference.Also applies to: 189-189
crates/katana/rpc/rpc/src/starknet/read.rs (2)
93-94: Ohayo, sensei! LGTM with a small suggestion.The renaming of
versiontoprotocol_versionimproves clarity. Great job!Consider adding a brief comment explaining the significance of the protocol version, e.g.:
// The protocol version determines the set of supported features and behaviors protocol_version: this.inner.backend.chain_spec.version.clone(),
243-245: Ohayo once more, sensei! Great consistency, with a tiny suggestion.The renaming of
versiontoprotocol_versionis consistent with the previous changes. Excellent work maintaining uniformity!For even better consistency, consider keeping the field order the same as in the previous occurrences. This doesn't affect functionality but enhances readability:
- protocol_version: this.inner.backend.chain_spec.version.clone(), timestamp: block_env.timestamp, sequencer_address: block_env.sequencer_address, + protocol_version: this.inner.backend.chain_spec.version.clone(),crates/katana/core/src/backend/mod.rs (2)
39-39: Ohayo sensei! Remember to address the TODO commentThe
do_mine_blockfunction has a// TODO: add test for this functioncomment. Adding tests will help ensure the reliability and correctness of this critical function.Would you like assistance in creating unit tests for
do_mine_block?
101-107: Ohayo sensei! Consider adding tests forcommit_blockThe new
commit_blockfunction is pivotal in the block creation process. Writing unit tests for this method will ensure its correctness and prevent future regressions.Do you need assistance in writing tests for
commit_block?crates/katana/primitives/src/block.rs (2)
Line range hint
122-130: Updatecompute_hashto Use Actual Field ValuesOhayo, sensei! In the
compute_hashmethod, usingFelt::ZEROfor fields likestate_root,transactions_commitment, and others may lead to incorrect block hashes. It's important to use the actual values of these fields to accurately compute the block hash.Consider modifying the method as follows:
compute_hash_on_elements(&vec![ self.number.into(), // block number - Felt::ZERO, // state root + self.state_root, // state root self.sequencer_address.into(), // sequencer address self.timestamp.into(), // block timestamp - Felt::ZERO, // transaction commitment + self.transactions_commitment, // transaction commitment - Felt::ZERO, // event commitment + self.events_commitment, // event commitment - Felt::ZERO, // protocol version + self.protocol_version.into(), // protocol version Felt::ZERO, // extra data self.parent_hash, // parent hash ])
171-178: Update Documentation forSealedBlockStructOhayo, sensei! The
SealedBlockstruct has been modified, and it now includes ahashfield and no longer nestsSealedHeader. Please update the documentation comments to reflect these changes for clarity.Ensure that the comments provide accurate information about the struct's current state.
crates/katana/core/src/service/block_producer.rs (1)
Line range hint
600-601: Ohayo, sensei! Address the TODO for updating block environment creation.There's a TODO comment indicating that we should create a new block environment based on the current state of L1 to determine the proper gas prices. Implementing this enhancement would improve the accuracy of gas pricing in block production.
Would you like assistance in implementing this feature, or should we open a new GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
- crates/katana/core/src/backend/mod.rs (4 hunks)
- crates/katana/core/src/backend/storage.rs (1 hunks)
- crates/katana/core/src/service/block_producer.rs (1 hunks)
- crates/katana/executor/tests/fixtures/mod.rs (4 hunks)
- crates/katana/primitives/src/block.rs (4 hunks)
- crates/katana/primitives/src/chain_spec.rs (4 hunks)
- crates/katana/rpc/rpc-types/src/block.rs (3 hunks)
- crates/katana/rpc/rpc/src/starknet/read.rs (3 hunks)
- crates/katana/storage/provider/src/providers/db/mod.rs (3 hunks)
- crates/katana/storage/provider/src/providers/fork/mod.rs (1 hunks)
- crates/katana/storage/provider/src/providers/in_memory/mod.rs (1 hunks)
- crates/katana/storage/provider/tests/block.rs (4 hunks)
- crates/katana/storage/provider/tests/fixtures.rs (2 hunks)
- crates/katana/storage/provider/tests/utils.rs (2 hunks)
- crates/saya/core/src/lib.rs (4 hunks)
- crates/saya/provider/src/rpc/mod.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (37)
crates/katana/storage/provider/tests/utils.rs (2)
71-71: Ohayo again, sensei! Consistent change spotted!The modification here mirrors the one in the
generate_dummy_blocks_and_receiptsfunction, changing fromblock.header.hashtoblock.hash. This consistency is excellent and helps maintain a uniform approach to accessing block hashes across the codebase.
49-49: Ohayo, sensei! The block hash assignment looks good!The change from
block.header.hashtoblock.hashaligns with the updated block structure. This modification simplifies access to the block hash and improves code readability.To ensure consistency across the codebase, let's verify the
Blockstruct's interface:✅ Verification successful
Ohayo, sensei! Verified the hash assignment change!
The shift from
block.header.hashtoblock.hashis consistently implemented across the codebase and aligns perfectly with the updatedBlockstruct. Everything looks good!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Block struct's interface # Test: Search for the Block struct definition rg --type rust -A 10 'struct Block' # Test: Search for usages of block.hash rg --type rust 'block\.hash'Length of output: 15791
crates/katana/storage/provider/tests/fixtures.rs (1)
191-192: Ohayo, sensei! Simplified SealedBlock structure looks sharp!The removal of the
SealedHeaderwrapper and direct inclusion ofhashandheaderfields inSealedBlockis a clean improvement. This change streamlines the block representation, making it more straightforward and easier to work with. Nice job on reducing unnecessary complexity!crates/katana/rpc/rpc-types/src/block.rs (3)
156-159: Ohayo again, sensei! This change looks good too.The modification from
header.versiontoheader.protocol_versionis consistent with the previous struct and aligns with the PR objectives. No other significant changes are observed here.
271-271: Ohayo once more, sensei! This change completes the trifecta.The modification from
header.versiontoheader.protocol_versionis consistent across all three structs (PendingBlockWithTxs,PendingBlockWithTxHashes, andPendingBlockWithReceipts). This consistency is excellent and aligns perfectly with the PR objectives.
Line range hint
75-82: Ohayo, sensei! LGTM with a small suggestion.The change from
header.versiontoheader.protocol_versionlooks good and aligns with the PR objectives. However, I noticed thatl1_data_gas_priceis initialized with default values. Is this intentional?Let's make sure this default initialization is correct:
crates/katana/executor/tests/fixtures/mod.rs (2)
92-92: Ohayo, sensei! Nice rename for clarity!The change from
versiontoprotocol_versionis a great improvement. It makes the code more explicit and easier to understand. UsingCURRENT_STARKNET_VERSIONensures that our test fixtures are always up-to-date with the latest protocol version. Domo arigato for this enhancement!
Line range hint
1-289: Ohayo, sensei! Great work on improving the test fixtures!Overall, the changes in this file are positive and improve the code's clarity. Here's a summary of the key points:
- Consistently renamed
versiontoprotocol_version, which makes the code more explicit and easier to understand.- Used
CURRENT_STARKNET_VERSIONto ensure test fixtures are up-to-date.To further improve the code:
- Decide whether
protocol_versionneeds to be cloned for each block or if it can be shared.- Make the usage of
clone()consistent across all blocks based on the decision above.These small tweaks will make the test fixtures even more robust and maintainable. Domo arigato for your attention to detail, sensei!
crates/katana/storage/provider/tests/block.rs (5)
103-104: Ohayo, sensei! These changes look sharp!The updates to block header field references simplify the code and align with the new
Blockstructure. This improves readability and maintainability.
120-120: Excellent update, sensei!The modification to
block_idconstruction usingblock.block.hashensures consistency in block identifier references and aligns with the newBlockstructure.
122-123: Ohayo! These changes are on point, sensei!The updates to expected block number and hash variables correctly reflect the new
Blockstructure. This ensures that our test cases are using the right fields for comparisons, maintaining the accuracy of our tests.
213-214: Ohayo, sensei! Consistency is key, and you've nailed it!These changes in the
insert_block_empty_test_implfunction maintain consistency with the earlier updates. They ensure that the newBlockstructure is correctly used across different test scenarios, which is crucial for comprehensive testing.Also applies to: 226-229
Line range hint
1-368: Ohayo, sensei! Let's wrap this up with a bow!The changes in this file consistently update block header field references to align with the new
Blockstructure. These modifications enhance code clarity and maintain the accuracy of our test implementations. Great job on keeping everything consistent and up-to-date!crates/katana/storage/provider/src/providers/in_memory/mod.rs (1)
466-469: Ohayo, sensei! Streamlined block attribute access looks good!The changes to accessing block hash, number, and header directly from the block structure improve code clarity and reduce nesting. This is a positive change that should make the code more readable and less error-prone.
To ensure consistency across the codebase, let's verify if similar changes are needed elsewhere:
If the script returns results, consider updating those instances for consistency.
crates/katana/storage/provider/src/providers/fork/mod.rs (1)
472-475: Ohayo, sensei! Simplified block data access looks great!The changes to accessing block hash, number, and header are a welcome improvement. By removing unnecessary levels of indirection, you've made the code more readable and potentially more efficient. Great job on streamlining the data structure access!
crates/katana/rpc/rpc/src/starknet/read.rs (2)
185-186: Ohayo again, sensei! Consistency is key, and you've nailed it!The renaming of
versiontoprotocol_versionis consistent with the previous change, maintaining uniformity across different methods. Well done!
Line range hint
1-624: Ohayo, sensei! Let's wrap up this review.The changes in this file consistently rename
versiontoprotocol_versionacross different methods. This improves clarity without introducing any functional changes. Great job maintaining consistency throughout the file!A few minor suggestions were made for even better readability and documentation. Overall, the changes look solid and well-implemented.
crates/saya/core/src/lib.rs (4)
154-154: Ohayo, sensei! Nice simplification of state root access.The change from
block_before_the_first?.header.header.state_roottoblock_before_the_first?.header.state_rootimproves code readability by removing an unnecessary level of indirection. This aligns well with the overall goal of simplifying block-related access patterns.
296-296: Ohayo again, sensei! Excellent work on maintaining consistency.The simplification of
block.header.header.state_roottoblock.header.state_rootin theprefetch_blocksmethod is consistent with the earlier change. This modification enhances code readability and maintains a uniform approach to accessing block header fields throughout the codebase.
330-330: Ohayo once more, sensei! Kudos for maintaining consistency across the codebase.The simplification of
block.header.header.numbertoblock.header.numberin theFetchedBlockInfostruct initialization is in line with the previous changes. This modification further enhances code readability and ensures a consistent approach to accessing block header fields throughout the file.
403-403: Ohayo, sensei! Nice simplification, but let's double-check this one.The change from
block.block.header.hashtoblock.block.hashsimplifies the access to the block hash, which is great for consistency. However, this pattern differs slightly from the previous header field access simplifications.Could you please verify that
block.block.hashis indeed the correct way to access the block hash in the new structure? This will ensure that we're maintaining both simplicity and correctness.✅ Verification successful
Ohayo, sensei! The update to
block.block.hashis consistent with existing codebase usage. Approved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct access pattern for block hash rg --type rust 'block\.block\.hash' crates/Length of output: 1543
crates/katana/storage/provider/src/providers/db/mod.rs (2)
609-612: Ohayo, sensei! Nice simplification of block property access.The changes here improve the code's readability by directly accessing the block hash, number, and header from the block structure. This approach is more straightforward and reduces the chance of errors. Good job!
885-885: Ohayo again, sensei! Test cases updated correctly.The test cases have been properly updated to reflect the changes in how block properties are accessed. This ensures that the tests remain valid and continue to verify the correct functionality of the code. Well done on maintaining consistency!
Also applies to: 924-926
crates/katana/core/src/backend/mod.rs (6)
5-5: Ohayo sensei! New imports are appropriately addedThe added imports for
Block,FinalityStatus,GasPrices,Header,SealedBlock, andSealedBlockWithStatusare necessary for the new block commitment functionality.
10-11: Ohayo sensei! Necessary imports forReceiptandTxWithHashImporting
ReceiptandTxWithHashensures that the code has access to the required types for handling transactions and receipts.
62-65: Ohayo sensei! Good refactoring withcommit_blockmethodExtracting the block commitment logic into the
commit_blockmethod enhances modularity and readability. This separation of concerns makes the codebase more maintainable.
74-74: Ohayo sensei! Logging statement is clear and informativeThe logging of
block_numberandtx_countwith the message "Block mined." provides valuable information for monitoring the block creation process.
109-109: Ohayo sensei! Handling potential errors when retrievingparent_hashRetrieving the latest block hash with
self.blockchain.provider().latest_hash()?is good, but ensure that any potential errors are appropriately handled or propagated.Do all calling functions handle
BlockProductionErrorcorrectly in caselatest_hash()fails?
126-133:⚠️ Potential issueOhayo sensei! Ensure correct computation of commitment fields
In the
Header, fields likestate_root,events_commitment,receipts_commitment,state_diff_commitment, andtransactions_commitmentare currently set toFelt::ZERO. These fields should reflect the actual state and data of the block.It's important to compute and assign the correct values to these commitment fields to maintain blockchain integrity. Do you need assistance in implementing these computations?
crates/katana/primitives/src/block.rs (2)
152-153: Confirm Correct Block Hash Computation insealMethodOhayo, sensei! When sealing the block in the
sealmethod, ensure that thecompute_hashfunction accurately reflects all relevant header fields after the updates.After updating the
compute_hashmethod, verify that the block hash is correctly computed during the sealing process.
185-185: VerifyunsealMethod Functionality After ChangesOhayo, sensei! With the updates to the
SealedBlockstruct, please confirm that theunsealmethod still functions correctly and returns a validBlockinstance.It's important to ensure that the method adapts appropriately to the structural changes made.
crates/katana/core/src/backend/storage.rs (1)
363-365: LGTM!Ohayo, sensei! The added assertions correctly verify that the
block_hashandblock_numbermatch the expected values.crates/katana/core/src/service/block_producer.rs (1)
584-584: Ohayo, sensei! Ensure consistent use ofprotocol_version.The field
versioninPartialHeaderhas been renamed toprotocol_versionon line 584. Please verify that all references to this field throughout the codebase have been updated accordingly to prevent any inconsistencies or potential errors.You can run the following script to find any remaining instances of
PartialHeaderusingversion:✅ Verification successful
Ohayo, sensei! No remaining instances of
PartialHeaderusingversionwere found. To ensure everything is perfectly consistent, I'll run an additional check below.
Ohayo, sensei! All instances of
PartialHeaderusingversionhave been successfully updated toprotocol_version. No inconsistencies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of 'PartialHeader' where 'version' is still used. rg --type rust -A 5 -B 5 'PartialHeader.*version'Length of output: 51
Script:
#!/bin/bash # Description: Search for any remaining instances of 'PartialHeader' using 'protocol_version'. rg --type rust 'PartialHeader.*protocol_version'Length of output: 50
crates/katana/primitives/src/chain_spec.rs (4)
58-63: Initializing New Header Fields with Default ValuesOhayo, sensei! The addition of the new header fields (
events_count,transaction_count,events_commitment,receipts_commitment,state_diff_commitment,transactions_commitment) initialized to zero ensures that the genesis block starts with the correct default state. This is a good practice to prevent any unintended behavior due to uninitialized fields.
349-352: Including New Header Fields in Test Block InitializationOhayo, sensei! By initializing the new header fields (
events_commitment,receipts_commitment,state_diff_commitment,transactions_commitment) in theexpected_blockwithin the tests, you're ensuring that the tests accurately reflect the updatedHeaderstruct. This helps maintain test reliability as the codebase evolves.
362-363: Initializingtransaction_countandevents_countin Test HeaderOhayo, sensei! Adding
transaction_countandevents_countwith zero values in the test block header aligns the test data with the updatedHeaderstructure. This ensures that all header fields are properly accounted for during testing.
384-385: Adding Assertions for New Header Fields in TestsOhayo, sensei! Including assertions for
transaction_countandevents_countstrengthens the test suite by verifying that these fields are correctly initialized in theactual_block. This is essential for catching any future regressions related to these fields.
| block.l1_gas_price.price_in_wei.to_u128().unwrap(), | ||
| block.l1_gas_price.price_in_fri.to_u128().unwrap(), |
There was a problem hiding this comment.
Handle potential errors when converting gas prices
Ohayo, sensei! Using unwrap() on the result of to_u128() can cause a panic if the conversion fails. It's safer to handle the Option properly to avoid potential runtime errors.
Consider handling the conversion errors as follows:
- block.l1_gas_price.price_in_wei.to_u128().unwrap(),
- block.l1_gas_price.price_in_fri.to_u128().unwrap(),
+ block.l1_gas_price.price_in_wei.to_u128().ok_or_else(|| anyhow!("Conversion to u128 failed for l1_gas_price.price_in_wei"))?,
+ block.l1_gas_price.price_in_fri.to_u128().ok_or_else(|| anyhow!("Conversion to u128 failed for l1_gas_price.price_in_fri"))?,Similarly for l1_data_gas_prices:
- block.l1_data_gas_price.price_in_wei.to_u128().unwrap(),
- block.l1_data_gas_price.price_in_fri.to_u128().unwrap(),
+ block.l1_data_gas_price.price_in_wei.to_u128().ok_or_else(|| anyhow!("Conversion to u128 failed for l1_data_gas_price.price_in_wei"))?,
+ block.l1_data_gas_price.price_in_fri.to_u128().ok_or_else(|| anyhow!("Conversion to u128 failed for l1_data_gas_price.price_in_fri"))?,Also applies to: 104-105
| events_count: 0, | ||
| transaction_count: 0, | ||
| events_commitment: Felt::ZERO, | ||
| receipts_commitment: Felt::ZERO, | ||
| state_diff_commitment: Felt::ZERO, | ||
| transactions_commitment: Felt::ZERO, |
There was a problem hiding this comment.
Initialize header fields with actual block data
Ohayo, sensei! The events_count, transaction_count, and commitment fields (events_commitment, receipts_commitment, state_diff_commitment, transactions_commitment) are currently initialized with default values (0 or Felt::ZERO). Consider populating these fields with actual values from the block data to ensure accurate header information.
Apply this diff to initialize the fields with actual data:
- events_count: 0,
- transaction_count: 0,
- events_commitment: Felt::ZERO,
- receipts_commitment: Felt::ZERO,
- state_diff_commitment: Felt::ZERO,
- transactions_commitment: Felt::ZERO,
+ events_count: block.events.len(),
+ transaction_count: block.transactions.len(),
+ events_commitment: block.event_commitment,
+ receipts_commitment: block.receipt_commitment,
+ state_diff_commitment: block.state_diff_commitment,
+ transactions_commitment: block.transaction_commitment,📝 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.
| events_count: 0, | |
| transaction_count: 0, | |
| events_commitment: Felt::ZERO, | |
| receipts_commitment: Felt::ZERO, | |
| state_diff_commitment: Felt::ZERO, | |
| transactions_commitment: Felt::ZERO, | |
| events_count: block.events.len(), | |
| transaction_count: block.transactions.len(), | |
| events_commitment: block.event_commitment, | |
| receipts_commitment: block.receipt_commitment, | |
| state_diff_commitment: block.state_diff_commitment, | |
| transactions_commitment: block.transaction_commitment, |
| timestamp: block.timestamp, | ||
| state_root: block.new_root, | ||
| sequencer_address: block.sequencer_address.into(), | ||
| protocol_version: ProtocolVersion::parse(&block.starknet_version).unwrap(), |
There was a problem hiding this comment.
Handle potential errors when parsing protocol version
Ohayo, sensei! Using unwrap() on ProtocolVersion::parse(&block.starknet_version) can cause a panic if parsing fails. Consider handling the potential error to avoid runtime panics.
Modify the code as follows:
- protocol_version: ProtocolVersion::parse(&block.starknet_version).unwrap(),
+ protocol_version: ProtocolVersion::parse(&block.starknet_version)?,Ensure that the method signature supports returning the error appropriately.
Committable suggestion was skipped due to low confidence.
| events_count: 0, | ||
| transaction_count: 0, | ||
| state_root: Felt::ZERO, | ||
| events_commitment: Felt::ZERO, | ||
| number: BlockNumber::default(), | ||
| state_root: Felt::default(), | ||
| receipts_commitment: Felt::ZERO, | ||
| state_diff_commitment: Felt::ZERO, | ||
| parent_hash: BlockHash::default(), | ||
| l1_gas_prices: GasPrices::default(), | ||
| protocol_version: ProtocolVersion::default(), | ||
| sequencer_address: ContractAddress::default(), | ||
| transactions_commitment: Felt::ZERO, | ||
| l1_data_gas_prices: GasPrices::default(), | ||
| sequencer_address: ContractAddress::default(), | ||
| l1_da_mode: L1DataAvailabilityMode::Calldata, | ||
| protocol_version: ProtocolVersion::default(), |
There was a problem hiding this comment.
Ensure Appropriate Default Values for New Fields
Ohayo, sensei! In the Default implementation for Header, the new fields are initialized with default values. Please confirm that these defaults align with the expected initial state to prevent any unintended behavior.
If necessary, adjust the default values to match the intended use cases of these fields.
| /// Represents a pending block header. | ||
| #[derive(Debug, Clone)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| pub struct PartialHeader { | ||
| pub parent_hash: BlockHash, | ||
| pub number: BlockNumber, | ||
| pub parent_hash: Felt, | ||
| pub timestamp: u64, | ||
| pub sequencer_address: ContractAddress, | ||
| pub version: ProtocolVersion, | ||
| pub l1_gas_prices: GasPrices, | ||
| pub l1_data_gas_prices: GasPrices, | ||
| pub l1_da_mode: L1DataAvailabilityMode, | ||
| pub protocol_version: ProtocolVersion, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Rename PartialHeader to PendingHeader for Consistency
Ohayo, sensei! The documentation comment at line 41 refers to a "pending block header," but the struct is still named PartialHeader. To maintain consistency and clarity, consider renaming the struct to PendingHeader.
Apply this diff to rename the struct:
/// Represents a pending block header.
-pub struct PartialHeader {
+pub struct PendingHeader {
pub parent_hash: BlockHash,
pub number: BlockNumber,
pub timestamp: u64,
pub sequencer_address: ContractAddress,
pub l1_gas_prices: GasPrices,
pub l1_data_gas_prices: GasPrices,
pub l1_da_mode: L1DataAvailabilityMode,
pub protocol_version: ProtocolVersion,
}📝 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.
| /// Represents a pending block header. | |
| #[derive(Debug, Clone)] | |
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |
| pub struct PartialHeader { | |
| pub parent_hash: BlockHash, | |
| pub number: BlockNumber, | |
| pub parent_hash: Felt, | |
| pub timestamp: u64, | |
| pub sequencer_address: ContractAddress, | |
| pub version: ProtocolVersion, | |
| pub l1_gas_prices: GasPrices, | |
| pub l1_data_gas_prices: GasPrices, | |
| pub l1_da_mode: L1DataAvailabilityMode, | |
| pub protocol_version: ProtocolVersion, | |
| /// Represents a pending block header. | |
| #[derive(Debug, Clone)] | |
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |
| pub struct PendingHeader { | |
| pub parent_hash: BlockHash, | |
| pub number: BlockNumber, | |
| pub timestamp: u64, | |
| pub sequencer_address: ContractAddress, | |
| pub l1_gas_prices: GasPrices, | |
| pub l1_data_gas_prices: GasPrices, | |
| pub l1_da_mode: L1DataAvailabilityMode, | |
| pub protocol_version: ProtocolVersion, |
| pub state_diff_commitment: Felt, | ||
| pub transactions_commitment: Felt, | ||
| pub receipts_commitment: Felt, | ||
| pub events_commitment: Felt, | ||
| pub state_root: Felt, | ||
| pub timestamp: u64, | ||
| pub transaction_count: u32, | ||
| pub events_count: u32, |
There was a problem hiding this comment.
💡 Codebase verification
🚨 Issues Found: Missing Initialization of New Header Fields
Ohayo, sensei! It looks like there are several instances where the new fields in the Header struct aren't being initialized properly. Please review and update the following locations to include all required fields:
crates/saya/provider/src/rpc/mod.rs: header: Header {crates/katana/storage/provider/tests/utils.rs: let header = Header { parent_hash, number: i, ..Default::default() };crates/katana/storage/provider/tests/fixtures.rs: header: Header { number: i, ..Default::default() },crates/katana/storage/provider/src/providers/db/mod.rs: let header = Header { parent_hash: 199u8.into(), number: 0, ..Default::default() };crates/katana/core/src/backend/mod.rs: let header = Header { parent_hash, l1_gas_prices, l1_data_gas_prices, state_root: Felt::ZERO, number: block_env.number, timestamp: block_env.timestamp, l1_da_mode: L1DataAvailabilityMode::Calldata, sequencer_address: block_env.sequencer_address, protocol_version: self.chain_spec.version.clone(), };crates/katana/core/src/backend/storage.rs: header: Header { parent_hash: Felt::ZERO, number: 1, l1_gas_prices: GasPrices::default(), l1_data_gas_prices: GasPrices::default(), l1_da_mode: L1DataAvailabilityMode::Calldata, timestamp: 123456, ..Default::default() }crates/katana/executor/tests/fixtures/mod.rs: header: PartialHeader { protocol_version: protocol_version.clone(), number: 1, timestamp: 100, sequencer_address, parent_hash: 123u64.into(), l1_gas_prices: gas_prices.clone(), l1_data_gas_prices: gas_prices.clone(), l1_da_mode: L1DataAvailabilityMode::Calldata, },crates/katana/executor/tests/fixtures/mod.rs: header: PartialHeader { protocol_version: protocol_version.clone(), number: 2, timestamp: 200, sequencer_address, parent_hash: 1234u64.into(), l1_gas_prices: gas_prices.clone(), l1_data_gas_prices: gas_prices.clone(), l1_da_mode: L1DataAvailabilityMode::Calldata, },crates/katana/executor/tests/fixtures/mod.rs: header: PartialHeader { protocol_version: protocol_version, number: 3, timestamp: 300, sequencer_address, parent_hash: 12345u64.into(), l1_gas_prices: gas_prices.clone(), l1_data_gas_prices: gas_prices.clone(), l1_da_mode: L1DataAvailabilityMode::Calldata, },
Please ensure that all Header initializations include the new fields to maintain consistency and avoid potential runtime issues.
🔗 Analysis chain
Verify Initialization of New Fields in Header
Ohayo, sensei! The added fields in the Header struct enhance the block header's data representation. Please ensure that these new fields are properly initialized and computed throughout the codebase where Header is used.
To check for proper initialization and usage of the new fields, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all instances of `Header` initialization include the new fields.
# Search for `Header` initializations that might be missing the new fields.
rg --type rust 'Header\s*\{[^}]*\}' -U --pcre2 -r '$0' \
| grep -v 'state_diff_commitment' \
| grep -v 'transactions_commitment' \
| grep -v 'receipts_commitment' \
| grep -v 'events_commitment' \
| grep -v 'transaction_count' \
| grep -v 'events_count'
Length of output: 22799
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/storage/db/src/version.rs (1)
84-84: Ohayo again, sensei! The test case update is on point!The test case has been correctly updated to assert that
CURRENT_DB_VERSIONequals 3, which is consistent with the constant change. This is crucial for maintaining the integrity of our version tracking.A small suggestion to improve clarity:
Consider updating the error message to include the actual version:
- assert_eq!(CURRENT_DB_VERSION, 3, "Invalid current database version") + assert_eq!(CURRENT_DB_VERSION, 3, "Invalid current database version. Expected 3, got {}", CURRENT_DB_VERSION)This way, if the version changes in the future, the test failure message will be more informative.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
- crates/katana/primitives/src/block.rs (4 hunks)
- crates/katana/storage/db/src/version.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
crates/katana/storage/db/src/version.rs (2)
8-8: Ohayo, sensei! The database version increment looks good!The change from version 2 to 3 aligns perfectly with the breaking changes introduced in this PR. This increment is crucial for maintaining compatibility and preventing older databases from being used with the new structure. Great job on keeping the versioning up to date!
8-8: Ohayo once more, sensei! Let's consider the broader impact of this change.While the version increment is correct and necessary, it's important to ensure we have a smooth transition for our users.
Could you please confirm the following:
- Is there a migration path for existing databases from version 2 to 3?
- Has the user documentation been updated to reflect this change and provide guidance for users with existing databases?
If these aren't in place yet, it would be beneficial to include them in this PR or create follow-up tasks to address them.
crates/katana/primitives/src/block.rs (7)
45-52: LGTM! Improved type safety and clarity inPartialHeaderOhayo, sensei! The changes in the
PartialHeaderstruct look good. Theparent_hashfield now uses the more specificBlockHashtype, which improves type safety. Renamingversiontoprotocol_versionenhances clarity about the field's purpose.
99-112: LGTM! Appropriate default values for new fieldsOhayo, sensei! The
Defaultimplementation forHeaderhas been updated to include appropriate default values for the new fields. This addresses the concern raised in a previous review comment about ensuring proper initialization of these fields.
152-153: Simplified block sealing processOhayo, sensei! The
sealmethod has been simplified to directly compute the hash using thecompute_hashmethod. While this streamlines the process, it's worth noting that this change is coupled with the modifications tocompute_hashdiscussed earlier. Please ensure that this simplification doesn't compromise the integrity of the block sealing process.
185-186: LGTM! Simplified unsealing processOhayo, sensei! The
unsealmethod has been updated to directly return theheaderfromSealedBlock. This change aligns well with the simplification of theSealedBlockstruct and streamlines the unsealing process.
171-179: LGTM! SimplifiedSealedBlockstructureOhayo, sensei! The
SealedBlockstruct has been simplified by including ahashfield directly instead of a nestedSealedHeader. This change improves the clarity of the structure.However, please ensure that all code that previously accessed the
SealedHeaderhas been updated accordingly.To check for potentially affected code, you can run:
#!/bin/bash # Description: Find uses of SealedHeader that might need updates # Search for SealedHeader usage rg --type rust 'SealedHeader' crates/katana
Line range hint
122-130: Clarify the use ofFelt::ZEROin hash computationOhayo, sensei! The
compute_hashmethod now usesFelt::ZEROfor the state root and transaction commitment instead of their actual values. Could you please clarify the reasoning behind this change? It seems to potentially affect the uniqueness of the block hash.To understand the impact of this change, you can run the following script to check for other uses of
compute_hash:#!/bin/bash # Description: Find other uses of compute_hash method # Search for uses of compute_hash rg --type rust 'compute_hash' crates/katana
80-92: LGTM! Enhanced block header with new fieldsOhayo, sensei! The
Headerstruct has been significantly improved with the addition of new fields that provide more detailed information about the block's contents. This enhancement aligns well with the PR objectives.However, please ensure that these changes don't break serialization compatibility with existing systems.
To verify serialization compatibility, you can run the following script:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2560 +/- ##
==========================================
- Coverage 69.44% 69.32% -0.13%
==========================================
Files 397 397
Lines 51222 51297 +75
==========================================
- Hits 35571 35561 -10
- Misses 15651 15736 +85 ☔ View full report in Codecov by Sentry. |
implement the [`Arbitray`](https://docs.rs/arbitrary/latest/arbitrary/trait.Arbitrary.html) trait for all types that are used in the database. this is for generating random values of all the database types. helpful for due to Rust's orphan rule, there are some types that we use from external crate (mainly `starknet-rs`) that don't implement the trait, so we can't directly derive it. to get around this, the simplest solution is to just own the types and define it ourselves. this approach requires that we do more marshalling between our primitives types to the rpc types (rpc we mostly use types from starknet-rs). which is not the most elegant solution. i think eventually we probably should not rely on external crate for our rpc types and maintain them ourselves for more flexibility (if the changes that we need cant be included upstream). the idea for this PR, is to use this for generating bunch of random values for all db types and use them in a benchmark. --- test db needs to be updated because of the we define the [`ExecutionResources`](https://github.com/dojoengine/dojo/blob/a4ee208b517daead41ac1a6b855af3abb03294c3/crates/katana/primitives/src/trace.rs#L12-L20) and it doesn't serde into the same format as the one we used before (from the [cairo-vm](https://github.com/dojoengine/dojo/blob/a4ee208b517daead41ac1a6b855af3abb03294c3/crates/katana/primitives/src/trace.rs#L12-L20)) i would prefer to keep the breaking serde, mainly because it doesnt de/serialize the `builtin_instance_counter` as raw strings of the builtin names. therefore more storage optimized. we're still using the builin name types from cairo-vm anyway so marshalling between them is straightforward as we dont need to convert the individual map entry. though this changes break the db format, as we already bumped it at #2560, and it hasnt been included in a release yet, theres no need to bump it again.
implement the [`Arbitray`](https://docs.rs/arbitrary/latest/arbitrary/trait.Arbitrary.html) trait for all types that are used in the database. this is for generating random values of all the database types. helpful for due to Rust's orphan rule, there are some types that we use from external crate (mainly `starknet-rs`) that don't implement the trait, so we can't directly derive it. to get around this, the simplest solution is to just own the types and define it ourselves. this approach requires that we do more marshalling between our primitives types to the rpc types (rpc we mostly use types from starknet-rs). which is not the most elegant solution. i think eventually we probably should not rely on external crate for our rpc types and maintain them ourselves for more flexibility (if the changes that we need cant be included upstream). the idea for this PR, is to use this for generating bunch of random values for all db types and use them in a benchmark. --- test db needs to be updated because of the we define the [`ExecutionResources`](https://github.com/dojoengine/dojo/blob/a4ee208b517daead41ac1a6b855af3abb03294c3/crates/katana/primitives/src/trace.rs#L12-L20) and it doesn't serde into the same format as the one we used before (from the [cairo-vm](https://github.com/dojoengine/dojo/blob/a4ee208b517daead41ac1a6b855af3abb03294c3/crates/katana/primitives/src/trace.rs#L12-L20)) i would prefer to keep the breaking serde, mainly because it doesnt de/serialize the `builtin_instance_counter` as raw strings of the builtin names. therefore more storage optimized. we're still using the builin name types from cairo-vm anyway so marshalling between them is straightforward as we dont need to convert the individual map entry. though this changes break the db format, as we already bumped it at #2560, and it hasnt been included in a release yet, theres no need to bump it again.
ref #2547
add new fields for block commitments;
state_diff_commitment,transactions_commitment,receipts_commitment,events_commitment,transaction_count,events_count. as described in the starknet docs.also removed
SealedHeaderand flatten the hash computation logic intoSealedBlockdirectly.this BREAKS database compatibility prior to this PR, therefore, the db version has been bumped to
3.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation