Clean up TLOAD and TSTORE and add tests (#1245)

* cleanup todos and comments

* Add tests for reverted TSTORE and TSTORE in static call

* remove dead code

---------

Co-authored-by: Mason Liang <mason@scroll.io>

bus-mapping/src/evm/opcodes.rs

@@ -115,7 +115,6 @@ use extcodecopy::Extcodecopy;
 use extcodehash::Extcodehash;
 use extcodesize::Extcodesize;
 use gasprice::GasPrice;
-// use invalid_tx::InvalidTx;
 use logs::Log;
 use mload::Mload;
 use mstore::Mstore;

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

@@ -56,11 +56,6 @@ impl Opcode for Tstore {
             state.call()?.address.to_word(),
         )?;
 
-        // let key = geth_step.stack.nth_last(0)?;
-        // let key_stack_position = geth_step.stack.nth_last_filled(0);
-        // let value = geth_step.stack.nth_last(1)?;
-        // let value_stack_position = geth_step.stack.nth_last_filled(1);
-
         let key = state.stack_pop(&mut exec_step)?;
         let value = state.stack_pop(&mut exec_step)?;
         #[cfg(feature = "enable-stack")]
@@ -69,9 +64,6 @@ impl Opcode for Tstore {
             assert_eq!(value, geth_step.stack.nth_last(1)?);
         }
 
-        // state.stack_read(&mut exec_step, key_stack_position, key)?;
-        // state.stack_read(&mut exec_step, value_stack_position, value)?;
-
         let (_, value_prev) = state.sdb.get_transient_storage(&contract_addr, &key);
         let value_prev = *value_prev;
 
@@ -110,7 +102,7 @@ mod tstore_tests {
     #[test]
     fn tstore_opcode() {
         let code = bytecode! {
-            // Write 0x6f to storage slot 0
+            // Write 0x6f to transient storage slot 0
             PUSH1(0x6fu64)
             PUSH1(0x00u64)
             TSTORE

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

@@ -172,62 +172,26 @@ 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()
-        }
+    #[derive(Copy, Clone, Debug)]
+    enum FailureReason {
+        Sstore,
+        TStore,
+        CallWithValue,
     }
 
     #[test]
     fn test_write_protection() {
-        // test sstore with write protection error
-        test_internal_write_protection(false);
-        // test call with write protection error
-        test_internal_write_protection(true);
+        for reason in [
+            FailureReason::Sstore,
+            FailureReason::CallWithValue,
+            FailureReason::TStore,
+        ] {
+            test_internal_write_protection(reason)
+        }
     }
 
     // ErrorWriteProtection error happen in internal call
-    fn test_internal_write_protection(is_call: bool) {
+    fn test_internal_write_protection(reason: FailureReason) {
         let mut caller_bytecode = bytecode! {
             PUSH1(0)
             PUSH1(0)
@@ -248,26 +212,36 @@ mod test {
             PUSH1(0x02)
         };
 
-        if is_call {
-            callee_bytecode.append(&bytecode! {
-                PUSH1(0)
-                PUSH1(0)
-                PUSH1(10)
-                PUSH1(200)  // non zero value
-                PUSH20(Address::repeat_byte(0xff).to_word())
-                PUSH2(10000)  // gas
-                //this call got error: ErrorWriteProtection
-                CALL
-                RETURN
-                STOP
-            });
-        } else {
-            callee_bytecode.append(&bytecode! {
-                // this SSTORE got error: ErrorWriteProtection
-                SSTORE
-                STOP
-            });
-        }
+        match reason {
+            FailureReason::CallWithValue => {
+                callee_bytecode.append(&bytecode! {
+                    PUSH1(0)
+                    PUSH1(0)
+                    PUSH1(10)
+                    PUSH1(200)  // non zero value
+                    PUSH20(Address::repeat_byte(0xff).to_word())
+                    PUSH2(10000)  // gas
+                    //this call got error: ErrorWriteProtection
+                    CALL
+                    RETURN
+                    STOP
+                });
+            }
+            FailureReason::Sstore => {
+                callee_bytecode.append(&bytecode! {
+                    // this SSTORE got error: ErrorWriteProtection
+                    SSTORE
+                    STOP
+                });
+            }
+            FailureReason::TStore => {
+                callee_bytecode.append(&bytecode! {
+                    // this TSTORE got error: ErrorWriteProtection
+                    TSTORE
+                    STOP
+                });
+            }
+        };
 
         test_ok(
             Account {

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

@@ -182,4 +182,36 @@ mod test {
 
         CircuitTestBuilder::new_from_test_ctx(ctx).run();
     }
+
+    #[test]
+    fn test_revert() {
+        let key = Word::from(34);
+        let value = Word::from(100);
+
+        let bytecode = bytecode! {
+            PUSH32(value)
+            PUSH32(key)
+            TSTORE
+            PUSH32(0)
+            PUSH32(0)
+            REVERT
+        };
+        let ctx = TestContext::<2, 1>::new(
+            None,
+            |accs| {
+                accs[0]
+                    .address(MOCK_ACCOUNTS[0])
+                    .balance(Word::from(10u64.pow(19)))
+                    .code(bytecode);
+                accs[1]
+                    .address(MOCK_ACCOUNTS[1])
+                    .balance(Word::from(10u64.pow(19)));
+            },
+            tx_from_1_to_0,
+            |block, _txs| block,
+        )
+        .unwrap();
+
+        CircuitTestBuilder::new_from_test_ctx(ctx).run();
+    }
 }

zkevm-circuits/src/evm_circuit/util/constraint_builder.rs

@@ -1116,7 +1116,7 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> {
         self.rw_lookup(
             "account_transient_storage_read",
             false.expr(),
-            RwTableTag::TransientStorage,
+            RwTableTag::AccountTransientStorage,
             RwValues::new(
                 tx_id,
                 account_address,
@@ -1142,7 +1142,7 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> {
     ) {
         self.reversible_write(
             "account_transient_storage_write",
-            RwTableTag::TransientStorage,
+            RwTableTag::AccountTransientStorage,
             RwValues::new(
                 tx_id,
                 account_address,

zkevm-circuits/src/table.rs

@@ -471,7 +471,7 @@ pub enum RwTableTag {
     /// Account Storage operation
     AccountStorage,
     /// Account Transient Storage operation
-    TransientStorage,
+    AccountTransientStorage,
     /// Call Context operation
     CallContext,
     /// Tx Log operation
@@ -491,7 +491,7 @@ impl RwTableTag {
                 | RwTableTag::TxRefund
                 | RwTableTag::Account
                 | RwTableTag::AccountStorage
-                | RwTableTag::TransientStorage
+                | RwTableTag::AccountTransientStorage
         )
     }
 }

zkevm-circuits/src/witness/rw.rs

@@ -600,8 +600,7 @@ impl Rw {
             Self::Memory { .. } => RwTableTag::Memory,
             Self::Stack { .. } => RwTableTag::Stack,
             Self::AccountStorage { .. } => RwTableTag::AccountStorage,
-            // TODO: pick one!
-            Self::AccountTransientStorage { .. } => RwTableTag::TransientStorage,
+            Self::AccountTransientStorage { .. } => RwTableTag::AccountTransientStorage,
             Self::TxAccessListAccount { .. } => RwTableTag::TxAccessListAccount,
             Self::TxAccessListAccountStorage { .. } => RwTableTag::TxAccessListAccountStorage,
             Self::TxRefund { .. } => RwTableTag::TxRefund,
@@ -941,7 +940,7 @@ impl From<&operation::OperationContainer> for RwMap {
                 .collect(),
         );
         rws.insert(
-            RwTableTag::TransientStorage,
+            RwTableTag::AccountTransientStorage,
             container
                 .transient_storage
                 .iter()

zkevm-circuits/src/witness/step.rs

@@ -253,7 +253,7 @@ pub(super) fn step_convert(step: &circuit_input_builder::ExecStep, block_num: u6
                     operation::Target::Memory => RwTableTag::Memory,
                     operation::Target::Stack => RwTableTag::Stack,
                     operation::Target::Storage => RwTableTag::AccountStorage,
-                    operation::Target::TransientStorage => RwTableTag::TransientStorage,
+                    operation::Target::TransientStorage => RwTableTag::AccountTransientStorage,
                     operation::Target::TxAccessListAccount => RwTableTag::TxAccessListAccount,
                     operation::Target::TxAccessListAccountStorage => {
                         RwTableTag::TxAccessListAccountStorage