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.

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

Open
wants to merge 54 commits into
base: weiihann/improve-state-trie
Choose a base branch
from

Conversation

weiihann
Copy link
Contributor

@weiihann weiihann commented Dec 31, 2024

This PR introduces a complete new Trie implementation. Some of the major changes:

  • Separation of node types - Makes code more intuitive, reduce redundant storage, separation of concerns, easier to extend functionalities
  • Performance Optimization - Stores trie in-memory, cache node hashes, Trie.Update + Trie.Commit is 4x faster
  • Range Proof - Solves the edge case of gapped range proof from the previous implementation

This 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.

@weiihann weiihann changed the base branch from main to weiihann/improve-state-trie January 23, 2025 03:51
@weiihann weiihann marked this pull request as ready for review January 23, 2025 04:42
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 71.99159% with 533 lines in your changes missing coverage. Please review.

Project coverage is 73.36%. Comparing base (f9b4aed) to head (3dca1e3).

Files with missing lines Patch % Lines
core/trie2/proof.go 69.16% 99 Missing and 37 partials ⚠️
core/trie2/trieutils/bitarray.go 74.48% 131 Missing and 4 partials ⚠️
core/trie2/trie.go 78.04% 69 Missing and 14 partials ⚠️
core/trie2/triedb/pathdb/database.go 50.51% 41 Missing and 7 partials ⚠️
core/trie2/node_enc.go 58.55% 31 Missing and 15 partials ⚠️
core/trie2/node.go 44.23% 29 Missing ⚠️
core/trie2/trienode/nodeset.go 72.22% 10 Missing and 5 partials ⚠️
core/trie2/id.go 20.00% 12 Missing ⚠️
core/trie2/hasher.go 86.66% 8 Missing and 2 partials ⚠️
utils/orderedset.go 0.00% 6 Missing ⚠️
... and 4 more
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.
📢 Have feedback on the report? Share it here.

@weiihann weiihann force-pushed the weiihann/node-type branch 2 times, most recently from 1219c57 to 0d6173d Compare February 10, 2025 00:03
@weiihann weiihann added the State All things related to state label Feb 11, 2025
@rodrigo-pino rodrigo-pino added Refactor 3 reviews Requires at least 3 reviews labels Feb 25, 2025
@rianhughes rianhughes self-requested a review February 25, 2025 09:41
@AnkushinDaniil AnkushinDaniil self-requested a review February 25, 2025 09:47
Comment on lines +12 to +18
var (
_ Node = (*BinaryNode)(nil)
_ Node = (*EdgeNode)(nil)
_ Node = (*HashNode)(nil)
_ Node = (*ValueNode)(nil)
)

Copy link
Contributor

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

Copy link
Contributor Author

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.

Comment on lines +40 to +41
// nilValueNode is used when collapsing internal trie nodes for hashing, since unset children need to be hashed correctly
var nilValueNode = &ValueNode{felt.Felt{}}
Copy link
Contributor

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 }
Copy link
Contributor

@rianhughes rianhughes Feb 25, 2025

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

Copy link
Contributor Author

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.

docs

docs
@weiihann weiihann changed the title Refactor Trie feat(trie): implement new trie architecture with separated node types Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 reviews Requires at least 3 reviews Refactor State All things related to state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants