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

Commit

Permalink
Handle accounts with non-zero code hashes in BeginTx (#1079)
Browse files Browse the repository at this point in the history 
This occurs when an address already exists because it has a non-zero
balance.

---------

Co-authored-by: Mason Liang <mason@scroll.io>
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
  • Loading branch information
@z2trillion @ChihChengLiang
3 people authored Sep 29, 2023
1 parent 11aee94 commit 2f3f874
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 36 deletions.
2 changes: 1 addition & 1 deletion bus-mapping/src/circuit_input_builder/input_state_ref.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ impl<'a> CircuitInputStateRef<'a> {
sender_balance
);
// If receiver doesn't exist, create it
if (!receiver_exists && !value.is_zero()) || must_create {
if !receiver_exists && (!value.is_zero() || must_create) {
self.push_op_reversible(
step,
AccountOp {
Expand Down
2 changes: 1 addition & 1 deletion bus-mapping/src/evm/opcodes/begin_end_tx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
} else {
(Word::zero(), true)
};
if !state.is_precompiled(&call.address) && !call.is_create() {
if !state.is_precompiled(&call.address) {
state.account_read(
&mut exec_step,
call.address,
Expand Down
53 changes: 37 additions & 16 deletions zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,13 +216,13 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
let no_callee_code = is_empty_code_hash.expr() + callee_not_exists.expr();

// TODO: And not precompile
cb.condition(not::expr(tx_is_create.expr()), |cb| {
cb.account_read(
tx_callee_address.to_word(),
AccountFieldTag::CodeHash,
code_hash.to_word(),
); // rwc_delta += 1
});
// i think this needs to be removed....

cb.account_read(
tx_callee_address.to_word(),
AccountFieldTag::CodeHash,
code_hash.to_word(),
);

// Transfer value from caller to callee, creating account if necessary.
let transfer_with_gas_fee = TransferWithGasFeeGadget::construct(
Expand Down Expand Up @@ -338,7 +338,7 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
// - Write CallContext IsRoot
// - Write CallContext IsCreate
// - Write CallContext CodeHash
rw_counter: Delta(22.expr() + transfer_with_gas_fee.rw_delta()),
rw_counter: Delta(23.expr() + transfer_with_gas_fee.rw_delta()),
call_id: To(call_id.expr()),
is_root: To(true.expr()),
is_create: To(tx_is_create.expr()),
Expand Down Expand Up @@ -513,14 +513,14 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {

let is_coinbase_warm = rws.next().tx_access_list_value_pair().1;
let mut callee_code_hash = zero;
if !is_precompiled(&tx.to_or_contract_addr()) && !tx.is_create() {
if !is_precompiled(&tx.to_or_contract_addr()) {
callee_code_hash = rws.next().account_codehash_pair().1;
}
let callee_exists = is_precompiled(&tx.to_or_contract_addr())
|| (!tx.is_create() && !callee_code_hash.is_zero());
let callee_exists =
is_precompiled(&tx.to_or_contract_addr()) || !callee_code_hash.is_zero();
let caller_balance_sub_fee_pair = rws.next().account_balance_pair();
let must_create = tx.is_create();
if (!callee_exists && !tx.value.is_zero()) || must_create {
if !callee_exists && (!tx.value.is_zero() || must_create) {
callee_code_hash = rws.next().account_codehash_pair().1;
}
let mut caller_balance_sub_value_pair = (zero, zero);
Expand Down Expand Up @@ -634,13 +634,12 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {

#[cfg(test)]
mod test {
use std::vec;

use crate::{evm_circuit::test::rand_bytes, test_util::CircuitTestBuilder};
use bus_mapping::evm::OpcodeId;
use eth_types::{self, bytecode, evm_types::GasCost, word, Bytecode, Word};

use eth_types::{self, bytecode, evm_types::GasCost, word, Address, Bytecode, Word};
use ethers_core::utils::get_contract_address;
use mock::{eth, gwei, MockTransaction, TestContext, MOCK_ACCOUNTS};
use std::vec;

fn gas(call_data: &[u8]) -> Word {
Word::from(
Expand Down Expand Up @@ -920,4 +919,26 @@ mod test {
begin_tx_deploy(0x1020304050607080u64);
begin_tx_deploy(0xfffffffffffffffeu64);
}

#[test]
fn create_tx_for_existing_account() {
let address = Address::repeat_byte(23);
let nonce = 10;
let new_address = get_contract_address(address, nonce + 1);

let ctx = TestContext::<1, 2>::new(
None,
|accs| {
accs[0].address(address).nonce(nonce).balance(eth(10));
},
|mut txs, _| {
txs[0].from(address).to(new_address).value(eth(2)); // Initialize new_address with some balance and an empty code hash
txs[1].from(address); // Run a CREATE tx on new_address
},
|block, _| block,
)
.unwrap();

CircuitTestBuilder::new_from_test_ctx(ctx).run();
}
}
26 changes: 25 additions & 1 deletion zkevm-circuits/src/evm_circuit/execution/extcodehash.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ mod test {
address, bytecode, geth_types::Account, Address, Bytecode, Bytes, ToWord, Word, U256, U64,
};
use lazy_static::lazy_static;
use mock::TestContext;
use mock::{eth, TestContext};

lazy_static! {
static ref EXTERNAL_ADDRESS: Address =
Expand Down Expand Up @@ -263,4 +263,28 @@ mod test {
test_ok(Some(account), false);
}
}

#[test]
// Regression test to ensure that the code hash for an account that is is being initialized is
// the empty code hash.
fn create_tx_extcodehash() {
let code = bytecode! {
ADDRESS
EXTCODEHASH
};

let ctx = TestContext::<1, 1>::new(
None,
|accs| {
accs[0].address(Address::repeat_byte(23)).balance(eth(10));
},
|mut txs, accs| {
txs[0].from(accs[0].address).input(code.into());
},
|block, _tx| block.number(0xcafeu64),
)
.unwrap();

CircuitTestBuilder::new_from_test_ctx(ctx).run()
}
}
35 changes: 18 additions & 17 deletions zkevm-circuits/src/evm_circuit/util/common_gadget.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::{
step::ExecutionState,
table::{FixedTableTag, Lookup},
util::{
and,
constraint_builder::{
EVMConstraintBuilder, ReversionInfo, StepStateTransition,
Transition::{Delta, Same, To},
Expand Down Expand Up @@ -358,7 +359,7 @@ impl<F: Field> TransferToGadget<F> {
) -> Self {
let value_is_zero = IsZeroWordGadget::construct(cb, &value);
if account_write {
Self::creat_account(
Self::create_account(
cb,
receiver_address.clone(),
receiver_exists.clone(),
Expand All @@ -384,7 +385,7 @@ impl<F: Field> TransferToGadget<F> {
}
}

pub(crate) fn creat_account(
pub(crate) fn create_account(
cb: &mut EVMConstraintBuilder<F>,
receiver_address: Word<Expression<F>>,
receiver_exists: Expression<F>,
Expand All @@ -393,9 +394,9 @@ impl<F: Field> TransferToGadget<F> {
reversion_info: Option<&mut ReversionInfo<F>>,
) {
cb.condition(
or::expr([
not::expr(value_is_zero.expr()) * not::expr(receiver_exists.expr()),
must_create,
and::expr([
not::expr(receiver_exists.expr()),
or::expr([not::expr(value_is_zero.expr()), must_create]),
]),
|cb| {
cb.account_write(
Expand Down Expand Up @@ -430,10 +431,10 @@ impl<F: Field> TransferToGadget<F> {

pub(crate) fn rw_delta(&self) -> Expression<F> {
// +1 Write Account (receiver) CodeHash (account creation via code_hash update)
or::expr([
not::expr(self.value_is_zero.expr()) * not::expr(self.receiver_exists.expr()),
self.must_create.clone()]
) +
and::expr([
not::expr(self.receiver_exists.expr()),
or::expr([not::expr(self.value_is_zero.expr()), self.must_create.clone()]),
]) +
// +1 Write Account (receiver) Balance
not::expr(self.value_is_zero.expr())
}
Expand Down Expand Up @@ -470,7 +471,7 @@ impl<F: Field> TransferWithGasFeeGadget<F> {
UpdateBalanceGadget::construct(cb, sender_address.to_word(), vec![gas_fee], None);
let value_is_zero = IsZeroWordGadget::construct(cb, &value);
// If receiver doesn't exist, create it
TransferToGadget::creat_account(
TransferToGadget::create_account(
cb,
receiver_address.clone(),
receiver_exists.clone(),
Expand Down Expand Up @@ -509,10 +510,10 @@ impl<F: Field> TransferWithGasFeeGadget<F> {
// +1 Write Account (sender) Balance (Not Reversible tx fee)
1.expr() +
// +1 Write Account (receiver) CodeHash (account creation via code_hash update)
or::expr([
not::expr(self.value_is_zero.expr()) * not::expr(self.receiver.receiver_exists.expr()),
and::expr([not::expr(self.receiver.receiver_exists.expr()), or::expr([
not::expr(self.value_is_zero.expr()),
self.receiver.must_create.clone()]
) * 1.expr() +
)]) * 1.expr() +
// +1 Write Account (sender) Balance
// +1 Write Account (receiver) Balance
not::expr(self.value_is_zero.expr()) * 2.expr()
Expand All @@ -521,10 +522,10 @@ impl<F: Field> TransferWithGasFeeGadget<F> {
pub(crate) fn reversible_w_delta(&self) -> Expression<F> {
// NOTE: Write Account (sender) Balance (Not Reversible tx fee)
// +1 Write Account (receiver) CodeHash (account creation via code_hash update)
or::expr([
not::expr(self.value_is_zero.expr()) * not::expr(self.receiver.receiver_exists.expr()),
and::expr([not::expr(self.receiver.receiver_exists.expr()), or::expr([
not::expr(self.value_is_zero.expr()),
self.receiver.must_create.clone()]
) +
)]) +
// +1 Write Account (sender) Balance
// +1 Write Account (receiver) Balance
not::expr(self.value_is_zero.expr()) * 2.expr()
Expand Down Expand Up @@ -590,7 +591,7 @@ impl<F: Field> TransferGadget<F> {
) -> Self {
let value_is_zero = IsZeroWordGadget::construct(cb, &value);
// If receiver doesn't exist, create it
TransferToGadget::creat_account(
TransferToGadget::create_account(
cb,
receiver_address.clone(),
receiver_exists.clone(),
Expand Down

0 comments on commit 2f3f874

Please sign in to comment.