From 3f8207a60c2a2a466fc62553b851fd7803c2af30 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 22 Oct 2024 22:29:10 -0400 Subject: [PATCH 1/9] move storage to end of proposal processing --- crates/task-impls/src/events.rs | 18 ++++++++-------- crates/task-impls/src/helpers.rs | 12 +---------- crates/task-impls/src/quorum_vote/mod.rs | 21 +++++++++++++------ crates/task-impls/src/request.rs | 2 +- .../src/byzantine/byzantine_behaviour.rs | 2 +- .../tests_1/quorum_proposal_recv_task.rs | 2 +- .../testing/tests/tests_1/quorum_vote_task.rs | 8 +++---- .../tests/tests_1/upgrade_task_with_vote.rs | 10 ++++----- .../tests/tests_1/vote_dependency_handle.rs | 2 +- 9 files changed, 38 insertions(+), 39 deletions(-) diff --git a/crates/task-impls/src/events.rs b/crates/task-impls/src/events.rs index 8fc85a6031..4aea9b5ec8 100644 --- a/crates/task-impls/src/events.rs +++ b/crates/task-impls/src/events.rs @@ -100,7 +100,7 @@ pub enum HotShotEvent { /// 2. The proposal has been correctly signed by the leader of the current view /// 3. The justify QC is valid /// 4. The proposal passes either liveness or safety check. - QuorumProposalValidated(QuorumProposal, Leaf), + QuorumProposalValidated(Proposal>, Leaf), /// A quorum proposal is missing for a view that we need. QuorumProposalRequestSend( ProposalRequestPayload, @@ -267,9 +267,14 @@ impl HotShotEvent { Some(v.view_number()) } HotShotEvent::QuorumProposalRecv(proposal, _) - | HotShotEvent::QuorumProposalSend(proposal, _) => Some(proposal.data.view_number()), + | HotShotEvent::QuorumProposalSend(proposal, _) + | HotShotEvent::QuorumProposalValidated(proposal, _) + | HotShotEvent::QuorumProposalResponseSend(_, proposal) + | HotShotEvent::QuorumProposalResponseRecv(proposal) + | HotShotEvent::QuorumProposalPreliminarilyValidated(proposal) => { + Some(proposal.data.view_number()) + } HotShotEvent::QuorumVoteSend(vote) => Some(vote.view_number()), - HotShotEvent::QuorumProposalValidated(proposal, _) => Some(proposal.view_number()), HotShotEvent::DaProposalRecv(proposal, _) | HotShotEvent::DaProposalValidated(proposal, _) | HotShotEvent::DaProposalSend(proposal, _) => Some(proposal.data.view_number()), @@ -311,11 +316,6 @@ impl HotShotEvent { } HotShotEvent::QuorumProposalRequestSend(req, _) | HotShotEvent::QuorumProposalRequestRecv(req, _) => Some(req.view_number), - HotShotEvent::QuorumProposalResponseSend(_, proposal) - | HotShotEvent::QuorumProposalResponseRecv(proposal) - | HotShotEvent::QuorumProposalPreliminarilyValidated(proposal) => { - Some(proposal.data.view_number()) - } HotShotEvent::QuorumVoteDependenciesValidated(view_number) | HotShotEvent::ViewChange(view_number) | HotShotEvent::ViewSyncTimeout(view_number, _, _) @@ -398,7 +398,7 @@ impl Display for HotShotEvent { HotShotEvent::QuorumProposalValidated(proposal, _) => write!( f, "QuorumProposalValidated(view_number={:?})", - proposal.view_number() + proposal.data.view_number() ), HotShotEvent::DaProposalSend(proposal, _) => write!( f, diff --git a/crates/task-impls/src/helpers.rs b/crates/task-impls/src/helpers.rs index cd4bf5b3cd..d9414cf236 100644 --- a/crates/task-impls/src/helpers.rs +++ b/crates/task-impls/src/helpers.rs @@ -31,7 +31,6 @@ use hotshot_types::{ election::Membership, node_implementation::{ConsensusTime, NodeImplementation, NodeType, Versions}, signature_key::SignatureKey, - storage::Storage, BlockPayload, ValidatedState, }, utils::{Terminator, View, ViewInner}, @@ -552,15 +551,6 @@ pub async fn validate_proposal_safety_and_liveness< }); } - // Update our persistent storage of the proposal. If we cannot store the proposal reutrn - // and error so we don't vote - task_state - .storage - .write() - .await - .append_proposal(&proposal) - .await?; - // We accept the proposal, notify the application layer broadcast_event( Event { @@ -577,7 +567,7 @@ pub async fn validate_proposal_safety_and_liveness< // Notify other tasks broadcast_event( Arc::new(HotShotEvent::QuorumProposalValidated( - proposal.data.clone(), + proposal.clone(), parent_leaf, )), &event_stream, diff --git a/crates/task-impls/src/quorum_vote/mod.rs b/crates/task-impls/src/quorum_vote/mod.rs index 7d0fa4833b..a2d638ebb7 100644 --- a/crates/task-impls/src/quorum_vote/mod.rs +++ b/crates/task-impls/src/quorum_vote/mod.rs @@ -280,7 +280,7 @@ impl + 'static, V: Versions> Handl match event.as_ref() { #[allow(unused_assignments)] HotShotEvent::QuorumProposalValidated(proposal, parent_leaf) => { - let proposal_payload_comm = proposal.block_header.payload_commitment(); + let proposal_payload_comm = proposal.data.block_header.payload_commitment(); if let Some(comm) = payload_commitment { if proposal_payload_comm != comm { error!("Quorum proposal has inconsistent payload commitment with DAC or VID."); @@ -290,11 +290,17 @@ impl + 'static, V: Versions> Handl payload_commitment = Some(proposal_payload_comm); } let parent_commitment = parent_leaf.commit(&self.upgrade_lock).await; - let proposed_leaf = Leaf::from_quorum_proposal(proposal); + let proposed_leaf = Leaf::from_quorum_proposal(&proposal.data); if proposed_leaf.parent_commitment() != parent_commitment { warn!("Proposed leaf parent commitment does not match parent leaf payload commitment. Aborting vote."); return; } + // Update our persistent storage of the proposal. If we cannot store the proposal reutrn + // and error so we don't vote + if let Err(e) = self.storage.write().await.append_proposal(proposal).await { + error!("failed to store proposal, not voting. error = {e:#}"); + return; + } leaf = Some(proposed_leaf); } HotShotEvent::DaCertificateValidated(cert) => { @@ -419,7 +425,7 @@ impl, V: Versions> QuorumVoteTaskS let event_view = match dependency_type { VoteDependency::QuorumProposal => { if let HotShotEvent::QuorumProposalValidated(proposal, _) = event { - proposal.view_number + proposal.data.view_number } else { return false; } @@ -543,17 +549,20 @@ impl, V: Versions> QuorumVoteTaskS let current_epoch = self.consensus.read().await.cur_epoch(); match event.as_ref() { HotShotEvent::QuorumProposalValidated(proposal, _leaf) => { - trace!("Received Proposal for view {}", *proposal.view_number()); + trace!( + "Received Proposal for view {}", + *proposal.data.view_number() + ); // Handle the event before creating the dependency task. if let Err(e) = - handle_quorum_proposal_validated(proposal, &event_sender, self).await + handle_quorum_proposal_validated(&proposal.data, &event_sender, self).await { debug!("Failed to handle QuorumProposalValidated event; error = {e:#}"); } self.create_dependency_task_if_new( - proposal.view_number, + proposal.data.view_number, current_epoch, event_receiver, &event_sender, diff --git a/crates/task-impls/src/request.rs b/crates/task-impls/src/request.rs index 8cd336e7b1..ba7508415d 100644 --- a/crates/task-impls/src/request.rs +++ b/crates/task-impls/src/request.rs @@ -96,7 +96,7 @@ impl> TaskState for NetworkRequest ) -> Result<()> { match event.as_ref() { HotShotEvent::QuorumProposalValidated(proposal, _) => { - let prop_view = proposal.view_number(); + let prop_view = proposal.data.view_number(); let current_epoch = self.state.read().await.cur_epoch(); // If we already have the VID shares for the next view, do nothing. diff --git a/crates/testing/src/byzantine/byzantine_behaviour.rs b/crates/testing/src/byzantine/byzantine_behaviour.rs index 825108e48b..e92e33cdd4 100644 --- a/crates/testing/src/byzantine/byzantine_behaviour.rs +++ b/crates/testing/src/byzantine/byzantine_behaviour.rs @@ -186,7 +186,7 @@ impl + std::fmt::Debug, V: Version ]; } HotShotEvent::QuorumProposalValidated(proposal, _) => { - self.validated_proposals.push(proposal.clone()); + self.validated_proposals.push(proposal.data.clone()); } _ => {} } 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..d5a83a1953 100644 --- a/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs +++ b/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs @@ -111,7 +111,7 @@ async fn test_quorum_proposal_recv_task() { .await, )), exact(QuorumProposalValidated( - proposals[1].data.clone(), + proposals[1].clone(), leaves[0].clone(), )), exact(ViewChange(ViewNumber::new(2))), diff --git a/crates/testing/tests/tests_1/quorum_vote_task.rs b/crates/testing/tests/tests_1/quorum_vote_task.rs index e981ed9bda..3030a1aea2 100644 --- a/crates/testing/tests/tests_1/quorum_vote_task.rs +++ b/crates/testing/tests/tests_1/quorum_vote_task.rs @@ -75,7 +75,7 @@ async fn test_quorum_vote_task_success() { // Send the quorum proposal, DAC, VID share data, and validated state, in which case a dummy // vote can be formed and the view number will be updated. let inputs = vec![random![ - QuorumProposalValidated(proposals[1].data.clone(), leaves[0].clone()), + QuorumProposalValidated(proposals[1].clone(), leaves[0].clone()), DaCertificateRecv(dacs[1].clone()), VidShareRecv(leaders[1], vids[1].0[0].clone()), ]]; @@ -150,11 +150,11 @@ async fn test_quorum_vote_task_miss_dependency() { // Send two of quorum proposal, DAC, VID share data, in which case there's no vote. let inputs = vec![ random![ - QuorumProposalValidated(proposals[1].data.clone(), leaves[0].clone()), + QuorumProposalValidated(proposals[1].clone(), leaves[0].clone()), VidShareRecv(leaders[1], vid_share(&vids[1].0, handle.public_key())), ], random![ - QuorumProposalValidated(proposals[2].data.clone(), leaves[1].clone()), + QuorumProposalValidated(proposals[2].clone(), leaves[1].clone()), DaCertificateRecv(dacs[2].clone()), ], random![ @@ -223,7 +223,7 @@ async fn test_quorum_vote_task_incorrect_dependency() { // Send the correct quorum proposal and DAC, and incorrect VID share data. let inputs = vec![random![ - QuorumProposalValidated(proposals[1].data.clone(), leaves[0].clone()), + QuorumProposalValidated(proposals[1].clone(), leaves[0].clone()), DaCertificateRecv(dacs[1].clone()), VidShareRecv(leaders[0], vids[0].0[0].clone()), ]]; 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..212ca114bb 100644 --- a/crates/testing/tests/tests_1/upgrade_task_with_vote.rs +++ b/crates/testing/tests/tests_1/upgrade_task_with_vote.rs @@ -108,27 +108,27 @@ async fn test_upgrade_task_with_vote() { let inputs = vec![ random![ - QuorumProposalValidated(proposals[1].data.clone(), leaves[0].clone()), + QuorumProposalValidated(proposals[1].clone(), leaves[0].clone()), DaCertificateRecv(dacs[1].clone()), VidShareRecv(leaders[1], vids[1].0[0].clone()), ], random![ - QuorumProposalValidated(proposals[2].data.clone(), leaves[1].clone()), + QuorumProposalValidated(proposals[2].clone(), leaves[1].clone()), DaCertificateRecv(dacs[2].clone()), VidShareRecv(leaders[2], vids[2].0[0].clone()), ], random![ - QuorumProposalValidated(proposals[3].data.clone(), leaves[2].clone()), + QuorumProposalValidated(proposals[3].clone(), leaves[2].clone()), DaCertificateRecv(dacs[3].clone()), VidShareRecv(leaders[3], vids[3].0[0].clone()), ], random![ - QuorumProposalValidated(proposals[4].data.clone(), leaves[3].clone()), + QuorumProposalValidated(proposals[4].clone(), leaves[3].clone()), DaCertificateRecv(dacs[4].clone()), VidShareRecv(leaders[4], vids[4].0[0].clone()), ], random![QuorumProposalValidated( - proposals[5].data.clone(), + proposals[5].clone(), leaves[5].clone() ),], ]; diff --git a/crates/testing/tests/tests_1/vote_dependency_handle.rs b/crates/testing/tests/tests_1/vote_dependency_handle.rs index 1a8519ce2c..c216fae2f5 100644 --- a/crates/testing/tests/tests_1/vote_dependency_handle.rs +++ b/crates/testing/tests/tests_1/vote_dependency_handle.rs @@ -70,7 +70,7 @@ async fn test_vote_dependency_handle() { // the dependency handles do not (yet) work with the existing test suite. let all_inputs = vec![ DaCertificateValidated(dacs[1].clone()), - QuorumProposalValidated(proposals[1].data.clone(), leaves[0].clone()), + QuorumProposalValidated(proposals[1].clone(), leaves[0].clone()), VidShareValidated(vids[1].0[0].clone()), ] .into_iter() From d33de66ef116247c2f10a1f6f62221a65970d11f Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 22 Oct 2024 22:46:56 -0400 Subject: [PATCH 2/9] don't fetch proposal so until we really need it --- crates/task-impls/src/helpers.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/crates/task-impls/src/helpers.rs b/crates/task-impls/src/helpers.rs index d9414cf236..28cc8bc634 100644 --- a/crates/task-impls/src/helpers.rs +++ b/crates/task-impls/src/helpers.rs @@ -370,25 +370,6 @@ pub(crate) async fn parent_leaf_and_state( "Somehow we formed a QC but are not the leader for the next view {next_proposal_view_number:?}", ); let parent_view_number = consensus.read().await.high_qc().view_number(); - if !consensus - .read() - .await - .validated_state_map() - .contains_key(&parent_view_number) - { - let _ = fetch_proposal( - parent_view_number, - event_sender.clone(), - event_receiver.clone(), - quorum_membership, - consensus.clone(), - public_key.clone(), - private_key.clone(), - upgrade_lock, - ) - .await - .context("Failed to fetch proposal")?; - } let consensus_reader = consensus.read().await; let parent_view_number = consensus_reader.high_qc().view_number(); let parent_view = consensus_reader.validated_state_map().get(&parent_view_number).context( From 5d8965b2fc66c4f5790be690024138ae169da8a5 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 22 Oct 2024 22:50:04 -0400 Subject: [PATCH 3/9] whoops wrong fetch --- crates/task-impls/src/helpers.rs | 19 +++++++++++++++++ .../src/quorum_proposal_recv/handlers.rs | 21 +------------------ .../src/quorum_proposal_recv/mod.rs | 10 +-------- 3 files changed, 21 insertions(+), 29 deletions(-) diff --git a/crates/task-impls/src/helpers.rs b/crates/task-impls/src/helpers.rs index 28cc8bc634..d9414cf236 100644 --- a/crates/task-impls/src/helpers.rs +++ b/crates/task-impls/src/helpers.rs @@ -370,6 +370,25 @@ pub(crate) async fn parent_leaf_and_state( "Somehow we formed a QC but are not the leader for the next view {next_proposal_view_number:?}", ); let parent_view_number = consensus.read().await.high_qc().view_number(); + if !consensus + .read() + .await + .validated_state_map() + .contains_key(&parent_view_number) + { + let _ = fetch_proposal( + parent_view_number, + event_sender.clone(), + event_receiver.clone(), + quorum_membership, + consensus.clone(), + public_key.clone(), + private_key.clone(), + upgrade_lock, + ) + .await + .context("Failed to fetch proposal")?; + } let consensus_reader = consensus.read().await; let parent_view_number = consensus_reader.high_qc().view_number(); let parent_view = consensus_reader.validated_state_map().get(&parent_view_number).context( diff --git a/crates/task-impls/src/quorum_proposal_recv/handlers.rs b/crates/task-impls/src/quorum_proposal_recv/handlers.rs index 2193bf3dfd..053d01749e 100644 --- a/crates/task-impls/src/quorum_proposal_recv/handlers.rs +++ b/crates/task-impls/src/quorum_proposal_recv/handlers.rs @@ -121,7 +121,6 @@ pub(crate) async fn handle_quorum_proposal_recv< proposal: &Proposal>, quorum_proposal_sender_key: &TYPES::SignatureKey, event_sender: &Sender>>, - event_receiver: &Receiver>>, task_state: &mut QuorumProposalRecvTaskState, ) -> Result<()> { let quorum_proposal_sender_key = quorum_proposal_sender_key.clone(); @@ -155,7 +154,7 @@ pub(crate) async fn handle_quorum_proposal_recv< .await; // Get the parent leaf and state. - let mut parent_leaf = task_state + let parent_leaf = task_state .consensus .read() .await @@ -163,24 +162,6 @@ pub(crate) async fn handle_quorum_proposal_recv< .get(&justify_qc.data.leaf_commit) .cloned(); - parent_leaf = match parent_leaf { - Some(p) => Some(p), - None => fetch_proposal( - justify_qc.view_number(), - event_sender.clone(), - event_receiver.clone(), - Arc::clone(&task_state.quorum_membership), - OuterConsensus::new(Arc::clone(&task_state.consensus.inner_consensus)), - // Note that we explicitly use the node key here instead of the provided key in the signature. - // This is because the key that we receive is for the prior leader, so the payload would be routed - // incorrectly. - task_state.public_key.clone(), - task_state.private_key.clone(), - &task_state.upgrade_lock, - ) - .await - .ok(), - }; let consensus_read = task_state.consensus.read().await; let parent = match parent_leaf { diff --git a/crates/task-impls/src/quorum_proposal_recv/mod.rs b/crates/task-impls/src/quorum_proposal_recv/mod.rs index 281e472fd0..5a69fa0125 100644 --- a/crates/task-impls/src/quorum_proposal_recv/mod.rs +++ b/crates/task-impls/src/quorum_proposal_recv/mod.rs @@ -129,15 +129,7 @@ impl, V: Versions> event_receiver: Receiver>>, ) { if let HotShotEvent::QuorumProposalRecv(proposal, sender) = event.as_ref() { - match handle_quorum_proposal_recv( - proposal, - sender, - &event_sender, - &event_receiver, - self, - ) - .await - { + match handle_quorum_proposal_recv(proposal, sender, &event_sender, self).await { Ok(()) => { self.cancel_tasks(proposal.data.view_number()).await; } From 8c40b12576663a53da89d4552c177e4c618e1dc7 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 22 Oct 2024 23:13:57 -0400 Subject: [PATCH 4/9] fix test --- .../tests/tests_1/quorum_proposal_recv_task.rs | 12 ------------ 1 file changed, 12 deletions(-) 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 d5a83a1953..5ae7777bfb 100644 --- a/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs +++ b/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs @@ -208,17 +208,6 @@ async fn test_quorum_proposal_recv_task_liveness_check() { leaders[2] )]]; - // make the request payload - let req = ProposalRequestPayload { - view_number: ViewNumber::new(2), - key: handle.public_key(), - }; - - // make the signed commitment - let signature = - ::SignatureKey::sign(handle.private_key(), req.commit().as_ref()) - .unwrap(); - let expectations = vec![Expectations::from_outputs(all_predicates![ exact(QuorumProposalPreliminarilyValidated(proposals[2].clone())), exact(ViewChange(ViewNumber::new(3))), @@ -233,7 +222,6 @@ async fn test_quorum_proposal_recv_task_liveness_check() { ) .await, )), - exact(QuorumProposalRequestSend(req, signature)), exact(HighQcUpdated(proposals[2].data.justify_qc.clone())), ])]; From 44ff70ca128a2c5627cc5f44278880ce05d53733 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 22 Oct 2024 23:37:53 -0400 Subject: [PATCH 5/9] spawn fetch proposal --- .../src/quorum_proposal_recv/handlers.rs | 47 +++++++++++++++++++ .../src/quorum_proposal_recv/mod.rs | 10 +++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/crates/task-impls/src/quorum_proposal_recv/handlers.rs b/crates/task-impls/src/quorum_proposal_recv/handlers.rs index 053d01749e..45c982e150 100644 --- a/crates/task-impls/src/quorum_proposal_recv/handlers.rs +++ b/crates/task-impls/src/quorum_proposal_recv/handlers.rs @@ -10,6 +10,7 @@ use std::sync::Arc; use anyhow::{bail, Context, Result}; use async_broadcast::{broadcast, Receiver, Sender}; +use async_compatibility_layer::art::async_spawn; use async_lock::RwLockUpgradableReadGuard; use committable::Committable; use hotshot_types::{ @@ -20,6 +21,7 @@ use hotshot_types::{ traits::{ election::Membership, node_implementation::{NodeImplementation, NodeType}, + signature_key::SignatureKey, storage::Storage, ValidatedState, }, @@ -104,6 +106,35 @@ async fn validate_proposal_liveness( + view: TYPES::View, + event_sender: Sender>>, + event_receiver: Receiver>>, + membership: Arc, + consensus: OuterConsensus, + sender_public_key: TYPES::SignatureKey, + sender_private_key: ::PrivateKey, + upgrade_lock: UpgradeLock, +) { + async_spawn(async move { + let lock = upgrade_lock; + + let _ = fetch_proposal( + view, + event_sender, + event_receiver, + membership, + consensus, + sender_public_key, + sender_private_key, + &lock, + ) + .await; + }); +} + /// Handles the `QuorumProposalRecv` event by first validating the cert itself for the view, and then /// updating the states, which runs when the proposal cannot be found in the internal state map. /// @@ -121,6 +152,7 @@ pub(crate) async fn handle_quorum_proposal_recv< proposal: &Proposal>, quorum_proposal_sender_key: &TYPES::SignatureKey, event_sender: &Sender>>, + event_receiver: &Receiver>>, task_state: &mut QuorumProposalRecvTaskState, ) -> Result<()> { let quorum_proposal_sender_key = quorum_proposal_sender_key.clone(); @@ -162,6 +194,21 @@ pub(crate) async fn handle_quorum_proposal_recv< .get(&justify_qc.data.leaf_commit) .cloned(); + if parent_leaf.is_none() { + spawn_fetch_proposal( + justify_qc.view_number(), + event_sender.clone(), + event_receiver.clone(), + Arc::clone(&task_state.quorum_membership), + OuterConsensus::new(Arc::clone(&task_state.consensus.inner_consensus)), + // Note that we explicitly use the node key here instead of the provided key in the signature. + // This is because the key that we receive is for the prior leader, so the payload would be routed + // incorrectly. + task_state.public_key.clone(), + task_state.private_key.clone(), + task_state.upgrade_lock.clone(), + ); + } let consensus_read = task_state.consensus.read().await; let parent = match parent_leaf { diff --git a/crates/task-impls/src/quorum_proposal_recv/mod.rs b/crates/task-impls/src/quorum_proposal_recv/mod.rs index 5a69fa0125..281e472fd0 100644 --- a/crates/task-impls/src/quorum_proposal_recv/mod.rs +++ b/crates/task-impls/src/quorum_proposal_recv/mod.rs @@ -129,7 +129,15 @@ impl, V: Versions> event_receiver: Receiver>>, ) { if let HotShotEvent::QuorumProposalRecv(proposal, sender) = event.as_ref() { - match handle_quorum_proposal_recv(proposal, sender, &event_sender, self).await { + match handle_quorum_proposal_recv( + proposal, + sender, + &event_sender, + &event_receiver, + self, + ) + .await + { Ok(()) => { self.cancel_tasks(proposal.data.view_number()).await; } From 39c66721ef1e770397492b702ec4613fcfa5dd3e Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Tue, 22 Oct 2024 23:49:27 -0400 Subject: [PATCH 6/9] revert test --- .../tests/tests_1/quorum_proposal_recv_task.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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 5ae7777bfb..d5a83a1953 100644 --- a/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs +++ b/crates/testing/tests/tests_1/quorum_proposal_recv_task.rs @@ -208,6 +208,17 @@ async fn test_quorum_proposal_recv_task_liveness_check() { leaders[2] )]]; + // make the request payload + let req = ProposalRequestPayload { + view_number: ViewNumber::new(2), + key: handle.public_key(), + }; + + // make the signed commitment + let signature = + ::SignatureKey::sign(handle.private_key(), req.commit().as_ref()) + .unwrap(); + let expectations = vec![Expectations::from_outputs(all_predicates![ exact(QuorumProposalPreliminarilyValidated(proposals[2].clone())), exact(ViewChange(ViewNumber::new(3))), @@ -222,6 +233,7 @@ async fn test_quorum_proposal_recv_task_liveness_check() { ) .await, )), + exact(QuorumProposalRequestSend(req, signature)), exact(HighQcUpdated(proposals[2].data.justify_qc.clone())), ])]; From e774e5dc32def3851482724c0429cadecd387757 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Mon, 28 Oct 2024 13:45:10 -0400 Subject: [PATCH 7/9] fixes after merge --- crates/task-impls/src/quorum_proposal_recv/handlers.rs | 6 +++--- crates/task-impls/src/quorum_vote/mod.rs | 9 ++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/task-impls/src/quorum_proposal_recv/handlers.rs b/crates/task-impls/src/quorum_proposal_recv/handlers.rs index 1aee92f68d..df92e7f6c5 100644 --- a/crates/task-impls/src/quorum_proposal_recv/handlers.rs +++ b/crates/task-impls/src/quorum_proposal_recv/handlers.rs @@ -213,7 +213,7 @@ pub(crate) async fn handle_quorum_proposal_recv< let parent = match parent_leaf { Some(leaf) => { - if let (Some(state), _) = consensus_reader.state_and_delta(leaf.view_number()) { + if let (Some(state), _) = consensus_read.state_and_delta(leaf.view_number()) { Some((leaf, Arc::clone(&state))) } else { bail!("Parent state not found! Consensus internally inconsistent"); @@ -222,7 +222,7 @@ pub(crate) async fn handle_quorum_proposal_recv< None => None, }; - if justify_qc.view_number() > consensus_reader.high_qc().view_number { + if justify_qc.view_number() > consensus_read.high_qc().view_number { if let Err(e) = task_state .storage .write() @@ -233,7 +233,7 @@ pub(crate) async fn handle_quorum_proposal_recv< bail!("Failed to store High QC, not voting; error = {:?}", e); } } - drop(consensus_reader); + drop(consensus_read); let mut consensus_writer = task_state.consensus.write().await; if let Err(e) = consensus_writer.update_high_qc(justify_qc.clone()) { diff --git a/crates/task-impls/src/quorum_vote/mod.rs b/crates/task-impls/src/quorum_vote/mod.rs index 23b654c64a..d94e4cc105 100644 --- a/crates/task-impls/src/quorum_vote/mod.rs +++ b/crates/task-impls/src/quorum_vote/mod.rs @@ -287,7 +287,7 @@ impl + 'static, V: Versions> Handl HotShotEvent::QuorumProposalValidated(proposal, parent_leaf) => { let proposal_payload_comm = proposal.data.block_header.payload_commitment(); if let Some(ref comm) = payload_commitment { - if proposal_payload_comm != comm { + if proposal_payload_comm != *comm { tracing::error!("Quorum proposal has inconsistent payload commitment with DAC or VID."); return; } @@ -303,7 +303,7 @@ impl + 'static, V: Versions> Handl // Update our persistent storage of the proposal. If we cannot store the proposal reutrn // and error so we don't vote if let Err(e) = self.storage.write().await.append_proposal(proposal).await { - error!("failed to store proposal, not voting. error = {e:#}"); + tracing::error!("failed to store proposal, not voting. error = {e:#}"); return; } leaf = Some(proposed_leaf); @@ -555,7 +555,10 @@ impl, V: Versions> QuorumVoteTaskS match event.as_ref() { HotShotEvent::QuorumProposalValidated(proposal, _leaf) => { let cur_epoch = self.consensus.read().await.cur_epoch(); - tracing::trace!("Received Proposal for view {}", *proposal.data.view_number()); + tracing::trace!( + "Received Proposal for view {}", + *proposal.data.view_number() + ); // Handle the event before creating the dependency task. if let Err(e) = From e7ec8c0bbaef34e32e202cf99d2f0b0ab6f8ace7 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Mon, 28 Oct 2024 16:55:00 -0400 Subject: [PATCH 8/9] return no proposal if the dep errors --- crates/task-impls/src/helpers.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/task-impls/src/helpers.rs b/crates/task-impls/src/helpers.rs index 119e25fff4..3c2d5c423a 100644 --- a/crates/task-impls/src/helpers.rs +++ b/crates/task-impls/src/helpers.rs @@ -118,6 +118,9 @@ pub(crate) async fn fetch_proposal( } } + } else { + // If the dep returns early return none + return None; } } From 4817b59ea1bef83feb20a2f4b04e0c76029a47d5 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Mon, 28 Oct 2024 17:00:53 -0400 Subject: [PATCH 9/9] rename to reader --- crates/task-impls/src/quorum_proposal_recv/handlers.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/task-impls/src/quorum_proposal_recv/handlers.rs b/crates/task-impls/src/quorum_proposal_recv/handlers.rs index df92e7f6c5..488854e95d 100644 --- a/crates/task-impls/src/quorum_proposal_recv/handlers.rs +++ b/crates/task-impls/src/quorum_proposal_recv/handlers.rs @@ -209,11 +209,11 @@ pub(crate) async fn handle_quorum_proposal_recv< task_state.upgrade_lock.clone(), ); } - let consensus_read = task_state.consensus.read().await; + let consensus_reader = task_state.consensus.read().await; let parent = match parent_leaf { Some(leaf) => { - if let (Some(state), _) = consensus_read.state_and_delta(leaf.view_number()) { + if let (Some(state), _) = consensus_reader.state_and_delta(leaf.view_number()) { Some((leaf, Arc::clone(&state))) } else { bail!("Parent state not found! Consensus internally inconsistent"); @@ -222,7 +222,7 @@ pub(crate) async fn handle_quorum_proposal_recv< None => None, }; - if justify_qc.view_number() > consensus_read.high_qc().view_number { + if justify_qc.view_number() > consensus_reader.high_qc().view_number { if let Err(e) = task_state .storage .write() @@ -233,7 +233,7 @@ pub(crate) async fn handle_quorum_proposal_recv< bail!("Failed to store High QC, not voting; error = {:?}", e); } } - drop(consensus_read); + drop(consensus_reader); let mut consensus_writer = task_state.consensus.write().await; if let Err(e) = consensus_writer.update_high_qc(justify_qc.clone()) {