Define a Blockchain interface

[alcuadrado]

As I commented multiple times before, I'm working on an EIP1193-compatible provider backed by ethereumjs-vm for Buidler.

When testing it against real-world smart contract test suites, I profiled it and discovered that ethereumjs-blockchain was taking ~30% of the time, making my provider waaaay slower than running the tests against Ganache. I starting investigating and discovered that Ganache doesn't use ethereumjs-blockchain. This led me to think that it was too slow for that task.

I eventually discovered that ethereumjs-blockchain is not that slow. It still serializes and deserializes blocks much more times than needed for my use-case, but the root of the problem was that I had an O(n) algorithm in my code that was calling getBlock lots and lots of times 😓

Despite fixing my issue, I kept thinking about this, and I'm now convinced that neither this library nor ethereumjs-block should use ethereumjs-blockchain directly, but should let the user choose their blockchain implementation. This is complicated now, as the Blockchain type is used in multiple places. This can still be done by hacking the type system with an any or extending ethereumjs-blockchain, but that's far from ideal.

This PR creates a Blockchain interface that includes just the necessary functions for this library and ethereumjs-block to work.

It also defines a MockBlockchain that is used if no blockchain is provided to the VM constructor. I guess this was the reason to have a FakeBlockchain before.

Finally, if we do this, ethereumjs-blockchain can be a dev dependency. This will lead to faster installations for users of this module that choose not to use ethereumjs-blockchain.

What do you think @s1na and @holgerd77?

I also think that similar reasoning may apply to the StateManager. If we define it as an interface, users of this library can plug-in their own implementations easily.

[s1na]

Some general questions:

1. Having an explicit interface would be cleaner. I was wondering however if it's functionally any different than sub-classing the ethereumjs-blockchain class? that could also be passed to the VM right?
2. Should the interface be defined here or in ethereumjs-blockchain? Not sure what's the TS convention on this. E.g. in Go, typically consumers define interfaces, but in TS you should say class X implements I, and it wouldn't be very clean to import ethereumjs-vm in ethereumjs-blockchain to say class Blockchain imports BlockchainInterface.
3. I'd still instantiate the VM with a ethereumjs-blockchain by default, because I assume that's what most users would want to use. I expect only some would want to provide their own implementations.

[alcuadrado]

1. Having an explicit interface would be cleaner. I was wondering however if it's functionally any different than sub-classing the ethereumjs-blockchain class? that could also be passed to the VM right?

Yes, that could be passed. The problem with this approach is that it's not clear what to override. The only safe way is to override everything, which is not ideal.

Using a class as an interface is also possible in TS. You could do class MyBlockchain implements Blockchain, but it has the same issues.

2. but in TS you should say class X implements I

You don't need that, as TS has structural typing, like Go. But it's safer to do it, as the compiler can ensure your implementation matches the interface.

and it wouldn't be very clean to import ethereumjs-vm in ethereumjs-blockchain to say class Blockchain imports BlockchainInterface.

That's true, so I guess it should be placed in ethereumjs-blockchain.

3. I'd still instantiate the VM with a ethereumjs-blockchain by default, because I assume that's what most users would want to use. I expect only some would want to provide their own implementations.

I'm not sure about this, as not long ago the default implementation was FakeBlockchain. But if we define the interface in ethereumjs-blockchain we will need it as a dependency, not dev dependency, so using it by default doesn't have any impact.

[holgerd77]

Yes, I would also very much say that this should be defined in ethereumjs-blockchain.

[holgerd77]

I would generally have a tendency to move further bigger additions to the VM like this one (depending on how it will be finally implemented) to be targeted for a v4.1.0 release, so that we start to tie things together and that we don't fritter away to much, what do you think? I think we should have some kind of feature freeze at some point. No reason to not have a relatively closely following v4.1.0 version if it makes sense.

For a second beta I am currently not sure if we should rather wait for the block TS transition - seems not too far away that this will be merged - and include that as well, if you think otherwise please ping me.

[alcuadrado]

I would generally have a tendency to move further bigger additions to the VM like this one (depending on how it will be finally implemented) to be targeted for a v4.1.0 release, so that we start to tie things together and that we don't fritter away to much, what do you think?

I completely agree. This doesn't need to be included in v4, especially if we define the interface in ethereumjs-blockchain.

For a second beta I am currently not sure if we should rather wait for the block TS transition - seems not too far away that this will be merged - and include that as well, if you think otherwise please ping me.

I think it -block migration should be ready today / early tomorrow. But in order to integrate it here, we have to do it in -blockchain and -vm, as we introduced some breaking changes during the ts migration.

[holgerd77]

@s1na is the patricia tree TS update something we also want to take into the first v4 release (this would very much complete everything) or is this too early and should we give this a bit more time and also release with v4.1?

[s1na]

is the patricia tree TS update something we also want to take into the first v4 release (this would very much complete everything) or is this too early and should we give this a bit more time and also release with v4.1?

Well, it would be nice if that were included, because currently the whole StateManager vs PStateManager is a bit hacky. But I'm not sure when I could have that ready (with promisification, etc.).

Generally one thing we can do to make decisions about versions easier is to try and have somewhat regular releases. Whatever makes it by the deadline goes into the release, everything else remains for the next one. Kinda like how the hardforks are being planned now. Not sure what you think of that @holgerd77?

[holgerd77]

@s1na feel that it's too static and partially too much of a burden, since we are just a handful of people and often concentrate over some time on just 2-3 libraries for concurrent work. I would rather stick to the current concept. It's currently on the VM a bit lofty during this beta releases period, but otherwise it works reasonably well I would say.

[evertonfraga]

I eventually discovered that ethereumjs-blockchain is not that slow. It still serializes and deserializes blocks much more times than needed for my use-case.

Totally! This was one of my main takeaways while integrating Block v3.0.0 as well.

I like the idea of making the in-memory/mock version the default and users define alternative implementations.

One practical implication of having it defined at ethereumjs-blockchain is that I would definitely want to use this mock on ethereumjs-block tests, but I didn't want to have this circular dependency block -> blockchain -> block. This case might be an exception, though, and I might just want to copy the file instead.

[ryanio]

A BlockchainInterface was introduced in a5a7f8e, so I will close this PR as resolved 👍