From 04ccbad9ff72136811e4a3bb6fc186c247ad72fc Mon Sep 17 00:00:00 2001 From: Satya Vusirikala Date: Sat, 19 Oct 2024 17:09:50 -0700 Subject: [PATCH 1/3] Refactoring ledger info with unverified signatures --- consensus/consensus-types/src/order_vote.rs | 7 - consensus/consensus-types/src/vote.rs | 7 - .../src/block_storage/block_store_test.rs | 2 - consensus/src/pending_order_votes.rs | 33 +-- consensus/src/pending_votes.rs | 40 ++-- consensus/src/pipeline/buffer_item.rs | 49 ++-- consensus/src/pipeline/buffer_manager.rs | 2 +- consensus/src/round_manager_test.rs | 17 +- types/src/ledger_info.rs | 209 +++++------------- 9 files changed, 129 insertions(+), 237 deletions(-) diff --git a/consensus/consensus-types/src/order_vote.rs b/consensus/consensus-types/src/order_vote.rs index b487f9a1668d8..ecf8dbf4901de 100644 --- a/consensus/consensus-types/src/order_vote.rs +++ b/consensus/consensus-types/src/order_vote.rs @@ -76,13 +76,6 @@ impl OrderVote { self.signature.is_verified() } - /// Only the verify method in validator verifier can set the signature status verified. - /// This method additionally lets the tests to set the status to verified. - #[cfg(any(test, feature = "fuzzing"))] - pub fn set_verified(&self) { - self.signature.set_verified(); - } - pub fn epoch(&self) -> u64 { self.ledger_info.epoch() } diff --git a/consensus/consensus-types/src/vote.rs b/consensus/consensus-types/src/vote.rs index 20400be9a92c8..1370fc5771381 100644 --- a/consensus/consensus-types/src/vote.rs +++ b/consensus/consensus-types/src/vote.rs @@ -122,13 +122,6 @@ impl Vote { self.signature.is_verified() } - /// Only the verify method in validator verifier can set the signature status verified. - /// This method additionally lets the tests to set the status to verified. - #[cfg(any(test, feature = "fuzzing"))] - pub fn set_verified(&self) { - self.signature.set_verified(); - } - /// Returns the 2-chain timeout. pub fn generate_2chain_timeout(&self, qc: QuorumCert) -> TwoChainTimeout { TwoChainTimeout::new( diff --git a/consensus/src/block_storage/block_store_test.rs b/consensus/src/block_storage/block_store_test.rs index 09513e648bca5..41def8f1c322d 100644 --- a/consensus/src/block_storage/block_store_test.rs +++ b/consensus/src/block_storage/block_store_test.rs @@ -300,7 +300,6 @@ async fn test_insert_vote() { voter, ) .unwrap(); - vote.set_verified(); let vote_res = pending_votes.insert_vote(&vote, &validator_verifier); // first vote of an author is accepted @@ -330,7 +329,6 @@ async fn test_insert_vote() { final_voter, ) .unwrap(); - vote.set_verified(); match pending_votes.insert_vote(&vote, &validator_verifier) { VoteReceptionResult::NewQuorumCertificate(qc) => { assert_eq!(qc.certified_block().id(), block.id()); diff --git a/consensus/src/pending_order_votes.rs b/consensus/src/pending_order_votes.rs index 58aaf8b90a852..397e3f0f314ab 100644 --- a/consensus/src/pending_order_votes.rs +++ b/consensus/src/pending_order_votes.rs @@ -7,7 +7,7 @@ use aptos_consensus_types::{common::Author, order_vote::OrderVote, quorum_cert:: use aptos_crypto::{hash::CryptoHash, HashValue}; use aptos_logger::prelude::*; use aptos_types::{ - ledger_info::{LedgerInfo, LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures}, + ledger_info::{LedgerInfo, LedgerInfoWithSignatures, SignatureAggregator}, validator_verifier::{ValidatorVerifier, VerifyError}, }; use std::{collections::HashMap, sync::Arc}; @@ -33,7 +33,7 @@ pub enum OrderVoteReceptionResult { #[derive(Debug, PartialEq, Eq)] enum OrderVoteStatus { EnoughVotes(LedgerInfoWithSignatures), - NotEnoughVotes(LedgerInfoWithUnverifiedSignatures), + NotEnoughVotes(SignatureAggregator), } /// A PendingVotes structure keep track of order votes for the last few rounds @@ -75,7 +75,7 @@ impl PendingOrderVotes { verified_quorum_cert.expect( "Quorum Cert is expected when creating a new entry in pending order votes", ), - OrderVoteStatus::NotEnoughVotes(LedgerInfoWithUnverifiedSignatures::new( + OrderVoteStatus::NotEnoughVotes(SignatureAggregator::new( order_vote.ledger_info().clone(), )), ) @@ -89,7 +89,7 @@ impl PendingOrderVotes { li_with_sig.clone(), )) }, - OrderVoteStatus::NotEnoughVotes(li_with_sig) => { + OrderVoteStatus::NotEnoughVotes(sig_aggregator) => { // we don't have enough votes for this ledger info yet let validator_voting_power = validator_verifier.get_voting_power(&order_vote.author()); @@ -109,8 +109,9 @@ impl PendingOrderVotes { order_vote.author() ); } - li_with_sig.add_signature(order_vote.author(), order_vote.signature_with_status()); - match li_with_sig.check_voting_power(validator_verifier, true) { + sig_aggregator + .add_signature(order_vote.author(), order_vote.signature_with_status()); + match sig_aggregator.check_voting_power(validator_verifier, true) { Ok(aggregated_voting_power) => { assert!( aggregated_voting_power >= validator_verifier.quorum_voting_power(), @@ -120,7 +121,11 @@ impl PendingOrderVotes { let _timer = counters::VERIFY_MSG .with_label_values(&["order_vote_aggregate_and_verify"]) .start_timer(); - li_with_sig.aggregate_and_verify(validator_verifier) + sig_aggregator.aggregate_and_verify(validator_verifier).map( + |(ledger_info, aggregated_sig)| { + LedgerInfoWithSignatures::new(ledger_info, aggregated_sig) + }, + ) }; match verification_result { Ok(ledger_info_with_sig) => { @@ -159,8 +164,8 @@ impl PendingOrderVotes { OrderVoteStatus::EnoughVotes(li_with_sig) => { li_with_sig.ledger_info().round() > highest_ordered_round }, - OrderVoteStatus::NotEnoughVotes(li_with_sig) => { - li_with_sig.ledger_info().round() > highest_ordered_round + OrderVoteStatus::NotEnoughVotes(sig_aggregator) => { + sig_aggregator.data().round() > highest_ordered_round }, }); } @@ -211,7 +216,6 @@ mod tests { ); // first time a new order vote is added -> OrderVoteAdded - order_vote_1_author_0.set_verified(); assert_eq!( pending_order_votes.insert_order_vote( &order_vote_1_author_0, @@ -234,7 +238,6 @@ mod tests { li2.clone(), signers[1].sign(&li2).expect("Unable to sign ledger info"), ); - order_vote_2_author_1.set_verified(); assert_eq!( pending_order_votes.insert_order_vote( &order_vote_2_author_1, @@ -252,7 +255,6 @@ mod tests { li2.clone(), signers[2].sign(&li2).expect("Unable to sign ledger info"), ); - order_vote_2_author_2.set_verified(); match pending_order_votes.insert_order_vote(&order_vote_2_author_2, &verifier, None) { OrderVoteReceptionResult::NewLedgerInfoWithSignatures((_qc, li_with_sig)) => { assert!(li_with_sig.check_voting_power(&verifier).is_ok()); @@ -319,7 +321,6 @@ mod tests { OrderVoteReceptionResult::VoteAdded(1) ); - vote_0.set_verified(); assert_eq!( pending_order_votes.insert_order_vote(&vote_0, &verifier, None), OrderVoteReceptionResult::VoteAdded(1) @@ -341,9 +342,9 @@ mod tests { .get(&li_hash) .unwrap(); match order_vote_status { - OrderVoteStatus::NotEnoughVotes(li_with_sig) => { - assert_eq!(li_with_sig.verified_voters().count(), 2); - assert_eq!(li_with_sig.unverified_voters().count(), 0); + OrderVoteStatus::NotEnoughVotes(sig_aggregator) => { + assert_eq!(sig_aggregator.verified_voters().count(), 2); + assert_eq!(sig_aggregator.unverified_voters().count(), 0); }, _ => { panic!("QC should not be formed yet."); diff --git a/consensus/src/pending_votes.rs b/consensus/src/pending_votes.rs index e6651681766b3..45d9bf47534c0 100644 --- a/consensus/src/pending_votes.rs +++ b/consensus/src/pending_votes.rs @@ -22,7 +22,7 @@ use aptos_consensus_types::{ use aptos_crypto::{bls12381, hash::CryptoHash, HashValue}; use aptos_logger::prelude::*; use aptos_types::{ - ledger_info::{LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures}, + ledger_info::{LedgerInfo, LedgerInfoWithSignatures, SignatureAggregator}, validator_verifier::{ValidatorVerifier, VerifyError}, }; use std::{collections::HashMap, fmt, sync::Arc}; @@ -59,7 +59,7 @@ pub enum VoteReceptionResult { #[derive(Debug, PartialEq, Eq)] pub enum VoteStatus { EnoughVotes(LedgerInfoWithSignatures), - NotEnoughVotes(LedgerInfoWithUnverifiedSignatures), + NotEnoughVotes(SignatureAggregator), } #[derive(Debug)] @@ -325,9 +325,7 @@ impl PendingVotes { let (hash_index, status) = self.li_digest_to_votes.entry(li_digest).or_insert_with(|| { ( len, - VoteStatus::NotEnoughVotes(LedgerInfoWithUnverifiedSignatures::new( - vote.ledger_info().clone(), - )), + VoteStatus::NotEnoughVotes(SignatureAggregator::new(vote.ledger_info().clone())), ) }); @@ -366,12 +364,12 @@ impl PendingVotes { li_with_sig.clone(), ))); }, - VoteStatus::NotEnoughVotes(li_with_sig) => { + VoteStatus::NotEnoughVotes(sig_aggregator) => { // add this vote to the ledger info with signatures - li_with_sig.add_signature(vote.author(), vote.signature_with_status()); + sig_aggregator.add_signature(vote.author(), vote.signature_with_status()); // check if we have enough signatures to create a QC - match li_with_sig.check_voting_power(validator_verifier, true) { + match sig_aggregator.check_voting_power(validator_verifier, true) { // a quorum of signature was reached, a new QC is formed Ok(aggregated_voting_power) => { assert!( @@ -383,7 +381,11 @@ impl PendingVotes { .with_label_values(&["vote_aggregate_and_verify"]) .start_timer(); - li_with_sig.aggregate_and_verify(validator_verifier) + sig_aggregator.aggregate_and_verify(validator_verifier).map( + |(ledger_info, aggregated_sig)| { + LedgerInfoWithSignatures::new(ledger_info, aggregated_sig) + }, + ) }; match verification_result { Ok(ledger_info_with_sig) => { @@ -527,13 +529,13 @@ impl fmt::Display for PendingVotes { VoteStatus::EnoughVotes(_li) => { write!(f, "LI {} has aggregated QC", li_digest)?; }, - VoteStatus::NotEnoughVotes(li) => { + VoteStatus::NotEnoughVotes(sig_aggregator) => { write!( f, "LI {} has {} verified votes, {} unverified votes", li_digest, - li.verified_voters().count(), - li.unverified_voters().count(), + sig_aggregator.verified_voters().count(), + sig_aggregator.unverified_voters().count(), )?; }, } @@ -599,7 +601,6 @@ mod tests { let vote_data_1_author_0 = Vote::new(vote_data_1, signers[0].author(), li1, &signers[0]).unwrap(); - vote_data_1_author_0.set_verified(); // first time a new vote is added -> VoteAdded assert_eq!( pending_votes.insert_vote(&vote_data_1_author_0, &validator_verifier), @@ -622,7 +623,6 @@ mod tests { &signers[0], ) .unwrap(); - vote_data_2_author_0.set_verified(); assert_eq!( pending_votes.insert_vote(&vote_data_2_author_0, &validator_verifier), VoteReceptionResult::EquivocateVote @@ -636,7 +636,6 @@ mod tests { &signers[1], ) .unwrap(); - vote_data_2_author_1.set_verified(); assert_eq!( pending_votes.insert_vote(&vote_data_2_author_1, &validator_verifier), VoteReceptionResult::VoteAdded(1) @@ -645,7 +644,6 @@ mod tests { // two votes for the ledger info -> NewQuorumCertificate let vote_data_2_author_2 = Vote::new(vote_data_2, signers[2].author(), li2, &signers[2]).unwrap(); - vote_data_2_author_2.set_verified(); match pending_votes.insert_vote(&vote_data_2_author_2, &validator_verifier) { VoteReceptionResult::NewQuorumCertificate(qc) => { assert!(qc @@ -722,7 +720,6 @@ mod tests { partial_sigs.add_signature(signers[0].author(), vote_0.signature().clone()); // same author voting for the same thing -> DuplicateVote - vote_0.set_verified(); assert_eq!( pending_votes.insert_vote(&vote_0, &validator_verifier), VoteReceptionResult::DuplicateVote @@ -744,9 +741,9 @@ mod tests { assert_eq!(validator_verifier.pessimistic_verify_set().len(), 1); let (_, vote_status) = pending_votes.li_digest_to_votes.get(&li_hash).unwrap(); match vote_status { - VoteStatus::NotEnoughVotes(li_with_sig) => { - assert_eq!(li_with_sig.verified_voters().count(), 2); - assert_eq!(li_with_sig.unverified_voters().count(), 0); + VoteStatus::NotEnoughVotes(sig_aggregator) => { + assert_eq!(sig_aggregator.verified_voters().count(), 2); + assert_eq!(sig_aggregator.unverified_voters().count(), 0); }, _ => { panic!("QC should not be formed yet."); @@ -805,7 +802,6 @@ mod tests { let vote0 = random_vote_data(); let mut vote0_author_0 = Vote::new(vote0, signers[0].author(), li0, &signers[0]).unwrap(); - vote0_author_0.set_verified(); assert_eq!( pending_votes.insert_vote(&vote0_author_0, &validator_verifier), VoteReceptionResult::VoteAdded(1) @@ -825,7 +821,6 @@ mod tests { let li1 = random_ledger_info(); let vote1 = random_vote_data(); let mut vote1_author_1 = Vote::new(vote1, signers[1].author(), li1, &signers[1]).unwrap(); - vote1_author_1.set_verified(); assert_eq!( pending_votes.insert_vote(&vote1_author_1, &validator_verifier), VoteReceptionResult::VoteAdded(1) @@ -852,7 +847,6 @@ mod tests { let timeout = vote2_author_2.generate_2chain_timeout(certificate_for_genesis()); let signature = timeout.sign(&signers[2]).unwrap(); vote2_author_2.add_2chain_timeout(timeout, signature); - vote2_author_2.set_verified(); match pending_votes.insert_vote(&vote2_author_2, &validator_verifier) { VoteReceptionResult::New2ChainTimeoutCertificate(tc) => { assert!(validator_verifier diff --git a/consensus/src/pipeline/buffer_item.rs b/consensus/src/pipeline/buffer_item.rs index 3326eb2f23f96..b7723c7a64bed 100644 --- a/consensus/src/pipeline/buffer_item.rs +++ b/consensus/src/pipeline/buffer_item.rs @@ -17,7 +17,7 @@ use aptos_logger::prelude::*; use aptos_reliable_broadcast::DropGuard; use aptos_types::{ block_info::BlockInfo, - ledger_info::{LedgerInfo, LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures}, + ledger_info::{LedgerInfo, LedgerInfoWithSignatures, SignatureAggregator}, validator_verifier::ValidatorVerifier, }; use futures::future::BoxFuture; @@ -40,18 +40,18 @@ fn generate_commit_ledger_info( ) } -fn ledger_info_with_unverified_signatures( +fn signature_aggregator( unverified_votes: HashMap, commit_ledger_info: &LedgerInfo, -) -> LedgerInfoWithUnverifiedSignatures { - let mut li_with_sig = LedgerInfoWithUnverifiedSignatures::new(commit_ledger_info.clone()); +) -> SignatureAggregator { + let mut sig_aggregator = SignatureAggregator::new(commit_ledger_info.clone()); for vote in unverified_votes.values() { let sig = vote.signature_with_status(); if vote.ledger_info() == commit_ledger_info { - li_with_sig.add_signature(vote.author(), sig); + sig_aggregator.add_signature(vote.author(), sig); } } - li_with_sig + sig_aggregator } // we differentiate buffer items at different stages @@ -68,7 +68,7 @@ pub struct OrderedItem { pub struct ExecutedItem { pub executed_blocks: Vec, - pub partial_commit_proof: LedgerInfoWithUnverifiedSignatures, + pub partial_commit_proof: SignatureAggregator, pub callback: StateComputerCommitCallBackType, pub commit_info: BlockInfo, pub ordered_proof: LedgerInfoWithSignatures, @@ -76,7 +76,7 @@ pub struct ExecutedItem { pub struct SignedItem { pub executed_blocks: Vec, - pub partial_commit_proof: LedgerInfoWithUnverifiedSignatures, + pub partial_commit_proof: SignatureAggregator, pub callback: StateComputerCommitCallBackType, pub commit_vote: CommitVote, pub rb_handle: Option<(Instant, DropGuard)>, @@ -173,11 +173,14 @@ impl BufferItem { order_vote_enabled, ); - let mut partial_commit_proof = ledger_info_with_unverified_signatures( - unverified_votes, - &commit_ledger_info, - ); - if let Ok(commit_proof) = partial_commit_proof.aggregate_and_verify(validator) { + let mut partial_commit_proof = + signature_aggregator(unverified_votes, &commit_ledger_info); + if let Ok(commit_proof) = partial_commit_proof + .aggregate_and_verify(validator) + .map(|(ledger_info, aggregated_sig)| { + LedgerInfoWithSignatures::new(ledger_info, aggregated_sig) + }) + { debug!( "{} advance to aggregated from ordered", commit_proof.commit_info() @@ -217,10 +220,13 @@ impl BufferItem { // we don't add the signature here, it'll be added when receiving the commit vote from self let commit_vote = CommitVote::new_with_signature( author, - partial_commit_proof.ledger_info().clone(), + partial_commit_proof.data().clone(), signature, ); - debug!("{} advance to signed", partial_commit_proof.commit_info()); + debug!( + "{} advance to signed", + partial_commit_proof.data().commit_info() + ); Self::Signed(Box::new(SignedItem { executed_blocks, @@ -250,7 +256,10 @@ impl BufferItem { partial_commit_proof: local_commit_proof, .. } = *signed_item; - assert_eq!(local_commit_proof.commit_info(), commit_proof.commit_info()); + assert_eq!( + local_commit_proof.data().commit_info(), + commit_proof.commit_info() + ); debug!( "{} advance to aggregated with commit decision", commit_proof.commit_info() @@ -316,6 +325,9 @@ impl BufferItem { .partial_commit_proof .clone() .aggregate_and_verify(validator) + .map(|(ledger_info, aggregated_sig)| { + LedgerInfoWithSignatures::new(ledger_info, aggregated_sig) + }) { return Self::Aggregated(Box::new(AggregatedItem { executed_blocks: signed_item.executed_blocks, @@ -339,6 +351,9 @@ impl BufferItem { if let Ok(commit_proof) = executed_item .partial_commit_proof .aggregate_and_verify(validator) + .map(|(ledger_info, aggregated_sig)| { + LedgerInfoWithSignatures::new(ledger_info, aggregated_sig) + }) { return Self::Aggregated(Box::new(AggregatedItem { executed_blocks: executed_item.executed_blocks, @@ -405,7 +420,7 @@ impl BufferItem { } }, Self::Signed(signed) => { - if signed.partial_commit_proof.commit_info() == target_commit_info { + if signed.partial_commit_proof.data().commit_info() == target_commit_info { signed.partial_commit_proof.add_signature(author, signature); return Ok(()); } diff --git a/consensus/src/pipeline/buffer_manager.rs b/consensus/src/pipeline/buffer_manager.rs index 1b9136efb971a..8fb509e4880af 100644 --- a/consensus/src/pipeline/buffer_manager.rs +++ b/consensus/src/pipeline/buffer_manager.rs @@ -472,7 +472,7 @@ impl BufferManager { let executed_item = item.unwrap_executed_ref(); let request = self.create_new_request(SigningRequest { ordered_ledger_info: executed_item.ordered_proof.clone(), - commit_ledger_info: executed_item.partial_commit_proof.ledger_info().clone(), + commit_ledger_info: executed_item.partial_commit_proof.data().clone(), }); if cursor == self.signing_root { let sender = self.signing_phase_tx.clone(); diff --git a/consensus/src/round_manager_test.rs b/consensus/src/round_manager_test.rs index 7bccb2747163e..944f6b10d514f 100644 --- a/consensus/src/round_manager_test.rs +++ b/consensus/src/round_manager_test.rs @@ -656,7 +656,6 @@ fn process_and_vote_on_proposal( info!("Processing votes on node {}", proposer_node.identity_desc()); if process_votes { for vote_msg in votes { - vote_msg.vote().set_verified(); timed_block_on( runtime, proposer_node.round_manager.process_vote_msg(vote_msg), @@ -708,7 +707,6 @@ fn new_round_on_quorum_cert() { .await .unwrap(); let vote_msg = node.next_vote().await; - vote_msg.vote().set_verified(); // Adding vote to form a QC node.round_manager.process_vote_msg(vote_msg).await.unwrap(); @@ -1654,7 +1652,7 @@ fn sync_on_partial_newer_sync_info() { runtime.spawn(playground.start()); timed_block_on(&runtime, async { // commit block 1 after 4 rounds - for i in 1..=4 { + for _ in 1..=4 { let proposal_msg = node.next_proposal().await; node.round_manager @@ -1662,9 +1660,6 @@ fn sync_on_partial_newer_sync_info() { .await .unwrap(); let vote_msg = node.next_vote().await; - if i < 2 { - vote_msg.vote().set_verified(); - } // Adding vote to form a QC node.round_manager.process_vote_msg(vote_msg).await.unwrap(); } @@ -1761,7 +1756,6 @@ fn safety_rules_crash() { // sign proposal reset_safety_rules(&mut node); - vote_msg.vote().set_verified(); node.round_manager.process_vote_msg(vote_msg).await.unwrap(); } @@ -1801,9 +1795,6 @@ fn echo_timeout() { // node 0 doesn't timeout and should echo the timeout after 2 timeout message for i in 0..3 { let timeout_vote = node_0.next_vote().await; - if i < 2 { - timeout_vote.vote().set_verified(); - } let result = node_0.round_manager.process_vote_msg(timeout_vote).await; // first and third message should not timeout if i == 0 || i == 2 { @@ -1817,13 +1808,10 @@ fn echo_timeout() { let node_1 = &mut nodes[1]; // it receives 4 timeout messages (1 from each) and doesn't echo since it already timeout - for i in 0..4 { + for _ in 0..4 { let timeout_vote = node_1.next_vote().await; // Verifying only some vote messages to check that round manager can accept both // verified and unverified votes - if i < 2 { - timeout_vote.vote().set_verified(); - } node_1 .round_manager .process_vote_msg(timeout_vote) @@ -2203,7 +2191,6 @@ pub fn forking_retrieval_test() { } let vote_msg_on_timeout = node.next_vote().await; - vote_msg_on_timeout.vote().set_verified(); assert!(vote_msg_on_timeout.vote().is_timeout()); if node.id != behind_node { let result = node diff --git a/types/src/ledger_info.rs b/types/src/ledger_info.rs index 840c2c2f62793..a51a151c12645 100644 --- a/types/src/ledger_info.rs +++ b/types/src/ledger_info.rs @@ -12,7 +12,10 @@ use crate::{ transaction::Version, validator_verifier::{ValidatorVerifier, VerifyError}, }; -use aptos_crypto::{bls12381, hash::HashValue}; +use aptos_crypto::{ + bls12381, + hash::{CryptoHash, HashValue}, +}; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; use derivative::Derivative; #[cfg(any(test, feature = "fuzzing"))] @@ -390,7 +393,7 @@ pub struct SignatureWithStatus { } impl SignatureWithStatus { - pub fn set_verified(&self) { + pub(crate) fn set_verified(&self) { self.verification_status.store(true, Ordering::SeqCst); } @@ -436,29 +439,25 @@ impl<'de> Deserialize<'de> for SignatureWithStatus { /// verify the aggregated signature at once. If the aggregated signature is invalid, then we verify each individual /// unverified signature and remove the invalid signatures. #[derive(Clone, Debug, Eq, PartialEq)] -pub struct LedgerInfoWithUnverifiedSignatures { - ledger_info: LedgerInfo, +pub struct SignatureAggregator { + data: T, signatures: BTreeMap, } -impl Display for LedgerInfoWithUnverifiedSignatures { +impl Display for SignatureAggregator { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { - write!(f, "{}", self.ledger_info) + write!(f, "{}", self.data) } } -impl LedgerInfoWithUnverifiedSignatures { - pub fn new(ledger_info: LedgerInfo) -> Self { +impl SignatureAggregator { + pub fn new(data: T) -> Self { Self { - ledger_info, + data, signatures: BTreeMap::default(), } } - pub fn commit_info(&self) -> &BlockInfo { - self.ledger_info.commit_info() - } - pub fn add_signature(&mut self, validator: AccountAddress, signature: &SignatureWithStatus) { self.signatures.insert(validator, signature.clone()); } @@ -511,40 +510,34 @@ impl LedgerInfoWithUnverifiedSignatures { fn filter_invalid_signatures(&mut self, verifier: &ValidatorVerifier) { let signatures = mem::take(&mut self.signatures); - self.signatures = verifier.filter_invalid_signatures(self.ledger_info(), signatures); + self.signatures = verifier.filter_invalid_signatures(&self.data, signatures); } /// Try to aggregate all signatures if the voting power is enough. If the aggregated signature is - /// valid, return the LedgerInfoWithSignatures. Also merge valid unverified signatures into verified. + /// valid, return the aggregated signature. Also merge valid unverified signatures into verified. pub fn aggregate_and_verify( &mut self, verifier: &ValidatorVerifier, - ) -> Result { + ) -> Result<(T, AggregateSignature), VerifyError> { let aggregated_sig = self.try_aggregate(verifier)?; - match verifier.verify_multi_signatures(self.ledger_info(), &aggregated_sig) { + match verifier.verify_multi_signatures(&self.data, &aggregated_sig) { Ok(_) => { // We are not marking all the signatures as "verified" here, as two malicious // voters can collude and create a valid aggregated signature. - Ok(LedgerInfoWithSignatures::new( - self.ledger_info.clone(), - aggregated_sig, - )) + Ok((self.data.clone(), aggregated_sig)) }, Err(_) => { self.filter_invalid_signatures(verifier); - let aggregate_sig = self.try_aggregate(verifier)?; - Ok(LedgerInfoWithSignatures::new( - self.ledger_info.clone(), - aggregate_sig, - )) + let aggregated_sig = self.try_aggregate(verifier)?; + Ok((self.data.clone(), aggregated_sig)) }, } } - pub fn ledger_info(&self) -> &LedgerInfo { - &self.ledger_info + pub fn data(&self) -> &T { + &self.data } } @@ -701,7 +694,7 @@ mod tests { } #[test] - fn test_ledger_info_with_unverified_signatures() { + fn test_signature_aggregator() { let ledger_info = LedgerInfo::new(BlockInfo::empty(), HashValue::random()); const NUM_SIGNERS: u8 = 7; // Generate NUM_SIGNERS random signers. @@ -722,21 +715,20 @@ mod tests { ValidatorVerifier::new_with_quorum_voting_power(validator_infos, 5) .expect("Incorrect quorum size."); - let mut ledger_info_with_unverified_signatures = - LedgerInfoWithUnverifiedSignatures::new(ledger_info.clone()); + let mut signature_aggregator = SignatureAggregator::new(ledger_info.clone()); let mut partial_sig = PartialSignatures::empty(); let sig = SignatureWithStatus::from(validator_signers[0].sign(&ledger_info).unwrap()); sig.set_verified(); - ledger_info_with_unverified_signatures.add_signature(validator_signers[0].author(), &sig); + signature_aggregator.add_signature(validator_signers[0].author(), &sig); partial_sig.add_signature( validator_signers[0].author(), validator_signers[0].sign(&ledger_info).unwrap(), ); - ledger_info_with_unverified_signatures.add_signature( + signature_aggregator.add_signature( validator_signers[1].author(), &SignatureWithStatus::from(validator_signers[1].sign(&ledger_info).unwrap()), ); @@ -747,13 +739,13 @@ mod tests { let sig2 = SignatureWithStatus::from(validator_signers[2].sign(&ledger_info).unwrap()); sig2.set_verified(); - ledger_info_with_unverified_signatures.add_signature(validator_signers[2].author(), &sig2); + signature_aggregator.add_signature(validator_signers[2].author(), &sig2); partial_sig.add_signature( validator_signers[2].author(), validator_signers[2].sign(&ledger_info).unwrap(), ); - ledger_info_with_unverified_signatures.add_signature( + signature_aggregator.add_signature( validator_signers[3].author(), &SignatureWithStatus::from(validator_signers[3].sign(&ledger_info).unwrap()), ); @@ -762,83 +754,44 @@ mod tests { validator_signers[3].sign(&ledger_info).unwrap(), ); + assert_eq!(signature_aggregator.all_voters().count(), 4); + assert_eq!(signature_aggregator.unverified_voters().count(), 2); + assert_eq!(signature_aggregator.verified_voters().count(), 2); assert_eq!( - ledger_info_with_unverified_signatures.all_voters().count(), - 4 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .unverified_voters() - .count(), - 2 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .verified_voters() - .count(), - 2 - ); - assert_eq!( - ledger_info_with_unverified_signatures.check_voting_power(&validator_verifier, true), + signature_aggregator.check_voting_power(&validator_verifier, true), Err(VerifyError::TooLittleVotingPower { voting_power: 4, expected_voting_power: 5 }) ); - ledger_info_with_unverified_signatures.add_signature( + signature_aggregator.add_signature( validator_signers[4].author(), &SignatureWithStatus::from(bls12381::Signature::dummy_signature()), ); + assert_eq!(signature_aggregator.all_voters().count(), 5); + assert_eq!(signature_aggregator.unverified_voters().count(), 3); + assert_eq!(signature_aggregator.verified_voters().count(), 2); assert_eq!( - ledger_info_with_unverified_signatures.all_voters().count(), - 5 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .unverified_voters() - .count(), - 3 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .verified_voters() - .count(), - 2 - ); - assert_eq!( - ledger_info_with_unverified_signatures + signature_aggregator .check_voting_power(&validator_verifier, true) .unwrap(), 5 ); assert_eq!( - ledger_info_with_unverified_signatures.aggregate_and_verify(&validator_verifier), + signature_aggregator.aggregate_and_verify(&validator_verifier), Err(VerifyError::TooLittleVotingPower { voting_power: 4, expected_voting_power: 5 }) ); - assert_eq!( - ledger_info_with_unverified_signatures - .unverified_voters() - .count(), - 0 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .verified_voters() - .count(), - 4 - ); - assert_eq!( - ledger_info_with_unverified_signatures.all_voters().count(), - 4 - ); + assert_eq!(signature_aggregator.unverified_voters().count(), 0); + assert_eq!(signature_aggregator.verified_voters().count(), 4); + assert_eq!(signature_aggregator.all_voters().count(), 4); assert_eq!(validator_verifier.pessimistic_verify_set().len(), 1); - ledger_info_with_unverified_signatures.add_signature( + signature_aggregator.add_signature( validator_signers[5].author(), &SignatureWithStatus::from(validator_signers[5].sign(&ledger_info).unwrap()), ); @@ -847,91 +800,49 @@ mod tests { validator_signers[5].sign(&ledger_info).unwrap(), ); + assert_eq!(signature_aggregator.all_voters().count(), 5); + assert_eq!(signature_aggregator.unverified_voters().count(), 1); + assert_eq!(signature_aggregator.verified_voters().count(), 4); assert_eq!( - ledger_info_with_unverified_signatures.all_voters().count(), - 5 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .unverified_voters() - .count(), - 1 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .verified_voters() - .count(), - 4 - ); - assert_eq!( - ledger_info_with_unverified_signatures + signature_aggregator .check_voting_power(&validator_verifier, true) .unwrap(), 5 ); - let aggregate_sig = LedgerInfoWithSignatures::new( - ledger_info.clone(), - validator_verifier - .aggregate_signatures(partial_sig.signatures_iter()) - .unwrap(), - ); + let aggregate_sig = validator_verifier + .aggregate_signatures(partial_sig.signatures_iter()) + .unwrap(); assert_eq!( - ledger_info_with_unverified_signatures + signature_aggregator .aggregate_and_verify(&validator_verifier) .unwrap(), - aggregate_sig - ); - assert_eq!( - ledger_info_with_unverified_signatures - .unverified_voters() - .count(), - 1 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .verified_voters() - .count(), - 4 + (ledger_info.clone(), aggregate_sig.clone()) ); + assert_eq!(signature_aggregator.unverified_voters().count(), 1); + assert_eq!(signature_aggregator.verified_voters().count(), 4); assert_eq!(validator_verifier.pessimistic_verify_set().len(), 1); - ledger_info_with_unverified_signatures.add_signature( + signature_aggregator.add_signature( validator_signers[6].author(), &SignatureWithStatus::from(bls12381::Signature::dummy_signature()), ); + assert_eq!(signature_aggregator.all_voters().count(), 6); assert_eq!( - ledger_info_with_unverified_signatures.all_voters().count(), - 6 - ); - assert_eq!( - ledger_info_with_unverified_signatures + signature_aggregator .check_voting_power(&validator_verifier, true) .unwrap(), 6 ); assert_eq!( - ledger_info_with_unverified_signatures + signature_aggregator .aggregate_and_verify(&validator_verifier) .unwrap(), - aggregate_sig - ); - assert_eq!( - ledger_info_with_unverified_signatures - .unverified_voters() - .count(), - 0 - ); - assert_eq!( - ledger_info_with_unverified_signatures - .verified_voters() - .count(), - 5 - ); - assert_eq!( - ledger_info_with_unverified_signatures.all_voters().count(), - 5 + (ledger_info.clone(), aggregate_sig) ); + assert_eq!(signature_aggregator.unverified_voters().count(), 0); + assert_eq!(signature_aggregator.verified_voters().count(), 5); + assert_eq!(signature_aggregator.all_voters().count(), 5); assert_eq!(validator_verifier.pessimistic_verify_set().len(), 2); } } From a112d3d6fef37178bc5917ff2979c57f2f4fe75a Mon Sep 17 00:00:00 2001 From: Satya Vusirikala Date: Sat, 19 Oct 2024 20:32:02 -0700 Subject: [PATCH 2/3] Minor comment --- consensus/src/pending_votes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/src/pending_votes.rs b/consensus/src/pending_votes.rs index 45d9bf47534c0..300ecd67fae23 100644 --- a/consensus/src/pending_votes.rs +++ b/consensus/src/pending_votes.rs @@ -365,7 +365,7 @@ impl PendingVotes { ))); }, VoteStatus::NotEnoughVotes(sig_aggregator) => { - // add this vote to the ledger info with signatures + // add this vote to the signature aggregator sig_aggregator.add_signature(vote.author(), vote.signature_with_status()); // check if we have enough signatures to create a QC From 580bbb327587e989d52d7b2eeed7715b6150a8a3 Mon Sep 17 00:00:00 2001 From: Satya Vusirikala Date: Mon, 21 Oct 2024 15:18:41 -0700 Subject: [PATCH 3/3] Address PR comments --- consensus/src/pipeline/buffer_item.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consensus/src/pipeline/buffer_item.rs b/consensus/src/pipeline/buffer_item.rs index b7723c7a64bed..67fcca255875e 100644 --- a/consensus/src/pipeline/buffer_item.rs +++ b/consensus/src/pipeline/buffer_item.rs @@ -40,7 +40,7 @@ fn generate_commit_ledger_info( ) } -fn signature_aggregator( +fn create_signature_aggregator( unverified_votes: HashMap, commit_ledger_info: &LedgerInfo, ) -> SignatureAggregator { @@ -174,7 +174,7 @@ impl BufferItem { ); let mut partial_commit_proof = - signature_aggregator(unverified_votes, &commit_ledger_info); + create_signature_aggregator(unverified_votes, &commit_ledger_info); if let Ok(commit_proof) = partial_commit_proof .aggregate_and_verify(validator) .map(|(ledger_info, aggregated_sig)| {