Bring in QED-it Tests #93

defuse · 2019-07-25T13:14:51Z

This PR was originally submitted here: zcash/sapling-crypto#101

This pulls in a bunch of tests QED-it wrote during their audit that were spread across many branches in their sapling-crypto-internal repository. I've left their history intact so the best way to review it is to just look at the final diff, except for...

This commit, which includes changes outside of tests, and
This commit, which is my attempt to fix the broken linear relation testing in that commit (the check in that commit effectively just checks that p1 != p2 again, not what it was intended to do).
I deleted a find_group_hash test since it didn't look like it was doing anything useful, and I don't want merging to block on me figuring out how to implement that test well. Raise a separate issue and assign me if you would like some find_group_hash unit tests!

I did not bring in this change to the circuit code, because I don't understand it. I also did not bring in a bunch of changes to comments and variable renamings in the aurel_comments branch and the sum_bug branch, but I did bring in the new tests from those branches.

defuse · 2019-07-25T16:23:06Z

This needs some cleanup and improvements before it can be merged. In particular the group_hash tests are incomplete. Edit: Done!

defuse

ACK. I didn't spend too long looking for errors in the tests, just made sure they are all doing something sane.

defuse · 2019-07-25T20:29:59Z

sapling-crypto/src/circuit/ecc.rs

+
+        // take all the points from the script
+        // assert should be different than multiplying by cofactor, which is the solution
+        // is user input verified? https://github.com/zcash/librustzcash/blob/f5d2afb4eabac29b1b1cc860d66e45a5b48b4f88/src/rustzcash.rs#L299


I think this is talking about how we're missing an explicit check that ask != 0 in our code? Should I just delete these comments?

defuse · 2019-08-01T15:40:39Z

Uh oh I accidentally pushed the branch to this repo, deleted it, then rebased and now Travis is broken: https://travis-ci.org/zcash/librustzcash/builds/566534948?utm_source=github_status&utm_medium=notification

$ git clone --depth=50 --branch=qed-it-lrz https://github.com/zcash/librustzcash.git zcash/librustzcash
Cloning into 'zcash/librustzcash'...
warning: Could not find remote branch qed-it-lrz to clone.
fatal: Remote branch qed-it-lrz not found in upstream origin

Why does it think there should be a qed-it-lrz in zcash/librustzcash?

str4d · 2019-08-19T00:21:06Z

Now that #104 has been merged, this PR needs to be refactored to account for sapling-crypto being split up. I can do this @defuse if you don't have time.

str4d · 2019-08-22T22:59:00Z

I've started rebasing this, and I've decided that I'm not going to preserve all history because there is a lot of cruft within the commits, and in particular there are some commits that are clearly mislabeled (the code they refer to appears in an adjacent commit, or spread across several commits). I will refactor retaining the intention of the changes, and as much authorship as I can.

str4d · 2019-08-23T10:47:24Z

Rebased on master, cleaned up some of the commits (so they match what they say on the tin), and formatted most of the commits. The PR will probably need to be rebased again once #113 lands.

defuse

utACK ~~modulo my question about using just the first pedersen hash test vector~~.

zcash_primitives/src/pedersen_hash.rs

defuse · 2019-08-29T21:59:40Z

Added a commit to make use of all the test vectors. That required increasing the number of Pedersen hash generators to 6. Someone should check that I did so correctly. Alternatively I can delete the test vectors that are too long.

I ran the entire suite of tests and they pass. :-)

Edit: Errrr what? I guess not?? Edit2: Oh, Travis is testing the merge with master not the PR by itself, it just needs to be rebased again (there are a few places crate:: needs to be prefixed to a use).

Eirik0

I think this was broken by #113. I added my suggestions locally and rebased on to master and was able to build without error.

zcash_primitives/src/pedersen_hash.rs

zcash_primitives/src/test_vectors/pedersen_hash_vectors.rs

str4d · 2019-08-26T19:13:30Z

zcash_primitives/src/group_hash.rs

+            let domain = domains[i];
+            let msg = msgs[i];
+
+            let gh: edwards::Point<Bls12, _> = find_group_hash(&msg, &domain, &params);


This is unused. The test appears to actually be testing that edwards::Point::get_for_y returns one of the expected x-coordinates, and not the output of find_group_hash at all.

(Edit: I see the test is deleted later.)

str4d · 2019-08-26T19:17:00Z

zcash_primitives/src/group_hash.rs

+                .write_u32::<LittleEndian>(m)
+                .unwrap();
+            let p: edwards::Point<Bls12, _> =
+                find_group_hash(&segment_number, b"Zcash_PH", &params);


p is unused.

(Edit: I see the test is deleted later.)

zcash_primitives/src/pedersen_hash.rs

str4d · 2019-08-26T19:23:52Z

zcash_primitives/src/test_vectors/pedersen_hash_vectors.rs

@@ -0,0 +1,716 @@
+//! Test vectors from https://github.com/zcash-hackworks/zcash-test-vectors/blob/master/sapling_pedersen.py


