Corrected thresholds and removed unnecessary weight calculations

node/actors/bft/src/leader/replica_commit.rs

@@ -121,27 +121,24 @@ impl StateMachine {
         // to the same proposal.
 
         // We add the message to the incrementally-constructed QC.
-        self.commit_qcs
+        let commit_qc = self
+            .commit_qcs
             .entry(message.view.number)
-            .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis()))
-            .add(&signed_message, self.config.genesis());
-
-        let cache_entry = self
-            .commit_message_cache
-            .entry(message.view.number)
-            .or_default();
+            .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis()));
+        commit_qc.add(&signed_message, self.config.genesis());
 
         // We store the message in our cache.
-        cache_entry.insert(author.clone(), signed_message.clone());
+        self.commit_message_cache
+            .entry(message.view.number)
+            .or_default()
+            .insert(author.clone(), signed_message.clone());
 
         // Now we check if we have enough weight to continue.
-        let weight = self.config.genesis().validators.weight_from_msgs(
-            cache_entry
-                .values()
-                .filter(|m| m.msg.proposal == message.proposal)
-                .collect::<Vec<_>>()
-                .as_slice(),
-        );
+        let weight = self
+            .config
+            .genesis()
+            .validators
+            .weight_from_signers(commit_qc.signers.clone());
         if weight < self.config.genesis().validators.threshold() {
             return Ok(());
         };

node/actors/bft/src/leader/replica_prepare.rs

@@ -2,7 +2,7 @@
 use super::StateMachine;
 use tracing::instrument;
 use zksync_concurrency::{ctx, error::Wrap};
-use zksync_consensus_roles::validator::{self, ProtocolVersion};
+use zksync_consensus_roles::validator::{self, ProtocolVersion, Signers};
 
 /// Errors that can occur when processing a "replica prepare" message.
 #[derive(Debug, thiserror::Error)]
