Skip to content

Commit

Permalink
Add reason to byzantine validator error (MystenLabs#3695)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lxfind
lxfind authored Aug 2, 2022
1 parent 7942def commit c40b122
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 60 deletions.
1 change: 1 addition & 0 deletions crates/sui-core/src/authority_active/gossip/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ impl GossipDigestHandler {
// But it should know the certificate!
Err(SuiError::ByzantineAuthoritySuspicion {
authority: peer_name,
reason: "Gossip peer is expected to have certificate".to_string(),
})
}
}
Expand Down
35 changes: 14 additions & 21 deletions crates/sui-core/src/authority_aggregator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,8 +488,8 @@ where
let source_client = &self.authority_clients[&source_authority];
let destination_client = &self.authority_clients[&destination_authority];

source_client.report_client_error(inner_err.clone());
destination_client.report_client_error(inner_err);
source_client.report_client_error(&inner_err);
destination_client.report_client_error(&inner_err);

debug!(
?source_authority,
Expand Down Expand Up @@ -1493,14 +1493,7 @@ where
));
}
}
maybe_err => {
// Returning Ok but without signed effects is unexpected.
let err = match maybe_err {
Err(err) => err,
Ok(_) => SuiError::ByzantineAuthoritySuspicion {
authority: name,
}
};
Err(err) => {
state.errors.push(err);
state.bad_stake += weight;
if state.bad_stake > validity {
Expand All @@ -1512,6 +1505,7 @@ where
return Err(SuiError::QuorumFailedToExecuteCertificate { errors: state.errors });
}
}
_ => { unreachable!("SafeClient should have ruled out this case") }
}
Ok(ReduceOutput::Continue(state))
})
Expand Down Expand Up @@ -1815,7 +1809,13 @@ where
if authorities.is_some() {
// The caller is passing in authorities that have claimed to have the
// cert and effects, so if they now say they don't, they're byzantine.
Err(SuiError::ByzantineAuthoritySuspicion { authority })
Err(SuiError::ByzantineAuthoritySuspicion {
authority,
reason: format!(
"Validator claimed to have the cert and effects for tx {:?} but did not return them when queried",
digests.transaction,
)
})
} else {
Err(SuiError::TransactionNotFound {
digest: digests.transaction,
Expand Down Expand Up @@ -1902,19 +1902,12 @@ where
return Ok(ReduceOutput::End(state));
}
}

// validator returned OK but did not give us an effects
Ok(_) => {
info!(?name, "peer failed to return effects");
state.errors.push((
name,
SuiError::ByzantineAuthoritySuspicion { authority: name },
));
}

Err(e) => {
state.errors.push((name, e));
}
_ => {
unreachable!("SafeClient should have ruled out this case")
}
}

let weight_remaining = total_weight - state.cumulative_weight;
Expand Down
102 changes: 67 additions & 35 deletions crates/sui-core/src/safe_client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use sui_types::{
error::{SuiError, SuiResult},
messages::*,
};
use tracing::{info, warn};
use tracing::info;

#[derive(Clone)]
pub struct SafeClient<C> {
Expand Down Expand Up @@ -60,14 +60,16 @@ impl<C> SafeClient<C> {
fp_ensure!(
signed_transaction.auth_sign_info.authority == self.address,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Unexpected validator address in the signed tx signature".to_string()
}
);
// Check it's the right transaction
fp_ensure!(
signed_transaction.digest() == digest,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Unexpected digest in the signed tx".to_string()
}
);
}
Expand All @@ -79,7 +81,8 @@ impl<C> SafeClient<C> {
fp_ensure!(
certificate.digest() == digest,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Unexpected digest in the certified tx".to_string()
}
);
}
Expand All @@ -91,22 +94,26 @@ impl<C> SafeClient<C> {
fp_ensure!(
signed_effects.auth_signature.authority == self.address,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Unexpected validator address in the signed effects signature"
.to_string()
}
);
// Checks it concerns the right tx
fp_ensure!(
signed_effects.effects.transaction_digest == *digest,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Unexpected tx digest in the signed effects".to_string()
}
);
// check that the effects digest is correct.
if let Some(effects_digest) = effects_digest {
fp_ensure!(
signed_effects.digest() == effects_digest,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Effects digest does not match with expected digest".to_string()
}
);
}
Expand All @@ -130,15 +137,17 @@ impl<C> SafeClient<C> {
fp_ensure!(
object_id == &request.object_id,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Object ID mismatch".to_string()
}
);
if let ObjectInfoRequestKind::PastObjectInfo(requested_version) = &request.request_kind
{
fp_ensure!(
version == requested_version,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Object version mismatch".to_string()
}
);
}
Expand All @@ -152,7 +161,10 @@ impl<C> SafeClient<C> {
ObjectInfoRequestKind::LatestObjectInfo(_)
),
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason:
"Object and lock data returned when request kind is not LatestObjectInfo"
.to_string()
}
);

Expand All @@ -163,7 +175,9 @@ impl<C> SafeClient<C> {
fp_ensure!(
object_and_lock.object.compute_object_reference() == obj_ref,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Requested object reference mismatch with returned object"
.to_string()
}
);
}
Expand All @@ -173,6 +187,8 @@ impl<C> SafeClient<C> {
// Otherwise the authority has inconsistent data.
return Err(SuiError::ByzantineAuthoritySuspicion {
authority: self.address,
reason: "Object returned without the object reference in response"
.to_string(),
});
}
};
Expand All @@ -183,7 +199,9 @@ impl<C> SafeClient<C> {
fp_ensure!(
signed_transaction.auth_sign_info.authority == self.address,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Unexpected validator address in the signed tx signature"
.to_string()
}
);
}
Expand Down Expand Up @@ -235,7 +253,8 @@ impl<C> SafeClient<C> {
fp_ensure!(
reconstructed_batch == signed_batch.batch,
SuiError::ByzantineAuthoritySuspicion {
authority: self.address
authority: self.address,
reason: "Inconsistent batch".to_string()
}
);
}
Expand All @@ -245,7 +264,7 @@ impl<C> SafeClient<C> {

/// This function is used by the higher level authority logic to report an
/// error that could be due to this authority.
pub fn report_client_error(&self, error: SuiError) {
pub fn report_client_error(&self, error: &SuiError) {
info!(?error, authority =? self.address, "Client error");
}
}
Expand Down Expand Up @@ -302,12 +321,28 @@ where
.handle_transaction(transaction)
.await?;
if let Err(err) = self.check_transaction_response(&digest, None, &transaction_info) {
self.report_client_error(err.clone());
self.report_client_error(&err);
return Err(err);
}
Ok(transaction_info)
}

fn verify_certificate_response(
&self,
digest: &TransactionDigest,
response: &TransactionInfoResponse,
) -> SuiResult {
fp_ensure!(
response.signed_effects.is_some(),
SuiError::ByzantineAuthoritySuspicion {
authority: self.address,
reason: "An Ok response from handle_certificate must contain signed effects"
.to_string()
}
);
self.check_transaction_response(digest, None, response)
}

/// Execute a certificate.
pub async fn handle_certificate(
&self,
Expand All @@ -319,8 +354,8 @@ where
.handle_certificate(certificate)
.await?;

if let Err(err) = self.check_transaction_response(&digest, None, &transaction_info) {
self.report_client_error(err.clone());
if let Err(err) = self.verify_certificate_response(&digest, &transaction_info) {
self.report_client_error(&err);
return Err(err);
}
Ok(transaction_info)
Expand All @@ -344,7 +379,7 @@ where
.handle_object_info_request(request.clone())
.await?;
if let Err(err) = self.check_object_response(&request, &response) {
self.report_client_error(err.clone());
self.report_client_error(&err);
return Err(err);
}
Ok(response)
Expand All @@ -362,7 +397,7 @@ where
.await?;

if let Err(err) = self.check_transaction_response(&digest, None, &transaction_info) {
self.report_client_error(err.clone());
self.report_client_error(&err);
return Err(err);
}
Ok(transaction_info)
Expand All @@ -383,7 +418,7 @@ where
Some(&digests.effects),
&transaction_info,
) {
self.report_client_error(err.clone());
self.report_client_error(&err);
return Err(err);
}
Ok(transaction_info)
Expand Down Expand Up @@ -469,17 +504,12 @@ where
.authority_client
.handle_checkpoint(request.clone())
.await?;
if let Err(err) = self.verify_checkpoint_response(&request, &resp) {
warn!(
"Potential byzantine validator {} due to: {:?}",
self.address, err
);
Err(SuiError::ByzantineAuthoritySuspicion {
authority: self.address,
})
} else {
Ok(resp)
}
self.verify_checkpoint_response(&request, &resp)
.map_err(|err| {
self.report_client_error(&err);
err
})?;
Ok(resp)
}

/// Handle Batch information requests for this authority.
Expand Down Expand Up @@ -514,7 +544,7 @@ where
signed_batch,
txs_and_last_batch,
) {
client.report_client_error(err.clone());
client.report_client_error(&err);
Some(Err(err))
} else {
// Insert a fresh vector for the new batch of transactions
Expand All @@ -528,9 +558,11 @@ where
// And here we insert the tuple into the batch.
match txs_and_last_batch {
None => {
let err =
SuiError::ByzantineAuthoritySuspicion { authority: address };
client.report_client_error(err.clone());
let err = SuiError::ByzantineAuthoritySuspicion {
authority: address,
reason: "Stream does not start with a batch".to_string(),
};
client.report_client_error(&err);
Some(Err(err))
}
Some(txs) => {
Expand Down
9 changes: 5 additions & 4 deletions crates/sui-types/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,10 +290,11 @@ pub enum SuiError {
AuthorityInformationUnavailable,
#[error("Failed to update authority.")]
AuthorityUpdateFailure,
#[error(
"We have received cryptographic level of evidence that authority {authority:?} is faulty in a Byzantine manner."
)]
ByzantineAuthoritySuspicion { authority: AuthorityName },
#[error("Validator {authority:?} is faulty in a Byzantine manner: {reason:?}")]
ByzantineAuthoritySuspicion {
authority: AuthorityName,
reason: String,
},
#[error(
"Sync from authority failed. From {xsource:?} to {destination:?}, digest {tx_digest:?}: {error:?}",
)]
Expand Down

0 comments on commit c40b122

Please sign in to comment.