From fc5a921466d0bc5aaecab831686538e3305628d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Tue, 26 Mar 2024 10:15:04 -0300 Subject: [PATCH] Simplified ValidatorCommittee internal structure and methods --- node/actors/bft/src/leader/replica_commit.rs | 17 +------- node/actors/executor/src/lib.rs | 2 +- node/actors/network/src/consensus/mod.rs | 2 +- node/actors/network/src/testonly.rs | 2 +- node/libs/roles/src/validator/conv.rs | 6 +-- .../roles/src/validator/messages/consensus.rs | 42 +++++++++---------- .../src/validator/messages/leader_commit.rs | 2 +- .../src/validator/messages/leader_prepare.rs | 2 +- node/libs/roles/src/validator/tests.rs | 12 ++---- 9 files changed, 31 insertions(+), 56 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index d22ec8a6..8a513386 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -131,15 +131,6 @@ impl StateMachine { .entry(message.view.number) .or_default(); - // We check validators weight from current messages, stored by proposal - let weight_before = self.config.genesis().validators.weight_from_msgs( - cache_entry - .values() - .filter(|m| m.msg.proposal == message.proposal) - .collect::>() - .as_slice(), - ); - // We store the message in our cache. cache_entry.insert(author.clone(), signed_message.clone()); @@ -151,17 +142,11 @@ impl StateMachine { .collect::>() .as_slice(), ); - let threshold = self.config.genesis().validators.threshold(); - if weight < threshold { + if weight < self.config.genesis().validators.threshold() { return Ok(()); }; - // 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 -------------- - let now = ctx.now(); metrics::METRICS .leader_commit_phase_latency diff --git a/node/actors/executor/src/lib.rs b/node/actors/executor/src/lib.rs index 2e0fad85..1fabc8be 100644 --- a/node/actors/executor/src/lib.rs +++ b/node/actors/executor/src/lib.rs @@ -106,7 +106,7 @@ impl Executor { .block_store .genesis() .validators - .iter() + .iter_keys() .any(|key| key == &validator.key.public()) { anyhow::bail!("this validator doesn't belong to the consensus"); diff --git a/node/actors/network/src/consensus/mod.rs b/node/actors/network/src/consensus/mod.rs index 32a90540..f025b086 100644 --- a/node/actors/network/src/consensus/mod.rs +++ b/node/actors/network/src/consensus/mod.rs @@ -63,7 +63,7 @@ impl Network { /// Constructs a new consensus network state. pub(crate) fn new(ctx: &ctx::Ctx, gossip: Arc) -> Option> { let key = gossip.cfg.validator_key.clone()?; - let validators: HashSet<_> = gossip.genesis().validators.iter().cloned().collect(); + let validators: HashSet<_> = gossip.genesis().validators.iter_keys().cloned().collect(); Some(Arc::new(Self { key, inbound: PoolWatch::new(validators.clone(), 0), diff --git a/node/actors/network/src/testonly.rs b/node/actors/network/src/testonly.rs index 022bb747..fcb2152a 100644 --- a/node/actors/network/src/testonly.rs +++ b/node/actors/network/src/testonly.rs @@ -196,7 +196,7 @@ impl Instance { pub async fn wait_for_consensus_connections(&self) { let consensus_state = self.net.consensus.as_ref().unwrap(); - let want: HashSet<_> = self.genesis().validators.iter().cloned().collect(); + let want: HashSet<_> = self.genesis().validators.iter_keys().cloned().collect(); consensus_state .inbound .subscribe() diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index f3a5163c..e874edf3 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -46,11 +46,7 @@ impl ProtoFmt for Genesis { fn build(&self) -> Self::Proto { Self::Proto { fork: Some(self.fork.build()), - validators: self - .validators - .weighted_validators_iter() - .map(|v| v.build()) - .collect(), + validators: self.validators.iter().map(|v| v.build()).collect(), } } } diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index f56c3f81..b844428a 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -5,7 +5,6 @@ use bit_vec::BitVec; use std::{ collections::{BTreeMap, BTreeSet}, fmt, - iter::zip, }; use zksync_consensus_crypto::{keccak256::Keccak256, ByteFmt, Text, TextFmt}; use zksync_consensus_utils::enum_util::{BadVariantError, Variant}; @@ -76,9 +75,8 @@ impl Default for Fork { /// We represent each validator by its validator public key. #[derive(Clone, Debug, PartialEq, Eq)] pub struct ValidatorCommittee { - vec: Vec, + vec: Vec, indexes: BTreeMap, - weights: Vec, } impl ValidatorCommittee { @@ -89,41 +87,40 @@ impl ValidatorCommittee { /// Creates a new ValidatorCommittee from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); - let mut weights = BTreeMap::new(); + let mut weighted_validators = BTreeMap::new(); for validator in validators { anyhow::ensure!( set.insert(validator.key.clone()), "Duplicate validator in ValidatorCommittee" ); - weights.insert(validator.key, validator.weight); + weighted_validators.insert(validator.key.clone(), validator); } anyhow::ensure!( !set.is_empty(), "ValidatorCommittee must contain at least one validator" ); Ok(Self { - vec: set.iter().cloned().collect(), + vec: set + .iter() + .map(|key| weighted_validators[key].clone()) + .collect(), indexes: set .clone() .into_iter() .enumerate() .map(|(i, pk)| (pk, i)) .collect(), - weights: set.iter().map(|pk| weights[pk]).collect(), }) } - /// Iterates over validators. - pub fn iter(&self) -> impl Iterator { + /// Iterates over weighted validators. + pub fn iter(&self) -> impl Iterator { self.vec.iter() } - /// Iterates over validators. - pub fn weighted_validators_iter(&self) -> impl Iterator + '_ { - zip(&self.vec, &self.weights).map(|(key, weight)| WeightedValidator { - key: key.clone(), - weight: *weight, - }) + /// Iterates over validator keys. + pub fn iter_keys(&self) -> impl Iterator { + self.vec.iter().map(|v| &v.key) } /// Returns the number of validators. @@ -138,7 +135,7 @@ impl ValidatorCommittee { } /// Get validator by its index in the committee. - pub fn get(&self, index: usize) -> Option<&validator::PublicKey> { + pub fn get(&self, index: usize) -> Option<&WeightedValidator> { self.vec.get(index) } @@ -150,7 +147,7 @@ impl ValidatorCommittee { /// Computes the validator for the given view. pub fn view_leader(&self, view_number: ViewNumber) -> validator::PublicKey { let index = view_number.0 as usize % self.len(); - self.get(index).unwrap().clone() + self.get(index).unwrap().key.clone() } /// Signature threshold for this validator committee. @@ -165,11 +162,11 @@ impl ValidatorCommittee { /// Compute the sum of signers weights. pub fn weight_from_signers(&self, signers: Signers) -> u64 { - self.weights + self.vec .iter() .enumerate() .filter(|(i, _)| signers.0[*i]) - .map(|(_, weight)| weight) + .map(|(_, v)| v.weight) .sum() } @@ -178,16 +175,17 @@ impl ValidatorCommittee { signed .iter() .map(|s| { - self.weights[self + 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 { - self.weights.iter().sum() + self.vec.iter().map(|v| v.weight).sum() } } @@ -423,7 +421,7 @@ pub enum Phase { } /// Validator representation inside a ValidatorCommittee. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct WeightedValidator { /// Validator key pub key: validator::PublicKey, diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index 91f7d40d..f6093796 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -113,7 +113,7 @@ impl CommitQC { // Now we can verify the signature. let messages_and_keys = genesis .validators - .iter() + .iter_keys() .enumerate() .filter(|(i, _)| self.signers.0[*i]) .map(|(_, pk)| (self.message.clone(), pk)); diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index af722dc8..206de4b1 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -140,7 +140,7 @@ impl PrepareQC { let messages_and_keys = self.map.clone().into_iter().flat_map(|(msg, signers)| { genesis .validators - .iter() + .iter_keys() .enumerate() .filter(|(i, _)| signers.0[*i]) .map(|(_, pk)| (msg.clone(), pk)) diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index da9c9204..b4f95b8e 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -204,10 +204,8 @@ fn test_commit_qc() { let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { - validators: ValidatorCommittee::new( - setup1.genesis.validators.weighted_validators_iter().take(3), - ) - .unwrap(), + validators: ValidatorCommittee::new(setup1.genesis.validators.iter().take(3).cloned()) + .unwrap(), fork: setup1.genesis.fork.clone(), }; let validator_weight = setup1.genesis.validators.max_weight(); @@ -244,10 +242,8 @@ fn test_prepare_qc() { let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { - validators: ValidatorCommittee::new( - setup1.genesis.validators.weighted_validators_iter().take(3), - ) - .unwrap(), + validators: ValidatorCommittee::new(setup1.genesis.validators.iter().take(3).cloned()) + .unwrap(), fork: setup1.genesis.fork.clone(), };