Simlify ErrorStack circuit with ResponsibleOpcode lookup (privacy…

…-scaling-explorations#1159) Close privacy-scaling-explorations#1154 ### Summary 1. Update `valid_stack_ptr_range` function to `invalid_stack_ptrs`. It returns the invalid stack pointers of specified opcode. 2. Add Enum `ResponsibleOp` and update `ExecutionState::responsible_opcodes` to return a vector of ResponsibleOp. 3. Update `FixedTableTag.ResponsibleOpcode` build for invalid stack pointers, and delete `FixedTableTag.OpcodeStack`. 4. Update `ErrorStack` circuit with `ResponsibleOpcode` lookup.
ed255 · Feb 22, 2023 · 95986bf · 95986bf
1 parent 599628e
commit 95986bf
Show file tree

Hide file tree

Showing 6 changed files with 90 additions and 106 deletions.
diff --git a/eth-types/src/evm_types/opcode_ids.rs b/eth-types/src/evm_types/opcode_ids.rs
@@ -4,8 +4,8 @@ use core::fmt::Debug;
 use lazy_static::lazy_static;
 use regex::Regex;
 use serde::{de, Deserialize, Serialize};
-use std::fmt;
 use std::str::FromStr;
+use std::{fmt, matches};
 use strum_macros::EnumIter;
 
 /// Opcode enum. One-to-one corresponding to an `u8` value.
@@ -663,9 +663,9 @@ impl OpcodeId {
         }
     }
 