@@ -132,44 +132,34 @@ impl StateMachine {
         // ----------- All checks finished. Now we process the message. --------------
 
         // We add the message to the incrementally-constructed QC.
-        self.prepare_qcs
+        let prepare_qc = self
+            .prepare_qcs
             .entry(message.view.number)
-            .or_insert_with(|| validator::PrepareQC::new(message.view.clone()))
-            .add(&signed_message, self.config.genesis());
-
-        // Work on current view messages
-        let entry = self
-            .prepare_message_cache
-            .entry(message.view.number)
-            .or_default();
-
-        // We check validators weight from current messages
-        let msgs_before: Vec<_> = entry.values().collect();
-        let weight_before = self
-            .config
-            .genesis()
-            .validators
-            .weight_from_msgs(&msgs_before);
+            .or_insert_with(|| validator::PrepareQC::new(message.view.clone()));
+        prepare_qc.add(&signed_message, self.config.genesis());
 
         // We store the message in our cache.
-        entry.insert(author.clone(), signed_message);
+        self.prepare_message_cache
+            .entry(message.view.number)
+            .or_default()
+            .insert(author.clone(), signed_message.clone());
 
         // Now we check if we have enough weight to continue.
-        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 {
+        let weight = self.config.genesis().validators.weight_from_signers(
+            prepare_qc
+                .map
+                .get(&signed_message.msg)
+                .unwrap_or(&Signers::new(self.config.genesis().validators.len()))
+                .clone(),
+        );
+        if weight < self.config.genesis().validators.threshold() {
             return Ok(());
         }
 
         // Remove replica prepare messages for this view, so that we don't create a new block proposal
         // for this same view if we receive another replica prepare message after this.
         self.prepare_message_cache.remove(&message.view.number);
 
-        // Check that previous weight did not reach threshold
-        // to ensure this is the first time the threshold has been reached
-        debug_assert!(weight_before < threshold);
-
         // ----------- Update the state machine --------------
 
         self.view = message.view.number;

node/actors/bft/src/testonly/ut_harness.rs

@@ -83,7 +83,7 @@ impl UTHarness {
     /// Creates a new `UTHarness` with minimally-significant validator set size.
     pub(crate) async fn new_many(ctx: &ctx::Ctx) -> (UTHarness, BlockStoreRunner) {
         let num_validators = 6;
-        assert!(validator::faulty_replicas(num_validators) > 0);
+        assert!(validator::max_faulty_weight(num_validators as u64) > 0);
         UTHarness::new(ctx, num_validators).await
     }
 
@@ -225,7 +225,7 @@ impl UTHarness {
         msg: ReplicaPrepare,
     ) -> Signed<LeaderPrepare> {
         let expected_validator_weight =
-            self.genesis().validators.max_weight() / self.keys.len() as u64;
+            self.genesis().validators.total_weight() / self.keys.len() as u64;
         let want_threshold = self.genesis().validators.threshold();
         let mut leader_prepare = None;
         let msgs: Vec<_> = self.keys.iter().map(|k| k.sign_msg(msg.clone())).collect();
@@ -262,7 +262,7 @@ impl UTHarness {
         msg: ReplicaCommit,
     ) -> Signed<LeaderCommit> {
         let expected_validator_weight =
-            self.genesis().validators.max_weight() / self.keys.len() as u64;
+            self.genesis().validators.total_weight() / self.keys.len() as u64;
         let want_threshold = self.genesis().validators.threshold();
         let mut first_match = true;
         for (i, key) in self.keys.iter().enumerate() {

node/actors/bft/src/tests.rs

@@ -9,7 +9,10 @@ async fn run_test(behavior: Behavior, network: Network) {
 
     const NODES: usize = 11;
     let mut nodes = vec![behavior; NODES];
-    for n in &mut nodes[0..validator::threshold(NODES)] {
+    // validator::threshold(NODES) will calculate required nodes to validate a message
+    // assuming uniform weight distribution
+    let honest_nodes_amount = validator::threshold(NODES as u64) as usize;
+    for n in &mut nodes[0..honest_nodes_amount] {
         *n = Behavior::Honest;
     }
     Test {

node/libs/roles/src/validator/messages/consensus.rs

@@ -80,10 +80,6 @@ pub struct ValidatorCommittee {
 }
 
 impl ValidatorCommittee {
-    /// Required weight to verify signatures.
-    /// Currently 80.00%
-    const THRESHOLD: u64 = 8000;
-
     /// Creates a new ValidatorCommittee from a list of validator public keys.
     pub fn new(validators: impl IntoIterator<Item = WeightedValidator>) -> anyhow::Result<Self> {
         let mut set = BTreeSet::new();
@@ -150,14 +146,14 @@ impl ValidatorCommittee {
         self.get(index).unwrap().key.clone()
     }
 
-    /// Signature threshold for this validator committee.
+    /// Signature weight threshold for this validator committee.
     pub fn threshold(&self) -> u64 {
-        Self::THRESHOLD
+        threshold(self.total_weight())
     }
 
     /// Maximal number of faulty replicas allowed in this validator committee.
-    pub fn faulty_replicas(&self) -> usize {
-        faulty_replicas(self.len())
+    pub fn max_faulty_weight(&self) -> u64 {
+        max_faulty_weight(self.total_weight())
     }
 
     /// Compute the sum of signers weights.
@@ -170,41 +166,28 @@ impl ValidatorCommittee {
             .sum()
     }
 
-    /// Compute the sum of signers weights.
-    pub fn weight_from_msgs<T: Variant<Msg>>(&self, signed: &[&validator::Signed<T>]) -> u64 {
-        signed
-            .iter()
-            .map(|s| {
-                self.vec[self
-                    .index(&s.key)
-                    .expect("Signer is not in validator committee")]
-                .weight
-            })
-            .sum()
-    }
-
     /// Sum of all validators' weight in the committee
-    pub fn max_weight(&self) -> u64 {
+    pub fn total_weight(&self) -> u64 {
         self.vec.iter().map(|v| v.weight).sum()
     }
 }
 
-/// Calculate the consensus threshold, the minimum number of votes for any consensus action to be valid,
-/// for a given number of replicas.
-pub fn threshold(n: usize) -> usize {
-    n - faulty_replicas(n)
+/// Calculate the consensus threshold, the minimum votes' weight for any consensus action to be valid,
+/// for a given committee total weight.
+pub fn threshold(total_weight: u64) -> u64 {
+    total_weight - max_faulty_weight(total_weight)
 }
 
-/// Calculate the maximum number of faulty replicas, for a given number of replicas.
-pub fn faulty_replicas(n: usize) -> usize {
-    // Calculate the allowed maximum number of faulty replicas. We want the following relationship to hold:
+/// Calculate the maximum allowed weight for faulty replicas, for a given total weight.
+pub fn max_faulty_weight(total_weight: u64) -> u64 {
+    // Calculate the allowed maximum weight of faulty replicas. We want the following relationship to hold:
     //      n = 5*f + 1
-    // for n total replicas and f faulty replicas. This results in the following formula for the maximum
-    // number of faulty replicas:
+    // for n total weight and f faulty weight. This results in the following formula for the maximum
+    // weight of faulty replicas:
     //      f = floor((n - 1) / 5)
-    // Because of this, it doesn't make sense to have 5*f + 2 or 5*f + 3 replicas. It won't increase the number
-    // of allowed faulty replicas.
-    (n - 1) / 5
+    // Because of this, it doesn't make sense to have 5*f + 2 or 5*f + 3 replicas. It won't increase the
+    // allowed weight for faulty replicas.
+    (total_weight - 1) / 5
 }
 
 /// Genesis of the blockchain, unique for each blockchain instance.

node/libs/roles/src/validator/messages/leader_prepare.rs

@@ -56,14 +56,15 @@ impl PrepareQC {
     /// Get the highest block voted and check if there's a quorum of votes for it. To have a quorum
     /// in this situation, we require 2*f+1 votes, where f is the maximum number of faulty replicas.
     pub fn high_vote(&self, genesis: &Genesis) -> Option<BlockHeader> {
-        let mut count: HashMap<_, usize> = HashMap::new();
+        let mut count: HashMap<_, u64> = HashMap::new();
         for (msg, signers) in &self.map {
             if let Some(v) = &msg.high_vote {
-                *count.entry(v.proposal).or_default() += signers.count();
+                *count.entry(v.proposal).or_default() +=
+                    genesis.validators.weight_from_signers(signers.clone());
             }
         }
         // We only take one value from the iterator because there can only be at most one block with a quorum of 2f+1 votes.
-        let min = 2 * genesis.validators.faulty_replicas() + 1;
+        let min = 2 * genesis.validators.max_faulty_weight() + 1;
         count.into_iter().find(|x| x.1 >= min).map(|x| x.0)
     }
 

node/libs/roles/src/validator/testonly.rs

@@ -23,7 +23,7 @@ impl Setup {
     /// New `Setup` with a given `fork`.
     pub fn new_with_fork(rng: &mut impl Rng, validators: usize, fork: Fork) -> Self {
         let keys: Vec<SecretKey> = (0..validators).map(|_| rng.gen()).collect();
-        let weight = 10000 / validators as u64;
+        let weight = 1;
         let genesis = Genesis {
             validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator {
                 key: k.public(),
@@ -301,7 +301,7 @@ impl Distribution<Signers> for Standard {
 impl Distribution<ValidatorCommittee> for Standard {
     fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> ValidatorCommittee {
         let count = rng.gen_range(1..11);
-        let weight = 10000 / count;
+        let weight = 1;
         let public_keys = (0..count).map(|_| WeightedValidator {
             key: rng.gen(),
             weight,

node/libs/roles/src/validator/tests.rs

@@ -208,7 +208,7 @@ fn test_commit_qc() {
             .unwrap(),
         fork: setup1.genesis.fork.clone(),
     };
-    let validator_weight = setup1.genesis.validators.max_weight();
+    let validator_weight = setup1.genesis.validators.total_weight();
 
     for i in 0..setup1.keys.len() + 1 {
         let view = rng.gen();
@@ -260,7 +260,7 @@ fn test_prepare_qc() {
                 &setup1.genesis,
             );
         }
-        let expected_weight = n as u64 * setup1.genesis.validators.max_weight() / 6;
+        let expected_weight = n as u64 * setup1.genesis.validators.total_weight() / 6;
         if expected_weight >= setup1.genesis.validators.threshold() {
             qc.verify(&setup1.genesis).unwrap();
         } else {