enha + fix: possible storage bug and remove states vec from inmemory temporary storage

[carneiro-cw]

With the introduction of the storage cache #1887 we made it so the inmemory temporary storage only held one block in its block Vec. However this combined with #1888 introduces a possibly critical bug. When the pending block is evicted from the temporary storage it takes a few ms to be committed to the permanent storage, however executions can still happen in the interim and if an execution tries to read a slot/account that has not yet been committed to the permanent but has already been evicted from the temporary it would get the wrong value.

The cache implementation masks this bug pretty well, since all the recent changes will probably be in the cache, however it is not guaranteed that it won't evict some slot/account before we have committed the block to the permanent storage, and therefore we should not rely on it for this purpose.

This PR makes it so the temporary storage holds 2 blocks, the pending block, and the latest to avoid this issue.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Race Condition The new implementation uses separate RwLocks for pending_block and latest_block. This could potentially lead to race conditions if not handled carefully. The reviewer should ensure that all operations involving both locks are performed in a consistent order to prevent deadlocks. Error Handling The new implementation uses unwrap() in several places when acquiring locks. This could lead to panics if a lock is poisoned. Consider using expect() with meaningful error messages or proper error handling. Performance Concern The read_account and read_slot methods now check both pending_block and latest_block. This could potentially be slower than the previous implementation. The reviewer should consider if this new approach provides the necessary consistency without sacrificing too much performance.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Implement proper error handling to prevent potential panics and improve code robustness Implement proper error handling in the check_conflicts method instead of using unwrap(). This will prevent potential panics and improve the robustness of the code. src/eth/storage/temporary/inmemory.rs [44-77] fn check_conflicts(&self, execution: &EvmExecution) -> anyhow::Result<Option<ExecutionConflicts>> { let mut conflicts = ExecutionConflictsBuilder::default(); for (&address, change) in &execution.changes { // check account info conflicts if let Some(account) = self.read_account(address)? { if let Some(expected) = change.nonce.take_original_ref() { let original = &account.nonce; if expected != original { conflicts.add_nonce(address, *original, *expected); } } if let Some(expected) = change.balance.take_original_ref() { let original = &account.balance; if expected != original { conflicts.add_balance(address, *original, *expected); } } } // check slots conflicts for (&slot_index, slot_change) in &change.slots { if let Some(expected) = slot_change.take_original_ref() { - let Some(original) = self.read_slot(address, slot_index)? else { - continue; - }; - if expected.value != original.value { - conflicts.add_slot(address, slot_index, original.value, expected.value); + if let Some(original) = self.read_slot(address, slot_index)? { + if expected.value != original.value { + conflicts.add_slot(address, slot_index, original.value, expected.value); + } } } } } Ok(conflicts.build()) } Suggestion importance[1-10]: 7 Why: The suggestion improves error handling by removing the use of unwrap() and properly handling potential errors. This enhances the code's robustness and prevents potential panics, which is crucial for a storage implementation.	7
General	Use a more efficient data structure for storing and accessing blocks to improve performance and memory usage Use a more efficient data structure for storing and accessing blocks, such as a circular buffer or a fixed-size array, instead of relying on RwLock<Option> for the latest block. src/eth/storage/temporary/inmemory.rs [28-31] +use std::collections::VecDeque; + pub struct InMemoryTemporaryStorage { pub pending_block: RwLock<InMemoryTemporaryStorageState>, - pub latest_block: RwLock<Option<InMemoryTemporaryStorageState>>, + pub previous_blocks: RwLock<VecDeque<InMemoryTemporaryStorageState>>, } +impl InMemoryTemporaryStorage { + pub fn new(block_number: BlockNumber, max_blocks: usize) -> Self { + Self { + pending_block: RwLock::new(InMemoryTemporaryStorageState { + block: PendingBlock::new_at_now(block_number), + accounts: HashMap::default(), + }), + previous_blocks: RwLock::new(VecDeque::with_capacity(max_blocks)), + } + } + // ... (implement methods to manage the VecDeque) +} + Suggestion importance[1-10]: 6 Why: The suggestion proposes using a VecDeque for storing previous blocks, which could potentially improve performance and memory usage. However, the impact is moderate as the current implementation is functional, and the optimization's benefits depend on the specific use case.	6

[gabriel-aranha-cw]

Final benchmark:
Run ID: bench-1557210719

Git Info:

• Branch: main
• Commit: d1c86fe
• PR: enha + fix: possible storage bug and remove states vec from inmemory temporary storage #1904

Configuration:

• Target Account Strategy: Default

RPS Stats: Max: 1244.00, Min: 767.00, Avg: 1099.94, StdDev: 70.23
TPS Stats: Max: 1236.00, Min: 866.00, Avg: 1066.76, StdDev: 75.87

Plot: View Plot