Improve `ec` traits. #294

Pratyush · 2021-06-05T06:42:11Z

Description

closes: #41.

The aim is to also address #27 and #268.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (master)
Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
Wrote unit tests
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

Pratyush · 2021-06-05T06:43:37Z

ff/src/fields/mod.rs

+/// Fields that have a cyclotomic subgroup, and which can leverage efficient inversion
+/// and squaring operations for elements in this subgroup.
+pub trait CyclotomicField: Field {


New field that should be implemented for fields that have more efficient operations for their cyclotomic subgroup, such as faster inverses or squaring.

ff/src/fields/mod.rs

Pratyush · 2021-06-05T06:44:41Z

ff/src/fields/models/fp12_2over3over2.rs

@@ -50,88 +50,19 @@ impl<P: Fp12Parameters> QuadExtParameters for Fp12ParamsWrapper<P> {
    fn mul_base_field_by_frob_coeff(fe: &mut Self::BaseField, power: usize) {
        fe.mul_assign_by_fp2(Self::FROBENIUS_COEFF_C1[power % Self::DEGREE_OVER_BASE_PRIME_FIELD]);
    }
-
-    fn cyclotomic_exp(fe: &Fp12<P>, exponent: impl AsRef<[u64]>) -> Fp12<P> {


Moved to the CyclotomicField trait.

Pratyush · 2021-06-05T06:45:13Z

ff/src/fields/models/fp12_2over3over2.rs

    }
 }

-// TODO: make `const fn` in 1.46.
-pub fn characteristic_square_mod_6_is_one(characteristic: &[u64]) -> bool {
+pub const fn characteristic_square_mod_6_is_one(characteristic: &[u64]) -> bool {


This can now be a const fn.

ff/src/fields/models/fp2.rs

Pratyush · 2021-06-05T06:45:35Z

ff/src/fields/models/fp4.rs

@@ -49,6 +50,15 @@ impl<P: Fp4Parameters> QuadExtParameters for Fp4ParamsWrapper<P> {

 pub type Fp4<P> = QuadExtField<Fp4ParamsWrapper<P>>;

+impl<P: Fp4Parameters> CyclotomicField for Fp4<P> {
+    fn cyclotomic_inverse_in_place(&mut self) -> Option<&mut Self> {


Same for Fp4.

Pratyush · 2021-06-05T06:46:04Z

ff/src/fields/models/fp6_2over3.rs

@@ -23,6 +23,15 @@ pub trait Fp6Parameters: 'static + Send + Sync {
    }
 }

+impl<P: Fp6Parameters> CyclotomicField for Fp6<P> {
+    fn cyclotomic_inverse_in_place(&mut self) -> Option<&mut Self> {


I think technically we can also have cyclotomic squaring ala Fp12, but I didn't want to implement that in this PR.

Pratyush · 2021-06-05T06:46:35Z

ff/src/fields/models/quadratic_extension.rs

-
-    /// A specializable method for exponentiating that is to be used
-    /// *only* when `fe` is known to be in the cyclotommic subgroup.
-    fn cyclotomic_exp(fe: &QuadExtField<Self>, exponent: impl AsRef<[u64]>) -> QuadExtField<Self> {


There's no need for this to be in the QuadExtParameters trait anymore, which simplifies things slightly.

Pratyush · 2021-06-05T06:46:50Z

ff/src/fields/models/quadratic_extension.rs

-    /// This is only to be used when the element is *known* to be in the cyclotomic subgroup.
-    pub fn cyclotomic_exp(&self, exponent: impl AsRef<[u64]>) -> Self {
-        P::cyclotomic_exp(self, exponent)
-    }


Moved to the CyclotomicField trait.

ff/src/fields/mod.rs

Pratyush · 2021-06-05T06:51:56Z

ff/src/biginteger/arithmetic.rs

@@ -52,7 +52,7 @@ pub(crate) fn mac_discard(a: u64, b: u64, c: u64, carry: &mut u64) {
    *carry = (tmp >> 64) as u64;
 }

-pub fn find_wnaf(num: &[u64]) -> Vec<i64> {
+pub fn find_wnaf(num: &[u64]) -> Vec<i8> {


Drive-by improvement to memory consumption of find_wnaf.

Pratyush · 2021-06-05T06:55:00Z

ec/src/models/mod.rs

+// pub mod bls12;
+// pub mod bn;
+// pub mod bw6;
+// pub mod mnt4;
+// pub mod mnt6;


TODO: revert this once things compile.

Pratyush · 2021-06-05T06:55:32Z

ec/src/models/short_weierstrass_jacobian.rs

 };

 use ark_ff::{
-    bytes::{FromBytes, ToBytes},


We no longer implement FromBytes and ToBytes, as part of a move towards #5.

Pratyush · 2021-06-05T06:56:19Z

ec/src/models/short_weierstrass_jacobian.rs

-        self.into_projective() == *other
+impl<P: Parameters> PartialEq<Projective<P>> for Affine<P> {
+    fn eq(&self, other: &Projective<P>) -> bool {
+        Projective::from(*self) == *other


TODO: specialize this to take advantage of the fact that self.z == 1.

Pratyush · 2021-06-05T06:58:45Z

ec/src/models/short_weierstrass_jacobian.rs

    #[inline]
    fn default() -> Self {
        Self::zero()
    }
 }

-impl<P: Parameters> GroupProjective<P> {
+impl<P: Parameters> Projective<P> {
+    /// Construct a [`Self`] *without* validating that the coordinates form a valid point.


TODO: address #27 and #268

Pratyush · 2021-06-05T06:59:50Z

ec/src/msm/mod.rs

-/// [`Explanation of usage`]
-///
-/// [`Explanation of usage`]: https://github.com/scipr-lab/zexe/issues/79#issue-556220473
+/// The result of this function is only approximately `ln(a)`. See [here](https://github.com/arkworks-rs/snark/79#issue-556220473) for details.


drive-by fix

Pratyush · 2021-06-05T07:00:09Z

ec/src/pairing.rs

+use ark_std::rand::{Rng, distributions::{Distribution, Standard}};
+use crate::*;
+
+pub trait Pairing: Sized + 'static + Copy + Debug + Sync + Send + Eq {


Revampled Pairing trait.

Pratyush · 2021-06-05T07:00:38Z

ec/src/pairing.rs

+    /// An element of G1.
+    type G1: CurveGroup<ScalarField = Self::ScalarField>
+        + MulAssign<Self::ScalarField>; // needed due to https://github.com/rust-lang/rust/issues/69640


Note that there is no longer a separate G1Affine and G1Projective.

Pratyush · 2021-06-05T07:01:22Z

ec/src/pairing.rs

+        // Sample a random G1 element
+        let g1 = P::G1::rand(rng);
+        // Sample a random G2 element
+        let g2 = P::G2::rand(rng);
+        P::pairing(g1, g2)


Sampling a random GT element requires sampling random g1 and g2 elements, and then pairing them.

I wonder if this impacts anything..

Does it not suffice to sample a random GT element, and then take the final exponentiation on it?

That's a good idea, it should work. That can also provide us with a generator, right?

It'd only provide generators independent of pairing outputs, which maybe useful sometimes, but most protocols need a generator e(G_1,G_2) if they need a G_t generator at all.

Pratyush · 2021-06-05T07:02:24Z

ec/src/pairing.rs

+        // Sample a random G1 element
+        let g1 = P::G1::generator();
+        // Sample a random G2 element
+        let g2 = P::G2::generator();
+        P::pairing(g1.into(), g2.into())


To obtain the GT generator, we just pair the G1 and G2 generators. Since this value is fixed, we can technically hardcode it, but that requires work, and is also unlikely to really be used anywhere.

https://indiv0.github.io/lazycell/lazycell/
https://doc.rust-lang.org/std/lazy/index.html

ooh I wonder if we should use that for other constants too now...

Atomic laziness incurs a performance hit of like 2ns I hear, but afaik most protocols access their generator only rarely, so whatever. It's more likely recomputation once per thread or once per exec causes pain in WASM or unix CLI binaries, so then the fastest would be pre-computed or if const generics work then

const GENERATOR: Self::UniqueRepr = { let g1 = P::G1::generator(); let g2 = P::G2::generator(); P::pairing(g1.into(), g2.into()) };

I'd expect const generics to be better by the time anyone cases, so maybe lazycell now with a performance TODO for the future.

As an aside, we've layers of tables that depend upon tables in https://github.com/w3f/rs-ec-perf/ so I used https://gitlab.com/okannen/static_init to avoid the 2ns performance hit, but it brings portability issues so likely unsuitable here. As a further aside, rand had a long discussion around mutable thread locals in rust-random/rand#968

Pratyush · 2021-06-05T07:03:19Z

ec/src/lib.rs


 pub mod msm;

 pub mod wnaf;

-pub trait PairingEngine: Sized + 'static + Copy + Debug + Sync + Send + Eq + PartialEq {


The PairingEngine trait is renamed to Pairing and the implementation now lives in the pairing module.

ValarDragon · 2021-06-05T20:21:12Z

Cyclotomic field work LGTM, I suggest we move that to a standalone PR to avoid a massive PR at the end of the refactor that will be super hard to review

Co-authored-by: Dev Ojha <[email protected]>

ValarDragon · 2021-07-13T14:59:02Z

ec/README.md

-* [*Short Weierstrass*](https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/short_weierstrass_jacobian.rs) curves. The `AffineCurve` in this case is in typical Short Weierstrass point representation, and the `ProjectiveCurve` is using points in [Jacobian Coordinates](https://en.wikibooks.org/wiki/Cryptography/Prime_Curve/Jacobian_Coordinates).
-* [*Twisted Edwards*](https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/twisted_edwards_extended.rs) curves. The `AffineCurve` in this case is in standard Twisted Edwards curve representation, whereas the `ProjectiveCurve` uses points in [Extended Twisted Edwards Coordinates](https://eprint.iacr.org/2008/522.pdf).
+* [*Short Weierstrass*](https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/short_weierstrass.rs) curves. The underlying canonical representation uses standard short Weierstrass coordinates, while arithmetic relies on formulae over [Jacobian Coordinates](https://en.wikibooks.org/wiki/Cryptography/Prime_Curve/Jacobian_Coordinates).
+* [*Twisted Edwards*](https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/twisted_edwards.rs) curves. The underlying canonical representation uses standard twisted Edwards coordinates, while arithmetic relies on formulae over [Extended Twisted Edwards Coordinates](https://eprint.iacr.org/2008/522.pdf).


Suggested change

* [*Twisted Edwards*](https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/twisted_edwards.rs) curves. The underlying canonical representation uses standard twisted Edwards coordinates, while arithmetic relies on formulae over [Extended Twisted Edwards Coordinates](https://eprint.iacr.org/2008/522.pdf).

* [*Twisted Edwards*](https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/twisted_edwards.rs) curves. The underlying canonical representation uses standard Twisted Edwards coordinates, while arithmetic relies on formulae over [Extended Twisted Edwards Coordinates](https://eprint.iacr.org/2008/522.pdf).

bhgomes · 2022-04-13T18:58:03Z

ec/src/pairing.rs

+    /// Perform a miller loop with some number of (G1, G2) pairs.
+    #[must_use]
+    fn miller_loop<'a>(
+        i: impl IntoIterator<Item = &'a (Self::G1Prepared, Self::G2Prepared)>,
+    ) -> MillerLoopOutput<Self>;
+
+    /// Perform final exponentiation of the result of a miller loop.
+    #[must_use]
+    fn final_exponentiation(mlo: MillerLoopOutput<Self>) -> Option<PairingOutput<Self>>;
+
+    /// Computes a product of pairings.
+    #[must_use]
+    fn product_of_pairings<'a>(
+        i: impl IntoIterator<Item = &'a (Self::G1Prepared, Self::G2Prepared)>,
+    ) -> PairingOutput<Self> {
+        Self::final_exponentiation(Self::miller_loop(i)).unwrap()
+    }


Can we please use a pair of references instead of a reference to a pair?

impl IntoIterator<Item = (&'a Self::G1Prepared, &'a Self::G2Prepared)>

It's almost impossible to compose this API with others that involve calling the pairing algorithm more than once or using custom data structures.

Yes, that's a good suggestion.

Add CyclotomicField to represent cyclotomic operations

72de274

Pratyush changed the title ~~Add CyclotomicField to represent cyclotomic operations~~ Improve ec traits. Jun 5, 2021