From 2fc9dc814dbd95675f8fadd5ab52535e1d509435 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 10:26:25 +0000
Subject: [PATCH 01/21] book: Fix TODOs on Actions design page

Closes zcash/orchard#78.
---
 book/src/design/actions.md | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/book/src/design/actions.md b/book/src/design/actions.md
index 3da38a95..5bbcec98 100644
--- a/book/src/design/actions.md
+++ b/book/src/design/actions.md
@@ -18,10 +18,9 @@ bundle of actions, where each action is both a spend and an output. This provide
 inherent arity-hiding as multi-JoinSplit Sprout, but using Sapling value commitments to
 balance the transaction without doubling its size.
 
-TODO: Depending on the circuit cost, we _may_ switch to having an action internally
-represent either a spend or an output. Externally spends and outputs would still be
-indistinguishable, but the transaction would be larger.
-
 ## Memo fields
 
-TODO: One memo per tx vs one memo per output
+Each Orchard action has a memo field for its corresponding output, as with Sprout and
+Sapling. We did at one point consider having a single Orchard memo field per transaction,
+and/or having a mechanism for enabling multiple recipients to decrypt the same memo, but
+these were decided against in order to keep the overall design simpler.

From f08a2a35c4d05c877656f09f572ec967bfbc8a8d Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 11:18:42 +0000
Subject: [PATCH 02/21] Rename `ak` to `ak_P` in the circuit implementation

Closes zcash/orchard#260.
---
 src/circuit.rs | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 873d2a71..a58a8e81 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -320,6 +320,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         }
     }
 
