From 04a1dc8b07488163611f3e91016a37a3f4b7a2b4 Mon Sep 17 00:00:00 2001 From: Balaji Arun Date: Wed, 9 Oct 2024 13:37:24 -0700 Subject: [PATCH] address feedback --- consensus/src/block_storage/block_store.rs | 9 +++------ consensus/src/liveness/round_state.rs | 2 +- consensus/src/pending_votes.rs | 6 ++---- consensus/src/quorum_store/batch_proof_queue.rs | 4 ++-- .../src/quorum_store/tests/proof_manager_test.rs | 11 ++--------- consensus/src/round_manager.rs | 3 ++- testsuite/generate-format/tests/staged/consensus.yaml | 7 +++++++ 7 files changed, 19 insertions(+), 23 deletions(-) diff --git a/consensus/src/block_storage/block_store.rs b/consensus/src/block_storage/block_store.rs index e0a43c830be27..f56dfa80c23eb 100644 --- a/consensus/src/block_storage/block_store.rs +++ b/consensus/src/block_storage/block_store.rs @@ -473,12 +473,9 @@ impl BlockStore { self.pending_blocks.clone() } - pub async fn wait_for_payload(&self, block: &Block) -> anyhow::Result<()> { - tokio::time::timeout( - Duration::from_secs(1), - self.payload_manager.get_transactions(block), - ) - .await??; + pub async fn wait_for_payload(&self, block: &Block, deadline: Duration) -> anyhow::Result<()> { + let duration = deadline.saturating_sub(self.time_service.get_current_timestamp()); + tokio::time::timeout(duration, self.payload_manager.get_transactions(block)).await??; Ok(()) } diff --git a/consensus/src/liveness/round_state.rs b/consensus/src/liveness/round_state.rs index 124d4ebc15418..2c7ea4c198da6 100644 --- a/consensus/src/liveness/round_state.rs +++ b/consensus/src/liveness/round_state.rs @@ -272,7 +272,7 @@ impl RoundState { NewRoundReason::QCReady } else { let prev_round_timeout_reason = - prev_round_timeout_reason.expect("TC must be present for timeout round."); + prev_round_timeout_reason.unwrap_or(RoundTimeoutReason::Unknown); NewRoundReason::Timeout(prev_round_timeout_reason) }; diff --git a/consensus/src/pending_votes.rs b/consensus/src/pending_votes.rs index eb60f36354425..e6651681766b3 100644 --- a/consensus/src/pending_votes.rs +++ b/consensus/src/pending_votes.rs @@ -91,10 +91,6 @@ impl TwoChainTimeoutVotes { &mut self.partial_2chain_tc } - pub(super) fn partial_2chain_tc(&self) -> &TwoChainTimeoutWithPartialSignatures { - &self.partial_2chain_tc - } - fn aggregated_timeout_reason(&self, verifier: &ValidatorVerifier) -> RoundTimeoutReason { let mut reason_voting_power: HashMap = HashMap::new(); let mut missing_batch_authors: HashMap = HashMap::new(); @@ -112,6 +108,8 @@ impl TwoChainTimeoutVotes { verifier.get_voting_power(author).unwrap_or_default() as u128; } RoundTimeoutReason::PayloadUnavailable { + // Since we care only about the variant type, we replace the bitvec + // with a placeholder. missing_authors: BitVec::with_num_bits(verifier.len() as u16), } }, diff --git a/consensus/src/quorum_store/batch_proof_queue.rs b/consensus/src/quorum_store/batch_proof_queue.rs index 937327a868f64..c542d97d7d9e3 100644 --- a/consensus/src/quorum_store/batch_proof_queue.rs +++ b/consensus/src/quorum_store/batch_proof_queue.rs @@ -532,7 +532,7 @@ impl BatchProofQueue { } } - let max_batch_ts_usecs = min_batch_age_usecs + let max_batch_creation_ts_usecs = min_batch_age_usecs .map(|min_age| aptos_infallible::duration_since_epoch().as_micros() as u64 - min_age); let mut iters = vec![]; for (_, batches) in self @@ -547,7 +547,7 @@ impl BatchProofQueue { // Ensure that the batch was created at least `min_batch_age_usecs` ago to // reduce the chance of inline fetches. - if max_batch_ts_usecs + if max_batch_creation_ts_usecs .is_some_and(|max_create_ts| batch_create_ts_usecs > max_create_ts) { return None; diff --git a/consensus/src/quorum_store/tests/proof_manager_test.rs b/consensus/src/quorum_store/tests/proof_manager_test.rs index f19501d416414..3eebe4c667937 100644 --- a/consensus/src/quorum_store/tests/proof_manager_test.rs +++ b/consensus/src/quorum_store/tests/proof_manager_test.rs @@ -13,18 +13,11 @@ use aptos_consensus_types::{ use aptos_crypto::HashValue; use aptos_types::{aggregate_signature::AggregateSignature, PeerId}; use futures::channel::oneshot; -use std::{cmp::max, collections::HashSet, time::Duration}; +use std::{cmp::max, collections::HashSet}; fn create_proof_manager() -> ProofManager { let batch_store = batch_store_for_test(5 * 1024 * 1024); - ProofManager::new( - PeerId::random(), - 10, - 10, - batch_store, - true, - Duration::from_secs(60).as_micros() as u64, - ) + ProofManager::new(PeerId::random(), 10, 10, batch_store, true, 1) } fn create_proof(author: PeerId, expiration: u64, batch_sequence: u64) -> ProofOfStore { diff --git a/consensus/src/round_manager.rs b/consensus/src/round_manager.rs index 56c95f02316f4..3c8cdb6993159 100644 --- a/consensus/src/round_manager.rs +++ b/consensus/src/round_manager.rs @@ -991,9 +991,10 @@ impl RoundManager { .with_label_values(&["missing"]) .inc(); let start_time = Instant::now(); + let deadline = self.round_state.current_round_deadline(); let future = async move { ( - block_store.wait_for_payload(&proposal).await, + block_store.wait_for_payload(&proposal, deadline).await, proposal, start_time, ) diff --git a/testsuite/generate-format/tests/staged/consensus.yaml b/testsuite/generate-format/tests/staged/consensus.yaml index 1e81da1a2692b..f9bb0aaf27da6 100644 --- a/testsuite/generate-format/tests/staged/consensus.yaml +++ b/testsuite/generate-format/tests/staged/consensus.yaml @@ -854,6 +854,13 @@ RoundTimeoutReason: Unknown: UNIT 1: ProposalNotReceived: UNIT + 2: + PayloadUnavailable: + STRUCT: + - missing_authors: + TYPENAME: BitVec + 3: + NoQC: UNIT Script: STRUCT: - code: BYTES