Changed LeaderProposal to not contain BlockHeader. It matches the spe…

…c more closely and it greatly simplifies verification.

node/libs/roles/src/proto/validator/messages.proto

@@ -72,9 +72,8 @@ message ReplicaNewView {
 }
 
 message LeaderProposal {
-  optional BlockHeader proposal = 1; // required
-  optional bytes proposal_payload = 2; // optional (depending on justification)
-  optional ProposalJustification justification = 3; // required
+  optional bytes proposal_payload = 1; // optional (depending on justification)
+  optional ProposalJustification justification = 2; // required
 }
 
 message CommitQC {

node/libs/roles/src/validator/conv.rs

@@ -290,15 +290,13 @@ impl ProtoFmt for LeaderProposal {
 
     fn read(r: &Self::Proto) -> anyhow::Result<Self> {
         Ok(Self {
-            proposal: read_required(&r.proposal).context("proposal")?,
             proposal_payload: r.proposal_payload.as_ref().map(|p| Payload(p.clone())),
             justification: read_required(&r.justification).context("justification")?,
         })
     }
 
     fn build(&self) -> Self::Proto {
         Self::Proto {
-            proposal: Some(self.proposal.build()),
             proposal_payload: self.proposal_payload.as_ref().map(|p| p.0.clone()),
             justification: Some(self.justification.build()),
         }

node/libs/roles/src/validator/messages/block.rs

@@ -26,6 +26,16 @@ impl Payload {
     pub fn hash(&self) -> PayloadHash {
         PayloadHash(Keccak256::new(&self.0))
     }
+
+    /// Returns the length of the payload.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the payload is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
 }
 
 /// Hash of the Payload.

node/libs/roles/src/validator/messages/committee.rs

@@ -161,7 +161,7 @@ pub struct WeightedValidator {
     pub weight: Weight,
 }
 
-/// Voting weight;
+/// Voting weight.
 pub type Weight = u64;
 
 /// The mode used for selecting leader for a given view.

node/libs/roles/src/validator/messages/consensus.rs

@@ -152,7 +152,8 @@ impl fmt::Display for ViewNumber {
 pub struct Signers(pub BitVec);
 
 impl Signers {
-    /// Constructs an empty signers set.
+    /// Constructs a new Signers bitmap with the given number of validators. All
+    /// bits are set to false.
     pub fn new(n: usize) -> Self {
         Self(BitVec::from_elem(n, false))
     }

node/libs/roles/src/validator/messages/leader_proposal.rs

@@ -1,13 +1,11 @@
 use super::{
-    BlockHeader, BlockNumber, CommitQC, CommitQCVerifyError, Genesis, Payload, PayloadHash,
-    TimeoutQC, TimeoutQCVerifyError, View,
+    BlockNumber, CommitQC, CommitQCVerifyError, Genesis, Payload, PayloadHash, TimeoutQC,
+    TimeoutQCVerifyError, View,
 };
 
 /// A proposal message from the leader.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct LeaderProposal {
-    /// The header of the block that the leader is proposing.
-    pub proposal: BlockHeader,
     /// Payload of the block that the leader is proposing.
     /// `None` iff this is a reproposal.
     pub proposal_payload: Option<Payload>,
@@ -26,47 +24,7 @@ impl LeaderProposal {
         // Check that the justification is valid.
         self.justification
             .verify(genesis)
-            .map_err(LeaderProposalVerifyError::Justification)?;
-
-        // Get the implied block number and payload hash and check it against the proposal.
-        let (implied_block_number, implied_payload) = self.justification.get_implied_block(genesis);
-
-        if self.proposal.number != implied_block_number {
-            return Err(LeaderProposalVerifyError::BadBlockNumber {
-                got: self.proposal.number,
-                want: implied_block_number,
-            });
-        }
-
-        if let Some(payload_hash) = implied_payload {
-            if self.proposal.payload != payload_hash {
-                return Err(LeaderProposalVerifyError::BadPayloadHash {
-                    got: self.proposal.payload,
-                    want: payload_hash,
-                });
-            }
-        }
-
-        // Check if we are correctly proposing a new block or re-proposing an old one.
-        if implied_payload.is_none() && self.proposal_payload.is_none() {
-            return Err(LeaderProposalVerifyError::ReproposalWhenPreviousFinalized);
-        }
-
-        if implied_payload.is_some() && self.proposal_payload.is_some() {
-            return Err(LeaderProposalVerifyError::NewProposalWhenPreviousNotFinalized);
-        }
-
-        // 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 {
-                    header: self.proposal.payload,
-                    payload: payload.hash(),
-                });
-            }
-        }
-
-        Ok(())
+            .map_err(LeaderProposalVerifyError::Justification)
     }
 }
 
@@ -76,36 +34,6 @@ pub enum LeaderProposalVerifyError {
     /// Invalid Justification.
     #[error("Invalid justification: {0:#}")]
     Justification(ProposalJustificationVerifyError),
-    /// Bad block number.
-    #[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:?}")]
-    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")]
-    NewProposalWhenPreviousNotFinalized,
-    /// 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:?}")]
-    MismatchedPayload {
-        /// Payload hash on block header.
-        header: PayloadHash,
-        /// Correct payload hash.
-        payload: PayloadHash,
-    },
 }
 
 /// Justification for a proposal. This is either a Commit QC or a Timeout QC.

node/libs/roles/src/validator/messages/tests/leader_proposal.rs

