refactor(trie): abstract database implementation with an interface

[faustbrian]

Resubmission of #1895 with squashed history and rebase on develop

[codecov]

Codecov Report

Merging #1917 (5bf632b) into develop (5bf632b) will not change coverage.
The diff coverage is n/a.

❗ Current head 5bf632b differs from pull request most recent head f6acd2c. Consider uploading reports for the commit f6acd2c to get more accurate results

Flag	Coverage Δ
block	85.28% <0.00%> (ø)
blockchain	83.13% <0.00%> (ø)
client	77.75% <0.00%> (ø)
common	95.57% <0.00%> (ø)
devp2p	82.83% <0.00%> (ø)
ethash	90.81% <0.00%> (ø)
statemanager	84.24% <0.00%> (ø)
trie	80.64% <0.00%> (ø)
tx	92.09% <0.00%> (ø)
util	87.29% <0.00%> (ø)
vm	81.08% <0.00%> (ø)

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

[holgerd77]

Rebased this via UI after testing locally.

[holgerd77]

LGTM, great great work, thanks so much! 😄

Will merge.

[faustbrian]

@holgerd77 would you accept a PR that extracts the LevelDB package into a trie-level package? Might be a bit late into your process now but might be worth it to extract it as soon as possible after your current releases are done so that the trie package can be completely free of the level dependency.

[holgerd77]

Could you outline a potential API usage (instantiation) in 4-5 lines?

[faustbrian]

Those 2 would be how I imagine it to be used, basically just extracting the existing LevelDB class to allow the trie package to become easier to use in browsers by removing the level dependency.

import { Trie } from "@ethereumjs/trie";
import { LevelDB } from "@ethereumjs/trie-level";

const trie = new Trie({ db: new LevelDB() });

import { Trie } from "@ethereumjs/trie";
import { LevelDB } from "@ethereumjs/trie-level";
import level from "level";

const trie = new Trie({ db: new LevelDB(level("path")) });

[holgerd77]

Ok, thanks, will ask within the team! 🙂

[holgerd77]

(I guess chances are 50:50)

[faustbrian]

@holgerd77 created a draft of the changes to show what it would actually entail - https://github.com/faustbrian/ethereumjs-monorepo/commit/da41ce53a0b1b5b124751eec4aa17c5417706d6a

[holgerd77]

@holgerd77 created a draft of the changes to show what it would actually entail - faustbrian@da41ce5

Thanks a lot for all this pro-active engagement, that's really great!! 👍

I guess we do not have an agreement yet, opinions are somewhat mixed and reserved, will keep you updated!

[holgerd77]

@holgerd77 created a draft of the changes to show what it would actually entail - faustbrian@da41ce5

Hi there, to follow-up: still no clear opinion crystalizing out. Do you see (have yourself) some substantial practical gains from this separation or is this rather some "we should do it since this is how things should be?" thing?

(To be clear: I (and I guess I can say: we) would also very much value the second argument, however in the current situation first argument would give a stronger reasoning to do since we do have to limit ourselves right now and from the "it would be structurally cleaner to do x" perspective we still have a lot on the plate which we would/could/would want to realize at some point (but also can't/won't do right now for very much practical/procedural reasons))

[holgerd77]

(and so be totally transparent on this: my current tendency is to rather not do since this does feel as overloading to me, this this current release round is just already demanding a lot from the team due to it's dramatically large scope it already has)

[faustbrian]

Pretty much "how it should be done" but also practical gains resulting from that because then the trie package can be used in browsers which would resolve #1056. Alternatively you could also upgrade level/level to ^8.0 and that way get an easy way to add browser support without removing LevelDB.

[holgerd77]

Pretty much "how it should be done" but also practical gains resulting from that because then the trie package can be used in browsers which would resolve #1056. Alternatively you could also upgrade level/level to ^8.0 and that way get an easy way to add browser support without removing LevelDB.

Thanks, we have various level usages in our package suite - 4-6 or so and all on a similar version range - e.g. also as a block data store in the blockchain package, in the client.

Can you judge on the effort to then bring all of this to level 8? This might then also be a way here, would also fit well in the scope of our release process, to do such an upgrade right now.

[faustbrian]

I upgraded it in one of my own applications and that only took an hour with testing if everything worked but I can work on a PR tomorrow and submit a draft during the weekend so you can see what it looks like.

[holgerd77]

I upgraded it in of my own applications and that only took an hour with testing if everything worked but I can work on a PR tomorrow and submit a draft during the weekend so you can see what it looks like.

Uh. If it is not too much of a burden too you this would be great! 💯

Thanks a lot for all these improvement offers - also for being flexible what to integrate. So when you do a "level search with the packages scope, you get 8 occurrences of level and level-related (level-mem) packages.

Is this strictly a breaking change, or would old DBs still work, do you know that (we would nevertheless do towards develop, just asking)?

Also, in case you do you might want to wait for #1928, this is our very last rebase of develop towards the master branch, from that on we will have no more master changes and develop will get merged into master soon (days, sometime next week).

[faustbrian]

As far as I am aware from documentation and my own testing there is no breaking changes to the data structures so the databases should be fine. The 8.0 release was primarily API refactors to make the package more flexible for browser and server usage.

[faustbrian]

@holgerd77 I subscribed to #1928 and will work on the PR as soon as I get a notification that it is merged.

[holgerd77]

Ah, the PR ist just a preparatory test rebase and won't get merged. Instead the result will get pushed as the new develop branch (and so your PR should then still be submitted towards develop).

Will notify you once develop is updated.

[holgerd77]

@faustbrian Hi there, we have now merged in develop into master. So if you find some time you can submit a PR directly towards master now 🙂.

According to our timeline from #1914 a feature freeze for the v6 master is intended to be applied on June 12. We are - mostly - in time, but this might also get 1-2 more days though.

[faustbrian]

Will work on it tomorrow or on friday and submit the PR as soon as possible then.