From bb663bc27e776af81e66c94d9016bde2dbba9968 Mon Sep 17 00:00:00 2001
From: Weikeng Chen <w.k@berkeley.edu>
Date: Mon, 25 Mar 2024 04:11:16 -0700
Subject: [PATCH] Fix incorrect decomposition in GLV (#803)

* update the implementation

* update the changelog

* fmt
---
 CHANGELOG.md              |  1 +
 ec/Cargo.toml             |  1 +
 ec/src/scalar_mul/glv.rs  | 20 +++++++++++++++++---
 test-templates/src/glv.rs | 20 +++++++++++++++-----
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 351953b47..8ac55bcd9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -40,6 +40,7 @@
 ### Bugfixes
 
 - [\#747](https://github.com/arkworks-rs/algebra/pull/747) (`ark-ff-macros`) Fix fetching attributes in `MontConfig` macro
+- [\#803](https://github.com/arkworks-rs/algebra/pull/803) (`ark-ec`, `ark-test-template`) Fix incorrect decomposition in GLV
 
 ## v0.4.2
 
diff --git a/ec/Cargo.toml b/ec/Cargo.toml
index 3380fb4d8..67568d3b2 100644
--- a/ec/Cargo.toml
+++ b/ec/Cargo.toml
@@ -23,6 +23,7 @@ ark-poly.workspace = true
 derivative = { workspace = true, features = ["use_core"] }
 num-bigint.workspace = true
 num-traits.workspace = true
+num-integer.workspace = true
 rayon = { workspace = true, optional = true }
 zeroize = { workspace = true, features = ["zeroize_derive"] }
 hashbrown.workspace = true
diff --git a/ec/src/scalar_mul/glv.rs b/ec/src/scalar_mul/glv.rs
index 4bc979641..b3acfb1a8 100644
--- a/ec/src/scalar_mul/glv.rs
+++ b/ec/src/scalar_mul/glv.rs
@@ -3,8 +3,10 @@ use crate::{
     AdditiveGroup, CurveGroup,
 };
 use ark_ff::{PrimeField, Zero};
+use ark_std::ops::{AddAssign, Neg};
 use num_bigint::{BigInt, BigUint, Sign};
-use num_traits::Signed;
+use num_integer::Integer;
+use num_traits::{One, Signed};
 
 /// The GLV parameters for computing the endomorphism and scalar decomposition.
 pub trait GLVConfig: Send + Sync + 'static + SWCurveConfig {
@@ -39,8 +41,20 @@ pub trait GLVConfig: Send + Sync + 'static + SWCurveConfig {
         // The inverse of N is 1/r * Matrix([[n22, -n12], [-n21, n11]]).
         // so β = (k*n22, -k*n12)/r
 
-        let beta_1 = &scalar * &n22 / &r;
-        let beta_2 = &scalar * &n12 / &r;
+        let beta_1 = {
+            let (mut div, rem) = (&scalar * &n22).div_rem(&r);
+            if (&rem + &rem) > r {
+                div.add_assign(BigInt::one());
+            }
+            div
+        };
+        let beta_2 = {
+            let (mut div, rem) = (&scalar * &n12.clone().neg()).div_rem(&r);
+            if (&rem + &rem) > r {
+                div.add_assign(BigInt::one());
+            }
+            div
+        };
 
         // b = vector([int(beta[0]), int(beta[1])]) * self.curve.N
         // b = (β1N11 + β2N21, β1N12 + β2N22) with the signs!
diff --git a/test-templates/src/glv.rs b/test-templates/src/glv.rs
index 866594faf..05beb0a09 100644
--- a/test-templates/src/glv.rs
+++ b/test-templates/src/glv.rs
@@ -1,12 +1,10 @@
-use std::ops::Mul;
-
 use ark_ec::{
     scalar_mul::{glv::GLVConfig, sw_double_and_add_affine, sw_double_and_add_projective},
     short_weierstrass::{Affine, Projective},
     AffineRepr, CurveGroup, PrimeGroup,
 };
-use ark_ff::PrimeField;
-use ark_std::UniformRand;
+use ark_ff::{BigInteger, PrimeField};
+use ark_std::{ops::Mul, UniformRand};
 
 pub fn glv_scalar_decomposition<P: GLVConfig>() {
     let mut rng = ark_std::test_rng();
@@ -28,7 +26,19 @@ pub fn glv_scalar_decomposition<P: GLVConfig>() {
         if !is_k1_positive && !is_k2_positive {
             assert_eq!(-k1 - k2 * P::LAMBDA, k);
         }
-        // could be nice to check if k1 and k2 are indeed small.
+
+        // check if k1 and k2 are indeed small.
+        let expected_max_bits = (P::ScalarField::MODULUS_BIT_SIZE + 1) / 2;
+        assert!(
+            k1.into_bigint().num_bits() <= expected_max_bits,
+            "k1 has {} bits",
+            k1.into_bigint().num_bits()
+        );
+        assert!(
+            k2.into_bigint().num_bits() <= expected_max_bits,
+            "k2 has {} bits",
+            k2.into_bigint().num_bits()
+        );
     }
 }