Can we split block_hash into a separate Blockchain trait?

[Wodann]

In our implementation of Project Rethnet for Hardhat, we're running into an issue because we have separate data models for the blockchain (blocks) and state management (accounts, account storage, code) when implementing the Database[Ref] traits.

A possible solution would be to separate that trait into two traits: Database[Ref] for state queries and a Blockchain trait that directly deals with the blockchain for retrieving block hashes by their block number`.

Is this something that you think would make sense for revm's internals?

If so, would you want to have two separate containers for caching internally?

[Wodann]

I created a reference implementation: #299

[gakonst]

Seems fine as a solution, but just to ask the question: Why not write a struct Wrapper which is impl Database that abstracts over your 2 databases? Instead of adding a new generic like #299?

[Wodann]

The necessity for a wrapper type makes it such that you need to introduce thread-safe synchronisation patterns for the wrapped objects, thus limiting the individual access of two otherwise completely disparate data models

[rakita]

For history, copying my response from telegram:

Yeah, they are saparate things, it didnt felt like it is a big of a deal as you can always have a wrapper around Database that takes two traits. Idea for database is to have only one trait that you need to implement to bind the outside of revm, spliting it in two is maybe the way

So lets do this. We can have those two traits BlockHash and State for example, that is requirements that you have/need and it does make sense.

And have DatabaseComponents<BH,S> that would take those two and implement Database trait, revm would take only Database and you would have separate components. How does this sound to you?

[Wodann]

The only potential downside of combining the two traits into a wrapper trait Database is that end-users won't be able to have separate error types for BlockHash vs State. I guess one might work around that by adding a:

pub enum DatabaseError<BH, S> {
  BlockHashError(A),
  StateError(S),
}

but then we're back to the situation where both the BH & S generics need to be passed to all functions & structs, which I assume you're trying to avoid?

My reference implementation still uses the same error type between the two, but that's only because we internally store the error (and I didn't want to do the extra work of changing that before we agreed).

[rakita]

Database has its own Error type

revm/crates/revm/src/db.rs

Lines 22 to 23 in 488ef8a

	pub trait Database {
	type Error;

Could you have that type DatabaseError without propagating BH and S?

[Wodann]

If you want the two traits to allow for separate error types but only pass one trait to the EVM, you'd need two types on the Database trait:

trait Database {
  type StateError;
  type BlockchainError;

  ...
}

At that point it's starting to feel like code duplication to me, though.

[rakita]

pub struct DatabaseComponents<BH:BlockHash,S:State> {
   block_hash: BH,
   state: S,
}

pub enum ComponentError<BHE, SE> {
  BlockHashError(BHE),
  StateError(SE),
}

impl<S:State, BH: BlockHash> Database for DatabaseComponents<S,BH> {
    type Error = ComponentError<S::Error, BH::Error>
    
    ...
    // wrap component error to `ComponentError` type when is triggered inside fn
}

ComponentError aggregates it and when that error is returned you can extract it back if needed.

Note: I have just noticed that db error is not even returned, it is set when something happens inside EvmData but it is not returned back. Added comment here: #309 (comment)

[Wodann]

Note: I have just noticed that db error is not even returned, it is set when something happens inside EvmData but it is not returned back. Added comment here

Good catch!

ComponentError aggregates it and when that error is returned you can extract it back if needed.

Yeah, implementing it like that would indeed avoid having to have two generics on the EVM, so if you prefer adding the wrapper instead, feel free to. In the end the decision is yours 🙂

[Wodann]

I finally got around to implementing the design you proposed, @rakita: #324

It had some difficulties that I needed to work around to avoid duplication of error types and trait functions. I'm curious what you think

[rakita]

Here is database component idea: https://github.com/bluealloy/revm/blob/primitives/crates/revm/src/db/components.rs

will move it to revm-primitives with Database trait

[Wodann]

I see you opted for code duplication between the Database and State/BlockHash traits. My fear was that not using the super-trait approach would leave room for them to go out-of-sync.

[rakita]

merged here: #334