Rm deadcode of EVM circuit (#1525)

### Description

- Removed #[allow(dead_code)]  and `#![allow(unused_imports)]`.
- Removed most of the dead code. Kept a few with the "_" initial name.
- Reorganize dev modules and feature flags for "zkevm-circuits" crate. 
- ~~"test" flag is only used when test utils are shared within
"zkevm-circuits"~~ "test" flag is gone.
   - "test-util" flag is used when utils are shared with testool crate.
- "test-circuits" flag is reserved for test circuits that are shared
with integration tests.


### Issue Link


### Type of change

- [x] New feature (non-breaking change which adds functionality)


### Rationale

These are the reason I keep a method:

- The method is a sensible API and is tested. Although it is unused but
it makes sense to keep it. The `remainder` method of the Constant
Division Gadget is such an example.
- Variants in Enums. We use the `match` macro for them so it is
important we don't miss any variant. It makes sense to keep them even
when we don't use them.
- Work in progress and recent works. Cell manager and _error_depth are
examples.

I remove a method or a gadget for these reasons.

- If it has been there for a long time but never used.
- Default removing them to question us hard if the gadget really sparks
joy. batched_is_zero and binary number gadgets are such cases.

### How Has This Been Tested?

```
cargo build --all-features
cargo test // just for build, without running the full tests
```

Author: ChihChengLiang

.github/workflows/test-features.yml

@@ -35,8 +35,8 @@ jobs:
           - zkevm-circuits
         feature:
           - default
-          - test
           - test-circuits
+          - test-util
           - warn-unimplemented
     steps:
       - uses: actions/checkout@v2

circuit-benchmarks/Cargo.toml

@@ -9,7 +9,7 @@ license = "MIT OR Apache-2.0"
 [dependencies]
 halo2_proofs = { git = "https://github.com/privacy-scaling-explorations/halo2.git", tag = "v2023_04_20" }
 ark-std = { version = "0.3", features = ["print-trace"] }
-zkevm-circuits = { path = "../zkevm-circuits", features = ["test"]}
+zkevm-circuits = { path = "../zkevm-circuits", features = ["test-circuits"] }
 bus-mapping = { path = "../bus-mapping",  features = ["test"] }
 rand_xorshift = "0.3"
 rand = "0.8"

integration-tests/Cargo.toml

@@ -13,7 +13,7 @@ serde_json = "1.0.66"
 serde = { version = "1.0.130", features = ["derive"] }
 bus-mapping = { path = "../bus-mapping" , features = ["test"] }
 eth-types = { path = "../eth-types" }
-zkevm-circuits = { path = "../zkevm-circuits", features = ["test"] }
+zkevm-circuits = { path = "../zkevm-circuits", features = ["test-circuits"] }
 tokio = { version = "1.13", features = ["macros", "rt-multi-thread"] }
 url = "2.2.2"
 pretty_assertions = "1.0.0"

testool/Cargo.toml

@@ -29,7 +29,7 @@ strum_macros = "0.24"
 thiserror = "1.0"
 toml = "0.5"
 yaml-rust = "0.4.5"
-zkevm-circuits = { path="../zkevm-circuits", features=["test"] }
+zkevm-circuits = { path="../zkevm-circuits", features=["test-util"] }
 rand_chacha = "0.3"
 rand = "0.8"
 halo2_proofs = { git = "https://github.com/privacy-scaling-explorations/halo2.git", tag = "v2023_04_20" }

zkevm-circuits/Cargo.toml

@@ -49,8 +49,10 @@ serde_json = "1.0.78"
 
 [features]
 default = []
-test = ["ethers-signers", "mock", "bus-mapping/test"]
+# We export some test circuits for other crates to consume
 test-circuits = []
+# Test utilities for testool crate to consume
+test-util = ["dep:mock"]
 warn-unimplemented = ["eth-types/warn-unimplemented"]
 stats = ["warn-unimplemented", "dep:cli-table"]
 

zkevm-circuits/src/bytecode_circuit.rs

@@ -4,12 +4,11 @@
 pub mod bytecode_unroller;
 /// Bytecode circuit
 pub mod circuit;
-pub(crate) mod param;
 
-#[cfg(any(feature = "test", test, feature = "test-circuits"))]
+#[cfg(any(test, feature = "test-circuits"))]
 mod dev;
 /// Bytecode circuit tester
-#[cfg(any(feature = "test", test))]
+#[cfg(test)]
 mod test;
-#[cfg(any(feature = "test", test, feature = "test-circuits"))]
+#[cfg(feature = "test-circuits")]
 pub use dev::BytecodeCircuit as TestBytecodeCircuit;

zkevm-circuits/src/bytecode_circuit/circuit.rs

@@ -25,10 +25,9 @@ use halo2_proofs::{
 use log::trace;
 use std::vec;
 
-use super::{
-    bytecode_unroller::{unroll, UnrolledBytecode},
-    param::PUSH_TABLE_WIDTH,
-};
+use super::bytecode_unroller::{unroll, UnrolledBytecode};
+
+const PUSH_TABLE_WIDTH: usize = 2;
 
 #[derive(Debug, Clone, Default)]
 /// Row for assignment

zkevm-circuits/src/bytecode_circuit/param.rs

@@ -1,8 +1,7 @@
-#![allow(unused_imports)]
 use crate::{
     bytecode_circuit::{bytecode_unroller::*, circuit::BytecodeCircuit},
     table::BytecodeFieldTag,
-    util::{is_push, keccak, unusable_rows, Challenges, SubCircuit},
+    util::{is_push, keccak, unusable_rows, SubCircuit},
 };
 use bus_mapping::evm::OpcodeId;
 use eth_types::{Bytecode, Field, Word};

zkevm-circuits/src/bytecode_circuit/test.rs

@@ -3,11 +3,11 @@
 //! etc.
 pub(crate) mod util;
 
-#[cfg(any(feature = "test", test, feature = "test-circuits"))]
+#[cfg(any(test, feature = "test-circuits"))]
 mod dev;
 #[cfg(test)]
 mod test;
-#[cfg(any(feature = "test", test, feature = "test-circuits"))]
+#[cfg(feature = "test-circuits")]
 pub use dev::CopyCircuit as TestCopyCircuit;
 
 use bus_mapping::{

zkevm-circuits/src/copy_circuit.rs

@@ -12,10 +12,10 @@ pub mod step;
 pub mod table;
 pub(crate) mod util;
 
-#[cfg(any(feature = "test", test))]
+#[cfg(test)]
 pub(crate) mod test;
 use self::step::HasExecutionState;
-#[cfg(any(feature = "test", test, feature = "test-circuits"))]
+#[cfg(feature = "test-circuits")]
 pub use self::EvmCircuit as TestEvmCircuit;
 
 pub use crate::witness;
@@ -184,7 +184,8 @@ impl<F: Field> EvmCircuit<F> {
         }
     }
 
-    pub fn new_dev(block: Block<F>, fixed_table_tags: Vec<FixedTableTag>) -> Self {
+    pub fn get_test_cicuit_from_block(block: Block<F>) -> Self {
+        let fixed_table_tags = detect_fixed_table_tags(&block);
         Self {
             block: Some(block),
             fixed_table_tags,
@@ -290,7 +291,7 @@ pub(crate) fn detect_fixed_table_tags<F: Field>(block: &Block<F>) -> Vec<FixedTa
         .collect()
 }
 
-#[cfg(any(feature = "test", test))]
+#[cfg(any(feature = "test-util", test))]
 pub(crate) mod cached {
     use super::*;
     use halo2_proofs::halo2curves::bn256::Fr;

zkevm-circuits/src/evm_circuit.rs

@@ -311,7 +311,9 @@ pub struct ExecutionConfig<F> {
     error_oog_code_store: Box<DummyGadget<F, 0, 0, { ExecutionState::ErrorOutOfGasCodeStore }>>,
     error_invalid_jump: Box<ErrorInvalidJumpGadget<F>>,
     error_invalid_opcode: Box<ErrorInvalidOpcodeGadget<F>>,
+    #[allow(dead_code, reason = "under active development")]
     error_depth: Box<DummyGadget<F, 0, 0, { ExecutionState::ErrorDepth }>>,
+    #[allow(dead_code, reason = "under active development")]
     error_contract_address_collision:
         Box<DummyGadget<F, 0, 0, { ExecutionState::ErrorContractAddressCollision }>>,
     error_invalid_creation_code: Box<ErrorInvalidCreationCodeGadget<F>>,

zkevm-circuits/src/evm_circuit/execution.rs

@@ -47,7 +47,6 @@ pub(crate) struct BeginTxGadget<F> {
     tx_callee_address: WordCell<F>,
     call_callee_address: AccountAddress<F>,
     tx_is_create: Cell<F>,
-    tx_value: Word32Cell<F>,
     tx_call_data_length: Cell<F>,
     tx_call_data_gas_cost: Cell<F>,
     tx_call_data_word_length: ConstantDivisionGadget<F, N_BYTES_U64>,
@@ -480,7 +479,6 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
             tx_callee_address,
             call_callee_address,
             tx_is_create,
-            tx_value,
             tx_call_data_length,
             tx_call_data_gas_cost,
             tx_call_data_word_length,

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

@@ -689,7 +689,6 @@ mod test {
     use lazy_static::lazy_static;
     use mock::{eth, TestContext};
 
-    const CALLEE_ADDRESS: Address = Address::repeat_byte(0xff);
     lazy_static! {
         static ref CALLER_ADDRESS: Address = address!("0x00bbccddee000000000000000000000000002400");
     }

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

@@ -27,8 +27,6 @@ pub(crate) struct EndBlockGadget<F> {
     max_txs: Cell<F>,
 }
 
-const EMPTY_BLOCK_N_RWS: u64 = 0;
-
 impl<F: Field> ExecutionGadget<F> for EndBlockGadget<F> {
     const NAME: &'static str = "EndBlock";
 

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

@@ -243,16 +243,6 @@ mod test {
         .run();
     }
 
-    // internal call test
-    struct Stack {
-        gas: u64,
-        value: Word,
-        cd_offset: u64,
-        cd_length: u64,
-        rd_offset: u64,
-        rd_length: u64,
-    }
-
     fn callee(code: Bytecode) -> Account {
         let code = code.to_vec();
         let is_empty = code.is_empty();
@@ -265,53 +255,6 @@ mod test {
         }
     }
 
-    fn caller(opcode: OpcodeId, stack: Stack, caller_is_success: bool) -> Account {
-        let is_call = opcode == OpcodeId::CALL;
-        let terminator = if caller_is_success {
-            OpcodeId::RETURN
-        } else {
-            OpcodeId::REVERT
-        };
-
-        // Call twice for testing both cold and warm access
-        let mut bytecode = bytecode! {
-            PUSH32(Word::from(stack.rd_length))
-            PUSH32(Word::from(stack.rd_offset))
-            PUSH32(Word::from(stack.cd_length))
-            PUSH32(Word::from(stack.cd_offset))
-        };
-        if is_call {
-            bytecode.push(32, stack.value);
-        }
-        bytecode.append(&bytecode! {
-            PUSH32(Address::repeat_byte(0xff).to_word())
-            PUSH32(Word::from(stack.gas))
-            .write_op(opcode)
-            PUSH32(Word::from(stack.rd_length))
-            PUSH32(Word::from(stack.rd_offset))
-            PUSH32(Word::from(stack.cd_length))
-            PUSH32(Word::from(stack.cd_offset))
-        });
-        if is_call {
-            bytecode.push(32, stack.value);
-        }
-        bytecode.append(&bytecode! {
-            PUSH32(Address::repeat_byte(0xff).to_word())
-            PUSH32(Word::from(stack.gas))
-            .write_op(opcode)
-            PUSH1(0)
-            PUSH1(0)
-            .write_op(terminator)
-        });
-
-        Account {
-            address: Address::repeat_byte(0xfe),
-            balance: Word::from(10).pow(20.into()),
-            code: bytecode.to_vec().into(),
-            ..Default::default()
-        }
-    }
-
     // jump or jumpi error happen in internal call
     fn test_internal_jump_error(is_jumpi: bool) {
         let mut caller_bytecode = bytecode! {

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

@@ -367,10 +367,7 @@ mod test {
     #[derive(Default)]
     struct TestingData {
         key: U256,
-        value: U256,
-        value_prev: U256,
         original_value: U256,
-        is_warm: bool,
         gas_cost: u64,
         bytecode: Bytecode,
     }
@@ -443,10 +440,7 @@ mod test {
 
             Self {
                 key,
-                value,
-                value_prev,
                 original_value,
-                is_warm,
                 gas_cost,
                 bytecode,
             }

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

@@ -17,7 +17,7 @@ use crate::{
     },
 };
 use eth_types::{evm_types::OpcodeId, Field, ToLittleEndian};
-use halo2_proofs::plonk::Error;
+use halo2_proofs::{circuit::Value, plonk::Error};
 
 #[derive(Clone, Debug)]
 pub(crate) struct ErrorOOGStaticMemoryGadget<F> {
@@ -99,6 +99,8 @@ impl<F: Field> ExecutionGadget<F> for ErrorOOGStaticMemoryGadget<F> {
         step: &ExecStep,
     ) -> Result<(), Error> {
         let opcode = step.opcode().unwrap();
+        self.opcode
+            .assign(region, offset, Value::known(F::from(opcode.as_u64())))?;
 
         // Inputs/Outputs
         let address = block.get_rws(step, 0).stack_value();

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

@@ -144,21 +144,10 @@ impl<F: Field> ExecutionGadget<F> for ErrorWriteProtectionGadget<F> {
 mod test {
     use crate::test_util::CircuitTestBuilder;
     use eth_types::{
-        address, bytecode, bytecode::Bytecode, evm_types::OpcodeId, geth_types::Account, Address,
-        ToWord, Word, U64,
+        address, bytecode, bytecode::Bytecode, geth_types::Account, Address, ToWord, Word, U64,
     };
     use mock::TestContext;
 
-    // internal call test
-    struct Stack {
-        gas: u64,
-        value: Word,
-        cd_offset: u64,
-        cd_length: u64,
-        rd_offset: u64,
-        rd_length: u64,
-    }
-
     fn callee(code: Bytecode) -> Account {
         let code = code.to_vec();
         let is_empty = code.is_empty();
@@ -172,52 +161,6 @@ mod test {
         }
     }
 
-    fn caller(opcode: OpcodeId, stack: Stack, caller_is_success: bool) -> Account {
-        let is_call = opcode == OpcodeId::CALL;
-        let terminator = if caller_is_success {
-            OpcodeId::RETURN
-        } else {
-            OpcodeId::REVERT
-        };
-
-        let mut bytecode = bytecode! {
-            PUSH32(Word::from(stack.rd_length))
-            PUSH32(Word::from(stack.rd_offset))
-            PUSH32(Word::from(stack.cd_length))
-            PUSH32(Word::from(stack.cd_offset))
-        };
-        if is_call {
-            bytecode.push(32, stack.value);
-        }
-        bytecode.append(&bytecode! {
-            PUSH32(Address::repeat_byte(0xff).to_word())
-            PUSH32(Word::from(stack.gas))
-            .write_op(opcode)
-            PUSH32(Word::from(stack.rd_length))
-            PUSH32(Word::from(stack.rd_offset))
-            PUSH32(Word::from(stack.cd_length))
-            PUSH32(Word::from(stack.cd_offset))
-        });
-        if is_call {
-            bytecode.push(32, stack.value);
-        }
-        bytecode.append(&bytecode! {
-            PUSH32(Address::repeat_byte(0xff).to_word())
-            PUSH32(Word::from(stack.gas))
-            .write_op(opcode)
-            PUSH1(0)
-            PUSH1(0)
-            .write_op(terminator)
-        });
-
-        Account {
-            address: Address::repeat_byte(0xfe),
-            balance: Word::from(10).pow(20.into()),
-            code: bytecode.to_vec().into(),
-            ..Default::default()
-        }
-    }
-
     #[test]
     fn test_write_protection() {
         // test sstore with write protection error

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

@@ -137,6 +137,11 @@ mod test {
         test_ok(rand_range(34..1 << 11));
     }
 
+    #[test]
+    fn invalid_jump_err() {
+        test_invalid_jump(34);
+    }
+
     #[test]
     #[ignore]
     fn jump_gadget_rand_huge_bytecode() {

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

@@ -423,12 +423,4 @@ impl<F: Field> Lookup<F> {
                 .collect(),
         }
     }
-
-    pub(crate) fn degree(&self) -> usize {
-        self.input_exprs()
-            .iter()
-            .map(|expr| expr.degree())
-            .max()
-            .unwrap()
-    }
 }