feat: all storage access through StratusStorage

[dinhani-cw]

• All storage access goes through StratusStorage service.
• Simplified metrics blocks focusing on code clarity.
• Removed unused or deprecated metrics.
• Created metrics for new methods.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR involves significant changes across multiple files and modules, impacting both functionality and metrics. The changes are spread across storage, execution, and metrics, requiring a thorough understanding of the system's architecture and the implications of these changes on performance and functionality.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of #[cfg(feature = "metrics")] in some parts without ensuring that the metrics are conditionally compiled could lead to runtime errors if the metrics feature is not enabled.
	Performance Concern: The extensive use of metrics and tracing across various functions, especially in critical paths like block processing and transaction execution, might introduce performance overhead. It's crucial to ensure that these metrics are essential and optimized.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/storage/stratus_storage.rs
suggestion	Consider adding error handling for the new asynchronous operations introduced in StratusStorage. For instance, the method set_active_block_number_as_next could fail due to database issues, but there's no handling or retry logic. This could lead to unhandled errors during block processing. [important]
relevant line	self.set_active_block_number(last_mined_block.next()).await?;

---

relevant file	src/eth/storage/stratus_storage.rs
suggestion	It might be beneficial to implement logging for key actions within the StratusStorage methods, especially those that change the state, such as save_block and reset. This would aid in debugging and provide a clear trace of operations in production environments. [medium]
relevant line	self.perm.save_block(block).await?;

---

relevant file	src/eth/block_miner.rs
suggestion	Ensure that the method mine_external properly handles the case where block.split_transactions() might not correctly separate local and external transactions due to potential changes in transaction structure or processing logic. Adding a validation step to confirm the transaction types could prevent future bugs. [important]
relevant line	let (local_txs, external_txs) = block.split_transactions();

---

