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

Cannot use database by reference when registering handlers #1144

Closed
Wodann opened this issue Mar 2, 2024 · 8 comments
Closed

Cannot use database by reference when registering handlers #1144

Wodann opened this issue Mar 2, 2024 · 8 comments
Labels

Comments

@Wodann
Copy link
Contributor

Wodann commented Mar 2, 2024

Similar to previous versions of the revm that used Inspectors, the new implementation that uses handlers couples the lifetime of the database with the HandleRegister functions.

I proposed a fix for previous versions of revm in this PR.

This commit shows how to produce irresolvable compile errors with revm v6.1.0.

The only way I can think of to resolve this issue is by removing the DB generic from the HandleRegister and EvmContext by providing access to the database using a &mut dyn Database<Error = DBErrorT>, as DBErrorT should minimise the chances of lifetime conflicts. This would decouple the user's type from the Evm and avoid lifetime issues.

@rakita
Copy link
Member

rakita commented Mar 2, 2024

Every reference to db would make this unusable as the handler register could potentially make additional references.

This is not the case if we use generic over DatabaseRef as the compiler doesn't know if it is a reference or not.

Made PR here: Wodann#4
Additionally, removed some lifetimes from the handler register, as they didn't do anything.

@rakita rakita added the question label Mar 2, 2024
@rakita
Copy link
Member

rakita commented Mar 2, 2024

btw would definitely want to include this example inside repo.

@Wodann
Copy link
Contributor Author

Wodann commented Mar 4, 2024

Thanks for the fix. I've created a PR with the cleaned up example: #1150

@Wodann
Copy link
Contributor Author

Wodann commented Mar 4, 2024

@rakita At first your proposed solution of using trait generics to avoid the lifetime constrained seemed to work. Every time I applied this solution, the errors bubbled up one layer. Each time I was able to design a workaround (albeit not very elegant), until I ran into this issue:

NomicFoundation/hardhat@ef9240a#r139362460

This shows that in most cases, the compiler is able to generate some form of code that adheres to lifetime constraints, but the EvmContext's lifetime is still tied to the the lifetime of the database, imposing unnecessary constraints on the the user of the API.

I tried creating another minimal example for you to look at but given that we're many layers into the EDR code, it's not a trivial effort. I'm happy to try and explain the use case to you in more detail, but at a high level it looks like this:

  • Instead of a Database, we use DatabaseComponents with a split state and blockchain
  • fn mine_block: mines a block using the provided blockchain and mutable state: Box<dyn SyncState<StateErrorT>>. This function specifically requires ownership of a Box, as that's returned and inserted into the blockchain later on, to ensure immutable operation.
  • SyncState is a super-trait that implements StateRef, DatabaseCommit, and StateDebug (a trait defined in EDR for generating a state root, among others)
  • BlockBuilder is used by the mine_block function to construct a valid Eth block. The fn BlockBuilder::add_transaction is used to add individual transactions. This was converted to your proposed design using trait generics for the blockchain and state.

I might be doing something wrong, but it feels like the inclusion of the DB generic in the EvmContext is imposing restrictions on the API that could be circumvented using my previously proposed alternative:

The only way I can think of to resolve this issue is by removing the DB generic from the HandleRegister and EvmContext by providing access to the database using a &mut dyn Database<Error = DBErrorT>, as DBErrorT should minimise the chances of lifetime conflicts. This would decouple the user's type from the Evm and avoid lifetime issues.

@Wodann
Copy link
Contributor Author

Wodann commented Mar 5, 2024

I was able to work around the lifetime issue by moving the data into the Evm and then retrieving it at the end of the usage, instead of using references. See NomicFoundation/hardhat@5a0c555

The downside of this is that it pollutes a lot of APIs, which is why I wanted to pass in the state as a reference.

@rakita
Copy link
Member

rakita commented Mar 5, 2024

I was able to work around the lifetime issue by moving the data into the Evm and then retrieving it at the end of the usage, instead of using references. See NomicFoundation/hardhat@5a0c555

The downside of this is that it pollutes a lot of APIs, which is why I wanted to pass in the state as a reference.

Yeah if you have any reference it will get "potentially" tied to the handler. as ref signature allows that.

This could work to send it back and forth.

I wonder if doing something like trait GetDB { get_db(&self) -> &mut dyn Database} would work better.

@Wodann
Copy link
Contributor Author

Wodann commented Mar 5, 2024

I wonder if doing something like trait GetDB { get_db(&self) -> &mut dyn Database} would work better.

Do you mean instead of storing the Database inside the Evm? That would solve it, yes.

@rakita
Copy link
Member

rakita commented Mar 12, 2024

I wonder if doing something like trait GetDB { get_db(&self) -> &mut dyn Database} would work better.

Do you mean instead of storing the Database inside the Evm? That would solve it, yes.

API is restrictive but allows reusage of handlers boxes. For a solution, I would recommend the same thing that you did, return the database or Context back from Evm and create Evm only when you need it. Or introduce Database that has inner mutability.

&mut dyn Database<Error = DBErrorT> is not going to help as it introduces lifetime and additionally erasing the DB generic with dyn is not something that we want.

Will close this issue for now, as there is nothing to be done.

@rakita rakita closed this as completed Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants