Add `trie.del` #3486

acolytec3 · 2024-07-05T15:28:10Z

Adds trie.del as a simple extension of trie.put

Adds a new trie.del function which wraps trie.put
Add special handling in trie.put for the deleted leaf value
1. If no leaf node exists at the specified key (i.e. findPath returns no leaf node), return early (since you can't delete a value in a leaf node that doesn't exist)
2. Writes the special VerkleLeafNodeValues.Deleted when inserting the value into the leaf node.

gabrocheleau · 2024-07-06T12:06:45Z

packages/verkle/src/verkleTree.ts

@@ -250,11 +250,28 @@ export class VerkleTree {
        )
      }
    } else {
+      if (equalsBytes(value, createDeletedLeafValue())) {


Should this not throw an error instead of silently returning?

Now that I've re-read the spec, I'm not sure what the expected behavior is. If it's just writing 0 to the trie, then this behavior is actually invalid and we should just let it create a new leaf node and write what I'm calling the deleted leaf value here. Will ask kev

My instinct here is that we should actually just create a new leaf node and write zeroes to it. There is no "deletion" in verkle tries so writing zeroes to a leaf value is the same as "deleting" a value in the trie. Will update this section of code accordingly (i.e. just remove this special handling and allow `trie.del to write zeroes)

packages/verkle/test/verkle.spec.ts

gabrocheleau · 2024-07-06T12:16:00Z

packages/verkle/test/verkle.spec.ts

@@ -211,7 +211,7 @@ describe('Verkle tree', () => {
    assert.deepEqual(val2, hexToBytes(values[2]), 'confirm values[2] can be retrieved from trie')
  })



We could also add a test for the expected behavior when attempting to delete a value that does not exist? Left a related comment in the other file (i.e. should this throw or return?)

gabrocheleau

Nice work, cool to see so much progress on the verkle tree front lately!
Left some comments, can be merged once addressed.

codecov · 2024-07-06T12:19:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (d24ca11) to head (380beec).
Report is 46 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (d24ca11) and HEAD (380beec). Click for more details.

HEAD has 4 uploads less than BASE

Flag BASE (d24ca11) HEAD (380beec)

common 1 0

blockchain 1 0

util 1 0

statemanager 1 0

Additional details and impacted files

Flag	Coverage Δ
blockchain	`?`
common	`?`
statemanager	`?`
util	`?`
wallet	`0.00% <ø> (?)`

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

gabrocheleau · 2024-07-09T16:17:05Z

packages/verkle/src/verkleTree.ts

+          ['DEL']
+        )
+      leafNode.setValue(suffix, VerkleLeafNodeValue.Deleted)
+    }
    leafNode.setValue(suffix, value)


Isn't line 268 overwriting what has been done in line 266 within the if block?

Indeed. Will address in the next commit.

packages/verkle/test/verkle.spec.ts

gabrocheleau · 2024-07-09T18:43:55Z

One general Q. We currently return undefined for both VerkleNodeLeafValues (Untouched and Deleted) when calling getValue on a leaf node. Is this what we want? Are there ever cases where blurring the distinction between Ontouched & Deleted would be problematic (in which case we might want to return undefined & null respectively, although I know you're not a big fan of treating those differently?)

acolytec3 · 2024-07-09T18:49:23Z

One general Q. We currently return undefined for both VerkleNodeLeafValues (Untouched and Deleted) when calling getValue on a leaf node. Is this what we want? Are there ever cases where blurring the distinction between Ontouched & Deleted would be problematic (in which case we might want to return undefined & null respectively, although I know you're not a big fan of treating those differently?)

Good point. We should returns an array of zeroes for the "deleted" case. This comes back to the underlying point that there really is no concept of "deleted" in a verkle trie. There's just untouched, and then touched (with maybe a special flag for zeroes). For the moment, I think it makes the most sense to update trie.get to return an array of 0s unless the leaf value requested is untouched.

gabrocheleau · 2024-07-09T18:57:01Z

One general Q. We currently return undefined for both VerkleNodeLeafValues (Untouched and Deleted) when calling getValue on a leaf node. Is this what we want? Are there ever cases where blurring the distinction between Ontouched & Deleted would be problematic (in which case we might want to return undefined & null respectively, although I know you're not a big fan of treating those differently?)

Good point. We should returns an array of zeroes for the "deleted" case. This comes back to the underlying point that there really is no concept of "deleted" in a verkle trie. There's just untouched, and then touched (with maybe a special flag for zeroes). For the moment, I think it makes the most sense to update trie.get to return an array of 0s unless the leaf value requested is untouched.

Makes sense! Thanks for addressing.

acolytec3 added 3 commits July 5, 2024 11:21

add trie.del function and tests

bf2e879

lint

634b1db

Add last put to test

084ddb3

acolytec3 added the package: verkle label Jul 5, 2024

gabrocheleau reviewed Jul 6, 2024

View reviewed changes

packages/verkle/test/verkle.spec.ts Outdated Show resolved Hide resolved

gabrocheleau reviewed Jul 6, 2024

View reviewed changes

test: adjust test case description

2ba791c

Allow zeroes to be written for non-existent leafnode

021402e

gabrocheleau reviewed Jul 9, 2024

View reviewed changes

acolytec3 added 3 commits July 9, 2024 13:52

clean up utility methods

564d576

address feedback, lint

df15d50

fixes

380beec

gabrocheleau reviewed Jul 9, 2024

View reviewed changes

packages/verkle/test/verkle.spec.ts Outdated Show resolved Hide resolved

Update packages/verkle/test/verkle.spec.ts

8be9ee3

return zeroes when leaf value is "deleted"

256e19b

gabrocheleau previously approved these changes Jul 9, 2024

View reviewed changes

fix test

0efa9c5

acolytec3 dismissed gabrocheleau’s stale review via 0efa9c5 July 9, 2024 19:02

gabrocheleau self-requested a review July 9, 2024 19:12

gabrocheleau approved these changes Jul 9, 2024

View reviewed changes

gabrocheleau merged commit c6ff99a into master Jul 9, 2024
34 checks passed

holgerd77 deleted the add-trie.del branch July 9, 2024 20:09

Mikhai56 approved these changes Aug 10, 2024

View reviewed changes

ethereumjs deleted a comment from Mikhai56 Aug 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `trie.del` #3486

Add `trie.del` #3486

acolytec3 commented Jul 5, 2024

gabrocheleau Jul 6, 2024

acolytec3 Jul 6, 2024

acolytec3 Jul 9, 2024

gabrocheleau Jul 6, 2024

gabrocheleau left a comment

codecov bot commented Jul 6, 2024 •

edited

Loading

gabrocheleau Jul 9, 2024

acolytec3 Jul 9, 2024

gabrocheleau commented Jul 9, 2024

acolytec3 commented Jul 9, 2024

gabrocheleau commented Jul 9, 2024

		@@ -211,7 +211,7 @@ describe('Verkle tree', () => {
		assert.deepEqual(val2, hexToBytes(values[2]), 'confirm values[2] can be retrieved from trie')
		})

Add trie.del #3486

Add trie.del #3486

Conversation

acolytec3 commented Jul 5, 2024

gabrocheleau Jul 6, 2024

Choose a reason for hiding this comment

acolytec3 Jul 6, 2024

Choose a reason for hiding this comment

acolytec3 Jul 9, 2024

Choose a reason for hiding this comment

gabrocheleau Jul 6, 2024

Choose a reason for hiding this comment

gabrocheleau left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2024 • edited Loading

Codecov Report

gabrocheleau Jul 9, 2024

Choose a reason for hiding this comment

acolytec3 Jul 9, 2024

Choose a reason for hiding this comment

gabrocheleau commented Jul 9, 2024

acolytec3 commented Jul 9, 2024

gabrocheleau commented Jul 9, 2024

Add `trie.del` #3486

Add `trie.del` #3486

codecov bot commented Jul 6, 2024 •

edited

Loading