Usually this means that the linked file can be used to generate the test vectors. We don't have the generation process for these test vectors yet; it would be good to find it. Non-blocking, because we can always update the test vectors later if there is a mismatch (although note that one of the commits from the audit explicitly tweaked some test vectors to test edge cases, so there may not be a simple generation process).

I think they're made by https://github.com/QED-it/go-jubjub

str4d · 2019-08-26T19:26:42Z

zcash_proofs/src/circuit/ecc.rs

+        check_small_order_from_strs(
+            "948411088638444611740115537621561973758360269817276634325562542866802143934",
+            "19260245455242183936012133194672327304390353749328020389743628630787497879844",
+        );


We could extend this to test all points in all small-order subgroups, since there are so few of them (i.e. raise this to the power of 0..8 and check each one).

bellman/src/gadgets/boolean.rs

zcash_primitives/src/jubjub/mod.rs

…ctors.

Eirik0 · 2019-10-07T23:50:40Z

zcash_primitives/src/jubjub/mod.rs

+            }
+
+            // check for a generator being the sum of any other two
+            for (j, p2) in pedersen_hash_generators.iter().enumerate() {


I experimented with trying to combine this loop with the one above, but wasn't able to get it to work properly. If this function were performance critical, we could first add all of the items to a HashSet, and then check if combinations of p1+p2 were in that set.

Eirik0

utACK. I did not verify all of the test vectors, but I went through the changes to the production code, and everything looks good.

Bring in QED-it Tests

2279da4 Merge pull request zcash#38 from debris/docs 2e57190 Remove documentation entry from Cargo.toml 346d540 bellman 0.2.0 8d79665 Merge pull request zcash#93 from defuse/qed-it-lrz f50079f Crate docs 701cb2b Update READMEs ccf1ee9 CI: Check intra-doc links ddd390a Add READMEs to Cargo.toml files 54d3122 Add missing cs.is_satisfied() to bellman test 52bf23c Fix build warnings 581ad35 boolean: adds tests for alloc_conditionally 0403396 blake2s: adds test vectors from go-jubjub 9f24e47 Fix blake2s test data length assertion. 42d5b3b Add blake2s test vectors for varying sizes from go-jubjub b2597de pedersen_hash: removes debug prints c903fad pedersen hashes: example of size limit bug bc697c1 bellman: Fix compile errors without multicore feature a4e5df9 Upgrade to hex-literal 0.2 c063509 Migrate bellman to crossbeam 0.7 1775843 Take self directly in into_* functions 614d784 Rename into_ -> to_ where &self is used. 08664b1 Address various clippy warnings/errors in bellman bb11ef2 cargo fmt cff2e2f cargo fix --edition-idioms for bellman dc2a280 Add edition = 2018 1a2bc19 cargo fmt ad37878 cargo fix --edition for bellman e73d1a2 cargo fmt bellman dfb86fc Move generic circuit gadgets into bellman 9b3d766 Migrate to rand 0.7 055280f Migrate ff, group, pairing, and bellman to rand 0.6 533d586 Migrate bellman to rand 0.5 bfa9aaf Merge pull request zcash#61 from rex4539/fix-typos 3dd8490 Place bellman multicore operations behind a (default) feature flag 955e679 Merge pull request zcash#46 from str4d/ff-traits d4ddaa9 Fix typos 12f93f2 Add ff and group crates to Cargo workspace 2e35a32 Update sapling-crypto crate to use ff crate 2019e63 Update workspace after pulling in external crates git-subtree-dir: bellman git-subtree-split: 2279da4

defuse self-assigned this Jul 25, 2019

defuse force-pushed the qed-it-lrz branch from b6cb931 to db7855d Compare July 25, 2019 15:53

defuse changed the title ~~Bring in QED-it Tests~~ [WIP] Bring in QED-it Tests Jul 25, 2019

defuse force-pushed the qed-it-lrz branch from d4f4405 to 480db6e Compare July 25, 2019 16:30

defuse commented Jul 25, 2019

View reviewed changes

defuse changed the title ~~[WIP] Bring in QED-it Tests~~ Bring in QED-it Tests Jul 25, 2019

defuse removed their assignment Jul 26, 2019

defuse force-pushed the qed-it-lrz branch from 13bf8d5 to 3200f17 Compare August 1, 2019 15:34

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

str4d force-pushed the qed-it-lrz branch from 3200f17 to 564666e Compare August 23, 2019 10:37

str4d requested review from ebfull, Eirik0 and str4d and removed request for ebfull August 24, 2019 00:05

defuse commented Aug 26, 2019

View reviewed changes

zcash_primitives/src/pedersen_hash.rs Outdated Show resolved Hide resolved

Eirik0 suggested changes Sep 4, 2019

View reviewed changes

zcash_primitives/src/pedersen_hash.rs Outdated Show resolved Hide resolved

zcash_primitives/src/test_vectors/pedersen_hash_vectors.rs Outdated Show resolved Hide resolved

zcash_primitives/src/test_vectors/pedersen_hash_vectors.rs Outdated Show resolved Hide resolved

str4d reviewed Sep 16, 2019

View reviewed changes

kobigurk added 6 commits September 16, 2019 20:12

pedersen_hash: prints hashes, adds comments

8541b2b

pedersen_hash: adds tests for Daniel's vector

414d651

pedersen hashes: example of size limit bug

15633ad

pedersen_hash: show a tighter limit for hash sizes

4835be0

pedersen_hash: removes debug prints

4349685

pedersen_hash: adds test vectors for the circuit implementation

7ee61c4

kobigurk and others added 20 commits September 16, 2019 20:28

adds test vectors for NoteCommit

abd0392

input circuit: removes debug prints

2b92493

group_hash: adds test vectors generated by go-jubjub

2edcc12

blake2s: adds test vectors from go-jubjub

2ee7b10

A test vector for pedersen hash

f5dfe07

Many test vectors for pedersen hash

9080b4c

Move test vectors into own module

be18eb2

Long and random PH test vectors

6240c02

PH test vectors for edge-cases

68ba93f

ecc: tests for assert_not_small_order

bb0a769

ecc: test_assert_not_small_order also tests for the generators

ace929c

ecc: makes assert_not_small_order tests deeper

e0c5ef2

boolean: adds tests for alloc_conditionally

f3533e2

Calculate number of constraints and more comprehensive test

d56758d

Test PH circuit for the size used in the Merkle tree

3efb7f9

adds test for linear relation between pedersen hash generators

15b4c37

Fix broken linear relation checking

e391594

Fix build warnings

caab37c

Remove unfinished find_group_hash test.

4dff805

Increase the number of pedersen hash generators, exercise all test ve…

3701c2b

…ctors.

str4d force-pushed the qed-it-lrz branch from 4935121 to 3701c2b Compare September 16, 2019 23:08

Add missing cs.is_satisfied() to bellman test

39a73c8

Eirik0 self-requested a review September 18, 2019 21:08

str4d mentioned this pull request Sep 28, 2019

Bring in QED-it Tests zcash/sapling-crypto#101

Closed

ebfull approved these changes Oct 7, 2019

View reviewed changes

Eirik0 reviewed Oct 7, 2019

View reviewed changes

Eirik0 approved these changes Oct 7, 2019

View reviewed changes

str4d merged commit 2399d9f into zcash:master Oct 8, 2019

ebfull pushed a commit to ebfull/librustzcash that referenced this pull request Mar 4, 2020

Merge pull request zcash#93 from defuse/qed-it-lrz

8d79665

Bring in QED-it Tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring in QED-it Tests #93

Bring in QED-it Tests #93

defuse commented Jul 25, 2019 •

edited

Loading

defuse commented Jul 25, 2019 •

edited

Loading

defuse left a comment

defuse Jul 25, 2019

defuse commented Aug 1, 2019 •

edited

Loading

str4d commented Aug 19, 2019

str4d commented Aug 22, 2019

str4d commented Aug 23, 2019

defuse left a comment •

edited

Loading

defuse commented Aug 29, 2019 •

edited

Loading

Eirik0 left a comment

str4d Aug 26, 2019

str4d Aug 26, 2019

str4d Aug 26, 2019

defuse Sep 18, 2019

str4d Aug 26, 2019

Eirik0 Oct 7, 2019

Eirik0 left a comment •

edited

Loading

		@@ -0,0 +1,716 @@
		//! Test vectors from https://github.com/zcash-hackworks/zcash-test-vectors/blob/master/sapling_pedersen.py

Bring in QED-it Tests #93

Bring in QED-it Tests #93

Conversation

defuse commented Jul 25, 2019 • edited Loading

defuse commented Jul 25, 2019 • edited Loading

defuse left a comment

Choose a reason for hiding this comment

defuse Jul 25, 2019

Choose a reason for hiding this comment

defuse commented Aug 1, 2019 • edited Loading

str4d commented Aug 19, 2019

str4d commented Aug 22, 2019

str4d commented Aug 23, 2019

defuse left a comment • edited Loading

Choose a reason for hiding this comment

defuse commented Aug 29, 2019 • edited Loading

Eirik0 left a comment

Choose a reason for hiding this comment

str4d Aug 26, 2019

Choose a reason for hiding this comment

str4d Aug 26, 2019

Choose a reason for hiding this comment

str4d Aug 26, 2019

Choose a reason for hiding this comment

defuse Sep 18, 2019

Choose a reason for hiding this comment

str4d Aug 26, 2019

Choose a reason for hiding this comment

Eirik0 Oct 7, 2019

Choose a reason for hiding this comment

Eirik0 left a comment • edited Loading

Choose a reason for hiding this comment

defuse commented Jul 25, 2019 •

edited

Loading

defuse commented Jul 25, 2019 •

edited

Loading

defuse commented Aug 1, 2019 •

edited

Loading

defuse left a comment •

edited

Loading

defuse commented Aug 29, 2019 •

edited

Loading

Eirik0 left a comment •

edited

Loading