Fix: commit minner nonce to ensure the nonce check by EVM

.github/workflows/grevm-test.yml

@@ -15,6 +15,8 @@ jobs:
       # check out
       - name: Checkout repository
         uses: actions/checkout@v3
+        with:
+          submodules: 'true'
 
       # install rust tools
       - name: Set up Rust
@@ -24,4 +26,4 @@ jobs:
           override: true
 
       - name: Run tests
-        run: cargo test -- --skip mainnet
+        run: cargo test

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/native_transfers.rs

@@ -272,10 +272,10 @@ fn native_loaded_not_existing_account() {
 
 #[test]
 fn native_transfer_with_beneficiary() {
-    let block_size = 100; // number of transactions
+    let block_size = 20; // number of transactions
     let accounts = common::mock_block_accounts(START_ADDRESS, block_size);
     let db = InMemoryDB::new(accounts, Default::default(), Default::default());
-    let mut txs: Vec<TxEnv> = (START_ADDRESS..START_ADDRESS + block_size - 4)
+    let mut txs: Vec<TxEnv> = (START_ADDRESS..START_ADDRESS + block_size)
         .map(|i| {
             let address = Address::from(U160::from(i));
             TxEnv {
@@ -291,71 +291,57 @@ fn native_transfer_with_beneficiary() {
         .collect();
     let start_address = Address::from(U160::from(START_ADDRESS));
     let miner_address = Address::from(U160::from(MINER_ADDRESS));
-    // 19 => 20
-    txs.insert(
-        20,
-        TxEnv {
-            caller: Address::from(U160::from(START_ADDRESS + 19)),
-            transact_to: TransactTo::Call(Address::from(U160::from(START_ADDRESS + 20))),
-            value: U256::from(100),
-            gas_limit: common::TRANSFER_GAS_LIMIT,
-            gas_price: U256::from(1),
-            nonce: None,
-            ..TxEnv::default()
-        },
-    );
     // miner => start
-    // failed for: LackOfFoundForMaxFee in the first round
-    txs.insert(
-        40,
-        TxEnv {
-            caller: miner_address,
-            transact_to: TransactTo::Call(start_address),
-            value: U256::from(100),
-            gas_limit: common::TRANSFER_GAS_LIMIT,
-            gas_price: U256::from(1),
-            nonce: None,
-            ..TxEnv::default()
-        },
-    );
+    txs.push(TxEnv {
+        caller: miner_address,
+        transact_to: TransactTo::Call(start_address),
+        value: U256::from(1),
+        gas_limit: common::TRANSFER_GAS_LIMIT,
+        gas_price: U256::from(1),
+        nonce: Some(1),
+        ..TxEnv::default()
+    });
+    // miner => start
+    txs.push(TxEnv {
+        caller: miner_address,
+        transact_to: TransactTo::Call(start_address),
+        value: U256::from(1),
+        gas_limit: common::TRANSFER_GAS_LIMIT,
+        gas_price: U256::from(1),
+        nonce: Some(2),
+        ..TxEnv::default()
+    });
     // start => miner
-    txs.insert(
-        60,
-        TxEnv {
-            caller: start_address,
-            transact_to: TransactTo::Call(miner_address),
-            value: U256::from(100),
-            gas_limit: common::TRANSFER_GAS_LIMIT,
-            gas_price: U256::from(1),
-            nonce: None,
-            ..TxEnv::default()
-        },
-    );
+    txs.push(TxEnv {
+        caller: start_address,
+        transact_to: TransactTo::Call(miner_address),
+        value: U256::from(1),
+        gas_limit: common::TRANSFER_GAS_LIMIT,
+        gas_price: U256::from(1),
+        nonce: Some(2),
+        ..TxEnv::default()
+    });
     // miner => miner
-    txs.insert(
-        80,
-        TxEnv {
-            caller: miner_address,
-            transact_to: TransactTo::Call(miner_address),
-            value: U256::from(100),
-            gas_limit: common::TRANSFER_GAS_LIMIT,
-            gas_price: U256::from(1),
-            nonce: None,
-            ..TxEnv::default()
-        },
-    );
+    txs.push(TxEnv {
+        caller: miner_address,
+        transact_to: TransactTo::Call(miner_address),
+        value: U256::from(1),
+        gas_limit: common::TRANSFER_GAS_LIMIT,
+        gas_price: U256::from(1),
+        nonce: Some(3),
+        ..TxEnv::default()
+    });
     common::compare_evm_execute(
         db,
         txs,
-        false,
+        true,
         [
             ("grevm.parallel_round_calls", DebugValue::Counter(2)),
             ("grevm.sequential_execute_calls", DebugValue::Counter(0)),
-            ("grevm.parallel_tx_cnt", DebugValue::Counter(block_size as u64)),
-            ("grevm.conflict_tx_cnt", DebugValue::Counter(5)),
-            ("grevm.unconfirmed_tx_cnt", DebugValue::Counter(75)),
-            ("grevm.reusable_tx_cnt", DebugValue::Counter(75)),
-            ("grevm.partition_num_tx_diff", DebugValue::Gauge(1.0.into())),
+            ("grevm.parallel_tx_cnt", DebugValue::Counter(24 as u64)),
+            ("grevm.conflict_tx_cnt", DebugValue::Counter(4)),
+            ("grevm.unconfirmed_tx_cnt", DebugValue::Counter(0)),
+            ("grevm.reusable_tx_cnt", DebugValue::Counter(0)),
         ]
         .into_iter()
         .collect(),