Address review comments

Co-authored-by: Jack Grigg <jack@electriccoin.co>

Author: therealyingtong

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,
@@ -9,31 +9,50 @@ use halo2::{
 };
 
 /// Trait allowing circuit's fixed points to be enumerated.
-pub trait FixedPoints<C: CurveAffine>: Clone + fmt::Debug {}
+pub trait FixedPoints<C: CurveAffine>: Clone + 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>;
     /// 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,19 +69,21 @@ 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,
         b: &Self::Point,
     ) -> 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 +94,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,70 +115,77 @@ 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,
+}
+
 /// 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,
         mut layouter: impl Layouter<C::Base>,
         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,
         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,
-            })
+            .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 +200,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 +210,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 +227,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 +242,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 +260,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 {