Skip to content
This repository was archived by the owner on Feb 27, 2025. It is now read-only.

Lr/split validation #3495

Merged
lukaszrzasik merged 10 commits into from
Jul 31, 2024
Merged

Lr/split validation #3495

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8bf609a
Split validation
lukaszrzasik Jul 26, 2024
1383d7a
Remove byzantine code for now
lukaszrzasik Jul 26, 2024
bdd911e
Adjust tests
lukaszrzasik Jul 26, 2024
0df5de2
Fix one more test
lukaszrzasik Jul 26, 2024
20e9397
Change the broadcast event indiciating correclty the High QC has been…
lukaszrzasik Jul 26, 2024
63b5aff
Cleanup
lukaszrzasik Jul 29, 2024
63c5e28
Add more verbose description of the `QuorumProposalValidated` event
lukaszrzasik Jul 30, 2024
9957263
Fix a call to `validate_signature`
lukaszrzasik Jul 30, 2024
3161e3a
Remove an obsolete comment.
lukaszrzasik Jul 30, 2024
419bcfc
Merge branch 'main' into lr/split-validation
lukaszrzasik Jul 30, 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
3 changes: 0 additions & 3 deletions crates/task-impls/src/consensus/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,15 +293,13 @@ pub(crate) async fn handle_quorum_proposal_recv<TYPES: NodeType, I: NodeImplemen

validate_proposal_view_and_certs(
proposal,
&sender,
task_state.cur_view,
&task_state.quorum_membership,
&task_state.timeout_membership,
)
.context("Failed to validate proposal view and attached certs")?;

let view = proposal.data.view_number();
let view_leader_key = task_state.quorum_membership.leader(view);
let justify_qc = proposal.data.justify_qc.clone();

