refactor(katana): move forked blockchain creation logic to core #2545
Conversation
WalkthroughOhayo! The recent changes introduce significant modifications to 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: 4
🧹 Outside diff range and nitpick comments (10)
crates/katana/node/src/config/fork.rs (2)
5-11: Ohayo! TheForkingConfigstruct looks solid, sensei!The struct is well-designed with descriptive field names and appropriate types. The use of
Option<BlockHashOrNumber>for theblockfield is a smart choice, allowing flexibility in block specification.One small suggestion for future enhancement:
Consider adding aDefaultimplementation or a constructor method to create aForkingConfigwith sensible defaults. This could make it easier for users to create instances with minimal configuration.Here's an example of how you could add a
Defaultimplementation:impl Default for ForkingConfig { fn default() -> Self { Self { url: Url::parse("http://localhost:8545").expect("Invalid default URL"), block: None, } } }This is just a suggestion for future improvement, not a critical change. What do you think, sensei?
4-10: Ohayo! The documentation is looking good, sensei!The doc comments are clear, concise, and informative. They provide a good overview of the struct's purpose and explain each field's role effectively.
A small suggestion to make it even better:
Consider adding an example usage of theForkingConfigstruct in the documentation. This can help users understand how to create and use the struct in their code.Here's an example of how you could enhance the documentation:
/// Node forking configurations. /// /// # Example /// /// ``` /// use katana_node::config::fork::ForkingConfig; /// use starknet::providers::Url; /// /// let config = ForkingConfig { /// url: Url::parse("https://example.com/rpc").unwrap(), /// block: Some(BlockHashOrNumber::Number(1000)), /// }; /// ``` #[derive(Debug, Clone)] pub struct ForkingConfig { // ... (existing fields) }What do you think about adding this example, sensei?
crates/katana/node/src/config/mod.rs (1)
27-29: Ohayo! New field looks great, sensei! Just a tiny suggestion.The addition of the
forkingfield to theConfigstruct is well-placed and correctly typed asOption<ForkingConfig>. This aligns perfectly with the PR objective of integrating forking functionality.Consider adding a brief doc comment for the
forkingfield, similar to the other fields in the struct. This would enhance the documentation and make it easier for other developers to understand the purpose of this field. Here's a suggested addition:/// Database options. pub db: DbConfig, + /// Forking options. pub forking: Option<ForkingConfig>, /// Rpc options. pub rpc: RpcConfig,bin/katana/src/utils.rs (1)
27-35: Ohayo! Excellent new function, sensei!The parse_block_hash_or_number function is a great addition. It elegantly handles parsing of both block hashes and numbers, returning a BlockHashOrNumber enum. The logic is sound and the error handling is good.
A small suggestion to improve consistency:
Consider adding a context message to the BlockHash parsing as well:
- Ok(BlockHashOrNumber::Hash(BlockHash::from_hex(value)?)) + Ok(BlockHashOrNumber::Hash(BlockHash::from_hex(value).context("could not parse block hash")?))This would make the error handling consistent between both branches.
crates/katana/primitives/src/block.rs (1)
199-206: Ohayo again, sensei! This implementation is also sugoi, but let's make it even better!The
Fromtrait implementation for convertingBlockHashOrNumbertoBlockIdOrTagis well-structured and correctly handles both variants. It provides a seamless conversion between these types.For consistency with the
Displayimplementation, consider using the same variable names in the pattern matching:impl From<BlockHashOrNumber> for BlockIdOrTag { fn from(value: BlockHashOrNumber) -> Self { match value { - BlockHashOrNumber::Hash(hash) => BlockIdOrTag::Hash(hash), - BlockHashOrNumber::Num(number) => BlockIdOrTag::Number(number), + BlockHashOrNumber::Hash(hash) => BlockIdOrTag::Hash(hash), + BlockHashOrNumber::Num(num) => BlockIdOrTag::Number(num), } } }This small change aligns the variable naming with the
Displayimplementation, enhancing overall code consistency.crates/katana/storage/provider/src/providers/fork/backend.rs (4)
155-155: Ensure consistent logging practices, sensei.The use of
trace!for the "Forking backend started." message might make it less noticeable during debugging. If this log is crucial, consider usinginfo!to maintain consistency and visibility.Apply this diff if you agree:
-trace!(target: LOG_TARGET, "Forking backend started."); +info!(target: LOG_TARGET, "Forking backend started.");
Line range hint
129-132: Refactor for code duplication reduction, sensei.The pattern used in handling different
BackendRequestvariants is similar across cases. Consider abstracting the common logic into a helper function to reduce code duplication and improve maintainability.Here's how you might refactor it:
fn handle_request<T, F>(&mut self, payload: T, sender: OneshotSender<BackendResult<F>>, request_fn: impl Fn(Arc<P>, BlockId, T) -> F) where F: Future<Output = Result<U, StarknetProviderError>> + Send + 'static, U: Send + 'static, { let provider = self.provider.clone(); let block = self.block; let fut = Box::pin(async move { let res = request_fn(provider, block, payload) .await .map_err(BackendError::StarknetProvider); sender.send(res).expect("failed to send result"); }); self.pending_requests.push(fut); }Then, you can use this in your match arms to handle each request type.
Line range hint
302-313: Consider caching zero values carefully, sensei.The logic for invalidating zero nonce and class hash values to force re-fetching from the provider might lead to unnecessary network requests if zero is a valid and common value. Evaluate whether zero is an expected value for nonce and class hashes in your context.
If zero is a valid value, consider implementing an explicit "not fetched" state or using
Option<Nonce>to represent the absence of a value distinctly from zero.
Line range hint
341-355: Ohayo, sensei! HandleContractNotFoundandClassHashNotFounderrors uniformly.In the
handle_not_found_errfunction, you are mapping specific errors toNone. Ensure that all relevant "not found" errors are covered, and consider logging the errors for better traceability.Add any additional error variants that represent "not found" cases, and consider logging unexpected errors.
fn handle_not_found_err<T>(result: Result<T, BackendError>) -> Result<Option<T>, BackendError> { match result { Ok(value) => Ok(Some(value)), Err(BackendError::StarknetProvider(StarknetProviderError::StarknetError( StarknetError::ContractNotFound | StarknetError::ClassHashNotFound | StarknetError::BlockNotFound, // Add any other relevant errors ))) => Ok(None), Err(e) => { error!(target: LOG_TARGET, %e, "Unexpected error in handle_not_found_err."); Err(e) }, } }crates/katana/core/src/backend/storage.rs (1)
123-129: Handle unexpected chain IDs gracefully.While parsing the
chain_idusingparse_cairo_short_string, consider logging a warning when parsing fails. Currently, if the chain ID is not ASCII-encoded, it defaults to displaying in hexadecimal format. Explicitly informing users about the fallback can improve transparency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
- bin/katana/src/cli/node.rs (8 hunks)
- bin/katana/src/utils.rs (2 hunks)
- crates/katana/core/Cargo.toml (1 hunks)
- crates/katana/core/src/backend/config.rs (0 hunks)
- crates/katana/core/src/backend/storage.rs (6 hunks)
- crates/katana/node/Cargo.toml (0 hunks)
- crates/katana/node/src/config/fork.rs (1 hunks)
- crates/katana/node/src/config/mod.rs (2 hunks)
- crates/katana/node/src/lib.rs (2 hunks)
- crates/katana/primitives/src/block.rs (2 hunks)
- crates/katana/storage/provider/src/providers/fork/backend.rs (2 hunks)
💤 Files with no reviewable changes (2)
- crates/katana/core/src/backend/config.rs
- crates/katana/node/Cargo.toml
🧰 Additional context used
🔇 Additional comments (25)
crates/katana/node/src/config/fork.rs (1)
1-2: Ohayo! These imports look spot-on, sensei!The imports for
BlockHashOrNumberandUrlare well-chosen and directly relevant to theForkingConfigstruct being defined. Nice work on keeping it clean and focused!crates/katana/node/src/config/mod.rs (2)
3-3: Ohayo! New module looks good, sensei!The addition of the
forkmodule aligns well with the PR objective of moving forked blockchain creation logic. This organization will help keep the forking-related code separate and maintainable.
9-9: Ohayo! Import looks spot-on, sensei!The import of
ForkingConfigfrom the newforkmodule is correctly placed and necessary for using the type in theConfigstruct. Good job on maintaining proper module organization!bin/katana/src/utils.rs (2)
3-4: Ohayo! New imports look good, sensei!The added imports for anyhow and katana_primitives::block are necessary for the changes made in this file. They support improved error handling and the new block parsing functionality.
21-21: Nice simplification, sensei!The change to the parse_genesis function signature is a good simplification. It maintains the same functionality while adhering to Rust's conventions for error handling. Ohayo to cleaner code!
crates/katana/core/Cargo.toml (1)
24-24: Ohayo! New dependency looks good, sensei!The addition of
num-traitsas a workspace-managed dependency is a solid choice. This library will provide useful numerical traits that can enhance our mathematical operations in the codebase. Nice work on keeping it consistent with our workspace structure!crates/katana/primitives/src/block.rs (1)
18-25: Ohayo, sensei! This implementation looks sugoi!The
Displaytrait implementation forBlockHashOrNumberis well-crafted and follows Rust best practices. It provides a clear and consistent string representation for both variants of the enum.crates/katana/node/src/lib.rs (3)
47-47: Ohayo sensei! The addition of thetracing::infoimport is appropriate.The
info!macro is used later in the code to log the start of the RPC server, so importingtracing::infois necessary.
184-186: Potential side effects from mutatingconfig.chainOhayo sensei! In the
buildfunction,&mut config.chainis passed toBlockchain::new_from_forked, indicating thatconfig.chainmay be mutated within this function. Please verify if mutatingconfig.chainis intentional and doesn't introduce unintended side effects elsewhere in the code.To ensure that mutating
config.chaindoesn't affect other parts of the codebase, consider reviewing its usage after this point.
191-191: Ohayo sensei! Consistent initialization without forking configuration.The blockchain is correctly initialized with
InMemoryProviderwhen no forking configuration or database directory is provided. This ensures that the node can operate in a standalone mode.bin/katana/src/cli/node.rs (5)
78-87: Ohayo sensei, forking functionality arguments are well-definedThe new command-line arguments for the forking functionality are correctly implemented with proper usage of
clapattributes and validations. The aliases and value parsers are appropriately configured.
175-175: Ohayo sensei, conflict declarations are correctly specifiedThe
conflicts_with_allattribute forgenesisensures mutual exclusivity withfork_rpc_url,seed, andtotal_accounts, which is logical and prevents configuration issues.
260-262: Ohayo sensei, logging configuration is properly adjustedThe
DEFAULT_LOG_FILTERnow includesforking::backend=trace, which is appropriate for debugging the new forking functionality. The string continuation is correctly used.
286-290: Ohayo sensei, inclusion of forking config in node configurationThe
forkingconfiguration is correctly obtained fromself.forking_config()?and included in the overallConfigstruct. This integration ensures that the forking settings are passed throughout the node components.
359-365: Ohayo sensei,forking_configmethod is correctly implementedThe
forking_configmethod properly constructs theForkingConfigwhenfork_rpc_urlis provided. It handles the optionalfork_blockand returnsNonewhen forking is not enabled.crates/katana/storage/provider/src/providers/fork/backend.rs (1)
Line range hint
507-518: Ensure tests are not ignored unintentionally, sensei.In the
#[ignore]attribute on the testfetch_from_fork_will_err_if_backend_thread_not_running, verify if it's intentionally ignored or if it should be re-enabled.You can run the following script to list all ignored tests:
#!/bin/bash # Description: List all ignored tests in the codebase. rg --type rust '#\[ignore\]' --context 2This will help you identify and review any tests that are currently being ignored.
crates/katana/core/src/backend/storage.rs (9)
1-2: Ohayo sensei! Addition ofArcimport is appropriate.The inclusion of
use std::sync::Arc;is necessary for the shared ownership of theproviderin theForkedProvider. This ensures thread-safe reference counting when dealing with asynchronous operations.
Line range hint
84-106: Consistency achieved withnew_with_chainmethod refactoring.Renaming
new_with_genesistonew_with_chainenhances clarity, reflecting that the method now handles chain specifications beyond just the genesis block. The updated logic correctly checks for the genesis block initialization and appropriately initializes it if absent.
113-113: Ohayo! Efficient reuse ofnew_with_chaininnew_with_dbmethod.By calling
Self::new_with_chain(DbProvider::new(db), chain), the code reduces redundancy and ensures consistency in blockchain initialization across different methods.
117-176: Great addition ofnew_from_forkedmethod for forking capability.Introducing the asynchronous
new_from_forkedmethod enhances the blockchain's flexibility by allowing initialization from a forked network. This is a valuable feature for testing and development purposes.
147-149: Ohayo sensei! Properly disallowing forking from pending blocks.The check to prevent forking from a pending block aligns with best practices, ensuring that the blockchain state is consistent and reliable.
172-172: Ohayo! Correct usage ofArcwithForkedProvider.Wrapping the provider with
Arc::newinForkedProvider::new(Arc::new(provider), block_id)?;ensures thread-safe shared ownership, which is essential for asynchronous operations.
216-216: Ohayo sensei! Updating trait imports in tests is good practice.Ensuring that all necessary traits are imported in the test module maintains test reliability and clarity.
229-229: Ensure tests reflect the changes in initialization methods.In the test
blockchain_from_genesis_states, the call toBlockchain::new_with_chaincorrectly reflects the refactored method name. Verify that all test cases are updated similarly to prevent failures during testing.
Line range hint
183-189: Verify all references tonew_with_block_and_stateare updated.The method
new_with_block_and_statehas been renamed tonew_with_genesis_block_and_statefor clarity. Please ensure that all references to the old method name are updated throughout the codebase to prevent any linkage or compilation errors.Run the following script to confirm no lingering references:
| @@ -488,8 +496,7 @@ mod test { | |||
|
|
|||
| assert!(config.dev.fee); | |||
| assert!(config.dev.account_validation); | |||
| assert_eq!(config.starknet.fork_rpc_url, None); | |||
| assert_eq!(config.starknet.fork_block_number, None); | |||
| assert!(config.forking.is_none()); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, consider adding tests for forking configuration
Currently, the tests verify that config.forking is None by default. It would be beneficial to add test cases that validate the forking configuration when fork_rpc_url and fork_block are provided. This ensures the new forking functionality is properly tested and helps prevent regressions.
| @@ -22,7 +22,7 @@ use katana_primitives::Felt; | |||
| use parking_lot::Mutex; | |||
| use starknet::core::types::{BlockId, ContractClass as RpcContractClass, StarknetError}; | |||
| use starknet::providers::{Provider, ProviderError as StarknetProviderError}; | |||
| use tracing::{error, info, trace}; | |||
| use tracing::{error, trace}; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider adjusting the logging level for better clarity.
The logging level for the message indicating the start of the forking backend has been changed from info to trace. This might make it less visible in standard logs. If this message is important for users to know when the backend starts, it might be better to keep it at the info level.
Apply this diff to revert the logging level back to info:
-use tracing::{error, trace};
+use tracing::{error, info};
...
-trace!(target: LOG_TARGET, "Forking backend started.");
+info!(target: LOG_TARGET, "Forking backend started.");Committable suggestion was skipped due to low confidence.
| @@ -152,7 +152,7 @@ | |||
| }) | |||
| .map_err(BackendError::BackendThreadInit)?; | |||
|
|
|||
| info!(target: LOG_TARGET, "Forking backend started."); | |||
| trace!(target: LOG_TARGET, "Forking backend started."); | |||
There was a problem hiding this comment.
💡 Codebase verification
Logging Level Change Impact
Ohayo, sensei! Changing the logging level from info! to trace! in crates/katana/storage/provider/src/providers/fork/backend.rs may reduce the visibility of this log message in production environments. Please ensure this change aligns with our overall logging strategy to maintain essential log visibility during standard operations.
🔗 Analysis chain
Ohayo, sensei! Verify the impact of changing the logging level.
Changing the logging level from info! to trace! may affect the visibility of this log message in production environments. Ensure that this change aligns with the desired verbosity levels across different deployment configurations.
You can run the following script to check where trace! logs are output:
Ohayo, sensei! It looks like there was an issue with the previous script. Let's try running a corrected version to verify the logging levels:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify logging levels in the codebase.
# Find all instances of `trace!` and `info!` in the codebase.
rg --type rust 'trace!\(|info!\('
Length of output: 2493
Script:
#!/bin/bash
# Description: Verify logging levels in the codebase.
# Find all instances of `trace!` and `info!` in Rust files.
rg --type rust 'trace!\(|info!\('
Length of output: 23789
| let status = match block.status { | ||
| BlockStatus::AcceptedOnL1 => FinalityStatus::AcceptedOnL1, | ||
| BlockStatus::AcceptedOnL2 => FinalityStatus::AcceptedOnL2, | ||
| // we already checked for pending block earlier. so this should never happen. | ||
| _ => bail!("qed; block status shouldn't be pending"), | ||
| }; |
There was a problem hiding this comment.
Consider handling all possible block statuses explicitly.
In the match statement for block.status, only AcceptedOnL1 and AcceptedOnL2 are handled, with other statuses resulting in an error. To enhance robustness, consider handling other possible statuses like Rejected or future statuses explicitly, or provide a more descriptive error message.
Apply this diff to handle additional statuses:
match block.status {
BlockStatus::AcceptedOnL1 => FinalityStatus::AcceptedOnL1,
BlockStatus::AcceptedOnL2 => FinalityStatus::AcceptedOnL2,
+ BlockStatus::Rejected => bail!("Cannot fork from a rejected block"),
// we already checked for pending block earlier. so this should never happen.
_ => bail!("Unexpected block status: {:?}", block.status),
}📝 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.
| let status = match block.status { | |
| BlockStatus::AcceptedOnL1 => FinalityStatus::AcceptedOnL1, | |
| BlockStatus::AcceptedOnL2 => FinalityStatus::AcceptedOnL2, | |
| // we already checked for pending block earlier. so this should never happen. | |
| _ => bail!("qed; block status shouldn't be pending"), | |
| }; | |
| let status = match block.status { | |
| BlockStatus::AcceptedOnL1 => FinalityStatus::AcceptedOnL1, | |
| BlockStatus::AcceptedOnL2 => FinalityStatus::AcceptedOnL2, | |
| BlockStatus::Rejected => bail!("Cannot fork from a rejected block"), | |
| // we already checked for pending block earlier. so this should never happen. | |
| _ => bail!("Unexpected block status: {:?}", block.status), | |
| }; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2545 +/- ##
==========================================
- Coverage 69.54% 69.42% -0.13%
==========================================
Files 388 388
Lines 49907 49881 -26
==========================================
- Hits 34706 34628 -78
- Misses 15201 15253 +52 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
ForkingConfigstructure for enhanced configuration capabilities.Bug Fixes
Refactor
Documentation