Blockchain/Ethash/Trie: remove level dependency from blockchain #2669

gabrocheleau · 2023-04-27T20:56:44Z

No description provided.

…MapDB

codecov · 2023-04-27T21:03:59Z

Codecov Report

❗ No coverage uploaded for pull request base (develop-v7@ee0d44c). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
block	`90.30% <0.00%> (?)`
blockchain	`90.55% <0.00%> (?)`
client	`86.89% <0.00%> (?)`
common	`96.06% <0.00%> (?)`
devp2p	`91.84% <0.00%> (?)`
ethash	`∅ <0.00%> (?)`
evm	`79.49% <0.00%> (?)`
rlp	`∅ <0.00%> (?)`
statemanager	`80.92% <0.00%> (?)`
trie	`89.90% <0.00%> (?)`
tx	`95.50% <0.00%> (?)`
util	`81.32% <0.00%> (?)`
vm	`81.36% <0.00%> (?)`

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

holgerd77 · 2023-04-28T12:22:10Z

packages/blockchain/package.json

    "debug": "^4.3.3",
    "ethereum-cryptography": "^1.1.2",
-    "level": "^8.0.0",
    "lru-cache": "^5.1.1",
    "memory-level": "^1.0.0"


memory-level also already ready for removal here? Just asking.

holgerd77 · 2023-04-28T12:23:01Z

Cool 🤩, had some first look over the changes, I would generally say this already very much goes into a good direction! 👍

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>

…l-refactor

holgerd77

Great refactor, totally excited about this! 🎉

Some comments, biggest thing likely this encoding thing (can also continue to discuss in the chat).

packages/blockchain/src/consensus/clique.ts

holgerd77 · 2023-05-09T09:21:19Z

packages/blockchain/src/consensus/ethash.ts

@@ -45,7 +44,7 @@ export class EthashConsensus implements Consensus {
  public async genesisInit(): Promise<void> {}
  public async setup({ blockchain }: ConsensusOptions): Promise<void> {
    this.blockchain = blockchain
-    this._ethash = new Ethash(this.blockchain.db as unknown as EthashCacheDB)
+    this._ethash = new Ethash(this.blockchain.db as any)


Not for this PR, but should we really take this occasion and loosen the binding between blockchain and the Ethash class/package? So eventually make this an optional dependency? 🤔

I think this is not an easy side-shot though but needs some additional design thoughts, regarding consensus instantiation (eventually) on mainnet, thinks like that.

But at the end should not be that complicated/invasive and I guess it's maybe/likely the time for that.

holgerd77 · 2023-05-09T09:22:48Z

packages/blockchain/src/db/manager.ts

        }
-        body.push([])
+        if (body.length !== 3) body.push([])


Not have the complete picture here, is the case at this point that body length might equal 3 sufficiently covered or alternatively not possible to reach due to previous code?

I would need @g11tech to validate but I think this should be sufficient.

holgerd77 · 2023-05-09T09:23:52Z

packages/blockchain/src/db/map.ts

+  open() {
+    return Promise.resolve()
+  }
+}


Is this 1:1 the same as the Trie version?

Wonder if we want to move this to Util as well? (Code) bloat seems limited to me, not such an extensive implementation.

Would this make sense?

Looks like it. I'll move it and adjust the imports accordingly.

holgerd77 · 2023-05-09T09:32:48Z

packages/client/lib/execution/level.ts

+      value = await this._leveldb.get(key, {
+        valueEncoding: encoding,
+      })
+      if (value === null) return undefined


Ah, this is too hacky here, we need to find another solution for that. 🙂

(so I am getting this right that this would also be the "default" implementation for other Blockchain LevelDB users, right?)

My tendency would be that we make encoding a mandatory method parameter in the DB interface and then just always pass this explicitly in our implementations and one can then decide in the wrapper implementation s (like LevelDB, so this one here) to use this information or not.

Does this make sense?

We can though I don't know if we should or not. The nicest sort ofd "default" way would be that the DB just takes what we pass in without doing anything to it and returns that same value when we pass in the same key (more or less how our MapDB does). The reason I have to do all this special handling is that level assumes that all data should be a certain datatype and format when you dump it into the DB. I'm not an expert on key/value DBs but if other key/value DBs require encoding details and if the names that specify those encoding types is identical, I think it's fine. If not, I'd prefer to err on the side of not overspecifying what should be in the wrapper.

The wrapper is a code part which is not in our control. People will just copy over this code part if they want to use Blockchain with LevelDB. So this should have a very stable API and be robust and generic in the inside. We do not want to tell people "oh, and please add if (key === NewKeyWeAddedToBlockchain) in line 34 of your wrapper" when we publish an update of our Blockchain library where we add some new functionality. 🙂

holgerd77 · 2023-05-09T09:33:52Z

packages/client/lib/execution/vmexecution.ts

