diff --git a/crates/task-impls/src/helpers.rs b/crates/task-impls/src/helpers.rs index a3cd151971..add742dd19 100644 --- a/crates/task-impls/src/helpers.rs +++ b/crates/task-impls/src/helpers.rs @@ -236,21 +236,35 @@ impl Default for LeafChainTraversalOutcome { /// /// Upon receipt then of a proposal for view 9, assuming it is valid, this entire process will repeat, and /// the anchor view will be set to view 6, with the locked view as view 7. -pub async fn decide_from_proposal( - proposal: &QuorumProposal, +pub async fn decide_from_high_qc( consensus: OuterConsensus, existing_upgrade_cert: Arc>>>, public_key: &TYPES::SignatureKey, ) -> LeafChainTraversalOutcome { + let mut res = LeafChainTraversalOutcome::default(); let consensus_reader = consensus.read().await; let existing_upgrade_cert_reader = existing_upgrade_cert.read().await; - let view_number = proposal.view_number(); - let parent_view_number = proposal.justify_qc.view_number(); + let high_qc = consensus_reader.high_qc(); + let view_number = high_qc.view_number(); + let Some(high_qc_leaf) = consensus_reader + .saved_leaves() + .get(&high_qc.data().leaf_commit) + else { + tracing::warn!("Leaf ascension failed; No leaf corresponding to high QC found"); + return res; + }; + let Some(parent_leaf) = consensus_reader + .saved_leaves() + .get(&high_qc_leaf.parent_commitment()) + else { + tracing::warn!("Leaf ascension failed; No leaf corresponding to high QC's parent found"); + return res; + }; + let parent_view_number = parent_leaf.view_number(); let old_anchor_view = consensus_reader.last_decided_view(); let mut last_view_number_visited = view_number; - let mut current_chain_length = 0usize; - let mut res = LeafChainTraversalOutcome::default(); + let mut current_chain_length = 1usize; if let Err(e) = consensus_reader.visit_leaf_ancestors( parent_view_number, diff --git a/crates/task-impls/src/quorum_proposal_recv/handlers.rs b/crates/task-impls/src/quorum_proposal_recv/handlers.rs index 3d1957c84a..91af7e7aaf 100644 --- a/crates/task-impls/src/quorum_proposal_recv/handlers.rs +++ b/crates/task-impls/src/quorum_proposal_recv/handlers.rs @@ -193,32 +193,8 @@ pub(crate) async fn handle_quorum_proposal_recv< } None => None, }; - - if justify_qc.view_number() > consensus_read.high_qc().view_number { - if let Err(e) = task_state - .storage - .write() - .await - .update_high_qc(justify_qc.clone()) - .await - { - bail!("Failed to store High QC, not voting; error = {:?}", e); - } - } drop(consensus_read); - let mut consensus_write = task_state.consensus.write().await; - if let Err(e) = consensus_write.update_high_qc(justify_qc.clone()) { - tracing::trace!("{e:?}"); - } - drop(consensus_write); - - broadcast_event( - HotShotEvent::HighQcUpdated(justify_qc.clone()).into(), - event_sender, - ) - .await; - let Some((parent_leaf, _parent_state)) = parent else { tracing::warn!( "Proposal's parent missing from storage with commitment: {:?}", diff --git a/crates/task-impls/src/quorum_vote/handlers.rs b/crates/task-impls/src/quorum_vote/handlers.rs index 656737524f..fbc0f41d5c 100644 --- a/crates/task-impls/src/quorum_vote/handlers.rs +++ b/crates/task-impls/src/quorum_vote/handlers.rs @@ -6,6 +6,7 @@ use std::sync::Arc; +use anyhow::{bail, Result}; use async_broadcast::Sender; use chrono::Utc; use hotshot_types::{ @@ -19,12 +20,11 @@ use hotshot_types::{ vote::HasViewNumber, }; use tracing::instrument; -use utils::anytrace::*; use super::QuorumVoteTaskState; use crate::{ events::HotShotEvent, - helpers::{broadcast_event, decide_from_proposal, LeafChainTraversalOutcome}, + helpers::{broadcast_event, decide_from_high_qc, LeafChainTraversalOutcome}, quorum_vote::Versions, }; @@ -39,6 +39,33 @@ pub(crate) async fn handle_quorum_proposal_validated< sender: &Sender>>, task_state: &mut QuorumVoteTaskState, ) -> Result<()> { + let consensus_read = task_state.consensus.read().await; + let justify_qc = proposal.justify_qc.clone(); + if justify_qc.view_number() > consensus_read.high_qc().view_number { + if let Err(e) = task_state + .storage + .write() + .await + .update_high_qc(justify_qc.clone()) + .await + { + bail!("Failed to store High QC, not voting; error = {:?}", e); + } + } + drop(consensus_read); + + let mut consensus_write = task_state.consensus.write().await; + if let Err(e) = consensus_write.update_high_qc(justify_qc.clone()) { + tracing::trace!("{e:?}"); + } + drop(consensus_write); + + broadcast_event( + HotShotEvent::HighQcUpdated(justify_qc.clone()).into(), + sender, + ) + .await; + let LeafChainTraversalOutcome { new_locked_view_number, new_decided_view_number, @@ -47,8 +74,7 @@ pub(crate) async fn handle_quorum_proposal_validated< leaves_decided, included_txns, decided_upgrade_cert, - } = decide_from_proposal( - proposal, + } = decide_from_high_qc( OuterConsensus::new(Arc::clone(&task_state.consensus.inner_consensus)), Arc::clone(&task_state.upgrade_lock.decided_upgrade_certificate), &task_state.public_key, diff --git a/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs b/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs index 55ef973f9d..cb3941ccbd 100644 --- a/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs +++ b/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs @@ -98,7 +98,6 @@ async fn test_quorum_proposal_recv_task() { let expectations = vec![Expectations::from_outputs(vec![ exact(QuorumProposalPreliminarilyValidated(proposals[1].clone())), - exact(HighQcUpdated(proposals[1].data.justify_qc.clone())), exact(ValidatedStateUpdated( ViewNumber::new(2), build_fake_view_with_leaf_and_state( @@ -234,7 +233,6 @@ async fn test_quorum_proposal_recv_task_liveness_check() { .await, )), exact(QuorumProposalRequestSend(req, signature)), - exact(HighQcUpdated(proposals[2].data.justify_qc.clone())), ])]; let state = diff --git a/crates/testing/tests/tests_1/quorum_vote_task.rs b/crates/testing/tests/tests_1/quorum_vote_task.rs index e981ed9bda..547ab5c922 100644 --- a/crates/testing/tests/tests_1/quorum_vote_task.rs +++ b/crates/testing/tests/tests_1/quorum_vote_task.rs @@ -85,6 +85,7 @@ async fn test_quorum_vote_task_success() { exact(VidShareValidated(vids[1].0[0].clone())), exact(QuorumVoteDependenciesValidated(ViewNumber::new(2))), validated_state_updated(), + exact(HighQcUpdated(proposals[1].data.justify_qc.clone())), quorum_vote_send(), ])]; @@ -164,10 +165,12 @@ async fn test_quorum_vote_task_miss_dependency() { ]; let expectations = vec![ - Expectations::from_outputs(all_predicates![exact(VidShareValidated( - vids[1].0[0].clone() - ))]), Expectations::from_outputs(all_predicates![ + exact(VidShareValidated(vids[1].0[0].clone())), + exact(HighQcUpdated(proposals[1].data.justify_qc.clone())), + ]), + Expectations::from_outputs(all_predicates![ + exact(HighQcUpdated(proposals[2].data.justify_qc.clone())), exact(LockedViewUpdated(ViewNumber::new(1))), exact(DaCertificateValidated(dacs[2].clone())) ]), @@ -231,6 +234,7 @@ async fn test_quorum_vote_task_incorrect_dependency() { let expectations = vec![Expectations::from_outputs(all_predicates![ exact(DaCertificateValidated(dacs[1].clone())), exact(VidShareValidated(vids[0].0[0].clone())), + exact(HighQcUpdated(proposals[1].data.justify_qc.clone())), ])]; let quorum_vote_state = diff --git a/crates/testing/tests/tests_1/upgrade_task_with_vote.rs b/crates/testing/tests/tests_1/upgrade_task_with_vote.rs index 88112683a5..fd6049e44a 100644 --- a/crates/testing/tests/tests_1/upgrade_task_with_vote.rs +++ b/crates/testing/tests/tests_1/upgrade_task_with_vote.rs @@ -139,10 +139,12 @@ async fn test_upgrade_task_with_vote() { exact(VidShareValidated(vids[1].0[0].clone())), exact(QuorumVoteDependenciesValidated(ViewNumber::new(2))), validated_state_updated(), + exact(HighQcUpdated(proposals[1].data.justify_qc.clone())), quorum_vote_send(), ]), Expectations::from_outputs_and_task_states( all_predicates![ + exact(HighQcUpdated(proposals[2].data.justify_qc.clone())), exact(LockedViewUpdated(ViewNumber::new(1))), exact(DaCertificateValidated(dacs[2].clone())), exact(VidShareValidated(vids[2].0[0].clone())), @@ -154,6 +156,7 @@ async fn test_upgrade_task_with_vote() { ), Expectations::from_outputs_and_task_states( all_predicates![ + exact(HighQcUpdated(proposals[3].data.justify_qc.clone())), exact(LockedViewUpdated(ViewNumber::new(2))), exact(LastDecidedViewUpdated(ViewNumber::new(1))), leaf_decided(), @@ -167,6 +170,7 @@ async fn test_upgrade_task_with_vote() { ), Expectations::from_outputs_and_task_states( all_predicates![ + exact(HighQcUpdated(proposals[4].data.justify_qc.clone())), exact(LockedViewUpdated(ViewNumber::new(3))), exact(LastDecidedViewUpdated(ViewNumber::new(2))), leaf_decided(), @@ -180,6 +184,7 @@ async fn test_upgrade_task_with_vote() { ), Expectations::from_outputs_and_task_states( all_predicates![ + exact(HighQcUpdated(proposals[5].data.justify_qc.clone())), exact(LockedViewUpdated(ViewNumber::new(4))), exact(LastDecidedViewUpdated(ViewNumber::new(3))), leaf_decided(),