-
Notifications
You must be signed in to change notification settings - Fork 108
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
change(state): Wrap commitment trees into Arc
#4757
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4757 +/- ##
==========================================
- Coverage 78.80% 78.79% -0.01%
==========================================
Files 306 306
Lines 37661 37656 -5
==========================================
- Hits 29678 29671 -7
- Misses 7983 7985 +2 |
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.
Looks good!
f9b33b5
to
4a3df89
Compare
The comment is not valid because Zebra uses `bridgetree::Frontier`s from the `incrementalmerkletree` crate to represent its note commitment trees. This `struct` does not support popping elements from the tree.
4a3df89
to
a0fc1cb
Compare
031e832
to
32548ed
Compare
It looks like these changes might help with some state hangs, but we need to test them after PR #4776 merges. |
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.
Looks great, thanks for this!
I admin-merged this PR because:
|
I thought I marked this PR as ready for review more than a week ago, but apparently, I didn't. Sorry for that. |
Motivation
Zebra maintains a copy of the same note commitment tree with each non-finalized state when handling multiple non-finalized states. It would be better to maintain only
Arc
s, and use copy-on-write.Solution
This PR wraps Sprout, Sapling and Orchard note commitment trees into
Arc
s in both finalized and non-finalized states.Closes #4766.
Review
We should check if the changes affect syncing speed.