Miscellaneous Changes: Serialization, additional helper methods, reorg #235

jon-chuang · 2021-03-17T11:10:05Z

Description

Miscelaneous changes associated with celo-org merge.
Ranging from harmless to API changes.
closes: #127

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

jon-chuang · 2021-03-17T11:15:10Z

To reviewers:

Not sure if you'd like for me to refactor further. This is the least cohesive PRs of those to come since I've staged them to build atop one another, and this is the remaining changes that don't have any specific topic it is tied to. However, if you think it would be cleaner to extract the diff of one of the branches jonch/celo-org-... feel free to let me know or to do so yourself.

ec/src/models/short_weierstrass_jacobian.rs

ff/src/biginteger/mod.rs

Pratyush · 2021-03-17T11:37:04Z

ff/src/biginteger/mod.rs

+    /// Takes two slices of u64 representing big integers and returns a bigger
+    /// BigInteger of type Self representing their product. Preferably used
+    /// only for even NUM_LIMBS. We require the invariant that this.len() ==
+    /// other.len() == NUM_LIMBS / 2
+    fn mul_no_reduce(this: &[u64], other: &[u64]) -> Self;


Does this need to be a method on bigint? Can't it just be a free-standing function that is generic over B: BigInteger?

So something like fn mul_no_reduce<B: BigInteger>(this: &[u64], other: &[u64]) -> B?

Hmm definitely this is possible. But this way, one doesn't need to import additional methods.

The usage is let b = B::mul_no_reduce(this, other)

Is your concern that there would be too many methods?

Yeah, I think I want to reduce the API surface of our existing traits, because implementing them is kind of a pain right now.

How do you use this API in your later PRs? Maybe it's possible to come up with an API that fits slightly better into the rest of the methods, as the current design API works with slices and only for for even-limb BigInts.

This (impl pain) isn't really an issue for the BigInteger trait, however?

The question is whether one sees potential additional use elsewhere.

But, as the impact is small, I can move it to a separate method now, under an empty struct BigIntegerHelper

Pratyush · 2021-03-17T11:37:22Z

ff/src/biginteger/mod.rs

+    /// Similar to `mul_no_reduce` but accepts slices of len == NUM_LIMBS and
+    /// only returns lower half of the result
+    fn mul_no_reduce_lo(this: &[u64], other: &[u64]) -> Self;


Same for this

Maybe we can change the parameters here? fn mul_no_reduce_lo(&self, other: &Self) -> Self? Does your later code rely heavily on slices?

Another approach would be to use something like num-bigint for those places, because that explicitly supports these kinds of operations, and I imagine that the bigint ops won't dominate the GLV ops?

Pratyush · 2021-03-17T11:37:50Z

ff/src/biginteger/mod.rs

+    /// Copies data from a slice to Self in a len agnostic way,
+    // based on whichever of the two is shorter.
+    fn from_slice(slice: &[u64]) -> Self;


Same for this.

ff/src/fields/mod.rs

Pratyush · 2021-03-17T11:40:57Z

test-curves/Cargo.toml

@@ -9,10 +9,15 @@ edition = "2018"
 publish = false

 [dependencies]
+paste = "0.1"


I think this is not needed in this PR?

Pratyush · 2021-03-17T11:41:08Z

test-curves/Cargo.toml

+
+lazy_static = { version = "1.4.0", optional = true }
+accel = { git = "https://github.com/jon-chuang/accel", package = "accel", optional = true }
+


Same for this

Co-authored-by: Pratyush Mishra <[email protected]>

Pratyush · 2021-03-17T11:43:23Z

ec/Cargo.toml

+[dev-dependencies]
+rand_xorshift = "0.2"


We can use ark_std::rand::XorShiftRng directly, no need to import this.

Suggested change

[dev-dependencies]

rand_xorshift = "0.2"

yeah I'll have to change this...

Pratyush · 2021-03-17T11:44:40Z

ec/src/lib.rs

@@ -214,6 +214,8 @@ pub trait ProjectiveCurve:
        self = res;
        self
    }
+
+    fn get_x(&mut self) -> &mut Self::BaseField;


I don't think this is well defined for projective curve representations, as the same curve point can have different projective coordinates, which means that the X coordinate can differ.

(Also no code in this PR uses it, but I guess your later PRs will?)

Hmm yes, I can remove it for now, it will be used only for a generic script to generate parameters.

The purpose is to mutate x, in the use case for the purpose of applying an endomorphism, for which the particular Z coordinate does not matter.

I see, maybe it would be better to add xyz_endo_in_place/xyz_endo methods in the appropriate location (in the ModelParameters trait, perhaps?)

possibly, but the method already defined later on would apply multiplication by a constant endo param (P::glv_endomorphism_in_place(&mut p.x);).

This more generic method allows you to do whatever you want with x. I don't think this is an issue in itself if you know what you're doing... If you're worried about clutter, well, unfortunately I can't think of a workaround.

Further, as far as the curves we have are concerned, get_x and get_y for the ProjectiveCurve trait seem fairly reasonable...

But we can discuss it further in the PR for which it is required.

