-
Notifications
You must be signed in to change notification settings - Fork 198
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.
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
feat(trie): implement new trie architecture with separated node types #2355
base: weiihann/improve-state-trie
Are you sure you want to change the base?
Conversation
0bd5aa9
to
182c9fe
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## weiihann/improve-state-trie #2355 +/- ##
===============================================================
- Coverage 73.61% 73.36% -0.25%
===============================================================
Files 136 150 +14
Lines 16658 18561 +1903
===============================================================
+ Hits 12262 13617 +1355
- Misses 3535 3994 +459
- Partials 861 950 +89 ☔ View full report in Codecov by Sentry. |
7ef23f2
to
c5ff49f
Compare
1219c57
to
0d6173d
Compare
b3692b6
to
18b6b2a
Compare
use hashFn
fe205cb
to
9b03c30
Compare
var ( | ||
_ Node = (*BinaryNode)(nil) | ||
_ Node = (*EdgeNode)(nil) | ||
_ Node = (*HashNode)(nil) | ||
_ Node = (*ValueNode)(nil) | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a doc-string for these nodes please? Also, I guess valueNode is a leafNode with a distance of 1 between it and it's parent, and HashNode is a leafNode with a distance greater than 1??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashNode
just represents an unresolved node, it could be leaf or non-leaf.
ValueNode
is a leaf node yes.
// nilValueNode is used when collapsing internal trie nodes for hashing, since unset children need to be hashed correctly | ||
var nilValueNode = &ValueNode{felt.Felt{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self to come back to this
func (n *HashNode) Hash(crypto.HashFn) *felt.Felt { return &n.Felt } | ||
func (n *ValueNode) Hash(crypto.HashFn) *felt.Felt { return &n.Felt } | ||
|
||
func (n *BinaryNode) cache() (*HashNode, bool) { return n.flags.hash, n.flags.dirty } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about renaming cache()
to flags()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think cache()
is more understandable, because we technically cache the hash of the node.
284fcad
to
8622d51
Compare
8622d51
to
be972d8
Compare
This PR introduces a complete new
Trie
implementation. Some of the major changes:Trie.Update
+Trie.Commit
is 4x fasterThis PR is a standalone package and has not been integrated with the other parts of Juno. The integration and replacement of legacy trie implementation will be given in another PR.
Also, do note that the implementation of
BitArray
is exactly the same as before, please ignore it during the review process.