From 7f881f59ece0af953cadc1058dd6da1f9693d6a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Fran=C3=A7a?= Date: Tue, 22 Oct 2024 01:30:16 +0100 Subject: [PATCH] Changed LeaderProposal to not contain BlockHeader. It matches the spec more closely and it greatly simplifies verification. --- .../roles/src/proto/validator/messages.proto | 5 +- node/libs/roles/src/validator/conv.rs | 2 - .../roles/src/validator/messages/block.rs | 10 +++ .../roles/src/validator/messages/committee.rs | 2 +- .../roles/src/validator/messages/consensus.rs | 3 +- .../src/validator/messages/leader_proposal.rs | 78 +------------------ .../messages/tests/leader_proposal.rs | 68 ---------------- .../roles/src/validator/messages/tests/mod.rs | 1 - .../src/validator/messages/tests/versions.rs | 4 +- node/libs/roles/src/validator/testonly.rs | 1 - 10 files changed, 20 insertions(+), 154 deletions(-) diff --git a/node/libs/roles/src/proto/validator/messages.proto b/node/libs/roles/src/proto/validator/messages.proto index 2b7038c5..209c4889 100644 --- a/node/libs/roles/src/proto/validator/messages.proto +++ b/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 { diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index 3dade788..5addcab2 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -290,7 +290,6 @@ impl ProtoFmt for LeaderProposal { fn read(r: &Self::Proto) -> anyhow::Result { 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")?, }) @@ -298,7 +297,6 @@ impl ProtoFmt for LeaderProposal { 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()), } diff --git a/node/libs/roles/src/validator/messages/block.rs b/node/libs/roles/src/validator/messages/block.rs index 7db2628b..2c0f74af 100644 --- a/node/libs/roles/src/validator/messages/block.rs +++ b/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. diff --git a/node/libs/roles/src/validator/messages/committee.rs b/node/libs/roles/src/validator/messages/committee.rs index 018dbb4f..1241c540 100644 --- a/node/libs/roles/src/validator/messages/committee.rs +++ b/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. diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index 1b723714..1ff67b4d 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/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)) } diff --git a/node/libs/roles/src/validator/messages/leader_proposal.rs b/node/libs/roles/src/validator/messages/leader_proposal.rs index c421e054..b0ea5faa 100644 --- a/node/libs/roles/src/validator/messages/leader_proposal.rs +++ b/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, @@ -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. diff --git a/node/libs/roles/src/validator/messages/tests/leader_proposal.rs b/node/libs/roles/src/validator/messages/tests/leader_proposal.rs index 11b66e63..54b99946 100644 --- a/node/libs/roles/src/validator/messages/tests/leader_proposal.rs +++ b/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, }; diff --git a/node/libs/roles/src/validator/messages/tests/mod.rs b/node/libs/roles/src/validator/messages/tests/mod.rs index b8b703b7..798f74ed 100644 --- a/node/libs/roles/src/validator/messages/tests/mod.rs +++ b/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()), } diff --git a/node/libs/roles/src/validator/messages/tests/versions.rs b/node/libs/roles/src/validator/messages/tests/versions.rs index 3832c523..a19f152a 100644 --- a/node/libs/roles/src/validator/messages/tests/versions.rs +++ b/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", ); } } diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index eff17eda..b4d10bcb 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -523,7 +523,6 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> LeaderProposal { LeaderProposal { - proposal: rng.gen(), proposal_payload: rng.gen(), justification: rng.gen(), }