Ensure exactly one selection in get_from_vec_alloc_relaxed_r1cs. #97

porcuquine · 2023-11-03T16:18:17Z

This PR fixes #94. I think the alternate approach (checking consistency of the chosen and received target index) is probably a little better. I may take a shot at this along with investigating a refactoring to avoid duplication of the bit-checking, etc. in a separate PR. Meanwhile, this should fix the immediate issue straightforwardly.

adr1anh · 2023-11-03T16:48:28Z

I think with this we can remove this comment. Checking the sum of the equal_bits is equivalent to range-check, so the comment no longer applies.

Otherwise the change looks good to me.

porcuquine · 2023-11-03T16:50:53Z

Done.

porcuquine · 2023-11-03T23:11:15Z

src/supernova/test.rs

@@ -658,7 +658,7 @@ fn test_recursive_circuit() {
  let ro_consts1: ROConstantsCircuit<G2> = PoseidonConstantsCircuit::default();
  let ro_consts2: ROConstantsCircuit<G1> = PoseidonConstantsCircuit::default();

-  test_recursive_circuit_with::<G1, G2>(&params1, &params2, ro_consts1, ro_consts2, 9836, 12035);
+  test_recursive_circuit_with::<G1, G2>(&params1, &params2, ro_consts1, ro_consts2, 9835, 12028);


porcuquine · 2023-11-03T23:12:56Z

I played around, and this was the best I could do for constraints. I think it's a good stopping point. Now we reuse the selector, and I added an optimization for allocation of equality with constant.

huitseeker

Looks good to me! The comments on the multicase could benefit from being more descriptive about what they designate (this should be clear to Lurk developers, but I'm thinking about the nova context where we'd upstream this code).

src/gadgets/utils.rs

adr1anh

The second optimization round looks good to me. Left a small suggestion for the future "case" gadget.

adr1anh · 2023-11-06T08:35:57Z

src/gadgets/utils.rs

+  let t = AllocatedNum::alloc(cs.namespace(|| "t"), || {
+    Ok(if *a.get_value().get()? == b {
+      F::ONE
+    } else {
+      (*a.get_value().get()? - b).invert().unwrap()
+    })
+  })?;


In situations where we want to compute a boolean selector vector, we may want to leverage batch_invert when computing the ts for each bit. Inversion is an expensive operation, and the threshold at which batch_invert becomes cheaper than individual inversions is very low (each additional element to the batch is only 3 field multiplications.

This backports the following Arecibo PRs: - #2 - #3 - #10 - #16 - #23 - #30 - #28 - #41 - #45 - #50 - #56 - #51 - #72 - #92 - #95 - #97 - #101 - #110 - #106 - #112 - #114 - #119 - #120 - #127 - #123 - #131 - #174 - #175 - #182 Co-authored-by: WYATT <[email protected]> Co-authored-by: Hanting Zhang <[email protected]> Co-authored-by: Ming <[email protected]> Co-authored-by: porcuquine <[email protected]> Co-authored-by: Samuel Burnham <[email protected]> Co-authored-by: Matej Penciak <[email protected]> Co-authored-by: Adrian Hamelink <[email protected]>

porcuquine requested a review from adr1anh November 3, 2023 16:18

porcuquine force-pushed the fix-get-from-vec branch from bd30e2c to 916d8b6 Compare November 3, 2023 16:29

porcuquine commented Nov 3, 2023

View reviewed changes

huitseeker reviewed Nov 5, 2023

View reviewed changes

src/gadgets/utils.rs Outdated Show resolved Hide resolved

src/gadgets/utils.rs Outdated Show resolved Hide resolved

src/gadgets/utils.rs Outdated Show resolved Hide resolved

src/gadgets/utils.rs Show resolved Hide resolved

adr1anh previously approved these changes Nov 6, 2023

View reviewed changes

porcuquine added 6 commits November 6, 2023 09:38

Ensure exactly one selection.

5a4e98d

Remove outdated comment.

504c4eb

Return selector.

4866fad

Reuse selector.

5ae53bc

Optimize constant equality allocation.

1ae44b8

Clippy.

77f6ead

porcuquine force-pushed the fix-get-from-vec branch from 7e1d08b to 77f6ead Compare November 6, 2023 17:39

Small refactorings.

82e47c4

porcuquine dismissed adr1anh’s stale review via 82e47c4 November 6, 2023 18:30

Add proptest.

21ec7e3

porcuquine enabled auto-merge November 6, 2023 18:44

huitseeker approved these changes Nov 6, 2023

View reviewed changes

porcuquine added this pull request to the merge queue Nov 6, 2023

Merged via the queue into lurk-lang:dev with commit a256108 Nov 6, 2023
3 checks passed

porcuquine deleted the fix-get-from-vec branch November 6, 2023 19:02

huitseeker added the supernova label Nov 29, 2023

huitseeker mentioned this pull request Dec 18, 2023

feat: implement SuperNova microsoft/Nova#283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure exactly one selection in get_from_vec_alloc_relaxed_r1cs. #97

Ensure exactly one selection in get_from_vec_alloc_relaxed_r1cs. #97

porcuquine commented Nov 3, 2023

adr1anh commented Nov 3, 2023 •

edited

Loading

porcuquine commented Nov 3, 2023

porcuquine Nov 3, 2023

porcuquine commented Nov 3, 2023

huitseeker left a comment

adr1anh left a comment

adr1anh Nov 6, 2023

Ensure exactly one selection in get_from_vec_alloc_relaxed_r1cs. #97

Ensure exactly one selection in get_from_vec_alloc_relaxed_r1cs. #97

Conversation

porcuquine commented Nov 3, 2023

adr1anh commented Nov 3, 2023 • edited Loading

porcuquine commented Nov 3, 2023

porcuquine Nov 3, 2023

Choose a reason for hiding this comment

porcuquine commented Nov 3, 2023

huitseeker left a comment

Choose a reason for hiding this comment

adr1anh left a comment

Choose a reason for hiding this comment

adr1anh Nov 6, 2023

Choose a reason for hiding this comment

adr1anh commented Nov 3, 2023 •

edited

Loading