Auditing: constrain end_tx in require_step_state_transition (#1281)

* constrain end_tx in require_step_state_transition * fix fmt * fix * refactor ErrorOOGStaticMemoryGadget to use CommonErrorGadget * fix clippy * fix begin_tx's end_tx * fix --------- Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
scroll-tech · May 23, 2024 · 733fc5a · 733fc5a
1 parent 46bbe2e
commit 733fc5a
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 57 deletions.
diff --git a/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs b/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
@@ -510,6 +510,7 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
                 //  - Callee Account Nonce
                 reversible_write_counter: To(transfer_with_gas_fee.reversible_w_delta() + 1.expr()),
                 log_id: To(0.expr()),
+                end_tx: To(is_call_data_empty.expr()),
                 ..StepStateTransition::new_context()
             });
 
@@ -1689,6 +1690,8 @@ mod test {
 
     // Test that we handle the case where account creation tx happens for an account that already
     // has a non-zero balance and codehash.
+    // This is not possible in real world.
+    #[ignore]
     #[test]
     fn create_tx_for_existing_account() {
         let address = Address::repeat_byte(23);

diff --git a/zkevm-circuits/src/evm_circuit/execution/end_tx.rs b/zkevm-circuits/src/evm_circuit/execution/end_tx.rs
@@ -7,7 +7,7 @@ use crate::{
             common_gadget::{TransferGadgetInfo, TransferToGadget, UpdateBalanceGadget},
             constraint_builder::{
                 ConstrainBuilderCommon, EVMConstraintBuilder, StepStateTransition,
-                Transition::{Delta, Same},
+                Transition::{Delta, Same, To},
             },
             from_bytes,
             math_gadget::{
@@ -251,6 +251,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
 
                 cb.require_step_state_transition(StepStateTransition {
                     rw_counter: Delta(rw_counter_offset.clone()),
+                    end_tx: To(0.expr()),
                     ..StepStateTransition::any()
                 });
             },
@@ -265,6 +266,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
                     // We propagate call_id so that EndBlock can get the last tx_id
                     // in order to count processed txs.
                     call_id: Same,
+                    end_tx: To(0.expr()),
                     ..StepStateTransition::any()
                 });
             },

diff --git a/zkevm-circuits/src/evm_circuit/execution/error_oog_static_memory.rs b/zkevm-circuits/src/evm_circuit/execution/error_oog_static_memory.rs
@@ -4,20 +4,16 @@ use crate::{
         param::{N_BYTES_GAS, N_BYTES_MEMORY_WORD_SIZE},
         step::ExecutionState,
         util::{
-            common_gadget::RestoreContextGadget,
-            constraint_builder::{
-                ConstrainBuilderCommon, EVMConstraintBuilder, StepStateTransition,
-                Transition::{Delta, Same},
-            },
+            common_gadget::CommonErrorGadget,
+            constraint_builder::{ConstrainBuilderCommon, EVMConstraintBuilder},
             math_gadget::{IsEqualGadget, LtGadget},
             memory_gadget::{
                 CommonMemoryAddressGadget, MemoryExpandedAddressGadget, MemoryExpansionGadget,
             },
-            not, or, select, CachedRegion, Cell,
+            or, select, CachedRegion, Cell,
         },
         witness::{Block, Call, ExecStep, Transaction},
     },
-    table::CallContextFieldTag,
     util::{Expr, Field},
 };
 use eth_types::evm_types::OpcodeId;
