feat: dumb interval miner

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves multiple changes across various components including configuration, logging, and thread management. The complexity of the changes, especially with the introduction of new features like interval mining and handling of different storage types, requires careful consideration to ensure thread safety, error handling, and overall system stability.
🧪 Relevant tests	No
⚡ Possible issues	Thread Safety: The method spawn_interval_miner in BlockMiner uses threads without explicit handling of potential race conditions or shared state issues.
	Error Handling: The loop in interval_miner function continues indefinitely even after repeated failures, which might not be the desired behavior in production environments.
	Logging Consistency: The change in logging from "configuring" to "starting" across multiple modules might affect existing log parsing tools or monitoring setups.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/block_miner.rs
suggestion	Consider implementing error handling strategies for the thread spawned in spawn_interval_miner. For instance, restarting the miner thread after a certain number of consecutive failures could be beneficial. [important]
relevant line	.expect("spawning interval miner should not fail");

---

relevant file	src/eth/block_miner.rs
suggestion	To enhance the robustness of the interval miner, it's advisable to add a mechanism to stop the mining loop under certain conditions, such as a shutdown signal from the application. This can prevent unnecessary resource usage when the application is trying to close. [important]
relevant line	loop {

---

relevant file	src/eth/block_miner.rs
suggestion	It's recommended to handle the potential panic from thread::Builder::new() by checking the result before calling name("interval-miner".into()). This ensures that the thread configuration does not cause a runtime panic. [medium]
relevant line	let t = thread::Builder::new().name("interval-miner".into());

---

relevant file	src/config.rs
suggestion	Ensure that the parse_duration function used in MinerConfig is robust against various formats and errors in duration parsing to prevent runtime panics or misconfigurations. [medium]
relevant line	#[arg(long = "block-time", value_parser=parse_duration, env = "BLOCK_TIME")]

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Improve error handling for thread spawning to prevent potential crashes Consider handling the potential failure of thread::Builder::new().name("interval-miner".into()).spawn(...) more gracefully. Currently, the code uses expect which will cause a panic if the thread cannot be spawned. It's generally better to handle such errors in a way that doesn't cause the entire program to crash, especially in a production environment. You could log the error and continue, or use a retry mechanism with a limit. src/eth/block_miner.rs [48-50] let t = thread::Builder::new().name("interval-miner".into()); -t.spawn(move || interval_miner(self, block_time)) - .expect("spawning interval miner should not fail"); +if let Err(e) = t.spawn(move || interval_miner(self, block_time)) { + tracing::error!("Failed to spawn interval miner: {:?}", e); + // Handle the error, e.g., retry or escalate +} Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a potential crash due to the use of expect in thread spawning, which is a critical issue in production environments. Implementing graceful error handling is crucial for robustness.	8
Performance	Replace thread::sleep with tokio::time::sleep for better responsiveness in asynchronous operations The loop inside interval_miner function uses thread::sleep(block_time) which might introduce delays in handling other tasks or shutting down. Consider using a more responsive approach like tokio::time::sleep which can be awaited and allows the task to be paused and resumed, making it more efficient and responsive, especially in an asynchronous environment. src/eth/block_miner.rs [256-257] loop { - thread::sleep(block_time); + tokio::time::sleep(block_time).await; tracing::info!("mining block"); Suggestion importance[1-10]: 7 Why: This suggestion correctly points out the inefficiency of thread::sleep in an asynchronous context and proposes a more suitable alternative with tokio::time::sleep, enhancing responsiveness and efficiency.	7
Enhancement	Implement a backoff strategy for retries in block committing to improve error handling The error handling in the mining and committing blocks loop could be improved by introducing a backoff strategy for retries. This would prevent immediate retries in case of persistent issues, reducing the load on the system and potentially allowing time for recovery. src/eth/block_miner.rs [270-273] +let mut attempts = 0; loop { match futures::executor::block_on(miner.commit(block.clone())) { Ok(_) => break, + Err(e) if attempts < MAX_RETRIES => { + attempts += 1; + let wait_time = std::time::Duration::from_secs(attempts * 2); // exponential backoff + thread::sleep(wait_time); + tracing::warn!(reason = ?e, "retrying commit block"); + } Err(e) => { - tracing::error!(reason = ?e, "failed to commit block"); - continue; + tracing::error!(reason = ?e, "failed to commit block after retries"); + break; } } } Suggestion importance[1-10]: 7 Why: The suggestion to implement a backoff strategy for retries is valuable for reducing system load and allowing recovery time during errors, which can significantly improve the system's stability and performance under failure conditions.	7
Maintainability	Add control mechanism for the mining loop to allow for graceful termination The current implementation of the interval_miner function uses a continuous loop which can lead to high CPU usage if not managed correctly. Consider implementing a mechanism to control the execution, such as checking for a shutdown signal or condition that allows for a graceful termination of the loop. src/eth/block_miner.rs [256-257] -loop { +while running.load(Ordering::SeqCst) { thread::sleep(block_time); tracing::info!("mining block"); +} Suggestion importance[1-10]: 6 Why: Adding a control mechanism for loop termination is a good practice for maintainability and controlled shutdown, although it's not as critical as error handling or performance issues.	6