diff --git a/Code/common/src/vote.rs b/Code/common/src/vote.rs index 6dd388d13..cb09c58c3 100644 --- a/Code/common/src/vote.rs +++ b/Code/common/src/vote.rs @@ -23,9 +23,12 @@ where /// The round for which the vote is for. fn round(&self) -> Round; - /// The value being voted for. + /// Get a reference to the value being voted for. fn value(&self) -> Option<&::Id>; + /// Take ownership of the value being voted for. + fn take_value(self) -> Option<::Id>; + /// The type of vote. fn vote_type(&self) -> VoteType; diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index 7add9bee1..814158cbf 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -165,7 +165,7 @@ where self.apply_event(proposal.round(), event) } Round::Some(_) - if self.votes.check_threshold( + if self.votes.is_threshold_met( &proposal.pol_round(), VoteType::Prevote, Threshold::Value(proposal.value().id()), diff --git a/Code/test/src/vote.rs b/Code/test/src/vote.rs index 761101aeb..6ba30a3e0 100644 --- a/Code/test/src/vote.rs +++ b/Code/test/src/vote.rs @@ -40,6 +40,10 @@ impl malachite_common::Vote for Vote { self.value.as_ref() } + fn take_value(self) -> Option { + self.value + } + fn vote_type(&self) -> VoteType { self.typ } diff --git a/Code/test/tests/vote_count.rs b/Code/test/tests/round_votes.rs similarity index 100% rename from Code/test/tests/vote_count.rs rename to Code/test/tests/round_votes.rs diff --git a/Code/vote/src/count.rs b/Code/vote/src/count.rs index fb627ca9b..b124e0b26 100644 --- a/Code/vote/src/count.rs +++ b/Code/vote/src/count.rs @@ -1,13 +1,13 @@ use alloc::collections::BTreeMap; -use malachite_common::{Consensus, ValueId, Vote}; - +// TODO: Introduce newtype +// QUESTION: Over what type? i64? pub type Weight = u64; /// A value and the weight of votes for it. #[derive(Clone, Debug, PartialEq, Eq)] pub struct ValuesWeights { - value_weights: BTreeMap, + value_weights: BTreeMap, Weight>, } impl ValuesWeights { @@ -18,21 +18,26 @@ impl ValuesWeights { } /// Add weight to the value and return the new weight. - pub fn add_weight(&mut self, value: ValueId, weight: Weight) -> Weight + pub fn add(&mut self, value: Option, weight: Weight) -> Weight where ValueId: Ord, { let entry = self.value_weights.entry(value).or_insert(0); - *entry += weight; + *entry += weight; // FIXME: Deal with overflows *entry } - /// Return the value with the highest weight and said weight, if any. - pub fn highest_weighted_value(&self) -> Option<(&ValueId, Weight)> { - self.value_weights - .iter() - .max_by_key(|(_, weight)| *weight) - .map(|(value, weight)| (value, *weight)) + /// Return the weight of the value, or 0 if it is not present. + pub fn get(&self, value: &Option) -> Weight + where + ValueId: Ord, + { + self.value_weights.get(value).cloned().unwrap_or(0) + } + + /// Return the sum of the weights of all values. + pub fn sum(&self) -> Weight { + self.value_weights.values().sum() // FIXME: Deal with overflows } } @@ -45,64 +50,68 @@ impl Default for ValuesWeights { /// VoteCount tallys votes of the same type. /// Votes are for nil or for some value. #[derive(Clone, Debug, Default, PartialEq, Eq)] -pub struct VoteCount -where - C: Consensus, -{ - // Weight of votes for nil - pub nil: Weight, - /// Weight of votes for the values - pub values_weights: ValuesWeights>, +pub struct VoteCount { + /// Weight of votes for the values, including nil + pub values_weights: ValuesWeights, + /// Total weight pub total: Weight, } -impl VoteCount -where - C: Consensus, -{ +impl VoteCount { pub fn new(total: Weight) -> Self { VoteCount { - nil: 0, total, values_weights: ValuesWeights::new(), } } - /// Add vote to internal counters and return the highest threshold. - pub fn add_vote(&mut self, vote: C::Vote, weight: Weight) -> Threshold> { - if let Some(value) = vote.value() { - let new_weight = self.values_weights.add_weight(value.clone(), weight); + /// Add vote for a vlaue to internal counters and return the highest threshold. + pub fn add_vote(&mut self, value: Option, weight: Weight) -> Threshold + where + Value: Clone + Ord, + { + let new_weight = self.values_weights.add(value.clone(), weight); - // Check if we have a quorum for this value. - if is_quorum(new_weight, self.total) { - return Threshold::Value(value.clone()); - } - } else { - self.nil += weight; + match value { + Some(value) if is_quorum(new_weight, self.total) => Threshold::Value(value), - // Check if we have a quorum for nil. - if is_quorum(self.nil, self.total) { - return Threshold::Nil; - } - } + None if is_quorum(new_weight, self.total) => Threshold::Nil, + + _ => { + let sum_weight = self.values_weights.sum(); - // Check if we have a quorum for any value, using the highest weighted value, if any. - if let Some((_max_value, max_weight)) = self.values_weights.highest_weighted_value() { - if is_quorum(max_weight + self.nil, self.total) { - return Threshold::Any; + if is_quorum(sum_weight, self.total) { + Threshold::Any + } else { + Threshold::Init + } } } - - // No quorum - Threshold::Init } - pub fn check_threshold(&self, threshold: Threshold>) -> bool { + + /// Return whether or not the threshold is met, ie. if we have a quorum for that threshold. + pub fn is_threshold_met(&self, threshold: Threshold) -> bool + where + Value: Clone + Ord, + { match threshold { + Threshold::Value(value) => { + let weight = self.values_weights.get(&Some(value)); + is_quorum(weight, self.total) + } + + Threshold::Nil => { + let weight = self.values_weights.get(&None); + is_quorum(weight, self.total) + } + + Threshold::Any => { + let sum_weight = self.values_weights.sum(); + is_quorum(sum_weight, self.total) + } + Threshold::Init => false, - Threshold::Any => self.values_weights.highest_weighted_value().is_some(), - Threshold::Nil => self.nil > 0, - Threshold::Value(value) => self.values_weights.value_weights.contains_key(&value), } } } @@ -125,9 +134,122 @@ pub enum Threshold { } /// Returns whether or note `value > (2/3)*total`. -pub fn is_quorum(value: Weight, total: Weight) -> bool { +fn is_quorum(value: Weight, total: Weight) -> bool { 3 * value > 2 * total } #[cfg(test)] -mod tests {} +mod tests { + use super::*; + + #[test] + fn values_weights() { + let mut vw = ValuesWeights::new(); + + assert_eq!(vw.get(&None), 0); + assert_eq!(vw.get(&Some(1)), 0); + + assert_eq!(vw.add(None, 1), 1); + assert_eq!(vw.get(&None), 1); + assert_eq!(vw.get(&Some(1)), 0); + + assert_eq!(vw.add(Some(1), 1), 1); + assert_eq!(vw.get(&None), 1); + assert_eq!(vw.get(&Some(1)), 1); + + assert_eq!(vw.add(None, 1), 2); + assert_eq!(vw.get(&None), 2); + assert_eq!(vw.get(&Some(1)), 1); + + assert_eq!(vw.add(Some(1), 1), 2); + assert_eq!(vw.get(&None), 2); + assert_eq!(vw.get(&Some(1)), 2); + + assert_eq!(vw.add(Some(2), 1), 1); + assert_eq!(vw.get(&None), 2); + assert_eq!(vw.get(&Some(1)), 2); + assert_eq!(vw.get(&Some(2)), 1); + + // FIXME: Test for and deal with overflows + } + + #[test] + #[allow(clippy::bool_assert_comparison)] + fn vote_count_nil() { + let mut vc = VoteCount::new(4); + + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), false); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(None, 1), Threshold::Init); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), false); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(None, 1), Threshold::Init); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), false); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(None, 1), Threshold::Nil); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), true); + assert_eq!(vc.is_threshold_met(Threshold::Nil), true); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(Some(1), 1), Threshold::Any); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), true); + assert_eq!(vc.is_threshold_met(Threshold::Nil), true); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + } + + #[test] + #[allow(clippy::bool_assert_comparison)] + fn vote_count_value() { + let mut vc = VoteCount::new(4); + + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), false); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(Some(1), 1), Threshold::Init); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), false); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(Some(1), 1), Threshold::Init); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), false); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(Some(1), 1), Threshold::Value(1)); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), true); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), true); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + + assert_eq!(vc.add_vote(Some(2), 1), Threshold::Any); + assert_eq!(vc.is_threshold_met(Threshold::Init), false); + assert_eq!(vc.is_threshold_met(Threshold::Any), true); + assert_eq!(vc.is_threshold_met(Threshold::Nil), false); + assert_eq!(vc.is_threshold_met(Threshold::Value(1)), true); + assert_eq!(vc.is_threshold_met(Threshold::Value(2)), false); + } +} diff --git a/Code/vote/src/keeper.rs b/Code/vote/src/keeper.rs index a24f63829..5299361f5 100644 --- a/Code/vote/src/keeper.rs +++ b/Code/vote/src/keeper.rs @@ -47,7 +47,8 @@ where Self::to_event(vote_type, threshold) } - pub fn check_threshold( + /// Check if a threshold is met, ie. if we have a quorum for that threshold. + pub fn is_threshold_met( &self, round: &Round, vote_type: VoteType, @@ -59,8 +60,8 @@ where }; match vote_type { - VoteType::Prevote => round.prevotes.check_threshold(threshold), - VoteType::Precommit => round.precommits.check_threshold(threshold), + VoteType::Prevote => round.prevotes.is_threshold_met(threshold), + VoteType::Precommit => round.precommits.is_threshold_met(threshold), } } diff --git a/Code/vote/src/lib.rs b/Code/vote/src/lib.rs index 78863e9c8..1947b1d87 100644 --- a/Code/vote/src/lib.rs +++ b/Code/vote/src/lib.rs @@ -28,8 +28,8 @@ where pub height: C::Height, pub round: Round, - pub prevotes: VoteCount, - pub precommits: VoteCount, + pub prevotes: VoteCount>, + pub precommits: VoteCount>, } impl RoundVotes @@ -47,8 +47,8 @@ where pub fn add_vote(&mut self, vote: C::Vote, weight: Weight) -> Threshold> { match vote.vote_type() { - VoteType::Prevote => self.prevotes.add_vote(vote, weight), - VoteType::Precommit => self.precommits.add_vote(vote, weight), + VoteType::Prevote => self.prevotes.add_vote(vote.take_value(), weight), + VoteType::Precommit => self.precommits.add_vote(vote.take_value(), weight), } } } diff --git a/codecov.yml b/codecov.yml index 97d7c517a..4a394a747 100644 --- a/codecov.yml +++ b/codecov.yml @@ -21,4 +21,6 @@ coverage: paths: - "Code" - changes: true + changes: + default: + informational: true