From 4bf6202c350adbcef6827714cb47dfc8553cb3cd Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Mon, 26 Apr 2021 23:17:08 +0800
Subject: [PATCH 01/11] Modify ECC gadget to work with chip refactor

---
 Cargo.toml                |   2 +-
 src/circuit/gadget/ecc.rs | 208 ++++++++++++++++++++++++++++----------
 2 files changed, 154 insertions(+), 56 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 1fb2e67c..828299e8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -32,7 +32,7 @@ subtle = "2.3"
 
 [dependencies.halo2]
 git = "https://github.com/zcash/halo2.git"
-rev = "6acacf1aca12f34fc311aa59056e40adc0e6d8bd"
+rev = "cae6f6af725cf1f5bc94e126a0b41e9ac602a302"
 
 [dependencies.pasta_curves]
 git = "https://github.com/zcash/pasta_curves.git"
diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 049c6b9e..36f27446 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -12,129 +12,227 @@ use halo2::{
 pub trait FixedPoints<C: CurveAffine>: Clone + fmt::Debug {}
 
 /// The set of circuit instructions required to use the ECC gadgets.
-pub trait EccInstructions<C: CurveAffine>: Chip<Field = C::Base> {
-    /// Variable representing an element of the elliptic curve's scalar field.
-    type Scalar: Clone + fmt::Debug;
+pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
+    /// Variable representing a full-width element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
+    type ScalarFixed: Clone + fmt::Debug;
+    /// Variable representing a signed short element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
+    type ScalarFixedShort: Clone + fmt::Debug;
     /// Variable representing an elliptic curve point.
     type Point: Clone + fmt::Debug;
+    /// Variable representing the x-coordinate of an elliptic curve point.
+    type X: Clone + fmt::Debug;
     /// Variable representing the set of fixed bases in the circuit.
     type FixedPoints: FixedPoints<C>;
     /// Variable representing a fixed elliptic curve point (constant in the circuit).
     type FixedPoint: Clone + fmt::Debug;
 
-    /// Witnesses the given scalar as a private input to the circuit.
-    fn witness_scalar(
-        layouter: &mut impl Layouter<Self>,
+    /// Witnesses the given full-width scalar as a private input to the circuit for fixed-based scalar mul.
+    fn witness_scalar_fixed(
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
         value: Option<C::Scalar>,
-    ) -> Result<Self::Scalar, Error>;
+    ) -> Result<Self::ScalarFixed, Error>;
+
+    /// Witnesses the given signed short scalar as a private input to the circuit for fixed-based scalar mul.
+    fn witness_scalar_fixed_short(
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        value: Option<C::Scalar>,
+    ) -> Result<Self::ScalarFixedShort, Error>;
 
     /// Witnesses the given point as a private input to the circuit.
     fn witness_point(
-        layouter: &mut impl Layouter<Self>,
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
         value: Option<C>,
     ) -> Result<Self::Point, Error>;
 
+    /// Extracts the x-coordinate of a point.
+    fn extract_p(point: &Self::Point) -> &Self::X;
+
     /// Gets a fixed point into the circuit.
-    fn get_fixed(
-        layouter: &mut impl Layouter<Self>,
-        fixed_points: Self::FixedPoints,
-    ) -> Result<Self::FixedPoint, Error>;
+    fn get_fixed(&self, fixed_points: Self::FixedPoints) -> Result<Self::FixedPoint, Error>;
 
     /// Performs point addition, returning `a + b`.
     fn add(
-        layouter: &mut impl Layouter<Self>,
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
         a: &Self::Point,
         b: &Self::Point,
     ) -> Result<Self::Point, Error>;
 
-    /// Performs point doubling, returning `[2] a`.
-    fn double(layouter: &mut impl Layouter<Self>, a: &Self::Point) -> Result<Self::Point, Error>;
+    /// Performs complete point addition, returning `a + b`.
+    fn add_complete(
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        a: &Self::Point,
+        b: &Self::Point,
+    ) -> Result<Self::Point, Error>;
 
     /// Performs variable-base scalar multiplication, returning `[scalar] base`.
     fn mul(
-        layouter: &mut impl Layouter<Self>,
-        scalar: &Self::Scalar,
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        scalar: C::Scalar,
         base: &Self::Point,
     ) -> Result<Self::Point, Error>;
 
-    /// Performs fixed-base scalar multiplication, returning `[scalar] base`.
+    /// Performs fixed-base scalar multiplication using a full-width scalar, returning `[scalar] base`.
     fn mul_fixed(
-        layouter: &mut impl Layouter<Self>,
-        scalar: &Self::Scalar,
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        scalar: &Self::ScalarFixed,
+        base: &Self::FixedPoint,
+    ) -> Result<Self::Point, Error>;
+
+    /// Performs fixed-base scalar multiplication using a short signed scalar, returning `[scalar] base`.
+    fn mul_fixed_short(
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        scalar: &Self::ScalarFixedShort,
         base: &Self::FixedPoint,
     ) -> Result<Self::Point, Error>;
 }
 
-/// An element of the given elliptic curve's scalar field.
+/// A full-width element of the given elliptic curve's scalar field, to be used for fixed-base scalar mul.
 #[derive(Debug)]
-pub struct Scalar<C: CurveAffine, EccChip: EccInstructions<C>> {
-    inner: EccChip::Scalar,
+pub struct ScalarFixed<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+    chip: EccChip,
+    inner: EccChip::ScalarFixed,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C>> Scalar<C, EccChip> {
-    /// Constructs a new point with the given value.
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> ScalarFixed<C, EccChip> {
+    /// Constructs a new ScalarFixed with the given value.
     pub fn new(
-        mut layouter: impl Layouter<EccChip>,
+        chip: EccChip,
+        mut layouter: impl Layouter<C::Base>,
         value: Option<C::Scalar>,
     ) -> Result<Self, Error> {
-        EccChip::witness_scalar(&mut layouter, value).map(|inner| Scalar { inner })
+        chip.witness_scalar_fixed(&mut layouter, value)
+            .map(|inner| ScalarFixed {
+                chip: chip.clone(),
+                inner,
+            })
+    }
+}
+
+/// A signed short element of the given elliptic curve's scalar field, to be used for fixed-base scalar mul.
+#[derive(Debug)]
+pub struct ScalarFixedShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+    chip: EccChip,
+    inner: EccChip::ScalarFixedShort,
+}
+
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> ScalarFixedShort<C, EccChip> {
+    /// Constructs a new ScalarFixedShort with the given value.
+    pub fn new(
+        chip: EccChip,
+        mut layouter: impl Layouter<C::Base>,
+        value: Option<C::Scalar>,
+    ) -> Result<Self, Error> {
+        chip.witness_scalar_fixed_short(&mut layouter, value)
+            .map(|inner| ScalarFixedShort {
+                chip: chip.clone(),
+                inner,
+            })
     }
 }
 
 /// An elliptic curve point over the given curve.
 #[derive(Debug)]
-pub struct Point<C: CurveAffine, EccChip: EccInstructions<C>> {
+pub struct Point<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+    chip: EccChip,
     inner: EccChip::Point,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C>> Point<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> Point<C, EccChip> {
     /// Constructs a new point with the given value.
-    pub fn new(mut layouter: impl Layouter<EccChip>, value: Option<C>) -> Result<Self, Error> {
-        EccChip::witness_point(&mut layouter, value).map(|inner| Point { inner })
+    pub fn new(
+        &self,
+        mut layouter: impl Layouter<C::Base>,
+        value: Option<C>,
+    ) -> Result<Self, Error> {
+        self.chip
+            .witness_point(&mut layouter, value)
+            .map(|inner| Point {
+                chip: self.chip.clone(),
+                inner,
+            })
+    }
+
+    /// Extracts the x-coordinate of a point.
+    pub fn extract_p(&self) -> X<C, EccChip> {
+        X::from_inner(self.chip.clone(), EccChip::extract_p(&self.inner).clone())
+    }
+
+    /// Wraps the given point (obtained directly from an instruction) in a gadget.
+    pub fn from_inner(chip: EccChip, inner: EccChip::Point) -> Self {
+        Point { chip, inner }
     }
 
     /// Returns `self + other`.
-    pub fn add(&self, mut layouter: impl Layouter<EccChip>, other: &Self) -> Result<Self, Error> {
-        EccChip::add(&mut layouter, &self.inner, &other.inner).map(|inner| Point { inner })
-    }
-
-    /// Returns `[2] self`.
-    pub fn double(&self, mut layouter: impl Layouter<EccChip>) -> Result<Self, Error> {
-        EccChip::double(&mut layouter, &self.inner).map(|inner| Point { inner })
+    pub fn add(&self, mut layouter: impl Layouter<C::Base>, other: &Self) -> Result<Self, Error> {
+        self.chip
+            .add(&mut layouter, &self.inner, &other.inner)
+            .map(|inner| Point {
+                chip: self.chip.clone(),
+                inner,
+            })
     }
 
     /// Returns `[by] self`.
-    pub fn mul(
-        &self,
-        mut layouter: impl Layouter<EccChip>,
-        by: &Scalar<C, EccChip>,
-    ) -> Result<Self, Error> {
-        EccChip::mul(&mut layouter, &by.inner, &self.inner).map(|inner| Point { inner })
+    pub fn mul(&self, mut layouter: impl Layouter<C::Base>, by: C::Scalar) -> Result<Self, Error> {
+        self.chip
+            .mul(&mut layouter, by, &self.inner)
+            .map(|inner| Point {
+                chip: self.chip.clone(),
+                inner,
+            })
+    }
+}
+
+/// The x-coordinate of an elliptic curve point over the given curve.
+#[derive(Debug)]
+pub struct X<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+    chip: EccChip,
+    inner: EccChip::X,
+}
+
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> X<C, EccChip> {
+    /// Wraps the given x-coordinate (obtained directly from an instruction) in a gadget.
+    pub fn from_inner(chip: EccChip, inner: EccChip::X) -> Self {
+        X { chip, inner }
     }
 }
 
 /// A constant elliptic curve point over the given curve, for which scalar multiplication
 /// is more efficient.
-#[derive(Debug)]
-pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C>> {
+#[derive(Clone, Debug)]
+pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+    chip: EccChip,
     inner: EccChip::FixedPoint,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C>> FixedPoint<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> FixedPoint<C, EccChip> {
     /// Gets a reference to the specified fixed point in the circuit.
-    pub fn get(
-        mut layouter: impl Layouter<EccChip>,
-        point: EccChip::FixedPoints,
-    ) -> Result<Self, Error> {
-        EccChip::get_fixed(&mut layouter, point).map(|inner| FixedPoint { inner })
+    pub fn get(chip: EccChip, point: EccChip::FixedPoints) -> Result<Self, Error> {
+        chip.get_fixed(point).map(|inner| FixedPoint {
+            chip: chip.clone(),
+            inner,
+        })
     }
 
     /// Returns `[by] self`.
     pub fn mul(
         &self,
-        mut layouter: impl Layouter<EccChip>,
-        by: &Scalar<C, EccChip>,
+        mut layouter: impl Layouter<C::Base>,
+        by: &ScalarFixed<C, EccChip>,
     ) -> Result<Point<C, EccChip>, Error> {
-        EccChip::mul_fixed(&mut layouter, &by.inner, &self.inner).map(|inner| Point { inner })
+        self.chip
+            .mul_fixed(&mut layouter, &by.inner, &self.inner)
+            .map(|inner| Point {
+                chip: self.chip.clone(),
+                inner,
+            })
     }
 }

From 4f2b4d29355825c35cbefcb928013985b64f6dda Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 4 May 2021 13:13:04 +0800
Subject: [PATCH 02/11] Address review comments

Co-authored-by: Jack Grigg <jack@electriccoin.co>
---
 src/circuit/gadget/ecc.rs | 169 +++++++++++++++++++++++++++-----------
 1 file changed, 121 insertions(+), 48 deletions(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 36f27446..5e325c86 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -1,6 +1,6 @@
 //! Gadgets for elliptic curve operations.
 
-use std::fmt;
+use std::fmt::Debug;
 
 use halo2::{
     arithmetic::CurveAffine,
@@ -8,32 +8,48 @@ use halo2::{
     plonk::Error,
 };
 
-/// Trait allowing circuit's fixed points to be enumerated.
-pub trait FixedPoints<C: CurveAffine>: Clone + fmt::Debug {}
-
 /// The set of circuit instructions required to use the ECC gadgets.
 pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
+    /// Variable representing an element of the elliptic curve's base field, that
+    /// is used as a scalar in variable-base scalar mul.
+    ///
+    /// It is not true in general that a scalar field element fits in a curve's
+    /// base field, and in particular it is untrue for the Pallas curve, whose
+    /// scalar field `Fq` is larger than its base field `Fp`.
+    ///
+    /// However, the only use of variable-base scalar mul in the Orchard protocol
+    /// is in deriving diversified addresses `[ivk] g_d`,  and `ivk` is guaranteed
+    /// to be in the base field of the curve. (See non-normative notes in
+    /// https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents.)
+    type ScalarVar: Clone + Debug;
     /// Variable representing a full-width element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
-    type ScalarFixed: Clone + fmt::Debug;
+    type ScalarFixed: Clone + Debug;
     /// Variable representing a signed short element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
-    type ScalarFixedShort: Clone + fmt::Debug;
+    type ScalarFixedShort: Clone + Debug;
     /// Variable representing an elliptic curve point.
-    type Point: Clone + fmt::Debug;
+    type Point: Clone + Debug;
     /// Variable representing the x-coordinate of an elliptic curve point.
-    type X: Clone + fmt::Debug;
+    type X: Clone + Debug;
     /// Variable representing the set of fixed bases in the circuit.
-    type FixedPoints: FixedPoints<C>;
+    type FixedPoints: Clone + Debug;
     /// Variable representing a fixed elliptic curve point (constant in the circuit).
-    type FixedPoint: Clone + fmt::Debug;
+    type FixedPoint: Clone + Debug;
 
-    /// Witnesses the given full-width scalar as a private input to the circuit for fixed-based scalar mul.
+    /// Witnesses the given base field element as a private input to the circuit for variable-base scalar mul.
+    fn witness_scalar_var(
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        value: Option<C::Base>,
+    ) -> Result<Self::ScalarVar, Error>;
+
+    /// Witnesses the given full-width scalar as a private input to the circuit for fixed-base scalar mul.
     fn witness_scalar_fixed(
         &self,
         layouter: &mut impl Layouter<C::Base>,
         value: Option<C::Scalar>,
     ) -> Result<Self::ScalarFixed, Error>;
 
-    /// Witnesses the given signed short scalar as a private input to the circuit for fixed-based scalar mul.
+    /// Witnesses the given signed short scalar as a private input to the circuit for fixed-base scalar mul.
     fn witness_scalar_fixed_short(
         &self,
         layouter: &mut impl Layouter<C::Base>,
@@ -50,11 +66,13 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     /// Extracts the x-coordinate of a point.
     fn extract_p(point: &Self::Point) -> &Self::X;
 
-    /// Gets a fixed point into the circuit.
+    /// Returns a fixed point that had been previously loaded into the circuit.
+    /// The pre-loaded cells are used to set up equality constraints in other
+    /// parts of the circuit where the fixed base is used.
     fn get_fixed(&self, fixed_points: Self::FixedPoints) -> Result<Self::FixedPoint, Error>;
 
-    /// Performs point addition, returning `a + b`.
-    fn add(
+    /// Performs incomplete point addition, returning `a + b`.
+    fn add_incomplete(
         &self,
         layouter: &mut impl Layouter<C::Base>,
         a: &Self::Point,
@@ -62,7 +80,7 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     ) -> Result<Self::Point, Error>;
 
     /// Performs complete point addition, returning `a + b`.
-    fn add_complete(
+    fn add(
         &self,
         layouter: &mut impl Layouter<C::Base>,
         a: &Self::Point,
@@ -73,7 +91,7 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     fn mul(
         &self,
         layouter: &mut impl Layouter<C::Base>,
-        scalar: C::Scalar,
+        scalar: &Self::ScalarVar,
         base: &Self::Point,
     ) -> Result<Self::Point, Error>;
 
@@ -94,14 +112,43 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     ) -> Result<Self::Point, Error>;
 }
 
+/// An element of the given elliptic curve's base field, that is used as a scalar
+/// in variable-base scalar mul.
+///
+/// It is not true in general that a scalar field element fits in a curve's
+/// base field, and in particular it is untrue for the Pallas curve, whose
+/// scalar field `Fq` is larger than its base field `Fp`.
+///
+/// However, the only use of variable-base scalar mul in the Orchard protocol
+/// is in deriving diversified addresses `[ivk] g_d`,  and `ivk` is guaranteed
+/// to be in the base field of the curve. (See non-normative notes in
+/// https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents.)
+#[derive(Debug)]
+pub struct ScalarVar<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+    chip: EccChip,
+    inner: EccChip::ScalarVar,
+}
+
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarVar<C, EccChip> {
+    /// Constructs a new ScalarVar with the given value.
+    pub fn new(
+        chip: EccChip,
+        mut layouter: impl Layouter<C::Base>,
+        value: Option<C::Base>,
+    ) -> Result<Self, Error> {
+        chip.witness_scalar_var(&mut layouter, value)
+            .map(|inner| ScalarVar { chip, inner })
+    }
+}
+
 /// A full-width element of the given elliptic curve's scalar field, to be used for fixed-base scalar mul.
 #[derive(Debug)]
-pub struct ScalarFixed<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+pub struct ScalarFixed<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
     chip: EccChip,
     inner: EccChip::ScalarFixed,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> ScalarFixed<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixed<C, EccChip> {
     /// Constructs a new ScalarFixed with the given value.
     pub fn new(
         chip: EccChip,
@@ -109,21 +156,18 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> ScalarFixed<C, EccChip
         value: Option<C::Scalar>,
     ) -> Result<Self, Error> {
         chip.witness_scalar_fixed(&mut layouter, value)
-            .map(|inner| ScalarFixed {
-                chip: chip.clone(),
-                inner,
-            })
+            .map(|inner| ScalarFixed { chip, inner })
     }
 }
 
 /// A signed short element of the given elliptic curve's scalar field, to be used for fixed-base scalar mul.
 #[derive(Debug)]
-pub struct ScalarFixedShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+pub struct ScalarFixedShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
     chip: EccChip,
     inner: EccChip::ScalarFixedShort,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> ScalarFixedShort<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixedShort<C, EccChip> {
     /// Constructs a new ScalarFixedShort with the given value.
     pub fn new(
         chip: EccChip,
@@ -131,33 +175,26 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> ScalarFixedShort<C, Ec
         value: Option<C::Scalar>,
     ) -> Result<Self, Error> {
         chip.witness_scalar_fixed_short(&mut layouter, value)
-            .map(|inner| ScalarFixedShort {
-                chip: chip.clone(),
-                inner,
-            })
+            .map(|inner| ScalarFixedShort { chip, inner })
     }
 }
 
 /// An elliptic curve point over the given curve.
 #[derive(Debug)]
-pub struct Point<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+pub struct Point<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
     chip: EccChip,
     inner: EccChip::Point,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> Point<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccChip> {
     /// Constructs a new point with the given value.
     pub fn new(
-        &self,
+        chip: EccChip,
         mut layouter: impl Layouter<C::Base>,
         value: Option<C>,
     ) -> Result<Self, Error> {
-        self.chip
-            .witness_point(&mut layouter, value)
-            .map(|inner| Point {
-                chip: self.chip.clone(),
-                inner,
-            })
+        let point = chip.witness_point(&mut layouter, value);
+        point.map(|inner| Point { chip, inner })
     }
 
     /// Extracts the x-coordinate of a point.
@@ -172,6 +209,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> Point<C, EccChip> {
 
     /// Returns `self + other`.
     pub fn add(&self, mut layouter: impl Layouter<C::Base>, other: &Self) -> Result<Self, Error> {
+        assert_eq!(format!("{:?}", self.chip), format!("{:?}", other.chip));
         self.chip
             .add(&mut layouter, &self.inner, &other.inner)
             .map(|inner| Point {
@@ -181,9 +219,14 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> Point<C, EccChip> {
     }
 
     /// Returns `[by] self`.
-    pub fn mul(&self, mut layouter: impl Layouter<C::Base>, by: C::Scalar) -> Result<Self, Error> {
+    pub fn mul(
+        &self,
+        mut layouter: impl Layouter<C::Base>,
+        by: &ScalarVar<C, EccChip>,
+    ) -> Result<Self, Error> {
+        assert_eq!(format!("{:?}", self.chip), format!("{:?}", by.chip));
         self.chip
-            .mul(&mut layouter, by, &self.inner)
+            .mul(&mut layouter, &by.inner, &self.inner)
             .map(|inner| Point {
                 chip: self.chip.clone(),
                 inner,
@@ -193,12 +236,12 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> Point<C, EccChip> {
 
 /// The x-coordinate of an elliptic curve point over the given curve.
 #[derive(Debug)]
-pub struct X<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+pub struct X<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
     chip: EccChip,
     inner: EccChip::X,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> X<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> X<C, EccChip> {
     /// Wraps the given x-coordinate (obtained directly from an instruction) in a gadget.
     pub fn from_inner(chip: EccChip, inner: EccChip::X) -> Self {
         X { chip, inner }
@@ -208,18 +251,16 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> X<C, EccChip> {
 /// A constant elliptic curve point over the given curve, for which scalar multiplication
 /// is more efficient.
 #[derive(Clone, Debug)]
-pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C> + Clone> {
+pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
     chip: EccChip,
     inner: EccChip::FixedPoint,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> FixedPoint<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPoint<C, EccChip> {
     /// Gets a reference to the specified fixed point in the circuit.
     pub fn get(chip: EccChip, point: EccChip::FixedPoints) -> Result<Self, Error> {
-        chip.get_fixed(point).map(|inner| FixedPoint {
-            chip: chip.clone(),
-            inner,
-        })
+        chip.get_fixed(point)
+            .map(|inner| FixedPoint { chip, inner })
     }
 
     /// Returns `[by] self`.
@@ -228,6 +269,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> FixedPoint<C, EccChip>
         mut layouter: impl Layouter<C::Base>,
         by: &ScalarFixed<C, EccChip>,
     ) -> Result<Point<C, EccChip>, Error> {
+        assert_eq!(format!("{:?}", self.chip), format!("{:?}", by.chip));
         self.chip
             .mul_fixed(&mut layouter, &by.inner, &self.inner)
             .map(|inner| Point {
@@ -236,3 +278,34 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone> FixedPoint<C, EccChip>
             })
     }
 }
+
+/// A constant elliptic curve point over the given curve, used in scalar multiplication
+/// with a short signed exponent
+#[derive(Clone, Debug)]
+pub struct FixedPointShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+    chip: EccChip,
+    inner: EccChip::FixedPoint,
+}
+
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPointShort<C, EccChip> {
+    /// Gets a reference to the specified fixed point in the circuit.
+    pub fn get(chip: EccChip, point: EccChip::FixedPoints) -> Result<Self, Error> {
+        chip.get_fixed(point)
+            .map(|inner| FixedPointShort { chip, inner })
+    }
+
+    /// Returns `[by] self`.
+    pub fn mul(
+        &self,
+        mut layouter: impl Layouter<C::Base>,
+        by: &ScalarFixedShort<C, EccChip>,
+    ) -> Result<Point<C, EccChip>, Error> {
+        assert_eq!(format!("{:?}", self.chip), format!("{:?}", by.chip));
+        self.chip
+            .mul_fixed_short(&mut layouter, &by.inner, &self.inner)
+            .map(|inner| Point {
+                chip: self.chip.clone(),
+                inner,
+            })
+    }
+}

From 6a64bc1c37052d45c66bc6a82ac2436be8741da9 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Thu, 6 May 2021 12:50:50 +0800
Subject: [PATCH 03/11] Expose Point.add_incomplete()

---
 src/circuit/gadget/ecc.rs | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 5e325c86..9164ae97 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -207,7 +207,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccCh
         Point { chip, inner }
     }
 
-    /// Returns `self + other`.
+    /// Returns `self + other` using complete addition.
     pub fn add(&self, mut layouter: impl Layouter<C::Base>, other: &Self) -> Result<Self, Error> {
         assert_eq!(format!("{:?}", self.chip), format!("{:?}", other.chip));
         self.chip
@@ -218,6 +218,21 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccCh
             })
     }
 
+    /// Returns `self + other` using incomplete addition.
+    pub fn add_incomplete(
+        &self,
+        mut layouter: impl Layouter<C::Base>,
+        other: &Self,
+    ) -> Result<Self, Error> {
+        assert_eq!(format!("{:?}", self.chip), format!("{:?}", other.chip));
+        self.chip
+            .add_incomplete(&mut layouter, &self.inner, &other.inner)
+            .map(|inner| Point {
+                chip: self.chip.clone(),
+                inner,
+            })
+    }
+
     /// Returns `[by] self`.
     pub fn mul(
         &self,

From db60fd226277bfa02e9bfd5c9b022d6f8066d6c7 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Thu, 6 May 2021 15:42:01 +0800
Subject: [PATCH 04/11] Add FixedPointShort associated type

---
 src/circuit/gadget/ecc.rs | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 9164ae97..4f2d4ef5 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -34,6 +34,9 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     type FixedPoints: Clone + Debug;
     /// Variable representing a fixed elliptic curve point (constant in the circuit).
     type FixedPoint: Clone + Debug;
+    /// Variable representing a fixed elliptic curve point (constant in the circuit)
+    /// to be used in scalar multiplication with a short signed exponent.
+    type FixedPointShort: Clone + Debug;
 
     /// Witnesses the given base field element as a private input to the circuit for variable-base scalar mul.
     fn witness_scalar_var(
@@ -71,6 +74,13 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     /// parts of the circuit where the fixed base is used.
     fn get_fixed(&self, fixed_points: Self::FixedPoints) -> Result<Self::FixedPoint, Error>;
 
+    /// Returns a fixed point to be used in scalar multiplication with a signed
+    /// short exponent.
+    fn get_fixed_short(
+        &self,
+        fixed_points: Self::FixedPoints,
+    ) -> Result<Self::FixedPointShort, Error>;
+
     /// Performs incomplete point addition, returning `a + b`.
     fn add_incomplete(
         &self,
@@ -108,7 +118,7 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
         &self,
         layouter: &mut impl Layouter<C::Base>,
         scalar: &Self::ScalarFixedShort,
-        base: &Self::FixedPoint,
+        base: &Self::FixedPointShort,
     ) -> Result<Self::Point, Error>;
 }
 
@@ -299,13 +309,13 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPoint<C,
 #[derive(Clone, Debug)]
 pub struct FixedPointShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
     chip: EccChip,
-    inner: EccChip::FixedPoint,
+    inner: EccChip::FixedPointShort,
 }
 
 impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPointShort<C, EccChip> {
     /// Gets a reference to the specified fixed point in the circuit.
     pub fn get(chip: EccChip, point: EccChip::FixedPoints) -> Result<Self, Error> {
-        chip.get_fixed(point)
+        chip.get_fixed_short(point)
             .map(|inner| FixedPointShort { chip, inner })
     }
 

From 74c797165f13607beb5894c20de5852476b0d943 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 12:03:40 +0800
Subject: [PATCH 05/11] Add range check for short scalar

---
 src/circuit/gadget/ecc.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 4f2d4ef5..abd84748 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -1,9 +1,10 @@
 //! Gadgets for elliptic curve operations.
 
+use ff::Field;
 use std::fmt::Debug;
 
 use halo2::{
-    arithmetic::CurveAffine,
+    arithmetic::{CurveAffine, FieldExt},
     circuit::{Chip, Layouter},
     plonk::Error,
 };
@@ -25,6 +26,8 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     /// Variable representing a full-width element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
     type ScalarFixed: Clone + Debug;
     /// Variable representing a signed short element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
+    ///
+    /// A `ScalarFixedShort` must be in the range [-(2^64 - 1), 2^64 - 1].
     type ScalarFixedShort: Clone + Debug;
     /// Variable representing an elliptic curve point.
     type Point: Clone + Debug;
@@ -179,11 +182,29 @@ pub struct ScalarFixedShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone
 
 impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixedShort<C, EccChip> {
     /// Constructs a new ScalarFixedShort with the given value.
+    ///
+    /// # Panics
+    ///
+    /// The short scalar must be in the range [-(2^64 - 1), (2^64 - 1)].
     pub fn new(
         chip: EccChip,
         mut layouter: impl Layouter<C::Base>,
         value: Option<C::Scalar>,
     ) -> Result<Self, Error> {
+        // Check that the scalar is in the range [-(2^64 - 1), (2^64 - 1)]
+        if let Some(value) = value {
+            let mut sign = C::Scalar::one();
+
+            // T = (p-1) / 2
+            let t = (C::Scalar::zero() - C::Scalar::one()) * C::Scalar::TWO_INV;
+
+            if value > t {
+                sign = -sign;
+            }
+            let magnitude = value * sign;
+            assert!(magnitude < C::Scalar::from_u128(1 << 64));
+        }
+
         chip.witness_scalar_fixed_short(&mut layouter, value)
             .map(|inner| ScalarFixedShort { chip, inner })
     }

From c8076c2864e64c0d70eb9658d4f019dc5f817b0e Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 12:09:49 +0800
Subject: [PATCH 06/11] Add FixedPointsShort associated type

---
 src/circuit/gadget/ecc.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index abd84748..6351d6b4 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -35,6 +35,9 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     type X: Clone + Debug;
     /// Variable representing the set of fixed bases in the circuit.
     type FixedPoints: Clone + Debug;
+    /// Variable representing the set of fixed bases to be used in scalar multiplication
+    /// with a short signed exponent.
+    type FixedPointsShort: Clone + Debug;
     /// Variable representing a fixed elliptic curve point (constant in the circuit).
     type FixedPoint: Clone + Debug;
     /// Variable representing a fixed elliptic curve point (constant in the circuit)
@@ -81,7 +84,7 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     /// short exponent.
     fn get_fixed_short(
         &self,
-        fixed_points: Self::FixedPoints,
+        fixed_points: Self::FixedPointsShort,
     ) -> Result<Self::FixedPointShort, Error>;
 
     /// Performs incomplete point addition, returning `a + b`.
@@ -335,7 +338,7 @@ pub struct FixedPointShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone +
 
 impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPointShort<C, EccChip> {
     /// Gets a reference to the specified fixed point in the circuit.
-    pub fn get(chip: EccChip, point: EccChip::FixedPoints) -> Result<Self, Error> {
+    pub fn get(chip: EccChip, point: EccChip::FixedPointsShort) -> Result<Self, Error> {
         chip.get_fixed_short(point)
             .map(|inner| FixedPointShort { chip, inner })
     }

From edea9bde73cb409f7cfb8c4e7640369c0d07c0d8 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 12:26:59 +0800
Subject: [PATCH 07/11] Document incomplete point addition error handling

---
 src/circuit/gadget/ecc.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 6351d6b4..50ba31dd 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -88,6 +88,8 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     ) -> Result<Self::FixedPointShort, Error>;
 
     /// Performs incomplete point addition, returning `a + b`.
+    ///
+    /// This returns an error in exceptional cases.
     fn add_incomplete(
         &self,
         layouter: &mut impl Layouter<C::Base>,

From caa37915621a53168a9a97a482735a303ed05140 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 16:07:40 +0800
Subject: [PATCH 08/11] Documentation fixes.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
---
 src/circuit/gadget/ecc.rs | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 50ba31dd..dd0e1b47 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -23,20 +23,23 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     /// to be in the base field of the curve. (See non-normative notes in
     /// https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents.)
     type ScalarVar: Clone + Debug;
-    /// Variable representing a full-width element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
+    /// Variable representing a full-width element of the elliptic curve's
+    /// scalar field, to be used for fixed-base scalar mul.
     type ScalarFixed: Clone + Debug;
-    /// Variable representing a signed short element of the elliptic curve's scalar field, to be used for fixed-base scalar mul.
+    /// Variable representing a signed short element of the elliptic curve's
+    /// scalar field, to be used for fixed-base scalar mul.
     ///
     /// A `ScalarFixedShort` must be in the range [-(2^64 - 1), 2^64 - 1].
     type ScalarFixedShort: Clone + Debug;
     /// Variable representing an elliptic curve point.
     type Point: Clone + Debug;
-    /// Variable representing the x-coordinate of an elliptic curve point.
+    /// Variable representing the affine short Weierstrass x-coordinate of an
+    /// elliptic curve point.
     type X: Clone + Debug;
     /// Variable representing the set of fixed bases in the circuit.
     type FixedPoints: Clone + Debug;
-    /// Variable representing the set of fixed bases to be used in scalar multiplication
-    /// with a short signed exponent.
+    /// Variable representing the set of fixed bases to be used in scalar
+    /// multiplication with a short signed exponent.
     type FixedPointsShort: Clone + Debug;
     /// Variable representing a fixed elliptic curve point (constant in the circuit).
     type FixedPoint: Clone + Debug;
@@ -44,21 +47,24 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
     /// to be used in scalar multiplication with a short signed exponent.
     type FixedPointShort: Clone + Debug;
 
-    /// Witnesses the given base field element as a private input to the circuit for variable-base scalar mul.
+    /// Witnesses the given base field element as a private input to the circuit
+    /// for variable-base scalar mul.
     fn witness_scalar_var(
         &self,
         layouter: &mut impl Layouter<C::Base>,
         value: Option<C::Base>,
     ) -> Result<Self::ScalarVar, Error>;
 
-    /// Witnesses the given full-width scalar as a private input to the circuit for fixed-base scalar mul.
+    /// Witnesses the given full-width scalar as a private input to the circuit
+    /// for fixed-base scalar mul.
     fn witness_scalar_fixed(
         &self,
         layouter: &mut impl Layouter<C::Base>,
         value: Option<C::Scalar>,
     ) -> Result<Self::ScalarFixed, Error>;
 
-    /// Witnesses the given signed short scalar as a private input to the circuit for fixed-base scalar mul.
+    /// Witnesses the given signed short scalar as a private input to the circuit
+    /// for fixed-base scalar mul.
     fn witness_scalar_fixed_short(
         &self,
         layouter: &mut impl Layouter<C::Base>,

From af30f4b141879354c5f9285acf49642e970d34ba Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 16:12:06 +0800
Subject: [PATCH 09/11] Add Eq to the EccChip trait

---
 src/circuit/gadget/ecc.rs | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index dd0e1b47..6dcc6945 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -148,12 +148,12 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
 /// to be in the base field of the curve. (See non-normative notes in
 /// https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents.)
 #[derive(Debug)]
-pub struct ScalarVar<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct ScalarVar<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::ScalarVar,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarVar<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> ScalarVar<C, EccChip> {
     /// Constructs a new ScalarVar with the given value.
     pub fn new(
         chip: EccChip,
@@ -167,12 +167,12 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarVar<C, E
 
 /// A full-width element of the given elliptic curve's scalar field, to be used for fixed-base scalar mul.
 #[derive(Debug)]
-pub struct ScalarFixed<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct ScalarFixed<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::ScalarFixed,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixed<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> ScalarFixed<C, EccChip> {
     /// Constructs a new ScalarFixed with the given value.
     pub fn new(
         chip: EccChip,
@@ -186,12 +186,14 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixed<C,
 
 /// A signed short element of the given elliptic curve's scalar field, to be used for fixed-base scalar mul.
 #[derive(Debug)]
-pub struct ScalarFixedShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct ScalarFixedShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::ScalarFixedShort,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixedShort<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq>
+    ScalarFixedShort<C, EccChip>
+{
     /// Constructs a new ScalarFixedShort with the given value.
     ///
     /// # Panics
@@ -223,12 +225,12 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> ScalarFixedSho
 
 /// An elliptic curve point over the given curve.
 #[derive(Debug)]
-pub struct Point<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct Point<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::Point,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> Point<C, EccChip> {
     /// Constructs a new point with the given value.
     pub fn new(
         chip: EccChip,
@@ -251,7 +253,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccCh
 
     /// Returns `self + other` using complete addition.
     pub fn add(&self, mut layouter: impl Layouter<C::Base>, other: &Self) -> Result<Self, Error> {
-        assert_eq!(format!("{:?}", self.chip), format!("{:?}", other.chip));
+        assert_eq!(self.chip, other.chip);
         self.chip
             .add(&mut layouter, &self.inner, &other.inner)
             .map(|inner| Point {
@@ -266,7 +268,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccCh
         mut layouter: impl Layouter<C::Base>,
         other: &Self,
     ) -> Result<Self, Error> {
-        assert_eq!(format!("{:?}", self.chip), format!("{:?}", other.chip));
+        assert_eq!(self.chip, other.chip);
         self.chip
             .add_incomplete(&mut layouter, &self.inner, &other.inner)
             .map(|inner| Point {
@@ -293,12 +295,12 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> Point<C, EccCh
 
 /// The x-coordinate of an elliptic curve point over the given curve.
 #[derive(Debug)]
-pub struct X<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct X<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::X,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> X<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> X<C, EccChip> {
     /// Wraps the given x-coordinate (obtained directly from an instruction) in a gadget.
     pub fn from_inner(chip: EccChip, inner: EccChip::X) -> Self {
         X { chip, inner }
@@ -308,12 +310,12 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> X<C, EccChip>
 /// A constant elliptic curve point over the given curve, for which scalar multiplication
 /// is more efficient.
 #[derive(Clone, Debug)]
-pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::FixedPoint,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPoint<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> FixedPoint<C, EccChip> {
     /// Gets a reference to the specified fixed point in the circuit.
     pub fn get(chip: EccChip, point: EccChip::FixedPoints) -> Result<Self, Error> {
         chip.get_fixed(point)
@@ -339,12 +341,12 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPoint<C,
 /// A constant elliptic curve point over the given curve, used in scalar multiplication
 /// with a short signed exponent
 #[derive(Clone, Debug)]
-pub struct FixedPointShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> {
+pub struct FixedPointShort<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
     inner: EccChip::FixedPointShort,
 }
 
-impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug> FixedPointShort<C, EccChip> {
+impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> FixedPointShort<C, EccChip> {
     /// Gets a reference to the specified fixed point in the circuit.
     pub fn get(chip: EccChip, point: EccChip::FixedPointsShort) -> Result<Self, Error> {
         chip.get_fixed_short(point)

From 2962115aef13bd8612acecba08f2b0709fed03f2 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 16:45:43 +0800
Subject: [PATCH 10/11] Reintroduce point doubling API

---
 src/circuit/gadget/ecc.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index 6dcc6945..ea26dcb8 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -111,6 +111,13 @@ pub trait EccInstructions<C: CurveAffine>: Chip<C::Base> {
         b: &Self::Point,
     ) -> Result<Self::Point, Error>;
 
+    /// Performs point doubling, returning `[2] a`.
+    fn double(
+        &self,
+        layouter: &mut impl Layouter<C::Base>,
+        a: &Self::Point,
+    ) -> Result<Self::Point, Error>;
+
     /// Performs variable-base scalar multiplication, returning `[scalar] base`.
     fn mul(
         &self,
@@ -293,7 +300,8 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> Point<C,
     }
 }
 
-/// The x-coordinate of an elliptic curve point over the given curve.
+/// The affine short Weierstrass x-coordinate of an elliptic curve point over the
+/// given curve.
 #[derive(Debug)]
 pub struct X<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,

From ff504c1a3f0adb8d2b9ae77136c86aefd75ffd71 Mon Sep 17 00:00:00 2001
From: therealyingtong <yingtong@z.cash>
Date: Tue, 18 May 2021 17:14:13 +0800
Subject: [PATCH 11/11] Address review comments.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
---
 src/circuit/gadget/ecc.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/circuit/gadget/ecc.rs b/src/circuit/gadget/ecc.rs
index ea26dcb8..31d252e3 100644
--- a/src/circuit/gadget/ecc.rs
+++ b/src/circuit/gadget/ecc.rs
@@ -290,7 +290,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> Point<C,
         mut layouter: impl Layouter<C::Base>,
         by: &ScalarVar<C, EccChip>,
     ) -> Result<Self, Error> {
-        assert_eq!(format!("{:?}", self.chip), format!("{:?}", by.chip));
+        assert_eq!(self.chip, by.chip);
         self.chip
             .mul(&mut layouter, &by.inner, &self.inner)
             .map(|inner| Point {
@@ -315,8 +315,8 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> X<C, EccC
     }
 }
 
-/// A constant elliptic curve point over the given curve, for which scalar multiplication
-/// is more efficient.
+/// A constant elliptic curve point over the given curve, for which window tables have
+/// been provided to make scalar multiplication more efficient.
 #[derive(Clone, Debug)]
 pub struct FixedPoint<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> {
     chip: EccChip,
@@ -336,7 +336,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> FixedPoin
         mut layouter: impl Layouter<C::Base>,
         by: &ScalarFixed<C, EccChip>,
     ) -> Result<Point<C, EccChip>, Error> {
-        assert_eq!(format!("{:?}", self.chip), format!("{:?}", by.chip));
+        assert_eq!(self.chip, by.chip);
         self.chip
             .mul_fixed(&mut layouter, &by.inner, &self.inner)
             .map(|inner| Point {
@@ -367,7 +367,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> FixedPoin
         mut layouter: impl Layouter<C::Base>,
         by: &ScalarFixedShort<C, EccChip>,
     ) -> Result<Point<C, EccChip>, Error> {
-        assert_eq!(format!("{:?}", self.chip), format!("{:?}", by.chip));
+        assert_eq!(self.chip, by.chip);
         self.chip
             .mul_fixed_short(&mut layouter, &by.inner, &self.inner)
             .map(|inner| Point {