Ganache Beta Releases Update Feedback (old: trie docs fix, do not merge)

[davidmurdoch]

No description provided.

[acolytec3]

LGTM and thank you!

[codecov]

Codecov Report

Merging #2159 (2fed8ac) into master (762f1f9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	93.07% <ø> (ø)
blockchain	88.82% <ø> (ø)
client	86.67% <ø> (ø)
common	98.09% <ø> (ø)
devp2p	92.30% <ø> (+0.08%)	⬆️
ethash	∅ <ø> (∅)
evm	62.96% <ø> (ø)
rlp	∅ <ø> (∅)
statemanager	88.66% <ø> (ø)
trie	89.27% <100.00%> (ø)
tx	97.98% <ø> (ø)
util	93.14% <ø> (ø)
vm	86.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[davidmurdoch]

Merging #2159 (c4fc4cb) into master (f5a1d77) will decrease coverage by 6.13%

A 6.13% reduction in coverage seems a bit much for this change. Haha. Is Codecov not working correctly?

[acolytec3]

Merging #2159 (c4fc4cb) into master (f5a1d77) will decrease coverage by 6.13%

A 6.13% reduction in coverage seems a bit much for this change. Haha. Is Codecov not working correctly?

Yes, we have a glitch. A number of PRs are currently stuck with this issue. Hoping to get it resolved on the next day or two.

[acolytec3]

Note, merging this would conflict with #2167 so we should decide what we're doing with that and then either merge or close this.

[davidmurdoch]

FYI: we've spent about 30 hours updating Ganache to use the new Level infrastructure (and other ejs breaking changes). We're discussing feasibility of forking the VM and Common so we can backport the Merge hardfork into v5, if #2167 lands we might have to since it'll likely be much faster to backport than to do this all again.

[faustbrian]

@davidmurdoch is there anything special that Ganache does that the Level upgrade took 30 hours? It's a fairly straightforward change that shouldn't require much changes since all that was changed was to abstract the database layer to make it swappable and upgrade the level dependencies, which in itself are backwards compatible.

[holgerd77]

FYI: we've spent about 30 hours updating Ganache to use the new Level infrastructure (and other ejs breaking changes). We're discussing feasibility of forking the VM and Common so we can backport the Merge hardfork into v5, if #2167 lands we might have to since it'll likely be much faster to backport than to do this all again.

Yes, can second @faustbrian, it would be great if you can provide some insight here what were/are the problems and/or blockers around the Level DB upgrades (or is this general underlying major Level DB version jump problematic for you? 🤔). This was intended to be just an interface upgrade/generalization to ease the use of other backends and we didn't expect major problems here.

[davidmurdoch]

First, I want to say that the new betas do look good, and you've all done great work on it. I'm loving the consistency of the packages, the namespacing, the new interfaces, and really the way the @ethereumjs project is being developed is inspiring and exciting.

---

The issues we're have here is that there are like 30+ breaking changes (I don't really know how many there are... we've got 64 files changed, 809 insertions, and 945 deletions, so far). These breaking changes have to be dealt with simultaneously in order to get Ganache to start up. So while each change on its own might not be so bad, all changes need to be done at the same time for packages that make use of the ethereumjs ecosystem.

The type changes are a huge headache here, as they are incompatible in weird ways. An example:

// old way:
import vm from "@ethereumjs/vm";
const vm = await VM.create(...);
vm.on("step", () => { ... });

// new way:
import { VM } from "@ethereumjs/vm";
import { EVM } from "@ethereumjs/evm";
const vm = await VM.create(...);
(vm.evm as unknown as EVM).on("step", () => { ... });

The step event was moved to a new property who's typescript type doesn't support an Event Emitter interface so we have to cast it to now. But we can't cast directly. Same thing with having to do vm.eei as unknown as EEI (and the eei property is actually missing a TSDoc comment describing what it is and what it is for -- I meant to open an issue for this).

---

Navigation through the published code is now much more difficult than it used to be due to the use of Interfaces instead of classes. If I want navigate to the source for vm.eei I can't do that directly in my IDE, since it just takes me to an Interface instead of the implementation.

---

Previously, the types were next to the JS files, now they are in a separate folder, which isn't a huge deal, but I prefer the old way.

---

The RLP package changing to use UInt8Array instead of Buffer now requires that we use our own RLP package that uses Buffer only (we polyfill Buffer in the browser).

---

While not your fault, the level package types are problematic too. One example here is that an AbstractSublevel instance can't be assign to the AbstractLevel type - which doesn't make sense -- but it is the way their types are. They've got a bunch of other issues with their types too that have made this upgrade challenging; they seem to only partially apply the generics they which leads to unnecessary type incompatibilities.

The Trie package's LevelDB type requires that the Level instance passed in be an AbstractLevel<string | Buffer | Uint8Array, string | Buffer, string | Buffer>, so Ganache's own AbstractLevel<Buffer, Buffer, Buffer> instance is incompatible -- but it shouldn't be (adding generics to the LevelDB would probably help here).

The Level update and its interface are not backwards compatible; there are many breaking changes that affect us.

Ganache allows users to specify a memdown instance (or any AbstractLevelDown interface); I think we'll need to write a translation layer for memdown and the new level interface (as it doesn't provide a way wrap an old AbstractLeveldown interface).

---

We were still using Common.forCustomChain, so we finally had to move the params around in order to switch to Common.custom.

---

