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

Merge Transfer and TransferWithGas gadget #1777

Merged
merged 22 commits into from
Mar 5, 2024
Merged

Merge Transfer and TransferWithGas gadget #1777

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
11be6ee
make balance update readable
ChihChengLiang Feb 21, 2024
2811228
visibility control
ChihChengLiang Feb 21, 2024
7f73a7f
unused comment
ChihChengLiang Feb 21, 2024
45e19c6
must create is known
ChihChengLiang Feb 21, 2024
f62b1cc
a sure place to dedup create account
ChihChengLiang Feb 21, 2024
b919f09
account_write is always true
ChihChengLiang Feb 21, 2024
0241779
the abstraction is no longer needed
ChihChengLiang Feb 21, 2024
618f033
merge transfer gadgets ugly
ChihChengLiang Feb 21, 2024
37c3fd1
WIP
ChihChengLiang Feb 22, 2024
f82c3a7
deduplicate and fix busmapping constraint
ChihChengLiang Feb 22, 2024
79d2c3f
fix the read write order
ChihChengLiang Feb 22, 2024
52e80c4
doc fix
ChihChengLiang Feb 22, 2024
0ee4aa4
dedup busmapping transfer
ChihChengLiang Feb 22, 2024
e3e1b1c
rm must_create
ChihChengLiang Feb 22, 2024
56ccc66
move all the transfer rws handling to transfer gadget
ChihChengLiang Feb 23, 2024
7a13f05
the early return skipped the receiver creation
ChihChengLiang Feb 23, 2024
598ad11
feedback
ChihChengLiang Feb 27, 2024
0522b5c
nitpick
ChihChengLiang Feb 27, 2024
77cfbcb
point to Geth\#28666 in the comment
ChihChengLiang Mar 5, 2024
990afe1
rename receiver -> receiver_balance
ChihChengLiang Mar 5, 2024
7449289
rename opcode_is_create -> is_create
ChihChengLiang Mar 5, 2024
f8773e2
expand receiver creation args
ChihChengLiang Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 16 additions & 56 deletions bus-mapping/src/circuit_input_builder/input_state_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,13 +559,13 @@ impl<'a> CircuitInputStateRef<'a> {
/// balance by `value`. If `fee` is existing (not None), also need to push 1
/// non-reversible [`AccountOp`] to update `sender` balance by `fee`.
#[allow(clippy::too_many_arguments)]
pub fn transfer_with_fee(
pub fn transfer(
&mut self,
step: &mut ExecStep,
sender: Address,
receiver: Address,
receiver_exists: bool,
must_create: bool,
opcode_is_create: bool,
value: Word,
fee: Option<Word>,
) -> Result<(), Error> {
Expand Down Expand Up @@ -608,68 +608,29 @@ impl<'a> CircuitInputStateRef<'a> {
sender_balance_prev,
sender_balance
);
// If receiver doesn't exist, create it
if !receiver_exists && (!value.is_zero() || must_create) {

if !value.is_zero() {
self.push_op_reversible(
step,
AccountOp {
address: receiver,
field: AccountField::CodeHash,
value: CodeDB::empty_code_hash().to_word(),
value_prev: Word::zero(),
address: sender,
field: AccountField::Balance,
value: sender_balance,
value_prev: sender_balance_prev,
},
)?;
}
if value.is_zero() {
// Skip transfer if value == 0
return Ok(());
}

self.push_op_reversible(
step,
AccountOp {
address: sender,
field: AccountField::Balance,
value: sender_balance,
value_prev: sender_balance_prev,
},
)?;

let (_found, receiver_account) = self.sdb.get_account(&receiver);
let receiver_balance_prev = receiver_account.balance;
let receiver_balance = receiver_account.balance + value;
self.push_op_reversible(
self.transfer_to(
step,
AccountOp {
address: receiver,
field: AccountField::Balance,
value: receiver_balance,
value_prev: receiver_balance_prev,
},
)?;

Ok(())
}

/// Same functionality with `transfer_with_fee` but with `fee` set zero.
pub fn transfer(
&mut self,
step: &mut ExecStep,
sender: Address,
receiver: Address,
receiver_exists: bool,
must_create: bool,
value: Word,
) -> Result<(), Error> {
self.transfer_with_fee(
step,
sender,
receiver,
receiver_exists,
must_create,
opcode_is_create,
value,
None,
)
true,
)?;

Ok(())
}

/// Transfer to an address. Create an account if it is not existed before.
Expand All @@ -678,12 +639,11 @@ impl<'a> CircuitInputStateRef<'a> {
step: &mut ExecStep,
receiver: Address,
receiver_exists: bool,
must_create: bool,
opcode_is_create: bool,
value: Word,
reversible: bool,
) -> Result<(), Error> {
// If receiver doesn't exist, create it
if (!receiver_exists && !value.is_zero()) || must_create {
if !receiver_exists && (!value.is_zero() || opcode_is_create) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition slips through the old PR that fixes all creation conditions.
https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1079/files/c0e6909d5135aa74e3ce415879d993a34050a823#r1336618956

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to think carefully about this condition. I would love input from reviewers.

  • What is receiver_exists?
    • We can check if code_hash is 0. It is an artificial rule we added before.
    • Nonce is 0. Should we add this? address_collision in create has this check.
  • opcode_is_create
    • originally called must_create. I think this covers 3 cases:
    • EOA transfer to create an account. Can we call this situtation "opcode_is_create"?
    • Create receiver with CREATE opcode
    • Create receiver with CREATE2 opcode

Copy link
Contributor

@KimiWu123 KimiWu123 Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From geth impl., an account which doesn't exist will be created only when transfer value is non-zero.

But, no matter this account exsists or not, it'll be created again in CREATE/2 opcode. You can see createAccount is called in create and then will find this

// createObject creates a new state object. If there is an existing account with
// the given address, it is overwritten and returned as the second return value.

in createObject

The conclusion is the original condition is correct. In your fix, the account won't be re-created if the receiver exists.

Copy link
Contributor

@KimiWu123 KimiWu123 Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the naming of opcode_is_create. I would vote is_create_account or force_create_account as you mentioned "EOA transfer to create an account", it doesn't fix the name.

Copy link
Collaborator Author

@ChihChengLiang ChihChengLiang Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an account which doesn't exist will be created only when transfer value is non-zero.

I think this is correct and it matches our behavior.

no matter this account exists or not, it'll be created again in CREATE/2 opcode.

This is incorrect. Three lines above your quoted createAccount locates a nonzero nonce and non-empty hash check that early exits the function and returns an ErrContractAddressCollision error.

The EIP-1014: Skinny CREATE2 also states "if nonce or code (of the create2 destination address) is nonzero, then the create-operation fails."

So for this PR, the conclusion is that a CREATEX opcode does not guarantee an overwrite of an account.

But you might wonder, why the create opcode only checks non empty code hash and non zero nonce. What about storage root and balance? Can we have an account that has code="" and nonce=0, but storage root !=0 or balance !=0? The tl;dr is we can't, for non-trivial reason.

I fell into a rabbit hole of a historical study and how the EIP was defined. Fortunately, Gary wrote the best summary of this issue. See ethereum/go-ethereum#28666.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion. Maybe we could put the link ethereum/go-ethereum#28666 in our comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added to the comment in the constraint part, which might be the place people will check more frequently 77cfbcb.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EOA transfer to create an account. Can we call this situtation "opcode_is_create"?

My opinion for opcode_is_create to represents create an account with tx.to = null is a bit confusing. How about just name it is_create to align with other place e.g. call.is_create?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed the renaming part before. I fixed the renaming here 7449289

self.account_write(
step,
receiver,
Expand Down
2 changes: 1 addition & 1 deletion bus-mapping/src/evm/opcodes/begin_end_tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
}

// Transfer with fee
state.transfer_with_fee(
state.transfer(
&mut exec_step,
call.caller_address,
call.address,
Expand Down
1 change: 1 addition & 0 deletions bus-mapping/src/evm/opcodes/callop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
callee_exists,
false,
call.value,
None,
)?;
}

Expand Down
3 changes: 2 additions & 1 deletion bus-mapping/src/evm/opcodes/create.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,9 @@ impl<const IS_CREATE2: bool> Opcode for Create<IS_CREATE2> {
caller.address,
callee.address,
callee_exists,
!callee_exists,
true,
callee.value,
None,
)?;

// EIP 161, increase callee's nonce
Expand Down
41 changes: 13 additions & 28 deletions zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
step::ExecutionState,
util::{
and,
common_gadget::TransferWithGasFeeGadget,
common_gadget::TransferGadget,
constraint_builder::{
ConstrainBuilderCommon, EVMConstraintBuilder, ReversionInfo, StepStateTransition,
Transition::{Delta, To},
Expand All @@ -14,7 +14,7 @@ use crate::{
math_gadget::{
ContractCreateGadget, IsEqualWordGadget, IsZeroWordGadget, RangeCheckGadget,
},
not, or,
not,
tx::{BeginTxHelperGadget, TxDataGadget},
AccountAddress, CachedRegion, Cell, StepRws,
},
Expand All @@ -41,7 +41,7 @@ pub(crate) struct BeginTxGadget<F> {
call_callee_address: AccountAddress<F>,
reversion_info: ReversionInfo<F>,
sufficient_gas_left: RangeCheckGadget<F, N_BYTES_GAS>,
transfer_with_gas_fee: TransferWithGasFeeGadget<F>,
transfer_with_gas_fee: TransferGadget<F, true>,
code_hash: WordLoHiCell<F>,
is_empty_code_hash: IsEqualWordGadget<F, WordLoHi<Expression<F>>, WordLoHi<Expression<F>>>,
caller_nonce_hash_bytes: Word32Cell<F>,
Expand Down Expand Up @@ -170,17 +170,16 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
AccountFieldTag::CodeHash,
code_hash.to_word(),
);

// Transfer value from caller to callee, creating account if necessary.
let transfer_with_gas_fee = TransferWithGasFeeGadget::construct(
let transfer_with_gas_fee = TransferGadget::construct(
cb,
tx.caller_address.to_word(),
tx.callee_address.to_word(),
not::expr(callee_not_exists.expr()),
or::expr([tx.is_create.expr(), callee_not_exists.expr()]),
tx.is_create.expr(),
tx.value.clone(),
tx.mul_gas_fee_by_gas.product().clone(),
&mut reversion_info,
Some(tx.mul_gas_fee_by_gas.product().clone()),
);

let caller_nonce_hash_bytes = cb.query_word32();
Expand Down Expand Up @@ -467,18 +466,13 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
}
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) {
callee_code_hash = rws.next().account_codehash_pair().1;
}
let mut caller_balance_sub_value_pair = (zero, zero);
let mut callee_balance_pair = (zero, zero);
if !tx.value.is_zero() {
caller_balance_sub_value_pair = rws.next().account_balance_pair();
callee_balance_pair = rws.next().account_balance_pair();
};

self.transfer_with_gas_fee.assign(
region,
offset,
&mut rws,
(callee_exists, tx.value, tx.is_create()),
Some(gas_fee),
)?;
self.begin_tx.assign(region, offset, tx)?;
self.tx.assign(region, offset, tx)?;

Expand All @@ -502,15 +496,6 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
)?;
self.sufficient_gas_left
.assign(region, offset, F::from(tx.gas() - step.gas_cost))?;
self.transfer_with_gas_fee.assign(
region,
offset,
caller_balance_sub_fee_pair,
caller_balance_sub_value_pair,
callee_balance_pair,
tx.value,
gas_fee,
)?;
self.code_hash
.assign_u256(region, offset, callee_code_hash)?;
self.is_empty_code_hash.assign_u256(
Expand Down
19 changes: 7 additions & 12 deletions zkevm-circuits/src/evm_circuit/execution/callop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ pub(crate) struct CallOpGadget<F> {
is_warm: Cell<F>,
is_warm_prev: Cell<F>,
callee_reversion_info: ReversionInfo<F>,
transfer: TransferGadget<F>,
transfer: TransferGadget<F, false>,
// current handling Call* opcode's caller balance
caller_balance: WordLoHi<Cell<F>>,
// check if insufficient balance case
Expand Down Expand Up @@ -242,9 +242,10 @@ impl<F: Field> ExecutionGadget<F> for CallOpGadget<F> {
caller_address.to_word(),
callee_address.to_word(),
not::expr(call_gadget.callee_not_exists.expr()),
0.expr(),
false.expr(),
call_gadget.value.clone(),
&mut callee_reversion_info,
None,
)
});
// rwc_delta = 8 + is_delegatecall * 2 + call_gadget.rw_delta() +
Expand Down Expand Up @@ -866,19 +867,13 @@ impl<F: Field> ExecutionGadget<F> for CallOpGadget<F> {
depth.low_u64() < 1025 && (!(is_call || is_callcode) || caller_balance >= value);

// conditionally assign
if is_call && is_precheck_ok && !value.is_zero() {
if !callee_exists {
rws.next().account_codehash_pair(); // callee hash
}

let caller_balance_pair = rws.next().account_balance_pair();
let callee_balance_pair = rws.next().account_balance_pair();
if is_call && is_precheck_ok {
self.transfer.assign(
region,
offset,
caller_balance_pair,
callee_balance_pair,
value,
&mut rws,
(callee_exists, value, false),
None,
)?;
}

Expand Down
22 changes: 7 additions & 15 deletions zkevm-circuits/src/evm_circuit/execution/create.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub(crate) struct CreateGadget<F, const IS_CREATE2: bool, const S: ExecutionStat
callee_nonce: Cell<F>,
prev_code_hash: WordLoHiCell<F>,
prev_code_hash_is_zero: IsZeroWordGadget<F, WordLoHi<Expression<F>>>,
transfer: TransferGadget<F>,
transfer: TransferGadget<F, false>,
create: ContractCreateGadget<F, IS_CREATE2>,

init_code: MemoryAddressGadget<F>,
Expand Down Expand Up @@ -333,10 +333,11 @@ impl<F: Field, const IS_CREATE2: bool, const S: ExecutionState> ExecutionGadget<
cb,
create.caller_address(),
contract_addr.to_word(),
0.expr(),
1.expr(),
false.expr(),
true.expr(),
value.clone(),
&mut callee_reversion_info,
None,
);

// EIP 161, the nonce of a newly created contract is 1
Expand Down Expand Up @@ -638,21 +639,12 @@ impl<F: Field, const IS_CREATE2: bool, const S: ExecutionState> ExecutionGadget<

let code_hash = if is_precheck_ok {
if !is_address_collision {
// transfer
if callee_prev_code_hash.is_zero() {
rws.next(); // codehash update
}
let [caller_balance_pair, callee_balance_pair] = if !value.is_zero() {
[(); 2].map(|_| rws.next().account_balance_pair())
} else {
[(0.into(), 0.into()), (0.into(), 0.into())]
};
self.transfer.assign(
region,
offset,
caller_balance_pair,
callee_balance_pair,
value,
&mut rws,
(!callee_prev_code_hash.is_zero(), value, true),
None,
)?;
}

Expand Down
40 changes: 14 additions & 26 deletions zkevm-circuits/src/evm_circuit/execution/end_tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::{
MulWordByU64Gadget,
},
tx::EndTxHelperGadget,
CachedRegion, Cell,
CachedRegion, Cell, StepRws,
},
witness::{Block, Call, ExecStep, Transaction},
},
Expand Down Expand Up @@ -75,10 +75,9 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
tx_gas_price.clone(),
effective_refund.min() + cb.curr.state.gas_left.expr(),
);
let gas_fee_refund = UpdateBalanceGadget::construct(
cb,
let gas_fee_refund = cb.increase_balance(
tx_caller_address.to_word(),
vec![mul_gas_price_by_refund.product().clone()],
mul_gas_price_by_refund.product().clone(),
None,
);

Expand Down Expand Up @@ -111,7 +110,6 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
false.expr(),
mul_effective_tip_by_gas_used.product().clone(),
None,
true,
);

let end_tx = EndTxHelperGadget::construct(
Expand Down Expand Up @@ -152,9 +150,11 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
step: &ExecStep,
) -> Result<(), Error> {
let gas_used = tx.gas() - step.gas_left;
let (refund, _) = block.get_rws(step, 2).tx_refund_value_pair();
let (caller_balance, caller_balance_prev) = block.get_rws(step, 3).account_balance_pair();
let (coinbase_code_hash_prev, _) = block.get_rws(step, 4).account_codehash_pair();
let mut rws = StepRws::new(block, step);
rws.offset_add(2);
let (refund, _) = rws.next().tx_refund_value_pair();
let (caller_balance, caller_balance_prev) = rws.next().account_balance_pair();
let (coinbase_code_hash_prev, _) = rws.next().account_codehash_pair();

self.tx_id
.assign(region, offset, Value::known(F::from(tx.id)))?;
Expand Down Expand Up @@ -208,24 +208,12 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
.assign_u256(region, offset, coinbase_code_hash_prev)?;
self.coinbase_code_hash_is_zero
.assign_u256(region, offset, coinbase_code_hash_prev)?;
if !coinbase_reward.is_zero() {
let coinbase_balance_pair = block
.get_rws(
step,
if coinbase_code_hash_prev.is_zero() {
6
} else {
5
},
)
.account_balance_pair();
self.coinbase_reward.assign(
region,
offset,
coinbase_balance_pair,
effective_tip * gas_used,
)?;
}
self.coinbase_reward.assign(
region,
offset,
&mut rws,
(!coinbase_code_hash_prev.is_zero(), coinbase_reward, false),
)?;
self.is_persistent.assign(
region,
offset,
Expand Down
Loading