if !justify_qc.is_valid_cert(task_state.quorum_membership.as_ref()) {
Expand Down Expand Up @@ -489,7 +487,6 @@ pub(crate) async fn handle_quorum_proposal_recv<TYPES: NodeType, I: NodeImplemen
OuterConsensus::new(Arc::clone(&task_state.consensus.inner_consensus)),
Arc::clone(&task_state.decided_upgrade_certificate),
Arc::clone(&task_state.quorum_membership),
view_leader_key,
event_stream.clone(),
sender,
task_state.output_event_stream.clone(),
Expand Down
19 changes: 19 additions & 0 deletions crates/task-impls/src/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ pub enum HotShotEvent<TYPES: NodeType> {
/// All dependencies for the quorum vote are validated.
QuorumVoteDependenciesValidated(TYPES::Time),
/// A quorum proposal with the given parent leaf is validated.
/// The full validation checks include:
Copy link
Contributor

@jparr721 jparr721 Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this!

/// 1. The proposal is not for an old view
/// 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<TYPES>, Leaf<TYPES>),
/// A quorum proposal is missing for a view that we meed
QuorumProposalRequest(ProposalMissing<TYPES>),
Expand Down Expand Up @@ -191,6 +196,13 @@ pub enum HotShotEvent<TYPES: NodeType> {

/// A new high_qc has been updated in `Consensus`.
HighQcUpdated(QuorumCertificate<TYPES>),

/// A quorum proposal has been preliminarily validated.
/// The preliminary checks include:
/// 1. The proposal is not for an old view
/// 2. The proposal has been correctly signed by the leader of the current view
/// 3. The justify QC is valid
QuorumProposalPreliminarilyValidated(Proposal<TYPES, QuorumProposal<TYPES>>),
Copy link
Contributor

@jparr721 jparr721 Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it'd be helpful to elucidate how this event is different from the QuorumProposalValidated event? Unfortunately, it looks like the QuorumProposalValidated event's doc comment doesn't give much information right now, so an unfamiliar reader might not be able to trivially differentiate the two events without tracing the validation paths. We could also update the validated event as well with a more descriptive doc comment.

}

impl<TYPES: NodeType> Display for HotShotEvent<TYPES> {
Expand Down Expand Up @@ -428,6 +440,13 @@ impl<TYPES: NodeType> Display for HotShotEvent<TYPES> {
HotShotEvent::HighQcUpdated(cert) => {
write!(f, "HighQcUpdated(view_number={:?})", cert.view_number())
}
HotShotEvent::QuorumProposalPreliminarilyValidated(proposal) => {
write!(
f,
"QuorumProposalPreliminarilyValidated(view_number={:?}",
proposal.data.view_number()
)
}
}
}
}
28 changes: 8 additions & 20 deletions crates/task-impls/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ use hotshot_types::{
block_contents::BlockHeader,
election::Membership,
node_implementation::{ConsensusTime, NodeType},
signature_key::SignatureKey,
BlockPayload, ValidatedState,
},
utils::{Terminator, View, ViewInner},
Expand Down Expand Up @@ -353,7 +352,6 @@ pub async fn validate_proposal_safety_and_liveness<TYPES: NodeType>(
consensus: OuterConsensus<TYPES>,
decided_upgrade_certificate: Arc<RwLock<Option<UpgradeCertificate<TYPES>>>>,
quorum_membership: Arc<TYPES::Membership>,
view_leader_key: TYPES::SignatureKey,
event_stream: Sender<Arc<HotShotEvent<TYPES>>>,
sender: TYPES::SignatureKey,
event_sender: Sender<Event<TYPES>>,
Expand Down Expand Up @@ -397,18 +395,6 @@ pub async fn validate_proposal_safety_and_liveness<TYPES: NodeType>(
)
.await;

// Validate the proposal's signature. This should also catch if the leaf_commitment does not equal our calculated parent commitment
//
// There is a mistake here originating in the genesis leaf/qc commit. This should be replaced by:
//
// proposal.validate_signature(&quorum_membership)?;
//
// in a future PR.
ensure!(
view_leader_key.validate(&proposal.signature, proposed_leaf.commit().as_ref()),
"Could not verify proposal."
);

UpgradeCertificate::validate(&proposal.data.upgrade_certificate, &quorum_membership)?;

// Validate that the upgrade certificate is re-attached, if we saw one on the parent
Expand Down Expand Up @@ -487,7 +473,6 @@ pub async fn validate_proposal_safety_and_liveness<TYPES: NodeType>(
/// If any validation or view number check fails.
pub fn validate_proposal_view_and_certs<TYPES: NodeType>(
proposal: &Proposal<TYPES, QuorumProposal<TYPES>>,
sender: &TYPES::SignatureKey,
cur_view: TYPES::Time,
quorum_membership: &Arc<TYPES::Membership>,
timeout_membership: &Arc<TYPES::Membership>,
Expand All @@ -499,11 +484,14 @@ pub fn validate_proposal_view_and_certs<TYPES: NodeType>(
proposal.data.clone()
);

let view_leader_key = quorum_membership.leader(view);
ensure!(
view_leader_key == *sender,
"Leader key does not match key in proposal"
);
// Validate the proposal's signature. This should also catch if the leaf_commitment does not equal our calculated parent commitment
//
// There is a mistake here originating in the genesis leaf/qc commit. This should be replaced by:
Copy link
Contributor

@jparr721 jparr721 Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we might be able to remove this comment entirely? Looks like we can keep the above, but the "there is a mistake here" part seems to be rectified.

Copy link
Contributor Author

@lukaszrzasik lukaszrzasik Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right!

//
// proposal.validate_signature(&quorum_membership)?;
//
// in a future PR.
proposal.validate_signature(quorum_membership)?;

// Verify a timeout certificate OR a view sync certificate exists and is valid.
if proposal.data.justify_qc.view_number() != view - 1 {
Expand Down
7 changes: 4 additions & 3 deletions crates/task-impls/src/quorum_proposal/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>> QuorumProposalTaskState<TYPE
}
}
ProposalDependency::Proposal => {
if let HotShotEvent::QuorumProposalRecv(proposal, _) = event {
if let HotShotEvent::QuorumProposalPreliminarilyValidated(proposal) = event
{
proposal.data.view_number() + 1
} else {
return false;
Expand Down Expand Up @@ -219,7 +220,7 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>> QuorumProposalTaskState<TYPE
HotShotEvent::SendPayloadCommitmentAndMetadata(..) => {
payload_commitment_dependency.mark_as_completed(Arc::clone(&event));
}
HotShotEvent::QuorumProposalRecv(..) => {
HotShotEvent::QuorumProposalPreliminarilyValidated(..) => {
proposal_dependency.mark_as_completed(event);
}
HotShotEvent::QcFormed(quorum_certificate) => match quorum_certificate {
Expand Down Expand Up @@ -430,7 +431,7 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>> QuorumProposalTaskState<TYPE
event,
);
}
HotShotEvent::QuorumProposalRecv(proposal, _) => {
HotShotEvent::QuorumProposalPreliminarilyValidated(proposal) => {
let view_number = proposal.data.view_number();

// All nodes get the latest proposed view as a proxy of `cur_view` of olde.
Expand Down
13 changes: 9 additions & 4 deletions crates/task-impls/src/quorum_proposal_recv/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,13 @@ pub(crate) async fn handle_quorum_proposal_recv<TYPES: NodeType, I: NodeImplemen

validate_proposal_view_and_certs(
proposal,
&sender,
task_state.cur_view,
&task_state.quorum_membership,
&task_state.timeout_membership,
)
.context("Failed to validate proposal view or attached certs")?;

let view_number = proposal.data.view_number();
let view_leader_key = task_state.quorum_membership.leader(view_number);
let justify_qc = proposal.data.justify_qc.clone();

if !justify_qc.is_valid_cert(task_state.quorum_membership.as_ref()) {
Expand All @@ -139,6 +137,14 @@ pub(crate) async fn handle_quorum_proposal_recv<TYPES: NodeType, I: NodeImplemen
bail!("Invalid justify_qc in proposal for view {}", *view_number);
}

broadcast_event(
Arc::new(HotShotEvent::QuorumProposalPreliminarilyValidated(
proposal.clone(),
)),
event_sender,
)
.await;

// NOTE: We could update our view with a valid TC but invalid QC, but that is not what we do here
if let Err(e) = update_view::<TYPES>(
view_number,
Expand Down Expand Up @@ -210,7 +216,7 @@ pub(crate) async fn handle_quorum_proposal_recv<TYPES: NodeType, I: NodeImplemen
drop(consensus_write);

broadcast_event(
HotShotEvent::UpdateHighQc(justify_qc.clone()).into(),
HotShotEvent::HighQcUpdated(justify_qc.clone()).into(),
event_sender,
)
.await;
Expand All @@ -230,7 +236,6 @@ pub(crate) async fn handle_quorum_proposal_recv<TYPES: NodeType, I: NodeImplemen
OuterConsensus::new(Arc::clone(&task_state.consensus.inner_consensus)),
Arc::clone(&task_state.decided_upgrade_certificate),
Arc::clone(&task_state.quorum_membership),
view_leader_key,
event_sender.clone(),
sender,
task_state.output_event_stream.clone(),
Expand Down
2 changes: 1 addition & 1 deletion crates/task-impls/src/quorum_proposal_recv/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ impl<TYPES: NodeType, I: NodeImplementation<TYPES>> QuorumProposalRecvTaskState<
)
.await;
}
Err(e) => debug!(?e, "Failed to propose"),
Err(e) => debug!(?e, "Failed to validate the proposal"),
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions crates/testing/tests/tests_1/quorum_proposal_recv_task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ async fn test_quorum_proposal_recv_task() {
)]];

let expectations = vec![Expectations::from_outputs(vec![
exact(QuorumProposalPreliminarilyValidated(proposals[1].clone())),
exact(ViewChange(ViewNumber::new(2))),
exact(UpdateHighQc(proposals[1].data.justify_qc.clone())),
exact(HighQcUpdated(proposals[1].data.justify_qc.clone())),
exact(ValidatedStateUpdated(
ViewNumber::new(2),
build_fake_view_with_leaf_and_state(
Expand Down Expand Up @@ -189,6 +190,7 @@ async fn test_quorum_proposal_recv_task_liveness_check() {
)]];

let expectations = vec![Expectations::from_outputs(all_predicates![
exact(QuorumProposalPreliminarilyValidated(proposals[2].clone())),
exact(ViewChange(ViewNumber::new(3))),
exact(ValidatedStateUpdated(
ViewNumber::new(3),
Expand All @@ -200,7 +202,7 @@ async fn test_quorum_proposal_recv_task_liveness_check() {
),
)),
quorum_proposal_missing(),
exact(UpdateHighQc(proposals[2].data.justify_qc.clone())),
exact(HighQcUpdated(proposals[2].data.justify_qc.clone())),
vote_now(),
])];

Expand Down
16 changes: 8 additions & 8 deletions crates/testing/tests/tests_1/quorum_proposal_task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ async fn test_quorum_proposal_task_quorum_proposal_view_gt_1() {
),
],
random![
QuorumProposalRecv(proposals[0].clone(), leaders[0]),
QuorumProposalPreliminarilyValidated(proposals[0].clone()),
QcFormed(either::Left(proposals[1].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(2)),
Expand All @@ -211,7 +211,7 @@ async fn test_quorum_proposal_task_quorum_proposal_view_gt_1() {
),
],
random![
QuorumProposalRecv(proposals[1].clone(), leaders[1]),
QuorumProposalPreliminarilyValidated(proposals[1].clone()),
QcFormed(either::Left(proposals[2].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(3)),
Expand All @@ -227,7 +227,7 @@ async fn test_quorum_proposal_task_quorum_proposal_view_gt_1() {
),
],
random![
QuorumProposalRecv(proposals[2].clone(), leaders[2]),
QuorumProposalPreliminarilyValidated(proposals[2].clone()),
QcFormed(either::Left(proposals[3].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(4)),
Expand All @@ -243,7 +243,7 @@ async fn test_quorum_proposal_task_quorum_proposal_view_gt_1() {
),
],
random![
QuorumProposalRecv(proposals[3].clone(), leaders[3]),
QuorumProposalPreliminarilyValidated(proposals[3].clone()),
QcFormed(either::Left(proposals[4].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(5)),
Expand Down Expand Up @@ -533,7 +533,7 @@ async fn test_quorum_proposal_task_liveness_check() {
),
],
random![
QuorumProposalRecv(proposals[0].clone(), leaders[0]),
QuorumProposalPreliminarilyValidated(proposals[0].clone()),
QcFormed(either::Left(proposals[1].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(2)),
Expand All @@ -549,7 +549,7 @@ async fn test_quorum_proposal_task_liveness_check() {
),
],
random![
QuorumProposalRecv(proposals[1].clone(), leaders[1]),
QuorumProposalPreliminarilyValidated(proposals[1].clone()),
QcFormed(either::Left(proposals[2].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(3)),
Expand All @@ -565,7 +565,7 @@ async fn test_quorum_proposal_task_liveness_check() {
),
],
random![
QuorumProposalRecv(proposals[2].clone(), leaders[2]),
QuorumProposalPreliminarilyValidated(proposals[2].clone()),
QcFormed(either::Left(proposals[3].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(4)),
Expand All @@ -581,7 +581,7 @@ async fn test_quorum_proposal_task_liveness_check() {
),
],
random![
QuorumProposalRecv(proposals[3].clone(), leaders[3]),
QuorumProposalPreliminarilyValidated(proposals[3].clone()),
QcFormed(either::Left(proposals[4].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(5)),
Expand Down
4 changes: 2 additions & 2 deletions crates/testing/tests/tests_1/upgrade_task_with_proposal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ async fn test_upgrade_task_with_proposal() {
),
],
random![
QuorumProposalRecv(proposals[0].clone(), leaders[0]),
QuorumProposalPreliminarilyValidated(proposals[0].clone()),
QcFormed(either::Left(proposals[1].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(2)),
Expand All @@ -172,7 +172,7 @@ async fn test_upgrade_task_with_proposal() {
],
InputOrder::Random(upgrade_vote_recvs),
random![
QuorumProposalRecv(proposals[1].clone(), leaders[1]),
QuorumProposalPreliminarilyValidated(proposals[1].clone()),
QcFormed(either::Left(proposals[2].data.justify_qc.clone())),
SendPayloadCommitmentAndMetadata(
build_payload_commitment(&quorum_membership, ViewNumber::new(3)),
Expand Down
2 changes: 1 addition & 1 deletion crates/types/src/consensus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ impl<TYPES: NodeType> Consensus<TYPES> {
view.leaf_commitment()
.ok_or_else(|| HotShotError::InvalidState {
context: format!(
"Visited failed view {start_from:?} leaf. Expected successfuil leaf"
"Visited failed view {start_from:?} leaf. Expected successful leaf"
),
})?
} else {
Expand Down