verkle: verkle package #2923
verkle: verkle package #2923
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…/ethereumjs-monorepo into verkle/verkle-package
There was a problem hiding this comment.
Ok, I have given this a look (but not in-depth), some minor comments and questions.
In general regarding the structure: my first thought would be that this is great, it is essentially MPT but now with Verkle support (so I assume it would be rather "easy" (?) to swap MPT for Verkle - of course only at the start of a client not having to do transition MPT -> Verkle).
I will need to study Verkle a bit to also review the internals, since I cannot comment on that right now.
packages/verkle/src/db/checkpoint.ts
Outdated
| // Nothing has been found in diff cache, look up from disk | ||
| const valueHex = await this.db.get(keyHex, { | ||
| keyEncoding: KeyEncoding.String, | ||
| valueEncoding: ValueEncoding.String, |
There was a problem hiding this comment.
I think with Verkle we use a completely new database, right? (So not related to old MPT DB). If that's the case, can we immediately use valueEncoding set to ValueEncoding.Bytes? Then we don't have to convert bytesToUnprefixedHex early on and we don't have to deal with a breaking release for the underlying database later on.
There was a problem hiding this comment.
(same maybe also for keys directly)
There was a problem hiding this comment.
Good point, will go with bytes.
There was a problem hiding this comment.
I've updated all key/value types to Uint8Arrays (db, cache), but kept the Checkpoint keyValueMap as Map<string, Uint8Array | undefined> since Uint8Arrays can't directly be used as Map keys (since two uint8arrays are not equal even if they contain the same data)
There was a problem hiding this comment.
I'm now reading that using Uint8Array as key for the cache (the cache uses the LRUCache package) might cause issues. I will do some testing but I think I'll revert that part, so we'd end up with:
- A DB that uses Uint8Arrays for keys and values
- A cache (LRU-cache) that uses
stringfor keys and Uint8Array as values - A keyValueMap (
Checkpointtype) that usesstringas keys and Uint8Array as values.
There was a problem hiding this comment.
Indeed, this fails:
So I'll switch the cache to use hexstrings for keys.
|
Very very very cool!!! ❤️ 🤩 This looks like an extremely solid fundament already where we can further build upon! Would vote on taking this in pretty soon (this or next week)! |
| bytes[7] = Number((hi >> BigInt(24)) & BigInt(0xff)) | ||
|
|
||
| return bytes | ||
| } |
There was a problem hiding this comment.
These new helper methods look more "advanced" (lol) than our classic conversion helpers for BE! 🙂 🤔 Also love that they explicitly target already 32/64 bit e.g..
Can you say something how they compare and relate to our existing methods, e.g. performance wise? This explicit bit-length also has the nice (and eventually performance-increasing) side effect that it omits the 0-padding if I see correctly?
If these methods are better than our old ones: might it be a mid-term option to also adopt this for our BE methods?
(would like to at least high-level settle on these kind of questions before we take new methods with new naming schemes and everything into Util)
There was a problem hiding this comment.
I've researched this a bit, and I think we could strike a balance between high performance and readability (the current implementation is quite opaque) by using DataView to go from Uint8Arrays to ints/bigints. I'm reading that DataViews are very performant & optimized, and the methods have built-in support for BE/LE. See this reference for more info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView/getBigInt64
There was a problem hiding this comment.
When you mention "adopting this for our BE methods", what methods are you referring to? bytesToBigInt and bytesToInt? I can implement alternative versions using DataViews and compare performance.
There was a problem hiding this comment.
This is looking great! I'm not sure if something like this already exists for this package, but since it is in development, something worth considering that might attract help/contributors is if we could keep an updated list of issues with an overall master tracker on work that still needs to get done.
Co-authored-by: Scorbajio <[email protected]>
Co-authored-by: Scorbajio <[email protected]>
Co-authored-by: Scorbajio <[email protected]>
Co-authored-by: Scorbajio <[email protected]>
|
https://github.com/ethereumjs/ethereumjs-monorepo/actions/runs/6645612344/job/18057315347 |
This PR adds a new package: @ethrereumjs/verkle. Similarly to the trie package, this package will implement verkle tries as specified https://notes.ethereum.org/@vbuterin/verkle_tree_eip, enabling us to create verkle tries, do operations on them, and use them to verify and generate proofs.
Notes
I've identified cryptographic primitives that we are missing for a complete implementation (based on Go's implementation here: https://github.com/gballet/go-ethereum/blob/kaustinen-with-shapella/trie/trie.go).
Pointclass. Comes frombanderwagon.Element. Quite fundamental as it represents a polynominal commitment, with a number of methods used repeatedly in the implementation.toFrMultiple. Implementation comes from Banderwagon'sMultiMapToScalarFieldmethod.Frclass. Field representation class. Comes from Banderwagon'sfr.Element.ipa.IPAConfigingo-ipa/ipa. (link). Used for example when creating new leaf nodes.