Take into account code review for ZSA features

halo2_gadgets/CHANGELOG.md

@@ -21,7 +21,7 @@ and this project adheres to Rust's notion of
   instead of directly taking a `LookupRangeCheckConfig<F, K>` reference
 - `halo2_gadgets::sinsemilla::merkle::chip::MerkleConfig::cond_swap_config` is now public
 - `halo2_gadgets::sinsemilla::chip::SinsemillaChip::configure` has a new input
-  `init_from_private_point` to enable the evaluation of Sinsemilla hash from a private point.
+  `allow_init_from_private_point` to enable the evaluation of Sinsemilla hash from a private point.
 
 ## [0.3.1] - 2024-12-16
 - `halo2_gadgets::poseidon::primitives` is now a re-export of the new `halo2_poseidon`

halo2_gadgets/src/ecc.rs

@@ -120,8 +120,8 @@ pub trait EccInstructions<C: CurveAffine>:
         b: &B,
     ) -> Result<Self::Point, Error>;
 
-    /// Performs variable-base sign-scalar multiplication, returning `[sign] point`
-    /// `sign` must be in {-1, 1}.
+    /// Performs variable-base sign-scalar multiplication, returning `[sign] point`.
+    /// This constrains `sign` to be in {-1, 1}.
     fn mul_sign(
         &self,
         layouter: &mut impl Layouter<C::Base>,
@@ -463,7 +463,7 @@ impl<C: CurveAffine, EccChip: EccInstructions<C> + Clone + Debug + Eq> Point<C,
     }
 
     /// Returns `[sign] self`.
-    /// `sign` must be in {-1, 1}.
+    /// This constrains `sign` to be in {-1, 1}.
     pub fn mul_sign(
         &self,
         mut layouter: impl Layouter<C::Base>,

halo2_gadgets/src/ecc/chip.rs

@@ -560,7 +560,7 @@
     }
 
     /// Performs variable-base sign-scalar multiplication, returning `[sign] point`.
-    /// `sign` must be in {-1, 1}.
+    /// This constrains `sign` to be in {-1, 1}.
     fn mul_sign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
@@ -604,7 +604,7 @@
     ) -> Result<(Self::Point, Self::ScalarFixed), Error> {
         let config = self.config().mul_fixed_full.clone();
         config.assign(
             layouter.namespace(|| format!("fixed-base mul of {:?}", base)),
             scalar,
             base,
         )
@@ -618,7 +618,7 @@
     ) -> Result<(Self::Point, Self::ScalarFixedShort), Error> {
         let config = self.config().mul_fixed_short.clone();
         config.assign(
             layouter.namespace(|| format!("short fixed-base mul of {:?}", base)),
             scalar,
             base,
         )
@@ -632,7 +632,7 @@
     ) -> Result<Self::Point, Error> {
         let config = self.config().mul_fixed_base_field.clone();
         config.assign(
             layouter.namespace(|| format!("base-field elem fixed-base mul of {:?}", base)),
             base_field_elem,
             base,
         )

halo2_gadgets/src/ecc/chip/mul_fixed/short.rs

@@ -243,7 +243,7 @@
     }
 
     /// Multiply the point by sign, using the q_mul_fixed_short gate.
-    /// Constraints `sign` in {-1, 1}
+    /// This constrains `sign` to be in {-1, 1}.
     pub(crate) fn assign_scalar_sign(
         &self,
         mut layouter: impl Layouter<pallas::Base>,
@@ -576,11 +576,11 @@
             "-1".into()
         } else {
             // Format value as hex.
             let s = format!("{:?}", v);
             // Remove leading zeroes.
             let s = s.strip_prefix("0x").unwrap();
             let s = s.trim_start_matches('0');
             format!("0x{}", s)
         }
     }
 

halo2_gadgets/src/sinsemilla.rs

@@ -147,7 +147,7 @@
                 |(i, piece)| -> Result<MessagePiece<C, SinsemillaChip, K, MAX_WORDS>, Error> {
                     MessagePiece::from_bitstring(
                         chip.clone(),
                         layouter.namespace(|| format!("message piece {}", i)),
                         piece,
                     )
                 },
