Trie proof API refactor #2949

ScottyPoi · 2023-08-09T09:43:23Z

This PR improves upon the proof methods in the Trie library.

The Trie methods fromProof and verifyProof require a new Trie object to be instantiated by the user simply for the purpose of calling the methods. This can be confusing, and seems unnecessarily indirect.

With static versions of the same methods, users can now accomplish these tasks in one step.

Trie.fromProof

Works just like trie.fromProof, with an additional opts parameter to pass in keyHashing settings.

Now instead of:

const trie = new Trie(opts)
await trie.fromProof(proof)

We can just do:

const trie = await Trie.fromProof(proof, opts)

Trie.verifyProof

Works just like trie.verifyProof, with an additional opts parameter to pass in keyHashing settings.
Does not require the rootHash parameter as expected by class method.

Now instead of:

const trie = new Trie(opts)
const val = trie.verifyProof(trie.root(), key, proof, opts)

We can just do:

const val = Trie.verifyProof(key, proof, opts)

codecov · 2023-08-09T09:54:31Z

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (6ca6fd5) 86.01% compared to head (7404c04) 87.86%.

Additional details and impacted files

Flag	Coverage Δ
block	`88.57% <ø> (ø)`
blockchain	`91.60% <ø> (ø)`
client	`84.56% <ø> (+0.03%)`	⬆️
common	`98.26% <ø> (ø)`
devp2p	`82.12% <ø> (?)`
ethash	`∅ <ø> (∅)`
evm	`76.92% <ø> (ø)`
genesis	`99.98% <ø> (?)`
rlp	`∅ <ø> (∅)`
statemanager	`86.56% <100.00%> (-0.01%)`	⬇️
trie	`89.13% <76.92%> (-0.59%)`	⬇️
tx	`95.89% <ø> (ø)`
util	`89.13% <ø> (ø)`
vm	`80.26% <ø> (ø)`
wallet	`88.35% <ø> (ø)`

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

scorbajio · 2023-08-09T17:11:00Z

Linking #2927

jochem-brouwer

I'm not sure if we want to make fromProof static

jochem-brouwer · 2023-08-09T19:16:43Z

packages/trie/src/trie.ts

@@ -113,6 +113,50 @@ export class Trie {
    return new Trie(opts)
  }

