Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add trie.del #3486

Merged
merged 11 commits into from
Jul 9, 2024
Merged

Add trie.del #3486

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bf2e879
add trie.del function and tests
acolytec3 Jul 5, 2024
634b1db
lint
acolytec3 Jul 5, 2024
084ddb3
Add last put to test
acolytec3 Jul 5, 2024
2ba791c
test: adjust test case description
gabrocheleau Jul 6, 2024
021402e
Allow zeroes to be written for non-existent leafnode
acolytec3 Jul 9, 2024
564d576
clean up utility methods
acolytec3 Jul 9, 2024
df15d50
address feedback, lint
acolytec3 Jul 9, 2024
380beec
fixes
acolytec3 Jul 9, 2024
8be9ee3
Update packages/verkle/test/verkle.spec.ts
gabrocheleau Jul 9, 2024
256e19b
return zeroes when leaf value is "deleted"
acolytec3 Jul 9, 2024
0efa9c5
fix test
acolytec3 Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion packages/verkle/src/verkleTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { loadVerkleCrypto } from 'verkle-cryptography-wasm'
import { CheckpointDB } from './db/checkpoint.js'
import { InternalNode } from './node/internalNode.js'
import { LeafNode } from './node/leafNode.js'
import { type VerkleNode } from './node/types.js'
import { VerkleLeafNodeValue, type VerkleNode, createDeletedLeafValue } from './node/types.js'
import { decodeNode, isLeafNode } from './node/util.js'
import {
type Proof,
Expand Down Expand Up @@ -250,11 +250,28 @@ export class VerkleTree {
)
}
} else {
if (equalsBytes(value, createDeletedLeafValue())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not throw an error instead of silently returning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

// Special case for when the deleted leaf value is passed to `put`
// You can't delete a value on a leaf node that doesn't exist
this.DEBUG &&
this.debug(`Leaf node with stem: ${bytesToHex(stem)} not found in trie`, ['DEL'])
return
}
// Leaf node doesn't exist, create a new one
leafNode = await LeafNode.create(stem, this.verkleCrypto)
this.DEBUG && this.debug(`Creating new leaf node at stem: ${bytesToHex(stem)}`, ['PUT'])
}
// Update value in leaf node and push to putStack
if (equalsBytes(value, createDeletedLeafValue())) {
// Special case for when the deleted leaf value is passed to `put`
// Writing the deleted leaf value to the suffix indicated in the key
this.DEBUG &&
this.debug(
`Deleting value at suffix: ${suffix} in leaf node with stem: ${bytesToHex(stem)}`,
['DEL']
)
leafNode.setValue(suffix, VerkleLeafNodeValue.Deleted)
}
leafNode.setValue(suffix, value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Will address in the next commit.

this.DEBUG &&
this.debug(
Expand Down Expand Up @@ -319,6 +336,12 @@ export class VerkleTree {
await this.saveStack(putStack)
}

async del(key: Uint8Array): Promise<void> {
const stem = key.slice(0, 31)
const suffix = key[key.length - 1]
this.DEBUG && this.debug(`Stem: ${bytesToHex(stem)}; Suffix: ${suffix}`, ['DEL'])
await this.put(key, createDeletedLeafValue())
}
/**
* Helper method for updating or creating the parent internal node for a given leaf node
* @param leafNode the child leaf node that will be referenced by the new/updated internal node
Expand Down
8 changes: 7 additions & 1 deletion packages/verkle/test/verkle.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ describe('Verkle tree', () => {
assert.deepEqual(val2, hexToBytes(values[2]), 'confirm values[2] can be retrieved from trie')
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

it('should put values and find them', async () => {
it('should put, find, delete, and the put values (again)', async () => {
acolytec3 marked this conversation as resolved.
Show resolved Hide resolved
const keys = [
// Two keys with the same stem but different suffixes
'0x318dea512b6f3237a2d4763cf49bf26de3b617fb0cabe38a97807a5549df4d01',
Expand Down Expand Up @@ -241,5 +241,11 @@ describe('Verkle tree', () => {
assert.deepEqual(await trie.get(hexToBytes(keys[0])), hexToBytes(values[0]))
assert.deepEqual(await trie.get(hexToBytes(keys[2])), hexToBytes(values[2]))
assert.deepEqual(await trie.get(hexToBytes(keys[3])), hexToBytes(values[3]))

await trie.del(hexToBytes(keys[0]))
assert.deepEqual(await trie.get(hexToBytes(keys[0])), undefined)

await trie.put(hexToBytes(keys[0]), hexToBytes(values[0]))
assert.deepEqual(await trie.get(hexToBytes(keys[0])), hexToBytes(values[0]))
})
})
Loading