General code quality improvements.

src/circuit/mod.rs

@@ -29,6 +29,13 @@ use jubjub::{
 
 use constants;
 
+
+// TODO: This should probably be removed and we
+// should use existing helper methods on `Option`
+// for mapping with an error.
+/// This basically is just an extension to `Option`
+/// which allows for a convenient mapping to an
+/// error on `None`.
 trait Assignment<T> {
     fn get(&self) -> Result<&T, SynthesisError>;
 }
@@ -42,6 +49,7 @@ impl<T> Assignment<T> for Option<T> {
     }
 }
 
+/// This is an instance of the `Spend` circuit.
 pub struct Spend<'a, E: JubjubEngine> {
     pub params: &'a E::Params,
     /// Value of the note being spent
@@ -75,6 +83,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
         )?;
 
         {
+            // Compute the note value in the exponent
             let gv = ecc::fixed_base_multiplication(
                 cs.namespace(|| "compute the value in the exponent"),
                 FixedGenerators::ValueCommitmentValue,
@@ -82,12 +91,15 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
                 self.params
             )?;
         
-            // Booleanize the randomness
+            // Booleanize the randomness. This does not ensure
+            // the bit representation is "in the field" because
+            // it doesn't matter for security.
             let hr = boolean::field_into_boolean_vec_le(
                 cs.namespace(|| "hr"),
                 self.value_randomness
             )?;
 
+            // Compute the randomness in the exponent
             let hr = ecc::fixed_base_multiplication(
                 cs.namespace(|| "computation of randomization for value commitment"),
                 FixedGenerators::ValueCommitmentRandomness,
@@ -95,12 +107,14 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
                 self.params
             )?;
 
+            // Compute the Pedersen commitment to the value
             let gvhr = gv.add(
                 cs.namespace(|| "computation of value commitment"),
                 &hr,
                 self.params
             )?;
 
+            // Expose the commitment as an input to the circuit
             gvhr.inputize(cs.namespace(|| "value commitment"))?;
         }
 
@@ -118,6 +132,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
             // demonstrate the prover knows it. If they know a
             // congruency then that's equivalent.
 
+            // Compute rk = [rsk] ProvingPublicKey
             rk = ecc::fixed_base_multiplication(
                 cs.namespace(|| "computation of rk"),
                 FixedGenerators::ProofGenerationKey,
@@ -133,29 +148,40 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
             self.params
         )?;
 
+        // There are no sensible attacks on small order points
+        // of ak (that we're aware of!) but it's a cheap check,
+        // so we do it.
         ak.assert_not_small_order(
             cs.namespace(|| "ak not small order"),
             self.params
         )?;
 
         // Unpack ak and rk for input to BLAKE2s
+
+        // This is the "viewing key" preimage for CRH^ivk
         let mut vk = vec![];
-        let mut rho_preimage = vec![];
         vk.extend(
             ak.repr(cs.namespace(|| "representation of ak"))?
         );
+
+        // This is the nullifier randomness preimage for PRF^nr
+        let mut nr_preimage = vec![];
+
+        // Extend vk and nr preimages with the representation of
+        // rk.
         {
             let repr_rk = rk.repr(
                 cs.namespace(|| "representation of rk")
             )?;
 
             vk.extend(repr_rk.iter().cloned());
-            rho_preimage.extend(repr_rk);
+            nr_preimage.extend(repr_rk);
         }
 
         assert_eq!(vk.len(), 512);
+        assert_eq!(nr_preimage.len(), 256);
 