relevant file	src/eth/executor.rs
suggestion	Optimize the transaction routing logic in Executor to reduce computational overhead. Consider caching results of route_transactions if the transactions within a block do not frequently change their route categorization, to minimize repeated computations. [medium]
relevant line	let tx_routes = route_transactions(&block.transactions, receipts)?;

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Improve error handling and logging for transaction execution saving Ensure that the save_execution method in TransactionRelayer properly logs or handles the case where the transaction execution fails to save, to aid in debugging and operational monitoring. src/eth/transaction_relayer.rs [37] -self.storage.save_execution(tx_execution).await?; +match self.storage.save_execution(tx_execution).await { + Ok(_) => (), + Err(e) => tracing::error!("Failed to save execution: {}", e), +} Suggestion importance[1-10]: 8 Why: This suggestion is highly relevant as it improves error handling and operational transparency in the save_execution method, which is critical for debugging and monitoring the system's behavior.	8
	Add error handling for storage operations to enhance robustness Consider handling potential errors from stratus_storage.save_accounts(initial_accounts.clone()).await? to ensure robustness in case of storage failures. src/bin/importer_offline.rs [76] +stratus_storage.save_accounts(initial_accounts.clone()).await?; - Suggestion importance[1-10]: 7 Why: The suggestion to add error handling for stratus_storage.save_accounts(initial_accounts.clone()).await? is valid and improves robustness. Error handling is crucial for storage operations to ensure the application handles failures gracefully.	7
	Add error handling for block saving to ensure data integrity Consider verifying the success of the self.storage.save_block(block.clone()).await? operation and handle any potential errors to ensure the block is saved correctly. src/eth/block_miner.rs [142] +self.storage.save_block(block.clone()).await?; - Suggestion importance[1-10]: 7 Why: The suggestion to verify the success of self.storage.save_block(block.clone()).await? and handle potential errors is valid. Proper error handling ensures that the block saving process is reliable and data integrity is maintained.	7
	Implement error handling for transaction execution saving It's crucial to handle the error scenario for storage.save_execution(TransactionExecution::External(tx)).await? to manage failures in transaction execution saving gracefully. src/eth/executor.rs [144] +storage.save_execution(TransactionExecution::External(tx)).await?; - Suggestion importance[1-10]: 7 Why: The suggestion to handle errors for storage.save_execution(TransactionExecution::External(tx)).await? is appropriate. Managing failures in transaction execution saving is crucial for maintaining the consistency and reliability of the system.	7
	Enhance the method to handle multi-threaded scenarios safely To avoid potential race conditions or inconsistencies, ensure that the finish_block method properly handles the transition between blocks, especially in multi-threaded contexts. src/eth/storage/temporary_storage.rs [37] -async fn finish_block(&self) -> anyhow::Result<PendingBlock>; +async fn finish_block(&self) -> anyhow::Result<PendingBlock> { + // Ensure thread safety or state consistency here + ... +} Suggestion importance[1-10]: 7 Why: The suggestion is valid as it addresses potential concurrency issues in the finish_block method, which is crucial for maintaining state consistency in a multi-threaded environment.	7
Possible bug	Add error handling for retrieving the mined block number before setting the active block number Consider handling the case where self.perm.read_mined_block_number().await? returns an error before attempting to set the active block number. This can prevent the propagation of incorrect state changes if the mining block number cannot be retrieved. src/eth/storage/stratus_storage.rs [114-116] let last_mined_block = self.read_mined_block_number().await?; -self.set_active_block_number(last_mined_block.next()).await?; +if let Ok(block_number) = last_mined_block { + self.set_active_block_number(block_number.next()).await?; +} else { + return last_mined_block.map_err(|e| e); +} Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a potential issue where an error in retrieving the mined block number could lead to incorrect state changes. Adding error handling would prevent this, making it a significant improvement.	8
	Ensure metrics::now() does not cause runtime errors by validating its output To avoid potential runtime panics, ensure that the metrics::now() function always returns a valid start time or handle the case where it might fail or return an invalid value. src/eth/storage/stratus_storage.rs [76-78] -let start = metrics::now(); -let result = self.temp.read_active_block_number().await; -metrics::inc_storage_read_active_block_number(start.elapsed(), result.is_ok()); +if let Ok(start) = metrics::now() { + let result = self.temp.read_active_block_number().await; + metrics::inc_storage_read_active_block_number(start.elapsed(), result.is_ok()); +} else { + // Handle error or invalid start time +} Suggestion importance[1-10]: 6 Why: While the suggestion to validate the output of metrics::now() is valid to prevent potential runtime errors, the impact is moderate as it depends on the reliability of the metrics::now() function, which is typically expected to be stable.	6
Maintainability	Refactor metric collection into a helper function to reduce code duplication and improve maintainability Refactor the repeated pattern of metric collection and method invocation under the #[cfg(feature = "metrics")] condition to a helper function to improve code maintainability and reduce duplication. src/eth/storage/stratus_storage.rs [76-79] #[cfg(feature = "metrics")] -{ - let start = metrics::now(); - let result = self.temp.read_active_block_number().await; - metrics::inc_storage_read_active_block_number(start.elapsed(), result.is_ok()); - result -} +self.metric_wrapped_call(self.temp.read_active_block_number).await Suggestion importance[1-10]: 7 Why: This suggestion is beneficial for maintainability by reducing code duplication. Refactoring the repeated metric collection pattern into a helper function would streamline the code and improve readability.	7
Enhancement	Add comprehensive error handling for account and slot retrieval methods Consider adding error handling for the read_account and read_slot methods to manage scenarios where the account or slot cannot be found or accessed, beyond just returning None. src/eth/storage/temporary_storage.rs [47-51] -async fn read_account(&self, address: &Address) -> anyhow::Result<Option<Account>>; -async fn read_slot(&self, address: &Address, index: &SlotIndex) -> anyhow::Result<Option<Slot>>; +async fn read_account(&self, address: &Address) -> anyhow::Result<Option<Account>> { + // Additional error handling logic here + ... +} +async fn read_slot(&self, address: &Address, index: &SlotIndex) -> anyhow::Result<Option<Slot>> { + // Additional error handling logic here + ... +} Suggestion importance[1-10]: 5 Why: The suggestion is reasonable for enhancing robustness, but the methods already return a Result type which can encapsulate errors, making the suggestion only moderately beneficial.	5
Performance	Optimize account saving by utilizing batch writes to improve performance Optimize the save_accounts function by batching the database write operations if not already implemented in the perm.save_accounts method, to improve performance especially when dealing with large vectors of accounts. src/eth/storage/stratus_storage.rs [164-166] +// Assuming `perm.save_accounts` can handle batch writes efficiently let result = self.perm.save_accounts(accounts).await; metrics::inc_storage_save_accounts(start.elapsed(), result.is_ok()); Suggestion importance[1-10]: 3 Why: The suggestion assumes that the perm.save_accounts method might not be optimized for batch writes without evidence from the PR code. This makes the suggestion speculative and not directly applicable unless confirmed.	3