From 85c70513c9b544a8e1d9e2dcafa27b7c2ee12b2f Mon Sep 17 00:00:00 2001 From: Satya Vusirikala Date: Mon, 23 Sep 2024 19:14:38 -0700 Subject: [PATCH] Using signature with status --- consensus/consensus-types/src/order_vote.rs | 19 ++- consensus/consensus-types/src/vote.rs | 14 +- consensus/safety-rules/src/test_utils.rs | 2 +- .../src/block_storage/block_store_test.rs | 11 +- consensus/src/epoch_manager.rs | 74 +++++++---- consensus/src/liveness/round_state.rs | 6 +- consensus/src/pending_order_votes.rs | 74 +++-------- consensus/src/pending_votes.rs | 108 +++++----------- consensus/src/recovery_manager.rs | 3 - consensus/src/round_manager.rs | 39 +----- consensus/src/round_manager_test.rs | 49 +++---- types/src/ledger_info.rs | 121 +++++++++++++++--- 12 files changed, 260 insertions(+), 260 deletions(-) diff --git a/consensus/consensus-types/src/order_vote.rs b/consensus/consensus-types/src/order_vote.rs index a75c4f55d859a..3125376b15c90 100644 --- a/consensus/consensus-types/src/order_vote.rs +++ b/consensus/consensus-types/src/order_vote.rs @@ -6,7 +6,10 @@ use crate::common::Author; use anyhow::{ensure, Context}; use aptos_crypto::{bls12381, HashValue}; use aptos_short_hex_str::AsShortHexStr; -use aptos_types::{ledger_info::LedgerInfo, validator_verifier::ValidatorVerifier}; +use aptos_types::{ + ledger_info::{LedgerInfo, SignatureWithStatus}, + validator_verifier::ValidatorVerifier, +}; use serde::{Deserialize, Serialize}; use std::fmt::{Debug, Display, Formatter}; @@ -16,8 +19,8 @@ pub struct OrderVote { author: Author, /// LedgerInfo of a block that is going to be ordered in case this vote gathers QC. ledger_info: LedgerInfo, - /// Signature of the LedgerInfo. - signature: bls12381::Signature, + /// Signature on the LedgerInfo along with a status on whether the signature is verified. + signature: SignatureWithStatus, } impl Display for OrderVote { @@ -48,7 +51,7 @@ impl OrderVote { Self { author, ledger_info, - signature, + signature: SignatureWithStatus::from(signature), } } @@ -60,7 +63,7 @@ impl OrderVote { &self.ledger_info } - pub fn signature(&self) -> &bls12381::Signature { + pub fn signature(&self) -> &SignatureWithStatus { &self.signature } @@ -80,8 +83,10 @@ impl OrderVote { /// Verifies the signature on the LedgerInfo. pub fn verify_signature(&self, validator: &ValidatorVerifier) -> anyhow::Result<()> { validator - .verify(self.author(), &self.ledger_info, &self.signature) - .context("Failed to verify OrderVote") + .verify(self.author(), &self.ledger_info, self.signature.signature()) + .context("Failed to verify OrderVote")?; + self.signature.set_verified(); + Ok(()) } /// Performs full verification including the signature verification. diff --git a/consensus/consensus-types/src/vote.rs b/consensus/consensus-types/src/vote.rs index 3eeccda26edd6..db3ca6876a667 100644 --- a/consensus/consensus-types/src/vote.rs +++ b/consensus/consensus-types/src/vote.rs @@ -9,7 +9,8 @@ use anyhow::{ensure, Context}; use aptos_crypto::{bls12381, hash::CryptoHash, CryptoMaterialError}; use aptos_short_hex_str::AsShortHexStr; use aptos_types::{ - ledger_info::LedgerInfo, validator_signer::ValidatorSigner, + ledger_info::{LedgerInfo, SignatureWithStatus}, + validator_signer::ValidatorSigner, validator_verifier::ValidatorVerifier, }; use serde::{Deserialize, Serialize}; @@ -27,8 +28,8 @@ pub struct Vote { author: Author, /// LedgerInfo of a block that is going to be committed in case this vote gathers QC. ledger_info: LedgerInfo, - /// Signature of the LedgerInfo - signature: bls12381::Signature, + /// Signature on the LedgerInfo along with a status on whether the signature is verified. + signature: SignatureWithStatus, /// The 2-chain timeout and corresponding signature. two_chain_timeout: Option<(TwoChainTimeout, bls12381::Signature)>, } @@ -83,7 +84,7 @@ impl Vote { vote_data, author, ledger_info, - signature, + signature: SignatureWithStatus::from(signature), two_chain_timeout: None, } } @@ -108,7 +109,7 @@ impl Vote { } /// Return the signature of the vote - pub fn signature(&self) -> &bls12381::Signature { + pub fn signature(&self) -> &SignatureWithStatus { &self.signature } @@ -163,8 +164,9 @@ impl Vote { /// Verifies the signature on the LedgerInfo. pub fn verify_signature(&self, validator: &ValidatorVerifier) -> anyhow::Result<()> { validator - .verify(self.author(), &self.ledger_info, &self.signature) + .verify(self.author(), &self.ledger_info, self.signature.signature()) .context("Failed to verify Vote signature")?; + self.signature.set_verified(); Ok(()) } diff --git a/consensus/safety-rules/src/test_utils.rs b/consensus/safety-rules/src/test_utils.rs index ce161c0a5fb14..80ad29719e9cc 100644 --- a/consensus/safety-rules/src/test_utils.rs +++ b/consensus/safety-rules/src/test_utils.rs @@ -173,7 +173,7 @@ pub fn make_proposal_with_parent_and_overrides( PartialSignatures::empty(), ); - ledger_info_with_signatures.add_signature(vote.author(), vote.signature().clone()); + ledger_info_with_signatures.add_signature(vote.author(), vote.signature().signature().clone()); let qc = QuorumCert::new( vote_data, diff --git a/consensus/src/block_storage/block_store_test.rs b/consensus/src/block_storage/block_store_test.rs index 3731e49012f8a..ff0abf885b07f 100644 --- a/consensus/src/block_storage/block_store_test.rs +++ b/consensus/src/block_storage/block_store_test.rs @@ -23,7 +23,7 @@ use aptos_consensus_types::{ }; use aptos_crypto::{HashValue, PrivateKey}; use aptos_types::{ - epoch_state::EpochState, ledger_info::VerificationStatus, validator_signer::ValidatorSigner, + epoch_state::EpochState, validator_signer::ValidatorSigner, validator_verifier::random_validator_verifier, }; use proptest::prelude::*; @@ -302,14 +302,14 @@ async fn test_insert_vote() { voter, ) .unwrap(); - let vote_res = - pending_votes.insert_vote(&vote, epoch_state.clone(), VerificationStatus::Verified); + vote.signature().set_verified(); + let vote_res = pending_votes.insert_vote(&vote, epoch_state.clone()); // first vote of an author is accepted assert_eq!(vote_res, VoteReceptionResult::VoteAdded(i as u128)); // filter out duplicates assert_eq!( - pending_votes.insert_vote(&vote, epoch_state.clone(), VerificationStatus::Verified), + pending_votes.insert_vote(&vote, epoch_state.clone()), VoteReceptionResult::DuplicateVote, ); // qc is still not there @@ -332,7 +332,8 @@ async fn test_insert_vote() { final_voter, ) .unwrap(); - match pending_votes.insert_vote(&vote, epoch_state.clone(), VerificationStatus::Verified) { + vote.signature().set_verified(); + match pending_votes.insert_vote(&vote, epoch_state.clone()) { VoteReceptionResult::NewQuorumCertificate(qc) => { assert_eq!(qc.certified_block().id(), block.id()); block_store diff --git a/consensus/src/epoch_manager.rs b/consensus/src/epoch_manager.rs index 7c9fd6f3fbc8a..08ee02b89c835 100644 --- a/consensus/src/epoch_manager.rs +++ b/consensus/src/epoch_manager.rs @@ -1445,20 +1445,29 @@ impl EpochManager

{ { if let UnverifiedEvent::OrderVoteMsg(order_vote) = &unverified_event { order_vote.verify_metadata()?; - Self::forward_event_to( + Self::forward_event( + quorum_store_msg_tx, round_manager_tx, - ( - peer_id, - discriminant(&VerifiedEvent::UnverifiedOrderVoteMsg( - order_vote.clone(), - )), - ), - ( - peer_id, - VerifiedEvent::UnverifiedOrderVoteMsg(order_vote.clone()), - ), - ) - .context("round manager sending unverified order vote to round manager")?; + buffered_proposal_tx, + peer_id, + VerifiedEvent::OrderVoteMsg(order_vote.clone()), + payload_manager, + pending_blocks, + ); + // Self::forward_event_to( + // round_manager_tx, + // ( + // peer_id, + // discriminant(&VerifiedEvent::OrderVoteMsg( + // order_vote.clone(), + // )), + // ), + // ( + // peer_id, + // VerifiedEvent::OrderVoteMsg(order_vote.clone()), + // ), + // ) + // .context("round manager sending unverified order vote to round manager")?; return Ok(()); } } @@ -1478,21 +1487,32 @@ impl EpochManager

{ .observe(start_time.elapsed().as_secs_f64()); match result { Ok(()) => { - if let Err(e) = Self::forward_event_to( + // if let Err(e) = + Self::forward_event( + quorum_store_msg_tx, round_manager_tx, - ( - peer_id, - discriminant(&VerifiedEvent::UnverifiedVoteMsg( - vote.clone(), - )), - ), - (peer_id, VerifiedEvent::UnverifiedVoteMsg(vote.clone())), - ) - .context( - "round manager sending unverified vote to round manager", - ) { - warn!("Failed to forward unverified event: {}", e); - }; + buffered_proposal_tx, + peer_id, + VerifiedEvent::VoteMsg(vote.clone()), + payload_manager, + pending_blocks, + ); + + // Self::forward_event_to( + // round_manager_tx, + // ( + // peer_id, + // discriminant(&VerifiedEvent::VoteMsg( + // vote.clone(), + // )), + // ), + // (peer_id, VerifiedEvent::VoteMsg(vote)), + // ) + // .context( + // "round manager sending unverified vote to round manager", + // ) { + // warn!("Failed to forward unverified event: {}", e); + // }; }, Err(e) => { error!( diff --git a/consensus/src/liveness/round_state.rs b/consensus/src/liveness/round_state.rs index a44a7100622aa..105bd897b4369 100644 --- a/consensus/src/liveness/round_state.rs +++ b/consensus/src/liveness/round_state.rs @@ -13,7 +13,7 @@ use aptos_consensus_types::{ }; use aptos_crypto::HashValue; use aptos_logger::{prelude::*, Schema}; -use aptos_types::{epoch_state::EpochState, ledger_info::VerificationStatus}; +use aptos_types::epoch_state::EpochState; use futures::future::AbortHandle; use serde::Serialize; use std::{fmt, sync::Arc, time::Duration}; @@ -274,11 +274,9 @@ impl RoundState { &mut self, vote: &Vote, epoch_state: Arc, - verification_status: VerificationStatus, ) -> VoteReceptionResult { if vote.vote_data().proposed().round() == self.current_round { - self.pending_votes - .insert_vote(vote, epoch_state, verification_status) + self.pending_votes.insert_vote(vote, epoch_state) } else { VoteReceptionResult::UnexpectedRound( vote.vote_data().proposed().round(), diff --git a/consensus/src/pending_order_votes.rs b/consensus/src/pending_order_votes.rs index 501d3bd2ed043..17d1c652f4cf9 100644 --- a/consensus/src/pending_order_votes.rs +++ b/consensus/src/pending_order_votes.rs @@ -8,10 +8,7 @@ use aptos_crypto::{hash::CryptoHash, HashValue}; use aptos_logger::prelude::*; use aptos_types::{ epoch_state::EpochState, - ledger_info::{ - LedgerInfo, LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures, - VerificationStatus, - }, + ledger_info::{LedgerInfo, LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures}, validator_verifier::VerifyError, }; use std::{collections::HashMap, sync::Arc}; @@ -67,7 +64,6 @@ impl PendingOrderVotes { &mut self, order_vote: &OrderVote, epoch_state: Arc, - verification_status: VerificationStatus, verified_quorum_cert: Option, ) -> OrderVoteReceptionResult { // derive data from order vote @@ -114,11 +110,7 @@ impl PendingOrderVotes { order_vote.author() ); } - li_with_sig.add_signature( - order_vote.author(), - order_vote.signature().clone(), - verification_status, - ); + li_with_sig.add_signature(order_vote.author(), order_vote.signature().clone()); match li_with_sig.check_voting_power(&epoch_state.verifier, true) { Ok(aggregated_voting_power) => { assert!( @@ -190,7 +182,7 @@ mod tests { aggregate_signature::PartialSignatures, block_info::BlockInfo, epoch_state::EpochState, - ledger_info::{LedgerInfo, VerificationStatus}, + ledger_info::LedgerInfo, validator_verifier::{random_validator_verifier, VerifyError}, }; use std::sync::Arc; @@ -222,11 +214,11 @@ mod tests { ); // first time a new order vote is added -> OrderVoteAdded + order_vote_1_author_0.signature().set_verified(); assert_eq!( pending_order_votes.insert_order_vote( &order_vote_1_author_0, epoch_state.clone(), - VerificationStatus::Verified, Some(qc.clone()) ), OrderVoteReceptionResult::VoteAdded(1) @@ -237,7 +229,6 @@ mod tests { pending_order_votes.insert_order_vote( &order_vote_1_author_0, epoch_state.clone(), - VerificationStatus::Verified, None ), OrderVoteReceptionResult::VoteAdded(1) @@ -250,11 +241,11 @@ mod tests { li2.clone(), signers[1].sign(&li2).expect("Unable to sign ledger info"), ); + order_vote_2_author_1.signature().set_verified(); assert_eq!( pending_order_votes.insert_order_vote( &order_vote_2_author_1, epoch_state.clone(), - VerificationStatus::Verified, Some(qc.clone()) ), OrderVoteReceptionResult::VoteAdded(1) @@ -268,10 +259,10 @@ mod tests { li2.clone(), signers[2].sign(&li2).expect("Unable to sign ledger info"), ); + order_vote_2_author_2.signature().set_verified(); match pending_order_votes.insert_order_vote( &order_vote_2_author_2, epoch_state.clone(), - VerificationStatus::Verified, None, ) { OrderVoteReceptionResult::NewLedgerInfoWithSignatures((_qc, li_with_sig)) => { @@ -308,14 +299,16 @@ mod tests { li.clone(), signers[0].sign(&li).expect("Unable to sign ledger info"), ); - partial_signatures.add_signature(signers[0].author(), vote_0.signature().clone()); + partial_signatures + .add_signature(signers[0].author(), vote_0.signature().signature().clone()); let vote_1 = OrderVote::new_with_signature( signers[1].author(), li.clone(), signers[1].sign(&li).expect("Unable to sign ledger info"), ); - partial_signatures.add_signature(signers[1].author(), vote_1.signature().clone()); + partial_signatures + .add_signature(signers[1].author(), vote_1.signature().signature().clone()); let vote_2 = OrderVote::new_with_signature( signers[2].author(), @@ -328,7 +321,8 @@ mod tests { li.clone(), signers[3].sign(&li).expect("Unable to sign ledger info"), ); - partial_signatures.add_signature(signers[3].author(), vote_3.signature().clone()); + partial_signatures + .add_signature(signers[3].author(), vote_3.signature().signature().clone()); let vote_4 = OrderVote::new_with_signature( signers[4].author(), @@ -337,43 +331,25 @@ mod tests { ); assert_eq!( - pending_order_votes.insert_order_vote( - &vote_0, - epoch_state.clone(), - VerificationStatus::Unverified, - Some(qc.clone()) - ), + pending_order_votes.insert_order_vote(&vote_0, epoch_state.clone(), Some(qc.clone())), OrderVoteReceptionResult::VoteAdded(1) ); + vote_0.signature().set_verified(); assert_eq!( - pending_order_votes.insert_order_vote( - &vote_0, - epoch_state.clone(), - VerificationStatus::Verified, - None - ), + pending_order_votes.insert_order_vote(&vote_0, epoch_state.clone(), None), OrderVoteReceptionResult::VoteAdded(1) ); + vote_1.signature().set_verified(); assert_eq!( - pending_order_votes.insert_order_vote( - &vote_1, - epoch_state.clone(), - VerificationStatus::Verified, - None - ), + pending_order_votes.insert_order_vote(&vote_1, epoch_state.clone(), None), OrderVoteReceptionResult::VoteAdded(2) ); assert_eq!(epoch_state.verifier.pessimistic_verify_set().len(), 0); assert_eq!( - pending_order_votes.insert_order_vote( - &vote_2, - epoch_state.clone(), - VerificationStatus::Unverified, - None - ), + pending_order_votes.insert_order_vote(&vote_2, epoch_state.clone(), None), OrderVoteReceptionResult::ErrorAggregatingSignature( VerifyError::TooLittleVotingPower { voting_power: 2, @@ -387,12 +363,7 @@ mod tests { .verifier .aggregate_signatures(partial_signatures.signatures_iter()) .unwrap(); - match pending_order_votes.insert_order_vote( - &vote_3, - epoch_state.clone(), - VerificationStatus::Unverified, - None, - ) { + match pending_order_votes.insert_order_vote(&vote_3, epoch_state.clone(), None) { OrderVoteReceptionResult::NewLedgerInfoWithSignatures((_qc, li_with_sig)) => { assert!(li_with_sig .check_voting_power(&epoch_state.verifier) @@ -405,12 +376,7 @@ mod tests { }, }; - match pending_order_votes.insert_order_vote( - &vote_4, - epoch_state.clone(), - VerificationStatus::Unverified, - None, - ) { + match pending_order_votes.insert_order_vote(&vote_4, epoch_state.clone(), None) { OrderVoteReceptionResult::NewLedgerInfoWithSignatures((_qc, li_with_sig)) => { assert!(li_with_sig .check_voting_power(&epoch_state.verifier) diff --git a/consensus/src/pending_votes.rs b/consensus/src/pending_votes.rs index 9bbbdaa1ffcba..e00cad4bbb8e8 100644 --- a/consensus/src/pending_votes.rs +++ b/consensus/src/pending_votes.rs @@ -19,9 +19,7 @@ use aptos_crypto::{hash::CryptoHash, HashValue}; use aptos_logger::prelude::*; use aptos_types::{ epoch_state::EpochState, - ledger_info::{ - LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures, VerificationStatus, - }, + ledger_info::{LedgerInfoWithSignatures, LedgerInfoWithUnverifiedSignatures}, validator_verifier::VerifyError, }; use std::{collections::HashMap, fmt, sync::Arc}; @@ -92,7 +90,6 @@ impl PendingVotes { &mut self, vote: &Vote, epoch_state: Arc, - verification_status: VerificationStatus, ) -> VoteReceptionResult { // derive data from vote let li_digest = vote.ledger_info().hash(); @@ -185,11 +182,7 @@ impl PendingVotes { }, VoteStatus::NotEnoughVotes(li_with_sig) => { // add this vote to the ledger info with signatures - li_with_sig.add_signature( - vote.author(), - vote.signature().clone(), - verification_status, - ); + li_with_sig.add_signature(vote.author(), vote.signature().clone()); // check if we have enough signatures to create a QC match li_with_sig.check_voting_power(&epoch_state.verifier, true) { @@ -388,7 +381,7 @@ mod tests { aggregate_signature::PartialSignatures, block_info::BlockInfo, epoch_state::EpochState, - ledger_info::{LedgerInfo, VerificationStatus}, + ledger_info::LedgerInfo, validator_verifier::{random_validator_verifier, VerifyError}, }; use itertools::Itertools; @@ -424,23 +417,16 @@ 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.signature().set_verified(); // first time a new vote is added -> VoteAdded assert_eq!( - pending_votes.insert_vote( - &vote_data_1_author_0, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote_data_1_author_0, epoch_state.clone(),), VoteReceptionResult::VoteAdded(1) ); // same author voting for the same thing -> DuplicateVote assert_eq!( - pending_votes.insert_vote( - &vote_data_1_author_0, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote_data_1_author_0, epoch_state.clone(),), VoteReceptionResult::DuplicateVote ); @@ -454,12 +440,9 @@ mod tests { &signers[0], ) .unwrap(); + vote_data_2_author_0.signature().set_verified(); assert_eq!( - pending_votes.insert_vote( - &vote_data_2_author_0, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote_data_2_author_0, epoch_state.clone(),), VoteReceptionResult::EquivocateVote ); @@ -471,23 +454,17 @@ mod tests { &signers[1], ) .unwrap(); + vote_data_2_author_1.signature().set_verified(); assert_eq!( - pending_votes.insert_vote( - &vote_data_2_author_1, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote_data_2_author_1, epoch_state.clone(),), VoteReceptionResult::VoteAdded(1) ); // 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(); - match pending_votes.insert_vote( - &vote_data_2_author_2, - epoch_state.clone(), - VerificationStatus::Verified, - ) { + vote_data_2_author_2.signature().set_verified(); + match pending_votes.insert_vote(&vote_data_2_author_2, epoch_state.clone()) { VoteReceptionResult::NewQuorumCertificate(qc) => { assert!(qc .ledger_info() @@ -557,27 +534,28 @@ mod tests { // first time a new vote is added -> VoteAdded assert_eq!( - pending_votes.insert_vote(&vote_0, epoch_state.clone(), VerificationStatus::Unverified), + pending_votes.insert_vote(&vote_0, epoch_state.clone()), VoteReceptionResult::VoteAdded(1) ); - partial_sigs.add_signature(signers[0].author(), vote_0.signature().clone()); + partial_sigs.add_signature(signers[0].author(), vote_0.signature().signature().clone()); // same author voting for the same thing -> DuplicateVote + vote_0.signature().set_verified(); assert_eq!( - pending_votes.insert_vote(&vote_0, epoch_state.clone(), VerificationStatus::Verified), + pending_votes.insert_vote(&vote_0, epoch_state.clone()), VoteReceptionResult::DuplicateVote ); assert_eq!( - pending_votes.insert_vote(&vote_1, epoch_state.clone(), VerificationStatus::Unverified), + pending_votes.insert_vote(&vote_1, epoch_state.clone()), VoteReceptionResult::VoteAdded(2) ); - partial_sigs.add_signature(signers[1].author(), vote_1.signature().clone()); + partial_sigs.add_signature(signers[1].author(), vote_1.signature().signature().clone()); assert_eq!(epoch_state.verifier.pessimistic_verify_set().len(), 0); assert_eq!( - pending_votes.insert_vote(&vote_2, epoch_state.clone(), VerificationStatus::Unverified), + pending_votes.insert_vote(&vote_2, epoch_state.clone()), VoteReceptionResult::ErrorAggregatingSignature(VerifyError::TooLittleVotingPower { voting_power: 2, expected_voting_power: 3 @@ -586,16 +564,12 @@ mod tests { assert_eq!(epoch_state.verifier.pessimistic_verify_set().len(), 1); - partial_sigs.add_signature(signers[3].author(), vote_3.signature().clone()); + partial_sigs.add_signature(signers[3].author(), vote_3.signature().signature().clone()); let aggregated_sig = epoch_state .verifier .aggregate_signatures(partial_sigs.signatures_iter()) .unwrap(); - match pending_votes.insert_vote( - &vote_3, - epoch_state.clone(), - VerificationStatus::Unverified, - ) { + match pending_votes.insert_vote(&vote_3, epoch_state.clone()) { VoteReceptionResult::NewQuorumCertificate(qc) => { assert!(qc .ledger_info() @@ -611,11 +585,7 @@ mod tests { }, }; - match pending_votes.insert_vote( - &vote_4, - epoch_state.clone(), - VerificationStatus::Unverified, - ) { + match pending_votes.insert_vote(&vote_4, epoch_state.clone()) { VoteReceptionResult::NewQuorumCertificate(qc) => { assert!(qc .ledger_info() @@ -648,12 +618,9 @@ 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.signature().set_verified(); assert_eq!( - pending_votes.insert_vote( - &vote0_author_0, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote0_author_0, epoch_state.clone(),), VoteReceptionResult::VoteAdded(1) ); @@ -663,11 +630,7 @@ mod tests { vote0_author_0.add_2chain_timeout(timeout, signature); assert_eq!( - pending_votes.insert_vote( - &vote0_author_0, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote0_author_0, epoch_state.clone(),), VoteReceptionResult::VoteAdded(1) ); @@ -675,12 +638,9 @@ 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.signature().set_verified(); assert_eq!( - pending_votes.insert_vote( - &vote1_author_1, - epoch_state.clone(), - VerificationStatus::Verified - ), + pending_votes.insert_vote(&vote1_author_1, epoch_state.clone(),), VoteReceptionResult::VoteAdded(1) ); @@ -688,11 +648,7 @@ mod tests { let timeout = vote1_author_1.generate_2chain_timeout(certificate_for_genesis()); let signature = timeout.sign(&signers[1]).unwrap(); vote1_author_1.add_2chain_timeout(timeout, signature); - match pending_votes.insert_vote( - &vote1_author_1, - epoch_state.clone(), - VerificationStatus::Verified, - ) { + match pending_votes.insert_vote(&vote1_author_1, epoch_state.clone()) { VoteReceptionResult::EchoTimeout(voting_power) => { assert_eq!(voting_power, 2); }, @@ -709,12 +665,8 @@ 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); - - match pending_votes.insert_vote( - &vote2_author_2, - epoch_state.clone(), - VerificationStatus::Verified, - ) { + vote2_author_2.signature().set_verified(); + match pending_votes.insert_vote(&vote2_author_2, epoch_state.clone()) { VoteReceptionResult::New2ChainTimeoutCertificate(tc) => { assert!(epoch_state .verifier diff --git a/consensus/src/recovery_manager.rs b/consensus/src/recovery_manager.rs index 6de2d84685670..57e308570f93d 100644 --- a/consensus/src/recovery_manager.rs +++ b/consensus/src/recovery_manager.rs @@ -135,9 +135,6 @@ impl RecoveryManager { VerifiedEvent::VoteMsg(vote_msg) => { monitor!("process_recovery", self.process_vote_msg(*vote_msg).await) } - VerifiedEvent::UnverifiedVoteMsg(vote_msg) => { - monitor!("process_recovery", self.process_vote_msg(*vote_msg).await) - } VerifiedEvent::UnverifiedSyncInfo(sync_info) => { monitor!( "process_recovery", diff --git a/consensus/src/round_manager.rs b/consensus/src/round_manager.rs index e499b4044591c..1b07c772a9c4c 100644 --- a/consensus/src/round_manager.rs +++ b/consensus/src/round_manager.rs @@ -59,7 +59,6 @@ use aptos_safety_rules::TSafetyRules; use aptos_types::{ block_info::BlockInfo, epoch_state::EpochState, - ledger_info::VerificationStatus, on_chain_config::{ OnChainConsensusConfig, OnChainJWKConsensusConfig, OnChainRandomnessConfig, ValidatorTxnConfig, @@ -204,8 +203,6 @@ pub enum VerifiedEvent { VerifiedProposalMsg(Box), VoteMsg(Box), OrderVoteMsg(Box), - UnverifiedVoteMsg(Box), - UnverifiedOrderVoteMsg(Box), UnverifiedSyncInfo(Box), BatchMsg(Box), SignedBatchInfo(Box), @@ -1071,11 +1068,7 @@ impl RoundManager { Ok(vote) } - async fn process_order_vote_msg( - &mut self, - order_vote_msg: OrderVoteMsg, - verification_status: VerificationStatus, - ) -> anyhow::Result<()> { + async fn process_order_vote_msg(&mut self, order_vote_msg: OrderVoteMsg) -> anyhow::Result<()> { if self.onchain_config.order_vote_enabled() { fail_point!("consensus::process_order_vote_msg", |_| { Err(anyhow::anyhow!("Injected error in process_order_vote_msg")) @@ -1118,14 +1111,12 @@ impl RoundManager { self.pending_order_votes.insert_order_vote( order_vote_msg.order_vote(), self.epoch_state.clone(), - verification_status, Some(order_vote_msg.quorum_cert().clone()), ) } else { self.pending_order_votes.insert_order_vote( order_vote_msg.order_vote(), self.epoch_state.clone(), - verification_status, None, ) }; @@ -1193,11 +1184,7 @@ impl RoundManager { /// potential attacks). /// 2. Add the vote to the pending votes and check whether it finishes a QC. /// 3. Once the QC/TC successfully formed, notify the RoundState. - pub async fn process_vote_msg( - &mut self, - vote_msg: VoteMsg, - verification_status: VerificationStatus, - ) -> anyhow::Result<()> { + pub async fn process_vote_msg(&mut self, vote_msg: VoteMsg) -> anyhow::Result<()> { fail_point!("consensus::process_vote_msg", |_| { Err(anyhow::anyhow!("Injected error in process_vote_msg")) }); @@ -1211,7 +1198,7 @@ impl RoundManager { .await .context("[RoundManager] Stop processing vote")? { - self.process_vote(vote_msg.vote(), verification_status) + self.process_vote(vote_msg.vote()) .await .context("[RoundManager] Add a new vote")?; } @@ -1222,11 +1209,7 @@ impl RoundManager { /// If a new QC / TC is formed then /// 1) fetch missing dependencies if required, and then /// 2) call process_certificates(), which will start a new round in return. - async fn process_vote( - &mut self, - vote: &Vote, - verification_status: VerificationStatus, - ) -> anyhow::Result<()> { + async fn process_vote(&mut self, vote: &Vote) -> anyhow::Result<()> { let round = vote.vote_data().proposed().round(); if vote.is_timeout() { @@ -1271,9 +1254,7 @@ impl RoundManager { { return Ok(()); } - let vote_reception_result = - self.round_state - .insert_vote(vote, self.epoch_state.clone(), verification_status); + let vote_reception_result = self.round_state.insert_vote(vote, self.epoch_state.clone()); self.process_vote_reception_result(vote, vote_reception_result) .await } @@ -1583,16 +1564,10 @@ impl RoundManager { (peer_id, event) = event_rx.select_next_some() => { let result = match event { VerifiedEvent::VoteMsg(vote_msg) => { - monitor!("process_vote", self.process_vote_msg(*vote_msg, VerificationStatus::Verified).await) + monitor!("process_vote", self.process_vote_msg(*vote_msg).await) } VerifiedEvent::OrderVoteMsg(order_vote_msg) => { - monitor!("process_order_vote", self.process_order_vote_msg(*order_vote_msg, VerificationStatus::Verified).await) - } - VerifiedEvent::UnverifiedVoteMsg(vote_msg) => { - monitor!("process_unverified_vote", self.process_vote_msg(*vote_msg, VerificationStatus::Unverified).await) - } - VerifiedEvent::UnverifiedOrderVoteMsg(order_vote_msg) => { - monitor!("process_unverified_order_vote", self.process_order_vote_msg(*order_vote_msg, VerificationStatus::Unverified).await) + monitor!("process_order_vote", self.process_order_vote_msg(*order_vote_msg).await) } VerifiedEvent::UnverifiedSyncInfo(sync_info) => { monitor!( diff --git a/consensus/src/round_manager_test.rs b/consensus/src/round_manager_test.rs index 762442c8d05ca..9fdc5c06a26a9 100644 --- a/consensus/src/round_manager_test.rs +++ b/consensus/src/round_manager_test.rs @@ -66,7 +66,7 @@ use aptos_secure_storage::Storage; use aptos_types::{ epoch_state::EpochState, jwks::QuorumCertifiedUpdate, - ledger_info::{LedgerInfo, VerificationStatus}, + ledger_info::LedgerInfo, on_chain_config::{ ConsensusAlgorithmConfig, ConsensusConfigV1, OnChainConsensusConfig, OnChainJWKConsensusConfig, OnChainRandomnessConfig, ValidatorTxnConfig, @@ -637,11 +637,10 @@ 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().signature().set_verified(); timed_block_on( runtime, - proposer_node - .round_manager - .process_vote_msg(vote_msg, VerificationStatus::Verified), + proposer_node.round_manager.process_vote_msg(vote_msg), ) .unwrap(); } @@ -690,11 +689,9 @@ fn new_round_on_quorum_cert() { .await .unwrap(); let vote_msg = node.next_vote().await; + vote_msg.vote().signature().set_verified(); // Adding vote to form a QC - node.round_manager - .process_vote_msg(vote_msg, VerificationStatus::Verified) - .await - .unwrap(); + node.round_manager.process_vote_msg(vote_msg).await.unwrap(); // round 2 should start let proposal_msg = node.next_proposal().await; @@ -1553,7 +1550,7 @@ fn sync_on_partial_newer_sync_info() { runtime.spawn(playground.start()); timed_block_on(&runtime, async { // commit block 1 after 4 rounds - for _ in 1..=4 { + for i in 1..=4 { let proposal_msg = node.next_proposal().await; node.round_manager @@ -1561,11 +1558,11 @@ fn sync_on_partial_newer_sync_info() { .await .unwrap(); let vote_msg = node.next_vote().await; + if i < 2 { + vote_msg.vote().signature().set_verified(); + } // Adding vote to form a QC - node.round_manager - .process_vote_msg(vote_msg, VerificationStatus::Verified) - .await - .unwrap(); + node.round_manager.process_vote_msg(vote_msg).await.unwrap(); } let block_4 = node.next_proposal().await; node.round_manager @@ -1660,10 +1657,8 @@ fn safety_rules_crash() { // sign proposal reset_safety_rules(&mut node); - node.round_manager - .process_vote_msg(vote_msg, VerificationStatus::Verified) - .await - .unwrap(); + vote_msg.vote().signature().set_verified(); + node.round_manager.process_vote_msg(vote_msg).await.unwrap(); } // verify the last sign proposal happened @@ -1702,10 +1697,10 @@ 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; - let result = node_0 - .round_manager - .process_vote_msg(timeout_vote, VerificationStatus::Verified) - .await; + if i < 2 { + timeout_vote.vote().signature().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 { assert!(result.is_ok()); @@ -1718,11 +1713,16 @@ 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 _ in 0..4 { + for i 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().signature().set_verified(); + } node_1 .round_manager - .process_vote_msg(timeout_vote, VerificationStatus::Verified) + .process_vote_msg(timeout_vote) .await .unwrap(); } @@ -2041,11 +2041,12 @@ pub fn forking_retrieval_test() { } let vote_msg_on_timeout = node.next_vote().await; + vote_msg_on_timeout.vote().signature().set_verified(); assert!(vote_msg_on_timeout.vote().is_timeout()); if node.id != behind_node { let result = node .round_manager - .process_vote_msg(vote_msg_on_timeout, VerificationStatus::Verified) + .process_vote_msg(vote_msg_on_timeout) .await; if node.id == forking_node && i == 2 { diff --git a/types/src/ledger_info.rs b/types/src/ledger_info.rs index 1406f56dfc784..075ac353f36b0 100644 --- a/types/src/ledger_info.rs +++ b/types/src/ledger_info.rs @@ -14,6 +14,8 @@ use crate::{ }; use aptos_crypto::{bls12381, hash::HashValue}; use aptos_crypto_derive::{BCSCryptoHash, CryptoHasher}; +use aptos_infallible::RwLock; +use derivative::Derivative; #[cfg(any(test, feature = "fuzzing"))] use proptest_derive::Arbitrary; use rayon::iter::{IntoParallelIterator, ParallelIterator}; @@ -376,11 +378,52 @@ impl LedgerInfoWithVerifiedSignatures { } } +#[derive(Clone, Debug, PartialEq, Eq)] pub enum VerificationStatus { Verified, Unverified, } +#[derive(Clone, Debug, Derivative, Serialize)] +#[derivative(PartialEq, Eq)] +pub struct SignatureWithStatus { + signature: bls12381::Signature, + #[serde(skip)] + #[derivative(PartialEq = "ignore")] + status: Arc>, +} + +impl SignatureWithStatus { + pub fn set_verified(&self) { + let mut status = self.status.write(); + *status = VerificationStatus::Verified; + } + + pub fn signature(&self) -> &bls12381::Signature { + &self.signature + } + + pub fn from(signature: bls12381::Signature) -> Self { + Self { + signature, + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + } + } +} + +impl<'de> Deserialize<'de> for SignatureWithStatus { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let signature = bls12381::Signature::deserialize(deserializer)?; + Ok(SignatureWithStatus { + signature, + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + }) + } +} + /// This data structure is used to support the optimistic signature verification feature. /// Contains the ledger info and the signatures received on the ledger info from different validators. /// Some of the signatures could be verified before inserting into this data structure. Some of the signatures @@ -439,12 +482,15 @@ impl LedgerInfoWithUnverifiedSignatures { pub fn add_signature( &mut self, validator: AccountAddress, - signature: bls12381::Signature, - verification_status: VerificationStatus, + signature_with_status: SignatureWithStatus, ) { - match verification_status { - VerificationStatus::Verified => self.add_verified_signature(validator, signature), - VerificationStatus::Unverified => self.add_unverified_signature(validator, signature), + match *signature_with_status.status.read() { + VerificationStatus::Verified => { + self.add_verified_signature(validator, signature_with_status.signature) + }, + VerificationStatus::Unverified => { + self.add_unverified_signature(validator, signature_with_status.signature) + }, }; } @@ -591,6 +637,29 @@ mod tests { use super::*; use crate::{validator_signer::ValidatorSigner, validator_verifier::ValidatorConsensusInfo}; + // Write a test case to serialize and deserialize SignatureWithStatus + #[test] + fn test_signature_with_status_serde() { + let signature = bls12381::Signature::dummy_signature(); + let signature_with_status = SignatureWithStatus { + signature: signature.clone(), + status: Arc::new(RwLock::new(VerificationStatus::Verified)), + }; + let serialized_signature_with_status = + bcs::to_bytes(&signature_with_status).expect("Failed to serialize signature"); + let deserialized_signature_with_status: SignatureWithStatus = + bcs::from_bytes(&serialized_signature_with_status) + .expect("Failed to deserialize signature"); + assert_eq!( + deserialized_signature_with_status.signature(), + signature_with_status.signature() + ); + assert_eq!( + *deserialized_signature_with_status.status.read(), + VerificationStatus::Unverified + ); + } + #[test] fn test_signatures_hash() { let ledger_info = LedgerInfo::new(BlockInfo::empty(), HashValue::random()); @@ -679,8 +748,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[0].author(), - validator_signers[0].sign(&ledger_info).unwrap(), - VerificationStatus::Verified, + SignatureWithStatus { + signature: validator_signers[0].sign(&ledger_info).unwrap(), + status: Arc::new(RwLock::new(VerificationStatus::Verified)), + }, ); partial_sig.add_signature( validator_signers[0].author(), @@ -689,8 +760,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[1].author(), - validator_signers[1].sign(&ledger_info).unwrap(), - VerificationStatus::Unverified, + SignatureWithStatus { + signature: validator_signers[1].sign(&ledger_info).unwrap(), + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + }, ); partial_sig.add_signature( validator_signers[1].author(), @@ -699,8 +772,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[2].author(), - validator_signers[2].sign(&ledger_info).unwrap(), - VerificationStatus::Verified, + SignatureWithStatus { + signature: validator_signers[2].sign(&ledger_info).unwrap(), + status: Arc::new(RwLock::new(VerificationStatus::Verified)), + }, ); partial_sig.add_signature( validator_signers[2].author(), @@ -709,8 +784,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[3].author(), - validator_signers[3].sign(&ledger_info).unwrap(), - VerificationStatus::Unverified, + SignatureWithStatus { + signature: validator_signers[3].sign(&ledger_info).unwrap(), + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + }, ); partial_sig.add_signature( validator_signers[3].author(), @@ -742,8 +819,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[4].author(), - bls12381::Signature::dummy_signature(), - VerificationStatus::Unverified, + SignatureWithStatus { + signature: bls12381::Signature::dummy_signature(), + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + }, ); assert_eq!(ledger_info_with_mixed_signatures.all_voters().count(), 5); @@ -793,8 +872,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[5].author(), - validator_signers[5].sign(&ledger_info).unwrap(), - VerificationStatus::Unverified, + SignatureWithStatus { + signature: validator_signers[5].sign(&ledger_info).unwrap(), + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + }, ); partial_sig.add_signature( validator_signers[5].author(), @@ -852,8 +933,10 @@ mod tests { ledger_info_with_mixed_signatures.add_signature( validator_signers[6].author(), - bls12381::Signature::dummy_signature(), - VerificationStatus::Unverified, + SignatureWithStatus { + signature: bls12381::Signature::dummy_signature(), + status: Arc::new(RwLock::new(VerificationStatus::Unverified)), + }, ); assert_eq!(ledger_info_with_mixed_signatures.all_voters().count(), 6);