@@ -606,7 +606,7 @@
 
     fn configure<Lookup: PallasLookupRangeCheck>(
         meta: &mut ConstraintSystem<pallas::Base>,
-        init_from_private_point: bool,
+        allow_init_from_private_point: bool,
     ) -> EccSinsemillaConfig<Lookup> {
         let advices = [
             meta.advice_column(),
@@ -660,7 +660,7 @@
             lagrange_coeffs[0],
             lookup,
             range_check,
-            init_from_private_point,
+            allow_init_from_private_point,
         );
         let config2 = SinsemillaChip::configure(
             meta,
@@ -669,7 +669,7 @@
             lagrange_coeffs[1],
             lookup,
             range_check,
-            init_from_private_point,
+            allow_init_from_private_point,
         );
         (ecc_config, config1, config2)
     }
@@ -735,8 +735,8 @@
                         let point = merkle_crh
                             .hash_to_point(
                                 l.into_iter()
                                     .chain(left.into_iter())
                                     .chain(right.into_iter()),
                             )
                             .unwrap();
                         point.to_affine()

halo2_gadgets/src/sinsemilla/chip.rs

@@ -30,8 +30,8 @@ mod hash_to_point;
 
 /// Configuration for the Sinsemilla hash chip
 ///
-/// If `init_from_private_point` is true, the chip can compute a hash from a private point.
-/// However, compared to when `init_from_private_point` is set to false,
+/// If `allow_init_from_private_point` is true, the chip can compute a hash from a private point.
+/// However, compared to when `allow_init_from_private_point` is set to false,
 /// computing the hash from a public point will take one additional row.
 #[derive(Eq, PartialEq, Clone, Debug)]
 pub struct SinsemillaConfig<Hash, Commit, F, Lookup = PallasLookupRangeCheckConfig>
@@ -66,7 +66,7 @@ where
     lookup_config: Lookup,
 
     /// If true, it is possible to compute a hash from a private point.
-    init_from_private_point: bool,
+    allow_init_from_private_point: bool,
 
     _marker: PhantomData<(Hash, Commit, F)>,
 }
@@ -158,8 +158,8 @@ where
 
     /// Creates the Sinsemilla chip
     ///
-    /// If `init_from_private_point` is true, the chip can compute a hash from a private point.
-    /// However, compared to when `init_from_private_point` is set to false,
+    /// If `allow_init_from_private_point` is true, the chip can compute a hash from a private point.
+    /// However, compared to when `allow_init_from_private_point` is set to false,
     /// computing the hash from a public point will take one additional row.
     ///
     /// # Side-effects
@@ -174,7 +174,7 @@ where
         fixed_y_q: Column<Fixed>,
         lookup: (TableColumn, TableColumn, TableColumn),
         range_check: Lookup,
-        init_from_private_point: bool,
+        allow_init_from_private_point: bool,
     ) -> <Self as Chip<pallas::Base>>::Config {
         // Enable equality on all advice columns
         for advice in advices.iter() {
@@ -200,7 +200,7 @@ where
                 table_y: lookup.2,
             },
             lookup_config: range_check,
-            init_from_private_point,
+            allow_init_from_private_point,
             _marker: PhantomData,
         };
 
