Sapling commitment tree by str4d · Pull Request #68 · zcash/librustzcash

str4d · 2019-03-07T21:58:31Z

Implementation is similar to the one in zcashd for reviewability. Test vectors are taken from zcashd.

str4d · 2019-03-19T09:12:28Z

Addressed @ebfull's review comments.

str4d · 2019-04-05T19:56:57Z

Rebased on master to fix trivial merge conflict.

str4d · 2019-04-05T20:01:34Z

Force-pushed again to fix an indirect merge conflict.

str4d · 2019-04-16T17:13:38Z

The failure is in a doctest, due to (I believe) using syntax that was introduced in Rust 1.32, while this branch is still testing with Rust 1.31. If we merge #69 first, then the minimum Rust version will be 1.32, and this PR should pass.

Make PublicKey inner Point public so that we can use it during zk-SNARK verification

str4d · 2019-05-29T17:55:24Z

Force-pushed the last two commits to fix the doctest on older Rust versions (was missing an extern crate).

Eirik0

ACK. My comments are non-blocking.

Eirik0 · 2019-05-31T17:48:58Z

+        }
+    }
+
+    /// Returns the number of notes in the tree.


notes -> nodes (?)

I mean notes, since I'm referring to the leaf nodes (which are the appended cmu values from notes) rather than all nodes.

I suggest saying "leaf nodes", since it's possible we or someone else may want to reuse this code for something other than the note commitment tree (and nothing else about it depends on notes).

Eirik0 · 2019-05-31T17:51:24Z

+        CommitmentTree {
+            left: None,
+            right: None,
+            parents: vec![],


Would it be more correct to call this ancestors instead of parents?

Eh, I'm fine leaving this as-is for now, given that this is what we call it in the zcashd implementation. I'm happy to be outvoted however.

I was a little confused at first, but it's not too hard to figure out what's going on.

I would prefer ancestors; let's file a ticket to change this in both C++ and Rust. (Does not block.)

Eirik0

utACK

str4d · 2019-06-10T19:35:56Z

Rebased on master to fix some trivial merge conflicts in module declarations and imports.

Eirik0

reACK

This makes the merkle_tree module properly generic over the tree hash. It still hard-codes a depth 32 tree, because Rust doesn't yet support generic sizes, and we are unlikely to need to alter the tree depth in future circuit changes.

When sapling-crypto is refactored, the zcash_primitives::sapling constant would become the canonical one.

str4d · 2019-07-10T17:55:12Z

Rebased on master to fix trivial merge conflicts.

daira

ut(ACK+cov)

daira · 2019-06-04T19:25:19Z

+        }
+    }
+
+    /// Returns the number of notes in the tree.


I suggest saying "leaf nodes", since it's possible we or someone else may want to reuse this code for something other than the note commitment tree (and nothing else about it depends on notes).

daira · 2019-07-15T13:28:22Z