@@ -13,17 +13,12 @@ fn test_leader_proposal_verify() {
 
     // Valid proposal
     let payload: Payload = rng.gen();
-    let block_header = BlockHeader {
-        number: setup.next(),
-        payload: payload.hash(),
-    };
     let commit_qc = match setup.blocks.last().unwrap() {
         Block::Final(block) => block.justification.clone(),
         _ => unreachable!(),
     };
     let justification = ProposalJustification::Commit(commit_qc);
     let proposal = LeaderProposal {
-        proposal: block_header,
         proposal_payload: Some(payload.clone()),
         justification,
     };
@@ -38,62 +33,6 @@ fn test_leader_proposal_verify() {
         wrong_proposal.verify(&setup.genesis),
         Err(LeaderProposalVerifyError::Justification(_))
     );
-
-    // Invalid block number
-    let mut wrong_proposal = proposal.clone();
-    wrong_proposal.proposal.number = BlockNumber(1);
-
-    assert_matches!(
-        wrong_proposal.verify(&setup.genesis),
-        Err(LeaderProposalVerifyError::BadBlockNumber { .. })
-    );
-
-    // Wrong reproposal
-    let mut wrong_proposal = proposal.clone();
-    wrong_proposal.proposal_payload = None;
-
-    assert_matches!(
-        wrong_proposal.verify(&setup.genesis),
-        Err(LeaderProposalVerifyError::ReproposalWhenPreviousFinalized)
-    );
-
-    // Invalid payload
-    let mut wrong_proposal = proposal.clone();
-    wrong_proposal.proposal.payload = rng.gen();
-
-    assert_matches!(
-        wrong_proposal.verify(&setup.genesis),
-        Err(LeaderProposalVerifyError::MismatchedPayload { .. })
-    );
-
-    // New leader proposal with a reproposal
-    let timeout_qc = setup.make_timeout_qc(rng, ViewNumber(7), Some(&payload));
-    let justification = ProposalJustification::Timeout(timeout_qc);
-    let proposal = LeaderProposal {
-        proposal: block_header,
-        proposal_payload: None,
-        justification,
-    };
-
-    assert!(proposal.verify(&setup.genesis).is_ok());
-
-    // Invalid payload hash
-    let mut wrong_proposal = proposal.clone();
-    wrong_proposal.proposal.payload = rng.gen();
-
-    assert_matches!(
-        wrong_proposal.verify(&setup.genesis),
-        Err(LeaderProposalVerifyError::BadPayloadHash { .. })
-    );
-
-    // Wrong new proposal
-    let mut wrong_proposal = proposal.clone();
-    wrong_proposal.proposal_payload = Some(rng.gen());
-
-    assert_matches!(
-        wrong_proposal.verify(&setup.genesis),
-        Err(LeaderProposalVerifyError::NewProposalWhenPreviousNotFinalized)
-    );
 }
 
 #[test]
@@ -102,12 +41,7 @@ fn test_justification_get_implied_block() {
     let rng = &mut ctx.rng();
     let mut setup = Setup::new(rng, 6);
     setup.push_blocks(rng, 3);
-
     let payload: Payload = rng.gen();
-    let block_header = BlockHeader {
-        number: setup.next(),
-        payload: payload.hash(),
-    };
 
     // Justification with a commit QC
     let commit_qc = match setup.blocks.last().unwrap() {
@@ -116,7 +50,6 @@ fn test_justification_get_implied_block() {
     };
     let justification = ProposalJustification::Commit(commit_qc);
     let proposal = LeaderProposal {
-        proposal: block_header,
         proposal_payload: Some(payload.clone()),
         justification,
     };
@@ -131,7 +64,6 @@ fn test_justification_get_implied_block() {
     let timeout_qc = setup.make_timeout_qc(rng, ViewNumber(7), Some(&payload));
     let justification = ProposalJustification::Timeout(timeout_qc);
     let proposal = LeaderProposal {
-        proposal: block_header,
         proposal_payload: None,
         justification,
     };

node/libs/roles/src/validator/messages/tests/mod.rs

@@ -131,7 +131,6 @@ fn genesis_with_attesters() -> Genesis {
 /// Hardcoded `LeaderProposal`.
 fn leader_proposal() -> LeaderProposal {
     LeaderProposal {
-        proposal: block_header(),
         proposal_payload: Some(payload()),
         justification: ProposalJustification::Timeout(timeout_qc()),
     }

node/libs/roles/src/validator/messages/tests/versions.rs

@@ -95,8 +95,8 @@ mod version1 {
     fn leader_proposal_change_detector() {
         msg_change_detector(
             leader_proposal().insert(),
-            "validator_msg:keccak256:7b079e4ca3021834fa35745cb042fea6dd5bb89a91ca5ba31ed6ba1765a1e113",
-            "validator:signature:bls12_381:98ca0f24d87f938b22ac9c2a2720466cd157a502b31ae5627ce5fdbda6de0ad6d2e9b159cf816cd1583644f2f69ecb84",
+            "validator_msg:keccak256:4c1b2cf1e8fbb00cde86caee200491df15c45d5c88402e227c1f3e1b416c4255",
+            "validator:signature:bls12_381:81f865807067c6f70f17f9716e6d41c0103c2366abb6721408fb7d27ead6332798bd7b34d5f4a63e324082586b2c69a3",
         );
     }
 }

node/libs/roles/src/validator/testonly.rs

@@ -523,7 +523,6 @@ impl Distribution<ReplicaNewView> for Standard {
 impl Distribution<LeaderProposal> for Standard {
     fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> LeaderProposal {
         LeaderProposal {
-            proposal: rng.gen(),
             proposal_payload: rng.gen(),
             justification: rng.gen(),
         }