From 0f863b3d1319805d0be25fc68b03d47b82ce516e Mon Sep 17 00:00:00 2001 From: Gabriel Rocheleau Date: Tue, 23 Apr 2024 08:17:10 -0700 Subject: [PATCH 1/6] trie: fix del stack operation key formatting --- packages/trie/src/trie.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/trie/src/trie.ts b/packages/trie/src/trie.ts index ea2d9a14d3..522acaac8d 100644 --- a/packages/trie/src/trie.ts +++ b/packages/trie/src/trie.ts @@ -1,6 +1,7 @@ // Some more secure presets when using e.g. JS `call` 'use strict' +import { RLP } from '@ethereumjs/rlp' import { KeyEncoding, Lock, @@ -568,8 +569,11 @@ export class Trie { const deleteHashes = stack.map((e) => this.hash(e.serialize())) // Just as with `put`, the stack items all will have their keyhashes updated // So after deleting the node, one can safely delete these from the DB - ops = deleteHashes.map((e) => { - const key = this._opts.keyPrefix ? concatBytes(this._opts.keyPrefix, e) : e + + ops = deleteHashes.map((deletedHash) => { + const key = this._opts.keyPrefix + ? concatBytes(this._opts.keyPrefix, deletedHash) + : deletedHash return { type: 'del', key, @@ -1020,9 +1024,11 @@ export class Trie { // Since this branch is deleted, one can thus also delete this branch from the DB // So add this to the `opStack` and mark the keyhash to be deleted if (this._opts.useNodePruning) { + // If the branchNode has length < 32, it will be a Uint8Array[] instead of a Uint8Array + // In that case, we need to serialize and hash it into a Uint8Array, otherwise the operation will throw opStack.push({ type: 'del', - key: branchNode as Uint8Array, + key: key instanceof Uint8Array ? key : this.appliedKey(RLP.encode(key)), }) } From 7fbf55a8752e09cdabbe20513de25a9d4669808d Mon Sep 17 00:00:00 2001 From: Gabriel Rocheleau Date: Sat, 27 Apr 2024 10:39:55 -0700 Subject: [PATCH 2/6] trie: add test case --- packages/trie/test/trie/prune.spec.ts | 34 ++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/trie/test/trie/prune.spec.ts b/packages/trie/test/trie/prune.spec.ts index 716cb411d1..bee6d082bb 100644 --- a/packages/trie/test/trie/prune.spec.ts +++ b/packages/trie/test/trie/prune.spec.ts @@ -1,7 +1,9 @@ import { KECCAK256_RLP, equalsBytes, hexToBytes, randomBytes, utf8ToBytes } from '@ethereumjs/util' import { assert, describe, it } from 'vitest' -import { Trie } from '../../src/index.js' +import { Trie, isRawNode } from '../../src/index.js' + +import type { BranchNode } from '../../src/index.js' describe('Pruned trie tests', () => { it('should default to not prune the trie', async () => { @@ -151,6 +153,36 @@ describe('Pruned trie tests', () => { } }) + it('should successfully delete branch nodes that are <32 bytes length with node pruning', async () => { + // This test case was added after issue #3333 uncovered a problem with pruning trie paths that reference + // nodes with their unhashed values (occurs when nodes are less than <32 bytes). + const trie = new Trie({ + useNodePruning: true, + }) + + await trie.put(utf8ToBytes('key1'), utf8ToBytes('value1')) + const initialRoot = trie.root() + + await trie.put(utf8ToBytes('key2'), utf8ToBytes('value2')) + + // Because of the small values, the leaf nodes will be less than 32 bytes in length. + // As per the MPT spec, they will therefore be referenced directly and not by their hash. + // We should therefore expect two BranchNode branches that reference these 2 leaf nodes directly, instead of by their hashes. + // If a node is referenced directly, the item will be a RawNode (a Uint8Array[]). If it's referenced by its hash, it will be a Uint8Array + const path = await trie.findPath(utf8ToBytes('key1')) + const parentBranchNode = path.stack[1] as BranchNode + // Hex ASCII value for for `1` is 31, and for `2` is 32. We should expect a branching out at indexes 1 an 2. + assert.ok(isRawNode(parentBranchNode._branches[1]!), 'key1 node is not a rawNode') + assert.ok(isRawNode(parentBranchNode._branches[2]!), 'key2 node is not a rawNode') + + assert.notOk(equalsBytes(trie.root(), initialRoot), 'Root should have changed') + + // Delete the branch node + await trie.del(utf8ToBytes('key2')) + + assert.ok(equalsBytes(trie.root(), initialRoot), 'Root should be the same as the initial root') + }) + it('verifyPrunedIntegrity() => should correctly report unpruned Tries', async () => { // Create empty Trie (is pruned) let trie = new Trie() From 6c096a91d523e9d212600380c70e5369147dbc6e Mon Sep 17 00:00:00 2001 From: Gabriel Rocheleau Date: Sat, 27 Apr 2024 10:51:18 -0700 Subject: [PATCH 3/6] trie: more tentative fixes2 --- packages/trie/src/trie.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/trie/src/trie.ts b/packages/trie/src/trie.ts index 522acaac8d..e025a8c50b 100644 --- a/packages/trie/src/trie.ts +++ b/packages/trie/src/trie.ts @@ -527,7 +527,7 @@ export class Trie { // (This is the path from the root node to wherever it needs to insert nodes) // The items change, because the leaf value is updated, thus all keyhashes in the // stack should be updated as well, so that it points to the right key/value pairs of the path - const deleteHashes = stack.map((e) => this.hash(e.serialize())) + const deleteHashes = stack.map((e) => this.appliedKey(e.serialize())) ops = deleteHashes.map((e) => { const key = this._opts.keyPrefix ? concatBytes(this._opts.keyPrefix, e) : e return { @@ -1175,7 +1175,7 @@ export class Trie { // Track if key is found let found = false try { - await this.walkTrie(this.root(), async function (nodeRef, node, key, controller) { + await this.walkTrie(this.root(), async function (_, node, key, controller) { if (found) { // Abort all other children checks return @@ -1183,7 +1183,13 @@ export class Trie { if (node instanceof BranchNode) { for (const item of node._branches) { // If one of the branches matches the key, then it is found - if (item !== null && bytesToUnprefixedHex(item as Uint8Array) === dbkey) { + if ( + item !== null && + item.length !== 0 && + bytesToUnprefixedHex( + isRawNode(item) ? controller.trie.appliedKey(RLP.encode(item)) : item + ) === dbkey + ) { found = true return } From e2ebc23b20326814cf2a9c47c93ca5d6ee986ca2 Mon Sep 17 00:00:00 2001 From: Gabriel Rocheleau Date: Sun, 28 Apr 2024 15:43:53 -0700 Subject: [PATCH 4/6] trie: update branchnode parsing --- packages/trie/src/trie.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/trie/src/trie.ts b/packages/trie/src/trie.ts index e025a8c50b..365e933b28 100644 --- a/packages/trie/src/trie.ts +++ b/packages/trie/src/trie.ts @@ -1028,7 +1028,7 @@ export class Trie { // In that case, we need to serialize and hash it into a Uint8Array, otherwise the operation will throw opStack.push({ type: 'del', - key: key instanceof Uint8Array ? key : this.appliedKey(RLP.encode(key)), + key: isRawNode(branchNode) ? this.appliedKey(RLP.encode(branchNode)) : branchNode, }) } From 09befb2aadcab2bcdf1ca5485e952aceaf228b80 Mon Sep 17 00:00:00 2001 From: Gabriel Rocheleau Date: Sun, 28 Apr 2024 16:14:08 -0700 Subject: [PATCH 5/6] trie: simplifications --- packages/trie/src/trie.ts | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/packages/trie/src/trie.ts b/packages/trie/src/trie.ts index 365e933b28..5a153f69c5 100644 --- a/packages/trie/src/trie.ts +++ b/packages/trie/src/trie.ts @@ -1024,7 +1024,7 @@ export class Trie { // Since this branch is deleted, one can thus also delete this branch from the DB // So add this to the `opStack` and mark the keyhash to be deleted if (this._opts.useNodePruning) { - // If the branchNode has length < 32, it will be a Uint8Array[] instead of a Uint8Array + // If the branchNode has length < 32, it will be a RawNode (Uint8Array[]) instead of a Uint8Array // In that case, we need to serialize and hash it into a Uint8Array, otherwise the operation will throw opStack.push({ type: 'del', @@ -1061,24 +1061,24 @@ export class Trie { // update nodes while (stack.length) { - const node = stack.pop() as TrieNode - if (node instanceof LeafNode) { - key.splice(key.length - node.key().length) - } else if (node instanceof ExtensionNode) { + const node = stack.pop() + if (node === undefined) { + throw new Error('saveStack: missing node') + } + if (node instanceof LeafNode || node instanceof ExtensionNode) { key.splice(key.length - node.key().length) - if (lastRoot) { - node.value(lastRoot) - } - } else if (node instanceof BranchNode) { - if (lastRoot) { - const branchKey = key.pop() - node.setBranch(branchKey!, lastRoot) - } + } + if (node instanceof ExtensionNode && lastRoot !== undefined) { + node.value(lastRoot) + } + if (node instanceof BranchNode && lastRoot !== undefined) { + const branchKey = key.pop() + node.setBranch(branchKey!, lastRoot) } lastRoot = this._formatNode(node, stack.length === 0, opStack) as Uint8Array } - if (lastRoot) { + if (lastRoot !== undefined) { this.root(lastRoot) } @@ -1185,7 +1185,6 @@ export class Trie { // If one of the branches matches the key, then it is found if ( item !== null && - item.length !== 0 && bytesToUnprefixedHex( isRawNode(item) ? controller.trie.appliedKey(RLP.encode(item)) : item ) === dbkey From 644dd0ed24e4aa70784b1802b318c48aacbd2dff Mon Sep 17 00:00:00 2001 From: Gabriel Rocheleau Date: Sun, 28 Apr 2024 16:28:34 -0700 Subject: [PATCH 6/6] trie: fix appliedkey vs hash --- packages/trie/src/trie.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/trie/src/trie.ts b/packages/trie/src/trie.ts index 5a153f69c5..582c4f5e06 100644 --- a/packages/trie/src/trie.ts +++ b/packages/trie/src/trie.ts @@ -527,9 +527,12 @@ export class Trie { // (This is the path from the root node to wherever it needs to insert nodes) // The items change, because the leaf value is updated, thus all keyhashes in the // stack should be updated as well, so that it points to the right key/value pairs of the path - const deleteHashes = stack.map((e) => this.appliedKey(e.serialize())) - ops = deleteHashes.map((e) => { - const key = this._opts.keyPrefix ? concatBytes(this._opts.keyPrefix, e) : e + const deleteHashes = stack.map((e) => this.hash(e.serialize())) + ops = deleteHashes.map((deletedHash) => { + const key = this._opts.keyPrefix + ? concatBytes(this._opts.keyPrefix, deletedHash) + : deletedHash + return { type: 'del', key,