Action circuit #101

therealyingtong · 2021-06-04T08:45:51Z

Closes #4. Closes #157. Closes #153.

Canonicity checks for Sinsemilla inputs documented at #134.

codecov-commenter · 2021-06-04T09:34:04Z

Codecov Report

Merging #101 (3f506a0) into main (bd28b46) will decrease coverage by 0.23%.
The diff coverage is 84.81%.

@@            Coverage Diff             @@
##             main     zcash/orchard#101      +/-   ##
==========================================
- Coverage   87.36%   87.13%   -0.24%     
==========================================
  Files          62       63       +1     
  Lines        6294     7922    +1628     
==========================================
+ Hits         5499     6903    +1404     
- Misses        795     1019     +224

Impacted Files	Coverage Δ
src/circuit/gadget/ecc/chip/mul/overflow.rs	`81.81% <ø> (ø)`
src/constants.rs	`100.00% <ø> (ø)`
...rc/circuit/gadget/sinsemilla/chip/hash_to_point.rs	`78.97% <55.55%> (-1.13%)`	⬇️
src/circuit/gadget/ecc/chip/mul.rs	`85.65% <80.00%> (+1.20%)`	⬆️
src/circuit/gadget/sinsemilla/commit_ivk.rs	`80.59% <80.59%> (ø)`
src/circuit/gadget/sinsemilla/note_commit.rs	`81.06% <81.06%> (ø)`
src/circuit/gadget/sinsemilla/merkle.rs	`91.74% <83.33%> (-2.12%)`	⬇️
src/circuit.rs	`87.08% <86.78%> (-1.81%)`	⬇️
src/tree.rs	`81.94% <87.50%> (+3.22%)`	⬆️
src/circuit/gadget/sinsemilla.rs	`87.77% <88.00%> (+3.11%)`	⬆️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd28b46...3f506a0. Read the comment docs.

src/circuit.rs

src/circuit/gadget/sinsemilla/note_commit.rs

str4d

Flushing comments.

src/circuit/gadget/sinsemilla/note_commit.rs

src/circuit.rs

therealyingtong · 2021-07-27T01:44:24Z

src/circuit.rs