The new EVMResult type is incompatible with itself if the types are used in different packages that install their node_modules in different locations (packages aren't hoisted):

Argument of type 'import("/home/david/work/ganache-core/src/chains/ethereum/ethereum/node_modules/@ethereumjs/evm/dist/evm").EVMResult' is not assignable to parameter of type 'import("/home/david/work/ganache-core/src/chains/ethereum/utils/node_modules/@ethereumjs/evm/dist/evm").EVMResult'.
  The types of 'execResult.runState.interpreter' are incompatible between these types.
    Type 'import("/home/david/work/ganache-core/src/chains/ethereum/ethereum/node_modules/@ethereumjs/evm/dist/interpreter").Interpreter' is not assignable to type 'import("/home/david/work/ganache-core/src/chains/ethereum/utils/node_modules/@ethereumjs/evm/dist/interpreter").Interpreter'.
      Property '_vm' is protected but type 'Interpreter' is not a class derived from 'Interpreter'.

---

We use new EVM(...).executeMessage, which has moved or been removed (yet to figure this one out).

---

We use stateManager.touchAccount which has not only moved to eei, but been marked protected so we can't call it (without disabling the type checker on the line).

---

There are many other complexities to the upgrade, and we've still got 1000s of failing tests to go through.

I'm generally 👍 on the major versions here and if I was starting a new project I think the new packages would be a joy to use. So don't take my frustration with having to handle all these changes at once as a sign that the new packages are not better than before, because I do think they are!

We obviously have a deadline for getting Merge hardfork support in our tooling, but we now have to slog through all these breaking changes first just to get to the Merge hardfork, which is really all us and our users care about or need.

We definitely don't want to wait to update to the new versions (we really want those native bigints!), so backporting The Merge related changes is not ideal. We just can't spent another 30 hours trying to get the new upgrades working just so we can ship with The Merge support -- it just looking like backporting might be a better and more efficient use of our time right now.

[faustbrian]

I can only comment on the trie part as that is all I'm familiar with but based on what you wrote what your issue is it should be a non-issue and take a few minutes to fix because the whole point of abstracting the database layer was to get rid of these kinds of issues or limitations.

You can simply copy over the LevelDB class from before that change, extend the DB interface and continue to use the legacy level packages like memdown and friends. If you can't figure out how to do it or don't have the resources I can post a snippet here tomorrow with the old LevelDB implementation that can be used with this new approach.

The type issues you described with AbstractLevel should also be a non-issue because you can use your own implementation now without having any behaviour or types forced onto you. After #2167 this would be even easier and less of a hassle because there won't be clashing installations of dependencies.

[holgerd77]

@davidmurdoch thanks a lot for writing this all up, this is immensely helpful. Yeah, sorry, I can totally feel you. 😐 🙃

We'll go through this one-by-one hopefully we can take away some of the pain here. I have the impression that especially the type/interface stuff you described should be very much solvable, hopefully other things as well. There will for sure remain some update-complexity though.

[jochem-brouwer]

@davidmurdoch

I will reply to the points which I can help you with. Thanks for the detailed overview of the problems! 😄 👍

I have tried addressing the EVM type problem in #2182. If this looks OK to you then we can most likely also do this with EEI.

RLP now indeed returns Uint8Array, but you could wrap; arrToBuffArr(RLP.decode(<input>)), this converts it back to Buffer which we also internally use. This method can be imported from our util package.

new EVM(...).executeMessage -> I have digged a bit back, this is most likely renamed to runCall.

Not being able to call touchAccount is a bug.

I have to investigate this EVMResult type thing a bit more, it seems to work internally, but apparently not externally?

[faustbrian]

@davidmurdoch I've added an example for using level-mem which would be the implementation before the Level 8.0.0. upgrade, see d64040a. I also added recipes that can be copied into your project and are easy to modify since the DB interface is fairly slim, see 639c1d8.

The issue you described with needing AbstractLevel<Buffer, Buffer, Buffer> would be easily resolved after #2167 is merged. You could then copy the packages/trie/recipes/level.ts contents into your project and replace AbstractLevel<string | Buffer | Uint8Array, string | Buffer, string | Buffer> with AbstractLevel<Buffer, Buffer, Buffer> and you should be good to go with that custom implementation.

[holgerd77]

The RLP package changing to use UInt8Array instead of Buffer now requires that we use our own RLP package that uses Buffer only (we polyfill Buffer in the browser).

I've added some additional CHANGELOG notes on this with 14ae98e, let us know if docs are not sufficient for some cases or the conversion functions might miss some functionality/conversion cases.

[holgerd77]

Will take over the doc changes from here to #2191, then we do not have to solve the conflicts for the somewhat tiny changes here and keep the branch updated.

Will nevertheless keep open here as some proxy for the discussion. 🙂

[holgerd77]

@davidmurdoch planning another round of monorepo Beta releases this week, let us/me know if this works for you and if you have additional wishes for included features or changes respectively a release timeframe! 🙏

Thanks for testing our releases and for the patience along the way! 🙂

[davidmurdoch]

@holgerd77 will do! Thanks!

[holgerd77]

@davidmurdoch we have now released release candidate versions (RC 1) for all the libraries with the latest changes, see #2237.

[davidmurdoch]

Sorry I was MIA for a bit. I was away from my computer for a bit there. We'll hop back on the upgrade this week. Feel free to close this issue. :-) Thanks!