From 96a61a1ddb536279458f6bd6744ef425a7a154a8 Mon Sep 17 00:00:00 2001 From: Keyao Shen Date: Wed, 4 Oct 2023 17:37:38 -0700 Subject: [PATCH 1/3] Remove commitment check in is_valid_cert, update state even is missing parent leaf. --- crates/task-impls/src/consensus.rs | 330 +++++++++++++++------------- crates/types/src/traits/election.rs | 9 +- 2 files changed, 185 insertions(+), 154 deletions(-) diff --git a/crates/task-impls/src/consensus.rs b/crates/task-impls/src/consensus.rs index b4a38db5e5..3f2551245a 100644 --- a/crates/task-impls/src/consensus.rs +++ b/crates/task-impls/src/consensus.rs @@ -429,8 +429,13 @@ where // Validate the DAC. if self .committee_exchange - .is_valid_cert(cert, proposal.block_commitment) + .is_valid_cert(cert) { + // Validate the block commitment for non-genesis DAC. + if !cert.is_genesis() && cert.leaf_commitment() != proposal.block_commitment { + error!("Block commitment does not equal parent commitment"); + return false; + } self.quorum_exchange.create_yes_message( proposal.justify_qc.commit(), leaf.commit(), @@ -567,7 +572,6 @@ where Ok(Some(vote_token)) => { debug!("We were chosen for consensus committee on {:?}", view); let consensus = self.consensus.upgradable_read().await; - let message; // TODO ED Insert TC logic here @@ -582,111 +586,147 @@ where .cloned() }; - // Justify qc's leaf commitment is not the same as the parent's leaf commitment, but it should be (in this case) - let Some(parent) = parent else { - error!( - "Proposal's parent missing from storage with commitment: {:?}", - justify_qc.leaf_commitment() - ); - return; - }; - let parent_commitment = parent.commit(); - let leaf: SequencingLeaf<_> = SequencingLeaf { - view_number: view, - height: proposal.data.height, - justify_qc: justify_qc.clone(), - parent_commitment, - deltas: Right(proposal.data.block_commitment), - rejected: Vec::new(), - timestamp: time::OffsetDateTime::now_utc().unix_timestamp_nanos(), - proposer_id: sender.to_bytes(), - }; + // Validate the `justify_qc`. let justify_qc_commitment = justify_qc.commit(); - let leaf_commitment = leaf.commit(); + let invalid = !self.quorum_exchange.is_valid_cert(&justify_qc); + let leaf; - // Validate the `justify_qc`. - if !self - .quorum_exchange - .is_valid_cert(&justify_qc, parent_commitment) - { - error!("Invalid justify_qc in proposal!. parent commitment is {:?} justify qc is {:?}", parent_commitment, justify_qc.clone()); + // Justify qc's leaf commitment is not the same as the parent's leaf commitment, but it should be (in this case) + if let Some(parent) = parent { + let message; + leaf = SequencingLeaf { + view_number: view, + height: proposal.data.height, + justify_qc: justify_qc.clone(), + parent_commitment: parent.commit(), + deltas: Right(proposal.data.block_commitment), + rejected: Vec::new(), + timestamp: time::OffsetDateTime::now_utc().unix_timestamp_nanos(), + proposer_id: sender.to_bytes(), + }; + let parent_commitment = parent.commit(); + let leaf_commitment = leaf.commit(); - message = self.quorum_exchange.create_no_message::( - justify_qc_commitment, - leaf_commitment, - view, - vote_token, - ); - } - // Validate the `height`. - else if leaf.height != parent.height + 1 { - error!( - "Incorrect height in proposal (expected {}, got {})", - parent.height + 1, - leaf.height - ); - message = self.quorum_exchange.create_no_message( - justify_qc_commitment, - leaf_commitment, - view, - vote_token, - ); - } - // Validate the signature. - else if !view_leader_key - .validate(&proposal.signature, leaf_commitment.as_ref()) - { - error!(?proposal.signature, "Could not verify proposal."); - message = self.quorum_exchange.create_no_message( - justify_qc_commitment, - leaf_commitment, - view, - vote_token, - ); - } - // Create a positive vote if either liveness or safety check - // passes. - else { - // Liveness check. - let liveness_check = justify_qc.view_number > consensus.locked_view; - - // Safety check. - // Check if proposal extends from the locked leaf. - let outcome = consensus.visit_leaf_ancestors( - justify_qc.view_number, - Terminator::Inclusive(consensus.locked_view), - false, - |leaf| { - // if leaf view no == locked view no then we're done, report success by - // returning true - leaf.view_number != consensus.locked_view - }, - ); - let safety_check = outcome.is_ok(); - if let Err(e) = outcome { - self.api.send_view_error(view, Arc::new(e)).await; - } + if invalid { + error!("Invalid justify_qc in proposal! parent commitment is {:?} justify qc is {:?}", parent_commitment, justify_qc.clone()); - // Skip if both saftey and liveness checks fail. - if !safety_check && !liveness_check { - error!("Failed safety check and liveness check"); + message = self.quorum_exchange.create_no_message::( + justify_qc_commitment, + leaf_commitment, + view, + vote_token, + ); + } + // Validate the leaf commitment for non-genesis QC. + else if !justify_qc.is_genesis() + && justify_qc.leaf_commitment() != parent_commitment + { + error!("Leaf commitment does not equal parent commitment"); + message = self.quorum_exchange.create_no_message::( + justify_qc_commitment, + leaf_commitment, + view, + vote_token, + ); + } + // Validate the `height`. + else if leaf.height != parent.height + 1 { + error!( + "Incorrect height in proposal (expected {}, got {})", + parent.height + 1, + leaf.height + ); message = self.quorum_exchange.create_no_message( justify_qc_commitment, leaf_commitment, view, vote_token, ); - } else { - // Generate a message with yes vote. - message = self.quorum_exchange.create_yes_message( + } + // Validate the signature. + else if !view_leader_key + .validate(&proposal.signature, leaf_commitment.as_ref()) + { + error!(?proposal.signature, "Could not verify proposal."); + message = self.quorum_exchange.create_no_message( justify_qc_commitment, leaf_commitment, view, vote_token, ); } + // Create a positive vote if either liveness or safety check + // passes. + // Liveness check. + else { + let liveness_check = justify_qc.view_number > consensus.locked_view; + + // Safety check. + // Check if proposal extends from the locked leaf. + let outcome = consensus.visit_leaf_ancestors( + justify_qc.view_number, + Terminator::Inclusive(consensus.locked_view), + false, + |leaf| { + // if leaf view no == locked view no then we're done, report success by + // returning true + leaf.view_number != consensus.locked_view + }, + ); + let safety_check = outcome.is_ok(); + if let Err(e) = outcome { + self.api.send_view_error(view, Arc::new(e)).await; + } + + // Skip if both saftey and liveness checks fail. + if !safety_check && !liveness_check { + error!("Failed safety check and liveness check"); + message = self.quorum_exchange.create_no_message( + justify_qc_commitment, + leaf_commitment, + view, + vote_token, + ); + } + // Generate a message with yes vote. + else { + message = self.quorum_exchange.create_yes_message( + justify_qc_commitment, + leaf_commitment, + view, + vote_token, + ); + } + } + + if let GeneralConsensusMessage::Vote(vote) = message { + debug!("Sending vote to next leader {:?}", vote); + }; + } else { + // Allow missing parent so we can update the state, but we won't + // vote in this case. + error!( + "Proposal's parent missing from storage with commitment: {:?}", + justify_qc.leaf_commitment() + ); + + if invalid { + error!("Invalid justify_qc in proposal {:?}", justify_qc.clone()); + } + leaf = SequencingLeaf { + view_number: view, + height: proposal.data.height, + justify_qc: justify_qc.clone(), + parent_commitment: justify_qc.leaf_commitment(), + deltas: Right(proposal.data.block_commitment), + rejected: Vec::new(), + timestamp: time::OffsetDateTime::now_utc().unix_timestamp_nanos(), + proposer_id: sender.to_bytes(), + }; } + // TODO (Keyao) Update consensus state only if all verifications pass. + // let high_qc = leaf.justify_qc.clone(); let mut new_anchor_view = consensus.last_decided_view; let mut new_locked_view = consensus.locked_view; @@ -702,64 +742,64 @@ where if parent_view + 1 == view { current_chain_length += 1; if let Err(e) = consensus.visit_leaf_ancestors( - parent_view, - Terminator::Exclusive(old_anchor_view), - true, - |leaf| { - if !new_decide_reached { - if last_view_number_visited == leaf.view_number + 1 { - last_view_number_visited = leaf.view_number; - current_chain_length += 1; - if current_chain_length == 2 { - new_locked_view = leaf.view_number; - new_commit_reached = true; - // The next leaf in the chain, if there is one, is decided, so this - // leaf's justify_qc would become the QC for the decided chain. - new_decide_qc = Some(leaf.justify_qc.clone()); - } else if current_chain_length == 3 { - new_anchor_view = leaf.view_number; - new_decide_reached = true; - } - } else { - // nothing more to do here... we don't have a new chain extension - return false; - } - } - // starting from the first iteration with a three chain, e.g. right after the else if case nested in the if case above - if new_decide_reached { - let mut leaf = leaf.clone(); - - // If the full block is available for this leaf, include it in the leaf - // chain that we send to the client. - if let Some(block) = - consensus.saved_blocks.get(leaf.get_deltas_commitment()) - { - if let Err(err) = leaf.fill_deltas(block.clone()) { - error!("unable to fill leaf {} with block {}, block will not be available: {}", - leaf.commit(), block.commit(), err); + parent_view, + Terminator::Exclusive(old_anchor_view), + true, + |leaf| { + if !new_decide_reached { + if last_view_number_visited == leaf.view_number + 1 { + last_view_number_visited = leaf.view_number; + current_chain_length += 1; + if current_chain_length == 2 { + new_locked_view = leaf.view_number; + new_commit_reached = true; + // The next leaf in the chain, if there is one, is decided, so this + // leaf's justify_qc would become the QC for the decided chain. + new_decide_qc = Some(leaf.justify_qc.clone()); + } else if current_chain_length == 3 { + new_anchor_view = leaf.view_number; + new_decide_reached = true; + } + } else { + // nothing more to do here... we don't have a new chain extension + return false; + } } - } - - leaf_views.push(leaf.clone()); - match &leaf.deltas { - Left(block) => { - let txns = block.contained_transactions(); - for txn in txns { - included_txns.insert(txn); + // starting from the first iteration with a three chain, e.g. right after the else if case nested in the if case above + if new_decide_reached { + let mut leaf = leaf.clone(); + + // If the full block is available for this leaf, include it in the leaf + // chain that we send to the client. + if let Some(block) = + consensus.saved_blocks.get(leaf.get_deltas_commitment()) + { + if let Err(err) = leaf.fill_deltas(block.clone()) { + error!("unable to fill leaf {} with block {}, block will not be available: {}", + leaf.commit(), block.commit(), err); + } + } + + leaf_views.push(leaf.clone()); + match &leaf.deltas { + Left(block) => { + let txns = block.contained_transactions(); + for txn in txns { + included_txns.insert(txn); + } + } + Right(_) => {} } } - Right(_) => {} + true + }, + ) { + error!("publishing view error"); + self.output_event_stream.publish(Event { + view_number: view, + event: EventType::Error { error: e.into() }, + }).await; } - } - true - }, - ) { - error!("publishing view error"); - self.output_event_stream.publish(Event { - view_number: view, - event: EventType::Error { error: e.into() }, - }).await; - } } let included_txns_set: HashSet<_> = if new_decide_reached { @@ -883,10 +923,6 @@ where // Update current view and publish a view change event so other tasks also update self.update_view(new_view).await; - - if let GeneralConsensusMessage::Vote(vote) = message { - debug!("Sending vote to next leader {:?}", vote); - }; } } } diff --git a/crates/types/src/traits/election.rs b/crates/types/src/traits/election.rs index 81a173e954..fdc435163b 100644 --- a/crates/types/src/traits/election.rs +++ b/crates/types/src/traits/election.rs @@ -342,18 +342,13 @@ pub trait ConsensusExchange: Send + Sync { /// The contents of a vote on `commit`. fn vote_data(&self, commit: Commitment) -> VoteData; - /// Validate a QC. - fn is_valid_cert(&self, qc: &Self::Certificate, commit: Commitment) -> bool { + /// Validate a certificate. + fn is_valid_cert(&self, qc: &Self::Certificate) -> bool { if qc.is_genesis() && qc.view_number() == TYPES::Time::genesis() { return true; } let leaf_commitment = qc.leaf_commitment(); - if leaf_commitment != commit { - error!("Leaf commitment does not equal parent commitment"); - return false; - } - match qc.signatures() { AssembledSignature::DA(qc) => { let real_commit = VoteData::DA(leaf_commitment).commit(); From 6579a5b83276f1b3aaab92ee78834af1979380fd Mon Sep 17 00:00:00 2001 From: Keyao Shen Date: Wed, 4 Oct 2023 20:01:54 -0700 Subject: [PATCH 2/3] Check ancestors only if having parent leaf --- crates/task-impls/src/consensus.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/crates/task-impls/src/consensus.rs b/crates/task-impls/src/consensus.rs index 3f2551245a..268249518c 100644 --- a/crates/task-impls/src/consensus.rs +++ b/crates/task-impls/src/consensus.rs @@ -592,7 +592,7 @@ where let leaf; // Justify qc's leaf commitment is not the same as the parent's leaf commitment, but it should be (in this case) - if let Some(parent) = parent { + if let Some(parent) = parent.clone() { let message; leaf = SequencingLeaf { view_number: view, @@ -736,12 +736,13 @@ where let mut new_decide_qc = None; let mut leaf_views = Vec::new(); let mut included_txns = HashSet::new(); - let old_anchor_view = consensus.last_decided_view; - let parent_view = leaf.justify_qc.view_number; - let mut current_chain_length = 0usize; - if parent_view + 1 == view { - current_chain_length += 1; - if let Err(e) = consensus.visit_leaf_ancestors( + if parent.is_some() { + let old_anchor_view = consensus.last_decided_view; + let parent_view = leaf.justify_qc.view_number; + let mut current_chain_length = 0usize; + if parent_view + 1 == view { + current_chain_length += 1; + if let Err(e) = consensus.visit_leaf_ancestors( parent_view, Terminator::Exclusive(old_anchor_view), true, @@ -800,6 +801,7 @@ where event: EventType::Error { error: e.into() }, }).await; } + } } let included_txns_set: HashSet<_> = if new_decide_reached { @@ -904,7 +906,7 @@ where drop(consensus); if should_propose { debug!( - "Attempting to publish proposal after voting; now in view: {}", + "Attempting to publish proposal before voting; now in view: {}", *new_view ); self.publish_proposal_if_able(qc.clone(), qc.view_number + 1) From 5f2a69de33c699d1e4074493f18977dd4dc7caf5 Mon Sep 17 00:00:00 2001 From: Brendon Fish Date: Thu, 5 Oct 2023 10:03:46 -0400 Subject: [PATCH 3/3] store leaf and return if missing parent, fix subscribe in view sync --- crates/task-impls/src/consensus.rs | 38 +++++++++++++++++++++++++----- crates/task-impls/src/view_sync.rs | 20 ++++++++-------- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/crates/task-impls/src/consensus.rs b/crates/task-impls/src/consensus.rs index 268249518c..12d7313075 100644 --- a/crates/task-impls/src/consensus.rs +++ b/crates/task-impls/src/consensus.rs @@ -339,8 +339,9 @@ where // Justify qc's leaf commitment is not the same as the parent's leaf commitment, but it should be (in this case) let Some(parent) = parent else { error!( - "Proposal's parent missing from storage with commitment: {:?}", - justify_qc.leaf_commitment() + "Proposal's parent missing from storage with commitment: {:?}, proposal view {:?}", + justify_qc.leaf_commitment(), + proposal.view_number, ); return false; }; @@ -408,8 +409,9 @@ where // Justify qc's leaf commitment is not the same as the parent's leaf commitment, but it should be (in this case) let Some(parent) = parent else { error!( - "Proposal's parent missing from storage with commitment: {:?}", - justify_qc.leaf_commitment() + "Proposal's parent missing from storage with commitment: {:?}, proposal view {:?}", + justify_qc.leaf_commitment(), + proposal.view_number, ); return false; }; @@ -706,12 +708,14 @@ where // Allow missing parent so we can update the state, but we won't // vote in this case. error!( - "Proposal's parent missing from storage with commitment: {:?}", - justify_qc.leaf_commitment() + "Proposal's parent missing from storage with commitment: {:?}, proposal view {:?}", + justify_qc.leaf_commitment(), + proposal.data.view_number, ); if invalid { error!("Invalid justify_qc in proposal {:?}", justify_qc.clone()); + return; } leaf = SequencingLeaf { view_number: view, @@ -723,6 +727,28 @@ where timestamp: time::OffsetDateTime::now_utc().unix_timestamp_nanos(), proposer_id: sender.to_bytes(), }; + if !view_leader_key + .validate(&proposal.signature, leaf.commit().as_ref()) + { + error!(?proposal.signature, "Could not verify proposal."); + return; + } + + let mut consensus = RwLockUpgradableReadGuard::upgrade(consensus).await; + consensus.state_map.insert( + view, + View { + view_inner: ViewInner::Leaf { + leaf: leaf.commit(), + }, + }, + ); + consensus.saved_leaves.insert(leaf.commit(), leaf.clone()); + drop(consensus); + // The valid QC and signature on the proposal is evidence we can go to the next view + // even though we can't vote in this round because we missed the last proposal. + self.update_view(TYPES::Time::new(*view + 1)).await; + return; } // TODO (Keyao) Update consensus state only if all verifications pass. diff --git a/crates/task-impls/src/view_sync.rs b/crates/task-impls/src/view_sync.rs index e35ec6f399..da52ceaf82 100644 --- a/crates/task-impls/src/view_sync.rs +++ b/crates/task-impls/src/view_sync.rs @@ -516,25 +516,25 @@ where // panic!("Starting view sync!"); // Spawn replica task let next_view = *view_number + 1; + // Subscribe to the view after we are leader since we know we won't propose in the next view if we are leader. + let subscribe_view = if self.exchange.is_leader(TYPES::Time::new(next_view)) { + next_view + 1 + } else { + next_view + }; // Subscribe to the next view just in case there is progress being made self.exchange .network() - .inject_consensus_info(ConsensusIntentEvent::PollForProposal(next_view)) + .inject_consensus_info(ConsensusIntentEvent::PollForProposal( + subscribe_view, + )) .await; self.exchange .network() - .inject_consensus_info(ConsensusIntentEvent::PollForDAC(next_view)) + .inject_consensus_info(ConsensusIntentEvent::PollForDAC(subscribe_view)) .await; - if self.exchange.is_leader(TYPES::Time::new(next_view + 1)) { - debug!("Polling for quorum votes for view {}", next_view); - self.exchange - .network() - .inject_consensus_info(ConsensusIntentEvent::PollForVotes(next_view)) - .await; - } - let mut replica_state = ViewSyncReplicaTaskState { current_view: self.current_view, next_view: TYPES::Time::new(next_view),