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

chore: Patch revm to current head commit #5032

Closed
wants to merge 2 commits into from

Conversation

clabby
Copy link
Collaborator

@clabby clabby commented Oct 15, 2023

Overview

Note
WIP - waiting on a few changes to land in revm to pull out of draft and finalize.

Upgrades revm to a commit that includes bluealloy/revm#789. Necessary for moving forwards with the op-reth upgrades in #4377.

TODO

  • Fix JS tracer fault. New revm API no longer passes the eval, how do we check if the result was a failure?
  • Remove contract.rs - the file is no longer needed with create2_from_code being implemented on Address.
  • Patch crate rather than override crates.io dep.

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Merging #5032 (0922b76) into main (7e088da) will increase coverage by 3.89%.
The diff coverage is 3.06%.

Impacted file tree graph

Files Coverage Δ
crates/revm/revm-inspectors/src/access_list.rs 50.00% <100.00%> (+50.00%) ⬆️
...es/revm/revm-inspectors/src/tracing/js/bindings.rs 16.52% <0.00%> (ø)
crates/revm/revm-inspectors/src/tracing/opcount.rs 0.00% <0.00%> (ø)
crates/revm/revm-inspectors/src/tracing/types.rs 0.00% <0.00%> (ø)
crates/rpc/rpc/src/eth/api/call.rs 48.65% <50.00%> (+48.65%) ⬆️
crates/rpc/rpc/src/eth/api/transactions.rs 18.23% <0.00%> (+12.73%) ⬆️
crates/rpc/rpc/src/eth/revm_utils.rs 44.89% <0.00%> (+29.69%) ⬆️
...es/revm/revm-inspectors/src/tracing/js/builtins.rs 13.73% <0.00%> (+0.22%) ⬆️
crates/revm/revm-inspectors/src/tracing/utils.rs 0.00% <0.00%> (ø)
...s/revm/revm-inspectors/src/tracing/builder/geth.rs 0.00% <0.00%> (ø)
... and 8 more

... and 115 files with indirect coverage changes

Flag Coverage Δ
integration-tests 13.95% <3.06%> (?)
unit-tests 62.13% <0.00%> (-0.06%) ⬇️

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

Components Coverage Δ
reth binary 30.12% <ø> (+0.13%) ⬆️
blockchain tree 80.82% <ø> (ø)
pipeline 88.37% <ø> (ø)
storage (db) 73.59% <ø> (+3.28%) ⬆️
trie 94.48% <ø> (-0.04%) ⬇️
txpool 47.83% <ø> (+2.98%) ⬆️
networking 74.77% <ø> (+3.47%) ⬆️
rpc 55.61% <14.28%> (+15.32%) ⬆️
consensus 62.89% <ø> (-0.08%) ⬇️
revm 27.81% <2.46%> (+0.76%) ⬆️
payload builder 7.96% <0.00%> (+4.56%) ⬆️
primitives 84.72% <ø> (+1.61%) ⬆️

@clabby
Copy link
Collaborator Author

clabby commented Oct 15, 2023

There is also a disallowed license within the aurora-engine-modexp dependency of this version of revm that was introduced in bluealloy/revm#769. cc @rakita
image

@rakita
Copy link
Collaborator

rakita commented Oct 16, 2023

There is also a disallowed license within the aurora-engine-modexp dependency of this version of revm that was introduced in

Thanks for flagging this, there are a few CC0 licences that we use in reth, you can add it to the list here:

reth/deny.toml

Lines 60 to 70 in a60dbfd

# Allow 1 or more licenses on a per-crate basis, so that particular licenses
# aren't accepted for every possible crate as with the normal allow list
exceptions = [
# CC0 is a permissive license but somewhat unclear status for source code
# so we prefer to not have dependencies using it
# https://tldrlegal.com/license/creative-commons-cc0-1.0-universal
{ allow = ["CC0-1.0"], name = "secp256k1" },
{ allow = ["CC0-1.0"], name = "secp256k1-sys" },
{ allow = ["CC0-1.0"], name = "tiny-keccak" },
{ allow = ["CC0-1.0"], name = "more-asserts" },
]

@rakita
Copy link
Collaborator

rakita commented Oct 16, 2023

Would probably need to wait for this PR: bluealloy/revm#804 as it removed some footgun

@clabby
Copy link
Collaborator Author

clabby commented Oct 16, 2023

Would probably need to wait for this PR: bluealloy/revm#804 as it removed some footgun

Sounds good. I upgraded op-reth to use the OP changes in revm this past weekend, did find a few edge cases that I'll upstream fixes into revm for once the node is fully synced (there may be more, collecting fixes as I go). Would be great to get those in before the next release as well if possible 😄

Can work off of a patched version of revm for now to stay unblocked. Around block 2.5 million on base goerli right now.

@mattsse
Copy link
Collaborator

mattsse commented Oct 20, 2023

closing this because I need to get this over the line and I'm unable to push to this branch directly

#5109

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.

3 participants