feat: inmemory temporary storage conflict detection and related improvements

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant changes across multiple files with complex logic related to blockchain transaction execution, conflict detection, and state management. The changes impact core functionalities and require a deep understanding of the existing system and the modifications to ensure correctness and performance.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The method check_conflicts in InMemoryTemporaryStorageState might return None even when there are conflicts due to the use of take_original_ref() which does not actually check the current state against the proposed changes.
	Performance Concern: The repeated locking and unlocking of state in save_execution could lead to performance bottlenecks, especially under high load. Consider optimizing the state management to reduce the frequency of locks or use more granular locking strategies.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/storage/inmemory/inmemory_temporary.rs
suggestion	Consider implementing more efficient conflict detection logic in check_conflicts. Instead of using take_original_ref(), directly compare the stored account state with the incoming execution changes to accurately detect conflicts. This change is important to ensure the integrity of transaction execution and state management. [important]
relevant line	if let Some(expected) = change.nonce.take_original_ref() {

---

relevant file	src/eth/executor.rs
suggestion	Optimize the handling of transaction execution results in reexecute_external by reducing the number of times the state is locked and unlocked. Consider batching state updates or using a more granular locking mechanism to improve performance. This optimization is important for enhancing the scalability and responsiveness of the system. [important]
relevant line	storage.save_execution_to_temp(tx_execution).await?;

---

relevant file	src/eth/block_miner.rs
suggestion	Refactor the mine_with_many_transactions method to improve readability and maintainability. Consider breaking down the method into smaller, more focused sub-methods, each handling a specific part of the block mining process. This refactoring is important for future maintenance and potential enhancements. [medium]
relevant line	pub async fn mine_with_many_transactions(&self, transactions: NonEmpty<(TransactionInput, EvmExecutionResult)>) -> anyhow::Result {

---

relevant file	src/eth/storage/inmemory/inmemory_temporary.rs
suggestion	Improve the error handling in save_execution to provide more detailed error messages and context when conflicts are detected or other errors occur. Use the context method from the anyhow crate to add meaningful error descriptions. This improvement is important for debugging and operational transparency. [medium]
relevant line	return Err(StorageError::Conflict(conflicts)).context("execution conflicts with current state");

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a check to ensure the transactions iterator is not empty to prevent potential runtime panics Consider handling the case where transactions.into_iter().enumerate() might produce an empty iterator, which would cause block_timestamp to panic due to calling minimum_by on an empty iterator. To prevent this, add a check to ensure transactions is not empty before proceeding with the block initialization. src/eth/block_miner.rs [108-112] +if transactions.is_empty() { + return Err(anyhow::Error::msg("No transactions provided")); +} let block_timestamp = transactions .minimum_by(|(_, e1), (_, e2)| e1.execution.block_timestamp.cmp(&e2.execution.block_timestamp)) .1 .execution .block_timestamp; Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a potential runtime panic due to calling minimum_by on an empty iterator, which is a critical issue that could crash the program.	8
	Handle the case of empty transactions to prevent errors during block initialization The method mine_with_many_transactions should handle the case where transactions is empty to prevent errors during block initialization. Consider returning an early error if transactions is empty. src/eth/block_miner.rs [105] pub async fn mine_with_many_transactions(&self, transactions: NonEmpty<(TransactionInput, EvmExecutionResult)>) -> anyhow::Result<Block> { + if transactions.is_empty() { + return Err(anyhow::Error::msg("No transactions provided")); + } Suggestion importance[1-10]: 8 Why: This suggestion is crucial as it addresses the potential error of initializing a block with empty transactions, which could lead to unexpected behavior or errors.	8
Performance	Check for conflicts before acquiring a write lock to optimize performance Optimize the save_execution method by checking for conflicts before acquiring a write lock. This change can potentially reduce the lock contention and improve performance by avoiding unnecessary lock acquisitions when conflicts are detected early. src/eth/storage/inmemory/inmemory_temporary.rs [138-140] -let mut state = self.lock_write().await; +let state = self.lock_read().await; if let Some(conflicts) = state.check_conflicts(&tx.result.execution) { return Err(StorageError::Conflict(conflicts)).context("execution conflicts with current state"); } +let mut state = self.lock_write().await; Suggestion importance[1-10]: 8 Why: Checking for conflicts before acquiring a write lock is a significant optimization that can reduce lock contention and improve performance, especially in high-concurrency environments. This suggestion addresses a crucial aspect of performance optimization.	8
	Use references when iterating over logs to avoid unnecessary cloning To improve the efficiency of log processing, consider using iter() instead of clone() for iterating over logs in EvmExecutionResult. This avoids unnecessary data cloning, reducing memory usage and potentially improving performance. src/eth/block_miner.rs [123] -for mined_log in evm_result.execution.logs.clone() { +for mined_log in &evm_result.execution.logs { Suggestion importance[1-10]: 6 Why: The suggestion to use references instead of cloning the logs is valid and improves performance by reducing unnecessary memory usage.	6
Error handling	Implement error handling for lock acquisition methods Add error handling for the lock_read and lock_write methods in InMemoryTemporaryStorage. Currently, the methods assume that acquiring the lock will always succeed. Consider using a timeout or handling the potential error to avoid deadlocks or panics in case the lock cannot be acquired. src/eth/storage/inmemory/inmemory_temporary.rs [36-42] -pub async fn lock_read(&self) -> RwLockReadGuard<'_, InMemoryTemporaryStorageState> { - self.state.read().await +pub async fn lock_read(&self) -> anyhow::Result<RwLockReadGuard<'_, InMemoryTemporaryStorageState>> { + self.state.read().await.map_err(|e| anyhow!("Failed to acquire read lock: {}", e)) } -pub async fn lock_write(&self) -> RwLockWriteGuard<'_, InMemoryTemporaryStorageState> { - self.state.write().await +pub async fn lock_write(&self) -> anyhow::Result<RwLockWriteGuard<'_, InMemoryTemporaryStorageState>> { + self.state.write().await.map_err(|e| anyhow!("Failed to acquire write lock: {}", e)) } Suggestion importance[1-10]: 7 Why: Adding error handling for lock acquisition is a good practice to prevent potential deadlocks and improve the robustness of the code. However, the existing code does not show any issues with lock handling, so the improvement is preventive rather than corrective.	7
	Improve error handling in conflict checks by logging missing accounts Refactor the check_conflicts method to handle potential None values more gracefully. Instead of using continue which skips the current iteration, consider logging these cases or handling them explicitly to ensure that all potential conflicts are accounted for. src/eth/storage/inmemory/inmemory_temporary.rs [84-85] for (address, change) in &execution.changes { - let Some(account) = self.accounts.get(address) else { continue }; - ... + if let Some(account) = self.accounts.get(address) { + ... + } else { + tracing::warn!("Account not found for address: {:?}", address); + } } Suggestion importance[1-10]: 6 Why: The suggestion to improve error handling by logging missing accounts during conflict checks is valid and enhances the traceability of errors. However, the impact on overall functionality might be limited, as it primarily affects logging.	6
Enhancement	Add error handling for setting the active block number to improve error reporting and handling To ensure that the reexecute_external function handles errors gracefully when setting the active block number, consider adding error handling for the set_active_block_number call. src/eth/executor.rs [93] -storage.set_active_block_number(block.number()).await?; +storage.set_active_block_number(block.number()).await.map_err(|e| { + tracing::error!("Failed to set active block number: {:?}", e); + e +})?; Suggestion importance[1-10]: 5 Why: Adding detailed error handling for setting the active block number enhances the robustness of the code by providing better error reporting, although it's more of an enhancement than a critical fix.	5