Refactor VoteCount (#15)

* Rename `check_threshold` to `is_threshold_met`

* Track nil as a regular value in the vote count

* Add unit test for `ValuesWeights`

* Remove dependency on `Consensus` trait in `VoteCount`

* Do not fail PRs on unexpected coverage changes not visible in diff

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<&<C::Value as Value>::Id>;
 
+    /// Take ownership of the value being voted for.
+    fn take_value(self) -> Option<<C::Value as Value>::Id>;
+
     /// The type of vote.
     fn vote_type(&self) -> VoteType;
 

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()),

Code/test/src/vote.rs

@@ -40,6 +40,10 @@ impl malachite_common::Vote<TestConsensus> for Vote {
         self.value.as_ref()
     }
 
+    fn take_value(self) -> Option<ValueId> {
+        self.value
+    }
+
     fn vote_type(&self) -> VoteType {
         self.typ
     }

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<ValueId> {
-    value_weights: BTreeMap<ValueId, Weight>,
+    value_weights: BTreeMap<Option<ValueId>, Weight>,
 }
 
 impl<ValueId> ValuesWeights<ValueId> {
@@ -18,21 +18,26 @@ impl<ValueId> ValuesWeights<ValueId> {
     }
 
     /// 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<ValueId>, 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<ValueId>) -> 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<ValueId> Default for ValuesWeights<ValueId> {
 /// VoteCount tallys votes of the same type.
 /// Votes are for nil or for some value.
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
-pub struct VoteCount<C>
-where
-    C: Consensus,
-{
-    // Weight of votes for nil
-    pub nil: Weight,
-    /// Weight of votes for the values
-    pub values_weights: ValuesWeights<ValueId<C>>,
+pub struct VoteCount<Value> {
+    /// Weight of votes for the values, including nil
+    pub values_weights: ValuesWeights<Value>,
+
     /// Total weight
     pub total: Weight,
 }
 
-impl<C> VoteCount<C>
-where
-    C: Consensus,
-{
+impl<Value> VoteCount<Value> {
     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<ValueId<C>> {
-        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<Value>, weight: Weight) -> Threshold<Value>
+    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<ValueId<C>>) -> 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<Value>) -> 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<ValueId> {
 }
 
 /// 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);
+    }
+}

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),
         }
     }
 

Code/vote/src/lib.rs

@@ -28,8 +28,8 @@ where
     pub height: C::Height,
     pub round: Round,
 
-    pub prevotes: VoteCount<C>,
-    pub precommits: VoteCount<C>,
+    pub prevotes: VoteCount<ValueId<C>>,
+    pub precommits: VoteCount<ValueId<C>>,
 }
 
 impl<C> RoundVotes<C>
@@ -47,8 +47,8 @@ where
 
     pub fn add_vote(&mut self, vote: C::Vote, weight: Weight) -> Threshold<ValueId<C>> {
         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),
         }
     }
 }

codecov.yml

@@ -21,4 +21,6 @@ coverage:
         paths:
           - "Code"
 
-    changes: true
+    changes:
+      default:
+        informational: true