feat: finishing blocks

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves multiple changes across different files with significant logic modifications and refactoring. The changes impact core functionalities such as block mining, transaction execution, and storage operations, which require careful review to ensure correctness and maintainability.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of conflict checks in src/eth/storage/inmemory/inmemory_permanent.rs might lead to data inconsistencies if concurrent writes occur.
	Performance Concern: The method finish_block in src/eth/storage/inmemory/inmemory_temporary.rs could lead to performance issues as it processes all states even if they are not needed for the current operation.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/block_miner.rs
suggestion	Consider handling the error from self.storage.temp.finish_block().await?; more gracefully. Currently, if finish_block fails, the error will propagate and potentially cause the miner to stop or behave unexpectedly. It might be beneficial to log this error and continue with a fallback mechanism. [important]
relevant line	let _ = self.storage.temp.finish_block().await?;

---

relevant file	src/eth/storage/inmemory/inmemory_temporary.rs
suggestion	Implement a more efficient data structure for states in InMemoryTemporaryStorage. The current implementation using a Vec can lead to performance issues as it grows, especially with the O(N) complexity for operations that could be O(1). Consider using a HashMap with block numbers as keys for faster access. [important]
relevant line	pub states: RwLock>,

---

relevant file	src/eth/storage/inmemory/inmemory_temporary.rs
suggestion	To prevent potential data races or inconsistencies, ensure that the method finish_block does not allow more than one pending block as mentioned in the TODO comment. This could be enforced by adding a check at the beginning of the method to see if a pending block already exists and handling it accordingly. [important]
relevant line	/// TODO: we cannot allow more than one pending block. Where to put this check?

---

relevant file	src/eth/storage/inmemory/inmemory_temporary.rs
suggestion	Optimize the reset method to clear only the necessary parts of the state or consider a more efficient way to reset the state to avoid performance bottlenecks, especially when called frequently in scenarios with high transaction volumes. [medium]
relevant line	state.tail.clear();

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Remove the incorrect import of Ok from the anyhow crate The import use anyhow::Ok; is incorrect because Ok is not a part of the anyhow crate, but rather it is a variant of the Result enum in Rust's standard library. This line should be removed to avoid confusion and potential compilation errors. src/eth/storage/inmemory/inmemory_temporary.rs [6] -use anyhow::Ok; +// Removed incorrect import Suggestion importance[1-10]: 10 Why: The import use anyhow::Ok; is indeed incorrect as Ok is part of the standard library's Result enum, not the anyhow crate. Removing this prevents compilation errors and confusion.	10
Possible bug	Handle potential errors from finish_block to prevent silent failures Consider handling the error case where finish_block fails instead of ignoring its result. This ensures that the function does not silently fail, which could lead to inconsistent state or unexpected behavior. src/eth/block_miner.rs [46] -let _ = self.storage.temp.finish_block().await?; +let finish_result = self.storage.temp.finish_block().await?; +if finish_result.is_err() { + return Err(finish_result.err().unwrap()); +} Suggestion importance[1-10]: 8 Why: Handling errors from finish_block is crucial to prevent silent failures and potential inconsistencies in the system state, which can be critical in a blockchain context.	8
Enhancement	Ensure that no more than one block is pending at a time in finish_block The method finish_block should include a check to ensure that there is only one pending block at a time, as indicated by the TODO comment. Implement this check before proceeding with the rest of the method's logic to ensure data consistency. src/eth/storage/inmemory/inmemory_temporary.rs [244-245] -/// TODO: we cannot allow more than one pending block. Where to put this check? async fn finish_block(&self) -> anyhow::Result<BlockNumber> { + let states = self.lock_read().await; + if states.len() > 1 { + return Err(anyhow::anyhow!("More than one pending block is not allowed")); + } Suggestion importance[1-10]: 8 Why: Adding a check to ensure only one pending block at a time in finish_block is crucial for maintaining data consistency and preventing logical errors in block handling.	8
	Replace the TODO comment with detailed method behavior documentation Replace the TODO comment with a more descriptive comment or documentation that explains the limitations and expected behavior of the methods in RocksTemporaryStorage. src/eth/storage/rocks/rocks_temporary.rs [43] -/// TODO: some methods are just delegating to inmemory and probably not working in a persistent way +/// Note: Some methods delegate to InMemoryTemporaryStorage and may not provide full persistence. Ensure appropriate usage or enhance persistence features. Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a vague TODO comment and proposes a more informative replacement, which can improve code documentation and clarity.	7
	Use a more descriptive error message for better error traceability Consider using a more specific error message in log_and_err! when no active block number is available during flushing, to aid in debugging and maintenance. src/eth/storage/rocks/rocks_temporary.rs [143] -return log_and_err!("no active block number when flushing rocksdb data"); +return log_and_err!("Failed to flush RocksDB data: No active block number set."); Suggestion importance[1-10]: 7 Why: The suggestion correctly targets an existing log message in the PR diff and proposes a more descriptive alternative, which can aid in debugging and maintenance.	7
	Improve error messages to include specific transaction details Replace the generic error message with a more specific one that includes details about the local transaction causing the failure. This will help in debugging and understanding the failure context better. src/eth/block_miner.rs [83] -return log_and_err!("failed to mine mixed block because one of the local execution is a success"); +return log_and_err!("failed to mine mixed block because the local transaction with ID {} is a success", tx.id()); Suggestion importance[1-10]: 6 Why: Providing more specific error messages can significantly aid in debugging, although the impact on the overall system functionality might be moderate.	6
