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

[Flow EVM] update EVM version to the one supporting configurable precompiles #5524

Merged
merged 15 commits into from
Mar 21, 2024

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Mar 7, 2024

Depends on :

Closes: #5512

This PR updates the Flow EVM to use a version of EVM that supports configurable precompiles.
While we are going to attempt to merge the changes to the upstream, we use an internal fork of EVM.

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.61%. Comparing base (c0b292e) to head (0f5f8f5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5524      +/-   ##
==========================================
- Coverage   55.63%   55.61%   -0.02%     
==========================================
  Files        1036     1036              
  Lines      101233   101215      -18     
==========================================
- Hits        56320    56292      -28     
- Misses      40590    40595       +5     
- Partials     4323     4328       +5     
Flag Coverage Δ
unittests 55.61% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms marked this pull request as ready for review March 7, 2024 19:55
@ramtinms ramtinms requested review from sideninja and turbolent March 7, 2024 19:56
@@ -361,3 +361,5 @@ require (
replace github.com/onflow/flow-go => ../

replace github.com/onflow/flow-go/insecure => ../insecure

replace github.com/ethereum/go-ethereum v1.13.5 => github.com/onflow/go-ethereum v0.0.2
Copy link
Contributor

Choose a reason for hiding this comment

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

should we match versions (tag onflow/go-ethereum with same tag from which it's based of, so we don't make mistakes in future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, going to update the version to a proper one after we merge the changes on the other PR

go.mod Outdated
@@ -319,3 +319,5 @@ require (

// Using custom fork until https://github.com/onflow/flow-go/issues/5338 is resolved
replace github.com/ipfs/boxo => github.com/onflow/boxo v0.0.0-20240201202436-f2477b92f483

replace github.com/ethereum/go-ethereum v1.13.5 => github.com/onflow/go-ethereum v0.0.2
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the build for anyone importing flow-go as a dependency (that didn't copy this replace). Would it make sense to just change all the imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, unfortunately, it won't work unless we replace a lot of internal references in the onflow/go-ethereum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try one more time

@ramtinms
Copy link
Contributor Author

@janezpodhostnik @sideninja updated the other repo onflow/go-ethereum#5 to use internal references, so this PR can skip the replace statement and import the forked repo directly. As soon as the other PR gets approved I'll update this PR with a proper version tag.

@@ -115,6 +115,7 @@ require (
github.com/elastic/gosigar v0.14.2 // indirect
github.com/emirpasic/gods v1.18.1 // indirect
github.com/ethereum/c-kzg-4844 v0.4.0 // indirect
github.com/ethereum/go-ethereum v1.13.10 // indirect
Copy link
Contributor

@sideninja sideninja Mar 21, 2024

Choose a reason for hiding this comment

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

why is this still used? what does go mod why say?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated the references in the EVM code base, the rest are using regular go-ethereum (e.g. RLP encoding stuff, etc)

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Perfect. I would still say it's worth matching the versions of our fork.

@ramtinms ramtinms enabled auto-merge March 21, 2024 18:17
@ramtinms ramtinms added this pull request to the merge queue Mar 21, 2024
Merged via the queue into master with commit 7af17df Mar 21, 2024
55 checks passed
@ramtinms ramtinms deleted the ramtin/5512-update-to-evm-fork branch March 21, 2024 19:15
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.

[Flow EVM] fix the possibility of concurrent Precompile setup calls
4 participants