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

imp(evm): enable EIP 3855 during upgrade #1851

Merged
merged 10 commits into from
Oct 13, 2023

Conversation

MalteHerrmann
Copy link
Contributor

@MalteHerrmann MalteHerrmann commented Oct 12, 2023

Description

This PR adds the necessary logic to enable EIP 3855 during the v14.2.0 upgrade.

More context can be found in this Medium article.


Closes ENG-2149

@linear
Copy link

linear bot commented Oct 12, 2023

ENG-2149 Update EVM Params with missing EIPs

Context

There was a user request on the Evmos Builders TG Channel who wanted to deploy a smart contract which used Solidity v0.8.20. This requires the PUSH0 opcode to be available, which is added in EIP 3855.

In the Geth version we are using at this moment (v1.10.26-evmos-rc2) this opcode is not enabled by default (there is no check for the Shanghai fork) and has to be added by populating the ExtraEIPs field in the EVM parameters.

Expected

  • Check for missing EIPs other than this described one

  • Enable them on

    • testnet
    • mainnet

Update

Two EIPs are enabled with the Shanghai instruction set:

  • EIP 3855: PUSH0 instruction
    Introduce a new instruction which pushes the constant value 0 onto the stack.
  • EIP 3860: Limit and meter initcode
    Limit the maximum size of initcode to 49152 and apply extra gas cost of 2 for every 32-byte chunk of initcode.
    → This is not supported in Geth v1.10.26 (see here)

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #1851 (143515a) into release/v14.2.x (fc2c9a2) will decrease coverage by 0.04%.
The diff coverage is 70.58%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           release/v14.2.x    #1851      +/-   ##
===================================================
- Coverage            70.50%   70.46%   -0.04%     
===================================================
  Files                  313      314       +1     
  Lines                23312    23343      +31     
===================================================
+ Hits                 16436    16449      +13     
- Misses                6052     6069      +17     
- Partials               824      825       +1     
Files Coverage Δ
app/app.go 82.77% <100.00%> (+0.01%) ⬆️
x/evm/types/params.go 72.38% <100.00%> (+1.67%) ⬆️
app/upgrades/v14_2/upgrades.go 25.00% <50.00%> (ø)

@MalteHerrmann
Copy link
Contributor Author

E2E tests have been run locally and pass 🤷

@MalteHerrmann MalteHerrmann marked this pull request as ready for review October 12, 2023 11:36
@MalteHerrmann MalteHerrmann requested a review from a team as a code owner October 12, 2023 11:36
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

Great work @MalteHerrmann!!

Left one comment

Should we add the EIP in the AvailableExtraEIPs as well?

app/upgrades/v14_2/upgrades.go Outdated Show resolved Hide resolved
@MalteHerrmann
Copy link
Contributor Author

Should we add the EIP in the AvailableExtraEIPs as well?

That slice is only used once in migration tests. I will move it directly to the migration tests and remove it from the params types. ✌️ Good catch!

Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @MalteHerrmann !!

@GAtom22
Copy link
Contributor

GAtom22 commented Oct 12, 2023

would be great to understand why is breaking the e2e tests, cause this may break them for future PRs

@MalteHerrmann
Copy link
Contributor Author

would be great to understand why is breaking the e2e tests, cause this may break them for future PRs

I agree, it's kind of hard to debug because there's not much info printed in the logs and they run fine locally 🤔 I was thinking that we should split the workflows for CLI tests and upgrade tests to be able to more easily identify which ones are failing too

app/upgrades/v14_2/upgrades.go Show resolved Hide resolved
@fedekunze
Copy link
Contributor

@MalteHerrmann why is this being pushed to the release branch and not main?

@MalteHerrmann
Copy link
Contributor Author

@MalteHerrmann why is this being pushed to the release branch and not main?

Because this is only affecting the upgrade handler which doesn't exist on main yet. This is a good point - I will backport the upgrade handler to main and then change the target of this PR to point to main as well.

@MalteHerrmann
Copy link
Contributor Author

and then change the target of this PR to point to main as well.

@fedekunze actually it will be easier to merge it like this and then backport to main

@MalteHerrmann MalteHerrmann dismissed fedekunze’s stale review October 12, 2023 16:46

Addressed all comments

x/evm/types/params.go Outdated Show resolved Hide resolved
app/upgrades/v14_2/upgrades.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Left a minor comment about the dup check approach

app/upgrades/v14_2/upgrades.go Show resolved Hide resolved
x/evm/types/params.go Show resolved Hide resolved
x/evm/types/params.go Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
@MalteHerrmann MalteHerrmann merged commit d5218c9 into release/v14.2.x Oct 13, 2023
22 of 23 checks passed
@MalteHerrmann MalteHerrmann deleted the malte/add-push0-eip branch October 13, 2023 13:34
@MalteHerrmann
Copy link
Contributor Author

@Mergifyio backport main

@mergify
Copy link
Contributor

mergify bot commented Oct 13, 2023

backport main

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 13, 2023
* enable EIP 3850 during upgrade

* adjust changelog

* address linter

* apply suggestions from code review

* move slice only used in testing to tests package

* check duplicate EIPs in validateEIPs function

* fix linter

* apply suggestions from code review

* address linter

* apply fail fast pattern

(cherry picked from commit d5218c9)
MalteHerrmann added a commit that referenced this pull request Oct 13, 2023
imp(evm): enable EIP 3855 during upgrade (#1851)

* enable EIP 3850 during upgrade

* adjust changelog

* address linter

* apply suggestions from code review

* move slice only used in testing to tests package

* check duplicate EIPs in validateEIPs function

* fix linter

* apply suggestions from code review

* address linter

* apply fail fast pattern

(cherry picked from commit d5218c9)

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
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.

None yet

3 participants