feat: introduce control over runloop #16
Conversation
WalkthroughThis pull request updates the package version and adjusts multiple components within the simulation network’s control flow. The CLI command for starting the simulation is renamed, and several functions are modified to incorporate new channel parameters and enums to handle commands and clock events. The print messages in the scaffolding component have been updated, and the communication mechanism for mempool transactions has shifted from a Tokio broadcast channel to a Crossbeam channel. Additionally, a new RunloopTriggerMode enum and extra parameters in configuration improve control over simulation behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as "CLI Parser"
participant TUI as "TUI Simnet"
participant Core as "Simnet Core"
participant Clock as "Clock Manager"
U->>CLI: Issue command ("run")
CLI->>TUI: Parse and forward command
TUI->>Core: Call start_app(simnet_events, simnet_commands)
Core->>Clock: Dispatch clock commands (via channel)
Clock-->>Core: Respond with clock events/ticks
Core->>TUI: Update simulation with events/commands
TUI->>CLI: Reflect updated state to user
Note over Core: Concurrently handling events and commands using crossbeam's select.
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
crates/core/src/simnet/mod.rs (3)
50-55: Consider documenting each SimnetCommand variant
It may help future maintainers if each variant has a short docstring describing scenarios in which it should be used, particularly where multiple runloop commands coexist.
169-190: Manual mode logic & SlotForward command
Switching to manual mode upon receiving SlotForward is user-friendly. Consider removing or implementing SlotBackward if it’s not needed, to avoid confusion.
204-265: Sequential account fetch & potential optimizations
As volumes grow, repeatedly fetching unknown program IDs from the remote RPC could become a bottleneck. You could add caching or parallel fetching if performance is impacted.crates/cli/src/cli/simnet/mod.rs (1)
99-105: Consider error handling for channel sends.The start_app call is updated correctly with the new parameters, but consider adding error handling for potential channel send failures in the TUI.
- tui::simnet::start_app( - simnet_events_rx, - simnet_commands_tx, - cmd.debug, - deploy_progress_rx, - ) - .map_err(|e| format!("{}", e))?; + tui::simnet::start_app( + simnet_events_rx, + simnet_commands_tx.clone(), + cmd.debug, + deploy_progress_rx, + ) + .map_err(|e| { + if e.to_string().contains("channel") { + format!("Channel communication error: {}", e) + } else { + format!("{}", e) + } + })?;crates/cli/src/tui/simnet.rs (2)
296-316: Consider adding keyboard shortcut help text.The new keyboard shortcuts for runloop control should be documented in the help text to improve discoverability.
-const HELP_TEXT: &str = "(Esc) quit | (↑) move up | (↓) move down"; +const HELP_TEXT: &str = "(Esc) quit | (↑) move up | (↓) move down | (Space) toggle clock | (Tab) forward slot | (t) transaction mode | (c) clock mode";
296-316: Consider error feedback for mode switches.When mode switches fail (e.g., channel send errors), the user should receive visual feedback.
Char('t') => { - let _ = app + if let Err(e) = app .simnet_commands_tx - .send(SimnetCommand::UpdateRunloopMode( - RunloopTriggerMode::Transaction, - )); + .send(SimnetCommand::UpdateRunloopMode(RunloopTriggerMode::Transaction)) + { + app.events.push_front(( + EventType::Failure, + Local::now(), + format!("Failed to switch to transaction mode: {}", e), + )); + } else { + app.events.push_front(( + EventType::Info, + Local::now(), + "Switched to transaction mode".into(), + )); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(1 hunks)crates/cli/src/cli/mod.rs(1 hunks)crates/cli/src/cli/simnet/mod.rs(3 hunks)crates/cli/src/scaffold/mod.rs(4 hunks)crates/cli/src/tui/simnet.rs(8 hunks)crates/core/src/lib.rs(1 hunks)crates/core/src/rpc/mod.rs(3 hunks)crates/core/src/simnet/mod.rs(5 hunks)crates/core/src/types.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- crates/cli/src/scaffold/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: run_cargo_checks
🔇 Additional comments (21)
crates/core/src/simnet/mod.rs (10)
2-3: Use of Crossbeam concurrency utilities is appropriate
Leveraging crossbeam::select and crossbeam_channel for concurrency here looks correct and aligns well with your needs to handle multiple event sources concurrently.
24-24: Importing RunloopTriggerMode and SurfpoolConfig
Adding these items to the import list is consistent with the new runloop control logic.
64-66: ClockEvent extension
Currently defines only a Tick event. If you anticipate more granular events in the future (e.g., synchronization or special timepoints), this enum provides a good foundation for expansion.
71-71: Signature updated for receiving SimnetCommand
Including simnet_commands_rx in the start function properly accommodates your new runloop command control.
95-95: Potential concern with unbounded mempool channel
While unbounded channels are simpler, unregulated transaction throughput could result in high memory usage. If this is a concern, consider switching to a bounded channel or add backpressure logic.
122-124: Separate channels for clock events vs. commands
Using distinct channels (clock_event_tx and clock_command_tx) makes it clearer how each type of message is handled.
151-153: Initial runloop_trigger_mode setup
Sending the initial EpochInfoUpdate and setting runloop_trigger_mode from config is a clean way to bootstrap the simulation state.
155-168: Clock-based slot creation
Only creating a slot on clock ticks when the mode is RunloopTriggerMode::Clock keeps behavior consistent with your design goals.
191-196: Batching mempool transactions
Accumulating transactions in memory before processing can be efficient. If real-time processing is a priority, consider processing them as soon as they arrive.
200-202: Skipping the slot creation early
Short-circuiting the loop if create_slot is false makes the main loop more performant by avoiding unnecessary logic.crates/core/src/types.rs (2)
1-6: RunloopTriggerMode enumerates key triggering strategies
Clock, Manual, and Transaction modes are distinct and straightforward, providing flexibility for your runloop control.
18-18: Adding runloop_trigger_mode to SimnetConfig
This insertion seamlessly ties the runloop mode to the core configuration.crates/core/src/lib.rs (3)
15-19: Import updates for concurrency and command handling
Including Receiver and SimnetCommand is consistent with the new approach to controlling the simulation.
27-27: Expanded start_simnet signature
Accepting simnet_commands_rx ensures that command-based runloop updates are supported throughout the library.
29-29: Forwarding the channel correctly
Passing simnet_commands_rx into simnet::start finalizes the integration of runloop commands into your async function.crates/core/src/rpc/mod.rs (2)
3-3: LGTM! Using crossbeam for efficient channel communication.The switch to crossbeam channels is a good choice for this use case, as they provide better performance characteristics compared to tokio's broadcast channels when multi-consumer functionality isn't needed.
29-29: Verify channel usage pattern.The change from broadcast to crossbeam channels implies a shift in the message consumption pattern. Please ensure that there is only one consumer for these transaction messages, as crossbeam channels don't support the broadcast pattern where multiple consumers receive the same message.
Run this script to verify the channel usage:
Also applies to: 80-80
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for multiple consumers of mempool_tx channel # Search for mempool_tx receiver creation rg -A 5 "mempool_tx.*recv" # Search for channel cloning which might indicate multiple consumers rg "mempool_tx.*clone"Length of output: 228
Channel consumer usage verified.
The investigation shows that the cloning is limited to the sender side (as seen in the mod.rs clone operation) with no evidence of multiple receiver (consumer) clones. This confirms that only one consumer exists for the transaction messages.
crates/cli/src/cli/mod.rs (1)
52-52: LGTM! Improved command naming.The renaming from "simnet" to "run" follows CLI best practices by using a common verb. Good that backward compatibility is maintained by keeping "simnet" as an alias.
crates/cli/src/cli/simnet/mod.rs (2)
26-26: LGTM! Default to Clock mode.Good choice to initialize with RunloopTriggerMode::Clock as the default mode, providing a predictable starting state.
30-31: LGTM! Proper channel setup for commands and events.Good separation of concerns using distinct channels for commands and events.
crates/cli/src/tui/simnet.rs (1)
289-291: LGTM! Proper Ctrl+C handling.Good addition of proper Ctrl+C handling for clean application termination.
| pub enum ClockCommand { | ||
| Pause, | ||
| Resume, | ||
| Toggle, | ||
| UpdateSlotInterval(u64), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add capability for a graceful clock shutdown
The clock commands cover pausing and resuming; consider adding a “Shutdown” variant so the clock thread can exit cleanly.
Here is a suggested addition:
pub enum ClockCommand {
Pause,
Resume,
Toggle,
UpdateSlotInterval(u64),
+ Shutdown,
}Then handle it in the clock thread by breaking out of the loop.
📝 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.
| pub enum ClockCommand { | |
| Pause, | |
| Resume, | |
| Toggle, | |
| UpdateSlotInterval(u64), | |
| } | |
| pub enum ClockCommand { | |
| Pause, | |
| Resume, | |
| Toggle, | |
| UpdateSlotInterval(u64), | |
| Shutdown, | |
| } |
| let mut slot_time = config.simnet.slot_time; | ||
| let _handle = hiro_system_kit::thread_named("clock").spawn(move || { | ||
| let mut enabled = true; | ||
| loop { | ||
| match clock_command_rx.try_recv() { | ||
| Ok(ClockCommand::Pause) => { | ||
| enabled = false; | ||
| } | ||
| Ok(ClockCommand::Resume) => { | ||
| enabled = true; | ||
| } | ||
| Ok(ClockCommand::Toggle) => { | ||
| enabled = !enabled; | ||
| } | ||
| Ok(ClockCommand::UpdateSlotInterval(updated_slot_time)) => { | ||
| slot_time = updated_slot_time; | ||
| } | ||
| Err(_e) => {} | ||
| } | ||
| sleep(Duration::from_millis(slot_time)); | ||
| if enabled { | ||
| let _ = clock_event_tx.send(ClockEvent::Tick); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure graceful shutdown of the clock thread
This loop runs indefinitely, and it can only be paused/resumed or have its interval updated. Consider adding a shutdown mechanism (e.g., a ClockCommand::Shutdown) to break out of the loop to free resources.
Example:
loop {
match clock_command_rx.try_recv() {
...
+ Ok(ClockCommand::Shutdown) => break,
...
}
...
}📝 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 mut slot_time = config.simnet.slot_time; | |
| let _handle = hiro_system_kit::thread_named("clock").spawn(move || { | |
| let mut enabled = true; | |
| loop { | |
| match clock_command_rx.try_recv() { | |
| Ok(ClockCommand::Pause) => { | |
| enabled = false; | |
| } | |
| Ok(ClockCommand::Resume) => { | |
| enabled = true; | |
| } | |
| Ok(ClockCommand::Toggle) => { | |
| enabled = !enabled; | |
| } | |
| Ok(ClockCommand::UpdateSlotInterval(updated_slot_time)) => { | |
| slot_time = updated_slot_time; | |
| } | |
| Err(_e) => {} | |
| } | |
| sleep(Duration::from_millis(slot_time)); | |
| if enabled { | |
| let _ = clock_event_tx.send(ClockEvent::Tick); | |
| } | |
| } | |
| }); | |
| let mut slot_time = config.simnet.slot_time; | |
| let _handle = hiro_system_kit::thread_named("clock").spawn(move || { | |
| let mut enabled = true; | |
| loop { | |
| match clock_command_rx.try_recv() { | |
| Ok(ClockCommand::Pause) => { | |
| enabled = false; | |
| } | |
| Ok(ClockCommand::Resume) => { | |
| enabled = true; | |
| } | |
| Ok(ClockCommand::Toggle) => { | |
| enabled = !enabled; | |
| } | |
| Ok(ClockCommand::UpdateSlotInterval(updated_slot_time)) => { | |
| slot_time = updated_slot_time; | |
| } | |
| + Ok(ClockCommand::Shutdown) => break, | |
| Err(_e) => {} | |
| } | |
| sleep(Duration::from_millis(slot_time)); | |
| if enabled { | |
| let _ = clock_event_tx.send(ClockEvent::Tick); | |
| } | |
| } | |
| }); |
This PR improves the developer experience (Ctrl+C to quit) and introduces three “runloop trigger modes”:
• Clock Mode (c) – Default mode. Creates a slot every N milliseconds (default: 400ms). Can be paused and resumed using the Space key.
• Transaction Mode (t) – Creates a slot whenever a transaction is received. Switching to this mode disables Clock Mode.
• Manual Mode (Tab) – Creates a slot each time the Tab key is pressed. Switching to this mode disables Clock Mode.