-            let s = meta.query_selector(selector);
+        // Addition of two field elements poseidon_hash(nk, rho_old) + psi_old.
+        let q_add = meta.selector();
+        meta.create_gate("poseidon_hash(nk, rho_old) + psi_old", |meta| {


We could merge this into the same gate as q_orchard.

daira

utACK for the QEDIT review.

I want to go back and do a more thorough review of the canonicity checks, but that shouldn't block merging.

str4d

One blocking comment, otherwise utACK with various comments and suggestions.

src/circuit/gadget/sinsemilla/note_commit.rs

str4d · 2021-07-27T01:54:39Z

src/circuit/gadget/sinsemilla/note_commit.rs

+            let k_3 = meta.query_advice(advices[5], Rotation::cur());
+
+            // j = LSB + (2)k_0 + (2^10)k_1
+            let j = meta.query_advice(advices[0], Rotation::next());


I agree that we need to witness j in this region (even though it could be computed here), because we need a cell for this intermediate result that we can constrain to be the input to the decomposition.

src/circuit/gadget/sinsemilla/note_commit.rs

src/circuit.rs

Instead of separately witnessing k_1 and equating it to z1_j, we can directly make use of z1_j in the gate. This allows us to fit the region into a 5 x 2 area, improving the layout. Co-authored-by: Jack Grigg <jack@electriccoin.co>

By rearranging the pieces in the gate, we remove a prev() query and preserve proximity between pieces involved in the same constraint. This commit also includes several minor fixes: - use strict mode for decomposition of j in y-coordinate check; - Name All Polynomial Constraints; - remove point_repr() helper function; - variable renaming and docfixes. Co-authored-by: Jack Grigg <jack@electriccoin.co>

Co-authored-by: Daira Hopwood <daira@jacaranda.org> Co-authored-by: Jack Grigg <jack@electriccoin.co>

str4d

utACK modulo test fixes. Circuit layout LGTM.

src/circuit/gadget/sinsemilla/note_commit.rs

therealyingtong force-pushed the action-circuit branch 2 times, most recently from 6b443de to 6bde1a7 Compare June 4, 2021 09:23

therealyingtong force-pushed the action-circuit branch 2 times, most recently from d74bf7c to 73dd71f Compare June 4, 2021 17:06

therealyingtong changed the base branch from base-action-circuit to merkle-chip June 4, 2021 17:07

therealyingtong force-pushed the merkle-chip branch from ca15012 to 19bcfa0 Compare June 5, 2021 01:01

therealyingtong force-pushed the action-circuit branch from 73dd71f to e50d757 Compare June 5, 2021 01:08

therealyingtong force-pushed the merkle-chip branch 3 times, most recently from 8a5f541 to 527963c Compare June 5, 2021 09:05

therealyingtong force-pushed the action-circuit branch 2 times, most recently from 303f6cc to 645cab3 Compare June 5, 2021 13:43

therealyingtong force-pushed the merkle-chip branch 2 times, most recently from 2731554 to 8602455 Compare June 6, 2021 12:13

therealyingtong force-pushed the action-circuit branch from 645cab3 to ced4d11 Compare June 6, 2021 12:23

therealyingtong force-pushed the merkle-chip branch 2 times, most recently from 63ecab5 to 6d71190 Compare June 6, 2021 16:51

therealyingtong force-pushed the action-circuit branch from ced4d11 to 58fe6fc Compare June 6, 2021 17:03

therealyingtong force-pushed the merkle-chip branch from 6d71190 to 5c93f76 Compare June 7, 2021 05:50

therealyingtong force-pushed the action-circuit branch from 58fe6fc to cecde82 Compare June 7, 2021 10:29

str4d force-pushed the merkle-chip branch from 5c93f76 to f0031b2 Compare June 8, 2021 00:55

str4d force-pushed the action-circuit branch from 3eb4068 to 4fc35bc Compare June 8, 2021 01:05

therealyingtong commented Jun 8, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

therealyingtong force-pushed the merkle-chip branch from f0031b2 to db607b6 Compare June 8, 2021 23:17

therealyingtong force-pushed the action-circuit branch from c9480e5 to 7dd8190 Compare June 8, 2021 23:24

therealyingtong force-pushed the merkle-chip branch from db607b6 to 312f404 Compare June 9, 2021 12:17

therealyingtong force-pushed the action-circuit branch from 7dd8190 to e9393ec Compare June 9, 2021 12:26

therealyingtong force-pushed the merkle-chip branch from 312f404 to c100aae Compare June 10, 2021 18:31

therealyingtong force-pushed the action-circuit branch from e9393ec to c5a34a1 Compare June 10, 2021 18:37

therealyingtong force-pushed the action-circuit branch from 5ab9717 to b3ccd3f Compare July 26, 2021 19:17

therealyingtong commented Jul 26, 2021

View reviewed changes

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

therealyingtong commented Jul 26, 2021

View reviewed changes

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

str4d reviewed Jul 26, 2021

View reviewed changes

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

daira reviewed Jul 27, 2021

View reviewed changes

src/circuit.rs Outdated Show resolved Hide resolved

therealyingtong commented Jul 27, 2021

View reviewed changes

daira approved these changes Jul 27, 2021

View reviewed changes

therealyingtong mentioned this pull request Jul 27, 2021

Merge lookup arguments in lookup_range_check helper. #161

Merged

str4d requested changes Jul 27, 2021

View reviewed changes

therealyingtong and others added 2 commits July 27, 2021 12:49

therealyingtong force-pushed the action-circuit branch from 69957cb to 7aa3174 Compare July 27, 2021 05:52

therealyingtong and others added 3 commits July 27, 2021 15:23

note_commit::tests: Constrain output of NoteCommit to expected point.

fa135fe

commit_ivk::tests: Check value of output ivk against expected ivk.

664125f

circuit.rs: Minor cleanups and column optimisations.

3f506a0

Co-authored-by: Daira Hopwood <daira@jacaranda.org> Co-authored-by: Jack Grigg <jack@electriccoin.co>

str4d approved these changes Jul 27, 2021

View reviewed changes

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

src/circuit/gadget/sinsemilla/note_commit.rs Outdated Show resolved Hide resolved

Fix y-coordinate recovery in NoteCommit tests

620e227

str4d marked this pull request as ready for review July 27, 2021 08:48

str4d merged commit bb90f2e into main Jul 27, 2021

str4d deleted the action-circuit branch July 27, 2021 08:49

This was referenced Jul 28, 2021

Add Action circuit benchmarks, fix Builder bug #167

Merged

Reduce marginal proof cost by eliminating queries #168

Closed

Transaction building support #35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Action circuit #101

Action circuit #101

therealyingtong commented Jun 4, 2021 •

edited

Loading

codecov-commenter commented Jun 4, 2021 •

edited

Loading

str4d left a comment

therealyingtong Jul 27, 2021

daira left a comment

str4d left a comment

str4d Jul 27, 2021

str4d left a comment

Action circuit #101

Action circuit #101

Conversation

therealyingtong commented Jun 4, 2021 • edited Loading

codecov-commenter commented Jun 4, 2021 • edited Loading

Codecov Report

str4d left a comment

Choose a reason for hiding this comment

therealyingtong Jul 27, 2021

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

str4d Jul 27, 2021

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

therealyingtong commented Jun 4, 2021 •

edited

Loading

codecov-commenter commented Jun 4, 2021 •

edited

Loading