From 88743caad8dd1821908a23dae70353a8c546783d Mon Sep 17 00:00:00 2001 From: Noah Vesely Date: Thu, 29 Mar 2018 00:39:24 -0600 Subject: [PATCH] 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 | 21 ++++++----- src/share/validation.rs | 81 +++++++++++----------------------------- tests/recovery_errors.rs | 15 +++++++- 3 files changed, 46 insertions(+), 71 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 40fe950..b05605e 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -59,16 +59,6 @@ error_chain! { display("The shares are incompatible with each other.") } - IncompatibleThresholds(sets: Vec>) { - 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>) { - 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, 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, 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 { diff --git a/src/share/validation.rs b/src/share/validation.rs index 0395da0..9b4e158 100644 --- a/src/share/validation.rs +++ b/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(shares: &Vec) -> 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 slen_compatibility_sets = HashMap::new(); + let mut threshold = 0; + 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(shares: &Vec) -> 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 - .entry(slen) - .or_insert_with(HashSet::new); - let slen_set = slen_compatibility_sets.get_mut(&slen).unwrap(); - slen_set.insert(id); + if threshold == 0 { + threshold = threshold_; + } else if threshold_ != threshold { + bail!(ErrorKind::InconsistentThresholds( + 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)) } diff --git a/tests/recovery_errors.rs b/tests/recovery_errors.rs index c8f13e1..fa862f3 100644 --- a/tests/recovery_errors.rs +++ b/tests/recovery_errors.rs @@ -67,8 +67,8 @@ fn test_recover_duplicate_shares_number() { } #[test] -#[should_panic(expected = "IncompatibleDataLengths")] -fn test_recover_incompatible_data_lengths() { +#[should_panic(expected = "InconsistentSecretLengths")] +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() {