issue286: make `ThresholdSign` and `ThresholdDecryption` APIs consistent (#316)

* issue286: make `ThresholdSign` and `ThresholdDecryption` APIs consistent

This gives both methods a `set_message(payload)` and a `sign()` method. If `sign` is called prior to
`set_message` or if was already called, then an empty step is returned. Otherwise share messages are
sent out to peers and `try_output` is called.

* Incorporating feedback from review of pull request #316

* Fixing the stack overflow and responding to further feedback

* Updates module documentation to reflect the API change from #286

* adds a new constructor to `ThresholdSign` and `ThresholdDecryption`

The `new_with_<payloadname>` function allows setting payload on construction
instead of as a separate call.

src/binary_agreement/binary_agreement.rs

@@ -317,14 +317,16 @@ impl<N: NodeIdT, S: SessionIdT> BinaryAgreement<N, S> {
     /// Creates the initial coin state for the current epoch, i.e. sets it to the predetermined
     /// value, or initializes a `ThresholdSign` instance.
     fn coin_state(&self) -> Result<CoinState<N>> {
-        Ok(match self.epoch % 3 {
-            0 => CoinState::Decided(true),
-            1 => CoinState::Decided(false),
+        match self.epoch % 3 {
+            0 => Ok(CoinState::Decided(true)),
+            1 => Ok(CoinState::Decided(false)),
             _ => {
                 let coin_id = bincode::serialize(&(&self.session_id, self.epoch))?;
-                CoinState::InProgress(Box::new(ThresholdSign::new(self.netinfo.clone(), coin_id)))
+                let mut ts = ThresholdSign::new(self.netinfo.clone());
+                ts.set_document(coin_id).map_err(Error::InvokeCoin)?;
+                Ok(CoinState::InProgress(Box::new(ts)))
             }
-        })
+        }
     }
 
     /// Decides on a value and broadcasts a `Term` message with that value.

src/honey_badger/epoch_state.rs

@@ -50,7 +50,10 @@ where
     /// Handles a ciphertext input.
     fn set_ciphertext(&mut self, ciphertext: Ciphertext) -> td::Result<td::Step<N>> {
         match self {
-            DecryptionState::Ongoing(ref mut td) => td.handle_input(ciphertext),
+            DecryptionState::Ongoing(ref mut td) => {
+                td.set_ciphertext(ciphertext)?;
+                td.start_decryption()
+            }
             DecryptionState::Complete(_) => Ok(td::Step::default()),
         }
     }

src/threshold_decryption.rs

@@ -7,8 +7,9 @@
 //!
 //! The algorithm uses a threshold encryption scheme: A message encrypted to the network's public
 //! key can be collaboratively decrypted by combining at least _f + 1_ decryption shares. Each
-//! validator holds a secret key share, and uses it to produce and multicast a decryption share.
-//! The algorithm outputs as soon as _f + 1_ of them have been received.
+//! validator holds a secret key share, and uses it to produce and multicast a decryption share once
+//! a ciphertext is provided. The algorithm outputs as soon as it receives a ciphertext and _f + 1_
+//! threshold shares.
 
 use std::collections::BTreeMap;
 use std::sync::Arc;
@@ -53,6 +54,8 @@ pub enum Error {
     UnknownSender,
     #[fail(display = "Decryption failed: {:?}", _0)]
     Decryption(crypto::error::Error),
+    #[fail(display = "Start decryption called before setting ciphertext")]
+    CiphertextIsNone,
 }
 
 /// A threshold decryption result.
@@ -71,6 +74,8 @@ pub struct ThresholdDecryption<N> {
     ciphertext: Option<Ciphertext>,
     /// All received threshold decryption shares.
     shares: BTreeMap<N, DecryptionShare>,
+    /// Whether we already sent our shares.
+    had_input: bool,
     /// Whether we have already returned the output.
     terminated: bool,
 }
@@ -79,13 +84,13 @@ pub type Step<N> = ::Step<ThresholdDecryption<N>>;
 
 impl<N: NodeIdT> DistAlgorithm for ThresholdDecryption<N> {
     type NodeId = N;
-    type Input = Ciphertext;
+    type Input = ();
     type Output = Vec<u8>;
     type Message = Message;
     type Error = Error;
 
-    fn handle_input(&mut self, input: Ciphertext) -> Result<Step<N>> {
-        self.set_ciphertext(input)
+    fn handle_input(&mut self, _input: ()) -> Result<Step<N>> {
+        self.start_decryption()
     }
 
     fn handle_message(&mut self, sender_id: &N, message: Message) -> Result<Step<N>> {
@@ -108,29 +113,49 @@ impl<N: NodeIdT> ThresholdDecryption<N> {
             netinfo,
             ciphertext: None,
             shares: BTreeMap::new(),
+            had_input: false,
             terminated: false,
         }
     }
 
+    /// Creates a new instance of `ThresholdDecryption`, including setting the ciphertext to
+    /// decrypt.
+    pub fn new_with_ciphertext(netinfo: Arc<NetworkInfo<N>>, ct: Ciphertext) -> Result<Self> {
+        let mut td = ThresholdDecryption::new(netinfo);
+        td.set_ciphertext(ct)?;
+        Ok(td)
+    }
+
     /// Sets the ciphertext, sends the decryption share, and tries to decrypt it.
     /// This must be called exactly once, with the same ciphertext in all participating nodes.
     /// If we have enough shares, outputs the plaintext.
-    pub fn set_ciphertext(&mut self, ct: Ciphertext) -> Result<Step<N>> {
+    pub fn set_ciphertext(&mut self, ct: Ciphertext) -> Result<()> {
         if self.ciphertext.is_some() {
             return Err(Error::MultipleInputs(Box::new(ct)));
         }
-        if !self.netinfo.is_validator() {
-            self.ciphertext = Some(ct);
-            return Ok(self.try_output()?);
+        if !ct.verify() {
+            return Err(Error::InvalidCiphertext(Box::new(ct.clone())));
         }
-        let share = match self.netinfo.secret_key_share().decrypt_share(&ct) {
-            None => return Err(Error::InvalidCiphertext(Box::new(ct))),
-            Some(share) => share,
-        };
         self.ciphertext = Some(ct);
-        let our_id = self.our_id().clone();
+        Ok(())
+    }
+
+    /// Sends our decryption shares to peers, and if we have collected enough, returns the decrypted
+    /// message. Returns an error if the ciphertext hasn't been received yet.
+    pub fn start_decryption(&mut self) -> Result<Step<N>> {
+        if self.had_input {
+            return Ok(Step::default()); // Don't waste time on redundant shares.
+        }
+        let ct = self.ciphertext.clone().ok_or(Error::CiphertextIsNone)?;
         let mut step = Step::default();
         step.fault_log.extend(self.remove_invalid_shares());
+        self.had_input = true;
+        if !self.netinfo.is_validator() {
+            step.extend(self.try_output()?);
+            return Ok(step);
+        }
+        let share = self.netinfo.secret_key_share().decrypt_share_no_verify(&ct);
+        let our_id = self.our_id().clone();
         let msg = Target::All.message(Message(share.clone()));
         step.messages.push(msg);
         self.shares.insert(our_id, share);
@@ -152,6 +177,10 @@ impl<N: NodeIdT> ThresholdDecryption<N> {
         if self.terminated {
             return Ok(Step::default()); // Don't waste time on redundant shares.
         }
+        // Before checking the share, ensure the sender is a known validator
+        self.netinfo
+            .public_key_share(sender_id)
+            .ok_or(Error::UnknownSender)?;
         let Message(share) = message;
         if !self.is_share_valid(sender_id, &share) {
             let fault_kind = FaultKind::UnverifiedDecryptionShareSender;
@@ -198,9 +227,10 @@ impl<N: NodeIdT> ThresholdDecryption<N> {
         }
         let ct = match self.ciphertext {
             None => return Ok(Step::default()), // Still waiting for the ciphertext.
-            Some(ref ct) => ct,
+            Some(ref ct) => ct.clone(),
         };
         self.terminated = true;
+        let step = self.start_decryption()?; // Before terminating, make sure we sent our share.
         let plaintext = {
             let to_idx = |(id, share)| {
                 let idx = self
@@ -212,9 +242,9 @@ impl<N: NodeIdT> ThresholdDecryption<N> {
             let share_itr = self.shares.iter().map(to_idx);
             self.netinfo
                 .public_key_set()
-                .decrypt(share_itr, ct)
+                .decrypt(share_itr, &ct)
                 .map_err(Error::Decryption)?
         };
-        Ok(Step::default().with_output(plaintext))
+        Ok(step.with_output(plaintext))
     }
 }

src/threshold_sign.rs

@@ -1,8 +1,10 @@
 //! # Collaborative Threshold Signing
 //!
-//! The algorithm is instantiated with data to sign, and waits for the input (no data, just `()`),
-//! then sends a signature share to the others. When at least _f + 1_ correct validators have done
-//! so, each node outputs the same, valid signature of the data.
+//! The algorithm is instantiated and waits to recieve a document to sign, as well as signature
+//! shares from threshold signature validation peers. Once `set_document` is successfully called,
+//! then `handle_input(())` or `sign()` is called, which then sends a signature share to each
+//! threshold signature validation peer. When at least _f + 1_ validators have shared their
+//! signatures in this manner, each node outputs the same, valid signature of the data.
 //!
 //! In addition to signing, this can also be used as a source of pseudorandomness: The signature
 //! cannot be known until more than _f_ validators have contributed their shares.
@@ -23,18 +25,22 @@ use log::debug;
 use rand_derive::Rand;
 use serde_derive::{Deserialize, Serialize};
 
-use fault_log::{Fault, FaultKind};
+use fault_log::{Fault, FaultKind, FaultLog};
 use {DistAlgorithm, NetworkInfo, NodeIdT, Target};
 
 /// A threshold signing error.
 #[derive(Clone, Eq, PartialEq, Debug, Fail)]
 pub enum Error {
+    #[fail(display = "Redundant input provided")]
+    MultipleMessagesToSign,
     #[fail(display = "CombineAndVerifySigCrypto error: {}", _0)]
     CombineAndVerifySigCrypto(crypto::error::Error),
     #[fail(display = "Unknown sender")]
     UnknownSender,
     #[fail(display = "Signature verification failed")]
     VerificationFailed,
+    #[fail(display = "Document hash is not set, cannot sign or verify signatures")]
+    DocumentHashIsNone,
 }
 
 /// A threshold signing result.
@@ -59,8 +65,8 @@ impl Message {
 #[derive(Debug)]
 pub struct ThresholdSign<N> {
     netinfo: Arc<NetworkInfo<N>>,
-    /// The hash of the data to be signed.
-    msg_hash: G2,
+    /// The hash of the document to be signed.
+    doc_hash: Option<G2>,
     /// All received threshold signature shares.
     received_shares: BTreeMap<N, SignatureShare>,
     /// Whether we already sent our shares.
@@ -99,28 +105,49 @@ impl<N: NodeIdT> DistAlgorithm for ThresholdSign<N> {
 }
 
 impl<N: NodeIdT> ThresholdSign<N> {
-    /// Creates a new instance of `ThresholdSign`, with the goal to collaboratively sign `msg`.
-    pub fn new<M: AsRef<[u8]>>(netinfo: Arc<NetworkInfo<N>>, msg: M) -> Self {
+    /// Creates a new instance of `ThresholdSign`, with the goal to collaboratively sign `doc`.
+    pub fn new(netinfo: Arc<NetworkInfo<N>>) -> Self {
         ThresholdSign {
             netinfo,
-            msg_hash: hash_g2(msg),
+            doc_hash: None,
             received_shares: BTreeMap::new(),
             had_input: false,
             terminated: false,
         }
     }
 
+    /// Creates a new instance of `ThresholdSign`, including setting the document to sign.
+    pub fn new_with_document<M: AsRef<[u8]>>(netinfo: Arc<NetworkInfo<N>>, doc: M) -> Result<Self> {
+        let mut ts = ThresholdSign::new(netinfo);
+        ts.set_document(doc)?;
+        Ok(ts)
+    }
+
+    /// Sets doc_hash. Signature shares can only be sent after this function is completed.
+    pub fn set_document<M: AsRef<[u8]>>(&mut self, doc: M) -> Result<()> {
+        if self.doc_hash.is_some() {
+            return Err(Error::MultipleMessagesToSign);
+        }
+        self.doc_hash = Some(hash_g2(doc));
+        Ok(())
+    }
+
     /// Sends our signature shares, and if we have collected enough, returns the full signature.
+    /// Returns an error if the message to sign hasn't been received yet.
     pub fn sign(&mut self) -> Result<Step<N>> {
         if self.had_input {
+            // Don't waste time on redundant shares.
             return Ok(Step::default());
         }
+        let hash = self.doc_hash.ok_or(Error::DocumentHashIsNone)?;
         self.had_input = true;
+        let mut step = Step::default();
+        step.fault_log.extend(self.remove_invalid_shares());
         if !self.netinfo.is_validator() {
-            return self.try_output();
+            return Ok(step.join(self.try_output()?));
         }
-        let msg = Message(self.netinfo.secret_key_share().sign_g2(self.msg_hash));
-        let mut step: Step<_> = Target::All.message(msg.clone()).into();
+        let msg = Message(self.netinfo.secret_key_share().sign_g2(hash));
+        step.messages.push(Target::All.message(msg.clone()));
         let id = self.our_id().clone();
         step.extend(self.handle_message(&id, msg)?);
         Ok(step)
@@ -136,13 +163,11 @@ impl<N: NodeIdT> ThresholdSign<N> {
             return Ok(Step::default());
         }
         let Message(share) = message;
-        if !self
-            .netinfo
+        // Before checking the share, ensure the sender is a known validator
+        self.netinfo
             .public_key_share(sender_id)
-            .ok_or(Error::UnknownSender)?
-            .verify_g2(&share, self.msg_hash)
-        {
-            // Report the faulty node and ignore the invalid share.
+            .ok_or(Error::UnknownSender)?;
+        if !self.is_share_valid(sender_id, &share) {
             let fault_kind = FaultKind::UnverifiedSignatureShareSender;
             return Ok(Fault::new(sender_id.clone(), fault_kind).into());
         }
@@ -150,12 +175,44 @@ impl<N: NodeIdT> ThresholdSign<N> {
         self.try_output()
     }
 
+    /// Removes all shares that are invalid, and returns faults for their senders.
+    fn remove_invalid_shares(&mut self) -> FaultLog<N> {
+        let faulty_senders: Vec<N> = self
+            .received_shares
+            .iter()
+            .filter(|(id, share)| !self.is_share_valid(id, share))
+            .map(|(id, _)| id.clone())
+            .collect();
+        let mut fault_log = FaultLog::default();
+        for id in faulty_senders {
+            self.received_shares.remove(&id);
+            fault_log.append(id, FaultKind::UnverifiedSignatureShareSender);
+        }
+        fault_log
+    }
+
+    /// Returns `true` if the share is valid, or if we don't have the message data yet.
+    fn is_share_valid(&self, id: &N, share: &SignatureShare) -> bool {
+        let hash = match self.doc_hash {
+            None => return true, // No document yet. Verification postponed.
+            Some(ref doc_hash) => doc_hash,
+        };
+        match self.netinfo.public_key_share(id) {
+            None => false, // Unknown sender.
+            Some(pk_i) => pk_i.verify_g2(&share, *hash),
+        }
+    }
+
     fn try_output(&mut self) -> Result<Step<N>> {
-        if self.had_input && self.received_shares.len() > self.netinfo.num_faulty() {
-            let sig = self.combine_and_verify_sig()?;
+        let hash = match self.doc_hash {
+            Some(hash) => hash,
+            None => return Ok(Step::default()),
+        };
+        if !self.terminated && self.received_shares.len() > self.netinfo.num_faulty() {
+            let sig = self.combine_and_verify_sig(hash)?;
+            self.terminated = true;
             let step = self.sign()?; // Before terminating, make sure we sent our share.
             debug!("{} output {:?}", self, sig);
-            self.terminated = true;
             Ok(step.with_output(sig))
         } else {
             debug!(
@@ -168,7 +225,7 @@ impl<N: NodeIdT> ThresholdSign<N> {
         }
     }
 
-    fn combine_and_verify_sig(&self) -> Result<Signature> {
+    fn combine_and_verify_sig(&self, hash: G2) -> Result<Signature> {
         // Pass the indices of sender nodes to `combine_signatures`.
         let to_idx = |(id, share)| (self.netinfo.node_index(id).unwrap(), share);
         let shares = self.received_shares.iter().map(to_idx);
@@ -181,7 +238,7 @@ impl<N: NodeIdT> ThresholdSign<N> {
             .netinfo
             .public_key_set()
             .public_key()
-            .verify_g2(&sig, self.msg_hash)
+            .verify_g2(&sig, hash)
         {
             Err(Error::VerificationFailed)
         } else {
@@ -192,6 +249,6 @@ impl<N: NodeIdT> ThresholdSign<N> {
 
 impl<N: NodeIdT> fmt::Display for ThresholdSign<N> {
     fn fmt(&self, f: &mut fmt::Formatter) -> result::Result<(), fmt::Error> {
-        write!(f, "{:?} TS({:?})", self.our_id(), self.msg_hash)
+        write!(f, "{:?} TS({:?})", self.our_id(), self.doc_hash)
     }
 }

tests/binary_agreement_mitm.rs

@@ -270,7 +270,8 @@ impl AbaCommonCoinAdversary {
                         .expect("Adversary netinfo mutex not populated");
                     let coin_id = bincode::serialize(&(0 as SessionId, epoch))
                         .expect("Failed to serialize coin_id");
-                    let mut coin = ThresholdSign::new(netinfo, coin_id);
+                    let mut coin = ThresholdSign::new_with_document(netinfo, coin_id)
+                        .expect("Failed to set the coin's ID");
                     let _ = coin
                         .handle_input(())
                         .expect("Calling handle_input on Coin failed");

tests/threshold_sign.rs

@@ -106,7 +106,10 @@ where
             let adversary = |_| new_adversary(num_good_nodes, num_faulty_nodes);
             let nonce = format!("My very unique nonce {:x}:{}", unique_id, i);
             info!("Nonce: {}", nonce);
-            let new_coin = |netinfo: _| ThresholdSign::new(netinfo, nonce.clone());
+            let new_coin = |netinfo: _| {
+                ThresholdSign::new_with_document(netinfo, nonce.clone())
+                    .expect("Failed to set the new coin's ID")
+            };
             let network = TestNetwork::new(num_good_nodes, num_faulty_nodes, adversary, new_coin);
             let coin = test_threshold_sign(network).parity();
             if coin {