Skip to content

Commit

Permalink
Fixed asserts and tests to use weights instead of amount of signers
Browse files Browse the repository at this point in the history
  • Loading branch information
ElFantasma committed Mar 21, 2024
1 parent 4c8e981 commit 5ff923d
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 34 deletions.
15 changes: 13 additions & 2 deletions node/actors/bft/src/leader/replica_commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 --------------

Expand Down
35 changes: 22 additions & 13 deletions node/actors/bft/src/leader/replica_prepare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<validator::ReplicaPrepare>> = 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(());
Expand All @@ -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 --------------

Expand Down
38 changes: 27 additions & 11 deletions node/actors/bft/src/testonly/ut_harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -224,15 +224,23 @@ impl UTHarness {
ctx: &ctx::Ctx,
msg: ReplicaPrepare,
) -> Signed<LeaderPrepare> {
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()
Expand All @@ -252,15 +260,23 @@ impl UTHarness {
ctx: &ctx::Ctx,
msg: ReplicaCommit,
) -> Signed<LeaderCommit> {
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()
Expand Down
17 changes: 11 additions & 6 deletions node/libs/roles/src/validator/messages/consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Item = WeightedValidator>) -> anyhow::Result<Self> {
Expand Down Expand Up @@ -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<usize> {
self.indexes.get(validator).copied()
}
Expand All @@ -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())
}
Expand Down
8 changes: 6 additions & 2 deletions node/libs/roles/src/validator/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

Check warning on line 203 in node/libs/roles/src/validator/tests.rs

View workflow job for this annotation

GitHub Actions / typos

"ceate" should be "create".
let setup1 = Setup::new(rng, 6);
let setup2 = Setup::new(rng, 6);
let genesis3 = Genesis {
Expand All @@ -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!(
Expand All @@ -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

Check warning on line 242 in node/libs/roles/src/validator/tests.rs

View workflow job for this annotation

GitHub Actions / typos

"ceate" should be "create".
let setup1 = Setup::new(rng, 6);
let setup2 = Setup::new(rng, 6);
let genesis3 = Genesis {
Expand All @@ -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!(
Expand Down

0 comments on commit 5ff923d

Please sign in to comment.