Update arkworks to 0.4.2 (master) #2588

volhovm · 2024-09-20T14:51:53Z

Main part of update (towards develop = mina's compatible) has been done here: MinaProtocol/mina#15940

This upgrades master too. It's non-trivial since master has a lot of stuff develop doesn't.

How to review:

Probably by commit/folder.
95% is just renamings according to https://github.com/arkworks-rs/algebra/blob/master/CHANGELOG.md

…ible) Update arkworks to 0.4.2 for pallas and vesta Update test cases to new version of test suite 0.4.2 Upgrade utils to arkworks 0.4.2 Upgrade poseidon to arkworks 0.4.2 Upgrade export test vectors of poseidon to arkworks 0.4.2 Upgrade groupmap to arkworks 0.4.2 Upgrade hasher to arkworks 0.4.2 Upgrade signer to arkworks 0.4.2 Upgrade turshi to arkworks 0.4.2 Convert poly-comm to arkworks 0.4.2 Upgrade arkworks for `kimchi` and other libraries/tools Fixup compilation errors in OCaml conversion helpers Adjust serde_as regression test to 0.4.2 Use compressed serialization Fix erroneous implicit Affine->Proj conversions Fix from_address bug Fix ocaml printing: use hex instead of integer

codecov · 2024-09-21T15:26:12Z

Codecov Report

Attention: Patch coverage is 73.82716% with 106 lines in your changes missing coverage. Please review.

Project coverage is 71.21%. Comparing base (26fa105) to head (47bcd2d).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
poly-commitment/src/combine.rs	13.11%	53 Missing ⚠️
curves/src/pasta/fields/mod.rs	0.00%	10 Missing ⚠️
poly-commitment/src/commitment.rs	64.28%	10 Missing ⚠️
kimchi/src/curve.rs	63.15%	7 Missing ⚠️
curves/src/pasta/curves/pallas.rs	0.00%	4 Missing ⚠️
curves/src/pasta/curves/vesta.rs	0.00%	4 Missing ⚠️
kimchi/src/snarky/folding.rs	0.00%	4 Missing ⚠️
arrabiata/src/prover.rs	0.00%	2 Missing ⚠️
folding/src/expressions.rs	0.00%	2 Missing ⚠️
kimchi/src/circuits/expr.rs	0.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2588      +/-   ##
==========================================
+ Coverage   70.89%   71.21%   +0.31%     
==========================================
  Files         241      243       +2     
  Lines       57720    57627      -93     
==========================================
+ Hits        40923    41037     +114     
+ Misses      16797    16590     -207

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kimchi/src/tests/framework.rs

o1vm/src/legacy/tests.rs

dannywillems · 2024-09-23T10:25:41Z

Regarding commit messages, I am against having something like:

Fix serialization? (what? why?)

Could you change to something more descriptive (by answering the questions, for instance, in the commit message)?

dannywillems · 2024-09-23T10:30:44Z

msm/src/fec/mod.rs

-        use mina_curves::pasta::{Fp, Pallas};
+        // NOTE: this uses Pallas-in-Pallas emulation.
+        use mina_curves::pasta::Pallas;
+        type Fp = <Pallas as AffineRepr>::ScalarField;


The scalar field of Pallas is Fq: https://github.com/o1-labs/proof-systems/blob/master/curves/src/pasta/curves/pallas.rs#L13

Renamed 👍 Also I now understand that it was a bug in the original code: it should've been Fq all along. It's just that the old version of arkworks allowed to scale the generator by pretty much whatever, this code compiles on 0.3.0 🤯 :

let gen = Pallas::prime_subgroup_generator(); let kp: Fp = UniformRand::rand(rng); let kp2: Fq = UniformRand::rand(rng); let p: Pallas = gen.mul(kp).into(); let p2: Pallas = gen.mul(kp2).into();

In particular, it's because mul is defined as follows.

fn mul<S: Into<<Self::ScalarField as PrimeField>::BigInt>>(&self, by: S) -> GroupProjective<P> {

dannywillems · 2024-09-23T10:35:36Z

msm/src/serialization/interpreter.rs

@@ -802,10 +802,9 @@ mod tests {
            lookups::LookupTable,
            N_INTERMEDIATE_LIMBS,
        },
-        Ff1, LIMB_BITSIZE, N_LIMBS,
+        Ff1, Fp, LIMB_BITSIZE, N_LIMBS,


Not that this is not the same field. Fp points to ark_bn254::Fp in msm/src/lib.rs. Is it expected to change the field?

OK indeed you're right. I have no idea why this module uses native = foreign in tests instead, but I returned it back/made it clear now.

quite confusing actually, I spent some time thinking about the same issue in the other file :)

dannywillems · 2024-09-23T10:37:00Z

o1vm/src/legacy/tests.rs