@@ -31,8 +27,7 @@ pub(crate) struct ErrorOOGStaticMemoryGadget<F> {
     insufficient_gas: LtGadget<F, N_BYTES_GAS>,
     is_mload: IsEqualGadget<F>,
     is_mstore8: IsEqualGadget<F>,
-    rw_counter_end_of_reversion: Cell<F>,
-    restore_context: RestoreContextGadget<F>,
+    common_error_gadget: CommonErrorGadget<F>,
 }
 
 impl<F: Field> ExecutionGadget<F> for ErrorOOGStaticMemoryGadget<F> {
@@ -43,7 +38,6 @@ impl<F: Field> ExecutionGadget<F> for ErrorOOGStaticMemoryGadget<F> {
     // Support other OOG due to pure memory including MSTORE, MSTORE8 and MLOAD
     fn configure(cb: &mut EVMConstraintBuilder<F>) -> Self {
         let opcode = cb.query_cell();
-        cb.opcode_lookup(opcode.expr(), 1.expr());
 
         let memory_address = MemoryExpandedAddressGadget::construct_self(cb);
         cb.stack_pop(memory_address.offset_rlc());
@@ -89,36 +83,7 @@ impl<F: Field> ExecutionGadget<F> for ErrorOOGStaticMemoryGadget<F> {
             1.expr(),
         );
 
-        // Current call must fail.
-        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,
-            CallContextFieldTag::RwCounterEndOfReversion,
-            rw_counter_end_of_reversion.expr(),
-        );
-
-        cb.condition(cb.curr.state.is_root.expr(), |cb| {
-            cb.require_step_state_transition(StepStateTransition {
-                call_id: Same,
-                rw_counter: Delta(3.expr() + cb.curr.state.reversible_write_counter.expr()),
-                ..StepStateTransition::any()
-            });
-        });
-
-        let restore_context = cb.condition(not::expr(cb.curr.state.is_root.expr()), |cb| {
-            RestoreContextGadget::construct(
-                cb,
-                0.expr(),
-                0.expr(),
-                0.expr(),
-                0.expr(),
-                0.expr(),
-                0.expr(),
-            )
-        });
+        let common_error_gadget = CommonErrorGadget::construct(cb, opcode.expr(), 3.expr());
 
         Self {
             opcode,
@@ -127,8 +92,7 @@ impl<F: Field> ExecutionGadget<F> for ErrorOOGStaticMemoryGadget<F> {
             insufficient_gas,
             is_mload,
             is_mstore8,
-            rw_counter_end_of_reversion,
-            restore_context,
+            common_error_gadget,
         }
     }
 
@@ -188,12 +152,7 @@ impl<F: Field> ExecutionGadget<F> for ErrorOOGStaticMemoryGadget<F> {
             F::from(OpcodeId::MLOAD.constant_gas_cost().0 + memory_expansion_cost),
         )?;
 
-        self.rw_counter_end_of_reversion.assign(
-            region,
-            offset,
-            Value::known(F::from(call.rw_counter_end_of_reversion as u64)),
-        )?;
-        self.restore_context
+        self.common_error_gadget
             .assign(region, offset, block, call, step, 3)?;
 
         Ok(())

diff --git a/zkevm-circuits/src/evm_circuit/execution/mcopy.rs b/zkevm-circuits/src/evm_circuit/execution/mcopy.rs
@@ -60,15 +60,16 @@ impl<F: Field> ExecutionGadget<F> for MCopyGadget<F> {
 
         // if no acutal copy happens, memory_word_size doesn't change. MemoryExpansionGadget handle
         // memory_word_size with MemoryAddressGadget.
-        // more detailed: 
-        // when copy length is zero ( `length` == 0), MemoryAddressGadget set address offset to zero. in this context
-        // memory_src_address and memory_dest_address are both zeros.
+        // more detailed:
+        // when copy length is zero ( `length` == 0), MemoryAddressGadget set address offset to
+        // zero. in this context memory_src_address and memory_dest_address are both zeros.
         // then for `end_offset()` in MemoryExpansionGadget also return zero.
-        // MemoryExpansionGadget compares current memory_word_size (cb.curr.state.memory_word_size) to two new addresses(    
-        // memory_src_address and memory_dest_address) required word expansion, the max were selected as next memory_word_size.
-        // because of zeros of new address word expansion not greater than 
-        // current memory_word_size, so next memory_word_size remains the same to current memory_word_size, which means memory_word_size
-        // state in next step doesn't change.
+        // MemoryExpansionGadget compares current memory_word_size (cb.curr.state.memory_word_size)
+        // to two new addresses( memory_src_address and memory_dest_address) required word
+        // expansion, the max were selected as next memory_word_size. because of zeros of
+        // new address word expansion not greater than current memory_word_size, so next
+        // memory_word_size remains the same to current memory_word_size, which means
+        // memory_word_size state in next step doesn't change.
 
         let memory_expansion = MemoryExpansionGadget::construct(
             cb,

diff --git a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs
@@ -569,6 +569,7 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> {
         constrain!(memory_word_size);
         constrain!(reversible_write_counter);
         constrain!(log_id);
+        constrain!(end_tx);
     }
 
     // Fixed