@@ -224,7 +224,7 @@ where
         // https://p.z.cash/halo2-0.1:sinsemilla-constraints?partial
         meta.create_gate("Initial y_Q", |meta| {
             let q_s4 = meta.query_selector(config.q_sinsemilla4);
-            let y_q = if init_from_private_point {
+            let y_q = if allow_init_from_private_point {
                 meta.query_advice(config.double_and_add.x_p, Rotation::prev())
             } else {
                 meta.query_fixed(config.fixed_y_q)
@@ -356,7 +356,7 @@ where
         Q: &Self::NonIdentityPoint,
         message: Self::Message,
     ) -> Result<(Self::NonIdentityPoint, Vec<Self::RunningSum>), Error> {
-        if !self.config().init_from_private_point {
+        if !self.config().allow_init_from_private_point {
             return Err(Error::IllegalHashFromPrivatePoint);
         }
 

halo2_gadgets/src/sinsemilla/chip/hash_to_point.rs

@@ -85,7 +85,7 @@ where
         ),
         Error,
     > {
-        if !self.config().init_from_private_point {
+        if !self.config().allow_init_from_private_point {
             return Err(Error::IllegalHashFromPrivatePoint);
         }
 
@@ -97,8 +97,9 @@ where
         self.check_hash_result(EccPointQ::PrivatePoint(Q.clone()), message, &x_a, &y_a);
 
         x_a.value()
-            .zip(y_a.value())
-            .error_if_known_and(|(x_a, y_a)| x_a.is_zero_vartime() || y_a.is_zero_vartime())?;
+            .error_if_known_and(|x_a| x_a.is_zero_vartime())?;
+        y_a.value()
+            .error_if_known_and(|y_a| y_a.is_zero_vartime())?;
         Ok((
             NonIdentityEccPoint::from_coordinates_unchecked(x_a.0, y_a),
             zs_sum,
@@ -108,12 +109,12 @@ where
     #[allow(non_snake_case)]
     /// Assign the coordinates of the initial public point `Q`.
     ///
-    /// If init_from_private_point is not set,
+    /// If allow_init_from_private_point is not set,
     /// | offset | x_A | q_sinsemilla4 | fixed_y_q |
     /// --------------------------------------
     /// |   0    | x_Q |   1           |   y_Q     |
     ///
-    /// If init_from_private_point is set,
+    /// If allow_init_from_private_point is set,
     /// | offset | x_A | x_P | q_sinsemilla4 |
     /// --------------------------------------
     /// |   0    |     | y_Q |               |
@@ -132,12 +133,12 @@ where
 
         // Constrain the initial x_a, lambda_1, lambda_2, x_p using the q_sinsemilla4
         // selector.
-        let y_a: Y<pallas::Base> = if config.init_from_private_point {
+        let y_a: Y<pallas::Base> = if config.allow_init_from_private_point {
             // Enable `q_sinsemilla4` on the second row.
-            config.q_sinsemilla4.enable(region, 1)?;
+            config.q_sinsemilla4.enable(region, offset + 1)?;
             let y_a: AssignedCell<Assigned<pallas::Base>, pallas::Base> = region
                 .assign_advice_from_constant(
-                    || "fixed y_q",
+                    || "variable y_q",
                     config.double_and_add.x_p,
                     offset,
                     y_q.into(),
@@ -160,7 +161,7 @@ where
         // Constrain the initial x_q to equal the x-coordinate of the domain's `Q`.
         let x_a: X<pallas::Base> = {
             let x_a = region.assign_advice_from_constant(
-                || "fixed x_q",
+                || "variable x_q",
                 config.double_and_add.x_a,
                 offset,
                 x_q.into(),
@@ -186,7 +187,7 @@ where
     ) -> Result<(usize, X<pallas::Base>, Y<pallas::Base>), Error> {
         let config = self.config().clone();
 
-        if !config.init_from_private_point {
+        if !config.allow_init_from_private_point {
             return Err(Error::IllegalHashFromPrivatePoint);
         }
 

halo2_gadgets/src/sinsemilla/merkle.rs

@@ -158,7 +158,7 @@
             // Compute the node in layer l from its children:
             //     M^l_i = MerkleCRH(l, M^{l+1}_{2i}, M^{l+1}_{2i+1})
             node = chip.hash_layer(
                 layouter.namespace(|| format!("MerkleCRH({}, left, right)", l)),
                 Q,
                 l,
                 pair.0,
@@ -235,7 +235,7 @@
     #[allow(clippy::type_complexity)]
     fn configure<Lookup: PallasLookupRangeCheck>(
         meta: &mut ConstraintSystem<pallas::Base>,
-        init_from_private_point: bool,
+        allow_init_from_private_point: bool,
     ) -> (
         MerkleConfig<TestHashDomain, TestCommitDomain, TestFixedBases, Lookup>,
         MerkleConfig<TestHashDomain, TestCommitDomain, TestFixedBases, Lookup>,
@@ -278,7 +278,7 @@
             fixed_y_q_1,
             lookup,
             range_check,
-            init_from_private_point,
+            allow_init_from_private_point,
         );
         let config1 = MerkleChip::configure(meta, sinsemilla_config_1);
 
@@ -289,7 +289,7 @@
             fixed_y_q_2,
             lookup,
             range_check,
-            init_from_private_point,
+            allow_init_from_private_point,
         );
         let config2 = MerkleChip::configure(meta, sinsemilla_config_2);
 

halo2_gadgets/src/utilities/cond_swap.rs

@@ -1,4 +1,4 @@
-//! Gadget and chip for a conditional swap utility.
+//! Gadget and chip for conditional swap and mux utilities.
 
 use super::{bool_check, ternary, UtilitiesInstructions};
 
@@ -12,7 +12,7 @@ use halo2_proofs::{
 use pasta_curves::pallas;
 use std::marker::PhantomData;
 
-/// Instructions for a conditional swap gadget.
+/// Instructions for conditional swap and mux gadgets.
 pub trait CondSwapInstructions<F: Field>: UtilitiesInstructions<F> {
     #[allow(clippy::type_complexity)]
     /// Given an input pair (a,b) and a `swap` boolean flag, returns

halo2_gadgets/src/utilities/lookup_range_check.rs

@@ -53,7 +53,7 @@
             .map(|inner| Self {
                 inner,
                 num_bits,
                 _phantom: PhantomData::default(),
             })
     }
 }
@@ -129,7 +129,7 @@
         strict: bool,
     ) -> Result<RunningSum<F>, Error> {
         layouter.assign_region(
             || format!("{:?} words range check", num_words),
             |mut region| {
                 // Copy `element` and initialize running sum `z_0 = element` to decompose it.
                 let z_0 =
@@ -253,7 +253,7 @@
     ) -> Result<(), Error> {
         assert!(num_bits < K);
         layouter.assign_region(
             || format!("Range check {:?} bits", num_bits),
             |mut region| {
                 // Copy `element` to use in the k-bit lookup.
                 let element =
@@ -276,7 +276,7 @@
     ) -> Result<AssignedCell<F, F>, Error> {
         assert!(num_bits <= K);
         layouter.assign_region(
             || format!("Range check {:?} bits", num_bits),
             |mut region| {
                 // Witness `element` to use in the k-bit lookup.
                 let element = region.assign_advice(
@@ -471,7 +471,7 @@
         let shifted = element.value().into_field() * F::from(1 << (K - num_bits));
 
         region.assign_advice(
             || format!("element * 2^({}-{})", K, num_bits),
             self.running_sum,
             1,
             || shifted,
@@ -480,7 +480,7 @@
         // Assign 2^{-num_bits} from a fixed column.
         let inv_two_pow_s = F::from(1 << num_bits).invert().unwrap();
         region.assign_advice_from_constant(
             || format!("2^(-{})", num_bits),
             self.running_sum,
             2,
             inv_two_pow_s,
@@ -519,7 +519,7 @@
     /// a value in `table_idx` constrains it to be within this range. The table
     /// can be loaded outside this helper.
     ///
-    /// # Side-effects
+    /// # Side effects
     ///
     /// Both the `running_sum` and `constants` columns will be equality-enabled.
     pub fn configure_with_tag(

halo2_proofs/src/plonk/error.rs

@@ -40,7 +40,7 @@
     ColumnNotInPermutation(Column<Any>),
     /// An error relating to a lookup table.
     TableError(TableError),
-    /// Try to hash from a private point when init_from_private_point is not set.
+    /// Try to hash from a private point when allow_init_from_private_point is not set.
     IllegalHashFromPrivatePoint,
 }
 
@@ -66,12 +66,12 @@
             Error::ConstraintSystemFailure => write!(f, "The constraint system is not satisfied"),
             Error::BoundsFailure => write!(f, "An out-of-bounds index was passed to the backend"),
             Error::Opening => write!(f, "Multi-opening proof was invalid"),
             Error::Transcript(e) => write!(f, "Transcript error: {}", e),
             Error::NotEnoughRowsAvailable { current_k } => write!(
                 f,
                 "k = {} is too small for the given circuit. Try using a larger value of k",
                 current_k,
             ),
             Error::InstanceTooLarge => write!(f, "Instance vectors are larger than the circuit"),
             Error::NotEnoughColumnsForConstants => {
                 write!(
@@ -79,12 +79,12 @@
                     "Too few fixed columns are enabled for global constants usage"
                 )
             }
             Error::ColumnNotInPermutation(column) => write!(
                 f,
                 "Column {:?} must be included in the permutation. Help: try applying `meta.enable_equalty` on the column",
                 column
             ),
             Error::TableError(error) => write!(f, "{}", error),
             Error::IllegalHashFromPrivatePoint =>  write!(
                 f,
                 "Hashing from private point is disabled"
@@ -119,26 +119,26 @@
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             TableError::ColumnNotAssigned(col) => {
                 write!(
                     f,
                     "{:?} not fully assigned. Help: assign a value at offset 0.",
                     col
                 )
             }
             TableError::UnevenColumnLengths((col, col_len), (table, table_len)) => write!(
                 f,
                 "{:?} has length {} while {:?} has length {}",
                 col, col_len, table, table_len
             ),
             TableError::UsedColumn(col) => {
                 write!(f, "{:?} has already been used", col)
             }
             TableError::OverwriteDefault(col, default, val) => {
                 write!(
                     f,
                     "Attempted to overwrite default value {} with {} in {:?}",
                     default, val, col
                 )
             }
         }
     }