From 1267e79f30585aa68a43f10de8fb0a05c5caa224 Mon Sep 17 00:00:00 2001 From: Moshe Shababo Date: Fri, 13 Oct 2023 07:52:11 -0500 Subject: [PATCH] refactor: define custom error values for `consensus` crate (#6) 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). --- node/actors/consensus/src/leader/error.rs | 54 +++++++++++--- .../consensus/src/leader/replica_commit.rs | 29 ++++---- .../consensus/src/leader/replica_prepare.rs | 37 +++++----- node/actors/consensus/src/leader/tests.rs | 2 +- node/actors/consensus/src/replica/error.rs | 68 ++++++++++++++++++ .../consensus/src/replica/leader_commit.rs | 26 +++---- .../consensus/src/replica/leader_prepare.rs | 72 +++++++++---------- node/actors/consensus/src/replica/mod.rs | 1 + node/libs/crypto/src/bls12_381/mod.rs | 2 +- 9 files changed, 195 insertions(+), 96 deletions(-) create mode 100644 node/actors/consensus/src/replica/error.rs diff --git a/node/actors/consensus/src/leader/error.rs b/node/actors/consensus/src/leader/error.rs index 1e024270..32b05713 100644 --- a/node/actors/consensus/src/leader/error.rs +++ b/node/actors/consensus/src/leader/error.rs @@ -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() } } diff --git a/node/actors/consensus/src/leader/replica_commit.rs b/node/actors/consensus/src/leader/replica_commit.rs index d9d4e948..9a1634d8 100644 --- a/node/actors/consensus/src/leader/replica_commit.rs +++ b/node/actors/consensus/src/leader/replica_commit.rs @@ -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; @@ -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. @@ -39,10 +38,9 @@ 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 -------------- @@ -50,14 +48,14 @@ impl StateMachine { // 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. -------------- @@ -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()); diff --git a/node/actors/consensus/src/leader/replica_prepare.rs b/node/actors/consensus/src/leader/replica_prepare.rs index 912807ad..5a55be53 100644 --- a/node/actors/consensus/src/leader/replica_prepare.rs +++ b/node/actors/consensus/src/leader/replica_prepare.rs @@ -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; @@ -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. @@ -41,10 +40,9 @@ 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 -------------- @@ -52,7 +50,7 @@ impl StateMachine { // 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 -------------- @@ -60,16 +58,16 @@ impl StateMachine { 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. -------------- @@ -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 -------------- diff --git a/node/actors/consensus/src/leader/tests.rs b/node/actors/consensus/src/leader/tests.rs index b5d18417..19949cfd 100644 --- a/node/actors/consensus/src/leader/tests.rs +++ b/node/actors/consensus/src/leader/tests.rs @@ -36,6 +36,6 @@ async fn replica_commit() { &consensus.inner, test_replica_msg.cast().unwrap() ), - Err(Error::MissingProposal) + Err(Error::ReplicaCommitMissingProposal) ); } diff --git a/node/actors/consensus/src/replica/error.rs b/node/actors/consensus/src/replica/error.rs new file mode 100644 index 00000000..4e16cd2f --- /dev/null +++ b/node/actors/consensus/src/replica/error.rs @@ -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, +} diff --git a/node/actors/consensus/src/replica/leader_commit.rs b/node/actors/consensus/src/replica/leader_commit.rs index 471d4831..352117c6 100644 --- a/node/actors/consensus/src/replica/leader_commit.rs +++ b/node/actors/consensus/src/replica/leader_commit.rs @@ -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; @@ -14,7 +14,7 @@ impl StateMachine { ctx: &ctx::Ctx, consensus: &ConsensusInner, signed_message: validator::Signed, - ) -> anyhow::Result<()> { + ) -> Result<(), Error> { // ----------- Checking origin of the message -------------- // Unwrap message. @@ -24,18 +24,18 @@ 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 -------------- @@ -43,7 +43,7 @@ impl StateMachine { // 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 -------------- @@ -51,7 +51,7 @@ impl StateMachine { 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. -------------- diff --git a/node/actors/consensus/src/replica/leader_prepare.rs b/node/actors/consensus/src/replica/leader_prepare.rs index a38ee194..df479c3f 100644 --- a/node/actors/consensus/src/replica/leader_prepare.rs +++ b/node/actors/consensus/src/replica/leader_prepare.rs @@ -1,6 +1,5 @@ use super::StateMachine; -use crate::ConsensusInner; -use anyhow::{bail, Context}; +use crate::{inner::ConsensusInner, replica::error::Error}; use concurrency::ctx; use network::io::{ConsensusInputMessage, Target}; use roles::validator; @@ -15,7 +14,7 @@ impl StateMachine { ctx: &ctx::Ctx, consensus: &ConsensusInner, signed_message: validator::Signed, - ) -> anyhow::Result<()> { + ) -> Result<(), Error> { // ----------- Checking origin of the message -------------- // Unwrap message. @@ -25,31 +24,31 @@ impl StateMachine { .justification .map .first_key_value() - .context("Received leader prepare message with empty map in the justification.")? + .ok_or(Error::LeaderPrepareJustificationWithEmptyMap)? .0 .view; // Check that it comes from the correct leader. if author != &consensus.view_leader(view) { - bail!( - "Received leader prepare message with invalid leader.\nCorrect leader: {:?}\nReceived leader: {:?}", - consensus.view_leader(view), author - ); + return Err(Error::LeaderPrepareInvalidLeader { + correct_leader: consensus.view_leader(view), + received_leader: author.clone(), + }); } // If the message is from the "past", we discard it. if (view, validator::Phase::Prepare) < (self.view, self.phase) { - bail!( - "Received leader prepare message for a past view/phase.\nCurrent view: {:?}\nCurrent phase: {:?}", - self.view, self.phase - ); + return Err(Error::LeaderPrepareOld { + current_view: self.view, + current_phase: self.phase, + }); } // ----------- Checking the signed part of the message -------------- signed_message .verify() - .with_context(|| "Received leader prepare message with invalid signature.")?; + .map_err(Error::LeaderPrepareInvalidSignature)?; // ----------- Checking the justification of the message -------------- @@ -57,7 +56,7 @@ impl StateMachine { message .justification .verify(&consensus.validator_set, consensus.threshold()) - .with_context(|| "Received leader prepare message with invalid PrepareQC.")?; + .map_err(Error::LeaderPrepareInvalidPrepareQC)?; // Get the highest block voted and check if there's a quorum of votes for it. To have a quorum // in this situation, we require 2*f+1 votes, where f is the maximum number of faulty replicas. @@ -90,16 +89,16 @@ impl StateMachine { highest_qc .verify(&consensus.validator_set, consensus.threshold()) - .with_context(|| "Received leader prepare message with invalid highest QC.")?; + .map_err(Error::LeaderPrepareInvalidHighQC)?; // 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 messages don't contain QCs from the future. if highest_qc.message.view >= view { - bail!( - "Received leader prepare message with a highest QC from the future.\nHighest QC view: {:?}\nCurrent view: {:?}", - highest_qc.message.view, view - ); + return Err(Error::LeaderPrepareHighQCOfFutureView { + high_qc_view: highest_qc.message.view, + current_view: view, + }); } // Try to create a finalized block with this CommitQC and our block proposal cache. @@ -120,32 +119,31 @@ impl StateMachine { highest_qc.message.proposal_block_hash, )) { - bail!( - "Received new block proposal when the previous proposal was not finalized." - ); + return Err(Error::LeaderPrepareProposalWhenPreviousNotFinalized); } if highest_qc.message.proposal_block_hash != block.parent { - bail!( - "Received block proposal with invalid parent hash.\nCorrect parent hash: {:#?}\nReceived parent hash: {:#?}\nBlock: {:?}", - highest_qc.message.proposal_block_hash, block.parent, block - ); + return Err(Error::LeaderPrepareProposalInvalidParentHash { + correct_parent_hash: highest_qc.message.proposal_block_hash, + received_parent_hash: block.parent, + block: block.clone(), + }); } if highest_qc.message.proposal_block_number.next() != block.number { - bail!( - "Received block proposal with non-sequential number.\nCorrect proposal number: {}\nReceived proposal number: {}\nBlock: {:?}", - highest_qc.message.proposal_block_number.next().0, block.number.0, block - ); + return Err(Error::LeaderPrepareProposalNonSequentialNumber { + correct_number: highest_qc.message.proposal_block_number.next().0, + received_number: block.number.0, + block: block.clone(), + }); } // Check that the payload doesn't exceed the maximum size. if block.payload.len() > ConsensusInner::PAYLOAD_MAX_SIZE { - bail!( - "Received block proposal with too large payload.\nPayload size: {}\nBlock: {:?}", - block.payload.len(), - block - ); + return Err(Error::LeaderPrepareProposalOversizedPayload { + payload_size: block.payload.len(), + block: block.clone(), + }); } (block.number, block.hash(), Some(block)) @@ -159,7 +157,7 @@ impl StateMachine { highest_qc.message.proposal_block_hash, )) { - bail!("Received block reproposal when the previous proposal was finalized."); + return Err(Error::LeaderPrepareReproposalWhenFinalized); } if highest_vote.unwrap() @@ -168,7 +166,7 @@ impl StateMachine { commit_vote.proposal_block_hash, ) { - bail!("Received invalid block re-proposal.",); + return Err(Error::LeaderPrepareReproposalInvalidBlock); } (highest_vote.unwrap().0, highest_vote.unwrap().1, None) diff --git a/node/actors/consensus/src/replica/mod.rs b/node/actors/consensus/src/replica/mod.rs index e76e045f..556aab63 100644 --- a/node/actors/consensus/src/replica/mod.rs +++ b/node/actors/consensus/src/replica/mod.rs @@ -3,6 +3,7 @@ //! node will perform both the replica and leader roles simultaneously. mod block; +mod error; mod leader_commit; mod leader_prepare; mod new_view; diff --git a/node/libs/crypto/src/bls12_381/mod.rs b/node/libs/crypto/src/bls12_381/mod.rs index 53f9833f..505c48f6 100644 --- a/node/libs/crypto/src/bls12_381/mod.rs +++ b/node/libs/crypto/src/bls12_381/mod.rs @@ -203,7 +203,7 @@ impl Ord for AggregateSignature { } /// Error type for generating and interacting with BLS keys/signatures -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, PartialEq)] #[non_exhaustive] pub enum Error { /// Signature verification failure