-    /// Returns the constant min & stack pointer of `OpcodeId`
-    pub const fn valid_stack_ptr_range(&self) -> (u32, u32) {
-        match self {
+    /// Returns invalid stack pointers of `OpcodeId`
+    pub fn invalid_stack_ptrs(&self) -> Vec<u32> {
+        let (min_stack_ptr, max_stack_ptr): (u32, u32) = match self {
             // `min_stack_pointer` 0 means stack overflow never happen, for example, `OpcodeId::ADD`
             // can only encounter underflow error, but never encounter overflow error.
             // `max_stack_pointer` means max stack poniter for op code normally run. for example,
@@ -816,7 +816,14 @@ impl OpcodeId {
             OpcodeId::REVERT => (0, 1022),
             OpcodeId::SELFDESTRUCT => (0, 1023),
             _ => (0, 0),
-        }
+        };
+
+        debug_assert!(max_stack_ptr <= 1024);
+
+        (0..min_stack_ptr)
+            // Range (1025..=1024) is valid and it should be converted to an empty vector.
+            .chain(max_stack_ptr.checked_add(1).unwrap()..=1024)
+            .collect()
     }
 
     /// Returns `true` if the `OpcodeId` has memory access
@@ -867,10 +874,20 @@ impl OpcodeId {
         }
     }
 
+    /// Returns the all valid opcodes.
+    pub fn valid_opcodes() -> Vec<Self> {
+        (u8::MIN..=u8::MAX).fold(vec![], |mut acc, val| {
+            if !matches!(val.into(), Self::INVALID(_)) {
+                acc.push(val.into());
+            }
+            acc
+        })
+    }
+
     /// Returns the all invalid opcodes.
     pub fn invalid_opcodes() -> Vec<Self> {
         (u8::MIN..=u8::MAX).fold(vec![], |mut acc, val| {
-            if let Self::INVALID(val) = val.into() {
+            if matches!(val.into(), Self::INVALID(_)) {
                 acc.push(Self::INVALID(val));
             }
             acc

diff --git a/zkevm-circuits/src/evm_circuit/execution/error_stack.rs b/zkevm-circuits/src/evm_circuit/execution/error_stack.rs
@@ -1,13 +1,13 @@
 use crate::evm_circuit::{
     execution::ExecutionGadget,
     step::ExecutionState,
+    table::{FixedTableTag, Lookup},
     util::{
         common_gadget::RestoreContextGadget,
         constraint_builder::{
             ConstraintBuilder, StepStateTransition,
             Transition::{Delta, Same},
         },
-        math_gadget::LtGadget,
         CachedRegion, Cell,
     },
     witness::{Block, Call, ExecStep, Transaction},
@@ -17,15 +17,9 @@ use crate::util::Expr;
 use eth_types::Field;
 use halo2_proofs::{circuit::Value, plonk::Error};
 
-const N_BYTES_STACK: usize = 2;
-
 #[derive(Clone, Debug)]
 pub(crate) struct ErrorStackGadget<F> {
     opcode: Cell<F>,
-    min_stack_pointer: Cell<F>,
-    max_stack_pointer: Cell<F>,
-    is_overflow: LtGadget<F, N_BYTES_STACK>,
-    is_underflow: LtGadget<F, N_BYTES_STACK>,
     rw_counter_end_of_reversion: Cell<F>,
     restore_context: RestoreContextGadget<F>,
 }
@@ -39,40 +33,22 @@ impl<F: Field> ExecutionGadget<F> for ErrorStackGadget<F> {
         let opcode = cb.query_cell();
         cb.opcode_lookup(opcode.expr(), 1.expr());
 
-        let min_stack_pointer = cb.query_cell();
-        let max_stack_pointer = cb.query_cell();
-        let rw_counter_end_of_reversion = cb.query_cell();
-
-        cb.opcode_stack_lookup(
-            opcode.expr(),
-            min_stack_pointer.expr(),
-            max_stack_pointer.expr(),
-        );
-        // Check whether current stack pointer is underflow or overflow
-
-        let is_overflow = LtGadget::construct(
-            cb,
-            cb.curr.state.stack_pointer.expr(),
-            min_stack_pointer.expr(),
-        );
-        let is_underflow = LtGadget::construct(
-            cb,
-            max_stack_pointer.expr(),
-            cb.curr.state.stack_pointer.expr(),
-        );
-        // is_overflow and is_underflow is bool ensure by LtGadget.
-
-        // constrain one of [is_underflow, is_overflow] must be true when stack error
-        // happens
-        cb.require_equal(
-            "is_underflow + is_overflow = 1",
-            is_underflow.expr() + is_overflow.expr(),
-            1.expr(),
+        cb.add_lookup(
+            "Responsible opcode lookup for invalid stack pointer",
+            Lookup::Fixed {
+                tag: FixedTableTag::ResponsibleOpcode.expr(),
+                values: [
+                    Self::EXECUTION_STATE.as_u64().expr(),
+                    opcode.expr(),
+                    cb.curr.state.stack_pointer.expr(),
+                ],
+            },
         );
 
         // current call must be failed.
         cb.call_context_lookup(false.expr(), None, CallContextFieldTag::IsSuccess, 0.expr());
 
+        let rw_counter_end_of_reversion = cb.query_cell();
         cb.call_context_lookup(
             false.expr(),
             None,
@@ -124,10 +100,6 @@ impl<F: Field> ExecutionGadget<F> for ErrorStackGadget<F> {
 
         Self {
             opcode,
-            min_stack_pointer,
-            max_stack_pointer,
-            is_overflow,
-            is_underflow,
             rw_counter_end_of_reversion,
             restore_context,
         }
@@ -143,29 +115,8 @@ impl<F: Field> ExecutionGadget<F> for ErrorStackGadget<F> {
         step: &ExecStep,
     ) -> Result<(), Error> {
         let opcode = step.opcode.unwrap();
-
-        let (min_stack, max_stack) = opcode.valid_stack_ptr_range();
-
         self.opcode
             .assign(region, offset, Value::known(F::from(opcode.as_u64())))?;
-        // Inputs/Outputs
-        self.min_stack_pointer
-            .assign(region, offset, Value::known(F::from(min_stack as u64)))?;
-        self.max_stack_pointer
-            .assign(region, offset, Value::known(F::from(max_stack as u64)))?;
-
-        self.is_overflow.assign(
-            region,
-            offset,
-            F::from(step.stack_pointer as u64),
-            F::from(min_stack as u64),
-        )?;
-        self.is_underflow.assign(
-            region,
-            offset,
-            F::from(max_stack as u64),
-            F::from(step.stack_pointer as u64),
-        )?;
 
         self.rw_counter_end_of_reversion.assign(
             region,

diff --git a/zkevm-circuits/src/evm_circuit/step.rs b/zkevm-circuits/src/evm_circuit/step.rs
@@ -161,7 +161,18 @@ impl ExecutionState {
             || self.halts_in_exception()
     }
 
-    pub(crate) fn responsible_opcodes(&self) -> Vec<OpcodeId> {
+    pub(crate) fn responsible_opcodes(&self) -> Vec<ResponsibleOp> {
+        if matches!(self, Self::ErrorStack) {
+            return OpcodeId::valid_opcodes()
+                .into_iter()
+                .flat_map(|op| {
+                    op.invalid_stack_ptrs()
+                        .into_iter()
+                        .map(move |stack_ptr| ResponsibleOp::InvalidStackPtr(op, stack_ptr))
+                })
+                .collect();
+        }
+
         match self {
             Self::STOP => vec![OpcodeId::STOP],
             Self::ADD_SUB => vec![OpcodeId::ADD, OpcodeId::SUB],
@@ -304,6 +315,9 @@ impl ExecutionState {
             Self::ErrorInvalidOpcode => OpcodeId::invalid_opcodes(),
             _ => vec![],
         }
+        .into_iter()
+        .map(Into::into)
+        .collect()
     }
 
     pub fn get_step_height_option(&self) -> Option<usize> {
@@ -316,6 +330,31 @@ impl ExecutionState {
     }
 }
 
+/// Enum of Responsible opcode mapping to execution state.
+#[derive(Debug)]
+pub(crate) enum ResponsibleOp {
+    /// Raw opcode
+    Op(OpcodeId),
+    /// Corresponding to ExecutionState::ErrorStack
+    InvalidStackPtr(OpcodeId, u32),
+}
+
+/// Helper for easy transform from a raw OpcodeId to ResponsibleOp.
+impl From<OpcodeId> for ResponsibleOp {
+    fn from(opcode: OpcodeId) -> Self {
+        Self::Op(opcode)
+    }
+}
+
+impl ResponsibleOp {
+    pub(crate) fn opcode(&self) -> OpcodeId {
+        *match self {
+            ResponsibleOp::Op(opcode) => opcode,
+            ResponsibleOp::InvalidStackPtr(opcode, _) => opcode,
+        }
+    }
+}
+
 /// Dynamic selector that generates expressions of degree 2 to select from N
 /// possible targets using N/2 + 1 cells.
 #[derive(Clone, Debug)]

diff --git a/zkevm-circuits/src/evm_circuit/table.rs b/zkevm-circuits/src/evm_circuit/table.rs
@@ -1,4 +1,4 @@
-use crate::evm_circuit::step::ExecutionState;
+use crate::evm_circuit::step::{ExecutionState, ResponsibleOp};
 use crate::impl_expr;
 pub use crate::table::TxContextFieldTag;
 use bus_mapping::evm::OpcodeId;
@@ -26,7 +26,6 @@ pub enum FixedTableTag {
     ResponsibleOpcode,
     Pow2,
     ConstantGasCost,
-    OpcodeStack,
 }
 impl_expr!(FixedTableTag);
 
@@ -78,17 +77,22 @@ impl FixedTableTag {
             })),
             Self::ResponsibleOpcode => {
                 Box::new(ExecutionState::iter().flat_map(move |execution_state| {
-                    execution_state
-                        .responsible_opcodes()
-                        .into_iter()
-                        .map(move |opcode| {
+                    execution_state.responsible_opcodes().into_iter().map(
+                        move |responsible_opcode| {
+                            let (op, aux) = match responsible_opcode {
+                                ResponsibleOp::Op(op) => (op, F::zero()),
+                                ResponsibleOp::InvalidStackPtr(op, stack_ptr) => {
+                                    (op, F::from(u64::from(stack_ptr)))
+                                }
+                            };
                             [
                                 tag,
                                 F::from(execution_state.as_u64()),
-                                F::from(opcode.as_u64()),
-                                F::zero(),
+                                F::from(op.as_u64()),
+                                aux,
                             ]
-                        })
+                        },
+                    )
                 }))
             }
             Self::Pow2 => Box::new((0..256).map(move |value| {
@@ -111,18 +115,6 @@ impl FixedTableTag {
                         ]
                     }),
             ),
-            Self::OpcodeStack => Box::new(
-                OpcodeId::iter()
-                    .filter(move |opcode| opcode.constant_gas_cost().0 > 0)
-                    .map(move |opcode| {
-                        [
-                            tag,
-                            F::from(opcode.as_u64()),
-                            F::from(opcode.valid_stack_ptr_range().0 as u64),
-                            F::from(opcode.valid_stack_ptr_range().1 as u64),
-                        ]
-                    }),
-            ),
         }
     }
 }

diff --git a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs
@@ -551,22 +551,6 @@ impl<'a, F: Field> ConstraintBuilder<'a, F> {
         );
     }
 
-    // look up opcode's min and max stack pointer
-    pub(crate) fn opcode_stack_lookup(
-        &mut self,
-        opcode: Expression<F>,
-        min_stack: Expression<F>,
-        max_stack: Expression<F>,
-    ) {
-        self.add_lookup(
-            "op code stack info",
-            Lookup::Fixed {
-                tag: FixedTableTag::OpcodeStack.expr(),
-                values: [opcode, min_stack, max_stack],
-            },
-        );
-    }
-
     // Opcode
 
     pub(crate) fn opcode_lookup(&mut self, opcode: Expression<F>, is_code: Expression<F>) {

diff --git a/zkevm-circuits/src/stats.rs b/zkevm-circuits/src/stats.rs
@@ -156,7 +156,8 @@ pub(crate) fn print_circuit_stats_by_states(
         if !fn_filter(state) {
             continue;
         }
-        for opcode in state.responsible_opcodes() {
+        for responsible_op in state.responsible_opcodes() {
+            let opcode = responsible_op.opcode();
             let mut code = bytecode! {
                 PUSH2(0x00)
                 EXTCODESIZE // Warm up 0x0 address