From 08dbad07142715beb1a3982ef4ab9c0ee250db07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Mon, 11 Mar 2024 10:32:45 -0300 Subject: [PATCH 01/35] Added weights to ValidatorSet --- node/libs/roles/src/validator/conv.rs | 3 ++- .../roles/src/validator/messages/consensus.rs | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 04886834..acba464d 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -36,7 +36,8 @@ impl ProtoFmt for Genesis { .validators .iter() .enumerate() - .map(|(i, v)| PublicKey::read(v).context(i)) + // TODO: obtain weight + .map(|(i, v)| PublicKey::read(v).context(i).map(|key| (key, 0))) .collect::>() .context("validators")?; Ok(Self { diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 870355b0..5c0fa63a 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -76,20 +76,24 @@ impl Default for Fork { } } + /// A struct that represents a set of validators. It is used to store the current validator set. /// We represent each validator by its validator public key. #[derive(Clone, Debug, PartialEq, Eq)] pub struct ValidatorSet { vec: Vec, - map: BTreeMap, + indexes: BTreeMap, + weights: BTreeMap, } impl ValidatorSet { /// Creates a new ValidatorSet from a list of validator public keys. - pub fn new(validators: impl IntoIterator) -> anyhow::Result { + pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); - for validator in validators { - anyhow::ensure!(set.insert(validator), "Duplicate validator in ValidatorSet"); + let mut weights = BTreeMap::new(); + for (key, weight) in validators { + anyhow::ensure!(set.insert(key), "Duplicate validator in ValidatorSet"); + weights.insert(key, weight); } anyhow::ensure!( !set.is_empty(), @@ -97,7 +101,8 @@ impl ValidatorSet { ); Ok(Self { vec: set.iter().cloned().collect(), - map: set.into_iter().enumerate().map(|(i, pk)| (pk, i)).collect(), + indexes: set.into_iter().enumerate().map(|(i, pk)| (pk, i)).collect(), + weights, }) } @@ -114,7 +119,7 @@ impl ValidatorSet { /// Returns true if the given validator is in the validator set. pub fn contains(&self, validator: &validator::PublicKey) -> bool { - self.map.contains_key(validator) + self.indexes.contains_key(validator) } /// Get validator by its index in the set. @@ -124,7 +129,7 @@ impl ValidatorSet { /// Get the index of a validator in the set. pub fn index(&self, validator: &validator::PublicKey) -> Option { - self.map.get(validator).copied() + self.indexes.get(validator).copied() } /// Computes the validator for the given view. From 67a1e50949f696b310a3106b968b765d636039dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Tue, 12 Mar 2024 22:59:50 -0300 Subject: [PATCH 02/35] Added WeightedValidator to represent weighted validators in a ValidatorSet --- node/libs/roles/src/proto/validator.proto | 8 ++++- node/libs/roles/src/validator/conv.rs | 29 +++++++++++++--- .../roles/src/validator/messages/consensus.rs | 33 ++++++++++++++++--- node/libs/roles/src/validator/testonly.rs | 13 ++++++-- node/libs/roles/src/validator/tests.rs | 6 ++-- 5 files changed, 75 insertions(+), 14 deletions(-) diff --git a/node/libs/roles/src/proto/validator.proto b/node/libs/roles/src/proto/validator.proto index f1ec2438..f9420e78 100644 --- a/node/libs/roles/src/proto/validator.proto +++ b/node/libs/roles/src/proto/validator.proto @@ -12,7 +12,7 @@ message Fork { message Genesis { optional Fork fork = 1; // required - repeated PublicKey validators = 2; + repeated WeightedValidator validators = 2; } message GenesisHash { @@ -173,3 +173,9 @@ message Signature { message AggregateSignature { optional bytes bn254 = 1; // required } + +message WeightedValidator { + optional PublicKey key = 1; // required + optional uint32 weight = 2; // required + +} \ No newline at end of file diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index acba464d..1188c41f 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -2,7 +2,7 @@ use super::{ AggregateSignature, BlockHeader, BlockHeaderHash, BlockNumber, CommitQC, ConsensusMsg, FinalBlock, Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, Signature, Signed, Signers, ValidatorSet, View, ViewNumber, + ReplicaPrepare, Signature, Signed, Signers, ValidatorSet, View, ViewNumber, WeightedValidator, }; use crate::{node::SessionId, proto::validator as proto}; use anyhow::Context as _; @@ -36,8 +36,7 @@ impl ProtoFmt for Genesis { .validators .iter() .enumerate() - // TODO: obtain weight - .map(|(i, v)| PublicKey::read(v).context(i).map(|key| (key, 0))) + .map(|(i, v)| WeightedValidator::read(v).context(i)) .collect::>() .context("validators")?; Ok(Self { @@ -48,7 +47,11 @@ impl ProtoFmt for Genesis { fn build(&self) -> Self::Proto { Self::Proto { fork: Some(self.fork.build()), - validators: self.validators.iter().map(|x| x.build()).collect(), + validators: self + .validators + .weighted_validators_iter() + .map(|v| v.build()) + .collect(), } } } @@ -454,3 +457,21 @@ impl ProtoFmt for AggregateSignature { } } } + +impl ProtoFmt for WeightedValidator { + type Proto = proto::WeightedValidator; + + fn read(r: &Self::Proto) -> anyhow::Result { + Ok(Self { + key: read_required(&r.key).context("key")?, + weight: *required(&r.weight).context("weight")?, + }) + } + + fn build(&self) -> Self::Proto { + Self::Proto { + key: Some(self.key.build()), + weight: Some(self.weight), + } + } +} diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 5c0fa63a..e87e2afe 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -3,6 +3,7 @@ use super::{ BlockHeaderHash, BlockNumber, LeaderCommit, LeaderPrepare, Msg, ReplicaCommit, ReplicaPrepare, }; use crate::validator; +use anyhow::Context; use bit_vec::BitVec; use std::{ collections::{BTreeMap, BTreeSet}, @@ -76,7 +77,6 @@ impl Default for Fork { } } - /// A struct that represents a set of validators. It is used to store the current validator set. /// We represent each validator by its validator public key. #[derive(Clone, Debug, PartialEq, Eq)] @@ -88,12 +88,18 @@ pub struct ValidatorSet { impl ValidatorSet { /// Creates a new ValidatorSet from a list of validator public keys. - pub fn new(validators: impl IntoIterator) -> anyhow::Result { + pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); let mut weights = BTreeMap::new(); - for (key, weight) in validators { - anyhow::ensure!(set.insert(key), "Duplicate validator in ValidatorSet"); - weights.insert(key, weight); + for validator in validators { + anyhow::ensure!( + set.insert(validator.key.clone()), + "Duplicate validator in ValidatorSet" + ); + weights.insert( + validator.key, + validator.weight.try_into().context("weight")?, + ); } anyhow::ensure!( !set.is_empty(), @@ -111,6 +117,14 @@ impl ValidatorSet { self.vec.iter() } + /// Iterates over validators. + pub fn weighted_validators_iter(&self) -> impl Iterator + '_ { + self.weights.iter().map(|(key, weight)| WeightedValidator { + key: key.clone(), + weight: *weight as u32, + }) + } + /// Returns the number of validators. #[allow(clippy::len_without_is_empty)] // a valid `ValidatorSet` is always non-empty by construction pub fn len(&self) -> usize { @@ -379,3 +393,12 @@ pub enum Phase { Prepare, Commit, } + +/// Validator representation inside a ValidatorSet. +#[derive(Debug, Clone)] +pub struct WeightedValidator { + /// Validator key + pub key: validator::PublicKey, + /// Validator weight inside the ValidatorSet. + pub weight: u32, +} diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 27290f29..baf15d46 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -4,6 +4,7 @@ use super::{ FinalBlock, Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, ReplicaPrepare, SecretKey, Signature, Signed, Signers, ValidatorSet, View, ViewNumber, + WeightedValidator, }; use bit_vec::BitVec; use rand::{ @@ -22,8 +23,13 @@ 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 u32; let genesis = Genesis { - validators: ValidatorSet::new(keys.iter().map(|k| k.public())).unwrap(), + validators: ValidatorSet::new(keys.iter().map(|k| WeightedValidator { + key: k.public(), + weight, + })) + .unwrap(), fork, }; Self(SetupInner { @@ -302,7 +308,10 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> ValidatorSet { let count = rng.gen_range(1..11); - let public_keys = (0..count).map(|_| rng.gen()); + let public_keys = (0..count).map(|_| WeightedValidator { + key: rng.gen(), + weight: 1000, + }); ValidatorSet::new(public_keys).unwrap() } } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 5f38b3d8..42434e5b 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -215,7 +215,8 @@ fn test_commit_qc() { let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { - validators: ValidatorSet::new(setup1.genesis.validators.iter().take(3).cloned()).unwrap(), + validators: ValidatorSet::new(setup1.genesis.validators.weighted_validators_iter().take(3)) + .unwrap(), fork: setup1.genesis.fork.clone(), }; @@ -249,7 +250,8 @@ fn test_prepare_qc() { let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { - validators: ValidatorSet::new(setup1.genesis.validators.iter().take(3).cloned()).unwrap(), + validators: ValidatorSet::new(setup1.genesis.validators.weighted_validators_iter().take(3)) + .unwrap(), fork: setup1.genesis.fork.clone(), }; From b48ec124048f2f00550b63c8306384e6145f487d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 14 Mar 2024 18:26:19 -0300 Subject: [PATCH 03/35] Signers weight calculation --- node/actors/network/src/gossip/tests.rs | 8 +++- .../roles/src/validator/messages/consensus.rs | 8 ++++ .../src/validator/messages/leader_commit.rs | 4 ++ node/libs/roles/src/validator/tests.rs | 42 +++++++++++++++++++ 4 files changed, 60 insertions(+), 2 deletions(-) diff --git a/node/actors/network/src/gossip/tests.rs b/node/actors/network/src/gossip/tests.rs index 575a8492..6c50fd1f 100644 --- a/node/actors/network/src/gossip/tests.rs +++ b/node/actors/network/src/gossip/tests.rs @@ -14,7 +14,7 @@ use zksync_concurrency::{ testonly::{abort_on_panic, set_timeout}, time, }; -use zksync_consensus_roles::validator::{self, BlockNumber, FinalBlock}; +use zksync_consensus_roles::validator::{self, BlockNumber, FinalBlock, WeightedValidator}; use zksync_consensus_storage::testonly::new_store; #[tokio::test] @@ -143,7 +143,11 @@ async fn test_validator_addrs() { let rng = &mut ctx::test_root(&ctx::RealClock).rng(); let keys: Vec = (0..8).map(|_| rng.gen()).collect(); - let validators = validator::ValidatorSet::new(keys.iter().map(|k| k.public())).unwrap(); + let validators = validator::ValidatorSet::new(keys.iter().map(|k| WeightedValidator { + key: k.public(), + weight: 1250, + })) + .unwrap(); let va = ValidatorAddrsWatch::default(); let mut sub = va.subscribe(); diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index e87e2afe..7c994b3b 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -161,6 +161,14 @@ impl ValidatorSet { pub fn faulty_replicas(&self) -> usize { faulty_replicas(self.len()) } + + /// Compute the sum of signers weights. + pub fn weight(&self, signers: Signers) -> usize { + self.iter() + .enumerate() + .filter(|(i, _)| signers.0[*i]) + .fold(0, |acc, (_, pk)| acc + self.weights[pk]) + } } /// Calculate the consensus threshold, the minimum number of votes for any consensus action to be valid, diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index f9eb7aa9..669965ac 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -100,6 +100,10 @@ impl CommitQC { return Err(Error::BadSignersSet); } + // Verify the signer's weights is enough. + // TODO replace threshold check + let weight = genesis.validators.weight(self.signers.clone()); + // Verify that we have enough signers. let num_signers = self.signers.count(); let threshold = genesis.validators.threshold(); diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 42434e5b..52277523 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -282,3 +282,45 @@ fn test_prepare_qc() { assert!(qc.verify(&genesis3).is_err()); } } + +#[test] +fn test_validator_set_weights() { + use PrepareQCVerifyError as Error; + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + + let setup1 = Setup::new(rng, 6); + let setup2 = Setup::new(rng, 6); + let genesis3 = Genesis { + validators: ValidatorSet::new(setup1.genesis.validators.weighted_validators_iter().take(3)) + .unwrap(), + fork: setup1.genesis.fork.clone(), + }; + + let view: ViewNumber = rng.gen(); + let msgs: Vec<_> = (0..3) + .map(|_| make_replica_prepare(rng, view, &setup1)) + .collect(); + + for n in 0..setup1.keys.len() + 1 { + let mut qc = PrepareQC::new(msgs[0].view.clone()); + for key in &setup1.keys[0..n] { + qc.add( + &key.sign_msg(msgs.choose(rng).unwrap().clone()), + &setup1.genesis, + ); + } + if n >= setup1.genesis.validators.threshold() { + qc.verify(&setup1.genesis).unwrap(); + } else { + assert_matches!( + qc.verify(&setup1.genesis), + Err(Error::NotEnoughSigners { .. }) + ); + } + + // Mismatching validator sets. + assert!(qc.verify(&setup2.genesis).is_err()); + assert!(qc.verify(&genesis3).is_err()); + } +} From 0707c46a1352a20fe0cb312982e97d69ad5565f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 15 Mar 2024 19:38:13 -0300 Subject: [PATCH 04/35] Completed test and renamed ValidatorSet to ValidatorCommittee --- node/actors/network/src/gossip/tests.rs | 2 +- .../network/src/gossip/validator_addrs.rs | 4 +- node/libs/roles/src/validator/conv.rs | 5 +- .../roles/src/validator/messages/consensus.rs | 6 +- .../src/validator/messages/leader_commit.rs | 2 +- .../src/validator/messages/leader_prepare.rs | 4 ++ node/libs/roles/src/validator/testonly.rs | 10 +-- node/libs/roles/src/validator/tests.rs | 69 +++++++++---------- 8 files changed, 52 insertions(+), 50 deletions(-) diff --git a/node/actors/network/src/gossip/tests.rs b/node/actors/network/src/gossip/tests.rs index 6c50fd1f..0807e678 100644 --- a/node/actors/network/src/gossip/tests.rs +++ b/node/actors/network/src/gossip/tests.rs @@ -143,7 +143,7 @@ async fn test_validator_addrs() { let rng = &mut ctx::test_root(&ctx::RealClock).rng(); let keys: Vec = (0..8).map(|_| rng.gen()).collect(); - let validators = validator::ValidatorSet::new(keys.iter().map(|k| WeightedValidator { + let validators = validator::ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), weight: 1250, })) diff --git a/node/actors/network/src/gossip/validator_addrs.rs b/node/actors/network/src/gossip/validator_addrs.rs index ed176bf5..5fed3760 100644 --- a/node/actors/network/src/gossip/validator_addrs.rs +++ b/node/actors/network/src/gossip/validator_addrs.rs @@ -41,7 +41,7 @@ impl ValidatorAddrs { /// Returns true iff some new entry was added. pub(super) fn update( &mut self, - validators: &validator::ValidatorSet, + validators: &validator::ValidatorCommittee, data: &[Arc>], ) -> anyhow::Result { let mut changed = false; @@ -97,7 +97,7 @@ impl ValidatorAddrsWatch { /// invalid entry should be banned. pub(crate) async fn update( &self, - validators: &validator::ValidatorSet, + validators: &validator::ValidatorCommittee, data: &[Arc>], ) -> anyhow::Result<()> { let this = self.0.lock().await; diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 1188c41f..7885acf8 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -2,7 +2,8 @@ use super::{ AggregateSignature, BlockHeader, BlockHeaderHash, BlockNumber, CommitQC, ConsensusMsg, FinalBlock, Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, Signature, Signed, Signers, ValidatorSet, View, ViewNumber, WeightedValidator, + ReplicaPrepare, Signature, Signed, Signers, ValidatorCommittee, View, ViewNumber, + WeightedValidator, }; use crate::{node::SessionId, proto::validator as proto}; use anyhow::Context as _; @@ -41,7 +42,7 @@ impl ProtoFmt for Genesis { .context("validators")?; Ok(Self { fork: read_required(&r.fork).context("fork")?, - validators: ValidatorSet::new(validators.into_iter()).context("validators")?, + validators: ValidatorCommittee::new(validators.into_iter()).context("validators")?, }) } fn build(&self) -> Self::Proto { diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 7c994b3b..3865fcd0 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -80,13 +80,13 @@ impl Default for Fork { /// A struct that represents a set of validators. It is used to store the current validator set. /// We represent each validator by its validator public key. #[derive(Clone, Debug, PartialEq, Eq)] -pub struct ValidatorSet { +pub struct ValidatorCommittee { vec: Vec, indexes: BTreeMap, weights: BTreeMap, } -impl ValidatorSet { +impl ValidatorCommittee { /// Creates a new ValidatorSet from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); @@ -194,7 +194,7 @@ pub fn faulty_replicas(n: usize) -> usize { pub struct Genesis { // TODO(gprusak): add blockchain id here. /// Set of validators of the chain. - pub validators: ValidatorSet, + pub validators: ValidatorCommittee, /// Fork of the chain to follow. pub fork: Fork, } diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index 669965ac..6e7eba94 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -102,7 +102,7 @@ impl CommitQC { // Verify the signer's weights is enough. // TODO replace threshold check - let weight = genesis.validators.weight(self.signers.clone()); + let _weight = genesis.validators.weight(self.signers.clone()); // Verify that we have enough signers. let num_signers = self.signers.count(); diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 15580bb3..9560fe44 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -101,6 +101,7 @@ impl PrepareQC { pub fn verify(&self, genesis: &Genesis) -> Result<(), PrepareQCVerifyError> { use PrepareQCVerifyError as Error; let mut sum = Signers::new(genesis.validators.len()); + // Check the ReplicaPrepare messages. for (i, (msg, signers)) in self.map.iter().enumerate() { if msg.view != self.view { @@ -126,6 +127,9 @@ impl PrepareQC { sum |= signers; } + let weight = genesis.validators.weight(sum.clone()); + dbg!(weight); + // Verify that we have enough signers. let threshold = genesis.validators.threshold(); if sum.count() < threshold { diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index baf15d46..6819897d 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -3,7 +3,7 @@ use super::{ AggregateSignature, BlockHeader, BlockHeaderHash, BlockNumber, CommitQC, ConsensusMsg, FinalBlock, Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, SecretKey, Signature, Signed, Signers, ValidatorSet, View, ViewNumber, + ReplicaPrepare, SecretKey, Signature, Signed, Signers, ValidatorCommittee, View, ViewNumber, WeightedValidator, }; use bit_vec::BitVec; @@ -25,7 +25,7 @@ impl Setup { let keys: Vec = (0..validators).map(|_| rng.gen()).collect(); let weight = (10000 / validators) as u32; let genesis = Genesis { - validators: ValidatorSet::new(keys.iter().map(|k| WeightedValidator { + validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), weight, })) @@ -305,14 +305,14 @@ impl Distribution for Standard { } } -impl Distribution for Standard { - fn sample(&self, rng: &mut R) -> ValidatorSet { +impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> ValidatorCommittee { let count = rng.gen_range(1..11); let public_keys = (0..count).map(|_| WeightedValidator { key: rng.gen(), weight: 1000, }); - ValidatorSet::new(public_keys).unwrap() + ValidatorCommittee::new(public_keys).unwrap() } } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 52277523..90b4d133 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -215,8 +215,10 @@ fn test_commit_qc() { let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { - validators: ValidatorSet::new(setup1.genesis.validators.weighted_validators_iter().take(3)) - .unwrap(), + validators: ValidatorCommittee::new( + setup1.genesis.validators.weighted_validators_iter().take(3), + ) + .unwrap(), fork: setup1.genesis.fork.clone(), }; @@ -250,8 +252,10 @@ fn test_prepare_qc() { let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { - validators: ValidatorSet::new(setup1.genesis.validators.weighted_validators_iter().take(3)) - .unwrap(), + validators: ValidatorCommittee::new( + setup1.genesis.validators.weighted_validators_iter().take(3), + ) + .unwrap(), fork: setup1.genesis.fork.clone(), }; @@ -284,43 +288,36 @@ fn test_prepare_qc() { } #[test] -fn test_validator_set_weights() { - use PrepareQCVerifyError as Error; +fn test_validator_committee_weights() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); - let setup1 = Setup::new(rng, 6); - let setup2 = Setup::new(rng, 6); - let genesis3 = Genesis { - validators: ValidatorSet::new(setup1.genesis.validators.weighted_validators_iter().take(3)) - .unwrap(), - fork: setup1.genesis.fork.clone(), - }; - - let view: ViewNumber = rng.gen(); - let msgs: Vec<_> = (0..3) - .map(|_| make_replica_prepare(rng, view, &setup1)) + let setup = Setup::new(rng, 6); + // Validators weights + let weights = [800, 800, 800, 6000, 800, 800]; + // Expected sum of the validators weights + let sums = [800, 1600, 2400, 8400, 9200, 10000]; + let validators: Vec = weights + .iter() + .enumerate() + .map(|(i, w)| WeightedValidator { + key: setup.keys[i].public(), + weight: *w, + }) .collect(); - for n in 0..setup1.keys.len() + 1 { - let mut qc = PrepareQC::new(msgs[0].view.clone()); - for key in &setup1.keys[0..n] { - qc.add( - &key.sign_msg(msgs.choose(rng).unwrap().clone()), - &setup1.genesis, - ); - } - if n >= setup1.genesis.validators.threshold() { - qc.verify(&setup1.genesis).unwrap(); - } else { - assert_matches!( - qc.verify(&setup1.genesis), - Err(Error::NotEnoughSigners { .. }) - ); - } + let genesis = Genesis { + validators: ValidatorCommittee::new(validators).unwrap(), + fork: setup.genesis.fork.clone(), + }; - // Mismatching validator sets. - assert!(qc.verify(&setup2.genesis).is_err()); - assert!(qc.verify(&genesis3).is_err()); + let view: ViewNumber = rng.gen(); + let msg = make_replica_prepare(rng, view, &setup); + let mut qc = PrepareQC::new(msg.view.clone()); + for n in 0..6 { + let key = &setup.keys[n]; + qc.add(&key.sign_msg(msg.clone()), &setup.genesis); + let signers = &qc.map[&msg]; + assert_eq!(genesis.validators.weight(signers.clone()), sums[n]); } } From 416517a5d462798d6ed131ea2cb2c0067f8920d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 15 Mar 2024 20:15:40 -0300 Subject: [PATCH 05/35] Simplified weights representation --- .../roles/src/validator/messages/consensus.rs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 3865fcd0..3ce32b70 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -8,6 +8,7 @@ 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}; @@ -83,7 +84,7 @@ impl Default for Fork { pub struct ValidatorCommittee { vec: Vec, indexes: BTreeMap, - weights: BTreeMap, + weights: Vec, } impl ValidatorCommittee { @@ -107,8 +108,13 @@ impl ValidatorCommittee { ); Ok(Self { vec: set.iter().cloned().collect(), - indexes: set.into_iter().enumerate().map(|(i, pk)| (pk, i)).collect(), - weights, + indexes: set + .clone() + .into_iter() + .enumerate() + .map(|(i, pk)| (pk, i)) + .collect(), + weights: set.iter().map(|pk| weights[pk]).collect(), }) } @@ -119,7 +125,7 @@ impl ValidatorCommittee { /// Iterates over validators. pub fn weighted_validators_iter(&self) -> impl Iterator + '_ { - self.weights.iter().map(|(key, weight)| WeightedValidator { + zip(&self.vec, &self.weights).map(|(key, weight)| WeightedValidator { key: key.clone(), weight: *weight as u32, }) @@ -164,10 +170,12 @@ impl ValidatorCommittee { /// Compute the sum of signers weights. pub fn weight(&self, signers: Signers) -> usize { - self.iter() + self.weights + .iter() .enumerate() .filter(|(i, _)| signers.0[*i]) - .fold(0, |acc, (_, pk)| acc + self.weights[pk]) + .map(|(_, weight)| weight) + .sum() } } From e6425e41e73a8e609b2eb876d9f1bac9954f7f28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Mon, 18 Mar 2024 09:13:49 -0300 Subject: [PATCH 06/35] Different weights per validator in test --- node/libs/roles/src/validator/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 90b4d133..a6c2359f 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -294,9 +294,9 @@ fn test_validator_committee_weights() { let setup = Setup::new(rng, 6); // Validators weights - let weights = [800, 800, 800, 6000, 800, 800]; + let weights = [1000, 600, 800, 6000, 900, 700]; // Expected sum of the validators weights - let sums = [800, 1600, 2400, 8400, 9200, 10000]; + let sums = [1000, 1600, 2400, 8400, 9300, 10000]; let validators: Vec = weights .iter() .enumerate() @@ -314,10 +314,10 @@ fn test_validator_committee_weights() { let view: ViewNumber = rng.gen(); let msg = make_replica_prepare(rng, view, &setup); let mut qc = PrepareQC::new(msg.view.clone()); - for n in 0..6 { + for (n, weight) in sums.iter().enumerate() { let key = &setup.keys[n]; qc.add(&key.sign_msg(msg.clone()), &setup.genesis); let signers = &qc.map[&msg]; - assert_eq!(genesis.validators.weight(signers.clone()), sums[n]); + assert_eq!(genesis.validators.weight(signers.clone()), *weight); } } From 7689c4f2d2de2bcb5160659639f02288990f9d2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Tue, 19 Mar 2024 11:59:51 -0300 Subject: [PATCH 07/35] Fixed outdated names --- .../libs/roles/src/validator/messages/consensus.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 9d5c7a2e..83ca4f6a 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -83,14 +83,14 @@ pub struct ValidatorCommittee { } impl ValidatorCommittee { - /// Creates a new ValidatorSet from a list of validator public keys. + /// 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(); for validator in validators { anyhow::ensure!( set.insert(validator.key.clone()), - "Duplicate validator in ValidatorSet" + "Duplicate validator in ValidatorCommittee" ); weights.insert( validator.key, @@ -99,7 +99,7 @@ impl ValidatorCommittee { } anyhow::ensure!( !set.is_empty(), - "ValidatorSet must contain at least one validator" + "ValidatorCommittee must contain at least one validator" ); Ok(Self { vec: set.iter().cloned().collect(), @@ -127,7 +127,7 @@ impl ValidatorCommittee { } /// Returns the number of validators. - #[allow(clippy::len_without_is_empty)] // a valid `ValidatorSet` is always non-empty by construction + #[allow(clippy::len_without_is_empty)] // a valid `ValidatorCommittee` is always non-empty by construction pub fn len(&self) -> usize { self.vec.len() } @@ -354,7 +354,7 @@ impl Signers { self.0.iter().filter(|b| *b).count() } - /// Size of the corresponding ValidatorSet. + /// Size of the corresponding ValidatorCommittee. pub fn len(&self) -> usize { self.0.len() } @@ -405,11 +405,11 @@ pub enum Phase { Commit, } -/// Validator representation inside a ValidatorSet. +/// Validator representation inside a ValidatorCommittee. #[derive(Debug, Clone)] pub struct WeightedValidator { /// Validator key pub key: validator::PublicKey, - /// Validator weight inside the ValidatorSet. + /// Validator weight inside the ValidatorCommittee. pub weight: u32, } From 4c8e9812621a65df63ce9e2c5a2615cb5df37a29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Wed, 20 Mar 2024 23:41:43 -0300 Subject: [PATCH 08/35] Updated threshold logic to use weights instead of amount of signers --- node/actors/bft/src/leader/replica_commit.rs | 7 +++--- node/actors/bft/src/leader/replica_prepare.rs | 11 ++++++---- .../roles/src/validator/messages/consensus.rs | 19 ++++++++++++++-- .../src/validator/messages/leader_commit.rs | 22 ++++++++----------- .../src/validator/messages/leader_prepare.rs | 22 +++++++++---------- node/libs/roles/src/validator/tests.rs | 9 +++++--- 6 files changed, 53 insertions(+), 37 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index 2a794bd8..e142e3a2 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -70,7 +70,7 @@ impl StateMachine { }); } - // Check that the message signer is in the validator set. + // Check that the message signer is in the validator committee. if !self.config.genesis().validators.contains(author) { return Err(Error::NonValidatorSigner { signer: author.clone(), @@ -140,8 +140,9 @@ 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.into_iter().find(|(_, v)| v.len() >= threshold) + let Some((_, replica_messages)) = by_proposal + .into_iter() + .find(|(_, v)| self.config.genesis().validators.weight_from_msgs(v) >= threshold) else { return Ok(()); }; diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index 4694f682..6e074302 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/node/actors/bft/src/leader/replica_prepare.rs @@ -144,13 +144,16 @@ impl StateMachine { .insert(author.clone(), signed_message); // Now we check if we have enough messages to continue. - let num_messages = self + let messages: Vec<&validator::Signed> = self .prepare_message_cache .get(&message.view.number) .unwrap() - .len(); + .values() + .collect(); - if num_messages < self.config.genesis().validators.threshold() { + let weight = self.config.genesis().validators.weight_from_msgs(&messages); + let threshold = self.config.genesis().validators.threshold(); + if weight < threshold { return Ok(()); } @@ -158,7 +161,7 @@ impl StateMachine { // for this same view if we receive another replica prepare message after this. self.prepare_message_cache.remove(&message.view.number); - debug_assert_eq!(num_messages, self.config.genesis().validators.threshold()); + debug_assert!(weight >= 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 83ca4f6a..f08d68cd 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -83,6 +83,9 @@ pub struct ValidatorCommittee { } impl ValidatorCommittee { + /// Required weight to verify signatures. + const THRESHOLD: usize = 100_00; + /// Creates a new ValidatorCommittee from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); @@ -155,7 +158,7 @@ impl ValidatorCommittee { /// Signature threshold for this validator set. pub fn threshold(&self) -> usize { - threshold(self.len()) + Self::THRESHOLD } /// Maximal number of faulty replicas allowed in this validator set. @@ -164,7 +167,7 @@ impl ValidatorCommittee { } /// Compute the sum of signers weights. - pub fn weight(&self, signers: Signers) -> usize { + pub fn weight_from_signers(&self, signers: Signers) -> usize { self.weights .iter() .enumerate() @@ -172,6 +175,18 @@ impl ValidatorCommittee { .map(|(_, weight)| weight) .sum() } + + /// Compute the sum of signers weights. + pub fn weight_from_msgs>(&self, signed: &Vec<&validator::Signed>) -> usize { + signed + .iter() + .map(|s| { + self.weights[self + .index(&s.key) + .expect("Signer is not in validator committee")] + }) + .sum() + } } /// Calculate the consensus threshold, the minimum number of votes for any consensus action to be valid, diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index 6e7eba94..cfc2924b 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -41,12 +41,12 @@ pub enum CommitQCVerifyError { /// Bad signer set. #[error("signers set doesn't match genesis")] BadSignersSet, - /// Not enough signers. - #[error("not enough signers: got {got}, want {want}")] - NotEnoughSigners { - /// Got signers. + /// Weight not reached. + #[error("Signers have not reached wanted weight: got {got}, want {want}")] + WeightNotReached { + /// Got weight. got: usize, - /// Want signers. + /// Want weight. want: usize, }, /// Bad signature. @@ -101,15 +101,11 @@ impl CommitQC { } // Verify the signer's weights is enough. - // TODO replace threshold check - let _weight = genesis.validators.weight(self.signers.clone()); - - // Verify that we have enough signers. - let num_signers = self.signers.count(); + let weight = genesis.validators.weight_from_signers(self.signers.clone()); let threshold = genesis.validators.threshold(); - if num_signers < threshold { - return Err(Error::NotEnoughSigners { - got: num_signers, + if weight < threshold { + return Err(Error::WeightNotReached { + got: weight, want: threshold, }); } diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 3b64c998..488340ff 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -30,12 +30,12 @@ pub enum PrepareQCVerifyError { /// Bad message format. #[error(transparent)] BadFormat(anyhow::Error), - /// Not enough signers. - #[error("not enough signers: got {got}, want {want}")] - NotEnoughSigners { - /// Got signers. + /// Weight not reached. + #[error("Signers have not reached wanted weight: got {got}, want {want}")] + WeightNotReached { + /// Got weight. got: usize, - /// Want signers. + /// Want weight. want: usize, }, /// Bad signature. @@ -127,14 +127,12 @@ impl PrepareQC { sum |= signers; } - let weight = genesis.validators.weight(sum.clone()); - dbg!(weight); - - // Verify that we have enough signers. + // Verify the signer's weights is enough. + let weight = genesis.validators.weight_from_signers(sum.clone()); let threshold = genesis.validators.threshold(); - if sum.count() < threshold { - return Err(Error::NotEnoughSigners { - got: sum.count(), + if weight < threshold { + return Err(Error::WeightNotReached { + got: weight, want: threshold, }); } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 1a9983d3..0fbd9399 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -221,7 +221,7 @@ fn test_commit_qc() { } else { assert_matches!( qc.verify(&setup1.genesis), - Err(Error::NotEnoughSigners { .. }) + Err(Error::WeightNotReached { .. }) ); } @@ -265,7 +265,7 @@ fn test_prepare_qc() { } else { assert_matches!( qc.verify(&setup1.genesis), - Err(Error::NotEnoughSigners { .. }) + Err(Error::WeightNotReached { .. }) ); } @@ -306,6 +306,9 @@ fn test_validator_committee_weights() { let key = &setup.keys[n]; qc.add(&key.sign_msg(msg.clone()), &setup.genesis); let signers = &qc.map[&msg]; - assert_eq!(genesis.validators.weight(signers.clone()), *weight); + assert_eq!( + genesis.validators.weight_from_signers(signers.clone()), + *weight + ); } } From 5ff923d1af108807fe8276f10bc62ac0f68028f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 21 Mar 2024 19:14:56 -0300 Subject: [PATCH 09/35] Fixed asserts and tests to use weights instead of amount of signers --- node/actors/bft/src/leader/replica_commit.rs | 15 +++++++- node/actors/bft/src/leader/replica_prepare.rs | 35 ++++++++++------- node/actors/bft/src/testonly/ut_harness.rs | 38 +++++++++++++------ .../roles/src/validator/messages/consensus.rs | 17 ++++++--- node/libs/roles/src/validator/tests.rs | 8 +++- 5 files changed, 79 insertions(+), 34 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index e142e3a2..54fdef2b 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -127,11 +127,20 @@ impl StateMachine { .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis())) .add(&signed_message, self.config.genesis()); - // We store the message in our cache. let cache_entry = self .commit_message_cache .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 store the message in our cache. cache_entry.insert(author.clone(), signed_message.clone()); // Now we check if we have enough messages to continue. @@ -146,7 +155,9 @@ impl StateMachine { else { return Ok(()); }; - debug_assert_eq!(replica_messages.len(), threshold); + // 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); // ----------- Update the state machine -------------- diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index 6e074302..e2a75835 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/node/actors/bft/src/leader/replica_prepare.rs @@ -137,21 +137,28 @@ impl StateMachine { .or_insert_with(|| validator::PrepareQC::new(message.view.clone())) .add(&signed_message, self.config.genesis()); - // We store the message in our cache. - self.prepare_message_cache + // Work on current view messages + let entry = self + .prepare_message_cache .entry(message.view.number) - .or_default() - .insert(author.clone(), signed_message); + .or_default(); - // Now we check if we have enough messages to continue. - let messages: Vec<&validator::Signed> = self - .prepare_message_cache - .get(&message.view.number) - .unwrap() - .values() - .collect(); + // We check validators weight from current messages + let previous_weight = self + .config + .genesis() + .validators + .weight_from_msgs(&entry.values().collect()); + + // We store the message in our cache. + entry.insert(author.clone(), signed_message); - let weight = self.config.genesis().validators.weight_from_msgs(&messages); + // Now we check if we have enough weight to continue. + let weight = self + .config + .genesis() + .validators + .weight_from_msgs(&entry.values().collect()); let threshold = self.config.genesis().validators.threshold(); if weight < threshold { return Ok(()); @@ -161,7 +168,9 @@ impl StateMachine { // for this same view if we receive another replica prepare message after this. self.prepare_message_cache.remove(&message.view.number); - debug_assert!(weight >= threshold); + // 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); // ----------- Update the state machine -------------- diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index 29590cce..9b26535e 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -7,12 +7,12 @@ use crate::{ testonly, Config, PayloadManager, }; use assert_matches::assert_matches; -use std::{cmp::Ordering, sync::Arc}; +use std::sync::Arc; use zksync_concurrency::ctx; use zksync_consensus_network as network; use zksync_consensus_roles::validator::{ self, CommitQC, LeaderCommit, LeaderPrepare, Phase, PrepareQC, ReplicaCommit, ReplicaPrepare, - SecretKey, Signed, ViewNumber, + SecretKey, Signed, ValidatorCommittee, ViewNumber, }; use zksync_consensus_storage::{ testonly::{in_memory, new_store}, @@ -224,15 +224,23 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaPrepare, ) -> Signed { + let expected_validator_weight = ValidatorCommittee::MAX_WEIGHT / self.keys.len(); 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(); + let mut first_match = true; for (i, msg) in msgs.into_iter().enumerate() { let res = self.process_replica_prepare(ctx, msg).await; - match (i + 1).cmp(&want_threshold) { - Ordering::Equal => leader_prepare = res.unwrap(), - Ordering::Less => assert!(res.unwrap().is_none()), - Ordering::Greater => assert_matches!(res, Err(replica_prepare::Error::Old { .. })), + match ( + (i + 1) * expected_validator_weight < want_threshold, + first_match, + ) { + (true, _) => assert!(res.unwrap().is_none()), + (false, true) => { + first_match = false; + leader_prepare = res.unwrap() + } + (false, false) => assert_matches!(res, Err(replica_prepare::Error::Old { .. })), } } leader_prepare.unwrap() @@ -252,15 +260,23 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaCommit, ) -> Signed { + let expected_validator_weight = ValidatorCommittee::MAX_WEIGHT / self.keys.len(); + let want_threshold = self.genesis().validators.threshold(); + let mut first_match = true; for (i, key) in self.keys.iter().enumerate() { let res = self .leader .process_replica_commit(ctx, key.sign_msg(msg.clone())); - let want_threshold = self.genesis().validators.threshold(); - match (i + 1).cmp(&want_threshold) { - Ordering::Equal => res.unwrap(), - Ordering::Less => res.unwrap(), - Ordering::Greater => assert_matches!(res, Err(replica_commit::Error::Old { .. })), + match ( + (i + 1) * expected_validator_weight < want_threshold, + first_match, + ) { + (true, _) => res.unwrap(), + (false, true) => { + first_match = false; + res.unwrap() + } + (false, false) => assert_matches!(res, Err(replica_commit::Error::Old { .. })), } } self.try_recv().unwrap() diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index f08d68cd..07e16a6e 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -83,8 +83,13 @@ pub struct ValidatorCommittee { } impl ValidatorCommittee { + /// Maximum validator weight + /// 100.00% + pub const MAX_WEIGHT: usize = 10000; + /// Required weight to verify signatures. - const THRESHOLD: usize = 100_00; + /// Currently 80.00% + const THRESHOLD: usize = 8000; /// Creates a new ValidatorCommittee from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { @@ -135,17 +140,17 @@ impl ValidatorCommittee { self.vec.len() } - /// Returns true if the given validator is in the validator set. + /// Returns true if the given validator is in the validator committee. pub fn contains(&self, validator: &validator::PublicKey) -> bool { self.indexes.contains_key(validator) } - /// Get validator by its index in the set. + /// Get validator by its index in the committee. pub fn get(&self, index: usize) -> Option<&validator::PublicKey> { self.vec.get(index) } - /// Get the index of a validator in the set. + /// Get the index of a validator in the committee. pub fn index(&self, validator: &validator::PublicKey) -> Option { self.indexes.get(validator).copied() } @@ -156,12 +161,12 @@ impl ValidatorCommittee { self.get(index).unwrap().clone() } - /// Signature threshold for this validator set. + /// Signature threshold for this validator committee. pub fn threshold(&self) -> usize { Self::THRESHOLD } - /// Maximal number of faulty replicas allowed in this validator set. + /// Maximal number of faulty replicas allowed in this validator committee. pub fn faulty_replicas(&self) -> usize { faulty_replicas(self.len()) } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 0fbd9399..34649cde 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -200,6 +200,7 @@ fn test_commit_qc() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); + // This will ceate equally weighted validators let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { @@ -216,7 +217,8 @@ fn test_commit_qc() { for key in &setup1.keys[0..i] { qc.add(&key.sign_msg(qc.message.clone()), &setup1.genesis); } - if i >= setup1.genesis.validators.threshold() { + let expected_weight = i * ValidatorCommittee::MAX_WEIGHT / 6; + if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { assert_matches!( @@ -237,6 +239,7 @@ fn test_prepare_qc() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); + // This will ceate equally weighted validators let setup1 = Setup::new(rng, 6); let setup2 = Setup::new(rng, 6); let genesis3 = Genesis { @@ -260,7 +263,8 @@ fn test_prepare_qc() { &setup1.genesis, ); } - if n >= setup1.genesis.validators.threshold() { + let expected_weight = n * ValidatorCommittee::MAX_WEIGHT / 6; + if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { assert_matches!( From 532a9aacc5c40b79f724d7d53239ad6e94b77b07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 22 Mar 2024 14:52:11 -0300 Subject: [PATCH 10/35] 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>(&self, signed: &Vec<&validator::Signed>) -> usize { + pub fn weight_from_msgs>(&self, signed: &[&validator::Signed]) -> 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 { From 955d7782929d53311c9b85fa2d84e3588ff95c08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 22 Mar 2024 15:16:16 -0300 Subject: [PATCH 11/35] removed unnecessary borrow --- node/actors/bft/src/leader/replica_commit.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index 03ee1b85..c84e4919 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -163,7 +163,7 @@ impl StateMachine { .config .genesis() .validators - .weight_from_msgs(&by_proposal_before.entry(proposal).or_default()); + .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); From e20cdbd5bb6316bef9f7c5db594ad704ff021545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 22 Mar 2024 15:20:11 -0300 Subject: [PATCH 12/35] fix typo --- node/actors/bft/src/leader/replica_commit.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index c84e4919..d1c8e19d 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -132,7 +132,7 @@ impl StateMachine { .entry(message.view.number) .or_default(); - // We check validators weight from current messages, stored by prFoposal + // We check validators weight from current messages, stored by proposal let mut by_proposal_before: HashMap<_, Vec<_>> = HashMap::new(); let entry_before = cache_entry.clone(); for msg in entry_before.values() { From 8652d0e5f52dd2c4c876e78f267439e0afb9ab41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 22 Mar 2024 16:41:51 -0300 Subject: [PATCH 13/35] minor style fixes --- node/actors/bft/src/leader/replica_commit.rs | 2 +- node/libs/roles/src/validator/testonly.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index d1c8e19d..e55700cc 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -159,12 +159,12 @@ impl StateMachine { }; // Check that previous weight did not reach threshold + // to ensure this is the first time the threshold has been reached 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); // ----------- Update the state machine -------------- diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 879d40d8..b2ce90a3 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 u32; + let weight = (ValidatorCommittee::MAX_WEIGHT / validators) as u32; let genesis = Genesis { validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), @@ -303,7 +303,7 @@ impl Distribution for Standard { let count = rng.gen_range(1..11); let public_keys = (0..count).map(|_| WeightedValidator { key: rng.gen(), - weight: 1000, + weight: ValidatorCommittee::MAX_WEIGHT as u32, }); ValidatorCommittee::new(public_keys).unwrap() } From 77a3483d05d0c6a3a5ce1137c7e988af5a625a99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Mon, 25 Mar 2024 10:31:38 -0300 Subject: [PATCH 14/35] Improved weight checks in replica_commit --- node/actors/bft/src/leader/replica_commit.rs | 44 +++++++++----------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index e55700cc..3e152d01 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -1,7 +1,6 @@ //! Handler of a ReplicaCommit message. use super::StateMachine; use crate::metrics; -use std::collections::HashMap; use tracing::instrument; use zksync_concurrency::{ctx, metrics::LatencyHistogramExt as _}; use zksync_consensus_network::io::{ConsensusInputMessage, Target}; @@ -133,39 +132,36 @@ impl StateMachine { .or_default(); // We check validators weight from current messages, stored by proposal - 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); - } + let weight_before = self.config.genesis().validators.weight_from_msgs( + cache_entry + .clone() + .iter() + .map(|(_, m)| m) + .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()); - // Now we check if we have enough messages to continue. - let mut by_proposal: HashMap<_, Vec<_>> = HashMap::new(); - for msg in cache_entry.values() { - by_proposal.entry(msg.msg.proposal).or_default().push(msg); - } + // Now we check if we have enough weight to continue. + let weight = self.config.genesis().validators.weight_from_msgs( + cache_entry + .iter() + .map(|(_, m)| m) + .filter(|m| m.msg.proposal == message.proposal) + .collect::>() + .as_slice(), + ); let threshold = self.config.genesis().validators.threshold(); - let Some((proposal, _replica_messages)) = by_proposal - .into_iter() - .find(|(_, v)| self.config.genesis().validators.weight_from_msgs(v) >= threshold) - else { + if weight < threshold { return Ok(()); }; // Check that previous weight did not reach threshold // to ensure this is the first time the threshold has been reached - let previous_weight = self - .config - .genesis() - .validators - .weight_from_msgs(by_proposal_before.entry(proposal).or_default()); - debug_assert!(previous_weight < threshold); + debug_assert!(weight_before < threshold); // ----------- Update the state machine -------------- From 152283ec9cdfb58969e15f2026c238f041133cdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Mon, 25 Mar 2024 13:04:00 -0300 Subject: [PATCH 15/35] Changed weight type to u64 and several clippy corrections --- node/actors/bft/src/leader/replica_commit.rs | 7 ++----- node/actors/bft/src/testonly/ut_harness.rs | 8 ++++---- node/libs/roles/src/proto/validator.proto | 2 +- .../roles/src/validator/messages/consensus.rs | 19 +++++++++---------- .../src/validator/messages/leader_commit.rs | 6 +++--- .../src/validator/messages/leader_prepare.rs | 6 +++--- node/libs/roles/src/validator/testonly.rs | 4 ++-- node/libs/roles/src/validator/tests.rs | 4 ++-- 8 files changed, 26 insertions(+), 30 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index 3e152d01..d22ec8a6 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -134,9 +134,7 @@ impl StateMachine { // We check validators weight from current messages, stored by proposal let weight_before = self.config.genesis().validators.weight_from_msgs( cache_entry - .clone() - .iter() - .map(|(_, m)| m) + .values() .filter(|m| m.msg.proposal == message.proposal) .collect::>() .as_slice(), @@ -148,8 +146,7 @@ impl StateMachine { // Now we check if we have enough weight to continue. let weight = self.config.genesis().validators.weight_from_msgs( cache_entry - .iter() - .map(|(_, m)| m) + .values() .filter(|m| m.msg.proposal == message.proposal) .collect::>() .as_slice(), diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index 9b26535e..f3c7be12 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -224,7 +224,7 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaPrepare, ) -> Signed { - let expected_validator_weight = ValidatorCommittee::MAX_WEIGHT / self.keys.len(); + let expected_validator_weight = ValidatorCommittee::MAX_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(); @@ -232,7 +232,7 @@ impl UTHarness { for (i, msg) in msgs.into_iter().enumerate() { let res = self.process_replica_prepare(ctx, msg).await; match ( - (i + 1) * expected_validator_weight < want_threshold, + (i + 1) as u64 * expected_validator_weight < want_threshold, first_match, ) { (true, _) => assert!(res.unwrap().is_none()), @@ -260,7 +260,7 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaCommit, ) -> Signed { - let expected_validator_weight = ValidatorCommittee::MAX_WEIGHT / self.keys.len(); + let expected_validator_weight = ValidatorCommittee::MAX_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() { @@ -268,7 +268,7 @@ impl UTHarness { .leader .process_replica_commit(ctx, key.sign_msg(msg.clone())); match ( - (i + 1) * expected_validator_weight < want_threshold, + (i + 1) as u64 * expected_validator_weight < want_threshold, first_match, ) { (true, _) => res.unwrap(), diff --git a/node/libs/roles/src/proto/validator.proto b/node/libs/roles/src/proto/validator.proto index 66050009..29de5b7b 100644 --- a/node/libs/roles/src/proto/validator.proto +++ b/node/libs/roles/src/proto/validator.proto @@ -169,6 +169,6 @@ message AggregateSignature { message WeightedValidator { optional PublicKey key = 1; // required - optional uint32 weight = 2; // required + optional uint64 weight = 2; // required } \ No newline at end of file diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 8223ca34..19b8859d 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -1,7 +1,6 @@ //! Messages related to the consensus protocol. use super::{BlockNumber, LeaderCommit, LeaderPrepare, Msg, ReplicaCommit, ReplicaPrepare}; use crate::validator; -use anyhow::Context; use bit_vec::BitVec; use std::{ collections::{BTreeMap, BTreeSet}, @@ -79,17 +78,17 @@ impl Default for Fork { pub struct ValidatorCommittee { vec: Vec, indexes: BTreeMap, - weights: Vec, + weights: Vec, } impl ValidatorCommittee { /// Maximum validator weight /// 100.00% - pub const MAX_WEIGHT: usize = 10000; + pub const MAX_WEIGHT: u64 = 10000; /// Required weight to verify signatures. /// Currently 80.00% - const THRESHOLD: usize = 8000; + const THRESHOLD: u64 = 8000; /// Creates a new ValidatorCommittee from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { @@ -102,7 +101,7 @@ impl ValidatorCommittee { ); weights.insert( validator.key, - validator.weight.try_into().context("weight")?, + validator.weight, ); } anyhow::ensure!( @@ -130,7 +129,7 @@ impl ValidatorCommittee { pub fn weighted_validators_iter(&self) -> impl Iterator + '_ { zip(&self.vec, &self.weights).map(|(key, weight)| WeightedValidator { key: key.clone(), - weight: *weight as u32, + weight: *weight, }) } @@ -162,7 +161,7 @@ impl ValidatorCommittee { } /// Signature threshold for this validator committee. - pub fn threshold(&self) -> usize { + pub fn threshold(&self) -> u64 { Self::THRESHOLD } @@ -172,7 +171,7 @@ impl ValidatorCommittee { } /// Compute the sum of signers weights. - pub fn weight_from_signers(&self, signers: Signers) -> usize { + pub fn weight_from_signers(&self, signers: Signers) -> u64 { self.weights .iter() .enumerate() @@ -182,7 +181,7 @@ impl ValidatorCommittee { } /// Compute the sum of signers weights. - pub fn weight_from_msgs>(&self, signed: &[&validator::Signed]) -> usize { + pub fn weight_from_msgs>(&self, signed: &[&validator::Signed]) -> u64 { signed .iter() .map(|s| { @@ -431,5 +430,5 @@ pub struct WeightedValidator { /// Validator key pub key: validator::PublicKey, /// Validator weight inside the ValidatorCommittee. - pub weight: u32, + pub weight: u64, } diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index cfc2924b..91f7d40d 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -45,9 +45,9 @@ pub enum CommitQCVerifyError { #[error("Signers have not reached wanted weight: got {got}, want {want}")] WeightNotReached { /// Got weight. - got: usize, + got: u64, /// Want weight. - want: usize, + want: u64, }, /// Bad signature. #[error("bad signature: {0:#}")] @@ -100,7 +100,7 @@ impl CommitQC { return Err(Error::BadSignersSet); } - // Verify the signer's weights is enough. + // Verify the signers' weight is enough. let weight = genesis.validators.weight_from_signers(self.signers.clone()); let threshold = genesis.validators.threshold(); if weight < threshold { diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 488340ff..af722dc8 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -34,9 +34,9 @@ pub enum PrepareQCVerifyError { #[error("Signers have not reached wanted weight: got {got}, want {want}")] WeightNotReached { /// Got weight. - got: usize, + got: u64, /// Want weight. - want: usize, + want: u64, }, /// Bad signature. #[error("bad signature: {0:#}")] @@ -127,7 +127,7 @@ impl PrepareQC { sum |= signers; } - // Verify the signer's weights is enough. + // Verify the signers' weight is enough. let weight = genesis.validators.weight_from_signers(sum.clone()); let threshold = genesis.validators.threshold(); if weight < threshold { diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index b2ce90a3..0934de53 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 = (ValidatorCommittee::MAX_WEIGHT / validators) as u32; + let weight = ValidatorCommittee::MAX_WEIGHT / validators as u64; let genesis = Genesis { validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), @@ -303,7 +303,7 @@ impl Distribution for Standard { let count = rng.gen_range(1..11); let public_keys = (0..count).map(|_| WeightedValidator { key: rng.gen(), - weight: ValidatorCommittee::MAX_WEIGHT as u32, + weight: ValidatorCommittee::MAX_WEIGHT, }); ValidatorCommittee::new(public_keys).unwrap() } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index a78315c2..fcb2a33f 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -217,7 +217,7 @@ fn test_commit_qc() { for key in &setup1.keys[0..i] { qc.add(&key.sign_msg(qc.message.clone()), &setup1.genesis); } - let expected_weight = i * ValidatorCommittee::MAX_WEIGHT / 6; + let expected_weight = i as u64 * ValidatorCommittee::MAX_WEIGHT / 6; if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { @@ -263,7 +263,7 @@ fn test_prepare_qc() { &setup1.genesis, ); } - let expected_weight = n * ValidatorCommittee::MAX_WEIGHT / 6; + let expected_weight = n as u64 * ValidatorCommittee::MAX_WEIGHT / 6; if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { From 94e4ef49484ccd1490cd652c8f9fe05323a6c51f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Mon, 25 Mar 2024 18:21:22 -0300 Subject: [PATCH 16/35] Removed unnecessary constants --- node/actors/bft/src/testonly/ut_harness.rs | 8 +++++--- .../libs/roles/src/validator/messages/consensus.rs | 14 ++++++-------- node/libs/roles/src/validator/testonly.rs | 5 +++-- node/libs/roles/src/validator/tests.rs | 5 +++-- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index f3c7be12..93a3b681 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -12,7 +12,7 @@ use zksync_concurrency::ctx; use zksync_consensus_network as network; use zksync_consensus_roles::validator::{ self, CommitQC, LeaderCommit, LeaderPrepare, Phase, PrepareQC, ReplicaCommit, ReplicaPrepare, - SecretKey, Signed, ValidatorCommittee, ViewNumber, + SecretKey, Signed, ViewNumber, }; use zksync_consensus_storage::{ testonly::{in_memory, new_store}, @@ -224,7 +224,8 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaPrepare, ) -> Signed { - let expected_validator_weight = ValidatorCommittee::MAX_WEIGHT / self.keys.len() as u64; + let expected_validator_weight = + self.genesis().validators.max_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(); @@ -260,7 +261,8 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaCommit, ) -> Signed { - let expected_validator_weight = ValidatorCommittee::MAX_WEIGHT / self.keys.len() as u64; + let expected_validator_weight = + self.genesis().validators.max_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/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 19b8859d..f56c3f81 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -82,10 +82,6 @@ pub struct ValidatorCommittee { } impl ValidatorCommittee { - /// Maximum validator weight - /// 100.00% - pub const MAX_WEIGHT: u64 = 10000; - /// Required weight to verify signatures. /// Currently 80.00% const THRESHOLD: u64 = 8000; @@ -99,10 +95,7 @@ impl ValidatorCommittee { set.insert(validator.key.clone()), "Duplicate validator in ValidatorCommittee" ); - weights.insert( - validator.key, - validator.weight, - ); + weights.insert(validator.key, validator.weight); } anyhow::ensure!( !set.is_empty(), @@ -191,6 +184,11 @@ impl ValidatorCommittee { }) .sum() } + + /// Sum of all validators' weight in the committee + pub fn max_weight(&self) -> u64 { + self.weights.iter().sum() + } } /// Calculate the consensus threshold, the minimum number of votes for any consensus action to be valid, diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 0934de53..1e5c69b8 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 = ValidatorCommittee::MAX_WEIGHT / validators as u64; + let weight = 10000 / validators as u64; let genesis = Genesis { validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), @@ -301,9 +301,10 @@ 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 public_keys = (0..count).map(|_| WeightedValidator { key: rng.gen(), - weight: ValidatorCommittee::MAX_WEIGHT, + weight, }); ValidatorCommittee::new(public_keys).unwrap() } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index fcb2a33f..da9c9204 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -210,6 +210,7 @@ fn test_commit_qc() { .unwrap(), fork: setup1.genesis.fork.clone(), }; + let validator_weight = setup1.genesis.validators.max_weight(); for i in 0..setup1.keys.len() + 1 { let view = rng.gen(); @@ -217,7 +218,7 @@ fn test_commit_qc() { for key in &setup1.keys[0..i] { qc.add(&key.sign_msg(qc.message.clone()), &setup1.genesis); } - let expected_weight = i as u64 * ValidatorCommittee::MAX_WEIGHT / 6; + let expected_weight = i as u64 * validator_weight / 6; if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { @@ -263,7 +264,7 @@ fn test_prepare_qc() { &setup1.genesis, ); } - let expected_weight = n as u64 * ValidatorCommittee::MAX_WEIGHT / 6; + let expected_weight = n as u64 * setup1.genesis.validators.max_weight() / 6; if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { 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 17/35] 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(), }; 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 18/35] 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 { From 6a650e79b584ac1fd0f7e01dcf373830578ab496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Wed, 27 Mar 2024 13:59:52 -0300 Subject: [PATCH 19/35] Checking weights sum for overflow --- .../roles/src/validator/messages/consensus.rs | 10 +++++++- node/libs/roles/src/validator/tests.rs | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 183bb4b2..ac30b40e 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -77,6 +77,7 @@ impl Default for Fork { pub struct ValidatorCommittee { vec: Vec, indexes: BTreeMap, + total_weight: u64, } impl ValidatorCommittee { @@ -84,11 +85,17 @@ impl ValidatorCommittee { pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); let mut weighted_validators = BTreeMap::new(); + let mut total_weight: u64 = 0; for validator in validators { anyhow::ensure!( set.insert(validator.key.clone()), "Duplicate validator in ValidatorCommittee" ); + if let Some(result) = total_weight.checked_add(validator.weight) { + total_weight = result + } else { + anyhow::bail!("Sum of weights overflows in ValidatorCommittee") + } weighted_validators.insert(validator.key.clone(), validator); } anyhow::ensure!( @@ -106,6 +113,7 @@ impl ValidatorCommittee { .enumerate() .map(|(i, pk)| (pk, i)) .collect(), + total_weight, }) } @@ -168,7 +176,7 @@ impl ValidatorCommittee { /// Sum of all validators' weight in the committee pub fn total_weight(&self) -> u64 { - self.vec.iter().map(|v| v.weight).sum() + self.total_weight } } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index e1c3ce00..07e9dcf3 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::validator::testonly::Setup; +use anyhow::Error; use assert_matches::assert_matches; use rand::{seq::SliceRandom, Rng}; use std::vec; @@ -313,3 +314,25 @@ fn test_validator_committee_weights() { ); } } + +#[test] +fn test_validator_weights_sanity() { + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + + let setup = Setup::new(rng, 6); + // Validators weights + let weight = u64::MAX / 5; + let weights = [weight, weight, weight, weight, weight, weight]; + let validators: Vec = weights + .iter() + .enumerate() + .map(|(i, w)| WeightedValidator { + key: setup.keys[i].public(), + weight: *w, + }) + .collect(); + + // Creation should overflow + assert_matches!(ValidatorCommittee::new(validators), Err(Error { .. })); +} From 9913ed075068fa35b14f62cf00ac32d9afe20bf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Wed, 27 Mar 2024 14:13:30 -0300 Subject: [PATCH 20/35] Fixed merge error --- node/actors/network/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/actors/network/src/lib.rs b/node/actors/network/src/lib.rs index c8d3e70d..836ccaba 100644 --- a/node/actors/network/src/lib.rs +++ b/node/actors/network/src/lib.rs @@ -156,7 +156,7 @@ impl Runner { // If we are active validator ... if validators.contains(&c.key.public()) { // Maintain outbound connections. - for peer in validators.iter() { + for peer in validators.iter_keys() { s.spawn(async { c.maintain_connection(ctx, peer).await; Ok(()) From bf36b3c2844f742a83a51e553f49c86b11faa78a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 5 Apr 2024 16:15:47 -0300 Subject: [PATCH 21/35] Fixed 'Bft - 424 fix aggregation of ReplicaCommit messages' --- node/actors/bft/src/leader/replica_commit.rs | 15 ++++--- node/actors/bft/src/leader/replica_prepare.rs | 2 +- node/actors/bft/src/leader/state_machine.rs | 5 ++- node/actors/bft/src/leader/tests.rs | 42 +++++++++++++++++++ .../roles/src/validator/messages/consensus.rs | 2 +- .../src/validator/messages/leader_commit.rs | 2 +- .../src/validator/messages/leader_prepare.rs | 5 +-- node/libs/roles/src/validator/tests.rs | 9 ++-- 8 files changed, 63 insertions(+), 19 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index d81d3358..64b0b411 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -1,4 +1,5 @@ //! Handler of a ReplicaCommit message. + use super::StateMachine; use crate::metrics; use tracing::instrument; @@ -117,13 +118,12 @@ impl StateMachine { // ----------- All checks finished. Now we process the message. -------------- - // TODO: we have a bug here since we don't check whether replicas commit - // to the same proposal. - // We add the message to the incrementally-constructed QC. let commit_qc = self .commit_qcs .entry(message.view.number) + .or_default() + .entry(message.clone()) .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis())); commit_qc.add(&signed_message, self.config.genesis()); @@ -138,7 +138,7 @@ impl StateMachine { .config .genesis() .validators - .weight_from_signers(commit_qc.signers.clone()); + .weight(commit_qc.signers.clone()); if weight < self.config.genesis().validators.threshold() { return Ok(()); }; @@ -159,7 +159,12 @@ impl StateMachine { self.commit_message_cache.remove(&message.view.number); // Consume the incrementally-constructed QC for this view. - let justification = self.commit_qcs.remove(&message.view.number).unwrap(); + let justification = self + .commit_qcs + .remove(&message.view.number) + .unwrap() + .remove(message) + .unwrap(); // Broadcast the leader commit message to all replicas (ourselves included). let output_message = ConsensusInputMessage { diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index b04b8bb4..d66842fb 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/node/actors/bft/src/leader/replica_prepare.rs @@ -145,7 +145,7 @@ impl StateMachine { .insert(author.clone(), signed_message.clone()); // Now we check if we have enough weight to continue. - let weight = self.config.genesis().validators.weight_from_signers( + let weight = self.config.genesis().validators.weight( prepare_qc .map .get(&signed_message.msg) diff --git a/node/actors/bft/src/leader/state_machine.rs b/node/actors/bft/src/leader/state_machine.rs index 8d44909a..fe10d32c 100644 --- a/node/actors/bft/src/leader/state_machine.rs +++ b/node/actors/bft/src/leader/state_machine.rs @@ -41,8 +41,9 @@ pub(crate) struct StateMachine { validator::ViewNumber, HashMap>, >, - /// Commit QCs indexed by view number. - pub(crate) commit_qcs: BTreeMap, + /// Commit QCs indexed by view number and then by message. + pub(crate) commit_qcs: + BTreeMap>, } impl StateMachine { diff --git a/node/actors/bft/src/leader/tests.rs b/node/actors/bft/src/leader/tests.rs index 9974c5c8..4a47b9f9 100644 --- a/node/actors/bft/src/leader/tests.rs +++ b/node/actors/bft/src/leader/tests.rs @@ -572,3 +572,45 @@ async fn replica_commit_unexpected_proposal() { .await .unwrap(); } + +#[tokio::test] +async fn replica_commit_different_proposals() { + zksync_concurrency::testonly::abort_on_panic(); + let ctx = &ctx::test_root(&ctx::RealClock); + scope::run!(ctx, |ctx, s| async { + let (mut util, runner) = UTHarness::new_many(ctx).await; + s.spawn_bg(runner.run(ctx)); + + let replica_commit = util.new_replica_commit(ctx).await; + + // Process a modified replica_commit (ie. from a malicious or wrong node) + let mut bad_replica_commit = replica_commit.clone(); + bad_replica_commit.proposal.number = replica_commit.proposal.number.next(); + util.process_replica_commit(ctx, util.sign(bad_replica_commit)) + .await + .unwrap(); + + // The rest of the validators sign the correct one + let mut replica_commit_result = None; + for i in 1..util.keys.len() { + replica_commit_result = util + .process_replica_commit(ctx, util.keys[i].sign_msg(replica_commit.clone())) + .await + .unwrap(); + } + + // Check correct proposal has been committed + assert_matches!( + replica_commit_result, + Some(leader_commit) => { + assert_eq!( + leader_commit.msg.justification.message.proposal, + replica_commit.proposal + ); + } + ); + Ok(()) + }) + .await + .unwrap(); +} diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index ac30b40e..7512589e 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -165,7 +165,7 @@ impl ValidatorCommittee { } /// Compute the sum of signers weights. - pub fn weight_from_signers(&self, signers: Signers) -> u64 { + pub fn weight(&self, signers: Signers) -> u64 { self.vec .iter() .enumerate() diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index 58d57b57..b5d9583b 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -101,7 +101,7 @@ impl CommitQC { } // Verify the signers' weight is enough. - let weight = genesis.validators.weight_from_signers(self.signers.clone()); + let weight = genesis.validators.weight(self.signers.clone()); let threshold = genesis.validators.threshold(); if weight < threshold { return Err(Error::WeightNotReached { diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 6314708b..49b38315 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -59,8 +59,7 @@ impl PrepareQC { 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() += - genesis.validators.weight_from_signers(signers.clone()); + *count.entry(v.proposal).or_default() += genesis.validators.weight(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. @@ -129,7 +128,7 @@ impl PrepareQC { } // Verify the signers' weight is enough. - let weight = genesis.validators.weight_from_signers(sum.clone()); + let weight = genesis.validators.weight(sum.clone()); let threshold = genesis.validators.threshold(); if weight < threshold { return Err(Error::WeightNotReached { diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 07e9dcf3..5ab89b0e 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -209,7 +209,7 @@ fn test_commit_qc() { .unwrap(), fork: setup1.genesis.fork.clone(), }; - let validator_weight = setup1.genesis.validators.total_weight(); + let validator_weight = setup1.genesis.validators.total_weight() / 6; for i in 0..setup1.keys.len() + 1 { let view = rng.gen(); @@ -217,7 +217,7 @@ fn test_commit_qc() { for key in &setup1.keys[0..i] { qc.add(&key.sign_msg(qc.message.clone()), &setup1.genesis); } - let expected_weight = i as u64 * validator_weight / 6; + let expected_weight = i as u64 * validator_weight; if expected_weight >= setup1.genesis.validators.threshold() { qc.verify(&setup1.genesis).unwrap(); } else { @@ -308,10 +308,7 @@ fn test_validator_committee_weights() { let key = &setup.keys[n]; qc.add(&key.sign_msg(msg.clone()), &setup.genesis); let signers = &qc.map[&msg]; - assert_eq!( - genesis.validators.weight_from_signers(signers.clone()), - *weight - ); + assert_eq!(genesis.validators.weight(signers.clone()), *weight); } } From 89d9d4caec628446d641841c440a06073fd37df7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Tue, 9 Apr 2024 11:32:55 -0300 Subject: [PATCH 22/35] Fixed simpler suggestions in PR --- node/actors/bft/src/testonly/ut_harness.rs | 6 ++---- .../roles/src/validator/messages/consensus.rs | 15 ++++++--------- .../roles/src/validator/messages/leader_commit.rs | 6 +++--- .../src/validator/messages/leader_prepare.rs | 6 +++--- node/libs/roles/src/validator/tests.rs | 6 +++--- 5 files changed, 17 insertions(+), 22 deletions(-) diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index daf0c15b..7d828954 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -226,14 +226,13 @@ impl UTHarness { ) -> Signed { let expected_validator_weight = 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(); let mut first_match = true; for (i, msg) in msgs.into_iter().enumerate() { let res = self.process_replica_prepare(ctx, msg).await; match ( - (i + 1) as u64 * expected_validator_weight < want_threshold, + (i + 1) as u64 * expected_validator_weight < self.genesis().validators.threshold(), first_match, ) { (true, _) => assert!(res.unwrap().is_none()), @@ -263,14 +262,13 @@ impl UTHarness { ) -> Signed { let expected_validator_weight = 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() { let res = self .leader .process_replica_commit(ctx, key.sign_msg(msg.clone())); match ( - (i + 1) as u64 * expected_validator_weight < want_threshold, + (i + 1) as u64 * expected_validator_weight < self.genesis().validators.threshold(), first_match, ) { (true, _) => res.unwrap(), diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 3ba95737..42628ba0 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -1,6 +1,7 @@ //! Messages related to the consensus protocol. use super::{BlockNumber, LeaderCommit, LeaderPrepare, Msg, ReplicaCommit, ReplicaPrepare}; use crate::validator; +use anyhow::Context; use bit_vec::BitVec; use std::{ collections::{BTreeMap, BTreeSet}, @@ -91,11 +92,9 @@ impl ValidatorCommittee { set.insert(validator.key.clone()), "Duplicate validator in ValidatorCommittee" ); - if let Some(result) = total_weight.checked_add(validator.weight) { - total_weight = result - } else { - anyhow::bail!("Sum of weights overflows in ValidatorCommittee") - } + total_weight = total_weight + .checked_add(validator.weight) + .context("Sum of weights overflows in ValidatorCommittee")?; weighted_validators.insert(validator.key.clone(), validator); } anyhow::ensure!( @@ -148,7 +147,7 @@ impl ValidatorCommittee { self.indexes.get(validator).copied() } - /// Computes the validator for the given view. + /// Computes the leader 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().key.clone() @@ -159,7 +158,7 @@ impl ValidatorCommittee { threshold(self.total_weight()) } - /// Maximal number of faulty replicas allowed in this validator committee. + /// Maximal weight of faulty replicas allowed in this validator committee. pub fn max_faulty_weight(&self) -> u64 { max_faulty_weight(self.total_weight()) } @@ -193,8 +192,6 @@ pub fn max_faulty_weight(total_weight: u64) -> u64 { // 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 - // allowed weight for faulty replicas. (total_weight - 1) / 5 } diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index b5d9583b..dce7d7fe 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -42,8 +42,8 @@ pub enum CommitQCVerifyError { #[error("signers set doesn't match genesis")] BadSignersSet, /// Weight not reached. - #[error("Signers have not reached wanted weight: got {got}, want {want}")] - WeightNotReached { + #[error("Signers have not reached threshold weight: got {got}, want {want}")] + NotEnoughSigners { /// Got weight. got: u64, /// Want weight. @@ -104,7 +104,7 @@ impl CommitQC { let weight = genesis.validators.weight(self.signers.clone()); let threshold = genesis.validators.threshold(); if weight < threshold { - return Err(Error::WeightNotReached { + return Err(Error::NotEnoughSigners { got: weight, want: threshold, }); diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 49b38315..5eafb6f7 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -31,8 +31,8 @@ pub enum PrepareQCVerifyError { #[error(transparent)] BadFormat(anyhow::Error), /// Weight not reached. - #[error("Signers have not reached wanted weight: got {got}, want {want}")] - WeightNotReached { + #[error("Signers have not reached threshold weight: got {got}, want {want}")] + NotEnoughSigners { /// Got weight. got: u64, /// Want weight. @@ -131,7 +131,7 @@ impl PrepareQC { let weight = genesis.validators.weight(sum.clone()); let threshold = genesis.validators.threshold(); if weight < threshold { - return Err(Error::WeightNotReached { + return Err(Error::NotEnoughSigners { got: weight, want: threshold, }); diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 5ab89b0e..b7aa96d0 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -223,7 +223,7 @@ fn test_commit_qc() { } else { assert_matches!( qc.verify(&setup1.genesis), - Err(Error::WeightNotReached { .. }) + Err(Error::NotEnoughSigners { .. }) ); } @@ -267,7 +267,7 @@ fn test_prepare_qc() { } else { assert_matches!( qc.verify(&setup1.genesis), - Err(Error::WeightNotReached { .. }) + Err(Error::NotEnoughSigners { .. }) ); } @@ -320,7 +320,7 @@ fn test_validator_weights_sanity() { let setup = Setup::new(rng, 6); // Validators weights let weight = u64::MAX / 5; - let weights = [weight, weight, weight, weight, weight, weight]; + let weights = [weight; 6]; let validators: Vec = weights .iter() .enumerate() From 5b3de38bf6d87c0f90f346a42e36a3a55263dcce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 11 Apr 2024 08:52:45 -0300 Subject: [PATCH 23/35] Fixed weight calculation bug in process_replica_prepare and added a test for it --- node/actors/bft/src/leader/replica_prepare.rs | 19 +++--- node/actors/bft/src/leader/tests.rs | 65 +++++++++++++++++++ 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index d66842fb..2fbb58dd 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, Signers}; +use zksync_consensus_roles::validator; /// Errors that can occur when processing a "replica prepare" message. #[derive(Debug, thiserror::Error)] @@ -11,9 +11,9 @@ pub(crate) enum Error { #[error("incompatible protocol version (message version: {message_version:?}, local version: {local_version:?}")] IncompatibleProtocolVersion { /// Message version. - message_version: ProtocolVersion, + message_version: validator::ProtocolVersion, /// Local version. - local_version: ProtocolVersion, + local_version: validator::ProtocolVersion, }, /// Message signer isn't part of the validator set. #[error("Message signer isn't part of the validator set (signer: {signer:?})")] @@ -145,13 +145,12 @@ impl StateMachine { .insert(author.clone(), signed_message.clone()); // Now we check if we have enough weight to continue. - let weight = self.config.genesis().validators.weight( - prepare_qc - .map - .get(&signed_message.msg) - .unwrap_or(&Signers::new(self.config.genesis().validators.len())) - .clone(), - ); + let weight: u64 = prepare_qc + .clone() + .map + .into_values() + .map(|signers| self.config.genesis().validators.weight(signers)) + .sum(); if weight < self.config.genesis().validators.threshold() { return Ok(()); } diff --git a/node/actors/bft/src/leader/tests.rs b/node/actors/bft/src/leader/tests.rs index 4a47b9f9..07228889 100644 --- a/node/actors/bft/src/leader/tests.rs +++ b/node/actors/bft/src/leader/tests.rs @@ -333,6 +333,68 @@ async fn replica_prepare_high_qc_of_future_view() { .unwrap(); } +/// Check all ReplicaPrepare are included for weight calculation +/// even on different messages for the same view. +#[tokio::test] +async fn replica_prepare_different_messages() { + zksync_concurrency::testonly::abort_on_panic(); + let ctx = &ctx::test_root(&ctx::RealClock); + scope::run!(ctx, |ctx, s| async { + let (mut util, runner) = UTHarness::new_many(ctx).await; + s.spawn_bg(runner.run(ctx)); + + util.produce_block(ctx).await; + + let view = util.replica_view(); + let replica_prepare = util.new_replica_prepare(); + + // Create a different proposal for the same view + let proposal = replica_prepare.clone().high_vote.unwrap().proposal; + let mut different_proposal = proposal; + different_proposal.number = different_proposal.number.next(); + + // Create a new ReplicaPrepare with the different proposal + let mut other_replica_prepare = replica_prepare.clone(); + let mut high_vote = other_replica_prepare.high_vote.clone().unwrap(); + high_vote.proposal = different_proposal; + let high_qc = util.new_commit_qc(|msg| { + msg.proposal = different_proposal; + msg.view = view.clone() + }); + + other_replica_prepare.high_vote = Some(high_vote); + other_replica_prepare.high_qc = Some(high_qc); + + let validators = util.keys.len(); + + // half of the validators sign replica_prepare + for i in 0..validators / 2 { + util.process_replica_prepare(ctx, util.keys[i].sign_msg(replica_prepare.clone())) + .await + .unwrap(); + } + + let mut replica_commit_result = None; + // The rest (minus one) of the validators sign other_replica_prepare + for i in validators / 2..validators - 1 { + replica_commit_result = util + .process_replica_prepare(ctx, util.keys[i].sign_msg(other_replica_prepare.clone())) + .await + .unwrap(); + } + + // That should be enough for a proposal to be committed + assert_matches!(replica_commit_result, Some(_)); + + // Check the first proposal has been committed (as it has more votes) + let message = replica_commit_result.unwrap().msg; + assert_eq!(message.proposal, proposal); + Ok(()) + }) + .await + .unwrap(); +} + #[tokio::test] async fn replica_commit_sanity() { zksync_concurrency::testonly::abort_on_panic(); @@ -573,6 +635,9 @@ async fn replica_commit_unexpected_proposal() { .unwrap(); } +/// Proposal should be the same for every ReplicaCommit +/// Check it doesn't fail if one validator sends a different propsal in +/// the ReplicaCommit #[tokio::test] async fn replica_commit_different_proposals() { zksync_concurrency::testonly::abort_on_panic(); From f3d18aa6cd6035bacab6bf367b7a52d9719e8424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 11 Apr 2024 09:00:36 -0300 Subject: [PATCH 24/35] Fixed typo --- node/actors/bft/src/leader/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/actors/bft/src/leader/tests.rs b/node/actors/bft/src/leader/tests.rs index 07228889..df4ce0e1 100644 --- a/node/actors/bft/src/leader/tests.rs +++ b/node/actors/bft/src/leader/tests.rs @@ -636,7 +636,7 @@ async fn replica_commit_unexpected_proposal() { } /// Proposal should be the same for every ReplicaCommit -/// Check it doesn't fail if one validator sends a different propsal in +/// Check it doesn't fail if one validator sends a different proposal in /// the ReplicaCommit #[tokio::test] async fn replica_commit_different_proposals() { From b15857c11cc8778abef7ee37612abacaea54608d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 11 Apr 2024 09:09:29 -0300 Subject: [PATCH 25/35] Making it backwards compatible --- node/libs/roles/src/proto/validator.proto | 3 ++- node/libs/roles/src/validator/conv.rs | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/node/libs/roles/src/proto/validator.proto b/node/libs/roles/src/proto/validator.proto index 29de5b7b..4da73223 100644 --- a/node/libs/roles/src/proto/validator.proto +++ b/node/libs/roles/src/proto/validator.proto @@ -11,7 +11,8 @@ message Fork { message Genesis { optional Fork fork = 1; // required - repeated WeightedValidator validators = 2; + repeated PublicKey validator_keys = 2; + repeated WeightedValidator validators = 3; } message GenesisHash { diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index e874edf3..9dc87a23 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -46,6 +46,7 @@ impl ProtoFmt for Genesis { fn build(&self) -> Self::Proto { Self::Proto { fork: Some(self.fork.build()), + validator_keys: vec![], validators: self.validators.iter().map(|v| v.build()).collect(), } } From 89316f170784f2009dc71c8b2ae910cca11846b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 11 Apr 2024 19:15:17 -0300 Subject: [PATCH 26/35] Making it backwards compatible and modifying tests to check it --- node/actors/bft/src/testonly/ut_harness.rs | 6 +-- node/libs/roles/src/validator/conv.rs | 53 +++++++++++++++---- .../roles/src/validator/messages/consensus.rs | 17 +++++- .../roles/src/validator/messages/tests.rs | 7 ++- node/libs/roles/src/validator/testonly.rs | 10 ++-- node/libs/roles/src/validator/tests.rs | 3 ++ 6 files changed, 74 insertions(+), 22 deletions(-) diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index 7d828954..b4f26a5b 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -224,8 +224,7 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaPrepare, ) -> Signed { - let expected_validator_weight = - self.genesis().validators.total_weight() / self.keys.len() as u64; + let expected_validator_weight = self.genesis().validators.iter().next().unwrap().weight; let mut leader_prepare = None; let msgs: Vec<_> = self.keys.iter().map(|k| k.sign_msg(msg.clone())).collect(); let mut first_match = true; @@ -260,8 +259,7 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaCommit, ) -> Signed { - let expected_validator_weight = - self.genesis().validators.total_weight() / self.keys.len() as u64; + let expected_validator_weight = self.genesis().validators.iter().next().unwrap().weight; let mut first_match = true; for (i, key) in self.keys.iter().enumerate() { let res = self diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 9dc87a23..7fdd494b 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -31,23 +31,54 @@ impl ProtoFmt for Fork { impl ProtoFmt for Genesis { type Proto = proto::Genesis; fn read(r: &Self::Proto) -> anyhow::Result { - let validators: Vec<_> = r - .validators - .iter() - .enumerate() - .map(|(i, v)| WeightedValidator::read(v).context(i)) - .collect::>() - .context("validators")?; + let (validators, encoding_version) = + // current genesis encoding version 1 + if !r.validators.is_empty() { + ( + r.validators + .iter() + .enumerate() + .map(|(i, v)| WeightedValidator::read(v).context(i)) + .collect::>() + .context("validators")?, + 1, + ) + // legacy genesis encoding version 0 + } else if !r.validator_keys.is_empty() { + ( + r.validator_keys + .iter() + .enumerate() + .map(|(i, v)| anyhow::Ok(WeightedValidator { + key: PublicKey::read(v).context(i)?, + weight: 1, + })) + .collect::>() + .context("validators")?, + 0, + ) + // empty validator set + } else { + (vec![], 0) + }; Ok(Self { fork: read_required(&r.fork).context("fork")?, validators: ValidatorCommittee::new(validators.into_iter()).context("validators")?, + encoding_version, }) } fn build(&self) -> Self::Proto { - Self::Proto { - fork: Some(self.fork.build()), - validator_keys: vec![], - validators: self.validators.iter().map(|v| v.build()).collect(), + match self.encoding_version { + 0 => Self::Proto { + fork: Some(self.fork.build()), + validator_keys: self.validators.iter().map(|v| v.key.build()).collect(), + validators: vec![], + }, + 1.. => Self::Proto { + fork: Some(self.fork.build()), + validator_keys: vec![], + 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 39ebd1ad..5ce6e8f6 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -10,6 +10,9 @@ use std::{ use zksync_consensus_crypto::{keccak256::Keccak256, ByteFmt, Text, TextFmt}; use zksync_consensus_utils::enum_util::{BadVariantError, Variant}; +/// Current genesis encoding version +pub const CURRENT_GENESIS_ENCODING_VERSION: usize = 1; + /// Version of the consensus algorithm that the validator is using. /// It allows to prevent misinterpretation of messages signed by validators /// using different versions of the binaries. @@ -75,7 +78,7 @@ impl Default for Fork { /// A struct that represents a set of validators. It is used to store the current validator set. /// We represent each validator by its validator public key. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, Default)] pub struct ValidatorCommittee { vec: Vec, indexes: BTreeMap, @@ -200,6 +203,8 @@ pub fn max_faulty_weight(total_weight: u64) -> u64 { #[derive(Debug, Clone, PartialEq)] pub struct Genesis { // TODO(gprusak): add blockchain id here. + /// Genesis encoding version + pub encoding_version: usize, /// Set of validators of the chain. pub validators: ValidatorCommittee, /// Fork of the chain to follow. @@ -217,6 +222,16 @@ impl Genesis { } } +impl Default for Genesis { + fn default() -> Self { + Self { + encoding_version: CURRENT_GENESIS_ENCODING_VERSION, + validators: ValidatorCommittee::default(), + fork: Fork::default(), + } + } +} + impl TextFmt for GenesisHash { fn decode(text: Text) -> anyhow::Result { text.strip("genesis_hash:keccak256:")? diff --git a/node/libs/roles/src/validator/messages/tests.rs b/node/libs/roles/src/validator/messages/tests.rs index 8ba950c2..5bacbe2e 100644 --- a/node/libs/roles/src/validator/messages/tests.rs +++ b/node/libs/roles/src/validator/messages/tests.rs @@ -33,8 +33,13 @@ fn fork() -> Fork { /// Hardcoded genesis. fn genesis() -> Genesis { Genesis { - validators: ValidatorSet::new(keys().iter().map(|k| k.public())).unwrap(), + validators: ValidatorCommittee::new(keys().iter().map(|k| WeightedValidator { + key: k.public(), + weight: 1, + })) + .unwrap(), fork: fork(), + encoding_version: 0, } } diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 3cb2529c..816c5e25 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -4,7 +4,7 @@ use super::{ ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, ReplicaPrepare, SecretKey, Signature, Signed, Signers, ValidatorCommittee, View, ViewNumber, - WeightedValidator, + WeightedValidator, CURRENT_GENESIS_ENCODING_VERSION, }; use bit_vec::BitVec; use rand::{ @@ -23,14 +23,14 @@ 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 = 1; let genesis = Genesis { validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), - weight, + weight: 1, })) .unwrap(), fork, + ..Default::default() }; Self(SetupInner { keys, @@ -197,6 +197,7 @@ impl Distribution for Standard { Genesis { validators: rng.gen(), fork: rng.gen(), + encoding_version: rng.gen_range(0..=CURRENT_GENESIS_ENCODING_VERSION), } } } @@ -301,10 +302,9 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> ValidatorCommittee { let count = rng.gen_range(1..11); - let weight = 1; let public_keys = (0..count).map(|_| WeightedValidator { key: rng.gen(), - weight, + weight: 1, }); ValidatorCommittee::new(public_keys).unwrap() } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index e17f376e..ee7a0e0e 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -208,6 +208,7 @@ fn test_commit_qc() { validators: ValidatorCommittee::new(setup1.genesis.validators.iter().take(3).cloned()) .unwrap(), fork: setup1.genesis.fork.clone(), + ..Default::default() }; let validator_weight = setup1.genesis.validators.total_weight() / 6; @@ -246,6 +247,7 @@ fn test_prepare_qc() { validators: ValidatorCommittee::new(setup1.genesis.validators.iter().take(3).cloned()) .unwrap(), fork: setup1.genesis.fork.clone(), + ..Default::default() }; let view: ViewNumber = rng.gen(); @@ -299,6 +301,7 @@ fn test_validator_committee_weights() { let genesis = Genesis { validators: ValidatorCommittee::new(validators).unwrap(), fork: setup.genesis.fork.clone(), + ..Default::default() }; let view: ViewNumber = rng.gen(); From dccfaad4be062a04ae9a8c85530fc3136a879260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Thu, 11 Apr 2024 20:12:45 -0300 Subject: [PATCH 27/35] some more corrections from PR review --- node/actors/bft/src/leader/replica_commit.rs | 6 +----- node/actors/bft/src/leader/replica_prepare.rs | 3 +-- node/libs/roles/src/validator/messages/consensus.rs | 2 +- node/libs/roles/src/validator/messages/leader_commit.rs | 2 +- node/libs/roles/src/validator/messages/leader_prepare.rs | 4 ++-- node/libs/roles/src/validator/tests.rs | 5 ++--- 6 files changed, 8 insertions(+), 14 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index 64b0b411..a1df12a1 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -134,11 +134,7 @@ impl StateMachine { .insert(author.clone(), signed_message.clone()); // Now we check if we have enough weight to continue. - let weight = self - .config - .genesis() - .validators - .weight(commit_qc.signers.clone()); + let weight = self.config.genesis().validators.weight(&commit_qc.signers); 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 2fbb58dd..0f6b193d 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/node/actors/bft/src/leader/replica_prepare.rs @@ -146,9 +146,8 @@ impl StateMachine { // Now we check if we have enough weight to continue. let weight: u64 = prepare_qc - .clone() .map - .into_values() + .values() .map(|signers| self.config.genesis().validators.weight(signers)) .sum(); if weight < self.config.genesis().validators.threshold() { diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 5ce6e8f6..9490f6ed 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -168,7 +168,7 @@ impl ValidatorCommittee { } /// Compute the sum of signers weights. - pub fn weight(&self, signers: Signers) -> u64 { + pub fn weight(&self, signers: &Signers) -> u64 { self.vec .iter() .enumerate() diff --git a/node/libs/roles/src/validator/messages/leader_commit.rs b/node/libs/roles/src/validator/messages/leader_commit.rs index dce7d7fe..a83fa4ed 100644 --- a/node/libs/roles/src/validator/messages/leader_commit.rs +++ b/node/libs/roles/src/validator/messages/leader_commit.rs @@ -101,7 +101,7 @@ impl CommitQC { } // Verify the signers' weight is enough. - let weight = genesis.validators.weight(self.signers.clone()); + let weight = genesis.validators.weight(&self.signers); let threshold = genesis.validators.threshold(); if weight < threshold { return Err(Error::NotEnoughSigners { diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 5eafb6f7..64a7500c 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -59,7 +59,7 @@ impl PrepareQC { 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() += genesis.validators.weight(signers.clone()); + *count.entry(v.proposal).or_default() += genesis.validators.weight(signers); } } // We only take one value from the iterator because there can only be at most one block with a quorum of 2f+1 votes. @@ -128,7 +128,7 @@ impl PrepareQC { } // Verify the signers' weight is enough. - let weight = genesis.validators.weight(sum.clone()); + let weight = genesis.validators.weight(&sum); let threshold = genesis.validators.threshold(); if weight < threshold { return Err(Error::NotEnoughSigners { diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index ee7a0e0e..49ab4f81 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -1,6 +1,5 @@ use super::*; use crate::validator::testonly::Setup; -use anyhow::Error; use assert_matches::assert_matches; use rand::{seq::SliceRandom, Rng}; use std::vec; @@ -311,7 +310,7 @@ fn test_validator_committee_weights() { let key = &setup.keys[n]; qc.add(&key.sign_msg(msg.clone()), &setup.genesis); let signers = &qc.map[&msg]; - assert_eq!(genesis.validators.weight(signers.clone()), *weight); + assert_eq!(genesis.validators.weight(signers), *weight); } } @@ -334,5 +333,5 @@ fn test_validator_weights_sanity() { .collect(); // Creation should overflow - assert_matches!(ValidatorCommittee::new(validators), Err(Error { .. })); + assert_matches!(ValidatorCommittee::new(validators), Err(_)); } From c074717b31ef39e30c86a47a00a794979b06f2aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 12 Apr 2024 11:12:13 -0300 Subject: [PATCH 28/35] Fixing protocol buffer backward compatibility --- node/libs/roles/src/proto/validator.proto | 4 ++-- node/libs/roles/src/validator/conv.rs | 17 +++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/node/libs/roles/src/proto/validator.proto b/node/libs/roles/src/proto/validator.proto index 4da73223..d779bcac 100644 --- a/node/libs/roles/src/proto/validator.proto +++ b/node/libs/roles/src/proto/validator.proto @@ -11,8 +11,8 @@ message Fork { message Genesis { optional Fork fork = 1; // required - repeated PublicKey validator_keys = 2; - repeated WeightedValidator validators = 3; + repeated PublicKey validators = 2 [deprecated = true]; + repeated WeightedValidator validators_v1 = 3; } message GenesisHash { diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 7fdd494b..2dba0745 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -28,14 +28,15 @@ impl ProtoFmt for Fork { } } +#[allow(deprecated)] impl ProtoFmt for Genesis { type Proto = proto::Genesis; fn read(r: &Self::Proto) -> anyhow::Result { let (validators, encoding_version) = // current genesis encoding version 1 - if !r.validators.is_empty() { + if !r.validators_v1.is_empty() { ( - r.validators + r.validators_v1 .iter() .enumerate() .map(|(i, v)| WeightedValidator::read(v).context(i)) @@ -44,9 +45,9 @@ impl ProtoFmt for Genesis { 1, ) // legacy genesis encoding version 0 - } else if !r.validator_keys.is_empty() { + } else if !r.validators.is_empty() { ( - r.validator_keys + r.validators .iter() .enumerate() .map(|(i, v)| anyhow::Ok(WeightedValidator { @@ -71,13 +72,13 @@ impl ProtoFmt for Genesis { match self.encoding_version { 0 => Self::Proto { fork: Some(self.fork.build()), - validator_keys: self.validators.iter().map(|v| v.key.build()).collect(), - validators: vec![], + validators: self.validators.iter().map(|v| v.key.build()).collect(), + validators_v1: vec![], }, 1.. => Self::Proto { fork: Some(self.fork.build()), - validator_keys: vec![], - validators: self.validators.iter().map(|v| v.build()).collect(), + validators: vec![], + validators_v1: self.validators.iter().map(|v| v.build()).collect(), }, } } From 2f581685b5bdeb854b1d697966585ee7c517ff3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 12 Apr 2024 14:46:38 -0300 Subject: [PATCH 29/35] Several PR review corrections --- node/actors/bft/src/leader/tests.rs | 6 +++--- node/actors/bft/src/testonly/ut_harness.rs | 8 ++++---- node/actors/bft/src/tests.rs | 2 +- node/actors/network/src/gossip/tests.rs | 2 +- .../actors/network/src/gossip/validator_addrs.rs | 4 ++-- node/libs/roles/src/validator/conv.rs | 9 ++++----- .../roles/src/validator/messages/consensus.rs | 10 ++++++---- node/libs/roles/src/validator/messages/tests.rs | 2 +- node/libs/roles/src/validator/testonly.rs | 16 ++++++++-------- node/libs/roles/src/validator/tests.rs | 10 ++++------ 10 files changed, 34 insertions(+), 35 deletions(-) diff --git a/node/actors/bft/src/leader/tests.rs b/node/actors/bft/src/leader/tests.rs index df4ce0e1..8c451992 100644 --- a/node/actors/bft/src/leader/tests.rs +++ b/node/actors/bft/src/leader/tests.rs @@ -375,15 +375,15 @@ async fn replica_prepare_different_messages() { } let mut replica_commit_result = None; - // The rest (minus one) of the validators sign other_replica_prepare - for i in validators / 2..validators - 1 { + // The rest of the validators until threshold sign other_replica_prepare + for i in validators / 2..util.genesis().validators.threshold() as usize { replica_commit_result = util .process_replica_prepare(ctx, util.keys[i].sign_msg(other_replica_prepare.clone())) .await .unwrap(); } - // That should be enough for a proposal to be committed + // That should be enough for a proposal to be committed (even with different proposals) assert_matches!(replica_commit_result, Some(_)); // Check the first proposal has been committed (as it has more votes) diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index b4f26a5b..bdf63792 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -224,14 +224,14 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaPrepare, ) -> Signed { - let expected_validator_weight = self.genesis().validators.iter().next().unwrap().weight; let mut leader_prepare = None; let msgs: Vec<_> = self.keys.iter().map(|k| k.sign_msg(msg.clone())).collect(); let mut first_match = true; for (i, msg) in msgs.into_iter().enumerate() { let res = self.process_replica_prepare(ctx, msg).await; match ( - (i + 1) as u64 * expected_validator_weight < self.genesis().validators.threshold(), + (i + 1) as u64 * self.genesis().validators.iter().next().unwrap().weight + < self.genesis().validators.threshold(), first_match, ) { (true, _) => assert!(res.unwrap().is_none()), @@ -259,14 +259,14 @@ impl UTHarness { ctx: &ctx::Ctx, msg: ReplicaCommit, ) -> Signed { - let expected_validator_weight = self.genesis().validators.iter().next().unwrap().weight; let mut first_match = true; for (i, key) in self.keys.iter().enumerate() { let res = self .leader .process_replica_commit(ctx, key.sign_msg(msg.clone())); match ( - (i + 1) as u64 * expected_validator_weight < self.genesis().validators.threshold(), + (i + 1) as u64 * self.genesis().validators.iter().next().unwrap().weight + < self.genesis().validators.threshold(), first_match, ) { (true, _) => res.unwrap(), diff --git a/node/actors/bft/src/tests.rs b/node/actors/bft/src/tests.rs index b7f2a344..943c7b73 100644 --- a/node/actors/bft/src/tests.rs +++ b/node/actors/bft/src/tests.rs @@ -10,7 +10,7 @@ async fn run_test(behavior: Behavior, network: Network) { const NODES: usize = 11; let mut nodes = vec![behavior; NODES]; // validator::threshold(NODES) will calculate required nodes to validate a message - // assuming uniform weight distribution + // assuming each node weight is 1 let honest_nodes_amount = validator::threshold(NODES as u64) as usize; for n in &mut nodes[0..honest_nodes_amount] { *n = Behavior::Honest; diff --git a/node/actors/network/src/gossip/tests.rs b/node/actors/network/src/gossip/tests.rs index 63ec7975..b897e0a6 100644 --- a/node/actors/network/src/gossip/tests.rs +++ b/node/actors/network/src/gossip/tests.rs @@ -143,7 +143,7 @@ async fn test_validator_addrs() { let rng = &mut ctx::test_root(&ctx::RealClock).rng(); let keys: Vec = (0..8).map(|_| rng.gen()).collect(); - let validators = validator::ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { + let validators = validator::Committee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), weight: 1250, })) diff --git a/node/actors/network/src/gossip/validator_addrs.rs b/node/actors/network/src/gossip/validator_addrs.rs index 1d962d1e..51c736af 100644 --- a/node/actors/network/src/gossip/validator_addrs.rs +++ b/node/actors/network/src/gossip/validator_addrs.rs @@ -41,7 +41,7 @@ impl ValidatorAddrs { /// Returns true iff some new entry was added. pub(super) fn update( &mut self, - validators: &validator::ValidatorCommittee, + validators: &validator::Committee, data: &[Arc>], ) -> anyhow::Result { let mut changed = false; @@ -119,7 +119,7 @@ impl ValidatorAddrsWatch { /// invalid entry should be banned. pub(crate) async fn update( &self, - validators: &validator::ValidatorCommittee, + validators: &validator::Committee, data: &[Arc>], ) -> anyhow::Result<()> { let this = self.0.lock().await; diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 2dba0745..22f107bc 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -1,9 +1,8 @@ use super::{ - AggregateSignature, BlockHeader, BlockNumber, CommitQC, ConsensusMsg, FinalBlock, Fork, - ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, + AggregateSignature, BlockHeader, BlockNumber, CommitQC, Committee, ConsensusMsg, FinalBlock, + Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, Signature, Signed, Signers, ValidatorCommittee, View, ViewNumber, - WeightedValidator, + ReplicaPrepare, Signature, Signed, Signers, View, ViewNumber, WeightedValidator, }; use crate::{node::SessionId, proto::validator as proto}; use anyhow::Context as _; @@ -64,7 +63,7 @@ impl ProtoFmt for Genesis { }; Ok(Self { fork: read_required(&r.fork).context("fork")?, - validators: ValidatorCommittee::new(validators.into_iter()).context("validators")?, + validators: Committee::new(validators.into_iter()).context("validators")?, encoding_version, }) } diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 9490f6ed..fc32bbec 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -79,13 +79,13 @@ impl Default for Fork { /// A struct that represents a set of validators. It is used to store the current validator set. /// We represent each validator by its validator public key. #[derive(Clone, Debug, PartialEq, Eq, Default)] -pub struct ValidatorCommittee { +pub struct Committee { vec: Vec, indexes: BTreeMap, total_weight: u64, } -impl ValidatorCommittee { +impl Committee { /// Creates a new ValidatorCommittee from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { let mut set = BTreeSet::new(); @@ -168,7 +168,9 @@ impl ValidatorCommittee { } /// Compute the sum of signers weights. + /// Panics if signers length does not match the number of validators in committee pub fn weight(&self, signers: &Signers) -> u64 { + assert_eq!(self.vec.len(), signers.len()); self.vec .iter() .enumerate() @@ -206,7 +208,7 @@ pub struct Genesis { /// Genesis encoding version pub encoding_version: usize, /// Set of validators of the chain. - pub validators: ValidatorCommittee, + pub validators: Committee, /// Fork of the chain to follow. pub fork: Fork, } @@ -226,7 +228,7 @@ impl Default for Genesis { fn default() -> Self { Self { encoding_version: CURRENT_GENESIS_ENCODING_VERSION, - validators: ValidatorCommittee::default(), + validators: Committee::default(), fork: Fork::default(), } } diff --git a/node/libs/roles/src/validator/messages/tests.rs b/node/libs/roles/src/validator/messages/tests.rs index 5bacbe2e..eecbec77 100644 --- a/node/libs/roles/src/validator/messages/tests.rs +++ b/node/libs/roles/src/validator/messages/tests.rs @@ -33,7 +33,7 @@ fn fork() -> Fork { /// Hardcoded genesis. fn genesis() -> Genesis { Genesis { - validators: ValidatorCommittee::new(keys().iter().map(|k| WeightedValidator { + validators: Committee::new(keys().iter().map(|k| WeightedValidator { key: k.public(), weight: 1, })) diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 816c5e25..859a23f1 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -1,10 +1,10 @@ //! Test-only utilities. use super::{ - AggregateSignature, BlockHeader, BlockNumber, CommitQC, ConsensusMsg, FinalBlock, Fork, - ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, + AggregateSignature, BlockHeader, BlockNumber, CommitQC, Committee, ConsensusMsg, FinalBlock, + Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, SecretKey, Signature, Signed, Signers, ValidatorCommittee, View, ViewNumber, - WeightedValidator, CURRENT_GENESIS_ENCODING_VERSION, + ReplicaPrepare, SecretKey, Signature, Signed, Signers, View, ViewNumber, WeightedValidator, + CURRENT_GENESIS_ENCODING_VERSION, }; use bit_vec::BitVec; use rand::{ @@ -24,7 +24,7 @@ impl Setup { pub fn new_with_fork(rng: &mut impl Rng, validators: usize, fork: Fork) -> Self { let keys: Vec = (0..validators).map(|_| rng.gen()).collect(); let genesis = Genesis { - validators: ValidatorCommittee::new(keys.iter().map(|k| WeightedValidator { + validators: Committee::new(keys.iter().map(|k| WeightedValidator { key: k.public(), weight: 1, })) @@ -299,14 +299,14 @@ impl Distribution for Standard { } } -impl Distribution for Standard { - fn sample(&self, rng: &mut R) -> ValidatorCommittee { +impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> Committee { let count = rng.gen_range(1..11); let public_keys = (0..count).map(|_| WeightedValidator { key: rng.gen(), weight: 1, }); - ValidatorCommittee::new(public_keys).unwrap() + Committee::new(public_keys).unwrap() } } diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 49ab4f81..18c21053 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -204,8 +204,7 @@ 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.iter().take(3).cloned()) - .unwrap(), + validators: Committee::new(setup1.genesis.validators.iter().take(3).cloned()).unwrap(), fork: setup1.genesis.fork.clone(), ..Default::default() }; @@ -243,8 +242,7 @@ 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.iter().take(3).cloned()) - .unwrap(), + validators: Committee::new(setup1.genesis.validators.iter().take(3).cloned()).unwrap(), fork: setup1.genesis.fork.clone(), ..Default::default() }; @@ -298,7 +296,7 @@ fn test_validator_committee_weights() { .collect(); let genesis = Genesis { - validators: ValidatorCommittee::new(validators).unwrap(), + validators: Committee::new(validators).unwrap(), fork: setup.genesis.fork.clone(), ..Default::default() }; @@ -333,5 +331,5 @@ fn test_validator_weights_sanity() { .collect(); // Creation should overflow - assert_matches!(ValidatorCommittee::new(validators), Err(_)); + assert_matches!(Committee::new(validators), Err(_)); } From 1764d1321e7c756161f8fccbf8ab8c45393348e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 12 Apr 2024 16:50:22 -0300 Subject: [PATCH 30/35] Removed redundant BTtreeMap --- node/actors/bft/src/tests.rs | 2 +- .../roles/src/validator/messages/consensus.rs | 38 ++++++++----------- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/node/actors/bft/src/tests.rs b/node/actors/bft/src/tests.rs index 943c7b73..18fd5185 100644 --- a/node/actors/bft/src/tests.rs +++ b/node/actors/bft/src/tests.rs @@ -10,7 +10,7 @@ async fn run_test(behavior: Behavior, network: Network) { const NODES: usize = 11; let mut nodes = vec![behavior; NODES]; // validator::threshold(NODES) will calculate required nodes to validate a message - // assuming each node weight is 1 + // given each node weight is 1 let honest_nodes_amount = validator::threshold(NODES as u64) as usize; for n in &mut nodes[0..honest_nodes_amount] { *n = Behavior::Honest; diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index fc32bbec..9af9062e 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -3,10 +3,7 @@ use super::{BlockNumber, LeaderCommit, LeaderPrepare, Msg, ReplicaCommit, Replic use crate::validator; use anyhow::Context; use bit_vec::BitVec; -use std::{ - collections::{BTreeMap, BTreeSet}, - fmt, -}; +use std::{collections::BTreeMap, fmt}; use zksync_consensus_crypto::{keccak256::Keccak256, ByteFmt, Text, TextFmt}; use zksync_consensus_utils::enum_util::{BadVariantError, Variant}; @@ -86,35 +83,30 @@ pub struct Committee { } impl Committee { - /// Creates a new ValidatorCommittee from a list of validator public keys. + /// Creates a new Committee from a list of validator public keys. pub fn new(validators: impl IntoIterator) -> anyhow::Result { - let mut set = BTreeSet::new(); let mut weighted_validators = BTreeMap::new(); let mut total_weight: u64 = 0; for validator in validators { anyhow::ensure!( - set.insert(validator.key.clone()), - "Duplicate validator in ValidatorCommittee" + !weighted_validators.contains_key(&validator.key), + "Duplicate validator in validator Committee" ); total_weight = total_weight .checked_add(validator.weight) - .context("Sum of weights overflows in ValidatorCommittee")?; + .context("Sum of weights overflows in validator Committee")?; weighted_validators.insert(validator.key.clone(), validator); } anyhow::ensure!( - !set.is_empty(), - "ValidatorCommittee must contain at least one validator" + !weighted_validators.is_empty(), + "Validator Committee must contain at least one validator" ); Ok(Self { - vec: set - .iter() - .map(|key| weighted_validators[key].clone()) - .collect(), - indexes: set - .clone() - .into_iter() + vec: weighted_validators.values().cloned().collect(), + indexes: weighted_validators + .values() .enumerate() - .map(|(i, pk)| (pk, i)) + .map(|(i, v)| (v.key.clone(), i)) .collect(), total_weight, }) @@ -131,7 +123,7 @@ impl Committee { } /// Returns the number of validators. - #[allow(clippy::len_without_is_empty)] // a valid `ValidatorCommittee` is always non-empty by construction + #[allow(clippy::len_without_is_empty)] // a valid `Committee` is always non-empty by construction pub fn len(&self) -> usize { self.vec.len() } @@ -375,7 +367,7 @@ impl Signers { self.0.iter().filter(|b| *b).count() } - /// Size of the corresponding ValidatorCommittee. + /// Size of the corresponding validator Committee. pub fn len(&self) -> usize { self.0.len() } @@ -432,11 +424,11 @@ pub enum Phase { Commit, } -/// Validator representation inside a ValidatorCommittee. +/// Validator representation inside a Committee. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WeightedValidator { /// Validator key pub key: validator::PublicKey, - /// Validator weight inside the ValidatorCommittee. + /// Validator weight inside the Committee. pub weight: u64, } From 0df72f94e225b0441c3b58124c9f4b6ecf20e5a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20Dimitroff=20H=C3=B3di?= Date: Fri, 12 Apr 2024 17:45:22 -0300 Subject: [PATCH 31/35] Removed commit_message_cache as it is no longer needed --- node/actors/bft/src/leader/replica_commit.rs | 35 ++++++-------------- node/actors/bft/src/leader/state_machine.rs | 6 ---- 2 files changed, 11 insertions(+), 30 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index a1df12a1..16a77e20 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -96,14 +96,18 @@ impl StateMachine { return Err(Error::NotLeaderInView); } + // Get current incrementally-constructed QC to work on it + let commit_qc = self + .commit_qcs + .entry(message.view.number) + .or_default() + .entry(message.clone()) + .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis())); + // If we already have a message from the same validator and for the same view, we discard it. - if let Some(existing_message) = self - .commit_message_cache - .get(&message.view.number) - .and_then(|x| x.get(author)) - { + if commit_qc.signers.0[self.config.genesis().validators.index(author).unwrap()] { return Err(Error::DuplicateMessage { - existing_message: existing_message.msg.clone(), + existing_message: commit_qc.message.clone(), }); } @@ -118,21 +122,9 @@ impl StateMachine { // ----------- All checks finished. Now we process the message. -------------- - // We add the message to the incrementally-constructed QC. - let commit_qc = self - .commit_qcs - .entry(message.view.number) - .or_default() - .entry(message.clone()) - .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis())); + // Add the message to the QC. commit_qc.add(&signed_message, self.config.genesis()); - // We store the message in our cache. - 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(&commit_qc.signers); if weight < self.config.genesis().validators.threshold() { @@ -150,10 +142,6 @@ impl StateMachine { // ----------- Prepare our message and send it. -------------- - // Remove replica commit messages for this view, so that we don't create a new leader commit - // for this same view if we receive another replica commit message after this. - self.commit_message_cache.remove(&message.view.number); - // Consume the incrementally-constructed QC for this view. let justification = self .commit_qcs @@ -176,7 +164,6 @@ impl StateMachine { // Clean the caches. self.prepare_message_cache.retain(|k, _| k >= &self.view); - self.commit_message_cache.retain(|k, _| k >= &self.view); Ok(()) } diff --git a/node/actors/bft/src/leader/state_machine.rs b/node/actors/bft/src/leader/state_machine.rs index fe10d32c..3abf73ca 100644 --- a/node/actors/bft/src/leader/state_machine.rs +++ b/node/actors/bft/src/leader/state_machine.rs @@ -36,11 +36,6 @@ pub(crate) struct StateMachine { pub(crate) prepare_qcs: BTreeMap, /// Newest prepare QC composed from the `ReplicaPrepare` messages. pub(crate) prepare_qc: sync::watch::Sender>, - /// A cache of replica commit messages indexed by view number and validator. - pub(crate) commit_message_cache: BTreeMap< - validator::ViewNumber, - HashMap>, - >, /// Commit QCs indexed by view number and then by message. pub(crate) commit_qcs: BTreeMap>, @@ -68,7 +63,6 @@ impl StateMachine { phase_start: ctx.now(), prepare_message_cache: BTreeMap::new(), prepare_qcs: BTreeMap::new(), - commit_message_cache: BTreeMap::new(), prepare_qc: sync::watch::channel(None).0, commit_qcs: BTreeMap::new(), inbound_pipe: recv, From 5afab5826458dc9ba51762ddef4ce1432003c812 Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Mon, 15 Apr 2024 12:21:16 -0300 Subject: [PATCH 32/35] Addressed several suggestions in PR review --- node/actors/bft/src/leader/replica_prepare.rs | 9 +++----- node/actors/bft/src/testonly/run.rs | 11 ++++++---- node/actors/bft/src/tests.rs | 6 ++--- node/libs/roles/src/proto/validator.proto | 3 +++ node/libs/roles/src/validator/conv.rs | 2 +- .../src/validator/messages/leader_prepare.rs | 8 +++++++ .../roles/src/validator/messages/tests.rs | 22 ++++++++++++++----- node/libs/roles/src/validator/testonly.rs | 15 ++++++++----- 8 files changed, 51 insertions(+), 25 deletions(-) diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index 0f6b193d..4e3a6f1b 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/node/actors/bft/src/leader/replica_prepare.rs @@ -145,12 +145,9 @@ impl StateMachine { .insert(author.clone(), signed_message.clone()); // Now we check if we have enough weight to continue. - let weight: u64 = prepare_qc - .map - .values() - .map(|signers| self.config.genesis().validators.weight(signers)) - .sum(); - if weight < self.config.genesis().validators.threshold() { + if prepare_qc.weight(&self.config.genesis().validators) + < self.config.genesis().validators.threshold() + { return Ok(()); } diff --git a/node/actors/bft/src/testonly/run.rs b/node/actors/bft/src/testonly/run.rs index af06ea04..9d346174 100644 --- a/node/actors/bft/src/testonly/run.rs +++ b/node/actors/bft/src/testonly/run.rs @@ -17,7 +17,7 @@ pub(crate) enum Network { #[derive(Clone)] pub(crate) struct Test { pub(crate) network: Network, - pub(crate) nodes: Vec, + pub(crate) nodes: Vec<(Behavior, u64)>, pub(crate) blocks_to_finalize: usize, } @@ -25,7 +25,10 @@ impl Test { /// Run a test with the given parameters. pub(crate) async fn run(&self, ctx: &ctx::Ctx) -> anyhow::Result<()> { let rng = &mut ctx.rng(); - let setup = validator::testonly::Setup::new(rng, self.nodes.len()); + let setup = validator::testonly::Setup::new_with_weights( + rng, + self.nodes.iter().map(|(_, w)| *w).collect(), + ); let nets: Vec<_> = network::testonly::new_configs(rng, &setup, 1); let mut nodes = vec![]; let mut honest = vec![]; @@ -33,12 +36,12 @@ impl Test { for (i, net) in nets.into_iter().enumerate() { let (store, runner) = new_store(ctx, &setup.genesis).await; s.spawn_bg(runner.run(ctx)); - if self.nodes[i] == Behavior::Honest { + if self.nodes[i].0 == Behavior::Honest { honest.push(store.clone()); } nodes.push(Node { net, - behavior: self.nodes[i], + behavior: self.nodes[i].0, block_store: store, }); } diff --git a/node/actors/bft/src/tests.rs b/node/actors/bft/src/tests.rs index 18fd5185..eaddccb2 100644 --- a/node/actors/bft/src/tests.rs +++ b/node/actors/bft/src/tests.rs @@ -8,12 +8,12 @@ async fn run_test(behavior: Behavior, network: Network) { let ctx = &ctx::test_root(&ctx::RealClock); const NODES: usize = 11; - let mut nodes = vec![behavior; NODES]; + let mut nodes = vec![(behavior, 1u64); NODES]; // validator::threshold(NODES) will calculate required nodes to validate a message // given each node weight is 1 let honest_nodes_amount = validator::threshold(NODES as u64) as usize; for n in &mut nodes[0..honest_nodes_amount] { - *n = Behavior::Honest; + n.0 = Behavior::Honest; } Test { network, @@ -188,7 +188,7 @@ async fn non_proposing_leader() { let ctx = &ctx::test_root(&ctx::AffineClock::new(5.)); Test { network: Network::Real, - nodes: vec![Behavior::Honest, Behavior::HonestNotProposing], + nodes: vec![(Behavior::Honest, 1), (Behavior::HonestNotProposing, 1)], blocks_to_finalize: 10, } .run(ctx) diff --git a/node/libs/roles/src/proto/validator.proto b/node/libs/roles/src/proto/validator.proto index d779bcac..5b5883b9 100644 --- a/node/libs/roles/src/proto/validator.proto +++ b/node/libs/roles/src/proto/validator.proto @@ -9,6 +9,9 @@ message Fork { optional uint64 first_block = 2; // required; BlockNumber } +// validators and validators_v1 are mutually exclusive. +// They should be put into a oneof, but it may not recognize this as +// a backward compatible change: https://buf.build/docs/breaking/rules#field_same_oneof message Genesis { optional Fork fork = 1; // required repeated PublicKey validators = 2 [deprecated = true]; diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 22f107bc..f307723c 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -57,7 +57,7 @@ impl ProtoFmt for Genesis { .context("validators")?, 0, ) - // empty validator set + // empty validator set, will raise an exception in Committee:new() } else { (vec![], 0) }; diff --git a/node/libs/roles/src/validator/messages/leader_prepare.rs b/node/libs/roles/src/validator/messages/leader_prepare.rs index 64a7500c..2e034ff1 100644 --- a/node/libs/roles/src/validator/messages/leader_prepare.rs +++ b/node/libs/roles/src/validator/messages/leader_prepare.rs @@ -151,6 +151,14 @@ impl PrepareQC { .verify_messages(messages_and_keys) .map_err(Error::BadSignature) } + + /// Calculates the weight of current PrepareQC signing validators + pub fn weight(&self, committee: &validator::Committee) -> u64 { + self.map + .values() + .map(|signers| committee.weight(signers)) + .sum() + } } /// A Prepare message from a leader. diff --git a/node/libs/roles/src/validator/messages/tests.rs b/node/libs/roles/src/validator/messages/tests.rs index eecbec77..ef2092f1 100644 --- a/node/libs/roles/src/validator/messages/tests.rs +++ b/node/libs/roles/src/validator/messages/tests.rs @@ -31,7 +31,7 @@ fn fork() -> Fork { } /// Hardcoded genesis. -fn genesis() -> Genesis { +fn genesis(encoding_version: usize) -> Genesis { Genesis { validators: Committee::new(keys().iter().map(|k| WeightedValidator { key: k.public(), @@ -39,7 +39,7 @@ fn genesis() -> Genesis { })) .unwrap(), fork: fork(), - encoding_version: 0, + encoding_version, } } @@ -57,13 +57,23 @@ fn payload_hash_change_detector() { /// Even if it was, ALL versions of genesis need to be supported FOREVER, /// unless we introduce dynamic regenesis. #[test] -fn genesis_hash_change_detector() { +fn genesis_v0_hash_change_detector() { let want: GenesisHash = Text::new( "genesis_hash:keccak256:9c9bfa303e8d2d451a7fadd327f5f1b957a37c84d7b27b9e1cf7b92fd83af7ae", ) .decode() .unwrap(); - assert_eq!(want, genesis().hash()); + assert_eq!(want, genesis(0).hash()); +} + +#[test] +fn genesis_v1_hash_change_detector() { + let want: GenesisHash = Text::new( + "genesis_hash:keccak256:6370cfce637395629f05599082993c446c2c66145d440287a985ac98ad210b41", + ) + .decode() + .unwrap(); + assert_eq!(want, genesis(1).hash()); } mod version1 { @@ -112,7 +122,7 @@ mod version1 { /// Hardcoded `CommitQC`. fn commit_qc() -> CommitQC { - let genesis = genesis(); + let genesis = genesis(1); let replica_commit = replica_commit(); let mut x = CommitQC::new(replica_commit.clone(), &genesis); for k in keys() { @@ -140,7 +150,7 @@ mod version1 { /// Hardcoded `PrepareQC`. fn prepare_qc() -> PrepareQC { let mut x = PrepareQC::new(view()); - let genesis = genesis(); + let genesis = genesis(1); let replica_prepare = replica_prepare(); for k in keys() { x.add(&k.sign_msg(replica_prepare.clone()), &genesis); diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index 859a23f1..cebf1dd2 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -21,12 +21,12 @@ pub struct Setup(SetupInner); 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(); + pub fn new_with_fork(rng: &mut impl Rng, weights: Vec, fork: Fork) -> Self { + let keys: Vec = (0..weights.len()).map(|_| rng.gen()).collect(); let genesis = Genesis { - validators: Committee::new(keys.iter().map(|k| WeightedValidator { + validators: Committee::new(keys.iter().enumerate().map(|(i, k)| WeightedValidator { key: k.public(), - weight: 1, + weight: weights[i], })) .unwrap(), fork, @@ -41,11 +41,16 @@ impl Setup { /// New `Setup`. pub fn new(rng: &mut impl Rng, validators: usize) -> Self { + Self::new_with_weights(rng, vec![1; validators]) + } + + /// New `Setup`. + pub fn new_with_weights(rng: &mut impl Rng, weights: Vec) -> Self { let fork = Fork { number: ForkNumber(rng.gen_range(0..100)), first_block: BlockNumber(rng.gen_range(0..100)), }; - Self::new_with_fork(rng, validators, fork) + Self::new_with_fork(rng, weights, fork) } /// Next block to finalize. From d6f2a8acdd84b70e97abe56be4cf67b252ae66cc Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Mon, 15 Apr 2024 16:04:31 -0300 Subject: [PATCH 33/35] Improved some tests and added zero weight validator test --- .../roles/src/validator/messages/consensus.rs | 4 ++ node/libs/roles/src/validator/tests.rs | 51 +++++++++---------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 9af9062e..2cafd37c 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -92,6 +92,10 @@ impl Committee { !weighted_validators.contains_key(&validator.key), "Duplicate validator in validator Committee" ); + anyhow::ensure!( + validator.weight > 0, + "Validator weight has to be a positive value" + ); total_weight = total_weight .checked_add(validator.weight) .context("Sum of weights overflows in validator Committee")?; diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 18c21053..56491bf6 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -281,25 +281,10 @@ fn test_validator_committee_weights() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); - let setup = Setup::new(rng, 6); - // Validators weights - let weights = [1000, 600, 800, 6000, 900, 700]; + // Validators with non-uniform weights + let setup = Setup::new_with_weights(rng, vec![1000, 600, 800, 6000, 900, 700]); // Expected sum of the validators weights let sums = [1000, 1600, 2400, 8400, 9300, 10000]; - let validators: Vec = weights - .iter() - .enumerate() - .map(|(i, w)| WeightedValidator { - key: setup.keys[i].public(), - weight: *w, - }) - .collect(); - - let genesis = Genesis { - validators: Committee::new(validators).unwrap(), - fork: setup.genesis.fork.clone(), - ..Default::default() - }; let view: ViewNumber = rng.gen(); let msg = make_replica_prepare(rng, view, &setup); @@ -308,24 +293,19 @@ fn test_validator_committee_weights() { let key = &setup.keys[n]; qc.add(&key.sign_msg(msg.clone()), &setup.genesis); let signers = &qc.map[&msg]; - assert_eq!(genesis.validators.weight(signers), *weight); + assert_eq!(setup.genesis.validators.weight(signers), *weight); } } #[test] -fn test_validator_weights_sanity() { +fn test_committee_weights_overflow_check() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); - let setup = Setup::new(rng, 6); - // Validators weights - let weight = u64::MAX / 5; - let weights = [weight; 6]; - let validators: Vec = weights + let validators: Vec = [u64::MAX / 5; 6] .iter() - .enumerate() - .map(|(i, w)| WeightedValidator { - key: setup.keys[i].public(), + .map(|w| WeightedValidator { + key: rng.gen::().public(), weight: *w, }) .collect(); @@ -333,3 +313,20 @@ fn test_validator_weights_sanity() { // Creation should overflow assert_matches!(Committee::new(validators), Err(_)); } + +#[test] +fn test_committee_with_zero_weights() { + let ctx = ctx::test_root(&ctx::RealClock); + let rng = &mut ctx.rng(); + + let validators: Vec = vec![1000, 0, 800, 6000, 0, 700] + .iter() + .map(|w| WeightedValidator { + key: rng.gen::().public(), + weight: *w, + }) + .collect(); + + // Committee creation should error on zero weight validators + assert_matches!(Committee::new(validators), Err(_)); +} From 5e13fb32f393920973d3242db59b22b747ea6386 Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Tue, 16 Apr 2024 14:24:28 -0300 Subject: [PATCH 34/35] Proper duplicate signature check and some suggested code improvements --- node/actors/bft/src/leader/replica_commit.rs | 17 +++++++----- node/actors/bft/src/leader/state_machine.rs | 3 +++ node/actors/bft/src/leader/tests.rs | 20 ++++++++++++-- node/libs/roles/src/validator/conv.rs | 24 ++++++++--------- .../roles/src/validator/messages/consensus.rs | 17 ++++++++---- .../roles/src/validator/messages/tests.rs | 27 ++++++++++++++----- node/libs/roles/src/validator/testonly.rs | 16 +++++++---- node/libs/roles/src/validator/tests.rs | 2 +- 8 files changed, 87 insertions(+), 39 deletions(-) diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index 16a77e20..f0e817ac 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -39,10 +39,10 @@ pub(crate) enum Error { #[error("invalid message: {0:#}")] InvalidMessage(#[source] anyhow::Error), /// Duplicate message from a replica. - #[error("duplicate message from a replica (existing message: {existing_message:?}")] - DuplicateMessage { - /// Existing message from the same replica. - existing_message: validator::ReplicaCommit, + #[error("Replica signed more than one message for same view (message: {message:?}")] + DuplicateSignature { + /// Offending message. + message: validator::ReplicaCommit, }, /// Invalid message signature. #[error("invalid signature: {0:#}")] @@ -105,11 +105,14 @@ impl StateMachine { .or_insert_with(|| CommitQC::new(message.clone(), self.config.genesis())); // If we already have a message from the same validator and for the same view, we discard it. - if commit_qc.signers.0[self.config.genesis().validators.index(author).unwrap()] { - return Err(Error::DuplicateMessage { - existing_message: commit_qc.message.clone(), + let validator_view = self.validator_views.get(author); + if validator_view.is_some_and(|view_number| *view_number >= message.view.number) { + return Err(Error::DuplicateSignature { + message: commit_qc.message.clone(), }); } + self.validator_views + .insert(author.clone(), message.view.number); // ----------- Checking the signed part of the message -------------- diff --git a/node/actors/bft/src/leader/state_machine.rs b/node/actors/bft/src/leader/state_machine.rs index 3abf73ca..8e6dc15b 100644 --- a/node/actors/bft/src/leader/state_machine.rs +++ b/node/actors/bft/src/leader/state_machine.rs @@ -39,6 +39,8 @@ pub(crate) struct StateMachine { /// Commit QCs indexed by view number and then by message. pub(crate) commit_qcs: BTreeMap>, + /// Latest view a validator has signed a message for. + pub(crate) validator_views: BTreeMap, } impl StateMachine { @@ -66,6 +68,7 @@ impl StateMachine { prepare_qc: sync::watch::channel(None).0, commit_qcs: BTreeMap::new(), inbound_pipe: recv, + validator_views: BTreeMap::new(), }; (this, send) diff --git a/node/actors/bft/src/leader/tests.rs b/node/actors/bft/src/leader/tests.rs index 8c451992..d3df3c9d 100644 --- a/node/actors/bft/src/leader/tests.rs +++ b/node/actors/bft/src/leader/tests.rs @@ -546,15 +546,31 @@ async fn replica_commit_already_exists() { .await .unwrap() .is_none()); + + // Processing twice same ReplicaCommit for same view gets DuplicateSignature error let res = util .process_replica_commit(ctx, util.sign(replica_commit.clone())) .await; assert_matches!( res, - Err(replica_commit::Error::DuplicateMessage { existing_message }) => { - assert_eq!(existing_message, replica_commit) + Err(replica_commit::Error::DuplicateSignature { message }) => { + assert_eq!(message, replica_commit) } ); + + // Processing twice different ReplicaCommit for same view gets DuplicateSignature error too + let mut different_replica_commit = replica_commit.clone(); + different_replica_commit.proposal.number = replica_commit.proposal.number.next(); + let res = util + .process_replica_commit(ctx, util.sign(different_replica_commit.clone())) + .await; + assert_matches!( + res, + Err(replica_commit::Error::DuplicateSignature { message }) => { + assert_eq!(message, different_replica_commit) + } + ); + Ok(()) }) .await diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index f307723c..8070e1b6 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -1,8 +1,8 @@ use super::{ AggregateSignature, BlockHeader, BlockNumber, CommitQC, Committee, ConsensusMsg, FinalBlock, - Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, - Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, Signature, Signed, Signers, View, ViewNumber, WeightedValidator, + Fork, ForkNumber, Genesis, GenesisHash, GenesisVersion, LeaderCommit, LeaderPrepare, Msg, + MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, + ReplicaCommit, ReplicaPrepare, Signature, Signed, Signers, View, ViewNumber, WeightedValidator, }; use crate::{node::SessionId, proto::validator as proto}; use anyhow::Context as _; @@ -31,7 +31,7 @@ impl ProtoFmt for Fork { impl ProtoFmt for Genesis { type Proto = proto::Genesis; fn read(r: &Self::Proto) -> anyhow::Result { - let (validators, encoding_version) = + let (validators, version) = // current genesis encoding version 1 if !r.validators_v1.is_empty() { ( @@ -41,7 +41,7 @@ impl ProtoFmt for Genesis { .map(|(i, v)| WeightedValidator::read(v).context(i)) .collect::>() .context("validators")?, - 1, + GenesisVersion(1), ) // legacy genesis encoding version 0 } else if !r.validators.is_empty() { @@ -55,26 +55,26 @@ impl ProtoFmt for Genesis { })) .collect::>() .context("validators")?, - 0, + GenesisVersion(0), ) - // empty validator set, will raise an exception in Committee:new() + // empty validator set, Committee:new() will later return an error. } else { - (vec![], 0) + (vec![], GenesisVersion::CURRENT) }; Ok(Self { fork: read_required(&r.fork).context("fork")?, validators: Committee::new(validators.into_iter()).context("validators")?, - encoding_version, + version, }) } fn build(&self) -> Self::Proto { - match self.encoding_version { - 0 => Self::Proto { + match self.version { + GenesisVersion(0) => Self::Proto { fork: Some(self.fork.build()), validators: self.validators.iter().map(|v| v.key.build()).collect(), validators_v1: vec![], }, - 1.. => Self::Proto { + GenesisVersion(1..) => Self::Proto { fork: Some(self.fork.build()), validators: vec![], validators_v1: 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 2cafd37c..52639b9c 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -7,9 +7,6 @@ use std::{collections::BTreeMap, fmt}; use zksync_consensus_crypto::{keccak256::Keccak256, ByteFmt, Text, TextFmt}; use zksync_consensus_utils::enum_util::{BadVariantError, Variant}; -/// Current genesis encoding version -pub const CURRENT_GENESIS_ENCODING_VERSION: usize = 1; - /// Version of the consensus algorithm that the validator is using. /// It allows to prevent misinterpretation of messages signed by validators /// using different versions of the binaries. @@ -197,12 +194,22 @@ pub fn max_faulty_weight(total_weight: u64) -> u64 { (total_weight - 1) / 5 } +/// Version of the Genesis representation. +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct GenesisVersion(pub u32); + +impl GenesisVersion { + /// Version 0 - deprecated: Committee encoding does not account weights, assume weight=1 per validator + /// Version 1: Validators with weight within Committee + pub const CURRENT: Self = Self(1); +} + /// Genesis of the blockchain, unique for each blockchain instance. #[derive(Debug, Clone, PartialEq)] pub struct Genesis { // TODO(gprusak): add blockchain id here. /// Genesis encoding version - pub encoding_version: usize, + pub version: GenesisVersion, /// Set of validators of the chain. pub validators: Committee, /// Fork of the chain to follow. @@ -223,7 +230,7 @@ impl Genesis { impl Default for Genesis { fn default() -> Self { Self { - encoding_version: CURRENT_GENESIS_ENCODING_VERSION, + version: GenesisVersion::CURRENT, validators: Committee::default(), fork: Fork::default(), } diff --git a/node/libs/roles/src/validator/messages/tests.rs b/node/libs/roles/src/validator/messages/tests.rs index ef2092f1..ea2c8e8b 100644 --- a/node/libs/roles/src/validator/messages/tests.rs +++ b/node/libs/roles/src/validator/messages/tests.rs @@ -30,8 +30,8 @@ fn fork() -> Fork { } } -/// Hardcoded genesis. -fn genesis(encoding_version: usize) -> Genesis { +/// Hardcoded v0 genesis. +fn genesis_v0() -> Genesis { Genesis { validators: Committee::new(keys().iter().map(|k| WeightedValidator { key: k.public(), @@ -39,7 +39,20 @@ fn genesis(encoding_version: usize) -> Genesis { })) .unwrap(), fork: fork(), - encoding_version, + version: GenesisVersion(0), + } +} + +/// Hardcoded v1 genesis. +fn genesis_v1() -> Genesis { + Genesis { + validators: Committee::new(keys().iter().map(|k| WeightedValidator { + key: k.public(), + weight: 1, + })) + .unwrap(), + fork: fork(), + version: GenesisVersion(1), } } @@ -63,7 +76,7 @@ fn genesis_v0_hash_change_detector() { ) .decode() .unwrap(); - assert_eq!(want, genesis(0).hash()); + assert_eq!(want, genesis_v0().hash()); } #[test] @@ -73,7 +86,7 @@ fn genesis_v1_hash_change_detector() { ) .decode() .unwrap(); - assert_eq!(want, genesis(1).hash()); + assert_eq!(want, genesis_v1().hash()); } mod version1 { @@ -122,7 +135,7 @@ mod version1 { /// Hardcoded `CommitQC`. fn commit_qc() -> CommitQC { - let genesis = genesis(1); + let genesis = genesis_v1(); let replica_commit = replica_commit(); let mut x = CommitQC::new(replica_commit.clone(), &genesis); for k in keys() { @@ -150,7 +163,7 @@ mod version1 { /// Hardcoded `PrepareQC`. fn prepare_qc() -> PrepareQC { let mut x = PrepareQC::new(view()); - let genesis = genesis(1); + let genesis = genesis_v1(); let replica_prepare = replica_prepare(); for k in keys() { x.add(&k.sign_msg(replica_prepare.clone()), &genesis); diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index cebf1dd2..9af49731 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -1,10 +1,10 @@ //! Test-only utilities. use super::{ AggregateSignature, BlockHeader, BlockNumber, CommitQC, Committee, ConsensusMsg, FinalBlock, - Fork, ForkNumber, Genesis, GenesisHash, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, - Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, - ReplicaPrepare, SecretKey, Signature, Signed, Signers, View, ViewNumber, WeightedValidator, - CURRENT_GENESIS_ENCODING_VERSION, + Fork, ForkNumber, Genesis, GenesisHash, GenesisVersion, LeaderCommit, LeaderPrepare, Msg, + MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, + ReplicaCommit, ReplicaPrepare, SecretKey, Signature, Signed, Signers, View, ViewNumber, + WeightedValidator, }; use bit_vec::BitVec; use rand::{ @@ -202,11 +202,17 @@ impl Distribution for Standard { Genesis { validators: rng.gen(), fork: rng.gen(), - encoding_version: rng.gen_range(0..=CURRENT_GENESIS_ENCODING_VERSION), + version: rng.gen(), } } } +impl Distribution for Standard { + fn sample(&self, rng: &mut R) -> GenesisVersion { + GenesisVersion(rng.gen_range(0..=GenesisVersion::CURRENT.0)) + } +} + impl Distribution for Standard { fn sample(&self, rng: &mut R) -> PayloadHash { PayloadHash(rng.gen()) diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 56491bf6..d9975ed0 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -319,7 +319,7 @@ fn test_committee_with_zero_weights() { let ctx = ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); - let validators: Vec = vec![1000, 0, 800, 6000, 0, 700] + let validators: Vec = [1000, 0, 800, 6000, 0, 700] .iter() .map(|w| WeightedValidator { key: rng.gen::().public(), From 608328c1d3c3bc6761216eed14b9ec8d096bb1a5 Mon Sep 17 00:00:00 2001 From: Esteban Dimitroff Hodi Date: Tue, 16 Apr 2024 14:39:57 -0300 Subject: [PATCH 35/35] Rewrote minimal validator amount assertion in UTHarness::new_many() --- node/actors/bft/src/testonly/ut_harness.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index bdf63792..a99dae9f 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -83,8 +83,9 @@ 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::max_faulty_weight(num_validators as u64) > 0); - UTHarness::new(ctx, num_validators).await + let (util, runner) = UTHarness::new(ctx, num_validators).await; + assert!(util.genesis().validators.max_faulty_weight() > 0); + (util, runner) } /// Triggers replica timeout, validates the new ReplicaPrepare