refactor(katana): stage sync pipeline #2502
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2502 +/- ##
==========================================
- Coverage 68.96% 68.95% -0.02%
==========================================
Files 372 375 +3
Lines 48568 48641 +73
==========================================
+ Hits 33497 33542 +45
- Misses 15071 15099 +28 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo! This pull request introduces significant modifications across several files in the Changes
Possibly related PRs
Suggested labels
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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (15)
crates/katana/pipeline/Cargo.toml (1)
8-18: Ohayo! Dependencies are looking strong, sensei!The dependencies are well-organized and make good use of workspace-level versions, ensuring consistency across the project. The split between project-specific and external libraries improves readability.
For even better readability, consider adding a comment to separate the two groups of dependencies:
katana-tasks.workspace = true +# External dependencies anyhow.workspace = true async-trait.workspace = truecrates/katana/rpc/rpc/src/starknet/mod.rs (1)
56-56: Ohayo again, sensei! Let's summarize the refactoring.The removal of
Arcwrapper forBlockProducer<EF>simplifies the ownership model and potentially improves performance. However, we should consider the following:
- Verify that
BlockProduceris not shared across multiple threads, which could introduce thread-safety issues.- Ensure this change aligns with the overall architecture and doesn't negatively impact other parts of the system.
- Update any documentation or comments related to the threading model or ownership of
BlockProducer.Great job on simplifying the code, sensei! Just make sure we're not introducing any subtle bugs in the process.
Also applies to: 64-64
crates/katana/pipeline/src/lib.rs (2)
66-68: Ohayo, sensei! Consider testing theDefaultimplementationTesting the
defaultmethod ensures that the pipeline initializes correctly with no stages, maintaining expected behavior when usingPipeline::default().Would you like help in adding tests for the
Defaultimplementation?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-68: crates/katana/pipeline/src/lib.rs#L66-L68
Added lines #L66 - L68 were not covered by tests
72-76: Ohayo, sensei! Add tests for theDebugimplementation ofPipelineIncluding tests for the custom
Debugimplementation helps verify that the debug output correctly represents the pipeline's internal state, which is valuable for debugging purposes.Would you like assistance in creating tests for the
Debugimplementation?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 72-76: crates/katana/pipeline/src/lib.rs#L72-L76
Added lines #L72 - L76 were not covered by testscrates/katana/pipeline/src/stage/sequencing.rs (3)
37-56: Ohayo, sensei! Clarify the use of a non-resolving future inrun_messaging.In the
run_messagingmethod, whenmessaging_configisNone, a task that awaits a pending future is spawned. Consider providing a comment or explanation to clarify the intention behind spawning a task that never resolves. This will enhance maintainability and help others understand the design choice.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-41: crates/katana/pipeline/src/stage/sequencing.rs#L39-L41
Added lines #L39 - L41 were not covered by tests
[warning] 43-45: crates/katana/pipeline/src/stage/sequencing.rs#L43-L45
Added lines #L43 - L45 were not covered by tests
70-72: Ohayo, sensei! Increase test coverage for theidmethod.Static analysis indicates that the
idmethod (lines 70-72) is not covered by tests. Adding unit tests for this method will improve reliability and ensure that the correctStageIdis always returned.Would you like assistance in creating unit tests for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 70-72: crates/katana/pipeline/src/stage/sequencing.rs#L70-L72
Added lines #L70 - L72 were not covered by tests
39-45: Ohayo, sensei! Enhance test coverage forrun_messagingwhenmessaging_configisSome.Static analysis shows that lines 39-45 are not covered by tests. To ensure the messaging service behaves correctly when a configuration is provided, consider adding tests that cover the initialization and task spawning of
MessagingServiceandMessagingTask.Would you like assistance in generating tests for this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-41: crates/katana/pipeline/src/stage/sequencing.rs#L39-L41
Added lines #L39 - L41 were not covered by tests
[warning] 43-45: crates/katana/pipeline/src/stage/sequencing.rs#L43-L45
Added lines #L43 - L45 were not covered by testscrates/katana/pool/src/validation/stateful.rs (7)
Line range hint
33-39: Simplifystatefield by removing unnecessaryBoxOhayo sensei! In the
Innerstruct, thestatefield is defined asArc<Box<dyn StateProvider>>. SinceArcalready provides heap allocation and shared ownership, wrapping aBoxaround the trait object is redundant. Consider changing the type toArc<dyn StateProvider>to simplify the code and reduce unnecessary indirection.Suggested change:
struct Inner { // execution context cfg_env: CfgEnv, block_env: BlockEnv, execution_flags: SimulationFlag, - state: Arc<Box<dyn StateProvider>>, + state: Arc<dyn StateProvider>, pool_nonces: HashMap<ContractAddress, Nonce>, }
Line range hint
47-58: Updatenewmethod to acceptArc<dyn StateProvider>Ohayo sensei! In the
newmethod, you're wrappingstatein bothBoxandArc. To align with the simplifiedstatefield, consider changing the method to acceptArc<dyn StateProvider>directly. This reduces unnecessary heap allocations and simplifies ownership.Suggested change:
pub fn new( - state: Box<dyn StateProvider>, + state: Arc<dyn StateProvider>, execution_flags: SimulationFlag, cfg_env: CfgEnv, block_env: BlockEnv, permit: Arc<Mutex<()>>, ) -> Self { let inner = Arc::new(Mutex::new(Inner { cfg_env, block_env, execution_flags, - state: Arc::new(state), + state, pool_nonces: HashMap::new(), })); Self { permit, inner } }
Line range hint
60-65: Updateupdatemethod to align withstatetypeOhayo sensei! Following the previous suggestion, the
updatemethod should also be updated to acceptArc<dyn StateProvider>instead ofBox<dyn StateProvider>to maintain consistency and simplify ownership.Suggested change:
pub fn update(&self, new_state: Arc<dyn StateProvider>, block_env: BlockEnv) { let mut this = self.inner.lock(); this.block_env = block_env; - this.state = Arc::new(new_state); + this.state = new_state; }
Line range hint
71-74: Simplifypreparemethod by removing redundant boxingOhayo sensei! In the
preparemethod, you cloneself.state, which is already anArc, and then wrap it in aBox. SinceStateProviderDbcan accept anArc<dyn StateProvider>, you can passself.state.clone()directly without additional boxing.Suggested change:
fn prepare(&self) -> StatefulValidator<StateProviderDb<'static>> { - let state = Box::new(self.state.clone()); + let state = self.state.clone(); let cached_state = CachedState::new(StateProviderDb::new(state)); let context = block_context_from_envs(&self.block_env, &self.cfg_env); StatefulValidator::create(cached_state, context) }
Line range hint
85-93: Handle potential errors when retrieving nonce from stateOhayo sensei! In the
validatemethod, the callthis.state.nonce(address).unwrap().unwrap_or_default()may panic ifnonce(address)returns an error. To ensure robustness, consider properly handling the potential error instead of usingunwrap().Suggested change:
} else { - this.state.nonce(address).unwrap().unwrap_or_default() + match this.state.nonce(address) { + Ok(Some(nonce)) => nonce, + Ok(None) => Nonce::default(), + Err(e) => { + // Handle the error appropriately + return Err(Error { hash: tx.hash(), error: Box::new(e) }); + } + } };
Line range hint
95-100: Incomplete comment inskip_validatelogicOhayo sensei! It seems the comment above the
skip_validateassignment is incomplete or duplicated:// we skip validation for invoke tx with nonce 1 and nonce 0 in the state, thisPlease complete or clarify the comment to enhance code readability.
Suggested change:
// Check if validation of an invoke transaction should be skipped due to deploy_account not // being processed yet. This feature is used to improve UX for users sending -// we skip validation for invoke tx with nonce 1 and nonce 0 in the state, this +// We skip validation for invoke transactions with nonce 1 when the current nonce is 0.
Line range hint
119-123: Avoid unwrapping errors invalidatefunctionOhayo sensei! In the
validatefunction, when mapping theStatefulValidatorError, the nested use ofunwrap()can cause a panic if an error occurs. To enhance error handling and prevent potential panics, consider refining the error propagation.Consider revising the
map_invalid_tx_errfunction to handle all error variants appropriately, and ensure that anyErrcases are properly propagated without usingunwrap().crates/katana/core/src/service/block_producer.rs (1)
83-85: Ohayo, sensei! Consider refactoring to eliminate code duplication in constructors.The initialization of
produceris identical across theinterval,on_demand, andinstantconstructors. Refactoring can enhance maintainability.You can introduce a helper method to streamline the initialization:
+impl<EF: ExecutorFactory> BlockProducer<EF> { + fn new_with_mode(mode: BlockProducerMode<EF>) -> Self { + let producer = Arc::new(RwLock::new(mode)); + Self { producer } + } /// Creates a block producer that mines a new block every `interval` milliseconds. pub fn interval(backend: Arc<Backend<EF>>, interval: u64) -> Self { - let producer = IntervalBlockProducer::new(backend, Some(interval)); - let producer = Arc::new(RwLock::new(BlockProducerMode::Interval(producer))); - Self { producer } + let mode = BlockProducerMode::Interval(IntervalBlockProducer::new(backend, Some(interval))); + Self::new_with_mode(mode) } /// Creates a new block producer that will only be possible to mine by calling the /// `katana_generateBlock` RPC method. pub fn on_demand(backend: Arc<Backend<EF>>) -> Self { - let producer = IntervalBlockProducer::new(backend, None); - let producer = Arc::new(RwLock::new(BlockProducerMode::Interval(producer))); - Self { producer } + let mode = BlockProducerMode::Interval(IntervalBlockProducer::new(backend, None)); + Self::new_with_mode(mode) } /// Creates a block producer that mines a new block as soon as there are ready transactions in /// the transactions pool. pub fn instant(backend: Arc<Backend<EF>>) -> Self { - let producer = InstantBlockProducer::new(backend); - let producer = Arc::new(RwLock::new(BlockProducerMode::Instant(producer))); - Self { producer } + let mode = BlockProducerMode::Instant(InstantBlockProducer::new(backend)); + Self::new_with_mode(mode) } }Also applies to: 91-93, 99-101
📜 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 (14)
- Cargo.toml (2 hunks)
- crates/katana/core/src/service/block_producer.rs (2 hunks)
- crates/katana/core/src/service/mod.rs (2 hunks)
- crates/katana/node/Cargo.toml (1 hunks)
- crates/katana/node/src/lib.rs (6 hunks)
- crates/katana/pipeline/Cargo.toml (1 hunks)
- crates/katana/pipeline/src/lib.rs (1 hunks)
- crates/katana/pipeline/src/stage/mod.rs (1 hunks)
- crates/katana/pipeline/src/stage/sequencing.rs (1 hunks)
- crates/katana/pool/src/validation/stateful.rs (1 hunks)
- crates/katana/rpc/rpc/src/dev.rs (1 hunks)
- crates/katana/rpc/rpc/src/saya.rs (1 hunks)
- crates/katana/rpc/rpc/src/starknet/mod.rs (1 hunks)
- crates/katana/rpc/rpc/src/torii.rs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
crates/katana/pipeline/src/lib.rs
[warning] 17-17: crates/katana/pipeline/src/lib.rs#L17
Added line #L17 was not covered by tests
[warning] 52-53: crates/katana/pipeline/src/lib.rs#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 66-68: crates/katana/pipeline/src/lib.rs#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 72-76: crates/katana/pipeline/src/lib.rs#L72-L76
Added lines #L72 - L76 were not covered by testscrates/katana/pipeline/src/stage/mod.rs
[warning] 14-18: crates/katana/pipeline/src/stage/mod.rs#L14-L18
Added lines #L14 - L18 were not covered by tests
[warning] 21-21: crates/katana/pipeline/src/stage/mod.rs#L21
Added line #L21 was not covered by testscrates/katana/pipeline/src/stage/sequencing.rs
[warning] 39-41: crates/katana/pipeline/src/stage/sequencing.rs#L39-L41
Added lines #L39 - L41 were not covered by tests
[warning] 43-45: crates/katana/pipeline/src/stage/sequencing.rs#L43-L45
Added lines #L43 - L45 were not covered by tests
[warning] 70-72: crates/katana/pipeline/src/stage/sequencing.rs#L70-L72
Added lines #L70 - L72 were not covered by tests
🔇 Additional comments (25)
crates/katana/pipeline/Cargo.toml (1)
1-6: Ohayo! Package metadata looks sharp, sensei!The package metadata is well-structured and makes good use of workspace-level configurations. The explicit naming of the package as "katana-pipeline" is clear and follows best practices.
crates/katana/node/Cargo.toml (1)
12-12: Ohayo! LGTM, sensei! This addition aligns perfectly with our mission!The inclusion of
katana-pipelineas a workspace dependency is spot-on for our refactoring goals. It's clear you're setting the stage for our epic pipeline architecture overhaul. Keep up the stellar work!crates/katana/rpc/rpc/src/dev.rs (2)
19-20: Ohayo again, sensei! The constructor change looks good!The update to the
newmethod signature is consistent with the struct field change. This simplification in the API is a positive step towards cleaner code.Let's verify that all calling code has been updated accordingly:
#!/bin/bash # Description: Check for any remaining Arc<BlockProducer> passed to DevApi::new echo "Searching for DevApi::new calls with Arc<BlockProducer>:" rg --type rust "DevApi::new\(.*Arc<BlockProducer"
15-15: Ohayo, sensei! LGTM on simplifyingblock_producerownership.The removal of
Arcwrapper forblock_produceraligns well with the refactoring objectives. This change simplifies the ownership model and potentially improves performance by reducing atomic reference counting.To ensure this change doesn't impact thread safety, let's verify the usage of
BlockProducer:✅ Verification successful
Ohayo, sensei! It looks like the previous scripts ran into some syntax hiccups. Let’s give it another go with the corrected shell scripts:
Ohayo, sensei! The removal of the
Arcwrapper forblock_producerhas been successfully verified. No remainingArc<BlockProducer>usages or multi-threaded access patterns were found, ensuring that thread safety is maintained with the simplified ownership model.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining Arc<BlockProducer> usages and multi-threaded access patterns # Test 1: Search for any remaining Arc<BlockProducer> usages echo "Searching for Arc<BlockProducer> usages:" rg --type rust "Arc<BlockProducer" # Test 2: Check for potential multi-threaded access to BlockProducer echo "Checking for potential multi-threaded access to BlockProducer:" rg --type rust "(?i)(thread|tokio|async).*BlockProducer"Length of output: 370
Script:
#!/bin/bash # Description: Check for any remaining Arc<BlockProducer> usages and multi-threaded access patterns # Test 1: Search for any remaining Arc<BlockProducer> usages echo "Searching for Arc<BlockProducer> usages:" rg --type rust 'Arc<BlockProducer>' # Test 2: Check for potential multi-threaded access to BlockProducer echo "Checking for potential multi-threaded access to BlockProducer:" rg --type rust '(?i)(thread|tokio|async).*BlockProducer'Length of output: 322
crates/katana/rpc/rpc/src/saya.rs (3)
24-24: Ohayo! The Clone implementation looks good, sensei.The direct cloning of
block_produceris consistent with the struct definition change. It maintains the expectedClonebehavior while simplifying the code.
29-29: Ohayo, sensei! Thenewmethod update looks good.The change in the
block_producerparameter type is consistent with the struct definition change. It simplifies the API by removing the need forArcwrapping at the call site.Let's ensure all calling code has been updated accordingly:
#!/bin/bash # Description: Check for any remaining Arc<BlockProducer> usages # Expected result: No instances of Arc<BlockProducer> should be found rg --type rust 'Arc<BlockProducer>'
19-19: Ohayo, sensei! LGTM on simplifying ownership.The removal of
Arcwrapper forblock_produceraligns well with the refactoring objectives. It simplifies the ownership model and improves code clarity.However, let's ensure this change doesn't impact performance significantly. Could you confirm the size of
BlockProducer<EF>? If it's a large struct, direct cloning might be costly.✅ Verification successful
Ohayo, sensei! It looks like there was an issue with the previous script. Let's try a different approach to locate the
BlockProducerstruct.
Ohayo, sensei! It seems
ast-grepcouldn't locate theBlockProducerstruct. Let's try usingrgto find its definition.
Ohayo, sensei! Thanks for providing the struct locations. To ensure that removing the
Arcwrapper doesn't introduce performance issues, let's determine the size of theBlockProducer<EF>struct by counting its fields.
Ohayo, sensei! It seems the previous attempt didn't capture any fields. Let's refine our approach to accurately count the fields in the
BlockProducer<EF>struct.
Ohayo, sensei! 👍 With
BlockProducer<EF>having just 5 fields, removing theArcwrapper is perfectly fine and shouldn't impact performance. LGTM on simplifying ownership!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the size of BlockProducer struct # Expected result: The struct definition should be found, allowing us to estimate its size ast-grep --lang rust --pattern 'struct BlockProducer<$_> { $$ }'Length of output: 150
Script:
#!/bin/bash # Description: Locate the BlockProducer struct definition using ast-grep # Expected result: The struct definition of BlockProducer should be found ast-grep --lang rust --pattern 'struct BlockProducer<$_> { $$ }'Length of output: 66
Script:
#!/bin/bash # Description: Search for the BlockProducer struct definition using ripgrep # Expected result: Paths to files where BlockProducer is defined rg 'struct\s+BlockProducer' --type rustLength of output: 221
Script:
#!/bin/bash # Description: Count the number of fields in BlockProducer struct # Expected result: Number indicating the number of fields rg -A 20 'struct BlockProducer<' crates/katana/core/src/service/block_producer.rs | grep -E '^\s*\w+:' | wc -lLength of output: 114
Script:
#!/bin/bash # Description: Accurately count the number of fields in BlockProducer struct # Expected result: Number indicating the actual number of fields # Extract the struct definition and count lines that likely represent fields rg -A 50 'struct\s+BlockProducer<' crates/katana/core/src/service/block_producer.rs | \ grep -E '^\s*(pub\s+)?\w+\s*:' | wc -lLength of output: 128
Cargo.toml (2)
25-25: Ohayo! New pipeline crate added to the workspace.The addition of "crates/katana/pipeline" to the workspace members looks good, sensei! It aligns well with the PR objectives of refactoring the syncing operation.
Could you provide more details about the purpose and functionality of this new pipeline crate?
91-91: Ohayo! New katana-pipeline dependency added.The addition of the "katana-pipeline" dependency is consistent with the new workspace member, sensei. This allows other crates in the workspace to utilize the new pipeline functionality.
crates/katana/rpc/rpc/src/torii.rs (4)
Line range hint
1-238: Ohayo, sensei! Overall, these changes look solid.The modifications to
ToriiApisimplify its ownership model and API by removing unnecessaryArcwrappers. This aligns well with the PR's objective of refactoring the syncing operation. The changes are consistent across the struct definition,Cloneimplementation, andnewmethod.However, there are a few points to consider:
- Performance: The direct cloning of
BlockProducer<EF>might impact performance if it's a large struct. Consider benchmarking before and after these changes.- Clone implementation: Ensure that
BlockProducer<EF>has an efficientCloneimplementation.- Call sites: All call sites of
ToriiApi::newshould be updated to passBlockProducer<EF>directly instead ofArc<BlockProducer<EF>>.These changes contribute to a cleaner and more straightforward API, which should improve code maintainability in the long run.
39-40: Ohayo, sensei! The new method update looks great.The change in the
newmethod signature to acceptBlockProducer<EF>directly is consistent with the updated field type. This simplifies the API by removing the need forArc::newwhen creating aToriiApiinstance.Let's ensure all call sites of this method are updated accordingly:
#!/bin/bash # Description: Verify all call sites of ToriiApi::new are updated # Test: Search for calls to ToriiApi::new rg --type rust 'ToriiApi::new' # Manually review the results to ensure all call sites have been updated # to pass BlockProducer<EF> directly instead of Arc<BlockProducer<EF>>.
33-33: Ohayo! The Clone implementation looks good, sensei.The change to directly clone
block_produceris consistent with the updated field type. This maintains the correctness of theCloneimplementation.To ensure optimal performance, let's verify that
BlockProducer<EF>implements an efficientClone. Run this script to check:#!/bin/bash # Description: Verify the Clone implementation of BlockProducer<EF> # Test: Look for the Clone implementation of BlockProducer ast-grep --lang rust --pattern $'impl Clone for BlockProducer<$_> { $$$ }' # If found, manually review the implementation to ensure it's efficient. # If not found, consider adding a custom Clone implementation if needed.
25-25: Ohayo, sensei! LGTM on simplifying ownership.The change from
Arc<BlockProducer<EF>>toBlockProducer<EF>simplifies the ownership model, which aligns well with the refactoring objectives. This modification reduces complexity by removing unnecessary reference counting.However, let's ensure this doesn't negatively impact performance, especially if
BlockProducer<EF>is a large struct. Consider running performance tests to verify:crates/katana/rpc/rpc/src/starknet/mod.rs (2)
64-64: Consistent change in constructor, nice work sensei!The
newmethod signature has been updated to match theInnerstruct change. This ensures consistency across the codebase.
56-56: Ohayo, sensei! Simplified ownership model detected.The change from
Arc<BlockProducer<EF>>toBlockProducer<EF>removes the atomic reference counting. This could potentially improve performance by reducing overhead. However, we should verify that this doesn't introduce any thread-safety issues, especially ifBlockProduceris shared across multiple threads.Let's check if
BlockProduceris used in a multi-threaded context:✅ Verification successful
Ohayo, sensei! Let's gather more insights on
BlockProducerusage.Since the initial search didn't yield any results, we'll perform a more comprehensive search to ensure
BlockProducerisn't used in any multi-threaded contexts.
Ohayo, sensei!
BlockProducerownership change looks good.After a thorough search, we've confirmed that removing the
Arcwrapper aroundBlockProducer<EF>does not introduce any thread-safety issues. The internal use ofArc<RwLock<>>withinBlockProducerensures safe concurrent access where necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BlockProducer usage in async contexts or with thread-related types rg --type rust -e "BlockProducer.*async" -e "BlockProducer.*thread" -e "BlockProducer.*tokio"Length of output: 95
Script:
#!/bin/bash # Comprehensive search for all usages of BlockProducer to ensure no multi-threaded contexts rg --type rust 'BlockProducer'Length of output: 8077
crates/katana/pipeline/src/stage/mod.rs (1)
27-34: Ohayo sensei! TheStagetrait definition looks great.The asynchronous
executemethod and clear trait structure provide a solid foundation for implementing pipeline stages.crates/katana/pipeline/src/stage/sequencing.rs (1)
47-53: Ohayo, sensei! Consider the necessity of spawning a pending task whenmessaging_configisNone.When no messaging configuration is provided, a task that awaits a pending future is spawned. Assess whether this task is necessary. If it's meant to keep the application structure consistent, ensure this design choice is documented.
To confirm if any components rely on the existence of the "Messaging" task regardless of configuration, you can search for dependencies:
crates/katana/core/src/service/mod.rs (2)
45-45: Ohayo sensei! Confirm the updated ownership in thenewfunction is acceptable.The
newfunction now takes ownership ofBlockProducer<EF>instead of anArcwrapped version. Ensure that this change aligns with the intended ownership model and thatBlockProduceris not required to be shared beyond this context.
35-35: Ohayo sensei! Verify the removal ofArcfromblock_producer.By changing
block_producerto ownBlockProducer<EF>directly, we need to ensure that no other components require shared ownership ofBlockProducer. Please confirm thatBlockProduceris not shared elsewhere and that transferring ownership here does not impact other parts of the codebase.Run the following script to check for other usages of
Arc<BlockProducer:crates/katana/node/src/lib.rs (4)
59-59: Ohayo, sensei! Simplified ownership ofblock_producerinHandlestructThe
block_producerfield in theHandlestruct now has direct ownership ofBlockProducer<BlockifierFactory>instead of anArc. This change simplifies the ownership model and reduces the overhead of reference counting.
230-230: Ohayo, sensei! Properly spawning the pipeline futureThe pipeline is correctly spawned using
task_manager.spawn(pipeline.into_future());, making appropriate use of theIntoFuturetrait.
217-223: 🛠️ Refactor suggestionOhayo, sensei! Verify cloning of
block_producerin Sequencing stageIn the initialization of the
Sequencingstage,block_producer.clone()is used:let sequencing = stage::Sequencing::new( pool.clone(), backend.clone(), task_manager.clone(), block_producer.clone(), sequencer_config.messaging.clone(), );Since
block_producernow has direct ownership, cloning it may have performance implications ifBlockProduceris not cheaply cloneable. Please verify that cloning is intentional and thatBlockProducerimplementsCloneefficiently. If shared ownership is required, consider wrapping it in anArcagain.To check if
BlockProducerimplementsClone, you can run the following script:
242-242: Ohayo, sensei! Updatedspawnfunction signature requires attentionThe
spawnfunction signature has been updated to acceptBlockProducer<EF>directly instead ofArc<BlockProducer<EF>>:pub async fn spawn<EF: ExecutorFactory>( node_components: (TxPool, Arc<Backend<EF>>, BlockProducer<EF>, TxValidator), config: ServerConfig, ) -> Result<RpcServer> {Ensure that all calls to
spawnare updated accordingly to pass theBlockProducer<EF>without anArc. Verify that this change aligns with the intended ownership model and doesn't introduce unintended ownership or borrowing issues.Here's a script to find all invocations of
spawn:crates/katana/core/src/service/block_producer.rs (2)
77-77: Ohayo, sensei! UpdatingproducertoArc<RwLock<...>>enhances thread-safe shared ownership.The conversion allows multiple instances to share and modify the
producerconcurrently, which is crucial for efficient block production in a multi-threaded environment.
149-153: Ohayo, sensei! ImplementingCloneforBlockProducer<EF>enhances concurrency support.This allows for safe cloning of the
BlockProducer, sharing the underlyingArc<RwLock<...>>without risking data races, which is beneficial for multi-threaded operations.
| impl core::fmt::Display for StageId { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| match self { | ||
| StageId::Sequencing => write!(f, "Sequencing"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Consider adding tests for StageId's Display implementation.
The Display implementation for StageId (lines 14-18) is not currently covered by tests, as indicated by the static analysis tools. Including unit tests will ensure that the formatted output remains correct, especially if new variants are added to StageId in the future.
Would you like assistance in creating these unit tests?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-18: crates/katana/pipeline/src/stage/mod.rs#L14-L18
Added lines #L14 - L18 were not covered by tests
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| #[error(transparent)] | ||
| Other(#[from] anyhow::Error), | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Adding tests for the Error enum would be beneficial.
The Error enum derived from thiserror::Error (line 21) is not covered by tests. Testing the error handling ensures that errors are correctly propagated and formatted, which is crucial for effective debugging and reliability.
Would you like help in writing tests for the Error enum?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-21: crates/katana/pipeline/src/stage/mod.rs#L21
Added line #L21 was not covered by tests
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| #[error(transparent)] | ||
| Stage(#[from] stage::Error), | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consider adding tests for the Error enum to enhance error handling
Including unit tests for the Error enum will ensure that error propagation from stages is functioning correctly. This will improve the robustness of the pipeline's error handling mechanism.
Would you like assistance in creating unit tests for the Error enum or opening a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-17: crates/katana/pipeline/src/lib.rs#L17
Added line #L17 was not covered by tests
| pub async fn run(&mut self) -> PipelineResult { | ||
| for stage in &mut self.stages { | ||
| info!(id = %stage.id(), "Executing stage"); | ||
| stage.execute().await?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Ensure the run method is adequately tested
The run method is central to the pipeline's operation. Adding tests to verify that stages execute in order and handle errors appropriately will increase confidence in the pipeline's reliability.
Can I assist in writing tests for the run method or open a GitHub issue to address this?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-53: crates/katana/pipeline/src/lib.rs#L52-L53
Added lines #L52 - L53 were not covered by tests
| async fn execute(&mut self) -> StageResult { | ||
| let _ = self.run_messaging().await?; | ||
| let _ = self.run_block_production().await; | ||
| future::pending::<StageResult>().await | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Document the perpetual awaiting in execute method.
The execute method awaits a future::pending::<StageResult>(), causing it to never resolve. If this is intentional to keep the stage running indefinitely, consider adding documentation or comments to explain this behavior for future maintainability.
staged sync pipeline inspired by erigon's staged sync architecture.
this is designed in the anticipation of a blockchain syncing logic in katana. syncing operation in katana would be split into stages, where each stage is responsible for handling certain tasks in the syncing process.
the abstraction in this PR is designed to encourage good separation of concern and also help understanding all the main processes (ie syncing and sequencing) throughout a katana runtime.
Summary by CodeRabbit
New Features
katana-pipelinepackage with a modular architecture for task management.Pipelinestruct to manage execution stages and support asynchronous operations.Sequencingstage for managing transaction and block production tasks.Bug Fixes
Documentation
Refactor
DevApi,SayaApi, andStarknetApi.