@@ -50,7 +51,7 @@ export class VMExecution extends Execution {

    if (this.config.vm === undefined) {
      const trie = new Trie({
-        db: new LevelDB(this.stateDB),
+        db: new LevelDB(this.stateDB) as DB,


Is this casting necessary here?

It is in this instance since the client DB allows more key and value types than does the trie.

Can we optimally align this so that we have one single interface for Trie, Blockchain and eventual other usages?

(this goes a bit together with the special-handling topic for the encoding values)

packages/client/lib/sync/fetcher/accountfetcher.ts

packages/ethash/src/level.ts

holgerd77 · 2023-05-09T09:38:29Z

packages/vm/test/level.ts

+  open() {
+    return this._leveldb.open()
+  }
+}


Why is LevelDB necessary for VM? Wouldn't MemoryDB do it?

I swapped it out for a MapDB borrowed from blockchain and it seems to work fine so will update here.

👍

What is with the version in api/utils.ts? Can we there also use MapDB?

And can we then delete this level.ts file or is it still necessary?

…l-refactor

…umjs/ethereumjs-monorepo into blockchain/level-refactor

holgerd77

Ok, looks great, I think I will merge this in here now.

Feel free to do continued refinements though.

gabrocheleau added 4 commits April 26, 2023 16:58

blockchain: remove level dependency from blockchain

7c444cb

util: refactor DB types from trie to util

ec3a913

util: make db interface generic

510b0c6

blockchain: remove abstract-level usage in favor of DB interface and …

89f4741

…MapDB

gabrocheleau added PR state: WIP type: refactor package: blockchain package: util target: develop-v7 dependencies Pull requests that update a dependency file labels Apr 27, 2023

holgerd77 reviewed Apr 28, 2023

View reviewed changes

acolytec3 force-pushed the blockchain/level-refactor branch from 127e6b5 to 89f4741 Compare April 28, 2023 15:17

acolytec3 and others added 9 commits May 3, 2023 22:07

WIP removal of level DB stuff (#2673)

6c9d0b1

Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>

Add correct value encoding for keys in level

98dd481

Apply correct db interface typing

9c2e40d

Fix level encoding for strings

29579c6

More typing fixes

9810b92

Merge remote-tracking branch 'origin/develop-v7' into blockchain/leve…

a752c60

…l-refactor

Merge remote-tracking branch 'origin/develop-v7' into blockchain/leve…

ab7fc78

…l-refactor

Fix batch value encoding

c7ea77a

fix deprecated property usage

ccd8be3

holgerd77 requested changes May 9, 2023

View reviewed changes

acolytec3 added 6 commits May 9, 2023 10:27

Replace level with mapdb in VM

abcd64d

move mapDB to util

d35960e

Cleanup

86cea58

Merge remote-tracking branch 'origin/develop-v7' into blockchain/leve…

521d164

…l-refactor

Add key/value encodings back to interface

2b1e081

Remove level dependency from ethash

238626d

Add db ops to blockchain

e1faad9

acolytec3 added package: ethash package: client package: trie package: vm and removed PR state: WIP labels May 10, 2023

acolytec3 and others added 13 commits May 10, 2023 14:45

remove unused imports

c8d95df

remove leveldb

aca0e1d

update trie to use strings

ef57f9b

Slight fixes

b5fa536

lint

7f1d7e0

fix trie tests

7d807cf

cast trie db as any

cd0ab87

Merge remote-tracking branch 'origin/develop-v7' into blockchain/leve…

ac004af

…l-refactor

client: improve levelDB types and remove unnecessary typecasting

b1346f2

Merge branch 'blockchain/level-refactor' of https://github.com/ethere…

d6d5387

…umjs/ethereumjs-monorepo into blockchain/level-refactor

client: remove additional unnecessary typecasting

16a5ab9

client: type issue

9d3e04b

Merge branch 'develop-v7' into blockchain/level-refactor

1b5af37

holgerd77 approved these changes May 12, 2023

View reviewed changes

holgerd77 merged commit 355b03b into develop-v7 May 12, 2023
39 checks passed

holgerd77 deleted the blockchain/level-refactor branch May 12, 2023 09:48

holgerd77 changed the title ~~blockchain: remove level dependency from blockchain~~ Blockchain / Ethash / Trie: remove level dependency from blockchain Jul 6, 2023

holgerd77 changed the title ~~Blockchain / Ethash / Trie: remove level dependency from blockchain~~ Blockchain/Ethash/Trie: remove level dependency from blockchain Jul 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain/Ethash/Trie: remove level dependency from blockchain #2669

Blockchain/Ethash/Trie: remove level dependency from blockchain #2669

gabrocheleau commented Apr 27, 2023

codecov bot commented Apr 27, 2023 •

edited

holgerd77 Apr 28, 2023

holgerd77 commented Apr 28, 2023

holgerd77 left a comment

holgerd77 May 9, 2023

holgerd77 May 9, 2023

acolytec3 May 9, 2023

holgerd77 May 9, 2023

acolytec3 May 9, 2023

holgerd77 May 9, 2023

acolytec3 May 9, 2023

holgerd77 May 9, 2023

holgerd77 May 9, 2023

acolytec3 May 9, 2023

holgerd77 May 9, 2023

holgerd77 May 9, 2023

acolytec3 May 9, 2023

holgerd77 May 9, 2023

holgerd77 left a comment

Blockchain/Ethash/Trie: remove level dependency from blockchain #2669

Blockchain/Ethash/Trie: remove level dependency from blockchain #2669

Conversation

gabrocheleau commented Apr 27, 2023

codecov bot commented Apr 27, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

holgerd77 commented Apr 28, 2023

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2023 •

edited