Refactor Ginger-Lib #144

phoinic · 2021-12-09T00:09:03Z

This PR contains the refactored algebra module and adjustments for this refactoring in other modules. The details of this refactoring are described in document "Refactoring ginger-lib". Key changes:

Group trait is a basic trait for Fields, Polynomials and Curves
Curve trait introduced
Projective representation for curve points is default
Affine representation used only for efficient mixed arithmetic and moved to the Curve trait as a nested type
All curves removed except Tweedle and secp256k1
All curves and fields models which are not used be Tweedle and secp256k1 are removed

Known Issues:

In algebra / secp256k1 unit tests there are hardcoded points in affine representation. Correct values for Jacobian representation should be placed. For now these UTs are broken and marked as ignored
In primitives / merkle_tree / optimized the UTs were switched from MNT curve to tweedle. The hardcoded expected output values should be updated with correct ones. For now these UTs are broken and marked as ignored
In proof-systems / darlin / tests inside TestCircuit there is a comment:
// This calculation is wrong, as enforce_equal allocates new variables.
// However, fixing this may cause unit tests to crash since num_constraints
// and num_variables are generated at random and an underflow may happen.
// Fix both
UTs can randomly failed or pass this part with error "substract with overflow"
Inside r1cs / gadgets / crypto / crh / pedersen the unit-test was switched from JubJub to Tweedle. But with tweedle this UT failed for some reason. This is not a refactoring related problem because in case of changing JubJub to Tweedle inside none-refactored ginger-lib we have the same issue with this UT. For now UT is marked as ignored

…adget

primitives/src/crh/bowe_hopwood/mod.rs

… to Marlin and PolyCommit

UlrichHaboeck75

As this is a quite cross-over pull request, it's hard to give a summary. Please see my comments below.

README.md

algebra/src/curves/check_curve_parameters.sage

UlrichHaboeck75 · 2022-03-21T16:40:42Z

algebra/src/curves/check_curve_parameters.sage

+#### Checking if the file contains Short Weierstrass parameters. If not, the check is interrupted.
+#### If there are Twisted Edwards and Montgomery parameters, they are discarded.
+if 'SWModelParameters' in readfile:
+    if 'TEModelParameters' in readfile:
+        to_be_removed = readfile[readfile.index('impl TEModelParameters'):readfile.index('impl SWModelParameters')]
+        readfile = readfile.replace(to_be_removed,'')
+else:
+    print("WARNING! THE FILE HAS NO SHORT WEIERSTRASS PARAMETERS!")
+    sys.exit()


@DDT92 for now the script is only for SWModelParameters. Would be cool to handle TEModelParameters and MontgomeryModelParameters too!

algebra/src/curves/ed25519/mod.rs

r1cs/gadgets/std/src/groups/curves/short_weierstrass/short_weierstrass_jacobian.rs

r1cs/gadgets/std/src/groups/curves/short_weierstrass/short_weierstrass_projective.rs

UlrichHaboeck75 · 2022-04-01T11:56:53Z

r1cs/gadgets/std/src/groups/curves/twisted_edwards/mod.rs

