Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring ledger info with unverified signatures #15023

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Refactoring ledger info with unverified signatures #15023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04ccbad
Refactoring ledger info with unverified signatures
vusirikala Oct 20, 2024
a112d3d
Minor comment
vusirikala Oct 20, 2024
580bbb3
Address PR comments
vusirikala Oct 21, 2024
ff11a5b
Merge branch 'main' into satya/refactor_sig_aggregator
vusirikala Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions consensus/consensus-types/src/order_vote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
7 changes: 0 additions & 7 deletions consensus/consensus-types/src/vote.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 0 additions & 2 deletions consensus/src/block_storage/block_store_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand Down
33 changes: 17 additions & 16 deletions consensus/src/pending_order_votes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -33,7 +33,7 @@ pub enum OrderVoteReceptionResult {
#[derive(Debug, PartialEq, Eq)]
enum OrderVoteStatus {
EnoughVotes(LedgerInfoWithSignatures),
NotEnoughVotes(LedgerInfoWithUnverifiedSignatures),
NotEnoughVotes(SignatureAggregator<LedgerInfo>),
}

/// A PendingVotes structure keep track of order votes for the last few rounds
Expand Down Expand Up @@ -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(),
)),
)
Expand All @@ -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());
Expand All @@ -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(),
Expand All @@ -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) => {
Expand Down Expand Up @@ -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
},
});
}
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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());
Expand Down Expand Up @@ -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)
Expand All @@ -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.");
Expand Down
42 changes: 18 additions & 24 deletions consensus/src/pending_votes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -59,7 +59,7 @@ pub enum VoteReceptionResult {
#[derive(Debug, PartialEq, Eq)]
pub enum VoteStatus {
EnoughVotes(LedgerInfoWithSignatures),
NotEnoughVotes(LedgerInfoWithUnverifiedSignatures),
NotEnoughVotes(SignatureAggregator<LedgerInfo>),
}

#[derive(Debug)]
Expand Down Expand Up @@ -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())),
)
});

Expand Down Expand Up @@ -366,12 +364,12 @@ impl PendingVotes {
li_with_sig.clone(),
)));
},
VoteStatus::NotEnoughVotes(li_with_sig) => {
// add this vote to the ledger info with signatures
li_with_sig.add_signature(vote.author(), vote.signature_with_status());
VoteStatus::NotEnoughVotes(sig_aggregator) => {
// 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
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!(
Expand All @@ -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) => {
Expand Down Expand Up @@ -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(),
)?;
},
}
Expand Down Expand Up @@ -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),
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.");
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand Down
Loading
Loading