refactor: allow chain-specific configuration of Evm #1378

Wodann · 2024-05-03T19:32:21Z

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

…on table

rakita

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.

rakita · 2024-05-08T12:28:39Z

crates/precompile/Cargo.toml

-]
-negate-optimism-default-handler = [
-    "revm-primitives/negate-optimism-default-handler",
-]


Would not remove this yet, but will come back to this after i see what is done inside primitives

If you're happy with the general approach, I can also adopt this pattern for the last couple of `#[cfg(feature = "optimism")] and then we can be certain that the features are gone from other parts of the code.

rakita · 2024-05-08T12:29:58Z

crates/precompile/src/lib.rs

-    pub const fn from_spec_id(spec_id: revm_primitives::SpecId) -> Self {
-        use revm_primitives::SpecId::*;
+    pub const fn from_spec_id(spec_id: revm_primitives::EthSpecId) -> Self {
+        use revm_primitives::EthSpecId::*;


EthSpecId would revert back the name to SpecId. We don't need to specify that it is Eth and it makes too much changes in this PR.

Resolved in c99bc9a

rakita · 2024-05-08T12:31:32Z

crates/primitives/src/chain_spec.rs

+
+use core::{fmt::Debug, hash::Hash};
+
+pub trait ChainSpec: Clone + Copy + Debug + Sized + 'static {


Would remove Copy so that we are more explicit with clone.

Resolved by a09af7f

rakita · 2024-05-08T12:42:42Z

crates/primitives/src/chain_spec.rs

+            type HaltReason: Clone + Debug + PartialEq + Eq + Hash + From<crate::HaltReason> + serde::de::DeserializeOwned + serde::Serialize;
+        } else {
+            /// The type that enumerates chain-specific halt reasons.
+            type HaltReason: Clone + Debug + PartialEq + Eq + Hash + From<crate::HaltReason>;


So with this, we are still having HaltReason. Need to check this but Shouldn't we have From<InstructionResult> here

This line handles the conversion from InstructionResult to type HaltReason thanks to the From<crate::HaltReason>

rakita · 2024-05-08T12:44:08Z

crates/primitives/src/chain_spec.rs

+}
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct MainnetChainSpec;


To nit here, it is not exactly MainnetChainSpec as it works on all testnets, but EthChainSpec.

Resolved by 565eaa9

rakita · 2024-05-08T12:54:40Z

crates/primitives/src/result.rs


 /// Generic result of EVM execution. Used to represent error and generic output.
 pub type EVMResultGeneric<T, DBError> = core::result::Result<T, EVMError<DBError>>;

 #[derive(Debug, Clone, PartialEq, Eq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-pub struct ResultAndState {
+pub struct ResultAndState<ChainSpecT: ChainSpec> {


It seems unnecessary to tie the Result to the ChainSpec as it only needs the HaltReason.

This brings up all cast functions that do not have a great purpose.

The cast functions were a remnant from another attempt to achieve this. I've removed them in 26dec59.

W.r.t using the generic ChainSpec or HaltReason; up to you. I chose HaltReason in case some hardforks introduce custom SuccessReasons down the line.

rakita · 2024-05-08T12:59:12Z

crates/revm/benches/bench.rs

+#[cfg(feature = "optimism")]
+type BenchChainSpec = revm::optimism::OptimismChainSpec;
+#[cfg(not(feature = "optimism"))]
+type BenchChainSpec = revm::MainnetChainSpec;


This is okay to be left just Mainnet

Resolved by 53eb5b4

rakita · 2024-05-08T13:01:25Z

crates/revm/src/builder.rs

@@ -28,61 +29,68 @@ pub struct SetGenericStage;
 /// Requires the database and external context to be set.
 pub struct HandlerStage;

-impl<'a> Default for EvmBuilder<'a, SetGenericStage, (), EmptyDB> {
+impl<'a, ChainSpecT: ChainSpec> Default


This should be default, so variant of ChainSpec should be set, MainnetChainSpec probably

Resolved by 5f87d2f

rakita · 2024-05-08T13:06:59Z

crates/revm/src/builder.rs

+impl<'a, ChainSpecT: ChainSpec, EXT, DB: Database>
+    EvmBuilder<'a, SetGenericStage, ChainSpecT, EXT, DB>
+{
+    pub fn with_chain_spec<NewChainSpecT: ChainSpec>(


Resolved by 9d9e3e4

rakita · 2024-05-08T13:15:35Z

crates/revm/src/builder.rs

+        EvmBuilder {
+            context: self.context,
+            handler: EvmBuilder::<'_, SetGenericStage, NewChainSpecT, _, _>::handler(
+                NewChainSpecT::Hardfork::default(),


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.

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

Wodann · 2024-05-21T21:47:26Z

crates/revm/src/custom_upcode.rs

@@ -0,0 +1,253 @@
+use revm_interpreter::{


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

Wodann · 2024-05-24T17:56:42Z

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 · 2024-05-24T17:57:45Z

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

Wodann · 2024-05-24T17:58:40Z

crates/revm/src/optimism/env.rs

+    /// L1 block info used to compute the L1-cost fee.
+    ///
+    /// Needs to be provided for Optimism non-deposit transactions.
+    l1_block_info: Option<L1BlockInfo>,


@rakita my understanding from our earlier conversation was that you wanted me to add the l1_block_info to the chain-specific block environment.

Wodann · 2024-05-24T18:01:09Z

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 ?

Wodann added 3 commits May 1, 2024 22:00

refactor: split SpecId into two separate enums

af4c176

WIP

cc7b46d

refactor: allow chain-specific configuration of Evm

45c9517

Wodann force-pushed the refactor/opt-spec-id branch from 0e87f0b to 45c9517 Compare May 6, 2024 21:12

research: show that we can use the Spec to implement custom instructi…

c8d1ff1

…on table

Wodann mentioned this pull request May 7, 2024

Can we replay an on-chain optimism transaction using revm? NomicFoundation/edr#377

Closed

fix: don't include the custom_opcode.rs file

cc64e26

rakita reviewed May 20, 2024

View reviewed changes

Wodann added 7 commits May 21, 2024 21:26

refactor: rename EthSpecId to SpecId

c99bc9a

refactor: rename MainnetChainSpec to EthChainSpec

565eaa9

misc: always use EthChainSpec for benchmarks

53eb5b4

docs: add documentation to with_chain_spec

9d9e3e4

misc: always use EthChainSpec for EvmBuilder::default()

5f87d2f

misc: remove Copy trait from ChainSpec

a09af7f

misc: remove old cast functions

26dec59

Wodann commented May 21, 2024

View reviewed changes

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 added 3 commits May 22, 2024 23:34

refactor: decouple optimism-specific InvalidTransaction variants

ee40e3f

refactor: decouple optimism-specific transaction fields

bfb553d

refactor: decouple L1 block info from InnerEvmContext

48d55ef

Wodann commented May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: allow chain-specific configuration of Evm #1378

refactor: allow chain-specific configuration of Evm #1378

Wodann commented May 3, 2024

rakita left a comment

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024 •

edited

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

rakita May 8, 2024

Wodann May 21, 2024

Wodann May 21, 2024

Wodann commented May 24, 2024 •

edited

Wodann commented May 24, 2024

Wodann May 24, 2024

Wodann commented May 24, 2024


		use core::{fmt::Debug, hash::Hash};

		pub trait ChainSpec: Clone + Copy + Debug + Sized + 'static {

refactor: allow chain-specific configuration of Evm #1378

Are you sure you want to change the base?

refactor: allow chain-specific configuration of Evm #1378

Conversation

Wodann commented May 3, 2024

rakita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wodann May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wodann commented May 24, 2024 • edited

Wodann commented May 24, 2024

Choose a reason for hiding this comment

Wodann commented May 24, 2024

Wodann May 21, 2024 •

edited

Wodann commented May 24, 2024 •

edited