Verkle Implementation: Build out Trie Processing #3430
Conversation
electra.yaml
Outdated
| - el_type: ethereumjs | ||
| el_image: ethereumjs:local | ||
| cl_type: grandine | ||
| cl_image: ethpandaops/grandine:feature-electra |
There was a problem hiding this comment.
Doesn't seem like that should belong to this PR?
There was a problem hiding this comment.
It doesn't. Accidentally included and will remove.
| // Index of the child pointed by the next byte in the key | ||
| const childIndex = stem[this.depth] | ||
|
|
||
| const child = this.children[childIndex] | ||
|
|
||
| if (child instanceof LeafNode) { | ||
| // TODO: Understand the intent of what cowChild is suppoded to do |
There was a problem hiding this comment.
cow stands for "copy-on-write". As far as I recall cowChild is used as a record to mark the children that have been modified, in order to stack the commitment updates at the end since that is way more efficient.
|
|
||
| // TODO - Why is the leaf node set at depth + 2 instead of + 1)? |
There was a problem hiding this comment.
I think that this is because the leaf is inserted in the position of the suffix level commitment. So we have:
depth n : internal node:
depth n+1: the extension level commitment
depth n+2: the suffix level commitment
See this picture for how that corresponds to the structure:
| newBranch.children[nextByteInExistingKey] = child | ||
| child.depth += 1 | ||
|
|
||
| const nextByteInInsertedKey = stem[this.depth + 1] | ||
| if (nextByteInInsertedKey === nextByteInExistingKey) { | ||
| return newBranch.insertStem(stem, values, resolver) | ||
| return newBranch.insertStem(stem, values, resolver, verkleCrypto) |
There was a problem hiding this comment.
| return newBranch.insertStem(stem, values, resolver, verkleCrypto) | |
| return newBranch.insertStem(stem, values, resolver) |
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. |
|
This is ready for final review. |
| static fromRawNode(rawNode: Uint8Array[], depth: number): InternalNode { | ||
| static fromRawNode( | ||
| rawNode: Uint8Array[], | ||
| depth: number, |
There was a problem hiding this comment.
Can we remove depth from this API?
packages/verkle/src/node/leafNode.ts
Outdated
| static async create( | ||
| stem: Uint8Array, | ||
| values: Uint8Array[], | ||
| depth: number, |
There was a problem hiding this comment.
Can we remove depth here too?
packages/verkle/src/node/leafNode.ts
Outdated
| } | ||
|
|
||
| static fromRawNode(rawNode: Uint8Array[], depth: number): LeafNode { | ||
| static fromRawNode(rawNode: Uint8Array[], depth: number, verkleCrypto: VerkleCrypto): LeafNode { |
packages/verkle/src/verkleTree.ts
Outdated
| } | ||
| async findPath(key: Uint8Array): Promise<Path> { | ||
| this.DEBUG && this.debug(`Path (${key.length}): [${bytesToHex(key)}]`, ['FIND_PATH']) | ||
| // TODO: Decide if we should allow keys longer than 31 bytes (since a verkle stem can never be longer than that) |
There was a problem hiding this comment.
Can we remove this Todo? We've already discussed this right?
| // Calculate the index of the last matching byte in the key | ||
| const matchingKeyLength = matchingBytesLength(key, child.path) | ||
| const foundNode = equalsBytes(key, child.path) | ||
| if (foundNode || child.path.length >= key.length || decodedNode instanceof LeafNode) { |
There was a problem hiding this comment.
Is the last condition necessary? If I'm thinking about this correctly, whenever the child path length is longer than the key, the found node would necessarily be a leaf node?
holgerd77
changed the title
| @@ -2,24 +2,22 @@ import { RLP } from '@ethereumjs/rlp' | |||
|
|
|||
| import { type VerkleNodeInterface, type VerkleNodeOptions, type VerkleNodeType } from './types.js' | |||
|
|
|||
| import type { VerkleCrypto } from 'verkle-cryptography-wasm' | |||
There was a problem hiding this comment.
This should now use the import from verkle in Util, just opened a related issue here ethereumjs/verkle-cryptography-wasm#54
I would go as far and argue that we should also remove the verkle-cryptography-wasm dependency from this package as well and dependency inject, and so to have this wider strategic goal over the head that this should be replaceable and also here optimally a JS implementation being used.
We then also automatically continue to evolve a generic interface around this and keep thinking about what's needed and what not (I e.g. still like the idea that we take the more hashing related parts out directly and move over to the pedersen/poseidon hash usage from Pauls libraries https://github.com/paulmillr/scure-starknet (if applicable) and with this slowly shrink down the surface of what we need to compile over into the WASM stuff.
Feels to me that there is still too much in.
Practical note: if depency injection feels a bit impractical for now for testing and the like we could also decide to do at a later stage.
There was a problem hiding this comment.
Ah, but the main argument for direct dependency injection (so, this would mean: having a mandatory crypto constructor argument) is actually that we avoid having the same bundle problems again once/if we have Verkle added somewhere as a dependency.
Co-authored-by: Gabriel Rocheleau <contact@rockwaterweb.com>
packages/verkle/src/verkleTree.ts
Outdated
| async put(_key: Uint8Array, _value: Uint8Array): Promise<void> { | ||
| throw new Error('not implemented') | ||
| // verifyKeyLength(key) | ||
| // const stem = key.slice(0, 31) | ||
| // this.DEBUG && this.debug(`Key: ${bytesToHex(key)}`, ['PUT']) | ||
| // this.DEBUG && this.debug(`Value: ${bytesToHex(value)}`, ['PUT']) |
There was a problem hiding this comment.
Not a big fan of having a lot of commented out code merge into the master branch. Could we take note of this code peripherally and just re-add it in the follow-up PR? Not a major deal as this is an experimental package though
There was a problem hiding this comment.
Sure, have stashed it locally
Foundations for a full verkle trie implementation:
VerkleTrieclass to remove unnecessary code that was borrowed fromtrieas well asgo-verkleInternalNodeandLeafNodeclasses to make them easier to work with (e.g. commitments are updated internally when you update a value/child in the node)childrenarray withinInternalNodeso that each element ofchildrenstores both the commitment for that child node as well as the path to that node (which is either a partial stem - if the child node is an Internal Node, or a full stem if the child node is a Leaf Node)findPathto use the new structure of the internal nodes described above so that walking the trie is merely a matter of retrieving the child node at each level of thepathkey provided as an input tofindPathdepthas a property on theVerkleNodeclass (since no obvious use case)