Use batch optimizations, load params in groth16::Verifier, verify Spend & Output descriptions #1713

dconnolly · 2021-02-08T22:29:24Z

Motivation

Tie together the existing async groth16::Verifier to the bellman batch optimizations and the required loading in of the different VerifyingKeys and Params for each statement type.

Solution

Supports the bellman batch math optimizations, and creates 3 static instances,
one for Spend and Output per their different VerifyingKeys.

Depends on #1704, and zkcrypto/bellman#59

The code in this pull request has:

Documentation Comments
Unit Tests and Property Tests

Review

@yaahc @teor2345 @oxarbitrage

Related Issues

#1645, #406

Follow Up Work

#1835

dconnolly · 2021-02-11T16:51:54Z

Rebased onto #1704 while in progress

dconnolly · 2021-02-22T21:17:22Z

@yaahc I'm running into conflicts between bellman's rand/rand_core traits, and redjubjub's rand/rand_core traits, which I thought I had resolved to the same, but rustc still mad at me, maybe you can take a look?

Supports the bellman batch math optimizations, and creates 3 static instances, one for Spend, Output, and JoinSplit, per their different VerifyingKeys

teor2345 · 2021-03-02T07:39:55Z

Commit a143434 (merge 0a0d11b) synced within 3 hours on testnet, and 5 hours on mainnet. That's pretty typical for me, I'm network-limited, and my local peers are a much larger proportion of testnet. (Also testnet data is much smaller.)

Didn't notice much CPU load, but I do have a lot of cores.

I've restarted with the latest version of this PR 525a6f6 (merge 611b335), and I'll let it run overnight. Feel free to ping me if I forget to update this ticket.

yaahc · 2021-03-02T20:10:56Z

zebra-chain/Cargo.toml

+#redjubjub = "0.2"
+redjubjub = {git = "https://github.com/ZcashFoundation/redjubjub", rev = "8101eaff1cb2fca45334f77a65caa4c46e3d545b"}


we should probably stick with using the [patch.crates-io] section in the top level toml for specifying git deps

Let's also open a follow-up ticket that describes when we can un-patch the crates.

The existing ticket is #1086, but we should probably start a new one for crypto crates.

yaahc · 2021-03-02T20:17:03Z

zebra-chain/src/sapling/output.rs

