From 532a9aacc5c40b79f724d7d53239ad6e94b77b07 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?=
 <esteban.dimitroff@lambdaclass.com>
Date: Fri, 22 Mar 2024 14:52:11 -0300
Subject: [PATCH] Fixing check for weight before adding new signature in
 replica commit

---
 node/actors/bft/src/leader/replica_commit.rs  | 24 ++++++++++++-------
 node/actors/bft/src/leader/replica_prepare.rs | 14 +++++------
 .../roles/src/validator/messages/consensus.rs |  2 +-
 node/libs/roles/src/validator/tests.rs        |  4 ++--
 4 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs
index 54fdef2b..03ee1b85 100644
--- a/node/actors/bft/src/leader/replica_commit.rs
+++ b/node/actors/bft/src/leader/replica_commit.rs
@@ -132,13 +132,15 @@ impl StateMachine {
             .entry(message.view.number)
             .or_default();
 
-        // We check validators weight from current messages
-        // TODO: this is wrong, we have to calculate previous weights by proposal
-        let previous_weight = self
-            .config
-            .genesis()
-            .validators
-            .weight_from_msgs(&cache_entry.values().collect());
+        // We check validators weight from current messages, stored by prFoposal
+        let mut by_proposal_before: HashMap<_, Vec<_>> = HashMap::new();
+        let entry_before = cache_entry.clone();
+        for msg in entry_before.values() {
+            by_proposal_before
+                .entry(msg.msg.proposal)
+                .or_default()
+                .push(msg);
+        }
 
         // We store the message in our cache.
         cache_entry.insert(author.clone(), signed_message.clone());
@@ -149,13 +151,19 @@ impl StateMachine {
             by_proposal.entry(msg.msg.proposal).or_default().push(msg);
         }
         let threshold = self.config.genesis().validators.threshold();
-        let Some((_, replica_messages)) = by_proposal
+        let Some((proposal, _replica_messages)) = by_proposal
             .into_iter()
             .find(|(_, v)| self.config.genesis().validators.weight_from_msgs(v) >= threshold)
         else {
             return Ok(());
         };
+
         // Check that previous weight did not reach threshold
+        let previous_weight = self
+            .config
+            .genesis()
+            .validators
+            .weight_from_msgs(&by_proposal_before.entry(proposal).or_default());
         // to ensure this is the first time the threshold has been reached
         debug_assert!(previous_weight < threshold);
 
diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs
index e2a75835..1259bef4 100644
--- a/node/actors/bft/src/leader/replica_prepare.rs
+++ b/node/actors/bft/src/leader/replica_prepare.rs
@@ -144,21 +144,19 @@ impl StateMachine {
             .or_default();
 
         // We check validators weight from current messages
-        let previous_weight = self
+        let msgs_before: Vec<_> = entry.values().collect();
+        let weight_before = self
             .config
             .genesis()
             .validators
-            .weight_from_msgs(&entry.values().collect());
+            .weight_from_msgs(&msgs_before);
 
         // We store the message in our cache.
         entry.insert(author.clone(), signed_message);
 
         // Now we check if we have enough weight to continue.
-        let weight = self
-            .config
-            .genesis()
-            .validators
-            .weight_from_msgs(&entry.values().collect());
+        let msgs: Vec<_> = entry.values().collect();
+        let weight = self.config.genesis().validators.weight_from_msgs(&msgs);
         let threshold = self.config.genesis().validators.threshold();
         if weight < threshold {
             return Ok(());
@@ -170,7 +168,7 @@ impl StateMachine {
 
         // Check that previous weight did not reach threshold
         // to ensure this is the first time the threshold has been reached
-        debug_assert!(previous_weight < threshold);
+        debug_assert!(weight_before < threshold);
 
         // ----------- Update the state machine --------------
 
diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs
index 07e16a6e..8223ca34 100644
--- a/node/libs/roles/src/validator/messages/consensus.rs
+++ b/node/libs/roles/src/validator/messages/consensus.rs
@@ -182,7 +182,7 @@ impl ValidatorCommittee {
     }
 
     /// Compute the sum of signers weights.
-    pub fn weight_from_msgs<T: Variant<Msg>>(&self, signed: &Vec<&validator::Signed<T>>) -> usize {
+    pub fn weight_from_msgs<T: Variant<Msg>>(&self, signed: &[&validator::Signed<T>]) -> usize {
         signed
             .iter()
             .map(|s| {
diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs
index 34649cde..a78315c2 100644
--- a/node/libs/roles/src/validator/tests.rs
+++ b/node/libs/roles/src/validator/tests.rs
@@ -200,7 +200,7 @@ fn test_commit_qc() {
     let ctx = ctx::test_root(&ctx::RealClock);
     let rng = &mut ctx.rng();
 
-    // This will ceate equally weighted validators
+    // This will create equally weighted validators
     let setup1 = Setup::new(rng, 6);
     let setup2 = Setup::new(rng, 6);
     let genesis3 = Genesis {
@@ -239,7 +239,7 @@ fn test_prepare_qc() {
     let ctx = ctx::test_root(&ctx::RealClock);
     let rng = &mut ctx.rng();
 
-    // This will ceate equally weighted validators
+    // This will create equally weighted validators
     let setup1 = Setup::new(rng, 6);
     let setup2 = Setup::new(rng, 6);
     let genesis3 = Genesis {