Data integrity	Consider reintroducing or replacing the conflict checking logic in save_block The removal of conflict checking logic in the save_block method could lead to data integrity issues. If the conflict checking was intentionally removed, ensure that there is an alternative mechanism in place to handle potential data conflicts, or consider reintroducing this logic with necessary modifications. src/eth/storage/inmemory/inmemory_permanent.rs [283] +// Reintroduce conflict checking logic +let account_changes = block.compact_account_changes(); +if let Some(conflicts) = Self::check_conflicts(&state, &account_changes) { + return Err(StorageError::Conflict(conflicts)).context("storage conflict"); +} let block = Arc::new(block); Suggestion importance[1-10]: 8 Why: The removal of conflict checking logic could indeed lead to data integrity issues. This suggestion is highly relevant as it addresses a potential major issue in the system's functionality and data consistency.	8
Performance	Reduce unnecessary cloning to improve performance Instead of using clone() on tx_input multiple times, consider borrowing where possible to avoid unnecessary data cloning which can impact performance. src/eth/block_miner.rs [289-291] -let evm_input = EvmInput::from_eth_transaction(tx_input.clone()); -let evm_result = self.execute_in_evm(evm_input).await?; -relayer.forward(tx_input.clone(), evm_result.clone()).await?; +let evm_input = EvmInput::from_eth_transaction(&tx_input); +let evm_result = self.execute_in_evm(&evm_input).await?; +relayer.forward(&tx_input, &evm_result).await?; Suggestion importance[1-10]: 7 Why: Reducing unnecessary cloning can improve performance, especially in a blockchain context where efficiency is crucial. The suggestion correctly identifies a potential optimization in the handling of transaction data.	7
	Add a check for empty transaction lists to avoid unnecessary processing Consider adding a check to ensure that txs is not empty before proceeding to mine local transactions. This prevents unnecessary operations and potential errors when there are no transactions to process. src/eth/block_miner.rs [97-107] let (local_txs, external_txs) = partition_transactions(txs); +if local_txs.is_empty() { + return Ok(Block::new_at_now(number)); +} if not(external_txs.is_empty()) { return log_and_err!("failed to mine local block because one of the transactions is an external transaction"); } match NonEmpty::from_vec(local_txs) { Some(txs) => block_from_local(number, txs), None => Ok(Block::new_at_now(number)), } Suggestion importance[1-10]: 7 Why: Adding a check for empty transaction lists can prevent unnecessary processing and potential errors, improving the robustness and efficiency of the mining process.	7
	Improve the efficiency of operations on states by using a more suitable data structure The comment on line 37 indicates a performance issue with the complexity of operations being O(N). Consider implementing a more efficient data structure or algorithm that can achieve O(1) complexity for the operations on states. src/eth/storage/inmemory/inmemory_temporary.rs [37-38] -/// TODO: very inneficient, it is O(N), but it should be 0(1) -pub states: RwLock<NonEmpty<InMemoryTemporaryStorageState>>, +// Consider using a HashMap or another efficient data structure +pub states: RwLock<HashMap<BlockNumber, InMemoryTemporaryStorageState>>, Suggestion importance[1-10]: 7 Why: The suggestion to use a more efficient data structure like HashMap for states is valid and addresses the performance concern noted in the comment. However, the exact implementation details and impact on other parts of the code would need careful consideration.	7
	Optimize memory ordering in atomic operations for better performance It's recommended to use a more specific memory ordering than Ordering::SeqCst for performance reasons, especially if this code is performance-critical. Ordering::SeqCst ensures total ordering, which is often more conservative than necessary. Consider using Ordering::Relaxed for operations that do not require ordering guarantees, or Ordering::AcqRel for those that do. src/eth/storage/inmemory/inmemory_permanent.rs [153-158] -self.block_number.load(Ordering::SeqCst).into() -self.block_number.store(number.as_u64(), Ordering::SeqCst); +self.block_number.load(Ordering::Relaxed).into() +self.block_number.store(number.as_u64(), Ordering::AcqRel); Suggestion importance[1-10]: 6 Why: The suggestion to optimize memory ordering is valid for performance improvements. However, the choice between SeqCst, Relaxed, and AcqRel should be made carefully based on the specific requirements for memory ordering guarantees in the context, making this suggestion potentially risky without deeper analysis.	6
Best practice	Ensure explicit release of locks to prevent deadlocks Ensure that the lock obtained on self.temp is explicitly released after operations to prevent potential deadlocks or resource contention in concurrent environments. src/eth/storage/rocks/rocks_temporary.rs [146] -let temp = self.temp.lock_write().await; +{ + let temp = self.temp.lock_write().await; + // Perform operations +} // Lock is explicitly released here Suggestion importance[1-10]: 6 Why: The suggestion addresses a potential issue with lock handling in concurrent environments. However, the Rust async block automatically releases the lock at the end of the scope, making the explicit release mostly stylistic rather than functional in this context.	6
Maintainability	Add error logging for better diagnostics in failure scenarios Adding debug logs for error scenarios can greatly help in diagnosing issues during runtime. Consider adding error logging in methods where operations can fail, such as atomic operations or database writes. src/eth/storage/inmemory/inmemory_permanent.rs [157] tracing::debug!(%number, "setting mined block number"); +if let Err(e) = self.block_number.store(number.as_u64(), Ordering::SeqCst) { + tracing::error!("Failed to set mined block number: {:?}", e); +} Suggestion importance[1-10]: 5 Why: Adding error logging is a good practice for maintainability and diagnosing issues. However, the specific example provided (logging errors on atomic store operations) is not applicable as these operations do not inherently support error reporting in their standard usage in Rust.	5