improve testool for scroll mode (#831)

* default: 97.6

* 98.6

* 98.9%

* 99.0%

* create gadget: disable reading callee codehash if !pre_check_ok

* fix base fee

* solved all

* fixed finally?

* ...

* done

Author: lispc

bus-mapping/src/circuit_input_builder/l2.rs

@@ -40,11 +40,18 @@ impl From<&ZktrieState> for StateDB {
         }
 
         for (storage_key, data) in mpt_state.storage() {
-            if !data.as_ref().is_zero() {
-                //TODO: add an warning on non-existed account?
-                let (_, acc) = sdb.get_account_mut(&storage_key.0);
-                acc.storage.insert(storage_key.1, *data.as_ref());
-            }
+            // Since the StateDB is a partical db, 0 means we know it is zero instead of "unknown".
+            //if !data.as_ref().is_zero() {
+            log::trace!(
+                "trace sdb: addr {:?} key {:?} value {:?}",
+                storage_key.0,
+                storage_key.1,
+                *data.as_ref()
+            );
+            //TODO: add an warning on non-existed account?
+            let (_, acc) = sdb.get_account_mut(&storage_key.0);
+            acc.storage.insert(storage_key.1, *data.as_ref());
+            //}
         }
         sdb
     }

bus-mapping/src/evm/opcodes/create.rs

@@ -192,22 +192,25 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
             H256::zero()
         };
 
-        // use CodeHash rw not zero to check address already exists
-        state.account_read(
-            &mut exec_step,
-            address,
-            AccountField::CodeHash,
-            code_hash_previous.to_word(),
-        );
-
-        // read callee nonce for address collision checking
-        if !code_hash_previous.is_zero() {
+        // If precheck is not ok, we even do not need to read codehash of callee.
+        if is_precheck_ok {
+            // use CodeHash rw not zero to check address already exists
             state.account_read(
                 &mut exec_step,
                 address,
-                AccountField::Nonce,
-                callee_account.nonce,
+                AccountField::CodeHash,
+                code_hash_previous.to_word(),
             );
+
+            // read callee nonce for address collision checking
+            if !code_hash_previous.is_zero() {
+                state.account_read(
+                    &mut exec_step,
+                    address,
+                    AccountField::Nonce,
+                    callee_account.nonce,
+                );
+            }
         }
 
         if is_precheck_ok && !is_address_collision {

bus-mapping/src/util.rs

@@ -18,18 +18,20 @@ pub static CHECK_MEM_STRICT: Lazy<bool> = Lazy::new(|| read_env_var("CHECK_MEM_S
 pub const POSEIDON_HASH_BYTES_IN_FIELD: usize = 31;
 
 /// Default code hash
-pub(crate) fn hash_code(code: &[u8]) -> Hash {
+pub fn hash_code(code: &[u8]) -> Hash {
     #[cfg(feature = "scroll")]
     return hash_code_poseidon(code);
     #[cfg(not(feature = "scroll"))]
     return hash_code_keccak(code);
 }
 
-pub(crate) fn hash_code_keccak(code: &[u8]) -> Hash {
+/// Keccak code hash
+pub fn hash_code_keccak(code: &[u8]) -> Hash {
     eth_types::H256(ethers_core::utils::keccak256(code))
 }
 
-pub(crate) fn hash_code_poseidon(code: &[u8]) -> Hash {
+/// Poseidon code hash
+pub fn hash_code_poseidon(code: &[u8]) -> Hash {
     use poseidon_circuit::hash::{Hashable, MessageHashable, HASHABLE_DOMAIN_SPEC};
 
     let bytes_in_field = POSEIDON_HASH_BYTES_IN_FIELD;

external-tracer/src/lib.rs

@@ -120,7 +120,9 @@ pub fn l2trace(config: &TraceConfig) -> Result<BlockTrace, Error> {
 
     log::trace!("trace: {}", trace_string);
 
-    let trace = serde_json::from_str(&trace_string).map_err(Error::SerdeError)?;
+    let mut trace: BlockTrace = serde_json::from_str(&trace_string).map_err(Error::SerdeError)?;
+    // l2 geth returns nil base_fee even if it is not 0..
+    trace.header.base_fee_per_gas = Some(config.block_constants.base_fee);
     Ok(trace)
 }
 

geth-utils/l2geth/go.mod

@@ -5,7 +5,7 @@ go 1.18
 require (
 	github.com/holiman/uint256 v1.2.0
 	github.com/imdario/mergo v0.3.15
-	github.com/scroll-tech/go-ethereum v1.10.14-0.20230827010026-8e452d413b6f
+	github.com/scroll-tech/go-ethereum v1.10.14-0.20230827145523-9be082146425
 )
 
 require (

geth-utils/l2geth/go.sum

@@ -109,6 +109,16 @@ github.com/scroll-tech/go-ethereum v1.10.14-0.20230820103920-be1600f07899 h1:bEx
 github.com/scroll-tech/go-ethereum v1.10.14-0.20230820103920-be1600f07899/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
 github.com/scroll-tech/go-ethereum v1.10.14-0.20230827010026-8e452d413b6f h1:FaxROT7QkVZ0ezyW0VxLDjc3yy0h1DDRNC12vitsYmA=
 github.com/scroll-tech/go-ethereum v1.10.14-0.20230827010026-8e452d413b6f/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827094559-de45f08802ca h1:tbbaZgk0dI7ZMCJCcYVnQiy1B2xfN4qqOqeELRIYLJE=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827094559-de45f08802ca/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827133453-e17c463eaee2 h1:0OVWWVN6VL5zR+OIrl1j2cBto5s44aoNHw7xKIqBVvU=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827133453-e17c463eaee2/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827134248-41544bfba4db h1:P2qM3/F3CPfGUyxivM9GEVZyGdn7ausd4/3y0QVS6IE=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827134248-41544bfba4db/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827135613-d4f288562708 h1:4O2FUl4E5NiW8vRlUKISOD2QqcKU/BBG7+VARSFSVKQ=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827135613-d4f288562708/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827145523-9be082146425 h1:xyKoHzmC7yekIvRouztNS6NP7EW7UPh+tjA6jLMP7Q8=
+github.com/scroll-tech/go-ethereum v1.10.14-0.20230827145523-9be082146425/go.mod h1:DiN3p2inoXOxGffxSswDKqWjQ7bU+Mp0c9v0XQXKmaA=
 github.com/scroll-tech/zktrie v0.6.0 h1:xLrMAO31Yo2BiPg1jtYKzcjpEFnXy8acbB7iIsyshPs=
 github.com/scroll-tech/zktrie v0.6.0/go.mod h1:XvNo7vAk8yxNyTjBDj5WIiFzYW4bx/gJ78+NK6Zn6Uk=
 github.com/shirou/gopsutil v3.21.4-0.20210419000835-c7a38de76ee5+incompatible h1:Bn1aCHHRnjv4Bl16T8rcaFjYSrGrIZvpiGO6P3Q4GpU=

testool/src/statetest/executor.rs

@@ -1,8 +1,12 @@
 use super::{AccountMatch, StateTest, StateTestResult};
 use crate::config::TestSuite;
-use bus_mapping::circuit_input_builder::{CircuitInputBuilder, CircuitsParams, PrecompileEcParams};
+use bus_mapping::{
+    circuit_input_builder::{CircuitInputBuilder, CircuitsParams, PrecompileEcParams},
+    state_db::CodeDB,
+};
 use eth_types::{
-    geth_types, geth_types::TxType, Address, Bytes, GethExecTrace, ToBigEndian, U256, U64,
+    geth_types, geth_types::TxType, Address, Bytes, GethExecTrace, ToBigEndian, ToWord, H256, U256,
+    U64,
 };
 use ethers_core::{
     types::{transaction::eip2718::TypedTransaction, TransactionRequest},
@@ -43,6 +47,9 @@ pub enum StateTestError {
     SkipTestMaxSteps(usize),
     #[error("SkipTestSelfDestruct")]
     SkipTestSelfDestruct,
+    #[error("SkipTestDifficulty")]
+    // scroll evm always returns 0 for "difficulty" opcode
+    SkipTestDifficulty,
     #[error("SkipTestBalanceOverflow")]
     SkipTestBalanceOverflow,
     #[error("Exception(expected:{expected:?}, found:{found:?})")]
@@ -53,12 +60,15 @@ impl StateTestError {
     pub fn is_skip(&self) -> bool {
         // Avoid lint `variant is never constructed` if no feature skip-self-destruct.
         let _ = StateTestError::SkipTestSelfDestruct;
+        let _ = StateTestError::SkipTestDifficulty;
 
         matches!(
             self,
             StateTestError::SkipTestMaxSteps(_)
                 | StateTestError::SkipTestMaxGasLimit(_)
                 | StateTestError::SkipTestSelfDestruct
+                | StateTestError::SkipTestBalanceOverflow
+                | StateTestError::SkipTestDifficulty
         )
     }
 }
@@ -214,12 +224,22 @@ fn check_geth_traces(
     verbose: bool,
 ) -> Result<(), StateTestError> {
     #[cfg(feature = "skip-self-destruct")]
+    if geth_traces.iter().any(|gt| {
+        gt.struct_logs.iter().any(|sl| {
+            sl.op == eth_types::evm_types::OpcodeId::SELFDESTRUCT
+                || sl.op == eth_types::evm_types::OpcodeId::INVALID(0xff)
+        })
+    }) {
+        return Err(StateTestError::SkipTestSelfDestruct);
+    }
+
+    #[cfg(feature = "scroll")]
     if geth_traces.iter().any(|gt| {
         gt.struct_logs
             .iter()
-            .any(|sl| sl.op == eth_types::evm_types::OpcodeId::SELFDESTRUCT)
+            .any(|sl| sl.op == eth_types::evm_types::OpcodeId::DIFFICULTY)
     }) {
-        return Err(StateTestError::SkipTestSelfDestruct);
+        return Err(StateTestError::SkipTestDifficulty);
     }
 
     if geth_traces[0].struct_logs.len() as u64 > suite.max_steps {
@@ -508,22 +528,22 @@ pub fn run_test(
 
     #[cfg(feature = "scroll")]
     let result = trace_config_to_witness_block_l2(
-        trace_config,
+        trace_config.clone(),
         st,
         suite,
         circuits_params,
         circuits_config.verbose,
     )?;
     #[cfg(not(feature = "scroll"))]
     let result = trace_config_to_witness_block_l1(
-        trace_config,
+        trace_config.clone(),
         st,
         suite,
         circuits_params,
         circuits_config.verbose,
     )?;
 
-    let (witness_block, builder) = match result {
+    let (witness_block, mut builder) = match result {
         Some((witness_block, builder)) => (witness_block, builder),
         None => return Ok(()),
     };
@@ -545,6 +565,35 @@ pub fn run_test(
         prover.assert_satisfied_par();
     };
 
+    //#[cfg(feature = "scroll")]
+    {
+        // fill these "untouched" storage slots
+        // It is better to fill these info after (instead of before) bus-mapping re-exec.
+        // To prevent these data being used unexpectedly.
+        for account in trace_config.accounts.values() {
+            builder.code_db.insert(account.code.to_vec());
+            let (exist, acc_in_local_sdb) = builder.sdb.get_account_mut(&account.address);
+            if !exist {
+                // modified from bus-mapping/src/mock.rs
+                let keccak_code_hash = H256(keccak256(account.code.to_vec()));
+                let code_hash = CodeDB::hash(&account.code.to_vec());
+                *acc_in_local_sdb = bus_mapping::state_db::Account {
+                    nonce: account.nonce,
+                    balance: account.balance,
+                    storage: account.storage.clone(),
+                    code_hash,
+                    keccak_code_hash,
+                    code_size: account.code.len().to_word(),
+                };
+            } else {
+                for (k, v) in &account.storage {
+                    if !acc_in_local_sdb.storage.contains_key(k) {
+                        acc_in_local_sdb.storage.insert(*k, *v);
+                    }
+                }
+            }
+        }
+    }
     check_post(&builder, &post)?;
 
     Ok(())

testool/src/statetest/spec.rs

@@ -3,10 +3,7 @@ use eth_types::{geth_types::Account, Address, Bytes, Word, H256, U256};
 use ethers_core::{k256::ecdsa::SigningKey, utils::secret_key_to_address};
 use std::{collections::HashMap, str::FromStr};
 
-/// TODO: Explain this
-#[cfg(feature = "scroll")]
-pub const DEFAULT_BASE_FEE: u32 = 0;
-#[cfg(not(feature = "scroll"))]
+/// https://github.com/ethereum/tests/pull/857 "set default gasPrice to 10"
 pub const DEFAULT_BASE_FEE: u32 = 10;
 
 #[derive(PartialEq, Eq, Debug, Clone)]

zkevm-circuits/src/evm_circuit/execution/create.rs

@@ -285,20 +285,22 @@ impl<F: Field, const IS_CREATE2: bool, const S: ExecutionState> ExecutionGadget<
             );
         });
 
-        // check for address collision case by code hash previous
-        cb.account_read(
-            new_address.clone(),
-            AccountFieldTag::CodeHash,
-            code_hash_previous.expr(),
-        );
         // callee address's nonce
         let code_hash_is_zero = IsZeroGadget::construct(cb, code_hash_previous.expr());
-        cb.condition(not::expr(code_hash_is_zero.expr()), |cb| {
+        // check for address collision case by code hash previous
+        cb.condition(is_precheck_ok.expr(), |cb| {
             cb.account_read(
                 new_address.clone(),
-                AccountFieldTag::Nonce,
-                callee_nonce.expr(),
+                AccountFieldTag::CodeHash,
+                code_hash_previous.expr(),
             );
+            cb.condition(not::expr(code_hash_is_zero.expr()), |cb| {
+                cb.account_read(
+                    new_address.clone(),
+                    AccountFieldTag::Nonce,
+                    callee_nonce.expr(),
+                );
+            });
         });
 
         let code_hash_is_empty =
@@ -651,7 +653,7 @@ impl<F: Field, const IS_CREATE2: bool, const S: ExecutionState> ExecutionGadget<
         let is_precheck_ok =
             call.depth < 1025 && caller_balance >= value && caller_nonce < u64::MAX;
         if is_precheck_ok {
-            rws.next();
+            rws.next(); // caller nonce += 1
         }
         let [callee_rw_counter_end_of_reversion, callee_is_persistent] =
             [(); 2].map(|_| rws.next().call_context_value());
@@ -667,14 +669,18 @@ impl<F: Field, const IS_CREATE2: bool, const S: ExecutionState> ExecutionGadget<
         )?;
 
         // retrieve code_hash for creating address
-        let code_hash_previous = rws.next().account_codehash_pair();
-        let callee_nonce = if !code_hash_previous.0.is_zero() {
+        let code_hash_previous = if is_precheck_ok {
+            rws.next().account_codehash_pair().0
+        } else {
+            0.into()
+        };
+        let callee_nonce = if !code_hash_previous.is_zero() {
             rws.next().account_nonce_pair().1.low_u64()
         } else {
             0
         };
 
-        let code_hash_previous_rlc = region.code_hash(code_hash_previous.0);
+        let code_hash_previous_rlc = region.code_hash(code_hash_previous);
         self.code_hash_previous
             .assign(region, offset, code_hash_previous_rlc)?;
         self.code_hash_is_zero
@@ -692,8 +698,8 @@ impl<F: Field, const IS_CREATE2: bool, const S: ExecutionState> ExecutionGadget<
             .assign(region, offset, F::from(callee_nonce))?;
 
         let callee_nonce_is_zero = callee_nonce == 0;
-        let codehash_non_empty = !code_hash_previous.0.is_zero()
-            && code_hash_previous.0 != CodeDB::empty_code_hash().to_word();
+        let codehash_non_empty = !code_hash_previous.is_zero()
+            && code_hash_previous != CodeDB::empty_code_hash().to_word();
         let is_address_collision = !callee_nonce_is_zero || codehash_non_empty;
         if is_precheck_ok && !is_address_collision {
             /*

zkevm-circuits/src/super_circuit/test.rs

@@ -18,7 +18,9 @@ use rand_chacha::ChaCha20Rng;
 use std::env::set_var;
 
 use crate::witness::block_apply_mpt_state;
-use eth_types::{address, bytecode, l2_types::BlockTrace, word, Bytecode, ToWord, Word};
+#[cfg(feature = "scroll")]
+use eth_types::l2_types::BlockTrace;
+use eth_types::{address, bytecode, word, Bytecode, ToWord, Word};
 
 #[test]
 fn super_circuit_degree() {