From f88d7c6ea336d4b71e52f53c6ae2934e29accf43 Mon Sep 17 00:00:00 2001
From: Dev Ojha <ValarDragon@users.noreply.github.com>
Date: Fri, 15 Jan 2021 12:55:26 -0600
Subject: [PATCH] Make `mul_by_inverse` use one constraint (#42)

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
---
 CHANGELOG.md         |  1 +
 src/fields/fp/mod.rs | 15 ---------------
 src/fields/mod.rs    | 25 +++++++++----------------
 3 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4b22a95..6a9f89c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,7 @@
 - #9 Fix bug in short weierstrass projective curve point's to_affine method
 - #29 Fix `to_non_unique_bytes` for `BLS12::G1Prepared`
 - #34 Fix `mul_by_inverse` for constants
+- #42 Fix regression in `mul_by_inverse` constraint count
 
 ## v0.1.0
 
diff --git a/src/fields/fp/mod.rs b/src/fields/fp/mod.rs
index a6fabcf..e3535df 100644
--- a/src/fields/fp/mod.rs
+++ b/src/fields/fp/mod.rs
@@ -716,21 +716,6 @@ impl<F: PrimeField> FieldVar<F, F> for FpVar<F> {
         }
     }
 
-    /// Returns (self / denominator), but requires fewer constraints than
-    /// self * denominator.inverse()
-    /// It is up to the caller to ensure that denominator is non-zero,
-    /// since in that case the result is unconstrained.
-    #[tracing::instrument(target = "r1cs")]
-    fn mul_by_inverse(&self, denominator: &Self) -> Result<Self, SynthesisError> {
-        use FpVar::*;
-        match (self, denominator) {
-            (Constant(s), Constant(d)) => Ok(Constant(*s / *d)),
-            (Var(s), Constant(d)) => Ok(Var(s.mul_constant(d.inverse().get()?))),
-            (Constant(s), Var(d)) => Ok(Var(d.inverse()?.mul_constant(*s))),
-            (Var(s), Var(d)) => Ok(Var(d.inverse()?.mul(s))),
-        }
-    }
-
     #[tracing::instrument(target = "r1cs")]
     fn frobenius_map(&self, power: usize) -> Result<Self, SynthesisError> {
         match self {
diff --git a/src/fields/mod.rs b/src/fields/mod.rs
index d51194e..d6294df 100644
--- a/src/fields/mod.rs
+++ b/src/fields/mod.rs
@@ -5,7 +5,7 @@ use core::{
     ops::{Add, AddAssign, Mul, MulAssign, Sub, SubAssign},
 };
 
-use crate::{prelude::*, Assignment};
+use crate::prelude::*;
 
 /// This module contains a generic implementation of cubic extension field
 /// variables. That is, it implements the R1CS equivalent of
@@ -155,23 +155,16 @@ pub trait FieldVar<F: Field, ConstraintF: Field>:
     /// Computes `result` such that `self * result == Self::one()`.
     fn inverse(&self) -> Result<Self, SynthesisError>;
 
-    /// Returns `(self / denominator)`. but requires fewer constraints than
-    /// `self * denominator.inverse()`.
-    /// It is up to the caller to ensure that denominator is non-zero,
+    /// Returns `(self / d)`. but requires fewer constraints than `self * d.inverse()`.
+    /// It is up to the caller to ensure that `d` is non-zero,
     /// since in that case the result is unconstrained.
-    fn mul_by_inverse(&self, denominator: &Self) -> Result<Self, SynthesisError> {
-        if denominator.is_constant() {
-            Ok(denominator.inverse()? * self)
+    fn mul_by_inverse(&self, d: &Self) -> Result<Self, SynthesisError> {
+        let d_inv = if self.is_constant() || d.is_constant() {
+            d.inverse()?
         } else {
-            let result = Self::new_witness(self.cs(), || {
-                let denominator_inv_native = denominator.value()?.inverse().get()?;
-                let result = self.value()? * &denominator_inv_native;
-                Ok(result)
-            })?;
-            result.mul_equals(&denominator, &self)?;
-
-            Ok(result)
-        }
+            Self::new_witness(self.cs(), || Ok(d.value()?.inverse().unwrap_or(F::zero())))?
+        };
+        Ok(d_inv * self)
     }
 
     /// Computes the frobenius map over `self`.