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

Sprout note commitment trees #3051

Merged
merged 47 commits into from
Nov 18, 2021
Merged

Sprout note commitment trees #3051

merged 47 commits into from
Nov 18, 2021

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Nov 11, 2021

Motivation

Zebra needs Spout note commitment trees.

Designs

https://github.com/ZcashFoundation/zebra/blob/a1cec27871f22dd1d8a8440e4fd4e7b8ff111374/book/src/dev/rfcs/drafts/0005-treestate.md

https://zips.z.cash/protocol/protocol.pdf#merkletree

Solution

This PR follows the implementation of the note commitment trees for Sapling.

Closes #2484.

Review

@dconnolly can review.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

This PR is a prerequisite for #2485.

@upbqdn upbqdn added NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) P-Medium labels Nov 11, 2021
@upbqdn upbqdn added this to the 2021 Sprint 22 milestone Nov 11, 2021
@upbqdn
Copy link
Member Author

upbqdn commented Nov 11, 2021

I want to add more tests from Sapling once I solve the currently failing test.

@mpguerra mpguerra removed this from the 2021 Sprint 22 milestone Nov 12, 2021
@mpguerra mpguerra requested a review from dconnolly November 12, 2021 08:25
zebra-chain/src/sprout/tests/tree.rs Outdated Show resolved Hide resolved
zebra-chain/src/sprout/tree.rs Show resolved Hide resolved
zebra-chain/src/sprout/tree.rs Show resolved Hide resolved
zebra-chain/src/sprout/tests/tree.rs Outdated Show resolved Hide resolved
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
conradoplg
conradoplg previously approved these changes Nov 18, 2021
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great work searching for those transactions with JoinSplits. (I found it interesting that it took that long for them to appear in the blockchain).

I just added an optional suggestion that we can do in a separate ticket if you prefer.

zebra-chain/src/sprout/tree.rs Outdated Show resolved Hide resolved
@upbqdn
Copy link
Member Author

upbqdn commented Nov 18, 2021

I found it interesting that it took that long for them to appear in the blockchain.

That was surprising to me as well. What I did there was a manual binary search for the first anchor different from the genesis one, which is the hash of an empty note commitment tree anyway. I felt the mighty power of logarithms when I was doing the search. :D

@upbqdn upbqdn enabled auto-merge (squash) November 18, 2021 21:17
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor changes since the last approval

@upbqdn upbqdn merged commit 8963007 into main Nov 18, 2021
@upbqdn upbqdn deleted the sprout-note-commitment-trees branch November 18, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement incremental, sparse note commitment trees for Sprout
5 participants