(1) testool: use cache skip success cases (2) testool enable parallel compilation (3) fix many corner cases (#798)

* use cache skip success cases

* update comments

* dynamic tx table

* clippy

* make skip_tests/skip_paths opt

* write cached result

* many fixes

* write cached result

* fix oob

* better err log

* fix write_cache

* update codehash

* Revert "update codehash"

This reverts commit 0e3b077.

* parallel compile testcase

* testool: --use_cache to enable cache

---------

Co-authored-by: Zhang Zhuo <[email protected]>

Author: lightsing

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

@@ -104,7 +104,6 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
         let callee_code_hash = callee_call.code_hash;
         let callee_acc = state.sdb.get_account(&callee_address).1;
         let callee_exists = !callee_acc.is_empty();
-
         let (callee_code_hash_word, is_empty_code_hash) = if callee_exists {
             (
                 callee_code_hash.to_word(),
@@ -170,14 +169,14 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
         let is_precompile = code_address
             .map(|ref addr| is_precompiled(addr))
             .unwrap_or(false);
-        // TODO: What about transfer for CALLCODE?
+        // CALLCODE does not need to do real transfer.
         // Transfer value only for CALL opcode, is_precheck_ok = true.
         if callee_call.kind == CallKind::Call && is_precheck_ok {
             state.transfer(
                 &mut exec_step,
                 callee_call.caller_address,
                 callee_call.address,
-                callee_exists || is_precompile,
+                callee_exists,
                 false,
                 callee_call.value,
             )?;
@@ -473,7 +472,9 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
 
                 debug_assert_eq!(
                     geth_steps[0].gas.0 - gas_cost - precompile_call_gas_cost + stipend,
-                    geth_steps[1].gas.0
+                    geth_steps[1].gas.0,
+                    "precompile_call_gas_cost wrong {:?}",
+                    precompile_step.exec_state
                 );
 
                 Ok(vec![exec_step, precompile_step])

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

@@ -244,7 +244,7 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
 
         if !is_precheck_ok {
             for (field, value) in [
-                (CallContextField::LastCalleeId, 0.into()),
+                (CallContextField::LastCalleeId, callee.call_id.into()),
                 (CallContextField::LastCalleeReturnDataOffset, 0.into()),
                 (CallContextField::LastCalleeReturnDataLength, 0.into()),
             ] {
@@ -314,12 +314,13 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
 
         if length == 0 || is_address_collision {
             for (field, value) in [
-                (CallContextField::LastCalleeId, 0.into()),
+                (CallContextField::LastCalleeId, callee.call_id.into()),
                 (CallContextField::LastCalleeReturnDataOffset, 0.into()),
                 (CallContextField::LastCalleeReturnDataLength, 0.into()),
             ] {
                 state.call_context_write(&mut exec_step, caller.call_id, field, value);
             }
+            state.caller_ctx_mut()?.return_data.clear();
             state.handle_return(&mut exec_step, geth_steps, false)?;
         }
 

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

@@ -27,13 +27,16 @@ impl Opcode for Returndatasize {
         );
 
         // TODO: fix error in deposit_ether.json...
-        if value.as_usize() != state.call_ctx()?.return_data.len() {
+        let real_return_data_len = value.as_usize();
+        let local_return_data_len = state.call_ctx()?.return_data.len();
+        if real_return_data_len != local_return_data_len {
             log::error!(
                 "return_data.len() != RETURNDATASIZE value, {} != {}, step: {:?}",
-                state.call_ctx()?.return_data.len(),
-                value.as_usize(),
+                local_return_data_len,
+                real_return_data_len,
                 geth_step
             );
+            debug_assert_eq!(real_return_data_len, local_return_data_len);
         }
 
         state.stack_write(

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

@@ -31,7 +31,9 @@ impl Opcode for Stop {
             CallContextField::IsSuccess,
             1.into(),
         );
-
+        if let Ok(caller) = state.caller_ctx_mut() {
+            caller.return_data.clear();
+        }
         state.handle_return(&mut exec_step, geth_steps, !call.is_root)?;
 
         Ok(vec![exec_step])

testool/Cargo.toml

@@ -39,6 +39,7 @@ ctor = "0.1.22"
 
 [features]
 default = ["ignore-test-docker", "skip-self-destruct", "shanghai"]
+onephase = ["zkevm-circuits/onephase"]
 ignore-test-docker = []
 skip-self-destruct = []
 shanghai = ["bus-mapping/shanghai", "eth-types/shanghai", "mock/shanghai", "zkevm-circuits/shanghai"]

testool/Config.toml

@@ -167,15 +167,6 @@ tests = []
 
 # skipped tests, do not need to be fixed  --------------------------------------------------
 
-[[skip_tests]]
-desc = "geth should emit ErrGasUintOverflow rather than ErrOutOfGas"
-tests = [
-	"MSTORE_Bounds2_d0_g0_v0",
-	"createNameRegistratorOutOfMemoryBonds1_d0_g0_v0",
-	"createNameRegistratorOutOfMemoryBonds0_d0_g0_v0",
-	"CallToNameRegistratorMemOOGAndInsufficientBalance_d0_g0_v0",
-]
-
 # ignored paths -------------------------------------------------------------------------
 
 [[skip_paths]]

testool/src/compiler.rs

@@ -10,6 +10,7 @@ use std::{
     path::PathBuf,
     process::{Command, Stdio},
     str::FromStr,
+    sync::Mutex,
 };
 
 struct Cache {
@@ -201,16 +202,17 @@ struct SourceLocation {
 
 #[derive(Default)]
 pub struct Compiler {
-    cache: Option<Cache>,
+    cache: Option<Mutex<Cache>>,
     compile: bool,
 }
 
 impl Compiler {
     pub fn new(compile: bool, cache_path: Option<PathBuf>) -> Result<Self> {
-        let cache = cache_path.map(Cache::new).transpose()?;
+        let cache = cache_path.map(Cache::new).transpose()?.map(Mutex::new);
         Ok(Compiler { compile, cache })
     }
 
+    /// the concurrency level of the exec is controlled by rayon parallelism
     fn exec(args: &[&str], stdin: &str) -> Result<String> {
         let mut child = Command::new("docker")
             .args(args)
@@ -242,7 +244,7 @@ impl Compiler {
     }
 
     /// compiles ASM code
-    pub fn asm(&mut self, src: &str) -> Result<Bytes> {
+    pub fn asm(&self, src: &str) -> Result<Bytes> {
         let mut bytecode = Bytecode::default();
         for op in src.split(';') {
             let op = match bytecode::OpcodeWithData::from_str(op.trim()) {
@@ -256,9 +258,13 @@ impl Compiler {
     }
 
     /// compiles LLL code
-    pub fn lll(&mut self, src: &str) -> Result<Bytes> {
-        if let Some(bytecode) = self.cache.as_mut().and_then(|c| c.get(src)) {
-            return Ok(bytecode.clone());
+    pub fn lll(&self, src: &str) -> Result<Bytes> {
+        if let Some(bytecode) = self
+            .cache
+            .as_ref()
+            .and_then(|c| c.lock().unwrap().get(src).cloned())
+        {
+            return Ok(bytecode);
         }
         if !self.compile {
             bail!("No way to compile LLLC for '{}'", src)
@@ -267,26 +273,30 @@ impl Compiler {
         let stdout = Self::exec(&["run", "-i", "--rm", "lllc"], src)?;
         let bytecode = Bytes::from(hex::decode(stdout.trim())?);
 
-        if let Some(cache) = &mut self.cache {
-            cache.insert(src, bytecode.clone())?;
+        if let Some(ref cache) = self.cache {
+            cache.lock().unwrap().insert(src, bytecode.clone())?;
         }
 
         Ok(bytecode)
     }
 
     /// compiles YUL code
-    pub fn yul(&mut self, src: &str) -> Result<Bytes> {
+    pub fn yul(&self, src: &str) -> Result<Bytes> {
         self.solc(Language::Yul, src)
     }
 
     /// compiles Solidity code
-    pub fn solidity(&mut self, src: &str) -> Result<Bytes> {
+    pub fn solidity(&self, src: &str) -> Result<Bytes> {
         self.solc(Language::Solidity, src)
     }
 
-    fn solc(&mut self, language: Language, src: &str) -> Result<Bytes> {
-        if let Some(bytecode) = self.cache.as_mut().and_then(|c| c.get(src)) {
-            return Ok(bytecode.clone());
+    fn solc(&self, language: Language, src: &str) -> Result<Bytes> {
+        if let Some(bytecode) = self
+            .cache
+            .as_ref()
+            .and_then(|c| c.lock().unwrap().get(src).cloned())
+        {
+            return Ok(bytecode);
         }
         if !self.compile {
             bail!("No way to compile {:?} for '{}'", language, src)
@@ -312,8 +322,8 @@ impl Compiler {
 
         let bytecode = Bytes::from(hex::decode(bytecode)?);
 
-        if let Some(cache) = &mut self.cache {
-            cache.insert(src, bytecode.clone())?;
+        if let Some(ref cache) = self.cache {
+            cache.lock().unwrap().insert(src, bytecode.clone())?;
         }
 
         Ok(bytecode)

testool/src/config.rs

@@ -7,7 +7,9 @@ const CONFIG_FILE: &str = "Config.toml";
 pub struct Config {
     pub suite: Vec<TestSuite>,
     pub set: Vec<TestsSet>,
+    #[serde(default)]
     pub skip_paths: Vec<SkipPaths>,
+    #[serde(default)]
     pub skip_tests: Vec<SkipTests>,
 }
 

testool/src/main.rs

@@ -47,9 +47,13 @@ struct Args {
     #[clap(long)]
     ls: bool,
 
-    /// Cache execution results
+    /// Cache execution results, default to be latest result file
     #[clap(long)]
-    cache: Option<String>,
+    cache: Option<PathBuf>,
+
+    /// do not use cache
+    #[clap(long)]
+    use_cache: bool,
 
     /// whitelist level from cache result
     #[clap(short, long, value_parser, value_delimiter = ',')]
@@ -155,30 +159,55 @@ fn go() -> Result<()> {
             REPORT_FOLDER, args.suite, timestamp, git_hash
         );
 
+        let cache_file_name = if !args.use_cache {
+            None
+        } else {
+            let mut history_reports =
+                glob::glob(format!("{REPORT_FOLDER}/{}.*.*.csv", args.suite).as_str())?
+                    .collect::<Result<Vec<PathBuf>, glob::GlobError>>()?
+                    .into_iter()
+                    .map(|path| {
+                        path.metadata()
+                            .and_then(|meta| meta.created())
+                            .map(|created| (path, created))
+                    })
+                    .collect::<Result<Vec<(PathBuf, SystemTime)>, std::io::Error>>()?;
+            // sort by timestamp
+            history_reports.sort_by_key(|(_, created)| *created);
+            // use latest cache if exists
+            args.cache
+                .or_else(|| history_reports.pop().map(|(path, _)| path))
+        };
+
         // when running a report, the tests result of the containing cache file
         // are used, but by default removing all Ignored tests
         // Another way is to skip the test which level not in whitelist_levels
-        let mut previous_results = if let Some(cache_filename) = args.cache {
+        let mut previous_results = if let Some(cache_filename) = cache_file_name {
             let whitelist_levels = HashSet::<ResultLevel>::from_iter(args.levels);
 
-            let mut previous_results = Results::from_file(PathBuf::from(cache_filename))?;
+            let mut previous_results = Results::from_file(cache_filename).unwrap_or_else(|_| {
+                log::warn!("malformed cache file, won't use cache");
+                Results::default()
+            });
             if !whitelist_levels.is_empty() {
                 // if whitelist is provided, test not in whitelist will be skip
                 previous_results
                     .tests
                     .retain(|_, test| !whitelist_levels.contains(&test.level));
             } else {
-                // by default only skip ignore
-                previous_results
-                    .tests
-                    .retain(|_, test| test.level != ResultLevel::Ignored);
+                // by default skip ignore and success tests
+                previous_results.tests.retain(|_, test| {
+                    test.level == ResultLevel::Ignored || test.level == ResultLevel::Success
+                });
             }
 
             previous_results
         } else {
             Results::default()
         };
+
         previous_results.set_cache(PathBuf::from(csv_filename));
+        previous_results.write_cache()?;
         run_statetests_suite(state_tests, &circuits_config, &suite, &mut previous_results)?;
 
         // filter non-csv files and files from the same commit
@@ -209,7 +238,7 @@ fn go() -> Result<()> {
         info!("{}", html_filename);
     } else {
         let mut results = if let Some(cache_filename) = args.cache {
-            Results::with_cache(PathBuf::from(cache_filename))?
+            Results::with_cache(cache_filename)?
         } else {
             Results::default()
         };

testool/src/statetest/executor.rs

@@ -148,20 +148,7 @@ fn into_traceconfig(st: StateTest) -> (String, TraceConfig, StateTestResult) {
     let sig = wallet.sign_transaction_sync(&tx);
     let rlp_signed = tx.rlp_signed(&sig).to_vec();
     let tx_hash = keccak256(tx.rlp_signed(&sig));
-    let mut accounts = st.pre;
-    for i in 1..=9 {
-        let mut addr_bytes = [0u8; 20];
-        addr_bytes[19] = i as u8;
-        let address = Address::from(addr_bytes);
-        accounts
-            .entry(address)
-            .or_insert(eth_types::geth_types::Account {
-                // balance: 1.into(),
-                // nonce: 1.into(),
-                address,
-                ..Default::default()
-            });
-    }
+    let accounts = st.pre;
 
     (
         st.id,
@@ -322,14 +309,14 @@ pub fn run_test(
     if !circuits_config.super_circuit {
         let circuits_params = CircuitsParams {
             max_txs: 1,
-            max_rws: 0,
-            max_calldata: 5000,
+            max_rws: 0,      // dynamic
+            max_calldata: 0, // dynamic
             max_bytecode: 5000,
             max_mpt_rows: 5000,
             max_copy_rows: 55000,
-            max_evm_rows: 0,
+            max_evm_rows: 0, // dynamic
             max_exp_steps: 5000,
-            max_keccak_rows: 0,
+            max_keccak_rows: 0, // dynamic?
             max_inner_blocks: 64,
             max_rlp_rows: 6000,
             max_ec_ops: PrecompileEcParams {

testool/src/statetest/json.rs

@@ -98,11 +98,11 @@ impl Refs {
 }
 
 pub struct JsonStateTestBuilder<'a> {
-    compiler: &'a mut Compiler,
+    compiler: &'a Compiler,
 }
 
 impl<'a> JsonStateTestBuilder<'a> {
-    pub fn new(compiler: &'a mut Compiler) -> Self {
+    pub fn new(compiler: &'a Compiler) -> Self {
         Self { compiler }
     }
 
@@ -379,8 +379,8 @@ mod test {
 "#;
     #[test]
     fn test_json_parse() -> Result<()> {
-        let mut compiler = Compiler::new(true, None)?;
-        let mut builder = JsonStateTestBuilder::new(&mut compiler);
+        let compiler = Compiler::new(true, None)?;
+        let mut builder = JsonStateTestBuilder::new(&compiler);
         let test = builder.load_json("test_path", JSON)?.remove(0);
 
         let acc095e = Address::from_str("0x095e7baea6a6c7c4c2dfeb977efac326af552d87")?;

testool/src/statetest/parse.rs

@@ -61,7 +61,7 @@ fn decompose_tags(expr: &str) -> HashMap<String, String> {
 
 /// returns the element as calldata bytes, supports 0x, :raw, :abi, :yul and
 /// { LLL }
-pub fn parse_calldata(compiler: &mut Compiler, as_str: &str) -> Result<(Bytes, Option<Label>)> {
+pub fn parse_calldata(compiler: &Compiler, as_str: &str) -> Result<(Bytes, Option<Label>)> {
     let tags = decompose_tags(as_str);
     let label = tags.get(":label").cloned();
 
@@ -96,7 +96,7 @@ pub fn parse_calldata(compiler: &mut Compiler, as_str: &str) -> Result<(Bytes, O
 }
 
 /// parse entry as code, can be 0x, :raw or { LLL }
-pub fn parse_code(compiler: &mut Compiler, as_str: &str) -> Result<Bytes> {
+pub fn parse_code(compiler: &Compiler, as_str: &str) -> Result<Bytes> {
     let tags = decompose_tags(as_str);
 
     let code = if let Some(notag) = tags.get("") {