From 52e86e38b5bc17c75eb92c394d57205c41d5cc89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcin=20G=C3=B3rny?= Date: Tue, 30 Nov 2021 20:05:53 +0100 Subject: [PATCH 1/4] Split cases for c0.is_qr and c0.is_nqr --- ff/src/fields/models/quadratic_extension.rs | 8 +++++++- test-curves/src/bls12_381/tests.rs | 11 ++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/ff/src/fields/models/quadratic_extension.rs b/ff/src/fields/models/quadratic_extension.rs index 6477f972d..8c38bdd95 100644 --- a/ff/src/fields/models/quadratic_extension.rs +++ b/ff/src/fields/models/quadratic_extension.rs @@ -350,7 +350,13 @@ where // Square root based on the complex method. See // https://eprint.iacr.org/2012/685.pdf (page 15, algorithm 8) if self.c1.is_zero() { - return self.c0.sqrt().map(|c0| Self::new(c0, P::BaseField::zero())); + if self.c0.legendre().is_qr() { + return self.c0.sqrt().map(|c0| Self::new(c0, P::BaseField::zero())); + } else { + return (self.c0 * P::NONRESIDUE) + .sqrt() + .map(|res| Self::new(P::BaseField::zero(), res)); + } } // Try computing the square root // Check at the end of the algorithm if it was a square root diff --git a/test-curves/src/bls12_381/tests.rs b/test-curves/src/bls12_381/tests.rs index 7c83fe4d7..54220863a 100644 --- a/test-curves/src/bls12_381/tests.rs +++ b/test-curves/src/bls12_381/tests.rs @@ -1,6 +1,6 @@ #![allow(unused_imports)] use ark_ec::{models::SWModelParameters, AffineCurve, PairingEngine, ProjectiveCurve}; -use ark_ff::{Field, One, UniformRand, Zero}; +use ark_ff::{Field, One, UniformRand, Zero, SquareRootField}; use crate::bls12_381::{g1, Fq, Fq2, Fq6, FqParameters, Fr, G1Affine, G1Projective}; use ark_algebra_test_templates::{curves::*, fields::*, groups::*}; @@ -75,3 +75,12 @@ fn test_g1_generator() { assert!(generator.is_on_curve()); assert!(generator.is_in_correct_subgroup_assuming_on_curve()); } + +#[test] +fn test_fq2_sqrt() { + let fq2 = Fq2::new(-Fq::one(), Fq::zero()); + assert!(fq2.sqrt().is_some()); // should be a residue, expect c0 = 0, c1 = -1 + assert_eq!(fq2.c0, Fq::zero()); + assert_eq!(fq2.c1, -Fq::one()); + +} From bf528b191b0daf56549685f3038ea020eb51ba6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcin=20G=C3=B3rny?= Date: Sun, 5 Dec 2021 15:35:11 +0100 Subject: [PATCH 2/4] Fix bug for sqrt on QuadExtField when c1 = 0 & c0.legendre.is_qnr() Should use sqrt(c0/beta) --- ff/src/fields/models/quadratic_extension.rs | 9 ++++++++- test-curves/src/bls12_381/tests.rs | 9 ++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/ff/src/fields/models/quadratic_extension.rs b/ff/src/fields/models/quadratic_extension.rs index 8c38bdd95..971c50fcf 100644 --- a/ff/src/fields/models/quadratic_extension.rs +++ b/ff/src/fields/models/quadratic_extension.rs @@ -350,10 +350,17 @@ where // Square root based on the complex method. See // https://eprint.iacr.org/2012/685.pdf (page 15, algorithm 8) if self.c1.is_zero() { + // for c = c0 + c1 * x, we have c1 = 0 + // sqrt(c) == sqrt(c0) is an element of Fp2, i.e. sqrt(c0) = a = a0 + a1 * x for some a0, a1 in Fp + // squaring both sides: c0 = a0^2 + a1^2 * x^2 + (2 * a0 * a1 * x) = a0^2 + (a1^2 * P::NONRESIDUE) + // since there are no `x` terms on LHS, a0 * a1 = 0 + // so either a0 = sqrt(c0) or a1 = sqrt(c0/P::NONRESIDUE) if self.c0.legendre().is_qr() { + // either c0 is a valid sqrt in the base field return self.c0.sqrt().map(|c0| Self::new(c0, P::BaseField::zero())); } else { - return (self.c0 * P::NONRESIDUE) + // or we need to compute sqrt(c0/P::NONRESIDUE) + return (self.c0.div(P::NONRESIDUE)) .sqrt() .map(|res| Self::new(P::BaseField::zero(), res)); } diff --git a/test-curves/src/bls12_381/tests.rs b/test-curves/src/bls12_381/tests.rs index 54220863a..c9c419975 100644 --- a/test-curves/src/bls12_381/tests.rs +++ b/test-curves/src/bls12_381/tests.rs @@ -78,9 +78,8 @@ fn test_g1_generator() { #[test] fn test_fq2_sqrt() { - let fq2 = Fq2::new(-Fq::one(), Fq::zero()); - assert!(fq2.sqrt().is_some()); // should be a residue, expect c0 = 0, c1 = -1 - assert_eq!(fq2.c0, Fq::zero()); - assert_eq!(fq2.c1, -Fq::one()); - + // regression test for the case when c1=0 and c0.legendre().is_qnr() + let fq2_sqrt = Fq2::new(-Fq::one(), Fq::zero()).sqrt().unwrap(); + assert_eq!(fq2_sqrt.c0, Fq::zero()); + assert_eq!(fq2_sqrt.c1, Fq::one()); } From e402d8a00b831e69adadaa284d4855a7019150a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcin=20G=C3=B3rny?= Date: Sun, 5 Dec 2021 15:50:45 +0100 Subject: [PATCH 3/4] Remove the changes to tests.rs file bls12-381 regression tests will go into the curves repo --- test-curves/src/bls12_381/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test-curves/src/bls12_381/tests.rs b/test-curves/src/bls12_381/tests.rs index fbec8f37a..8a0305a58 100644 --- a/test-curves/src/bls12_381/tests.rs +++ b/test-curves/src/bls12_381/tests.rs @@ -1,6 +1,6 @@ - #![allow(unused_imports)] +#![allow(unused_imports)] use ark_ec::{models::SWModelParameters, AffineCurve, PairingEngine, ProjectiveCurve}; -use ark_ff::{Field, One, UniformRand, Zero, SquareRootField}; +use ark_ff::{Field, One, UniformRand, Zero}; use crate::bls12_381::{g1, Fq, Fq2, Fq6, FqParameters, Fr, G1Affine, G1Projective}; use ark_algebra_test_templates::{ From 84eca74300defdedc715f5f8b538cffc87fb12a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcin=20G=C3=B3rny?= Date: Sun, 5 Dec 2021 15:59:30 +0100 Subject: [PATCH 4/4] Add a line to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b4c54bab..cd33f8114 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ ### Bugfixes - [\#350](https://github.com/arkworks-rs/algebra/pull/350) (ark-serialize) Fix issues with santiation whenever a non-standard `Result` type is in scope. +- [\#358](https://github.com/arkworks-rs/algebra/pull/358) (ark-ff) Fix the bug for `QuadExtField::sqrt` when `c1 = 0 && c0.legendre.is_qnr()` ## v0.3.0