feat(katana): start block timer only if there txs #2950
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
==========================================
+ Coverage 56.55% 56.70% +0.14%
==========================================
Files 419 420 +1
Lines 55475 55554 +79
==========================================
+ Hits 31376 31502 +126
+ Misses 24099 24052 -47 ☔ View full report in Codecov by Sentry. |
|
LFG |
WalkthroughOhayo, sensei! This pull request introduces enhancements to the Katana core crate, focusing on improving the block producer functionality and test coverage. The changes include adding dev dependencies, refactoring the Changes
Sequence DiagramsequenceDiagram
participant BP as BlockProducer
participant Backend as Backend
participant Blockchain as Blockchain
BP->>Backend: new(chain_spec, blockchain, gas_oracle, executor_factory)
Backend-->>BP: Backend Instance
BP->>BP: Initialize timer
alt Transactions Queued
BP->>Blockchain: Mine Block
Blockchain-->>BP: Block Mined
end
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 (5)
crates/katana/core/src/service/block_producer_tests.rs (2)
34-43: Ohayo sensei! Validate forced mining preconditions.While
interval_force_mine_without_transactionsensures a new block is mined with no pending TXs, consider verifying that no leftover TXs from previous states or partial executions would interfere.
45-70: Ohayo sensei! Great coverage of timed mining logic.The
interval_mine_after_timertest effectively verifies the timer-based block production after queuing a transaction and waiting. Consider adding multi-transaction scenarios or random intervals to further validate concurrency.crates/katana/core/src/backend/mod.rs (1)
49-64: Ohayo sensei! Nice addition of a dedicated constructor.The
Backend::newmethod cleans up initialization usage nicely. However, ensure any partial logics or side effects (like forcibly updatingblock_context_generator) are tested consistently.crates/katana/core/src/service/block_producer.rs (2)
Line range hint
219-243: Ohayo sensei! The factory-based initialization is well-structured.The
IntervalBlockProducer::newmethod keeps the code DRY by reusingbackenddependencies. If you anticipate more advanced block-time scheduling logic, consider splitting the environment update steps into a separate helper function for clarity.
Line range hint
394-434: Ohayo sensei! Thorough state machine for timer + execution + mining.The loop ensures queued transactions are processed before starting the timer, and the timer triggers block mining only if no mining is ongoing. Consider adding logs when the timer is re-queued (lines 404-406) to aid debugging.
+ tracing::debug!(target: LOG_TARGET, "Timer re-queued because mining is ongoing.");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/katana/core/Cargo.toml(1 hunks)crates/katana/core/src/backend/mod.rs(2 hunks)crates/katana/core/src/service/block_producer.rs(6 hunks)crates/katana/core/src/service/block_producer_tests.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (8)
crates/katana/core/src/service/block_producer_tests.rs (3)
14-21: Ohayo sensei! Good test environment setup.The
test_backendhelper function nicely encapsulates the ephemeral DB andNoopExecutorFactorycreation. This approach ensures an isolated test environment without side effects.
23-32: Ohayo sensei! Consider verifying timer behavior even if interval is set.Although
interval_initial_statechecks thatproducer.timerisNone, it might be worth adding a scenario to confirm that if an interval value is specified (e.g.,Some(1000)), no timer is actually started without transactions.
72-81: Ohayo sensei! Random transaction generation is neat.
dummy_transactioncleverly usesarbitraryfor test coverage. If collisions or reproducibility are crucial, consider seeding randomness for repeatable results.crates/katana/core/src/backend/mod.rs (1)
37-37: Ohayo sensei! Simplified generic bound fosters code flexibility.Removing
EF: ExecutorFactoryconstraints from theBackendstruct signature broadens usage possibilities for custom executors. Ensure thorough type constraints remain in internal methods.crates/katana/core/src/service/block_producer.rs (3)
31-34: Ohayo sensei! Kudos for extracting tests into a separate module.Isolating tests into
block_producer_tests.rshelps maintain readability: less clutter in the main file. The#[cfg(test)]usage is straightforward.
188-193: Ohayo sensei! Clarify block-time usage.The
block_timefield now only starts the timer after at least one transaction arrives, preventing empty blocks. This is consistent with the PR’s objective of avoiding empty blocks.
211-215: Ohayo sensei! Great doc comment for timer logic.Documenting the
timerfield’s conditions clarifies usage. This ensures future maintainers know exactly when the producer’s interval-based block production is active.crates/katana/core/Cargo.toml (1)
49-52: Ohayo sensei! Additional dev dependencies look good.Adding
arbitraryandrandas dev-dependencies helps with generating more robust tests. Consider pinning minor versions if test determinism becomes critical.
| @@ -194,23 +202,21 @@ pub struct IntervalBlockProducer<EF: ExecutorFactory> { | |||
| /// Listeners notified when a new executed tx is added. | |||
| tx_execution_listeners: RwLock<Vec<Sender<Vec<TxWithOutcome>>>>, | |||
|
|
|||
| // Usage with `validator` | |||
There was a problem hiding this comment.
Ohayo sensei! Revisit raw unlock usage.
There's an unsafe { self.permit.raw().unlock() }; call. Using raw lock/unlock can risk deadlocks or inconsistent states if not carefully managed. Review if higher-level parking_lot patterns suffice.
- unsafe { self.permit.raw().unlock() };
+ // Consider a more idiomatic .lock() in a smaller scope,
+ // or rely on RAII to avoid manual unlocks.Committable suggestion skipped: line range outside the PR's diff.
Only start the block interval timer when we have executed txs. This means we no longer produce empty blocks after every fixed interval.
Summary by CodeRabbit
Release Notes
New Dependencies
arbitraryandrandas development dependenciesBackend Improvements
Backendstruct flexibilityBlock Producer Updates
intervaltoblock_timefor clarityTesting