Remove deprecated, unreachable_code and add docs to Evm circuit (priv…

…acy-scaling-explorations#1524)

### Description

- rm deprecated, unreachable_code
- doc some parts.

### Issue Link

part of  privacy-scaling-explorations#1494

### Type of change

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

zkevm-circuits/src/evm_circuit.rs

@@ -1,6 +1,5 @@
 //! The EVM circuit implementation.
 
-#![allow(missing_docs)]
 use halo2_proofs::{
     circuit::{Layouter, SimpleFloorPlanner, Value},
     plonk::*,
@@ -41,6 +40,7 @@ pub struct EvmCircuitConfig<F> {
     fixed_table: [Column<Fixed>; 4],
     u8_table: UXTable<8>,
     u16_table: UXTable<16>,
+    /// The execution config
     pub execution: Box<ExecutionConfig<F>>,
     // External tables
     tx_table: TxTable,
@@ -183,26 +183,29 @@ impl<F: Field> EvmCircuit<F> {
             fixed_table_tags: FixedTableTag::iter().collect(),
         }
     }
-
-    pub fn get_test_cicuit_from_block(block: Block<F>) -> Self {
+    #[cfg(feature = "test-circuits")]
+    /// Construct the EvmCircuit with only subset of Fixed table tags required by tests to save
+    /// testing time
+    pub(crate) fn get_test_circuit_from_block(block: Block<F>) -> Self {
         let fixed_table_tags = detect_fixed_table_tags(&block);
         Self {
             block: Some(block),
             fixed_table_tags,
         }
     }
-
+    #[cfg(feature = "test-circuits")]
     /// Calculate which rows are "actually" used in the circuit
-    pub fn get_active_rows(block: &Block<F>) -> (Vec<usize>, Vec<usize>) {
+    pub(crate) fn get_active_rows(block: &Block<F>) -> (Vec<usize>, Vec<usize>) {
         let max_offset = Self::get_num_rows_required(block);
         // some gates are enabled on all rows
         let gates_row_ids = (0..max_offset).collect();
         // lookups are enabled at "q_step" rows and byte lookup rows
         let lookup_row_ids = (0..max_offset).collect();
         (gates_row_ids, lookup_row_ids)
     }
-
-    pub fn get_num_rows_required(block: &Block<F>) -> usize {
+    /// Get the minimum number of rows required to process the block
+    /// If unspecified, then compute it
+    pub(crate) fn get_num_rows_required(block: &Block<F>) -> usize {
         let evm_rows = block.circuits_params.max_evm_rows;
         if evm_rows == 0 {
             Self::get_min_num_rows_required(block)
@@ -211,8 +214,8 @@ impl<F: Field> EvmCircuit<F> {
             block.circuits_params.max_evm_rows + 1
         }
     }
-
-    pub fn get_min_num_rows_required(block: &Block<F>) -> usize {
+    /// Compute the minimum number of rows required to process the block
+    fn get_min_num_rows_required(block: &Block<F>) -> usize {
         let mut num_rows = 0;
         for transaction in &block.txs {
             for step in transaction.steps() {
@@ -343,8 +346,8 @@ pub(crate) mod cached {
     }
 
     impl EvmCircuitCached {
-        pub fn get_test_cicuit_from_block(block: Block<Fr>) -> Self {
-            Self(EvmCircuit::<Fr>::get_test_cicuit_from_block(block))
+        pub(crate) fn get_test_circuit_from_block(block: Block<Fr>) -> Self {
+            Self(EvmCircuit::<Fr>::get_test_circuit_from_block(block))
         }
     }
 }
@@ -495,7 +498,7 @@ mod evm_circuit_stats {
         let block = block_convert::<Fr>(&builder).unwrap();
         let k = block.get_test_degree();
 
-        let circuit = EvmCircuit::<Fr>::get_test_cicuit_from_block(block);
+        let circuit = EvmCircuit::<Fr>::get_test_circuit_from_block(block);
         let prover1 = MockProver::<Fr>::run(k, &circuit, vec![]).unwrap();
 
         let code = bytecode! {
@@ -516,7 +519,7 @@ mod evm_circuit_stats {
             .unwrap();
         let block = block_convert::<Fr>(&builder).unwrap();
         let k = block.get_test_degree();
-        let circuit = EvmCircuit::<Fr>::get_test_cicuit_from_block(block);
+        let circuit = EvmCircuit::<Fr>::get_test_circuit_from_block(block);
         let prover2 = MockProver::<Fr>::run(k, &circuit, vec![]).unwrap();
 
         assert_eq!(prover1.fixed(), prover2.fixed());

zkevm-circuits/src/evm_circuit/param.rs

@@ -1,3 +1,4 @@
+//! Constants and parameters for the EVM circuit
 use super::table::Table;
 use crate::evm_circuit::{step::ExecutionState, EvmCircuit};
 use halo2_proofs::{
@@ -22,12 +23,14 @@ pub const N_PHASE2_COLUMNS: usize = 1;
 pub const N_PHASE1_COLUMNS: usize =
     STEP_WIDTH - EVM_LOOKUP_COLS - N_PHASE2_COLUMNS - N_COPY_COLUMNS - N_U8_LOOKUPS - N_U16_LOOKUPS;
 
-// Number of copy columns
+/// Number of copy columns
 pub const N_COPY_COLUMNS: usize = 2;
 
+/// Number of columns reserved for u8 lookup
 pub const N_U8_LOOKUPS: usize = 24;
 
 // TODO shift #column/2 from u8 to u16
+/// Number of columns reserved for u16 lookup
 pub const N_U16_LOOKUPS: usize = 0;
 
 /// Amount of lookup columns in the EVM circuit dedicated to lookups.

zkevm-circuits/src/evm_circuit/step.rs

@@ -1,3 +1,8 @@
+//! EVM execution state. We model the EVM execution as a finite state machine. The execution of a
+//! EVM block goes from one state to another, but never at an undefined state. The EVM circuit
+//! enables the selectors and thus activates the constraints for the state that the execution
+//! reaches.
+
 use super::{
     param::MAX_STEP_HEIGHT,
     util::{evm_cm_distribute_advice, CachedRegion, Cell, CellType},
@@ -27,27 +32,37 @@ use std::{fmt::Display, iter};
 use strum::IntoEnumIterator;
 use strum_macros::EnumIter;
 
+#[allow(missing_docs, reason = "some docs here are tedious and not helpful")]
 #[allow(non_camel_case_types)]
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, EnumIter)]
+/// All the possible execution states that the computation of EVM can arrive.
+/// Some states are shared by multiple opcodes.
 pub enum ExecutionState {
     // Internal state
     BeginTx,
     EndTx,
     EndBlock,
     // Opcode successful cases
     STOP,
-    ADD_SUB,     // ADD, SUB
-    MUL_DIV_MOD, // MUL, DIV, MOD
-    SDIV_SMOD,   // SDIV, SMOD
-    SHL_SHR,     // SHL, SHR
+    /// ADD and SUB opcodes share this state
+    ADD_SUB,
+    /// MUL, DIV, MOD
+    MUL_DIV_MOD,
+    /// SDIV, SMOD
+    SDIV_SMOD,
+    /// SHL, SHR
+    SHL_SHR,
     ADDMOD,
     MULMOD,
     EXP,
     SIGNEXTEND,
-    CMP,  // LT, GT, EQ
-    SCMP, // SLT, SGT
+    /// LT, GT, EQ
+    CMP,
+    /// SLT, SGT
+    SCMP,
     ISZERO,
-    BITWISE, // AND, OR, XOR
+    /// AND, OR, XOR
+    BITWISE,
     NOT,
     BYTE,
     SAR,
@@ -69,11 +84,13 @@ pub enum ExecutionState {
     RETURNDATACOPY,
     EXTCODEHASH,
     BLOCKHASH,
-    BLOCKCTX, // TIMESTAMP, NUMBER, GASLIMIT, COINBASE, DIFFICULTY, BASEFEE
+    /// TIMESTAMP, NUMBER, GASLIMIT, COINBASE, DIFFICULTY, BASEFEE
+    BLOCKCTX,
     CHAINID,
     SELFBALANCE,
     POP,
-    MEMORY, // MLOAD, MSTORE, MSTORE8
+    /// MLOAD, MSTORE, MSTORE8
+    MEMORY,
     SLOAD,
     SSTORE,
     JUMP,
@@ -82,13 +99,18 @@ pub enum ExecutionState {
     MSIZE,
     GAS,
     JUMPDEST,
-    PUSH, // PUSH1, PUSH2, ..., PUSH32
-    DUP,  // DUP1, DUP2, ..., DUP16
-    SWAP, // SWAP1, SWAP2, ..., SWAP16
-    LOG,  // LOG0, LOG1, ..., LOG4
+    /// PUSH1, PUSH2, ..., PUSH32
+    PUSH,
+    /// DUP1, DUP2, ..., DUP16
+    DUP,
+    /// SWAP1, SWAP2, ..., SWAP16
+    SWAP,
+    /// LOG0, LOG1, ..., LOG4
+    LOG,
     CREATE,
-    CALL_OP,       // CALL, CALLCODE, DELEGATECALL, STATICCALL
-    RETURN_REVERT, // RETURN, REVERT
+    /// CALL, CALLCODE, DELEGATECALL, STATICCALL
+    CALL_OP,
+    RETURN_REVERT,
     CREATE2,
     SELFDESTRUCT,
     // Error cases
@@ -283,7 +305,7 @@ impl From<&ExecStep> for ExecutionState {
     }
 }
 
-pub trait HasExecutionState {
+pub(crate) trait HasExecutionState {
     fn execution_state(&self) -> ExecutionState;
 }
 
@@ -334,6 +356,7 @@ impl ExecutionState {
             || self.halts_in_exception()
     }
 
+    /// Get the opocdes that are related to the execution state
     pub fn responsible_opcodes(&self) -> Vec<ResponsibleOp> {
         if matches!(self, Self::ErrorStack) {
             return OpcodeId::valid_opcodes()
@@ -498,11 +521,12 @@ impl ExecutionState {
         .collect()
     }
 
+    /// Get the state hight
     pub fn get_step_height_option(&self) -> Option<usize> {
         EXECUTION_STATE_HEIGHT_MAP.get(self).copied()
     }
 
-    pub fn get_step_height(&self) -> usize {
+    pub(crate) fn get_step_height(&self) -> usize {
         self.get_step_height_option()
             .unwrap_or_else(|| panic!("Execution state unknown: {:?}", self))
     }
@@ -525,6 +549,7 @@ impl From<OpcodeId> for ResponsibleOp {
 }
 
 impl ResponsibleOp {
+    /// Get the opcode
     pub fn opcode(&self) -> OpcodeId {
         *match self {
             ResponsibleOp::Op(opcode) => opcode,

zkevm-circuits/src/evm_circuit/table.rs

@@ -1,3 +1,5 @@
+//! Fixed lookup tables and dynamic lookup tables for the EVM circuit
+
 use crate::{
     evm_circuit::step::{ExecutionState, ResponsibleOp},
     impl_expr,
@@ -11,28 +13,46 @@ use strum::IntoEnumIterator;
 use strum_macros::EnumIter;
 
 #[derive(Clone, Copy, Debug, EnumIter)]
+/// Tags for different fixed tables
 pub enum FixedTableTag {
+    /// x == 0
     Zero = 0,
+    /// 0 <= x < 5
     Range5,
+    /// 0 <= x < 16
     Range16,
+    /// 0 <= x < 32
     Range32,
+    /// 0 <= x < 64
     Range64,
+    /// 0 <= x < 128
     Range128,
+    /// 0 <= x < 256
     Range256,
+    /// 0 <= x < 512
     Range512,
+    /// 0 <= x < 1024
     Range1024,
+    /// -128 <= x < 128
     SignByte,
+    /// bitwise AND
     BitwiseAnd,
+    /// bitwise OR
     BitwiseOr,
+    /// bitwise XOR
     BitwiseXor,
+    /// lookup for corresponding opcode
     ResponsibleOpcode,
+    /// power of 2
     Pow2,
+    /// Lookup constant gas cost for opcodes
     ConstantGasCost,
 }
 impl_expr!(FixedTableTag);
 
 impl FixedTableTag {
-    pub fn build<F: Field>(&self) -> Box<dyn Iterator<Item = [F; 4]>> {
+    /// build up the fixed table row values
+    pub(crate) fn build<F: Field>(&self) -> Box<dyn Iterator<Item = [F; 4]>> {
         let tag = F::from(*self as u64);
         match self {
             Self::Zero => Box::new((0..1).map(move |_| [tag, F::ZERO, F::ZERO, F::ZERO])),
@@ -122,33 +142,57 @@ impl FixedTableTag {
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, EnumIter)]
+/// Each item represents the lookup table to query
 pub enum Table {
+    /// The range check table for u8
     U8,
+    /// The range check table for u16
     U16,
+    /// The rest of the fixed table. See [`FixedTableTag`]
     Fixed,
+    /// Lookup for transactions
     Tx,
+    /// Lookup for read write operations
     Rw,
+    /// Lookup for bytecode table
     Bytecode,
+    /// Lookup for block constants
     Block,
+    /// Lookup for copy table
     Copy,
+    /// Lookup for keccak table
     Keccak,
+    /// Lookup for exp table
     Exp,
 }
 
 #[derive(Clone, Debug)]
-pub struct RwValues<F> {
-    pub id: Expression<F>,
-    pub address: Expression<F>,
-    pub field_tag: Expression<F>,
-    pub storage_key: Word<Expression<F>>,
-    pub value: Word<Expression<F>>,
-    pub value_prev: Word<Expression<F>>,
-    pub init_val: Word<Expression<F>>,
+/// Read-Write Table fields
+pub(crate) struct RwValues<F> {
+    /// The unique identifier for the Read or Write. Depending on context, this field could be used
+    /// for Transaction ID or call ID
+    id: Expression<F>,
+    /// The position to Stack, Memory, or account, where the read or write takes place, depending
+    /// on the cell value of the [`bus_mapping::operation::Target`].
+    address: Expression<F>,
+    /// Could be [`crate::table::CallContextFieldTag`], [`crate::table::AccountFieldTag`],
+    /// [`crate::table::TxLogFieldTag`], or [`crate::table::TxReceiptFieldTag`] depending on
+    /// the cell value of the [`bus_mapping::operation::Target`]
+    field_tag: Expression<F>,
+    /// Storage key of two limbs
+    storage_key: Word<Expression<F>>,
+    /// The current storage value
+    value: Word<Expression<F>>,
+    /// The previous storage value
+    value_prev: Word<Expression<F>>,
+    /// The initial storage value before the current transaction
+    init_val: Word<Expression<F>>,
 }
 
 impl<F: Field> RwValues<F> {
+    /// Constructor for RwValues
     #[allow(clippy::too_many_arguments)]
-    pub fn new(
+    pub(crate) fn new(
         id: Expression<F>,
         address: Expression<F>,
         field_tag: Expression<F>,
@@ -167,6 +211,15 @@ impl<F: Field> RwValues<F> {
             init_val,
         }
     }
+
+    pub(crate) fn revert_value(&self) -> Self {
+        let new_self = self.clone();
+        Self {
+            value_prev: new_self.value,
+            value: new_self.value_prev,
+            ..new_self
+        }
+    }
 }
 
 #[derive(Clone, Debug)]