Support for Hyrax polynomial commitment scheme #1

srinathsetty · 2023-07-29T00:15:06Z

No description provided.

huitseeker

I left a few inline comments, please lmk if I can help upstream this to Nova!

src/provider/hyrax_pc.rs

huitseeker · 2023-08-04T13:20:36Z

src/provider/hyrax_pc.rs

+  fn commit(ck: &Self::CommitmentKey, v: &[G::Scalar]) -> Self::Commitment {
+    let poly = MultilinearPolynomial::new(v.to_vec());
+    let n = poly.len();
+    let ell = poly.get_num_vars();
+    assert_eq!(n, (2usize).pow(ell as u32));
+
+    let (left_num_vars, right_num_vars) = EqPolynomial::<G::Scalar>::compute_factored_lens(ell);
+    let L_size = (2usize).pow(left_num_vars as u32);
+    let R_size = (2usize).pow(right_num_vars as u32);
+    assert_eq!(L_size * R_size, n);
+
+    let comm = (0..L_size)
+      .collect::<Vec<usize>>()
+      .into_par_iter()
+      .map(|i| {
+        PedersenCommitmentEngine::commit(&ck.ck, &poly.get_Z()[R_size * i..R_size * (i + 1)])
+      })
+      .collect();
+
+    HyraxCommitment {
+      comm,
+      is_default: false,


I see. Rather than a boolean flag is_default, I think what you want to employ in the HyraxCommitment is a std::cell::OnceCell.
This OnceCell represents a slot that you can fill "only once" (with any value you want), after which it becomes immutable. This should allow you to implement logic for commitment and operations thereof without having to rely on error management, or flag-checking logic.

huitseeker · 2023-08-04T13:27:32Z

src/provider/hyrax_pc.rs

+macro_rules! define_add_variants {
+  (G = $g:path, LHS = $lhs:ty, RHS = $rhs:ty, Output = $out:ty) => {
+    impl<'b, G: $g> Add<&'b $rhs> for $lhs {
+      type Output = $out;
+      fn add(self, rhs: &'b $rhs) -> $out {
+        &self + rhs
+      }
+    }
+
+    impl<'a, G: $g> Add<$rhs> for &'a $lhs {
+      type Output = $out;
+      fn add(self, rhs: $rhs) -> $out {
+        self + &rhs
+      }
+    }
+
+    impl<G: $g> Add<$rhs> for $lhs {
+      type Output = $out;
+      fn add(self, rhs: $rhs) -> $out {
+        &self + &rhs
+      }
+    }
+  };
+}
+
+macro_rules! define_add_assign_variants {
+  (G = $g:path, LHS = $lhs:ty, RHS = $rhs:ty) => {
+    impl<G: $g> AddAssign<$rhs> for $lhs {
+      fn add_assign(&mut self, rhs: $rhs) {
+        *self += &rhs;
+      }
+    }
+  };
+}


Consider using forward_ref or even, if need be, forward_ref_generic. This is used in the compiler itself

huitseeker · 2023-08-04T13:33:50Z

src/provider/hyrax_pc.rs

+impl<G: Group> TranscriptReprTrait<G> for HyraxCommitment<G> {
+  fn to_transcript_bytes(&self) -> Vec<u8> {
+    let mut v = Vec::new();
+    v.append(&mut b"poly_commitment_begin".to_vec());
+
+    for c in &self.comm {
+      v.append(&mut c.to_transcript_bytes());
+    }
+
+    v.append(&mut b"poly_commitment_end".to_vec());
+    v
+  }
+}


The complex mutable logic of append requires an allocation for the rightmost argument, one that's not needed here since we perform it on the fly. Consider the following:

impl<G: Group> TranscriptReprTrait<G> for HyraxCommitment<G> { fn to_transcript_bytes(&self) -> Vec<u8> { let mut v = Vec::new(); v.extend_from_slice(b"poly_commitment_begin"); for c in &self.comm { v.extend_from_slice(&c.to_transcript_bytes()); } v.extend_from_slice(b"poly_commitment_end"); v } }

huitseeker · 2023-08-04T13:34:07Z

src/provider/hyrax_pc.rs

+}
+
+impl<G: Group> TranscriptReprTrait<G> for HyraxCompressedCommitment<G> {
+  fn to_transcript_bytes(&self) -> Vec<u8> {


See above for a more idiomatic formulation.

huitseeker · 2023-08-04T13:35:30Z

src/provider/hyrax_pc.rs

+  type EvaluationArgument = HyraxEvaluationArgument<G>;
+
+  fn setup(
+    ck: &<Self::CE as CommitmentEngineTrait<G>>::CommitmentKey,


ck will be copied in all instances of calls to this function, so it's best to take it by value and let the caller figure out how to come up with that copy (C-CALLER-CONTROL)

huitseeker · 2023-08-04T13:37:31Z

src/provider/pedersen.rs


-impl<G: Group> CommitmentKeyExtTrait<G> for CommitmentKey<G> {
-  type CE = CommitmentEngine<G>;
+  /// Reinterprets the commitments as a commitment key


It would be really good to have a line of comments or two explaining the re-interpretation and a justification for why it makes sense (naturally, this wasn't introduced in your PR, but relating the CommitmentKeyExtTrait to homomorphic properties is non-trivial enough to deserve a comment IMHO).

huitseeker · 2023-08-04T13:39:17Z

src/spartan/polynomial.rs

+    (0..R_size)
+      .map(|i| {
+        (0..L_size)
+          .map(|j| L[j] * self.Z[j * R_size + i])
+          .fold(Scalar::ZERO, |x, y| x + y)
+      })
+      .collect()


A.k.a.:

let mut res = Vec::with_capacity(R_size); for i in 0..R_size { let sum = (0..L_size) .map(|j| L[j] * self.Z[j * R_size + i]) .sum(); res.push(sum); } res

src/traits/commitment.rs

huitseeker · 2023-08-04T13:44:29Z

src/provider/hyrax_pc.rs

+  ) -> Result<Self::EvaluationArgument, SpartanError> {
+    transcript.absorb(b"poly_com", comm);
+
+    let poly_m = MultilinearPolynomial::<G::Scalar>::new(poly.to_vec());


Same as above: if you're going to need to copy the full vector, you probably want to be explicit about it and make argument poly: Vec<G::Scalar>. (C-CALLER-CONTROL)

Take this with a grain of salt: MultilinearPolynomial in general is a bit unwieldy and would probably benefit from a larger refactoring based on Iterator APIs

huitseeker · 2023-08-04T19:24:21Z

src/r1cs.rs

+    let W = {
+      let mut W = self.W.clone();
+      W.extend(vec![G::Scalar::ZERO; S.num_vars - W.len()]);
+      W
+    };


Nit: This is more performant written as:

let W = { assert!(self.W.len() <= S.num_vars()); let mut W = vec![G::Scalar::Zero; S.num_vars]; W[...self.W.len()].copy_from_slice(&self.W); };

because it avoids the possible allocation involved in extending from self.W.len() to S.num_vars()

sga001 · 2023-08-09T13:52:37Z

I left a few inline comments, please lmk if I can help upstream this to Nova!

Thanks for the comments. This code is unlikely to be useful in Nova, though, as it will make the folding and the folding verifier circuit (which runs on each step) more complicated and expensive.

huitseeker · 2023-08-09T21:42:42Z

@sga001 The performance point is well-taken, and I indeed see in it reason to not have Hyrax as the default PCS for Nova. However, I think having a hiding PCS may open up a different range of trust assumptions between the (possibly several) prover(s) involved in the folding.

sga001 · 2023-08-09T21:51:08Z

@sga001 The performance point is well-taken, and I indeed see in it reason to not have Hyrax as the default PCS for Nova. However, I think having a hiding PCS may open up a different range of trust assumptions between the (possibly several) prover(s) involved in the folding.

I'm not sure I understand the scenario you have in mind, but integrating Hyrax into Nova is non-trivial.
Another thing I want to make sure you know is that the Nova code base and this Spartan2 codebase are not zero-knowledge. Similarly, this specific Hyrax PC is not hiding.

Making them ZK is a bit of work (but doable and something we will do at some point); making Hyrax PC hiding is trivial though (just add blind).

srinathsetty · 2023-08-09T21:53:42Z

@sga001 The performance point is well-taken, and I indeed see in it reason to not have Hyrax as the default PCS for Nova. However, I think having a hiding PCS may open up a different range of trust assumptions between the (possibly several) prover(s) involved in the folding.

I think there may be some confusion here. I understand we eventually need to make Nova proofs (compressed or otherwise) zero-knowledge: microsoft/Nova#174. For compressed proofs, Hyrax provides a zero-knowledge transformation, but it does not require using Hyrax-PC. The current Nova code uses Spartan with IPA-PC to compress IVC proofs, one can apply zk transformation to that directly, which we plan to do at some point.

huitseeker · 2023-08-09T22:14:26Z

That makes sense, thanks for the clarification, @srinathsetty

huitseeker reviewed Aug 4, 2023

View reviewed changes

srinathsetty force-pushed the hyrax_pc2 branch from 0d79a8d to 50238d2 Compare August 4, 2023 20:40

huitseeker mentioned this pull request Aug 9, 2023

Zero-knowledge implementation gap microsoft/Nova#174

Open

srinathsetty force-pushed the hyrax_pc2 branch from 8a1e812 to 883d537 Compare August 29, 2023 22:12

srinathsetty force-pushed the hyrax_pc2 branch 3 times, most recently from 6fb4ef2 to f657684 Compare September 29, 2023 23:48

sga001 and others added 17 commits October 13, 2023 15:23

not quite there... annoying transcript stuff

fe13834

small stuff

e2f1762

small stuff

a7f6882

some progress

6fdf48c

fix transcript

aa17240

add doc, use generic types

e41cb22

local tests passing

035a191

evaluation engine

a2734dc

fix trait requirements, except for one

6ce34f2

checkpoint

aa07afc

remove Copy requirement on commitment type

1251d23

fix an issue

8c99733

test passes

ddb1e08

fix clippy warnings

b19c5ff

reduce ck size

ef280eb

pad shape and witness when created

ca87bf2

fix clippy

a80aa15

srinathsetty added 5 commits October 13, 2023 15:26

port sha256 bench from nova

b1de20d

fix bench

31ee086

remove unneeded Phantom

1df4cab

resolve conflicts

ce0cf51

fix clippy

63f5636

srinathsetty force-pushed the hyrax_pc2 branch from aef6168 to 63f5636 Compare October 16, 2023 21:14

srinathsetty added 2 commits January 10, 2024 12:02

update halo2curves; expose asm feature

dbda004

fix clippy

854f47f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Hyrax polynomial commitment scheme #1

Support for Hyrax polynomial commitment scheme #1

srinathsetty commented Jul 29, 2023

huitseeker left a comment

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023

huitseeker Aug 4, 2023 •

edited

Loading

sga001 commented Aug 9, 2023

huitseeker commented Aug 9, 2023

sga001 commented Aug 9, 2023

srinathsetty commented Aug 9, 2023 •

edited

Loading

huitseeker commented Aug 9, 2023

Support for Hyrax polynomial commitment scheme #1

Are you sure you want to change the base?

Support for Hyrax polynomial commitment scheme #1

Conversation

srinathsetty commented Jul 29, 2023

huitseeker 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

huitseeker Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

sga001 commented Aug 9, 2023

huitseeker commented Aug 9, 2023

sga001 commented Aug 9, 2023

srinathsetty commented Aug 9, 2023 • edited Loading

huitseeker commented Aug 9, 2023

huitseeker Aug 4, 2023 •

edited

Loading

srinathsetty commented Aug 9, 2023 •

edited

Loading