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

feat: introducing EvmWiring, a chain-specific configuration #1672

Merged
merged 64 commits into from
Sep 9, 2024

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented Jul 29, 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

Supercedes #1378 (to allow editing by maintainers)

@Wodann
Copy link
Contributor Author

Wodann commented Sep 6, 2024

@rakita if at any point it is helpful, I'd be happy to upgrade EDR to test the latest changes in our use case 🙂

@rakita
Copy link
Member

rakita commented Sep 6, 2024

@rakita if at any point it is helpful, I'd be happy to upgrade EDR to test the latest changes in our use case 🙂

Changes are mostly done and want to merge it as is. EvmBuilder it not great but would work on it in follow up PRs.

        let mut evm = Evm::<EthereumWiring<BenchmarkDB, ()>>::builder()
            .with_spec_id(SpecId::PRAGUE)
            .with_db(BenchmarkDB::new_bytecode(bytecode))
            .with_default_ext_ctx()

with_default_ext_ctx is required to build external ctx, if you build Evm without setting ext_ctx, build will panic.

Try to integrate and we can iterate. Reth has a simpler abstraction that creates Evm behind the trait so integration is easier.

@rakita rakita merged commit 077c2c3 into bluealloy:main Sep 9, 2024
27 checks passed
@Wodann
Copy link
Contributor Author

Wodann commented Sep 9, 2024

Awesome; I'll integrate today.

@@ -182,8 +182,7 @@ impl fmt::Display for EofDecodeError {
}
}

#[cfg(feature = "std")]
impl std::error::Error for EofDecodeError {}
impl core::error::Error for EofDecodeError {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@github-actions github-actions bot mentioned this pull request Sep 9, 2024
@Wodann
Copy link
Contributor Author

Wodann commented Sep 13, 2024

Awesome; I'll integrate today.

I've been working on integrating the added changes into EDR.

The addition of the type Database and type ExternalContext to EvmWiring have been proving incredibly hard (and maybe impossible) to integrate into our codebase.

We are using references to a dynamic trait object (runtime polymorphic) State and BlockHashRef for the database. As these now need to be added to the EthereumWiring, that's polluting the whole codebase with lifetimes, whereas the EvmWiring is intended to be used as a compile-time constant.

I was able to work around this partially by splitting the EvmWiring into two traits but I'm still running into issues with runtime arguments.

For example:

    let env = Env::boxed(cfg, block, transaction);
    let evm_builder = Evm::builder().with_db(WrapDatabaseRef(DatabaseComponents {
        state,
        block_hash: blockchain,
    }));

    // NOTE: Precompiles don't have access to the external context but are still typed on the `type ExternalContext` due
    // to the `EvmWiring`. This requires duplication of this code into the two branches of the if-else statements below.
    // This seems overly restrictive on the typing.
    let precompiles: HashMap<Address, ContextPrecompile<Wiring<ChainSpecT, _, ()>>> =
        custom_precompiles
            .iter()
            .map(|(address, precompile)| (*address, ContextPrecompile::from(precompile.clone())))
            .collect();

    let result = if let Some(debug_context) = debug_context {
        let mut evm = evm_builder
            .with_external_context(debug_context.data)
            .with_env(env)
            .append_handler_register(debug_context.register_handles_fn)
            .append_handler_register_box(Box::new(move |handler| {
                register_precompiles_handles(handler, precompiles.clone());
            }))
            .build();

        evm.transact_commit()
    } else {
        let mut evm = evm_builder
            .with_env(env)
            .append_handler_register_box(Box::new(move |handler| {
                register_precompiles_handles(handler, precompiles.clone());
            }))
            .build();

        evm.transact_commit()
    }?;

To keep code as reusable as possible, I think it would be beneficial to restrict trait bounds by the smallest amount as possible.

I'm still in the process of adapting these changes, so I'll share more insights as I uncover them.

@github-actions github-actions bot mentioned this pull request Sep 16, 2024
@rakita
Copy link
Member

rakita commented Sep 16, 2024

@Wodann can you show me an example, and we can start with that?

@Wodann
Copy link
Contributor Author

Wodann commented Sep 16, 2024

For multi-chain support, most of our codebase is generalised on the trait ChainSpec (old code). One such example is a trait Block, which exposes information about an Ethereum block. It's variable for the ChainSpec::Transaction and block receipt.

Introducing the EvmWiring would require this trait to also be generic over the Database and Ext, whereas a Block has nothing to do with these types.

Some other examples are:

For an example of how we use the wiring, see this code. We construct a state using runtime parameters here (old code) and here (new code). This is using immutable references, which would be intertwined with the trait EvmWiring in our codebase.

I still haven't gotten the compiler to accept the code, as we need to construct an EvmWiring from a ChainSpec (trait that doesn't know about Database and ExternalContext. I have a feeling that the only way to get this to work is using:

trait EvmWiring {
  type ChainSpec: ChainSpec;
  type Database: Database;
  type ExternalContext;
}

but I am yet to try that. I'll circle when I've given it a try.

@rakita
Copy link
Member

rakita commented Sep 17, 2024

Why would you want to restrict Block to all those generics:

#[auto_impl(Arc)]
pub trait Block<ChainSpecT: EvmSpec>: Debug {

It Looks brittle. I would try with associated types (for Tx) on Block, maybe this is a good path.

@Wodann
Copy link
Contributor Author

Wodann commented Sep 19, 2024

Why would you want to restrict Block to all those generics:

#[auto_impl(Arc)]
pub trait Block<ChainSpecT: EvmSpec>: Debug {

It Looks brittle. I would try with associated types (for Tx) on Block, maybe this is a good path.

Sure, in this case we that might simplify things.

However, there are some places where we need all or large parts of the RuntimeSpec, which is a constrained version (additional trait bounds) of ChainSpec (not the Database and ExternalContext).

For the exact reason you mention, we don't want to pollute our codebase with the ExternalContext and Database associated types, especially since those are not types with a 'static lifetime - in our use case.

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