Simplify share threshold and secret length consistency validation

I think that using hashmaps and hash sets was overkill and made the code much
longer and complicated than it needed to be.

The new code also produces more useful error messages that will hopefully help
users identify which share(s) are causing the inconsistency.

src/errors.rs

@@ -59,16 +59,6 @@ error_chain! {
             display("The shares are incompatible with each other.")
         }
 
-        IncompatibleThresholds(sets: Vec<HashSet<u8>>) {
-            description("The shares are incompatible with each other because they do not all have the same threshold.")
-            display("The shares are incompatible with each other because they do not all have the same threshold.")
-        }
-
-        IncompatibleDataLengths(sets: Vec<HashSet<u8>>) {
-            description("The shares are incompatible with each other because they do not all have the same share data length.")
-            display("The shares are incompatible with each other because they do not all have the same share data length.")
-        }
-
         MissingShares(provided: usize, required: u8) {
             description("The number of shares provided is insufficient to recover the secret.")
             display("{} shares are required to recover the secret, found only {}.", required, provided)
@@ -133,10 +123,21 @@ error_chain! {
             display("This share number ({}) has already been used by a previous share.", share_id)
         }
 
+        InconsistentSecretLengths(id: u8, slen_: usize, ids: Vec<u8>, slen: usize) {
+            description("The shares are incompatible with each other because they do not all have the same secret length.")
+            display("The share identifier {} had secret length {}, while the secret length {} was found for share identifier(s): {:?}.", id, slen_, slen, ids)
+        }
+
         InconsistentShares {
             description("The shares are inconsistent")
             display("The shares are inconsistent")
         }
+
+        InconsistentThresholds(id: u8, k_: u8, ids: Vec<u8>, k: u8) {
+            description("The shares are incompatible with each other because they do not all have the same threshold.")
+            display("The share identifier {} had k = {}, while k = {} was found for share identifier(s): {:?}.", id, k_, k, ids)
+        }
+
     }
 
     foreign_links {

src/share/validation.rs

@@ -1,5 +1,3 @@
-use std::collections::{HashMap, HashSet};
-
 use errors::*;
 use share::{IsShare, IsSignedShare};
 
@@ -32,11 +30,11 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
     let shares_count = shares.len();
 
     let mut ids = Vec::with_capacity(shares_count);
-    let mut k_compatibility_sets = HashMap::new();
+    let mut threshold = 0;
-    let mut slen_compatibility_sets = HashMap::new();
+    let mut slen = 0;
 
     for share in shares {
-        let (id, threshold, slen) = (
+        let (id, threshold_, slen_) = (
             share.get_id(),
             share.get_threshold(),
             share.get_data().len(),
@@ -44,75 +42,40 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
 
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
-        } else if threshold < 2 {
+        } else if threshold_ < 2 {
             bail!(ErrorKind::ShareParsingInvalidShareThreshold(threshold, id))
-        } else if slen < 1 {
+        } else if slen_ < 1 {
             bail!(ErrorKind::ShareParsingErrorEmptyShare(id))
         }
 
-        k_compatibility_sets
-            .entry(threshold)
-            .or_insert_with(HashSet::new);
-        let k_set = k_compatibility_sets.get_mut(&threshold).unwrap();
-        k_set.insert(id);
-
         if ids.iter().any(|&x| x == id) {
             bail!(ErrorKind::DuplicateShareId(id));
         }
 
-        slen_compatibility_sets
+        if threshold == 0 {
-            .entry(slen)
+            threshold = threshold_;
-            .or_insert_with(HashSet::new);
+        } else if threshold_ != threshold {
-        let slen_set = slen_compatibility_sets.get_mut(&slen).unwrap();
+            bail!(ErrorKind::InconsistentThresholds(
-        slen_set.insert(id);
+                id,
+                threshold_,
+                ids,
+                threshold
+            ))
+        }
+
+        if slen == 0 {
+            slen = slen_;
+        } else if slen_ != slen {
+            bail!(ErrorKind::InconsistentSecretLengths(id, slen_, ids, slen))
+        }
 
         ids.push(id);
     }
 
-    // Validate threshold
-    let k_sets = k_compatibility_sets.keys().count();
-
-    match k_sets {
-        1 => {} // All shares have the same roothash.
-        _ => {
-            bail! {
-                ErrorKind::IncompatibleThresholds(
-                    k_compatibility_sets
-                        .values()
-                        .map(|x| x.to_owned())
-                        .collect(),
-                )
-            }
-        }
-    }
-
-    // It is safe to unwrap because k_sets == 1
-    let threshold = k_compatibility_sets.keys().last().unwrap().to_owned();
-
     if shares_count < threshold as usize {
-        bail!(ErrorKind::MissingShares(shares_count, threshold));
+        bail!(ErrorKind::MissingShares(shares_count, threshold))
     }
 
-    // Validate share length consistency
-    let slen_sets = slen_compatibility_sets.keys().count();
-
-    match slen_sets {
-        1 => {} // All shares have the same `data` field len
-        _ => {
-            bail! {
-                ErrorKind::IncompatibleDataLengths(
-                    slen_compatibility_sets
-                        .values()
-                        .map(|x| x.to_owned())
-                        .collect(),
-                )
-            }
-        }
-    }
-
-    // It is safe to unwrap because slen_sets == 1
-    let slen = slen_compatibility_sets.keys().last().unwrap().to_owned();
-
     Ok((threshold, slen))
 }
 

tests/recovery_errors.rs

@@ -67,8 +67,8 @@ fn test_recover_duplicate_shares_number() {
 }
 
 #[test]
-#[should_panic(expected = "IncompatibleDataLengths")]
+#[should_panic(expected = "InconsistentSecretLengths")]
-fn test_recover_incompatible_data_lengths() {
+fn test_recover_inconsistent_secret_lengths() {
     let share1 = "2-1-CgnlCxRNtnkzENE".to_string();
     let share2 = "2-2-ChbG46L1zRszs0PPn63XnnupmZTcgYJ3".to_string();
 
@@ -77,6 +77,17 @@ fn test_recover_incompatible_data_lengths() {
     recover_secret(&shares, false).unwrap();
 }
 
+#[test]
+#[should_panic(expected = "InconsistentThresholds")]
+fn test_inconsistent_thresholds() {
+    let share1 = "2-1-CgnlCxRNtnkzENE".to_string();
+    let share2 = "3-2-CgkAnUgP3lfwjyM".to_string();
+
+    let shares = vec![share1, share2];
+
+    recover_secret(&shares, false).unwrap();
+}
+
 #[test]
 #[should_panic(expected = "MissingShares")]
 fn test_recover_too_few_shares() {