From aade576837b6c9cd3779a1287b53cf8c6da19b6f Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 30 Apr 2024 12:50:33 -0400 Subject: [PATCH] Fix TStore for reverts and in static calls (#1811) ### Description The bus mapping TSTORE did not handle the cases where the opcode's effect is revert and the error that happens if TSTORE is called in a static call. This adds tests for those two cases and then fixes the bus mapping. ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Refactor (no updates to logic) ### How Has This Been Tested? Added tests that fail before the bus mapping fixes are applied. ``` This PR contains: - [Add failing revert test](https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1811/commits/be4b13dd1251a370456c6ed86f04226af48aea08) - [Add failing write protection test for TStore](https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1811/commits/18845eb1a5fd624bddb0ac626c22a5d32389ea8f) - [Handle reverts and write protection failure for TSTORE in bus mapping](https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1811/commits/01050195879550fa68d9abd3d669ffc7b658d3f5) ``` --------- Co-authored-by: Mason Liang --- .../circuit_input_builder/input_state_ref.rs | 9 +++ .../src/evm/opcodes/error_write_protection.rs | 1 + bus-mapping/src/operation/container.rs | 7 +- .../execution/error_write_protection.rs | 73 ++++++++++++------- .../src/evm_circuit/execution/tstore.rs | 32 ++++++++ 5 files changed, 94 insertions(+), 28 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder/input_state_ref.rs b/bus-mapping/src/circuit_input_builder/input_state_ref.rs index 17cdddcadee..44e63653561 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -935,6 +935,14 @@ impl<'a> CircuitInputStateRef<'a> { None } } + OperationRef(Target::TransientStorage, idx) => { + let operation = &self.block.container.transient_storage[*idx]; + if operation.rw().is_write() && operation.reversible() { + Some(OpEnum::TransientStorage(operation.op().reverse())) + } else { + None + } + } OperationRef(Target::TxAccessListAccount, idx) => { let operation = &self.block.container.tx_access_list_account[*idx]; if operation.rw().is_write() && operation.reversible() { @@ -1436,6 +1444,7 @@ impl<'a> CircuitInputStateRef<'a> { OpcodeId::RETURNDATACOPY => Some(ExecError::ReturnDataOutOfBounds), // Break write protection (CALL with value will be handled below) OpcodeId::SSTORE + | OpcodeId::TSTORE | OpcodeId::CREATE | OpcodeId::CREATE2 | OpcodeId::SELFDESTRUCT diff --git a/bus-mapping/src/evm/opcodes/error_write_protection.rs b/bus-mapping/src/evm/opcodes/error_write_protection.rs index 65c3cb9eeee..b5571f5215d 100644 --- a/bus-mapping/src/evm/opcodes/error_write_protection.rs +++ b/bus-mapping/src/evm/opcodes/error_write_protection.rs @@ -31,6 +31,7 @@ impl Opcode for ErrorWriteProtection { // assert op code can only be following codes assert!([ OpcodeId::SSTORE, + OpcodeId::TSTORE, OpcodeId::CREATE, OpcodeId::CREATE2, OpcodeId::CALL, diff --git a/bus-mapping/src/operation/container.rs b/bus-mapping/src/operation/container.rs index 0e3551af9b0..e4064b5820c 100644 --- a/bus-mapping/src/operation/container.rs +++ b/bus-mapping/src/operation/container.rs @@ -124,8 +124,11 @@ impl OperationContainer { OperationRef::from((Target::Storage, self.storage.len() - 1)) } OpEnum::TransientStorage(op) => { - self.transient_storage - .push(Operation::new(rwc, rwc_inner_chunk, rw, op)); + self.transient_storage.push(if reversible { + Operation::new_reversible(rwc, rwc_inner_chunk, rw, op) + } else { + Operation::new(rwc, rwc_inner_chunk, rw, op) + }); OperationRef::from((Target::TransientStorage, self.transient_storage.len() - 1)) } OpEnum::TxAccessListAccount(op) => { diff --git a/zkevm-circuits/src/evm_circuit/execution/error_write_protection.rs b/zkevm-circuits/src/evm_circuit/execution/error_write_protection.rs index 02663549890..477bd2da611 100644 --- a/zkevm-circuits/src/evm_circuit/execution/error_write_protection.rs +++ b/zkevm-circuits/src/evm_circuit/execution/error_write_protection.rs @@ -43,7 +43,7 @@ impl ExecutionGadget for ErrorWriteProtectionGadget { let value = cb.query_word_unchecked(); let is_value_zero = cb.is_zero_word(&value); - // require_in_set method will spilit into more low degree expressions if exceed + // require_in_set method will split into more low degree expressions if exceed // max_degree. otherwise need to do fixed lookup for these opcodes // checking. cb.require_in_set( @@ -52,6 +52,7 @@ impl ExecutionGadget for ErrorWriteProtectionGadget { vec![ OpcodeId::CALL.expr(), OpcodeId::SSTORE.expr(), + OpcodeId::TSTORE.expr(), OpcodeId::CREATE.expr(), OpcodeId::CREATE2.expr(), OpcodeId::SELFDESTRUCT.expr(), @@ -155,16 +156,26 @@ mod test { Account::mock_code_balance(code) } + #[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) @@ -185,26 +196,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::mock_100_ether(caller_bytecode), diff --git a/zkevm-circuits/src/evm_circuit/execution/tstore.rs b/zkevm-circuits/src/evm_circuit/execution/tstore.rs index d50bbb0a686..d0a89397c29 100644 --- a/zkevm-circuits/src/evm_circuit/execution/tstore.rs +++ b/zkevm-circuits/src/evm_circuit/execution/tstore.rs @@ -178,4 +178,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(); + } }