Apply suggestions from code review

Co-authored-by: iquerejeta <[email protected]>

Author: curiecrypt

mithril-core/src/error.rs

@@ -131,17 +131,6 @@ pub enum RegisterError {
     UnregisteredInitializer,
 }
 
-// impl From<RegisterError> for StmSignatureError {
-//     fn from(e: RegisterError) -> Self {
-//         match e {
-//             RegisterError::SerializationError => Self::SerializationError,
-//             RegisterError::KeyInvalid(e) => Self::IvkInvalid(e.vk),
-//             RegisterError::KeyRegistered(_) => unreachable!(),
-//             RegisterError::UnregisteredInitializer => unreachable!(),
-//         }
-//     }
-// }
-
 impl<D: Digest + FixedOutput> From<MerkleTreeError<D>> for StmAggregateSignatureError<D> {
     fn from(e: MerkleTreeError<D>) -> Self {
         match e {

mithril-core/src/merkle_tree.rs

@@ -173,6 +173,7 @@ impl<D: Digest + FixedOutput> BatchPath<D> {
     }
 
     /// Try to convert a byte string into a `BatchPath`.
+    // todo: We should not panic if the size of the slice is invalid (I believe `bytes[offset + i * 8..offset + (i + 1) * 8]` will panic if bytes is not large enough. 
     pub fn from_bytes(bytes: &[u8]) -> Result<Self, MerkleTreeError<D>> {
         let mut u64_bytes = [0u8; 8];
         u64_bytes.copy_from_slice(&bytes[..8]);
@@ -268,6 +269,7 @@ impl<D: Clone + Digest> MerkleTreeCommitmentBatchCompat<D> {
     /// Returns an error if the proof is invalid.
     // todo: Update doc.
     // todo: Simplify the algorithm.
+    // todo: Maybe we want more granular errors, rather than only `BatchPathInvalid`
     pub fn check(
         &self,
         batch_val: &Vec<MTLeaf>,
@@ -670,25 +672,22 @@ mod tests {
         fn test_create_batch_proof((t, values) in arb_tree(30)) {
             let mut mt_index_list :Vec<usize> = Vec::new();
             for (i, _v) in values.iter().enumerate() {
-                let ind = Some(i as usize);
-                mt_index_list.push(ind.unwrap());
+                mt_index_list.push(i);
             }
             mt_index_list.sort_unstable();
-            let batch_proof = Some(t.get_batched_path(mt_index_list));
-            assert!(t.to_commitment_batch_compat().check(&values, &batch_proof.unwrap()).is_ok());
+            let batch_proof = t.get_batched_path(mt_index_list);
+            assert!(t.to_commitment_batch_compat().check(&values, &batch_proof).is_ok());
         }
 
         #[test]
         fn test_bytes_batch_path((t, values) in arb_tree(30)) {
             let mut mt_index_list :Vec<usize> = Vec::new();
             for (i, _v) in values.iter().enumerate() {
-                let ind = Some(i as usize);
-                mt_index_list.push(ind.unwrap());
+                mt_index_list.push(i);
             }
             mt_index_list.sort_unstable();
 
-            let batch_proof = Some(t.get_batched_path(mt_index_list));
-            let bp = batch_proof.unwrap();
+            let bp = t.get_batched_path(mt_index_list);
 
             let bytes = &bp.to_bytes();
             let deserialized = BatchPath::from_bytes(bytes).unwrap();
@@ -752,14 +751,8 @@ mod tests {
             (values, proof) in values_with_invalid_proof(10)
         ) {
             let t = MerkleTree::<Blake2b<U32>>::create(&values[1..]);
-            let mut indices = Vec::with_capacity(values.len() / 2);
-            let mut batch_values = Vec::with_capacity(values.len() / 2);
-
-            for _j in 0..values.len() / 2 {
-                let ind = i % (values.len() - 1);
-                indices.push(ind);
-                batch_values.push(values[ind]);
-            }
+            let indices = vec![i % (values.len() - 1); values.len() / 2];
+            let batch_values = vec![values[i % (values.len() - 1)]; values.len() / 2];
             let path = BatchPath{values: proof
                             .iter()
                             .map(|x|  Blake2b::<U32>::digest(x).to_vec())

mithril-core/src/stm.rs

@@ -427,7 +427,7 @@ impl<D: Digest + Clone + FixedOutput> StmClerk<D> {
     /// Aggregate a set of signatures for their corresponding indices.
     ///
     /// This function first deduplicates the repeated signatures, and if there are enough signatures, it collects the merkle tree indexes of unique signatures.
-    /// The list of merkle tree indexes is used to create a batch proof to be checked in verify aggregate.
+    /// The list of merkle tree indexes is used to create a batch proof, to prove that all signatures are from eligible signers.
     ///
     /// It returns an instance of `StmAggrSig`.
     pub fn aggregate(
@@ -622,14 +622,14 @@ impl StmSig {
         let sigma = Signature::from_bytes(&bytes[offset + 96..offset + 144])?;
 
         u64_bytes.copy_from_slice(&bytes[offset + 144..offset + 152]);
-        let mt_index = u64::from_be_bytes(u64_bytes);
+        let signer_index = u64::from_be_bytes(u64_bytes);
 
         Ok(StmSig {
             sigma,
             pk,
             stake,
             indexes,
-            signer_index: mt_index,
+            signer_index,
         })
     }
 
@@ -1176,8 +1176,7 @@ mod tests {
             })
         }
         #[test]
-        fn test_invalid_proof_path(tc in arb_proof_setup(10), _i in any::<usize>()) {
-            let _n = tc.n;
+        fn test_invalid_proof_path(tc in arb_proof_setup(10)) {
             with_proof_mod(tc, |aggr, _, _msg| {
                 let p = aggr.batch_proof.clone();
                 let mut index_list = p.indices.clone();