Handle zero-case in group scalar multiplication (#124)

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

Author: slumber

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Pending
 - [\#117](https://github.com/arkworks-rs/r1cs-std/pull/117) Fix result of `precomputed_base_scalar_mul_le` to not discard previous value.
+- [\#124](https://github.com/arkworks-rs/r1cs-std/pull/124) Fix `scalar_mul_le` constraints unsatisfiability when short Weierstrass point is zero.
 
 ### Breaking changes
 

Cargo.toml

@@ -36,6 +36,7 @@ ark-mnt4-753 = { version = "0.4.0", features = ["curve"], default-features = fal
 ark-mnt6-298 = { version = "0.4.0", default-features = false  }
 ark-mnt6-753 = { version = "0.4.0", default-features = false  }
 ark-pallas = { version = "0.4.0", features = ["curve"],  default-features = false  }
+ark-bn254 = { version = "0.4.0", features = ["curve"], default-features = false  }
 
 [features]
 default = ["std"]
@@ -85,4 +86,4 @@ ark-mnt4-298 = { git = "https://github.com/arkworks-rs/curves/" }
 ark-mnt4-753 = { git = "https://github.com/arkworks-rs/curves/" }
 ark-mnt6-298 = { git = "https://github.com/arkworks-rs/curves/" }
 ark-mnt6-753 = { git = "https://github.com/arkworks-rs/curves/" }
-ark-pallas = { git = "https://github.com/arkworks-rs/curves/" }
+ark-pallas = { git = "https://github.com/arkworks-rs/curves/" }

src/groups/curves/short_weierstrass/mod.rs

@@ -512,8 +512,18 @@ where
         }
         let self_affine = self.to_affine()?;
         let (x, y, infinity) = (self_affine.x, self_affine.y, self_affine.infinity);
-        // We first handle the non-zero case, and then later
-        // will conditionally select zero if `self` was zero.
+        // We first handle the non-zero case, and then later will conditionally select
+        // zero if `self` was zero. However, we also want to make sure that generated
+        // constraints are satisfiable in both cases.
+        //
+        // In particular, using non-sensible values for `x` and `y` in zero-case may cause
+        // `unchecked` operations to generate constraints that can never be satisfied, depending
+        // on the curve equation coefficients.
+        //
+        // The safest approach is to use coordinates of some point from the curve, thus not
+        // violating assumptions of `NonZeroAffine`. For instance, generator point.
+        let x = infinity.select(&F::constant(P::GENERATOR.x), &x)?;
+        let y = infinity.select(&F::constant(P::GENERATOR.y), &y)?;
         let non_zero_self = NonZeroAffineVar::new(x, y);
 
         let mut bits = bits.collect::<Vec<_>>();
@@ -968,3 +978,61 @@ where
         Ok(bytes)
     }
 }
+
+#[cfg(test)]
+mod test_sw_curve {
+    use crate::{
+        alloc::AllocVar,
+        eq::EqGadget,
+        fields::{fp::FpVar, nonnative::NonNativeFieldVar},
+        groups::{curves::short_weierstrass::ProjectiveVar, CurveVar},
+        ToBitsGadget,
+    };
+    use ark_ec::{
+        short_weierstrass::{Projective, SWCurveConfig},
+        CurveGroup,
+    };
+    use ark_ff::PrimeField;
+    use ark_relations::r1cs::{ConstraintSystem, Result};
+    use ark_std::UniformRand;
+    use num_traits::Zero;
+
+    fn zero_point_scalar_mul_satisfied<G>() -> Result<bool>
+    where
+        G: CurveGroup,
+        G::BaseField: PrimeField,
+        G::Config: SWCurveConfig,
+    {
+        let mut rng = ark_std::test_rng();
+
+        let cs = ConstraintSystem::new_ref();
+        let point_in = Projective::<G::Config>::zero();
+        let point_out = Projective::<G::Config>::zero();
+        let scalar = G::ScalarField::rand(&mut rng);
+
+        let point_in =
+            ProjectiveVar::<G::Config, FpVar<G::BaseField>>::new_witness(cs.clone(), || {
+                Ok(point_in)
+            })?;
+        let point_out =
+            ProjectiveVar::<G::Config, FpVar<G::BaseField>>::new_input(cs.clone(), || {
+                Ok(point_out)
+            })?;
+        let scalar = NonNativeFieldVar::new_input(cs.clone(), || Ok(scalar))?;
+
+        let mul = point_in.scalar_mul_le(scalar.to_bits_le().unwrap().iter())?;
+
+        point_out.enforce_equal(&mul)?;
+
+        cs.is_satisfied()
+    }
+
+    #[test]
+    fn test_zero_point_scalar_mul() {
+        assert!(zero_point_scalar_mul_satisfied::<ark_bls12_381::G1Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_pallas::Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_mnt4_298::G1Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_mnt6_298::G1Projective>().unwrap());
+        assert!(zero_point_scalar_mul_satisfied::<ark_bn254::G1Projective>().unwrap());
+    }
+}

src/groups/curves/short_weierstrass/non_zero_affine.rs

@@ -1,5 +1,5 @@
 use super::*;
-use ark_ec::AdditiveGroup;
+use ark_ff::AdditiveGroup;
 use ark_std::ops::Add;
 
 /// An affine representation of a prime order curve point that is guaranteed

src/pairing/mod.rs

@@ -1,6 +1,5 @@
 use crate::prelude::*;
-use ark_ec::pairing::Pairing;
-use ark_ec::CurveGroup;
+use ark_ec::{pairing::Pairing, CurveGroup};
 use ark_ff::Field;
 use ark_relations::r1cs::SynthesisError;
 use core::fmt::Debug;