Skip to content

Commit

Permalink
[checkpoints] Detect checkpoint hard fork early (MystenLabs#11003)
Browse files Browse the repository at this point in the history
This PR detects checkpoints hard fork early in checkpoint builder and
panics validator. This is needed to prevent chain from signing
certificates on the fast path, reducing potential fork impact.

There is some discussion whether panic is the right response in this
case. This PR follows pattern established in the `CheckpointExecutor`
and panics on fork. We can revisit both places in a separate PR if we
want different response in case of a fork.
  • Loading branch information
andll authored Apr 17, 2023
1 parent a5d1948 commit 1681a6c
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
11 changes: 11 additions & 0 deletions crates/sui-core/src/checkpoints/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ pub struct CheckpointSignatureAggregator {
summary: CheckpointSummary,
digest: CheckpointDigest,
signatures: StakeAggregator<AuthoritySignInfo, true>,
failures: StakeAggregator<AuthoritySignInfo, false>,
}

impl CheckpointBuilder {
Expand Down Expand Up @@ -1005,6 +1006,7 @@ impl CheckpointAggregator {
digest: summary.digest(),
summary,
signatures: StakeAggregator::new(self.epoch_store.committee().clone()),
failures: StakeAggregator::new(self.epoch_store.committee().clone()),
});
self.current.as_mut().unwrap()
};
Expand Down Expand Up @@ -1080,6 +1082,15 @@ impl CheckpointSignatureAggregator {
let author = signature.authority;
// consensus ensures that authority == narwhal_cert.author
if their_digest != self.digest {
if let InsertResult::QuorumReached(data) =
self.failures.insert_generic(author, signature)
{
panic!("Checkpoint fork detected - f+1 validators submitted checkpoint digest at seq {} different from our digest {}. Validators with different digests: {:?}",
self.summary.sequence_number,
self.digest,
data.keys()
);
}
warn!(
"Validator {:?} has mismatching checkpoint digest {} at seq {}, we have digest {}",
author.concise(),
Expand Down
10 changes: 8 additions & 2 deletions crates/sui-core/src/stake_aggregator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,13 @@ impl<S: Clone + Eq, const STRENGTH: bool> StakeAggregator<S, STRENGTH> {
/// A generic version of inserting arbitrary type of V (e.g. void type).
/// If V is AuthoritySignInfo, the `insert` function should be used instead since it does extra
/// checks and aggregations in the end.
pub fn insert_generic(&mut self, authority: AuthorityName, s: S) -> InsertResult<()> {
/// Returns Map authority -> S, without aggregating it.
/// If you want to get an aggregated signature instead, use `StakeAggregator::insert`
pub fn insert_generic(
&mut self,
authority: AuthorityName,
s: S,
) -> InsertResult<&HashMap<AuthorityName, S>> {
match self.data.entry(authority) {
Entry::Occupied(oc) => {
return InsertResult::Failed {
Expand All @@ -69,7 +75,7 @@ impl<S: Clone + Eq, const STRENGTH: bool> StakeAggregator<S, STRENGTH> {
if votes > 0 {
self.total_votes += votes;
if self.total_votes >= self.committee.threshold::<STRENGTH>() {
InsertResult::QuorumReached(())
InsertResult::QuorumReached(&self.data)
} else {
InsertResult::NotEnoughVotes {
bad_votes: 0,
Expand Down

0 comments on commit 1681a6c

Please sign in to comment.