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

Set default evmVersion to paris #4232

Closed
6 tasks
frangio opened this issue Aug 2, 2023 · 13 comments · Fixed by #4336
Closed
6 tasks

Set default evmVersion to paris #4232

frangio opened this issue Aug 2, 2023 · 13 comments · Fixed by #4336
Assignees
Labels
status:ready This issue is ready to be worked on type:improvement

Comments

@frangio
Copy link
Contributor

frangio commented Aug 2, 2023

Edit by @fvictorio, original issue below.

We should:

  • Use paris as the evmVersion targeted by solc unless the user explicitly set one. This should work for all possible solc configurations: the version shortcut, an explicit version, multiple versions, and overrides.
  • Tests for all those scenarios.
  • Update the docs to explain the situation, what Hardhat does, and how to change it.
  • We should manually test this. It's easy to do: we just compile a contract with the default version, and check that no PUSH0 opcodes are included. Pasting the bytecode in the EVM Codes Playground and changing the view to mnemonic is the easiest way to do this.
  • Add a test which does the same that the previous step does. It's probably enough to have a fixture project where we just assert the resulting bytecode is a previously manually validated one. This will use a fixed solc version, but that's fine since this is mainly a smoke test.
  • We should at least check that there are no surprises around the cache. The way to do this is to compile a project with the current version of Hardhat, then install the version with these changes, and compile again. This should trigger a re-compilation because the solc config changed.

Describe the feature

Solc defaults the evmVersion parameter to shanghai since version 0.8.20. The main issue caused by this is that the compiler emits PUSH0 as an opcode. This opcode is supported on Ethereum L1 but as far as I know it hasn't been adopted on any other chain so far (except perhaps Gnosis Chain?). Most notably, it hasn't been adopted by L2s. On all these other chains, the PUSH0 opcode will be seen as an invalid opcode and cause the code to revert.

Hardhat relies on the solc default for evmVersion. Foundry, for reference, defaults to paris.

There is some knowledge about the potential issues caused by using shanghai, but I wouldn't count on it being widely known. At some point we will need to face EVM differences across chains and layers, and setting the default to paris simply delays this. However, setting the default to shanghai at this moment seems premature and is just asking for someone to deploy a broken contract to production.

I suggest to set the default evmVersion to paris if the compiler version is >=0.8.20 and the user hasn't configured it themselves.

Search terms

evmVersion paris shanghai push0

@pcaversaccio
Copy link
Contributor

I second that and is also in line with #4137.

@frangio
Copy link
Contributor Author

frangio commented Aug 2, 2023

I hadn't seen #4137 but it is definitely related. Using an older Solidity version is not sustainable in the long run though, we should start getting developers used to the notion of diverging EVM versions and the configurability of the EVM target.

@pcaversaccio
Copy link
Contributor

I hadn't seen #4137 but it is definitely related. Using an older Solidity version is not sustainable in the long run though, we should start getting developers used to the notion of diverging EVM versions and the configurability of the EVM target.

Agreed, what's the best way to approach this from a devex angle?

@frangio
Copy link
Contributor Author

frangio commented Aug 2, 2023

My current thinking is that tools should default to the lowest highest common denominator(*) and allow users to opt-in to more advanced EVM features if they know their target environment supports them. So the developer experience should be "it just works". When something doesn't just work, because e.g. you're using transient storage, the error messages should guide the developer to configure evmVersion once they confirm that their target environment supports the desired EVM feature.

(*): The "lowest common denominator" across all EVM chains will be extremely behind at some point so I think it should be defined relative to a specific set of L1 and L2 chains, probably tracking those with most usage and interest.

@pcaversaccio
Copy link
Contributor

On that topic, there will be soonish a good comparison website that diffs the difference: https://www.evmdiff.com. Maybe this can be incorporated (once it's prod-ready) into the error messages.

@alcuadrado
Copy link
Member

For more context: this issue was opened after we discussed this with @frangio and we agreed that the situation merits an exception to our "we don't change solc's default evm" policy. Otherwise, this can be really frustrating right now.

There's an open question regarding when we can remove that default. I don't have a clear answer right now, but I don't think we need to have one atm.

@pcaversaccio
Copy link
Contributor

There's an open question regarding when we can remove that default. I don't have a clear answer right now, but I don't think we need to have one atm.

I don't know if this makes any sense, but what if we say that we change the custom EVM version back to the solc default once the top 10 chains according to TVL adopted PUSH0?

@alcuadrado
Copy link
Member

Yeah, I think something like that probably makes sense.

@frangio
Copy link
Contributor Author

frangio commented Aug 3, 2023

I don't think it will be possible to set it back to the default because the EVM will keep evolving (e.g. TSTORE coming soon). But it shouldn't remain in paris forever, it should track the max EVM version supported by all "top 10 chains" or some similar criterion.

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Aug 3, 2023

Yeah true, the highest common denominator wrt to the supported EVM versions.

@fvictorio
Copy link
Member

I created a separate issue for defining and documenting the policy: #4264

@fvictorio
Copy link
Member

Released in v2.17.3.

@SKYBITDev3
Copy link

Released in v2.17.3

I think the reversion of default EVM version of the Solidity compiler back to paris (from shanghai, currently the latest) warranted a minor (instead of patch) version number increment (e.g. 2.18.x), as the behavior and outputs have been changed.

As this particular issue has been closed, I've created a new issue so that some suitable action could be taken: #4391

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:ready This issue is ready to be worked on type:improvement
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants