Trie: Refactor SecureTrie as an option #2214

holgerd77 · 2022-08-23T11:15:22Z

Since both @faustbrian and @jochem-brouwer expressed support for this in some latest discussions this is executing on the SecureTrie refactoring to integrate the secure trie functionality as an option in the base Trie library and with this reduce inheritance complexity and be more flexible regarding the integration of future features.

This is also (obviously) a breaking change, so it needs to be integrated before we do the breaking releases RC versions at the end of the week.

Relatively straight forward up till now, nevertheless 5 secure trie tests still failing, will continue to investigate, if someone spots something let me know.

…onality

packages/trie/src/trie/trie.ts

codecov · 2022-08-23T11:20:59Z

Codecov Report

Merging #2214 (4bbc4cf) into master (310b9fa) will increase coverage by 1.99%.
The diff coverage is 87.23%.

Flag	Coverage Δ
block	`92.98% <ø> (?)`
blockchain	`88.88% <100.00%> (?)`
client	`87.00% <50.00%> (?)`
common	`98.08% <ø> (ø)`
devp2p	`92.33% <ø> (+0.21%)`	⬆️
ethash	`∅ <ø> (?)`
evm	`79.07% <ø> (ø)`
rlp	`∅ <ø> (∅)`
statemanager	`88.15% <100.00%> (?)`
trie	`89.88% <87.34%> (?)`
tx	`97.98% <ø> (?)`
util	`92.33% <ø> (ø)`
vm	`85.24% <ø> (?)`

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

faustbrian · 2022-08-23T11:41:36Z

@holgerd77 16b299e this fixes all but 1 failing test

holgerd77 · 2022-08-23T12:10:12Z

@holgerd77 16b299e this fixes all but 1 failing test

Ah, please wait with pushing, I have a lot of local commits and fixes as well.

…eckpointTrie

holgerd77 · 2022-08-23T12:30:28Z

@faustbrian haha, have completely forgotten all the other libraries, lol. 😜

Will continue and do the upstream fixes as well (this worked for so long since I deleted SecureTrie relatively late in the process).

faustbrian · 2022-08-23T12:37:00Z

@holgerd77 I pushed a change to rename secure to useHashedKeys because it's more explicit and the use prefix allows us to indicate that it performs some function. We could then also introduce useCheckpoints for example.

holgerd77 · 2022-08-23T12:46:51Z

@holgerd77 I pushed a change to rename secure to useHashedKeys because it's more explicit and the use prefix allows us to indicate that it performs some function. We could then also introduce useCheckpoints for example.

Argh, this parallel library fixing makes me a bit crazy, since I was locally also doing work. 🙂

I leave it over to you then. Can you please do the upstream library fixes as well then, I had this partly done locally but likely easier to just go over it once again with the renamed option.

I am generally ok, with the renaming, makes sense.

holgerd77 · 2022-08-23T13:19:26Z

Hmm, with Checkpointing, I am a bit drawn back and forth if it might be worth to think about an integration as well or if this is getting a bit insanely last-minute? 🤔

On the other hand this is not too much code to integrate (if we generally agree that this is a good idea).

I wondered if we could (would want) eventually just plainly integrate and completely leaving the option and an associated eventual panelty could be avoided by just creating the CP DB on an eventual first checkpoint() call (that's always the entrypoint, right?) and not already in the constructor.

So all functionality would simply be added and be there, but if people are not calling into anything checkpoint-related they would just have the plain Trie functionality without side effects. I guess this would work, might this be a path to follow? 🤔

holgerd77 · 2022-08-23T13:20:59Z

(did several edits to the previous comment, please read on site)

faustbrian · 2022-08-23T13:21:02Z

I already have a branch ready for the checkpointing. Just waiting for this to go green so I can merge it into my branch.

faustbrian · 2022-08-23T13:22:23Z

The implicit behaviour of changing the trie when calling any checkpointing methods is going down the same route as the convoluted implicit behaviour of the root persistence that was reverted 😅

holgerd77 · 2022-08-23T13:22:53Z

I already have a branch ready for the checkpointing. Just waiting for this to go green so I can merge it into my branch.

So what do you think, might it be worth (respectively eventually: better) to plainly do it without any option?

But also curious for your solution, happy to have a look first!

holgerd77 · 2022-08-23T13:25:08Z

The implicit behaviour of changing the trie when calling any checkpointing methods is going down the same route as the convoluted implicit behaviour of the root persistence that was reverted 😅

I think I would disagree, there is nothing implicit here in this case. If checkpointing is used, checkpointing works, that's all. I don't see where the implicit changing/modification of the trie would be.

But also not totally pushing for this solution. Just an additional idea.

faustbrian · 2022-08-23T13:26:16Z

The implicit change is that the underlying storage gets swapped out at runtime after you initialise the trie because DB has to be replaced with CheckpointDB which means your trie starts with this.db instanceof DB but then becomes this.db instanceof CheckpointDB at runtime.

faustbrian · 2022-08-23T14:16:20Z

@holgerd77 should be ready, all green.

faustbrian · 2022-08-23T14:48:29Z

packages/trie/test/trie/trie.spec.ts

@@ -17,26 +16,9 @@ for (const { constructor, title } of [
    constructor: CheckpointTrie,
    title: 'CheckpointTrie',
  },
-  {
-    constructor: SecureTrie,


I enabled and fixed these in #2215 together with checkpoint tests.

holgerd77

LGTM, will merge when CI passes, eventually someone else can also approve (including @faustbrian) sinc I can't do as PR creator, then I would not have to admin-merge.

holgerd77 · 2022-08-23T15:41:19Z

packages/client/lib/execution/vmexecution.ts

@@ -40,7 +40,7 @@ export class VMExecution extends Execution {
    super(options)

    if (isFalsy(this.config.vm)) {
-      const trie = new SecureTrie({ db: new LevelDB(this.stateDB) })
+      const trie = new CheckpointTrie({ db: new LevelDB(this.stateDB), useHashedKeys: true })


Yeah, really love this new option naming when reading with some distance! 👍

This is so much easier to grasp than having to deal with what a "Secure Trie" is.

faustbrian

Looks good other than commented out tests which I've fixed in a separate PR.

holgerd77 added 3 commits August 23, 2022 12:32

trie: removed deprecated prove() function (renamed to createProof())

fe64bd1

trie: added secure option to base trie, integrated secure trie functi…

ddadf76

…onality

trie: adopted secureTrie.spec.ts tests (5 failing)

aa731fb

holgerd77 added PR state: WIP type: refactor package: mpt labels Aug 23, 2022

faustbrian reviewed Aug 23, 2022

View reviewed changes

packages/trie/src/trie/trie.ts Outdated Show resolved Hide resolved

faustbrian reviewed Aug 23, 2022

View reviewed changes

packages/trie/src/trie/trie.ts Outdated Show resolved Hide resolved

trie: fixed copy function

3a511d8

holgerd77 added 3 commits August 23, 2022 13:43

trie: fixed secureTrie.spec.ts branching test

ce77025

trie: additional test fixes

5cb020a

trie: removed secure.ts SecureTrie class definition file

80156ec

trie: updated README

31f89cb

holgerd77 force-pushed the refactore-secure-trie-as-an-option branch from 16b299e to 31f89cb Compare August 23, 2022 12:11

holgerd77 added 2 commits August 23, 2022 14:20

trie: removed secure export, unified prepareTrieOpts and expand to Ch…

f090a80

…eckpointTrie

trie: renamed _secure -> _hashKeys and _hash() -> _hashKeysFunction

3042ff7

holgerd77 added PR state: needs review and removed PR state: WIP labels Aug 23, 2022

holgerd77 requested review from gabrocheleau and jochem-brouwer August 23, 2022 12:28

faustbrian force-pushed the refactore-secure-trie-as-an-option branch 2 times, most recently from 6a821ef to 925469b Compare August 23, 2022 13:17

refactor(trie): rename secure to useHashedKeys

a50c89c

faustbrian force-pushed the refactore-secure-trie-as-an-option branch from 4ec3364 to a50c89c Compare August 23, 2022 13:46

faustbrian reviewed Aug 23, 2022

View reviewed changes

Merge branch 'master' into refactore-secure-trie-as-an-option

4bbc4cf

holgerd77 commented Aug 23, 2022

View reviewed changes

faustbrian self-requested a review August 23, 2022 16:08

faustbrian approved these changes Aug 23, 2022

View reviewed changes

holgerd77 merged commit 241f566 into master Aug 23, 2022

holgerd77 deleted the refactore-secure-trie-as-an-option branch August 23, 2022 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trie: Refactor SecureTrie as an option #2214

Trie: Refactor SecureTrie as an option #2214

holgerd77 commented Aug 23, 2022

codecov bot commented Aug 23, 2022 •

edited

Loading

faustbrian commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

faustbrian commented Aug 23, 2022 •

edited

Loading

holgerd77 commented Aug 23, 2022

holgerd77 commented Aug 23, 2022 •

edited

Loading

holgerd77 commented Aug 23, 2022

faustbrian commented Aug 23, 2022

faustbrian commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

faustbrian commented Aug 23, 2022 •

edited

Loading

faustbrian commented Aug 23, 2022

faustbrian Aug 23, 2022

holgerd77 left a comment

holgerd77 Aug 23, 2022

faustbrian left a comment

Trie: Refactor SecureTrie as an option #2214

Trie: Refactor SecureTrie as an option #2214

Conversation

holgerd77 commented Aug 23, 2022

codecov bot commented Aug 23, 2022 • edited Loading

Codecov Report

faustbrian commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

faustbrian commented Aug 23, 2022 • edited Loading

holgerd77 commented Aug 23, 2022

holgerd77 commented Aug 23, 2022 • edited Loading

holgerd77 commented Aug 23, 2022

faustbrian commented Aug 23, 2022

faustbrian commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

holgerd77 commented Aug 23, 2022

faustbrian commented Aug 23, 2022 • edited Loading

faustbrian commented Aug 23, 2022

faustbrian Aug 23, 2022

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 Aug 23, 2022

Choose a reason for hiding this comment

faustbrian left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2022 •

edited

Loading

faustbrian commented Aug 23, 2022 •

edited

Loading

holgerd77 commented Aug 23, 2022 •

edited

Loading

faustbrian commented Aug 23, 2022 •

edited

Loading