review: review security audit findings (GREVM-R2-001 ~ 008)

Author: AshinGau

docs/security/2026-03-02-security-audit-round2-report.md

@@ -171,7 +171,7 @@ fn clear_destructed_entry(&self, account: Address) {
 
 **Recommendation:** Maintain a secondary index `DashMap<Address, HashSet<LocationAndType>>` to enable O(1) lookup of entries by address, avoiding the full table scan. Alternatively, prefix-scan the DashMap only for the target address keys.
 
-**Review Comments** reviewer: Xin GAO; state: ignored; comments: 
+**Review Comments** reviewer: Xin GAO; state: ignored; comments: Resolved by `fallback_sequential`
 
 ---
 
@@ -208,6 +208,8 @@ pub(crate) fn print(&self) {
 
 Or use `tracing`'s lazy evaluation with `tracing::debug!(...)` and deferred formatting.
 
+**Review Comments** reviewer: Xin GAO; state: ignored; comments: Only called in debug mode
+
 ---
 
 ## LOW Severity (3)
@@ -235,6 +237,8 @@ unsafe fn set(&self, index: usize, value: T) {
 }
 ```
 
+**Review Comments** reviewer: Xin GAO; state: ignored; comments:
+
 ---
 
 ### GREVM-R2-007: `IdentityHasher::write()` Panics with `unreachable!()`
@@ -267,6 +271,8 @@ fn write(&mut self, bytes: &[u8]) {
 }
 ```
 
+**Review Comments** reviewer: Xin GAO; state: ignored; comments:
+
 ---
 
 ### GREVM-R2-008: `parallel_apply_transitions_and_create_reverts` Clones Transitions via `get().cloned()`
@@ -295,6 +301,8 @@ fork_join_util(transitions_vec.len(), None, |start, end, _| {
 });
 ```
 
+**Review Comments** reviewer: Xin GAO; state: ignored; comments:
+
 ---
 
 ## INFO (2)

src/hint.rs

@@ -87,8 +87,6 @@ impl RWSet {
 /// Struct to hold shared transaction states and provide methods for parsing
 /// and handling execution hints for parallel transaction execution.
 pub(crate) struct ParallelExecutionHints {
-    /// SAFETY: Wrapped in `UnsafeCell` to allow disjoint parallel writes.
-    /// Each thread in `fork_join_util` writes only to its own index range.
     rw_set: UnsafeCell<Vec<RWSet>>,
     txs: Arc<Vec<TxEnv>>,
 }
@@ -106,8 +104,6 @@ impl ParallelExecutionHints {
         let mut last_write_tx: HashMap<LocationAndType, TxId> = HashMap::new();
         let mut dependent_tx: Vec<Option<TxId>> = vec![None; num_txs];
         let mut affect_txs = vec![HashSet::new(); num_txs];
-        // SAFETY: generate_dependency is called after parse_hints completes (fork_join is done),
-        // so no concurrent access exists.
         let rw_set = unsafe { &*self.rw_set.get() };
         for (txid, rw_set) in rw_set.iter().enumerate() {
             for location in rw_set.read_set.iter() {
@@ -128,8 +124,6 @@ impl ParallelExecutionHints {
         let txs = self.txs.clone();
         // Utilize fork-join utility to process transactions in parallel
         fork_join_util(txs.len(), None, |start_tx, end_tx, _| {
-            // SAFETY: Each thread writes to disjoint index ranges [start_tx..end_tx],
-            // guaranteed by fork_join_util's partitioning.
             let hints = unsafe { &mut *self.rw_set.get() };
             for index in start_tx..end_tx {
                 let tx_env = &txs[index];

src/scheduler.rs

@@ -41,6 +41,9 @@ use std::{
     time::Instant,
 };
 
+/// min number of txs to parallel execute.
+const MIN_PARALLEL_TXS: usize = 64;
+
 pub(crate) type MVMemory = DashMap<LocationAndType, BTreeMap<TxId, MemoryEntry>>;
 
 #[derive(Metrics)]
@@ -266,9 +269,6 @@ where
     env: BlockEnv,
     block_size: usize,
     txs: Arc<Vec<TxEnv>>,
-    /// SAFETY: `UnsafeCell` allows the commit thread to mutate state while worker threads read.
-    /// The commit thread has exclusive write access (serialized by finality ordering).
-    /// Worker threads only read via `DatabaseRef` (DashMap-based, thread-safe).
     state: UnsafeCell<ParallelState<DB>>,
     results: Mutex<Vec<ExecutionResult>>,
     tx_states: Vec<Mutex<TxState>>,
@@ -445,16 +445,15 @@ where
             std::env::var("GREVM_CONCURRENT_LEVEL")
                 .map_or(*CONCURRENT_LEVEL, |s| s.parse().unwrap_or(*CONCURRENT_LEVEL)),
         );
-        if *FALLBACK_SEQUENTIAL {
+        if *FALLBACK_SEQUENTIAL || self.block_size < MIN_PARALLEL_TXS {
             return self.fallback_sequential();
         }
         let commiter = Mutex::new(StateAsyncCommit::new(
             self.env.beneficiary,
             CommitGuard::new(&self.state),
             self.cfg.disable_nonce_check,
         ));
-        // SAFETY: Worker threads access `ParallelState` only through `DatabaseRef` (read-only,
-        // DashMap-based, inherently thread-safe). The commit thread is the sole writer.
+
         let state_ref = unsafe { &*self.state.get() };
         commiter.lock().init().map_err(|e| GrevmError { txid: 0, error: EVMError::Database(e) })?;
         thread::scope(|scope| {
@@ -571,9 +570,6 @@ where
         }
 
         let mut sequential_results = Vec::with_capacity(self.block_size - num_commit);
-        // SAFETY: fallback_sequential is called either before parallel execution starts
-        // (when FALLBACK_SEQUENTIAL is true) or after post_execute (after thread::scope
-        // has joined all threads), so no concurrent access exists.
         let mut commit_guard = CommitGuard::new(&self.state);
         let state_mut = commit_guard.state_mut();
         {

src/storage.rs

@@ -86,7 +86,6 @@ impl ParallelBundleState for BundleState {
                 let revert = transition.create_revert();
                 if let Some(revert) = revert {
                     state_size.fetch_add(present_bundle.size_hint(), Ordering::Relaxed);
-                    // SAFETY: fork_join_util guarantees each thread writes to disjoint indices.
                     unsafe { bundle_state.set(pos, Some((address, present_bundle))) };
                     if include_reverts {
                         unsafe { reverts.set(pos, Some((address, revert))) };