ethereum / py-evm

A Python implementation of the Ethereum Virtual Machine
https://py-evm.readthedocs.io/en/latest/
MIT License
2.26k stars 650 forks source link

Database architecture refactor: Adapters and Wrappers #507

Open pipermerriam opened 6 years ago

pipermerriam commented 6 years ago

Replaces https://github.com/ethereum/py-evm/issues/410

What is wrong?

The current architecture uses a single global ChainDB for each chain.

This is proving problematic because:

How can it be fixed

First, we establish the following new terminology.

We also establish a new convention/rule regarding the APIs exposed by wrappers. Wrapper APIs may only be used at the same level that the wrapper is applied.

For Example:

Suppose the VM class applies the JournalDB wrapper to the database connection, and then passes the resulting object down to the VMState. The VMState is not allowed to interact with any of the APIs exposed by the JournalDB.

Now, we establish the following new Adapter classes.

So, responsibility for databases is as follows.

With this structure in place, now we explore how wrappers come into play. Each level of the call stack will pass down it's base database which might be the actual base connection, or might be a wrapped version of the base connection. Everything is to assume that the db object it receives only exposes the base database API.

In the event that a database connection needs to be wrapped, it should occur at the level at which the wrapper APIs will be used.

For example:

gsalgado commented 6 years ago

Some of the network syncing code needs to interact with the database in ways that aren't currently supported (can't recall exactly but it has to do with an assumption that transactions are available in the database).

I believe that's #337?

gsalgado commented 6 years ago

Sounds good to me, but I'd like to point out that the ChainSyncer will need to make use of all wrappers as during a fast-sync it bypasses most of the Chain/VM and just stores stuff in the database. I don't think that's a problem, but thought it was worth mentioning

pipermerriam commented 6 years ago

Sounds good to me, but I'd like to point out that the ChainSyncer will need to make use of all wrappers as during a fast-sync it bypasses most of the Chain/VM and just stores stuff in the database. I don't think that's a problem, but thought it was worth mentioning

I don't have a clear enough picture of how these will fit together, but my current assumption/hypothesis is that this will give the syncing code more flexibility and control over how it interacts with the db, either allowing it to make use of new APIs we create on the adapters that the VM uses, or that maybe the chain syncing code will be able to have it's own special adapter.

pipermerriam commented 6 years ago

@carver assigned to you merely to suggest tackling this when you start into this codebase.

carver commented 6 years ago

Just checking in to say I'm reviewing this now. It's probably going to be a little while before there's new code, because I'm still piecing everything together. The direction outlined above looks solid.

I'm also interested in seeing how we can reduce the mutability of things further -- specifics TBD.

I'm going to keep digging into designs offline with whiteboards and other contributors, and report back here with major updates.

carver commented 6 years ago

This is really heavily coupled with #599 -- it's essentially blocked on that.

pipermerriam commented 6 years ago

I really want to say that you can probably make headway building on current master but your call because I can't guarantee that. 😢