Skip to content

Conversation

@gumb0
Copy link
Member

@gumb0 gumb0 commented Feb 7, 2022

Unfortunately we have to update the spec of this retroactive EIP, because there was misunderstanding regarding how CREATE/CREATE2 instructions report nonce overflow error.
It makes much more sense to return 0 rather than abort entire execution in this case, because this is like all other create-specific errors are reported (e.g. insufficient balance for transfer, account already exists, code size above limit, code starts with 0xEF)

And in fact I assumed that 0 result is the expected behavior both in go-ethereum implementation and in tests, and these are already merged and released.

cc @axic @holiman

@eth-bot
Copy link
Collaborator

eth-bot commented Feb 7, 2022

All tests passed; auto-merging...

(pass) eip-2681.md

classification
updateEIP
  • passed!

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

I'm okay with this change. I think creating a new EIP to convey this behavior would be more confusing that simply updating the final EIP.

@gumb0
Copy link
Member Author

gumb0 commented Feb 10, 2022

I've added a clarification regarding what happens to gas passed to CREATE in case of failure.

@axic axic requested a review from lightclient February 10, 2022 21:21
@axic
Copy link
Member

axic commented Feb 10, 2022

@lightclient, @gumb0 applied another clarification, can you please check again?

Copy link

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Looks like the code block backticks got lost in the update.

EIPS/eip-2681.md Outdated

1. Consider any transaction invalid, where the nonce exceeds or equals to `2^64-1`.
2. The `CREATE` and `CREATE2` instructions to abort with an exceptional halt, where the account nonce is `2^64-1`.
2. The CREATE and CREATE2 instructions execution ends with the result `0` pushed on stack, where the account nonce is `2^64-1`. Gas for initcode execution is not deducted in this case.

Choose a reason for hiding this comment

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

Suggested change
2. The CREATE and CREATE2 instructions execution ends with the result `0` pushed on stack, where the account nonce is `2^64-1`. Gas for initcode execution is not deducted in this case.
2. The `CREATE` and `CREATE2` instruction's execution ends with the result `0` pushed on stack, where the account nonce is `2^64-1`. Gas for initcode execution is not deducted in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed these, thanks.

Shouldn't it be instructions' (plural) as it's about 2 instructions?

Choose a reason for hiding this comment

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

Heh, I initially used instructions' in my suggestion by my confidence on when the right time to do that is incredibly low so I went with the "safer" instruction's and hoped no one would notice/correct me. 😬

I think instructions' is correct... if you are confident then lets go with that!

Choose a reason for hiding this comment

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

Suggested change
2. The CREATE and CREATE2 instructions execution ends with the result `0` pushed on stack, where the account nonce is `2^64-1`. Gas for initcode execution is not deducted in this case.
2. The `CREATE` and `CREATE2` instructions' execution ends with the result `0` pushed on stack, where the account nonce is `2^64-1`. Gas for initcode execution is not deducted in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

@axic axic requested a review from MicahZoltu February 18, 2022 20:58
@MicahZoltu MicahZoltu requested a review from axic February 19, 2022 08:09
@axic axic closed this Feb 21, 2022
@axic axic reopened this Feb 21, 2022
@eth-bot eth-bot enabled auto-merge (squash) February 21, 2022 17:23
@eth-bot eth-bot merged commit b932109 into ethereum:master Feb 21, 2022
@axic axic deleted the eip-2681-update branch February 21, 2022 17:27
PowerStream3604 pushed a commit to PowerStream3604/EIPs that referenced this pull request May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants