Skip to content

Commit

Permalink
[authority_aggregator] Fix a bug in process_certificate (MystenLabs#847)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lxfind
lxfind authored Mar 15, 2022
1 parent 6b6aae8 commit f006b18
Showing 1 changed file with 21 additions and 23 deletions.
44 changes: 21 additions & 23 deletions sui_core/src/authority_aggregator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ where
for returned_digest in &signed_effects.effects.dependencies {
// We check that we are not processing twice the same certificate, as
// it would be common if two objects used by one transaction, were also both
// mutated by the same preceeding transaction.
// mutated by the same preceding transaction.
if !candidate_certificates.contains(returned_digest) {
// Add this cert to the set we have processed
candidate_certificates.insert(*returned_digest);
Expand All @@ -181,7 +181,7 @@ where
/// Sync a certificate to an authority.
///
/// This function infers which authorities have the history related to
/// a certificate and attempts `retries` number of them, sampled accoding to
/// a certificate and attempts `retries` number of them, sampled according to
/// stake, in order to bring the destination authority up to date to accept
/// the certificate. The time devoted to each attempt is bounded by
/// `timeout_milliseconds`.
Expand Down Expand Up @@ -255,7 +255,7 @@ where
};

if timeout(timeout_period, logic).await.is_ok() {
// If the updates suceeds we return, since there is no need
// If the updates succeeds we return, since there is no need
// to try other sources.
return Ok(());
}
Expand All @@ -269,7 +269,7 @@ where
}

// Eventually we should add more information to this error about the destination
// and maybe event the certificiate.
// and maybe event the certificate.
Err(SuiError::AuthorityUpdateFailure)
}

Expand All @@ -278,7 +278,7 @@ where
///
/// FMap can do io, and returns a result V. An error there may not be fatal, and could be consumed by the
/// MReduce function to overall recover from it. This is necessary to ensure byzantine authorities cannot
/// interupt the logic of this function.
/// interrupt the logic of this function.
///
/// FReduce returns a result to a ReduceOutput. If the result is Err the function
/// shortcuts and the Err is returned. An Ok ReduceOutput result can be used to shortcut and return
Expand Down Expand Up @@ -394,7 +394,7 @@ where
|(mut total_stake, mut state), name, weight, result| {
Box::pin(async move {
// Here we increase the stake counter no matter if we got an error or not. The idea is that a
// call to ObjectInfoRequest should suceed for correct authorities no matter what. Therefore
// call to ObjectInfoRequest should succeed for correct authorities no matter what. Therefore
// if there is an error it means that we are accessing an incorrect authority. However, an
// object is final if it is on 2f+1 good nodes, and any set of 2f+1 intersects with this, so
// after we have 2f+1 of stake (good or bad) we should get a response with the object.
Expand Down Expand Up @@ -495,9 +495,9 @@ where
}

/// This function returns a map between object references owned and authorities that hold the objects
/// at this version, as well as a list of authorities that responsed to the query for the objects owned.
/// at this version, as well as a list of authorities that responded to the query for the objects owned.
///
/// We do not expose this function to users, as its output is hard for callers to interpet. In particular,
/// We do not expose this function to users, as its output is hard for callers to interpret. In particular,
/// some of the entries in the list might be the result of a query to a byzantine authority, so further
/// sanitization and checks are necessary to rely on this information.
///
Expand Down Expand Up @@ -596,7 +596,7 @@ where
// We update each object at each authority that does not have it.
for object_id in objects {
// Authorities to update.
let mut authorites: HashSet<AuthorityName> = self
let mut authorities: HashSet<AuthorityName> = self
.committee
.voting_rights
.iter()
Expand Down Expand Up @@ -637,7 +637,7 @@ where

// Remove authorities at this version, they will not need to be updated.
for (name, _signed_transaction) in object_authorities {
authorites.remove(&name);
authorities.remove(&name);
}

// NOTE: Just above we have access to signed transactions that have not quite
Expand All @@ -649,7 +649,7 @@ where
let entry = certs_to_sync
.entry(cert.transaction.digest())
.or_insert((cert.clone(), HashSet::new()));
entry.1.extend(authorites);
entry.1.extend(authorities);

// Return the latest version of an object, or a deleted object
match object_option {
Expand All @@ -663,7 +663,7 @@ where

for (_, (cert, authorities)) in certs_to_sync {
for name in authorities {
// For each certificate authority pair run a sync to upate this authority to this
// For each certificate authority pair run a sync to update this authority to this
// certificate.
// NOTE: this is right now done sequentially, we should do them in parallel using
// the usual FuturesUnordered.
Expand All @@ -684,7 +684,7 @@ where
}

/// Ask authorities for the user owned objects. Then download all objects at all versions present
/// on authorites, along with the certificates preceeding them, and update lagging authorities to
/// on authorities, along with the certificates preceding them, and update lagging authorities to
/// the latest version of the object.
///
/// This function returns all objects, including those that are
Expand Down Expand Up @@ -884,7 +884,7 @@ where
.ok_or(SuiError::ErrorWhileProcessingTransaction)
}

/// Process a certificate assuming that 2f+1 authorites already are up to date.
/// Process a certificate assuming that 2f+1 authorities already are up to date.
///
/// This call is meant to be called after `process_transaction` returns a certificate.
/// At that point (and after) enough authorities are up to date with all objects
Expand Down Expand Up @@ -992,15 +992,13 @@ where
timeout_after_quorum,
));
}
}

// Evan: couldn't we get here if entry.0 above is < threshold but response was valid? wtf??
// If instead we have more than f bad responses, then we fail.
state.bad_stake += weight;
if state.bad_stake > validity {
debug!(bad_stake = state.bad_stake,
"Too many bad responses from cert processing, validity threshold exceeded.");
return Err(SuiError::ErrorWhileRequestingCertificate);
} else {
state.bad_stake += weight;
if state.bad_stake > validity {
debug!(bad_stake = state.bad_stake,
"Too many bad responses from cert processing, validity threshold exceeded.");
return Err(SuiError::ErrorWhileRequestingCertificate);
}
}

Ok(ReduceOutput::Continue(state))
Expand Down

0 comments on commit f006b18

Please sign in to comment.