From 603882ff01b5bdf0cac37f79373979f86969ee92 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 15 Mar 2018 16:15:04 -0600
Subject: [PATCH 01/42] Small Rustfmt formatting fix to build.rs

---
 build.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/build.rs b/build.rs
index f1488f4e..b47873e2 100644
--- a/build.rs
+++ b/build.rs
@@ -78,8 +78,8 @@ fn main() {
     write!(
         f,
         "pub struct Tables {{ \
-             pub exp: [u8; 256], \
-             pub log: [u8; 256] \
+         pub exp: [u8; 256], \
+         pub log: [u8; 256] \
          }} \
          \
          pub static TABLES: Tables = "

From 5263953050c54581e27477fd868eee9a0d3127aa Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 15 Mar 2018 21:21:53 -0600
Subject: [PATCH 02/42] Initial barycentric Langrange interpolation

Implements barycentric Lagrange interpolation. Uses algorithm (3.1) from the
paper "Polynomial Interpolation: Langrange vs Newton" by Wilhelm Werner to find
the barycentric weights, and then evaluates at `Gf256::zero()` using the second
or "true" form of the barycentric interpolation formula.

I also earlier implemented a variant of this algorithm, Algorithm 2, from "A new
efficient algorithm for polynomial interpolation," which uses less total
operations than Werner's version, however, because it uses a lot more
multiplications or divisions (depending on how you choose to write it), it runs
slower given the running time of subtraction/ addition (equal) vs
multiplication, and especially division in the Gf256 module.

The new algorithm takes n^2 / 2 divisions and n^2 subtractions to calculate the
barycentric weights, and another n divisions, n multiplications, and 2n
additions to evaluate the polynomial*. The old algorithm runs in n^2 - n
divisions, n^2 multiplications, and n^2 subtractions. Without knowing the exact
running time of each of these operations, we can't say for sure, but I think a
good guess would be the new algorithm trends toward about 1/3 running time as n
-> infinity. It's also easy to see theoretically that for small n the original
lagrange algorithm is faster. This is backed up by benchmarks, which showed for
n >= 5, the new algorithm is faster. We can see that this is more or less what
we should expect given the running times in n of these algorithms.

To ensure we always run the faster algorithm, I've kept both versions and only
use the new one when 5 or more points are given.

Previously the tests in the lagrange module were allowed to pass nodes to the
interpolation algorithms with x = 0. Genuine shares will not be evaluated at x =
0, since then they would just be the secret, so:

1. Now nodes in tests start at x = 1 like `scheme::secret_share` deals them out.
2. I have added assert statements to reinforce this fact and guard against
   division by 0 panics.

This meant getting rid of the `evaluate_at_works` test, but
`interpolate_evaluate_at_0_eq_evaluate_at` provides a similar test.

Further work will include the use of barycentric weights in the `interpolate`
function.

A couple more interesting things to note about barycentric weights:

* Barycentric weights can be partially computed if less than threshold
  shares are present. When additional shares come in, computation can resume
  with no penalty to the total runtime.
* They can be determined totally independently from the y values of our points,
  and the x value we want to evaluate for. We only need to know the x values of
  our interpolation points.
---
 src/lagrange.rs   | 78 ++++++++++++++++++++++++++++++++++++-----------
 src/sss/scheme.rs |  2 +-
 2 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index 50baf8b5..2161e013 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,12 +1,28 @@
 use gf256::Gf256;
 use poly::Poly;
 
+// Minimum number of points where it becomes faster to use barycentric
+// Lagrange interpolation. Determined by benchmarking.
+const BARYCENTRIC_THRESHOLD: u8 = 5;
+
 /// Evaluates an interpolated polynomial at `Gf256::zero()` where
-/// the polynomial is determined using Lagrangian interpolation
-/// based on the given `points` in the G(2^8) Galois field.
-pub(crate) fn interpolate_at(points: &[(u8, u8)]) -> u8 {
+/// the polynomial is determined using either standard or barycentric
+/// Lagrange interpolation (depending on number of points, `k`) based
+/// on the given `points` in the G(2^8) Galois field.
+pub(crate) fn interpolate_at(k: u8, points: &[(u8, u8)]) -> u8 {
+    if k < BARYCENTRIC_THRESHOLD {
+        lagrange_interpolate_at(points)
+    } else {
+        barycentric_interpolate_at(k as usize, points)
+    }
+}
+
+/// Evaluates the polynomial at `Gf256::zero()` using standard Langrange
+/// interpolation.
+fn lagrange_interpolate_at(points: &[(u8, u8)]) -> u8 {
     let mut sum = Gf256::zero();
     for (i, &(raw_xi, raw_yi)) in points.iter().enumerate() {
+        assert_ne!(raw_xi, 0, "Invalid share x = 0");
         let xi = Gf256::from_byte(raw_xi);
         let yi = Gf256::from_byte(raw_yi);
         let mut prod = Gf256::one();
@@ -23,6 +39,36 @@ pub(crate) fn interpolate_at(points: &[(u8, u8)]) -> u8 {
     sum.to_byte()
 }
 
+/// Barycentric Lagrange interpolation algorithm from "Polynomial
+/// Interpolation: Langrange vs Newton" by Wilhelm Werner. Evaluates
+/// the polynomial at `Gf256::zero()`.
+fn barycentric_interpolate_at(k: usize, points: &[(u8, u8)]) -> u8 {
+    // Compute the barycentric weights `w`.
+    let mut w = vec![Gf256::zero(); k];
+    w[0] = Gf256::one();
+    let mut x = Vec::with_capacity(k);
+    x.push(Gf256::from_byte(points[0].0));
+    for i in 1..k {
+        x.push(Gf256::from_byte(points[i].0));
+        for j in 0..i {
+            let delta = x[j] - x[i];
+            assert_ne!(delta.poly, 0, "Duplicate shares");
+            w[j] /= delta;
+            w[i] -= w[j];
+        }
+    }
+    // Evaluate the second or "true" form of the barycentric
+    // interpolation formula at `Gf256::zero()`.
+    let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
+    for i in 0..k {
+        assert_ne!(x[i].poly, 0, "Invalid share x = 0");
+        let diff = w[i] / x[i];
+        num += diff * Gf256::from_byte(points[i].1);
+        denom += diff;
+    }
+    (num / denom).to_byte()
+}
+
 /// Computeds the coefficient of the Lagrange polynomial interpolated
 /// from the given `points`, in the G(2^8) Galois field.
 pub(crate) fn interpolate(points: &[(Gf256, Gf256)]) -> Poly {
@@ -31,6 +77,7 @@ pub(crate) fn interpolate(points: &[(Gf256, Gf256)]) -> Poly {
     let mut poly = vec![Gf256::zero(); len];
 
     for &(x, y) in points {
+        assert_ne!(x.poly, 0, "Invalid share x = 0");
         let mut coeffs = vec![Gf256::zero(); len];
         coeffs[0] = y;
 
@@ -71,24 +118,15 @@ mod tests {
 
     quickcheck! {
 
-        fn evaluate_at_works(ys: Vec<u8>) -> TestResult {
-            if ys.is_empty() || ys.len() > std::u8::MAX as usize {
-                return TestResult::discard();
-            }
-
-            let points = ys.iter().enumerate().map(|(x, y)| (x as u8, *y)).collect::<Vec<_>>();
-            let equals = interpolate_at(points.as_slice()) == ys[0];
-
-            TestResult::from_bool(equals)
-        }
-
-
         fn interpolate_evaluate_at_works(ys: Vec<Gf256>) -> TestResult {
             if ys.is_empty() || ys.len() > std::u8::MAX as usize {
                 return TestResult::discard();
             }
 
-            let points = ys.into_iter().enumerate().map(|(x, y)| (gf256!(x as u8), y)).collect::<Vec<_>>();
+            let points = ys.into_iter()
+                           .zip(1..std::u8::MAX)
+                           .map(|(y, x)| (gf256!(x), y))
+                           .collect::<Vec<_>>();
             let poly = interpolate(&points);
 
             for (x, y) in points {
@@ -105,7 +143,10 @@ mod tests {
                 return TestResult::discard();
             }
 
-            let points = ys.into_iter().enumerate().map(|(x, y)| (x as u8, y)).collect::<Vec<_>>();
+            let points = ys.into_iter()
+                           .zip(1..std::u8::MAX)
+                           .map(|(y, x)| (x, y))
+                           .collect::<Vec<_>>();
 
             let elems = points
                 .iter()
@@ -114,7 +155,8 @@ mod tests {
 
             let poly = interpolate(&elems);
 
-            let equals = poly.evaluate_at(Gf256::zero()).to_byte() == interpolate_at(points.as_slice());
+            let equals = poly.evaluate_at(Gf256::zero()).to_byte()
+                == interpolate_at(points.len() as u8, points.as_slice());
 
             TestResult::from_bool(equals)
         }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index acb3723d..8e288748 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -101,7 +101,7 @@ impl SSS {
             for s in shares.iter().take(threshold as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            secret.push(interpolate_at(&*col_in));
+            secret.push(interpolate_at(threshold, &*col_in));
         }
 
         Ok(secret)

From be20e7749f2d7850a5db3e2ae7bbe3b9bba8a4d8 Mon Sep 17 00:00:00 2001
From: Romain Ruetschi <romain.ruetschi@gmail.com>
Date: Mon, 19 Mar 2018 21:39:30 +0100
Subject: [PATCH 03/42] Use barycentric Lagrange interpolation in all cases.

While this is a slight regression in performance in the case
where k < 5, in absolute terms it is small enough to be neglible.
---
 src/lagrange.rs | 43 +++++++++----------------------------------
 1 file changed, 9 insertions(+), 34 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index 2161e013..ebf84e8b 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,53 +1,26 @@
 use gf256::Gf256;
 use poly::Poly;
 
-// Minimum number of points where it becomes faster to use barycentric
-// Lagrange interpolation. Determined by benchmarking.
-const BARYCENTRIC_THRESHOLD: u8 = 5;
-
 /// Evaluates an interpolated polynomial at `Gf256::zero()` where
-/// the polynomial is determined using either standard or barycentric
-/// Lagrange interpolation (depending on number of points, `k`) based
-/// on the given `points` in the G(2^8) Galois field.
+/// the polynomial is determined using barycentric Lagrange
+/// interpolation based on the given `points` in
+/// the G(2^8) Galois field.
 pub(crate) fn interpolate_at(k: u8, points: &[(u8, u8)]) -> u8 {
-    if k < BARYCENTRIC_THRESHOLD {
-        lagrange_interpolate_at(points)
-    } else {
-        barycentric_interpolate_at(k as usize, points)
-    }
-}
-
-/// Evaluates the polynomial at `Gf256::zero()` using standard Langrange
-/// interpolation.
-fn lagrange_interpolate_at(points: &[(u8, u8)]) -> u8 {
-    let mut sum = Gf256::zero();
-    for (i, &(raw_xi, raw_yi)) in points.iter().enumerate() {
-        assert_ne!(raw_xi, 0, "Invalid share x = 0");
-        let xi = Gf256::from_byte(raw_xi);
-        let yi = Gf256::from_byte(raw_yi);
-        let mut prod = Gf256::one();
-        for (j, &(raw_xj, _)) in points.iter().enumerate() {
-            if i != j {
-                let xj = Gf256::from_byte(raw_xj);
-                let delta = xi - xj;
-                assert_ne!(delta.poly, 0, "Duplicate shares");
-                prod *= xj / delta;
-            }
-        }
-        sum += prod * yi;
-    }
-    sum.to_byte()
+    barycentric_interpolate_at(k as usize, points)
 }
 
 /// Barycentric Lagrange interpolation algorithm from "Polynomial
 /// Interpolation: Langrange vs Newton" by Wilhelm Werner. Evaluates
 /// the polynomial at `Gf256::zero()`.
+#[inline]
 fn barycentric_interpolate_at(k: usize, points: &[(u8, u8)]) -> u8 {
     // Compute the barycentric weights `w`.
     let mut w = vec![Gf256::zero(); k];
     w[0] = Gf256::one();
+
     let mut x = Vec::with_capacity(k);
     x.push(Gf256::from_byte(points[0].0));
+
     for i in 1..k {
         x.push(Gf256::from_byte(points[i].0));
         for j in 0..i {
@@ -57,6 +30,7 @@ fn barycentric_interpolate_at(k: usize, points: &[(u8, u8)]) -> u8 {
             w[i] -= w[j];
         }
     }
+
     // Evaluate the second or "true" form of the barycentric
     // interpolation formula at `Gf256::zero()`.
     let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
@@ -66,6 +40,7 @@ fn barycentric_interpolate_at(k: usize, points: &[(u8, u8)]) -> u8 {
         num += diff * Gf256::from_byte(points[i].1);
         denom += diff;
     }
+
     (num / denom).to_byte()
 }
 

From e39bdaf0d18c3f3c87044bf50de3ca43f775f0f2 Mon Sep 17 00:00:00 2001
From: Romain Ruetschi <romain.ruetschi@gmail.com>
Date: Mon, 19 Mar 2018 21:40:55 +0100
Subject: [PATCH 04/42] Ensure there is at least one point in QuickCheck tests

---
 src/lagrange.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index ebf84e8b..4118818c 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -114,7 +114,7 @@ mod tests {
         }
 
         fn interpolate_evaluate_at_0_eq_evaluate_at(ys: Vec<u8>) -> TestResult {
-            if ys.len() > std::u8::MAX as usize {
+            if ys.is_empty() || ys.len() > std::u8::MAX as usize {
                 return TestResult::discard();
             }
 

From a2dafcf72548817c4e8f607536212ae17c48626f Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 15 Mar 2018 15:47:22 -0600
Subject: [PATCH 05/42] Use Horner's method for evaluating polynomials

Horner's method is an algorithm for calculating polynomials, which consists of
transforming the monomial form into a computationally efficient form. It is
pretty easy to understand:
https://en.wikipedia.org/wiki/Horner%27s_method#Description_of_the_algorithm

This implementation has resulted in a noticeable secret share generation speedup
as the RustySecrets benchmarks show, especially when calculating larger
polynomials:

Before:
test sss::generate_1kb_10_25 ... bench: 3,104,391 ns/iter (+/- 113,824)
test sss::generate_1kb_3_5 ... bench: 951,807 ns/iter (+/- 41,067)

After:
test sss::generate_1kb_10_25        ... bench:   2,071,655 ns/iter (+/- 46,445)
test sss::generate_1kb_3_5          ... bench:     869,875 ns/iter (+/- 40,246)
---
 src/sss/encode.rs | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/sss/encode.rs b/src/sss/encode.rs
index d2729fb4..dfb83029 100644
--- a/src/sss/encode.rs
+++ b/src/sss/encode.rs
@@ -6,13 +6,10 @@ use std::io::prelude::*;
 pub(crate) fn encode_secret_byte<W: Write>(src: &[u8], n: u8, w: &mut W) -> io::Result<()> {
     for raw_x in 1..(u16::from(n) + 1) {
         let x = Gf256::from_byte(raw_x as u8);
-        let mut fac = Gf256::one();
-        let mut acc = Gf256::zero();
-        for &coeff in src.iter() {
-            acc += fac * Gf256::from_byte(coeff);
-            fac *= x;
-        }
-        w.write_all(&[acc.to_byte()])?;
+        let sum = src.iter().rev().fold(Gf256::zero(), |acc, &coeff| {
+            Gf256::from_byte(coeff) + acc * x
+        });
+        w.write_all(&[sum.to_byte()])?;
     }
     Ok(())
 }

From 0b42b6b9cc6ceba183d03a2fa8258fcbf62a15b2 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 15 Mar 2018 15:57:02 -0600
Subject: [PATCH 06/42] Note algorithm in encode_secret_byte docstring

---
 src/sss/encode.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/sss/encode.rs b/src/sss/encode.rs
index dfb83029..abfe5494 100644
--- a/src/sss/encode.rs
+++ b/src/sss/encode.rs
@@ -2,7 +2,8 @@ use gf256::Gf256;
 use std::io;
 use std::io::prelude::*;
 
-/// evaluates a polynomial at x=1, 2, 3, ... n (inclusive)
+/// Evaluates a polynomial at x=1, 2, 3, ... n (inclusive) using
+/// Horner's method.
 pub(crate) fn encode_secret_byte<W: Write>(src: &[u8], n: u8, w: &mut W) -> io::Result<()> {
     for raw_x in 1..(u16::from(n) + 1) {
         let x = Gf256::from_byte(raw_x as u8);

From 5a9bfb913ad73898a9cd1fa62a72f540f8361325 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Fri, 16 Mar 2018 15:32:58 -0600
Subject: [PATCH 07/42] Update rand to ^0.4.2

RustySecrets makes minimal use of the rand library. It only initializes
the `ChaChaRng` with a seed, and `OsRng` in the standard way, and then calls
their `fill_bytes` methods, provided by the same Trait, and whose function
signature has not changed.  I have confirmed by looking at the code changes,
that there have been no changes to the relevant interfaces this library uses.
---
 Cargo.toml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 9baeef13..4da5b1af 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,7 +23,7 @@ dss = []
 
 [dependencies]
 base64 = "0.9.0"
-rand = "^0.3"
+rand = "^0.4.2"
 ring = "^0.12"
 merkle_sigs = "^1.4"
 protobuf = "^1.4"
@@ -63,4 +63,3 @@ tag-prefix = "v"
 tag-message = "Release version {{version}}."
 doc-commit-message = "Update documentation."
 dev-version-ext = "pre"
-

From b34a2094bc51a2453e1d2eeaabd6af52469ee846 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Wed, 21 Mar 2018 12:47:33 -0600
Subject: [PATCH 08/42] Add TODO note on unreleased Rng::try_fill_bytes

---
 src/sss/scheme.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 8e288748..d5021ba5 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -77,6 +77,8 @@ impl SSS {
         let mut osrng = OsRng::new()?;
         for (c, &s) in src.iter().enumerate() {
             col_in[0] = s;
+            // NOTE: switch to `try_fill_bytes` when it lands in a stable release:
+            // https://github.com/rust-lang-nursery/rand/commit/230b2258dbd99ff8bd991008c972d923d4b5d10c
             osrng.fill_bytes(&mut col_in[1..]);
             col_out.clear();
             encode_secret_byte(&*col_in, shares_count, &mut col_out)?;

From ecb22aa6435cde08c7f7105f4dbbb52957ad7877 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Mon, 26 Mar 2018 17:10:38 -0600
Subject: [PATCH 09/42] Remove `ShareIdentifierTooBig` error and validation

Since id is a `u8` it will never be greater than 255.
---
 src/errors.rs           | 5 -----
 src/share/validation.rs | 4 ----
 2 files changed, 9 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index c2b74961..69403576 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -59,11 +59,6 @@ error_chain! {
             display("The shares are incompatible with each other.")
         }
 
-        ShareIdentifierTooBig(id: u8, n: u8) {
-            description("Share identifier too big")
-            display("Found share identifier ({}) bigger than the maximum number of shares ({}).", id, n)
-        }
-
         MissingShares(provided: usize, required: usize) {
             description("The number of shares provided is insufficient to recover the secret.")
             display("{} shares are required to recover the secret, found only {}.", required, provided)
diff --git a/src/share/validation.rs b/src/share/validation.rs
index f8f94adb..139f3f77 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -37,10 +37,6 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
     for share in shares {
         let (id, threshold) = (share.get_id(), share.get_threshold());
 
-        if id > MAX_SHARES {
-            bail!(ErrorKind::ShareIdentifierTooBig(id, MAX_SHARES))
-        }
-
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
         }

From 2baa8ab3e7ff1983616a6641f976363c24144d92 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Mon, 26 Mar 2018 17:12:58 -0600
Subject: [PATCH 10/42] Remove `DuplicateShareData` error and validation

It's possible that two different points have the same data.

To give a concrete example consider the secret polynomial `x^2 + x + s`, where
`s` is the secret byte. Plugging in 214 and 215 (both elements of the cyclic
subgroup of order 2) for `x` will give the same result, `1 + s`.

More broadly, for any polynomial `b*x^t + b*x^(t-1) + ... + x + s`, where `t` is
the order of at least one subgroup of GF(256), for all subgroups of order `t`,
all elements of that subgroup, when chosen for `x`, will produce the same
result.

There are certainly other types of polynomials that have "share collisions."
This type was just easy to find because it exploits the nature of finite fields.
---
 src/errors.rs                 |  5 -----
 src/share/validation.rs       |  5 -----
 tests/recovery_errors.rs      | 11 -----------
 tests/ss1_recovery_errors.rs  | 26 --------------------------
 tests/thss_recovery_errors.rs | 23 -----------------------
 5 files changed, 70 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 69403576..315d47ba 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -118,11 +118,6 @@ error_chain! {
             display("This share number ({}) has already been used by a previous share.", share_id)
         }
 
-        DuplicateShareData(share_id: u8) {
-            description("The data encoded in this share is the same as the one found in a previous share")
-            display("The data encoded in share #{} is the same as the one found in a previous share.", share_id)
-        }
-
         InconsistentShares {
             description("The shares are inconsistent")
             display("The shares are inconsistent")
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 139f3f77..45e0e63b 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -55,11 +55,6 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
             bail!(ErrorKind::ShareParsingErrorEmptyShare(id))
         }
 
-        if result.iter().any(|s| s.get_data() == share.get_data()) && share.get_threshold() != 1 {
-            // When threshold = 1, shares data can be the same
-            bail!(ErrorKind::DuplicateShareData(id));
-        }
-
         result.push(share);
     }
 
diff --git a/tests/recovery_errors.rs b/tests/recovery_errors.rs
index 7a36dbe0..d986b699 100644
--- a/tests/recovery_errors.rs
+++ b/tests/recovery_errors.rs
@@ -66,17 +66,6 @@ fn test_recover_duplicate_shares_number() {
     recover_secret(&shares, false).unwrap();
 }
 
-#[test]
-#[should_panic(expected = "DuplicateShareData")]
-fn test_recover_duplicate_shares_data() {
-    let share1 = "2-1-CgnlCxRNtnkzENE".to_string();
-    let share2 = "2-2-CgnlCxRNtnkzENE".to_string();
-
-    let shares = vec![share1, share2];
-
-    recover_secret(&shares, false).unwrap();
-}
-
 #[test]
 #[should_panic(expected = "MissingShares")]
 fn test_recover_too_few_shares() {
diff --git a/tests/ss1_recovery_errors.rs b/tests/ss1_recovery_errors.rs
index 4ab71a2c..1ad4d11f 100644
--- a/tests/ss1_recovery_errors.rs
+++ b/tests/ss1_recovery_errors.rs
@@ -135,32 +135,6 @@ fn test_recover_duplicate_shares_number() {
     recover_secret(&shares).unwrap();
 }
 
-#[test]
-#[should_panic(expected = "DuplicateShareData")]
-fn test_recover_duplicate_shares_data() {
-    let hash = get_test_hash();
-    let share1 = Share {
-        id: 1,
-        threshold: TEST_THRESHOLD,
-        shares_count: TEST_SHARES_COUNT,
-        data: "1YAYwmOHqZ69jA".to_string().into_bytes(),
-        hash: hash.clone(),
-        metadata: None,
-    };
-    let share2 = Share {
-        id: 2,
-        threshold: TEST_THRESHOLD,
-        shares_count: TEST_SHARES_COUNT,
-        data: "1YAYwmOHqZ69jA".to_string().into_bytes(),
-        hash: hash.clone(),
-        metadata: None,
-    };
-
-    let shares = vec![share1, share2];
-
-    recover_secret(&shares).unwrap();
-}
-
 #[test]
 #[should_panic(expected = "MissingShares")]
 fn test_recover_too_few_shares() {
diff --git a/tests/thss_recovery_errors.rs b/tests/thss_recovery_errors.rs
index 4c6c58ae..173680a8 100644
--- a/tests/thss_recovery_errors.rs
+++ b/tests/thss_recovery_errors.rs
@@ -106,29 +106,6 @@ fn test_recover_duplicate_shares_number() {
     recover_secret(&shares).unwrap();
 }
 
-#[test]
-#[should_panic(expected = "DuplicateShareData")]
-fn test_recover_duplicate_shares_data() {
-    let share1 = Share {
-        id: 1,
-        threshold: 2,
-        shares_count: 2,
-        data: "1YAYwmOHqZ69jA".to_string().into_bytes(),
-        metadata: None,
-    };
-    let share2 = Share {
-        id: 2,
-        threshold: 2,
-        shares_count: 2,
-        data: "1YAYwmOHqZ69jA".to_string().into_bytes(),
-        metadata: None,
-    };
-
-    let shares = vec![share1, share2];
-
-    recover_secret(&shares).unwrap();
-}
-
 #[test]
 #[should_panic(expected = "MissingShares")]
 fn test_recover_too_few_shares() {

From 10209b674959ef1beba17852405dc5d724ca5ad9 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 27 Mar 2018 14:24:43 -0600
Subject: [PATCH 11/42] Add ErrorKind::ShareParsingInvalidShareThreshold

Ensures that threshold > 2 during the parsing process, since we ensure the same
during the splitting process.
---
 src/errors.rs            |  5 +++++
 src/share/validation.rs  |  2 ++
 tests/recovery_errors.rs | 11 +++++++++++
 3 files changed, 18 insertions(+)

diff --git a/src/errors.rs b/src/errors.rs
index 315d47ba..388d885b 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -92,6 +92,11 @@ error_chain! {
             display("Found invalid share identifier ({})", share_id)
         }
 
+        ShareParsingInvalidShareThreshold(k: u8, id: u8) {
+            description("Threshold k must be bigger than or equal to 2")
+            display("Threshold k must be bigger than or equal to 2. Got k = {} for share identifier {}.", k, id)
+        }
+
         InvalidSS1Parameters(r: usize, s: usize) {
             description("Invalid parameters for the SS1 sharing scheme")
             display("Invalid parameters for the SS1 sharing scheme: r = {}, s = {}.", r, s)
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 45e0e63b..4aa9069d 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -39,6 +39,8 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
 
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
+        } else if threshold < 2 {
+            bail!(ErrorKind::ShareParsingInvalidShareThreshold(threshold, id))
         }
 
         k_compatibility_sets
diff --git a/tests/recovery_errors.rs b/tests/recovery_errors.rs
index d986b699..1fd7295a 100644
--- a/tests/recovery_errors.rs
+++ b/tests/recovery_errors.rs
@@ -77,6 +77,17 @@ fn test_recover_too_few_shares() {
     recover_secret(&shares, false).unwrap();
 }
 
+#[test]
+#[should_panic(expected = "ShareParsingInvalidShareThreshold")]
+fn test_recover_invalid_share_threshold() {
+    let share1 = "1-1-CgnlCxRNtnkzENE".to_string();
+    let share2 = "1-1-CgkAnUgP3lfwjyM".to_string();
+
+    let shares = vec![share1, share2];
+
+    recover_secret(&shares, false).unwrap();
+}
+
 // See https://github.com/SpinResearch/RustySecrets/issues/43
 #[test]
 fn test_recover_too_few_shares_bug() {

From b48a74ab95e4ac4e9d296b1617942cd4d3ebdd44 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 27 Mar 2018 14:30:50 -0600
Subject: [PATCH 12/42] Simplify threshold consistency validation

Since the validation already confirms `shares` is not empty, `k_sets` will never
match 0.
---
 src/share/validation.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/share/validation.rs b/src/share/validation.rs
index 4aa9069d..fde5b224 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -64,7 +64,6 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
     let k_sets = k_compatibility_sets.keys().count();
 
     match k_sets {
-        0 => bail!(ErrorKind::EmptyShares),
         1 => {} // All shares have the same roothash.
         _ => {
             bail! {

From c7f2742de8ead0550599b89a4bcbb0ccc7334e31 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 27 Mar 2018 14:35:40 -0600
Subject: [PATCH 13/42] Fix arg order missing shares validation

The arguments were provided in the wrong order.
---
 src/share/validation.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/share/validation.rs b/src/share/validation.rs
index fde5b224..1c069faf 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -81,7 +81,7 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
     let threshold = k_compatibility_sets.keys().last().unwrap().to_owned();
 
     if shares_count < threshold as usize {
-        bail!(ErrorKind::MissingShares(threshold as usize, shares_count));
+        bail!(ErrorKind::MissingShares(shares_count, threshold as usize));
     }
 
     Ok((threshold, result))

From 6d04a587f8cb8915157fb2a75dcfdbf67649e550 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 27 Mar 2018 14:37:11 -0600
Subject: [PATCH 14/42] MissingShares should take `u8` for `required` arg

---
 src/errors.rs           | 2 +-
 src/share/validation.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 388d885b..b5d1beea 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -59,7 +59,7 @@ error_chain! {
             display("The shares are incompatible with each other.")
         }
 
-        MissingShares(provided: usize, required: usize) {
+        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)
         }
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 1c069faf..143f5dd1 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -81,7 +81,7 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
     let threshold = k_compatibility_sets.keys().last().unwrap().to_owned();
 
     if shares_count < threshold as usize {
-        bail!(ErrorKind::MissingShares(shares_count, threshold as usize));
+        bail!(ErrorKind::MissingShares(shares_count, threshold));
     }
 
     Ok((threshold, result))

From 8ab91bbf93451b1557ca629e86b4dea24459cee2 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 27 Mar 2018 14:47:02 -0600
Subject: [PATCH 15/42] More specific validation error when share thresholds
 mismatch

---
 src/errors.rs           | 5 +++++
 src/share/validation.rs | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/errors.rs b/src/errors.rs
index b5d1beea..9c300ff2 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -59,6 +59,11 @@ 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.")
+        }
+
         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)
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 143f5dd1..a94b4ddc 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -67,7 +67,7 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
         1 => {} // All shares have the same roothash.
         _ => {
             bail! {
-                ErrorKind::IncompatibleSets(
+                ErrorKind::IncompatibleThresholds(
                     k_compatibility_sets
                         .values()
                         .map(|x| x.to_owned())

From fefd5ab3694282470bf87d974bedc2d0b6c0b3d0 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 27 Mar 2018 15:22:21 -0600
Subject: [PATCH 16/42] Validate shares have the same data length

---
 src/errors.rs            |  5 +++++
 src/share/validation.rs  | 31 +++++++++++++++++++++++++++----
 tests/recovery_errors.rs | 11 +++++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 9c300ff2..40fe9508 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -64,6 +64,11 @@ error_chain! {
             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)
diff --git a/src/share/validation.rs b/src/share/validation.rs
index a94b4ddc..93d00782 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -33,14 +33,17 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
     let mut result: Vec<S> = Vec::with_capacity(shares_count);
 
     let mut k_compatibility_sets = HashMap::new();
+    let mut data_len_compatibility_sets = HashMap::new();
 
     for share in shares {
-        let (id, threshold) = (share.get_id(), share.get_threshold());
+        let (id, threshold, data_len) = (share.get_id(), share.get_threshold(), share.get_data().len());
 
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
         } else if threshold < 2 {
             bail!(ErrorKind::ShareParsingInvalidShareThreshold(threshold, id))
+        } else if data_len < 1 {
+            bail!(ErrorKind::ShareParsingErrorEmptyShare(id))
         }
 
         k_compatibility_sets
@@ -53,9 +56,12 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
             bail!(ErrorKind::DuplicateShareId(id));
         }
 
-        if share.get_data().is_empty() {
-            bail!(ErrorKind::ShareParsingErrorEmptyShare(id))
-        }
+        data_len_compatibility_sets
+            .entry(data_len)
+            .or_insert_with(HashSet::new);
+        let data_len_set = data_len_compatibility_sets.get_mut(&data_len).unwrap();
+        data_len_set.insert(id);
+
 
         result.push(share);
     }
@@ -84,6 +90,23 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
         bail!(ErrorKind::MissingShares(shares_count, threshold));
     }
 
+    // Validate share length consistency
+    let data_len_sets = data_len_compatibility_sets.keys().count();
+
+    match data_len_sets {
+        1 => {} // All shares have the same `data` field len
+        _ => {
+            bail! {
+                ErrorKind::IncompatibleDataLengths(
+                    data_len_compatibility_sets
+                        .values()
+                        .map(|x| x.to_owned())
+                        .collect(),
+                )
+            }
+        }
+    }
+
     Ok((threshold, result))
 }
 
diff --git a/tests/recovery_errors.rs b/tests/recovery_errors.rs
index 1fd7295a..c8f13e1d 100644
--- a/tests/recovery_errors.rs
+++ b/tests/recovery_errors.rs
@@ -66,6 +66,17 @@ fn test_recover_duplicate_shares_number() {
     recover_secret(&shares, false).unwrap();
 }
 
+#[test]
+#[should_panic(expected = "IncompatibleDataLengths")]
+fn test_recover_incompatible_data_lengths() {
+    let share1 = "2-1-CgnlCxRNtnkzENE".to_string();
+    let share2 = "2-2-ChbG46L1zRszs0PPn63XnnupmZTcgYJ3".to_string();
+
+    let shares = vec![share1, share2];
+
+    recover_secret(&shares, false).unwrap();
+}
+
 #[test]
 #[should_panic(expected = "MissingShares")]
 fn test_recover_too_few_shares() {

From 51f77faf47dfed3daa9541222d1b2c0e4d81ba85 Mon Sep 17 00:00:00 2001
From: Romain Ruetschi <romain.ruetschi@gmail.com>
Date: Wed, 28 Mar 2018 14:49:47 +0200
Subject: [PATCH 17/42] Disable `dss` benchmarks until we expose the module.

Closes #49
---
 benches/ss1.rs  | 1 +
 benches/thss.rs | 1 +
 2 files changed, 2 insertions(+)

diff --git a/benches/ss1.rs b/benches/ss1.rs
index 601e92f6..d32b72b1 100644
--- a/benches/ss1.rs
+++ b/benches/ss1.rs
@@ -1,5 +1,6 @@
 #![cfg(test)]
 #![feature(test)]
+#![cfg(feature = "dss")]
 
 extern crate rusty_secrets;
 extern crate test;
diff --git a/benches/thss.rs b/benches/thss.rs
index 95dc79c2..b11386b1 100644
--- a/benches/thss.rs
+++ b/benches/thss.rs
@@ -1,5 +1,6 @@
 #![cfg(test)]
 #![feature(test)]
+#![cfg(feature = "dss")]
 
 extern crate rusty_secrets;
 extern crate test;

From 5ee3cf2d0078d21aae37064335c637d9760b4708 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Wed, 28 Mar 2018 18:51:10 -0600
Subject: [PATCH 18/42] Change signatures of share validation fns

* Pass a ref to `Vec<Shares>` instead of recreating and moving the object
  through several functions.
* Return `slen`/ `data_len`, since we'll be using it anyway in `recover_secrets`
---
 src/dss/ss1/scheme.rs   |  3 ++-
 src/dss/thss/scheme.rs  |  5 ++---
 src/share/validation.rs | 22 ++++++++++++----------
 src/sss/scheme.rs       |  3 +--
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/src/dss/ss1/scheme.rs b/src/dss/ss1/scheme.rs
index 0801a286..9672e5ee 100644
--- a/src/dss/ss1/scheme.rs
+++ b/src/dss/ss1/scheme.rs
@@ -247,7 +247,8 @@ impl SS1 {
         &self,
         shares: &[Share],
     ) -> Result<(Vec<u8>, AccessStructure, Option<MetaData>)> {
-        let (_, shares) = validate_shares(shares.to_vec())?;
+        let shares = shares.to_vec();
+        validate_shares(&shares)?;
 
         let underlying_shares = shares
             .iter()
diff --git a/src/dss/thss/scheme.rs b/src/dss/thss/scheme.rs
index c11cfd70..141c932f 100644
--- a/src/dss/thss/scheme.rs
+++ b/src/dss/thss/scheme.rs
@@ -89,9 +89,8 @@ impl ThSS {
         &self,
         shares: &[Share],
     ) -> Result<(Vec<u8>, AccessStructure, Option<MetaData>)> {
-        let (threshold, shares) = validate_shares(shares.to_vec())?;
-
-        let cypher_len = shares[0].data.len();
+        let shares = shares.to_vec();
+        let (threshold, cypher_len) = validate_shares(&shares)?;
 
         let polys = (0..cypher_len)
             .map(|i| {
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 93d00782..8a256b1d 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -11,27 +11,27 @@ use share::{IsShare, IsSignedShare};
 
 /// TODO: Doc
 pub(crate) fn validate_signed_shares<S: IsSignedShare>(
-    shares: Vec<S>,
+    shares: &Vec<S>,
     verify_signatures: bool,
-) -> Result<(u8, Vec<S>)> {
-    let (threshold, shares) = validate_shares(shares)?;
+) -> Result<(u8, usize)> {
+    let result = validate_shares(shares)?;
 
     if verify_signatures {
         S::verify_signatures(&shares)?;
     }
 
-    Ok((threshold, shares))
+    Ok(result)
 }
 
 /// TODO: Doc
-pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)> {
+pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)> {
     if shares.is_empty() {
         bail!(ErrorKind::EmptyShares);
     }
 
     let shares_count = shares.len();
-    let mut result: Vec<S> = Vec::with_capacity(shares_count);
 
+    let mut ids = Vec::with_capacity(shares_count);
     let mut k_compatibility_sets = HashMap::new();
     let mut data_len_compatibility_sets = HashMap::new();
 
@@ -52,7 +52,7 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
         let k_set = k_compatibility_sets.get_mut(&threshold).unwrap();
         k_set.insert(id);
 
-        if result.iter().any(|s| s.get_id() == id) {
+        if ids.iter().any(|&x| x == id) {
             bail!(ErrorKind::DuplicateShareId(id));
         }
 
@@ -62,8 +62,7 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
         let data_len_set = data_len_compatibility_sets.get_mut(&data_len).unwrap();
         data_len_set.insert(id);
 
-
-        result.push(share);
+        ids.push(id);
     }
 
     // Validate threshold
@@ -107,7 +106,10 @@ pub(crate) fn validate_shares<S: IsShare>(shares: Vec<S>) -> Result<(u8, Vec<S>)
         }
     }
 
-    Ok((threshold, result))
+    // It is safe to unwrap because data_len_sets == 1
+    let slen = data_len_compatibility_sets.keys().last().unwrap().to_owned();
+
+    Ok((threshold, data_len))
 }
 
 pub(crate) fn validate_share_count(threshold: u8, shares_count: u8) -> Result<(u8, u8)> {
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index d5021ba5..4072c913 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -93,9 +93,8 @@ impl SSS {
     ///
     /// At least `k` distinct shares need to be provided to recover the share.
     pub fn recover_secret(shares: Vec<Share>, verify_signatures: bool) -> Result<Vec<u8>> {
-        let (threshold, shares) = validate_signed_shares(shares, verify_signatures)?;
+        let (threshold, slen) = validate_signed_shares(&shares, verify_signatures)?;
 
-        let slen = shares[0].data.len();
         let mut col_in = Vec::with_capacity(threshold as usize);
         let mut secret = Vec::with_capacity(slen);
         for byteindex in 0..slen {

From 2df49c5254eb95f4abba3bd2639eba5f9ae9d29e Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Wed, 28 Mar 2018 18:55:08 -0600
Subject: [PATCH 19/42] Standardize validation var identifier on

---
 src/share/validation.rs | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/share/validation.rs b/src/share/validation.rs
index 8a256b1d..0395da06 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -33,16 +33,20 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
 
     let mut ids = Vec::with_capacity(shares_count);
     let mut k_compatibility_sets = HashMap::new();
-    let mut data_len_compatibility_sets = HashMap::new();
+    let mut slen_compatibility_sets = HashMap::new();
 
     for share in shares {
-        let (id, threshold, data_len) = (share.get_id(), share.get_threshold(), share.get_data().len());
+        let (id, threshold, slen) = (
+            share.get_id(),
+            share.get_threshold(),
+            share.get_data().len(),
+        );
 
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
         } else if threshold < 2 {
             bail!(ErrorKind::ShareParsingInvalidShareThreshold(threshold, id))
-        } else if data_len < 1 {
+        } else if slen < 1 {
             bail!(ErrorKind::ShareParsingErrorEmptyShare(id))
         }
 
@@ -56,11 +60,11 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
             bail!(ErrorKind::DuplicateShareId(id));
         }
 
-        data_len_compatibility_sets
-            .entry(data_len)
+        slen_compatibility_sets
+            .entry(slen)
             .or_insert_with(HashSet::new);
-        let data_len_set = data_len_compatibility_sets.get_mut(&data_len).unwrap();
-        data_len_set.insert(id);
+        let slen_set = slen_compatibility_sets.get_mut(&slen).unwrap();
+        slen_set.insert(id);
 
         ids.push(id);
     }
@@ -90,14 +94,14 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
     }
 
     // Validate share length consistency
-    let data_len_sets = data_len_compatibility_sets.keys().count();
+    let slen_sets = slen_compatibility_sets.keys().count();
 
-    match data_len_sets {
+    match slen_sets {
         1 => {} // All shares have the same `data` field len
         _ => {
             bail! {
                 ErrorKind::IncompatibleDataLengths(
-                    data_len_compatibility_sets
+                    slen_compatibility_sets
                         .values()
                         .map(|x| x.to_owned())
                         .collect(),
@@ -106,10 +110,10 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
         }
     }
 
-    // It is safe to unwrap because data_len_sets == 1
-    let slen = data_len_compatibility_sets.keys().last().unwrap().to_owned();
+    // It is safe to unwrap because slen_sets == 1
+    let slen = slen_compatibility_sets.keys().last().unwrap().to_owned();
 
-    Ok((threshold, data_len))
+    Ok((threshold, slen))
 }
 
 pub(crate) fn validate_share_count(threshold: u8, shares_count: u8) -> Result<(u8, u8)> {

From 88134181e1a603d66d2b730cb11b6b37380ca753 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 29 Mar 2018 00:39:24 -0600
Subject: [PATCH 20/42] 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 40fe9508..b05605e7 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<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 {
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 0395da06..9b4e1587 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<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 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<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
-            .entry(slen)
-            .or_insert_with(HashSet::new);
-        let slen_set = slen_compatibility_sets.get_mut(&slen).unwrap();
-        slen_set.insert(id);
-
-        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(),
-                )
-            }
+        if threshold == 0 {
+            threshold = threshold_;
+        } else if threshold_ != threshold {
+            bail!(ErrorKind::InconsistentThresholds(
+                id,
+                threshold_,
+                ids,
+                threshold
+            ))
         }
-    }
 
-    // It is safe to unwrap because k_sets == 1
-    let threshold = k_compatibility_sets.keys().last().unwrap().to_owned();
+        if slen == 0 {
+            slen = slen_;
+        } else if slen_ != slen {
+            bail!(ErrorKind::InconsistentSecretLengths(id, slen_, ids, slen))
+        }
 
-    if shares_count < threshold as usize {
-        bail!(ErrorKind::MissingShares(shares_count, threshold));
+        ids.push(id);
     }
 
-    // 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(),
-                )
-            }
-        }
+    if shares_count < threshold as usize {
+        bail!(ErrorKind::MissingShares(shares_count, threshold))
     }
 
-    // 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 c8f13e1d..fa862f39 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() {

From 07de9be1ff0283c20b766680d7ef53bc4740f872 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 29 Mar 2018 01:22:54 -0600
Subject: [PATCH 21/42] Validation consistency between format & validation
 modules

The best place to catch share problems is immediately during parsing from
`&str`, however, because `validate_shares` takes any type that implements the
`IsShare` trait, and there's nothing about that trait that guarantees that the
share id, threshold, and secret length will be valid, I thought it best to leave
those three tests in `validate_shares` as a defensive coding practice.
---
 src/share/validation.rs  |  3 +++
 src/sss/format.rs        | 12 ++++++------
 tests/recovery_errors.rs | 15 +++++++++------
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/share/validation.rs b/src/share/validation.rs
index 9b4e1587..d7d302fe 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -40,6 +40,9 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
             share.get_data().len(),
         );
 
+        // Public-facing `Share::share_from_string` performs these three tests, but in case another
+        // type which implements `IsShare` is implemented later that doesn't do that validation,
+        // we'll leave them.
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
         } else if threshold_ < 2 {
diff --git a/src/sss/format.rs b/src/sss/format.rs
index 0e7c3eae..142ae8b8 100644
--- a/src/sss/format.rs
+++ b/src/sss/format.rs
@@ -49,12 +49,12 @@ pub(crate) fn share_from_string(s: &str, is_signed: bool) -> Result<Share> {
         (k, i, p3)
     };
 
-    if k < 1 || i < 1 {
-        bail! {
-            ErrorKind::ShareParsingError(
-                format!("Found illegal share info: threshold = {}, identifier = {}.", k, i),
-            )
-        }
+    if i < 1 {
+        bail!(ErrorKind::ShareParsingInvalidShareId(i))
+    } else if k < 2 {
+        bail!(ErrorKind::ShareParsingInvalidShareThreshold(k, i))
+    } else if p3.is_empty() {
+        bail!(ErrorKind::ShareParsingErrorEmptyShare(i))
     }
 
     let raw_data = base64::decode_config(p3, BASE64_CONFIG).chain_err(|| {
diff --git a/tests/recovery_errors.rs b/tests/recovery_errors.rs
index fa862f39..0e1e8f5b 100644
--- a/tests/recovery_errors.rs
+++ b/tests/recovery_errors.rs
@@ -11,6 +11,13 @@ fn test_recover_no_shares() {
     }
 }
 
+#[test]
+#[should_panic(expected = "ShareParsingErrorEmptyShare")]
+fn test_share_parsing_error_empty_share() {
+    let shares = vec!["2-1-".to_string()];
+    recover_secret(&shares, false).unwrap();
+}
+
 #[test]
 #[should_panic(expected = "ShareParsingError")]
 fn test_recover_2_parts_share() {
@@ -34,13 +41,9 @@ fn test_recover_incorrect_share_num() {
 }
 
 #[test]
-#[should_panic(expected = "ShareParsingError")]
+#[should_panic(expected = "ShareParsingInvalidShareId")]
 fn test_recover_0_share_num() {
-    let share1 = "2-0-1YAYwmOHqZ69jA".to_string();
-    let share2 = "2-1-YJZQDGm22Y77Gw".to_string();
-
-    let shares = vec![share1, share2];
-
+    let shares = vec!["2-0-1YAYwmOHqZ69jA".to_string()];
     recover_secret(&shares, false).unwrap();
 }
 

From 463d42b014a8c60bb0dbd3084a7c3f60e5d42f62 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 29 Mar 2018 01:26:44 -0600
Subject: [PATCH 22/42] Minor improvement to validation

---
 src/share/validation.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/share/validation.rs b/src/share/validation.rs
index d7d302fe..57794b9c 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -34,11 +34,9 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
     let mut slen = 0;
 
     for share in shares {
-        let (id, threshold_, slen_) = (
-            share.get_id(),
-            share.get_threshold(),
-            share.get_data().len(),
-        );
+        let id = share.get_id();
+        let threshold_ = share.get_threshold();
+        let slen_ = share.get_data().len();
 
         // Public-facing `Share::share_from_string` performs these three tests, but in case another
         // type which implements `IsShare` is implemented later that doesn't do that validation,
@@ -75,6 +73,8 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
         ids.push(id);
     }
 
+    // Only once the threshold is confirmed as consistent should we determine if shares are
+    // missing.
     if shares_count < threshold as usize {
         bail!(ErrorKind::MissingShares(shares_count, threshold))
     }

From a6ff8a7b95a0a37c6fb8cebb5eb92e8e1aebde6f Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 29 Mar 2018 02:53:57 -0600
Subject: [PATCH 23/42] Adds `no_more_than_five` formatter

This should be useful when validating very large sets of shares. Wouldn't want
to print out up to 254 shares.
---
 src/errors.rs | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index b05605e7..1fbe12b9 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -3,6 +3,7 @@
 #![allow(unknown_lints, missing_docs)]
 
 use std::collections::HashSet;
+use std::fmt;
 
 #[cfg(feature = "dss")]
 use dss::ss1;
@@ -125,7 +126,7 @@ error_chain! {
 
         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)
+            display("The share identifier {} had secret length {}, while the secret length {} was found for share identifier(s): {}.", id, slen_, slen, no_more_than_five(ids))
         }
 
         InconsistentShares {
@@ -135,7 +136,7 @@ error_chain! {
 
         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)
+            display("The share identifier {} had k = {}, while k = {} was found for share identifier(s): {}.", id, k_, k, no_more_than_five(ids))
         }
 
     }
@@ -145,3 +146,19 @@ error_chain! {
         IntegerParsingError(::std::num::ParseIntError);
     }
 }
+
+/// Takes a `Vec<T>` and formats it like the normal `fmt::Debug` implementation, unless it has more
+//than five elements, in which case the rest are replaced by ellipsis.
+fn no_more_than_five<T: fmt::Debug + fmt::Display>(vec: &Vec<T>) -> String {
+    let len = vec.len();
+    if len > 5 {
+        let mut string = String::from("[");
+        for item in vec.iter().take(5) {
+            string += &format!("{}, ", item);
+        }
+        string.push_str("...]");
+        string
+    } else {
+        format!("{:?}", vec)
+    }
+}

From 12cefb1d18ed0ccd8059e4b993ea66a959a40c10 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Mon, 2 Apr 2018 15:02:10 -0500
Subject: [PATCH 24/42] Rustfmt updates + refactor Travis configuration (#60)

* Update rustfmt compliance

Looks like rustfmt has made some improvements recently, so wanted to bring the
code up to date.

* Add rustfmt to nightly item in Travis matrix

* Use Travis Cargo cache

* Allow fast_finish in Travis

Items that match the `allow_failures` predicate (right now, just Rust nightly),
will still finish, but Travis won't wait for them to report a result if the
other builds have already finished.

* Run kcov in a separate matrix build in Travis

* Rework allowed_failures logic

We don't want rustfmt to match `allow_failures` just because it needs to use
nightly, while we do want nightly to match `allow_failures`. Env vars provide a
solution.

* Add --all switch to rustfmt Travis

* Test building docs in Travis

* Use exact Ubuntu dependencies listed for kcov

Some of the dependencies we were installing were not listed on
https://github.com/SimonKagstrom/kcov/blob/master/INSTALL.md, and we were
missing one dependency that was listed there. When `sudo: true` Travis uses
Ubuntu Trusty.

* No need to build before running kcov

kcov builds its own test executables.

* Generate `Cargo.lock` w/ `cargo update` before running kcov

As noted in aeb3906cce8e3e26c7bc80d6aec417b365f3d2f1 it is not necessary to
build the project before running kcov, but kcov does require a `Cargo.lock`
file, which can be generated with `cargo update`.
---
 .travis.yml                   | 56 +++++++++++++++++++++--------------
 benches/ss1.rs                | 22 +++++++++-----
 benches/sss.rs                | 10 +++----
 benches/thss.rs               | 10 +++----
 benches/wrapped_secrets.rs    | 16 +++++-----
 build.rs                      |  4 +--
 src/dss/format.rs             |  2 +-
 src/dss/metadata.rs           |  2 +-
 src/dss/mod.rs                |  2 +-
 src/dss/ss1/mod.rs            |  2 +-
 src/dss/ss1/scheme.rs         | 16 +++++-----
 src/dss/ss1/serialize.rs      |  4 +--
 src/dss/ss1/share.rs          |  2 +-
 src/dss/thss/scheme.rs        |  6 ++--
 src/dss/thss/serialize.rs     |  4 +--
 src/dss/thss/share.rs         |  2 +-
 src/dss/utils.rs              |  2 +-
 src/gf256.rs                  | 13 ++++----
 src/lagrange.rs               |  2 +-
 src/lib.rs                    |  6 ++--
 src/sss/format.rs             |  4 +--
 src/sss/scheme.rs             |  8 ++---
 src/sss/share.rs              |  4 +--
 src/wrapped_secrets/scheme.rs |  4 +--
 24 files changed, 114 insertions(+), 89 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 13be6b3c..cd35f73d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,38 +1,50 @@
-
-sudo: required
-
 language: rust
+cache: cargo # https://docs.travis-ci.com/user/caching/#Rust-Cargo-cache
 
 rust:
   - stable
   - beta
-  - nightly
 
 matrix:
+  # Since this item is allowed to fail, don't wait for it's result to mark the
+  # build complete.
+  fast_finish: true
   allow_failures:
-    - rust: nightly
+    - env: NAME='nightly'
+    - env: NAME='kcov'
+  include:
+    - env: NAME='nightly'
+      rust: nightly
+    - env: NAME='rustfmt'
+      rust: nightly
+      before_script:
+        - rustup component add rustfmt-preview
+      script:
+        - cargo fmt --all -- --write-mode=diff
+    - env: NAME='kcov'
+      sudo: required # travis-ci/travis-ci#9061
+      before_script:
+        - cargo install cargo-update || echo "cargo-update already installed"
+        - cargo install cargo-kcov || echo "cargo-kcov already installed"
+        - cargo install-update -a
+      script:
+        - cargo kcov --print-install-kcov-sh | sh
+        - cargo update # Creates `Cargo.lock` needed by next command
+        - cargo kcov --verbose --features dss --coveralls -- --verify --exclude-pattern=/.cargo,/usr/lib,src/proto
+      addons:
+        apt:
+          packages:
+            - libcurl4-openssl-dev
+            - libdw-dev
+            - binutils-dev
+            - libiberty-dev
+            - zlib1g-dev 
 
 env:
   global:
   - RUSTFLAGS="-C link-dead-code"
 
-addons:
-  apt:
-    packages:
-      - libcurl4-openssl-dev
-      - libdw-dev
-      - cmake
-      - g++
-      - pkg-config
-      - binutils-dev
-      - libiberty-dev
-
 script:
   - cargo build --verbose --all-features
   - cargo test --verbose --all-features
-
-after_success:
-  - cargo install cargo-kcov
-  - cargo kcov --print-install-kcov-sh | sh
-  - cargo kcov --verbose --features dss --coveralls -- --verify --exclude-pattern=/.cargo,/usr/lib,src/proto
-
+  - cargo doc --verbose --all-features
diff --git a/benches/ss1.rs b/benches/ss1.rs
index d32b72b1..f6e993a0 100644
--- a/benches/ss1.rs
+++ b/benches/ss1.rs
@@ -10,29 +10,37 @@ mod shared;
 mod ss1 {
 
     use rusty_secrets::dss::ss1;
-    use test::{black_box, Bencher};
     use shared;
+    use test::{black_box, Bencher};
 
     macro_rules! bench_generate {
-        ($name:ident, $k:expr, $n:expr, $secret:ident) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
 
                 b.iter(move || {
-                    let shares = ss1::split_secret($k, $n, &secret, ss1::Reproducibility::reproducible(), &None).unwrap();
+                    let shares = ss1::split_secret(
+                        $k,
+                        $n,
+                        &secret,
+                        ss1::Reproducibility::reproducible(),
+                        &None,
+                    ).unwrap();
                     black_box(shares);
                 });
             }
-        )
+        };
     }
 
     macro_rules! bench_recover {
-        ($name:ident, $k:expr, $n:expr, $secret:ident) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
-                let all_shares = ss1::split_secret($k, $n, &secret, ss1::Reproducibility::reproducible(), &None).unwrap();
+                let all_shares =
+                    ss1::split_secret($k, $n, &secret, ss1::Reproducibility::reproducible(), &None)
+                        .unwrap();
                 let shares = &all_shares.into_iter().take($k).collect::<Vec<_>>().clone();
 
                 b.iter(|| {
@@ -40,7 +48,7 @@ mod ss1 {
                     black_box(result);
                 });
             }
-        )
+        };
     }
 
     bench_generate!(generate_1kb_3_5, 3, 5, secret_1kb);
diff --git a/benches/sss.rs b/benches/sss.rs
index b48b6aac..65ead565 100644
--- a/benches/sss.rs
+++ b/benches/sss.rs
@@ -8,12 +8,12 @@ mod shared;
 
 mod sss {
 
-    use test::{black_box, Bencher};
     use rusty_secrets::sss;
     use shared;
+    use test::{black_box, Bencher};
 
     macro_rules! bench_generate {
-        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
@@ -23,11 +23,11 @@ mod sss {
                     black_box(shares);
                 });
             }
-        )
+        };
     }
 
     macro_rules! bench_recover {
-        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
@@ -39,7 +39,7 @@ mod sss {
                     black_box(result);
                 });
             }
-        )
+        };
     }
 
     bench_generate!(generate_1kb_3_5, 3, 5, secret_1kb, false);
diff --git a/benches/thss.rs b/benches/thss.rs
index b11386b1..c4dd97d5 100644
--- a/benches/thss.rs
+++ b/benches/thss.rs
@@ -10,11 +10,11 @@ mod shared;
 mod thss {
 
     use rusty_secrets::dss::thss;
-    use test::{black_box, Bencher};
     use shared;
+    use test::{black_box, Bencher};
 
     macro_rules! bench_generate {
-        ($name:ident, $k:expr, $n:expr, $secret:ident) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
@@ -24,11 +24,11 @@ mod thss {
                     black_box(shares);
                 });
             }
-        )
+        };
     }
 
     macro_rules! bench_recover {
-        ($name:ident, $k:expr, $n:expr, $secret:ident) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
@@ -40,7 +40,7 @@ mod thss {
                     black_box(result);
                 });
             }
-        )
+        };
     }
 
     bench_generate!(generate_1kb_3_5, 3, 5, secret_1kb);
diff --git a/benches/wrapped_secrets.rs b/benches/wrapped_secrets.rs
index 627f3766..af571419 100644
--- a/benches/wrapped_secrets.rs
+++ b/benches/wrapped_secrets.rs
@@ -8,30 +8,32 @@ mod shared;
 
 mod wrapped_secrets {
 
-    use test::{black_box, Bencher};
     use rusty_secrets::wrapped_secrets;
     use shared;
+    use test::{black_box, Bencher};
 
     macro_rules! bench_generate {
-        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
 
                 b.iter(move || {
-                    let shares = wrapped_secrets::split_secret($k, $n, secret, None, $signed).unwrap();
+                    let shares =
+                        wrapped_secrets::split_secret($k, $n, secret, None, $signed).unwrap();
                     black_box(shares);
                 });
             }
-        )
+        };
     }
 
     macro_rules! bench_recover {
-        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => (
+        ($name:ident, $k:expr, $n:expr, $secret:ident, $signed:expr) => {
             #[bench]
             fn $name(b: &mut Bencher) {
                 let secret = shared::$secret();
-                let all_shares = wrapped_secrets::split_secret($k, $n, &secret, None, $signed).unwrap();
+                let all_shares =
+                    wrapped_secrets::split_secret($k, $n, &secret, None, $signed).unwrap();
                 let shares = all_shares.into_iter().take($k).collect::<Vec<_>>();
 
                 b.iter(|| {
@@ -39,7 +41,7 @@ mod wrapped_secrets {
                     black_box(result);
                 });
             }
-        )
+        };
     }
 
     bench_generate!(generate_1kb_3_5, 3, 5, secret_1kb, false);
diff --git a/build.rs b/build.rs
index b47873e2..a4d50197 100644
--- a/build.rs
+++ b/build.rs
@@ -1,9 +1,9 @@
 use std::env;
+use std::fmt;
 use std::fs::File;
 use std::io::Write;
-use std::path::Path;
-use std::fmt;
 use std::num::Wrapping;
+use std::path::Path;
 
 const POLY: u8 = 0x1D;
 
diff --git a/src/dss/format.rs b/src/dss/format.rs
index 70669b5b..c4d0386c 100644
--- a/src/dss/format.rs
+++ b/src/dss/format.rs
@@ -1,7 +1,7 @@
 use std::error::Error;
 
-use protobuf::{self, Message};
 use base64;
+use protobuf::{self, Message};
 
 use errors::*;
 use proto::dss::ShareProto;
diff --git a/src/dss/metadata.rs b/src/dss/metadata.rs
index 86c3f924..fa11f24e 100644
--- a/src/dss/metadata.rs
+++ b/src/dss/metadata.rs
@@ -1,5 +1,5 @@
-use std::collections::BTreeMap;
 use ring::digest;
+use std::collections::BTreeMap;
 
 /// A share's public metadata.
 #[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, Default)]
diff --git a/src/dss/mod.rs b/src/dss/mod.rs
index ab45caa7..f7739d28 100644
--- a/src/dss/mod.rs
+++ b/src/dss/mod.rs
@@ -27,8 +27,8 @@
 //! **ErrDet**   | An inauthentic set of shares produced by an adversary will be flagged as such when fed to the recovery algorithm.
 //! **Repro**    | Share reproducible: The scheme can produce shares in a deterministic way.
 
-pub mod thss;
 pub mod ss1;
+pub mod thss;
 
 mod metadata;
 
diff --git a/src/dss/ss1/mod.rs b/src/dss/ss1/mod.rs
index 78335d2e..18a4b066 100644
--- a/src/dss/ss1/mod.rs
+++ b/src/dss/ss1/mod.rs
@@ -29,8 +29,8 @@ mod share;
 pub use self::share::*;
 
 mod scheme;
-use self::scheme::SS1;
 pub use self::scheme::Reproducibility;
+use self::scheme::SS1;
 
 use dss::AccessStructure;
 
diff --git a/src/dss/ss1/scheme.rs b/src/dss/ss1/scheme.rs
index 9672e5ee..49c9e4e6 100644
--- a/src/dss/ss1/scheme.rs
+++ b/src/dss/ss1/scheme.rs
@@ -1,17 +1,17 @@
 use std::collections::HashSet;
 
-use ring::{hkdf, hmac};
-use ring::rand::{SecureRandom, SystemRandom};
-use ring::digest::{Context, SHA256};
 use rand::{ChaChaRng, Rng, SeedableRng};
+use ring::digest::{Context, SHA256};
+use ring::rand::{SecureRandom, SystemRandom};
+use ring::{hkdf, hmac};
 
-use errors::*;
-use dss::{thss, AccessStructure};
-use dss::thss::{MetaData, ThSS};
-use dss::random::{random_bytes_count, FixedRandom, MAX_MESSAGE_SIZE};
-use share::validation::{validate_share_count, validate_shares};
 use super::share::*;
+use dss::random::{random_bytes_count, FixedRandom, MAX_MESSAGE_SIZE};
+use dss::thss::{MetaData, ThSS};
 use dss::utils;
+use dss::{thss, AccessStructure};
+use errors::*;
+use share::validation::{validate_share_count, validate_shares};
 use vol_hash::VOLHash;
 
 /// We bound the message size at about 16MB to avoid overflow in `random_bytes_count`.
diff --git a/src/dss/ss1/serialize.rs b/src/dss/ss1/serialize.rs
index ca040df2..15b48d91 100644
--- a/src/dss/ss1/serialize.rs
+++ b/src/dss/ss1/serialize.rs
@@ -1,8 +1,8 @@
-use errors::*;
 use super::{MetaData, Share};
 use dss::format::{format_share_protobuf, parse_share_protobuf};
-use proto::dss::{MetaDataProto, ShareProto};
 use dss::utils::{btreemap_to_hashmap, hashmap_to_btreemap};
+use errors::*;
+use proto::dss::{MetaDataProto, ShareProto};
 
 pub(crate) fn share_to_string(share: Share) -> String {
     let proto = share_to_protobuf(share);
diff --git a/src/dss/ss1/share.rs b/src/dss/ss1/share.rs
index 4ca41788..d1f6fccb 100644
--- a/src/dss/ss1/share.rs
+++ b/src/dss/ss1/share.rs
@@ -1,6 +1,6 @@
+use super::serialize::{share_from_string, share_to_string};
 use errors::*;
 use share::IsShare;
-use super::serialize::{share_from_string, share_to_string};
 
 pub use dss::metadata::MetaData;
 
diff --git a/src/dss/thss/scheme.rs b/src/dss/thss/scheme.rs
index 141c932f..e7d0d1e2 100644
--- a/src/dss/thss/scheme.rs
+++ b/src/dss/thss/scheme.rs
@@ -4,15 +4,15 @@ use std::fmt;
 
 use ring::rand::{SecureRandom, SystemRandom};
 
+use dss::random::{random_bytes, random_bytes_count, MAX_MESSAGE_SIZE};
 use errors::*;
 use gf256::Gf256;
-use dss::random::{random_bytes, random_bytes_count, MAX_MESSAGE_SIZE};
-use share::validation::{validate_share_count, validate_shares};
 use lagrange;
+use share::validation::{validate_share_count, validate_shares};
 
 use super::AccessStructure;
-use super::share::*;
 use super::encode::encode_secret;
+use super::share::*;
 
 /// We bound the message size at about 16MB to avoid overflow in `random_bytes_count`.
 /// Moreover, given the current performances, it is almost unpractical to run
diff --git a/src/dss/thss/serialize.rs b/src/dss/thss/serialize.rs
index 71909348..1111f55e 100644
--- a/src/dss/thss/serialize.rs
+++ b/src/dss/thss/serialize.rs
@@ -1,8 +1,8 @@
-use errors::*;
 use super::{MetaData, Share};
 use dss::format::{format_share_protobuf, parse_share_protobuf};
-use proto::dss::{MetaDataProto, ShareProto};
 use dss::utils::{btreemap_to_hashmap, hashmap_to_btreemap};
+use errors::*;
+use proto::dss::{MetaDataProto, ShareProto};
 
 pub(crate) fn share_to_string(share: Share) -> String {
     let proto = share_to_protobuf(share);
diff --git a/src/dss/thss/share.rs b/src/dss/thss/share.rs
index 15a942bd..f68bf15e 100644
--- a/src/dss/thss/share.rs
+++ b/src/dss/thss/share.rs
@@ -1,6 +1,6 @@
+use super::serialize::{share_from_string, share_to_string};
 use errors::*;
 use share::IsShare;
-use super::serialize::{share_from_string, share_to_string};
 
 pub use dss::metadata::MetaData;
 
diff --git a/src/dss/utils.rs b/src/dss/utils.rs
index 59a23622..20a0fedf 100644
--- a/src/dss/utils.rs
+++ b/src/dss/utils.rs
@@ -1,7 +1,7 @@
 use std;
 
-use std::hash::Hash;
 use std::collections::{BTreeMap, HashMap};
+use std::hash::Hash;
 
 /// Transmutes a `&[u8]` into a `&[u32]`.
 /// Despite `std::mem::transmute` being very unsafe in
diff --git a/src/gf256.rs b/src/gf256.rs
index 23546d91..49ad57ea 100644
--- a/src/gf256.rs
+++ b/src/gf256.rs
@@ -143,7 +143,9 @@ impl Neg for Gf256 {
 #[macro_export]
 #[doc(hidden)]
 macro_rules! gf256 {
-    ($e:expr) => (Gf256::from_byte($e))
+    ($e:expr) => {
+        Gf256::from_byte($e)
+    };
 }
 
 #[macro_export]
@@ -178,10 +180,10 @@ mod tests {
 
     mod vectors {
         use super::*;
+        use flate2::read::GzDecoder;
+        use itertools::Itertools;
         use std::fs::File;
         use std::io::{BufRead, BufReader};
-        use itertools::Itertools;
-        use flate2::read::GzDecoder;
 
         macro_rules! mk_test {
             ($id:ident, $op:expr, $val:expr) => {
@@ -196,7 +198,8 @@ mod tests {
                     });
 
                     let ref_path = format!("tests/fixtures/gf256/gf256_{}.txt.gz", stringify!($id));
-                    let reference = BufReader::new(GzDecoder::new(File::open(ref_path).unwrap()).unwrap());
+                    let reference =
+                        BufReader::new(GzDecoder::new(File::open(ref_path).unwrap()).unwrap());
 
                     for ((i, j, k), line) in results.zip(reference.lines()) {
                         let left = format!("{} {} {} = {}", i, $op, j, k);
@@ -204,7 +207,7 @@ mod tests {
                         assert_eq!(left, right);
                     }
                 }
-            }
+            };
         }
 
         mk_test!(add, "+", |i: Gf256, j: Gf256| i + j);
diff --git a/src/lagrange.rs b/src/lagrange.rs
index 4118818c..b49a91f4 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -86,10 +86,10 @@ pub(crate) fn interpolate(points: &[(Gf256, Gf256)]) -> Poly {
 #[allow(trivial_casts)]
 mod tests {
 
-    use std;
     use super::*;
     use gf256::*;
     use quickcheck::*;
+    use std;
 
     quickcheck! {
 
diff --git a/src/lib.rs b/src/lib.rs
index 4b884284..90d5c3c0 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -18,15 +18,15 @@ extern crate ring;
 
 #[macro_use]
 mod gf256;
-mod share;
-mod poly;
 mod lagrange;
+mod poly;
+mod share;
 mod vol_hash;
 
 pub mod errors;
+pub mod proto;
 pub mod sss;
 pub mod wrapped_secrets;
-pub mod proto;
 
 #[cfg(feature = "dss")]
 pub mod dss;
diff --git a/src/sss/format.rs b/src/sss/format.rs
index 142ae8b8..e6cb2a7e 100644
--- a/src/sss/format.rs
+++ b/src/sss/format.rs
@@ -1,9 +1,9 @@
+use base64;
 use errors::*;
 use merkle_sigs::{MerklePublicKey, Proof, PublicKey};
+use proto::wrapped::ShareProto;
 use protobuf::{self, Message, RepeatedField};
-use base64;
 use sss::{Share, HASH_ALGO};
-use proto::wrapped::ShareProto;
 use std::error::Error;
 
 const BASE64_CONFIG: base64::Config = base64::STANDARD_NO_PAD;
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 4072c913..836bec6c 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -1,13 +1,13 @@
 //! SSS provides Shamir's secret sharing with raw data.
 
-use rand::{OsRng, Rng};
 use merkle_sigs::sign_data_vec;
+use rand::{OsRng, Rng};
 
 use errors::*;
-use sss::{Share, HASH_ALGO};
-use sss::format::format_share_for_signing;
-use share::validation::{validate_share_count, validate_signed_shares};
 use lagrange::interpolate_at;
+use share::validation::{validate_share_count, validate_signed_shares};
+use sss::format::format_share_for_signing;
+use sss::{Share, HASH_ALGO};
 
 use super::encode::encode_secret_byte;
 
diff --git a/src/sss/share.rs b/src/sss/share.rs
index f46ad678..e3ff1ca1 100644
--- a/src/sss/share.rs
+++ b/src/sss/share.rs
@@ -1,8 +1,8 @@
-use std::error::Error;
 use std::collections::{HashMap, HashSet};
+use std::error::Error;
 
-use merkle_sigs::{MerklePublicKey, Proof};
 use merkle_sigs::verify_data_vec_signature;
+use merkle_sigs::{MerklePublicKey, Proof};
 
 use errors::*;
 use share::{IsShare, IsSignedShare};
diff --git a/src/wrapped_secrets/scheme.rs b/src/wrapped_secrets/scheme.rs
index 40c50f86..f3a58967 100644
--- a/src/wrapped_secrets/scheme.rs
+++ b/src/wrapped_secrets/scheme.rs
@@ -1,8 +1,8 @@
 use errors::*;
+use proto::VersionProto;
+use proto::wrapped::SecretProto;
 use protobuf;
 use protobuf::Message;
-use proto::wrapped::SecretProto;
-use proto::VersionProto;
 
 use sss::SSS;
 pub(crate) use sss::Share;

From d29415ecba6ee779c7cd68012be14bd0b9911366 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Wed, 21 Mar 2018 14:27:52 -0600
Subject: [PATCH 25/42] Refactor barycentric interpolation for modularity

This refactor makes the code a lot clearer, and separates barycentric
interpolation into parts that can be reused, such as in the partial
interpolation functionality I intend to implement.
---
 src/lagrange.rs   | 54 +++++++++++++++++++++++++++--------------------
 src/sss/scheme.rs |  2 +-
 2 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index b49a91f4..d3d56aca 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,28 +1,26 @@
 use gf256::Gf256;
 use poly::Poly;
 
-/// Evaluates an interpolated polynomial at `Gf256::zero()` where
-/// the polynomial is determined using barycentric Lagrange
-/// interpolation based on the given `points` in
-/// the G(2^8) Galois field.
-pub(crate) fn interpolate_at(k: u8, points: &[(u8, u8)]) -> u8 {
-    barycentric_interpolate_at(k as usize, points)
+/// Evaluates an interpolated polynomial at `Gf256::zero()` where the polynomial is determined
+/// using barycentric Lagrange interpolation based on the given `points` in the G(2^8) Galois
+/// field.
+pub(crate) fn interpolate_at(points: &[(u8, u8)]) -> u8 {
+    // Algorithm from "Polynomial Interpolation: Langrange vs Newton" by Wilhelm Werner.
+    let x = points.iter().map(|x| Gf256::from_byte(x.0)).collect();
+    let y = points.iter().map(|x| Gf256::from_byte(x.1)).collect();
+    let w = compute_barycentric_weights(&x);
+    let (num, denom) = compute_barycentric_num_denom_at(&x, &y, &w);
+    (num / denom).to_byte()
 }
 
-/// Barycentric Lagrange interpolation algorithm from "Polynomial
-/// Interpolation: Langrange vs Newton" by Wilhelm Werner. Evaluates
-/// the polynomial at `Gf256::zero()`.
+/// Compute the barycentric weights `w` corresponding to a set of `x` values.
 #[inline]
-fn barycentric_interpolate_at(k: usize, points: &[(u8, u8)]) -> u8 {
-    // Compute the barycentric weights `w`.
+fn compute_barycentric_weights(x: &Vec<Gf256>) -> Vec<Gf256> {
+    let k = x.len();
     let mut w = vec![Gf256::zero(); k];
     w[0] = Gf256::one();
 
-    let mut x = Vec::with_capacity(k);
-    x.push(Gf256::from_byte(points[0].0));
-
     for i in 1..k {
-        x.push(Gf256::from_byte(points[i].0));
         for j in 0..i {
             let delta = x[j] - x[i];
             assert_ne!(delta.poly, 0, "Duplicate shares");
@@ -31,17 +29,27 @@ fn barycentric_interpolate_at(k: usize, points: &[(u8, u8)]) -> u8 {
         }
     }
 
-    // Evaluate the second or "true" form of the barycentric
-    // interpolation formula at `Gf256::zero()`.
+    w
+}
+
+// Compute the numerator and denominator of the second or "true" form of the barycentric
+// interpolation formula at `Gf256::zero()`.
+#[inline]
+fn compute_barycentric_num_denom_at(
+    x: &Vec<Gf256>,
+    y: &Vec<Gf256>,
+    w: &Vec<Gf256>,
+) -> (Gf256, Gf256) {
     let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
-    for i in 0..k {
-        assert_ne!(x[i].poly, 0, "Invalid share x = 0");
-        let diff = w[i] / x[i];
-        num += diff * Gf256::from_byte(points[i].1);
+
+    for (i, &xi) in x.iter().enumerate() {
+        assert_ne!(xi.poly, 0, "Invalid share x = 0");
+        let diff = w[i] / xi;
+        num += diff * y[i];
         denom += diff;
     }
 
-    (num / denom).to_byte()
+    (num, denom)
 }
 
 /// Computeds the coefficient of the Lagrange polynomial interpolated
@@ -131,7 +139,7 @@ mod tests {
             let poly = interpolate(&elems);
 
             let equals = poly.evaluate_at(Gf256::zero()).to_byte()
-                == interpolate_at(points.len() as u8, points.as_slice());
+                == interpolate_at(points.as_slice());
 
             TestResult::from_bool(equals)
         }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 836bec6c..9afc1328 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -102,7 +102,7 @@ impl SSS {
             for s in shares.iter().take(threshold as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            secret.push(interpolate_at(threshold, &*col_in));
+            secret.push(interpolate_at(&*col_in));
         }
 
         Ok(secret)

From bc1f9d8ed14f21f989e331ea9ea19a86ad2c1808 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Thu, 22 Mar 2018 18:28:19 -0600
Subject: [PATCH 26/42] Introduce support for incremental secret computation

In this PR:

* Introduces `PartialSecret` struct and associated methods for interpolating and
  evaluating polynomials incrementally (or all-at-once for that matter).
  * Implements strict input validation for all public functions. With private
    ones we can reason about their inputs.
  * Uses this struct behind-the-scenes with `interpolate_at`.

Problems to be addressed later:

* There should be a higher level interface in sss.
* Error handling right now is mostly for example. Probably we should create some
  new `ErrorKinds`. I just used the most analagous ones as placeholders.
  Validation is comprehensive, I believe, which is good, but it should be DRYed
  out.
  * Numeric overflow is possible when we cast some `len()` to `u8` in order to
    satisfy the function signatures of certain `ErrorKinds`. This is a general
    bug, that I will make a separate PR for.

Future work:

* It is possible to pre-compute all barycentric weights for a given secret
  after receiving the first share(s) if `shares_count` is equal to `threshold`,
  but `Share`s don't include a `shares_count` field (presumably because this is
  unecessary information for reconstruction, and in the case of a share being
  compromised would provide the bad actor with more information).
* Use barycentric Lagrange interpolation to find coefficients (incrementally and
  all at once).
---
 src/lagrange.rs   | 194 ++++++++++++++++++++++++++++++++++++----------
 src/sss/scheme.rs |   3 +-
 2 files changed, 153 insertions(+), 44 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index d3d56aca..b53df3e7 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,59 +1,167 @@
+use std::u8;
+
+use errors::*;
 use gf256::Gf256;
 use poly::Poly;
 
 /// Evaluates an interpolated polynomial at `Gf256::zero()` where the polynomial is determined
 /// using barycentric Lagrange interpolation based on the given `points` in the G(2^8) Galois
 /// field.
-pub(crate) fn interpolate_at(points: &[(u8, u8)]) -> u8 {
-    // Algorithm from "Polynomial Interpolation: Langrange vs Newton" by Wilhelm Werner.
-    let x = points.iter().map(|x| Gf256::from_byte(x.0)).collect();
-    let y = points.iter().map(|x| Gf256::from_byte(x.1)).collect();
-    let w = compute_barycentric_weights(&x);
-    let (num, denom) = compute_barycentric_num_denom_at(&x, &y, &w);
-    (num / denom).to_byte()
+pub(crate) fn interpolate_at(threshold: u8, points: &[(u8, u8)]) -> Result<u8> {
+    if points.len() < threshold as usize {
+        bail!(ErrorKind::MissingShares(points.len(), threshold as usize));
+    }
+    let partial_comp = PartialSecret::new(threshold, points)?;
+    Ok(partial_comp.secret.unwrap())
 }
 
-/// Compute the barycentric weights `w` corresponding to a set of `x` values.
-#[inline]
-fn compute_barycentric_weights(x: &Vec<Gf256>) -> Vec<Gf256> {
-    let k = x.len();
-    let mut w = vec![Gf256::zero(); k];
-    w[0] = Gf256::one();
-
-    for i in 1..k {
-        for j in 0..i {
-            let delta = x[j] - x[i];
-            assert_ne!(delta.poly, 0, "Duplicate shares");
-            w[j] /= delta;
-            w[i] -= w[j];
+/// Stores the intermediate state of interpolation and evaluation at `Gf256::zero()` of a
+/// polynomial. A secret may be computed incrementally using barycentric Lagrange interpolation.
+/// The state is updated with new points until threshold points have been evaluated, at which point
+/// the `secret` field will be updated from `None` to `Some(u8)`.
+pub struct PartialSecret {
+    /// The secret byte. `None` until computation is complete.
+    secret: Option<u8>,
+    /// The number of shares necessary to recover the secret, a.k.a. the threshold.
+    threshold: u8,
+    /// The ids of the share (varies between 1 and n where n is the total number of generated
+    /// shares)
+    ids: Vec<Gf256>,
+    /// The differences of share values divided by their ids.
+    differences: Vec<Gf256>,
+    /// The barycentric weights.
+    weights: Vec<Gf256>,
+}
+
+impl PartialSecret {
+    /// Create a new partial computation given a `threshold` (to know when the computation is
+    /// finished), and an initial set of `points`.
+    #[inline]
+    pub fn new(threshold: u8, points: &[(u8, u8)]) -> Result<Self> {
+        if threshold < 2 {
+            bail!(ErrorKind::ThresholdTooSmall(threshold));
+        } else if points.len() == 0 {
+            bail!(ErrorKind::EmptyShares);
+        } else if points.len() > MAX_SHARES as usize {
+            bail!(ErrorKind::InvalidShareCountMax(
+                points.len() as u8,
+                MAX_SHARES
+            ));
         }
+
+        let mut ids = Vec::with_capacity(points.len());
+        let mut differences = Vec::with_capacity(points.len());
+        // If provided with more than `threshold` points, only the first threshold are considered.
+        for pi in points.iter().take(threshold as usize) {
+            if pi.0 == 0 {
+                bail!(ErrorKind::ShareParsingInvalidShareId(0));
+            }
+            let xi = Gf256::from_byte(pi.0);
+            if ids.iter().find(|&&xj| xi == xj).is_some() {
+                bail!(ErrorKind::DuplicateShareId(xi.poly));
+            }
+            let yi = Gf256::from_byte(pi.1);
+            ids.push(xi);
+            differences.push(yi / xi);
+        }
+
+        let mut partial_comp = Self {
+            secret: None,
+            threshold,
+            ids,
+            differences,
+            weights: vec![],
+        };
+
+        if partial_comp.ids.len() == 1 {
+            return Ok(partial_comp);
+        }
+
+        partial_comp.update_barycentric_weights();
+        Ok(partial_comp)
     }
 
-    w
-}
+    #[inline]
+    pub fn update(&mut self, points: &[(u8, u8)]) -> Result<()> {
+        if points.len() == 0 {
+            bail!(ErrorKind::EmptyShares);
+        } else if points.len() + self.ids.len() > MAX_SHARES as usize {
+            bail!(ErrorKind::InvalidShareCountMax(
+                (points.len() + self.ids.len()) as u8,
+                MAX_SHARES
+            ));
+        }
 
-// Compute the numerator and denominator of the second or "true" form of the barycentric
-// interpolation formula at `Gf256::zero()`.
-#[inline]
-fn compute_barycentric_num_denom_at(
-    x: &Vec<Gf256>,
-    y: &Vec<Gf256>,
-    w: &Vec<Gf256>,
-) -> (Gf256, Gf256) {
-    let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
-
-    for (i, &xi) in x.iter().enumerate() {
-        assert_ne!(xi.poly, 0, "Invalid share x = 0");
-        let diff = w[i] / xi;
-        num += diff * y[i];
-        denom += diff;
+        // If provided with more than than `threshold - self.ids.len()` points, only enough to
+        // satisfy the threshold are considered.
+        for pi in points.iter().take(self.threshold as usize - self.ids.len()) {
+            if pi.0 == 0 {
+                bail!(ErrorKind::ShareParsingInvalidShareId(0));
+            }
+            let xi = Gf256::from_byte(pi.0);
+            if self.ids.iter().find(|&&xj| xi == xj).is_some() {
+                bail!(ErrorKind::DuplicateShareId(xi.poly));
+            }
+            let yi = Gf256::from_byte(pi.1);
+            self.ids.push(xi);
+            self.differences.push(yi / xi);
+        }
+
+        self.update_barycentric_weights();
+        Ok(())
     }
 
-    (num, denom)
+    /// Update the barycentric weights `w` corresponding to a set of `x` values.
+    #[inline]
+    fn update_barycentric_weights(&mut self) {
+        let x = if self.weights.len() == 0 {
+            // Initialize initial weights.
+            self.weights = vec![Gf256::zero(); self.ids.len()];
+            self.weights[0] = Gf256::one();
+            1
+        } else {
+            // Initialize additional weights.
+            let initial_len = self.weights.len();
+            self.weights
+                .append(&mut vec![Gf256::zero(); self.ids.len() - initial_len]);
+            initial_len
+        };
+
+        // Update weights using algorithm (3.1) from "Polynomial Interpolation: Langrange vs
+        // Newton" by Wilhelm Werner.
+        for i in x..self.ids.len() {
+            for j in 0..i {
+                self.weights[j] /= self.ids[j] - self.ids[i];
+                self.weights[i] -= self.weights[j];
+            }
+        }
+
+        // If we have sufficient information, we can compute the secret.
+        if self.weights.len() == self.threshold as usize {
+            self.compute_secret();
+        }
+    }
+
+    // Compute the secret using the second or "true" form of the barycentric interpolation formula
+    // at `Gf256::zero()`.
+    #[inline]
+    fn compute_secret(&mut self) {
+        let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
+        for ((&xi, &di), &wi) in self.ids
+            .iter()
+            .zip(self.differences.iter())
+            .zip(self.weights.iter())
+        {
+            num += wi * di;
+            denom += wi / xi;
+        }
+        self.secret = Some((num / denom).to_byte());
+    }
 }
 
 /// Computeds the coefficient of the Lagrange polynomial interpolated
 /// from the given `points`, in the G(2^8) Galois field.
+#[inline]
 pub(crate) fn interpolate(points: &[(Gf256, Gf256)]) -> Poly {
     let len = points.len();
 
@@ -102,12 +210,12 @@ mod tests {
     quickcheck! {
 
         fn interpolate_evaluate_at_works(ys: Vec<Gf256>) -> TestResult {
-            if ys.is_empty() || ys.len() > std::u8::MAX as usize {
+            if ys.len() < 2 || ys.len() > u8::MAX as usize {
                 return TestResult::discard();
             }
 
             let points = ys.into_iter()
-                           .zip(1..std::u8::MAX)
+                           .zip(1..u8::MAX)
                            .map(|(y, x)| (gf256!(x), y))
                            .collect::<Vec<_>>();
             let poly = interpolate(&points);
@@ -122,12 +230,12 @@ mod tests {
         }
 
         fn interpolate_evaluate_at_0_eq_evaluate_at(ys: Vec<u8>) -> TestResult {
-            if ys.is_empty() || ys.len() > std::u8::MAX as usize {
+            if ys.len() < 2 || ys.len() > u8::MAX as usize {
                 return TestResult::discard();
             }
 
             let points = ys.into_iter()
-                           .zip(1..std::u8::MAX)
+                           .zip(1..u8::MAX)
                            .map(|(y, x)| (x, y))
                            .collect::<Vec<_>>();
 
@@ -139,7 +247,7 @@ mod tests {
             let poly = interpolate(&elems);
 
             let equals = poly.evaluate_at(Gf256::zero()).to_byte()
-                == interpolate_at(points.as_slice());
+                == interpolate_at(points.len() as u8, points.as_slice()).unwrap();
 
             TestResult::from_bool(equals)
         }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 9afc1328..0a3f9b12 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -102,7 +102,8 @@ impl SSS {
             for s in shares.iter().take(threshold as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            secret.push(interpolate_at(&*col_in));
+            let secret_byte = interpolate_at(threshold, &*col_in)?;
+            secret.push(secret_byte);
         }
 
         Ok(secret)

From dc67d1b27fa14a2618c926326b5e8da5394d3d6e Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Fri, 23 Mar 2018 00:23:50 -0600
Subject: [PATCH 27/42] Slight refactor of PartialSecret

Changes the `differences` field name to `diffs`, adds/ improves some
documentation, makes sure `update` fails if we've already evaluated sufficient
points to compute the secret, and adds the `shares_needed` convenience method.
---
 src/lagrange.rs | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index b53df3e7..58e2c0f5 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -28,7 +28,7 @@ pub struct PartialSecret {
     /// shares)
     ids: Vec<Gf256>,
     /// The differences of share values divided by their ids.
-    differences: Vec<Gf256>,
+    diffs: Vec<Gf256>,
     /// The barycentric weights.
     weights: Vec<Gf256>,
 }
@@ -50,7 +50,7 @@ impl PartialSecret {
         }
 
         let mut ids = Vec::with_capacity(points.len());
-        let mut differences = Vec::with_capacity(points.len());
+        let mut diffs = Vec::with_capacity(points.len());
         // If provided with more than `threshold` points, only the first threshold are considered.
         for pi in points.iter().take(threshold as usize) {
             if pi.0 == 0 {
@@ -62,14 +62,17 @@ impl PartialSecret {
             }
             let yi = Gf256::from_byte(pi.1);
             ids.push(xi);
-            differences.push(yi / xi);
+            // Storing these `diffs` instead of the `y` values allows us to do a little more
+            // precomputation, since we really only need `y / x` and not `y` to evaluate the second
+            // form of the barycentric interpolation formula.
+            diffs.push(yi / xi);
         }
 
         let mut partial_comp = Self {
             secret: None,
             threshold,
             ids,
-            differences,
+            diffs,
             weights: vec![],
         };
 
@@ -81,11 +84,14 @@ impl PartialSecret {
         Ok(partial_comp)
     }
 
+    /// Update the partial computation given an additional set of `points`.
     #[inline]
     pub fn update(&mut self, points: &[(u8, u8)]) -> Result<()> {
         if points.len() == 0 {
             bail!(ErrorKind::EmptyShares);
-        } else if points.len() + self.ids.len() > MAX_SHARES as usize {
+        } else if points.len() + self.ids.len() > MAX_SHARES as usize
+            || self.ids.len() == self.threshold as usize
+        {
             bail!(ErrorKind::InvalidShareCountMax(
                 (points.len() + self.ids.len()) as u8,
                 MAX_SHARES
@@ -104,7 +110,7 @@ impl PartialSecret {
             }
             let yi = Gf256::from_byte(pi.1);
             self.ids.push(xi);
-            self.differences.push(yi / xi);
+            self.diffs.push(yi / xi);
         }
 
         self.update_barycentric_weights();
@@ -137,19 +143,19 @@ impl PartialSecret {
         }
 
         // If we have sufficient information, we can compute the secret.
-        if self.weights.len() == self.threshold as usize {
+        if self.shares_needed() == 0 {
             self.compute_secret();
         }
     }
 
-    // Compute the secret using the second or "true" form of the barycentric interpolation formula
-    // at `Gf256::zero()`.
+    /// Compute the secret using the second or "true" form of the barycentric interpolation formula
+    /// at `Gf256::zero()`.
     #[inline]
     fn compute_secret(&mut self) {
         let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
         for ((&xi, &di), &wi) in self.ids
             .iter()
-            .zip(self.differences.iter())
+            .zip(self.diffs.iter())
             .zip(self.weights.iter())
         {
             num += wi * di;
@@ -157,6 +163,14 @@ impl PartialSecret {
         }
         self.secret = Some((num / denom).to_byte());
     }
+
+    /// Returns the number of shares needed to complete the computation.
+    #[inline]
+    pub fn shares_needed(&self) -> u8 {
+        // Safe to cast and subtract because `ids.len()` will be less than `MAX_SHARES` and <=
+        // `threshold`.
+        self.threshold - self.ids.len() as u8
+    }
 }
 
 /// Computeds the coefficient of the Lagrange polynomial interpolated

From c9f0a527dc4fa5591e84bc62556376b6b8b3a0a9 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Fri, 23 Mar 2018 00:27:17 -0600
Subject: [PATCH 28/42] Adds `evaluate_at_x` method to `PartialSecret`

This way we can reuse the computational work we've done if for some reason we
want to evaluate the same set of interpolated points at value other than
`Gf256::zero()`.

As noted, a slight sacrifice to efficiency was made when implementing this
function, in order to reduce the `PartialSecret` size, and increase
precomputation in the standard case of evaluating at `Gf256::zero()`.
---
 src/lagrange.rs | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index 58e2c0f5..046f2fbd 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -171,6 +171,34 @@ impl PartialSecret {
         // `threshold`.
         self.threshold - self.ids.len() as u8
     }
+
+    /// Evaluate the interpolated polynomial at the point `Gf256::from_byte(x)` in the G(2^8)
+    /// Galois field.
+    #[inline]
+    pub fn evaluate_at_x(&self, x: u8) -> Result<u8> {
+        if self.shares_needed() != 0 {
+            bail!(ErrorKind::MissingShares(
+                self.ids.len(),
+                self.threshold as usize
+            ));
+        }
+
+        let x = Gf256::from_byte(x);
+        let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
+        for ((&xi, &di), &wi) in self.ids
+            .iter()
+            .zip(self.diffs.iter())
+            .zip(self.weights.iter())
+        {
+            let delta = x - xi;
+            // Slightly slower to re-multiply the `diffs` by `xi` here, but otherwise we have to
+            // additionally store the `y` values in `PartialSecret`, or store `y` values instead of
+            // the `diffs` and precompute less in the standard case of evaluating at 0.
+            num += wi * di * xi / delta;
+            denom += wi / delta;
+        }
+        Ok((num / denom).to_byte())
+    }
 }
 
 /// Computeds the coefficient of the Lagrange polynomial interpolated

From b605a00309fb8c96e5264f8aa53667347fcebbcd Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Fri, 23 Mar 2018 02:13:25 -0600
Subject: [PATCH 29/42] Additional updates to PartialSecret

Makes `secret` and `threshold` fields public for easy access. Refines
`shares_needed` and adds `shares_evaluated` convenience functions. Refines
example error handling*.

* Note these are still just temporary values to illustrate what type of
validation we will be doing.
---
 src/lagrange.rs | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index 046f2fbd..e2405e3d 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -21,9 +21,9 @@ pub(crate) fn interpolate_at(threshold: u8, points: &[(u8, u8)]) -> Result<u8> {
 /// the `secret` field will be updated from `None` to `Some(u8)`.
 pub struct PartialSecret {
     /// The secret byte. `None` until computation is complete.
-    secret: Option<u8>,
+    pub secret: Option<u8>,
     /// The number of shares necessary to recover the secret, a.k.a. the threshold.
-    threshold: u8,
+    pub threshold: u8,
     /// The ids of the share (varies between 1 and n where n is the total number of generated
     /// shares)
     ids: Vec<Gf256>,
@@ -87,11 +87,14 @@ impl PartialSecret {
     /// Update the partial computation given an additional set of `points`.
     #[inline]
     pub fn update(&mut self, points: &[(u8, u8)]) -> Result<()> {
-        if points.len() == 0 {
+        if self.shares_needed() == 0 {
+            bail!(ErrorKind::InvalidShareCountMax(
+                (points.len() + self.ids.len()) as u8,
+                self.threshold
+            ));
+        } else if points.len() == 0 {
             bail!(ErrorKind::EmptyShares);
-        } else if points.len() + self.ids.len() > MAX_SHARES as usize
-            || self.ids.len() == self.threshold as usize
-        {
+        } else if points.len() + self.ids.len() > MAX_SHARES as usize {
             bail!(ErrorKind::InvalidShareCountMax(
                 (points.len() + self.ids.len()) as u8,
                 MAX_SHARES
@@ -166,10 +169,14 @@ impl PartialSecret {
 
     /// Returns the number of shares needed to complete the computation.
     #[inline]
-    pub fn shares_needed(&self) -> u8 {
-        // Safe to cast and subtract because `ids.len()` will be less than `MAX_SHARES` and <=
-        // `threshold`.
-        self.threshold - self.ids.len() as u8
+    pub fn shares_needed(&self) -> usize {
+        self.threshold as usize - self.ids.len()
+    }
+
+    /// Returns the number of shares that have been evaluated so far.
+    #[inline]
+    pub fn shares_evaluated(&self) -> usize {
+        self.ids.len()
     }
 
     /// Evaluate the interpolated polynomial at the point `Gf256::from_byte(x)` in the G(2^8)

From 8d00278d858059ec484dc3d4f6391d0656339ca5 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Fri, 23 Mar 2018 02:20:06 -0600
Subject: [PATCH 30/42] Add sample sss module partial secret recovery fns

These should be considered exemplary at this point, but I wanted to start to
flesh out a higher-level way to interact with the `PartialSecret` struct.
Besides more conceptual changes in terms of how to make this interface more
user-friendly, I think the validation needs to be DRYed out, and the error
handling refined.

In particular, all the functions that follow `begin_partial_secret_recovery` are
basically analogues to methods, and it feels like it would be nicer to call them
as such instead of as functions.

Mostly, I'm unsure of how the repository maintainers would like such an
interface to work, so only took my best jab at fleshing this out.
---
 src/sss/scheme.rs | 111 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 0a3f9b12..c051e690 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -1,13 +1,26 @@
 //! SSS provides Shamir's secret sharing with raw data.
 
+<<<<<<< HEAD
+=======
+use std::cmp::min;
+
+use rand::{OsRng, Rng};
+>>>>>>> Add sample sss module partial secret recovery fns
 use merkle_sigs::sign_data_vec;
 use rand::{OsRng, Rng};
 
 use errors::*;
+<<<<<<< HEAD
 use lagrange::interpolate_at;
 use share::validation::{validate_share_count, validate_signed_shares};
 use sss::format::format_share_for_signing;
 use sss::{Share, HASH_ALGO};
+=======
+use sss::{Share, HASH_ALGO};
+use sss::format::format_share_for_signing;
+use share::validation::{validate_share_count, validate_signed_shares};
+use lagrange::{interpolate_at, PartialSecret};
+>>>>>>> Add sample sss module partial secret recovery fns
 
 use super::encode::encode_secret_byte;
 
@@ -108,4 +121,102 @@ impl SSS {
 
         Ok(secret)
     }
+
+    /// Begins a partial secret recovery.
+    pub fn begin_partial_secret_recovery(
+        shares: Vec<Share>,
+        verify_signatures: bool,
+    ) -> Result<Vec<PartialSecret>> {
+        if shares.is_empty() {
+            bail!(ErrorKind::EmptyShares);
+        }
+
+        let (threshold, shares) = validate_signed_shares(shares, verify_signatures)?;
+
+        let slen = shares[0].data.len();
+        let mut col_in = Vec::with_capacity(min(shares.len(), threshold as usize));
+        let mut partial_secret = Vec::with_capacity(slen);
+        for byteindex in 0..slen {
+            col_in.clear();
+            for s in shares.iter().take(threshold as usize) {
+                col_in.push((s.id, s.data[byteindex]));
+            }
+            let partial_secret_byte = PartialSecret::new(threshold, &*col_in)?;
+            partial_secret.push(partial_secret_byte);
+        }
+
+        Ok(partial_secret)
+    }
+
+    /// Contines a partial secret recovery.
+    pub fn update_partial_secret(
+        partial_secret: &mut Vec<PartialSecret>,
+        shares: Vec<Share>,
+        verify_signatures: bool,
+    ) -> Result<()> {
+        if shares.is_empty() {
+            bail!(ErrorKind::EmptyShares);
+        } else if partial_secret.is_empty() {
+            bail!(ErrorKind::EmptyShares);
+        }
+        let threshold = partial_secret[0].threshold;
+        let shares_evaluated = partial_secret[0].shares_evaluated();
+        let shares_needed = partial_secret[0].shares_needed();
+        if shares_needed == 0 {
+            bail!(ErrorKind::InvalidShareCountMax(
+                (shares.len() + shares_evaluated) as u8,
+                threshold
+            ));
+        } else if shares.is_empty() {
+            bail!(ErrorKind::EmptyShares);
+        } else if shares_evaluated + shares.len() > MAX_SHARES as usize {
+            bail!(ErrorKind::InvalidShareCountMax(
+                (shares_evaluated + shares.len()) as u8,
+                MAX_SHARES
+            ));
+        }
+
+        let (threshold2, shares) = validate_signed_shares(shares, verify_signatures)?;
+        if threshold != threshold2 {
+            bail!(ErrorKind::InconsistentShares)
+        }
+
+        let slen = shares[0].data.len();
+        let mut col_in = Vec::with_capacity(shares_needed);
+        for byteindex in 0..slen {
+            col_in.clear();
+            for s in shares.iter().take(shares_needed) {
+                col_in.push((s.id, s.data[byteindex]));
+            }
+            partial_secret[byteindex].update(&*col_in)?;
+        }
+
+        Ok(())
+    }
+
+    /// Used to determine how many more shares are needed to finish computing a partial secret.
+    pub fn partial_secret_shares_needed(partial_secret: &Vec<PartialSecret>) -> Result<u8> {
+        if partial_secret.is_empty() {
+            bail!(ErrorKind::EmptyShares);
+        }
+        Ok(partial_secret[0].shares_needed() as u8)
+    }
+
+    /// Used to obtain the resulting secret when `threshold` shares have been evaluated.
+    pub fn get_final_secret(partial_secret: &Vec<PartialSecret>) -> Result<Vec<u8>> {
+        if partial_secret.is_empty() {
+            bail!(ErrorKind::EmptyShares);
+        }
+        let threshold = partial_secret[0].threshold;
+        let shares_evaluated = partial_secret[0].shares_evaluated();
+        let shares_needed = partial_secret[0].shares_needed();
+        if shares_needed != 0 {
+            bail!(ErrorKind::MissingShares(
+                shares_evaluated,
+                threshold as usize
+            ));
+        }
+        // Safe to unwrap because we already confirmed no more shares are needed.
+        Ok(partial_secret.iter().map(|ps| ps.secret.unwrap()).collect())
+    }
 }

From 0f4213dbe5244ab548bd2650f700b0a2bec3c24f Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Mon, 26 Mar 2018 16:08:40 -0600
Subject: [PATCH 31/42] DRY and validation refactor for `PartialSecrets`

* Create new `NoMoreSharesNeeded` `ErrorKind` to be used when a `PartialSecret`
  already holds a complete secret.
* Replaced large `if else` validation blocks with re-usable methods and
  functions.
* Created `update_diffs` function to DRY out code shared between `new` and
  `update` methods.
---
 src/errors.rs   |   6 +++
 src/lagrange.rs | 116 ++++++++++++++++++++++++++++--------------------
 2 files changed, 73 insertions(+), 49 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 1fbe12b9..becd2f6e 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -65,6 +65,12 @@ error_chain! {
             display("{} shares are required to recover the secret, found only {}.", required, provided)
         }
 
+        NoMoreSharesNeeded(required: u8) {
+            description("The number of shares evaluated has already met the threshold and the
+            secret is available.")
+            display("Only {} shares are required to recover the secret.", required)
+        }
+
         InvalidSignature(share_id: u8, signature: String) {
             description("The signature of this share is not valid.")
         }
diff --git a/src/lagrange.rs b/src/lagrange.rs
index e2405e3d..7d48795f 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,3 +1,4 @@
+use std::cmp::min;
 use std::u8;
 
 use errors::*;
@@ -38,48 +39,20 @@ impl PartialSecret {
     /// finished), and an initial set of `points`.
     #[inline]
     pub fn new(threshold: u8, points: &[(u8, u8)]) -> Result<Self> {
-        if threshold < 2 {
-            bail!(ErrorKind::ThresholdTooSmall(threshold));
-        } else if points.len() == 0 {
-            bail!(ErrorKind::EmptyShares);
-        } else if points.len() > MAX_SHARES as usize {
-            bail!(ErrorKind::InvalidShareCountMax(
-                points.len() as u8,
-                MAX_SHARES
-            ));
-        }
-
-        let mut ids = Vec::with_capacity(points.len());
-        let mut diffs = Vec::with_capacity(points.len());
-        // If provided with more than `threshold` points, only the first threshold are considered.
-        for pi in points.iter().take(threshold as usize) {
-            if pi.0 == 0 {
-                bail!(ErrorKind::ShareParsingInvalidShareId(0));
-            }
-            let xi = Gf256::from_byte(pi.0);
-            if ids.iter().find(|&&xj| xi == xj).is_some() {
-                bail!(ErrorKind::DuplicateShareId(xi.poly));
-            }
-            let yi = Gf256::from_byte(pi.1);
-            ids.push(xi);
-            // Storing these `diffs` instead of the `y` values allows us to do a little more
-            // precomputation, since we really only need `y / x` and not `y` to evaluate the second
-            // form of the barycentric interpolation formula.
-            diffs.push(yi / xi);
-        }
+        validate_threshold(threshold)?;
+        validate_shares_is_nonempty(points)?;
 
+        let capacity = min(threshold as usize, points.len());
         let mut partial_comp = Self {
             secret: None,
             threshold,
-            ids,
-            diffs,
+            ids: Vec::with_capacity(capacity),
+            diffs: Vec::with_capacity(capacity),
             weights: vec![],
         };
+        partial_comp.validate_total_shares_less_than_max(points)?;
 
-        if partial_comp.ids.len() == 1 {
-            return Ok(partial_comp);
-        }
-
+        partial_comp.update_diffs(points)?;
         partial_comp.update_barycentric_weights();
         Ok(partial_comp)
     }
@@ -87,20 +60,18 @@ impl PartialSecret {
     /// Update the partial computation given an additional set of `points`.
     #[inline]
     pub fn update(&mut self, points: &[(u8, u8)]) -> Result<()> {
-        if self.shares_needed() == 0 {
-            bail!(ErrorKind::InvalidShareCountMax(
-                (points.len() + self.ids.len()) as u8,
-                self.threshold
-            ));
-        } else if points.len() == 0 {
-            bail!(ErrorKind::EmptyShares);
-        } else if points.len() + self.ids.len() > MAX_SHARES as usize {
-            bail!(ErrorKind::InvalidShareCountMax(
-                (points.len() + self.ids.len()) as u8,
-                MAX_SHARES
-            ));
-        }
+        self.validate_shares_are_needed()?;
+        validate_shares_is_nonempty(points)?;
+        self.validate_total_shares_less_than_max(points)?;
+
+        self.update_diffs(points)?;
+        self.update_barycentric_weights();
+        Ok(())
+    }
 
+    /// Parse just the `points` we need to compute the secret into `x` values and `diffs`, making
+    // sure they are valid and unique.
+    fn update_diffs(&mut self, points: &[(u8, u8)]) -> Result<()> {
         // If provided with more than than `threshold - self.ids.len()` points, only enough to
         // satisfy the threshold are considered.
         for pi in points.iter().take(self.threshold as usize - self.ids.len()) {
@@ -113,16 +84,23 @@ impl PartialSecret {
             }
             let yi = Gf256::from_byte(pi.1);
             self.ids.push(xi);
+            // Storing these `diffs` instead of the `y` values allows us to do a little more
+            // precomputation, since we really only need `y / x` and not `y` to evaluate the second
+            // form of the barycentric interpolation formula.
             self.diffs.push(yi / xi);
         }
 
-        self.update_barycentric_weights();
         Ok(())
     }
 
     /// Update the barycentric weights `w` corresponding to a set of `x` values.
     #[inline]
     fn update_barycentric_weights(&mut self) {
+        // Need at least two points to start computing the barycentric weights.
+        if self.ids.len() == 1 {
+            return;
+        }
+
         let x = if self.weights.len() == 0 {
             // Initialize initial weights.
             self.weights = vec![Gf256::zero(); self.ids.len()];
@@ -167,6 +145,28 @@ impl PartialSecret {
         self.secret = Some((num / denom).to_byte());
     }
 
+    /// `bail!`s if `threshold` shares have already been evaluated.
+    #[inline]
+    fn validate_shares_are_needed(&self) -> Result<()> {
+        if self.shares_needed() == 0 {
+            bail!(ErrorKind::NoMoreSharesNeeded(self.threshold));
+        }
+        Ok(())
+    }
+
+    /// `bail!`s if shares already evaluated plus new points given is greater than `MAX_SHARES`.
+    #[inline]
+    fn validate_total_shares_less_than_max(&self, points: &[(u8, u8)]) -> Result<()> {
+        let shares_total = points.len() + self.shares_evaluated();
+        if shares_total > MAX_SHARES as usize {
+            bail!(ErrorKind::InvalidShareCountMax(
+                shares_total as u8,
+                MAX_SHARES
+            ));
+        }
+        Ok(())
+    }
+
     /// Returns the number of shares needed to complete the computation.
     #[inline]
     pub fn shares_needed(&self) -> usize {
@@ -208,6 +208,24 @@ impl PartialSecret {
     }
 }
 
+/// `bail!`s if `threshold` is less than 2.
+#[inline]
+pub fn validate_threshold(threshold: u8) -> Result<()> {
+    if threshold < 2 {
+        bail!(ErrorKind::ThresholdTooSmall(threshold));
+    }
+    Ok(())
+}
+
+/// `bail!`s if `points` is empty.
+#[inline]
+pub fn validate_shares_is_nonempty(points: &[(u8, u8)]) -> Result<()> {
+    if points.len() == 0 {
+        bail!(ErrorKind::EmptyShares);
+    }
+    Ok(())
+}
+
 /// Computeds the coefficient of the Lagrange polynomial interpolated
 /// from the given `points`, in the G(2^8) Galois field.
 #[inline]

From 34bdb4d9f108fd92305ebbae70139f3a0810ee10 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Sun, 1 Apr 2018 14:28:40 -0500
Subject: [PATCH 32/42] Introduce IncrementalRecovery and refactor validation

* Introduces the IncrementalRecovery struct, a struct that creates and updates
  many `PartialSecret`s from `Share`s, essentially introducing a higher-level
  interface.
* New validation functions were created to handle this case were not all shares
  arrive at once. Only the necessary metadata for validating further shares (the
  threshold, the secret length, the IDs that have been verified so far, and
  optionally the root hash that signed the shares validated so far) is stored by
  the `IncrementalRecovery` struct.
---
 src/errors.rs           |   9 +++
 src/lagrange.rs         | 148 ++++++++++++++++--------------------
 src/share/mod.rs        |  15 +++-
 src/share/validation.rs |  78 +++++++++++++++----
 src/sss/scheme.rs       | 162 ++++++++++++++++++++--------------------
 src/sss/share.rs        |  92 ++++++++++++++---------
 6 files changed, 282 insertions(+), 222 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index becd2f6e..c9021ef9 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -135,6 +135,11 @@ error_chain! {
             display("The share identifier {} had secret length {}, while the secret length {} was found for share identifier(s): {}.", id, slen_, slen, no_more_than_five(ids))
         }
 
+        InconsistentSignatures(id: u8, ids: Vec<u8>) {
+            description("The shares are incompatible with each other because they have valid signatures from different keys.")
+            display("The share identifier {} was signed by a different key than share identifier(s): {}.", id, no_more_than_five(ids))
+        }
+
         InconsistentShares {
             description("The shares are inconsistent")
             display("The shares are inconsistent")
@@ -145,6 +150,10 @@ error_chain! {
             display("The share identifier {} had k = {}, while k = {} was found for share identifier(s): {}.", id, k_, k, no_more_than_five(ids))
         }
 
+        PartialInterpolationNotComplete(k: u8, shares_interpolated: u8) {
+            description("The partial interpolation result is not complete because the number of points interpolated has not reached the threshold.")
+            display("In order to evaluate the secret polynomial at any point k = {} shares are needed, whereas only {} have been provided.", k, shares_interpolated)
+        }
     }
 
     foreign_links {
diff --git a/src/lagrange.rs b/src/lagrange.rs
index 7d48795f..cc7dc0b8 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,4 +1,3 @@
-use std::cmp::min;
 use std::u8;
 
 use errors::*;
@@ -9,11 +8,8 @@ use poly::Poly;
 /// using barycentric Lagrange interpolation based on the given `points` in the G(2^8) Galois
 /// field.
 pub(crate) fn interpolate_at(threshold: u8, points: &[(u8, u8)]) -> Result<u8> {
-    if points.len() < threshold as usize {
-        bail!(ErrorKind::MissingShares(points.len(), threshold as usize));
-    }
-    let partial_comp = PartialSecret::new(threshold, points)?;
-    Ok(partial_comp.secret.unwrap())
+    let partial_comp = PartialSecret::new(threshold, points);
+    Ok(partial_comp.get_secret().unwrap())
 }
 
 /// Stores the intermediate state of interpolation and evaluation at `Gf256::zero()` of a
@@ -22,11 +18,11 @@ pub(crate) fn interpolate_at(threshold: u8, points: &[(u8, u8)]) -> Result<u8> {
 /// the `secret` field will be updated from `None` to `Some(u8)`.
 pub struct PartialSecret {
     /// The secret byte. `None` until computation is complete.
-    pub secret: Option<u8>,
+    secret: Option<u8>,
     /// The number of shares necessary to recover the secret, a.k.a. the threshold.
-    pub threshold: u8,
+    threshold: u8,
     /// The ids of the share (varies between 1 and n where n is the total number of generated
-    /// shares)
+    /// shares).
     ids: Vec<Gf256>,
     /// The differences of share values divided by their ids.
     diffs: Vec<Gf256>,
@@ -34,54 +30,53 @@ pub struct PartialSecret {
     weights: Vec<Gf256>,
 }
 
+// `PartialSecret` is not a public-facing struct. We expect the functions that interact with it to
+// do validation of the `points` and other arguments it operates on. As a defensive programming
+// practice, we have included `assert!` statements, which should also clarify the validation
+// expectations of each method.
 impl PartialSecret {
     /// Create a new partial computation given a `threshold` (to know when the computation is
     /// finished), and an initial set of `points`.
     #[inline]
-    pub fn new(threshold: u8, points: &[(u8, u8)]) -> Result<Self> {
-        validate_threshold(threshold)?;
-        validate_shares_is_nonempty(points)?;
+    pub fn new(threshold: u8, points: &[(u8, u8)]) -> Self {
+        assert!(threshold >= 2, "Given k less than 2!");
+        assert!(!points.is_empty(), "Given an empty set of points!");
+        assert!(
+            points.len() <= threshold as usize,
+            "Given more than threshold shares!"
+        );
 
-        let capacity = min(threshold as usize, points.len());
         let mut partial_comp = Self {
             secret: None,
             threshold,
-            ids: Vec::with_capacity(capacity),
-            diffs: Vec::with_capacity(capacity),
+            ids: Vec::with_capacity(threshold as usize),
+            diffs: Vec::with_capacity(threshold as usize),
             weights: vec![],
         };
-        partial_comp.validate_total_shares_less_than_max(points)?;
 
-        partial_comp.update_diffs(points)?;
+        partial_comp.update_diffs(points);
         partial_comp.update_barycentric_weights();
-        Ok(partial_comp)
+        partial_comp
     }
 
     /// Update the partial computation given an additional set of `points`.
     #[inline]
-    pub fn update(&mut self, points: &[(u8, u8)]) -> Result<()> {
-        self.validate_shares_are_needed()?;
-        validate_shares_is_nonempty(points)?;
-        self.validate_total_shares_less_than_max(points)?;
-
-        self.update_diffs(points)?;
+    pub fn update(&mut self, points: &[(u8, u8)]) {
+        assert!(!points.is_empty(), "Given an empty set of points!");
+        assert!(
+            self.shares_interpolated() as usize + points.len() < self.threshold as usize,
+            "Given more than threshold shares!"
+        );
+
+        self.update_diffs(points);
         self.update_barycentric_weights();
-        Ok(())
     }
 
-    /// Parse just the `points` we need to compute the secret into `x` values and `diffs`, making
-    // sure they are valid and unique.
-    fn update_diffs(&mut self, points: &[(u8, u8)]) -> Result<()> {
-        // If provided with more than than `threshold - self.ids.len()` points, only enough to
-        // satisfy the threshold are considered.
-        for pi in points.iter().take(self.threshold as usize - self.ids.len()) {
-            if pi.0 == 0 {
-                bail!(ErrorKind::ShareParsingInvalidShareId(0));
-            }
+    /// Parse just the `points` we need to compute the secret into `x` values and `diffs`.
+    fn update_diffs(&mut self, points: &[(u8, u8)]) {
+        for pi in points.iter() {
             let xi = Gf256::from_byte(pi.0);
-            if self.ids.iter().find(|&&xj| xi == xj).is_some() {
-                bail!(ErrorKind::DuplicateShareId(xi.poly));
-            }
+            assert!(xi.poly != 0, "Given invalid share identifier 0!");
             let yi = Gf256::from_byte(pi.1);
             self.ids.push(xi);
             // Storing these `diffs` instead of the `y` values allows us to do a little more
@@ -89,8 +84,6 @@ impl PartialSecret {
             // form of the barycentric interpolation formula.
             self.diffs.push(yi / xi);
         }
-
-        Ok(())
     }
 
     /// Update the barycentric weights `w` corresponding to a set of `x` values.
@@ -118,7 +111,9 @@ impl PartialSecret {
         // Newton" by Wilhelm Werner.
         for i in x..self.ids.len() {
             for j in 0..i {
-                self.weights[j] /= self.ids[j] - self.ids[i];
+                let diff = self.ids[j] - self.ids[i];
+                assert!(diff.poly != 0, "Duplicate share identifiers encountered!");
+                self.weights[j] /= diff;
                 self.weights[i] -= self.weights[j];
             }
         }
@@ -145,49 +140,50 @@ impl PartialSecret {
         self.secret = Some((num / denom).to_byte());
     }
 
-    /// `bail!`s if `threshold` shares have already been evaluated.
+    /// If the partial computation is complete, return the secret, else an error.
     #[inline]
-    fn validate_shares_are_needed(&self) -> Result<()> {
-        if self.shares_needed() == 0 {
-            bail!(ErrorKind::NoMoreSharesNeeded(self.threshold));
+    pub fn get_secret(&self) -> Result<u8> {
+        if self.secret.is_none() {
+            bail!(ErrorKind::PartialInterpolationNotComplete(
+                self.threshold,
+                self.shares_interpolated()
+            ))
         }
-        Ok(())
+        // Safe to unwrap because we just confirmed it's not `None`.
+        Ok(self.secret.unwrap())
     }
 
-    /// `bail!`s if shares already evaluated plus new points given is greater than `MAX_SHARES`.
+    /// Returns the threshold for the partial computation.
     #[inline]
-    fn validate_total_shares_less_than_max(&self, points: &[(u8, u8)]) -> Result<()> {
-        let shares_total = points.len() + self.shares_evaluated();
-        if shares_total > MAX_SHARES as usize {
-            bail!(ErrorKind::InvalidShareCountMax(
-                shares_total as u8,
-                MAX_SHARES
-            ));
-        }
-        Ok(())
+    fn get_threshold(&self) -> u8 {
+        self.threshold
     }
 
     /// Returns the number of shares needed to complete the computation.
     #[inline]
-    pub fn shares_needed(&self) -> usize {
-        self.threshold as usize - self.ids.len()
+    pub fn shares_needed(&self) -> u8 {
+        // Casting is safe because `assert!` statements in `new` and `update` ensure
+        // `self.ids.len()` will be less than 255.
+        self.threshold - self.ids.len() as u8
     }
 
-    /// Returns the number of shares that have been evaluated so far.
+    /// Returns the number of shares that have been interpolated so far.
     #[inline]
-    pub fn shares_evaluated(&self) -> usize {
-        self.ids.len()
+    pub fn shares_interpolated(&self) -> u8 {
+        // Casting is safe because `assert!` statements in `new` and `update` ensure
+        // `self.ids.len()` will be less than 255.
+        self.ids.len() as u8
     }
 
     /// Evaluate the interpolated polynomial at the point `Gf256::from_byte(x)` in the G(2^8)
     /// Galois field.
     #[inline]
-    pub fn evaluate_at_x(&self, x: u8) -> Result<u8> {
+    fn evaluate_at_x(&self, x: u8) -> Result<u8> {
         if self.shares_needed() != 0 {
-            bail!(ErrorKind::MissingShares(
-                self.ids.len(),
-                self.threshold as usize
-            ));
+            bail!(ErrorKind::PartialInterpolationNotComplete(
+                self.threshold,
+                self.shares_interpolated()
+            ))
         }
 
         let x = Gf256::from_byte(x);
@@ -208,26 +204,8 @@ impl PartialSecret {
     }
 }
 
-/// `bail!`s if `threshold` is less than 2.
-#[inline]
-pub fn validate_threshold(threshold: u8) -> Result<()> {
-    if threshold < 2 {
-        bail!(ErrorKind::ThresholdTooSmall(threshold));
-    }
-    Ok(())
-}
-
-/// `bail!`s if `points` is empty.
-#[inline]
-pub fn validate_shares_is_nonempty(points: &[(u8, u8)]) -> Result<()> {
-    if points.len() == 0 {
-        bail!(ErrorKind::EmptyShares);
-    }
-    Ok(())
-}
-
-/// Computeds the coefficient of the Lagrange polynomial interpolated
-/// from the given `points`, in the G(2^8) Galois field.
+/// Computes the coefficient of the Lagrange polynomial interpolated from the given `points`, in
+//the G(2^8) Galois field.
 #[inline]
 pub(crate) fn interpolate(points: &[(Gf256, Gf256)]) -> Poly {
     let len = points.len();
diff --git a/src/share/mod.rs b/src/share/mod.rs
index beb1c5c8..49b39fb6 100644
--- a/src/share/mod.rs
+++ b/src/share/mod.rs
@@ -34,7 +34,16 @@ pub(crate) trait IsSignedShare: IsShare {
     /// Return the signature itself.
     fn get_signature(&self) -> &Self::Signature;
 
-    /// Verify the signatures of the given batch of shares.
-    /// Returns `Ok(())` if validation succeeds, and an `Err` otherwise.
-    fn verify_signatures(shares: &[Self]) -> Result<()>;
+    /// Verify a given batch of shares are all signed by the same root hash. Returns the root hash
+    /// if verification succeeds, and an `Err` otherwise.
+    fn verify_signatures(shares: &[Self]) -> Result<Vec<u8>>;
+
+    /// Verify the `shares` all have valid signatures from the `root_hash`. Pass a list of shares
+    /// identifiers already verified against this `root_hash`, if any, for better error messaging
+    /// if verification fails.
+    fn continue_verify_signatures(
+        shares: &[Self],
+        root_hash: &Vec<u8>,
+        already_verified_ids: &Vec<u8>,
+    ) -> Result<()>;
 }
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 57794b9c..aa3e1182 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -7,7 +7,7 @@ use share::{IsShare, IsSignedShare};
 // 2) Validate group consistency
 // 3) Validate other properties, in no specific order
 
-/// TODO: Doc
+/// TODO
 pub(crate) fn validate_signed_shares<S: IsSignedShare>(
     shares: &Vec<S>,
     verify_signatures: bool,
@@ -16,22 +16,78 @@ pub(crate) fn validate_signed_shares<S: IsSignedShare>(
 
     if verify_signatures {
         S::verify_signatures(&shares)?;
-    }
+    };
 
     Ok(result)
 }
 
-/// TODO: Doc
+pub(crate) fn begin_signed_share_validation<S: IsSignedShare>(
+    shares: &Vec<S>,
+    verify_signatures: bool,
+) -> Result<(u8, usize, Vec<u8>, Option<Vec<u8>>)> {
+    let (threshold, slen, ids) = _validate_shares(shares, None, None, None)?;
+
+    let root_hash = if verify_signatures {
+        Some(S::verify_signatures(&shares)?)
+    } else {
+        None
+    };
+
+    Ok((threshold, slen, ids, root_hash))
+}
+
+pub(crate) fn continue_signed_share_validation<S: IsSignedShare>(
+    shares: &Vec<S>,
+    already_verified_ids: &Vec<u8>,
+    threshold: u8,
+    slen: usize,
+    root_hash: Option<&Vec<u8>>,
+) -> Result<(Vec<u8>)> {
+    let (_, _, new_ids) = _validate_shares(
+        shares,
+        Some(threshold),
+        Some(slen),
+        Some(already_verified_ids),
+    )?;
+
+    if root_hash.is_some() {
+        S::continue_verify_signatures(shares, root_hash.unwrap(), already_verified_ids)?;
+    }
+
+    Ok(new_ids)
+}
+
+/// Validates a full set of shares.
 pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)> {
+    let (threshold, slen, _) = _validate_shares(shares, None, None, None)?;
+    let shares_count = shares.len();
+    if shares_count < threshold as usize {
+        bail!(ErrorKind::MissingShares(shares_count, threshold))
+    }
+    Ok((threshold, slen))
+}
+
+/// TODO: Doc
+fn _validate_shares<S: IsShare>(
+    shares: &Vec<S>,
+    threshold: Option<u8>,
+    slen: Option<usize>,
+    already_verified_ids: Option<&Vec<u8>>,
+) -> Result<(u8, usize, Vec<u8>)> {
     if shares.is_empty() {
         bail!(ErrorKind::EmptyShares);
     }
 
     let shares_count = shares.len();
-
-    let mut ids = Vec::with_capacity(shares_count);
-    let mut threshold = 0;
-    let mut slen = 0;
+    let mut ids = if already_verified_ids.is_some() {
+        let mut ids = already_verified_ids.unwrap().clone();
+        ids.reserve_exact(shares_count);
+        ids
+    } else {
+        Vec::with_capacity(shares_count)
+    };
+    let mut threshold = threshold.unwrap_or(0);
+    let mut slen = slen.unwrap_or(0);
 
     for share in shares {
         let id = share.get_id();
@@ -73,13 +129,7 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
         ids.push(id);
     }
 
-    // Only once the threshold is confirmed as consistent should we determine if shares are
-    // missing.
-    if shares_count < threshold as usize {
-        bail!(ErrorKind::MissingShares(shares_count, threshold))
-    }
-
-    Ok((threshold, slen))
+    Ok((threshold, slen, ids))
 }
 
 pub(crate) fn validate_share_count(threshold: u8, shares_count: u8) -> Result<(u8, u8)> {
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index c051e690..c0af3510 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -1,26 +1,14 @@
 //! SSS provides Shamir's secret sharing with raw data.
 
-<<<<<<< HEAD
-=======
-use std::cmp::min;
-
-use rand::{OsRng, Rng};
->>>>>>> Add sample sss module partial secret recovery fns
 use merkle_sigs::sign_data_vec;
 use rand::{OsRng, Rng};
 
 use errors::*;
-<<<<<<< HEAD
-use lagrange::interpolate_at;
-use share::validation::{validate_share_count, validate_signed_shares};
+use lagrange::{interpolate_at, PartialSecret};
+use share::validation::{begin_signed_share_validation, continue_signed_share_validation,
+                        validate_share_count, validate_signed_shares};
 use sss::format::format_share_for_signing;
 use sss::{Share, HASH_ALGO};
-=======
-use sss::{Share, HASH_ALGO};
-use sss::format::format_share_for_signing;
-use share::validation::{validate_share_count, validate_signed_shares};
-use lagrange::{interpolate_at, PartialSecret};
->>>>>>> Add sample sss module partial secret recovery fns
 
 use super::encode::encode_secret_byte;
 
@@ -121,102 +109,110 @@ impl SSS {
 
         Ok(secret)
     }
+}
 
-    /// Begins a partial secret recovery.
-    pub fn begin_partial_secret_recovery(
-        shares: Vec<Share>,
-        verify_signatures: bool,
-    ) -> Result<Vec<PartialSecret>> {
-        if shares.is_empty() {
-            bail!(ErrorKind::EmptyShares);
-        }
+/// `IncrementalRecovery` provides a way to incrementally recover a secret in cases where not all
+/// shares are available at once.
+pub(crate) struct IncrementalRecovery {
+    /// The state of each partially-recovered secret byte.
+    partial_secrets: Vec<PartialSecret>,
+    /// The ids of the share (varies between 1 and n where n is the total number of generated
+    /// shares).
+    ids: Vec<u8>,
+    /// The number of shares necessary to recover the secret.
+    threshold: u8,
+    /// The length of the secret.
+    slen: usize,
+    /// If the shares are signed, the root hash of the Merkle tree all shares are signed with.
+    root_hash: Option<Vec<u8>>,
+}
 
-        let (threshold, shares) = validate_signed_shares(shares, verify_signatures)?;
+impl IncrementalRecovery {
+    /// Begins a partial secret recovery.
+    pub fn new(shares: Vec<Share>, verify_signatures: bool) -> Result<Self> {
+        let (threshold, slen, ids, root_hash) =
+            begin_signed_share_validation(&shares, verify_signatures)?;
+
+        let mut incremental_recovery = Self {
+            partial_secrets: Vec::with_capacity(slen),
+            ids,
+            threshold,
+            slen,
+            root_hash,
+        };
 
-        let slen = shares[0].data.len();
-        let mut col_in = Vec::with_capacity(min(shares.len(), threshold as usize));
-        let mut partial_secret = Vec::with_capacity(slen);
+        let mut col_in = Vec::with_capacity(threshold as usize);
         for byteindex in 0..slen {
             col_in.clear();
             for s in shares.iter().take(threshold as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            let partial_secret_byte = PartialSecret::new(threshold, &*col_in)?;
-            partial_secret.push(partial_secret_byte);
+            let partial_secret = PartialSecret::new(threshold, &col_in);
+            incremental_recovery.partial_secrets.push(partial_secret);
         }
 
-        Ok(partial_secret)
+        Ok(incremental_recovery)
     }
 
     /// Contines a partial secret recovery.
-    pub fn update_partial_secret(
-        partial_secret: &mut Vec<PartialSecret>,
-        shares: Vec<Share>,
-        verify_signatures: bool,
-    ) -> Result<()> {
-        if shares.is_empty() {
-            bail!(ErrorKind::EmptyShares);
-        } else if partial_secret.is_empty() {
-            bail!(ErrorKind::EmptyShares);
-        }
-        let threshold = partial_secret[0].threshold;
-        let shares_evaluated = partial_secret[0].shares_evaluated();
-        let shares_needed = partial_secret[0].shares_needed();
-        if shares_needed == 0 {
-            bail!(ErrorKind::InvalidShareCountMax(
-                (shares.len() + shares_evaluated) as u8,
-                threshold
-            ));
-        } else if shares.is_empty() {
-            bail!(ErrorKind::EmptyShares);
-        } else if shares_evaluated + shares.len() > MAX_SHARES as usize {
-            bail!(ErrorKind::InvalidShareCountMax(
-                (shares_evaluated + shares.len()) as u8,
-                MAX_SHARES
-            ));
-        }
-
-        let (threshold2, shares) = validate_signed_shares(shares, verify_signatures)?;
-        if threshold != threshold2 {
-            bail!(ErrorKind::InconsistentShares)
+    pub fn update(&mut self, shares: Vec<Share>) -> Result<()> {
+        if self.root_hash.is_some() {
+            let root_hash = self.root_hash.clone().unwrap();
+            self.ids = continue_signed_share_validation(
+                &shares,
+                &self.ids,
+                self.threshold,
+                self.slen,
+                Some(&root_hash),
+            )?;
+        } else {
+            self.ids = continue_signed_share_validation(
+                &shares,
+                &self.ids,
+                self.threshold,
+                self.slen,
+                None,
+            )?;
         }
 
-        let slen = shares[0].data.len();
-        let mut col_in = Vec::with_capacity(shares_needed);
-        for byteindex in 0..slen {
+        let mut col_in = Vec::with_capacity(self.threshold as usize);
+        for byteindex in 0..self.slen {
             col_in.clear();
-            for s in shares.iter().take(shares_needed) {
+            for s in shares.iter().take(self.shares_needed() as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            partial_secret[byteindex].update(&*col_in)?;
+            self.partial_secrets[byteindex].update(&*col_in);
         }
 
         Ok(())
     }
 
     /// Used to determine how many more shares are needed to finish computing a partial secret.
-    pub fn partial_secret_shares_needed(partial_secret: &Vec<PartialSecret>) -> Result<u8> {
-        if partial_secret.is_empty() {
-            bail!(ErrorKind::EmptyShares);
-        }
-        Ok(partial_secret[0].shares_needed() as u8)
+    pub fn shares_interpolated(&self) -> u8 {
+        // Safe indexing because `PartialSecret::new` ensures `self.partial_secrets` will be of
+        // length at least 1.
+        self.partial_secrets[0].shares_interpolated()
+    }
+
+    /// Used to determine how many more shares are needed to finish computing a partial secret.
+    pub fn shares_needed(&self) -> u8 {
+        // Safe indexing because `PartialSecret::new` ensures `self.partial_secrets` will be of
+        // length at least 1.
+        self.partial_secrets[0].shares_needed()
     }
 
     /// Used to obtain the resulting secret when `threshold` shares have been evaluated.
-    pub fn get_final_secret(partial_secret: &Vec<PartialSecret>) -> Result<Vec<u8>> {
-        if partial_secret.is_empty() {
-            bail!(ErrorKind::EmptyShares);
-        }
-        let threshold = partial_secret[0].threshold;
-        let shares_evaluated = partial_secret[0].shares_evaluated();
-        let shares_needed = partial_secret[0].shares_needed();
-        if shares_needed != 0 {
-            bail!(ErrorKind::MissingShares(
-                shares_evaluated,
-                threshold as usize
-            ));
+    pub fn get_secret(&self) -> Result<Vec<u8>> {
+        if self.shares_needed() != 0 {
+            bail!(ErrorKind::PartialInterpolationNotComplete(
+                self.threshold,
+                self.shares_interpolated()
+            ))
         }
         // Safe to unwrap because we already confirmed no more shares are needed.
-        Ok(partial_secret.iter().map(|ps| ps.secret.unwrap()).collect())
+        Ok(self.partial_secrets
+            .iter()
+            .map(|ps| ps.get_secret().unwrap())
+            .collect())
     }
 }
diff --git a/src/sss/share.rs b/src/sss/share.rs
index e3ff1ca1..fb93ef57 100644
--- a/src/sss/share.rs
+++ b/src/sss/share.rs
@@ -1,4 +1,3 @@
-use std::collections::{HashMap, HashSet};
 use std::error::Error;
 
 use merkle_sigs::verify_data_vec_signature;
@@ -87,59 +86,78 @@ impl IsShare for Share {
 impl IsSignedShare for Share {
     type Signature = Option<SignaturePair>;
 
-    fn verify_signatures(shares: &[Self]) -> Result<()> {
-        let mut rh_compatibility_sets = HashMap::new();
+    fn verify_signatures(shares: &[Self]) -> Result<Vec<u8>> {
+        Self::_verify_signatures(shares, None, None)
+    }
+
+    // NOTE: the function of `already_verified_ids` is to improve error messages. If you're
+    // specifying a `root_hash` argument, it's recommended to include a list of ids already
+    // verified against that root hash (if any).
+    fn continue_verify_signatures(
+        shares: &[Self],
+        root_hash: &Vec<u8>,
+        already_verified_ids: &Vec<u8>,
+    ) -> Result<()> {
+        Self::_verify_signatures(shares, Some(root_hash), Some(already_verified_ids))?;
+        Ok(())
+    }
+
+    fn is_signed(&self) -> bool {
+        self.signature_pair.is_some()
+    }
+
+    fn get_signature(&self) -> &Self::Signature {
+        &self.signature_pair
+    }
+}
+
+impl Share {
+    fn _verify_signatures(
+        shares: &[Self],
+        root_hash: Option<&Vec<u8>>,
+        already_verified_ids: Option<&Vec<u8>>,
+    ) -> Result<Vec<u8>> {
+        let shares_count = shares.len();
+        let mut root_hash = if root_hash.is_some() {
+            root_hash.unwrap().clone()
+        } else {
+            vec![]
+        };
+        let mut ids = if already_verified_ids.is_some() {
+            let mut ids_ = already_verified_ids.unwrap().clone();
+            ids_.reserve_exact(shares_count);
+            ids_
+        } else {
+            Vec::with_capacity(shares_count)
+        };
 
         for share in shares {
+            let id = share.get_id();
             if !share.is_signed() {
-                bail!(ErrorKind::MissingSignature(share.get_id()));
+                bail!(ErrorKind::MissingSignature(id));
             }
 
             let sig_pair = share.signature_pair.as_ref().unwrap();
             let signature = &sig_pair.signature;
             let proof = &sig_pair.proof;
-            let root_hash = &proof.root_hash;
+            let root_hash_ = &proof.root_hash;
 
             verify_data_vec_signature(
                 format_share_for_signing(share.threshold, share.id, share.data.as_slice()),
                 &(signature.to_vec(), proof.clone()),
-                root_hash,
+                &root_hash_,
             ).map_err(|e| ErrorKind::InvalidSignature(share.id, String::from(e.description())))?;
 
-            rh_compatibility_sets
-                .entry(root_hash)
-                .or_insert_with(HashSet::new);
-
-            let rh_set = rh_compatibility_sets.get_mut(&root_hash).unwrap();
-            rh_set.insert(share.id);
-        }
-
-        let rh_sets = rh_compatibility_sets.keys().count();
-
-        match rh_sets {
-            0 => bail!(ErrorKind::EmptyShares),
-            1 => {} // All shares have the same roothash.
-            _ => {
-                bail! {
-                    ErrorKind::IncompatibleSets(
-                        rh_compatibility_sets
-                            .values()
-                            .map(|x| x.to_owned())
-                            .collect(),
-                    )
-                }
+            if root_hash.is_empty() {
+                root_hash = root_hash_.clone();
+            } else if *root_hash_ != root_hash {
+                bail!(ErrorKind::InconsistentSignatures(id, ids))
             }
-        }
-
-        Ok(())
-    }
 
-    fn is_signed(&self) -> bool {
-        self.signature_pair.is_some()
-    }
+            ids.push(id);
+        }
 
-    fn get_signature(&self) -> &Self::Signature {
-        &self.signature_pair
+        Ok(root_hash)
     }
 }
 

From 85ed1e29d37c1f4fc11d5984eb2f418e67fef9a0 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Sun, 1 Apr 2018 15:13:22 -0500
Subject: [PATCH 33/42] Lints to new incremental recovery code

* Most of the changes were recommended by clippy. A few very small changes were
  my own initiative.
* Most common change using references instead of moving values when the value is
  not consumed by the function body.
* Using `&Vec<_>` instead of `&[_]` requires one more reference and cannot be
  used with non-Vec-based slices.
* Added clippy linter directives to the `Add` and `Subtract` implementations for
  `gf256`. The functions are fine, but use weird binary operators because
  XOR, add, and subtract are all the same GF(256). I had to add these to stop
  the linter from erroring out.
---
 src/errors.rs           |  2 +-
 src/gf256.rs            |  2 ++
 src/lagrange.rs         |  2 +-
 src/share/mod.rs        |  4 ++--
 src/share/validation.rs | 22 +++++++++++-----------
 src/sss/scheme.rs       | 10 +++++-----
 src/sss/share.rs        | 18 +++++++++---------
 7 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index c9021ef9..fa7af019 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -164,7 +164,7 @@ error_chain! {
 
 /// Takes a `Vec<T>` and formats it like the normal `fmt::Debug` implementation, unless it has more
 //than five elements, in which case the rest are replaced by ellipsis.
-fn no_more_than_five<T: fmt::Debug + fmt::Display>(vec: &Vec<T>) -> String {
+fn no_more_than_five<T: fmt::Debug + fmt::Display>(vec: &[T]) -> String {
     let len = vec.len();
     if len > 5 {
         let mut string = String::from("[");
diff --git a/src/gf256.rs b/src/gf256.rs
index 49ad57ea..10c7d1a4 100644
--- a/src/gf256.rs
+++ b/src/gf256.rs
@@ -66,6 +66,7 @@ impl Gf256 {
     }
 }
 
+#[allow(suspicious_arithmetic_impl)]
 impl Add<Gf256> for Gf256 {
     type Output = Gf256;
     #[inline]
@@ -81,6 +82,7 @@ impl AddAssign<Gf256> for Gf256 {
     }
 }
 
+#[allow(suspicious_arithmetic_impl)]
 impl Sub<Gf256> for Gf256 {
     type Output = Gf256;
     #[inline]
diff --git a/src/lagrange.rs b/src/lagrange.rs
index cc7dc0b8..0ce87dca 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -94,7 +94,7 @@ impl PartialSecret {
             return;
         }
 
-        let x = if self.weights.len() == 0 {
+        let x = if self.weights.is_empty() {
             // Initialize initial weights.
             self.weights = vec![Gf256::zero(); self.ids.len()];
             self.weights[0] = Gf256::one();
diff --git a/src/share/mod.rs b/src/share/mod.rs
index 49b39fb6..d68a8b91 100644
--- a/src/share/mod.rs
+++ b/src/share/mod.rs
@@ -43,7 +43,7 @@ pub(crate) trait IsSignedShare: IsShare {
     /// if verification fails.
     fn continue_verify_signatures(
         shares: &[Self],
-        root_hash: &Vec<u8>,
-        already_verified_ids: &Vec<u8>,
+        root_hash: &[u8],
+        already_verified_ids: &[u8],
     ) -> Result<()>;
 }
diff --git a/src/share/validation.rs b/src/share/validation.rs
index aa3e1182..89a13951 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -9,26 +9,26 @@ use share::{IsShare, IsSignedShare};
 
 /// TODO
 pub(crate) fn validate_signed_shares<S: IsSignedShare>(
-    shares: &Vec<S>,
+    shares: &[S],
     verify_signatures: bool,
 ) -> Result<(u8, usize)> {
     let result = validate_shares(shares)?;
 
     if verify_signatures {
-        S::verify_signatures(&shares)?;
+        S::verify_signatures(shares)?;
     };
 
     Ok(result)
 }
 
 pub(crate) fn begin_signed_share_validation<S: IsSignedShare>(
-    shares: &Vec<S>,
+    shares: &[S],
     verify_signatures: bool,
 ) -> Result<(u8, usize, Vec<u8>, Option<Vec<u8>>)> {
     let (threshold, slen, ids) = _validate_shares(shares, None, None, None)?;
 
     let root_hash = if verify_signatures {
-        Some(S::verify_signatures(&shares)?)
+        Some(S::verify_signatures(shares)?)
     } else {
         None
     };
@@ -37,11 +37,11 @@ pub(crate) fn begin_signed_share_validation<S: IsSignedShare>(
 }
 
 pub(crate) fn continue_signed_share_validation<S: IsSignedShare>(
-    shares: &Vec<S>,
-    already_verified_ids: &Vec<u8>,
+    shares: &[S],
+    already_verified_ids: &[u8],
     threshold: u8,
     slen: usize,
-    root_hash: Option<&Vec<u8>>,
+    root_hash: Option<&[u8]>,
 ) -> Result<(Vec<u8>)> {
     let (_, _, new_ids) = _validate_shares(
         shares,
@@ -58,7 +58,7 @@ pub(crate) fn continue_signed_share_validation<S: IsSignedShare>(
 }
 
 /// Validates a full set of shares.
-pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)> {
+pub(crate) fn validate_shares<S: IsShare>(shares: &[S]) -> Result<(u8, usize)> {
     let (threshold, slen, _) = _validate_shares(shares, None, None, None)?;
     let shares_count = shares.len();
     if shares_count < threshold as usize {
@@ -69,10 +69,10 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &Vec<S>) -> Result<(u8, usize)
 
 /// TODO: Doc
 fn _validate_shares<S: IsShare>(
-    shares: &Vec<S>,
+    shares: &[S],
     threshold: Option<u8>,
     slen: Option<usize>,
-    already_verified_ids: Option<&Vec<u8>>,
+    already_verified_ids: Option<&[u8]>,
 ) -> Result<(u8, usize, Vec<u8>)> {
     if shares.is_empty() {
         bail!(ErrorKind::EmptyShares);
@@ -80,7 +80,7 @@ fn _validate_shares<S: IsShare>(
 
     let shares_count = shares.len();
     let mut ids = if already_verified_ids.is_some() {
-        let mut ids = already_verified_ids.unwrap().clone();
+        let mut ids = already_verified_ids.unwrap().to_vec();
         ids.reserve_exact(shares_count);
         ids
     } else {
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index c0af3510..2d79b837 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -129,9 +129,9 @@ pub(crate) struct IncrementalRecovery {
 
 impl IncrementalRecovery {
     /// Begins a partial secret recovery.
-    pub fn new(shares: Vec<Share>, verify_signatures: bool) -> Result<Self> {
+    pub fn new(shares: &[Share], verify_signatures: bool) -> Result<Self> {
         let (threshold, slen, ids, root_hash) =
-            begin_signed_share_validation(&shares, verify_signatures)?;
+            begin_signed_share_validation(shares, verify_signatures)?;
 
         let mut incremental_recovery = Self {
             partial_secrets: Vec::with_capacity(slen),
@@ -155,11 +155,11 @@ impl IncrementalRecovery {
     }
 
     /// Contines a partial secret recovery.
-    pub fn update(&mut self, shares: Vec<Share>) -> Result<()> {
+    pub fn update(&mut self, shares: &[Share]) -> Result<()> {
         if self.root_hash.is_some() {
             let root_hash = self.root_hash.clone().unwrap();
             self.ids = continue_signed_share_validation(
-                &shares,
+                shares,
                 &self.ids,
                 self.threshold,
                 self.slen,
@@ -167,7 +167,7 @@ impl IncrementalRecovery {
             )?;
         } else {
             self.ids = continue_signed_share_validation(
-                &shares,
+                shares,
                 &self.ids,
                 self.threshold,
                 self.slen,
diff --git a/src/sss/share.rs b/src/sss/share.rs
index fb93ef57..611ee957 100644
--- a/src/sss/share.rs
+++ b/src/sss/share.rs
@@ -95,8 +95,8 @@ impl IsSignedShare for Share {
     // verified against that root hash (if any).
     fn continue_verify_signatures(
         shares: &[Self],
-        root_hash: &Vec<u8>,
-        already_verified_ids: &Vec<u8>,
+        root_hash: &[u8],
+        already_verified_ids: &[u8],
     ) -> Result<()> {
         Self::_verify_signatures(shares, Some(root_hash), Some(already_verified_ids))?;
         Ok(())
@@ -114,19 +114,19 @@ impl IsSignedShare for Share {
 impl Share {
     fn _verify_signatures(
         shares: &[Self],
-        root_hash: Option<&Vec<u8>>,
-        already_verified_ids: Option<&Vec<u8>>,
+        root_hash: Option<&[u8]>,
+        already_verified_ids: Option<&[u8]>,
     ) -> Result<Vec<u8>> {
         let shares_count = shares.len();
         let mut root_hash = if root_hash.is_some() {
-            root_hash.unwrap().clone()
+            root_hash.unwrap().to_vec()
         } else {
             vec![]
         };
         let mut ids = if already_verified_ids.is_some() {
-            let mut ids_ = already_verified_ids.unwrap().clone();
-            ids_.reserve_exact(shares_count);
-            ids_
+            let mut ids = already_verified_ids.unwrap().to_vec();
+            ids.reserve_exact(shares_count);
+            ids
         } else {
             Vec::with_capacity(shares_count)
         };
@@ -145,7 +145,7 @@ impl Share {
             verify_data_vec_signature(
                 format_share_for_signing(share.threshold, share.id, share.data.as_slice()),
                 &(signature.to_vec(), proof.clone()),
-                &root_hash_,
+                root_hash_,
             ).map_err(|e| ErrorKind::InvalidSignature(share.id, String::from(e.description())))?;
 
             if root_hash.is_empty() {

From de0250b8c987cd15478f797dc612ea492d610ce0 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Sun, 1 Apr 2018 15:30:15 -0500
Subject: [PATCH 34/42] Remove `interpolate_at`, work directly with
 `PartialSecret`

Since `interpolate_at` had been reduced to a 2-line convenience function used in
a single place, I thought it better to simply move those two lines to that
single place.
---
 src/lagrange.rs   | 17 +++++------------
 src/sss/scheme.rs | 10 ++++++----
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index 0ce87dca..6f906196 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -4,14 +4,6 @@ use errors::*;
 use gf256::Gf256;
 use poly::Poly;
 
-/// Evaluates an interpolated polynomial at `Gf256::zero()` where the polynomial is determined
-/// using barycentric Lagrange interpolation based on the given `points` in the G(2^8) Galois
-/// field.
-pub(crate) fn interpolate_at(threshold: u8, points: &[(u8, u8)]) -> Result<u8> {
-    let partial_comp = PartialSecret::new(threshold, points);
-    Ok(partial_comp.get_secret().unwrap())
-}
-
 /// Stores the intermediate state of interpolation and evaluation at `Gf256::zero()` of a
 /// polynomial. A secret may be computed incrementally using barycentric Lagrange interpolation.
 /// The state is updated with new points until threshold points have been evaluated, at which point
@@ -290,11 +282,12 @@ mod tests {
                 .collect::<Vec<_>>();
 
             let poly = interpolate(&elems);
+            let result_poly = poly.evaluate_at(Gf256::zero()).to_byte();
+            // Safe to cast because if `ys.len() > 255` it is discarded.
+            let interpolation = PartialSecret::new(points.len() as u8, &points);
+            let result_interpolate = interpolation.get_secret().unwrap();
 
-            let equals = poly.evaluate_at(Gf256::zero()).to_byte()
-                == interpolate_at(points.len() as u8, points.as_slice()).unwrap();
-
-            TestResult::from_bool(equals)
+            TestResult::from_bool(result_poly == result_interpolate)
         }
 
     }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 2d79b837..233a444d 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -4,7 +4,7 @@ use merkle_sigs::sign_data_vec;
 use rand::{OsRng, Rng};
 
 use errors::*;
-use lagrange::{interpolate_at, PartialSecret};
+use lagrange::PartialSecret;
 use share::validation::{begin_signed_share_validation, continue_signed_share_validation,
                         validate_share_count, validate_signed_shares};
 use sss::format::format_share_for_signing;
@@ -103,8 +103,10 @@ impl SSS {
             for s in shares.iter().take(threshold as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            let secret_byte = interpolate_at(threshold, &*col_in)?;
-            secret.push(secret_byte);
+            let secret_byte_computation = PartialSecret::new(threshold, &col_in);
+            // Safe to unwrap because we have already validated we are providing `PartialSecret`
+            // with `threshold` secret bytes in `col_in`.
+            secret.push(secret_byte_computation.get_secret().unwrap());
         }
 
         Ok(secret)
@@ -181,7 +183,7 @@ impl IncrementalRecovery {
             for s in shares.iter().take(self.shares_needed() as usize) {
                 col_in.push((s.id, s.data[byteindex]));
             }
-            self.partial_secrets[byteindex].update(&*col_in);
+            self.partial_secrets[byteindex].update(&col_in);
         }
 
         Ok(())

From dc8b436fcc365adf93d750d199ebb5a0d92f071f Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Mon, 2 Apr 2018 17:15:07 -0500
Subject: [PATCH 35/42] Simplify verification

Temporarily error messages from `verify_signatures` will suffer, but this will
be rectified soon using `error_chain`.
---
 src/share/mod.rs        | 16 +++---------
 src/share/validation.rs | 21 ++++++++-------
 src/sss/scheme.rs       |  8 +++---
 src/sss/share.rs        | 57 +++++++++--------------------------------
 4 files changed, 30 insertions(+), 72 deletions(-)

diff --git a/src/share/mod.rs b/src/share/mod.rs
index d68a8b91..7f3e8868 100644
--- a/src/share/mod.rs
+++ b/src/share/mod.rs
@@ -34,16 +34,8 @@ pub(crate) trait IsSignedShare: IsShare {
     /// Return the signature itself.
     fn get_signature(&self) -> &Self::Signature;
 
-    /// Verify a given batch of shares are all signed by the same root hash. Returns the root hash
-    /// if verification succeeds, and an `Err` otherwise.
-    fn verify_signatures(shares: &[Self]) -> Result<Vec<u8>>;
-
-    /// Verify the `shares` all have valid signatures from the `root_hash`. Pass a list of shares
-    /// identifiers already verified against this `root_hash`, if any, for better error messaging
-    /// if verification fails.
-    fn continue_verify_signatures(
-        shares: &[Self],
-        root_hash: &[u8],
-        already_verified_ids: &[u8],
-    ) -> Result<()>;
+    /// Verify a given batch of shares are all signed by the same root hash, optionally specifying
+    /// which one using the `root_hash` argument. Returns the root hash if verification succeeds,
+    /// and an `Err` otherwise.
+    fn verify_signatures(shares: &[Self], root_hash: Option<&[u8]>) -> Result<Vec<u8>>;
 }
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 89a13951..64e87d9e 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -15,20 +15,20 @@ pub(crate) fn validate_signed_shares<S: IsSignedShare>(
     let result = validate_shares(shares)?;
 
     if verify_signatures {
-        S::verify_signatures(shares)?;
+        S::verify_signatures(shares, None)?;
     };
 
     Ok(result)
 }
 
-pub(crate) fn begin_signed_share_validation<S: IsSignedShare>(
+pub(crate) fn begin_partial_signed_share_validation<S: IsSignedShare>(
     shares: &[S],
     verify_signatures: bool,
 ) -> Result<(u8, usize, Vec<u8>, Option<Vec<u8>>)> {
-    let (threshold, slen, ids) = _validate_shares(shares, None, None, None)?;
+    let (threshold, slen, ids) = partial_validate_shares(shares, None, None, None)?;
 
     let root_hash = if verify_signatures {
-        Some(S::verify_signatures(shares)?)
+        Some(S::verify_signatures(shares, None)?)
     } else {
         None
     };
@@ -36,14 +36,14 @@ pub(crate) fn begin_signed_share_validation<S: IsSignedShare>(
     Ok((threshold, slen, ids, root_hash))
 }
 
-pub(crate) fn continue_signed_share_validation<S: IsSignedShare>(
+pub(crate) fn continue_partial_signed_share_validation<S: IsSignedShare>(
     shares: &[S],
     already_verified_ids: &[u8],
     threshold: u8,
     slen: usize,
     root_hash: Option<&[u8]>,
 ) -> Result<(Vec<u8>)> {
-    let (_, _, new_ids) = _validate_shares(
+    let (_, _, new_ids) = partial_validate_shares(
         shares,
         Some(threshold),
         Some(slen),
@@ -51,7 +51,7 @@ pub(crate) fn continue_signed_share_validation<S: IsSignedShare>(
     )?;
 
     if root_hash.is_some() {
-        S::continue_verify_signatures(shares, root_hash.unwrap(), already_verified_ids)?;
+        S::verify_signatures(shares, root_hash)?;
     }
 
     Ok(new_ids)
@@ -59,7 +59,7 @@ pub(crate) fn continue_signed_share_validation<S: IsSignedShare>(
 
 /// Validates a full set of shares.
 pub(crate) fn validate_shares<S: IsShare>(shares: &[S]) -> Result<(u8, usize)> {
-    let (threshold, slen, _) = _validate_shares(shares, None, None, None)?;
+    let (threshold, slen, _) = partial_validate_shares(shares, None, None, None)?;
     let shares_count = shares.len();
     if shares_count < threshold as usize {
         bail!(ErrorKind::MissingShares(shares_count, threshold))
@@ -67,13 +67,12 @@ pub(crate) fn validate_shares<S: IsShare>(shares: &[S]) -> Result<(u8, usize)> {
     Ok((threshold, slen))
 }
 
-/// TODO: Doc
-fn _validate_shares<S: IsShare>(
+pub(crate) fn partial_validate_shares<S: IsShare>(
     shares: &[S],
     threshold: Option<u8>,
     slen: Option<usize>,
     already_verified_ids: Option<&[u8]>,
-) -> Result<(u8, usize, Vec<u8>)> {
+)-> Result<(u8, usize, Vec<u8>)> {
     if shares.is_empty() {
         bail!(ErrorKind::EmptyShares);
     }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 233a444d..fa906220 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -5,7 +5,7 @@ use rand::{OsRng, Rng};
 
 use errors::*;
 use lagrange::PartialSecret;
-use share::validation::{begin_signed_share_validation, continue_signed_share_validation,
+use share::validation::{begin_partial_signed_share_validation, continue_partial_signed_share_validation,
                         validate_share_count, validate_signed_shares};
 use sss::format::format_share_for_signing;
 use sss::{Share, HASH_ALGO};
@@ -133,7 +133,7 @@ impl IncrementalRecovery {
     /// Begins a partial secret recovery.
     pub fn new(shares: &[Share], verify_signatures: bool) -> Result<Self> {
         let (threshold, slen, ids, root_hash) =
-            begin_signed_share_validation(shares, verify_signatures)?;
+            begin_partial_signed_share_validation(shares, verify_signatures)?;
 
         let mut incremental_recovery = Self {
             partial_secrets: Vec::with_capacity(slen),
@@ -160,7 +160,7 @@ impl IncrementalRecovery {
     pub fn update(&mut self, shares: &[Share]) -> Result<()> {
         if self.root_hash.is_some() {
             let root_hash = self.root_hash.clone().unwrap();
-            self.ids = continue_signed_share_validation(
+            self.ids = continue_partial_signed_share_validation(
                 shares,
                 &self.ids,
                 self.threshold,
@@ -168,7 +168,7 @@ impl IncrementalRecovery {
                 Some(&root_hash),
             )?;
         } else {
-            self.ids = continue_signed_share_validation(
+            self.ids = continue_partial_signed_share_validation(
                 shares,
                 &self.ids,
                 self.threshold,
diff --git a/src/sss/share.rs b/src/sss/share.rs
index 611ee957..23849ea1 100644
--- a/src/sss/share.rs
+++ b/src/sss/share.rs
@@ -86,50 +86,9 @@ impl IsShare for Share {
 impl IsSignedShare for Share {
     type Signature = Option<SignaturePair>;
 
-    fn verify_signatures(shares: &[Self]) -> Result<Vec<u8>> {
-        Self::_verify_signatures(shares, None, None)
-    }
-
-    // NOTE: the function of `already_verified_ids` is to improve error messages. If you're
-    // specifying a `root_hash` argument, it's recommended to include a list of ids already
-    // verified against that root hash (if any).
-    fn continue_verify_signatures(
-        shares: &[Self],
-        root_hash: &[u8],
-        already_verified_ids: &[u8],
-    ) -> Result<()> {
-        Self::_verify_signatures(shares, Some(root_hash), Some(already_verified_ids))?;
-        Ok(())
-    }
-
-    fn is_signed(&self) -> bool {
-        self.signature_pair.is_some()
-    }
-
-    fn get_signature(&self) -> &Self::Signature {
-        &self.signature_pair
-    }
-}
-
-impl Share {
-    fn _verify_signatures(
-        shares: &[Self],
-        root_hash: Option<&[u8]>,
-        already_verified_ids: Option<&[u8]>,
-    ) -> Result<Vec<u8>> {
-        let shares_count = shares.len();
-        let mut root_hash = if root_hash.is_some() {
-            root_hash.unwrap().to_vec()
-        } else {
-            vec![]
-        };
-        let mut ids = if already_verified_ids.is_some() {
-            let mut ids = already_verified_ids.unwrap().to_vec();
-            ids.reserve_exact(shares_count);
-            ids
-        } else {
-            Vec::with_capacity(shares_count)
-        };
+    fn verify_signatures(shares: &[Self], root_hash: Option<&[u8]>) -> Result<Vec<u8>> {
+        let mut root_hash = root_hash.unwrap_or(&[]).to_vec();
+        let mut ids = Vec::with_capacity(shares.len());
 
         for share in shares {
             let id = share.get_id();
@@ -143,7 +102,7 @@ impl Share {
             let root_hash_ = &proof.root_hash;
 
             verify_data_vec_signature(
-                format_share_for_signing(share.threshold, share.id, share.data.as_slice()),
+                format_share_for_signing(share.threshold, id, &share.data),
                 &(signature.to_vec(), proof.clone()),
                 root_hash_,
             ).map_err(|e| ErrorKind::InvalidSignature(share.id, String::from(e.description())))?;
@@ -159,6 +118,14 @@ impl Share {
 
         Ok(root_hash)
     }
+
+    fn is_signed(&self) -> bool {
+        self.signature_pair.is_some()
+    }
+
+    fn get_signature(&self) -> &Self::Signature {
+        &self.signature_pair
+    }
 }
 
 #[derive(Clone, Debug)]

From 4998e5ba4ba7a1881a84b89990a9e899aa0ea6d7 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Mon, 2 Apr 2018 20:06:56 -0500
Subject: [PATCH 36/42] Refactor share::validation and sss::scheme modules

Work still to be done: decide how to refactor the validation module once more so
that we don't have to separately check that we have threshold shares (see TODOs
in diff).
---
 src/dss/ss1/scheme.rs         |   9 +-
 src/dss/thss/scheme.rs        |   9 +-
 src/errors.rs                 |  10 +-
 src/lagrange.rs               |  18 +--
 src/share/validation.rs       |  82 +++--------
 src/sss/mod.rs                |   9 +-
 src/sss/scheme.rs             | 247 ++++++++++++++++------------------
 src/wrapped_secrets/scheme.rs |   6 +-
 tests/test_vectors.rs         |   2 +-
 9 files changed, 168 insertions(+), 224 deletions(-)

diff --git a/src/dss/ss1/scheme.rs b/src/dss/ss1/scheme.rs
index 49c9e4e6..c96619e7 100644
--- a/src/dss/ss1/scheme.rs
+++ b/src/dss/ss1/scheme.rs
@@ -248,7 +248,14 @@ impl SS1 {
         shares: &[Share],
     ) -> Result<(Vec<u8>, AccessStructure, Option<MetaData>)> {
         let shares = shares.to_vec();
-        validate_shares(&shares)?;
+        let (threshold, _) = validate_shares(&shares, None, None, None)?;
+        // TODO: rewrite validation mod, so we can nix this `if` statement.
+        if shares.len() < threshold as usize {
+            bail!(ErrorKind::MissingShares(
+                shares.len() as u8,
+                threshold
+            ))
+        }
 
         let underlying_shares = shares
             .iter()
diff --git a/src/dss/thss/scheme.rs b/src/dss/thss/scheme.rs
index e7d0d1e2..bc05c92e 100644
--- a/src/dss/thss/scheme.rs
+++ b/src/dss/thss/scheme.rs
@@ -90,7 +90,14 @@ impl ThSS {
         shares: &[Share],
     ) -> Result<(Vec<u8>, AccessStructure, Option<MetaData>)> {
         let shares = shares.to_vec();
-        let (threshold, cypher_len) = validate_shares(&shares)?;
+        let (threshold, cypher_len) = validate_shares(&shares, None, None, None)?;
+        // TODO: rewrite validation mod, so we can nix this `if` statement.
+        if shares.len() < threshold as usize {
+            bail!(ErrorKind::MissingShares(
+                shares.len() as u8,
+                threshold
+            ))
+        }
 
         let polys = (0..cypher_len)
             .map(|i| {
diff --git a/src/errors.rs b/src/errors.rs
index fa7af019..841d9623 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -60,7 +60,7 @@ error_chain! {
             display("The shares are incompatible with each other.")
         }
 
-        MissingShares(provided: usize, required: u8) {
+        MissingShares(provided: u8, 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)
         }
@@ -150,10 +150,10 @@ error_chain! {
             display("The share identifier {} had k = {}, while k = {} was found for share identifier(s): {}.", id, k_, k, no_more_than_five(ids))
         }
 
-        PartialInterpolationNotComplete(k: u8, shares_interpolated: u8) {
-            description("The partial interpolation result is not complete because the number of points interpolated has not reached the threshold.")
-            display("In order to evaluate the secret polynomial at any point k = {} shares are needed, whereas only {} have been provided.", k, shares_interpolated)
-        }
+        // PartialInterpolationNotComplete(k: u8, shares_interpolated: u8) {
+        //     description("The partial interpolation result is not complete because the number of points interpolated has not reached the threshold.")
+        //     display("In order to evaluate the secret polynomial at any point k = {} shares are needed, whereas only {} have been provided.", k, shares_interpolated)
+        // }
     }
 
     foreign_links {
diff --git a/src/lagrange.rs b/src/lagrange.rs
index 6f906196..83fa26a3 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -136,21 +136,15 @@ impl PartialSecret {
     #[inline]
     pub fn get_secret(&self) -> Result<u8> {
         if self.secret.is_none() {
-            bail!(ErrorKind::PartialInterpolationNotComplete(
-                self.threshold,
-                self.shares_interpolated()
+            bail!(ErrorKind::MissingShares(
+                self.shares_interpolated(),
+                self.threshold
             ))
         }
         // Safe to unwrap because we just confirmed it's not `None`.
         Ok(self.secret.unwrap())
     }
 
-    /// Returns the threshold for the partial computation.
-    #[inline]
-    fn get_threshold(&self) -> u8 {
-        self.threshold
-    }
-
     /// Returns the number of shares needed to complete the computation.
     #[inline]
     pub fn shares_needed(&self) -> u8 {
@@ -172,9 +166,9 @@ impl PartialSecret {
     #[inline]
     fn evaluate_at_x(&self, x: u8) -> Result<u8> {
         if self.shares_needed() != 0 {
-            bail!(ErrorKind::PartialInterpolationNotComplete(
-                self.threshold,
-                self.shares_interpolated()
+            bail!(ErrorKind::MissingShares(
+                self.shares_interpolated(),
+                self.threshold
             ))
         }
 
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 64e87d9e..dc35501d 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -6,73 +6,34 @@ use share::{IsShare, IsSignedShare};
 // 2) Validate duplicate shares share num && data
 // 2) Validate group consistency
 // 3) Validate other properties, in no specific order
-
-/// TODO
 pub(crate) fn validate_signed_shares<S: IsSignedShare>(
     shares: &[S],
     verify_signatures: bool,
-) -> Result<(u8, usize)> {
-    let result = validate_shares(shares)?;
-
-    if verify_signatures {
-        S::verify_signatures(shares, None)?;
-    };
-
-    Ok(result)
-}
-
-pub(crate) fn begin_partial_signed_share_validation<S: IsSignedShare>(
-    shares: &[S],
-    verify_signatures: bool,
-) -> Result<(u8, usize, Vec<u8>, Option<Vec<u8>>)> {
-    let (threshold, slen, ids) = partial_validate_shares(shares, None, None, None)?;
+    threshold: Option<u8>,
+    slen: Option<usize>,
+    already_verified_ids: Option<&[u8]>,
+    root_hash: Option<&[u8]>,
+) -> Result<(u8, usize, Option<Vec<u8>>)> {
+    let (threshold, slen) = validate_shares(shares, threshold, slen, already_verified_ids)?;
 
-    let root_hash = if verify_signatures {
-        Some(S::verify_signatures(shares, None)?)
+    let rhash = if root_hash.is_some() || verify_signatures {
+        if !verify_signatures {
+            // TODO: =><=
+        }
+        Some(S::verify_signatures(shares, root_hash)?)
     } else {
         None
     };
 
-    Ok((threshold, slen, ids, root_hash))
-}
-
-pub(crate) fn continue_partial_signed_share_validation<S: IsSignedShare>(
-    shares: &[S],
-    already_verified_ids: &[u8],
-    threshold: u8,
-    slen: usize,
-    root_hash: Option<&[u8]>,
-) -> Result<(Vec<u8>)> {
-    let (_, _, new_ids) = partial_validate_shares(
-        shares,
-        Some(threshold),
-        Some(slen),
-        Some(already_verified_ids),
-    )?;
-
-    if root_hash.is_some() {
-        S::verify_signatures(shares, root_hash)?;
-    }
-
-    Ok(new_ids)
-}
-
-/// Validates a full set of shares.
-pub(crate) fn validate_shares<S: IsShare>(shares: &[S]) -> Result<(u8, usize)> {
-    let (threshold, slen, _) = partial_validate_shares(shares, None, None, None)?;
-    let shares_count = shares.len();
-    if shares_count < threshold as usize {
-        bail!(ErrorKind::MissingShares(shares_count, threshold))
-    }
-    Ok((threshold, slen))
+    Ok((threshold, slen, rhash))
 }
 
-pub(crate) fn partial_validate_shares<S: IsShare>(
+pub(crate) fn validate_shares<S: IsShare>(
     shares: &[S],
     threshold: Option<u8>,
     slen: Option<usize>,
     already_verified_ids: Option<&[u8]>,
-)-> Result<(u8, usize, Vec<u8>)> {
+) -> Result<(u8, usize)> {
     if shares.is_empty() {
         bail!(ErrorKind::EmptyShares);
     }
@@ -85,8 +46,9 @@ pub(crate) fn partial_validate_shares<S: IsShare>(
     } else {
         Vec::with_capacity(shares_count)
     };
-    let mut threshold = threshold.unwrap_or(0);
-    let mut slen = slen.unwrap_or(0);
+    // Safe to index since we already confirmed `shares` is nonempty.
+    let threshold = threshold.unwrap_or_else(|| shares[0].get_threshold());
+    let slen = slen.unwrap_or_else(|| shares[0].get_data().len());
 
     for share in shares {
         let id = share.get_id();
@@ -106,10 +68,6 @@ pub(crate) fn partial_validate_shares<S: IsShare>(
 
         if ids.iter().any(|&x| x == id) {
             bail!(ErrorKind::DuplicateShareId(id));
-        }
-
-        if threshold == 0 {
-            threshold = threshold_;
         } else if threshold_ != threshold {
             bail!(ErrorKind::InconsistentThresholds(
                 id,
@@ -117,10 +75,6 @@ pub(crate) fn partial_validate_shares<S: IsShare>(
                 ids,
                 threshold
             ))
-        }
-
-        if slen == 0 {
-            slen = slen_;
         } else if slen_ != slen {
             bail!(ErrorKind::InconsistentSecretLengths(id, slen_, ids, slen))
         }
@@ -128,7 +82,7 @@ pub(crate) fn partial_validate_shares<S: IsShare>(
         ids.push(id);
     }
 
-    Ok((threshold, slen, ids))
+    Ok((threshold, slen))
 }
 
 pub(crate) fn validate_share_count(threshold: u8, shares_count: u8) -> Result<(u8, u8)> {
diff --git a/src/sss/mod.rs b/src/sss/mod.rs
index 7709d1b1..667380e4 100644
--- a/src/sss/mod.rs
+++ b/src/sss/mod.rs
@@ -8,8 +8,8 @@ pub(crate) use self::share::*;
 mod format;
 // pub use self::format::*;
 
-mod scheme;
-pub(crate) use self::scheme::*;
+pub(crate) mod scheme;
+use self::scheme::Recover;
 
 mod encode;
 
@@ -36,8 +36,7 @@ static HASH_ALGO: &'static Algorithm = &SHA512;
 /// }
 /// ```
 pub fn split_secret(k: u8, n: u8, secret: &[u8], sign_shares: bool) -> Result<Vec<String>> {
-    SSS::default()
-        .split_secret(k, n, secret, sign_shares)
+    scheme::split_secret(k, n, secret, sign_shares)
         .map(|shares| shares.into_iter().map(Share::into_string).collect())
 }
 
@@ -65,5 +64,5 @@ pub fn split_secret(k: u8, n: u8, secret: &[u8], sign_shares: bool) -> Result<Ve
 /// ```
 pub fn recover_secret(shares: &[String], verify_signatures: bool) -> Result<Vec<u8>> {
     let shares = Share::parse_all(shares, verify_signatures)?;
-    SSS::recover_secret(shares, verify_signatures)
+    Recover::recover_secret(&shares, verify_signatures)
 }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index fa906220..17fa5778 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -1,121 +1,91 @@
-//! SSS provides Shamir's secret sharing with raw data.
+//! Provides Shamir's secret sharing with raw data.
 
 use merkle_sigs::sign_data_vec;
 use rand::{OsRng, Rng};
 
 use errors::*;
 use lagrange::PartialSecret;
-use share::validation::{begin_partial_signed_share_validation, continue_partial_signed_share_validation,
-                        validate_share_count, validate_signed_shares};
+use share::IsShare;
+use share::validation::{validate_share_count, validate_signed_shares};
 use sss::format::format_share_for_signing;
 use sss::{Share, HASH_ALGO};
 
 use super::encode::encode_secret_byte;
 
-/// SSS provides Shamir's secret sharing with raw data.
-#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)]
-pub(crate) struct SSS;
-
-impl SSS {
-    /// Performs threshold k-out-of-n Shamir's secret sharing.
-    pub fn split_secret(
-        &self,
-        threshold: u8,
-        shares_count: u8,
-        secret: &[u8],
-        sign_shares: bool,
-    ) -> Result<Vec<Share>> {
-        let (threshold, shares_count) = validate_share_count(threshold, shares_count)?;
-        let shares = Self::secret_share(secret, threshold, shares_count)?;
-
-        let signatures = if sign_shares {
-            let shares_to_sign = shares
-                .iter()
-                .enumerate()
-                .map(|(i, x)| format_share_for_signing(threshold, (i + 1) as u8, x))
-                .collect::<Vec<_>>();
-
-            let sign = sign_data_vec(&shares_to_sign, HASH_ALGO)
-                .unwrap()
-                .into_iter()
-                .map(Some)
-                .collect::<Vec<_>>();
-
-            Some(sign)
-        } else {
-            None
-        };
+/// Performs threshold k-out-of-n Shamir's secret sharing.
+pub(crate) fn split_secret(
+    threshold: u8,
+    shares_count: u8,
+    secret: &[u8],
+    sign_shares: bool,
+) -> Result<Vec<Share>> {
+    let (threshold, shares_count) = validate_share_count(threshold, shares_count)?;
+    let shares = secret_share(secret, threshold, shares_count)?;
+
+    let signatures = if sign_shares {
+        let shares_to_sign = shares
+            .iter()
+            .enumerate()
+            .map(|(i, x)| format_share_for_signing(threshold, (i + 1) as u8, x))
+            .collect::<Vec<_>>();
 
-        let sig_pairs = signatures
-            .unwrap_or_else(|| vec![None; shares_count as usize])
+        let sign = sign_data_vec(&shares_to_sign, HASH_ALGO)
+            .unwrap()
             .into_iter()
-            .map(|sig_pair| sig_pair.map(From::from));
+            .map(Some)
+            .collect::<Vec<_>>();
 
-        let shares_and_sigs = shares.into_iter().enumerate().zip(sig_pairs);
+        Some(sign)
+    } else {
+        None
+    };
 
-        let result = shares_and_sigs.map(|((index, data), signature_pair)| {
-            // This is actually safe since we alwaays generate less than 256 shares.
-            let id = (index + 1) as u8;
+    let sig_pairs = signatures
+        .unwrap_or_else(|| vec![None; shares_count as usize])
+        .into_iter()
+        .map(|sig_pair| sig_pair.map(From::from));
 
-            Share {
-                id,
-                threshold,
-                data,
-                signature_pair,
-            }
-        });
+    let shares_and_sigs = shares.into_iter().enumerate().zip(sig_pairs);
 
-        Ok(result.collect())
-    }
+    let result = shares_and_sigs.map(|((index, data), signature_pair)| {
+        // This is actually safe since we alwaays generate less than 256 shares.
+        let id = (index + 1) as u8;
 
-    fn secret_share(src: &[u8], threshold: u8, shares_count: u8) -> Result<Vec<Vec<u8>>> {
-        let mut result = Vec::with_capacity(shares_count as usize);
-        for _ in 0..(shares_count as usize) {
-            result.push(vec![0u8; src.len()]);
-        }
-        let mut col_in = vec![0u8; threshold as usize];
-        let mut col_out = Vec::with_capacity(shares_count as usize);
-        let mut osrng = OsRng::new()?;
-        for (c, &s) in src.iter().enumerate() {
-            col_in[0] = s;
-            // NOTE: switch to `try_fill_bytes` when it lands in a stable release:
-            // https://github.com/rust-lang-nursery/rand/commit/230b2258dbd99ff8bd991008c972d923d4b5d10c
-            osrng.fill_bytes(&mut col_in[1..]);
-            col_out.clear();
-            encode_secret_byte(&*col_in, shares_count, &mut col_out)?;
-            for (&y, share) in col_out.iter().zip(result.iter_mut()) {
-                share[c] = y;
-            }
+        Share {
+            id,
+            threshold,
+            data,
+            signature_pair,
         }
-        Ok(result)
-    }
+    });
 
-    /// Recovers the secret from a k-out-of-n Shamir's secret sharing.
-    ///
-    /// At least `k` distinct shares need to be provided to recover the share.
-    pub fn recover_secret(shares: Vec<Share>, verify_signatures: bool) -> Result<Vec<u8>> {
-        let (threshold, slen) = validate_signed_shares(&shares, verify_signatures)?;
-
-        let mut col_in = Vec::with_capacity(threshold as usize);
-        let mut secret = Vec::with_capacity(slen);
-        for byteindex in 0..slen {
-            col_in.clear();
-            for s in shares.iter().take(threshold as usize) {
-                col_in.push((s.id, s.data[byteindex]));
-            }
-            let secret_byte_computation = PartialSecret::new(threshold, &col_in);
-            // Safe to unwrap because we have already validated we are providing `PartialSecret`
-            // with `threshold` secret bytes in `col_in`.
-            secret.push(secret_byte_computation.get_secret().unwrap());
-        }
+    Ok(result.collect())
+}
 
-        Ok(secret)
+fn secret_share(src: &[u8], threshold: u8, shares_count: u8) -> Result<Vec<Vec<u8>>> {
+    let mut result = Vec::with_capacity(shares_count as usize);
+    for _ in 0..(shares_count as usize) {
+        result.push(vec![0u8; src.len()]);
     }
+    let mut col_in = vec![0u8; threshold as usize];
+    let mut col_out = Vec::with_capacity(shares_count as usize);
+    let mut osrng = OsRng::new()?;
+    for (c, &s) in src.iter().enumerate() {
+        col_in[0] = s;
+        // NOTE: switch to `try_fill_bytes` when it lands in a stable release:
+        // https://github.com/rust-lang-nursery/rand/commit/230b2258dbd99ff8bd991008c972d923d4b5d10c
+        osrng.fill_bytes(&mut col_in[1..]);
+        col_out.clear();
+        encode_secret_byte(&*col_in, shares_count, &mut col_out)?;
+        for (&y, share) in col_out.iter().zip(result.iter_mut()) {
+            share[c] = y;
+        }
+    }
+    Ok(result)
 }
 
-/// `IncrementalRecovery` provides a way to incrementally recover a secret in cases where not all
-/// shares are available at once.
-pub(crate) struct IncrementalRecovery {
+/// `Recover` provides an interface for recovering a secret.
+pub(crate) struct Recover {
     /// The state of each partially-recovered secret byte.
     partial_secrets: Vec<PartialSecret>,
     /// The ids of the share (varies between 1 and n where n is the total number of generated
@@ -129,29 +99,29 @@ pub(crate) struct IncrementalRecovery {
     root_hash: Option<Vec<u8>>,
 }
 
-impl IncrementalRecovery {
+impl Recover {
+    /// Recovers the secret from a k-out-of-n Shamir's secret sharing.
+    ///
+    /// At least `k` distinct shares need to be provided to recover the share.
+    pub fn recover_secret(shares: &[Share], verify_signatures: bool) -> Result<Vec<u8>> {
+        let recovery = Self::new(shares, verify_signatures)?;
+        recovery.get_secret()
+    }
+
     /// Begins a partial secret recovery.
     pub fn new(shares: &[Share], verify_signatures: bool) -> Result<Self> {
-        let (threshold, slen, ids, root_hash) =
-            begin_partial_signed_share_validation(shares, verify_signatures)?;
+        let (threshold, slen, root_hash) =
+            validate_signed_shares(shares, verify_signatures, None, None, None, None)?;
 
         let mut incremental_recovery = Self {
             partial_secrets: Vec::with_capacity(slen),
-            ids,
+            ids: Vec::with_capacity(threshold as usize),
             threshold,
             slen,
             root_hash,
         };
 
-        let mut col_in = Vec::with_capacity(threshold as usize);
-        for byteindex in 0..slen {
-            col_in.clear();
-            for s in shares.iter().take(threshold as usize) {
-                col_in.push((s.id, s.data[byteindex]));
-            }
-            let partial_secret = PartialSecret::new(threshold, &col_in);
-            incremental_recovery.partial_secrets.push(partial_secret);
-        }
+        incremental_recovery.process_shares(shares);
 
         Ok(incremental_recovery)
     }
@@ -159,56 +129,69 @@ impl IncrementalRecovery {
     /// Contines a partial secret recovery.
     pub fn update(&mut self, shares: &[Share]) -> Result<()> {
         if self.root_hash.is_some() {
-            let root_hash = self.root_hash.clone().unwrap();
-            self.ids = continue_partial_signed_share_validation(
+            validate_signed_shares(
                 shares,
-                &self.ids,
-                self.threshold,
-                self.slen,
-                Some(&root_hash),
+                true,
+                Some(self.threshold),
+                Some(self.slen),
+                Some(&self.ids),
+                Some(&self.root_hash.clone().unwrap()),
             )?;
         } else {
-            self.ids = continue_partial_signed_share_validation(
+            validate_signed_shares(
                 shares,
-                &self.ids,
-                self.threshold,
-                self.slen,
+                false,
+                Some(self.threshold),
+                Some(self.slen),
+                Some(&self.ids),
                 None,
             )?;
         }
 
-        let mut col_in = Vec::with_capacity(self.threshold as usize);
+        self.process_shares(shares);
+        Ok(())
+    }
+
+    fn process_shares(&mut self, shares: &[Share]) {
+        let is_new_computation = self.shares_interpolated() == 0;
+        let shares_needed = self.shares_needed() as usize;
+
         for byteindex in 0..self.slen {
-            col_in.clear();
-            for s in shares.iter().take(self.shares_needed() as usize) {
-                col_in.push((s.id, s.data[byteindex]));
+            let col_in: Vec<(u8, u8)> = shares
+                .iter()
+                .take(shares_needed)
+                .map(|s| (s.id, s.data[byteindex]))
+                .collect();
+            if is_new_computation {
+                self.partial_secrets
+                    .push(PartialSecret::new(self.threshold, &col_in));
+            } else {
+                self.partial_secrets[byteindex].update(&col_in);
             }
-            self.partial_secrets[byteindex].update(&col_in);
         }
 
-        Ok(())
+        self.ids.extend(shares.iter().take(shares_needed).map(|s| s.get_id()));
     }
 
     /// Used to determine how many more shares are needed to finish computing a partial secret.
     pub fn shares_interpolated(&self) -> u8 {
-        // Safe indexing because `PartialSecret::new` ensures `self.partial_secrets` will be of
-        // length at least 1.
-        self.partial_secrets[0].shares_interpolated()
+        // Safe cast because validation ensures `self.ids.len() < 255`.
+        self.ids.len() as u8
     }
 
     /// Used to determine how many more shares are needed to finish computing a partial secret.
     pub fn shares_needed(&self) -> u8 {
-        // Safe indexing because `PartialSecret::new` ensures `self.partial_secrets` will be of
-        // length at least 1.
-        self.partial_secrets[0].shares_needed()
+        // Safe unsigned subraction because `process_shares` ensures we never interpolate more than
+        // `threshold`.
+        self.threshold - self.shares_interpolated()
     }
 
     /// Used to obtain the resulting secret when `threshold` shares have been evaluated.
     pub fn get_secret(&self) -> Result<Vec<u8>> {
         if self.shares_needed() != 0 {
-            bail!(ErrorKind::PartialInterpolationNotComplete(
-                self.threshold,
-                self.shares_interpolated()
+            bail!(ErrorKind::MissingShares(
+                self.shares_interpolated(),
+                self.threshold
             ))
         }
         // Safe to unwrap because we already confirmed no more shares are needed.
diff --git a/src/wrapped_secrets/scheme.rs b/src/wrapped_secrets/scheme.rs
index f3a58967..6e204f2c 100644
--- a/src/wrapped_secrets/scheme.rs
+++ b/src/wrapped_secrets/scheme.rs
@@ -4,8 +4,8 @@ use proto::wrapped::SecretProto;
 use protobuf;
 use protobuf::Message;
 
-use sss::SSS;
 pub(crate) use sss::Share;
+use sss::scheme::*;
 
 #[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)]
 pub(crate) struct WrappedSecrets;
@@ -30,14 +30,14 @@ impl WrappedSecrets {
 
         let data = rusty_secret.write_to_bytes().unwrap();
 
-        SSS::default().split_secret(k, n, data.as_slice(), sign_shares)
+        split_secret(k, n, data.as_slice(), sign_shares)
     }
 
     /// Recovers the secret from a k-out-of-n Shamir's secret sharing.
     ///
     /// At least `k` distinct shares need to be provided to recover the share.
     pub fn recover_secret(shares: Vec<Share>, verify_signatures: bool) -> Result<SecretProto> {
-        let secret = SSS::recover_secret(shares, verify_signatures)?;
+        let secret = Recover::recover_secret(&shares, verify_signatures)?;
 
         protobuf::parse_from_bytes::<SecretProto>(secret.as_slice())
             .chain_err(|| ErrorKind::SecretDeserializationError)
diff --git a/tests/test_vectors.rs b/tests/test_vectors.rs
index 3be698c7..c5f1f625 100644
--- a/tests/test_vectors.rs
+++ b/tests/test_vectors.rs
@@ -62,7 +62,7 @@ fn test_recover_es_test_vectors() {
 }
 
 #[test]
-fn test_recover_sellibitze_more_than_threshold_shars() {
+fn test_recover_sellibitze_more_than_threshold_shares() {
     let share1 = "2-1-1YAYwmOHqZ69jA";
     let share2 = "2-4-F7rAjX3UOa53KA";
     let share3 = "2-2-YJZQDGm22Y77Gw";

From b92ab2a6fa8566bc9e2a9b35fffe392f8ba04cf7 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 3 Apr 2018 00:28:34 -0500
Subject: [PATCH 37/42] Refactor validation

I think this refactor finally streamlines the `validation` module (at least for
what it needs to do now). Gets rid of those TODOs and creates two "types" of
validation function for incremental and all-at-once secret recovery.
---
 src/dss/ss1/scheme.rs   | 11 ++--------
 src/dss/thss/scheme.rs  | 11 ++--------
 src/share/validation.rs | 48 +++++++++++++++++++++++++++++++++++------
 src/sss/scheme.rs       | 33 ++++++++++------------------
 4 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/src/dss/ss1/scheme.rs b/src/dss/ss1/scheme.rs
index c96619e7..fd6bc9ec 100644
--- a/src/dss/ss1/scheme.rs
+++ b/src/dss/ss1/scheme.rs
@@ -11,7 +11,7 @@ use dss::thss::{MetaData, ThSS};
 use dss::utils;
 use dss::{thss, AccessStructure};
 use errors::*;
-use share::validation::{validate_share_count, validate_shares};
+use share::validation::{validate_all_shares, validate_share_count};
 use vol_hash::VOLHash;
 
 /// We bound the message size at about 16MB to avoid overflow in `random_bytes_count`.
@@ -248,14 +248,7 @@ impl SS1 {
         shares: &[Share],
     ) -> Result<(Vec<u8>, AccessStructure, Option<MetaData>)> {
         let shares = shares.to_vec();
-        let (threshold, _) = validate_shares(&shares, None, None, None)?;
-        // TODO: rewrite validation mod, so we can nix this `if` statement.
-        if shares.len() < threshold as usize {
-            bail!(ErrorKind::MissingShares(
-                shares.len() as u8,
-                threshold
-            ))
-        }
+        let (threshold, _) = validate_all_shares(&shares)?;
 
         let underlying_shares = shares
             .iter()
diff --git a/src/dss/thss/scheme.rs b/src/dss/thss/scheme.rs
index bc05c92e..85dcb2b5 100644
--- a/src/dss/thss/scheme.rs
+++ b/src/dss/thss/scheme.rs
@@ -8,7 +8,7 @@ use dss::random::{random_bytes, random_bytes_count, MAX_MESSAGE_SIZE};
 use errors::*;
 use gf256::Gf256;
 use lagrange;
-use share::validation::{validate_share_count, validate_shares};
+use share::validation::{validate_all_shares, validate_share_count};
 
 use super::AccessStructure;
 use super::encode::encode_secret;
@@ -90,14 +90,7 @@ impl ThSS {
         shares: &[Share],
     ) -> Result<(Vec<u8>, AccessStructure, Option<MetaData>)> {
         let shares = shares.to_vec();
-        let (threshold, cypher_len) = validate_shares(&shares, None, None, None)?;
-        // TODO: rewrite validation mod, so we can nix this `if` statement.
-        if shares.len() < threshold as usize {
-            bail!(ErrorKind::MissingShares(
-                shares.len() as u8,
-                threshold
-            ))
-        }
+        let (threshold, cypher_len) = validate_all_shares(&shares)?;
 
         let polys = (0..cypher_len)
             .map(|i| {
diff --git a/src/share/validation.rs b/src/share/validation.rs
index dc35501d..39ddff86 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -6,9 +6,20 @@ use share::{IsShare, IsSignedShare};
 // 2) Validate duplicate shares share num && data
 // 2) Validate group consistency
 // 3) Validate other properties, in no specific order
-pub(crate) fn validate_signed_shares<S: IsSignedShare>(
+pub(crate) fn validate_initial_signed_shares<S: IsSignedShare>(
     shares: &[S],
     verify_signatures: bool,
+) -> Result<(u8, usize, Option<Vec<u8>>)> {
+    if verify_signatures {
+        let root_hash = vec![];
+        validate_additional_signed_shares(shares, None, None, None, Some(&root_hash))
+    } else {
+        validate_additional_signed_shares(shares, None, None, None, None)
+    }
+}
+
+pub(crate) fn validate_additional_signed_shares<S: IsSignedShare>(
+    shares: &[S],
     threshold: Option<u8>,
     slen: Option<usize>,
     already_verified_ids: Option<&[u8]>,
@@ -16,18 +27,43 @@ pub(crate) fn validate_signed_shares<S: IsSignedShare>(
 ) -> Result<(u8, usize, Option<Vec<u8>>)> {
     let (threshold, slen) = validate_shares(shares, threshold, slen, already_verified_ids)?;
 
-    let rhash = if root_hash.is_some() || verify_signatures {
-        if !verify_signatures {
-            // TODO: =><=
-        }
+    let root_hash = if root_hash.is_some() {
         Some(S::verify_signatures(shares, root_hash)?)
     } else {
         None
     };
 
-    Ok((threshold, slen, rhash))
+    Ok((threshold, slen, root_hash))
+}
+
+/// Does check there at at least threshold shares.
+pub(crate) fn validate_all_signed_shares<S: IsSignedShare>(
+    shares: &[S],
+    verify_signature: bool,
+) -> Result<(u8, usize)> {
+    let result = validate_all_shares(shares)?;
+
+    if verify_signature {
+        S::verify_signatures(shares, None)?;
+    }
+
+    Ok(result)
+}
+
+/// Does check there at at least threshold shares.
+pub(crate) fn validate_all_shares<S: IsShare>(shares: &[S]) -> Result<(u8, usize)> {
+    let (threshold, slen) = validate_shares(shares, None, None, None)?;
+
+    // Safe to cast because `validate_shares` ensures `len() < 255`.
+    let shares_count = shares.len() as u8;
+    if shares_count < threshold {
+        bail!(ErrorKind::MissingShares(shares_count, threshold))
+    }
+
+    Ok((threshold, slen))
 }
 
+/// Does not check there at at least threshold shares.
 pub(crate) fn validate_shares<S: IsShare>(
     shares: &[S],
     threshold: Option<u8>,
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 17fa5778..1ef731ce 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -6,7 +6,7 @@ use rand::{OsRng, Rng};
 use errors::*;
 use lagrange::PartialSecret;
 use share::IsShare;
-use share::validation::{validate_share_count, validate_signed_shares};
+use share::validation::*;
 use sss::format::format_share_for_signing;
 use sss::{Share, HASH_ALGO};
 
@@ -111,7 +111,7 @@ impl Recover {
     /// Begins a partial secret recovery.
     pub fn new(shares: &[Share], verify_signatures: bool) -> Result<Self> {
         let (threshold, slen, root_hash) =
-            validate_signed_shares(shares, verify_signatures, None, None, None, None)?;
+            validate_initial_signed_shares(shares, verify_signatures)?;
 
         let mut incremental_recovery = Self {
             partial_secrets: Vec::with_capacity(slen),
@@ -128,25 +128,13 @@ impl Recover {
 
     /// Contines a partial secret recovery.
     pub fn update(&mut self, shares: &[Share]) -> Result<()> {
-        if self.root_hash.is_some() {
-            validate_signed_shares(
-                shares,
-                true,
-                Some(self.threshold),
-                Some(self.slen),
-                Some(&self.ids),
-                Some(&self.root_hash.clone().unwrap()),
-            )?;
-        } else {
-            validate_signed_shares(
-                shares,
-                false,
-                Some(self.threshold),
-                Some(self.slen),
-                Some(&self.ids),
-                None,
-            )?;
-        }
+        validate_additional_signed_shares(
+            shares,
+            Some(self.threshold),
+            Some(self.slen),
+            Some(&self.ids),
+            Some(&self.root_hash.clone().unwrap()),
+        )?;
 
         self.process_shares(shares);
         Ok(())
@@ -170,7 +158,8 @@ impl Recover {
             }
         }
 
-        self.ids.extend(shares.iter().take(shares_needed).map(|s| s.get_id()));
+        self.ids
+            .extend(shares.iter().take(shares_needed).map(|s| s.get_id()));
     }
 
     /// Used to determine how many more shares are needed to finish computing a partial secret.

From ae5ce4751d8f058970dcf2fada0dfa3322b24b34 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 3 Apr 2018 00:31:54 -0500
Subject: [PATCH 38/42] Recover::update should fail if no ::shares_needed

---
 src/errors.rs     | 2 +-
 src/sss/scheme.rs | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/errors.rs b/src/errors.rs
index 841d9623..9114f6d6 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -68,7 +68,7 @@ error_chain! {
         NoMoreSharesNeeded(required: u8) {
             description("The number of shares evaluated has already met the threshold and the
             secret is available.")
-            display("Only {} shares are required to recover the secret.", required)
+            display("Only {} shares are required to recover the secret. The secret should already be available.", required)
         }
 
         InvalidSignature(share_id: u8, signature: String) {
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 1ef731ce..7e2a00e8 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -128,6 +128,10 @@ impl Recover {
 
     /// Contines a partial secret recovery.
     pub fn update(&mut self, shares: &[Share]) -> Result<()> {
+        if self.shares_needed() == 0 {
+            bail!(ErrorKind::NoMoreSharesNeeded(self.threshold))
+        }
+
         validate_additional_signed_shares(
             shares,
             Some(self.threshold),

From 92c3d2b971a7feb4addb315f0b0ab40f259b1263 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 3 Apr 2018 02:02:43 -0500
Subject: [PATCH 39/42] Store redundant PartialSecret data in Recovery

`threshold` and `ids` are no longer stored in `PartialSecret`, reducing the size
of a `Recovery` object by up to 33% (as the number of shares interpolated
grows).

Optimizing for speed (since we will call methods on `PartialSecrets` much more
often than functions in `validate`), the `ids` field of `Recovery` is now of
type `Vec<Gf256>`.

`PartialSecret` no longer holds all its own state and relies on that state being
managed from the outside. While this is not ideal for using `PartialSecret`
independently of `Recovery`, I feel comfortable tying the two classes. I have
added additional assertions to `PartialSecret` to catch errors.
---
 src/lagrange.rs   | 145 +++++++++++++++++++---------------------------
 src/sss/scheme.rs |  26 +++++----
 2 files changed, 74 insertions(+), 97 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index 83fa26a3..d27928f4 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -12,11 +12,6 @@ pub struct PartialSecret {
     /// The secret byte. `None` until computation is complete.
     secret: Option<u8>,
     /// The number of shares necessary to recover the secret, a.k.a. the threshold.
-    threshold: u8,
-    /// The ids of the share (varies between 1 and n where n is the total number of generated
-    /// shares).
-    ids: Vec<Gf256>,
-    /// The differences of share values divided by their ids.
     diffs: Vec<Gf256>,
     /// The barycentric weights.
     weights: Vec<Gf256>,
@@ -30,47 +25,54 @@ impl PartialSecret {
     /// Create a new partial computation given a `threshold` (to know when the computation is
     /// finished), and an initial set of `points`.
     #[inline]
-    pub fn new(threshold: u8, points: &[(u8, u8)]) -> Self {
+    pub fn new(threshold: u8, ids: &[Gf256], new_ys: &[Gf256]) -> Self {
+        let (new_points, total_points) = (new_ys.len(), ids.len());
         assert!(threshold >= 2, "Given k less than 2!");
-        assert!(!points.is_empty(), "Given an empty set of points!");
+        assert_ne!(new_points, 0, "Given an empty set of points!");
         assert!(
-            points.len() <= threshold as usize,
+            total_points <= threshold as usize,
             "Given more than threshold shares!"
         );
+        assert_eq!(
+            new_points, total_points,
+            "Given an unequal number of x and y coordinates!"
+        );
 
         let mut partial_comp = Self {
             secret: None,
-            threshold,
-            ids: Vec::with_capacity(threshold as usize),
             diffs: Vec::with_capacity(threshold as usize),
             weights: vec![],
         };
 
-        partial_comp.update_diffs(points);
-        partial_comp.update_barycentric_weights();
+        partial_comp.update_diffs(ids, new_ys);
+        partial_comp.update_barycentric_weights(threshold, ids);
         partial_comp
     }
 
     /// Update the partial computation given an additional set of `points`.
     #[inline]
-    pub fn update(&mut self, points: &[(u8, u8)]) {
-        assert!(!points.is_empty(), "Given an empty set of points!");
+    pub fn update(&mut self, threshold: u8, ids: &[Gf256], ys: &[Gf256]) {
+        assert!(!ys.is_empty(), "Given an empty set of points!");
         assert!(
-            self.shares_interpolated() as usize + points.len() < self.threshold as usize,
+            ids.len() <= threshold as usize,
             "Given more than threshold shares!"
         );
+        assert!(
+            ids.len() >= ys.len(),
+            "The IDs of the shares processed should at least number the new y values."
+        );
 
-        self.update_diffs(points);
-        self.update_barycentric_weights();
+        self.update_diffs(ids, ys);
+        self.update_barycentric_weights(threshold, ids);
     }
 
     /// Parse just the `points` we need to compute the secret into `x` values and `diffs`.
-    fn update_diffs(&mut self, points: &[(u8, u8)]) {
-        for pi in points.iter() {
-            let xi = Gf256::from_byte(pi.0);
+    fn update_diffs(&mut self, ids: &[Gf256], new_ys: &[Gf256]) {
+        let (new_points, total_points) = (new_ys.len(), ids.len());
+        let ids = &ids[(total_points - new_points)..];
+
+        for (&xi, &yi) in ids.iter().zip(new_ys.iter()) {
             assert!(xi.poly != 0, "Given invalid share identifier 0!");
-            let yi = Gf256::from_byte(pi.1);
-            self.ids.push(xi);
             // Storing these `diffs` instead of the `y` values allows us to do a little more
             // precomputation, since we really only need `y / x` and not `y` to evaluate the second
             // form of the barycentric interpolation formula.
@@ -80,30 +82,30 @@ impl PartialSecret {
 
     /// Update the barycentric weights `w` corresponding to a set of `x` values.
     #[inline]
-    fn update_barycentric_weights(&mut self) {
+    fn update_barycentric_weights(&mut self, threshold: u8, ids: &[Gf256]) {
+        let total_points = ids.len();
+        let new_points = total_points - self.weights.len();
         // Need at least two points to start computing the barycentric weights.
-        if self.ids.len() == 1 {
+        if total_points == 1 {
             return;
         }
 
-        let x = if self.weights.is_empty() {
+        let start_weight = if self.weights.is_empty() {
             // Initialize initial weights.
-            self.weights = vec![Gf256::zero(); self.ids.len()];
+            self.weights = vec![Gf256::zero(); total_points];
             self.weights[0] = Gf256::one();
             1
         } else {
             // Initialize additional weights.
-            let initial_len = self.weights.len();
-            self.weights
-                .append(&mut vec![Gf256::zero(); self.ids.len() - initial_len]);
-            initial_len
+            self.weights.append(&mut vec![Gf256::zero(); new_points]);
+            total_points - new_points
         };
 
         // Update weights using algorithm (3.1) from "Polynomial Interpolation: Langrange vs
         // Newton" by Wilhelm Werner.
-        for i in x..self.ids.len() {
+        for i in start_weight..total_points {
             for j in 0..i {
-                let diff = self.ids[j] - self.ids[i];
+                let diff = ids[j] - ids[i];
                 assert!(diff.poly != 0, "Duplicate share identifiers encountered!");
                 self.weights[j] /= diff;
                 self.weights[i] -= self.weights[j];
@@ -111,21 +113,17 @@ impl PartialSecret {
         }
 
         // If we have sufficient information, we can compute the secret.
-        if self.shares_needed() == 0 {
-            self.compute_secret();
+        if threshold as usize - total_points == 0 {
+            self.compute_secret(ids);
         }
     }
 
     /// Compute the secret using the second or "true" form of the barycentric interpolation formula
     /// at `Gf256::zero()`.
     #[inline]
-    fn compute_secret(&mut self) {
+    fn compute_secret(&mut self, ids: &[Gf256]) {
         let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
-        for ((&xi, &di), &wi) in self.ids
-            .iter()
-            .zip(self.diffs.iter())
-            .zip(self.weights.iter())
-        {
+        for ((&xi, &di), &wi) in ids.iter().zip(self.diffs.iter()).zip(self.weights.iter()) {
             num += wi * di;
             denom += wi / xi;
         }
@@ -135,50 +133,25 @@ impl PartialSecret {
     /// If the partial computation is complete, return the secret, else an error.
     #[inline]
     pub fn get_secret(&self) -> Result<u8> {
-        if self.secret.is_none() {
-            bail!(ErrorKind::MissingShares(
-                self.shares_interpolated(),
-                self.threshold
-            ))
-        }
+        assert!(
+            self.secret.is_some(),
+            "Not enough shares have been interpolated to recover the secret!"
+        );
         // Safe to unwrap because we just confirmed it's not `None`.
         Ok(self.secret.unwrap())
     }
 
-    /// Returns the number of shares needed to complete the computation.
-    #[inline]
-    pub fn shares_needed(&self) -> u8 {
-        // Casting is safe because `assert!` statements in `new` and `update` ensure
-        // `self.ids.len()` will be less than 255.
-        self.threshold - self.ids.len() as u8
-    }
-
-    /// Returns the number of shares that have been interpolated so far.
-    #[inline]
-    pub fn shares_interpolated(&self) -> u8 {
-        // Casting is safe because `assert!` statements in `new` and `update` ensure
-        // `self.ids.len()` will be less than 255.
-        self.ids.len() as u8
-    }
-
-    /// Evaluate the interpolated polynomial at the point `Gf256::from_byte(x)` in the G(2^8)
+    /// Evaluate the interpolated polynomial at the point `gf256!(x)` in the G(2^8)
     /// Galois field.
     #[inline]
-    fn evaluate_at_x(&self, x: u8) -> Result<u8> {
-        if self.shares_needed() != 0 {
-            bail!(ErrorKind::MissingShares(
-                self.shares_interpolated(),
-                self.threshold
-            ))
-        }
+    fn evaluate_at_x(&self, x: Gf256, ids: &[Gf256]) -> Result<u8> {
+        assert!(
+            self.secret.is_some(),
+            "Not enough shares have been interpolated to recover the secret!"
+        );
 
-        let x = Gf256::from_byte(x);
         let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
-        for ((&xi, &di), &wi) in self.ids
-            .iter()
-            .zip(self.diffs.iter())
-            .zip(self.weights.iter())
-        {
+        for ((&xi, &di), &wi) in ids.iter().zip(self.diffs.iter()).zip(self.weights.iter()) {
             let delta = x - xi;
             // Slightly slower to re-multiply the `diffs` by `xi` here, but otherwise we have to
             // additionally store the `y` values in `PartialSecret`, or store `y` values instead of
@@ -265,20 +238,20 @@ mod tests {
                 return TestResult::discard();
             }
 
-            let points = ys.into_iter()
-                           .zip(1..u8::MAX)
-                           .map(|(y, x)| (x, y))
-                           .collect::<Vec<_>>();
+            // Safe to cast because if `ys.len() > 255` it is discarded.
+            let num_points = ys.len() as u8;
+            let ids: Vec<Gf256> = (1..(num_points + 1)).map(|x| gf256!(x)).collect();
+            let ys: Vec<Gf256> = ys.iter().map(|&y| gf256!(y)).collect();
 
-            let elems = points
-                .iter()
-                .map(|&(x, y)| (gf256!(x), gf256!(y)))
-                .collect::<Vec<_>>();
+            let elems: Vec<(Gf256, Gf256)> = ids.iter()
+                                                .zip(ys.iter())
+                                                .map(|(&x, &y)| (x, y))
+                                                .collect();
 
             let poly = interpolate(&elems);
             let result_poly = poly.evaluate_at(Gf256::zero()).to_byte();
-            // Safe to cast because if `ys.len() > 255` it is discarded.
-            let interpolation = PartialSecret::new(points.len() as u8, &points);
+
+            let interpolation = PartialSecret::new(num_points, &ids, &ys);
             let result_interpolate = interpolation.get_secret().unwrap();
 
             TestResult::from_bool(result_poly == result_interpolate)
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 7e2a00e8..a947c126 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -1,9 +1,12 @@
 //! Provides Shamir's secret sharing with raw data.
 
+use std::cmp::min;
+
 use merkle_sigs::sign_data_vec;
 use rand::{OsRng, Rng};
 
 use errors::*;
+use gf256::Gf256;
 use lagrange::PartialSecret;
 use share::IsShare;
 use share::validation::*;
@@ -90,7 +93,7 @@ pub(crate) struct Recover {
     partial_secrets: Vec<PartialSecret>,
     /// The ids of the share (varies between 1 and n where n is the total number of generated
     /// shares).
-    ids: Vec<u8>,
+    ids: Vec<Gf256>,
     /// The number of shares necessary to recover the secret.
     threshold: u8,
     /// The length of the secret.
@@ -132,11 +135,12 @@ impl Recover {
             bail!(ErrorKind::NoMoreSharesNeeded(self.threshold))
         }
 
+        let ids: Vec<u8> = self.ids.iter().map(|id| id.poly).collect();
         validate_additional_signed_shares(
             shares,
             Some(self.threshold),
             Some(self.slen),
-            Some(&self.ids),
+            Some(&ids),
             Some(&self.root_hash.clone().unwrap()),
         )?;
 
@@ -145,25 +149,25 @@ impl Recover {
     }
 
     fn process_shares(&mut self, shares: &[Share]) {
+        // Don't bother interpolating more than k shares.
+        let ub = min(self.shares_needed() as usize, shares.len());
+        let shares = &shares[..ub];
         let is_new_computation = self.shares_interpolated() == 0;
-        let shares_needed = self.shares_needed() as usize;
+        self.ids
+            .extend(shares.iter().map(|s| Gf256::from_byte(s.id)));
 
         for byteindex in 0..self.slen {
-            let col_in: Vec<(u8, u8)> = shares
+            let ys: Vec<Gf256> = shares
                 .iter()
-                .take(shares_needed)
-                .map(|s| (s.id, s.data[byteindex]))
+                .map(|s| Gf256::from_byte(s.data[byteindex]))
                 .collect();
             if is_new_computation {
                 self.partial_secrets
-                    .push(PartialSecret::new(self.threshold, &col_in));
+                    .push(PartialSecret::new(self.threshold, &self.ids, &ys));
             } else {
-                self.partial_secrets[byteindex].update(&col_in);
+                self.partial_secrets[byteindex].update(self.threshold, &self.ids, &ys);
             }
         }
-
-        self.ids
-            .extend(shares.iter().take(shares_needed).map(|s| s.get_id()));
     }
 
     /// Used to determine how many more shares are needed to finish computing a partial secret.

From eebdab2a2b3a91fcd55f9b3c8678d3d8b3f7d182 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 3 Apr 2018 02:12:44 -0500
Subject: [PATCH 40/42] Use gf256! instead of Gf256::from_byte

---
 src/sss/scheme.rs | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index a947c126..79920fe8 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -153,14 +153,10 @@ impl Recover {
         let ub = min(self.shares_needed() as usize, shares.len());
         let shares = &shares[..ub];
         let is_new_computation = self.shares_interpolated() == 0;
-        self.ids
-            .extend(shares.iter().map(|s| Gf256::from_byte(s.id)));
+        self.ids.extend(shares.iter().map(|s| gf256!(s.id)));
 
         for byteindex in 0..self.slen {
-            let ys: Vec<Gf256> = shares
-                .iter()
-                .map(|s| Gf256::from_byte(s.data[byteindex]))
-                .collect();
+            let ys: Vec<Gf256> = shares.iter().map(|s| gf256!(s.data[byteindex])).collect();
             if is_new_computation {
                 self.partial_secrets
                     .push(PartialSecret::new(self.threshold, &self.ids, &ys));

From 368c35b33dbd58260398eaa37239191ffc338b9c Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 3 Apr 2018 11:24:15 -0500
Subject: [PATCH 41/42] PartialSecrets to BarycentricWeights

Having crippled `PartialSecrets` sufficiently from its original form, I realized
it was best to carry this process out as far as possible, and make this even
more explicit. Storing an `secret: Option<u8>` didn't make sense when it no
longer stored `ids` and `thresholds` and was dependent on outside forces
providing that information, so it could know when to compute the secret. The
`compute_secret` method, now `evaluate_at_zero`, it's own function in the
`lagrange` module, also seemed out of place.

Now `threshold` doesn't needed to be passed to `BarycentricWeights`, and the
type is more accurately described by its name. An outside force is still
responsible for managing the `ids`, and when `threshold` shares have been
interpolated, it will need to call `evaluate_at_zero` with the
`BarycentricWeights` and `ids` (`Recovery` now does this).

The last change to `Recovery` was to make it hold its own `secret` (it
automatically computes this secret as soon as threshold shares have been
interpolated via calls to `new` and `update`). Thus `get_secret` will be faster
because we're just unwrapping a `Some(Vec<u8>)` and rewrapping it in `Ok`,
instead of needing to construct that `Vec<u8>` by iterating over `Option<u8>`s
we need to unwrap and `collect`.
---
 src/lagrange.rs   | 144 ++++++++++++++++++++++------------------------
 src/sss/scheme.rs |  46 +++++++++++----
 2 files changed, 103 insertions(+), 87 deletions(-)

diff --git a/src/lagrange.rs b/src/lagrange.rs
index d27928f4..b407f66b 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -6,70 +6,60 @@ use poly::Poly;
 
 /// Stores the intermediate state of interpolation and evaluation at `Gf256::zero()` of a
 /// polynomial. A secret may be computed incrementally using barycentric Lagrange interpolation.
-/// The state is updated with new points until threshold points have been evaluated, at which point
-/// the `secret` field will be updated from `None` to `Some(u8)`.
-pub struct PartialSecret {
-    /// The secret byte. `None` until computation is complete.
-    secret: Option<u8>,
+pub struct BarycentricWeights {
     /// The number of shares necessary to recover the secret, a.k.a. the threshold.
-    diffs: Vec<Gf256>,
+    pub diffs: Vec<Gf256>,
     /// The barycentric weights.
-    weights: Vec<Gf256>,
+    pub weights: Vec<Gf256>,
 }
 
-// `PartialSecret` is not a public-facing struct. We expect the functions that interact with it to
+// `BarycentricWeights` is not a public-facing struct. We expect the functions that interact with it to
 // do validation of the `points` and other arguments it operates on. As a defensive programming
 // practice, we have included `assert!` statements, which should also clarify the validation
 // expectations of each method.
-impl PartialSecret {
+impl BarycentricWeights {
     /// Create a new partial computation given a `threshold` (to know when the computation is
     /// finished), and an initial set of `points`.
     #[inline]
-    pub fn new(threshold: u8, ids: &[Gf256], new_ys: &[Gf256]) -> Self {
+    pub fn new(ids: &[Gf256], new_ys: &[Gf256]) -> Self {
         let (new_points, total_points) = (new_ys.len(), ids.len());
-        assert!(threshold >= 2, "Given k less than 2!");
         assert_ne!(new_points, 0, "Given an empty set of points!");
-        assert!(
-            total_points <= threshold as usize,
-            "Given more than threshold shares!"
-        );
         assert_eq!(
             new_points, total_points,
             "Given an unequal number of x and y coordinates!"
         );
 
         let mut partial_comp = Self {
-            secret: None,
-            diffs: Vec::with_capacity(threshold as usize),
+            diffs: Vec::with_capacity(total_points),
             weights: vec![],
         };
 
         partial_comp.update_diffs(ids, new_ys);
-        partial_comp.update_barycentric_weights(threshold, ids);
+        partial_comp.update_barycentric_weights(ids);
         partial_comp
     }
 
     /// Update the partial computation given an additional set of `points`.
     #[inline]
-    pub fn update(&mut self, threshold: u8, ids: &[Gf256], ys: &[Gf256]) {
-        assert!(!ys.is_empty(), "Given an empty set of points!");
-        assert!(
-            ids.len() <= threshold as usize,
-            "Given more than threshold shares!"
-        );
+    pub fn update(&mut self, ids: &[Gf256], new_ys: &[Gf256]) {
+        let (new_points, total_points) = (new_ys.len(), ids.len());
+        assert_ne!(new_points, 0, "Given an empty set of points!");
+        assert!(total_points >= 2, "In order to call update you must have already processed at least one point to make a `BarycentricWeights`, and you must provide at least a second in your call to update.");
         assert!(
-            ids.len() >= ys.len(),
+            total_points >= new_points,
             "The IDs of the shares processed should at least number the new y values."
         );
 
-        self.update_diffs(ids, ys);
-        self.update_barycentric_weights(threshold, ids);
+        self.update_diffs(ids, new_ys);
+        self.update_barycentric_weights(ids);
     }
 
-    /// Parse just the `points` we need to compute the secret into `x` values and `diffs`.
+    /// Parse the `new_ys` into `diffs`.
+    #[inline]
     fn update_diffs(&mut self, ids: &[Gf256], new_ys: &[Gf256]) {
         let (new_points, total_points) = (new_ys.len(), ids.len());
         let ids = &ids[(total_points - new_points)..];
+        self.diffs.reserve_exact(new_points);
 
         for (&xi, &yi) in ids.iter().zip(new_ys.iter()) {
             assert!(xi.poly != 0, "Given invalid share identifier 0!");
@@ -82,13 +72,13 @@ impl PartialSecret {
 
     /// Update the barycentric weights `w` corresponding to a set of `x` values.
     #[inline]
-    fn update_barycentric_weights(&mut self, threshold: u8, ids: &[Gf256]) {
+    fn update_barycentric_weights(&mut self, ids: &[Gf256]) {
         let total_points = ids.len();
-        let new_points = total_points - self.weights.len();
         // Need at least two points to start computing the barycentric weights.
         if total_points == 1 {
             return;
         }
+        let new_points = total_points - self.weights.len();
 
         let start_weight = if self.weights.is_empty() {
             // Initialize initial weights.
@@ -111,56 +101,60 @@ impl PartialSecret {
                 self.weights[i] -= self.weights[j];
             }
         }
-
-        // If we have sufficient information, we can compute the secret.
-        if threshold as usize - total_points == 0 {
-            self.compute_secret(ids);
-        }
     }
+}
 
-    /// Compute the secret using the second or "true" form of the barycentric interpolation formula
-    /// at `Gf256::zero()`.
-    #[inline]
-    fn compute_secret(&mut self, ids: &[Gf256]) {
-        let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
-        for ((&xi, &di), &wi) in ids.iter().zip(self.diffs.iter()).zip(self.weights.iter()) {
-            num += wi * di;
-            denom += wi / xi;
-        }
-        self.secret = Some((num / denom).to_byte());
-    }
+/// Compute the secret using the second or "true" form of the barycentric interpolation formula
+/// at `Gf256::zero()`.
+#[inline]
+pub fn evaluate_at_zero(wds: &BarycentricWeights, ids: &[Gf256]) -> u8 {
+    validate_evaluation_parameters(wds, ids);
 
-    /// If the partial computation is complete, return the secret, else an error.
-    #[inline]
-    pub fn get_secret(&self) -> Result<u8> {
-        assert!(
-            self.secret.is_some(),
-            "Not enough shares have been interpolated to recover the secret!"
-        );
-        // Safe to unwrap because we just confirmed it's not `None`.
-        Ok(self.secret.unwrap())
+    let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
+    for ((&xi, &di), &wi) in ids.iter().zip(wds.diffs.iter()).zip(wds.weights.iter()) {
+        num += wi * di;
+        denom += wi / xi;
     }
 
-    /// Evaluate the interpolated polynomial at the point `gf256!(x)` in the G(2^8)
-    /// Galois field.
-    #[inline]
-    fn evaluate_at_x(&self, x: Gf256, ids: &[Gf256]) -> Result<u8> {
-        assert!(
-            self.secret.is_some(),
-            "Not enough shares have been interpolated to recover the secret!"
-        );
+    (num / denom).to_byte()
+}
 
-        let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
-        for ((&xi, &di), &wi) in ids.iter().zip(self.diffs.iter()).zip(self.weights.iter()) {
-            let delta = x - xi;
-            // Slightly slower to re-multiply the `diffs` by `xi` here, but otherwise we have to
-            // additionally store the `y` values in `PartialSecret`, or store `y` values instead of
-            // the `diffs` and precompute less in the standard case of evaluating at 0.
-            num += wi * di * xi / delta;
-            denom += wi / delta;
-        }
-        Ok((num / denom).to_byte())
+/// Evaluate the interpolated polynomial at the point `gf256!(x)` in the G(2^8)
+/// Galois field.
+#[inline]
+fn evaluate_at_x(wds: &BarycentricWeights, ids: &[Gf256], x: Gf256) -> Result<u8> {
+    validate_evaluation_parameters(wds, ids);
+
+    let (mut num, mut denom) = (Gf256::zero(), Gf256::zero());
+    for ((&xi, &di), &wi) in ids.iter().zip(wds.diffs.iter()).zip(wds.weights.iter()) {
+        let delta = x - xi;
+        // Slightly slower to re-multiply the `diffs` by `xi` here, but otherwise we have to
+        // additionally store the `y` values in `BarycentricWeights`, or store `y` values instead
+        // of the `diffs` and precompute less in the standard case of evaluating at 0.
+        num += wi * di * xi / delta;
+        denom += wi / delta;
     }
+    Ok((num / denom).to_byte())
+}
+
+/// TODO:
+#[inline]
+fn validate_evaluation_parameters(wds: &BarycentricWeights, ids: &[Gf256]) {
+    let num_weights = wds.weights.len();
+    let num_diffs = wds.diffs.len();
+    assert_eq!(
+        num_weights, num_diffs,
+        "`BarycentricWeights` should contain the same number of weights and diffs!"
+    );
+    let num_points = ids.len();
+    assert_eq!(
+        num_weights, num_points,
+        "The number of barycentric weights is not equal to the number of IDs!"
+    );
+    assert!(
+        num_points >= 2,
+        "Can't evaluate a polynomial at 0 without at least having processed two points."
+    );
 }
 
 /// Computes the coefficient of the Lagrange polynomial interpolated from the given `points`, in
@@ -251,8 +245,8 @@ mod tests {
             let poly = interpolate(&elems);
             let result_poly = poly.evaluate_at(Gf256::zero()).to_byte();
 
-            let interpolation = PartialSecret::new(num_points, &ids, &ys);
-            let result_interpolate = interpolation.get_secret().unwrap();
+            let wds = BarycentricWeights::new(&ids, &ys);
+            let result_interpolate = evaluate_at_zero(&wds, &ids);
 
             TestResult::from_bool(result_poly == result_interpolate)
         }
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 79920fe8..0e5bbb06 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -7,7 +7,7 @@ use rand::{OsRng, Rng};
 
 use errors::*;
 use gf256::Gf256;
-use lagrange::PartialSecret;
+use lagrange::{evaluate_at_zero, BarycentricWeights};
 use share::IsShare;
 use share::validation::*;
 use sss::format::format_share_for_signing;
@@ -90,7 +90,7 @@ fn secret_share(src: &[u8], threshold: u8, shares_count: u8) -> Result<Vec<Vec<u
 /// `Recover` provides an interface for recovering a secret.
 pub(crate) struct Recover {
     /// The state of each partially-recovered secret byte.
-    partial_secrets: Vec<PartialSecret>,
+    barycentric: Vec<BarycentricWeights>,
     /// The ids of the share (varies between 1 and n where n is the total number of generated
     /// shares).
     ids: Vec<Gf256>,
@@ -100,6 +100,8 @@ pub(crate) struct Recover {
     slen: usize,
     /// If the shares are signed, the root hash of the Merkle tree all shares are signed with.
     root_hash: Option<Vec<u8>>,
+    /// The secret. `None` until computation is complete.
+    secret: Option<Vec<u8>>,
 }
 
 impl Recover {
@@ -117,11 +119,12 @@ impl Recover {
             validate_initial_signed_shares(shares, verify_signatures)?;
 
         let mut incremental_recovery = Self {
-            partial_secrets: Vec::with_capacity(slen),
+            barycentric: Vec::with_capacity(slen),
             ids: Vec::with_capacity(threshold as usize),
             threshold,
             slen,
             root_hash,
+            secret: None,
         };
 
         incremental_recovery.process_shares(shares);
@@ -153,17 +156,34 @@ impl Recover {
         let ub = min(self.shares_needed() as usize, shares.len());
         let shares = &shares[..ub];
         let is_new_computation = self.shares_interpolated() == 0;
+        // NOTE: `shares_interpolated` will be very temporarily be out of sync until the next for
+        // loop completes.
         self.ids.extend(shares.iter().map(|s| gf256!(s.id)));
 
         for byteindex in 0..self.slen {
             let ys: Vec<Gf256> = shares.iter().map(|s| gf256!(s.data[byteindex])).collect();
             if is_new_computation {
-                self.partial_secrets
-                    .push(PartialSecret::new(self.threshold, &self.ids, &ys));
+                self.barycentric
+                    .push(BarycentricWeights::new(&self.ids, &ys));
             } else {
-                self.partial_secrets[byteindex].update(self.threshold, &self.ids, &ys);
+                self.barycentric[byteindex].update(&self.ids, &ys);
             }
         }
+
+        // If we have sufficient information, we can compute the secret.
+        if self.shares_needed() == 0 {
+            self.compute_secret();
+        }
+    }
+
+    /// Computes the secret (called automatically when sufficient shares have been processed).
+    fn compute_secret(&mut self) {
+        self.secret = Some(
+            self.barycentric
+                .iter()
+                .map(|wds| evaluate_at_zero(wds, &self.ids))
+                .collect(),
+        );
     }
 
     /// Used to determine how many more shares are needed to finish computing a partial secret.
@@ -172,6 +192,11 @@ impl Recover {
         self.ids.len() as u8
     }
 
+    /// Returns the threshold of shares needed to recover the secret.
+    pub fn get_threshold(&self) -> u8 {
+        self.threshold
+    }
+
     /// Used to determine how many more shares are needed to finish computing a partial secret.
     pub fn shares_needed(&self) -> u8 {
         // Safe unsigned subraction because `process_shares` ensures we never interpolate more than
@@ -181,16 +206,13 @@ impl Recover {
 
     /// Used to obtain the resulting secret when `threshold` shares have been evaluated.
     pub fn get_secret(&self) -> Result<Vec<u8>> {
-        if self.shares_needed() != 0 {
+        if self.secret.is_some() {
+            Ok(self.secret.clone().unwrap())
+        } else {
             bail!(ErrorKind::MissingShares(
                 self.shares_interpolated(),
                 self.threshold
             ))
         }
-        // Safe to unwrap because we already confirmed no more shares are needed.
-        Ok(self.partial_secrets
-            .iter()
-            .map(|ps| ps.get_secret().unwrap())
-            .collect())
     }
 }

From 3e971e455a47c2e9a4917b5254d86c642e515831 Mon Sep 17 00:00:00 2001
From: Noah Vesely <fowlslegs@riseup.net>
Date: Tue, 3 Apr 2018 16:10:39 -0500
Subject: [PATCH 42/42] Misc minor fixes to new Partial Interpolation
 capability

---
 src/dss/ss1/scheme.rs   |  2 +-
 src/errors.rs           |  5 -----
 src/lagrange.rs         | 25 +++++++++++++++----------
 src/share/validation.rs |  7 ++++---
 src/sss/scheme.rs       |  1 -
 5 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/src/dss/ss1/scheme.rs b/src/dss/ss1/scheme.rs
index fd6bc9ec..01ad67d1 100644
--- a/src/dss/ss1/scheme.rs
+++ b/src/dss/ss1/scheme.rs
@@ -254,7 +254,7 @@ impl SS1 {
             .iter()
             .map(|share| thss::Share {
                 id: share.id,
-                threshold: share.threshold,
+                threshold,
                 shares_count: share.shares_count,
                 data: share.data.clone(),
                 metadata: share.metadata.clone(),
diff --git a/src/errors.rs b/src/errors.rs
index 9114f6d6..81fcf81f 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -149,11 +149,6 @@ error_chain! {
             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, no_more_than_five(ids))
         }
-
-        // PartialInterpolationNotComplete(k: u8, shares_interpolated: u8) {
-        //     description("The partial interpolation result is not complete because the number of points interpolated has not reached the threshold.")
-        //     display("In order to evaluate the secret polynomial at any point k = {} shares are needed, whereas only {} have been provided.", k, shares_interpolated)
-        // }
     }
 
     foreign_links {
diff --git a/src/lagrange.rs b/src/lagrange.rs
index b407f66b..368a99ce 100644
--- a/src/lagrange.rs
+++ b/src/lagrange.rs
@@ -1,4 +1,4 @@
-use std::u8;
+/// Implements barycentric Lagrange interpolation.
 
 use errors::*;
 use gf256::Gf256;
@@ -13,10 +13,9 @@ pub struct BarycentricWeights {
     pub weights: Vec<Gf256>,
 }
 
-// `BarycentricWeights` is not a public-facing struct. We expect the functions that interact with it to
-// do validation of the `points` and other arguments it operates on. As a defensive programming
-// practice, we have included `assert!` statements, which should also clarify the validation
-// expectations of each method.
+// `BarycentricWeights` is not a public-facing struct. We expect the functions that interact with
+// it to do validation of its operands (namely, the methods of `sss::Recover`.) Still, we have
+// included many assertions to guard against clearly wrong inputs.
 impl BarycentricWeights {
     /// Create a new partial computation given a `threshold` (to know when the computation is
     /// finished), and an initial set of `points`.
@@ -44,11 +43,17 @@ impl BarycentricWeights {
     pub fn update(&mut self, ids: &[Gf256], new_ys: &[Gf256]) {
         let (new_points, total_points) = (new_ys.len(), ids.len());
         assert_ne!(new_points, 0, "Given an empty set of points!");
-        assert!(total_points >= 2, "In order to call update you must have already processed at least one point to make a `BarycentricWeights`, and you must provide at least a second in your call to update.");
+        assert!(total_points > 1, "In order to call update you must have already processed at least
+        one point to make a `BarycentricWeights`, and you must provide at least a second in your
+        call to update.");
         assert!(
-            total_points >= new_points,
-            "The IDs of the shares processed should at least number the new y values."
+            total_points > new_points,
+            "During an update IDs of the shares processed should at least number the new y values."
         );
+        // We use diffs here and not weights because no weights are generated until at least 2
+        // points have been interpolated.
+        assert_eq!(new_points + self.diffs.len(), total_points, "The new points given plus the
+        existing diffs should be equal in length to the total points given.");
 
         self.update_diffs(ids, new_ys);
         self.update_barycentric_weights(ids);
@@ -137,7 +142,7 @@ fn evaluate_at_x(wds: &BarycentricWeights, ids: &[Gf256], x: Gf256) -> Result<u8
     Ok((num / denom).to_byte())
 }
 
-/// TODO:
+/// Validates the `BarycentricWeights` and `ids` can successfully be used to compute a secret byte.
 #[inline]
 fn validate_evaluation_parameters(wds: &BarycentricWeights, ids: &[Gf256]) {
     let num_weights = wds.weights.len();
@@ -203,7 +208,7 @@ mod tests {
     use super::*;
     use gf256::*;
     use quickcheck::*;
-    use std;
+    use std::u8;
 
     quickcheck! {
 
diff --git a/src/share/validation.rs b/src/share/validation.rs
index 39ddff86..7dd31057 100644
--- a/src/share/validation.rs
+++ b/src/share/validation.rs
@@ -91,9 +91,10 @@ pub(crate) fn validate_shares<S: IsShare>(
         let threshold_ = share.get_threshold();
         let slen_ = share.get_data().len();
 
-        // Public-facing `Share::share_from_string` performs these three tests, but in case another
-        // type which implements `IsShare` is implemented later that doesn't do that validation,
-        // we'll leave them.
+        // Public-facing `Share::share_from_string` performs the following three tests, so they are
+        // redudant in production for the time being. However, for our tests, not all shares tested
+        // come from strings (e.g., `dss::ss1::Share` we test by constructing from parts), so they
+        // should be left for now.
         if id < 1 {
             bail!(ErrorKind::ShareParsingInvalidShareId(id))
         } else if threshold_ < 2 {
diff --git a/src/sss/scheme.rs b/src/sss/scheme.rs
index 0e5bbb06..fc1032a1 100644
--- a/src/sss/scheme.rs
+++ b/src/sss/scheme.rs
@@ -8,7 +8,6 @@ use rand::{OsRng, Rng};
 use errors::*;
 use gf256::Gf256;
 use lagrange::{evaluate_at_zero, BarycentricWeights};
-use share::IsShare;
 use share::validation::*;
 use sss::format::format_share_for_signing;
 use sss::{Share, HASH_ALGO};