+    #[allow(non_snake_case)]
     fn synthesize(
         &self,
         config: Self::Config,
@@ -332,7 +333,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         let ecc_chip = config.ecc_chip();
 
         // Witness private inputs that are used across multiple checks.
-        let (psi_old, rho_old, cm_old, g_d_old, ak, nk, v_old, v_new) = {
+        let (psi_old, rho_old, cm_old, g_d_old, ak_P, nk, v_old, v_new) = {
             // Witness psi_old
             let psi_old = self.load_private(
                 layouter.namespace(|| "witness psi_old"),
@@ -361,12 +362,12 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 self.g_d_old.as_ref().map(|gd| gd.to_affine()),
             )?;
 
-            // Witness ak.
-            let ak: Option<pallas::Point> = self.ak.as_ref().map(|ak| ak.into());
-            let ak = NonIdentityPoint::new(
+            // Witness ak_P.
+            let ak_P: Option<pallas::Point> = self.ak.as_ref().map(|ak| ak.into());
+            let ak_P = NonIdentityPoint::new(
                 ecc_chip.clone(),
-                layouter.namespace(|| "ak"),
-                ak.map(|ak| ak.to_affine()),
+                layouter.namespace(|| "witness ak_P"),
+                ak_P.map(|ak_P| ak_P.to_affine()),
             )?;
 
             // Witness nk.
@@ -390,7 +391,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 self.v_new.map(|v_new| pallas::Base::from(v_new.inner())),
             )?;
 
-            (psi_old, rho_old, cm_old, g_d_old, ak, nk, v_old, v_new)
+            (psi_old, rho_old, cm_old, g_d_old, ak_P, nk, v_old, v_new)
         };
 
         // Merkle path validity check.
@@ -546,8 +547,8 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 spend_auth_g.mul(layouter.namespace(|| "[alpha] SpendAuthG"), self.alpha)?
             };
 
-            // [alpha] SpendAuthG + ak
-            let rk = alpha_commitment.add(layouter.namespace(|| "rk"), &ak)?;
+            // [alpha] SpendAuthG + ak_P
+            let rk = alpha_commitment.add(layouter.namespace(|| "rk"), &ak_P)?;
 
             // Constrain rk to equal public input
             layouter.constrain_instance(rk.inner().x().cell(), config.primary, RK_X)?;
@@ -559,13 +560,14 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             let commit_ivk_config = config.commit_ivk_config.clone();
 
             let ivk = {
+                let ak = ak_P.extract_p().inner().clone();
                 let rivk = self.rivk.map(|rivk| rivk.inner());
 
                 commit_ivk_config.assign_region(
                     config.sinsemilla_chip_1(),
                     ecc_chip.clone(),
                     layouter.namespace(|| "CommitIvk"),
-                    ak.extract_p().inner().clone(),
+                    ak,
                     nk,
                     rivk,
                 )?

From ae6a50611aff6e8e12c48627ff39a2d47d117e69 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 12:44:57 +0000
Subject: [PATCH 03/21] Pass `g_d_new` and `pk_d_new` directly to `Circuit`

The initial Action circuit specification indicated that only the byte
encodings of `g_d_new` and `pk_d_new` would be witnessed, but we ended
up witnessing the points directly instead. This commit removes the
leftover (and now redundant) encoding-decoding round trip.
---
 src/builder.rs |  5 ++---
 src/circuit.rs | 25 ++++++++++---------------
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/src/builder.rs b/src/builder.rs
index f85d08a2..77520302 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -4,7 +4,6 @@ use core::fmt;
 use core::iter;
 
 use ff::Field;
-use group::GroupEncoding;
 use nonempty::NonEmpty;
 use pasta_curves::pallas;
 use rand::{prelude::SliceRandom, CryptoRng, RngCore};
@@ -197,8 +196,8 @@ impl ActionInfo {
                 ak: Some(ak),
                 nk: Some(*self.spend.fvk.nk()),
                 rivk: Some(self.spend.fvk.rivk(self.spend.scope)),
-                g_d_new_star: Some((*note.recipient().g_d()).to_bytes()),
-                pk_d_new_star: Some(note.recipient().pk_d().to_bytes()),
+                g_d_new: Some(note.recipient().g_d()),
+                pk_d_new: Some(*note.recipient().pk_d()),
                 v_new: Some(note.value()),
                 psi_new: Some(note.rseed().psi(&note.rho())),
                 rcm_new: Some(note.rseed().rcm(&note.rho())),
diff --git a/src/circuit.rs b/src/circuit.rs
index a58a8e81..ff021786 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -108,8 +108,8 @@ pub struct Circuit {
     pub(crate) ak: Option<SpendValidatingKey>,
     pub(crate) nk: Option<NullifierDerivingKey>,
     pub(crate) rivk: Option<CommitIvkRandomness>,
-    pub(crate) g_d_new_star: Option<[u8; 32]>,
-    pub(crate) pk_d_new_star: Option<[u8; 32]>,
+    pub(crate) g_d_new: Option<NonIdentityPallasPoint>,
+    pub(crate) pk_d_new: Option<DiversifiedTransmissionKey>,
     pub(crate) v_new: Option<NoteValue>,
     pub(crate) psi_new: Option<pallas::Base>,
     pub(crate) rcm_new: Option<NoteCommitTrapdoor>,
@@ -619,11 +619,9 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         {
             let new_note_commit_config = config.new_note_commit_config.clone();
 
-            // Witness g_d_new_star
+            // Witness g_d_new
             let g_d_new = {
-                let g_d_new = self
-                    .g_d_new_star
-                    .map(|bytes| pallas::Affine::from_bytes(&bytes).unwrap());
+                let g_d_new = self.g_d_new.map(|g_d_new| g_d_new.to_affine());
                 NonIdentityPoint::new(
                     ecc_chip.clone(),
                     layouter.namespace(|| "witness g_d_new_star"),
@@ -631,11 +629,9 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 )?
             };
 
-            // Witness pk_d_new_star
+            // Witness pk_d_new
             let pk_d_new = {
-                let pk_d_new = self
-                    .pk_d_new_star
-                    .map(|bytes| pallas::Affine::from_bytes(&bytes).unwrap());
+                let pk_d_new = self.pk_d_new.map(|pk_d_new| pk_d_new.inner().to_affine());
                 NonIdentityPoint::new(
                     ecc_chip,
                     layouter.namespace(|| "witness pk_d_new"),
@@ -907,7 +903,6 @@ mod tests {
     use core::iter;
 
     use ff::Field;
-    use group::GroupEncoding;
     use halo2_proofs::dev::MockProver;
     use pasta_curves::pallas;
     use rand::{rngs::OsRng, RngCore};
@@ -956,8 +951,8 @@ mod tests {
                 ak: Some(ak),
                 nk: Some(nk),
                 rivk: Some(rivk),
-                g_d_new_star: Some((*output_note.recipient().g_d()).to_bytes()),
-                pk_d_new_star: Some(output_note.recipient().pk_d().to_bytes()),
+                g_d_new: Some(output_note.recipient().g_d()),
+                pk_d_new: Some(*output_note.recipient().pk_d()),
                 v_new: Some(output_note.value()),
                 psi_new: Some(output_note.rseed().psi(&output_note.rho())),
                 rcm_new: Some(output_note.rseed().rcm(&output_note.rho())),
@@ -1142,8 +1137,8 @@ mod tests {
             ak: None,
             nk: None,
             rivk: None,
-            g_d_new_star: None,
-            pk_d_new_star: None,
+            g_d_new: None,
+            pk_d_new: None,
             v_new: None,
             psi_new: None,
             rcm_new: None,

From 714f2e71593585fa3791aca2f1fcd27cab1dc661 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 13:23:01 +0000
Subject: [PATCH 04/21] Use `array::map` now that our MSRV supports it

---
 src/circuit.rs | 11 +++++------
 src/tree.rs    | 13 +++----------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index ff021786..ddde11f9 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -20,8 +20,8 @@ use self::commit_ivk::CommitIvkConfig;
 use self::note_commit::NoteCommitConfig;
 use crate::{
     constants::{
-        util::gen_const_array, NullifierK, OrchardCommitDomains, OrchardFixedBases,
-        OrchardFixedBasesFull, OrchardHashDomains, ValueCommitV, MERKLE_DEPTH_ORCHARD,
+        NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull,
+        OrchardHashDomains, ValueCommitV, MERKLE_DEPTH_ORCHARD,
     },
     keys::{
         CommitIvkRandomness, DiversifiedTransmissionKey, NullifierDerivingKey, SpendValidatingKey,
@@ -396,10 +396,9 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // Merkle path validity check.
         let anchor = {
-            let path: Option<[pallas::Base; MERKLE_DEPTH_ORCHARD]> = self.path.map(|typed_path| {
-                // TODO: Replace with array::map once MSRV is 1.55.0.
-                gen_const_array(|i| typed_path[i].inner())
-            });
+            let path = self
+                .path
+                .map(|typed_path| typed_path.map(|node| node.inner()));
             let merkle_inputs = MerklePath::construct(
                 config.merkle_chip_1(),
                 config.merkle_chip_2(),
diff --git a/src/tree.rs b/src/tree.rs
index 92ca0344..d9f4c6f5 100644
--- a/src/tree.rs
+++ b/src/tree.rs
@@ -5,7 +5,6 @@ use core::iter;
 use crate::{
     constants::{
         sinsemilla::{i2lebsp_k, L_ORCHARD_MERKLE, MERKLE_CRH_PERSONALIZATION},
-        util::gen_const_array_with_default,
         MERKLE_DEPTH_ORCHARD,
     },
     note::commitment::ExtractedNoteCommitment,
@@ -100,20 +99,14 @@ impl MerklePath {
     pub(crate) fn dummy(mut rng: &mut impl RngCore) -> Self {
         MerklePath {
             position: rng.next_u32(),
-            auth_path: gen_const_array_with_default(MerkleHashOrchard::empty_leaf(), |_| {
-                MerkleHashOrchard(pallas::Base::random(&mut rng))
-            }),
+            auth_path: [(); MERKLE_DEPTH_ORCHARD]
+                .map(|_| MerkleHashOrchard(pallas::Base::random(&mut rng))),
         }
     }
 
     /// Instantiates a new Merkle path given a leaf position and authentication path.
     pub(crate) fn new(position: u32, auth_path: [pallas::Base; MERKLE_DEPTH_ORCHARD]) -> Self {
-        Self::from_parts(
-            position,
-            gen_const_array_with_default(MerkleHashOrchard::empty_leaf(), |i| {
-                MerkleHashOrchard(auth_path[i])
-            }),
-        )
+        Self::from_parts(position, auth_path.map(MerkleHashOrchard))
     }
 
     /// Instantiates a new Merkle path given a leaf position and authentication path.

From 70b6eb362309434c99839c19decea86685bb1db8 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 13:54:42 +0000
Subject: [PATCH 05/21] Simplify witness synthesis for `v_net`

`NoteValue - NoteValue` is always guaranteed to produce a valid
`ValueSum`, so we make that infallible and introduce a new helper method
`ValueSum::magnitude_sign` that we use for circuit synthesis.
---
 CHANGELOG.md   |  3 +++
 src/action.rs  |  4 ++--
 src/builder.rs |  6 +++---
 src/bundle.rs  |  4 ++--
 src/circuit.rs | 33 ++++++++++++++-------------------
 src/value.rs   | 26 +++++++++++++++++++++++---
 6 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cb08c67a..d1d45bc4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,9 @@ and this project adheres to Rust's notion of
     `Bundle::{try_}map_authorization`.
   - `Flags::from_byte` now returns `Option<Flags>` instead of
     `io::Result<Flags>`.
+- `impl Sub for orchard::value::NoteValue` now returns `ValueSum` instead of
+  `Option<ValueSum>`, as the result is guaranteed to be within the valid range
+  of `ValueSum`.
 
 ## [0.1.0-beta.3] - 2022-04-06
 ### Added
diff --git a/src/action.rs b/src/action.rs
index f02d9e1e..d0b73f23 100644
--- a/src/action.rs
+++ b/src/action.rs
@@ -149,7 +149,7 @@ pub(crate) mod testing {
         ) -> Action<()> {
             let cmx = ExtractedNoteCommitment::from(note.commitment());
             let cv_net = ValueCommitment::derive(
-                (spend_value - output_value).unwrap(),
+                spend_value - output_value,
                 ValueCommitTrapdoor::zero()
             );
             // FIXME: make a real one from the note.
@@ -180,7 +180,7 @@ pub(crate) mod testing {
         ) -> Action<redpallas::Signature<SpendAuth>> {
             let cmx = ExtractedNoteCommitment::from(note.commitment());
             let cv_net = ValueCommitment::derive(
-                (spend_value - output_value).unwrap(),
+                spend_value - output_value,
                 ValueCommitTrapdoor::zero()
             );
 
diff --git a/src/builder.rs b/src/builder.rs
index 77520302..f6e8ff6d 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -124,7 +124,7 @@ impl ActionInfo {
     }
 
     /// Returns the value sum for this action.
-    fn value_sum(&self) -> Option<ValueSum> {
+    fn value_sum(&self) -> ValueSum {
         self.spend.note.value() - self.output.value
     }
 
@@ -134,7 +134,7 @@ impl ActionInfo {
     ///
     /// [orchardsend]: https://zips.z.cash/protocol/nu5.pdf#orchardsend
     fn build(self, mut rng: impl RngCore) -> (Action<SigningMetadata>, Circuit) {
-        let v_net = self.value_sum().expect("already checked this");
+        let v_net = self.value_sum();
         let cv_net = ValueCommitment::derive(v_net, self.rcv.clone());
 
         let nf_old = self.spend.note.nullifier(&self.spend.fvk);
@@ -335,7 +335,7 @@ impl Builder {
         let value_balance = pre_actions
             .iter()
             .fold(Some(ValueSum::zero()), |acc, action| {
-                acc? + action.value_sum()?
+                acc? + action.value_sum()
             })
             .ok_or(OverflowError)?;
 
diff --git a/src/bundle.rs b/src/bundle.rs
index 76f29af5..5421dadc 100644
--- a/src/bundle.rs
+++ b/src/bundle.rs
@@ -522,7 +522,7 @@ pub mod testing {
 
             output_value_gen.prop_flat_map(move |output_value| {
                 arb_unauthorized_action(spend_value, output_value)
-                    .prop_map(move |a| ((spend_value - output_value).unwrap(), a))
+                    .prop_map(move |a| (spend_value - output_value, a))
             })
         })
     }
@@ -547,7 +547,7 @@ pub mod testing {
 
             output_value_gen.prop_flat_map(move |output_value| {
                 arb_action(spend_value, output_value)
-                    .prop_map(move |a| ((spend_value - output_value).unwrap(), a))
+                    .prop_map(move |a| (spend_value - output_value, a))
             })
         })
     }
diff --git a/src/circuit.rs b/src/circuit.rs
index ddde11f9..e48dc89b 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -412,26 +412,21 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // Value commitment integrity.
         let v_net = {
-            // v_net = v_old - v_new
+            // Witness the magnitude and sign of v_net = v_old - v_new
             let v_net = {
-                // v_old, v_new are guaranteed to be 64-bit values. Therefore, we can
-                // move them into the base field.
-                let v_old = self.v_old.map(|v_old| pallas::Base::from(v_old.inner()));
-                let v_new = self.v_new.map(|v_new| pallas::Base::from(v_new.inner()));
+                let magnitude_sign = self.v_old.zip(self.v_new).map(|(v_old, v_new)| {
+                    let v_net = v_old - v_new;
+                    let (magnitude, sign) = v_net.magnitude_sign();
 
-                let magnitude_sign = v_old.zip(v_new).map(|(v_old, v_new)| {
-                    let is_negative = v_old < v_new;
-                    let magnitude = if is_negative {
-                        v_new - v_old
-                    } else {
-                        v_old - v_new
-                    };
-                    let sign = if is_negative {
-                        -pallas::Base::one()
-                    } else {
-                        pallas::Base::one()
-                    };
-                    (magnitude, sign)
+                    (
+                        // magnitude is guaranteed to be an unsigned 64-bit value.
+                        // Therefore, we can move it into the base field.
+                        pallas::Base::from(magnitude),
+                        match sign {
+                            crate::value::Sign::Positive => pallas::Base::one(),
+                            crate::value::Sign::Negative => -pallas::Base::one(),
+                        },
+                    )
                 });
 
                 let magnitude = self.load_private(
@@ -930,7 +925,7 @@ mod tests {
 
         let value = spent_note.value() - output_note.value();
         let rcv = ValueCommitTrapdoor::random(&mut rng);
-        let cv_net = ValueCommitment::derive(value.unwrap(), rcv.clone());
+        let cv_net = ValueCommitment::derive(value, rcv.clone());
 
         let path = MerklePath::dummy(&mut rng);
         let anchor = path.root(spent_note.commitment().into());
diff --git a/src/value.rs b/src/value.rs
index 31326fd8..751a80e6 100644
--- a/src/value.rs
+++ b/src/value.rs
@@ -116,7 +116,7 @@ impl NoteValue {
 }
 
 impl Sub for NoteValue {
-    type Output = Option<ValueSum>;
+    type Output = ValueSum;
 
     #[allow(clippy::suspicious_arithmetic_impl)]
     fn sub(self, rhs: Self) -> Self::Output {
@@ -125,20 +125,26 @@ impl Sub for NoteValue {
         a.checked_sub(b)
             .filter(|v| VALUE_SUM_RANGE.contains(v))
             .map(ValueSum)
+            .expect("u64 - u64 result is always in VALUE_SUM_RANGE")
     }
 }
 
+pub(crate) enum Sign {
+    Positive,
+    Negative,
+}
+
 /// A sum of Orchard note values.
 #[derive(Clone, Copy, Debug, Default, PartialEq)]
 pub struct ValueSum(i128);
 
 impl ValueSum {
     pub(crate) fn zero() -> Self {
-        // Default for i64 is zero.
+        // Default for i128 is zero.
         Default::default()
     }
 
-    /// Creates a value sum from its raw numeric value.
+    /// Creates a value sum from a raw i64 (which is always in range for this type).
     ///
     /// This only enforces that the value is a signed 63-bit integer. We use it internally
     /// in `Bundle::binding_validating_key`, where we are converting from the user-defined
@@ -147,6 +153,20 @@ impl ValueSum {
     pub(crate) fn from_raw(value: i64) -> Self {
         ValueSum(value as i128)
     }
+
+    /// Splits this value sum into its magnitude and sign.
+    pub(crate) fn magnitude_sign(&self) -> (u64, Sign) {
+        let (magnitude, sign) = if self.0.is_negative() {
+            (-self.0, Sign::Negative)
+        } else {
+            (self.0, Sign::Positive)
+        };
+        (
+            u64::try_from(magnitude)
+                .expect("ValueSum magnitude is in range for u64 by construction"),
+            sign,
+        )
+    }
 }
 
 impl Add for ValueSum {

From dafb357dc0e4da65612cea652bbae5ba540f1c96 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 17:54:28 +0000
Subject: [PATCH 06/21] Extract a `DeriveNullifier` gadget from the circuit

This introduces an `AddChip` implementing field element addition on a
single row, precisely matching what the nullifier integrity constraints
were relying on.
---
 src/circuit.rs                 | 96 ++++++++--------------------------
 src/circuit/gadget.rs          | 87 ++++++++++++++++++++++++++++--
 src/circuit/gadget/add_chip.rs | 86 ++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+), 76 deletions(-)
 create mode 100644 src/circuit/gadget/add_chip.rs

diff --git a/src/circuit.rs b/src/circuit.rs
index e48dc89b..814ac27a 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -16,12 +16,15 @@ use memuse::DynamicUsage;
 use pasta_curves::{arithmetic::CurveAffine, pallas, vesta};
 use rand::RngCore;
 
-use self::commit_ivk::CommitIvkConfig;
-use self::note_commit::NoteCommitConfig;
+use self::{
+    commit_ivk::CommitIvkConfig,
+    gadget::add_chip::{AddChip, AddConfig},
+    note_commit::NoteCommitConfig,
+};
 use crate::{
     constants::{
-        NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull,
-        OrchardHashDomains, ValueCommitV, MERKLE_DEPTH_ORCHARD,
+        OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains,
+        ValueCommitV, MERKLE_DEPTH_ORCHARD,
     },
     keys::{
         CommitIvkRandomness, DiversifiedTransmissionKey, NullifierDerivingKey, SpendValidatingKey,
@@ -39,10 +42,10 @@ use crate::{
 use halo2_gadgets::{
     ecc::{
         chip::{EccChip, EccConfig},
-        FixedPoint, FixedPointBaseField, FixedPointShort, NonIdentityPoint, Point,
+        FixedPoint, FixedPointShort, NonIdentityPoint, Point,
     },
-    poseidon::{Hash as PoseidonHash, Pow5Chip as PoseidonChip, Pow5Config as PoseidonConfig},
-    primitives::poseidon::{self, ConstantLength},
+    poseidon::{Pow5Chip as PoseidonChip, Pow5Config as PoseidonConfig},
+    primitives::poseidon,
     sinsemilla::{
         chip::{SinsemillaChip, SinsemillaConfig},
         merkle::{
@@ -76,9 +79,8 @@ const ENABLE_OUTPUT: usize = 8;
 pub struct Config {
     primary: Column<InstanceColumn>,
     q_orchard: Selector,
-    // Selector for the field addition gate poseidon_hash(nk, rho_old) + psi_old.
-    q_add: Selector,
     advices: [Column<Advice>; 10],
+    add_config: AddConfig,
     ecc_config: EccConfig<OrchardFixedBases>,
     poseidon_config: PoseidonConfig<pallas::Base, 3, 2>,
     merkle_config_1: MerkleConfig<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
@@ -182,16 +184,8 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             )
         });
 
-        // Addition of two field elements poseidon_hash(nk, rho_old) + psi_old.
-        let q_add = meta.selector();
-        meta.create_gate("poseidon_hash(nk, rho_old) + psi_old", |meta| {
-            let q_add = meta.query_selector(q_add);
-            let sum = meta.query_advice(advices[6], Rotation::cur());
-            let hash_old = meta.query_advice(advices[7], Rotation::cur());
-            let psi_old = meta.query_advice(advices[8], Rotation::cur());
-
-            Constraints::with_selector(q_add, Some(hash_old + psi_old - sum))
-        });
+        // Addition of two field elements.
+        let add_config = AddChip::configure(meta, advices[7], advices[8], advices[6]);
 
         // Fixed columns for the Sinsemilla generator lookup table
         let table_idx = meta.lookup_table_column();
@@ -306,8 +300,8 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         Config {
             primary,
             q_orchard,
-            q_add,
             advices,
+            add_config,
             ecc_config,
             poseidon_config,
             merkle_config_1,
@@ -471,61 +465,17 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // Nullifier integrity
         let nf_old = {
-            // hash_old = poseidon_hash(nk, rho_old)
-            let hash_old = {
-                let poseidon_message = [nk.clone(), rho_old.clone()];
-                let poseidon_hasher =
-                    PoseidonHash::<_, _, poseidon::P128Pow5T3, ConstantLength<2>, 3, 2>::init(
-                        config.poseidon_chip(),
-                        layouter.namespace(|| "Poseidon init"),
-                    )?;
-                poseidon_hasher.hash(
-                    layouter.namespace(|| "Poseidon hash (nk, rho_old)"),
-                    poseidon_message,
-                )?
-            };
-
-            // Add hash output to psi.
-            // `scalar` = poseidon_hash(nk, rho_old) + psi_old.
-            //
-            let scalar = layouter.assign_region(
-                || " `scalar` = poseidon_hash(nk, rho_old) + psi_old",
-                |mut region| {
-                    config.q_add.enable(&mut region, 0)?;
-
-                    hash_old.copy_advice(|| "copy hash_old", &mut region, config.advices[7], 0)?;
-                    psi_old.copy_advice(|| "copy psi_old", &mut region, config.advices[8], 0)?;
-
-                    let scalar_val = hash_old
-                        .value()
-                        .zip(psi_old.value())
-                        .map(|(hash_old, psi_old)| hash_old + psi_old);
-                    region.assign_advice(
-                        || "poseidon_hash(nk, rho_old) + psi_old",
-                        config.advices[6],
-                        0,
-                        || scalar_val.ok_or(plonk::Error::Synthesis),
-                    )
-                },
+            let nf_old = gadget::derive_nullifier(
+                layouter.namespace(|| "nf_old = DeriveNullifier_nk(rho_old, psi_old, cm_old)"),
+                config.poseidon_chip(),
+                config.add_chip(),
+                ecc_chip.clone(),
+                rho_old.clone(),
+                &psi_old,
+                &cm_old,
+                nk.clone(),
             )?;
 
-            // Multiply scalar by NullifierK
-            // `product` = [poseidon_hash(nk, rho_old) + psi_old] NullifierK.
-            //
-            let product = {
-                let nullifier_k = FixedPointBaseField::from_inner(ecc_chip.clone(), NullifierK);
-                nullifier_k.mul(
-                    layouter.namespace(|| "[poseidon_output + psi_old] NullifierK"),
-                    scalar,
-                )?
-            };
-
-            // Add cm_old to multiplied fixed base to get nf_old
-            // cm_old + [poseidon_output + psi_old] NullifierK
-            let nf_old = cm_old
-                .add(layouter.namespace(|| "nf_old"), &product)?
-                .extract_p();
-
             // Constrain nf_old to equal public input
             layouter.constrain_instance(nf_old.inner().cell(), config.primary, NF_OLD)?;
 
diff --git a/src/circuit/gadget.rs b/src/circuit/gadget.rs
index 2ba7c6fb..7cc0fc89 100644
--- a/src/circuit/gadget.rs
+++ b/src/circuit/gadget.rs
@@ -2,14 +2,26 @@
 
 use pasta_curves::pallas;
 
-use crate::constants::{OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains};
+use crate::constants::{NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains};
 use halo2_gadgets::{
-    ecc::chip::EccChip,
-    poseidon::Pow5Chip as PoseidonChip,
+    ecc::{chip::EccChip, EccInstructions, FixedPointBaseField, Point, X},
+    poseidon::{Hash as PoseidonHash, PoseidonSpongeInstructions, Pow5Chip as PoseidonChip},
+    primitives::poseidon::{self, ConstantLength},
     sinsemilla::{chip::SinsemillaChip, merkle::chip::MerkleChip},
 };
+use halo2_proofs::{
+    arithmetic::FieldExt,
+    circuit::{AssignedCell, Chip, Layouter},
+    plonk,
+};
+
+pub(in crate::circuit) mod add_chip;
 
 impl super::Config {
+    pub(super) fn add_chip(&self) -> add_chip::AddChip {
+        add_chip::AddChip::construct(self.add_config.clone())
+    }
+
     pub(super) fn ecc_chip(&self) -> EccChip<OrchardFixedBases> {
         EccChip::construct(self.ecc_config.clone())
     }
@@ -42,3 +54,72 @@ impl super::Config {
         PoseidonChip::construct(self.poseidon_config.clone())
     }
 }
+
+/// An instruction set for adding two circuit words (field elements).
+pub(in crate::circuit) trait AddInstruction<F: FieldExt>: Chip<F> {
+    /// Constraints `a + b` and returns the sum.
+    fn add(
+        &self,
+        layouter: impl Layouter<F>,
+        a: &AssignedCell<F, F>,
+        b: &AssignedCell<F, F>,
+    ) -> Result<AssignedCell<F, F>, plonk::Error>;
+}
+
+/// `DeriveNullifier` from [Section 4.16: Note Commitments and Nullifiers].
+///
+/// [Section 4.16: Note Commitments and Nullifiers]: https://zips.z.cash/protocol/protocol.pdf#commitmentsandnullifiers
+#[allow(clippy::too_many_arguments)]
+pub(in crate::circuit) fn derive_nullifier<
+    PoseidonChip: PoseidonSpongeInstructions<pallas::Base, poseidon::P128Pow5T3, ConstantLength<2>, 3, 2>,
+    AddChip: AddInstruction<pallas::Base>,
+    EccChip: EccInstructions<
+        pallas::Affine,
+        FixedPoints = OrchardFixedBases,
+        Var = AssignedCell<pallas::Base, pallas::Base>,
+    >,
+>(
+    mut layouter: impl Layouter<pallas::Base>,
+    poseidon_chip: PoseidonChip,
+    add_chip: AddChip,
+    ecc_chip: EccChip,
+    rho: AssignedCell<pallas::Base, pallas::Base>,
+    psi: &AssignedCell<pallas::Base, pallas::Base>,
+    cm: &Point<pallas::Affine, EccChip>,
+    nk: AssignedCell<pallas::Base, pallas::Base>,
+) -> Result<X<pallas::Affine, EccChip>, plonk::Error> {
+    // hash = poseidon_hash(nk, rho)
+    let hash = {
+        let poseidon_message = [nk, rho];
+        let poseidon_hasher =
+            PoseidonHash::init(poseidon_chip, layouter.namespace(|| "Poseidon init"))?;
+        poseidon_hasher.hash(
+            layouter.namespace(|| "Poseidon hash (nk, rho)"),
+            poseidon_message,
+        )?
+    };
+
+    // Add hash output to psi.
+    // `scalar` = poseidon_hash(nk, rho) + psi.
+    let scalar = add_chip.add(
+        layouter.namespace(|| "scalar = poseidon_hash(nk, rho) + psi"),
+        &hash,
+        psi,
+    )?;
+
+    // Multiply scalar by NullifierK
+    // `product` = [poseidon_hash(nk, rho) + psi] NullifierK.
+    //
+    let product = {
+        let nullifier_k = FixedPointBaseField::from_inner(ecc_chip, NullifierK);
+        nullifier_k.mul(
+            layouter.namespace(|| "[poseidon_output + psi] NullifierK"),
+            scalar,
+        )?
+    };
+
+    // Add cm to multiplied fixed base to get nf
+    // cm + [poseidon_output + psi] NullifierK
+    cm.add(layouter.namespace(|| "nf"), &product)
+        .map(|res| res.extract_p())
+}
diff --git a/src/circuit/gadget/add_chip.rs b/src/circuit/gadget/add_chip.rs
new file mode 100644
index 00000000..bf013528
--- /dev/null
+++ b/src/circuit/gadget/add_chip.rs
@@ -0,0 +1,86 @@
+use halo2_proofs::{
+    circuit::{AssignedCell, Chip, Layouter},
+    plonk::{self, Advice, Column, ConstraintSystem, Constraints, Selector},
+    poly::Rotation,
+};
+use pasta_curves::pallas;
+
+use super::AddInstruction;
+
+#[derive(Clone, Debug)]
+pub(in crate::circuit) struct AddConfig {
+    a: Column<Advice>,
+    b: Column<Advice>,
+    c: Column<Advice>,
+    q_add: Selector,
+}
+
+/// A chip implementing a single addition constraint `c = a + b` on a single row.
+pub(in crate::circuit) struct AddChip {
+    config: AddConfig,
+}
+
+impl Chip<pallas::Base> for AddChip {
+    type Config = AddConfig;
+    type Loaded = ();
+
+    fn config(&self) -> &Self::Config {
+        &self.config
+    }
+
+    fn loaded(&self) -> &Self::Loaded {
+        &()
+    }
+}
+
+impl AddChip {
+    pub(in crate::circuit) fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        a: Column<Advice>,
+        b: Column<Advice>,
+        c: Column<Advice>,
+    ) -> AddConfig {
+        let q_add = meta.selector();
+        meta.create_gate("Field element addition: c = a + b", |meta| {
+            let q_add = meta.query_selector(q_add);
+            let a = meta.query_advice(a, Rotation::cur());
+            let b = meta.query_advice(b, Rotation::cur());
+            let c = meta.query_advice(c, Rotation::cur());
+
+            Constraints::with_selector(q_add, Some(a + b - c))
+        });
+
+        AddConfig { a, b, c, q_add }
+    }
+
+    pub(in crate::circuit) fn construct(config: AddConfig) -> Self {
+        Self { config }
+    }
+}
+
+impl AddInstruction<pallas::Base> for AddChip {
+    fn add(
+        &self,
+        mut layouter: impl Layouter<pallas::Base>,
+        a: &AssignedCell<pallas::Base, pallas::Base>,
+        b: &AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<AssignedCell<pallas::Base, pallas::Base>, plonk::Error> {
+        layouter.assign_region(
+            || "c = a + b",
+            |mut region| {
+                self.config.q_add.enable(&mut region, 0)?;
+
+                a.copy_advice(|| "copy a", &mut region, self.config.a, 0)?;
+                b.copy_advice(|| "copy b", &mut region, self.config.b, 0)?;
+
+                let scalar_val = a.value().zip(b.value()).map(|(a, b)| a + b);
+                region.assign_advice(
+                    || "c",
+                    self.config.c,
+                    0,
+                    || scalar_val.ok_or(plonk::Error::Synthesis),
+                )
+            },
+        )
+    }
+}

From 3b922f8f486a3ea1f56522eb79b28024448630a8 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 20:05:00 +0000
Subject: [PATCH 07/21] Extract a `ValueCommit^Orchard` gadget from the circuit

---
 src/circuit.rs        | 29 ++++++++------------------
 src/circuit/gadget.rs | 47 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 814ac27a..c9609048 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -24,7 +24,7 @@ use self::{
 use crate::{
     constants::{
         OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains,
-        ValueCommitV, MERKLE_DEPTH_ORCHARD,
+        MERKLE_DEPTH_ORCHARD,
     },
     keys::{
         CommitIvkRandomness, DiversifiedTransmissionKey, NullifierDerivingKey, SpendValidatingKey,
@@ -42,7 +42,7 @@ use crate::{
 use halo2_gadgets::{
     ecc::{
         chip::{EccChip, EccConfig},
-        FixedPoint, FixedPointShort, NonIdentityPoint, Point,
+        FixedPoint, NonIdentityPoint, Point,
     },
     poseidon::{Pow5Chip as PoseidonChip, Pow5Config as PoseidonConfig},
     primitives::poseidon,
@@ -436,25 +436,12 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 (magnitude, sign)
             };
 
-            // commitment = [v_net] ValueCommitV
-            let (commitment, _) = {
-                let value_commit_v = ValueCommitV;
-                let value_commit_v = FixedPointShort::from_inner(ecc_chip.clone(), value_commit_v);
-                value_commit_v.mul(layouter.namespace(|| "[v_net] ValueCommitV"), v_net.clone())?
-            };
-
-            // blind = [rcv] ValueCommitR
-            let (blind, _rcv) = {
-                let rcv = self.rcv.as_ref().map(|rcv| rcv.inner());
-                let value_commit_r = OrchardFixedBasesFull::ValueCommitR;
-                let value_commit_r = FixedPoint::from_inner(ecc_chip.clone(), value_commit_r);
-
-                // [rcv] ValueCommitR
-                value_commit_r.mul(layouter.namespace(|| "[rcv] ValueCommitR"), rcv)?
-            };
-
-            // [v_net] ValueCommitV + [rcv] ValueCommitR
-            let cv_net = commitment.add(layouter.namespace(|| "cv_net"), &blind)?;
+            let cv_net = gadget::value_commit_orchard(
+                layouter.namespace(|| "cv_net = ValueCommit^Orchard_rcv(v_net)"),
+                ecc_chip.clone(),
+                v_net.clone(),
+                self.rcv.as_ref().map(|rcv| rcv.inner()),
+            )?;
 
             // Constrain cv_net to equal public input
             layouter.constrain_instance(cv_net.inner().x().cell(), config.primary, CV_NET_X)?;
diff --git a/src/circuit/gadget.rs b/src/circuit/gadget.rs
index 7cc0fc89..98cde020 100644
--- a/src/circuit/gadget.rs
+++ b/src/circuit/gadget.rs
@@ -2,9 +2,14 @@
 
 use pasta_curves::pallas;
 
-use crate::constants::{NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains};
+use crate::constants::{
+    NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains,
+    ValueCommitV,
+};
 use halo2_gadgets::{
-    ecc::{chip::EccChip, EccInstructions, FixedPointBaseField, Point, X},
+    ecc::{
+        chip::EccChip, EccInstructions, FixedPoint, FixedPointBaseField, FixedPointShort, Point, X,
+    },
     poseidon::{Hash as PoseidonHash, PoseidonSpongeInstructions, Pow5Chip as PoseidonChip},
     primitives::poseidon::{self, ConstantLength},
     sinsemilla::{chip::SinsemillaChip, merkle::chip::MerkleChip},
@@ -66,6 +71,44 @@ pub(in crate::circuit) trait AddInstruction<F: FieldExt>: Chip<F> {
     ) -> Result<AssignedCell<F, F>, plonk::Error>;
 }
 
+/// `ValueCommit^Orchard` from [Section 5.4.8.3 Homomorphic Pedersen commitments (Sapling and Orchard)].
+///
+/// [Section 5.4.8.3 Homomorphic Pedersen commitments (Sapling and Orchard)]: https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit
+pub(in crate::circuit) fn value_commit_orchard<
+    EccChip: EccInstructions<
+        pallas::Affine,
+        FixedPoints = OrchardFixedBases,
+        Var = AssignedCell<pallas::Base, pallas::Base>,
+    >,
+>(
+    mut layouter: impl Layouter<pallas::Base>,
+    ecc_chip: EccChip,
+    v: (
+        AssignedCell<pallas::Base, pallas::Base>,
+        AssignedCell<pallas::Base, pallas::Base>,
+    ),
+    rcv: Option<pallas::Scalar>,
+) -> Result<Point<pallas::Affine, EccChip>, plonk::Error> {
+    // commitment = [v] ValueCommitV
+    let (commitment, _) = {
+        let value_commit_v = ValueCommitV;
+        let value_commit_v = FixedPointShort::from_inner(ecc_chip.clone(), value_commit_v);
+        value_commit_v.mul(layouter.namespace(|| "[v] ValueCommitV"), v)?
+    };
+
+    // blind = [rcv] ValueCommitR
+    let (blind, _rcv) = {
+        let value_commit_r = OrchardFixedBasesFull::ValueCommitR;
+        let value_commit_r = FixedPoint::from_inner(ecc_chip, value_commit_r);
+
+        // [rcv] ValueCommitR
+        value_commit_r.mul(layouter.namespace(|| "[rcv] ValueCommitR"), rcv)?
+    };
+
+    // [v] ValueCommitV + [rcv] ValueCommitR
+    commitment.add(layouter.namespace(|| "cv"), &blind)
+}
+
 /// `DeriveNullifier` from [Section 4.16: Note Commitments and Nullifiers].
 ///
 /// [Section 4.16: Note Commitments and Nullifiers]: https://zips.z.cash/protocol/protocol.pdf#commitmentsandnullifiers

From a491688944b5890b3f998c58091b2c34ec227003 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Fri, 29 Apr 2022 20:24:28 +0000
Subject: [PATCH 08/21] Circuit cleanups and documentation

---
 src/circuit.rs | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index c9609048..4fab7291 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -160,9 +160,10 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             let anchor = meta.query_advice(advices[4], Rotation::cur());
             let pub_input_anchor = meta.query_advice(advices[5], Rotation::cur());
 
+            let enable_spends = meta.query_advice(advices[6], Rotation::cur());
+            let enable_outputs = meta.query_advice(advices[7], Rotation::cur());
+
             let one = Expression::Constant(pallas::Base::one());
-            let not_enable_spends = one.clone() - meta.query_advice(advices[6], Rotation::cur());
-            let not_enable_outputs = one - meta.query_advice(advices[7], Rotation::cur());
 
             Constraints::with_selector(
                 q_orchard,
@@ -175,10 +176,13 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                         "Either v_old = 0, or anchor equals public input",
                         v_old.clone() * (anchor - pub_input_anchor),
                     ),
-                    ("v_old = 0 or enable_spends = 1", v_old * not_enable_spends),
+                    (
+                        "v_old = 0 or enable_spends = 1",
+                        v_old * (one.clone() - enable_spends),
+                    ),
                     (
                         "v_new = 0 or enable_outputs = 1",
-                        v_new * not_enable_outputs,
+                        v_new * (one - enable_outputs),
                     ),
                 ],
             )
@@ -389,7 +393,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         };
 
         // Merkle path validity check.
-        let anchor = {
+        let root = {
             let path = self
                 .path
                 .map(|typed_path| typed_path.map(|node| node.inner()));
@@ -401,7 +405,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 path,
             );
             let leaf = cm_old.extract_p().inner().clone();
-            merkle_inputs.calculate_root(layouter.namespace(|| "MerkleCRH"), leaf)?
+            merkle_inputs.calculate_root(layouter.namespace(|| "Merkle path"), leaf)?
         };
 
         // Value commitment integrity.
@@ -510,6 +514,12 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 g_d_old.mul(layouter.namespace(|| "[ivk] g_d_old"), ivk.inner())?;
 
             // Constrain derived pk_d_old to equal witnessed pk_d_old
+            //
+            // This equality constraint is technically superfluous, because the assigned
+            // value of `derived_pk_d_old` is an equivalent witness. But it's nice to see
+            // an explicit connection between circuit-synthesized values, and explicit
+            // prover witnesses. We could get the best of both worlds with a write-on-copy
+            // abstraction (https://github.com/zcash/halo2/issues/334).
             let pk_d_old = NonIdentityPoint::new(
                 ecc_chip.clone(),
                 layouter.namespace(|| "witness pk_d_old"),
@@ -570,6 +580,9 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 )?
             };
 
+            // ρ^new = nf^old
+            let rho_new = nf_old.inner().clone();
+
             // Witness psi_new
             let psi_new = self.load_private(
                 layouter.namespace(|| "witness psi_new"),
@@ -589,7 +602,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 g_d_new.inner(),
                 pk_d_new.inner(),
                 v_new.clone(),
-                nf_old.inner().clone(),
+                rho_new,
                 psi_new,
                 rcm_new,
             )?;
@@ -600,10 +613,9 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             layouter.constrain_instance(cmx.inner().cell(), config.primary, CMX)?;
         }
 
-        // Constrain v_old - v_new = magnitude * sign
-        // Either v_old = 0, or anchor equals public input
+        // Constrain the remaining Orchard circuit checks.
         layouter.assign_region(
-            || "v_old - v_new = magnitude * sign",
+            || "Orchard circuit checks",
             |mut region| {
                 v_old.copy_advice(|| "v_old", &mut region, config.advices[0], 0)?;
                 v_new.copy_advice(|| "v_new", &mut region, config.advices[1], 0)?;
@@ -611,7 +623,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                 magnitude.copy_advice(|| "v_net magnitude", &mut region, config.advices[2], 0)?;
                 sign.copy_advice(|| "v_net sign", &mut region, config.advices[3], 0)?;
 
-                anchor.copy_advice(|| "anchor", &mut region, config.advices[4], 0)?;
+                root.copy_advice(|| "calculated root", &mut region, config.advices[4], 0)?;
                 region.assign_advice_from_instance(
                     || "pub input anchor",
                     config.primary,

From 3e40780313ccb33aeaf39fddd2091fc717e0e72b Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Mon, 2 May 2022 12:35:16 +0000
Subject: [PATCH 09/21] Adjust APIs of Commit^ivk circuit impl to separate
 gadget and chip

---
 src/circuit.rs            | 10 ++--
 src/circuit/commit_ivk.rs | 96 +++++++++++++++++++++++----------------
 src/circuit/gadget.rs     |  7 +++
 3 files changed, 68 insertions(+), 45 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 4fab7291..2adb3167 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -17,7 +17,7 @@ use pasta_curves::{arithmetic::CurveAffine, pallas, vesta};
 use rand::RngCore;
 
 use self::{
-    commit_ivk::CommitIvkConfig,
+    commit_ivk::{CommitIvkChip, CommitIvkConfig},
     gadget::add_chip::{AddChip, AddConfig},
     note_commit::NoteCommitConfig,
 };
@@ -288,8 +288,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // Configuration to handle decomposition and canonicity checking
         // for CommitIvk.
-        let commit_ivk_config =
-            CommitIvkConfig::configure(meta, advices, sinsemilla_config_1.clone());
+        let commit_ivk_config = CommitIvkChip::configure(meta, advices);
 
         // Configuration to handle decomposition and canonicity checking
         // for NoteCommit_old.
@@ -492,15 +491,14 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // Diversified address integrity.
         let pk_d_old = {
-            let commit_ivk_config = config.commit_ivk_config.clone();
-
             let ivk = {
                 let ak = ak_P.extract_p().inner().clone();
                 let rivk = self.rivk.map(|rivk| rivk.inner());
 
-                commit_ivk_config.assign_region(
+                gadget::commit_ivk(
                     config.sinsemilla_chip_1(),
                     ecc_chip.clone(),
+                    config.commit_ivk_chip(),
                     layouter.namespace(|| "CommitIvk"),
                     ak,
                     nk,
diff --git a/src/circuit/commit_ivk.rs b/src/circuit/commit_ivk.rs
index e2a1f72c..c8559f9a 100644
--- a/src/circuit/commit_ivk.rs
+++ b/src/circuit/commit_ivk.rs
@@ -10,10 +10,7 @@ use pasta_curves::{arithmetic::FieldExt, pallas};
 use crate::constants::{OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains, T_P};
 use halo2_gadgets::{
     ecc::{chip::EccChip, X},
-    sinsemilla::{
-        chip::{SinsemillaChip, SinsemillaConfig},
-        CommitDomain, Message, MessagePiece,
-    },
+    sinsemilla::{chip::SinsemillaChip, CommitDomain, Message, MessagePiece},
     utilities::{bitrange_subset, bool_check},
 };
 
@@ -21,26 +18,23 @@ use halo2_gadgets::{
 pub struct CommitIvkConfig {
     q_commit_ivk: Selector,
     advices: [Column<Advice>; 10],
-    sinsemilla_config:
-        SinsemillaConfig<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
 }
 
-impl CommitIvkConfig {
+#[derive(Clone, Debug)]
+pub struct CommitIvkChip {
+    config: CommitIvkConfig,
+}
+
+impl CommitIvkChip {
     pub(in crate::circuit) fn configure(
         meta: &mut ConstraintSystem<pallas::Base>,
         advices: [Column<Advice>; 10],
-        sinsemilla_config: SinsemillaConfig<
-            OrchardHashDomains,
-            OrchardCommitDomains,
-            OrchardFixedBases,
-        >,
-    ) -> Self {
+    ) -> CommitIvkConfig {
         let q_commit_ivk = meta.selector();
 
-        let config = Self {
+        let config = CommitIvkConfig {
             q_commit_ivk,
             advices,
-            sinsemilla_config,
         };
 
         // <https://zips.z.cash/protocol/nu5.pdf#concretesinsemillacommit>
@@ -224,22 +218,37 @@ impl CommitIvkConfig {
         config
     }
 
+    pub(in crate::circuit) fn construct(config: CommitIvkConfig) -> Self {
+        Self { config }
+    }
+}
+
+pub(in crate::circuit) mod gadgets {
+    use halo2_gadgets::utilities::lookup_range_check::LookupRangeCheckConfig;
+    use halo2_proofs::circuit::Chip;
+
+    use super::*;
+
+    /// `Commit^ivk` from [Section 5.4.8.4 Sinsemilla commitments].
+    ///
+    /// [Section 5.4.8.4 Sinsemilla commitments]: https://zips.z.cash/protocol/protocol.pdf#concretesinsemillacommit
     #[allow(non_snake_case)]
     #[allow(clippy::type_complexity)]
-    pub(in crate::circuit) fn assign_region(
-        &self,
+    pub(in crate::circuit) fn commit_ivk(
         sinsemilla_chip: SinsemillaChip<
             OrchardHashDomains,
             OrchardCommitDomains,
             OrchardFixedBases,
         >,
         ecc_chip: EccChip<OrchardFixedBases>,
+        commit_ivk_chip: CommitIvkChip,
         mut layouter: impl Layouter<pallas::Base>,
         ak: AssignedCell<pallas::Base, pallas::Base>,
         nk: AssignedCell<pallas::Base, pallas::Base>,
         rivk: Option<pallas::Scalar>,
     ) -> Result<X<pallas::Affine, EccChip<OrchardFixedBases>>, Error> {
-        // <https://zips.z.cash/protocol/nu5.pdf#concretesinsemillacommit>
+        let lookup_config = sinsemilla_chip.config().lookup_config();
+
         // We need to hash `ak || nk` where each of `ak`, `nk` is a field element (255 bits).
         //
         // a = bits 0..=249 of `ak`
@@ -273,13 +282,13 @@ impl CommitIvkConfig {
             });
 
             // Constrain b_0 to be 4 bits.
-            let b_0 = self.sinsemilla_config.lookup_config().witness_short_check(
+            let b_0 = lookup_config.witness_short_check(
                 layouter.namespace(|| "b_0 is 4 bits"),
                 b_0,
                 4,
             )?;
             // Constrain b_2 to be 5 bits.
-            let b_2 = self.sinsemilla_config.lookup_config().witness_short_check(
+            let b_2 = lookup_config.witness_short_check(
                 layouter.namespace(|| "b_2 is 5 bits"),
                 b_2,
                 5,
@@ -317,7 +326,7 @@ impl CommitIvkConfig {
                 .map(|(d_0, d_1)| d_0 + d_1 * pallas::Base::from(1 << 9));
 
             // Constrain d_0 to be 9 bits.
-            let d_0 = self.sinsemilla_config.lookup_config().witness_short_check(
+            let d_0 = lookup_config.witness_short_check(
                 layouter.namespace(|| "d_0 is 9 bits"),
                 d_0,
                 9,
@@ -347,12 +356,14 @@ impl CommitIvkConfig {
         let z13_a = zs[0][13].clone();
         let z13_c = zs[2][13].clone();
 
-        let (a_prime, z13_a_prime) = self.ak_canonicity(
+        let (a_prime, z13_a_prime) = ak_canonicity(
+            &lookup_config,
             layouter.namespace(|| "ak canonicity"),
             a.inner().cell_value(),
         )?;
 
-        let (b2_c_prime, z14_b2_c_prime) = self.nk_canonicity(
+        let (b2_c_prime, z14_b2_c_prime) = nk_canonicity(
+            &lookup_config,
             layouter.namespace(|| "nk canonicity"),
             b_2.clone(),
             c.inner().cell_value(),
@@ -378,7 +389,7 @@ impl CommitIvkConfig {
             z14_b2_c_prime,
         };
 
-        self.assign_gate(
+        commit_ivk_chip.config.assign_gate(
             layouter.namespace(|| "Assign cells used in canonicity gate"),
             gate_cells,
         )?;
@@ -389,7 +400,7 @@ impl CommitIvkConfig {
     #[allow(clippy::type_complexity)]
     // Check canonicity of `ak` encoding
     fn ak_canonicity(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
         a: AssignedCell<pallas::Base, pallas::Base>,
     ) -> Result<
@@ -413,7 +424,7 @@ impl CommitIvkConfig {
             let t_p = pallas::Base::from_u128(T_P);
             a + two_pow_130 - t_p
         });
-        let zs = self.sinsemilla_config.lookup_config().witness_check(
+        let zs = lookup_config.witness_check(
             layouter.namespace(|| "Decompose low 130 bits of (a + 2^130 - t_P)"),
             a_prime,
             13,
@@ -428,7 +439,7 @@ impl CommitIvkConfig {
     #[allow(clippy::type_complexity)]
     // Check canonicity of `nk` encoding
     fn nk_canonicity(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
         b_2: AssignedCell<pallas::Base, pallas::Base>,
         c: AssignedCell<pallas::Base, pallas::Base>,
@@ -456,7 +467,7 @@ impl CommitIvkConfig {
             let t_p = pallas::Base::from_u128(T_P);
             b_2 + c * two_pow_5 + two_pow_140 - t_p
         });
-        let zs = self.sinsemilla_config.lookup_config().witness_check(
+        let zs = lookup_config.witness_check(
             layouter.namespace(|| "Decompose low 140 bits of (b_2 + c * 2^5 + 2^140 - t_P)"),
             b2_c_prime,
             14,
@@ -467,7 +478,9 @@ impl CommitIvkConfig {
 
         Ok((b2_c_prime, zs[14].clone()))
     }
+}
 
+impl CommitIvkConfig {
     // Assign cells for the canonicity gate.
     /*
         The pieces are laid out in this configuration:
@@ -638,7 +651,7 @@ struct GateCells {
 mod tests {
     use core::iter;
 
-    use super::CommitIvkConfig;
+    use super::{gadgets, CommitIvkChip, CommitIvkConfig};
     use crate::constants::{
         fixed_bases::COMMIT_IVK_PERSONALIZATION, OrchardCommitDomains, OrchardFixedBases,
         OrchardHashDomains, L_ORCHARD_BASE, T_Q,
@@ -647,7 +660,7 @@ mod tests {
     use halo2_gadgets::{
         ecc::chip::{EccChip, EccConfig},
         primitives::sinsemilla::CommitDomain,
-        sinsemilla::chip::SinsemillaChip,
+        sinsemilla::chip::{SinsemillaChip, SinsemillaConfig},
         utilities::{lookup_range_check::LookupRangeCheckConfig, UtilitiesInstructions},
     };
     use halo2_proofs::{
@@ -671,7 +684,11 @@ mod tests {
         }
 
         impl Circuit<pallas::Base> for MyCircuit {
-            type Config = (CommitIvkConfig, EccConfig<OrchardFixedBases>);
+            type Config = (
+                SinsemillaConfig<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+                CommitIvkConfig,
+                EccConfig<OrchardFixedBases>,
+            );
             type FloorPlanner = SimpleFloorPlanner;
 
             fn without_witnesses(&self) -> Self {
@@ -730,8 +747,7 @@ mod tests {
                     range_check,
                 );
 
-                let commit_ivk_config =
-                    CommitIvkConfig::configure(meta, advices, sinsemilla_config);
+                let commit_ivk_config = CommitIvkChip::configure(meta, advices);
 
                 let ecc_config = EccChip::<OrchardFixedBases>::configure(
                     meta,
@@ -740,7 +756,7 @@ mod tests {
                     range_check,
                 );
 
-                (commit_ivk_config, ecc_config)
+                (sinsemilla_config, commit_ivk_config, ecc_config)
             }
 
             fn synthesize(
@@ -748,18 +764,19 @@ mod tests {
                 config: Self::Config,
                 mut layouter: impl Layouter<pallas::Base>,
             ) -> Result<(), Error> {
-                let (commit_ivk_config, ecc_config) = config;
+                let (sinsemilla_config, commit_ivk_config, ecc_config) = config;
 
                 // Load the Sinsemilla generator lookup table used by the whole circuit.
-                SinsemillaChip::<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>::load(commit_ivk_config.sinsemilla_config.clone(), &mut layouter)?;
+                SinsemillaChip::<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>::load(sinsemilla_config.clone(), &mut layouter)?;
 
                 // Construct a Sinsemilla chip
-                let sinsemilla_chip =
-                    SinsemillaChip::construct(commit_ivk_config.sinsemilla_config.clone());
+                let sinsemilla_chip = SinsemillaChip::construct(sinsemilla_config);
 
                 // Construct an ECC chip
                 let ecc_chip = EccChip::construct(ecc_config);
 
+                let commit_ivk_chip = CommitIvkChip::construct(commit_ivk_config.clone());
+
                 // Witness ak
                 let ak = self.load_private(
                     layouter.namespace(|| "load ak"),
@@ -777,9 +794,10 @@ mod tests {
                 // Use a random scalar for rivk
                 let rivk = pallas::Scalar::random(OsRng);
 
-                let ivk = commit_ivk_config.assign_region(
+                let ivk = gadgets::commit_ivk(
                     sinsemilla_chip,
                     ecc_chip,
+                    commit_ivk_chip,
                     layouter.namespace(|| "CommitIvk"),
                     ak,
                     nk,
diff --git a/src/circuit/gadget.rs b/src/circuit/gadget.rs
index 98cde020..e2b2fc07 100644
--- a/src/circuit/gadget.rs
+++ b/src/circuit/gadget.rs
@@ -2,6 +2,7 @@
 
 use pasta_curves::pallas;
 
+use super::commit_ivk::CommitIvkChip;
 use crate::constants::{
     NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains,
     ValueCommitV,
@@ -27,6 +28,10 @@ impl super::Config {
         add_chip::AddChip::construct(self.add_config.clone())
     }
 
+    pub(super) fn commit_ivk_chip(&self) -> CommitIvkChip {
+        CommitIvkChip::construct(self.commit_ivk_config.clone())
+    }
+
     pub(super) fn ecc_chip(&self) -> EccChip<OrchardFixedBases> {
         EccChip::construct(self.ecc_config.clone())
     }
@@ -166,3 +171,5 @@ pub(in crate::circuit) fn derive_nullifier<
     cm.add(layouter.namespace(|| "nf"), &product)
         .map(|res| res.extract_p())
 }
+
+pub(in crate::circuit) use crate::circuit::commit_ivk::gadgets::commit_ivk;

From bd104360a7876dcba0c98e165285df31335848e0 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Mon, 2 May 2022 16:09:38 +0000
Subject: [PATCH 10/21] Migrate to `halo2_gadgets::utilities::RangeConstrained`
 newtype

---
 Cargo.toml                 |   4 +-
 src/circuit/commit_ivk.rs  | 139 +++++++--------
 src/circuit/note_commit.rs | 345 +++++++++++++++++++------------------
 3 files changed, 246 insertions(+), 242 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index efe0a6de..672216be 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -82,5 +82,5 @@ debug = true
 debug = true
 
 [patch.crates-io]
-halo2_gadgets = { git = "https://github.com/zcash/halo2.git", rev = "0c33fa4e6e41464884765c8fb4cefebafd300ca2" }
-halo2_proofs = { git = "https://github.com/zcash/halo2.git", rev = "0c33fa4e6e41464884765c8fb4cefebafd300ca2" }
+halo2_gadgets = { git = "https://github.com/zcash/halo2.git", rev = "97864d714e33c5258b4ca3f25c9ddc1624e240dd" }
+halo2_proofs = { git = "https://github.com/zcash/halo2.git", rev = "97864d714e33c5258b4ca3f25c9ddc1624e240dd" }
diff --git a/src/circuit/commit_ivk.rs b/src/circuit/commit_ivk.rs
index c8559f9a..d755d5bc 100644
--- a/src/circuit/commit_ivk.rs
+++ b/src/circuit/commit_ivk.rs
@@ -11,7 +11,7 @@ use crate::constants::{OrchardCommitDomains, OrchardFixedBases, OrchardHashDomai
 use halo2_gadgets::{
     ecc::{chip::EccChip, X},
     sinsemilla::{chip::SinsemillaChip, CommitDomain, Message, MessagePiece},
-    utilities::{bitrange_subset, bool_check},
+    utilities::{bool_check, RangeConstrained},
 };
 
 #[derive(Clone, Debug)]
@@ -224,7 +224,7 @@ impl CommitIvkChip {
 }
 
 pub(in crate::circuit) mod gadgets {
-    use halo2_gadgets::utilities::lookup_range_check::LookupRangeCheckConfig;
+    use halo2_gadgets::utilities::{lookup_range_check::LookupRangeCheckConfig, RangeConstrained};
     use halo2_proofs::circuit::Chip;
 
     use super::*;
@@ -258,86 +258,64 @@ pub(in crate::circuit) mod gadgets {
         // d = d_0||d_1` = (bits 245..=253 of `nk`) || (bit 254 of `nk`)
 
         // `a` = bits 0..=249 of `ak`
-        let a = {
-            let a = ak.value().map(|value| bitrange_subset(value, 0..250));
-            MessagePiece::from_field_elem(
-                sinsemilla_chip.clone(),
-                layouter.namespace(|| "a"),
-                a,
-                25,
-            )?
-        };
+        let a = MessagePiece::from_subpieces(
+            sinsemilla_chip.clone(),
+            layouter.namespace(|| "a"),
+            [RangeConstrained::subset_of(ak.value(), 0..250)],
+        )?;
 
         // `b = b_0||b_1||b_2`
         //    = (bits 250..=253 of `ak`) || (bit 254 of  `ak`) || (bits 0..=4 of  `nk`)
         let (b_0, b_1, b_2, b) = {
-            let b_0 = ak.value().map(|value| bitrange_subset(value, 250..254));
-            let b_1 = ak.value().map(|value| bitrange_subset(value, 254..255));
-            let b_2 = nk.value().map(|value| bitrange_subset(value, 0..5));
-
-            let b = b_0.zip(b_1).zip(b_2).map(|((b_0, b_1), b_2)| {
-                let b1_shifted = b_1 * pallas::Base::from(1 << 4);
-                let b2_shifted = b_2 * pallas::Base::from(1 << 5);
-                b_0 + b1_shifted + b2_shifted
-            });
-
             // Constrain b_0 to be 4 bits.
-            let b_0 = lookup_config.witness_short_check(
-                layouter.namespace(|| "b_0 is 4 bits"),
-                b_0,
-                4,
-            )?;
-            // Constrain b_2 to be 5 bits.
-            let b_2 = lookup_config.witness_short_check(
-                layouter.namespace(|| "b_2 is 5 bits"),
-                b_2,
-                5,
+            let b_0 = RangeConstrained::witness_short(
+                &lookup_config,
+                layouter.namespace(|| "b_0"),
+                ak.value(),
+                250..254,
             )?;
             // b_1 will be boolean-constrained in the custom gate.
+            let b_1 = RangeConstrained::subset_of(ak.value(), 254..255);
+            // Constrain b_2 to be 5 bits.
+            let b_2 = RangeConstrained::witness_short(
+                &lookup_config,
+                layouter.namespace(|| "b_2"),
+                nk.value(),
+                0..5,
+            )?;
 
-            let b = MessagePiece::from_field_elem(
+            let b = MessagePiece::from_subpieces(
                 sinsemilla_chip.clone(),
                 layouter.namespace(|| "b = b_0 || b_1 || b_2"),
-                b,
-                1,
+                [b_0.value(), b_1, b_2.value()],
             )?;
 
             (b_0, b_1, b_2, b)
         };
 
         // c = bits 5..=244 of `nk`
-        let c = {
-            let c = nk.value().map(|value| bitrange_subset(value, 5..245));
-            MessagePiece::from_field_elem(
-                sinsemilla_chip.clone(),
-                layouter.namespace(|| "c"),
-                c,
-                24,
-            )?
-        };
+        let c = MessagePiece::from_subpieces(
+            sinsemilla_chip.clone(),
+            layouter.namespace(|| "c"),
+            [RangeConstrained::subset_of(nk.value(), 5..245)],
+        )?;
 
         // `d = d_0||d_1` = (bits 245..=253 of `nk`) || (bit 254 of `nk`)
         let (d_0, d_1, d) = {
-            let d_0 = nk.value().map(|value| bitrange_subset(value, 245..254));
-            let d_1 = nk.value().map(|value| bitrange_subset(value, 254..255));
-
-            let d = d_0
-                .zip(d_1)
-                .map(|(d_0, d_1)| d_0 + d_1 * pallas::Base::from(1 << 9));
-
             // Constrain d_0 to be 9 bits.
-            let d_0 = lookup_config.witness_short_check(
-                layouter.namespace(|| "d_0 is 9 bits"),
-                d_0,
-                9,
+            let d_0 = RangeConstrained::witness_short(
+                &lookup_config,
+                layouter.namespace(|| "d_0"),
+                nk.value(),
+                245..254,
             )?;
             // d_1 will be boolean-constrained in the custom gate.
+            let d_1 = RangeConstrained::subset_of(nk.value(), 254..255);
 
-            let d = MessagePiece::from_field_elem(
+            let d = MessagePiece::from_subpieces(
                 sinsemilla_chip.clone(),
                 layouter.namespace(|| "d = d_0 || d_1"),
-                d,
-                1,
+                [d_0.value(), d_1],
             )?;
 
             (d_0, d_1, d)
@@ -365,7 +343,7 @@ pub(in crate::circuit) mod gadgets {
         let (b2_c_prime, z14_b2_c_prime) = nk_canonicity(
             &lookup_config,
             layouter.namespace(|| "nk canonicity"),
-            b_2.clone(),
+            &b_2,
             c.inner().cell_value(),
         )?;
 
@@ -441,7 +419,7 @@ pub(in crate::circuit) mod gadgets {
     fn nk_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
-        b_2: AssignedCell<pallas::Base, pallas::Base>,
+        b_2: &RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         c: AssignedCell<pallas::Base, pallas::Base>,
     ) -> Result<
         (
@@ -461,7 +439,7 @@ pub(in crate::circuit) mod gadgets {
 
         // Decompose the low 140 bits of b2_c_prime = b_2 + c * 2^5 + 2^140 - t_P, and output
         // the running sum at the end of it. If b2_c_prime < 2^140, the running sum will be 0.
-        let b2_c_prime = b_2.value().zip(c.value()).map(|(b_2, c)| {
+        let b2_c_prime = b_2.inner().value().zip(c.value()).map(|(b_2, c)| {
             let two_pow_5 = pallas::Base::from(1 << 5);
             let two_pow_140 = pallas::Base::from_u128(1u128 << 70).square();
             let t_p = pallas::Base::from_u128(T_P);
@@ -521,22 +499,28 @@ impl CommitIvkConfig {
                         .copy_advice(|| "b", &mut region, self.advices[2], offset)?;
 
                     // Copy in `b_0`
-                    gate_cells
-                        .b_0
-                        .copy_advice(|| "b_0", &mut region, self.advices[3], offset)?;
+                    gate_cells.b_0.inner().copy_advice(
+                        || "b_0",
+                        &mut region,
+                        self.advices[3],
+                        offset,
+                    )?;
 
                     // Witness `b_1`
                     region.assign_advice(
                         || "Witness b_1",
                         self.advices[4],
                         offset,
-                        || gate_cells.b_1.ok_or(Error::Synthesis),
+                        || gate_cells.b_1.inner().ok_or(Error::Synthesis),
                     )?;
 
                     // Copy in `b_2`
-                    gate_cells
-                        .b_2
-                        .copy_advice(|| "b_2", &mut region, self.advices[5], offset)?;
+                    gate_cells.b_2.inner().copy_advice(
+                        || "b_2",
+                        &mut region,
+                        self.advices[5],
+                        offset,
+                    )?;
 
                     // Copy in z13_a
                     gate_cells.z13_a.copy_advice(
@@ -583,16 +567,19 @@ impl CommitIvkConfig {
                         .copy_advice(|| "d", &mut region, self.advices[2], offset)?;
 
                     // Copy in `d_0`
-                    gate_cells
-                        .d_0
-                        .copy_advice(|| "d_0", &mut region, self.advices[3], offset)?;
+                    gate_cells.d_0.inner().copy_advice(
+                        || "d_0",
+                        &mut region,
+                        self.advices[3],
+                        offset,
+                    )?;
 
                     // Witness `d_1`
                     region.assign_advice(
                         || "Witness d_1",
                         self.advices[4],
                         offset,
-                        || gate_cells.d_1.ok_or(Error::Synthesis),
+                        || gate_cells.d_1.inner().ok_or(Error::Synthesis),
                     )?;
 
                     // Copy in z13_c
@@ -634,11 +621,11 @@ struct GateCells {
     d: AssignedCell<pallas::Base, pallas::Base>,
     ak: AssignedCell<pallas::Base, pallas::Base>,
     nk: AssignedCell<pallas::Base, pallas::Base>,
-    b_0: AssignedCell<pallas::Base, pallas::Base>,
-    b_1: Option<pallas::Base>,
-    b_2: AssignedCell<pallas::Base, pallas::Base>,
-    d_0: AssignedCell<pallas::Base, pallas::Base>,
-    d_1: Option<pallas::Base>,
+    b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    b_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+    b_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    d_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    d_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     z13_a: AssignedCell<pallas::Base, pallas::Base>,
     a_prime: AssignedCell<pallas::Base, pallas::Base>,
     z13_a_prime: AssignedCell<pallas::Base, pallas::Base>,
diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index b457e78b..01a0afbf 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -17,7 +17,7 @@ use halo2_gadgets::{
         chip::{SinsemillaChip, SinsemillaConfig},
         CommitDomain, Message, MessagePiece,
     },
-    utilities::{bitrange_subset, bool_check},
+    utilities::{bool_check, FieldValue, RangeConstrained},
 };
 
 /// The values of the running sum at the start and end of the range being used for a
@@ -565,148 +565,134 @@ impl NoteCommitConfig {
         let psi_val = psi.value();
 
         // `a` = bits 0..=249 of `x(g_d)`
-        let a = {
-            let a = gd_x.map(|gd_x| bitrange_subset(gd_x, 0..250));
-            MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "a"), a, 25)?
-        };
+        let a = MessagePiece::from_subpieces(
+            chip.clone(),
+            layouter.namespace(|| "a"),
+            [RangeConstrained::subset_of(gd_x, 0..250)],
+        )?;
 
         // b = b_0 || b_1 || b_2 || b_3
         //   = (bits 250..=253 of x(g_d)) || (bit 254 of x(g_d)) || (ỹ bit of g_d) || (bits 0..=3 of pk★_d)
-        let (b_0, b_1, b_2, b_3, b) =
-            {
-                let b_0 = gd_x.map(|gd_x| bitrange_subset(gd_x, 250..254));
-                let b_1 = gd_x.map(|gd_x| bitrange_subset(gd_x, 254..255));
-                let b_2 = gd_y.map(|gd_y| bitrange_subset(gd_y, 0..1));
-                let b_3 = pkd_x.map(|pkd_x| bitrange_subset(pkd_x, 0..4));
+        let (b_0, b_1, b_2, b_3, b) = {
+            // Constrain b_0 to be 4 bits
+            let b_0 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "b_0"),
+                gd_x,
+                250..254,
+            )?;
 
-                // Constrain b_0 to be 4 bits
-                let b_0 = self.sinsemilla_config.lookup_config().witness_short_check(
-                    layouter.namespace(|| "b_0 is 4 bits"),
-                    b_0,
-                    4,
-                )?;
+            // b_1, b_2 will be boolean-constrained in the gate.
+            let b_1 = RangeConstrained::subset_of(gd_x, 254..255);
+            let b_2 = RangeConstrained::subset_of(gd_y, 0..1);
 
-                // Constrain b_3 to be 4 bits
-                let b_3 = self.sinsemilla_config.lookup_config().witness_short_check(
-                    layouter.namespace(|| "b_3 is 4 bits"),
-                    b_3,
-                    4,
-                )?;
+            // Constrain b_3 to be 4 bits
+            let b_3 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "b_3"),
+                pkd_x,
+                0..4,
+            )?;
 
-                // b_1, b_2 will be boolean-constrained in the gate.
+            let b = MessagePiece::from_subpieces(
+                chip.clone(),
+                layouter.namespace(|| "b"),
+                [b_0.value(), b_1, b_2, b_3.value()],
+            )?;
 
-                let b = b_0.value().zip(b_1).zip(b_2).zip(b_3.value()).map(
-                    |(((b_0, b_1), b_2), b_3)| {
-                        let b1_shifted = b_1 * pallas::Base::from(1 << 4);
-                        let b2_shifted = b_2 * pallas::Base::from(1 << 5);
-                        let b3_shifted = b_3 * pallas::Base::from(1 << 6);
-                        b_0 + b1_shifted + b2_shifted + b3_shifted
-                    },
-                );
-
-                let b =
-                    MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "b"), b, 1)?;
-
-                (b_0, b_1, b_2, b_3, b)
-            };
+            (b_0, b_1, b_2, b_3, b)
+        };
 
         // c = bits 4..=253 of pk★_d
-        let c = {
-            let c = pkd_x.map(|pkd_x| bitrange_subset(pkd_x, 4..254));
-            MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "c"), c, 25)?
-        };
+        let c = MessagePiece::from_subpieces(
+            chip.clone(),
+            layouter.namespace(|| "c"),
+            [RangeConstrained::subset_of(pkd_x, 4..254)],
+        )?;
 
         // d = d_0 || d_1 || d_2 || d_3
         //   = (bit 254 of x(pk_d)) || (ỹ bit of pk_d) || (bits 0..=7 of v) || (bits 8..=57 of v)
         let (d_0, d_1, d_2, d) = {
-            let d_0 = pkd_x.map(|pkd_x| bitrange_subset(pkd_x, 254..255));
-            let d_1 = pkd_y.map(|pkd_y| bitrange_subset(pkd_y, 0..1));
-            let d_2 = value_val.map(|value| bitrange_subset(value, 0..8));
-            let d_3 = value_val.map(|value| bitrange_subset(value, 8..58));
+            // d_0, d_1 will be boolean-constrained in the gate.
+            let d_0 = RangeConstrained::subset_of(pkd_x, 254..255);
+            let d_1 = RangeConstrained::subset_of(pkd_y, 0..1);
 
             // Constrain d_2 to be 8 bits
-            let d_2 = self.sinsemilla_config.lookup_config().witness_short_check(
-                layouter.namespace(|| "d_2 is 8 bits"),
-                d_2,
-                8,
+            let d_2 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "d_2"),
+                value_val,
+                0..8,
             )?;
 
-            // d_0, d_1 will be boolean-constrained in the gate.
             // d_3 = z1_d from the SinsemillaHash(d) running sum output.
+            let d_3 = RangeConstrained::subset_of(value_val, 8..58);
 
-            let d = d_0
-                .zip(d_1)
-                .zip(d_2.value())
-                .zip(d_3)
-                .map(|(((d_0, d_1), d_2), d_3)| {
-                    let d1_shifted = d_1 * pallas::Base::from(2);
-                    let d2_shifted = d_2 * pallas::Base::from(1 << 2);
-                    let d3_shifted = d_3 * pallas::Base::from(1 << 10);
-                    d_0 + d1_shifted + d2_shifted + d3_shifted
-                });
-
-            let d = MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "d"), d, 6)?;
+            let d = MessagePiece::from_subpieces(
+                chip.clone(),
+                layouter.namespace(|| "d"),
+                [d_0, d_1, d_2.value(), d_3],
+            )?;
 
             (d_0, d_1, d_2, d)
         };
 
         // e = e_0 || e_1 = (bits 58..=63 of v) || (bits 0..=3 of rho)
         let (e_0, e_1, e) = {
-            let e_0 = value_val.map(|value| bitrange_subset(value, 58..64));
-            let e_1 = rho_val.map(|rho| bitrange_subset(rho, 0..4));
-
             // Constrain e_0 to be 6 bits.
-            let e_0 = self.sinsemilla_config.lookup_config().witness_short_check(
-                layouter.namespace(|| "e_0 is 6 bits"),
-                e_0,
-                6,
+            let e_0 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "e_0"),
+                value_val,
+                58..64,
             )?;
 
             // Constrain e_1 to be 4 bits.
-            let e_1 = self.sinsemilla_config.lookup_config().witness_short_check(
-                layouter.namespace(|| "e_1 is 4 bits"),
-                e_1,
-                4,
+            let e_1 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "e_1"),
+                rho_val,
+                0..4,
             )?;
 
-            let e = e_0
-                .value()
-                .zip(e_1.value())
-                .map(|(e_0, e_1)| e_0 + e_1 * pallas::Base::from(1 << 6));
-            let e = MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "e"), e, 1)?;
+            let e = MessagePiece::from_subpieces(
+                chip.clone(),
+                layouter.namespace(|| "e"),
+                [e_0.value(), e_1.value()],
+            )?;
 
             (e_0, e_1, e)
         };
 
         // f = bits 4..=253 inclusive of rho
-        let f = {
-            let f = rho_val.map(|rho| bitrange_subset(rho, 4..254));
-            MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "f"), f, 25)?
-        };
+        let f = MessagePiece::from_subpieces(
+            chip.clone(),
+            layouter.namespace(|| "f"),
+            [RangeConstrained::subset_of(rho_val, 4..254)],
+        )?;
 
         // g = g_0 || g_1 || g_2
         //   = (bit 254 of rho) || (bits 0..=8 of psi) || (bits 9..=248 of psi)
         let (g_0, g_1, g) = {
-            let g_0 = rho_val.map(|rho| bitrange_subset(rho, 254..255));
-            let g_1 = psi_val.map(|psi| bitrange_subset(psi, 0..9));
-            let g_2 = psi_val.map(|psi| bitrange_subset(psi, 9..249));
+            // g_0 will be boolean-constrained in the gate.
+            let g_0 = RangeConstrained::subset_of(rho_val, 254..255);
 
             // Constrain g_1 to be 9 bits.
-            let g_1 = self.sinsemilla_config.lookup_config().witness_short_check(
-                layouter.namespace(|| "g_1 is 9 bits"),
-                g_1,
-                9,
+            let g_1 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "g_1"),
+                psi_val,
+                0..9,
             )?;
 
-            // g_0 will be boolean-constrained in the gate.
             // g_2 = z1_g from the SinsemillaHash(g) running sum output.
+            let g_2 = RangeConstrained::subset_of(psi_val, 9..249);
 
-            let g = g_0.zip(g_1.value()).zip(g_2).map(|((g_0, g_1), g_2)| {
-                let g1_shifted = g_1 * pallas::Base::from(2);
-                let g2_shifted = g_2 * pallas::Base::from(1 << 10);
-                g_0 + g1_shifted + g2_shifted
-            });
-            let g = MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "g"), g, 25)?;
+            let g = MessagePiece::from_subpieces(
+                chip.clone(),
+                layouter.namespace(|| "g"),
+                [g_0, g_1.value(), g_2],
+            )?;
 
             (g_0, g_1, g)
         };
@@ -714,23 +700,26 @@ impl NoteCommitConfig {
         // h = h_0 || h_1 || h_2
         //   = (bits 249..=253 of psi) || (bit 254 of psi) || 4 zero bits
         let (h_0, h_1, h) = {
-            let h_0 = psi_val.map(|psi| bitrange_subset(psi, 249..254));
-            let h_1 = psi_val.map(|psi| bitrange_subset(psi, 254..255));
-
             // Constrain h_0 to be 5 bits.
-            let h_0 = self.sinsemilla_config.lookup_config().witness_short_check(
-                layouter.namespace(|| "h_0 is 5 bits"),
-                h_0,
-                5,
+            let h_0 = RangeConstrained::witness_short(
+                &self.sinsemilla_config.lookup_config(),
+                layouter.namespace(|| "h_0"),
+                psi_val,
+                249..254,
             )?;
 
             // h_1 will be boolean-constrained in the gate.
+            let h_1 = RangeConstrained::subset_of(psi_val, 254..255);
 
-            let h = h_0
-                .value()
-                .zip(h_1)
-                .map(|(h_0, h_1)| h_0 + h_1 * pallas::Base::from(1 << 5));
-            let h = MessagePiece::from_field_elem(chip.clone(), layouter.namespace(|| "h"), h, 1)?;
+            let h = MessagePiece::from_subpieces(
+                chip.clone(),
+                layouter.namespace(|| "h"),
+                [
+                    h_0.value(),
+                    h_1,
+                    RangeConstrained::subset_of(Some(&pallas::Base::zero()), 0..4),
+                ],
+            )?;
 
             (h_0, h_1, h)
         };
@@ -878,7 +867,7 @@ impl NoteCommitConfig {
     fn pkd_x_canonicity(
         &self,
         mut layouter: impl Layouter<pallas::Base>,
-        b_3: AssignedCell<pallas::Base, pallas::Base>,
+        b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         c: AssignedCell<pallas::Base, pallas::Base>,
     ) -> Result<CanonicityBounds, Error> {
         // `x(pk_d)` = `b_3 (4 bits) || c (250 bits) || d_0 (1 bit)`
@@ -894,7 +883,7 @@ impl NoteCommitConfig {
         // Decompose the low 140 bits of b3_c_prime = b_3 + 2^4 c + 2^140 - t_P,
         // and output the running sum at the end of it.
         // If b3_c_prime < 2^140, the running sum will be 0.
-        let b3_c_prime = b_3.value().zip(c.value()).map(|(b_3, c)| {
+        let b3_c_prime = b_3.inner().value().zip(c.value()).map(|(b_3, c)| {
             let two_pow_4 = pallas::Base::from(1u64 << 4);
             let two_pow_140 = pallas::Base::from_u128(1u128 << 70).square();
             let t_p = pallas::Base::from_u128(T_P);
@@ -917,7 +906,7 @@ impl NoteCommitConfig {
     fn rho_canonicity(
         &self,
         mut layouter: impl Layouter<pallas::Base>,
-        e_1: AssignedCell<pallas::Base, pallas::Base>,
+        e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         f: AssignedCell<pallas::Base, pallas::Base>,
     ) -> Result<CanonicityBounds, Error> {
         // `rho` = `e_1 (4 bits) || f (250 bits) || g_0 (1 bit)`
@@ -930,7 +919,7 @@ impl NoteCommitConfig {
         //       to 130 bits. z13_f == 0 is directly checked in the gate.
         // - 0 ≤ e_1 + 2^4 f + 2^140 - t_P < 2^140 (14 ten-bit lookups)
 
-        let e1_f_prime = e_1.value().zip(f.value()).map(|(e_1, f)| {
+        let e1_f_prime = e_1.inner().value().zip(f.value()).map(|(e_1, f)| {
             let two_pow_4 = pallas::Base::from(1u64 << 4);
             let two_pow_140 = pallas::Base::from_u128(1u128 << 70).square();
             let t_p = pallas::Base::from_u128(T_P);
@@ -956,7 +945,7 @@ impl NoteCommitConfig {
     fn psi_canonicity(
         &self,
         mut layouter: impl Layouter<pallas::Base>,
-        g_1: AssignedCell<pallas::Base, pallas::Base>,
+        g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         g_2: AssignedCell<pallas::Base, pallas::Base>,
     ) -> Result<CanonicityBounds, Error> {
         // `psi` = `g_1 (9 bits) || g_2 (240 bits) || h_0 (5 bits) || h_1 (1 bit)`
@@ -970,7 +959,7 @@ impl NoteCommitConfig {
         // Decompose the low 130 bits of g1_g2_prime = g_1 + (2^9)g_2 + 2^130 - t_P,
         // and output the running sum at the end of it.
         // If g1_g2_prime < 2^130, the running sum will be 0.
-        let g1_g2_prime = g_1.value().zip(g_2.value()).map(|(g_1, g_2)| {
+        let g1_g2_prime = g_1.inner().value().zip(g_2.value()).map(|(g_1, g_2)| {
             let two_pow_9 = pallas::Base::from(1u64 << 9);
             let two_pow_130 = pallas::Base::from_u128(1u128 << 65).square();
             let t_p = pallas::Base::from_u128(T_P);
@@ -995,41 +984,47 @@ impl NoteCommitConfig {
         &self,
         mut layouter: impl Layouter<pallas::Base>,
         y: AssignedCell<pallas::Base, pallas::Base>,
-        lsb: Option<pallas::Base>,
-    ) -> Result<AssignedCell<pallas::Base, pallas::Base>, Error> {
+        lsb: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+    ) -> Result<RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>, Error>
+    {
         // Decompose the field element
         //      y = LSB || k_0 || k_1 || k_2 || k_3
         //        = (bit 0) || (bits 1..=9) || (bits 10..=249) || (bits 250..=253) || (bit 254)
-        let (k_0, k_1, k_2, k_3) = {
-            let k_0 = y.value().map(|y| bitrange_subset(y, 1..10));
-            let k_1 = y.value().map(|y| bitrange_subset(y, 10..250));
-            let k_2 = y.value().map(|y| bitrange_subset(y, 250..254));
-            let k_3 = y.value().map(|y| bitrange_subset(y, 254..255));
-
-            (k_0, k_1, k_2, k_3)
-        };
 
         // Range-constrain k_0 to be 9 bits.
-        let k_0 = self.sinsemilla_config.lookup_config().witness_short_check(
-            layouter.namespace(|| "Constrain k_0 to be 9 bits"),
-            k_0,
-            9,
+        let k_0 = RangeConstrained::witness_short(
+            &self.sinsemilla_config.lookup_config(),
+            layouter.namespace(|| "k_0"),
+            y.value(),
+            1..10,
         )?;
 
+        // k_1 will be constrained by the decomposition of j.
+        let k_1 = RangeConstrained::subset_of(y.value(), 10..250);
+
         // Range-constrain k_2 to be 4 bits.
-        let k_2 = self.sinsemilla_config.lookup_config().witness_short_check(
-            layouter.namespace(|| "Constrain k_2 to be 4 bits"),
-            k_2,
-            4,
+        let k_2 = RangeConstrained::witness_short(
+            &self.sinsemilla_config.lookup_config(),
+            layouter.namespace(|| "k_2"),
+            y.value(),
+            250..254,
         )?;
 
+        // k_3 will be boolean-constrained in the gate.
+        let k_3 = RangeConstrained::subset_of(y.value(), 254..255);
+
         // Decompose j = LSB + (2)k_0 + (2^10)k_1 using 25 ten-bit lookups.
         let (j, z1_j, z13_j) = {
-            let j = lsb.zip(k_0.value()).zip(k_1).map(|((lsb, k_0), k_1)| {
-                let two = pallas::Base::from(2);
-                let two_pow_10 = pallas::Base::from(1 << 10);
-                lsb + two * k_0 + two_pow_10 * k_1
-            });
+            let j = lsb
+                .inner()
+                .value()
+                .zip(k_0.inner().value())
+                .zip(k_1.inner().value())
+                .map(|((lsb, k_0), k_1)| {
+                    let two = pallas::Base::from(2);
+                    let two_pow_10 = pallas::Base::from(1 << 10);
+                    lsb + two * k_0 + two_pow_10 * k_1
+                });
             let zs = self.sinsemilla_config.lookup_config().witness_check(
                 layouter.namespace(|| "Decompose j = LSB + (2)k_0 + (2^10)k_1"),
                 j,
@@ -1067,22 +1062,28 @@ impl NoteCommitConfig {
                     // Copy y.
                     y.copy_advice(|| "copy y", &mut region, self.advices[5], offset)?;
                     // Witness LSB.
-                    let lsb = region.assign_advice(
-                        || "witness LSB",
-                        self.advices[6],
-                        offset,
-                        || lsb.ok_or(Error::Synthesis),
-                    )?;
+                    let lsb = region
+                        .assign_advice(
+                            || "witness LSB",
+                            self.advices[6],
+                            offset,
+                            || lsb.inner().value().copied().ok_or(Error::Synthesis),
+                        )
+                        // SAFETY: This is sound because we just assigned this cell from a
+                        // range-constrained value.
+                        .map(|cell| RangeConstrained::unsound_unchecked(cell, lsb.num_bits()))?;
                     // Witness k_0.
-                    k_0.copy_advice(|| "copy k_0", &mut region, self.advices[7], offset)?;
+                    k_0.inner()
+                        .copy_advice(|| "copy k_0", &mut region, self.advices[7], offset)?;
                     // Copy k_2.
-                    k_2.copy_advice(|| "copy k_2", &mut region, self.advices[8], offset)?;
+                    k_2.inner()
+                        .copy_advice(|| "copy k_2", &mut region, self.advices[8], offset)?;
                     // Witness k_3.
                     region.assign_advice(
                         || "witness k_3",
                         self.advices[9],
                         offset,
-                        || k_3.ok_or(Error::Synthesis),
+                        || k_3.inner().ok_or(Error::Synthesis),
                     )?;
 
                     lsb
@@ -1137,19 +1138,22 @@ impl NoteCommitConfig {
                 gate_cells.b.copy_advice(|| "b", &mut region, col_l, 0)?;
                 gate_cells
                     .b_0
+                    .inner()
                     .copy_advice(|| "b_0", &mut region, col_m, 0)?;
                 let b_1 = region.assign_advice(
                     || "b_1",
                     col_r,
                     0,
-                    || gate_cells.b_1.ok_or(Error::Synthesis),
+                    || gate_cells.b_1.inner().ok_or(Error::Synthesis),
                 )?;
 
                 gate_cells
                     .b_2
+                    .inner()
                     .copy_advice(|| "b_2", &mut region, col_m, 1)?;
                 gate_cells
                     .b_3
+                    .inner()
                     .copy_advice(|| "b_3", &mut region, col_r, 1)?;
 
                 Ok(b_1)
@@ -1170,14 +1174,16 @@ impl NoteCommitConfig {
                     || "d_0",
                     col_m,
                     0,
-                    || gate_cells.d_0.ok_or(Error::Synthesis),
+                    || gate_cells.d_0.inner().ok_or(Error::Synthesis),
                 )?;
                 gate_cells
                     .d_1
+                    .inner()
                     .copy_advice(|| "d_1", &mut region, col_r, 0)?;
 
                 gate_cells
                     .d_2
+                    .inner()
                     .copy_advice(|| "d_2", &mut region, col_m, 1)?;
                 gate_cells
                     .z1_d
@@ -1198,9 +1204,11 @@ impl NoteCommitConfig {
                 gate_cells.e.copy_advice(|| "e", &mut region, col_l, 0)?;
                 gate_cells
                     .e_0
+                    .inner()
                     .copy_advice(|| "e_0", &mut region, col_m, 0)?;
                 gate_cells
                     .e_1
+                    .inner()
                     .copy_advice(|| "e_1", &mut region, col_r, 0)?;
 
                 Ok(())
@@ -1221,11 +1229,12 @@ impl NoteCommitConfig {
                     || "g_0",
                     col_m,
                     0,
-                    || gate_cells.g_0.ok_or(Error::Synthesis),
+                    || gate_cells.g_0.inner().ok_or(Error::Synthesis),
                 )?;
 
                 gate_cells
                     .g_1
+                    .inner()
                     .copy_advice(|| "g_1", &mut region, col_l, 1)?;
                 gate_cells
                     .z1_g
@@ -1246,12 +1255,13 @@ impl NoteCommitConfig {
                 gate_cells.h.copy_advice(|| "h", &mut region, col_l, 0)?;
                 gate_cells
                     .h_0
+                    .inner()
                     .copy_advice(|| "h_0", &mut region, col_m, 0)?;
                 let h_1 = region.assign_advice(
                     || "h_1",
                     col_r,
                     0,
-                    || gate_cells.h_1.ok_or(Error::Synthesis),
+                    || gate_cells.h_1.inner().ok_or(Error::Synthesis),
                 )?;
 
                 Ok(h_1)
@@ -1271,6 +1281,7 @@ impl NoteCommitConfig {
 
                 gate_cells
                     .b_0
+                    .inner()
                     .copy_advice(|| "b_0", &mut region, col_m, 0)?;
                 b_1.copy_advice(|| "b_1", &mut region, col_m, 1)?;
 
@@ -1303,6 +1314,7 @@ impl NoteCommitConfig {
 
                 gate_cells
                     .b_3
+                    .inner()
                     .copy_advice(|| "b_3", &mut region, col_m, 0)?;
                 d_0.copy_advice(|| "d_0", &mut region, col_m, 1)?;
 
@@ -1334,12 +1346,14 @@ impl NoteCommitConfig {
                     .copy_advice(|| "value", &mut region, col_l, 0)?;
                 gate_cells
                     .d_2
+                    .inner()
                     .copy_advice(|| "d_2", &mut region, col_m, 0)?;
                 gate_cells
                     .z1_d
                     .copy_advice(|| "d3 = z1_d", &mut region, col_r, 0)?;
                 gate_cells
                     .e_0
+                    .inner()
                     .copy_advice(|| "e_0", &mut region, col_z, 0)?;
 
                 self.q_notecommit_value.enable(&mut region, 0)
@@ -1359,6 +1373,7 @@ impl NoteCommitConfig {
 
                 gate_cells
                     .e_1
+                    .inner()
                     .copy_advice(|| "e_1", &mut region, col_m, 0)?;
                 g_0.copy_advice(|| "g_0", &mut region, col_m, 1)?;
 
@@ -1393,10 +1408,12 @@ impl NoteCommitConfig {
                     .copy_advice(|| "psi", &mut region, col_l, 0)?;
                 gate_cells
                     .h_0
+                    .inner()
                     .copy_advice(|| "h_0", &mut region, col_l, 1)?;
 
                 gate_cells
                     .g_1
+                    .inner()
                     .copy_advice(|| "g_1", &mut region, col_m, 0)?;
                 h_1.copy_advice(|| "h_1", &mut region, col_m, 1)?;
 
@@ -1426,27 +1443,27 @@ impl NoteCommitConfig {
 struct GateCells {
     a: AssignedCell<pallas::Base, pallas::Base>,
     b: AssignedCell<pallas::Base, pallas::Base>,
-    b_0: AssignedCell<pallas::Base, pallas::Base>,
-    b_1: Option<pallas::Base>,
-    b_2: AssignedCell<pallas::Base, pallas::Base>,
-    b_3: AssignedCell<pallas::Base, pallas::Base>,
+    b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    b_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+    b_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     c: AssignedCell<pallas::Base, pallas::Base>,
     d: AssignedCell<pallas::Base, pallas::Base>,
-    d_0: Option<pallas::Base>,
-    d_1: AssignedCell<pallas::Base, pallas::Base>,
-    d_2: AssignedCell<pallas::Base, pallas::Base>,
+    d_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+    d_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    d_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     z1_d: AssignedCell<pallas::Base, pallas::Base>,
     e: AssignedCell<pallas::Base, pallas::Base>,
-    e_0: AssignedCell<pallas::Base, pallas::Base>,
-    e_1: AssignedCell<pallas::Base, pallas::Base>,
+    e_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     f: AssignedCell<pallas::Base, pallas::Base>,
     g: AssignedCell<pallas::Base, pallas::Base>,
-    g_0: Option<pallas::Base>,
-    g_1: AssignedCell<pallas::Base, pallas::Base>,
+    g_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+    g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     z1_g: AssignedCell<pallas::Base, pallas::Base>,
     h: AssignedCell<pallas::Base, pallas::Base>,
-    h_0: AssignedCell<pallas::Base, pallas::Base>,
-    h_1: Option<pallas::Base>,
+    h_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    h_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     gd_x: AssignedCell<pallas::Base, pallas::Base>,
     pkd_x: AssignedCell<pallas::Base, pallas::Base>,
     value: AssignedCell<pallas::Base, pallas::Base>,

From 314728aadae850c9439f0358d5c19d346ffbf289 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Mon, 2 May 2022 23:42:18 +0000
Subject: [PATCH 11/21] Update comments on `gadget::commit_ivk`

---
 src/circuit/commit_ivk.rs | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/circuit/commit_ivk.rs b/src/circuit/commit_ivk.rs
index d755d5bc..c3d2c0cc 100644
--- a/src/circuit/commit_ivk.rs
+++ b/src/circuit/commit_ivk.rs
@@ -105,10 +105,8 @@ impl CommitIvkChip {
             // Check that d_whole is consistent with the witnessed subpieces.
             let d_decomposition_check = d_whole - (d_0.clone() + d_1.clone() * two_pow_9);
 
-            // Check `b_1` is a single-bit value
+            // Check `b_1` and `d_1` are each a single-bit value.
             let b1_bool_check = bool_check(b_1.clone());
-
-            // Check `d_1` is a single-bit value
             let d1_bool_check = bool_check(d_1.clone());
 
             // Check that ak = a (250 bits) || b_0 (4 bits) || b_1 (1 bit)
@@ -256,6 +254,9 @@ pub(in crate::circuit) mod gadgets {
         //   = (bits 250..=253 of `ak`) || (bit 254 of  `ak`) || (bits 0..=4 of  `nk`)
         // c = bits 5..=244 of `nk`
         // d = d_0||d_1` = (bits 245..=253 of `nk`) || (bit 254 of `nk`)
+        //
+        // We start by witnessing all of the individual pieces, and range-constraining
+        // the short pieces b_0, b_2, and d_0.
 
         // `a` = bits 0..=249 of `ak`
         let a = MessagePiece::from_subpieces(
@@ -321,6 +322,11 @@ pub(in crate::circuit) mod gadgets {
             (d_0, d_1, d)
         };
 
+        // ivk = Commit^ivk_rivk(I2LEBSP_255(ak) || I2LEBSP_255(nk))
+        //
+        // `ivk = ⊥` is handled internally to `CommitDomain::short_commit`: incomplete
+        // addition constraints allows ⊥ to occur, and then during synthesis it detects
+        // these edge cases and raises an error (aborting proof creation).
         let (ivk, zs) = {
             let message = Message::from_pieces(
                 sinsemilla_chip.clone(),
@@ -331,6 +337,9 @@ pub(in crate::circuit) mod gadgets {
             domain.short_commit(layouter.namespace(|| "Hash ak||nk"), message, rivk)?
         };
 
+        // `CommitDomain::short_commit` returns the running sum for each `MessagePiece`.
+        // Grab the outputs for pieces `a` and `c` that we will need for canonicity checks
+        // on `ak` and `nk`.
         let z13_a = zs[0][13].clone();
         let z13_c = zs[2][13].clone();
 
@@ -375,8 +384,8 @@ pub(in crate::circuit) mod gadgets {
         Ok(ivk)
     }
 
+    /// Witnesses and decomposes the `a'` value we need to check the canonicity of `ak`.
     #[allow(clippy::type_complexity)]
-    // Check canonicity of `ak` encoding
     fn ak_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
@@ -409,13 +418,13 @@ pub(in crate::circuit) mod gadgets {
             false,
         )?;
         let a_prime = zs[0].clone();
-        assert_eq!(zs.len(), 14); // [z_0, z_1, ..., z13_a]
+        assert_eq!(zs.len(), 14); // [z_0, z_1, ..., z13]
 
         Ok((a_prime, zs[13].clone()))
     }
 
+    /// Witnesses and decomposes the `b2c'` value we need to check the canonicity of `nk`.
     #[allow(clippy::type_complexity)]
-    // Check canonicity of `nk` encoding
     fn nk_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
@@ -459,7 +468,7 @@ pub(in crate::circuit) mod gadgets {
 }
 
 impl CommitIvkConfig {
-    // Assign cells for the canonicity gate.
+    /// Assign cells for the canonicity gate.
     /*
         The pieces are laid out in this configuration:
 

From 0bad10d3eb671c0438afedb9789df0dfe456c440 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Tue, 3 May 2022 20:02:29 +0000
Subject: [PATCH 12/21] Replace `UtilitiesInstructions` usage with a dedicated
 helper

The new helper enables returning typed `AssignedCell`s, rather than only
`AssignedCell<F, F>`.
---
 src/circuit.rs             | 29 ++++++++++++++---------------
 src/circuit/gadget.rs      | 29 ++++++++++++++++++++++++++++-
 src/circuit/note_commit.rs | 23 +++++++++++------------
 3 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 2adb3167..6cf58e11 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -4,7 +4,7 @@ use core::fmt;
 
 use group::{Curve, GroupEncoding};
 use halo2_proofs::{
-    circuit::{floor_planner, AssignedCell, Layouter},
+    circuit::{floor_planner, Layouter},
     plonk::{
         self, Advice, Column, Constraints, Expression, Instance as InstanceColumn, Selector,
         SingleVerifier,
@@ -18,7 +18,10 @@ use rand::RngCore;
 
 use self::{
     commit_ivk::{CommitIvkChip, CommitIvkConfig},
-    gadget::add_chip::{AddChip, AddConfig},
+    gadget::{
+        add_chip::{AddChip, AddConfig},
+        assign_free_advice,
+    },
     note_commit::NoteCommitConfig,
 };
 use crate::{
@@ -53,7 +56,7 @@ use halo2_gadgets::{
             MerklePath,
         },
     },
-    utilities::{lookup_range_check::LookupRangeCheckConfig, UtilitiesInstructions},
+    utilities::lookup_range_check::LookupRangeCheckConfig,
 };
 
 mod commit_ivk;
@@ -118,10 +121,6 @@ pub struct Circuit {
     pub(crate) rcv: Option<ValueCommitTrapdoor>,
 }
 
-impl UtilitiesInstructions<pallas::Base> for Circuit {
-    type Var = AssignedCell<pallas::Base, pallas::Base>;
-}
-
 impl plonk::Circuit<pallas::Base> for Circuit {
     type Config = Config;
     type FloorPlanner = floor_planner::V1;
@@ -332,14 +331,14 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         // Witness private inputs that are used across multiple checks.
         let (psi_old, rho_old, cm_old, g_d_old, ak_P, nk, v_old, v_new) = {
             // Witness psi_old
-            let psi_old = self.load_private(
+            let psi_old = assign_free_advice(
                 layouter.namespace(|| "witness psi_old"),
                 config.advices[0],
                 self.psi_old,
             )?;
 
             // Witness rho_old
-            let rho_old = self.load_private(
+            let rho_old = assign_free_advice(
                 layouter.namespace(|| "witness rho_old"),
                 config.advices[0],
                 self.rho_old.map(|rho| rho.0),
@@ -368,21 +367,21 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             )?;
 
             // Witness nk.
-            let nk = self.load_private(
+            let nk = assign_free_advice(
                 layouter.namespace(|| "witness nk"),
                 config.advices[0],
                 self.nk.map(|nk| nk.inner()),
             )?;
 
             // Witness v_old.
-            let v_old = self.load_private(
+            let v_old = assign_free_advice(
                 layouter.namespace(|| "witness v_old"),
                 config.advices[0],
                 self.v_old.map(|v_old| pallas::Base::from(v_old.inner())),
             )?;
 
             // Witness v_new.
-            let v_new = self.load_private(
+            let v_new = assign_free_advice(
                 layouter.namespace(|| "witness v_new"),
                 config.advices[0],
                 self.v_new.map(|v_new| pallas::Base::from(v_new.inner())),
@@ -426,12 +425,12 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                     )
                 });
 
-                let magnitude = self.load_private(
+                let magnitude = assign_free_advice(
                     layouter.namespace(|| "v_net magnitude"),
                     config.advices[9],
                     magnitude_sign.map(|m_s| m_s.0),
                 )?;
-                let sign = self.load_private(
+                let sign = assign_free_advice(
                     layouter.namespace(|| "v_net sign"),
                     config.advices[9],
                     magnitude_sign.map(|m_s| m_s.1),
@@ -582,7 +581,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             let rho_new = nf_old.inner().clone();
 
             // Witness psi_new
-            let psi_new = self.load_private(
+            let psi_new = assign_free_advice(
                 layouter.namespace(|| "witness psi_new"),
                 config.advices[0],
                 self.psi_new,
diff --git a/src/circuit/gadget.rs b/src/circuit/gadget.rs
index e2b2fc07..fb471a94 100644
--- a/src/circuit/gadget.rs
+++ b/src/circuit/gadget.rs
@@ -1,5 +1,6 @@
 //! Gadgets used in the Orchard circuit.
 
+use ff::Field;
 use pasta_curves::pallas;
 
 use super::commit_ivk::CommitIvkChip;
@@ -18,7 +19,7 @@ use halo2_gadgets::{
 use halo2_proofs::{
     arithmetic::FieldExt,
     circuit::{AssignedCell, Chip, Layouter},
-    plonk,
+    plonk::{self, Advice, Assigned, Column},
 };
 
 pub(in crate::circuit) mod add_chip;
@@ -76,6 +77,32 @@ pub(in crate::circuit) trait AddInstruction<F: FieldExt>: Chip<F> {
     ) -> Result<AssignedCell<F, F>, plonk::Error>;
 }
 
+/// Witnesses the given value in a standalone region.
+///
+/// Usages of this helper are technically superfluous, as the single-cell region is only
+/// ever used in equality constraints. We could eliminate them with a write-on-copy
+/// abstraction (https://github.com/zcash/halo2/issues/334).
+pub(in crate::circuit) fn assign_free_advice<F: Field, V: Copy>(
+    mut layouter: impl Layouter<F>,
+    column: Column<Advice>,
+    value: Option<V>,
+) -> Result<AssignedCell<V, F>, plonk::Error>
+where
+    for<'v> Assigned<F>: From<&'v V>,
+{
+    layouter.assign_region(
+        || "load private",
+        |mut region| {
+            region.assign_advice(
+                || "load private",
+                column,
+                0,
+                || value.ok_or(plonk::Error::Synthesis),
+            )
+        },
+    )
+}
+
 /// `ValueCommit^Orchard` from [Section 5.4.8.3 Homomorphic Pedersen commitments (Sapling and Orchard)].
 ///
 /// [Section 5.4.8.3 Homomorphic Pedersen commitments (Sapling and Orchard)]: https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit
diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index 01a0afbf..d98bf0d6 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -1488,9 +1488,12 @@ mod tests {
     use core::iter;
 
     use super::NoteCommitConfig;
-    use crate::constants::{
-        fixed_bases::NOTE_COMMITMENT_PERSONALIZATION, OrchardCommitDomains, OrchardFixedBases,
-        OrchardHashDomains, L_ORCHARD_BASE, L_VALUE, T_Q,
+    use crate::{
+        circuit::gadget::assign_free_advice,
+        constants::{
+            fixed_bases::NOTE_COMMITMENT_PERSONALIZATION, OrchardCommitDomains, OrchardFixedBases,
+            OrchardHashDomains, L_ORCHARD_BASE, L_VALUE, T_Q,
+        },
     };
     use halo2_gadgets::{
         ecc::{
@@ -1499,13 +1502,13 @@ mod tests {
         },
         primitives::sinsemilla::CommitDomain,
         sinsemilla::chip::SinsemillaChip,
-        utilities::{lookup_range_check::LookupRangeCheckConfig, UtilitiesInstructions},
+        utilities::lookup_range_check::LookupRangeCheckConfig,
     };
 
     use ff::{Field, PrimeField, PrimeFieldBits};
     use group::Curve;
     use halo2_proofs::{
-        circuit::{AssignedCell, Layouter, SimpleFloorPlanner},
+        circuit::{Layouter, SimpleFloorPlanner},
         dev::MockProver,
         plonk::{Circuit, ConstraintSystem, Error},
     };
@@ -1528,10 +1531,6 @@ mod tests {
             psi: Option<pallas::Base>,
         }
 
-        impl UtilitiesInstructions<pallas::Base> for MyCircuit {
-            type Var = AssignedCell<pallas::Base, pallas::Base>;
-        }
-
         impl Circuit<pallas::Base> for MyCircuit {
             type Config = (NoteCommitConfig, EccConfig<OrchardFixedBases>);
             type FloorPlanner = SimpleFloorPlanner;
@@ -1669,7 +1668,7 @@ mod tests {
                     pallas::Base::from(rng.next_u64())
                 };
                 let value_var = {
-                    self.load_private(
+                    assign_free_advice(
                         layouter.namespace(|| "witness value"),
                         note_commit_config.advices[0],
                         Some(value),
@@ -1677,14 +1676,14 @@ mod tests {
                 };
 
                 // Witness rho
-                let rho = self.load_private(
+                let rho = assign_free_advice(
                     layouter.namespace(|| "witness rho"),
                     note_commit_config.advices[0],
                     self.rho,
                 )?;
 
                 // Witness psi
-                let psi = self.load_private(
+                let psi = assign_free_advice(
                     layouter.namespace(|| "witness psi"),
                     note_commit_config.advices[0],
                     self.psi,

From c4bf8105f2d0d16372567ebf91b37de9519c3479 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Tue, 3 May 2022 20:26:41 +0000
Subject: [PATCH 13/21] Use `AssignedCell<NoteValue, _>` for circuit note
 values

---
 src/circuit.rs             |  4 ++--
 src/circuit/note_commit.rs | 21 ++++++++++++---------
 src/value.rs               |  7 +++++++
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 6cf58e11..7aeb77f8 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -377,14 +377,14 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             let v_old = assign_free_advice(
                 layouter.namespace(|| "witness v_old"),
                 config.advices[0],
-                self.v_old.map(|v_old| pallas::Base::from(v_old.inner())),
+                self.v_old,
             )?;
 
             // Witness v_new.
             let v_new = assign_free_advice(
                 layouter.namespace(|| "witness v_new"),
                 config.advices[0],
-                self.v_new.map(|v_new| pallas::Base::from(v_new.inner())),
+                self.v_new,
             )?;
 
             (psi_old, rho_old, cm_old, g_d_old, ak_P, nk, v_old, v_new)
diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index d98bf0d6..e61735f6 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -7,7 +7,10 @@ use halo2_proofs::{
 };
 use pasta_curves::{arithmetic::FieldExt, pallas};
 
-use crate::constants::{OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains, T_P};
+use crate::{
+    constants::{OrchardCommitDomains, OrchardFixedBases, OrchardHashDomains, T_P},
+    value::NoteValue,
+};
 use halo2_gadgets::{
     ecc::{
         chip::{EccChip, NonIdentityEccPoint},
@@ -550,8 +553,7 @@ impl NoteCommitConfig {
         ecc_chip: EccChip<OrchardFixedBases>,
         g_d: &NonIdentityEccPoint,
         pk_d: &NonIdentityEccPoint,
-        // TODO: Set V to Orchard value type
-        value: AssignedCell<pallas::Base, pallas::Base>,
+        value: AssignedCell<NoteValue, pallas::Base>,
         rho: AssignedCell<pallas::Base, pallas::Base>,
         psi: AssignedCell<pallas::Base, pallas::Base>,
         rcm: Option<pallas::Scalar>,
@@ -560,7 +562,7 @@ impl NoteCommitConfig {
         let (pkd_x, pkd_y) = (pk_d.x(), pk_d.y());
         let (gd_x, gd_y) = (gd_x.value(), gd_y.value());
         let (pkd_x, pkd_y) = (pkd_x.value(), pkd_y.value());
-        let value_val = value.value();
+        let value_val = value.value().map(|v| pallas::Base::from(v.inner()));
         let rho_val = rho.value();
         let psi_val = psi.value();
 
@@ -621,12 +623,12 @@ impl NoteCommitConfig {
             let d_2 = RangeConstrained::witness_short(
                 &self.sinsemilla_config.lookup_config(),
                 layouter.namespace(|| "d_2"),
-                value_val,
+                value_val.as_ref(),
                 0..8,
             )?;
 
             // d_3 = z1_d from the SinsemillaHash(d) running sum output.
-            let d_3 = RangeConstrained::subset_of(value_val, 8..58);
+            let d_3 = RangeConstrained::subset_of(value_val.as_ref(), 8..58);
 
             let d = MessagePiece::from_subpieces(
                 chip.clone(),
@@ -643,7 +645,7 @@ impl NoteCommitConfig {
             let e_0 = RangeConstrained::witness_short(
                 &self.sinsemilla_config.lookup_config(),
                 layouter.namespace(|| "e_0"),
-                value_val,
+                value_val.as_ref(),
                 58..64,
             )?;
 
@@ -1466,7 +1468,7 @@ struct GateCells {
     h_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     gd_x: AssignedCell<pallas::Base, pallas::Base>,
     pkd_x: AssignedCell<pallas::Base, pallas::Base>,
-    value: AssignedCell<pallas::Base, pallas::Base>,
+    value: AssignedCell<NoteValue, pallas::Base>,
     rho: AssignedCell<pallas::Base, pallas::Base>,
     psi: AssignedCell<pallas::Base, pallas::Base>,
     a_prime: AssignedCell<pallas::Base, pallas::Base>,
@@ -1494,6 +1496,7 @@ mod tests {
             fixed_bases::NOTE_COMMITMENT_PERSONALIZATION, OrchardCommitDomains, OrchardFixedBases,
             OrchardHashDomains, L_ORCHARD_BASE, L_VALUE, T_Q,
         },
+        value::NoteValue,
     };
     use halo2_gadgets::{
         ecc::{
@@ -1665,7 +1668,7 @@ mod tests {
                 // A note value cannot be negative.
                 let value = {
                     let mut rng = OsRng;
-                    pallas::Base::from(rng.next_u64())
+                    NoteValue::from_raw(rng.next_u64())
                 };
                 let value_var = {
                     assign_free_advice(
diff --git a/src/value.rs b/src/value.rs
index 751a80e6..38db80c9 100644
--- a/src/value.rs
+++ b/src/value.rs
@@ -42,6 +42,7 @@ use core::ops::{Add, RangeInclusive, Sub};
 use bitvec::{array::BitArray, order::Lsb0};
 use ff::{Field, PrimeField};
 use group::{Curve, Group, GroupEncoding};
+use halo2_proofs::plonk::Assigned;
 use pasta_curves::{
     arithmetic::{CurveAffine, CurveExt},
     pallas,
@@ -115,6 +116,12 @@ impl NoteValue {
     }
 }
 
+impl From<&NoteValue> for Assigned<pallas::Base> {
+    fn from(v: &NoteValue) -> Self {
+        pallas::Base::from(v.inner()).into()
+    }
+}
+
 impl Sub for NoteValue {
     type Output = ValueSum;
 

From f7ed3025473346ae201d41a3945cb5ecb371b9ec Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Tue, 3 May 2022 21:29:37 +0000
Subject: [PATCH 14/21] Refactor NoteCommit gate configuration into per-region
 structs

---
 src/circuit/note_commit.rs | 597 +++++++++++++++++++++++++++----------
 1 file changed, 444 insertions(+), 153 deletions(-)

diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index e61735f6..03d9e425 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -43,95 +43,29 @@ type CanonicityBounds = (
         - psi is a base field element (255 bits).
 */
 
-#[allow(non_snake_case)]
+/// | A_6 | A_7 | A_8 | q_notecommit_b |
+/// ------------------------------------
+/// |  b  | b_0 | b_1 |       1        |
+/// |     | b_2 | b_3 |       0        |
 #[derive(Clone, Debug)]
-pub struct NoteCommitConfig {
+struct DecomposeB {
     q_notecommit_b: Selector,
-    q_notecommit_d: Selector,
-    q_notecommit_e: Selector,
-    q_notecommit_g: Selector,
-    q_notecommit_h: Selector,
-    q_notecommit_g_d: Selector,
-    q_notecommit_pk_d: Selector,
-    q_notecommit_value: Selector,
-    q_notecommit_rho: Selector,
-    q_notecommit_psi: Selector,
-    q_y_canon: Selector,
-    advices: [Column<Advice>; 10],
-    sinsemilla_config:
-        SinsemillaConfig<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
 }
 
-impl NoteCommitConfig {
-    #[allow(non_snake_case)]
-    #[allow(clippy::many_single_char_names)]
-    pub(in crate::circuit) fn configure(
+impl DecomposeB {
+    fn configure(
         meta: &mut ConstraintSystem<pallas::Base>,
-        advices: [Column<Advice>; 10],
-        sinsemilla_config: SinsemillaConfig<
-            OrchardHashDomains,
-            OrchardCommitDomains,
-            OrchardFixedBases,
-        >,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        two_pow_4: pallas::Base,
+        two_pow_5: pallas::Base,
+        two_pow_6: pallas::Base,
     ) -> Self {
         let q_notecommit_b = meta.selector();
-        let q_notecommit_d = meta.selector();
-        let q_notecommit_e = meta.selector();
-        let q_notecommit_g = meta.selector();
-        let q_notecommit_h = meta.selector();
-        let q_notecommit_g_d = meta.selector();
-        let q_notecommit_pk_d = meta.selector();
-        let q_notecommit_value = meta.selector();
-        let q_notecommit_rho = meta.selector();
-        let q_notecommit_psi = meta.selector();
-        let q_y_canon = meta.selector();
 
-        let config = Self {
-            q_notecommit_b,
-            q_notecommit_d,
-            q_notecommit_e,
-            q_notecommit_g,
-            q_notecommit_h,
-            q_notecommit_g_d,
-            q_notecommit_pk_d,
-            q_notecommit_value,
-            q_notecommit_rho,
-            q_notecommit_psi,
-            q_y_canon,
-            advices,
-            sinsemilla_config,
-        };
-
-        // Useful constants
-        let two = pallas::Base::from(2);
-        let two_pow_2 = pallas::Base::from(1 << 2);
-        let two_pow_4 = two_pow_2.square();
-        let two_pow_5 = two_pow_4 * two;
-        let two_pow_6 = two_pow_5 * two;
-        let two_pow_8 = two_pow_4.square();
-        let two_pow_9 = two_pow_8 * two;
-        let two_pow_10 = two_pow_9 * two;
-        let two_pow_58 = pallas::Base::from(1 << 58);
-        let two_pow_130 = Expression::Constant(pallas::Base::from_u128(1 << 65).square());
-        let two_pow_140 = Expression::Constant(pallas::Base::from_u128(1 << 70).square());
-        let two_pow_249 = pallas::Base::from_u128(1 << 124).square() * two;
-        let two_pow_250 = two_pow_249 * two;
-        let two_pow_254 = pallas::Base::from_u128(1 << 127).square();
-
-        let t_p = Expression::Constant(pallas::Base::from_u128(T_P));
-
-        // Columns used for MessagePiece and message input gates.
-        let col_l = config.advices[6];
-        let col_m = config.advices[7];
-        let col_r = config.advices[8];
-        let col_z = config.advices[9];
-
-        // | A_6 | A_7 | A_8 | q_notecommit_b |
-        // ------------------------------------
-        // |  b  | b_0 | b_1 |       1        |
-        // |     | b_2 | b_3 |       0        |
         meta.create_gate("NoteCommit MessagePiece b", |meta| {
-            let q_notecommit_b = meta.query_selector(config.q_notecommit_b);
+            let q_notecommit_b = meta.query_selector(q_notecommit_b);
 
             // b has been constrained to 10 bits by the Sinsemilla hash.
             let b = meta.query_advice(col_l, Rotation::cur());
@@ -158,12 +92,33 @@ impl NoteCommitConfig {
             )
         });
 
-        // | A_6 | A_7 | A_8 | q_notecommit_d |
-        // ------------------------------------
-        // |  d  | d_0 | d_1 |       1        |
-        // |     | d_2 | d_3 |       0        |
+        Self { q_notecommit_b }
+    }
+}
+
+/// | A_6 | A_7 | A_8 | q_notecommit_d |
+/// ------------------------------------
+/// |  d  | d_0 | d_1 |       1        |
+/// |     | d_2 | d_3 |       0        |
+#[derive(Clone, Debug)]
+struct DecomposeD {
+    q_notecommit_d: Selector,
+}
+
+impl DecomposeD {
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        two: pallas::Base,
+        two_pow_2: pallas::Base,
+        two_pow_10: pallas::Base,
+    ) -> Self {
+        let q_notecommit_d = meta.selector();
+
         meta.create_gate("NoteCommit MessagePiece d", |meta| {
-            let q_notecommit_d = meta.query_selector(config.q_notecommit_d);
+            let q_notecommit_d = meta.query_selector(q_notecommit_d);
 
             // d has been constrained to 60 bits by the Sinsemilla hash.
             let d = meta.query_advice(col_l, Rotation::cur());
@@ -190,11 +145,30 @@ impl NoteCommitConfig {
             )
         });
 
-        // | A_6 | A_7 | A_8 | q_notecommit_e |
-        // ------------------------------------
-        // |  e  | e_0 | e_1 |       1        |
+        Self { q_notecommit_d }
+    }
+}
+
+/// | A_6 | A_7 | A_8 | q_notecommit_e |
+/// ------------------------------------
+/// |  e  | e_0 | e_1 |       1        |
+#[derive(Clone, Debug)]
+struct DecomposeE {
+    q_notecommit_e: Selector,
+}
+
+impl DecomposeE {
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        two_pow_6: pallas::Base,
+    ) -> Self {
+        let q_notecommit_e = meta.selector();
+
         meta.create_gate("NoteCommit MessagePiece e", |meta| {
-            let q_notecommit_e = meta.query_selector(config.q_notecommit_e);
+            let q_notecommit_e = meta.query_selector(q_notecommit_e);
 
             // e has been constrained to 10 bits by the Sinsemilla hash.
             let e = meta.query_advice(col_l, Rotation::cur());
@@ -209,12 +183,31 @@ impl NoteCommitConfig {
             Constraints::with_selector(q_notecommit_e, Some(("decomposition", decomposition_check)))
         });
 
-        // | A_6 | A_7 | q_notecommit_g |
-        // ------------------------------
-        // |  g  | g_0 |       1        |
-        // | g_1 | g_2 |       0        |
+        Self { q_notecommit_e }
+    }
+}
+
+/// | A_6 | A_7 | q_notecommit_g |
+/// ------------------------------
+/// |  g  | g_0 |       1        |
+/// | g_1 | g_2 |       0        |
+#[derive(Clone, Debug)]
+struct DecomposeG {
+    q_notecommit_g: Selector,
+}
+
+impl DecomposeG {
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        two: pallas::Base,
+        two_pow_10: pallas::Base,
+    ) -> Self {
+        let q_notecommit_g = meta.selector();
+
         meta.create_gate("NoteCommit MessagePiece g", |meta| {
-            let q_notecommit_g = meta.query_selector(config.q_notecommit_g);
+            let q_notecommit_g = meta.query_selector(q_notecommit_g);
 
             // g has been constrained to 250 bits by the Sinsemilla hash.
             let g = meta.query_advice(col_l, Rotation::cur());
@@ -237,11 +230,30 @@ impl NoteCommitConfig {
             )
         });
 
-        // | A_6 | A_7 | A_8 | q_notecommit_h |
-        // ------------------------------------
-        // |  h  | h_0 | h_1 |       1        |
+        Self { q_notecommit_g }
+    }
+}
+
+/// | A_6 | A_7 | A_8 | q_notecommit_h |
+/// ------------------------------------
+/// |  h  | h_0 | h_1 |       1        |
+#[derive(Clone, Debug)]
+struct DecomposeH {
+    q_notecommit_h: Selector,
+}
+
+impl DecomposeH {
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        two_pow_5: pallas::Base,
+    ) -> Self {
+        let q_notecommit_h = meta.selector();
+
         meta.create_gate("NoteCommit MessagePiece h", |meta| {
-            let q_notecommit_h = meta.query_selector(config.q_notecommit_h);
+            let q_notecommit_h = meta.query_selector(q_notecommit_h);
 
             // h has been constrained to 10 bits by the Sinsemilla hash.
             let h = meta.query_advice(col_l, Rotation::cur());
@@ -262,12 +274,36 @@ impl NoteCommitConfig {
             )
         });
 
-        // |  A_6   | A_7 |   A_8   |     A_9     | q_notecommit_g_d |
-        // -----------------------------------------------------------
-        // | x(g_d) | b_0 | a       | z13_a       |        1         |
-        // |        | b_1 | a_prime | z13_a_prime |        0         |
+        Self { q_notecommit_h }
+    }
+}
+
+/// |  A_6   | A_7 |   A_8   |     A_9     | q_notecommit_g_d |
+/// -----------------------------------------------------------
+/// | x(g_d) | b_0 | a       | z13_a       |        1         |
+/// |        | b_1 | a_prime | z13_a_prime |        0         |
+#[derive(Clone, Debug)]
+struct GdCanonicity {
+    q_notecommit_g_d: Selector,
+}
+
+impl GdCanonicity {
+    #[allow(clippy::too_many_arguments)]
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        col_z: Column<Advice>,
+        two_pow_130: Expression<pallas::Base>,
+        two_pow_250: pallas::Base,
+        two_pow_254: pallas::Base,
+        t_p: Expression<pallas::Base>,
+    ) -> Self {
+        let q_notecommit_g_d = meta.selector();
+
         meta.create_gate("NoteCommit input g_d", |meta| {
-            let q_notecommit_g_d = meta.query_selector(config.q_notecommit_g_d);
+            let q_notecommit_g_d = meta.query_selector(q_notecommit_g_d);
 
             let gd_x = meta.query_advice(col_l, Rotation::cur());
 
@@ -290,7 +326,7 @@ impl NoteCommitConfig {
             };
 
             // a_prime = a + 2^130 - t_P
-            let a_prime_check = a + two_pow_130.clone() - t_p.clone() - a_prime;
+            let a_prime_check = a + two_pow_130 - t_p - a_prime;
 
             // The gd_x_canonicity_checks are enforced if and only if `b_1` = 1.
             // x(g_d) = a (250 bits) || b_0 (4 bits) || b_1 (1 bit)
@@ -309,12 +345,36 @@ impl NoteCommitConfig {
             )
         });
 
-        // |   A_6   | A_7 |    A_8     |      A_9       | q_notecommit_pk_d |
-        // -------------------------------------------------------------------
-        // | x(pk_d) | b_3 |    c       | z13_c          |         1         |
-        // |         | d_0 | b3_c_prime | z14_b3_c_prime |         0         |
+        Self { q_notecommit_g_d }
+    }
+}
+
+/// |   A_6   | A_7 |    A_8     |      A_9       | q_notecommit_pk_d |
+/// -------------------------------------------------------------------
+/// | x(pk_d) | b_3 |    c       | z13_c          |         1         |
+/// |         | d_0 | b3_c_prime | z14_b3_c_prime |         0         |
+#[derive(Clone, Debug)]
+struct PkdCanonicity {
+    q_notecommit_pk_d: Selector,
+}
+
+impl PkdCanonicity {
+    #[allow(clippy::too_many_arguments)]
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        col_z: Column<Advice>,
+        two_pow_4: pallas::Base,
+        two_pow_140: Expression<pallas::Base>,
+        two_pow_254: pallas::Base,
+        t_p: Expression<pallas::Base>,
+    ) -> Self {
+        let q_notecommit_pk_d = meta.selector();
+
         meta.create_gate("NoteCommit input pk_d", |meta| {
-            let q_notecommit_pk_d = meta.query_selector(config.q_notecommit_pk_d);
+            let q_notecommit_pk_d = meta.query_selector(q_notecommit_pk_d);
 
             let pkd_x = meta.query_advice(col_l, Rotation::cur());
 
@@ -337,8 +397,7 @@ impl NoteCommitConfig {
             };
 
             // b3_c_prime = b_3 + (2^4)c + 2^140 - t_P
-            let b3_c_prime_check =
-                b_3 + (c * two_pow_4) + two_pow_140.clone() - t_p.clone() - b3_c_prime;
+            let b3_c_prime_check = b_3 + (c * two_pow_4) + two_pow_140 - t_p - b3_c_prime;
 
             // The pkd_x_canonicity_checks are enforced if and only if `d_0` = 1.
             // `x(pk_d)` = `b_3 (4 bits) || c (250 bits) || d_0 (1 bit)`
@@ -356,11 +415,32 @@ impl NoteCommitConfig {
             )
         });
 
-        // |  A_6  | A_7 | A_8 | A_9 | q_notecommit_value |
-        // ------------------------------------------------
-        // | value | d_2 | d_3 | e_0 |          1         |
+        Self { q_notecommit_pk_d }
+    }
+}
+
+/// |  A_6  | A_7 | A_8 | A_9 | q_notecommit_value |
+/// ------------------------------------------------
+/// | value | d_2 | d_3 | e_0 |          1         |
+#[derive(Clone, Debug)]
+struct ValueCanonicity {
+    q_notecommit_value: Selector,
+}
+
+impl ValueCanonicity {
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        col_z: Column<Advice>,
+        two_pow_8: pallas::Base,
+        two_pow_58: pallas::Base,
+    ) -> Self {
+        let q_notecommit_value = meta.selector();
+
         meta.create_gate("NoteCommit input value", |meta| {
-            let q_notecommit_value = meta.query_selector(config.q_notecommit_value);
+            let q_notecommit_value = meta.query_selector(q_notecommit_value);
 
             let value = meta.query_advice(col_l, Rotation::cur());
             // d_2 has been constrained to 8 bits outside this gate.
@@ -377,12 +457,36 @@ impl NoteCommitConfig {
             Constraints::with_selector(q_notecommit_value, Some(("value_check", value_check)))
         });
 
-        // | A_6 | A_7 |    A_8     |      A_9       | q_notecommit_rho |
-        // --------------------------------------------------------------
-        // | rho | e_1 |    f       | z13_f          |        1         |
-        // |     | g_0 | e1_f_prime | z14_e1_f_prime |        0         |
+        Self { q_notecommit_value }
+    }
+}
+
+/// | A_6 | A_7 |    A_8     |      A_9       | q_notecommit_rho |
+/// --------------------------------------------------------------
+/// | rho | e_1 |    f       | z13_f          |        1         |
+/// |     | g_0 | e1_f_prime | z14_e1_f_prime |        0         |
+#[derive(Clone, Debug)]
+struct RhoCanonicity {
+    q_notecommit_rho: Selector,
+}
+
+impl RhoCanonicity {
+    #[allow(clippy::too_many_arguments)]
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        col_z: Column<Advice>,
+        two_pow_4: pallas::Base,
+        two_pow_140: Expression<pallas::Base>,
+        two_pow_254: pallas::Base,
+        t_p: Expression<pallas::Base>,
+    ) -> Self {
+        let q_notecommit_rho = meta.selector();
+
         meta.create_gate("NoteCommit input rho", |meta| {
-            let q_notecommit_rho = meta.query_selector(config.q_notecommit_rho);
+            let q_notecommit_rho = meta.query_selector(q_notecommit_rho);
 
             let rho = meta.query_advice(col_l, Rotation::cur());
 
@@ -404,7 +508,7 @@ impl NoteCommitConfig {
             };
 
             // e1_f_prime = e_1 + (2^4)f + 2^140 - t_P
-            let e1_f_prime_check = e_1 + (f * two_pow_4) + two_pow_140 - t_p.clone() - e1_f_prime;
+            let e1_f_prime_check = e_1 + (f * two_pow_4) + two_pow_140 - t_p - e1_f_prime;
 
             // The rho_canonicity_checks are enforced if and only if `g_0` = 1.
             // rho = e_1 (4 bits) || f (250 bits) || g_0 (1 bit)
@@ -422,12 +526,37 @@ impl NoteCommitConfig {
             )
         });
 
-        // | A_6 | A_7 |     A_8     |       A_9       | q_notecommit_psi |
-        // ----------------------------------------------------------------
-        // | psi | g_1 |   g_2       | z13_g           |        1         |
-        // | h_0 | h_1 | g1_g2_prime | z13_g1_g2_prime |        0         |
+        Self { q_notecommit_rho }
+    }
+}
+
+/// | A_6 | A_7 |     A_8     |       A_9       | q_notecommit_psi |
+/// ----------------------------------------------------------------
+/// | psi | g_1 |   g_2       | z13_g           |        1         |
+/// | h_0 | h_1 | g1_g2_prime | z13_g1_g2_prime |        0         |
+#[derive(Clone, Debug)]
+struct PsiCanonicity {
+    q_notecommit_psi: Selector,
+}
+
+impl PsiCanonicity {
+    #[allow(clippy::too_many_arguments)]
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        col_l: Column<Advice>,
+        col_m: Column<Advice>,
+        col_r: Column<Advice>,
+        col_z: Column<Advice>,
+        two_pow_9: pallas::Base,
+        two_pow_130: Expression<pallas::Base>,
+        two_pow_249: pallas::Base,
+        two_pow_254: pallas::Base,
+        t_p: Expression<pallas::Base>,
+    ) -> Self {
+        let q_notecommit_psi = meta.selector();
+
         meta.create_gate("NoteCommit input psi", |meta| {
-            let q_notecommit_psi = meta.query_selector(config.q_notecommit_psi);
+            let q_notecommit_psi = meta.query_selector(q_notecommit_psi);
 
             let psi = meta.query_advice(col_l, Rotation::cur());
             let h_0 = meta.query_advice(col_l, Rotation::next());
@@ -452,8 +581,7 @@ impl NoteCommitConfig {
             };
 
             // g1_g2_prime = g_1 + (2^9)g_2 + 2^130 - t_P
-            let g1_g2_prime_check =
-                g_1 + (g_2 * two_pow_9) + two_pow_130.clone() - t_p.clone() - g1_g2_prime;
+            let g1_g2_prime_check = g_1 + (g_2 * two_pow_9) + two_pow_130 - t_p - g1_g2_prime;
 
             // The psi_canonicity_checks are enforced if and only if `h_1` = 1.
             // `psi` = `g_1 (9 bits) || g_2 (240 bits) || h_0 (5 bits) || h_1 (1 bit)`
@@ -472,20 +600,41 @@ impl NoteCommitConfig {
             )
         });
 
-        /*
-            Check decomposition and canonicity of y-coordinates.
-            This is used for both y(g_d) and y(pk_d).
+        Self { q_notecommit_psi }
+    }
+}
 
-            y = LSB || k_0 || k_1 || k_2 || k_3
-              = (bit 0) || (bits 1..=9) || (bits 10..=249) || (bits 250..=253) || (bit 254)
+/// Check decomposition and canonicity of y-coordinates.
+/// This is used for both y(g_d) and y(pk_d).
+///
+/// y = LSB || k_0 || k_1 || k_2 || k_3
+///   = (bit 0) || (bits 1..=9) || (bits 10..=249) || (bits 250..=253) || (bit 254)
+///
+/// These pieces are laid out in the following configuration:
+/// | A_5 | A_6 |  A_7  |   A_8   |     A_9     | q_y_canon |
+/// ---------------------------------------------------------
+/// |  y  | lsb |  k_0  |   k_2   |     k_3     |     1     |
+/// |  j  | z1_j| z13_j | j_prime | z13_j_prime |     0     |
+/// where z1_j = k_1.
+#[derive(Clone, Debug)]
+struct YCanonicity {
+    q_y_canon: Selector,
+}
+
+impl YCanonicity {
+    #[allow(clippy::too_many_arguments)]
+    fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        advices: [Column<Advice>; 10],
+        two: pallas::Base,
+        two_pow_10: pallas::Base,
+        two_pow_130: Expression<pallas::Base>,
+        two_pow_250: pallas::Base,
+        two_pow_254: pallas::Base,
+        t_p: Expression<pallas::Base>,
+    ) -> Self {
+        let q_y_canon = meta.selector();
 
-            These pieces are laid out in the following configuration:
-                    | A_5 | A_6 |  A_7  |   A_8   |     A_9     |
-                    ----------------------------------------------
-                    |  y  | lsb |  k_0  |   k_2   |     k_3     |
-                    |  j  | z1_j| z13_j | j_prime | z13_j_prime |
-            where z1_j = k_1.
-        */
         meta.create_gate("y coordinate checks", |meta| {
             let q_y_canon = meta.query_selector(q_y_canon);
             let y = meta.query_advice(advices[5], Rotation::cur());
@@ -519,7 +668,7 @@ impl NoteCommitConfig {
                 let y_check =
                     y - (j.clone() + k_2.clone() * two_pow_250 + k_3.clone() * two_pow_254);
                 // Check that j_prime = j + 2^130 - t_P
-                let j_prime_check = j + two_pow_130 - t_p.clone() - j_prime;
+                let j_prime_check = j + two_pow_130 - t_p - j_prime;
 
                 iter::empty()
                     .chain(Some(("k3_check", k3_check)))
@@ -540,7 +689,149 @@ impl NoteCommitConfig {
             Constraints::with_selector(q_y_canon, decomposition_checks.chain(canonicity_checks))
         });
 
-        config
+        Self { q_y_canon }
+    }
+}
+
+#[allow(non_snake_case)]
+#[derive(Clone, Debug)]
+pub struct NoteCommitConfig {
+    b: DecomposeB,
+    d: DecomposeD,
+    e: DecomposeE,
+    g: DecomposeG,
+    h: DecomposeH,
+    g_d: GdCanonicity,
+    pk_d: PkdCanonicity,
+    value: ValueCanonicity,
+    rho: RhoCanonicity,
+    psi: PsiCanonicity,
+    y_canon: YCanonicity,
+    advices: [Column<Advice>; 10],
+    sinsemilla_config:
+        SinsemillaConfig<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+}
+
+impl NoteCommitConfig {
+    #[allow(non_snake_case)]
+    #[allow(clippy::many_single_char_names)]
+    pub(in crate::circuit) fn configure(
+        meta: &mut ConstraintSystem<pallas::Base>,
+        advices: [Column<Advice>; 10],
+        sinsemilla_config: SinsemillaConfig<
+            OrchardHashDomains,
+            OrchardCommitDomains,
+            OrchardFixedBases,
+        >,
+    ) -> Self {
+        // Useful constants
+        let two = pallas::Base::from(2);
+        let two_pow_2 = pallas::Base::from(1 << 2);
+        let two_pow_4 = two_pow_2.square();
+        let two_pow_5 = two_pow_4 * two;
+        let two_pow_6 = two_pow_5 * two;
+        let two_pow_8 = two_pow_4.square();
+        let two_pow_9 = two_pow_8 * two;
+        let two_pow_10 = two_pow_9 * two;
+        let two_pow_58 = pallas::Base::from(1 << 58);
+        let two_pow_130 = Expression::Constant(pallas::Base::from_u128(1 << 65).square());
+        let two_pow_140 = Expression::Constant(pallas::Base::from_u128(1 << 70).square());
+        let two_pow_249 = pallas::Base::from_u128(1 << 124).square() * two;
+        let two_pow_250 = two_pow_249 * two;
+        let two_pow_254 = pallas::Base::from_u128(1 << 127).square();
+
+        let t_p = Expression::Constant(pallas::Base::from_u128(T_P));
+
+        // Columns used for MessagePiece and message input gates.
+        let col_l = advices[6];
+        let col_m = advices[7];
+        let col_r = advices[8];
+        let col_z = advices[9];
+
+        let b = DecomposeB::configure(meta, col_l, col_m, col_r, two_pow_4, two_pow_5, two_pow_6);
+        let d = DecomposeD::configure(meta, col_l, col_m, col_r, two, two_pow_2, two_pow_10);
+        let e = DecomposeE::configure(meta, col_l, col_m, col_r, two_pow_6);
+        let g = DecomposeG::configure(meta, col_l, col_m, two, two_pow_10);
+        let h = DecomposeH::configure(meta, col_l, col_m, col_r, two_pow_5);
+
+        let g_d = GdCanonicity::configure(
+            meta,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+            two_pow_130.clone(),
+            two_pow_250,
+            two_pow_254,
+            t_p.clone(),
+        );
+
+        let pk_d = PkdCanonicity::configure(
+            meta,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+            two_pow_4,
+            two_pow_140.clone(),
+            two_pow_254,
+            t_p.clone(),
+        );
+
+        let value =
+            ValueCanonicity::configure(meta, col_l, col_m, col_r, col_z, two_pow_8, two_pow_58);
+
+        let rho = RhoCanonicity::configure(
+            meta,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+            two_pow_4,
+            two_pow_140,
+            two_pow_254,
+            t_p.clone(),
+        );
+
+        let psi = PsiCanonicity::configure(
+            meta,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+            two_pow_9,
+            two_pow_130.clone(),
+            two_pow_249,
+            two_pow_254,
+            t_p.clone(),
+        );
+
+        let y_canon = YCanonicity::configure(
+            meta,
+            advices,
+            two,
+            two_pow_10,
+            two_pow_130,
+            two_pow_250,
+            two_pow_254,
+            t_p,
+        );
+
+        Self {
+            b,
+            d,
+            e,
+            g,
+            h,
+            g_d,
+            pk_d,
+            value,
+            rho,
+            psi,
+            y_canon,
+            advices,
+            sinsemilla_config,
+        }
     }
 
     #[allow(clippy::many_single_char_names)]
@@ -1055,7 +1346,7 @@ impl NoteCommitConfig {
         layouter.assign_region(
             || "y canonicity",
             |mut region| {
-                self.q_y_canon.enable(&mut region, 0)?;
+                self.y_canon.q_y_canon.enable(&mut region, 0)?;
 
                 // Offset 0
                 let lsb = {
@@ -1135,7 +1426,7 @@ impl NoteCommitConfig {
         let b_1 = layouter.assign_region(
             || "NoteCommit MessagePiece b",
             |mut region| {
-                self.q_notecommit_b.enable(&mut region, 0)?;
+                self.b.q_notecommit_b.enable(&mut region, 0)?;
 
                 gate_cells.b.copy_advice(|| "b", &mut region, col_l, 0)?;
                 gate_cells
@@ -1169,7 +1460,7 @@ impl NoteCommitConfig {
         let d_0 = layouter.assign_region(
             || "NoteCommit MessagePiece d",
             |mut region| {
-                self.q_notecommit_d.enable(&mut region, 0)?;
+                self.d.q_notecommit_d.enable(&mut region, 0)?;
 
                 gate_cells.d.copy_advice(|| "d", &mut region, col_l, 0)?;
                 let d_0 = region.assign_advice(
@@ -1201,7 +1492,7 @@ impl NoteCommitConfig {
         layouter.assign_region(
             || "NoteCommit MessagePiece e",
             |mut region| {
-                self.q_notecommit_e.enable(&mut region, 0)?;
+                self.e.q_notecommit_e.enable(&mut region, 0)?;
 
                 gate_cells.e.copy_advice(|| "e", &mut region, col_l, 0)?;
                 gate_cells
@@ -1224,7 +1515,7 @@ impl NoteCommitConfig {
         let g_0 = layouter.assign_region(
             || "NoteCommit MessagePiece g",
             |mut region| {
-                self.q_notecommit_g.enable(&mut region, 0)?;
+                self.g.q_notecommit_g.enable(&mut region, 0)?;
 
                 gate_cells.g.copy_advice(|| "g", &mut region, col_l, 0)?;
                 let g_0 = region.assign_advice(
@@ -1252,7 +1543,7 @@ impl NoteCommitConfig {
         let h_1 = layouter.assign_region(
             || "NoteCommit MessagePiece h",
             |mut region| {
-                self.q_notecommit_h.enable(&mut region, 0)?;
+                self.h.q_notecommit_h.enable(&mut region, 0)?;
 
                 gate_cells.h.copy_advice(|| "h", &mut region, col_l, 0)?;
                 gate_cells
@@ -1299,7 +1590,7 @@ impl NoteCommitConfig {
                     .z13_a_prime
                     .copy_advice(|| "z13_a_prime", &mut region, col_z, 1)?;
 
-                self.q_notecommit_g_d.enable(&mut region, 0)
+                self.g_d.q_notecommit_g_d.enable(&mut region, 0)
             },
         )?;
 
@@ -1335,7 +1626,7 @@ impl NoteCommitConfig {
                     1,
                 )?;
 
-                self.q_notecommit_pk_d.enable(&mut region, 0)
+                self.pk_d.q_notecommit_pk_d.enable(&mut region, 0)
             },
         )?;
 
@@ -1358,7 +1649,7 @@ impl NoteCommitConfig {
                     .inner()
                     .copy_advice(|| "e_0", &mut region, col_z, 0)?;
 
-                self.q_notecommit_value.enable(&mut region, 0)
+                self.value.q_notecommit_value.enable(&mut region, 0)
             },
         )?;
 
@@ -1394,7 +1685,7 @@ impl NoteCommitConfig {
                     1,
                 )?;
 
-                self.q_notecommit_rho.enable(&mut region, 0)
+                self.rho.q_notecommit_rho.enable(&mut region, 0)
             },
         )?;
 
@@ -1436,7 +1727,7 @@ impl NoteCommitConfig {
                     1,
                 )?;
 
-                self.q_notecommit_psi.enable(&mut region, 0)
+                self.psi.q_notecommit_psi.enable(&mut region, 0)
             },
         )
     }

From 3ced2c9c0bc4787e1c055b5244d94cbe2fd443b6 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Tue, 3 May 2022 22:28:21 +0000
Subject: [PATCH 15/21] Refactor NoteCommit region assignment onto per-region
 structs

---
 src/circuit/note_commit.rs | 947 ++++++++++++++++++++++---------------
 1 file changed, 567 insertions(+), 380 deletions(-)

diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index 03d9e425..638b2ce0 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -50,6 +50,9 @@ type CanonicityBounds = (
 #[derive(Clone, Debug)]
 struct DecomposeB {
     q_notecommit_b: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
 }
 
 impl DecomposeB {
@@ -92,7 +95,46 @@ impl DecomposeB {
             )
         });
 
-        Self { q_notecommit_b }
+        Self {
+            q_notecommit_b,
+            col_l,
+            col_m,
+            col_r,
+        }
+    }
+
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        b: AssignedCell<pallas::Base, pallas::Base>,
+        b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        b_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+        b_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    ) -> Result<AssignedCell<pallas::Base, pallas::Base>, Error> {
+        layouter.assign_region(
+            || "NoteCommit MessagePiece b",
+            |mut region| {
+                self.q_notecommit_b.enable(&mut region, 0)?;
+
+                b.copy_advice(|| "b", &mut region, self.col_l, 0)?;
+                b_0.inner()
+                    .copy_advice(|| "b_0", &mut region, self.col_m, 0)?;
+                let b_1 = region.assign_advice(
+                    || "b_1",
+                    self.col_r,
+                    0,
+                    || b_1.inner().ok_or(Error::Synthesis),
+                )?;
+
+                b_2.inner()
+                    .copy_advice(|| "b_2", &mut region, self.col_m, 1)?;
+                b_3.inner()
+                    .copy_advice(|| "b_3", &mut region, self.col_r, 1)?;
+
+                Ok(b_1)
+            },
+        )
     }
 }
 
@@ -103,6 +145,9 @@ impl DecomposeB {
 #[derive(Clone, Debug)]
 struct DecomposeD {
     q_notecommit_d: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
 }
 
 impl DecomposeD {
@@ -145,7 +190,45 @@ impl DecomposeD {
             )
         });
 
-        Self { q_notecommit_d }
+        Self {
+            q_notecommit_d,
+            col_l,
+            col_m,
+            col_r,
+        }
+    }
+
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        d: AssignedCell<pallas::Base, pallas::Base>,
+        d_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+        d_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        d_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        z1_d: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<AssignedCell<pallas::Base, pallas::Base>, Error> {
+        layouter.assign_region(
+            || "NoteCommit MessagePiece d",
+            |mut region| {
+                self.q_notecommit_d.enable(&mut region, 0)?;
+
+                d.copy_advice(|| "d", &mut region, self.col_l, 0)?;
+                let d_0 = region.assign_advice(
+                    || "d_0",
+                    self.col_m,
+                    0,
+                    || d_0.inner().ok_or(Error::Synthesis),
+                )?;
+                d_1.inner()
+                    .copy_advice(|| "d_1", &mut region, self.col_r, 0)?;
+
+                d_2.inner()
+                    .copy_advice(|| "d_2", &mut region, self.col_m, 1)?;
+                z1_d.copy_advice(|| "d_3 = z1_d", &mut region, self.col_r, 1)?;
+
+                Ok(d_0)
+            },
+        )
     }
 }
 
@@ -155,6 +238,9 @@ impl DecomposeD {
 #[derive(Clone, Debug)]
 struct DecomposeE {
     q_notecommit_e: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
 }
 
 impl DecomposeE {
@@ -183,7 +269,35 @@ impl DecomposeE {
             Constraints::with_selector(q_notecommit_e, Some(("decomposition", decomposition_check)))
         });
 
-        Self { q_notecommit_e }
+        Self {
+            q_notecommit_e,
+            col_l,
+            col_m,
+            col_r,
+        }
+    }
+
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        e: AssignedCell<pallas::Base, pallas::Base>,
+        e_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    ) -> Result<(), Error> {
+        layouter.assign_region(
+            || "NoteCommit MessagePiece e",
+            |mut region| {
+                self.q_notecommit_e.enable(&mut region, 0)?;
+
+                e.copy_advice(|| "e", &mut region, self.col_l, 0)?;
+                e_0.inner()
+                    .copy_advice(|| "e_0", &mut region, self.col_m, 0)?;
+                e_1.inner()
+                    .copy_advice(|| "e_1", &mut region, self.col_r, 0)?;
+
+                Ok(())
+            },
+        )
     }
 }
 
@@ -194,6 +308,8 @@ impl DecomposeE {
 #[derive(Clone, Debug)]
 struct DecomposeG {
     q_notecommit_g: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
 }
 
 impl DecomposeG {
@@ -230,7 +346,41 @@ impl DecomposeG {
             )
         });
 
-        Self { q_notecommit_g }
+        Self {
+            q_notecommit_g,
+            col_l,
+            col_m,
+        }
+    }
+
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        g: AssignedCell<pallas::Base, pallas::Base>,
+        g_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+        g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        z1_g: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<AssignedCell<pallas::Base, pallas::Base>, Error> {
+        layouter.assign_region(
+            || "NoteCommit MessagePiece g",
+            |mut region| {
+                self.q_notecommit_g.enable(&mut region, 0)?;
+
+                g.copy_advice(|| "g", &mut region, self.col_l, 0)?;
+                let g_0 = region.assign_advice(
+                    || "g_0",
+                    self.col_m,
+                    0,
+                    || g_0.inner().ok_or(Error::Synthesis),
+                )?;
+
+                g_1.inner()
+                    .copy_advice(|| "g_1", &mut region, self.col_l, 1)?;
+                z1_g.copy_advice(|| "g_2 = z1_g", &mut region, self.col_m, 1)?;
+
+                Ok(g_0)
+            },
+        )
     }
 }
 
@@ -240,6 +390,9 @@ impl DecomposeG {
 #[derive(Clone, Debug)]
 struct DecomposeH {
     q_notecommit_h: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
 }
 
 impl DecomposeH {
@@ -274,7 +427,39 @@ impl DecomposeH {
             )
         });
 
-        Self { q_notecommit_h }
+        Self {
+            q_notecommit_h,
+            col_l,
+            col_m,
+            col_r,
+        }
+    }
+
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        h: AssignedCell<pallas::Base, pallas::Base>,
+        h_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        h_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+    ) -> Result<AssignedCell<pallas::Base, pallas::Base>, Error> {
+        layouter.assign_region(
+            || "NoteCommit MessagePiece h",
+            |mut region| {
+                self.q_notecommit_h.enable(&mut region, 0)?;
+
+                h.copy_advice(|| "h", &mut region, self.col_l, 0)?;
+                h_0.inner()
+                    .copy_advice(|| "h_0", &mut region, self.col_m, 0)?;
+                let h_1 = region.assign_advice(
+                    || "h_1",
+                    self.col_r,
+                    0,
+                    || h_1.inner().ok_or(Error::Synthesis),
+                )?;
+
+                Ok(h_1)
+            },
+        )
     }
 }
 
@@ -285,6 +470,10 @@ impl DecomposeH {
 #[derive(Clone, Debug)]
 struct GdCanonicity {
     q_notecommit_g_d: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
+    col_z: Column<Advice>,
 }
 
 impl GdCanonicity {
@@ -345,7 +534,45 @@ impl GdCanonicity {
             )
         });
 
-        Self { q_notecommit_g_d }
+        Self {
+            q_notecommit_g_d,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+        }
+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        gd_x: AssignedCell<pallas::Base, pallas::Base>,
+        a: AssignedCell<pallas::Base, pallas::Base>,
+        b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        b_1: AssignedCell<pallas::Base, pallas::Base>,
+        a_prime: AssignedCell<pallas::Base, pallas::Base>,
+        z13_a: AssignedCell<pallas::Base, pallas::Base>,
+        z13_a_prime: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<(), Error> {
+        layouter.assign_region(
+            || "NoteCommit input g_d",
+            |mut region| {
+                gd_x.copy_advice(|| "gd_x", &mut region, self.col_l, 0)?;
+
+                b_0.inner()
+                    .copy_advice(|| "b_0", &mut region, self.col_m, 0)?;
+                b_1.copy_advice(|| "b_1", &mut region, self.col_m, 1)?;
+
+                a.copy_advice(|| "a", &mut region, self.col_r, 0)?;
+                a_prime.copy_advice(|| "a_prime", &mut region, self.col_r, 1)?;
+
+                z13_a.copy_advice(|| "z13_a", &mut region, self.col_z, 0)?;
+                z13_a_prime.copy_advice(|| "z13_a_prime", &mut region, self.col_z, 1)?;
+
+                self.q_notecommit_g_d.enable(&mut region, 0)
+            },
+        )
     }
 }
 
@@ -356,6 +583,10 @@ impl GdCanonicity {
 #[derive(Clone, Debug)]
 struct PkdCanonicity {
     q_notecommit_pk_d: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
+    col_z: Column<Advice>,
 }
 
 impl PkdCanonicity {
@@ -415,7 +646,45 @@ impl PkdCanonicity {
             )
         });
 
-        Self { q_notecommit_pk_d }
+        Self {
+            q_notecommit_pk_d,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+        }
+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        pkd_x: AssignedCell<pallas::Base, pallas::Base>,
+        b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        c: AssignedCell<pallas::Base, pallas::Base>,
+        d_0: AssignedCell<pallas::Base, pallas::Base>,
+        b3_c_prime: AssignedCell<pallas::Base, pallas::Base>,
+        z13_c: AssignedCell<pallas::Base, pallas::Base>,
+        z14_b3_c_prime: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<(), Error> {
+        layouter.assign_region(
+            || "NoteCommit input pk_d",
+            |mut region| {
+                pkd_x.copy_advice(|| "pkd_x", &mut region, self.col_l, 0)?;
+
+                b_3.inner()
+                    .copy_advice(|| "b_3", &mut region, self.col_m, 0)?;
+                d_0.copy_advice(|| "d_0", &mut region, self.col_m, 1)?;
+
+                c.copy_advice(|| "c", &mut region, self.col_r, 0)?;
+                b3_c_prime.copy_advice(|| "b3_c_prime", &mut region, self.col_r, 1)?;
+
+                z13_c.copy_advice(|| "z13_c", &mut region, self.col_z, 0)?;
+                z14_b3_c_prime.copy_advice(|| "z14_b3_c_prime", &mut region, self.col_z, 1)?;
+
+                self.q_notecommit_pk_d.enable(&mut region, 0)
+            },
+        )
     }
 }
 
@@ -425,6 +694,10 @@ impl PkdCanonicity {
 #[derive(Clone, Debug)]
 struct ValueCanonicity {
     q_notecommit_value: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
+    col_z: Column<Advice>,
 }
 
 impl ValueCanonicity {
@@ -457,7 +730,36 @@ impl ValueCanonicity {
             Constraints::with_selector(q_notecommit_value, Some(("value_check", value_check)))
         });
 
-        Self { q_notecommit_value }
+        Self {
+            q_notecommit_value,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+        }
+    }
+
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        value: AssignedCell<NoteValue, pallas::Base>,
+        d_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        z1_d: AssignedCell<pallas::Base, pallas::Base>,
+        e_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+    ) -> Result<(), Error> {
+        layouter.assign_region(
+            || "NoteCommit input value",
+            |mut region| {
+                value.copy_advice(|| "value", &mut region, self.col_l, 0)?;
+                d_2.inner()
+                    .copy_advice(|| "d_2", &mut region, self.col_m, 0)?;
+                z1_d.copy_advice(|| "d3 = z1_d", &mut region, self.col_r, 0)?;
+                e_0.inner()
+                    .copy_advice(|| "e_0", &mut region, self.col_z, 0)?;
+
+                self.q_notecommit_value.enable(&mut region, 0)
+            },
+        )
     }
 }
 
@@ -468,6 +770,10 @@ impl ValueCanonicity {
 #[derive(Clone, Debug)]
 struct RhoCanonicity {
     q_notecommit_rho: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
+    col_z: Column<Advice>,
 }
 
 impl RhoCanonicity {
@@ -526,7 +832,45 @@ impl RhoCanonicity {
             )
         });
 
-        Self { q_notecommit_rho }
+        Self {
+            q_notecommit_rho,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+        }
+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        rho: AssignedCell<pallas::Base, pallas::Base>,
+        e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        f: AssignedCell<pallas::Base, pallas::Base>,
+        g_0: AssignedCell<pallas::Base, pallas::Base>,
+        e1_f_prime: AssignedCell<pallas::Base, pallas::Base>,
+        z13_f: AssignedCell<pallas::Base, pallas::Base>,
+        z14_e1_f_prime: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<(), Error> {
+        layouter.assign_region(
+            || "NoteCommit input rho",
+            |mut region| {
+                rho.copy_advice(|| "rho", &mut region, self.col_l, 0)?;
+
+                e_1.inner()
+                    .copy_advice(|| "e_1", &mut region, self.col_m, 0)?;
+                g_0.copy_advice(|| "g_0", &mut region, self.col_m, 1)?;
+
+                f.copy_advice(|| "f", &mut region, self.col_r, 0)?;
+                e1_f_prime.copy_advice(|| "e1_f_prime", &mut region, self.col_r, 1)?;
+
+                z13_f.copy_advice(|| "z13_f", &mut region, self.col_z, 0)?;
+                z14_e1_f_prime.copy_advice(|| "z14_e1_f_prime", &mut region, self.col_z, 1)?;
+
+                self.q_notecommit_rho.enable(&mut region, 0)
+            },
+        )
     }
 }
 
@@ -537,6 +881,10 @@ impl RhoCanonicity {
 #[derive(Clone, Debug)]
 struct PsiCanonicity {
     q_notecommit_psi: Selector,
+    col_l: Column<Advice>,
+    col_m: Column<Advice>,
+    col_r: Column<Advice>,
+    col_z: Column<Advice>,
 }
 
 impl PsiCanonicity {
@@ -600,7 +948,48 @@ impl PsiCanonicity {
             )
         });
 
-        Self { q_notecommit_psi }
+        Self {
+            q_notecommit_psi,
+            col_l,
+            col_m,
+            col_r,
+            col_z,
+        }
+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        psi: AssignedCell<pallas::Base, pallas::Base>,
+        g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        z1_g: AssignedCell<pallas::Base, pallas::Base>,
+        h_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        h_1: AssignedCell<pallas::Base, pallas::Base>,
+        g1_g2_prime: AssignedCell<pallas::Base, pallas::Base>,
+        z13_g: AssignedCell<pallas::Base, pallas::Base>,
+        z13_g1_g2_prime: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<(), Error> {
+        layouter.assign_region(
+            || "NoteCommit input psi",
+            |mut region| {
+                psi.copy_advice(|| "psi", &mut region, self.col_l, 0)?;
+                h_0.inner()
+                    .copy_advice(|| "h_0", &mut region, self.col_l, 1)?;
+
+                g_1.inner()
+                    .copy_advice(|| "g_1", &mut region, self.col_m, 0)?;
+                h_1.copy_advice(|| "h_1", &mut region, self.col_m, 1)?;
+
+                z1_g.copy_advice(|| "g_2 = z1_g", &mut region, self.col_r, 0)?;
+                g1_g2_prime.copy_advice(|| "g1_g2_prime", &mut region, self.col_r, 1)?;
+
+                z13_g.copy_advice(|| "z13_g", &mut region, self.col_z, 0)?;
+                z13_g1_g2_prime.copy_advice(|| "z13_g1_g2_prime", &mut region, self.col_z, 1)?;
+
+                self.q_notecommit_psi.enable(&mut region, 0)
+            },
+        )
     }
 }
 
@@ -619,6 +1008,7 @@ impl PsiCanonicity {
 #[derive(Clone, Debug)]
 struct YCanonicity {
     q_y_canon: Selector,
+    advices: [Column<Advice>; 10],
 }
 
 impl YCanonicity {
@@ -689,7 +1079,88 @@ impl YCanonicity {
             Constraints::with_selector(q_y_canon, decomposition_checks.chain(canonicity_checks))
         });
 
-        Self { q_y_canon }
+        Self { q_y_canon, advices }
+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn assign(
+        &self,
+        layouter: &mut impl Layouter<pallas::Base>,
+        y: AssignedCell<pallas::Base, pallas::Base>,
+        lsb: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+        k_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        k_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        k_3: RangeConstrained<pallas::Base, Option<pallas::Base>>,
+        j: AssignedCell<pallas::Base, pallas::Base>,
+        z1_j: AssignedCell<pallas::Base, pallas::Base>,
+        z13_j: AssignedCell<pallas::Base, pallas::Base>,
+        j_prime: AssignedCell<pallas::Base, pallas::Base>,
+        z13_j_prime: AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>, Error>
+    {
+        layouter.assign_region(
+            || "y canonicity",
+            |mut region| {
+                self.q_y_canon.enable(&mut region, 0)?;
+
+                // Offset 0
+                let lsb = {
+                    let offset = 0;
+
+                    // Copy y.
+                    y.copy_advice(|| "copy y", &mut region, self.advices[5], offset)?;
+                    // Witness LSB.
+                    let lsb = region
+                        .assign_advice(
+                            || "witness LSB",
+                            self.advices[6],
+                            offset,
+                            || lsb.inner().ok_or(Error::Synthesis),
+                        )
+                        // SAFETY: This is sound because we just assigned this cell from a
+                        // range-constrained value.
+                        .map(|cell| RangeConstrained::unsound_unchecked(cell, lsb.num_bits()))?;
+                    // Witness k_0.
+                    k_0.inner()
+                        .copy_advice(|| "copy k_0", &mut region, self.advices[7], offset)?;
+                    // Copy k_2.
+                    k_2.inner()
+                        .copy_advice(|| "copy k_2", &mut region, self.advices[8], offset)?;
+                    // Witness k_3.
+                    region.assign_advice(
+                        || "witness k_3",
+                        self.advices[9],
+                        offset,
+                        || k_3.inner().ok_or(Error::Synthesis),
+                    )?;
+
+                    lsb
+                };
+
+                // Offset 1
+                {
+                    let offset = 1;
+
+                    // Copy j.
+                    j.copy_advice(|| "copy j", &mut region, self.advices[5], offset)?;
+                    // Copy z1_j.
+                    z1_j.copy_advice(|| "copy z1_j", &mut region, self.advices[6], offset)?;
+                    // Copy z13_j.
+                    z13_j.copy_advice(|| "copy z13_j", &mut region, self.advices[7], offset)?;
+                    // Copy j_prime.
+                    j_prime.copy_advice(|| "copy j_prime", &mut region, self.advices[8], offset)?;
+                    // Copy z13_j_prime.
+                    z13_j_prime.copy_advice(
+                        || "copy z13_j_prime",
+                        &mut region,
+                        self.advices[9],
+                        offset,
+                    )?;
+                }
+
+                Ok(lsb)
+            },
+        )
     }
 }
 
@@ -1334,77 +1805,18 @@ impl NoteCommitConfig {
             j.clone(),
         )?;
 
-        /*
-
-            Assign y canonicity gate in the following configuration:
-                | A_5 | A_6 |  A_7  |   A_8   |     A_9     |
-                ----------------------------------------------
-                |  y  | lsb |  k_0  |   k_2   |     k_3     |
-                |  j  | z1_j| z13_j | j_prime | z13_j_prime |
-            where z1_j = k_1.
-        */
-        layouter.assign_region(
-            || "y canonicity",
-            |mut region| {
-                self.y_canon.q_y_canon.enable(&mut region, 0)?;
-
-                // Offset 0
-                let lsb = {
-                    let offset = 0;
-
-                    // Copy y.
-                    y.copy_advice(|| "copy y", &mut region, self.advices[5], offset)?;
-                    // Witness LSB.
-                    let lsb = region
-                        .assign_advice(
-                            || "witness LSB",
-                            self.advices[6],
-                            offset,
-                            || lsb.inner().value().copied().ok_or(Error::Synthesis),
-                        )
-                        // SAFETY: This is sound because we just assigned this cell from a
-                        // range-constrained value.
-                        .map(|cell| RangeConstrained::unsound_unchecked(cell, lsb.num_bits()))?;
-                    // Witness k_0.
-                    k_0.inner()
-                        .copy_advice(|| "copy k_0", &mut region, self.advices[7], offset)?;
-                    // Copy k_2.
-                    k_2.inner()
-                        .copy_advice(|| "copy k_2", &mut region, self.advices[8], offset)?;
-                    // Witness k_3.
-                    region.assign_advice(
-                        || "witness k_3",
-                        self.advices[9],
-                        offset,
-                        || k_3.inner().ok_or(Error::Synthesis),
-                    )?;
-
-                    lsb
-                };
-
-                // Offset 1
-                {
-                    let offset = 1;
-
-                    // Copy j.
-                    j.copy_advice(|| "copy j", &mut region, self.advices[5], offset)?;
-                    // Copy z1_j.
-                    z1_j.copy_advice(|| "copy z1_j", &mut region, self.advices[6], offset)?;
-                    // Copy z13_j.
-                    z13_j.copy_advice(|| "copy z13_j", &mut region, self.advices[7], offset)?;
-                    // Copy j_prime.
-                    j_prime.copy_advice(|| "copy j_prime", &mut region, self.advices[8], offset)?;
-                    // Copy z13_j_prime.
-                    z13_j_prime.copy_advice(
-                        || "copy z13_j_prime",
-                        &mut region,
-                        self.advices[9],
-                        offset,
-                    )?;
-                }
-
-                Ok(lsb)
-            },
+        self.y_canon.assign(
+            &mut layouter,
+            y,
+            lsb,
+            k_0,
+            k_2,
+            k_3,
+            j,
+            z1_j,
+            z13_j,
+            j_prime,
+            z13_j_prime,
         )
     }
 
@@ -1413,322 +1825,97 @@ impl NoteCommitConfig {
         mut layouter: impl Layouter<pallas::Base>,
         gate_cells: GateCells,
     ) -> Result<(), Error> {
-        // Columns used for MessagePiece gates.
-        let col_l = self.advices[6];
-        let col_m = self.advices[7];
-        let col_r = self.advices[8];
-        let col_z = self.advices[9];
-
-        // | A_6 | A_7 | A_8 | q_notecommit_b |
-        // ------------------------------------
-        // |  b  | b_0 | b_1 |       1        |
-        // |     | b_2 | b_3 |       0        |
-        let b_1 = layouter.assign_region(
-            || "NoteCommit MessagePiece b",
-            |mut region| {
-                self.b.q_notecommit_b.enable(&mut region, 0)?;
-
-                gate_cells.b.copy_advice(|| "b", &mut region, col_l, 0)?;
-                gate_cells
-                    .b_0
-                    .inner()
-                    .copy_advice(|| "b_0", &mut region, col_m, 0)?;
-                let b_1 = region.assign_advice(
-                    || "b_1",
-                    col_r,
-                    0,
-                    || gate_cells.b_1.inner().ok_or(Error::Synthesis),
-                )?;
-
-                gate_cells
-                    .b_2
-                    .inner()
-                    .copy_advice(|| "b_2", &mut region, col_m, 1)?;
-                gate_cells
-                    .b_3
-                    .inner()
-                    .copy_advice(|| "b_3", &mut region, col_r, 1)?;
-
-                Ok(b_1)
-            },
+        let b_1 = self.b.assign(
+            &mut layouter,
+            gate_cells.b,
+            gate_cells.b_0.clone(),
+            gate_cells.b_1,
+            gate_cells.b_2,
+            gate_cells.b_3.clone(),
         )?;
 
-        // | A_6 | A_7 | A_8 | q_notecommit_d |
-        // ------------------------------------
-        // |  d  | d_0 | d_1 |       1        |
-        // |     | d_2 | d_3 |       0        |
-        let d_0 = layouter.assign_region(
-            || "NoteCommit MessagePiece d",
-            |mut region| {
-                self.d.q_notecommit_d.enable(&mut region, 0)?;
-
-                gate_cells.d.copy_advice(|| "d", &mut region, col_l, 0)?;
-                let d_0 = region.assign_advice(
-                    || "d_0",
-                    col_m,
-                    0,
-                    || gate_cells.d_0.inner().ok_or(Error::Synthesis),
-                )?;
-                gate_cells
-                    .d_1
-                    .inner()
-                    .copy_advice(|| "d_1", &mut region, col_r, 0)?;
-
-                gate_cells
-                    .d_2
-                    .inner()
-                    .copy_advice(|| "d_2", &mut region, col_m, 1)?;
-                gate_cells
-                    .z1_d
-                    .copy_advice(|| "d_3 = z1_d", &mut region, col_r, 1)?;
-
-                Ok(d_0)
-            },
+        let d_0 = self.d.assign(
+            &mut layouter,
+            gate_cells.d,
+            gate_cells.d_0,
+            gate_cells.d_1,
+            gate_cells.d_2.clone(),
+            gate_cells.z1_d.clone(),
         )?;
 
-        // | A_6 | A_7 | A_8 | q_notecommit_e |
-        // ------------------------------------
-        // |  e  | e_0 | e_1 |       1        |
-        layouter.assign_region(
-            || "NoteCommit MessagePiece e",
-            |mut region| {
-                self.e.q_notecommit_e.enable(&mut region, 0)?;
-
-                gate_cells.e.copy_advice(|| "e", &mut region, col_l, 0)?;
-                gate_cells
-                    .e_0
-                    .inner()
-                    .copy_advice(|| "e_0", &mut region, col_m, 0)?;
-                gate_cells
-                    .e_1
-                    .inner()
-                    .copy_advice(|| "e_1", &mut region, col_r, 0)?;
-
-                Ok(())
-            },
+        self.e.assign(
+            &mut layouter,
+            gate_cells.e,
+            gate_cells.e_0.clone(),
+            gate_cells.e_1.clone(),
         )?;
 
-        // | A_6 | A_7 | q_notecommit_g |
-        // ------------------------------
-        // |  g  | g_0 |       1        |
-        // | g_1 | g_2 |       0        |
-        let g_0 = layouter.assign_region(
-            || "NoteCommit MessagePiece g",
-            |mut region| {
-                self.g.q_notecommit_g.enable(&mut region, 0)?;
-
-                gate_cells.g.copy_advice(|| "g", &mut region, col_l, 0)?;
-                let g_0 = region.assign_advice(
-                    || "g_0",
-                    col_m,
-                    0,
-                    || gate_cells.g_0.inner().ok_or(Error::Synthesis),
-                )?;
-
-                gate_cells
-                    .g_1
-                    .inner()
-                    .copy_advice(|| "g_1", &mut region, col_l, 1)?;
-                gate_cells
-                    .z1_g
-                    .copy_advice(|| "g_2 = z1_g", &mut region, col_m, 1)?;
-
-                Ok(g_0)
-            },
+        let g_0 = self.g.assign(
+            &mut layouter,
+            gate_cells.g,
+            gate_cells.g_0,
+            gate_cells.g_1.clone(),
+            gate_cells.z1_g.clone(),
         )?;
 
-        // | A_6 | A_7 | A_8 | q_notecommit_h |
-        // ------------------------------------
-        // |  h  | h_0 | h_1 |       1        |
-        let h_1 = layouter.assign_region(
-            || "NoteCommit MessagePiece h",
-            |mut region| {
-                self.h.q_notecommit_h.enable(&mut region, 0)?;
-
-                gate_cells.h.copy_advice(|| "h", &mut region, col_l, 0)?;
-                gate_cells
-                    .h_0
-                    .inner()
-                    .copy_advice(|| "h_0", &mut region, col_m, 0)?;
-                let h_1 = region.assign_advice(
-                    || "h_1",
-                    col_r,
-                    0,
-                    || gate_cells.h_1.inner().ok_or(Error::Synthesis),
-                )?;
-
-                Ok(h_1)
-            },
+        let h_1 = self.h.assign(
+            &mut layouter,
+            gate_cells.h,
+            gate_cells.h_0.clone(),
+            gate_cells.h_1,
         )?;
 
-        // |  A_6   | A_7 |   A_8   |     A_9     | q_notecommit_g_d |
-        // -----------------------------------------------------------
-        // | x(g_d) | b_0 | a       | z13_a       |        1         |
-        // |        | b_1 | a_prime | z13_a_prime |        0         |
-        layouter.assign_region(
-            || "NoteCommit input g_d",
-            |mut region| {
-                gate_cells
-                    .gd_x
-                    .copy_advice(|| "gd_x", &mut region, col_l, 0)?;
-
-                gate_cells
-                    .b_0
-                    .inner()
-                    .copy_advice(|| "b_0", &mut region, col_m, 0)?;
-                b_1.copy_advice(|| "b_1", &mut region, col_m, 1)?;
-
-                gate_cells.a.copy_advice(|| "a", &mut region, col_r, 0)?;
-                gate_cells
-                    .a_prime
-                    .copy_advice(|| "a_prime", &mut region, col_r, 1)?;
-
-                gate_cells
-                    .z13_a
-                    .copy_advice(|| "z13_a", &mut region, col_z, 0)?;
-                gate_cells
-                    .z13_a_prime
-                    .copy_advice(|| "z13_a_prime", &mut region, col_z, 1)?;
-
-                self.g_d.q_notecommit_g_d.enable(&mut region, 0)
-            },
+        self.g_d.assign(
+            &mut layouter,
+            gate_cells.gd_x,
+            gate_cells.a,
+            gate_cells.b_0,
+            b_1,
+            gate_cells.a_prime,
+            gate_cells.z13_a,
+            gate_cells.z13_a_prime,
         )?;
 
-        // |   A_6   | A_7 |    A_8     |      A_9       | q_notecommit_pk_d |
-        // -------------------------------------------------------------------
-        // | x(pk_d) | b_3 |    c       | z13_c          |         1         |
-        // |         | d_0 | b3_c_prime | z14_b3_c_prime |         0         |
-        layouter.assign_region(
-            || "NoteCommit input pk_d",
-            |mut region| {
-                gate_cells
-                    .pkd_x
-                    .copy_advice(|| "pkd_x", &mut region, col_l, 0)?;
-
-                gate_cells
-                    .b_3
-                    .inner()
-                    .copy_advice(|| "b_3", &mut region, col_m, 0)?;
-                d_0.copy_advice(|| "d_0", &mut region, col_m, 1)?;
-
-                gate_cells.c.copy_advice(|| "c", &mut region, col_r, 0)?;
-                gate_cells
-                    .b3_c_prime
-                    .copy_advice(|| "b3_c_prime", &mut region, col_r, 1)?;
-
-                gate_cells
-                    .z13_c
-                    .copy_advice(|| "z13_c", &mut region, col_z, 0)?;
-                gate_cells.z14_b3_c_prime.copy_advice(
-                    || "z14_b3_c_prime",
-                    &mut region,
-                    col_z,
-                    1,
-                )?;
-
-                self.pk_d.q_notecommit_pk_d.enable(&mut region, 0)
-            },
+        self.pk_d.assign(
+            &mut layouter,
+            gate_cells.pkd_x,
+            gate_cells.b_3,
+            gate_cells.c,
+            d_0,
+            gate_cells.b3_c_prime,
+            gate_cells.z13_c,
+            gate_cells.z14_b3_c_prime,
         )?;
 
-        // | value | d_2 | d_3 | e_0 |
-        layouter.assign_region(
-            || "NoteCommit input value",
-            |mut region| {
-                gate_cells
-                    .value
-                    .copy_advice(|| "value", &mut region, col_l, 0)?;
-                gate_cells
-                    .d_2
-                    .inner()
-                    .copy_advice(|| "d_2", &mut region, col_m, 0)?;
-                gate_cells
-                    .z1_d
-                    .copy_advice(|| "d3 = z1_d", &mut region, col_r, 0)?;
-                gate_cells
-                    .e_0
-                    .inner()
-                    .copy_advice(|| "e_0", &mut region, col_z, 0)?;
-
-                self.value.q_notecommit_value.enable(&mut region, 0)
-            },
+        self.value.assign(
+            &mut layouter,
+            gate_cells.value,
+            gate_cells.d_2,
+            gate_cells.z1_d,
+            gate_cells.e_0,
         )?;
 
-        // | A_6 | A_7 |    A_8     |      A_9       | q_notecommit_rho |
-        // --------------------------------------------------------------
-        // | rho | e_1 |    f       | z13_f          |        1         |
-        // |     | g_0 | e1_f_prime | z14_e1_f_prime |        0         |
-        layouter.assign_region(
-            || "NoteCommit input rho",
-            |mut region| {
-                gate_cells
-                    .rho
-                    .copy_advice(|| "rho", &mut region, col_l, 0)?;
-
-                gate_cells
-                    .e_1
-                    .inner()
-                    .copy_advice(|| "e_1", &mut region, col_m, 0)?;
-                g_0.copy_advice(|| "g_0", &mut region, col_m, 1)?;
-
-                gate_cells.f.copy_advice(|| "f", &mut region, col_r, 0)?;
-                gate_cells
-                    .e1_f_prime
-                    .copy_advice(|| "e1_f_prime", &mut region, col_r, 1)?;
-
-                gate_cells
-                    .z13_f
-                    .copy_advice(|| "z13_f", &mut region, col_z, 0)?;
-                gate_cells.z14_e1_f_prime.copy_advice(
-                    || "z14_e1_f_prime",
-                    &mut region,
-                    col_z,
-                    1,
-                )?;
-
-                self.rho.q_notecommit_rho.enable(&mut region, 0)
-            },
+        self.rho.assign(
+            &mut layouter,
+            gate_cells.rho,
+            gate_cells.e_1,
+            gate_cells.f,
+            g_0,
+            gate_cells.e1_f_prime,
+            gate_cells.z13_f,
+            gate_cells.z14_e1_f_prime,
         )?;
 
-        // | A_6 | A_7 |     A_8     |       A_9       | q_notecommit_psi |
-        // ----------------------------------------------------------------
-        // | psi | g_1 |   g_2       | z13_g           |        1         |
-        // | h_0 | h_1 | g1_g2_prime | z13_g1_g2_prime |        0         |
-        layouter.assign_region(
-            || "NoteCommit input psi",
-            |mut region| {
-                gate_cells
-                    .psi
-                    .copy_advice(|| "psi", &mut region, col_l, 0)?;
-                gate_cells
-                    .h_0
-                    .inner()
-                    .copy_advice(|| "h_0", &mut region, col_l, 1)?;
-
-                gate_cells
-                    .g_1
-                    .inner()
-                    .copy_advice(|| "g_1", &mut region, col_m, 0)?;
-                h_1.copy_advice(|| "h_1", &mut region, col_m, 1)?;
-
-                gate_cells
-                    .z1_g
-                    .copy_advice(|| "g_2 = z1_g", &mut region, col_r, 0)?;
-                gate_cells
-                    .g1_g2_prime
-                    .copy_advice(|| "g1_g2_prime", &mut region, col_r, 1)?;
-
-                gate_cells
-                    .z13_g
-                    .copy_advice(|| "z13_g", &mut region, col_z, 0)?;
-                gate_cells.z13_g1_g2_prime.copy_advice(
-                    || "z13_g1_g2_prime",
-                    &mut region,
-                    col_z,
-                    1,
-                )?;
-
-                self.psi.q_notecommit_psi.enable(&mut region, 0)
-            },
+        self.psi.assign(
+            &mut layouter,
+            gate_cells.psi,
+            gate_cells.g_1,
+            gate_cells.z1_g,
+            gate_cells.h_0,
+            h_1,
+            gate_cells.g1_g2_prime,
+            gate_cells.z13_g,
+            gate_cells.z13_g1_g2_prime,
         )
     }
 }

From bf99f1328256781109c33fc8ad4dfac5c9490874 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Wed, 4 May 2022 01:35:54 +0000
Subject: [PATCH 16/21] Refactor NoteCommit message piece decompositions onto
 per-region structs

---
 src/circuit/note_commit.rs | 432 +++++++++++++++++++++++--------------
 1 file changed, 276 insertions(+), 156 deletions(-)

diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index 638b2ce0..58584ff7 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -20,9 +20,18 @@ use halo2_gadgets::{
         chip::{SinsemillaChip, SinsemillaConfig},
         CommitDomain, Message, MessagePiece,
     },
-    utilities::{bool_check, FieldValue, RangeConstrained},
+    utilities::{
+        bool_check, lookup_range_check::LookupRangeCheckConfig, FieldValue, RangeConstrained,
+    },
 };
 
+type NoteCommitPiece = MessagePiece<
+    pallas::Affine,
+    SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+    10,
+    253,
+>;
+
 /// The values of the running sum at the start and end of the range being used for a
 /// canonicity check.
 type CanonicityBounds = (
@@ -43,6 +52,9 @@ type CanonicityBounds = (
         - psi is a base field element (255 bits).
 */
 
+/// b = b_0 || b_1 || b_2 || b_3
+///   = (bits 250..=253 of x(g_d)) || (bit 254 of x(g_d)) || (ỹ bit of g_d) || (bits 0..=3 of pk★_d)
+///
 /// | A_6 | A_7 | A_8 | q_notecommit_b |
 /// ------------------------------------
 /// |  b  | b_0 | b_1 |       1        |
@@ -103,10 +115,58 @@ impl DecomposeB {
         }
     }
 
+    #[allow(clippy::type_complexity)]
+    fn decompose(
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
+        chip: SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+        layouter: &mut impl Layouter<pallas::Base>,
+        g_d: &NonIdentityEccPoint,
+        pk_d: &NonIdentityEccPoint,
+    ) -> Result<
+        (
+            NoteCommitPiece,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+            RangeConstrained<pallas::Base, Option<pallas::Base>>,
+            RangeConstrained<pallas::Base, Option<pallas::Base>>,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        ),
+        Error,
+    > {
+        let (gd_x, gd_y) = (g_d.x(), g_d.y());
+
+        // Constrain b_0 to be 4 bits
+        let b_0 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "b_0"),
+            gd_x.value(),
+            250..254,
+        )?;
+
+        // b_1, b_2 will be boolean-constrained in the gate.
+        let b_1 = RangeConstrained::subset_of(gd_x.value(), 254..255);
+        let b_2 = RangeConstrained::subset_of(gd_y.value(), 0..1);
+
+        // Constrain b_3 to be 4 bits
+        let b_3 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "b_3"),
+            pk_d.x().value(),
+            0..4,
+        )?;
+
+        let b = MessagePiece::from_subpieces(
+            chip,
+            layouter.namespace(|| "b"),
+            [b_0.value(), b_1, b_2, b_3.value()],
+        )?;
+
+        Ok((b, b_0, b_1, b_2, b_3))
+    }
+
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        b: AssignedCell<pallas::Base, pallas::Base>,
+        b: NoteCommitPiece,
         b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         b_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
         b_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
@@ -117,7 +177,9 @@ impl DecomposeB {
             |mut region| {
                 self.q_notecommit_b.enable(&mut region, 0)?;
 
-                b.copy_advice(|| "b", &mut region, self.col_l, 0)?;
+                b.inner()
+                    .cell_value()
+                    .copy_advice(|| "b", &mut region, self.col_l, 0)?;
                 b_0.inner()
                     .copy_advice(|| "b_0", &mut region, self.col_m, 0)?;
                 let b_1 = region.assign_advice(
@@ -138,6 +200,9 @@ impl DecomposeB {
     }
 }
 
+/// d = d_0 || d_1 || d_2 || d_3
+///   = (bit 254 of x(pk_d)) || (ỹ bit of pk_d) || (bits 0..=7 of v) || (bits 8..=57 of v)
+///
 /// | A_6 | A_7 | A_8 | q_notecommit_d |
 /// ------------------------------------
 /// |  d  | d_0 | d_1 |       1        |
@@ -198,10 +263,52 @@ impl DecomposeD {
         }
     }
 
+    #[allow(clippy::type_complexity)]
+    fn decompose(
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
+        chip: SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+        layouter: &mut impl Layouter<pallas::Base>,
+        pk_d: &NonIdentityEccPoint,
+        value: &AssignedCell<NoteValue, pallas::Base>,
+    ) -> Result<
+        (
+            NoteCommitPiece,
+            RangeConstrained<pallas::Base, Option<pallas::Base>>,
+            RangeConstrained<pallas::Base, Option<pallas::Base>>,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        ),
+        Error,
+    > {
+        let value_val = value.value().map(|v| pallas::Base::from(v.inner()));
+
+        // d_0, d_1 will be boolean-constrained in the gate.
+        let d_0 = RangeConstrained::subset_of(pk_d.x().value(), 254..255);
+        let d_1 = RangeConstrained::subset_of(pk_d.y().value(), 0..1);
+
+        // Constrain d_2 to be 8 bits
+        let d_2 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "d_2"),
+            value_val.as_ref(),
+            0..8,
+        )?;
+
+        // d_3 = z1_d from the SinsemillaHash(d) running sum output.
+        let d_3 = RangeConstrained::subset_of(value_val.as_ref(), 8..58);
+
+        let d = MessagePiece::from_subpieces(
+            chip,
+            layouter.namespace(|| "d"),
+            [d_0, d_1, d_2.value(), d_3],
+        )?;
+
+        Ok((d, d_0, d_1, d_2))
+    }
+
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        d: AssignedCell<pallas::Base, pallas::Base>,
+        d: NoteCommitPiece,
         d_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
         d_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         d_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
@@ -212,7 +319,9 @@ impl DecomposeD {
             |mut region| {
                 self.q_notecommit_d.enable(&mut region, 0)?;
 
-                d.copy_advice(|| "d", &mut region, self.col_l, 0)?;
+                d.inner()
+                    .cell_value()
+                    .copy_advice(|| "d", &mut region, self.col_l, 0)?;
                 let d_0 = region.assign_advice(
                     || "d_0",
                     self.col_m,
@@ -232,6 +341,8 @@ impl DecomposeD {
     }
 }
 
+/// e = e_0 || e_1 = (bits 58..=63 of v) || (bits 0..=3 of rho)
+///
 /// | A_6 | A_7 | A_8 | q_notecommit_e |
 /// ------------------------------------
 /// |  e  | e_0 | e_1 |       1        |
@@ -277,10 +388,52 @@ impl DecomposeE {
         }
     }
 
+    #[allow(clippy::type_complexity)]
+    fn decompose(
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
+        chip: SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+        layouter: &mut impl Layouter<pallas::Base>,
+        value: &AssignedCell<NoteValue, pallas::Base>,
+        rho: &AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<
+        (
+            NoteCommitPiece,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        ),
+        Error,
+    > {
+        let value_val = value.value().map(|v| pallas::Base::from(v.inner()));
+
+        // Constrain e_0 to be 6 bits.
+        let e_0 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "e_0"),
+            value_val.as_ref(),
+            58..64,
+        )?;
+
+        // Constrain e_1 to be 4 bits.
+        let e_1 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "e_1"),
+            rho.value(),
+            0..4,
+        )?;
+
+        let e = MessagePiece::from_subpieces(
+            chip,
+            layouter.namespace(|| "e"),
+            [e_0.value(), e_1.value()],
+        )?;
+
+        Ok((e, e_0, e_1))
+    }
+
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        e: AssignedCell<pallas::Base, pallas::Base>,
+        e: NoteCommitPiece,
         e_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     ) -> Result<(), Error> {
@@ -289,7 +442,9 @@ impl DecomposeE {
             |mut region| {
                 self.q_notecommit_e.enable(&mut region, 0)?;
 
-                e.copy_advice(|| "e", &mut region, self.col_l, 0)?;
+                e.inner()
+                    .cell_value()
+                    .copy_advice(|| "e", &mut region, self.col_l, 0)?;
                 e_0.inner()
                     .copy_advice(|| "e_0", &mut region, self.col_m, 0)?;
                 e_1.inner()
@@ -301,6 +456,9 @@ impl DecomposeE {
     }
 }
 
+/// g = g_0 || g_1 || g_2
+///   = (bit 254 of rho) || (bits 0..=8 of psi) || (bits 9..=248 of psi)
+///
 /// | A_6 | A_7 | q_notecommit_g |
 /// ------------------------------
 /// |  g  | g_0 |       1        |
@@ -353,10 +511,48 @@ impl DecomposeG {
         }
     }
 
+    #[allow(clippy::type_complexity)]
+    fn decompose(
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
+        chip: SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+        layouter: &mut impl Layouter<pallas::Base>,
+        rho: &AssignedCell<pallas::Base, pallas::Base>,
+        psi: &AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<
+        (
+            NoteCommitPiece,
+            RangeConstrained<pallas::Base, Option<pallas::Base>>,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+        ),
+        Error,
+    > {
+        // g_0 will be boolean-constrained in the gate.
+        let g_0 = RangeConstrained::subset_of(rho.value(), 254..255);
+
+        // Constrain g_1 to be 9 bits.
+        let g_1 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "g_1"),
+            psi.value(),
+            0..9,
+        )?;
+
+        // g_2 = z1_g from the SinsemillaHash(g) running sum output.
+        let g_2 = RangeConstrained::subset_of(psi.value(), 9..249);
+
+        let g = MessagePiece::from_subpieces(
+            chip,
+            layouter.namespace(|| "g"),
+            [g_0, g_1.value(), g_2],
+        )?;
+
+        Ok((g, g_0, g_1))
+    }
+
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        g: AssignedCell<pallas::Base, pallas::Base>,
+        g: NoteCommitPiece,
         g_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
         g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         z1_g: AssignedCell<pallas::Base, pallas::Base>,
@@ -366,7 +562,9 @@ impl DecomposeG {
             |mut region| {
                 self.q_notecommit_g.enable(&mut region, 0)?;
 
-                g.copy_advice(|| "g", &mut region, self.col_l, 0)?;
+                g.inner()
+                    .cell_value()
+                    .copy_advice(|| "g", &mut region, self.col_l, 0)?;
                 let g_0 = region.assign_advice(
                     || "g_0",
                     self.col_m,
@@ -384,6 +582,9 @@ impl DecomposeG {
     }
 }
 
+/// h = h_0 || h_1 || h_2
+///   = (bits 249..=253 of psi) || (bit 254 of psi) || 4 zero bits
+///
 /// | A_6 | A_7 | A_8 | q_notecommit_h |
 /// ------------------------------------
 /// |  h  | h_0 | h_1 |       1        |
@@ -435,10 +636,48 @@ impl DecomposeH {
         }
     }
 
+    #[allow(clippy::type_complexity)]
+    fn decompose(
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
+        chip: SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
+        layouter: &mut impl Layouter<pallas::Base>,
+        psi: &AssignedCell<pallas::Base, pallas::Base>,
+    ) -> Result<
+        (
+            NoteCommitPiece,
+            RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
+            RangeConstrained<pallas::Base, Option<pallas::Base>>,
+        ),
+        Error,
+    > {
+        // Constrain h_0 to be 5 bits.
+        let h_0 = RangeConstrained::witness_short(
+            lookup_config,
+            layouter.namespace(|| "h_0"),
+            psi.value(),
+            249..254,
+        )?;
+
+        // h_1 will be boolean-constrained in the gate.
+        let h_1 = RangeConstrained::subset_of(psi.value(), 254..255);
+
+        let h = MessagePiece::from_subpieces(
+            chip,
+            layouter.namespace(|| "h"),
+            [
+                h_0.value(),
+                h_1,
+                RangeConstrained::subset_of(Some(&pallas::Base::zero()), 0..4),
+            ],
+        )?;
+
+        Ok((h, h_0, h_1))
+    }
+
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        h: AssignedCell<pallas::Base, pallas::Base>,
+        h: NoteCommitPiece,
         h_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         h_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     ) -> Result<AssignedCell<pallas::Base, pallas::Base>, Error> {
@@ -447,7 +686,9 @@ impl DecomposeH {
             |mut region| {
                 self.q_notecommit_h.enable(&mut region, 0)?;
 
-                h.copy_advice(|| "h", &mut region, self.col_l, 0)?;
+                h.inner()
+                    .cell_value()
+                    .copy_advice(|| "h", &mut region, self.col_l, 0)?;
                 h_0.inner()
                     .copy_advice(|| "h_0", &mut region, self.col_m, 0)?;
                 let h_1 = region.assign_advice(
@@ -1320,173 +1561,52 @@ impl NoteCommitConfig {
         psi: AssignedCell<pallas::Base, pallas::Base>,
         rcm: Option<pallas::Scalar>,
     ) -> Result<Point<pallas::Affine, EccChip<OrchardFixedBases>>, Error> {
-        let (gd_x, gd_y) = (g_d.x(), g_d.y());
-        let (pkd_x, pkd_y) = (pk_d.x(), pk_d.y());
-        let (gd_x, gd_y) = (gd_x.value(), gd_y.value());
-        let (pkd_x, pkd_y) = (pkd_x.value(), pkd_y.value());
-        let value_val = value.value().map(|v| pallas::Base::from(v.inner()));
-        let rho_val = rho.value();
-        let psi_val = psi.value();
+        let lookup_config = self.sinsemilla_config.lookup_config();
 
         // `a` = bits 0..=249 of `x(g_d)`
         let a = MessagePiece::from_subpieces(
             chip.clone(),
             layouter.namespace(|| "a"),
-            [RangeConstrained::subset_of(gd_x, 0..250)],
+            [RangeConstrained::subset_of(g_d.x().value(), 0..250)],
         )?;
 
         // b = b_0 || b_1 || b_2 || b_3
         //   = (bits 250..=253 of x(g_d)) || (bit 254 of x(g_d)) || (ỹ bit of g_d) || (bits 0..=3 of pk★_d)
-        let (b_0, b_1, b_2, b_3, b) = {
-            // Constrain b_0 to be 4 bits
-            let b_0 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "b_0"),
-                gd_x,
-                250..254,
-            )?;
-
-            // b_1, b_2 will be boolean-constrained in the gate.
-            let b_1 = RangeConstrained::subset_of(gd_x, 254..255);
-            let b_2 = RangeConstrained::subset_of(gd_y, 0..1);
-
-            // Constrain b_3 to be 4 bits
-            let b_3 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "b_3"),
-                pkd_x,
-                0..4,
-            )?;
-
-            let b = MessagePiece::from_subpieces(
-                chip.clone(),
-                layouter.namespace(|| "b"),
-                [b_0.value(), b_1, b_2, b_3.value()],
-            )?;
-
-            (b_0, b_1, b_2, b_3, b)
-        };
+        let (b, b_0, b_1, b_2, b_3) =
+            DecomposeB::decompose(&lookup_config, chip.clone(), &mut layouter, g_d, pk_d)?;
 
         // c = bits 4..=253 of pk★_d
         let c = MessagePiece::from_subpieces(
             chip.clone(),
             layouter.namespace(|| "c"),
-            [RangeConstrained::subset_of(pkd_x, 4..254)],
+            [RangeConstrained::subset_of(pk_d.x().value(), 4..254)],
         )?;
 
         // d = d_0 || d_1 || d_2 || d_3
         //   = (bit 254 of x(pk_d)) || (ỹ bit of pk_d) || (bits 0..=7 of v) || (bits 8..=57 of v)
-        let (d_0, d_1, d_2, d) = {
-            // d_0, d_1 will be boolean-constrained in the gate.
-            let d_0 = RangeConstrained::subset_of(pkd_x, 254..255);
-            let d_1 = RangeConstrained::subset_of(pkd_y, 0..1);
-
-            // Constrain d_2 to be 8 bits
-            let d_2 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "d_2"),
-                value_val.as_ref(),
-                0..8,
-            )?;
-
-            // d_3 = z1_d from the SinsemillaHash(d) running sum output.
-            let d_3 = RangeConstrained::subset_of(value_val.as_ref(), 8..58);
-
-            let d = MessagePiece::from_subpieces(
-                chip.clone(),
-                layouter.namespace(|| "d"),
-                [d_0, d_1, d_2.value(), d_3],
-            )?;
-
-            (d_0, d_1, d_2, d)
-        };
+        let (d, d_0, d_1, d_2) =
+            DecomposeD::decompose(&lookup_config, chip.clone(), &mut layouter, pk_d, &value)?;
 
         // e = e_0 || e_1 = (bits 58..=63 of v) || (bits 0..=3 of rho)
-        let (e_0, e_1, e) = {
-            // Constrain e_0 to be 6 bits.
-            let e_0 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "e_0"),
-                value_val.as_ref(),
-                58..64,
-            )?;
-
-            // Constrain e_1 to be 4 bits.
-            let e_1 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "e_1"),
-                rho_val,
-                0..4,
-            )?;
-
-            let e = MessagePiece::from_subpieces(
-                chip.clone(),
-                layouter.namespace(|| "e"),
-                [e_0.value(), e_1.value()],
-            )?;
-
-            (e_0, e_1, e)
-        };
+        let (e, e_0, e_1) =
+            DecomposeE::decompose(&lookup_config, chip.clone(), &mut layouter, &value, &rho)?;
 
         // f = bits 4..=253 inclusive of rho
         let f = MessagePiece::from_subpieces(
             chip.clone(),
             layouter.namespace(|| "f"),
-            [RangeConstrained::subset_of(rho_val, 4..254)],
+            [RangeConstrained::subset_of(rho.value(), 4..254)],
         )?;
 
         // g = g_0 || g_1 || g_2
         //   = (bit 254 of rho) || (bits 0..=8 of psi) || (bits 9..=248 of psi)
-        let (g_0, g_1, g) = {
-            // g_0 will be boolean-constrained in the gate.
-            let g_0 = RangeConstrained::subset_of(rho_val, 254..255);
-
-            // Constrain g_1 to be 9 bits.
-            let g_1 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "g_1"),
-                psi_val,
-                0..9,
-            )?;
-
-            // g_2 = z1_g from the SinsemillaHash(g) running sum output.
-            let g_2 = RangeConstrained::subset_of(psi_val, 9..249);
-
-            let g = MessagePiece::from_subpieces(
-                chip.clone(),
-                layouter.namespace(|| "g"),
-                [g_0, g_1.value(), g_2],
-            )?;
-
-            (g_0, g_1, g)
-        };
+        let (g, g_0, g_1) =
+            DecomposeG::decompose(&lookup_config, chip.clone(), &mut layouter, &rho, &psi)?;
 
         // h = h_0 || h_1 || h_2
         //   = (bits 249..=253 of psi) || (bit 254 of psi) || 4 zero bits
-        let (h_0, h_1, h) = {
-            // Constrain h_0 to be 5 bits.
-            let h_0 = RangeConstrained::witness_short(
-                &self.sinsemilla_config.lookup_config(),
-                layouter.namespace(|| "h_0"),
-                psi_val,
-                249..254,
-            )?;
-
-            // h_1 will be boolean-constrained in the gate.
-            let h_1 = RangeConstrained::subset_of(psi_val, 254..255);
-
-            let h = MessagePiece::from_subpieces(
-                chip.clone(),
-                layouter.namespace(|| "h"),
-                [
-                    h_0.value(),
-                    h_1,
-                    RangeConstrained::subset_of(Some(&pallas::Base::zero()), 0..4),
-                ],
-            )?;
-
-            (h_0, h_1, h)
-        };
+        let (h, h_0, h_1) =
+            DecomposeH::decompose(&lookup_config, chip.clone(), &mut layouter, &psi)?;
 
         // Check decomposition of `y(g_d)`.
         let b_2 = self.y_canonicity(layouter.namespace(|| "y(g_d) decomposition"), g_d.y(), b_2)?;
@@ -1549,26 +1669,26 @@ impl NoteCommitConfig {
 
         let gate_cells = GateCells {
             a: a.inner().cell_value(),
-            b: b.inner().cell_value(),
+            b,
             b_0,
             b_1,
             b_2,
             b_3,
             c: c.inner().cell_value(),
-            d: d.inner().cell_value(),
+            d,
             d_0,
             d_1,
             d_2,
             z1_d,
-            e: e.inner().cell_value(),
+            e,
             e_0,
             e_1,
             f: f.inner().cell_value(),
-            g: g.inner().cell_value(),
+            g,
             g_0,
             g_1,
             z1_g,
-            h: h.inner().cell_value(),
+            h,
             h_0,
             h_1,
             gd_x: g_d.x(),
@@ -1922,26 +2042,26 @@ impl NoteCommitConfig {
 
 struct GateCells {
     a: AssignedCell<pallas::Base, pallas::Base>,
-    b: AssignedCell<pallas::Base, pallas::Base>,
+    b: NoteCommitPiece,
     b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     b_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     b_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     c: AssignedCell<pallas::Base, pallas::Base>,
-    d: AssignedCell<pallas::Base, pallas::Base>,
+    d: NoteCommitPiece,
     d_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     d_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     d_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     z1_d: AssignedCell<pallas::Base, pallas::Base>,
-    e: AssignedCell<pallas::Base, pallas::Base>,
+    e: NoteCommitPiece,
     e_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     f: AssignedCell<pallas::Base, pallas::Base>,
-    g: AssignedCell<pallas::Base, pallas::Base>,
+    g: NoteCommitPiece,
     g_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     z1_g: AssignedCell<pallas::Base, pallas::Base>,
-    h: AssignedCell<pallas::Base, pallas::Base>,
+    h: NoteCommitPiece,
     h_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
     h_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
     gd_x: AssignedCell<pallas::Base, pallas::Base>,

From 8f15db1d013d8dba2d1b2cd99032db9e0cd4fa54 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Wed, 4 May 2022 01:53:10 +0000
Subject: [PATCH 17/21] Inline `NoteCommitConfig::assign_gate`

After the previous refactors, the `GateCells` struct now serves no
purpose. We also make a few type safety improvements at the same time.
---
 src/circuit/note_commit.rs | 247 +++++++++----------------------------
 1 file changed, 61 insertions(+), 186 deletions(-)

diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index 58584ff7..14848beb 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -788,8 +788,8 @@ impl GdCanonicity {
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        gd_x: AssignedCell<pallas::Base, pallas::Base>,
-        a: AssignedCell<pallas::Base, pallas::Base>,
+        g_d: &NonIdentityEccPoint,
+        a: NoteCommitPiece,
         b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         b_1: AssignedCell<pallas::Base, pallas::Base>,
         a_prime: AssignedCell<pallas::Base, pallas::Base>,
@@ -799,13 +799,15 @@ impl GdCanonicity {
         layouter.assign_region(
             || "NoteCommit input g_d",
             |mut region| {
-                gd_x.copy_advice(|| "gd_x", &mut region, self.col_l, 0)?;
+                g_d.x().copy_advice(|| "gd_x", &mut region, self.col_l, 0)?;
 
                 b_0.inner()
                     .copy_advice(|| "b_0", &mut region, self.col_m, 0)?;
                 b_1.copy_advice(|| "b_1", &mut region, self.col_m, 1)?;
 
-                a.copy_advice(|| "a", &mut region, self.col_r, 0)?;
+                a.inner()
+                    .cell_value()
+                    .copy_advice(|| "a", &mut region, self.col_r, 0)?;
                 a_prime.copy_advice(|| "a_prime", &mut region, self.col_r, 1)?;
 
                 z13_a.copy_advice(|| "z13_a", &mut region, self.col_z, 0)?;
@@ -900,9 +902,9 @@ impl PkdCanonicity {
     fn assign(
         &self,
         layouter: &mut impl Layouter<pallas::Base>,
-        pkd_x: AssignedCell<pallas::Base, pallas::Base>,
+        pk_d: &NonIdentityEccPoint,
         b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-        c: AssignedCell<pallas::Base, pallas::Base>,
+        c: NoteCommitPiece,
         d_0: AssignedCell<pallas::Base, pallas::Base>,
         b3_c_prime: AssignedCell<pallas::Base, pallas::Base>,
         z13_c: AssignedCell<pallas::Base, pallas::Base>,
@@ -911,13 +913,16 @@ impl PkdCanonicity {
         layouter.assign_region(
             || "NoteCommit input pk_d",
             |mut region| {
-                pkd_x.copy_advice(|| "pkd_x", &mut region, self.col_l, 0)?;
+                pk_d.x()
+                    .copy_advice(|| "pkd_x", &mut region, self.col_l, 0)?;
 
                 b_3.inner()
                     .copy_advice(|| "b_3", &mut region, self.col_m, 0)?;
                 d_0.copy_advice(|| "d_0", &mut region, self.col_m, 1)?;
 
-                c.copy_advice(|| "c", &mut region, self.col_r, 0)?;
+                c.inner()
+                    .cell_value()
+                    .copy_advice(|| "c", &mut region, self.col_r, 0)?;
                 b3_c_prime.copy_advice(|| "b3_c_prime", &mut region, self.col_r, 1)?;
 
                 z13_c.copy_advice(|| "z13_c", &mut region, self.col_z, 0)?;
@@ -1088,7 +1093,7 @@ impl RhoCanonicity {
         layouter: &mut impl Layouter<pallas::Base>,
         rho: AssignedCell<pallas::Base, pallas::Base>,
         e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-        f: AssignedCell<pallas::Base, pallas::Base>,
+        f: NoteCommitPiece,
         g_0: AssignedCell<pallas::Base, pallas::Base>,
         e1_f_prime: AssignedCell<pallas::Base, pallas::Base>,
         z13_f: AssignedCell<pallas::Base, pallas::Base>,
@@ -1103,7 +1108,9 @@ impl RhoCanonicity {
                     .copy_advice(|| "e_1", &mut region, self.col_m, 0)?;
                 g_0.copy_advice(|| "g_0", &mut region, self.col_m, 1)?;
 
-                f.copy_advice(|| "f", &mut region, self.col_r, 0)?;
+                f.inner()
+                    .cell_value()
+                    .copy_advice(|| "f", &mut region, self.col_r, 0)?;
                 e1_f_prime.copy_advice(|| "e1_f_prime", &mut region, self.col_r, 1)?;
 
                 z13_f.copy_advice(|| "z13_f", &mut region, self.col_z, 0)?;
@@ -1667,50 +1674,60 @@ impl NoteCommitConfig {
         let (g1_g2_prime, z13_g1_g2_prime) =
             self.psi_canonicity(layouter.namespace(|| "psi canonicity"), g_1.clone(), g_2)?;
 
-        let gate_cells = GateCells {
-            a: a.inner().cell_value(),
-            b,
-            b_0,
-            b_1,
-            b_2,
+        let b_1 = self
+            .b
+            .assign(&mut layouter, b, b_0.clone(), b_1, b_2, b_3.clone())?;
+
+        let d_0 = self
+            .d
+            .assign(&mut layouter, d, d_0, d_1, d_2.clone(), z1_d.clone())?;
+
+        self.e.assign(&mut layouter, e, e_0.clone(), e_1.clone())?;
+
+        let g_0 = self
+            .g
+            .assign(&mut layouter, g, g_0, g_1.clone(), z1_g.clone())?;
+
+        let h_1 = self.h.assign(&mut layouter, h, h_0.clone(), h_1)?;
+
+        self.g_d
+            .assign(&mut layouter, g_d, a, b_0, b_1, a_prime, z13_a, z13_a_prime)?;
+
+        self.pk_d.assign(
+            &mut layouter,
+            pk_d,
             b_3,
-            c: c.inner().cell_value(),
-            d,
+            c,
             d_0,
-            d_1,
-            d_2,
-            z1_d,
-            e,
-            e_0,
+            b3_c_prime,
+            z13_c,
+            z14_b3_c_prime,
+        )?;
+
+        self.value.assign(&mut layouter, value, d_2, z1_d, e_0)?;
+
+        self.rho.assign(
+            &mut layouter,
+            rho,
             e_1,
-            f: f.inner().cell_value(),
-            g,
+            f,
             g_0,
+            e1_f_prime,
+            z13_f,
+            z14_e1_f_prime,
+        )?;
+
+        self.psi.assign(
+            &mut layouter,
+            psi,
             g_1,
             z1_g,
-            h,
             h_0,
             h_1,
-            gd_x: g_d.x(),
-            pkd_x: pk_d.x(),
-            value,
-            rho,
-            psi,
-            a_prime,
-            b3_c_prime,
-            e1_f_prime,
             g1_g2_prime,
-            z13_a_prime,
-            z14_b3_c_prime,
-            z14_e1_f_prime,
-            z13_g1_g2_prime,
-            z13_a,
-            z13_c,
-            z13_f,
             z13_g,
-        };
-
-        self.assign_gate(layouter.namespace(|| "Assign gate cells"), gate_cells)?;
+            z13_g1_g2_prime,
+        )?;
 
         Ok(cm)
     }
@@ -1939,148 +1956,6 @@ impl NoteCommitConfig {
             z13_j_prime,
         )
     }
-
-    fn assign_gate(
-        &self,
-        mut layouter: impl Layouter<pallas::Base>,
-        gate_cells: GateCells,
-    ) -> Result<(), Error> {
-        let b_1 = self.b.assign(
-            &mut layouter,
-            gate_cells.b,
-            gate_cells.b_0.clone(),
-            gate_cells.b_1,
-            gate_cells.b_2,
-            gate_cells.b_3.clone(),
-        )?;
-
-        let d_0 = self.d.assign(
-            &mut layouter,
-            gate_cells.d,
-            gate_cells.d_0,
-            gate_cells.d_1,
-            gate_cells.d_2.clone(),
-            gate_cells.z1_d.clone(),
-        )?;
-
-        self.e.assign(
-            &mut layouter,
-            gate_cells.e,
-            gate_cells.e_0.clone(),
-            gate_cells.e_1.clone(),
-        )?;
-
-        let g_0 = self.g.assign(
-            &mut layouter,
-            gate_cells.g,
-            gate_cells.g_0,
-            gate_cells.g_1.clone(),
-            gate_cells.z1_g.clone(),
-        )?;
-
-        let h_1 = self.h.assign(
-            &mut layouter,
-            gate_cells.h,
-            gate_cells.h_0.clone(),
-            gate_cells.h_1,
-        )?;
-
-        self.g_d.assign(
-            &mut layouter,
-            gate_cells.gd_x,
-            gate_cells.a,
-            gate_cells.b_0,
-            b_1,
-            gate_cells.a_prime,
-            gate_cells.z13_a,
-            gate_cells.z13_a_prime,
-        )?;
-
-        self.pk_d.assign(
-            &mut layouter,
-            gate_cells.pkd_x,
-            gate_cells.b_3,
-            gate_cells.c,
-            d_0,
-            gate_cells.b3_c_prime,
-            gate_cells.z13_c,
-            gate_cells.z14_b3_c_prime,
-        )?;
-
-        self.value.assign(
-            &mut layouter,
-            gate_cells.value,
-            gate_cells.d_2,
-            gate_cells.z1_d,
-            gate_cells.e_0,
-        )?;
-
-        self.rho.assign(
-            &mut layouter,
-            gate_cells.rho,
-            gate_cells.e_1,
-            gate_cells.f,
-            g_0,
-            gate_cells.e1_f_prime,
-            gate_cells.z13_f,
-            gate_cells.z14_e1_f_prime,
-        )?;
-
-        self.psi.assign(
-            &mut layouter,
-            gate_cells.psi,
-            gate_cells.g_1,
-            gate_cells.z1_g,
-            gate_cells.h_0,
-            h_1,
-            gate_cells.g1_g2_prime,
-            gate_cells.z13_g,
-            gate_cells.z13_g1_g2_prime,
-        )
-    }
-}
-
-struct GateCells {
-    a: AssignedCell<pallas::Base, pallas::Base>,
-    b: NoteCommitPiece,
-    b_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    b_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
-    b_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    c: AssignedCell<pallas::Base, pallas::Base>,
-    d: NoteCommitPiece,
-    d_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
-    d_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    d_2: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    z1_d: AssignedCell<pallas::Base, pallas::Base>,
-    e: NoteCommitPiece,
-    e_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    f: AssignedCell<pallas::Base, pallas::Base>,
-    g: NoteCommitPiece,
-    g_0: RangeConstrained<pallas::Base, Option<pallas::Base>>,
-    g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    z1_g: AssignedCell<pallas::Base, pallas::Base>,
-    h: NoteCommitPiece,
-    h_0: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
-    h_1: RangeConstrained<pallas::Base, Option<pallas::Base>>,
-    gd_x: AssignedCell<pallas::Base, pallas::Base>,
-    pkd_x: AssignedCell<pallas::Base, pallas::Base>,
-    value: AssignedCell<NoteValue, pallas::Base>,
-    rho: AssignedCell<pallas::Base, pallas::Base>,
-    psi: AssignedCell<pallas::Base, pallas::Base>,
-    a_prime: AssignedCell<pallas::Base, pallas::Base>,
-    b3_c_prime: AssignedCell<pallas::Base, pallas::Base>,
-    e1_f_prime: AssignedCell<pallas::Base, pallas::Base>,
-    g1_g2_prime: AssignedCell<pallas::Base, pallas::Base>,
-    z13_a_prime: AssignedCell<pallas::Base, pallas::Base>,
-    z14_b3_c_prime: AssignedCell<pallas::Base, pallas::Base>,
-    z14_e1_f_prime: AssignedCell<pallas::Base, pallas::Base>,
-    z13_g1_g2_prime: AssignedCell<pallas::Base, pallas::Base>,
-    z13_a: AssignedCell<pallas::Base, pallas::Base>,
-    z13_c: AssignedCell<pallas::Base, pallas::Base>,
-    z13_f: AssignedCell<pallas::Base, pallas::Base>,
-    z13_g: AssignedCell<pallas::Base, pallas::Base>,
 }
 
 #[cfg(test)]

From 903f9e816010f8114bcebebc910472f83846423e Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Wed, 4 May 2022 02:55:42 +0000
Subject: [PATCH 18/21] Adjust APIs of NoteCommit circuit impl to separate
 gadget and chip

The separation isn't quite complete, as we removed the `GateCells`
abstraction, but it makes the outer APIs clearer.
---
 src/circuit.rs             |  16 +++--
 src/circuit/gadget.rs      |  11 +++-
 src/circuit/note_commit.rs | 121 +++++++++++++++++++++++++------------
 3 files changed, 98 insertions(+), 50 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 7aeb77f8..7e1faa65 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -22,7 +22,7 @@ use self::{
         add_chip::{AddChip, AddConfig},
         assign_free_advice,
     },
-    note_commit::NoteCommitConfig,
+    note_commit::{NoteCommitChip, NoteCommitConfig},
 };
 use crate::{
     constants::{
@@ -292,12 +292,12 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         // Configuration to handle decomposition and canonicity checking
         // for NoteCommit_old.
         let old_note_commit_config =
-            NoteCommitConfig::configure(meta, advices, sinsemilla_config_1.clone());
+            NoteCommitChip::configure(meta, advices, sinsemilla_config_1.clone());
 
         // Configuration to handle decomposition and canonicity checking
         // for NoteCommit_new.
         let new_note_commit_config =
-            NoteCommitConfig::configure(meta, advices, sinsemilla_config_2.clone());
+            NoteCommitChip::configure(meta, advices, sinsemilla_config_2.clone());
 
         Config {
             primary,
@@ -530,17 +530,16 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // Old note commitment integrity.
         {
-            let old_note_commit_config = config.old_note_commit_config.clone();
-
             let rcm_old = self.rcm_old.as_ref().map(|rcm_old| rcm_old.inner());
 
             // g★_d || pk★_d || i2lebsp_{64}(v) || i2lebsp_{255}(rho) || i2lebsp_{255}(psi)
-            let derived_cm_old = old_note_commit_config.assign_region(
+            let derived_cm_old = gadget::note_commit(
                 layouter.namespace(|| {
                     "g★_d || pk★_d || i2lebsp_{64}(v) || i2lebsp_{255}(rho) || i2lebsp_{255}(psi)"
                 }),
                 config.sinsemilla_chip_1(),
                 config.ecc_chip(),
+                config.note_commit_chip_old(),
                 g_d_old.inner(),
                 pk_d_old.inner(),
                 v_old.clone(),
@@ -555,8 +554,6 @@ impl plonk::Circuit<pallas::Base> for Circuit {
 
         // New note commitment integrity.
         {
-            let new_note_commit_config = config.new_note_commit_config.clone();
-
             // Witness g_d_new
             let g_d_new = {
                 let g_d_new = self.g_d_new.map(|g_d_new| g_d_new.to_affine());
@@ -590,12 +587,13 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             let rcm_new = self.rcm_new.as_ref().map(|rcm_new| rcm_new.inner());
 
             // g★_d || pk★_d || i2lebsp_{64}(v) || i2lebsp_{255}(rho) || i2lebsp_{255}(psi)
-            let cm_new = new_note_commit_config.assign_region(
+            let cm_new = gadget::note_commit(
                 layouter.namespace(|| {
                     "g★_d || pk★_d || i2lebsp_{64}(v) || i2lebsp_{255}(rho) || i2lebsp_{255}(psi)"
                 }),
                 config.sinsemilla_chip_2(),
                 config.ecc_chip(),
+                config.note_commit_chip_new(),
                 g_d_new.inner(),
                 pk_d_new.inner(),
                 v_new.clone(),
diff --git a/src/circuit/gadget.rs b/src/circuit/gadget.rs
index fb471a94..f35e431e 100644
--- a/src/circuit/gadget.rs
+++ b/src/circuit/gadget.rs
@@ -3,7 +3,7 @@
 use ff::Field;
 use pasta_curves::pallas;
 
-use super::commit_ivk::CommitIvkChip;
+use super::{commit_ivk::CommitIvkChip, note_commit::NoteCommitChip};
 use crate::constants::{
     NullifierK, OrchardCommitDomains, OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains,
     ValueCommitV,
@@ -64,6 +64,14 @@ impl super::Config {
     pub(super) fn poseidon_chip(&self) -> PoseidonChip<pallas::Base, 3, 2> {
         PoseidonChip::construct(self.poseidon_config.clone())
     }
+
+    pub(super) fn note_commit_chip_new(&self) -> NoteCommitChip {
+        NoteCommitChip::construct(self.new_note_commit_config.clone())
+    }
+
+    pub(super) fn note_commit_chip_old(&self) -> NoteCommitChip {
+        NoteCommitChip::construct(self.old_note_commit_config.clone())
+    }
 }
 
 /// An instruction set for adding two circuit words (field elements).
@@ -200,3 +208,4 @@ pub(in crate::circuit) fn derive_nullifier<
 }
 
 pub(in crate::circuit) use crate::circuit::commit_ivk::gadgets::commit_ivk;
+pub(in crate::circuit) use crate::circuit::note_commit::gadgets::note_commit;
diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index 14848beb..9fa64df7 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -1431,7 +1431,12 @@ pub struct NoteCommitConfig {
         SinsemillaConfig<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
 }
 
-impl NoteCommitConfig {
+#[derive(Clone, Debug)]
+pub struct NoteCommitChip {
+    config: NoteCommitConfig,
+}
+
+impl NoteCommitChip {
     #[allow(non_snake_case)]
     #[allow(clippy::many_single_char_names)]
     pub(in crate::circuit) fn configure(
@@ -1442,7 +1447,7 @@ impl NoteCommitConfig {
             OrchardCommitDomains,
             OrchardFixedBases,
         >,
-    ) -> Self {
+    ) -> NoteCommitConfig {
         // Useful constants
         let two = pallas::Base::from(2);
         let two_pow_2 = pallas::Base::from(1 << 2);
@@ -1536,7 +1541,7 @@ impl NoteCommitConfig {
             t_p,
         );
 
-        Self {
+        NoteCommitConfig {
             b,
             d,
             e,
@@ -1553,14 +1558,24 @@ impl NoteCommitConfig {
         }
     }
 
+    pub(in crate::circuit) fn construct(config: NoteCommitConfig) -> Self {
+        Self { config }
+    }
+}
+
+pub(in crate::circuit) mod gadgets {
+    use halo2_proofs::circuit::Chip;
+
+    use super::*;
+
     #[allow(clippy::many_single_char_names)]
     #[allow(clippy::type_complexity)]
     #[allow(clippy::too_many_arguments)]
-    pub(in crate::circuit) fn assign_region(
-        &self,
+    pub(in crate::circuit) fn note_commit(
         mut layouter: impl Layouter<pallas::Base>,
         chip: SinsemillaChip<OrchardHashDomains, OrchardCommitDomains, OrchardFixedBases>,
         ecc_chip: EccChip<OrchardFixedBases>,
+        note_commit_chip: NoteCommitChip,
         g_d: &NonIdentityEccPoint,
         pk_d: &NonIdentityEccPoint,
         value: AssignedCell<NoteValue, pallas::Base>,
@@ -1568,7 +1583,7 @@ impl NoteCommitConfig {
         psi: AssignedCell<pallas::Base, pallas::Base>,
         rcm: Option<pallas::Scalar>,
     ) -> Result<Point<pallas::Affine, EccChip<OrchardFixedBases>>, Error> {
-        let lookup_config = self.sinsemilla_config.lookup_config();
+        let lookup_config = chip.config().lookup_config();
 
         // `a` = bits 0..=249 of `x(g_d)`
         let a = MessagePiece::from_subpieces(
@@ -1616,9 +1631,17 @@ impl NoteCommitConfig {
             DecomposeH::decompose(&lookup_config, chip.clone(), &mut layouter, &psi)?;
 
         // Check decomposition of `y(g_d)`.
-        let b_2 = self.y_canonicity(layouter.namespace(|| "y(g_d) decomposition"), g_d.y(), b_2)?;
+        let b_2 = y_canonicity(
+            &lookup_config,
+            &note_commit_chip.config.y_canon,
+            layouter.namespace(|| "y(g_d) decomposition"),
+            g_d.y(),
+            b_2,
+        )?;
         // Check decomposition of `y(pk_d)`.
-        let d_1 = self.y_canonicity(
+        let d_1 = y_canonicity(
+            &lookup_config,
+            &note_commit_chip.config.y_canon,
             layouter.namespace(|| "y(pk_d) decomposition"),
             pk_d.y(),
             d_1,
@@ -1654,46 +1677,55 @@ impl NoteCommitConfig {
         let g_2 = z1_g.clone();
         let z13_g = zs[6][13].clone();
 
-        let (a_prime, z13_a_prime) = self.canon_bitshift_130(
+        let (a_prime, z13_a_prime) = canon_bitshift_130(
+            &lookup_config,
             layouter.namespace(|| "x(g_d) canonicity"),
             a.inner().cell_value(),
         )?;
 
-        let (b3_c_prime, z14_b3_c_prime) = self.pkd_x_canonicity(
+        let (b3_c_prime, z14_b3_c_prime) = pkd_x_canonicity(
+            &lookup_config,
             layouter.namespace(|| "x(pk_d) canonicity"),
             b_3.clone(),
             c.inner().cell_value(),
         )?;
 
-        let (e1_f_prime, z14_e1_f_prime) = self.rho_canonicity(
+        let (e1_f_prime, z14_e1_f_prime) = rho_canonicity(
+            &lookup_config,
             layouter.namespace(|| "rho canonicity"),
             e_1.clone(),
             f.inner().cell_value(),
         )?;
 
-        let (g1_g2_prime, z13_g1_g2_prime) =
-            self.psi_canonicity(layouter.namespace(|| "psi canonicity"), g_1.clone(), g_2)?;
+        let (g1_g2_prime, z13_g1_g2_prime) = psi_canonicity(
+            &lookup_config,
+            layouter.namespace(|| "psi canonicity"),
+            g_1.clone(),
+            g_2,
+        )?;
 
-        let b_1 = self
+        let cfg = note_commit_chip.config;
+
+        let b_1 = cfg
             .b
             .assign(&mut layouter, b, b_0.clone(), b_1, b_2, b_3.clone())?;
 
-        let d_0 = self
+        let d_0 = cfg
             .d
             .assign(&mut layouter, d, d_0, d_1, d_2.clone(), z1_d.clone())?;
 
-        self.e.assign(&mut layouter, e, e_0.clone(), e_1.clone())?;
+        cfg.e.assign(&mut layouter, e, e_0.clone(), e_1.clone())?;
 
-        let g_0 = self
+        let g_0 = cfg
             .g
             .assign(&mut layouter, g, g_0, g_1.clone(), z1_g.clone())?;
 
-        let h_1 = self.h.assign(&mut layouter, h, h_0.clone(), h_1)?;
+        let h_1 = cfg.h.assign(&mut layouter, h, h_0.clone(), h_1)?;
 
-        self.g_d
+        cfg.g_d
             .assign(&mut layouter, g_d, a, b_0, b_1, a_prime, z13_a, z13_a_prime)?;
 
-        self.pk_d.assign(
+        cfg.pk_d.assign(
             &mut layouter,
             pk_d,
             b_3,
@@ -1704,9 +1736,9 @@ impl NoteCommitConfig {
             z14_b3_c_prime,
         )?;
 
-        self.value.assign(&mut layouter, value, d_2, z1_d, e_0)?;
+        cfg.value.assign(&mut layouter, value, d_2, z1_d, e_0)?;
 
-        self.rho.assign(
+        cfg.rho.assign(
             &mut layouter,
             rho,
             e_1,
@@ -1717,7 +1749,7 @@ impl NoteCommitConfig {
             z14_e1_f_prime,
         )?;
 
-        self.psi.assign(
+        cfg.psi.assign(
             &mut layouter,
             psi,
             g_1,
@@ -1734,7 +1766,7 @@ impl NoteCommitConfig {
 
     // A canonicity check helper used in checking x(g_d), y(g_d), and y(pk_d).
     fn canon_bitshift_130(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
         a: AssignedCell<pallas::Base, pallas::Base>,
     ) -> Result<CanonicityBounds, Error> {
@@ -1752,7 +1784,7 @@ impl NoteCommitConfig {
             let t_p = pallas::Base::from_u128(T_P);
             a + two_pow_130 - t_p
         });
-        let zs = self.sinsemilla_config.lookup_config().witness_check(
+        let zs = lookup_config.witness_check(
             layouter.namespace(|| "Decompose low 130 bits of (a + 2^130 - t_P)"),
             a_prime,
             13,
@@ -1766,7 +1798,7 @@ impl NoteCommitConfig {
 
     // Check canonicity of `x(pk_d)` encoding
     fn pkd_x_canonicity(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
         b_3: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         c: AssignedCell<pallas::Base, pallas::Base>,
@@ -1791,7 +1823,7 @@ impl NoteCommitConfig {
             b_3 + (two_pow_4 * c) + two_pow_140 - t_p
         });
 
-        let zs = self.sinsemilla_config.lookup_config().witness_check(
+        let zs = lookup_config.witness_check(
             layouter.namespace(|| "Decompose low 140 bits of (b_3 + 2^4 c + 2^140 - t_P)"),
             b3_c_prime,
             14,
@@ -1805,7 +1837,7 @@ impl NoteCommitConfig {
 
     // Check canonicity of `rho` encoding
     fn rho_canonicity(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
         e_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         f: AssignedCell<pallas::Base, pallas::Base>,
@@ -1830,7 +1862,7 @@ impl NoteCommitConfig {
         // Decompose the low 140 bits of e1_f_prime = e_1 + 2^4 f + 2^140 - t_P,
         // and output the running sum at the end of it.
         // If e1_f_prime < 2^140, the running sum will be 0.
-        let zs = self.sinsemilla_config.lookup_config().witness_check(
+        let zs = lookup_config.witness_check(
             layouter.namespace(|| "Decompose low 140 bits of (e_1 + 2^4 f + 2^140 - t_P)"),
             e1_f_prime,
             14,
@@ -1844,7 +1876,7 @@ impl NoteCommitConfig {
 
     // Check canonicity of `psi` encoding
     fn psi_canonicity(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
         g_1: RangeConstrained<pallas::Base, AssignedCell<pallas::Base, pallas::Base>>,
         g_2: AssignedCell<pallas::Base, pallas::Base>,
@@ -1867,7 +1899,7 @@ impl NoteCommitConfig {
             g_1 + (two_pow_9 * g_2) + two_pow_130 - t_p
         });
 
-        let zs = self.sinsemilla_config.lookup_config().witness_check(
+        let zs = lookup_config.witness_check(
             layouter.namespace(|| "Decompose low 130 bits of (g_1 + (2^9)g_2 + 2^130 - t_P)"),
             g1_g2_prime,
             13,
@@ -1882,7 +1914,8 @@ impl NoteCommitConfig {
     // Check canonicity of y-coordinate given its LSB as a value.
     // Also, witness the LSB and return the witnessed cell.
     fn y_canonicity(
-        &self,
+        lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
+        y_canon: &YCanonicity,
         mut layouter: impl Layouter<pallas::Base>,
         y: AssignedCell<pallas::Base, pallas::Base>,
         lsb: RangeConstrained<pallas::Base, Option<pallas::Base>>,
@@ -1894,7 +1927,7 @@ impl NoteCommitConfig {
 
         // Range-constrain k_0 to be 9 bits.
         let k_0 = RangeConstrained::witness_short(
-            &self.sinsemilla_config.lookup_config(),
+            lookup_config,
             layouter.namespace(|| "k_0"),
             y.value(),
             1..10,
@@ -1905,7 +1938,7 @@ impl NoteCommitConfig {
 
         // Range-constrain k_2 to be 4 bits.
         let k_2 = RangeConstrained::witness_short(
-            &self.sinsemilla_config.lookup_config(),
+            lookup_config,
             layouter.namespace(|| "k_2"),
             y.value(),
             250..254,
@@ -1926,7 +1959,7 @@ impl NoteCommitConfig {
                     let two_pow_10 = pallas::Base::from(1 << 10);
                     lsb + two * k_0 + two_pow_10 * k_1
                 });
-            let zs = self.sinsemilla_config.lookup_config().witness_check(
+            let zs = lookup_config.witness_check(
                 layouter.namespace(|| "Decompose j = LSB + (2)k_0 + (2^10)k_1"),
                 j,
                 25,
@@ -1937,12 +1970,13 @@ impl NoteCommitConfig {
 
         // Decompose j_prime = j + 2^130 - t_P using 13 ten-bit lookups.
         // We can reuse the canon_bitshift_130 logic here.
-        let (j_prime, z13_j_prime) = self.canon_bitshift_130(
+        let (j_prime, z13_j_prime) = canon_bitshift_130(
+            lookup_config,
             layouter.namespace(|| "j_prime = j + 2^130 - t_P"),
             j.clone(),
         )?;
 
-        self.y_canon.assign(
+        y_canon.assign(
             &mut layouter,
             y,
             lsb,
@@ -1964,7 +1998,10 @@ mod tests {
 
     use super::NoteCommitConfig;
     use crate::{
-        circuit::gadget::assign_free_advice,
+        circuit::{
+            gadget::assign_free_advice,
+            note_commit::{gadgets, NoteCommitChip},
+        },
         constants::{
             fixed_bases::NOTE_COMMITMENT_PERSONALIZATION, OrchardCommitDomains, OrchardFixedBases,
             OrchardHashDomains, L_ORCHARD_BASE, L_VALUE, T_Q,
@@ -2068,7 +2105,7 @@ mod tests {
                     range_check,
                 );
                 let note_commit_config =
-                    NoteCommitConfig::configure(meta, advices, sinsemilla_config);
+                    NoteCommitChip::configure(meta, advices, sinsemilla_config);
 
                 let ecc_config = EccChip::<OrchardFixedBases>::configure(
                     meta,
@@ -2101,6 +2138,9 @@ mod tests {
                 // Construct an ECC chip
                 let ecc_chip = EccChip::construct(ecc_config);
 
+                // Construct a NoteCommit chip
+                let note_commit_chip = NoteCommitChip::construct(note_commit_config.clone());
+
                 // Witness g_d
                 let g_d = {
                     let g_d = self.gd_x.zip(self.gd_y_lsb).map(|(x, y_lsb)| {
@@ -2167,10 +2207,11 @@ mod tests {
 
                 let rcm = pallas::Scalar::random(OsRng);
 
-                let cm = note_commit_config.assign_region(
+                let cm = gadgets::note_commit(
                     layouter.namespace(|| "Hash NoteCommit pieces"),
                     sinsemilla_chip,
                     ecc_chip.clone(),
+                    note_commit_chip,
                     g_d.inner(),
                     pk_d.inner(),
                     value_var,

From b46e4822d28699ffc837ac8f571816e84b61c568 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Wed, 4 May 2022 02:59:22 +0000
Subject: [PATCH 19/21] Update comments on `gadget::note_commit`

---
 src/circuit/note_commit.rs | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index 9fa64df7..a143168e 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -1647,6 +1647,11 @@ pub(in crate::circuit) mod gadgets {
             d_1,
         )?;
 
+        // cm = NoteCommit^Orchard_rcm(g★_d || pk★_d || i2lebsp_{64}(v) || rho || psi)
+        //
+        // `cm = ⊥` is handled internally to `CommitDomain::commit`: incomplete addition
+        // constraints allows ⊥ to occur, and then during synthesis it detects these edge
+        // cases and raises an error (aborting proof creation).
         let (cm, zs) = {
             let message = Message::from_pieces(
                 chip.clone(),
@@ -1669,6 +1674,8 @@ pub(in crate::circuit) mod gadgets {
             )?
         };
 
+        // `CommitDomain::commit` returns the running sum for each `MessagePiece`. Grab
+        // the outputs that we will need for canonicity checks.
         let z13_a = zs[0][13].clone();
         let z13_c = zs[2][13].clone();
         let z1_d = zs[3][1].clone();
@@ -1677,6 +1684,7 @@ pub(in crate::circuit) mod gadgets {
         let g_2 = z1_g.clone();
         let z13_g = zs[6][13].clone();
 
+        // Witness and constrain the bounds we need to ensure canonicity.
         let (a_prime, z13_a_prime) = canon_bitshift_130(
             &lookup_config,
             layouter.namespace(|| "x(g_d) canonicity"),
@@ -1704,6 +1712,7 @@ pub(in crate::circuit) mod gadgets {
             g_2,
         )?;
 
+        // Finally, assign values to all of the NoteCommit regions.
         let cfg = note_commit_chip.config;
 
         let b_1 = cfg
@@ -1764,7 +1773,7 @@ pub(in crate::circuit) mod gadgets {
         Ok(cm)
     }
 
-    // A canonicity check helper used in checking x(g_d), y(g_d), and y(pk_d).
+    /// A canonicity check helper used in checking x(g_d), y(g_d), and y(pk_d).
     fn canon_bitshift_130(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
@@ -1796,7 +1805,7 @@ pub(in crate::circuit) mod gadgets {
         Ok((a_prime, zs[13].clone()))
     }
 
-    // Check canonicity of `x(pk_d)` encoding
+    /// Check canonicity of `x(pk_d)` encoding.
     fn pkd_x_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
@@ -1835,7 +1844,7 @@ pub(in crate::circuit) mod gadgets {
         Ok((b3_c_prime, zs[14].clone()))
     }
 
-    // Check canonicity of `rho` encoding
+    /// Check canonicity of `rho` encoding.
     fn rho_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
@@ -1874,7 +1883,7 @@ pub(in crate::circuit) mod gadgets {
         Ok((e1_f_prime, zs[14].clone()))
     }
 
-    // Check canonicity of `psi` encoding
+    /// Check canonicity of `psi` encoding.
     fn psi_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         mut layouter: impl Layouter<pallas::Base>,
@@ -1911,8 +1920,8 @@ pub(in crate::circuit) mod gadgets {
         Ok((g1_g2_prime, zs[13].clone()))
     }
 
-    // Check canonicity of y-coordinate given its LSB as a value.
-    // Also, witness the LSB and return the witnessed cell.
+    /// Check canonicity of y-coordinate given its LSB as a value.
+    /// Also, witness the LSB and return the witnessed cell.
     fn y_canonicity(
         lookup_config: &LookupRangeCheckConfig<pallas::Base, 10>,
         y_canon: &YCanonicity,

From 8c7bb5b95d29879baa84aabbaff23201d7989f33 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Wed, 4 May 2022 23:54:16 +0000
Subject: [PATCH 20/21] Rename `RangeConstrained::subset_of` to `bitrange_of`

---
 Cargo.toml                 |  4 ++--
 src/circuit/commit_ivk.rs  |  8 ++++----
 src/circuit/note_commit.rs | 28 ++++++++++++++--------------
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 275fc71e..19ec4988 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -82,6 +82,6 @@ debug = true
 debug = true
 
 [patch.crates-io]
-halo2_gadgets = { git = "https://github.com/zcash/halo2.git", rev = "97864d714e33c5258b4ca3f25c9ddc1624e240dd" }
-halo2_proofs = { git = "https://github.com/zcash/halo2.git", rev = "97864d714e33c5258b4ca3f25c9ddc1624e240dd" }
+halo2_gadgets = { git = "https://github.com/zcash/halo2.git", rev = "3800de59188a73b4e04f689c8bcc855a2fc7fdcf" }
+halo2_proofs = { git = "https://github.com/zcash/halo2.git", rev = "3800de59188a73b4e04f689c8bcc855a2fc7fdcf" }
 incrementalmerkletree = { git = "https://github.com/zcash/incrementalmerkletree.git", rev = "f23e3d89507849a24543121839eea6f40b141aff" }
diff --git a/src/circuit/commit_ivk.rs b/src/circuit/commit_ivk.rs
index c3d2c0cc..69d3ae67 100644
--- a/src/circuit/commit_ivk.rs
+++ b/src/circuit/commit_ivk.rs
@@ -262,7 +262,7 @@ pub(in crate::circuit) mod gadgets {
         let a = MessagePiece::from_subpieces(
             sinsemilla_chip.clone(),
             layouter.namespace(|| "a"),
-            [RangeConstrained::subset_of(ak.value(), 0..250)],
+            [RangeConstrained::bitrange_of(ak.value(), 0..250)],
         )?;
 
         // `b = b_0||b_1||b_2`
@@ -276,7 +276,7 @@ pub(in crate::circuit) mod gadgets {
                 250..254,
             )?;
             // b_1 will be boolean-constrained in the custom gate.
-            let b_1 = RangeConstrained::subset_of(ak.value(), 254..255);
+            let b_1 = RangeConstrained::bitrange_of(ak.value(), 254..255);
             // Constrain b_2 to be 5 bits.
             let b_2 = RangeConstrained::witness_short(
                 &lookup_config,
@@ -298,7 +298,7 @@ pub(in crate::circuit) mod gadgets {
         let c = MessagePiece::from_subpieces(
             sinsemilla_chip.clone(),
             layouter.namespace(|| "c"),
-            [RangeConstrained::subset_of(nk.value(), 5..245)],
+            [RangeConstrained::bitrange_of(nk.value(), 5..245)],
         )?;
 
         // `d = d_0||d_1` = (bits 245..=253 of `nk`) || (bit 254 of `nk`)
@@ -311,7 +311,7 @@ pub(in crate::circuit) mod gadgets {
                 245..254,
             )?;
             // d_1 will be boolean-constrained in the custom gate.
-            let d_1 = RangeConstrained::subset_of(nk.value(), 254..255);
+            let d_1 = RangeConstrained::bitrange_of(nk.value(), 254..255);
 
             let d = MessagePiece::from_subpieces(
                 sinsemilla_chip.clone(),
diff --git a/src/circuit/note_commit.rs b/src/circuit/note_commit.rs
index a143168e..0b7763c3 100644
--- a/src/circuit/note_commit.rs
+++ b/src/circuit/note_commit.rs
@@ -143,8 +143,8 @@ impl DecomposeB {
         )?;
 
         // b_1, b_2 will be boolean-constrained in the gate.
-        let b_1 = RangeConstrained::subset_of(gd_x.value(), 254..255);
-        let b_2 = RangeConstrained::subset_of(gd_y.value(), 0..1);
+        let b_1 = RangeConstrained::bitrange_of(gd_x.value(), 254..255);
+        let b_2 = RangeConstrained::bitrange_of(gd_y.value(), 0..1);
 
         // Constrain b_3 to be 4 bits
         let b_3 = RangeConstrained::witness_short(
@@ -282,8 +282,8 @@ impl DecomposeD {
         let value_val = value.value().map(|v| pallas::Base::from(v.inner()));
 
         // d_0, d_1 will be boolean-constrained in the gate.
-        let d_0 = RangeConstrained::subset_of(pk_d.x().value(), 254..255);
-        let d_1 = RangeConstrained::subset_of(pk_d.y().value(), 0..1);
+        let d_0 = RangeConstrained::bitrange_of(pk_d.x().value(), 254..255);
+        let d_1 = RangeConstrained::bitrange_of(pk_d.y().value(), 0..1);
 
         // Constrain d_2 to be 8 bits
         let d_2 = RangeConstrained::witness_short(
@@ -294,7 +294,7 @@ impl DecomposeD {
         )?;
 
         // d_3 = z1_d from the SinsemillaHash(d) running sum output.
-        let d_3 = RangeConstrained::subset_of(value_val.as_ref(), 8..58);
+        let d_3 = RangeConstrained::bitrange_of(value_val.as_ref(), 8..58);
 
         let d = MessagePiece::from_subpieces(
             chip,
@@ -527,7 +527,7 @@ impl DecomposeG {
         Error,
     > {
         // g_0 will be boolean-constrained in the gate.
-        let g_0 = RangeConstrained::subset_of(rho.value(), 254..255);
+        let g_0 = RangeConstrained::bitrange_of(rho.value(), 254..255);
 
         // Constrain g_1 to be 9 bits.
         let g_1 = RangeConstrained::witness_short(
@@ -538,7 +538,7 @@ impl DecomposeG {
         )?;
 
         // g_2 = z1_g from the SinsemillaHash(g) running sum output.
-        let g_2 = RangeConstrained::subset_of(psi.value(), 9..249);
+        let g_2 = RangeConstrained::bitrange_of(psi.value(), 9..249);
 
         let g = MessagePiece::from_subpieces(
             chip,
@@ -659,7 +659,7 @@ impl DecomposeH {
         )?;
 
         // h_1 will be boolean-constrained in the gate.
-        let h_1 = RangeConstrained::subset_of(psi.value(), 254..255);
+        let h_1 = RangeConstrained::bitrange_of(psi.value(), 254..255);
 
         let h = MessagePiece::from_subpieces(
             chip,
@@ -667,7 +667,7 @@ impl DecomposeH {
             [
                 h_0.value(),
                 h_1,
-                RangeConstrained::subset_of(Some(&pallas::Base::zero()), 0..4),
+                RangeConstrained::bitrange_of(Some(&pallas::Base::zero()), 0..4),
             ],
         )?;
 
@@ -1589,7 +1589,7 @@ pub(in crate::circuit) mod gadgets {
         let a = MessagePiece::from_subpieces(
             chip.clone(),
             layouter.namespace(|| "a"),
-            [RangeConstrained::subset_of(g_d.x().value(), 0..250)],
+            [RangeConstrained::bitrange_of(g_d.x().value(), 0..250)],
         )?;
 
         // b = b_0 || b_1 || b_2 || b_3
@@ -1601,7 +1601,7 @@ pub(in crate::circuit) mod gadgets {
         let c = MessagePiece::from_subpieces(
             chip.clone(),
             layouter.namespace(|| "c"),
-            [RangeConstrained::subset_of(pk_d.x().value(), 4..254)],
+            [RangeConstrained::bitrange_of(pk_d.x().value(), 4..254)],
         )?;
 
         // d = d_0 || d_1 || d_2 || d_3
@@ -1617,7 +1617,7 @@ pub(in crate::circuit) mod gadgets {
         let f = MessagePiece::from_subpieces(
             chip.clone(),
             layouter.namespace(|| "f"),
-            [RangeConstrained::subset_of(rho.value(), 4..254)],
+            [RangeConstrained::bitrange_of(rho.value(), 4..254)],
         )?;
 
         // g = g_0 || g_1 || g_2
@@ -1943,7 +1943,7 @@ pub(in crate::circuit) mod gadgets {
         )?;
 
         // k_1 will be constrained by the decomposition of j.
-        let k_1 = RangeConstrained::subset_of(y.value(), 10..250);
+        let k_1 = RangeConstrained::bitrange_of(y.value(), 10..250);
 
         // Range-constrain k_2 to be 4 bits.
         let k_2 = RangeConstrained::witness_short(
@@ -1954,7 +1954,7 @@ pub(in crate::circuit) mod gadgets {
         )?;
 
         // k_3 will be boolean-constrained in the gate.
-        let k_3 = RangeConstrained::subset_of(y.value(), 254..255);
+        let k_3 = RangeConstrained::bitrange_of(y.value(), 254..255);
 
         // Decompose j = LSB + (2)k_0 + (2^10)k_1 using 25 ten-bit lookups.
         let (j, z1_j, z13_j) = {

From 0603d602d0f0aa7b2ed4d23396479338601ceacb Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@electriccoin.co>
Date: Wed, 4 May 2022 23:57:26 +0000
Subject: [PATCH 21/21] Rename `anchor` to `root`, and `pub_input_anchor` to
 `anchor`

This ensures that we are consistent in the circuit in referring to the
public bundle anchor as `anchor`, and the calculated Merkle tree root as
`root`.
---
 src/circuit.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/circuit.rs b/src/circuit.rs
index 7e1faa65..7accc42d 100644
--- a/src/circuit.rs
+++ b/src/circuit.rs
@@ -145,7 +145,7 @@ impl plonk::Circuit<pallas::Base> for Circuit {
         ];
 
         // Constrain v_old - v_new = magnitude * sign
-        // Either v_old = 0, or anchor equals public input
+        // Either v_old = 0, or calculated root = anchor
         // Constrain v_old = 0 or enable_spends = 1.
         // Constrain v_new = 0 or enable_outputs = 1.
         let q_orchard = meta.selector();
@@ -156,8 +156,8 @@ impl plonk::Circuit<pallas::Base> for Circuit {
             let magnitude = meta.query_advice(advices[2], Rotation::cur());
             let sign = meta.query_advice(advices[3], Rotation::cur());
 
-            let anchor = meta.query_advice(advices[4], Rotation::cur());
-            let pub_input_anchor = meta.query_advice(advices[5], Rotation::cur());
+            let root = meta.query_advice(advices[4], Rotation::cur());
+            let anchor = meta.query_advice(advices[5], Rotation::cur());
 
             let enable_spends = meta.query_advice(advices[6], Rotation::cur());
             let enable_outputs = meta.query_advice(advices[7], Rotation::cur());
@@ -172,8 +172,8 @@ impl plonk::Circuit<pallas::Base> for Circuit {
                         v_old.clone() - v_new.clone() - magnitude * sign,
                     ),
                     (
-                        "Either v_old = 0, or anchor equals public input",
-                        v_old.clone() * (anchor - pub_input_anchor),
+                        "Either v_old = 0, or root = anchor",
+                        v_old.clone() * (root - anchor),
                     ),
                     (
                         "v_old = 0 or enable_spends = 1",