From 6ad24205b09d04d5e6df1ae841732828cea4ffd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Fran=C3=A7a?= Date: Wed, 16 Oct 2024 02:21:20 +0100 Subject: [PATCH] Nits. --- .../src/validator/messages/leader_proposal.rs | 33 +++--- .../src/validator/messages/replica_commit.rs | 88 ++++++++------ .../validator/messages/replica_new_view.rs | 2 +- .../src/validator/messages/replica_timeout.rs | 108 +++++++++++------- node/libs/roles/src/validator/tests.rs | 6 +- 5 files changed, 145 insertions(+), 92 deletions(-) diff --git a/node/libs/roles/src/validator/messages/leader_proposal.rs b/node/libs/roles/src/validator/messages/leader_proposal.rs index 613c6f69..c421e054 100644 --- a/node/libs/roles/src/validator/messages/leader_proposal.rs +++ b/node/libs/roles/src/validator/messages/leader_proposal.rs @@ -3,7 +3,7 @@ use super::{ TimeoutQC, TimeoutQCVerifyError, View, }; -/// A Proposal message from the leader. +/// A proposal message from the leader. #[derive(Clone, Debug, PartialEq, Eq)] pub struct LeaderProposal { /// The header of the block that the leader is proposing. @@ -56,7 +56,7 @@ impl LeaderProposal { return Err(LeaderProposalVerifyError::NewProposalWhenPreviousNotFinalized); } - // Check that the payload matches the header. + // Check that the payload matches the header, if it exists. if let Some(payload) = &self.proposal_payload { if payload.hash() != self.proposal.payload { return Err(LeaderProposalVerifyError::MismatchedPayload { @@ -74,10 +74,10 @@ impl LeaderProposal { #[derive(thiserror::Error, Debug)] pub enum LeaderProposalVerifyError { /// Invalid Justification. - #[error("justification: {0:#}")] + #[error("Invalid justification: {0:#}")] Justification(ProposalJustificationVerifyError), /// Bad block number. - #[error("bad block number: got {got:?}, want {want:?}")] + #[error("Bad block number: got {got:?}, want {want:?}")] BadBlockNumber { /// Received proposal number. got: BlockNumber, @@ -85,7 +85,7 @@ pub enum LeaderProposalVerifyError { want: BlockNumber, }, /// Bad payload hash on reproposal. - #[error("bad payload hash on reproposal: got {got:?}, want {want:?}")] + #[error("Bad payload hash on reproposal: got {got:?}, want {want:?}")] BadPayloadHash { /// Received payload hash. got: PayloadHash, @@ -93,13 +93,13 @@ pub enum LeaderProposalVerifyError { want: PayloadHash, }, /// New block proposal when the previous proposal was not finalized. - #[error("new block proposal when the previous proposal was not finalized")] + #[error("New block proposal when the previous proposal was not finalized")] NewProposalWhenPreviousNotFinalized, /// Re-proposal when the previous proposal was finalized. - #[error("block re-proposal when the previous proposal was finalized")] + #[error("Block re-proposal when the previous proposal was finalized")] ReproposalWhenPreviousFinalized, /// Mismatched payload. - #[error("block proposal with mismatched payload: header {header:?}, payload {payload:?}")] + #[error("Block proposal with mismatched payload: header {header:?}, payload {payload:?}")] MismatchedPayload { /// Payload hash on block header. header: PayloadHash, @@ -166,8 +166,13 @@ impl ProposalJustification { // Get the high commit QC of the timeout QC. We compare the high QC field of // all timeout votes in the QC, and get the highest one, if it exists. + // The high QC always exists, unless no block has been finalized yet in the chain. let high_qc = qc.high_qc(); + // If there was a high vote in the timeout QC, and either there was no high QC + // in the timeout QC, or the high vote is for a higher block than the high QC, + // then we need to repropose the high vote. + #[allow(clippy::unnecessary_unwrap)] // using a match would be more verbose if high_vote.is_some() && (high_qc.is_none() || high_vote.unwrap().number > high_qc.unwrap().header().number) @@ -177,8 +182,8 @@ impl ProposalJustification { (high_vote.unwrap().number, Some(high_vote.unwrap().payload)) } else { // Either the previous proposal was finalized or we know for certain - // that it couldn't have been finalized. Either way, we can propose - // a new block. + // that it couldn't have been finalized (because there is no high vote). + // Either way, we can propose a new block. let block_number = match high_qc { Some(qc) => qc.header().number.next(), None => BlockNumber(0), @@ -194,10 +199,10 @@ impl ProposalJustification { /// Error returned by `ProposalJustification::verify()`. #[derive(thiserror::Error, Debug)] pub enum ProposalJustificationVerifyError { - /// Invalid Timeout QC. - #[error("timeout qc: {0:#}")] + /// Invalid timeout QC. + #[error("Invalid timeout QC: {0:#}")] Timeout(TimeoutQCVerifyError), - /// Invalid Commit QC. - #[error("commit qc: {0:#}")] + /// Invalid commit QC. + #[error("Invalid commit QC: {0:#}")] Commit(CommitQCVerifyError), } diff --git a/node/libs/roles/src/validator/messages/replica_commit.rs b/node/libs/roles/src/validator/messages/replica_commit.rs index 1c645959..6d4b98f4 100644 --- a/node/libs/roles/src/validator/messages/replica_commit.rs +++ b/node/libs/roles/src/validator/messages/replica_commit.rs @@ -1,7 +1,7 @@ use super::{BlockHeader, Genesis, Signed, Signers, View}; use crate::validator; -/// A Commit message from a replica. +/// A commit message from a replica. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ReplicaCommit { /// View of this message. @@ -36,11 +36,11 @@ pub enum ReplicaCommitVerifyError { BadBlockNumber, } -/// A Commit Quorum Certificate. It is an aggregate of signed replica Commit messages. -/// The Quorum Certificate is supposed to be over identical messages, so we only need one message. +/// A Commit Quorum Certificate. It is an aggregate of signed ReplicaCommit messages. +/// The Commit Quorum Certificate is over identical messages, so we only need one message. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct CommitQC { - /// The replica Commit message that the QC is for. + /// The ReplicaCommit message that the QC is for. pub message: ReplicaCommit, /// The validators that signed this message. pub signers: Signers, @@ -68,39 +68,54 @@ impl CommitQC { } } - /// Add a validator's signature. - /// Signature is assumed to be already verified. + /// Add a validator's signature. This also verifies the message and the signature before adding. pub fn add( &mut self, msg: &Signed, genesis: &Genesis, ) -> Result<(), CommitQCAddError> { - if self.message != msg.msg { - return Err(CommitQCAddError::InconsistentMessages); - }; - + // Check if the signer is in the committee. let Some(i) = genesis.validators.index(&msg.key) else { return Err(CommitQCAddError::SignerNotInCommittee { signer: Box::new(msg.key.clone()), }); }; + // Check if already have a message from the same signer. if self.signers.0[i] { - return Err(CommitQCAddError::Exists); + return Err(CommitQCAddError::DuplicateSigner { + signer: Box::new(msg.key.clone()), + }); }; + // Verify the signature. + msg.verify().map_err(CommitQCAddError::BadSignature)?; + + // Check that the message is consistent with the CommitQC. + if self.message != msg.msg { + return Err(CommitQCAddError::InconsistentMessages); + }; + + // Check that the message itself is valid. + msg.msg + .verify(genesis) + .map_err(CommitQCAddError::InvalidMessage)?; + + // Add the signer to the signers map, and the signature to the aggregate signature. self.signers.0.set(i, true); self.signature.add(&msg.sig); Ok(()) } - /// Verifies the signature of the CommitQC. + /// Verifies the integrity of the CommitQC. pub fn verify(&self, genesis: &Genesis) -> Result<(), CommitQCVerifyError> { + // Check that the message is valid. self.message .verify(genesis) .map_err(CommitQCVerifyError::InvalidMessage)?; + // Check that the signers set has the same size as the validator set. if self.signers.len() != genesis.validators.len() { return Err(CommitQCVerifyError::BadSignersSet); } @@ -129,14 +144,40 @@ impl CommitQC { } } -/// Error returned by `CommitQc::verify()`. +/// Error returned by `CommitQC::add()`. +#[derive(thiserror::Error, Debug)] +pub enum CommitQCAddError { + /// Signer not present in the committee. + #[error("Signer not in committee: {signer:?}")] + SignerNotInCommittee { + /// Signer of the message. + signer: Box, + }, + /// Message from the same signer already present in QC. + #[error("Message from the same signer already in QC: {signer:?}")] + DuplicateSigner { + /// Signer of the message. + signer: Box, + }, + /// Bad signature. + #[error("Bad signature: {0:#}")] + BadSignature(#[source] anyhow::Error), + /// Inconsistent messages. + #[error("Trying to add signature for a different message")] + InconsistentMessages, + /// Invalid message. + #[error("Invalid message: {0:#}")] + InvalidMessage(ReplicaCommitVerifyError), +} + +/// Error returned by `CommitQC::verify()`. #[derive(thiserror::Error, Debug)] pub enum CommitQCVerifyError { /// Invalid message. #[error(transparent)] InvalidMessage(#[from] ReplicaCommitVerifyError), /// Bad signer set. - #[error("signers set doesn't match genesis")] + #[error("Signers set doesn't match validator set")] BadSignersSet, /// Weight not reached. #[error("Signers have not reached threshold weight: got {got}, want {want}")] @@ -147,23 +188,6 @@ pub enum CommitQCVerifyError { want: u64, }, /// Bad signature. - #[error("bad signature: {0:#}")] + #[error("Bad signature: {0:#}")] BadSignature(#[source] anyhow::Error), } - -/// Error returned by `CommitQC::add()`. -#[derive(thiserror::Error, Debug)] -pub enum CommitQCAddError { - /// Inconsistent messages. - #[error("Trying to add signature for a different message")] - InconsistentMessages, - /// Signer not present in the committee. - #[error("Signer not in committee: {signer:?}")] - SignerNotInCommittee { - /// Signer of the message. - signer: Box, - }, - /// Message already present in CommitQC. - #[error("Message already signed for CommitQC")] - Exists, -} diff --git a/node/libs/roles/src/validator/messages/replica_new_view.rs b/node/libs/roles/src/validator/messages/replica_new_view.rs index e127f02b..79b93e25 100644 --- a/node/libs/roles/src/validator/messages/replica_new_view.rs +++ b/node/libs/roles/src/validator/messages/replica_new_view.rs @@ -1,6 +1,6 @@ use super::{Genesis, ProposalJustification, ProposalJustificationVerifyError, View}; -/// A NewView message from a replica. +/// A new view message from a replica. #[derive(Clone, Debug, PartialEq, Eq)] pub struct ReplicaNewView { /// What attests to the validity of this view change. diff --git a/node/libs/roles/src/validator/messages/replica_timeout.rs b/node/libs/roles/src/validator/messages/replica_timeout.rs index 0c81f740..02f4b448 100644 --- a/node/libs/roles/src/validator/messages/replica_timeout.rs +++ b/node/libs/roles/src/validator/messages/replica_timeout.rs @@ -5,7 +5,7 @@ use super::{ use crate::validator; use std::collections::{BTreeMap, HashMap}; -/// A Timeout message from a replica. +/// A timeout message from a replica. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct ReplicaTimeout { /// View of this message. @@ -63,21 +63,21 @@ pub enum ReplicaTimeoutVerifyError { HighQC(CommitQCVerifyError), } -/// A quorum certificate of replica Timeout messages. Since not all Timeout messages are -/// identical (they have different high blocks and high QCs), we need to keep the high blocks -/// and high QCs in a map. We can still aggregate the signatures though. +/// A quorum certificate of ReplicaTimeout messages. Since not all ReplicaTimeout messages are +/// identical (they have different high blocks and high QCs), we need to keep the ReplicaTimeout +/// messages in a map. We can still aggregate the signatures though. #[derive(Clone, Debug, PartialEq, Eq)] pub struct TimeoutQC { /// View of this QC. pub view: View, /// Map from replica Timeout messages to the validators that signed them. pub map: BTreeMap, - /// Aggregate signature of the replica Timeout messages. + /// Aggregate signature of the ReplicaTimeout messages. pub signature: validator::AggregateSignature, } impl TimeoutQC { - /// Create a new empty instance for a given `ReplicaCommit` message and a validator set size. + /// Create a new empty TimeoutQC for a given view. pub fn new(view: View) -> Self { Self { view, @@ -86,11 +86,10 @@ impl TimeoutQC { } } - /// 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. - /// Note that it is possible to have 2 quorums: vote A and vote B, each with >2f weight, in a single - /// TimeoutQC (even in the unweighted case, because QC contains n-f signatures, not 4f+1). In such a - /// situation we say that there is no high vote. + /// Get the highest block voted and check if there's a subquorum of votes for it. To have a subquorum + /// in this situation, we require n-3*f votes, where f is the maximum number of faulty replicas. + /// Note that it is possible to have 2 subquorums: vote A and vote B, each with >n-3*f weight, in a single + /// TimeoutQC. In such a situation we say that there is no high vote. pub fn high_vote(&self, genesis: &Genesis) -> Option { let mut count: HashMap<_, u64> = HashMap::new(); for (msg, signers) in &self.map { @@ -99,7 +98,7 @@ impl TimeoutQC { } } - let min = 2 * genesis.validators.max_faulty_weight() + 1; + let min = genesis.validators.subquorum_threshold(); let mut high_votes: Vec<_> = count.into_iter().filter(|x| x.1 >= min).collect(); if high_votes.len() == 1 { @@ -117,28 +116,40 @@ impl TimeoutQC { .max_by_key(|qc| qc.view().number) } - /// Add a validator's signed message. - /// Message is assumed to be already verified. - // TODO: verify the message inside instead. + /// Add a validator's signed message. This also verifies the message and the signature before adding. pub fn add( &mut self, msg: &Signed, genesis: &Genesis, ) -> Result<(), TimeoutQCAddError> { - if msg.msg.view != self.view { - return Err(TimeoutQCAddError::InconsistentViews); - } - + // Check if the signer is in the committee. let Some(i) = genesis.validators.index(&msg.key) else { return Err(TimeoutQCAddError::SignerNotInCommittee { signer: Box::new(msg.key.clone()), }); }; + // Check if already have a message from the same signer. if self.map.values().any(|s| s.0[i]) { - return Err(TimeoutQCAddError::Exists); + return Err(TimeoutQCAddError::DuplicateSigner { + signer: Box::new(msg.key.clone()), + }); }; + // Verify the signature. + msg.verify().map_err(TimeoutQCAddError::BadSignature)?; + + // Check that the view is consistent with the TimeoutQC. + if msg.msg.view != self.view { + return Err(TimeoutQCAddError::InconsistentViews); + }; + + // Check that the message itself is valid. + msg.msg + .verify(genesis) + .map_err(TimeoutQCAddError::InvalidMessage)?; + + // Add the message plus signer to the map, and the signature to the aggregate signature. let e = self .map .entry(msg.msg.clone()) @@ -154,6 +165,7 @@ impl TimeoutQC { self.view .verify(genesis) .map_err(TimeoutQCVerifyError::View)?; + let mut sum = Signers::new(genesis.validators.len()); // Check the ReplicaTimeout messages. @@ -178,10 +190,11 @@ impl TimeoutQC { } msg.verify(genesis) .map_err(|err| TimeoutQCVerifyError::InvalidMessage(i, err))?; + sum |= signers; } - // Verify the signers' weight is enough. + // Check if the signers' weight is enough. let weight = genesis.validators.weight(&sum); let threshold = genesis.validators.quorum_threshold(); if weight < threshold { @@ -190,6 +203,7 @@ impl TimeoutQC { want: threshold, }); } + // Now we can verify the signature. let messages_and_keys = self.map.clone().into_iter().flat_map(|(msg, signers)| { genesis @@ -200,6 +214,7 @@ impl TimeoutQC { .map(|(_, pk)| (msg.clone(), pk)) .collect::>() }); + // TODO(gprusak): This reaggregating is suboptimal. self.signature .verify_messages(messages_and_keys) @@ -215,17 +230,43 @@ impl TimeoutQC { } } +/// Error returned by `TimeoutQC::add()`. +#[derive(thiserror::Error, Debug)] +pub enum TimeoutQCAddError { + /// Signer not present in the committee. + #[error("Signer not in committee: {signer:?}")] + SignerNotInCommittee { + /// Signer of the message. + signer: Box, + }, + /// Message from the same signer already present in QC. + #[error("Message from the same signer already in QC: {signer:?}")] + DuplicateSigner { + /// Signer of the message. + signer: Box, + }, + /// Bad signature. + #[error("Bad signature: {0:#}")] + BadSignature(#[source] anyhow::Error), + /// Inconsistent views. + #[error("Trying to add a message from a different view")] + InconsistentViews, + /// Invalid message. + #[error("Invalid message: {0:#}")] + InvalidMessage(ReplicaTimeoutVerifyError), +} + /// Error returned by `TimeoutQC::verify()`. #[derive(thiserror::Error, Debug)] pub enum TimeoutQCVerifyError { /// Bad view. - #[error("view: {0:#}")] + #[error("Bad view: {0:#}")] View(anyhow::Error), /// Inconsistent views. - #[error("inconsistent views of signed messages")] + #[error("Inconsistent views of signed messages")] InconsistentViews, /// Invalid message. - #[error("msg[{0}]: {1:#}")] + #[error("Invalid message: number [{0}], {1:#}")] InvalidMessage(usize, ReplicaTimeoutVerifyError), /// Bad message format. #[error(transparent)] @@ -239,23 +280,6 @@ pub enum TimeoutQCVerifyError { want: u64, }, /// Bad signature. - #[error("bad signature: {0:#}")] + #[error("Bad signature: {0:#}")] BadSignature(#[source] anyhow::Error), } - -/// Error returned by `TimeoutQC::add()`. -#[derive(thiserror::Error, Debug)] -pub enum TimeoutQCAddError { - /// Inconsistent views. - #[error("Trying to add a message from a different view")] - InconsistentViews, - /// Signer not present in the committee. - #[error("Signer not in committee: {signer:?}")] - SignerNotInCommittee { - /// Signer of the message. - signer: Box, - }, - /// Message already present in TimeoutQC. - #[error("Message already signed for TimeoutQC")] - Exists, -} diff --git a/node/libs/roles/src/validator/tests.rs b/node/libs/roles/src/validator/tests.rs index 6bba151b..f2616d49 100644 --- a/node/libs/roles/src/validator/tests.rs +++ b/node/libs/roles/src/validator/tests.rs @@ -295,7 +295,7 @@ fn test_commit_qc_add_errors() { &setup.validator_keys[0].sign_msg(msg.clone()), &setup.genesis ), - Err(Error::Exists { .. }) + Err(Error::DuplicateSigner { .. }) ); // Add same message signed by another validator. @@ -457,14 +457,14 @@ fn test_prepare_qc_add_errors() { &setup.validator_keys[0].sign_msg(msg.clone()), &setup.genesis ), - Err(Error::Exists { .. }) + Err(Error::DuplicateSigner { .. }) ); // Try to add a message for a validator that already added another message let msg2 = make_replica_timeout(rng, view, &setup); assert_matches!( qc.add(&setup.validator_keys[0].sign_msg(msg2), &setup.genesis), - Err(Error::Exists { .. }) + Err(Error::DuplicateSigner { .. }) ); // Add same message signed by another validator.