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

Commit

Permalink
fix many bugs of create op (#1657)
Browse files Browse the repository at this point in the history
### Description

i think this pr can fix many errs of testool report

1. fix bugs of
#1639
2. refactor rw reading using the rw iterator, which is far more friendly
to understand compared to the current manually maintained rw offset way.
3. another refactor here, is move the "init code copy event" from callee
stage to caller stage (before state.push_call). It is more intuitive.
Inside execution, we can think the initcode is "copied from
CopyDataType::Memory to CopyDataType::Bytecode" then is executed. So
doing the copy event inside callee stage is a bit wired.

Btw i have another double on hi-lo (which i am not very familiar with):
is this correct?
https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/09651d40afc59462fd943e9789be8f8b0ccb90d9/zkevm-circuits/src/evm_circuit/execution/create.rs#L545
is_address_collision is boolean, which is not equal to the expr used to
construct the is_zero gadget?

### Issue Link


#1639

### 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
  • Loading branch information
lispc committed Oct 25, 2023
1 parent 41ab519 commit 93791d1
Show file tree
Hide file tree
Showing 3 changed files with 236 additions and 181 deletions.
47 changes: 26 additions & 21 deletions bus-mapping/src/evm/opcodes/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {

let callee_account = &state.sdb.get_account(&address).1.clone();
let callee_exists = !callee_account.is_empty();
let is_address_collision =
callee_account.code_hash != CodeDB::empty_code_hash() || callee_account.nonce != 0;
state.stack_write(
&mut exec_step,
geth_step.stack.nth_last_filled(n_pop - 1),
Expand Down Expand Up @@ -124,7 +126,7 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
// operation happens in evm create() method before checking
// ErrContractAddressCollision
let code_hash_previous = if callee_exists {
if is_precheck_ok {
if is_precheck_ok && is_address_collision {
exec_step.error = Some(ExecError::ContractAddressCollision);
}
callee_account.code_hash
Expand All @@ -138,6 +140,15 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
AccountField::CodeHash,
code_hash_previous.to_word(),
);
// read callee nonce for address collision checking
if !code_hash_previous.is_zero() {
state.account_read(
&mut exec_step,
callee.address,
AccountField::Nonce,
callee_account.nonce.into(),
);
}
}

// Per EIP-150, all but one 64th of the caller's gas is sent to the
Expand All @@ -159,23 +170,15 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
state.call_context_write(&mut exec_step, caller.call_id, field, value);
}

let (initialization_code, code_hash) = if is_precheck_ok && length > 0 {
handle_copy(state, &mut exec_step, state.call()?.call_id, offset, length)?
} else {
(vec![], CodeDB::empty_code_hash())
};
state.push_call(callee.clone());
state.reversion_info_write(&mut exec_step, &callee);

// successful contract creation
if is_precheck_ok && !callee_exists {
let (initialization_code, code_hash) = if length > 0 {
handle_copy(
state,
&mut exec_step,
state.caller()?.call_id,
offset,
length,
)?
} else {
(vec![], CodeDB::empty_code_hash())
};

// handle init_code hashing & address generation
if is_precheck_ok {
// handle keccak_table_lookup
let keccak_input = if IS_CREATE2 {
let salt = geth_step.stack.nth_last(3)?;
Expand Down Expand Up @@ -206,7 +209,8 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {

state.block.sha3_inputs.push(keccak_input);
state.block.sha3_inputs.push(initialization_code);

}
if is_precheck_ok && !is_address_collision {
// Transfer function will skip transfer if the value is zero
state.transfer(
&mut exec_step,
Expand Down Expand Up @@ -251,14 +255,15 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
(CallContextField::IsStatic, false.to_word()),
(CallContextField::IsCreate, true.to_word()),
(CallContextField::CodeHash, code_hash.to_word()),
(CallContextField::Value, callee.value),
] {
state.call_context_write(&mut exec_step, callee.call_id, field, value);
}
}
// if it's empty init code
else {
for (field, value) in [
(CallContextField::LastCalleeId, 0.into()),
(CallContextField::LastCalleeId, callee.call_id.into()),
(CallContextField::LastCalleeReturnDataOffset, 0.into()),
(CallContextField::LastCalleeReturnDataLength, 0.into()),
] {
Expand All @@ -267,10 +272,10 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
state.handle_return(&mut exec_step, geth_steps, false)?;
};
}
// failed case: is_precheck_ok is false or callee_exists is true
// failed case: is_precheck_ok is false or is_address_collision is true
else {
for (field, value) in [
(CallContextField::LastCalleeId, 0.into()),
(CallContextField::LastCalleeId, callee.call_id.into()),
(CallContextField::LastCalleeReturnDataOffset, 0.into()),
(CallContextField::LastCalleeReturnDataLength, 0.into()),
] {
Expand All @@ -290,7 +295,7 @@ fn handle_copy(
offset: usize,
length: usize,
) -> Result<(Vec<u8>, H256), Error> {
let initialization_bytes = state.caller_ctx()?.memory.0[offset..(offset + length)].to_vec();
let initialization_bytes = state.call_ctx()?.memory.0[offset..(offset + length)].to_vec();

let initialization = Bytecode::from(initialization_bytes.clone());
let code_hash = initialization.hash_h256();
Expand Down
Loading

0 comments on commit 93791d1

Please sign in to comment.