Circuit for ErrorInvalidOpcode state

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

@@ -0,0 +1,42 @@
+_PR description_
+
+### Contents
+
+- _item_
+
+### Rationale
+
+_design decisions and extended information_
+
+
+<hr>
+
+## How to fill a PR description (remove this)
+
+Please give a concise description of your PR.
+
+The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request.
+
+MUST: Reference the issue to resolve
+
+### Single responsability
+
+Is RECOMMENDED to create single responsibility commits, but not mandatory.
+
+Anyway, you MUST enumerate the changes in a unitary way, e.g.
+
+```
+This PR contains:
+
+- Cleanup of xxxx, yyyy
+- Changed xxxx to yyyy in order to bla bla
+- Added xxxx function to ...
+- Refactored ....
+```
+
+### Design choices
+
+RECOMMENDED to:
+- What types of design choices did you face?
+- What decisions you have made?
+- Any valuable information that could help reviewers to think critically

bus-mapping/src/circuit_input_builder.rs

@@ -45,6 +45,8 @@ pub struct CircuitsParams {
     pub max_txs: usize,
     /// Maximum number of bytes from all txs calldata in the Tx Circuit
     pub max_calldata: usize,
+    /// Max ammount of rows that the CopyCircuit can have.
+    pub max_copy_rows: usize,
     /// Maximum number of bytes supported in the Bytecode Circuit
     pub max_bytecode: usize,
     // TODO: Rename for consistency
@@ -60,6 +62,9 @@ impl Default for CircuitsParams {
             max_rws: 1000,
             max_txs: 1,
             max_calldata: 256,
+            // TODO: Check whether this value is correct or we should increase/decrease based on
+            // this lib tests
+            max_copy_rows: 1000,
             max_bytecode: 512,
             keccak_padding: None,
         }

bus-mapping/src/circuit_input_builder/execution.rs

@@ -155,8 +155,11 @@ impl ExecState {
 /// Defines the various source/destination types for a copy event.
 #[derive(Clone, Copy, Debug, PartialEq, Eq, EnumIter)]
 pub enum CopyDataType {
+    /// When we need to pad the Copy rows of the circuit up to a certain maximum
+    /// with rows that are not "useful".
+    Padding = 0,
     /// When the source for the copy event is the bytecode table.
-    Bytecode = 1,
+    Bytecode,
     /// When the source/destination for the copy event is memory.
     Memory,
     /// When the source for the copy event is tx's calldata.
@@ -252,12 +255,12 @@ impl CopyEvent {
                     .checked_sub(self.src_addr)
                     .unwrap_or_default(),
             ),
-            CopyDataType::RlcAcc | CopyDataType::TxLog => unreachable!(),
+            CopyDataType::RlcAcc | CopyDataType::TxLog | CopyDataType::Padding => unreachable!(),
         };
         let destination_rw_increase = match self.dst_type {
             CopyDataType::RlcAcc | CopyDataType::Bytecode => 0,
             CopyDataType::TxLog | CopyDataType::Memory => u64::try_from(step_index).unwrap() / 2,
-            CopyDataType::TxCalldata => unreachable!(),
+            CopyDataType::TxCalldata | CopyDataType::Padding => unreachable!(),
         };
         source_rw_increase + destination_rw_increase
     }

bus-mapping/src/circuit_input_builder/input_state_ref.rs

@@ -1190,6 +1190,18 @@ impl<'a> CircuitInputStateRef<'a> {
                 CallContextField::IsSuccess,
                 0u64.into(),
             );
+
+            //Even call.rw_counter_end_of_reversion is zero for now, it will set in
+            //set_value_ops_call_context_rwc_eor later
+            // if call fails, no matter root or internal, read RwCounterEndOfReversion for
+            // circuit constraint.
+            self.call_context_read(
+                exec_step,
+                call.call_id,
+                CallContextField::RwCounterEndOfReversion,
+                call.rw_counter_end_of_reversion.into(),
+            );
+
             if call.is_root {
                 return Ok(());
             }

bus-mapping/src/evm/opcodes.rs

@@ -54,6 +54,7 @@ mod stop;
 mod swap;
 
 mod error_invalid_jump;
+mod error_invalid_opcode;
 mod error_oog_call;
 
 #[cfg(test)]
@@ -72,7 +73,8 @@ use codecopy::Codecopy;
 use codesize::Codesize;
 use create::DummyCreate;
 use dup::Dup;
-use error_invalid_jump::ErrorInvalidJump;
+use error_invalid_jump::InvalidJump;
+use error_invalid_opcode::InvalidOpcode;
 use error_oog_call::OOGCall;
 use exp::Exponentiation;
 use extcodecopy::Extcodecopy;
@@ -256,7 +258,8 @@ fn fn_gen_associated_ops(opcode_id: &OpcodeId) -> FnGenAssociatedOps {
 
 fn fn_gen_error_state_associated_ops(error: &ExecError) -> Option<FnGenAssociatedOps> {
     match error {
-        ExecError::InvalidJump => Some(ErrorInvalidJump::gen_associated_ops),
+        ExecError::InvalidJump => Some(InvalidJump::gen_associated_ops),
+        ExecError::InvalidOpcode => Some(InvalidOpcode::gen_associated_ops),
         ExecError::OutOfGas(OogError::Call) => Some(OOGCall::gen_associated_ops),
         // call & callcode can encounter InsufficientBalance error, Use pop-7 generic CallOpcode
         ExecError::InsufficientBalance => Some(CallOpcode::<7>::gen_associated_ops),

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

@@ -4,9 +4,9 @@ use crate::Error;
 use eth_types::{GethExecStep, ToAddress, ToWord, Word};
 
 #[derive(Debug, Copy, Clone)]
-pub(crate) struct ErrorInvalidJump;
+pub(crate) struct InvalidJump;
 
-impl Opcode for ErrorInvalidJump {
+impl Opcode for InvalidJump {
     fn gen_associated_ops(
         state: &mut CircuitInputStateRef,
         geth_steps: &[GethExecStep],

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

@@ -0,0 +1,29 @@
+use crate::circuit_input_builder::{CircuitInputStateRef, ExecStep};
+use crate::evm::Opcode;
+use crate::Error;
+use eth_types::GethExecStep;
+
+#[derive(Debug, Copy, Clone)]
+pub(crate) struct InvalidOpcode;
+
+impl Opcode for InvalidOpcode {
+    fn gen_associated_ops(
+        state: &mut CircuitInputStateRef,
+        geth_steps: &[GethExecStep],
+    ) -> Result<Vec<ExecStep>, Error> {
+        let geth_step = &geth_steps[0];
+        let mut exec_step = state.new_step(geth_step)?;
+
+        let next_step = if geth_steps.len() > 1 {
+            Some(&geth_steps[1])
+        } else {
+            None
+        };
+        exec_step.error = state.get_step_err(geth_step, next_step).unwrap();
+
+        state.gen_restore_context_ops(&mut exec_step, geth_steps)?;
+        state.handle_return(geth_step)?;
+
+        Ok(vec![exec_step])
+    }
+}

circuit-benchmarks/src/super_circuit.rs

@@ -71,7 +71,7 @@ mod tests {
 
         block.sign(&wallets);
 
-        let (_, circuit, instance, _) = SuperCircuit::<_, 1, 32, 512>::build(block).unwrap();
+        let (_, circuit, instance, _) = SuperCircuit::<_, 1, 32, 512, 512>::build(block).unwrap();
         let instance_refs: Vec<&[Fr]> = instance.iter().map(|v| &v[..]).collect();
 
         // Bench setup generation
@@ -96,7 +96,7 @@ mod tests {
             Challenge255<G1Affine>,
             ChaChaRng,
             Blake2bWrite<Vec<u8>, G1Affine, Challenge255<G1Affine>>,
-            SuperCircuit<Fr, 1, 32, 512>,
+            SuperCircuit<Fr, 1, 32, 512, 512>,
         >(
             &general_params,
             &pk,

integration-tests/src/integration_test_circuits.rs

@@ -38,6 +38,7 @@ const CIRCUITS_PARAMS: CircuitsParams = CircuitsParams {
     max_txs: 4,
     max_calldata: 4000,
     max_bytecode: 4000,
+    max_copy_rows: 16384,
     keccak_padding: None,
 };
 
@@ -344,6 +345,7 @@ pub async fn test_super_circuit_block(block_num: u64) {
     const MAX_CALLDATA: usize = 512;
     const MAX_RWS: usize = 5888;
     const MAX_BYTECODE: usize = 5000;
+    const MAX_COPY_ROWS: usize = 5888;
 
     log::info!("test super circuit, block number: {}", block_num);
     let cli = get_client();
@@ -354,14 +356,15 @@ pub async fn test_super_circuit_block(block_num: u64) {
             max_txs: MAX_TXS,
             max_calldata: MAX_CALLDATA,
             max_bytecode: MAX_BYTECODE,
+            max_copy_rows: MAX_COPY_ROWS,
             keccak_padding: None,
         },
     )
     .await
     .unwrap();
     let (builder, _) = cli.gen_inputs(block_num).await.unwrap();
     let (k, circuit, instance) =
-        SuperCircuit::<Fr, MAX_TXS, MAX_CALLDATA, MAX_RWS>::build_from_circuit_input_builder(
+        SuperCircuit::<Fr, MAX_TXS, MAX_CALLDATA, MAX_RWS, MAX_COPY_ROWS>::build_from_circuit_input_builder(
             &builder,
         )
         .unwrap();

integration-tests/tests/circuit_input_builder.rs

@@ -18,6 +18,7 @@ async fn test_circuit_input_builder_block(block_num: u64) {
             max_txs: 1,
             max_calldata: 4000,
             max_bytecode: 4000,
+            max_copy_rows: 16384,
             keccak_padding: None,
         },
     )

testool/Config.toml

@@ -3,7 +3,6 @@ id="default"
 path="tests/src/GeneralStateTestsFiller/**/*"
 max_gas = 500000
 max_steps = 1000
-max_rws = 51000
 unimplemented_opcodes = []
 ignore_tests = []
 
@@ -12,22 +11,18 @@ id="nightly"
 path="tests/src/GeneralStateTestsFiller/**/*"
 max_gas = 500000
 max_steps = 1000
-max_rws = 51000
 unimplemented_opcodes = [
-    "SAR",
-    "EXTCODECOPY",
     "CREATE",
     "CREATE2",
     "SELFDESTRUCT"
 ]
-ignore_tests=["&tofix"]
+ignore_tests=["&tofix", "&sigkill"]
 
 [[suite]]
 id = "light"
 path="tests/src/GeneralStateTestsFiller/**/*"
 max_gas = 500000
 max_steps = 1000
-max_rws = 51000
 unimplemented_opcodes = []
 allow_tests=[
 	"add_d0(add_neg1_neg1)_g0_v0",
@@ -174,6 +169,13 @@ allow_tests=[
 
 # must fix  ------------------------------------------------------------------------------
 
+[[set]]
+id = "sigkill"
+desc = "tests that sigkill"
+tests = [
+	"CallToNameRegistratorMemOOGAndInsufficientBalance_d0_g0_v0"
+]
+
 [[set]]
 id  = "tofix"
 desc = "***panicked at 'circuit should pass', contraint error"

testool/src/config.rs

@@ -18,7 +18,6 @@ pub struct TestSuite {
     pub path: String,
     pub max_gas: u64,
     pub max_steps: u64,
-    pub max_rws: u64,
 
     /// see [Implemented opcodes status](https://github.com/appliedzkp/zkevm-circuits/issues/477)
     pub unimplemented_opcodes: Vec<OpcodeId>,
@@ -31,9 +30,8 @@ impl Default for TestSuite {
         Self {
             id: "default".to_string(),
             path: String::default(),
-            max_gas: 100000,
-            max_steps: 1000,
-            max_rws: 50104,
+            max_gas: u64::MAX,
+            max_steps: u64::MAX,
             unimplemented_opcodes: Vec::new(),
             ignore_tests: Some(Vec::new()),
             allow_tests: None,

testool/src/main.rs

@@ -10,7 +10,7 @@ use anyhow::{bail, Result};
 use clap::Parser;
 use compiler::Compiler;
 use config::Config;
-use log::{debug, error, info};
+use log::{error, info};
 use statetest::{
     geth_trace, load_statetests_suite, run_statetests_suite, run_test, CircuitsConfig, Results,
     StateTest,
@@ -72,15 +72,13 @@ struct Args {
 const RESULT_CACHE: &str = "result.cache";
 
 fn run_single_test(test: StateTest, circuits_config: CircuitsConfig) -> Result<()> {
-    debug!("{}", &test);
-
+    println!("{}", &test);
     let trace = geth_trace(test.clone())?;
     crate::utils::print_trace(trace)?;
-    debug!(
+    println!(
         "result={:?}",
         run_test(test, TestSuite::default(), circuits_config)
     );
-
     Ok(())
 }
 

testool/src/statetest/executor.rs

@@ -287,6 +287,7 @@ pub fn run_test(
             max_rws: 55000,
             max_calldata: 5000,
             max_bytecode: 5000,
+            max_copy_rows: 55000,
             keccak_padding: None,
         };
         let block_data = BlockData::new_from_geth_data_with_params(geth_data, circuits_params);
@@ -312,7 +313,7 @@ pub fn run_test(
         geth_data.sign(&wallets);
 
         let (k, circuit, instance, _builder) =
-            SuperCircuit::<Fr, 1, 32, 255>::build(geth_data).unwrap();
+            SuperCircuit::<Fr, 1, 32, 255, 32>::build(geth_data).unwrap();
         builder = _builder;
 
         let prover = MockProver::run(k, &circuit, instance).unwrap();

testool/src/statetest/results.rs

@@ -320,7 +320,12 @@ impl Results {
                     result.details
                 );
             }
-            let entry = format!("{:?};{};{}\n", result.level, test_id, result.details);
+            let details = result
+                .details
+                .replace('\n', "")
+                .replace(' ', "")
+                .replace('\t', "");
+            let entry = format!("{:?};{};{}\n", result.level, test_id, details);
             if let Some(path) = &self.cache {
                 std::fs::OpenOptions::new()
                     .read(true)

testool/src/statetest/suite.rs

@@ -58,11 +58,18 @@ pub fn run_statetests_suite(
     results: &mut Results,
 ) -> Result<()> {
     // Filter already cached entries
+    let all_test_count = tcs.len();
     let tcs: Vec<StateTest> = tcs
         .into_iter()
-        .filter(|t| !results.contains(&t.id))
+        .filter(|t| !results.contains(&format!("{}#{}", t.id, t.path)))
         .collect();
 
+    log::info!(
+        "{} test results cached, {} remaining",
+        all_test_count - tcs.len(),
+        tcs.len()
+    );
+
     let results = Arc::new(RwLock::from(results));
 
     // for each test