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.

Sign up for GitHub

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

Jump to bottom

refactor: allow chain-specific configuration of Evm #1378

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

refactor: allow chain-specific configuration of Evm #1378

wants to merge 11 commits into from

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented May 3, 2024

Experiment to see whether it would be possible to separate optimism and mainnet SpecIds and other types, such as HaltReason.

I'm curious to hear whether any of this is in line with #918

@Wodann Wodann mentioned this pull request May 7, 2024
Closed
rakita
rakita reviewed May 20, 2024
View reviewed changes
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

I have started reviewing this last weeks, but didnt have time to finish it whole and getter better grasp on it.

I general like it, change is needed but would like to minimise renames to get smaller PR.

As this is potentially a breaking change will need look at reth code and think how this impacts them as we are in the middle of devnet for Prague.

crates/precompile/Cargo.toml Show resolved Hide resolved
crates/precompile/src/lib.rs Outdated Show resolved Hide resolved
crates/primitives/src/chain_spec.rs Outdated Show resolved Hide resolved
crates/primitives/src/chain_spec.rs Outdated Show resolved Hide resolved
crates/primitives/src/chain_spec.rs Outdated Show resolved Hide resolved
crates/primitives/src/result.rs Show resolved Hide resolved
crates/revm/benches/bench.rs Outdated Show resolved Hide resolved
crates/revm/src/builder.rs Outdated Show resolved Hide resolved
crates/revm/src/builder.rs Outdated Show resolved Hide resolved
crates/revm/src/builder.rs
EvmBuilder {
context: self.context,
handler: EvmBuilder::<'_, SetGenericStage, NewChainSpecT, _, _>::handler(
NewChainSpecT::Hardfork::default(),
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting to note, we now have ChainSpec and Hardfork as two separate entities. ChainSpec is here to define potential hardforks and hardfork type is here so it can be checked against ChainSpec.

We could have a trait Hardfork that will have a fn is_enabling() function. Would come back to this, those two should be split as ChainSpec is something that is going to grow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something you want this PR to cover or a separate one?

Copy link
Member

Choose a reason for hiding this comment

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

We can do that later. PR direction in general looks great!

Wodann
Wodann commented May 21, 2024
View reviewed changes
crates/revm/src/custom_upcode.rs
@@ -0,0 +1,253 @@
use revm_interpreter::{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file merely serves to illustrate that we can support custom opcodes. It should be removed before merging.

Copy link
Member

Choose a reason for hiding this comment

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

We can move it to examples later, fine with leaving it here for now.

@Wodann Wodann linked an issue May 22, 2024 that may be closed by this pull request
Refactor REVM to make optimism feature flag additive NomicFoundation/edr#464
Open
@Wodann
Copy link
Contributor Author

Wodann commented May 24, 2024 •
edited

Given that the general approach seemed to be the right direction, I removed the last instances of [cfg(feature = "optimism")] from the codebase by adding new types to the trait ChainSpec for variability of transaction and block environments.

Please let me know what you think. You should be able to review those changes commit-by-commit

@Wodann
Copy link
Contributor Author

Wodann commented May 24, 2024

I'll hold off on resolving merge conflicts until we're happy with the full design, if that's okay.

@Wodann
Copy link
Contributor Author

Wodann commented May 24, 2024

There are still some EVMError::Custom invocations in handler_register.rs. My understanding is that we could catch those when validating the environment.

That way we could use chain-specific errors static errors instead of strings. Alternatively, we could also add a custom error type to the trait ChainSpec.

That would allow us to remove the EVMError::Custom(String)

WDYT, @rakita ?

@rakita
Copy link
Member

rakita commented Jun 17, 2024

I really like this path. For organizational purposes, lets merge this into chain_spec branch so we can work on it and make PR there, and after we are fine with the design, we can think about rebasing it on the main.

I'll hold off on resolving merge conflicts until we're happy with the full design, if that's okay.

This is perfectly fine.

There are still some EVMError::Custom invocations in handler_register.rs. My understanding is that we could catch those when validating the environment.

That way we could use chain-specific errors static errors instead of strings. Alternatively, we could also add a custom error type to the trait ChainSpec.

That would allow us to remove the EVMError::Custom(String)

I would leave it as String for now, PR is getting big and we can add this later.

@rakita
Copy link
Member

rakita commented Jun 17, 2024

I made a PR that refactors few things #1528 I couldn't make it in nomicfoundation/revm didn't get the option for target repo to make a PR on.

I am still in investigation phase, but this looks great!

@Wodann
Copy link
Contributor Author

Wodann commented Jun 17, 2024 •
edited

I made a PR that refactors few things #1528 I couldn't make it in nomicfoundation/revm didn't get the option for target repo to make a PR on.

I am still in investigation phase, but this looks great!

Awesome! Is there anything you'd like me to do?

I added your commits to this PR, squashed, and rebased on main

@Wodann Wodann marked this pull request as ready for review June 17, 2024 20:00
@Wodann Wodann force-pushed the refactor/opt-spec-id branch 4 times, most recently from 637c088 to f9b2d03 Compare June 17, 2024 22:55
@rakita
Copy link
Member

rakita commented Jun 18, 2024

I made a PR that refactors few things #1528 I couldn't make it in nomicfoundation/revm didn't get the option for target repo to make a PR on.
I am still in investigation phase, but this looks great!

Awesome! Is there anything you'd like me to do?

I dont have any action items rn, but I will ping you if something comes up.
PR is in good state, but it is a massive change in both the code and the design, so I want to be sure that it is okay.

I added your commits to this PR, squashed, and rebased on main

Great!

Wodann
Wodann commented Jun 19, 2024
View reviewed changes
crates/primitives/src/result.rs
@@ -16,7 +16,6 @@ pub type EVMResultGeneric<T, ChainSpecT, DBError> = core::result::Result<
>;

#[derive(Debug, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing serde (de)serialisation allowed the removal of the serde-related trait bounds from the HaltReasonTrait. As the ExecutionResult and ResultAndState aren't part of the JSON-RPC of Ethereum, I think it should be safe to remove this.

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rakita rakita rakita left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor REVM to make optimism feature flag additive
2 participants
@Wodann @rakita