From 59576abf3d03ad3792b143eceabc94c4f3b5cc8d Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Tue, 15 Dec 2020 15:27:36 -0600 Subject: [PATCH 1/4] Remove legendre symbol from quad ext fields sqrt --- ff/src/fields/models/quadratic_extension.rs | 43 ++++++++++----------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/ff/src/fields/models/quadratic_extension.rs b/ff/src/fields/models/quadratic_extension.rs index 9076841de..57ea5f06e 100644 --- a/ff/src/fields/models/quadratic_extension.rs +++ b/ff/src/fields/models/quadratic_extension.rs @@ -278,34 +278,31 @@ where fn sqrt(&self) -> Option { // Square root based on the complex method. See // https://eprint.iacr.org/2012/685.pdf (page 15, algorithm 8) - use crate::LegendreSymbol::*; if self.c1.is_zero() { return self.c0.sqrt().map(|c0| Self::new(c0, P::BaseField::zero())); } let alpha = self.norm(); - // This is equivalent to self.legendre, see the comment on legendre above. - let legendre_symbol = alpha.legendre(); - match legendre_symbol { - Zero => Some(*self), - QuadraticNonResidue => None, - QuadraticResidue => { - // TODO: Precompute this - let two_inv = P::BaseField::one() - .double() - .inverse() - .expect("Two should always have an inverse"); - let alpha = alpha - .sqrt() - .expect("We are in the QR case, the norm should have a square root"); - let mut delta = (alpha + &self.c0) * &two_inv; - if delta.legendre().is_qnr() { - delta -= α - } - let c0 = delta.sqrt().expect("Delta must have a square root"); - let c0_inv = c0.inverse().expect("c0 must have an inverse"); - Some(Self::new(c0, self.c1 * &two_inv * &c0_inv)) - } + // TODO: Precompute this + let two_inv = P::BaseField::one() + .double() + .inverse() + .expect("Two should always have an inverse"); + let alpha = alpha + .sqrt() + .expect("We are in the QR case, the norm should have a square root"); + let mut delta = (alpha + &self.c0) * &two_inv; + if delta.legendre().is_qnr() { + delta -= α } + let c0 = delta.sqrt().expect("Delta must have a square root"); + let c0_inv = c0.inverse().expect("c0 must have an inverse"); + let sqrt_cand = Self::new(c0, self.c1 * &two_inv * &c0_inv); + // Check if sqrt_cand is actually the square root + // if not, there exists no square root. + if sqrt_cand.square() == *self { + return Some(sqrt_cand); + } + None } fn sqrt_in_place(&mut self) -> Option<&mut Self> { From d8b237d7abd616f4c7229355b3733bb6b46f3dbe Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Tue, 15 Dec 2020 15:43:12 -0600 Subject: [PATCH 2/4] Remove obsolete debug code that causes errors downstream --- ff/src/fields/arithmetic.rs | 11 ----------- ff/src/fields/models/quadratic_extension.rs | 9 +++++++++ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/ff/src/fields/arithmetic.rs b/ff/src/fields/arithmetic.rs index be5037dd6..7d31a5766 100644 --- a/ff/src/fields/arithmetic.rs +++ b/ff/src/fields/arithmetic.rs @@ -240,17 +240,6 @@ macro_rules! sqrt_impl { let mut b = x * &w; let mut v = $P::TWO_ADICITY as usize; - // t = self^t - #[cfg(debug_assertions)] - { - let mut check = b; - for _ in 0..(v - 1) { - check.square_in_place(); - } - if !check.is_one() { - panic!("Input is not a square root, but it passed the QR test") - } - } while !b.is_one() { let mut k = 0usize; diff --git a/ff/src/fields/models/quadratic_extension.rs b/ff/src/fields/models/quadratic_extension.rs index 57ea5f06e..27d1a6167 100644 --- a/ff/src/fields/models/quadratic_extension.rs +++ b/ff/src/fields/models/quadratic_extension.rs @@ -281,6 +281,8 @@ where if self.c1.is_zero() { return self.c0.sqrt().map(|c0| Self::new(c0, P::BaseField::zero())); } + // Try computing the square root + // Check at the end of the algorithm if it was a square root let alpha = self.norm(); // TODO: Precompute this let two_inv = P::BaseField::one() @@ -302,6 +304,13 @@ where if sqrt_cand.square() == *self { return Some(sqrt_cand); } + #[cfg(debug_assertions)] + { + use crate::fields::LegendreSymbol::*; + if (self.legendre() != QuadraticNonResidue) { + panic!("Input has a square root per its legendre symbol, but it was not found") + } + } None } From 4a1d3165092910f42d5cb20c7820f545977b2ac6 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Tue, 15 Dec 2020 15:45:36 -0600 Subject: [PATCH 3/4] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8087c0027..f620f2757 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,7 @@ - #122 (ark-poly) Add infrastructure for benchmarking `FFT`s. - #125 (ark-poly) Add parallelization to applying coset shifts within `coset_fft`. - #126 (ark-ec) Use `ark_ff::batch_inversion` for point normalization -- #131 (ark-ff) Speedup `sqrt` on `PrimeField` when a square root exists. (And slows it down when doesn't exist) +- #131, #137 (ark-ff) Speedup `sqrt` on fields when a square root exists. (And slows it down when doesn't exist) ### Bug fixes - #36 (ark-ec) In Short-Weierstrass curves, include an infinity bit in `ToConstraintField`. From 6c235b48d09b3c50e8a63a0df9c96dcca826b674 Mon Sep 17 00:00:00 2001 From: ValarDragon Date: Tue, 15 Dec 2020 15:48:45 -0600 Subject: [PATCH 4/4] fix lint --- ff/src/fields/models/quadratic_extension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ff/src/fields/models/quadratic_extension.rs b/ff/src/fields/models/quadratic_extension.rs index 27d1a6167..29c919d9d 100644 --- a/ff/src/fields/models/quadratic_extension.rs +++ b/ff/src/fields/models/quadratic_extension.rs @@ -307,7 +307,7 @@ where #[cfg(debug_assertions)] { use crate::fields::LegendreSymbol::*; - if (self.legendre() != QuadraticNonResidue) { + if self.legendre() != QuadraticNonResidue { panic!("Input has a square root per its legendre symbol, but it was not found") } }