trie: trie package renaming to mpt #3719

gabrocheleau · 2024-10-03T15:00:13Z

This PR renames the trie package to mpt

jochem-brouwer

Some comments. We should also update https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/README.md (and update the picture with dependencies to use mpt instead of trie)

jochem-brouwer · 2024-10-03T15:02:48Z

packages/block/package.json

@@ -49,7 +49,7 @@
  "dependencies": {
    "@ethereumjs/common": "^4.4.0",
    "@ethereumjs/rlp": "^5.0.2",
-    "@ethereumjs/trie": "^6.2.1",
+    "@ethereumjs/mpt": "^6.2.1",


Not sure if this is clean in the PR, do we want this rename + keep the same version here? 🤔 (I realize that this will likely mean we need a release of the new package though to pass CI - or will this be handled correctly?)

I thought the same, seemed weird not to update the version at all

jochem-brouwer · 2024-10-03T15:03:32Z

packages/client/archive/libp2p/net/package.json.browser.deps

@@ -65,7 +65,7 @@
    "@ethereumjs/evm": "2.1.0",
    "@ethereumjs/rlp": "5.0.1",
    "@ethereumjs/statemanager": "2.1.0",
-    "@ethereumjs/trie": "6.0.1",
+    "@ethereumjs/mpt": "6.0.1",


This points to mpt 6.0.1 which will (likely?) not hit a package at all?

I have updated everything to 6.2.2

jochem-brouwer · 2024-10-03T15:13:26Z

packages/client/test/sync/fetcher/trienodefetcher.spec.ts

@@ -1,5 +1,5 @@
+import { decodeNode } from '@ethereumjs/mpt'


Hmm upon seeing this I realized that when renaming the MPT methods we might not have renamed them all.

I checked the MPT exports: (might be good to look over those all to see if we can make any improvements here)

{ createMPT: [Getter], createMPTFromProof: [Getter], CheckpointDB: [Getter], MerklePatriciaTrie: [Getter], BranchMPTNode: [Getter], ExtensionMPTNode: [Getter], LeafMPTNode: [Getter], decodeRawNode: [Getter], isRawNode: [Getter], decodeNode: [Getter], verifyMPTProof: [Getter], verifyMPTRangeProof: [Getter], createMerkleProof: [Getter], updateMPTFromMerkleProof: [Getter], verifyMerkleProof: [Getter], verifyRangeProof: [Getter], ROOT_DB_KEY: [Getter], hasTerminator: [Getter], nibblesToBytes: [Getter], hexToKeybytes: [Getter], nibblesToCompactBytes: [Getter], bytesToNibbles: [Getter], compactBytesToNibbles: [Getter], nibbleTypeToByteType: [Getter], byteTypeToNibbleType: [Getter], pathToHexKey: [Getter], mergeAndFormatKeyPaths: [Getter], genesisStateRoot: [Getter], WalkController: [Getter] }

Suggestions for rename:

decodeRawNode -> decodeRawMPTNode
isRawNode -> isRawMPTNode
decodeNode -> decodeMPTNode

Names which might not be specific enough:

hasTerminator

We also have verifyMerkleProof, createMerkleProof, verifyRangeProof, this starts to get somewhat confusing? 🤔

Hmm upon seeing this I realized that when renaming the MPT methods we might not have renamed them all.

I checked the MPT exports: (might be good to look over those all to see if we can make any improvements here)

{ createMPT: [Getter], createMPTFromProof: [Getter], CheckpointDB: [Getter], MerklePatriciaTrie: [Getter], BranchMPTNode: [Getter], ExtensionMPTNode: [Getter], LeafMPTNode: [Getter], decodeRawNode: [Getter], isRawNode: [Getter], decodeNode: [Getter], verifyMPTProof: [Getter], verifyMPTRangeProof: [Getter], createMerkleProof: [Getter], updateMPTFromMerkleProof: [Getter], verifyMerkleProof: [Getter], verifyRangeProof: [Getter], ROOT_DB_KEY: [Getter], hasTerminator: [Getter], nibblesToBytes: [Getter], hexToKeybytes: [Getter], nibblesToCompactBytes: [Getter], bytesToNibbles: [Getter], compactBytesToNibbles: [Getter], nibbleTypeToByteType: [Getter], byteTypeToNibbleType: [Getter], pathToHexKey: [Getter], mergeAndFormatKeyPaths: [Getter], genesisStateRoot: [Getter], WalkController: [Getter] }

Suggestions for rename:

decodeRawNode -> decodeRawMPTNode isRawNode -> isRawMPTNode decodeNode -> decodeMPTNode

Names which might not be specific enough:

hasTerminator

We also have verifyMerkleProof, createMerkleProof, verifyRangeProof, this starts to get somewhat confusing? 🤔

I have now:

Updated all the ones you have suggested

Removed the hasTerminator export. It is only used as a helper within the file it's being declared, and it's unlikely that external usage would require such a niche and implementation-specific helper imo.

I am currently going through the different methods that have non-mpt names and trying to take this opportunity to make them clearer. Easy example is genesisStateRoot -> genesisMPTStateRoot. The merkle proofs are a bit more tricky since there is some overlap in naming, need to first familiarize myself with what each of them does.

Cool, will take a look. Did you also see Holger's comment on the previously merged PR? :) #3718 (comment)

I think this is now in a state where this can be reviewed again. I believe I've gone through all the renamings and made them clearer when possible.

jochem-brouwer · 2024-10-03T15:16:11Z

packages/mpt/package.json

  "bugs": {
-    "url": "https://github.com/ethereumjs/ethereumjs-monorepo/issues?q=is%3Aissue+label%3A%22package%3A+trie%22"
+    "url": "https://github.com/ethereumjs/ethereumjs-monorepo/issues?q=is%3Aissue+label%3A%22package%3A+mpt%22"


Ah ok, we should remove the package: trie label (or at least add package: mpt label) and update our general issue template to also point to the mpt label and not the trie label.

…ethereumjs/ethereumjs-monorepo into trie/rename-trie-package-to-mpt

jochem-brouwer · 2024-10-05T21:02:09Z

Side note: for this PR the trie /test trie (required job) is diabled and renamed, so this will need some coordination in order to get this merged (move required job to mpt/ test-mpt (not sure if this name is correct, but retrieved it from the passed CI jobs))

codecov · 2024-10-06T13:08:08Z

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 6.29%. Comparing base (4470cc3) to head (63f2d1c).
Report is 86 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	`?`
client	`0.00% <0.00%> (ø)`
tx	`76.70% <ø> (-1.08%)`	⬇️
wallet	`?`

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

gabrocheleau · 2024-10-06T21:59:43Z

Side note: for this PR the trie /test trie (required job) is diabled and renamed, so this will need some coordination in order to get this merged (move required job to mpt/ test-mpt (not sure if this name is correct, but retrieved it from the passed CI jobs))

Good point!

I do not think I have permission to do this. Neither can I edit the issue template, which should be renamed from trie to mpt. If someone else could handle that, that'd be great. Not sure who has permission aside from @holgerd77

jochem-brouwer · 2024-10-06T22:06:44Z

I think Holger is the only person who has admin rights to do so :)

holgerd77

Ok, I am sold on everything, thanks for the extensive answers and continued discussions on how to properly name things! 🤩

I've adjusted the CI mpt/trie thing (also for future runs), browser tests were hanging, seemed unrelated but I have for now deactivated so that we can merge this huge beast in. We should have an eye upon that though and eventually fix on follow-up.

holgerd77 · 2024-10-07T11:17:57Z

What's with verifyRangeProof() in proof/range.ts ? 🤔

gabrocheleau · 2024-10-07T12:10:07Z

What's with verifyRangeProof() in proof/range.ts ? 🤔

This one, to my knowledge, is only used internally by the verifyRangeMerkleProof in proof/index.ts, so I opted to leave it as is, but to not expose it externally

holgerd77 · 2024-10-08T10:53:09Z

At least the proof functionality naming is not consistent yet:

(or is it in some way I do not grasp?)

holgerd77 · 2024-10-08T10:54:20Z

Respectively: it's only verifyMPTProof(), which is not yet correct, isn't it?

holgerd77 · 2024-10-08T10:56:24Z

(in case you do a PR here, can you also update and put the code from proof/index.ts into its own file (I would suggest merkle.ts) and use index.ts only for the exports, as we do by convention? That would be nice! 🙂 )

gabrocheleau · 2024-10-08T16:13:09Z

(in case you do a PR here, can you also update and put the code from proof/index.ts into its own file (I would suggest merkle.ts) and use index.ts only for the exports, as we do by convention? That would be nice! 🙂 )

Done here: #3730

trie: trie package renaming to mpt

9bc20e5

gabrocheleau added the package: mpt label Oct 3, 2024

adjust ci with mpt

1533156

gabrocheleau added breaking PR state: needs review type: maintenance labels Oct 3, 2024

jochem-brouwer reviewed Oct 3, 2024

View reviewed changes

gabrocheleau added 2 commits October 3, 2024 18:42

trie: trie package renaming to mpt

d645f60

adjust ci with mpt

f564f41

ScottyPoi force-pushed the trie/rename-trie-package-to-mpt branch from 1533156 to f564f41 Compare October 4, 2024 00:42

Merge branch 'trie/rename-trie-package-to-mpt' of https://github.com/…

e7b6f43

…ethereumjs/ethereumjs-monorepo into trie/rename-trie-package-to-mpt

jochem-brouwer mentioned this pull request Oct 5, 2024

trie: rename trie helpers to mpt #3718

Merged

gabrocheleau added 3 commits October 6, 2024 08:23

mpt: update version

4f11a51

trie: renaming to mpt for clarity

3999c4a

mpt: rename

63f2d1c

gabrocheleau added 5 commits October 6, 2024 14:50

trie: adjust proof naming scheme

4b31e10

trie: fix isRawMPTNode import

d0ebdcf

trie: don't export internally used verifyRangeProof

8ba4cfb

mpt: minor adjustments

fe632cf

mpt: fix example script

8639573

holgerd77 approved these changes Oct 7, 2024

View reviewed changes

holgerd77 merged commit 2f70c44 into master Oct 7, 2024
38 of 39 checks passed

holgerd77 deleted the trie/rename-trie-package-to-mpt branch October 7, 2024 09:11

holgerd77 mentioned this pull request Oct 8, 2024

update README.md to reflect trie -> mpt rename #3727

Merged

gabrocheleau mentioned this pull request Oct 8, 2024

mpt: adjust verifyMPTProof naming and restructure index file #3730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trie: trie package renaming to mpt #3719

trie: trie package renaming to mpt #3719

gabrocheleau commented Oct 3, 2024

jochem-brouwer left a comment

jochem-brouwer Oct 3, 2024

gabrocheleau Oct 3, 2024

jochem-brouwer Oct 3, 2024

gabrocheleau Oct 6, 2024

jochem-brouwer Oct 3, 2024

gabrocheleau Oct 6, 2024

jochem-brouwer Oct 6, 2024

gabrocheleau Oct 6, 2024

jochem-brouwer Oct 3, 2024

jochem-brouwer commented Oct 5, 2024

codecov bot commented Oct 6, 2024 •

edited

Loading

gabrocheleau commented Oct 6, 2024

jochem-brouwer commented Oct 6, 2024

holgerd77 left a comment

holgerd77 commented Oct 7, 2024

gabrocheleau commented Oct 7, 2024

holgerd77 commented Oct 8, 2024

holgerd77 commented Oct 8, 2024

holgerd77 commented Oct 8, 2024

gabrocheleau commented Oct 8, 2024

trie: trie package renaming to mpt #3719

trie: trie package renaming to mpt #3719

Conversation

gabrocheleau commented Oct 3, 2024

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

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

jochem-brouwer commented Oct 5, 2024

codecov bot commented Oct 6, 2024 • edited Loading

Codecov Report

gabrocheleau commented Oct 6, 2024

jochem-brouwer commented Oct 6, 2024

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Oct 7, 2024

gabrocheleau commented Oct 7, 2024

holgerd77 commented Oct 8, 2024

holgerd77 commented Oct 8, 2024

holgerd77 commented Oct 8, 2024

gabrocheleau commented Oct 8, 2024

codecov bot commented Oct 6, 2024 •

edited

Loading