diff --git a/src/dynamic_honey_badger/mod.rs b/src/dynamic_honey_badger/mod.rs index 60e8b9e..148537a 100644 --- a/src/dynamic_honey_badger/mod.rs +++ b/src/dynamic_honey_badger/mod.rs @@ -153,8 +153,7 @@ where Message::HoneyBadger(_, hb_msg) => self.handle_honey_badger_message(sender_id, hb_msg), Message::KeyGen(_, kg_msg, sig) => self.handle_key_gen_message(sender_id, kg_msg, *sig), Message::SignedVote(signed_vote) => { - self.vote_counter.add_pending_vote(signed_vote)?; - Ok(FaultLog::new()) + self.vote_counter.add_pending_vote(sender_id, signed_vote) } } } @@ -259,7 +258,8 @@ where let mut batch = Batch::new(hb_batch.epoch + self.start_epoch); // Add the user transactions to `batch` and handle votes and DKG messages. for (id, int_contrib) in hb_batch.contributions { - self.vote_counter.add_committed_votes(int_contrib.votes)?; + let votes = int_contrib.votes; + fault_log.extend(self.vote_counter.add_committed_votes(&id, votes)?); batch.contributions.insert(id, int_contrib.contrib); for SignedKeyGenMsg(epoch, s_id, kg_msg, sig) in int_contrib.key_gen_messages { if epoch < self.start_epoch { diff --git a/src/dynamic_honey_badger/votes.rs b/src/dynamic_honey_badger/votes.rs index dd5fb2b..b75ad1e 100644 --- a/src/dynamic_honey_badger/votes.rs +++ b/src/dynamic_honey_badger/votes.rs @@ -9,6 +9,7 @@ use serde::{Deserialize, Serialize}; use super::{Change, Result}; use crypto::Signature; +use fault_log::{Fault, FaultKind, FaultLog}; /// A buffer and counter collecting pending and committed votes for validator set changes. /// @@ -59,13 +60,16 @@ where } /// Inserts a pending vote into the buffer, if it has a higher number than the existing one. - // TODO: Detect and report double votes where feasible. - pub fn add_pending_vote(&mut self, signed_vote: SignedVote) -> Result<()> { - if !self.validate(&signed_vote)? { - return Ok(()); - } + pub fn add_pending_vote( + &mut self, + sender_id: &NodeUid, + signed_vote: SignedVote, + ) -> Result> { if signed_vote.vote.era != self.era { - return Ok(()); // The vote is obsolete. + return Ok(FaultLog::new()); // The vote is obsolete. + } + if !self.validate(&signed_vote)? { + return Ok(Fault::new(sender_id.clone(), FaultKind::InvalidVoteSignature).into()); } match self.pending.entry(signed_vote.voter.clone()) { btree_map::Entry::Vacant(entry) => { @@ -77,7 +81,7 @@ where } } } - Ok(()) + Ok(FaultLog::new()) } /// Returns an iterator over all pending votes that are newer than their voter's committed @@ -91,20 +95,29 @@ where } // TODO: Document and return fault logs? - pub fn add_committed_votes(&mut self, signed_votes: I) -> Result<()> + pub fn add_committed_votes( + &mut self, + proposer_id: &NodeUid, + signed_votes: I, + ) -> Result> where I: IntoIterator>, { + let mut fault_log = FaultLog::new(); for signed_vote in signed_votes { - self.add_committed_vote(signed_vote)?; + fault_log.extend(self.add_committed_vote(proposer_id, signed_vote)?); } - Ok(()) + Ok(fault_log) } /// Inserts a committed vote into the counter, if it has a higher number than the existing one. - pub fn add_committed_vote(&mut self, signed_vote: SignedVote) -> Result<()> { + pub fn add_committed_vote( + &mut self, + proposer_id: &NodeUid, + signed_vote: SignedVote, + ) -> Result> { if !self.validate(&signed_vote)? || signed_vote.vote.era != self.era { - return Ok(()); + return Ok(Fault::new(proposer_id.clone(), FaultKind::InvalidCommittedVote).into()); } match self.committed.entry(signed_vote.voter.clone()) { btree_map::Entry::Vacant(entry) => { @@ -116,7 +129,7 @@ where } } } - Ok(()) + Ok(FaultLog::new()) } /// Returns the change that has a majority of votes, if any. @@ -175,6 +188,7 @@ mod tests { use super::{Change, SignedVote, VoteCounter}; use crypto::SecretKeySet; + use fault_log::{Fault, FaultKind, FaultLog}; use messaging::NetworkInfo; /// Returns a vector of `node_num` `VoteCounter`s, and some signed example votes. @@ -213,13 +227,22 @@ mod tests { let ct = &mut counters[0]; // Node 0 already contains its own vote for `Remove(3)`. Add two more. - ct.add_pending_vote(sv[1][2].clone()).expect("add pending"); - ct.add_pending_vote(sv[2][1].clone()).expect("add pending"); + let faults = ct + .add_pending_vote(&1, sv[1][2].clone()) + .expect("add pending"); + assert_eq!(faults, FaultLog::new()); + let faults = ct + .add_pending_vote(&2, sv[2][1].clone()) + .expect("add pending"); + assert_eq!(faults, FaultLog::new()); // Include a vote with a wrong signature. - ct.add_pending_vote(SignedVote { + let fake_vote = SignedVote { sig: sv[2][1].sig.clone(), ..sv[3][1].clone() - }).expect("add pending"); + }; + let faults = ct.add_pending_vote(&1, fake_vote).expect("add pending"); + let expected_fault = Fault::new(1, FaultKind::InvalidVoteSignature); + assert_eq!(faults, expected_fault.into()); assert_eq!( ct.pending_votes().collect::>(), vec![&sv[0][3], &sv[1][2], &sv[2][1]] @@ -227,8 +250,14 @@ mod tests { // Now add an older vote by node 1 and a newer one by node 2. Only the latter should be // included. - ct.add_pending_vote(sv[1][1].clone()).expect("add pending"); - ct.add_pending_vote(sv[2][2].clone()).expect("add pending"); + let faults = ct + .add_pending_vote(&3, sv[1][1].clone()) + .expect("add pending"); + assert_eq!(faults, FaultLog::new()); + let faults = ct + .add_pending_vote(&1, sv[2][2].clone()) + .expect("add pending"); + assert_eq!(faults, FaultLog::new()); assert_eq!( ct.pending_votes().collect::>(), vec![&sv[0][3], &sv[1][2], &sv[2][2]] @@ -236,7 +265,8 @@ mod tests { // Adding a committed vote removes it from the pending ones, unless it is older. let vote_batch = vec![sv[1][3].clone(), sv[2][1].clone(), sv[0][3].clone()]; - ct.add_committed_votes(vote_batch).expect("add committed"); + ct.add_committed_votes(&1, vote_batch) + .expect("add committed"); assert_eq!(ct.pending_votes().collect::>(), vec![&sv[2][2]]); } @@ -255,12 +285,18 @@ mod tests { sig: sv[2][1].sig.clone(), ..sv[3][1].clone() }); - ct.add_committed_votes(vote_batch).expect("add committed"); + let faults = ct + .add_committed_votes(&1, vote_batch) + .expect("add committed"); + let expected_fault = Fault::new(1, FaultKind::InvalidCommittedVote); + assert_eq!(faults, expected_fault.into(),); assert_eq!(ct.compute_majority(), None); // Adding the third vote for `Remove(1)` should return the change: It has the majority. - ct.add_committed_vote(sv[3][1].clone()) + let faults = ct + .add_committed_vote(&1, sv[3][1].clone()) .expect("add committed"); + assert_eq!(faults, FaultLog::new()); assert_eq!(ct.compute_majority(), Some(&Change::Remove(1))); } } diff --git a/src/fault_log.rs b/src/fault_log.rs index c06a2ef..ae4d768 100644 --- a/src/fault_log.rs +++ b/src/fault_log.rs @@ -33,12 +33,16 @@ pub enum FaultKind { InvalidAcceptMessage, /// `DynamicHoneyBadger`/`SyncKeyGen` received an invalid Propose message. InvalidProposeMessage, + /// `DynamicHoneyBadger` received a change vote with an invalid signature. + InvalidVoteSignature, + /// A validator committed an invalid vote in `DynamicHoneyBadger`. + InvalidCommittedVote, } /// A structure representing the context of a faulty node. This structure /// describes which node is faulty (`node_id`) and which faulty behavior /// that the node exhibited ('kind'). -#[derive(Clone)] +#[derive(Clone, Debug, PartialEq)] pub struct Fault { pub node_id: NodeUid, pub kind: FaultKind, @@ -59,7 +63,7 @@ impl Into> for Fault { } /// A structure used to contain reports of faulty node behavior. -#[derive(Clone)] +#[derive(Clone, Debug, PartialEq)] pub struct FaultLog(pub Vec>); impl FaultLog {