feat: miner read all values from storage

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves significant changes across multiple files, including logic changes in mining and execution functions, and the introduction of new helper functions. Understanding the impact of these changes on the system's behavior requires a good grasp of the existing architecture and the intended functionality of the modifications.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of the block parameter in mine_external() and mine_mixed() methods in BlockMiner class might lead to issues if the external block data is not properly managed or is missing in the storage, as these methods now rely entirely on the storage to provide the necessary block data.
	Logic Error: The new partitioning logic in partition_transactions() might introduce errors if the transaction kinds are not correctly identified and separated, leading to incorrect mining behavior.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/block_miner.rs
suggestion	Consider adding error handling for the case where block_number() does not return a valid block number in mine_external_transactions(). This could prevent potential runtime errors or inconsistencies if the block number is missing or invalid. [important]
relevant line	if tx.block_number() != block_number {

---

relevant file	src/eth/block_miner.rs
suggestion	It might be beneficial to implement logging for successful mining operations or critical steps within the mining functions to aid in debugging and monitoring the mining process. [medium]
relevant line	block_from_external(external_block, mined_txs)

---

relevant file	src/eth/block_miner.rs
suggestion	To enhance code clarity and maintainability, consider refactoring the mine_external() and mine_mixed() methods to separate the concerns of data retrieval, transaction partitioning, and block construction into distinct methods or structs. [medium]
relevant line	pub async fn mine_external(&self) -> anyhow::Result {

---

relevant file	src/eth/block_miner.rs
suggestion	Ensure that the new helper functions like read_external_block_and_executions() are covered by unit tests to verify their behavior, especially their interaction with the storage system and error handling. [important]
relevant line	async fn read_external_block_and_executions(storage: &StratusStorage) -> anyhow::Result<(ExternalBlock, Vec)> {

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestions
Bug	Replace the not function with the ! operator for proper negation in Rust. Replace the not function with the ! operator for negating the condition in Rust. src/eth/block_miner.rs [60-62] -if not(local_txs.is_empty()) { +if !local_txs.is_empty() { return log_and_err!("cannot mine external block because one of the transactions is not an external transaction"); }
	Add error handling to prevent potential runtime panic. Add error handling for the case when block_from_external returns an error, to prevent unwrapping on an Err value which could cause a panic. src/eth/block_miner.rs [65-66] let mined_txs = mine_external_transactions(external_block.number(), external_txs)?; -block_from_external(external_block, mined_txs) +block_from_external(external_block, mined_txs)?
	Correct the logic to properly handle successful transaction executions. Ensure that the is_success() method is correctly checked against the expected condition by negating the current check. src/eth/block_miner.rs [81-83] -if execution.is_success() { - return log_and_err!("cannot mine mixed block because one of the local execution is not a failure"); +if !execution.is_success() { + block.push_execution(tx, execution); }
Enhancement	Simplify error handling using pattern matching and error propagation. Use pattern matching to simplify the error handling in read_external_block_and_executions. src/eth/block_miner.rs [206-210] -let block = match storage.temp.read_external_block().await { - Ok(Some(block)) => block, - Ok(None) => return log_and_err!("no active external block being re-executed"), - Err(e) => return Err(e), -}; +let block = storage.temp.read_external_block().await? + .ok_or_else(|| anyhow::anyhow!("no active external block being re-executed"))?;
	Improve error messages to include specific transaction details. Use a more descriptive error message in mine_external_transactions to include which transaction caused the issue. src/eth/block_miner.rs [239-241] if tx.block_number() != block_number { - return log_and_err!("cannot mine external block because one of the transactions does not belong to the external block"); + return log_and_err!(format!("cannot mine external block because transaction {} does not belong to the external block", tx.hash())); }