Ah I meant that instead one would do something like P::glv_endomorphism_in_place(&mut p);, which fully encapsulates the details of the endomorphism; the signature would be something like

trait SWModelParameters { /// Existing stuff fn glv_endomorphism_in_place(p: &mut GroupProjective<Self>) { // do whatever } }

This takes the point as a whole, instead of the coordinate.

But yeah, we can discuss it in the GLV PR

Just checking, the plan is to remove it from this PR right?

…rks-rs/algebra into jonch/celo-org-serial-misc

ec/Cargo.toml

Pratyush · 2021-03-17T12:50:20Z

For the serialize changes, we should update the derive macro to additionally incorporate uncompressed_unchecked.

Also, maybe in a follow-up PR, let's make it so that the Serialize and Deserialize traits both have a (de)serialize_with_mode method:

trait CanonicalSerialize {
...
	fn serialize_with_mode<W: Write>(&self, compression: ShouldCompress, writer: W) {
		match compression {
			ShouldCompress::Yes => {
				// do stuff
			},
			ShouldCompress::No => {
				// do other stuff
			}
		}
	}

	// all other methods invoke `serialize_with_mode` with appropriate modes.
}

trait CanonicalDeserialize {
	fn deserialize_with_mode<R: Read>(&self, compression: ShouldCompress, check: ShouldCheck reader: R) -> Result<Self> {
		match compression {
			ShouldCompress::Yes => {
				// do stuff
			},
			ShouldCompress::No => {
				// do other stuff
			}
		}
		if let ShouldCheck::Yes = check {
			// perform checks
		}
		Ok(result)
	}
	
	// all other methods invoke `deserialize_with_mode` with appropriate modes.
}

(I omitted the check from serialize, because AFAIK there are no checks performed during serialization? This means that we remove serialize_unchecked and serialize_uncompressed_unchecked too.)

The reason why we want to add the above API is so that downstream implementers can just implement the logic in one method, instead of in four different ones.

Pratyush · 2021-03-17T12:59:33Z

ff/src/fields/mod.rs

+#[macro_export]
+macro_rules! field_new_from_raw_repr {
+    ($name:ident, $c0:expr) => {
+        $name {
+            0: $c0,
+            1: core::marker::PhantomData,
+        }
+    };
+    ($name:ident, $c0:expr, $c1:expr $(,)?) => {
+        $name {
+            c0: $c0,
+            c1: $c1,
+            _parameters: core::marker::PhantomData,
+        }
+    };
+    ($name:ident, $c0:expr, $c1:expr, $c2:expr $(,)?) => {
+        $name {
+            c0: $c0,
+            c1: $c1,
+            c2: $c2,
+            _parameters: core::marker::PhantomData,
+        }
+    };
+}


how is it different from the existing field_new?

Ah I see, you don't convert to montgomery form here. Is there a reason the original constants can't be in normal (not Montgomery) form?

Pratyush · 2021-03-17T13:01:13Z

ff/src/fields/mod.rs

@@ -418,6 +443,11 @@ pub trait PrimeField:
        Self::Params::T_MINUS_ONE_DIV_TWO
    }

+    /// Returns the trace minus one divided by two.


Suggested change

/// Returns the trace minus one divided by two.

/// Returns the prime modulus.

Pratyush · 2021-03-22T05:50:15Z

@jon-chuang is there anything I can do to help push this forward?

ValarDragon · 2021-03-23T20:39:22Z

ec/src/models/mod.rs

@@ -8,53 +8,15 @@ pub mod mnt6;
 pub mod short_weierstrass_jacobian;
 pub mod twisted_edwards_extended;

+pub use {


Glad this code was moved to the relevant locations!

ValarDragon · 2021-03-23T20:40:49Z

ec/src/models/mod.rs

-    type TEModelParameters: TEModelParameters<BaseField = Self::BaseField>;
+    type ScalarField: PrimeField
+        + SquareRootField
+        + From<<Self::ScalarField as PrimeField>::BigInt>


Do we need this From? Its a breaking change, so if so it will need to be noted in changelog

ValarDragon · 2021-03-23T20:44:05Z

serialize/Cargo.toml

@@ -15,6 +15,7 @@ edition = "2018"
 [dependencies]
 ark-serialize-derive = { path = "../serialize-derive", optional = true }
 ark-std = { git = "https://github.com/arkworks-rs/utils", default-features = false }
+paste = "0.1"


Is this needed?

Pratyush

We've implemented some of the serialization changes in #463. For the GLV related changes, we should probably flesh out a design before merging.

jon-chuang added 12 commits December 21, 2020 14:45

Merge celo/zexe w/ arkworks

54ccc1f

Merge branch 'master' into jonch/celo-merge

1689686

hotfix

4328859

remove cuda

96a6d75

remove downstream batched

b3ffa79

remove glv infra

44edd01

remove batch ops

a956c20

fmt

5219495

remove bw6 asm

3945a5c

remove remnants

308452e

remove remnants

bad1cf2

remove remnants

a478be0

jon-chuang requested a review from Pratyush March 17, 2021 11:13

fix serialization tests (compressed default)

f980cdf