Closed Wodann closed 1 year ago
I created a reference implementation: https://github.com/bluealloy/revm/pull/299
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?
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
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?
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).
Database
has its own Error
type
https://github.com/bluealloy/revm/blob/488ef8ab62f433b1b434d2d81bc744a2db8f735f/crates/revm/src/db.rs#L22-L23
Could you have that type DatabaseError
without propagating BH
and S
?
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.
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: https://github.com/bluealloy/revm/issues/309#issuecomment-1366545273
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 🙂
I finally got around to implementing the design you proposed, @rakita: https://github.com/bluealloy/revm/pull/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
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
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.
merged here: https://github.com/bluealloy/revm/pull/334
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 aBlockchain
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?