Fix: commit miner's nonce to ensure the nonce check by EVM

.github/workflows/grevm-ethereum.yml

@@ -16,16 +16,16 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v3
 
+      - name: Initialize ethereum tests
+        run: |
+          git submodule update --init tests/ethereum/tests
+
       # install rust tools
       - name: Set up Rust
         uses: actions-rs/toolchain@v1
         with:
           toolchain: stable
           override: true
 
-      - name: Clone ethereum/tests repository
-        run: |
-          git clone https://x-access-token:${{ secrets.GITHUB_TOKEN }}@github.com/ethereum/tests.git tests/ethereum/tests
-
       - name: Run tests
         run: cargo test --test ethereum

.github/workflows/grevm-test.yml

@@ -16,6 +16,10 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v3
 
+      - name: Initialize mainnet test_data
+        run: |
+          git submodule update --init test_data
+
       # install rust tools
       - name: Set up Rust
         uses: actions-rs/toolchain@v1
@@ -24,4 +28,4 @@ jobs:
           override: true
 
       - name: Run tests
-        run: cargo test -- --skip mainnet
+        run: cargo test -- --skip ethereum

.gitignore

@@ -15,44 +15,23 @@ target/
 # Generated by MacOS
 .DS_Store
 
-# Generated test-vectors for DB
-testdata/micro/db
-
-# Generated data for stage benchmarks
-crates/stages/testdata
-
 # Prometheus data dir
 data/
 
-# Proptest data
-proptest-regressions/
-
 # Release artifacts
 dist/
 
-# Database debugging tools
-db-tools/
-
 # VSCode
 .vscode
 
 # Coverage report
 lcov.info
 
-# Generated by ./etc/generate-jwt.sh
-jwttoken/
-
 # Cache directory for CCLS, if using it with MDBX sources
 .ccls-cache/
 
-# Generated by CMake due to MDBX sources
-crates/storage/libmdbx-rs/mdbx-sys/libmdbx/cmake-build-debug
-
 # Rust bug report
 rustc-ice-*
 
 # Rust lock file
 Cargo.lock
-
-# grevm etherum tests
-crates/grevm/tests/ethereum/tests/

.gitmodules

@@ -1,3 +1,6 @@
 [submodule "test_data"]
 	path = test_data
 	url = https://github.com/Galxe/grevm-test-data
+[submodule "tests/ethereum/tests"]
+	path = tests/ethereum/tests
+	url = https://github.com/ethereum/tests.git

Cargo.toml

@@ -7,6 +7,7 @@ description = "Create Parallel EVM"
 [dependencies]
 revm = "14.0.0"
 fastrace = "0.7"
+tracing = "0.1.40"
 
 # Alloy
 alloy-chains = "0.1.18"

src/partition.rs

@@ -34,8 +34,9 @@ where
 {
     spec_id: SpecId,
     env: Env,
-    coinbase: Address,
 
+    #[allow(dead_code)]
+    coinbase: Address,
     #[allow(dead_code)]
     partition_id: PartitionId,
 
@@ -124,9 +125,10 @@ where
                 let result = evm.transact();
                 match result {
                     Ok(result_and_state) => {
-                        let read_set = evm.db_mut().take_read_set();
-                        let (write_set, miner_update) =
-                            evm.db().generate_write_set(&result_and_state.state);
+                        let ResultAndState { result, mut state } = result_and_state;
+                        let mut read_set = evm.db_mut().take_read_set();
+                        let (write_set, miner_update, remove_miner) =
+                            evm.db().generate_write_set(&mut state);
 
                         // Check if the transaction can be skipped
                         // skip_validation=true does not necessarily mean the transaction can skip validation.
@@ -138,10 +140,13 @@ where
                         skip_validation &=
                             write_set.iter().all(|l| tx_states[txid].write_set.contains(l));
 
-                        let ResultAndState { result, mut state } = result_and_state;
-                        if miner_update.is_some() {
+                        if remove_miner {
                             // remove miner's state if we handle rewards separately
                             state.remove(&self.coinbase);
+                        } else {
+                            // add miner to read set, because it's in write set.
+                            // set miner's value to None to make this tx redo in next round if unconfirmed.
+                            read_set.insert(LocationAndType::Basic(self.coinbase), None);
                         }
                         // temporary commit to cache_db, to make use the remaining txs can read the updated data
                         let transition = evm.db_mut().temporary_commit(state);
@@ -156,7 +161,7 @@ where
                             execute_result: ResultAndTransition {
                                 result: Some(result),
                                 transition,
-                                miner_update: miner_update.unwrap_or_default(),
+                                miner_update,
                             },
                         };
                     }

src/scheduler.rs

@@ -19,6 +19,7 @@ use revm::primitives::{
     AccountInfo, Address, Bytecode, EVMError, Env, ExecutionResult, SpecId, TxEnv, B256, U256,
 };
 use revm::{CacheState, DatabaseRef, EvmBuilder};
+use tracing::info;
 
 struct ExecuteMetrics {
     /// Number of times parallel execution is called.
@@ -233,6 +234,7 @@ where
         let coinbase = env.block.coinbase;
         let num_partitions = *CPU_CORES * 2 + 1; // 2 * cpu + 1 for initial partition number
         let num_txs = txs.len();
+        info!("Parallel execute {} txs of SpecId {:?}", num_txs, spec_id);
         Self {
             spec_id,
             env,
@@ -463,6 +465,10 @@ where
         self.metrics.conflict_tx_cnt.increment(conflict_tx_cnt as u64);
         self.metrics.unconfirmed_tx_cnt.increment(unconfirmed_tx_cnt as u64);
         self.metrics.finality_tx_cnt.increment(finality_tx_cnt as u64);
+        info!(
+            "Find continuous finality txs: conflict({}), unconfirmed({}), finality({})",
+            conflict_tx_cnt, unconfirmed_tx_cnt, finality_tx_cnt
+        );
         return Ok(finality_tx_cnt);
     }
 
@@ -641,6 +647,7 @@ where
         }
 
         if self.num_finality_txs < self.txs.len() {
+            info!("Sequential execute {} remaining txs", self.txs.len() - self.num_finality_txs);
             self.execute_remaining_sequential()?;
         }
 

src/storage.rs

@@ -12,13 +12,13 @@ use std::sync::Arc;
 /// The miner's reward is calculated by subtracting the previous balance from the current balance.
 #[derive(Debug, Clone)]
 pub(crate) enum LazyUpdateValue {
-    Increase(u128, u64),
-    Decrease(u128, u64),
+    Increase(u128),
+    Decrease(u128),
 }
 
 impl Default for LazyUpdateValue {
     fn default() -> Self {
-        Self::Increase(0, 0)
+        Self::Increase(0)
     }
 }
 
@@ -27,11 +27,9 @@ impl LazyUpdateValue {
     pub(crate) fn merge(values: Vec<Self>) -> Self {
         let mut value: u128 = 0;
         let mut positive: bool = true;
-        let mut nonce: u64 = 0;
         for lazy_value in values {
             match lazy_value {
-                Self::Increase(inc, add_nonce) => {
-                    nonce += add_nonce;
+                Self::Increase(inc) => {
                     if positive {
                         value += inc;
                     } else {
@@ -43,8 +41,7 @@ impl LazyUpdateValue {
                         }
                     }
                 }
-                Self::Decrease(dec, add_nonce) => {
-                    nonce += add_nonce;
+                Self::Decrease(dec) => {
                     if positive {
                         if value > dec {
                             value -= dec;
@@ -59,9 +56,9 @@ impl LazyUpdateValue {
             }
         }
         if positive {
-            Self::Increase(value, nonce)
+            Self::Increase(value)
         } else {
-            Self::Decrease(value, nonce)
+            Self::Decrease(value)
         }
     }
 }
@@ -173,17 +170,12 @@ where
         for (address, update) in balances {
             let cache_account = self.load_cache_account(address)?;
             let mut info = cache_account.account_info().unwrap_or_default();
-            let (new_balance, add_nonce) = match update {
-                LazyUpdateValue::Increase(value, nonce) => {
-                    (info.balance.saturating_add(U256::from(value)), nonce)
-                }
-                LazyUpdateValue::Decrease(value, nonce) => {
-                    (info.balance.saturating_sub(U256::from(value)), nonce)
-                }
+            let new_balance = match update {
+                LazyUpdateValue::Increase(value) => info.balance.saturating_add(U256::from(value)),
+                LazyUpdateValue::Decrease(value) => info.balance.saturating_sub(U256::from(value)),
             };
-            if info.balance != new_balance || add_nonce != 0 {
+            if info.balance != new_balance {
                 info.balance = new_balance;
-                info.nonce += add_nonce;
                 transitions.push((address, cache_account.change(info, Default::default())));
             }
         }
@@ -369,11 +361,12 @@ impl<DB> PartitionDB<DB> {
     /// Returns the write set(exclude miner) and the miner's rewards.
     pub(crate) fn generate_write_set(
         &self,
-        changes: &EvmState,
-    ) -> (LocationSet, Option<LazyUpdateValue>) {
-        let mut miner_update: Option<LazyUpdateValue> = None;
+        changes: &mut EvmState,
+    ) -> (LocationSet, LazyUpdateValue, bool) {
+        let mut miner_update = LazyUpdateValue::default();
+        let mut remove_miner = true;
         let mut write_set = HashSet::new();
-        for (address, account) in changes {
+        for (address, account) in &mut *changes {
             if account.is_selfdestructed() {
                 write_set.insert(LocationAndType::Code(*address));
                 // When a contract account is destroyed, its remaining balance is sent to a
@@ -388,33 +381,32 @@ impl<DB> PartitionDB<DB> {
             // Lazy update miner's balance
             let mut miner_updated = false;
             if self.coinbase == *address {
-                match self.cache.accounts.get(address) {
+                let add_nonce = match self.cache.accounts.get(address) {
                     Some(miner) => match miner.account.as_ref() {
                         Some(miner) => {
                             if account.info.balance >= miner.info.balance {
-                                miner_update = Some(LazyUpdateValue::Increase(
+                                miner_update = LazyUpdateValue::Increase(
                                     (account.info.balance - miner.info.balance).to(),
-                                    account.info.nonce - miner.info.nonce,
-                                ));
+                                );
                             } else {
-                                miner_update = Some(LazyUpdateValue::Decrease(
+                                miner_update = LazyUpdateValue::Decrease(
                                     (miner.info.balance - account.info.balance).to(),
-                                    account.info.nonce - miner.info.nonce,
-                                ));
+                                );
                             }
-                            miner_updated = true;
+                            account.info.balance = miner.info.balance;
+                            account.info.nonce - miner.info.nonce
                         }
                         // LoadedNotExisting
                         None => {
-                            miner_update = Some(LazyUpdateValue::Increase(
-                                account.info.balance.to(),
-                                account.info.nonce,
-                            ));
-                            miner_updated = true;
+                            miner_update = LazyUpdateValue::Increase(account.info.balance.to());
+                            account.info.balance = U256::ZERO;
+                            account.info.nonce
                         }
                     },
                     None => panic!("Miner should be cached"),
-                }
+                };
+                miner_updated = true;
+                remove_miner = add_nonce == 0 && account.changed_storage_slots().count() == 0;
             }
 
             // If the account is touched, it means that the account's state has been modified
@@ -453,7 +445,7 @@ impl<DB> PartitionDB<DB> {
                 write_set.insert(LocationAndType::Storage(*address, *slot));
             }
         }
-        (write_set, miner_update)
+        (write_set, miner_update, remove_miner)
     }
 
     /// Temporary commit the state change after evm.transact() for each tx

tests/mainnet.rs

@@ -30,7 +30,7 @@ fn test_execute_alloy(block: Block, db: InMemoryDB) {
     let mut parallel_result = Err(GrevmError::UnreachableError(String::from("Init")));
     metrics::with_local_recorder(&recorder, || {
         let executor = GrevmScheduler::new(spec_id, env, db, txs);
-        parallel_result = executor.parallel_execute();
+        parallel_result = executor.force_parallel_execute(true, Some(23));
 
         let snapshot = recorder.snapshotter().snapshot();
         for (key, unit, desc, value) in snapshot.into_vec() {