Skip to content

Commit

Permalink
refactor: define custom error values for consensus crate (#6)
Browse files Browse the repository at this point in the history
Refactoring the `consensus` crate to define custom error values to be
used instead of the on-the-fly, non-formatted `anyhow` errors.

Closing
[BFT-308](https://linear.app/matterlabs/issue/BFT-308/refactor-errors-in-consensus-crate).
  • Loading branch information
moshababo authored Oct 13, 2023
1 parent 1d943ae commit 1267e79
Show file tree
Hide file tree
Showing 9 changed files with 195 additions and 96 deletions.
54 changes: 44 additions & 10 deletions node/actors/consensus/src/leader/error.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,55 @@
use roles::validator;
use thiserror::Error;

#[derive(Error, Debug)]
#[allow(clippy::missing_docs_in_private_items)]
pub(crate) enum Error {
#[error("Received replica commit message for a proposal that we don't have.")]
MissingProposal,
#[error(transparent)]
Other(#[from] anyhow::Error),
#[error("received replica commit message for a missing proposal")]
ReplicaCommitMissingProposal,
#[error("received replica commit message for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")]
ReplicaCommitOld {
current_view: validator::ViewNumber,
current_phase: validator::Phase,
},
#[error("received replica prepare message for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")]
ReplicaPrepareOld {
current_view: validator::ViewNumber,
current_phase: validator::Phase,
},
#[error("received replica commit message for a view when we are not a leader")]
ReplicaCommitWhenNotLeaderInView,
#[error("received replica prepare message for a view when we are not a leader")]
ReplicaPrepareWhenNotLeaderInView,
#[error("received replica commit message that already exists (existing message: {existing_message:?}")]
ReplicaCommitExists { existing_message: String },
#[error("received replica prepare message that already exists (existing message: {existing_message:?}")]
ReplicaPrepareExists { existing_message: String },
#[error("received replica commit message while number of received messages is below threshold. waiting for more (received: {num_messages:?}, threshold: {threshold:?}")]
ReplicaCommitNumReceivedBelowThreshold {
num_messages: usize,
threshold: usize,
},
#[error("received replica prepare message while number of received messages below threshold. waiting for more (received: {num_messages:?}, threshold: {threshold:?}")]
ReplicaPrepareNumReceivedBelowThreshold {
num_messages: usize,
threshold: usize,
},
#[error("received replica prepare message with high QC of a future view (high QC view: {high_qc_view:?}, current view: {current_view:?}")]
ReplicaPrepareHighQCOfFutureView {
high_qc_view: validator::ViewNumber,
current_view: validator::ViewNumber,
},
#[error("received replica commit message with invalid signature")]
ReplicaCommitInvalidSignature(#[source] crypto::bls12_381::Error),
#[error("received replica prepare message with invalid signature")]
ReplicaPrepareInvalidSignature(#[source] crypto::bls12_381::Error),
#[error("received replica prepare message with invalid high QC")]
ReplicaPrepareInvalidHighQC(#[source] anyhow::Error),
}

// TODO: This is only a temporary measure because anyhow::Error doesn't implement
// PartialEq. When we change all errors to thiserror, we can just derive PartialEq.
/// Needed due to source errors.
impl PartialEq for Error {
fn eq(&self, other: &Self) -> bool {
matches!(
(self, other),
(Error::MissingProposal, Error::MissingProposal) | (Error::Other(_), Error::Other(_))
)
self.to_string() == other.to_string()
}
}
29 changes: 14 additions & 15 deletions node/actors/consensus/src/leader/replica_commit.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use super::StateMachine;
use crate::{inner::ConsensusInner, leader::error::Error, metrics};
use anyhow::{anyhow, Context};
use concurrency::{ctx, metrics::LatencyHistogramExt as _};
use network::io::{ConsensusInputMessage, Target};
use roles::validator;
Expand All @@ -22,15 +21,15 @@ impl StateMachine {

// If the message is from the "past", we discard it.
if (message.view, validator::Phase::Commit) < (self.view, self.phase) {
return Err(Error::Other(anyhow!("Received replica commit message for a past view/phase.\nCurrent view: {:?}\nCurrent phase: {:?}",
self.view, self.phase)));
return Err(Error::ReplicaCommitOld {
current_view: self.view,
current_phase: self.phase,
});
}

// If the message is for a view when we are not a leader, we discard it.
if consensus.view_leader(message.view) != consensus.secret_key.public() {
return Err(Error::Other(anyhow!(
"Received replica commit message for a view when we are not a leader."
)));
return Err(Error::ReplicaCommitWhenNotLeaderInView);
}

// If we already have a message from the same validator and for the same view, we discard it.
Expand All @@ -39,25 +38,24 @@ impl StateMachine {
.get(&message.view)
.and_then(|x| x.get(author))
{
return Err(Error::Other(anyhow!(
"Received replica commit message that we already have.\nExisting message: {:?}",
existing_message
)));
return Err(Error::ReplicaCommitExists {
existing_message: format!("{:?}", existing_message),
});
}

// ----------- Checking the signed part of the message --------------

// Check the signature on the message.
signed_message
.verify()
.with_context(|| "Received replica commit message with invalid signature.")?;
.map_err(Error::ReplicaCommitInvalidSignature)?;

// ----------- Checking the contents of the message --------------

// We only accept replica commit messages for proposals that we have cached. That's so
// we don't need to store replica commit messages for different proposals.
if self.block_proposal_cache != Some(message) {
return Err(Error::MissingProposal);
return Err(Error::ReplicaCommitMissingProposal);
}

// ----------- All checks finished. Now we process the message. --------------
Expand All @@ -72,9 +70,10 @@ impl StateMachine {
let num_messages = self.commit_message_cache.get(&message.view).unwrap().len();

if num_messages < consensus.threshold() {
return Err(Error::Other(anyhow!("Received replica commit message. Waiting for more messages.\nCurrent number of messages: {}\nThreshold: {}",
num_messages,
consensus.threshold())));
return Err(Error::ReplicaCommitNumReceivedBelowThreshold {
num_messages,
threshold: consensus.threshold(),
});
}

debug_assert!(num_messages == consensus.threshold());
Expand Down
37 changes: 18 additions & 19 deletions node/actors/consensus/src/leader/replica_prepare.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use super::StateMachine;
use crate::{inner::ConsensusInner, leader::error::Error, metrics};
use anyhow::{anyhow, Context as _};
use concurrency::ctx;
use network::io::{ConsensusInputMessage, Target};
use rand::Rng;
Expand All @@ -24,15 +23,15 @@ impl StateMachine {

// If the message is from the "past", we discard it.
if (message.view, validator::Phase::Prepare) < (self.view, self.phase) {
return Err(Error::Other(anyhow!("Received replica prepare message for a past view/phase.\nCurrent view: {:?}\nCurrent phase: {:?}",
self.view, self.phase)));
return Err(Error::ReplicaPrepareOld {
current_view: self.view,
current_phase: self.phase,
});
}

// If the message is for a view when we are not a leader, we discard it.
if consensus.view_leader(message.view) != consensus.secret_key.public() {
return Err(Error::Other(anyhow!(
"Received replica prepare message for a view when we are not a leader."
)));
return Err(Error::ReplicaPrepareWhenNotLeaderInView);
}

// If we already have a message from the same validator and for the same view, we discard it.
Expand All @@ -41,35 +40,34 @@ impl StateMachine {
.get(&message.view)
.and_then(|x| x.get(author))
{
return Err(Error::Other(anyhow!(
"Received replica prepare message that we already have.\nExisting message: {:?}",
existing_message
)));
return Err(Error::ReplicaPrepareExists {
existing_message: format!("{:?}", existing_message),
});
}

// ----------- Checking the signed part of the message --------------

// Check the signature on the message.
signed_message
.verify()
.with_context(|| "Received replica prepare message with invalid signature.")?;
.map_err(Error::ReplicaPrepareInvalidSignature)?;

// ----------- Checking the contents of the message --------------

// Verify the high QC.
message
.high_qc
.verify(&consensus.validator_set, consensus.threshold())
.with_context(|| "Received replica prepare message with invalid high QC.")?;
.map_err(Error::ReplicaPrepareInvalidHighQC)?;

// If the high QC is for a future view, we discard the message.
// This check is not necessary for correctness, but it's useful to
// guarantee that our proposals don't contain QCs from the future.
if message.high_qc.message.view >= message.view {
return Err(Error::Other(anyhow!(
"Received replica prepare message with a high QC from the future.\nHigh QC view: {:?}\nCurrent view: {:?}",
message.high_qc.message.view, message.view
)));
return Err(Error::ReplicaPrepareHighQCOfFutureView {
high_qc_view: message.high_qc.message.view,
current_view: message.view,
});
}

// ----------- All checks finished. Now we process the message. --------------
Expand All @@ -84,9 +82,10 @@ impl StateMachine {
let num_messages = self.prepare_message_cache.get(&message.view).unwrap().len();

if num_messages < consensus.threshold() {
return Err(Error::Other(anyhow!("Received replica prepare message. Waiting for more messages.\nCurrent number of messages: {}\nThreshold: {}",
num_messages,
consensus.threshold())));
return Err(Error::ReplicaPrepareNumReceivedBelowThreshold {
num_messages,
threshold: consensus.threshold(),
});
}

// ----------- Creating the block proposal --------------
Expand Down
2 changes: 1 addition & 1 deletion node/actors/consensus/src/leader/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,6 @@ async fn replica_commit() {
&consensus.inner,
test_replica_msg.cast().unwrap()
),
Err(Error::MissingProposal)
Err(Error::ReplicaCommitMissingProposal)
);
}
68 changes: 68 additions & 0 deletions node/actors/consensus/src/replica/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
use crate::validator;
use roles::validator::BlockHash;
use thiserror::Error;

#[derive(Error, Debug)]
#[allow(clippy::missing_docs_in_private_items)]
pub(crate) enum Error {
#[error("received leader commit message with invalid leader (correct leader: {correct_leader:?}, received leader: {received_leader:?})]")]
LeaderCommitInvalidLeader {
correct_leader: validator::PublicKey,
received_leader: validator::PublicKey,
},
#[error("received leader prepare message with invalid leader (correct leader: {correct_leader:?}, received leader: {received_leader:?}])")]
LeaderPrepareInvalidLeader {
correct_leader: validator::PublicKey,
received_leader: validator::PublicKey,
},
#[error("received leader commit message for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")]
LeaderCommitOld {
current_view: validator::ViewNumber,
current_phase: validator::Phase,
},
#[error("received leader commit message for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")]
LeaderPrepareOld {
current_view: validator::ViewNumber,
current_phase: validator::Phase,
},
#[error("received leader commit message with invalid signature")]
LeaderCommitInvalidSignature(#[source] crypto::bls12_381::Error),
#[error("received leader prepare message with invalid signature")]
LeaderPrepareInvalidSignature(#[source] crypto::bls12_381::Error),
#[error("received leader commit message with invalid justification")]
LeaderCommitInvalidJustification(#[source] anyhow::Error),
#[error("received leader prepare message with empty map in the justification")]
LeaderPrepareJustificationWithEmptyMap,
#[error("received leader prepare message with invalid PrepareQC")]
LeaderPrepareInvalidPrepareQC(#[source] anyhow::Error),
#[error("received leader prepare message with invalid high QC")]
LeaderPrepareInvalidHighQC(#[source] anyhow::Error),
#[error("received leader prepare message with high QC of a future view (high QC view: {high_qc_view:?}, current view: {current_view:?}")]
LeaderPrepareHighQCOfFutureView {
high_qc_view: validator::ViewNumber,
current_view: validator::ViewNumber,
},
#[error("received leader prepare message with new block proposal when the previous proposal was not finalized")]
LeaderPrepareProposalWhenPreviousNotFinalized,
#[error("received leader prepare message with new block proposal with invalid parent hash (correct parent hash: {correct_parent_hash:#?}, received parent hash: {received_parent_hash:#?}, block: {block:?})")]
LeaderPrepareProposalInvalidParentHash {
correct_parent_hash: BlockHash,
received_parent_hash: BlockHash,
block: validator::Block,
},
#[error("received leader prepare message with block proposal with non-sequential number (correct proposal number: {correct_number}, received proposal number: {received_number}, block: {block:?})")]
LeaderPrepareProposalNonSequentialNumber {
correct_number: u64,
received_number: u64,
block: validator::Block,
},
#[error("received leader prepare message with block proposal with an oversized payload (payload size: {payload_size}, block: {block:?}")]
LeaderPrepareProposalOversizedPayload {
payload_size: usize,
block: validator::Block,
},
#[error("received leader prepare message with block re-proposal when the previous proposal was finalized")]
LeaderPrepareReproposalWhenFinalized,
#[error("received leader prepare message with block re-proposal of invalid block")]
LeaderPrepareReproposalInvalidBlock,
}
26 changes: 13 additions & 13 deletions node/actors/consensus/src/replica/leader_commit.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::StateMachine;
use crate::ConsensusInner;
use anyhow::{bail, Context};
use crate::{inner::ConsensusInner, replica::error::Error};

use concurrency::ctx;
use roles::validator;
use tracing::instrument;
Expand All @@ -14,7 +14,7 @@ impl StateMachine {
ctx: &ctx::Ctx,
consensus: &ConsensusInner,
signed_message: validator::Signed<validator::LeaderCommit>,
) -> anyhow::Result<()> {
) -> Result<(), Error> {
// ----------- Checking origin of the message --------------

// Unwrap message.
Expand All @@ -24,34 +24,34 @@ impl StateMachine {

// Check that it comes from the correct leader.
if author != &consensus.view_leader(view) {
bail!(
"Received leader commit message with invalid leader.\nCorrect leader: {:?}\nReceived leader: {:?}",
consensus.view_leader(view), author
);
return Err(Error::LeaderCommitInvalidLeader {
correct_leader: consensus.view_leader(view),
received_leader: author.clone(),
});
}

// If the message is from the "past", we discard it.
if (view, validator::Phase::Commit) < (self.view, self.phase) {
bail!(
"Received leader commit message for a past view/phase.\nCurrent view: {:?}\nCurrent phase: {:?}",
self.view, self.phase
);
return Err(Error::LeaderCommitOld {
current_view: self.view,
current_phase: self.phase,
});
}

// ----------- Checking the signed part of the message --------------

// Check the signature on the message.
signed_message
.verify()
.with_context(|| "Received leader commit message with invalid signature.")?;
.map_err(Error::LeaderCommitInvalidSignature)?;

// ----------- Checking the justification of the message --------------

// Verify the QuorumCertificate.
message
.justification
.verify(&consensus.validator_set, consensus.threshold())
.with_context(|| "Received leader commit message with invalid justification.")?;
.map_err(Error::LeaderCommitInvalidJustification)?;

// ----------- All checks finished. Now we process the message. --------------

Expand Down
Loading

0 comments on commit 1267e79

Please sign in to comment.