+        CommitmentTree {
+            left: None,
+            right: None,
+            parents: vec![],


I would prefer ancestors; let's file a ticket to change this in both C++ and Rust. (Does not block.)

daira · 2019-07-15T13:32:03Z

+        self.parents.iter().enumerate().fold(
+            match (self.left, self.right) {
+                (None, None) => 0,
+                (Some(_), None) | (None, Some(_)) => 1,


Is (None, Some(_)) ever possible?

Technically no; tree appending is lazy, so the state transition is:
(None, None) -> (Some(_), None) <--> (Some(_), Some(_))

I'll make it explicitly unreachable!().

daira · 2019-07-15T13:46:09Z

+            "00000002b02310f2e087e55bfd07ef5e242e3b87ee5d00c9ab52f61e6bd42542f93a6f55225747f3b5d5dab4e5a424f81f85c904ff43286e0f3fd07ef0b8c6a627b1145800",
+            "01b02310f2e087e55bfd07ef5e242e3b87ee5d00c9ab52f61e6bd42542f93a6f55000001225747f3b5d5dab4e5a424f81f85c904ff43286e0f3fd07ef0b8c6a627b1145800",
+            "00000002b02310f2e087e55bfd07ef5e242e3b87ee5d00c9ab52f61e6bd42542f93a6f55225747f3b5d5dab4e5a424f81f85c904ff43286e0f3fd07ef0b8c6a627b1145801017c3ea01a6e3a3d90cf59cd789e467044b5cd78eb2c84cc6816f960746d0e036c0000",
+            "01b02310f2e087e55bfd07ef5e242e3b87ee5d00c9ab52f61e6bd42542f93a6f55000001225747f3b5d5dab4e


I did not check the test vectors.

daira

ACK changes in 504c3ea.

fix: nullifier checks tx 2 and 3

str4d requested a review from ebfull March 7, 2019 21:58

str4d mentioned this pull request Mar 7, 2019

Block header parsing and transaction ID computation #66

Merged

ebfull requested changes Mar 15, 2019

View reviewed changes

ebfull self-requested a review March 26, 2019 23:14

ebfull approved these changes Mar 26, 2019

View reviewed changes

str4d force-pushed the sapling-commitment-tree branch from 67718b9 to 8ce260c Compare April 5, 2019 19:56

str4d force-pushed the sapling-commitment-tree branch from 8ce260c to ad460b3 Compare April 5, 2019 19:59

str4d mentioned this pull request Apr 16, 2019

Initial sync of blocks gets stuck zcash/zcash-android-wallet-sdk#23

Closed

bitcartel pushed a commit to bitcartel/librustzcash that referenced this pull request May 16, 2019

Merge pull request zcash#68 from ebfull/redjubjub-final-work

3b8246f

Make PublicKey inner Point public so that we can use it during zk-SNARK verification

str4d force-pushed the sapling-commitment-tree branch from 00f5345 to e6dc54d Compare May 29, 2019 17:54

mms710 requested review from Eirik0 and daira May 30, 2019 17:12

Eirik0 approved these changes May 31, 2019

View reviewed changes

ebfull approved these changes Jun 3, 2019

View reviewed changes

Eirik0 approved these changes Jun 3, 2019

View reviewed changes

str4d force-pushed the sapling-commitment-tree branch from f668e39 to 9b01585 Compare June 10, 2019 19:35

Eirik0 approved these changes Jun 10, 2019

View reviewed changes

str4d added 7 commits July 10, 2019 13:44

Move Sapling commitment tree hash into zcash_primitives

785f22c

Sapling commitment tree empty roots

8f3f95e

Sapling commitment tree

f4059a5

Move CommitmentTreeWitness into zcash_primitives

bf74915

Sapling incremental witnesses

667d610

Helper for serializing Option<T>

70a7069

Add serialization APIs to CommitmentTree and IncrementalWitness

9b41867

str4d added 6 commits July 10, 2019 13:53

Don't assert when parsing a CommitmentTreeWitness

98d7621

Use Iterator::all() in place of Iterator::fold()

263bbe1

Move merkle_tree::Node into sapling module

b9cea33

This makes the merkle_tree module properly generic over the tree hash. It still hard-codes a depth 32 tree, because Rust doesn't yet support generic sizes, and we are unlikely to need to alter the tree depth in future circuit changes.

Document merkle_tree module

e67560b

Unify Sapling tree depth constants

79006ec

When sapling-crypto is refactored, the zcash_primitives::sapling constant would become the canonical one.

Address Eirik's review comments

07dbfbe

str4d force-pushed the sapling-commitment-tree branch from 9b01585 to 07dbfbe Compare July 10, 2019 17:54

daira approved these changes Jul 15, 2019

View reviewed changes

Address Daira's review comments

504c3ea

str4d merged commit 5e3409e into zcash:master Jul 15, 2019

str4d deleted the sapling-commitment-tree branch July 15, 2019 14:52

daira reviewed Jul 15, 2019

View reviewed changes

str4d added this to the v0.1.0 milestone Aug 22, 2019

greg0x pushed a commit to valargroup/librustzcash that referenced this pull request Mar 12, 2026

Merge pull request zcash#68 from z-cale/roman/fix-nullifier-checks

c0ee50e

fix: nullifier checks tx 2 and 3

Conversation

str4d commented Mar 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

str4d commented Mar 19, 2019

Uh oh!

str4d commented Apr 5, 2019

Uh oh!

str4d commented Apr 5, 2019

Uh oh!

str4d commented Apr 16, 2019

Uh oh!

str4d commented May 29, 2019

Uh oh!

Eirik0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Eirik0 left a comment

Choose a reason for hiding this comment

Uh oh!

str4d commented Jun 10, 2019

Uh oh!

Eirik0 left a comment

Choose a reason for hiding this comment

Uh oh!

str4d commented Jul 10, 2019

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daira left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants