Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Updates conditions to advance from odd and even rounds #3119

Merged
merged 19 commits into from Mar 22, 2024
Merged
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 23 additions & 14 deletions node/bft/src/bft.rs
Expand Up @@ -322,6 +322,13 @@ impl<N: Network> BFT<N> {
committee: Committee<N>,
current_round: u64,
) -> bool {
// Retrieve the authors for the current round.
raychu86 marked this conversation as resolved.
Show resolved Hide resolved
let authors = certificates.into_iter().map(|c| c.author()).collect();
// Check if quorum threshold is reached.
if !committee.is_quorum_threshold_reached(&authors) {
debug!("BFT failed to reach quorum threshold in even round {current_round}");
return false;
}
// If the leader certificate is set for the current even round, return 'true'.
if let Some(leader_certificate) = self.leader_certificate.read().as_ref() {
if leader_certificate.round() == current_round {
Expand All @@ -330,11 +337,8 @@ impl<N: Network> BFT<N> {
}
// If the timer has expired, and we can achieve quorum threshold (2f + 1) without the leader, return 'true'.
if self.is_timer_expired() {
debug!("BFT (timer expired) - Checking for quorum threshold (without the leader)");
// Retrieve the certificate authors.
let authors = certificates.into_iter().map(|c| c.author()).collect();
// Determine if the quorum threshold is reached.
return committee.is_quorum_threshold_reached(&authors);
debug!("BFT (timer expired) - Advancing from round {current_round} to the next round (without the leader)");
return true;
}
// Otherwise, return 'false'.
false
Expand Down Expand Up @@ -363,14 +367,6 @@ impl<N: Network> BFT<N> {
error!("BFT does not compute stakes for the leader certificate in an even round");
return false;
}

// Retrieve the leader certificate.
let Some(leader_certificate) = self.leader_certificate.read().clone() else {
// If there is no leader certificate for the previous round, return 'true'.
return true;
};
// Retrieve the leader certificate ID.
let leader_certificate_id = leader_certificate.id();
// Retrieve the certificates for the current round.
let current_certificates = self.storage().get_certificates_for_round(current_round);
// Retrieve the committee lookback for the current round.
Expand All @@ -381,7 +377,20 @@ impl<N: Network> BFT<N> {
return false;
}
};

// Retrieve the authors of the current certificates.
raychu86 marked this conversation as resolved.
Show resolved Hide resolved
let authors = current_certificates.clone().into_iter().map(|c| c.author()).collect();
// Check if quorum threshold is reached.
if !committee_lookback.is_quorum_threshold_reached(&authors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this particular quroum_threshold_reached check needed? Does this not invalidate the subsequent checks below?

 `stake_with_leader >= committee_lookback.availability_threshold()
            || stake_without_leader >= committee_lookback.quorum_threshold()`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wouldn't invalidate the previous check because you can still see 'f+1' votes for the leader but not see '2f+1' certificates in the round. Here you should commit the leader, but wait to see '2f+1' certificates before advancing to the next round. Previously, the code was also returning 'true' if the timer had expired. But again, with an expired timer we should still include a quorum check before advancing.

debug!("BFT failed reach quorum threshold in odd round {current_round}. ");
return false;
}
// Retrieve the leader certificate.
let Some(leader_certificate) = self.leader_certificate.read().clone() else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this moved down? We can return early if the leader certificate is not found, and not incur the quorum threshold check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the leader was not found, you should advance to the next round only if you've seen a quorum of certificates. Otherwise, nodes may prematurely jump to the next round on a faulty leader before seeing quorum.

// If there is no leader certificate for the previous round, return 'true'.
return true;
};
// Retrieve the leader certificate ID.
let leader_certificate_id = leader_certificate.id();
// Compute the stake for the leader certificate.
let (stake_with_leader, stake_without_leader) =
self.compute_stake_for_leader_certificate(leader_certificate_id, current_certificates, &committee_lookback);
raychu86 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down