-            Ok(())
+            let mut power = self.clone();
+            let mut acc = Self::zero(cs.ns(|| "alloc acc"))?;
+            for (i, bit) in bits.enumerate() {


This is the ordinary double and add. We should check if there are improvements as we have for Weierstrass curves.

I added a TODO

UlrichHaboeck75 · 2022-04-01T12:05:21Z

r1cs/gadgets/std/src/groups/curves/twisted_edwards/mod.rs

+            CS: ConstraintSystemAbstract<ConstraintF>,
+            T: 'a + ToBitsGadget<ConstraintF> + ?Sized,
+            I: Iterator<Item = &'a T>,
+            B: Borrow<[TEExtended<P>]>,


As for the other msm gadgets. See my comments there.

UlrichHaboeck75 · 2022-04-01T12:10:27Z

r1cs/gadgets/std/src/groups/curves/twisted_edwards/mod.rs

@@ -1302,11 +914,12 @@ mod projective_impl {
        ) -> Result<Self, SynthesisError>
        where
            FN: FnOnce() -> Result<T, SynthesisError>,
-            T: Borrow<TEProjective<P>>,
+            T: Borrow<TEExtended<P>>,


A general comment: why do we enforce curve membership for an input?

Probably a leftover, but we do it also for SW curves. Can we safely remove such checks ?

lgiussan · 2022-04-01T14:30:38Z

algebra/src/groups/group_vec.rs

+use num_traits::Zero;
+use serde::*;
+
+#[derive(Clone, PartialEq, Eq, Debug, Hash, CanonicalSerialize, CanonicalDeserialize, Serialize, Deserialize)]


The issue of trailing zeros pops up also regarding the PartialEq and Eq traits, which cannot be derived if we want to correctly take into account trailing zeros.
I would suggest the following implementation instead (disclaimer: untested and adds a dependency to the crate itertools)

// The trait PartialEq cannot be simply derived, because this would cause wrong results when // comparing vectors which are equal apart from a different number of trailing zeros (in // particular when comparing different representations of the zero vector). impl<G: Group> PartialEq<Self> for GroupVec<G> { fn eq(&self, other: &Self) -> bool { self.0 .iter() .zip_longest(other.0.iter()) .all(|elems| match elems { EitherOrBoth::Both(g1, g2) => g1 == g2, EitherOrBoth::Left(g) => g.is_zero(), EitherOrBoth::Right(g) => g.is_zero(), }) } } impl<G: Group> Eq for GroupVec<G> {}

To me, I would enforce the comparison only with Polynomials/GroupVec of the same length.
If they aren't they are not equal.
If they are, and they have trailing zeros, then they must have the same amount of trailing zeros

I'll do it. This 0 repr for Polynomial and GroupVec only affects the DomainExtendedPolyCommit.
For the moment let's handle problems individually. When we are going to refactor, removing the DomainExtended and assuming that Commitments are always segmented, probably we can do better.

lgiussan · 2022-04-01T15:57:53Z

algebra/src/fft/polynomial/dense.rs

-use std::ops::{Add, AddAssign, Deref, DerefMut, Div, Mul, Neg, Sub, SubAssign};
+use std::io::{Read, Result as IoResult, Write};
+use std::ops::{Add, AddAssign, Deref, DerefMut, Div, Mul, MulAssign, Neg, Sub, SubAssign};
+use num_traits::Zero;

 /// Stores a polynomial in coefficient form.
 #[derive(Clone, PartialEq, Eq, Hash, Default, CanonicalSerialize, CanonicalDeserialize)]


I think that deriving PartialEq and Eq for DensePolynomial leads to the same issue as for GroupVec concerning the possible presence of trailing zeros. The problem could be solved in a similar way too.

I'll do. See my answer abovee

UlrichHaboeck75 · 2022-04-04T12:47:33Z

primitives/src/crh/bowe_hopwood/mod.rs

+pub trait PedersenWindow: Clone {
+    const WINDOW_SIZE: usize;
+    const NUM_WINDOWS: usize;
+}


Let us document here the definition of WINDOW_SIZE and NUM_WINDOWS, including security conditions.

…ation

… into force_deserialize

UlrichHaboeck75 · 2022-04-19T09:42:03Z

algebra/src/curves/mod.rs

@@ -29,6 +29,10 @@ pub mod tests;
 pub use self::models::*;

 /// Projective representation of an elliptic curve point.
+/// This trait t serves curve-specific functions not covered


Typo? This trait t ...

Fixed, thanks

r1cs/gadgets/std/src/groups/curves/short_weierstrass/short_weierstrass_jacobian.rs

UlrichHaboeck75 · 2022-04-19T09:48:49Z

algebra/src/curves/mod.rs

@@ -71,10 +75,10 @@ pub trait Curve:

    /// Convert, if possible, a batch of `self` points to their affine equivalent.
    #[inline]
-    fn batch_into_affine<'a>(vec_self: &'a [Self]) -> Result<Vec<Self::AffineRep>, Error> {
+    fn batch_into_affine(vec_self: Vec<Self>) -> Result<Vec<Self::AffineRep>, Error> {


Is there a reason for having a Vec instead of a slice, as in batch_from_affine()?

Logically yes: you are consuming Self to obtain an Affine (all the usages were like this in the end, and I don't see any reason why Projective and Affine are allowed to co-exist in the same piece of code at a given moment)

UlrichHaboeck75 · 2022-04-19T10:34:04Z

algebra/src/curves/tests.rs

+
+        // Force deserialize test
+        {
+            let a = Jacobian::<P>::rand(rng);


Why do we resample a?

(Not the same, but related: Why is a set zero in Line 373?)

No particular reason, just to have more randomness. About the second comment a is not set to zero, are you looking at the correct diff ?

UlrichHaboeck75 · 2022-04-19T10:50:57Z

algebra/src/curves/tests.rs

+    for _ in 0..ITERATIONS {
+        let g = Jacobian::<P>::rand(&mut rng);
+        let g_affine = g.try_into().unwrap();
+        let g_projective = Jacobian::<P>::from_affine(&g_affine);
+        assert_eq!(g, g_projective);
+    }


What happens if the sampled g is the zero element? Is this excluded by our implementation of the rng?

The rng works by sampling a random x and then plug it in the curve equation. I don't think it's possible to sample infinity

UlrichHaboeck75 · 2022-04-19T11:04:37Z

algebra/src/groups/group_vec.rs

@@ -137,6 +160,16 @@ impl<G: Group> Neg for GroupVec<G> {
    }
 }

+impl<'a, G: Group> Neg for &'a GroupVec<G> {


What exactly is the reason for this extra implementation of Neg (and the other arithmetic traits below)?

I think I needed mainly for re-using the group tests. Plus they don't harm, they provide additional capabilities

UlrichHaboeck75 · 2022-04-19T12:12:16Z

algebra/src/groups/group_vec.rs

+        let (a, mut b) = {
+            let rng = &mut thread_rng();
+            (GroupVec::<TweedleDee>::rand(MAX_LENGTH, rng), GroupVec::<TweedleDee>::rand(MAX_LENGTH, rng))
+        };


Wouldn't it be more approriate to sample group vecs of different lengths?

UlrichHaboeck75 · 2022-04-19T12:17:51Z

algebra/src/linear_combination.rs

+    }
+
+    /// Initialize a random LC from a single batching scalar and a vector of  points and checks that the result of combine is indeed
+    /// given by (0 * point_0) + (scalar * point_1) + (scalar^2 * point_2) + ... + (scalar^n-1 * point_n-1) 


given by (1 * point_0) + ...

Thanks, done

UlrichHaboeck75 · 2022-04-19T12:23:48Z

algebra/src/linear_combination.rs

+        let scalars = (0..MAX_LENGTH)
+            .map(|_| Fr::rand(rng))
+            .collect::<Vec<_>>();
+        let points = (0..MAX_LENGTH)
+            .map(|_| TweedleDee::rand(rng))
+            .collect::<Vec<_>>();


Why not choosing a different parameter for the number of samples? As I understand MAX_LENGTH is the length of a group vec, and typically a quite small number.

It is set to 10 currently, it's good enough I think

UlrichHaboeck75 · 2022-04-19T12:43:40Z

algebra/src/msm/variable_base.rs

+    /// Given base points `G[i]` together with scalars `c[i]`, `i=1,..n`, we write 
+    ///    c[i] = sum  c[i,j] * D^j,
+    ///    s[j] = Sum_i c[i,j] * G[i],
+    /// for each order `j`. 
+    /// The final result of the multi-scalar multiplication is then combined by 
+    ///    s = Sum_j  A^j * s[j], 
+    /// using double-and-add.


To me, this shortened explanation hides that the bucket method is applied to the digits from D.
How about

/// Given base points `G[i]` together with scalars `c[i]`, `i=1,..n`, we write /// c[i] = sum c[i,j] * D^j, /// with digits `c[i,j]` from the interval `{0,...,D-1}`, and compute /// s[j] = Sum_i c[i,j] * G[i], /// for each order `j` using the Pippenger-Yao bucket method.

Fine thanks !

DanieleDiBenedetto and others added 30 commits September 3, 2021 16:53

Refined test circuit for Final Darlin

5924fde

Added new benches

623740d

Added proof and vk size prints + other logs

d3225fa

Added new bench

17b2fad

added shell script

2296540

added todo for FinalDarlin test circuit

7d0c102

Adjusted to the refactored PC

1e93d2f

Fix marlin branch name

b911473

Degree bound removed from tests

d56bd60

Upated to the poly-commit refactoring phase 2

6e33192

Additional ops for polynomials of PC refactoring

f130a13

Added enforce comparison gadget imported from arkworks with Unit Test

5a71322

Merged with development

adba37d

Merge remote-tracking branch 'origin/development' into rc/feat/comp_g…

648eaa5

…adget

Enabled local path dependencie to algebra and r1cs-core

47a3d5a

Added CondSelect Gadget for UInt64

3c522fe

Added conditionally_add in FpGadget and UInt64

e10c4a5

Adjusted to pc refactored optimizations

06e8778

Darlin benchmark restored

252db67

print-trace for poly-commit removed

ba66f12

Cleaning unused curves

d927216

Reformat code

a558c29

Reformat code (2)

fbce641

Merged with development

61fea62

enforce_in_field rewritten using new enforce_smaller_or_equal_than_le

1bfed13

Algebra refactored

ca0bd35

Group trait for polynomial

d99c83a

Unused fields models removed and field became a group

12a32ab

Fixed field binding

0aa9131

Vector of group items

8f1858c

Restored doc in the curve module

e2d3a3f

DanieleDiBenedetto requested changes Mar 14, 2022

View reviewed changes

primitives/src/crh/bowe_hopwood/mod.rs Outdated Show resolved Hide resolved

DanieleDiBenedetto added 2 commits March 14, 2022 19:04

Other fixes + removed pedersen hash and commitment

1129b55

Refactored exponentiation functions in GroupGadget trait

98f78e1

DanieleDiBenedetto mentioned this pull request Mar 15, 2022

Misc optimizations for group gadgets #125

Open

DanieleDiBenedetto added 9 commits March 15, 2022 12:56

Removed any non poseidon-based merkle tree code

df0d5e2

Removed non field based Schnorr signature

79491f7

Merge branch 'development' into refactor_ginger_dev

f3b4c9d

Fixed curve test

b254ee9

Fixed proof-systems crate and aligned it with the most recent changes…

d8f93c0

… to Marlin and PolyCommit

Fix CI

dc062b0

Purged existing scripts from removed curves

f2371b6

Specialized random transformation curve test for TE curves

cad81d0

Removed unneeded lifetime parameters

de59738

UlrichHaboeck75 suggested changes Apr 1, 2022

View reviewed changes

lgiussan reviewed Apr 1, 2022

View reviewed changes

UlrichHaboeck75 reviewed Apr 4, 2022

View reviewed changes

DDT92 and others added 3 commits April 4, 2022 16:49

Addressed some comments by Ulrich.

083f383

Comments from code review + added tests for GroupVec and LinearCombin…

1185770

…ation

Minor fixes + re-introduced from_random_bytes function and renamed it…

d654def

… into force_deserialize

UlrichHaboeck75 reviewed Apr 19, 2022

View reviewed changes

UlrichHaboeck75 approved these changes Apr 19, 2022

View reviewed changes

DanieleDiBenedetto added 6 commits April 19, 2022 18:21

Commented broken test + addressed minor comments

7c84be6

Fix CI

dcc62b6

Added a small TODO related to r1cs_core::LinearCombination

3554b50

Merge branch 'rc/feat/comp_gadget' into refactor_ginger_dev

db24959

Fix automerge error

c283cf8

Removed short_weierstrass_projective

741a7b4

DanieleDiBenedetto closed this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Ginger-Lib #144

Refactor Ginger-Lib #144

phoinic commented Dec 9, 2021

UlrichHaboeck75 left a comment

UlrichHaboeck75 Mar 21, 2022

UlrichHaboeck75 Apr 1, 2022

DanieleDiBenedetto Apr 1, 2022

UlrichHaboeck75 Apr 1, 2022

UlrichHaboeck75 Apr 1, 2022

DanieleDiBenedetto Apr 1, 2022

lgiussan Apr 1, 2022

DanieleDiBenedetto Apr 1, 2022

DanieleDiBenedetto Apr 4, 2022

lgiussan Apr 1, 2022

DanieleDiBenedetto Apr 4, 2022

UlrichHaboeck75 Apr 4, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

UlrichHaboeck75 Apr 19, 2022

DanieleDiBenedetto Apr 19, 2022

Refactor Ginger-Lib #144

Refactor Ginger-Lib #144

Conversation

phoinic commented Dec 9, 2021

UlrichHaboeck75 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

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

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

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