-
Notifications
You must be signed in to change notification settings - Fork 856
Merge Transfer and TransferWithGas gadget #1777
Conversation
788cb4b
to
8d4d596
Compare
ee2917e
to
dacf577
Compare
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
42c3aa0
to
af62f15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not finished the review yet, but seems some changes needed and the light testing is falied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing the read-write inconsistency. I'll this back to draft
cf4e4d2
to
4a400a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I collected all the reads and writes from assignment functions everywhere back to the transfer gadget. I hope this reduces the code complexity.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not finished yet, but would like to discuss the callee existence earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replied some comments that I have quick answers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It looks nice and clean now. Great work! The remaining thing is this dicussion thread
c9f89d4
to
598ad11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KimiWu123, I replied to the feedback to the receiver creation check.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the code cleanup!
LGTM with few nitpicks
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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
Description
Thank @curryrasul for initializing this conversation.
Issue Link
Type of change
Refactor (no updates to logic)
Content
Test