Sapling proving and verifying API #30

str4d · 2018-08-14T22:48:24Z

This PR defines a Rust API (with no unsafe) for creating and verifying the Sapling components of a Zcash transaction.

Eirik0 · 2018-08-22T20:04:46Z

src/rustzcash.rs

-    (&mut data_to_be_signed[32..64]).copy_from_slice(&(unsafe { &*sighash_value })[..]);
-
-    // Deserialize rk
-    let rk = match redjubjub::PublicKey::<Bls12>::read(&(unsafe { &*rk })[..], &JUBJUB) {


We have moved this deserialization in to the implementation of the context but not the deserialization of cv, anchor, spend_auth_sig, etc. I think I would prefer either moving all the desializations or none, unless this is necessary.

Edit: after looking through the rest of the commits, I think this would make sense not to move this read.

I organised it like this because the serialized form is used in the signature, and I was trying to minimise serialization operations. But you are right that it would be semantically simpler to just treat the Rust API as having Rust types, and serialize on either side as required. I'll adjust.

Eirik0 · 2018-08-22T21:40:27Z

src/sapling.rs

+            return false;
+        }
+
+        if is_small_order(&epk) {


We used to accumulate the value commitment before we checked if epk was of small order. I think this makes more sense, but just wanted to confirm that we want to change this.

I figured it made sense to do the necessary checks before any data processing.

Eirik0 · 2018-08-22T21:43:12Z

src/sapling.rs

+        params: &JubjubBls12,
+    ) -> bool {
+        // Obtain current bvk from the context
+        let mut bvk = PublicKey(self.bvk.clone());


Below we refer to bvk.0 more than we refer to bvk. It doesn't matter because of the "zero-cost abstractions", but I like saying let mut bvk = self.bvk.clone(); here and PublicKey(bvk).verify( below

Eirik0 · 2018-08-22T21:44:24Z

src/sapling.rs

+        (&mut data_to_be_signed[32..64]).copy_from_slice(&sighash_value[..]);
+
+        // Verify the binding_sig
+        bvk.verify(


I like that we changed this to return the result of verify rather than having the conditional.

Eirik0 · 2018-08-23T17:07:09Z

src/sapling.rs

+            tmp.add_assign(&self.bsk);
+
+            // Update the context
+            self.bsk = tmp;


There are a few places where we are doing this or similar accumulate logic. Now that this is not unsafe it could be more easily done in one line. self.bsk.add_assign(&rcv) or something similar. The same goes for when we are de-accumulating. I'm not sure the best way to abstract this logic across the two contexts perhaps a struct ContextualPoint with the accumulate and de-accumulate methods? This can be done later.

Optimise Boolean::enforce_equal.

str4d · 2018-09-11T09:46:48Z

Rebased on master to fix merge conflicts, and addressed @Eirik0's serialisation comment.

str4d · 2018-09-11T10:20:42Z

Edited the proving and binding sig refactor to target the zcash_proofs crate, so that it is accessible by other crates (like zcash_wallet).

str4d · 2018-10-16T17:15:41Z

Rebased on master to fix merge conflicts.

str4d · 2018-10-16T17:24:38Z

Rebased to fix a build error I missed during the previous rebase.

Eirik0

utACK. There were a couple things I had questions on, and a couple things which could potentially be simplified, but those are beyond the scope of this refactoring.

Eirik0 · 2018-10-16T19:55:04Z

librustzcash/src/sapling.rs

+            return false;
+        }
+
+        // Accumulate the value commitment in the context


The block below this comment could be simplified, but that is beyond the scope of this refactor. See: #30 (comment)

Eirik0 · 2018-10-16T19:57:16Z

librustzcash/src/sapling.rs

+            let nullifier = multipack::bytes_to_bits_le(nullifier);
+            let nullifier = multipack::compute_multipacking::<Bls12>(&nullifier);
+
+            assert_eq!(nullifier.len(), 2);


It feels odd to have asserts in production code, but this has not changed in the move. I wonder if we should be explicitly panicking here with a better error message.

Edit: Actually, why are we not returning None/Err(())/false in the places that we are asserting?

This is an internal consistency check. We know (via the function's type signature) that nullifier is 32 bytes, and we "know" that when packed this should result in two field elements. The assertion here upgrades the "know" to a know, by catching any bugs that might result in a different-length packing.

Eirik0 · 2018-10-16T20:13:45Z

librustzcash/src/sapling.rs

+        spend_auth_sig: Signature,
+        zkproof: Proof<Bls12>,
+        verifying_key: &PreparedVerifyingKey<Bls12>,
+        params: &JubjubBls12,


Will params every be anything other than &JUBJUB? Why pass this in rather than use the constant?

That's something we should address across the entire codebase: which parts will only ever need JUBJUB, and which should be agnostic.

Eirik0 · 2018-10-16T20:26:03Z

librustzcash/src/sapling.rs

+};
+
+// This function computes `value` in the exponent of the value commitment base
+fn compute_value_balance(


It's a little hard to follow what happens with this function, but the total number of additions/removals across all commits adds up. The change are that the parameter value: i64_t becomes value: i64 (which makes sense), and we now pass in params rather than refer to &JUBJUB directly.

Eirik0 · 2018-10-16T21:05:41Z

zcash_proofs/src/sapling.rs

+            edwards::Point<Bls12, Unknown>,
+            PublicKey<Bls12>,
+        ),
+        (),


One general comment is that it feels odd to return a Result with an empty error (we do this in a few places in this file). I would think that either we would return an Option<...> if we don't care what the error is. Alternatively we could return a Result <...> with an error type that encapsulates the error types that could result from the functions that we are calling - this would allow us to take advantage of the ? operator if nothing else.

One benefit to returning a Result instead of an Option is that the standard rustc linters will generate a warning if a Result is unused (either via matching or an explicit unwrap()). As for improved error-handling (which is definitely a good idea), this should be tackled as part of a wider refactor.

Eirik0 · 2018-10-16T21:17:26Z

zcash_proofs/src/sapling.rs

+        // Verify the proof
+        match verify_proof(verifying_key, &proof, &public_input[..]) {
+            // No error, and proof verification successful
+            Ok(true) => {}


Ok(true) feels odd because it makes one wonder what Ok(false) means. It doesn't look like we ever use the Err from verify_proof. IMO it would make more sense if verify_proof just returned a bool, or if the error type it returned encapsulated the situation where this could be false (Result<(), ...>).

Yeah, I would also prefer to use Result<(), SomeErrorType>; this is something we should think about when doing an error-handling refactor (and which will require changes to bellman in this case).

str4d · 2018-11-08T02:08:21Z

Rebased on master to fix merge conflict.

str4d · 2018-11-08T23:50:13Z

I'm going to adjust this PR to move the verifier into zcash_proofs alongside the prover, in line with @ebfull's plan outlined in #41.

str4d · 2018-11-09T10:42:50Z

Force-pushed to rework the PR. It now only contains the proving and verifying API refactor; the spendAuthSig refactor will be in a subsequent PR (once #39 is merged).

daira · 2018-11-30T23:25:09Z

This is difficult to review (specifically, to check that the code movement is just movement). Can it be split into commits that move smaller pieces at a time?

str4d · 2018-11-30T23:36:25Z

@daira I'm not sure it can, at least while ensuring that the code compiles. I can go back and try it out, but no guarantee that I can simplify it in that way. I could also try doing some refactoring in-place before moving code, and see if that improves the diffs...

Eirik0 · 2018-11-30T23:40:38Z

If it isn't too much trouble I like the idea of refactoring in place and then moving. I also had wondered about making the new structs and then pulling in the functions one commit at a time. Not necessarily simpler but less to look at all at once.

str4d · 2018-12-01T00:13:36Z

@daira @Eirik0 I've added some refactoring commits which I think will help. LMK if I should try and break things down further.

str4d · 2019-01-03T21:59:16Z

Force-pushed to update the cargo fmt commit in order to trigger a Travis CI build.

ebfull · 2019-01-03T22:08:59Z

ACK

Eirik0

ACK. The refactoring looks good. Some of my previous comments still stand, but those can be addressed later.

38d38af3 Merge pull request zcash#32 from kevaundray/patch-1 af5598da Merge pull request zcash#33 from ZcashFoundation/scalar 109ec40d Add public Scalar type alias for Fr 8e9c5fe6 typo in Fr.rs 8e9337ee Merge pull request zcash#30 from rex4539/typos 5f4374c8 Fix typo git-subtree-dir: jubjub git-subtree-split: 38d38af3b792d2c55d815d214a7cd157dc8f71ad

Eirik0 reviewed Aug 23, 2018

View reviewed changes

str4d pushed a commit to str4d/librustzcash that referenced this pull request Aug 28, 2018

Merge pull request zcash#30 from plutomonkey/boolean_enforce_equal

3c07a2d

Optimise Boolean::enforce_equal.

str4d force-pushed the sapling-api-cleanup branch from 83aa9b6 to 527d698 Compare September 11, 2018 09:45

str4d force-pushed the sapling-api-cleanup branch from 527d698 to a6b3d14 Compare September 11, 2018 10:19

str4d force-pushed the sapling-api-cleanup branch from a6b3d14 to a492956 Compare October 16, 2018 17:15

str4d force-pushed the sapling-api-cleanup branch from a492956 to 0944cee Compare October 16, 2018 17:24

Eirik0 approved these changes Oct 16, 2018

View reviewed changes

str4d force-pushed the sapling-api-cleanup branch from 0944cee to 9139ec4 Compare November 8, 2018 02:07

str4d force-pushed the sapling-api-cleanup branch from 9139ec4 to 2de6301 Compare November 9, 2018 10:40

str4d changed the title ~~Sapling API cleanup~~ Sapling proving and verifying API Nov 9, 2018

str4d requested review from daira, ebfull and Eirik0 November 9, 2018 10:44

str4d mentioned this pull request Nov 30, 2018

Address from seed Electric-Coin-Company/zcash-android-wallet-sdk#1

Merged

str4d added 4 commits November 30, 2018 23:50

Refactor Sapling verification checks

eb2db2e

Move Sapling verification checks into zcash_proofs crate

e184180

Refactor Sapling proving and binding signature

922ffe6

Move Sapling proving and binding signature into zcash_proofs crate

e378229

str4d force-pushed the sapling-api-cleanup branch from 2de6301 to b83234a Compare December 1, 2018 00:11

cargo fmt 1.31.1

1a1c775

str4d force-pushed the sapling-api-cleanup branch from b83234a to 1a1c775 Compare January 3, 2019 21:52

str4d mentioned this pull request Jan 3, 2019

Migrate to ff and group crates #46

Merged

Eirik0 approved these changes Jan 5, 2019

View reviewed changes

ebfull merged commit f65c37e into zcash:master Jan 6, 2019

str4d deleted the sapling-api-cleanup branch January 6, 2019 10:08

Eirik0 mentioned this pull request Jan 7, 2019

[WIP] Unsafe consolidation #31

Closed

19 tasks

This was referenced Jan 15, 2019

Missing tests? #49

Closed

Add tests for the Sapling proving and verifying APIs #56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sapling proving and verifying API #30

Sapling proving and verifying API #30

str4d commented Aug 14, 2018

Eirik0 Aug 22, 2018 •

edited

str4d Aug 23, 2018

Eirik0 Aug 22, 2018

str4d Sep 11, 2018

Eirik0 Aug 22, 2018

Eirik0 Aug 22, 2018

Eirik0 Aug 23, 2018 •

edited

str4d commented Sep 11, 2018

str4d commented Sep 11, 2018

str4d commented Oct 16, 2018

str4d commented Oct 16, 2018

Eirik0 left a comment

Eirik0 Oct 16, 2018

Eirik0 Oct 16, 2018

str4d Oct 23, 2018

Eirik0 Oct 16, 2018

str4d Oct 23, 2018

Eirik0 Oct 16, 2018

Eirik0 Oct 16, 2018

str4d Oct 27, 2018

Eirik0 Oct 16, 2018

str4d Oct 27, 2018

str4d commented Nov 8, 2018

str4d commented Nov 8, 2018

str4d commented Nov 9, 2018

daira commented Nov 30, 2018

str4d commented Nov 30, 2018

Eirik0 commented Nov 30, 2018

str4d commented Dec 1, 2018

str4d commented Jan 3, 2019

ebfull commented Jan 3, 2019

Eirik0 left a comment

Sapling proving and verifying API #30

Sapling proving and verifying API #30

Conversation

str4d commented Aug 14, 2018

Eirik0 Aug 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eirik0 Aug 23, 2018 • edited

Choose a reason for hiding this comment

str4d commented Sep 11, 2018

str4d commented Sep 11, 2018

str4d commented Oct 16, 2018

str4d commented Oct 16, 2018

Eirik0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Nov 8, 2018

str4d commented Nov 8, 2018

str4d commented Nov 9, 2018

daira commented Nov 30, 2018

str4d commented Nov 30, 2018

Eirik0 commented Nov 30, 2018

str4d commented Dec 1, 2018

str4d commented Jan 3, 2019

ebfull commented Jan 3, 2019

Eirik0 left a comment

Choose a reason for hiding this comment

Eirik0 Aug 22, 2018 •

edited

Eirik0 Aug 23, 2018 •

edited