Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Commit

Permalink
Fix TStore for reverts and in static calls (#1811)
Browse files Browse the repository at this point in the history
### 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](be4b13d)
- [Add failing write protection test for TStore](18845eb)
- [Handle reverts and write protection failure for TSTORE in bus mapping](0105019)
```

---------

Co-authored-by: Mason Liang <mason@scroll.io>
  • Loading branch information
z2trillion and Mason Liang authored Apr 30, 2024
1 parent 91b59e7 commit 9a13854
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 28 deletions.
9 changes: 9 additions & 0 deletions bus-mapping/src/circuit_input_builder/input_state_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions bus-mapping/src/evm/opcodes/error_write_protection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 5 additions & 2 deletions bus-mapping/src/operation/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
73 changes: 47 additions & 26 deletions zkevm-circuits/src/evm_circuit/execution/error_write_protection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<F: Field> ExecutionGadget<F> for ErrorWriteProtectionGadget<F> {
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(
Expand All @@ -52,6 +52,7 @@ impl<F: Field> ExecutionGadget<F> for ErrorWriteProtectionGadget<F> {
vec![
OpcodeId::CALL.expr(),
OpcodeId::SSTORE.expr(),
OpcodeId::TSTORE.expr(),
OpcodeId::CREATE.expr(),
OpcodeId::CREATE2.expr(),
OpcodeId::SELFDESTRUCT.expr(),
Expand Down Expand Up @@ -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)
Expand All @@ -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),
Expand Down
32 changes: 32 additions & 0 deletions zkevm-circuits/src/evm_circuit/execution/tstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

0 comments on commit 9a13854

Please sign in to comment.