Implement trie.put
#3473
Implement trie.put
#3473
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| // TODO: Rewrite following logic in verkle.spec.ts "findPath validation" test | ||
| async put(_key: Uint8Array, _value: Uint8Array): Promise<void> { | ||
| throw new Error('not implemented') | ||
| async put(key: Uint8Array, value: Uint8Array): Promise<void> { |
There was a problem hiding this comment.
This function is very long. I find functions more difficult to read and debug when I have to scroll in order to see different parts of it.
I appreciate the way you use the comments to outline how things are happening step by step. I wonder if these different steps could be externalized into separate functions, so that the put function itself can fit on a screen?
There was a problem hiding this comment.
This is a 100% valid criticism. Lemme look at the logic and see if there's a way to break it down. I think you're right that it can be done in smaller chunks.
packages/verkle/src/verkleTree.ts
Outdated
| if (foundPath.node.type !== VerkleNodeType.Leaf) { | ||
| throw new Error( | ||
| `expected leaf node found at ${bytesToHex(stem)}. Got internal node instead` | ||
| ) | ||
| } | ||
| leafNode = foundPath.node as LeafNode |
There was a problem hiding this comment.
I can create some type guards for those, I think this will be useful in qute a few places.
There was a problem hiding this comment.
Just did: 9b00803 (#3473)
| let lastPath = leafNode.stem | ||
|
|
||
| // Step 2) Determine if a new internal node is needed | ||
| if (foundPath.stack.length > 1) { |
There was a problem hiding this comment.
Just to make sure I am getting this correctly, could you confirm that this scenario is correct:
- Let's say we start with a root node
- We put a key/value pair where the key starts with
0x01. findPathreturns the rootNode.- We create a leafNode, and add a pointer to the newly created leaf node at index
2of the root node. - We put another key/value pair that also starts with
0x01. findPathnow returns theleafNodethat was created in the previous put operation, along with theremainingvalues where the paths branch off.- We then create an internal node where the paths branch off, commit to the two children to it, and then commit the internal node to the root node (at index
2). - rootHash gets updated.
There was a problem hiding this comment.
That should be how it works. Not guaranteeing I got it exactly right but the tests seem to indicate so
There was a problem hiding this comment.
Cool, I didn't infer the above from how the function/tests were structured, but wrote it based on my understanding of verkle trees, good to hear it matches
There was a problem hiding this comment.
Looking at your comment again, let me confirm something. What you call "index 2" here would be rootNode.children[1] right? These are zero indexed arrays and so inserting a node that starts with 0x01 means you check rootNode.children[1] to see if there's an existing reference to some child node and if not, create a new leaf node with the value for the key that starts with 0x01 and then put the commitment from that new leaf node in rootNode.children[1]
There was a problem hiding this comment.
Ah yes that's what I meant, sorry for the confusing indexing
packages/verkle/src/verkleTree.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Helper method for updating or creating the parent internal node for a given leaf node | ||
| * @param leafNode the reference leaf node |
There was a problem hiding this comment.
By reference leaf node I assume you mean the "new leaf node". Do you think that "newLeafNode/new leaf node" would be a clearer naming?
There was a problem hiding this comment.
Hmm, it could be a new leaf node but it could also just be an existing leaf node where we've updated a value. In this instance, we wouldn't be creating a new internal node above but would still update the parent. Maybe I can call it "child leaf node"?
There was a problem hiding this comment.
Ah that's fair. I'm not actually sure I find child leaf node clearer. I think the comment makes it clear enough, thanks for addressing.
This builds out
trie.putfor theVerkleTrieclass