+  static async fromProof(proof: Proof, opts?: TrieOpts): Promise<Trie> {


What if I want to import two proofs? Or, I want to import a new proof in my trie later? This is now not possible anymore, right?

Ok, let's bring this back to the drawing board and first decide if this is a change we want respectively which makes sense and is thought through to the end.

Will give this a "blocked" label for now.

(this change (i.e. make fromProof static) does not seem to be referred in the linked issue)

What if I want to import two proofs? Or, I want to import a new proof in my trie later? This is now not possible anymore, right?

It seems like these tasks are possible with the new updateFromProof function.

Ah yes you are right, I somehow missed that.

Feel free to "unblock" this again or course of you guys think you've gotten answers for most of the questions here and things can be introduced in a non breaking way, you guys have a lot better overview than me here what's useful and makes sense regarding all this proof stuff!

yes, updateFromProof is just the old fromProof with the word update tagged on to distinguish from the other. To avoid actual breaking changes, we could instead use createFromProof for the static method, and leave fromProof as it was.

We now have #3186. I think we want to make the methods static in some cases? (But this is breaking)

Yes, the goal is to have a static version of verifyProof so that a new trie instantiation isn't needed for each proof verification, or the less-than-ideal proofTrie pattern where we keep a trie around in both the statemanager and snap sync fetchers to perform proof verification.

The changes for #3186 are included, so we should have a static and non-static version of verifyProof and fromProof. This shouldn't be breaking? Since users can just use the previously available non-static versions if they chose to.

jochem-brouwer · 2023-08-09T20:54:39Z

packages/trie/src/trie.ts

-  async fromProof(proof: Proof): Promise<void> {
+
+  async updateFromProof(proof: Proof): Promise<void> {
+    if (!equalsBytes(this.root(), this.hash(proof.shift()!))) {


I am not sure if I like this check, have to think about the practical downsides of this 🤔

If you try to update a sparse trie with an invalid proof, this will throw.
shift() because the root node is already in the trie, so we'll never need to add it.

🤔 I'd have to hear the upside to allowing a trie to update itself from a proof to a different trie!

packages/statemanager/src/stateManager.ts

packages/trie/src/trie.ts

ScottyPoi · 2023-08-10T04:03:34Z

Should be non-breaking now 🥚

…to trie-proof-api

jochem-brouwer

I am not entirely sure about the state of this PR. We now have #3186, it seems that if we want this in this seems to be merely marking some methods as static? (Note: we should make this non-breaking, so it should both be static and non-static). I will remove the blocked label until we resolve what we want with this PR. (I think the gist is to make the methods static which should have been static in the first place? 🤔 )

jochem-brouwer · 2023-12-29T20:42:19Z

packages/client/test/sim/beaconsync.md

Random changes in this file?

Must have been from one of the merges. I fixed it so it's just the single extra space removed and any other unnecessary changes excluded.

packages/client/test/testdata/blocks/kaustinen2.json

packages/statemanager/src/rpcStateManager.ts

jochem-brouwer · 2023-12-29T20:47:06Z

packages/trie/src/trie.ts

@@ -113,6 +113,50 @@ export class Trie {
    return new Trie(opts)
  }

+  static async fromProof(proof: Proof, opts?: TrieOpts): Promise<Trie> {


We now have #3186. I think we want to make the methods static in some cases? (But this is breaking)

holgerd77 · 2023-12-30T15:43:55Z

I will remove the blocked label until we resolve what we want with this PR

@jochem-brouwer I assume that you intended to write "I will not remove..." if not otherwise stated since that would make more sense 🙂

@scorbajio don't know, are you getting along with the things we get from #3186 ? If so, @jochem-brouwer: would you then recommend to close this PR and rather not merge?

jochem-brouwer · 2023-12-30T15:57:03Z

Yes indeed I did intend to write: I will not remove :)

I think that this PR can be updated with the changes of #3186, but I think we should update this PR to also include small changes such as also making verifyProof static (besides keeping it non-static, because otherwise it is breaking). So that would change this PR to a few lines of changes.

( @holgerd77 I think you intended to ping @ScottyPoi in your last post)

holgerd77 · 2024-01-02T10:42:34Z

@jochem-brouwer 👍

Ah, no, I indeed intended to mention @scorbajio since he requested this work to be updated and reviewed for some other follow up work.

…to trie-proof-api

…orepo into trie-proof-api

scorbajio

LGTM!

stale

jochem-brouwer

Two nits @scorbajio

jochem-brouwer · 2024-01-19T21:09:30Z

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

+      keys[keys.length - 1],
+      keys,
+      values,
+      <any>proof,


Do we need to this?

Oh wait, it is client, I think this is beyond the scope of this PR (should do a client type cleanup at some point)

Good point. I've reverted it for now. Will make the cleanup in a followup PR.

packages/client/test/net/protocol/snapprotocol.spec.ts

packages/trie/src/trie.ts

jochem-brouwer · 2024-01-19T21:13:36Z

packages/trie/src/trie.ts

+    await trie._db.batch(opStack)
+    trie.root(trie.hash(proof[0]))
+    await trie.persistRoot()
+    return trie


I think this one needs to be checked @scorbajio

This reverts commit 4801912.

…orepo into trie-proof-api

holgerd77 · 2024-01-22T11:40:18Z

Updated this via UI

I think we can take this in after having done a rough review? Will wait for a confirmation from @jochem-brouwer though (then optimally directly merge).

jochem-brouwer

Resolved merge conflicts and fixed the example. LGTM (assuming CI passes, will directly merge)

ScottyPoi added 7 commits August 9, 2023 03:24

trie: fromProof as static method

9f3e6f3

trie: static verify proof method

8e80c25

trie: change old fromProof to 'updateFromProof'

dfa3d39

update proof use in range.ts

0afe20e

update proof api use in statemanager

9a0dd27

trie: update tests

4175c5a

trie: update tests

61039b5

ScottyPoi added type: feature PR state: needs review type: refactor package: mpt labels Aug 9, 2023

Merge branch 'master' into trie-proof-api

d432975

ScottyPoi marked this pull request as ready for review August 9, 2023 10:18

jochem-brouwer reviewed Aug 9, 2023

View reviewed changes

holgerd77 added the PR/Issue state: blocked label Aug 9, 2023

jochem-brouwer reviewed Aug 9, 2023

View reviewed changes

scorbajio previously requested changes Aug 9, 2023

View reviewed changes

packages/statemanager/src/stateManager.ts Outdated Show resolved Hide resolved

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

ScottyPoi and others added 3 commits August 9, 2023 21:52

Revert breaking changes

fb08401

change names back

db1a0ea

Merge branch 'master' into trie-proof-api

10f1c2c

ScottyPoi and others added 3 commits August 10, 2023 16:48

Merge branch 'master' into trie-proof-api

2c741a1

Merge branch 'master' into trie-proof-api

6bebee8

Merge branch 'master' of github.com:ethereumjs/ethereumjs-monorepo in…

3f18051

…to trie-proof-api

jochem-brouwer reviewed Dec 29, 2023

View reviewed changes

scorbajio added 10 commits January 17, 2024 21:40

Merge branch 'master' of github.com:ethereumjs/ethereumjs-monorepo in…

7f5fc51

…to trie-proof-api

Make static version of verifyRangeProof

0ab8843

Merge branch 'trie-proof-api' of github.com:ethereumjs/ethereumjs-mon…

a73e361

…orepo into trie-proof-api

Simplify function names and group static functions

96a4f50

Put back necessary import

e4f651d

Fix linting issue

a3740a3

Remove proofTrie pattern

4801912

Fix customCrypto accessing error

1fa1376

Return proof as any to ignore @typescript-eslint/return-await error

68ec378

Remove unused import

914ecf6

scorbajio reviewed Jan 19, 2024

View reviewed changes

jochem-brouwer reviewed Jan 19, 2024

View reviewed changes

scorbajio added 7 commits January 19, 2024 19:46

Make static fromProof implementation less redundant

45296e6

Persist trie root in static fromProof

69d4b5d

Add docs for createFromProof

8d8cfd7

Revert "Remove proofTrie pattern"

52eaadf

This reverts commit 4801912.

Merge branch 'master' into trie-proof-api

65feb23

Add leveldb import to snapprotocol tests

e67fb78

Merge branch 'trie-proof-api' of github.com:ethereumjs/ethereumjs-mon…

f45cb77

…orepo into trie-proof-api

scorbajio requested a review from jochem-brouwer January 20, 2024 04:13

Merge branch 'master' into trie-proof-api

0c4ed3a

jochem-brouwer added 2 commits January 30, 2024 08:19

Merge branch 'master' into trie-proof-api

4e68b92

trie: update example

7404c04

jochem-brouwer approved these changes Jan 30, 2024

View reviewed changes

jochem-brouwer merged commit 5be55b2 into master Jan 30, 2024
45 of 46 checks passed

holgerd77 deleted the trie-proof-api branch January 30, 2024 08:17

holgerd77 removed the PR/Issue state: blocked label Jan 30, 2024

scorbajio mentioned this pull request Jan 30, 2024

Remove proofTrie patern #3263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trie proof API refactor #2949

Trie proof API refactor #2949

ScottyPoi commented Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading

scorbajio commented Aug 9, 2023

jochem-brouwer left a comment

jochem-brouwer Aug 9, 2023

holgerd77 Aug 9, 2023

holgerd77 Aug 9, 2023

jochem-brouwer Aug 9, 2023

scorbajio Aug 9, 2023 •

edited

Loading

jochem-brouwer Aug 9, 2023

holgerd77 Aug 9, 2023

ScottyPoi Aug 10, 2023

jochem-brouwer Dec 29, 2023

scorbajio Jan 3, 2024 •

edited

Loading

jochem-brouwer Aug 9, 2023

ScottyPoi Aug 10, 2023

ScottyPoi Aug 10, 2023

ScottyPoi commented Aug 10, 2023

jochem-brouwer left a comment

jochem-brouwer Dec 29, 2023

scorbajio Jan 3, 2024

jochem-brouwer Dec 29, 2023

holgerd77 commented Dec 30, 2023

jochem-brouwer commented Dec 30, 2023

holgerd77 commented Jan 2, 2024

scorbajio left a comment

jochem-brouwer left a comment

jochem-brouwer Jan 19, 2024

jochem-brouwer Jan 19, 2024

scorbajio Jan 20, 2024 •

edited

Loading

jochem-brouwer Jan 19, 2024

holgerd77 commented Jan 22, 2024

jochem-brouwer left a comment

Trie proof API refactor #2949

Trie proof API refactor #2949

Conversation

ScottyPoi commented Aug 9, 2023 • edited Loading

codecov bot commented Aug 9, 2023 • edited Loading

Codecov Report

scorbajio commented Aug 9, 2023

jochem-brouwer 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

scorbajio Aug 9, 2023 • edited Loading

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

scorbajio Jan 3, 2024 • edited Loading

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

ScottyPoi commented Aug 10, 2023

jochem-brouwer 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

holgerd77 commented Dec 30, 2023

jochem-brouwer commented Dec 30, 2023

holgerd77 commented Jan 2, 2024

scorbajio left a comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scorbajio Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Jan 22, 2024

jochem-brouwer left a comment

Choose a reason for hiding this comment

ScottyPoi commented Aug 9, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading

scorbajio Aug 9, 2023 •

edited

Loading

scorbajio Jan 3, 2024 •

edited

Loading

scorbajio Jan 20, 2024 •

edited

Loading