-        // Compute the incoming viewing key
+        // Compute the incoming viewing key ivk
         let mut ivk = blake2s::blake2s(
             cs.namespace(|| "computation of ivk"),
             &vk,
@@ -164,16 +190,24 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
 
         // Little endian bit order
         ivk.reverse();
-        ivk.truncate(E::Fs::CAPACITY as usize); // drop_5
 
-        // Witness g_d
+        // drop_5 to ensure it's in the field
+        ivk.truncate(E::Fs::CAPACITY as usize);
+
+        // Witness g_d. Ensures the point is on the
+        // curve, but not its order. If the prover
+        // manages to witness a commitment in the
+        // tree, then the Output circuit would have
+        // already guaranteed this.
+        // TODO: We might as well just perform the
+        // check again here, since it's not expensive.
         let g_d = ecc::EdwardsPoint::witness(
             cs.namespace(|| "witness g_d"),
             self.g_d,
             self.params
         )?;
 
-        // Compute pk_d
+        // Compute pk_d = g_d^ivk
         let pk_d = g_d.mul(
             cs.namespace(|| "compute pk_d"),
             &ivk,
@@ -181,6 +215,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
         )?;
 
         // Compute note contents
+        // value (in big endian) followed by g_d and pk_d
         let mut note_contents = vec![];
         note_contents.extend(value_bits.into_iter().rev());
         note_contents.extend(
@@ -206,12 +241,13 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
         )?;
 
         {
-            // Booleanize the randomness
+            // Booleanize the randomness for the note commitment
             let cmr = boolean::field_into_boolean_vec_le(
                 cs.namespace(|| "cmr"),
                 self.commitment_randomness
             )?;
 
+            // Compute the note commitment randomness in the exponent
             let cmr = ecc::fixed_base_multiplication(
                 cs.namespace(|| "computation of commitment randomness"),
                 FixedGenerators::NoteCommitmentRandomness,
@@ -219,6 +255,8 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
                 self.params
             )?;
 
+            // Randomize the note commitment. Pedersen hashes are not
+            // themselves hiding commitments.
             cm = cm.add(
                 cs.namespace(|| "randomization of note commitment"),
                 &cmr,
@@ -228,21 +266,30 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
 
         let tree_depth = self.auth_path.len();
 
+        // This will store (least significant bit first)
+        // the position of the note in the tree, for use
+        // in nullifier computation.
         let mut position_bits = vec![];
 
-        // Injective encoding.
+        // This is an injective encoding, as cur is a
+        // point in the prime order subgroup.
         let mut cur = cm.get_x().clone();
 
         for (i, e) in self.auth_path.into_iter().enumerate() {
             let cs = &mut cs.namespace(|| format!("merkle tree hash {}", i));
 
+            // Determines if the current subtree is the "right" leaf at this
+            // depth of the tree.
             let cur_is_right = boolean::Boolean::from(boolean::AllocatedBit::alloc(
                 cs.namespace(|| "position bit"),
                 e.map(|e| e.1)
             )?);
 
+            // Push this boolean for nullifier computation later
             position_bits.push(cur_is_right.clone());
 
+            // Witness the authentication path element adjacent
+            // at this depth.
             let path_element = num::AllocatedNum::alloc(
                 cs.namespace(|| "path element"),
                 || {
@@ -250,6 +297,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
                 }
             )?;
 
+            // Swap the two if the current subtree is on the right
             let (xl, xr) = num::AllocatedNum::conditionally_reverse(
                 cs.namespace(|| "conditional reversal of preimage"),
                 &cur,
@@ -265,6 +313,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
             preimage.extend(xl.into_bits_le(cs.namespace(|| "xl into bits"))?);
             preimage.extend(xr.into_bits_le(cs.namespace(|| "xr into bits"))?);
 
+            // Compute the new subtree value
             cur = pedersen_hash::pedersen_hash(
                 cs.namespace(|| "computation of pedersen hash"),
                 pedersen_hash::Personalization::MerkleTree(i),
@@ -278,7 +327,10 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
         // Expose the anchor
         cur.inputize(cs.namespace(|| "anchor"))?;
 
+        // Compute the cm + g^position for preventing
+        // faerie gold attacks
         {
+            // Compute the position in the exponent
             let position = ecc::fixed_base_multiplication(
                 cs.namespace(|| "g^position"),
                 FixedGenerators::NullifierPosition,
@@ -286,6 +338,7 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
                 self.params
             )?;
 
+            // Add the position to the commitment
             cm = cm.add(
                 cs.namespace(|| "faerie gold prevention"),
                 &position,
@@ -293,30 +346,36 @@ impl<'a, E: JubjubEngine> Circuit<E> for Spend<'a, E> {
             )?;
         }
         
-        // Let's compute rho = BLAKE2s(rk || cm + position)
+        // Let's compute nr = BLAKE2s(rk || cm + position)
-        rho_preimage.extend(
+        nr_preimage.extend(
             cm.repr(cs.namespace(|| "representation of cm"))?
         );
 
-        assert_eq!(rho_preimage.len(), 512);
+        assert_eq!(nr_preimage.len(), 512);
         
-        let mut rho = blake2s::blake2s(
+        // Compute nr
-            cs.namespace(|| "rho computation"),
+        let mut nr = blake2s::blake2s(
-            &rho_preimage,
+            cs.namespace(|| "nr computation"),
+            &nr_preimage,
             constants::PRF_NR_PERSONALIZATION
         )?;
 
         // Little endian bit order
-        rho.reverse();
+        nr.reverse();
-        rho.truncate(E::Fs::CAPACITY as usize); // drop_5
+
+        // We want the randomization in the field to
+        // simplify outside code.
+        // TODO: This isn't uniformly random.
+        nr.truncate(E::Fs::CAPACITY as usize);
 
         // Compute nullifier
         let nf = ak.mul(
             cs.namespace(|| "computation of nf"),
-            &rho,
+            &nr,
             self.params
         )?;
 
+        // Expose the nullifier publicly
         nf.inputize(cs.namespace(|| "nullifier"))?;
 
         Ok(())

src/group_hash.rs

@@ -1,5 +1,14 @@
-use jubjub::*;
+use jubjub::{
-use pairing::*;
+    JubjubEngine,
+    PrimeOrder,
+    edwards
+};
+
+use pairing::{
+    PrimeField,
+    PrimeFieldRepr
+};
+
 use blake2_rfc::blake2s::Blake2s;
 use constants;
 

src/jubjub/mod.rs

@@ -33,8 +33,14 @@ use pairing::bls12_381::{
     Fr
 };
 
+/// This is an implementation of the twisted Edwards Jubjub curve.
 pub mod edwards;
+
+/// This is an implementation of the birationally equivalent
+/// Montgomery curve.
 pub mod montgomery;
+
+/// This is an implementation of the scalar field for Jubjub.
 pub mod fs;
 
 #[cfg(test)]
@@ -83,7 +89,9 @@ pub enum FixedGenerators {
 /// offers a scalar field for the embedded curve (Jubjub)
 /// and some pre-computed parameters.
 pub trait JubjubEngine: Engine {
+    /// The scalar field of the Jubjub curve
     type Fs: PrimeField + SqrtField;
+    /// The parameters of Jubjub and the Sapling protocol
     type Params: JubjubParams<Self>;
 }
 
@@ -167,7 +175,7 @@ impl JubjubBls12 {
         let mut montgomery_2a = montgomery_a;
         montgomery_2a.double();
 
-        let mut tmp = JubjubBls12 {
+        let mut tmp_params = JubjubBls12 {
             // d = -(10240/10241)
             edwards_d: Fr::from_str("19257038036680949359750312669786877991949435402254120286184196891950884077233").unwrap(),
             // A = 40962
@@ -177,20 +185,24 @@ impl JubjubBls12 {
             // scaling factor = sqrt(4 / (a - d))
             scale: Fr::from_str("17814886934372412843466061268024708274627479829237077604635722030778476050649").unwrap(),
 
+            // We'll initialize these below
             pedersen_hash_generators: vec![],
             pedersen_circuit_generators: vec![],
-
             fixed_base_generators: vec![],
             fixed_base_circuit_generators: vec![],
         };
 
         // Create the bases for the Pedersen hashes
         {
+            // TODO: This currently does not match the specification
             let mut cur = 0;
             let mut pedersen_hash_generators = vec![];
 
+            // TODO: This generates more bases for the Pedersen hashes
+            // than necessary, which is just a performance issue in
+            // practice.
             while pedersen_hash_generators.len() < 5 {
-                let gh = group_hash(&[cur], constants::PEDERSEN_HASH_GENERATORS_PERSONALIZATION, &tmp);
+                let gh = group_hash(&[cur], constants::PEDERSEN_HASH_GENERATORS_PERSONALIZATION, &tmp_params);
                 // We don't want to overflow and start reusing generators
                 assert!(cur != u8::max_value());
                 cur += 1;
@@ -200,7 +212,20 @@ impl JubjubBls12 {
                 }
             }
 
-            tmp.pedersen_hash_generators = pedersen_hash_generators;
+            // Check for duplicates, far worse than spec inconsistencies!
+            for (i, p1) in pedersen_hash_generators.iter().enumerate() {
+                if p1 == &edwards::Point::zero() {
+                    panic!("Neutral element!");
+                }
+
+                for p2 in pedersen_hash_generators.iter().skip(i+1) {
+                    if p1 == p2 {
+                        panic!("Duplicate generator!");
+                    }
+                }
+            }
+
+            tmp_params.pedersen_hash_generators = pedersen_hash_generators;
         }
 
         // Create the bases for other parts of the protocol
@@ -211,10 +236,10 @@ impl JubjubBls12 {
                 // Each generator is found by invoking the group hash
                 // on tag 0x00, 0x01, ... until we find a valid result.
                 let find_first_gh = |personalization| {
-                    let mut cur = 0;
+                    let mut cur = 0u8;
 
                     loop {
-                        let gh = group_hash::<Bls12>(&[cur], personalization, &tmp);
+                        let gh = group_hash::<Bls12>(&[cur], personalization, &tmp_params);
                         // We don't want to overflow.
                         assert!(cur != u8::max_value());
                         cur += 1;
@@ -267,7 +292,7 @@ impl JubjubBls12 {
                 }
             }
 
-            tmp.fixed_base_generators = fixed_base_generators;
+            tmp_params.fixed_base_generators = fixed_base_generators;
         }
 
         // Create the 2-bit window table lookups for each 4-bit
@@ -276,10 +301,10 @@ impl JubjubBls12 {
             let mut pedersen_circuit_generators = vec![];
 
             // Process each segment
-            for mut gen in tmp.pedersen_hash_generators.iter().cloned() {
+            for mut gen in tmp_params.pedersen_hash_generators.iter().cloned() {
-                let mut gen = montgomery::Point::from_edwards(&gen, &tmp);
+                let mut gen = montgomery::Point::from_edwards(&gen, &tmp_params);
                 let mut windows = vec![];
-                for _ in 0..tmp.pedersen_hash_chunks_per_generator() {
+                for _ in 0..tmp_params.pedersen_hash_chunks_per_generator() {
                     // Create (x, y) coeffs for this chunk
                     let mut coeffs = vec![];
                     let mut g = gen.clone();
@@ -287,19 +312,19 @@ impl JubjubBls12 {
                     // coeffs = g, g*2, g*3, g*4
                     for _ in 0..4 {
                         coeffs.push(g.into_xy().expect("cannot produce O"));
-                        g = g.add(&gen, &tmp);
+                        g = g.add(&gen, &tmp_params);
                     }
                     windows.push(coeffs);
 
                     // Our chunks are separated by 2 bits to prevent overlap.
                     for _ in 0..4 {
-                        gen = gen.double(&tmp);
+                        gen = gen.double(&tmp_params);
                     }
                 }
                 pedersen_circuit_generators.push(windows);
             }
 
-            tmp.pedersen_circuit_generators = pedersen_circuit_generators;
+            tmp_params.pedersen_circuit_generators = pedersen_circuit_generators;
         }
 
         // Create the 3-bit window table lookups for fixed-base
@@ -307,14 +332,14 @@ impl JubjubBls12 {
         {
             let mut fixed_base_circuit_generators = vec![];
 
-            for mut gen in tmp.fixed_base_generators.iter().cloned() {
+            for mut gen in tmp_params.fixed_base_generators.iter().cloned() {
                 let mut windows = vec![];
-                for _ in 0..tmp.fixed_base_chunks_per_generator() {
+                for _ in 0..tmp_params.fixed_base_chunks_per_generator() {
                     let mut coeffs = vec![(Fr::zero(), Fr::one())];
                     let mut g = gen.clone();
                     for _ in 0..7 {
                         coeffs.push(g.into_xy());
-                        g = g.add(&gen, &tmp);
+                        g = g.add(&gen, &tmp_params);
                     }
                     windows.push(coeffs);
 
@@ -324,10 +349,10 @@ impl JubjubBls12 {
                 fixed_base_circuit_generators.push(windows);
             }
 
-            tmp.fixed_base_circuit_generators = fixed_base_circuit_generators;
+            tmp_params.fixed_base_circuit_generators = fixed_base_circuit_generators;
         }
 
-        tmp
+        tmp_params
     }
 }
 

src/lib.rs

@@ -10,8 +10,8 @@ extern crate byteorder;
 extern crate hex_literal;
 
 pub mod jubjub;
-pub mod circuit;
 pub mod group_hash;
+pub mod circuit;
 pub mod pedersen_hash;
 pub mod primitives;
-mod constants;
+pub mod constants;