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

refactor: decouple EVMData from Inspector trait #456

Closed

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented Apr 12, 2023

Currently, the Inspector trait has a Database generic that imposes its lifetime constraints onto the Inspector. This would prohibit user-provided Inspector implementations from being used in async contexts as when the Database implementation is using references, as lifetimes need to be 'static in async contexts.

E.g.

fn build_evm<'s, 'b, E>(
  some_state: &'s dyn StateRef<E>,
  some_blockhash: &'b dyn BlockHashRef<E
) -> EVM<DatabaseComponents<&'s dyn StateRef<E>, &'b dyn BlockHashRef<E>> {
  let mut evm = revm::EVM::new();
  evm.database(DatabaseComponents {
    some_state,
    some_blockhash,
  });
  
  evm
}

fn run_transaction(
  evm: EVM<DatabaseComponents<&'s dyn StateRef<E>, &'b dyn BlockHashRef<E>>,
  inspector: &mut dyn Inspector<DatabaseComponents<&'s dyn StateRef<E>, &'b dyn BlockHashRef<E>>>
) {
  evm.inspect_ref(inspector)
}

The above example illustrates how the lifetimes imposed on the database bleed into the inspector. It is however impossible for a user to create a dynamic object with the lifetime constraints mentioned on the inspector's bounds.

This PR decouples the Database from the Inspector by using a strategy pattern: a trait EVMData<E> that returns a &mut dyn Database<E>

NOTE: This is a breaking change to the existing public API

@Wodann Wodann force-pushed the refactor/decouple-inspector-evm-data branch from c67e9d4 to 5b6d110 Compare April 12, 2023 19:49
@rakita
Copy link
Member

rakita commented Apr 14, 2023

I think this is the right approach, but I intend to make more breaking changes to the inspector as I would like to have opcode aware inspections ( as in hook to only specified opcodes, or add new opcodes).

I am still not exactly sure how this would look like, it would need to be drop in replacement for current inspector trait, but it would be best to do all of this changes at once to minimize breaking versions.

@Wodann
Copy link
Contributor Author

Wodann commented Apr 14, 2023

If you'd like, I'm more than happy to experiment with and give feedback on different API designs you have 🙂

Do you have a rough estimate for when you plan on incorporating this?

@rakita
Copy link
Member

rakita commented May 4, 2023

If you'd like, I'm more than happy to experiment with and give feedback on different API designs you have 🙂

Do you have a rough estimate for when you plan on incorporating this?

I don't have any timeline but the idea is kinda broader and requires replacing the interpreter dispatching of opcodes, but i need to experiment a little with that.

@Wodann Wodann force-pushed the refactor/decouple-inspector-evm-data branch 2 times, most recently from c773409 to 9fdf446 Compare June 16, 2023 17:58
@Wodann
Copy link
Contributor Author

Wodann commented Aug 24, 2023

Hi @rakita. We'd like to include this in our release. Is there a chance of getting it in before a next (breaking) release of revm?

@Wodann Wodann force-pushed the refactor/decouple-inspector-evm-data branch from 9fdf446 to 19df2ba Compare August 24, 2023 15:26
@Wodann
Copy link
Contributor Author

Wodann commented Aug 24, 2023

Rebased to resolve conflicts

@Wodann Wodann force-pushed the refactor/decouple-inspector-evm-data branch from 19df2ba to f14e47a Compare September 26, 2023 13:59
@Wodann Wodann force-pushed the refactor/decouple-inspector-evm-data branch from af48dcf to f14e47a Compare October 6, 2023 16:00
@rakita
Copy link
Member

rakita commented Dec 5, 2023

This could be done inside Handler and EvmBuilder as it has a lot more control: #888
PR is still wip

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.

2 participants