Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP removal of level DB stuff #2673

Merged
merged 19 commits into from May 4, 2023

Conversation

acolytec3
Copy link
Contributor

No description provided.

@@ -177,12 +178,12 @@ export class CheckpointDB implements DB {
async del(key: Uint8Array): Promise<void> {
const keyHex = bytesToHex(key)
if (this._cache !== undefined) {
this._cache.set(keyHex, null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely not fully work. There are different semantics in the cache for „known to not exist“ (as meant here) which was null before and „not in the cache“ which was already undefined before, I battled with this a long time.

This comes into play when the cache get method is called, because this gives undefined when the key is not in the cache, now it also gives undefined for the above case (so there is this together-throwing).

This is why I added a somewhat more complex dict structure in the account cache for the account value or undefined, so there is an „outer“ and an „inner“ undefined, which makes up for the differentiation there.

Maybe it is easier to keep null here, I was actually never that happy with the account cache structure regarding this, since it creates some overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, I haven't really dug into the trie piece of it yet. I've been focused on trying to get blockchain passing all the tests while removing as much of the dependence on try/catch logic as possible.

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #2673 (3dfee9b) into blockchain/level-refactor (89f4741) will decrease coverage by 5.29%.
The diff coverage is 73.30%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 90.64% <ø> (ø)
blockchain 90.63% <65.11%> (?)
common 95.76% <ø> (ø)
devp2p 91.72% <ø> (-0.19%) ⬇️
ethash 82.60% <82.60%> (∅)
evm 79.35% <ø> (?)
rlp ∅ <ø> (∅)
statemanager 88.04% <ø> (?)
trie 89.37% <86.66%> (?)
tx ?
util 82.05% <100.00%> (+0.05%) ⬆️
vm 84.39% <ø> (?)

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

Comment on lines +43 to +45
open() {
return Promise.resolve()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually do anything? Or is this added in to make the interface consistent with the expected types?

@@ -49,7 +50,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<Uint8Array, Uint8Array>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
db: new LevelDB(this.stateDB) as DB<Uint8Array, Uint8Array>,
db: new LevelDB(this.stateDB) as DB,

I don't think the types are necessary since this defaults to Uint8Array.

@@ -202,7 +203,7 @@ export class AccountFetcher extends Fetcher<JobTask, AccountData[], AccountData>
}
}

const trie = new Trie({ db: new LevelDB() })
const trie = new Trie({ db: new LevelDB() as DB<Uint8Array, Uint8Array> })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@gabrocheleau gabrocheleau merged commit 6c9d0b1 into blockchain/level-refactor May 4, 2023
33 of 34 checks passed
holgerd77 pushed a commit that referenced this pull request May 12, 2023
* blockchain: remove level dependency from blockchain

* util: refactor DB types from trie to util

* util: make db interface generic

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

* WIP removal of level DB stuff (#2673)

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

* Add correct value encoding for keys in level

* Apply correct db interface typing

* Fix level encoding for strings

* More typing fixes

* Fix batch value encoding

* fix deprecated property usage

* Replace level with mapdb in VM

* move mapDB to util

* Cleanup

* Add key/value encodings back to interface

* Remove level dependency from ethash

* Add db ops to blockchain

* remove unused imports

* remove leveldb

* update trie to use strings

* Slight fixes

* lint

* fix trie tests

* cast trie db as any

* client: improve levelDB types and remove unnecessary typecasting

* client: remove additional unnecessary typecasting

* client: type issue

---------

Co-authored-by: acolytec3 <konjou@gmail.com>
Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com>
@holgerd77 holgerd77 deleted the level-refactor-part-23 branch June 6, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants