Nits.

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,32 +74,32 @@ 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,
         /// Correct proposal number.
         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,
         /// Correct payload hash.
         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),
 }

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<ReplicaCommit>,
         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<validator::PublicKey>,
+    },
+    /// 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<validator::PublicKey>,
+    },
+    /// 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<validator::PublicKey>,
-    },
-    /// Message already present in CommitQC.
-    #[error("Message already signed for CommitQC")]
-    Exists,
-}

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.