+    pub fn primary_inputs(&self) -> Vec<jubjub::Fq> {
+        let mut inputs = vec![];
+
+        let cv_affine = jubjub::AffinePoint::from_bytes(self.cv.into()).unwrap();


can we replace these unwraps with expects that describe why this should never fail

Bah unfortunately this is CtOption but I'll make it better anyway
https://docs.rs/subtle/2.2.2/subtle/struct.CtOption.html

wow I can't believe CtOption doesn't have an expect call...

You could use an extension trait here to add the expect method and have it just internally call unwrap_or_else(|| panic!(...))

yaahc · 2021-03-02T20:18:17Z

zebra-consensus/Cargo.toml

+#bellman = "0.8"
+bellman = { git = "https://github.com/zkcrypto/bellman", rev = "bd4af09f50a4d020a3672aff37c4f3f2da2bb36b" }


should also be specified via patch section

yaahc · 2021-03-02T20:20:11Z

zebra-consensus/src/primitives/groth16.rs

+/// Note that making a `Service` call requires mutable access to the service, so
+/// you should call `.clone()` on the global handle to create a local, mutable
+/// handle.
+pub static SPEND_VERIFIER: Lazy<
+    Fallback<Batch<Verifier, Item>, ServiceFn<fn(Item) -> Ready<Result<(), VerificationError>>>>,
+> = Lazy::new(|| {


we should not make this handle public if we want to enforce cloning, and instead expose a pub method that calls clone and returns that handle.

We should make a similar change to our other batch verifiers, but that can be done in a follow-up ticket.

Let's make sure we open that follow-up ticket :-)

Yeah we now have 4, going to be 5 batch verifier services that are currently clone()'d inside transaction::Verifier::call(), script_verifier is assigned to the Verifier instance on construction and later clone'd, the others are referenced and clone'd only within call. I think all are currently static besides script::Verifier. Are we doing this right?

I think consistency is important, but we were always going to have a lot of batch verifiers, so having 5 doesn't worry me.

To be specific: let's access, store, and clone them all the same way. Clones should be cheap.

yaahc · 2021-03-02T20:26:15Z

zebra-consensus/src/primitives/groth16/tests.rs

+    batch_flushes_on_max_items().await
+}
+
+#[spandoc::spandoc]


since you have a #[spandoc] attribute here, it might be a cool idea to add a /// SPANDOC: comment before the main line in the test where it can potentially fail, so we get a nice description of what it was doing in the error message. Otherwise we should probably remove the attribute since it's not doing anything.

Oh man, this got committed when it was not updated away from redjubjub yet, but I should include the better SPANDOC: comment either way

yaahc · 2021-03-02T20:30:28Z

zebra-consensus/src/transaction.rs

+                            let spend_rsp = spend_verifier
+                                .ready_and()
+                                .await?
+                                .call(primitives::groth16::ItemWrapper::from(spend).into());


minor: we can probably change the spend_verifier so its request type is Into<Item> so that this into call happens automatically inside the service

teor2345 · 2021-03-03T00:18:19Z

I've restarted with the latest version of this PR 525a6f6 (merge 611b335), and I'll let it run overnight. Feel free to ping me if I forget to update this ticket.

Seems fine 👍

teor2345 · 2021-03-03T06:08:04Z

zebra-chain/Cargo.toml

-rand_core = "0.5.1"
+rand_core = "0.6"


Does this add a dependency on multiple versions of rand_core?

Maybe we should check which forks of ed25519-zebra use rand_core = "0.6" now. But this isn't a high priority.

teor2345 · 2021-03-03T06:10:12Z

zebra-chain/src/sapling/output.rs

@@ -27,6 +27,30 @@ pub struct Output {
    pub zkproof: Groth16Proof,
 }

+impl Output {
+    /// Encodes the primary inputs for the proof statement as 5 Bls12_381 base


Should we have an assertion that checks the returned Vec's length?

teor2345 · 2021-03-03T06:10:16Z

zebra-chain/src/sapling/spend.rs

@@ -31,6 +31,35 @@ pub struct Spend {
    pub spend_auth_sig: redjubjub::Signature<SpendAuth>,
 }

+impl Spend {
+    /// Encodes the primary inputs for the proof statement as 7 Bls12_381 base


Should we have an assertion that checks the returned Vec's length?

teor2345 · 2021-03-03T06:12:39Z

zebra-consensus/Cargo.toml

@@ -30,11 +33,10 @@ tower-batch = { path = "../tower-batch/" }
 zebra-chain = { path = "../zebra-chain" }
 zebra-state = { path = "../zebra-state" }
 zebra-script = { path = "../zebra-script" }
-pairing = "0.18.0"
+wagyu-zcash-parameters = "0.2.0"


Do we need to pull in all 7 wagyu sub-crates? They're pretty large.

Does the redundant data get optimised away in release builds?

zebra-consensus/src/primitives/groth16.rs

teor2345 · 2021-03-03T06:53:54Z

zebra-consensus/src/primitives/groth16.rs

+/// Note that making a `Service` call requires mutable access to the service, so
+/// you should call `.clone()` on the global handle to create a local, mutable
+/// handle.
+pub static OUTPUT_VERIFIER: Lazy<


Similar to the above comment, let's have a public method that returns a clone.

teor2345 · 2021-03-03T06:55:46Z

zebra-consensus/src/primitives/groth16/hash_reader.rs

+
+use blake2b_simd::State;
+
+/// Abstraction over a reader which hashes the data being read.


Don't we already have an impl of HashReader somewhere?

If not, this seems like generic utility code, rather than groth16-specific code, so it belongs somewhere else in the module tree.

I think an equivalent is https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/serialization/sha256d.rs, but that's for SHA256d, this one is for Blake2b; we can more it to a top level module still in zebra-consensus, or zebra-chain.

zebra-consensus/src/primitives/groth16/params.rs

zebra-consensus/src/transaction.rs

zebrad/tests/acceptance.rs

teor2345

Looks good, I just have a few questions and task dependencies

Including over mainnet test vectors and a forced appropriate failure.

dconnolly added this to the 2021 Sprint 3 milestone Feb 8, 2021

dconnolly self-assigned this Feb 8, 2021

dconnolly force-pushed the use-groth16-batch-math branch 2 times, most recently from aaabbac to e9202da Compare February 11, 2021 16:51

dconnolly force-pushed the use-groth16-batch-math branch 2 times, most recently from c133db7 to a2315fb Compare February 22, 2021 01:54

dconnolly marked this pull request as ready for review February 23, 2021 02:32

yaahc mentioned this pull request Feb 23, 2021

Add support for loading verifying parameters #1704

Closed

2 tasks

mpguerra requested review from teor2345, yaahc and oxarbitrage February 24, 2021 13:41

dconnolly marked this pull request as draft February 24, 2021 18:58

dconnolly force-pushed the use-groth16-batch-math branch from eabe599 to a3ab377 Compare February 24, 2021 20:22

mpguerra linked an issue Feb 25, 2021 that may be closed by this pull request

Integrate Spend and Output async groth verifiers with transaction verifier #1645

Closed

10 tasks

mpguerra modified the milestones: 2021 Sprint 3, 2021 Sprint 4 Feb 26, 2021

yaahc and others added 4 commits February 27, 2021 22:57

Add support for loading verifying parameters

e7630b0

An incomplete update to the Groth16Verifier

7abbcd0

Supports the bellman batch math optimizations, and creates 3 static instances, one for Spend, Output, and JoinSplit, per their different VerifyingKeys

Point at zkcrypto/bellman for merged-in groth16 batch verification

cc302bf

Typo

cbbb725

Consensus rule: check that Output.(cv, epk) are of small order

525a6f6

yaahc suggested changes Mar 2, 2021

View reviewed changes