Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "EVM Improvements" section in Istanbul for account versioning #2156

Closed

Conversation

sorpaas
Copy link
Contributor

@sorpaas sorpaas commented Jun 28, 2019

This adds a section called "EVM Improvements" in Istanbul hard fork meta EIP. This defines a template to specify the technical details for how EIP-1702 is applied in relation with other EIPs:

  • First, we have the category "Version 0". In this section, we apply those EIPs that do not want account versioning to be applied this time, based on Petersburg EVM. After this, we freeze version 0.
  • And then, we define a new "Version 1" based on version 0, and apply those EIPs that want to use or are required to use account versioning.

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • EIP 1679 requires approval from one of (@5chdn, @axic)

@shemnon
Copy link
Contributor

shemnon commented Jun 28, 2019

So Istanbul is last call for any "version 0" semantic changes?

@sorpaas
Copy link
Contributor Author

sorpaas commented Jun 28, 2019

@shemnon If we reached the decision to freeze version 0, then yeah, that will be the case.

@gumb0
Copy link
Member

gumb0 commented Jul 1, 2019

What do you think about making Istanbul's version 7, motivation being that there are already 7 versions of EVM that existed on the main net (and continue to exist for clients that want to do full sync) https://github.com/ethereum/evmc/blob/9794d7c2f777545950f0185812766149b99bd019/include/evmc/evmc.h#L714

I can imagine that this could make the clients using EVMC API simpler, as they could use account's version directly to tell EVMC which EVM revision to run (with the only special case to run version 0 with EVMC_PETERSBURG)

For any other clients that similarly decouple the rules inside EVM from forks of the blockchain, the similar motivation could apply - there are already 7 rule sets for EVM, and further versions will extend this line.

cc @chfast @axic

@sorpaas
Copy link
Contributor Author

sorpaas commented Jul 1, 2019

@gumb0 Sounds like a good idea to me. :)

A small nit -- maybe we want to consider setting it to version 8 (or if we want to reserve 0 for special use case, setting it to version 9), in case we have two revisions for Istanbul (one set of changes applied directly on Petersburg VM as version 0, and another set of changes applied as a new version).

@shemnon
Copy link
Contributor

shemnon commented Jul 2, 2019

Maybe we should go the marketing route and set the version to "0x10", since this work is being done under the "Ethereum 1.x" banner. That gives us 16 updates to get serenity locked in.

@chfast
Copy link
Member

chfast commented Jul 5, 2019

I'm ok with using ordering from EVMC's EVM revision: https://github.com/ethereum/evmc/blob/master/include/evmc/evmc.h#L708-L790, so 7 for Istanbul. Or just bumping by 1, so 1 for Istanbul.

EIPS/eip-1679.md Outdated
@@ -71,6 +71,30 @@ This meta-EIP specifies the changes included in the Ethereum hardfork named Ista
- [EIP-2045](https://eips.ethereum.org/EIPS/eip-2045): Particle gas costs for EVM opcodes
- [EIP-2046](https://eips.ethereum.org/EIPS/eip-2046): Reduced gas cost for static calls made to precompiles

### EVM Improvements

Below we list all EVM improvement EIPs, where they're rleated to account versioning (EIP-1702).

Choose a reason for hiding this comment

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

*related

@gumb0
Copy link
Member

gumb0 commented Jul 10, 2019

Now I think that if the plan is not to increment version with each fork, but only sometimes for really breaking changes of EVM, then each version will be active in several forks / several revisions in EVMC, and then starting with 7 will be just more confusing, it's better to start with 1.

@shemnon
Copy link
Contributor

shemnon commented Jul 10, 2019

I think the only thing that mandates a version increment at this point is adding new validation steps/rules. We can't go back and invalidate previously deployed contracts. I mean we could but we shouldn't.

@chfast
Copy link
Member

chfast commented Jul 11, 2019

Now I think that if the plan is not to increment version with each fork, but only sometimes for really breaking changes of EVM, then each version will be active in several forks / several revisions in EVMC, and then starting with 7 will be just more confusing, it's better to start with 1.

Option here is to go fully along with EVMC by skipping EVM revision number that were not connected with account version bump. E.g. 0, 7, 10, 13, ....
But I do not insist these two sequences must be connected.

@axic
Copy link
Member

axic commented Nov 5, 2019

@sorpaas I'd argue we should close this since account versioning wasn't included in istanbul

@sorpaas sorpaas closed this Nov 6, 2019
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.

7 participants