feat(katana): automatically close block if resources limit reached#3003
feat(katana): automatically close block if resources limit reached#3003
Conversation
WalkthroughOhayo sensei! This PR refactors configuration and block production logic across the codebase. It updates function signatures to require a Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Node
participant Config as Config Object
participant SeqConf as SequencingConfig
participant BL as BlockLimits
participant BF as BlockifierFactory
Node->>Config: Load configuration
Config->>SeqConf: Retrieve sequencing settings
SeqConf->>BL: Call block_limits() (uses block_cairo_steps_limit if provided)
Node->>BF: Initialize BlockifierFactory with cfg, execution flags, and BL
sequenceDiagram
participant Executor as BlockExecutor
participant Transact as transact Function
participant Bouncer as Bouncer (optional)
participant State as TransactionalState
Executor->>Transact: Call transact(state, context, flags, tx, bouncer?)
Transact->>State: Create transactional state and process tx
Transact-->>Executor: Return (executed transactions, optionally leftover txs)
alt Bouncer provided
Executor->>Bouncer: Use bouncer for additional state validation
end
Executor-->>Executor: Update block full status and check for ExecutorError
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
101-104: Ohayo sensei! LeveragingBouncerWeights::max()and assigningn_stepsis neat.
A minor improvement: consider logging or tracing the finaln_stepsvalue for debugging resource caps.crates/katana/core/src/service/block_producer.rs (1)
1-10: Ohayo sensei! These poetic doc comments add a whimsical touch while staying organized.crates/katana/node/src/config/sequencing.rs (2)
14-22: Ohayo! Consider enhancing documentation for block_cairo_steps_limitThe documentation could be more helpful by including:
- Example values or recommended ranges
- Impact on transaction throughput
- Relationship with Starknet's current limits
/// The maximum number of Cairo steps in a block. - // /// The block will automatically be closed when the accumulated Cairo steps across all the /// transactions has reached this limit. /// /// NOTE: This only affect interval block production. /// + /// Example: Setting this to 1_000_000 would close the block after approximately + /// 1M Cairo steps, which is well below Starknet's current limit of ~10M steps. + /// /// See <https://docs.starknet.io/chain-info/#current_limits>.
26-28: Consider adding validation for block_cairo_steps_limitThe
block_limitsmethod could validate that the limit is within reasonable bounds when specified.pub fn block_limits(&self) -> BlockLimits { - BlockLimits { cairo_steps: self.block_cairo_steps_limit.unwrap_or(u64::MAX) } + let steps = self.block_cairo_steps_limit.unwrap_or(u64::MAX); + if let Some(limit) = self.block_cairo_steps_limit { + assert!(limit > 0, "block_cairo_steps_limit must be positive"); + } + BlockLimits { cairo_steps: steps } }crates/katana/executor/benches/execution.rs (1)
52-54: Document the new None parameter in transact call, senseiThe None parameter appears to be related to block limits, but its purpose isn't immediately clear from the context.
Add a comment explaining the parameter:
|(mut state, block_context, flags, tx)| { + // None: No block limits for benchmarking transact(&mut state, block_context, flags, tx, None) },crates/katana/executor/src/abstraction/error.rs (1)
7-13: Enhance LimitsExhausted error with specific limit informationConsider adding context about which limit was exhausted to help with debugging.
pub enum ExecutorError { - #[error("Limits exhausted")] - LimitsExhausted, + #[error("Limits exhausted: {limit_type} reached {current}/{max}")] + LimitsExhausted { + limit_type: &'static str, + current: u64, + max: u64, + }, #[error(transparent)] Other(Box<dyn core::error::Error + Send + Sync + 'static>), }crates/katana/executor/src/abstraction/executor.rs (1)
42-46: Enhanced return type for better error handlingThe new return type
ExecutorResult<(usize, Option<ExecutorError>)>provides valuable information:
- Number of transactions successfully executed
- Optional error if execution stopped due to resource limits
This design pattern allows for graceful handling of partial block execution, which is essential for implementing resource limits.
crates/katana/executor/src/abstraction/mod.rs (1)
27-31: Consider using a named constant for maximum steps, sensei!Instead of using
u64::MAXdirectly, consider defining a constant likeMAX_CAIRO_STEPSto make the code more maintainable and self-documenting.+const MAX_CAIRO_STEPS: u64 = u64::MAX; impl BlockLimits { pub fn max() -> Self { - Self { cairo_steps: u64::MAX } + Self { cairo_steps: MAX_CAIRO_STEPS } } }crates/katana/node/src/lib.rs (1)
185-189: Ohayo! The BlockifierFactory initialization looks good, sensei!The addition of
block_limits()parameter aligns well with the PR objective of implementing automatic block closure based on resource limits.Consider adding a comment explaining the significance of the block limits parameter for better code documentation:
let executor_factory = Arc::new(BlockifierFactory::new( cfg_env, execution_flags, + // Block limits control automatic block closure when resource thresholds are met config.sequencing.block_limits(), ));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(2 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/contracts/piltover(1 hunks)crates/katana/core/src/service/block_producer.rs(12 hunks)crates/katana/core/tests/backend.rs(2 hunks)crates/katana/executor/benches/concurrent.rs(2 hunks)crates/katana/executor/benches/execution.rs(1 hunks)crates/katana/executor/src/abstraction/error.rs(1 hunks)crates/katana/executor/src/abstraction/executor.rs(2 hunks)crates/katana/executor/src/abstraction/mod.rs(1 hunks)crates/katana/executor/src/implementation/blockifier/error.rs(3 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(9 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(4 hunks)crates/katana/executor/src/implementation/noop.rs(2 hunks)crates/katana/executor/tests/fixtures/mod.rs(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)crates/katana/node/src/config/sequencing.rs(1 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/rpc/rpc/tests/dev.rs(1 hunks)crates/katana/rpc/rpc/tests/forking.rs(1 hunks)crates/katana/rpc/rpc/tests/messaging.rs(1 hunks)crates/katana/rpc/rpc/tests/proofs.rs(1 hunks)crates/katana/rpc/rpc/tests/saya.rs(1 hunks)crates/katana/rpc/rpc/tests/starknet.rs(1 hunks)crates/katana/rpc/rpc/tests/torii.rs(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- crates/katana/contracts/piltover
- crates/katana/rpc/rpc/tests/proofs.rs
- crates/katana/rpc/rpc/tests/forking.rs
- crates/katana/rpc/rpc/tests/saya.rs
- crates/katana/rpc/rpc/tests/messaging.rs
- crates/katana/rpc/rpc/tests/starknet.rs
- crates/katana/rpc/rpc/tests/torii.rs
- crates/katana/rpc/rpc/tests/dev.rs
🔇 Additional comments (41)
crates/katana/executor/src/implementation/blockifier/mod.rs (13)
3-3: Ohayo sensei! New import looks clean.
26-28: Ohayo sensei! The expanded references are nicely structured for convenience.
37-37: Ohayo sensei! Adding thelimitsfield in the factory is a clear approach to store block constraints.
42-43: Ohayo sensei! The new constructor seamlessly integrates thelimitsparameter.
65-66: Ohayo sensei! Forwarding the clonedlimitstoStarknetVMProcessoris a consistent design choice.
86-86: Ohayo sensei! Including thebouncerfield enhances resource-limiting logic capabilities.
95-95: Ohayo sensei! AcceptingBlockLimitshere aligns well with the broadened resource constraints.
105-112: Ohayo sensei! Initializing fields with the newbounceris well-coordinated.
168-168: Ohayo sensei! Returning(usize, Option<ExecutorError>)is a great way to report partial progress and limit errors.
193-193: Ohayo sensei! Startingtotal_executedat zero is straightforward for counting executions.
205-211: Ohayo sensei! PassingSome(&mut self.bouncer)properly integrates block capacity checks.
213-239: Ohayo sensei! TheLimitsExhaustedbranch accurately returns partial results when block capacity is reached.
246-246: Ohayo sensei! Returning the executed count with no error suits the completion path for normal execution.crates/katana/core/src/service/block_producer.rs (8)
60-69: Ohayo sensei! Theis_block_limit_exhaustedmethod neatly isolates the error check logic.
89-90: Ohayo sensei! Returning leftover transactions with the main results is a clever enhancement for block capacity handling.
238-239: Ohayo sensei! Tracking whether the block is full with a boolean is straightforward and effective.
337-337: Ohayo sensei! Accepting the transactions vector bymutreference is a clean approach for partial execution logic.
341-341: Ohayo sensei! Extracting(total_executed, is_full)fromexecute_transactionssuits the new resource-limiting scenario.
349-349: Ohayo sensei! The skip offset referencing the newly executed transactions is correct for fetching relevant results.
360-364: Ohayo sensei! Splitting off leftover transactions when block is full is a neat solution to queue them for the next block.
493-504: Ohayo sensei! Re-queueing leftover transactions and waking the context is a smart way to handle block closure mid-interval.crates/katana/executor/src/implementation/blockifier/utils.rs (8)
6-6: Ohayo sensei! AddingBouncerandBouncerConfigimports aligns perfectly with the new resource checks.
17-18: Ohayo sensei! IntroducingTransactionalStateandUpdatableStatefosters layered or test-friendly state management.
61-61: Ohayo sensei! IncludingExecutorResultin the imports centralizes error handling seamlessly.
68-69: Ohayo sensei! The optionalbouncerparameter is a flexible way to enable or skip capacity checks.
70-71: Ohayo sensei! Switching to a genericUpdatableStateis an elegant solution for broader state usage patterns.
128-129: Ohayo sensei! The transactional state approach ensures each invocation is atomic and can be safely rolled back if needed.
132-144: Ohayo sensei! Invokingbouncer.try_update(...)here correctly ensures each transaction's resource usage is accounted for before committing.
153-155: Ohayo sensei! Committing state even on error aligns with the design, preserving partial changes and consistent state transitions.crates/katana/node/src/config/mod.rs (1)
9-9: LGTM! Clean integration of sequencing configurationThe sequencing module is well-integrated into the config structure with proper documentation.
Also applies to: 19-19, 47-48
crates/katana/executor/src/abstraction/executor.rs (1)
8-8: Ohayo! New error type import looks good sensei!The addition of ExecutorError import aligns with the enhanced error handling requirements.
crates/katana/core/tests/backend.rs (1)
8-8: Ohayo! Test configuration looks good sensei!Using
BlockLimits::max()in tests ensures no artificial resource constraints during testing.Consider if we should add tests with specific resource limits to verify the limiting behavior:
Also applies to: 29-29
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there are any tests with non-max block limits rg -l "BlockLimits::new|block_limits.*=(?!.*max)"Length of output: 328
Ohayo, sensei!
The initial regex check hit a snag due to unsupported look-around. To re-run the check correctly, please verify using the updated command below that enables PCRE2, which supports look-ahead:
If this command produces no output, it confirms that all tests (including the one on line 29) are using
BlockLimits::max()consistently. That said, as mentioned in the review comment, we might consider adding tests with specific (non-max) resource limits to verify the limiting behavior where appropriate.
🏁 Script executed:
#!/bin/bash # Re-run regex with PCRE2 support to check for any tests using non-max block limits rg --pcre2 -l "BlockLimits::new|block_limits.*=(?!.*max)"Length of output: 181
Ohayo, sensei!
It looks like our original script included a literal `` line, causing an error that interfered with the expected output. Let’s try a corrected script without the extra text. Please run the following command manually to confirm that no tests are using non-max block limits (this check covers both line 8 and line 29):
If this command produces no output, it confirms that all tests are correctly using
BlockLimits::max(). We might, however, consider adding tests that intentionally use specific (non-max) limits to validate the limiting behavior in varied scenarios.
🏁 Script executed:
#!/bin/bash # Check for non-max block limits usage in tests rg --pcre2 -l "BlockLimits::new|block_limits.*=(?!.*max)"Length of output: 59
Ohayo, sensei!
The recent check shows no tests using non-max block limits. Both the relevant lines (8 and 29) in the tests are consistently using
BlockLimits::max(). This confirms that the test configuration is intentional and sound. That said, adding tests with specific non-max limits could be an interesting way to verify the limiting behavior if needed.crates/katana/executor/benches/concurrent.rs (1)
12-12: Ohayo! Benchmark configuration updated appropriately sensei!Using
BlockLimits::max()in benchmarks ensures resource limits don't affect performance measurements.Consider adding benchmarks with various block limits to measure their impact:
Also applies to: 47-47
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there are any benchmarks with different block limits rg -l "benchmark.*block.*limit"Length of output: 33
Ohayo, sensei! I reviewed the benchmark configuration in crates/katana/executor/benches/concurrent.rs and confirmed that it now imports and uses
BlockLimits::max(), both at the top (line 12) and again around line 47. This ensures that resource limits won’t interfere with the performance measurements.However, no alternative benchmarks using different block limit values were found. If you’d like to evaluate how varying block limits affect performance, you might consider adding extra benchmarks that use non-maximal values.
crates/dojo/test-utils/src/sequencer.rs (1)
9-9: Ohayo! Test utility enhancement looks good sensei!The addition of
SequencingConfigparameter allows tests to specify custom sequencing behavior, including block limits.Consider if we should add helper methods for common sequencing configurations:
Also applies to: 117-117
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for existing helper methods rg -l "fn.*sequencing.*config"Length of output: 32
Ohayo, sensei! The updated test utility enhancements look solid with the inclusion of the
SequencingConfigparameter for flexible sequencing behavior. However, our initial search for helper methods related to common sequencing configurations (using the patternfn.*sequencing.*config) didn’t turn up any results. If you expected to see such helpers or if they would simplify test setups, please verify manually if adding them is desired. This suggestion also applies to lines 117‑117.crates/katana/executor/src/implementation/blockifier/error.rs (1)
105-113: Ohayo! Clean error mapping implementation, sensei!The new
Fromimplementation elegantly handles the block resource limits by mappingBlockFulltoLimitsExhausted, which aligns perfectly with the PR's objective of managing block resource limits.crates/katana/executor/src/implementation/noop.rs (1)
101-107: Ohayo! Clean update to execute_transactions, sensei!The return type change to include transaction count and optional error is well-implemented, maintaining the no-op behavior while supporting the new block resource limit feature.
crates/katana/executor/src/abstraction/mod.rs (1)
20-25: Ohayo! Well-documented struct, sensei!The
BlockLimitsstruct is clearly documented with a reference to the official Starknet documentation.crates/katana/executor/tests/fixtures/mod.rs (1)
274-276: Ohayo! Clean factory update, sensei!The addition of
BlockLimits::max()to the factory instantiation is appropriate for test fixtures, ensuring no artificial restrictions during testing.crates/katana/chain-spec/src/rollup/utils.rs (1)
339-339: Ohayo! The BlockLimits integration looks good, sensei!The addition of BlockLimits import and its usage in the executor function with
max()is appropriate for test scenarios where we want to avoid resource constraints.Also applies to: 389-390
crates/katana/cli/src/args.rs (2)
195-199: The sequencing config update looks good, sensei!The integration of the new block_cairo_steps_limit into the SequencingConfig is clean and consistent.
155-156: Ohayo! The log filter update looks good, sensei!The log filter modification maintains essential debug levels while removing redundant entries.
| #[arg(long = "sequencing.block-max-cairo-steps")] | ||
| #[arg(value_name = "TOTAL")] | ||
| pub block_cairo_steps_limit: Option<u64>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding value validation for the block steps limit, sensei!
While the new CLI argument is well-documented, it should validate that the provided value is positive.
Consider adding a value parser that validates the input:
#[arg(long = "sequencing.block-max-cairo-steps")]
#[arg(value_name = "TOTAL")]
#[arg(value_parser = clap::value_parser!(u64).range(1..))]
pub block_cairo_steps_limit: Option<u64>,
kariy
force-pushed
the
katana/block-limit
branch
from
8c639e3 to
3038816
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/katana/cli/src/args.rs (1)
349-412: Add config file merge logic for block_cairo_steps_limit.The new
block_cairo_steps_limitfield is not included in the config file merging logic. This means the value from the config file won't be used when not specified via CLI.Add the merge logic in the
with_config_filemethod:if self.block_time.is_none() { self.block_time = config.block_time; } + if self.block_cairo_steps_limit.is_none() { + self.block_cairo_steps_limit = config.block_cairo_steps_limit; + } + if self.db_dir.is_none() { self.db_dir = config.db_dir; }
🧹 Nitpick comments (3)
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
101-103: Consider initializing block_max_capacity with limits.The block capacity configuration could be initialized directly from the limits parameter.
-let mut block_max_capacity = BouncerWeights::max(); -block_max_capacity.n_steps = limits.cairo_steps as usize; +let block_max_capacity = BouncerWeights { + n_steps: limits.cairo_steps as usize, + ..BouncerWeights::max() +};crates/katana/core/src/service/block_producer.rs (1)
492-501: Consider adding debug logging for leftover transactions.Adding debug logs would help track the number of transactions that couldn't fit in the current block.
if let Some(leftovers) = leftovers { pin.is_block_full = true; + trace!( + target: LOG_TARGET, + leftover_count = leftovers.len(), + "Block full, queuing leftover transactions for next block" + ); // Push leftover transactions back to front of queue pin.queued.push_front(leftovers);crates/katana/executor/src/implementation/blockifier/utils.rs (1)
1-900: Consider documenting the resource limit behavior.The implementation of resource limits through the bouncer mechanism is solid. Consider adding documentation about:
- How resource limits are enforced
- What happens when limits are reached
- How transactions are handled when rejected due to resource constraints
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(2 hunks)crates/katana/cli/src/args.rs(3 hunks)crates/katana/core/src/service/block_producer.rs(10 hunks)crates/katana/core/tests/backend.rs(2 hunks)crates/katana/executor/benches/concurrent.rs(2 hunks)crates/katana/executor/benches/execution.rs(1 hunks)crates/katana/executor/src/abstraction/error.rs(1 hunks)crates/katana/executor/src/abstraction/executor.rs(2 hunks)crates/katana/executor/src/abstraction/mod.rs(1 hunks)crates/katana/executor/src/implementation/blockifier/error.rs(3 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(9 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(4 hunks)crates/katana/executor/src/implementation/noop.rs(2 hunks)crates/katana/executor/tests/fixtures/mod.rs(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)crates/katana/node/src/config/sequencing.rs(1 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/rpc/rpc/tests/dev.rs(1 hunks)crates/katana/rpc/rpc/tests/forking.rs(1 hunks)crates/katana/rpc/rpc/tests/messaging.rs(1 hunks)crates/katana/rpc/rpc/tests/proofs.rs(1 hunks)crates/katana/rpc/rpc/tests/saya.rs(1 hunks)crates/katana/rpc/rpc/tests/starknet.rs(1 hunks)crates/katana/rpc/rpc/tests/torii.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/katana/executor/src/implementation/blockifier/error.rs
- crates/katana/rpc/rpc/tests/messaging.rs
- crates/katana/executor/benches/concurrent.rs
- crates/katana/core/tests/backend.rs
- crates/katana/rpc/rpc/tests/proofs.rs
- crates/katana/executor/benches/execution.rs
- crates/katana/rpc/rpc/tests/torii.rs
- crates/katana/node/src/config/sequencing.rs
- crates/katana/executor/tests/fixtures/mod.rs
- crates/katana/rpc/rpc/tests/starknet.rs
- crates/dojo/test-utils/src/sequencer.rs
- crates/katana/executor/src/abstraction/error.rs
- crates/katana/executor/src/abstraction/mod.rs
- crates/katana/node/src/lib.rs
- crates/katana/rpc/rpc/tests/forking.rs
- crates/katana/rpc/rpc/tests/dev.rs
- crates/katana/rpc/rpc/tests/saya.rs
- crates/katana/chain-spec/src/rollup/utils.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: build
- GitHub Check: docs
🔇 Additional comments (15)
crates/katana/node/src/config/mod.rs (1)
9-9: LGTM! Clean and well-organized module structure, sensei!The changes follow Rust's module system best practices, with proper organization of imports and clear documentation.
Also applies to: 19-19, 47-48
crates/katana/cli/src/args.rs (1)
62-64: Add value validation for the block steps limit.While the new CLI argument is well-documented, it should validate that the provided value is positive.
crates/katana/executor/src/abstraction/executor.rs (2)
8-8: LGTM!The import of
ExecutorErroraligns with the enhanced error handling mechanism.
42-46: Improved return type provides better execution insights.The updated return type now includes both the count of executed transactions and potential execution errors, enabling better handling of resource limits and execution failures.
crates/katana/executor/src/implementation/noop.rs (2)
16-16: LGTM!The import of both error types maintains consistency with the trait requirements.
104-106: Appropriate no-op implementation.The implementation correctly returns zero executed transactions and no error, maintaining the expected behavior of a no-op executor.
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
3-3: LGTM!The import of bouncer types enables resource limit management.
crates/katana/core/src/service/block_producer.rs (3)
1-10: Ohayo sensei! Beautiful ASCII art and quote.The ASCII art and quote add a nice touch to the codebase.
62-67: Well-structured error handling.The
is_block_limit_exhaustedmethod provides a clean way to check for resource exhaustion.
341-341: Elegant handling of leftover transactions.The code efficiently handles transactions that couldn't be included in the current block by:
- Tracking the number of executed transactions
- Splitting off non-executed transactions
- Returning them for future processing
Also applies to: 361-362
crates/katana/executor/src/implementation/blockifier/utils.rs (5)
6-6: Ohayo sensei! The new imports look good!The addition of
Bouncerand state management types aligns well with the PR's objective of implementing resource limits.Also applies to: 17-18
63-69: Function signature change looks good!The addition of the optional
bouncerparameter enables resource limit checks while maintaining backward compatibility.
70-75: Nice improvement to state management!The change from
StateReadertoUpdatableStategeneric type makes the function more flexible and reusable.
727-898: Great test coverage, sensei!The test suite thoroughly covers:
- Chain ID conversions and invariants
- Call info transformations
- Event handling
126-144: Verify state management in error cases.The state changes are committed even in error cases. While this is generally correct for maintaining consistency, please ensure that this behavior is intended and documented.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for tests or documentation about state commitment behavior rg -A 5 "state.*commit" --type rustLength of output: 8784
Ohayo, sensei! Verification Summary – State Is Committed on All Branches
- Our search confirms that
tx_state.commit()is explicitly invoked in both the success and error branches.- While no separate test explicitly documents error-case state commitment behavior, related documentation in the chain-spec and state provider (e.g., in
crates/katana/storage/provider/src/traits/state.rs) implies that maintaining a consistent state commitment is the intended design.- This indicates that persisting state changes even when errors occur is deliberate. However, if further clarity is desired, adding inline comments or documentation would be beneficial for future readers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/katana/core/src/service/block_producer_tests.rs (1)
71-73: Ohayo sensei! Consider adding resource limit tests.While the current assertions verify basic block mining functionality, consider adding test cases that specifically verify the new block resource limit feature. This would help ensure that blocks are automatically closed when the
--sequencing.block-max-cairo-stepslimit is reached.Would you like me to help generate additional test cases that cover the resource limit scenarios?
crates/katana/core/src/service/block_producer.rs (2)
239-239: Consider using an atomic boolean for thread safety.The
is_block_fullflag might be accessed from multiple threads. Consider usingAtomicBoolfor better thread safety.- is_block_full: bool, + is_block_full: std::sync::atomic::AtomicBool,
487-502: Consider adding metrics for block fullness.It would be valuable to track how often blocks are being closed due to resource limits.
if let Some(leftovers) = leftovers { pin.is_block_full = true; + metrics::increment_counter!("katana_block_full_count"); + metrics::gauge!("katana_leftover_transactions", leftovers.len() as f64); // Push leftover transactions back to front of queue pin.queued.push_front(leftovers);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/katana/core/src/service/block_producer.rs(11 hunks)crates/katana/core/src/service/block_producer_tests.rs(1 hunks)crates/katana/executor/src/implementation/noop.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (8)
crates/katana/executor/src/implementation/noop.rs (2)
16-16: Ohayo sensei! LGTM on the error type import.The addition of
ExecutorErroralongsideExecutionErroraligns well with the PR's objective of handling resource limits.
101-106: Ohayo sensei! The updated return type looks good.The change to return
(usize, Option<ExecutorError>)provides the necessary information for tracking transaction counts and potential resource limit errors, which is essential for the block auto-closing feature.The NoopExecutor's implementation correctly maintains its "do-nothing" behavior while conforming to the new interface.
crates/katana/core/src/service/block_producer_tests.rs (2)
49-49: Ohayo sensei! Clear and accurate comment.The comment effectively describes the expected timer behavior when no block is opened.
57-69: Ohayo sensei! Robust polling implementation.The new implementation using
poll_nextwith conditional checks effectively tests both scenarios:
- When the block is still being produced (pending)
- When the block has been mined (completed)
This aligns well with the PR's objective of handling block closure conditions.
crates/katana/core/src/service/block_producer.rs (4)
60-68: LGTM! Clean error handling implementation.The new
is_block_limit_exhaustedmethod provides a clear way to check if a block's resource limit has been reached.
89-90: LGTM! Type signature enhancement.The updated
TxExecutionResulttype now properly handles leftover transactions when a block is full.
335-364: Ohayo sensei! The transaction execution logic looks solid.The function now properly handles block resource limits by:
- Tracking the total number of executed transactions
- Identifying when a block is full
- Returning non-executed transactions for the next block
421-457: LGTM! Robust block mining logic.The block mining logic has been enhanced to:
- Check if block is full before mining
- Reset timer appropriately
- Handle leftover transactions for the next block
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3003 +/- ##
==========================================
+ Coverage 56.98% 56.99% +0.01%
==========================================
Files 430 431 +1
Lines 57046 57151 +105
==========================================
+ Hits 32509 32575 +66
- Misses 24537 24576 +39 ☔ View full report in Codecov by Sentry. |
kariy
force-pushed
the
katana/block-limit
branch
from
2e9d60c to
ba30be7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
205-211: Consider adding error handling for bouncer initialization.While the implementation is correct, it would be good to handle potential errors during bouncer initialization.
- let result = utils::transact( - &mut state.inner, - block_context, - flags, - exec_tx, - Some(&mut self.bouncer), - ); + let result = match utils::transact( + &mut state.inner, + block_context, + flags, + exec_tx, + Some(&mut self.bouncer), + ) { + Ok(result) => Ok(result), + Err(e) => { + info!(target: LOG_TARGET, "Transaction failed due to bouncer: {}", e); + Err(e) + } + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/chain-spec/src/rollup/utils.rs(2 hunks)crates/katana/cli/src/args.rs(3 hunks)crates/katana/core/src/service/block_producer.rs(11 hunks)crates/katana/core/src/service/block_producer_tests.rs(1 hunks)crates/katana/core/tests/backend.rs(2 hunks)crates/katana/executor/benches/concurrent.rs(2 hunks)crates/katana/executor/benches/execution.rs(1 hunks)crates/katana/executor/src/abstraction/error.rs(1 hunks)crates/katana/executor/src/abstraction/executor.rs(2 hunks)crates/katana/executor/src/abstraction/mod.rs(1 hunks)crates/katana/executor/src/implementation/blockifier/error.rs(3 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(9 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(4 hunks)crates/katana/executor/src/implementation/noop.rs(2 hunks)crates/katana/executor/tests/fixtures/mod.rs(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)crates/katana/node/src/config/sequencing.rs(1 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/rpc/rpc/tests/dev.rs(1 hunks)crates/katana/rpc/rpc/tests/forking.rs(1 hunks)crates/katana/rpc/rpc/tests/messaging.rs(1 hunks)crates/katana/rpc/rpc/tests/proofs.rs(1 hunks)crates/katana/rpc/rpc/tests/saya.rs(1 hunks)crates/katana/rpc/rpc/tests/starknet.rs(1 hunks)crates/katana/rpc/rpc/tests/torii.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- crates/katana/executor/benches/concurrent.rs
- crates/katana/executor/benches/execution.rs
- crates/katana/rpc/rpc/tests/dev.rs
- crates/katana/executor/src/implementation/blockifier/error.rs
- crates/katana/rpc/rpc/tests/saya.rs
- crates/katana/rpc/rpc/tests/messaging.rs
- crates/katana/rpc/rpc/tests/forking.rs
- crates/katana/executor/src/abstraction/error.rs
- crates/katana/executor/src/abstraction/mod.rs
- crates/katana/rpc/rpc/tests/proofs.rs
- crates/katana/rpc/rpc/tests/torii.rs
- crates/katana/node/src/lib.rs
- crates/katana/executor/src/abstraction/executor.rs
- crates/katana/executor/tests/fixtures/mod.rs
- crates/katana/core/tests/backend.rs
- crates/dojo/test-utils/src/sequencer.rs
- crates/katana/core/src/service/block_producer_tests.rs
- crates/katana/node/src/config/mod.rs
- crates/katana/node/src/config/sequencing.rs
- crates/katana/chain-spec/src/rollup/utils.rs
- crates/katana/cli/src/args.rs
- crates/katana/rpc/rpc/tests/starknet.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (11)
crates/katana/core/src/service/block_producer.rs (4)
239-239: Ohayo sensei! Clean state management implementation.The
is_block_fullflag effectively tracks block capacity state, enabling automatic block closure when resource limits are reached.
335-364: Excellent transaction overflow handling, sensei!The implementation elegantly handles transactions that exceed block capacity by:
- Tracking executed transaction count
- Preserving non-executed transactions for the next block
- Maintaining transaction order
427-433: Smart block closure implementation!The dual-trigger approach ensures blocks are closed either:
- When the timer expires (normal case)
- Immediately when resource limits are reached
This provides optimal balance between regular block production and resource management.Also applies to: 444-457
491-500: Excellent queue management, sensei!The implementation ensures transaction ordering and continuity by:
- Preserving leftover transactions at the front of the queue
- Triggering immediate processing in the next block
- Maintaining transaction sequence across blocks
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
69-70: Clean bouncer integration, sensei!The transaction execution logic properly integrates resource tracking by:
- Using transactional state for atomic updates
- Checking resource limits before committing changes
- Properly handling state rollback on limit exhaustion
Also applies to: 127-128, 132-142
crates/katana/executor/src/implementation/noop.rs (2)
16-16: LGTM!The import of
ExecutorErroris necessary for the updated return type ofexecute_transactions.
104-105: LGTM!The updated return type and implementation correctly handle the new requirements for tracking executed transactions and potential resource limit errors.
crates/katana/executor/src/implementation/blockifier/mod.rs (4)
3-3: LGTM!Ohayo sensei! The import of Bouncer-related types is necessary for implementing resource limits.
37-37: LGTM!The addition of
BlockLimitstoBlockifierFactoryand its constructor provides a clean way to configure resource limits.Also applies to: 42-43
86-86: LGTM!The bouncer configuration correctly uses the Cairo steps limit from
BlockLimits.Also applies to: 101-103
241-242: LGTM!The error handling correctly distinguishes between resource exhaustion and other errors, allowing the block producer to handle them appropriately.
1 participant
Add resource limit parameter for interval block mining. Blocks will now be prompted to close when the total execution resources of their transactions reaches this configurable threshold. Configure via the new
--sequencing.block-max-cairo-stepsCLI argument.Transactions that have been queued to the block producer, but is unable to be included in the current block due to the resources limit, will be pushed to the next block.
Summary by CodeRabbit
New Features
Enhancements