From 49efb35744ee244189c8b83cb93bfde11c4baa91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Wed, 27 Mar 2024 09:51:50 -0300 Subject: [PATCH] Corrected thresholds and removed unnecessary weight calculations --- node/actors/bft/src/leader/replica_commit.rs | 29 +++++------ node/actors/bft/src/leader/replica_prepare.rs | 44 +++++++--------- node/actors/bft/src/testonly/ut_harness.rs | 6 +-- node/actors/bft/src/tests.rs | 5 +- .../roles/src/validator/messages/consensus.rs | 51 +++++++------------ .../src/validator/messages/leader_prepare.rs | 7 +-- node/libs/roles/src/validator/testonly.rs | 4 +- node/libs/roles/src/validator/tests.rs | 4 +- 8 files changed, 62 insertions(+), 88 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index 8a513386..72a616d8 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/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::>() - .as_slice(), - ); + let weight = self + .config + .genesis() + .validators + .weight_from_signers(commit_qc.signers.clone()); if weight < self.config.genesis().validators.threshold() { return Ok(()); }; diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index 1259bef4..5d3b92b3 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/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,33 +132,27 @@ 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(()); } @@ -166,10 +160,6 @@ impl StateMachine { // 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; diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index 93a3b681..daf0c15b 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/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 { 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 { 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() { diff --git a/node/actors/bft/src/tests.rs b/node/actors/bft/src/tests.rs index e6bb83b5..ba5c7a9d 100644 --- a/node/actors/bft/src/tests.rs +++ b/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 { diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index b844428a..183bb4b2 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/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) -> anyhow::Result { 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>(&self, signed: &[&validator::Signed]) -> 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. diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 206de4b1..660d4008 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/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 { - 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) } diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 1e5c69b8..a9337ae7 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/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 = (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 for Standard { impl Distribution for Standard { fn sample(&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, diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index b4f95b8e..e1c3ce00 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/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 {