@@ -475,6 +479,8 @@ pub mod keccak {
    // (Step, Left, Right)
    type KeccakFoldingPair = (Steps, KeccakFoldingSide, KeccakFoldingSide);

+    type KeccakDefaultFqSponge = DefaultFqSponge<ark_bn254::g1::Config, PlonkSpongeConstantsKimchi>;


Just a note: seems an additional alias added. LGTM.

poly-commitment/src/commitment.rs

dannywillems · 2024-09-23T10:41:32Z

poly-commitment/src/kzg.rs

+        let res = Pair::final_exponentiation(Pair::multi_miller_loop(to_loop_left, to_loop_right))
+            .unwrap();
+
+        res.0 == Pair::TargetField::one()


Checking this.

You can use res.is_zero(), see

impl<P: Pairing> Zero for PairingOutput<P> { /// The identity element, or "zero", of the group is the identity element of the multiplicative group of the underlying field, i.e., `P::TargetField::one()`. fn zero() -> Self { Self(P::TargetField::one()) } fn is_zero(&self) -> bool { self.0.is_one() } }

It is better to rely on the interface provided by the library than the inner representation (c.f. affine vs projective coordinates where we can reference x and y in both structures).

Agreed, will simplify.

dannywillems · 2024-09-23T10:43:02Z

poly-commitment/src/kzg.rs

+
+        res.0 == Pair::TargetField::one()
+
+        // @VOLHOVM remove this if not necessary


Is it a leftover from your tests?

Will remove :) it's from a merge conflict that I wasn't sure of.

poseidon/export_test_vectors/Cargo.toml

poseidon/src/sponge.rs

poly-commitment/src/kzg.rs

poly-commitment/tests/kzg.rs

dannywillems · 2024-09-23T10:59:38Z

poseidon/export_test_vectors/src/vectors.rs

@@ -89,8 +90,8 @@ pub fn generate(mode: Mode, param_type: ParamType) -> TestVectors {
            .collect();
        let mut output_bytes = vec![];
        output
-            .into_repr()
-            .serialize(&mut output_bytes)
+            .0


I would try to rely on a method on output instead of getting the inner element of the structure.
Also, simply adding a single test checking the serialization is still the same would be nice - it is used for further testing, not production code, but always good.

I just checked the result of serialization of output vs output.0 -- both compile, but the serialization format is different.

I guess I could add a test, but I'd rather do it in develop too, not as part of this PR, which merely upgrades the functionality. I'll also add a regression test there.

utils/src/field_helpers.rs

utils/src/serialization.rs

dannywillems · 2024-09-23T11:13:25Z

utils/src/serialization.rs

@@ -100,7 +100,7 @@ where
        S: serde::Serializer,
    {
        let mut bytes = vec![];
-        val.serialize_unchecked(&mut bytes)
+        val.serialize_uncompressed(&mut bytes)


Are we sure it doesn't check? It would imply a performance regression otherwise. I would be happy to see some regression tests for this.

So the "unchecked" capability for serialization has been completely removed (and probably rightly so, the common and more intuitive approach is that an object that you can access within the code is consistent by default, so we check consistency on construction but not after).

Another thing is that serialize_unchecked was not even defined separately for EC points: https://github.com/arkworks-rs/algebra/blob/v0.3.0/ec/src/models/short_weierstrass_jacobian.rs#L781

The default implementation of serialize_unchecked was serialize_uncompressed and that's what the library defined. So there's no regression. Which is why they got rid of it :)

Finally, there can be no regression because the only place SerializeAsUnchecked is used in is for serializing the SRS, which is done only on generation once in never explicitly by hand.

utils/src/serialization.rs

dannywillems · 2024-09-23T11:13:58Z

utils/tests/field_helpers.rs

@@ -125,7 +128,7 @@ fn field_big() {
    let field_zero = BaseField::from(0u32);

    assert_eq!(
-        BigUint::from_bytes_be(&field_zero.into_repr().to_bytes_be()),
+        BigUint::from_bytes_be(&field_zero.0.to_bytes_be()),


Can we use a method on it instead of getting the field?

Not only we can but we should, since now that I'm looking at it (it was some code written before me) the serialization is not correct. It should be PrimeField::into_bigint() instead of .0, and these two calls give different bigints (surprisingly!). I'll add a fix here + hotfix on develop + regression test.

dannywillems · 2024-09-23T11:15:27Z

utils/src/field_helpers.rs

    }

    fn from_hex(hex: &str) -> Result<F> {
        let bytes: Vec<u8> = hex::decode(hex).map_err(|_| FieldHelpersError::DecodeHex)?;
-        F::deserialize(&mut &bytes[..]).map_err(|_| FieldHelpersError::DeserializeBytes)
+        F::deserialize_uncompressed(&mut &bytes[..])


As always, I would be happy to have some regression tests. One test could be checking that deserialization/serialization on Fp::MODULUS + 1 works/doesn't work on both version. Same for the serialization. And also have some PBT (serialization followed by deserialization gives the same value).

We can write some in develop? As I mentioned before, we do have some non-trivial regression which is enough to at least see if curve points are serialized properly.

dannywillems

See comment.

circuit-construction/Cargo.toml

groupmap/src/lib.rs

volhovm

Thanks for the review!

I'll fix the workspace issues and remove the unnecessary comment, will also use simpler functions (multi_pairing, is_zero)
I'll rebase -i before merging so that the ugly name comment gets squashed into the previous one. Won't do it for now since we'll lose github comments then :|
Will add a hotfix for export_test_vectors on develop and cherry-pick it here too.
- Arkworks 0.4.2: fix test vectors serialization issue #2594
All the other comments especially w.r.t. serialization and regression tests I suggest to address within a new PR on develop. They're reasonable, but arguably regression tests are not part of moving arkworks 0.4.2 to master in particular.

o1vm/src/legacy/tests.rs

poly-commitment/src/commitment.rs

poly-commitment/src/kzg.rs

volhovm · 2024-09-23T17:58:33Z

poly-commitment/src/kzg.rs

+        let res = Pair::final_exponentiation(Pair::multi_miller_loop(to_loop_left, to_loop_right))
+            .unwrap();
+
+        res.0 == Pair::TargetField::one()


Agreed, will simplify.

volhovm · 2024-09-23T17:59:08Z

poly-commitment/src/kzg.rs

+
+        res.0 == Pair::TargetField::one()
+
+        // @VOLHOVM remove this if not necessary


Will remove :) it's from a merge conflict that I wasn't sure of.

volhovm · 2024-09-23T19:58:57Z

utils/src/serialization.rs

@@ -100,7 +100,7 @@ where
        S: serde::Serializer,
    {
        let mut bytes = vec![];
-        val.serialize_unchecked(&mut bytes)
+        val.serialize_uncompressed(&mut bytes)


So the "unchecked" capability for serialization has been completely removed (and probably rightly so, the common and more intuitive approach is that an object that you can access within the code is consistent by default, so we check consistency on construction but not after).

Another thing is that serialize_unchecked was not even defined separately for EC points: https://github.com/arkworks-rs/algebra/blob/v0.3.0/ec/src/models/short_weierstrass_jacobian.rs#L781

The default implementation of serialize_unchecked was serialize_uncompressed and that's what the library defined. So there's no regression. Which is why they got rid of it :)

Finally, there can be no regression because the only place SerializeAsUnchecked is used in is for serializing the SRS, which is done only on generation once in never explicitly by hand.

utils/src/serialization.rs

volhovm · 2024-09-23T20:14:48Z

utils/tests/field_helpers.rs

@@ -125,7 +128,7 @@ fn field_big() {
    let field_zero = BaseField::from(0u32);

    assert_eq!(
-        BigUint::from_bytes_be(&field_zero.into_repr().to_bytes_be()),
+        BigUint::from_bytes_be(&field_zero.0.to_bytes_be()),


Not only we can but we should, since now that I'm looking at it (it was some code written before me) the serialization is not correct. It should be PrimeField::into_bigint() instead of .0, and these two calls give different bigints (surprisingly!). I'll add a fix here + hotfix on develop + regression test.

kimchi/src/tests/framework.rs

circuit-construction/Cargo.toml

…tors

dannywillems · 2024-09-25T12:44:30Z

arrabiata/tests/witness.rs

@@ -291,8 +292,9 @@ where
    let res_y: BigInt = env.state[1].clone();

    let p1_proj: ProjectivePallas = p1.into();
-    let p1_r: Pallas = p1_proj.mul(r.clone().to_u64_digits().1).into();
-    let exp_res: Pallas = p1_r + env.srs_e2.h;
+    // @volhovm TODO check if mul_bigint is what was intended


Yes it is fine. And it is a test.
I'll remove the comment in a follow-up

poly-commitment/src/commitment.rs

volhovm force-pushed the volhovm/arkworks042-develop branch 2 times, most recently from bca9769 to 1b051ee Compare September 20, 2024 18:49

chiro-hiro and others added 2 commits September 20, 2024 19:51

Update to arkworks 0.4.2: arrabiata,folding,ivc,msm,o1vm

4bb1e61

volhovm force-pushed the volhovm/arkworks042-develop branch from 1b051ee to 4bb1e61 Compare September 20, 2024 18:51

volhovm marked this pull request as ready for review September 20, 2024 18:55

volhovm marked this pull request as draft September 20, 2024 21:11

Fix serialization? (what? why?)

e0d92f4

volhovm force-pushed the volhovm/arkworks042-develop branch from fa2c7cf to e0d92f4 Compare September 21, 2024 14:05

volhovm marked this pull request as ready for review September 21, 2024 17:58

volhovm requested a review from dannywillems September 21, 2024 19:17

This was referenced Sep 21, 2024

[berkeley] Update curves to arkworks 0.4.2 #1670

Closed

[compatible] Update curves to arkworks 0.4.2 #1669

Closed

Update arkworks to 0.4.2 for all crates #1265

Closed

volhovm requested review from mrmr1993, querolita, fabrizio-m and shimkiv and removed request for mrmr1993 September 21, 2024 19:19