From 075143dd0a9b3c615d4afcac4cd1c4bfac0250b1 Mon Sep 17 00:00:00 2001 From: moshababo Date: Tue, 10 Oct 2023 20:32:27 -0500 Subject: [PATCH 1/4] refactor: define custom error values for replica messages --- node/actors/consensus/src/leader/error.rs | 71 +++++++++++++++---- .../consensus/src/leader/replica_commit.rs | 35 ++++----- .../consensus/src/leader/replica_prepare.rs | 45 ++++++------ node/actors/consensus/src/leader/tests.rs | 4 +- 4 files changed, 102 insertions(+), 53 deletions(-) diff --git a/node/actors/consensus/src/leader/error.rs b/node/actors/consensus/src/leader/error.rs index 1e024270..1a118aa6 100644 --- a/node/actors/consensus/src/leader/error.rs +++ b/node/actors/consensus/src/leader/error.rs @@ -1,21 +1,66 @@ use thiserror::Error; +use roles::validator; #[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), +pub(crate) enum ReplicaMessageError { + #[error("commit for a missing proposal")] + CommitMissingProposal, + #[error("commit for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")] + CommitOld { + current_view: validator::ViewNumber, + current_phase: validator::Phase, + }, + #[error("prepare for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")] + PrepareOld { + current_view: validator::ViewNumber, + current_phase: validator::Phase, + }, + #[error("commit for a view when we are not a leader")] + CommitWhenNotLeaderInView, + #[error("prepare for a view when we are not a leader")] + PrepareWhenNotLeaderInView, + #[error("commit duplicated (existing message: {existing_message:?}")] + CommitDuplicated { + existing_message: String + }, + #[error("prepare duplicated (existing message: {existing_message:?}")] + PrepareDuplicated { + existing_message: String + }, + #[error("commit while number of received messages below threshold, waiting for more (received: {num_messages:?}, threshold: {threshold:?}")] + CommitNumReceivedBelowThreshold { + num_messages: usize, + threshold: usize, + }, + #[error("prepare while number of received messages below threshold, waiting for more (received: {num_messages:?}, threshold: {threshold:?}")] + PrepareNumReceivedBelowThreshold { + num_messages: usize, + threshold: usize, + }, + #[error("prepare with high QC of a future view (high QC view: {high_qc_view:?}, current view: {current_view:?}")] + PrepareHighQCOfFutureView { + high_qc_view: validator::ViewNumber, + current_view: validator::ViewNumber, + }, + #[error("commit with invalid signature")] + CommitInvalidSignature { + inner_err: crypto::bls12_381::Error + }, + #[error("prepare with invalid signature")] + PrepareInvalidSignature { + inner_err: crypto::bls12_381::Error + }, + #[error("prepare with invalid high QC")] + PrepareInvalidHighQC { + inner_err: 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. -impl PartialEq for Error { + +/// Needed due to inner errors. +impl PartialEq for ReplicaMessageError { fn eq(&self, other: &Self) -> bool { - matches!( - (self, other), - (Error::MissingProposal, Error::MissingProposal) | (Error::Other(_), Error::Other(_)) - ) + self.to_string() == other.to_string() } -} +} \ No newline at end of file diff --git a/node/actors/consensus/src/leader/replica_commit.rs b/node/actors/consensus/src/leader/replica_commit.rs index d9d4e948..bb7c1ce2 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 crate::{inner::ConsensusInner, leader::error::ReplicaMessageError, metrics}; use concurrency::{ctx, metrics::LatencyHistogramExt as _}; use network::io::{ConsensusInputMessage, Target}; use roles::validator; @@ -13,7 +12,7 @@ impl StateMachine { ctx: &ctx::Ctx, consensus: &ConsensusInner, signed_message: validator::Signed, - ) -> Result<(), Error> { + ) -> Result<(), ReplicaMessageError> { // ----------- Checking origin of the message -------------- // Unwrap message. @@ -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(ReplicaMessageError::CommitOld { + 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(ReplicaMessageError::CommitWhenNotLeaderInView); } // 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(ReplicaMessageError::CommitDuplicated { + existing_message: format!("{:?}", existing_message), + }) } // ----------- Checking the signed part of the message -------------- @@ -50,14 +48,16 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .with_context(|| "Received replica commit message with invalid signature.")?; + .map_err(|err| ReplicaMessageError::CommitInvalidSignature { + inner_err: err + })?; // ----------- 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(ReplicaMessageError::CommitMissingProposal); } // ----------- All checks finished. Now we process the message. -------------- @@ -72,9 +72,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(ReplicaMessageError::CommitNumReceivedBelowThreshold { + 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..950410d1 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 crate::{inner::ConsensusInner, leader::error::ReplicaMessageError, metrics}; use concurrency::ctx; use network::io::{ConsensusInputMessage, Target}; use rand::Rng; @@ -15,7 +14,7 @@ impl StateMachine { ctx: &ctx::Ctx, consensus: &ConsensusInner, signed_message: validator::Signed, - ) -> Result<(), Error> { + ) -> Result<(), ReplicaMessageError> { // ----------- Checking origin of the message -------------- // Unwrap message. @@ -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(ReplicaMessageError::PrepareOld { + 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(ReplicaMessageError::PrepareWhenNotLeaderInView) } // 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(ReplicaMessageError::PrepareDuplicated { + existing_message: format!("{:?}", existing_message), + }) } // ----------- Checking the signed part of the message -------------- @@ -52,7 +50,9 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .with_context(|| "Received replica prepare message with invalid signature.")?; + .map_err(|err| ReplicaMessageError::PrepareInvalidSignature { + inner_err: err + })?; // ----------- Checking the contents of the message -------------- @@ -60,16 +60,18 @@ impl StateMachine { message .high_qc .verify(&consensus.validator_set, consensus.threshold()) - .with_context(|| "Received replica prepare message with invalid high QC.")?; + .map_err(|err| ReplicaMessageError::PrepareInvalidHighQC { + inner_err: err + })?; // 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(ReplicaMessageError::PrepareHighQCOfFutureView { + high_qc_view: message.high_qc.message.view, + current_view: message.view, + }) } // ----------- All checks finished. Now we process the message. -------------- @@ -84,9 +86,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(ReplicaMessageError::PrepareNumReceivedBelowThreshold { + 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..2ce9ca63 100644 --- a/node/actors/consensus/src/leader/tests.rs +++ b/node/actors/consensus/src/leader/tests.rs @@ -1,4 +1,4 @@ -use crate::{leader::error::Error, testonly}; +use crate::{leader::error::ReplicaMessageError, testonly}; use concurrency::ctx; use rand::{rngs::StdRng, Rng, SeedableRng}; use roles::validator; @@ -36,6 +36,6 @@ async fn replica_commit() { &consensus.inner, test_replica_msg.cast().unwrap() ), - Err(Error::MissingProposal) + Err(ReplicaMessageError::CommitMissingProposal) ); } From 0b86475863c0e60c41114763987d9a6d0929f098 Mon Sep 17 00:00:00 2001 From: moshababo Date: Wed, 11 Oct 2023 10:46:05 -0500 Subject: [PATCH 2/4] cargo fmt --- node/actors/consensus/src/leader/error.rs | 25 ++++++------------- .../consensus/src/leader/replica_commit.rs | 14 +++++------ .../consensus/src/leader/replica_prepare.rs | 20 ++++++--------- 3 files changed, 21 insertions(+), 38 deletions(-) diff --git a/node/actors/consensus/src/leader/error.rs b/node/actors/consensus/src/leader/error.rs index 1a118aa6..4a063aca 100644 --- a/node/actors/consensus/src/leader/error.rs +++ b/node/actors/consensus/src/leader/error.rs @@ -1,5 +1,5 @@ -use thiserror::Error; use roles::validator; +use thiserror::Error; #[derive(Error, Debug)] #[allow(clippy::missing_docs_in_private_items)] @@ -21,13 +21,9 @@ pub(crate) enum ReplicaMessageError { #[error("prepare for a view when we are not a leader")] PrepareWhenNotLeaderInView, #[error("commit duplicated (existing message: {existing_message:?}")] - CommitDuplicated { - existing_message: String - }, + CommitDuplicated { existing_message: String }, #[error("prepare duplicated (existing message: {existing_message:?}")] - PrepareDuplicated { - existing_message: String - }, + PrepareDuplicated { existing_message: String }, #[error("commit while number of received messages below threshold, waiting for more (received: {num_messages:?}, threshold: {threshold:?}")] CommitNumReceivedBelowThreshold { num_messages: usize, @@ -44,23 +40,16 @@ pub(crate) enum ReplicaMessageError { current_view: validator::ViewNumber, }, #[error("commit with invalid signature")] - CommitInvalidSignature { - inner_err: crypto::bls12_381::Error - }, + CommitInvalidSignature { inner_err: crypto::bls12_381::Error }, #[error("prepare with invalid signature")] - PrepareInvalidSignature { - inner_err: crypto::bls12_381::Error - }, + PrepareInvalidSignature { inner_err: crypto::bls12_381::Error }, #[error("prepare with invalid high QC")] - PrepareInvalidHighQC { - inner_err: anyhow::Error - } + PrepareInvalidHighQC { inner_err: anyhow::Error }, } - /// Needed due to inner errors. impl PartialEq for ReplicaMessageError { fn eq(&self, other: &Self) -> bool { self.to_string() == other.to_string() } -} \ No newline at end of file +} diff --git a/node/actors/consensus/src/leader/replica_commit.rs b/node/actors/consensus/src/leader/replica_commit.rs index bb7c1ce2..31652df2 100644 --- a/node/actors/consensus/src/leader/replica_commit.rs +++ b/node/actors/consensus/src/leader/replica_commit.rs @@ -23,8 +23,8 @@ impl StateMachine { if (message.view, validator::Phase::Commit) < (self.view, self.phase) { return Err(ReplicaMessageError::CommitOld { current_view: self.view, - current_phase: self.phase - }) + current_phase: self.phase, + }); } // If the message is for a view when we are not a leader, we discard it. @@ -40,7 +40,7 @@ impl StateMachine { { return Err(ReplicaMessageError::CommitDuplicated { existing_message: format!("{:?}", existing_message), - }) + }); } // ----------- Checking the signed part of the message -------------- @@ -48,9 +48,7 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| ReplicaMessageError::CommitInvalidSignature { - inner_err: err - })?; + .map_err(|err| ReplicaMessageError::CommitInvalidSignature { inner_err: err })?; // ----------- Checking the contents of the message -------------- @@ -74,8 +72,8 @@ impl StateMachine { if num_messages < consensus.threshold() { return Err(ReplicaMessageError::CommitNumReceivedBelowThreshold { num_messages, - threshold: consensus.threshold() - }) + 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 950410d1..ad8fba43 100644 --- a/node/actors/consensus/src/leader/replica_prepare.rs +++ b/node/actors/consensus/src/leader/replica_prepare.rs @@ -26,12 +26,12 @@ impl StateMachine { return Err(ReplicaMessageError::PrepareOld { 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(ReplicaMessageError::PrepareWhenNotLeaderInView) + return Err(ReplicaMessageError::PrepareWhenNotLeaderInView); } // If we already have a message from the same validator and for the same view, we discard it. @@ -42,7 +42,7 @@ impl StateMachine { { return Err(ReplicaMessageError::PrepareDuplicated { existing_message: format!("{:?}", existing_message), - }) + }); } // ----------- Checking the signed part of the message -------------- @@ -50,9 +50,7 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| ReplicaMessageError::PrepareInvalidSignature { - inner_err: err - })?; + .map_err(|err| ReplicaMessageError::PrepareInvalidSignature { inner_err: err })?; // ----------- Checking the contents of the message -------------- @@ -60,9 +58,7 @@ impl StateMachine { message .high_qc .verify(&consensus.validator_set, consensus.threshold()) - .map_err(|err| ReplicaMessageError::PrepareInvalidHighQC { - inner_err: err - })?; + .map_err(|err| ReplicaMessageError::PrepareInvalidHighQC { inner_err: err })?; // 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 @@ -71,7 +67,7 @@ impl StateMachine { return Err(ReplicaMessageError::PrepareHighQCOfFutureView { high_qc_view: message.high_qc.message.view, current_view: message.view, - }) + }); } // ----------- All checks finished. Now we process the message. -------------- @@ -88,8 +84,8 @@ impl StateMachine { if num_messages < consensus.threshold() { return Err(ReplicaMessageError::PrepareNumReceivedBelowThreshold { num_messages, - threshold: consensus.threshold() - }) + threshold: consensus.threshold(), + }); } // ----------- Creating the block proposal -------------- From b545e10fb074f9c322536709282ff97daaf00d78 Mon Sep 17 00:00:00 2001 From: moshababo Date: Thu, 12 Oct 2023 22:59:07 -0500 Subject: [PATCH 3/4] PR fixes --- node/actors/consensus/src/leader/error.rs | 58 +++++++-------- .../consensus/src/leader/replica_commit.rs | 16 ++--- .../consensus/src/leader/replica_prepare.rs | 18 ++--- node/actors/consensus/src/leader/tests.rs | 4 +- 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, 166 insertions(+), 99 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 4a063aca..32b05713 100644 --- a/node/actors/consensus/src/leader/error.rs +++ b/node/actors/consensus/src/leader/error.rs @@ -3,52 +3,52 @@ use thiserror::Error; #[derive(Error, Debug)] #[allow(clippy::missing_docs_in_private_items)] -pub(crate) enum ReplicaMessageError { - #[error("commit for a missing proposal")] - CommitMissingProposal, - #[error("commit for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")] - CommitOld { +pub(crate) enum 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("prepare for a past view/phase (current view: {current_view:?}, current phase: {current_phase:?})")] - PrepareOld { + #[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("commit for a view when we are not a leader")] - CommitWhenNotLeaderInView, - #[error("prepare for a view when we are not a leader")] - PrepareWhenNotLeaderInView, - #[error("commit duplicated (existing message: {existing_message:?}")] - CommitDuplicated { existing_message: String }, - #[error("prepare duplicated (existing message: {existing_message:?}")] - PrepareDuplicated { existing_message: String }, - #[error("commit while number of received messages below threshold, waiting for more (received: {num_messages:?}, threshold: {threshold:?}")] - CommitNumReceivedBelowThreshold { + #[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("prepare while number of received messages below threshold, waiting for more (received: {num_messages:?}, threshold: {threshold:?}")] - PrepareNumReceivedBelowThreshold { + #[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("prepare with high QC of a future view (high QC view: {high_qc_view:?}, current view: {current_view:?}")] - PrepareHighQCOfFutureView { + #[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("commit with invalid signature")] - CommitInvalidSignature { inner_err: crypto::bls12_381::Error }, - #[error("prepare with invalid signature")] - PrepareInvalidSignature { inner_err: crypto::bls12_381::Error }, - #[error("prepare with invalid high QC")] - PrepareInvalidHighQC { inner_err: anyhow::Error }, + #[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), } -/// Needed due to inner errors. -impl PartialEq for ReplicaMessageError { +/// Needed due to source errors. +impl PartialEq for Error { fn eq(&self, other: &Self) -> bool { 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 31652df2..3bfb3f54 100644 --- a/node/actors/consensus/src/leader/replica_commit.rs +++ b/node/actors/consensus/src/leader/replica_commit.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::{inner::ConsensusInner, leader::error::ReplicaMessageError, metrics}; +use crate::{inner::ConsensusInner, leader::error::Error, metrics}; use concurrency::{ctx, metrics::LatencyHistogramExt as _}; use network::io::{ConsensusInputMessage, Target}; use roles::validator; @@ -12,7 +12,7 @@ impl StateMachine { ctx: &ctx::Ctx, consensus: &ConsensusInner, signed_message: validator::Signed, - ) -> Result<(), ReplicaMessageError> { + ) -> Result<(), Error> { // ----------- Checking origin of the message -------------- // Unwrap message. @@ -21,7 +21,7 @@ impl StateMachine { // If the message is from the "past", we discard it. if (message.view, validator::Phase::Commit) < (self.view, self.phase) { - return Err(ReplicaMessageError::CommitOld { + return Err(Error::ReplicaCommitOld { current_view: self.view, current_phase: self.phase, }); @@ -29,7 +29,7 @@ impl StateMachine { // 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(ReplicaMessageError::CommitWhenNotLeaderInView); + return Err(Error::ReplicaCommitWhenNotLeaderInView); } // If we already have a message from the same validator and for the same view, we discard it. @@ -38,7 +38,7 @@ impl StateMachine { .get(&message.view) .and_then(|x| x.get(author)) { - return Err(ReplicaMessageError::CommitDuplicated { + return Err(Error::ReplicaCommitExists { existing_message: format!("{:?}", existing_message), }); } @@ -48,14 +48,14 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| ReplicaMessageError::CommitInvalidSignature { inner_err: err })?; + .map_err(|err| Error::ReplicaCommitInvalidSignature(err))?; // ----------- 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(ReplicaMessageError::CommitMissingProposal); + return Err(Error::ReplicaCommitMissingProposal); } // ----------- All checks finished. Now we process the message. -------------- @@ -70,7 +70,7 @@ impl StateMachine { let num_messages = self.commit_message_cache.get(&message.view).unwrap().len(); if num_messages < consensus.threshold() { - return Err(ReplicaMessageError::CommitNumReceivedBelowThreshold { + return Err(Error::ReplicaCommitNumReceivedBelowThreshold { num_messages, threshold: consensus.threshold(), }); diff --git a/node/actors/consensus/src/leader/replica_prepare.rs b/node/actors/consensus/src/leader/replica_prepare.rs index ad8fba43..862c250e 100644 --- a/node/actors/consensus/src/leader/replica_prepare.rs +++ b/node/actors/consensus/src/leader/replica_prepare.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::{inner::ConsensusInner, leader::error::ReplicaMessageError, metrics}; +use crate::{inner::ConsensusInner, leader::error::Error, metrics}; use concurrency::ctx; use network::io::{ConsensusInputMessage, Target}; use rand::Rng; @@ -14,7 +14,7 @@ impl StateMachine { ctx: &ctx::Ctx, consensus: &ConsensusInner, signed_message: validator::Signed, - ) -> Result<(), ReplicaMessageError> { + ) -> Result<(), Error> { // ----------- Checking origin of the message -------------- // Unwrap message. @@ -23,7 +23,7 @@ impl StateMachine { // If the message is from the "past", we discard it. if (message.view, validator::Phase::Prepare) < (self.view, self.phase) { - return Err(ReplicaMessageError::PrepareOld { + return Err(Error::ReplicaPrepareOld { current_view: self.view, current_phase: self.phase, }); @@ -31,7 +31,7 @@ impl StateMachine { // 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(ReplicaMessageError::PrepareWhenNotLeaderInView); + return Err(Error::ReplicaPrepareWhenNotLeaderInView); } // If we already have a message from the same validator and for the same view, we discard it. @@ -40,7 +40,7 @@ impl StateMachine { .get(&message.view) .and_then(|x| x.get(author)) { - return Err(ReplicaMessageError::PrepareDuplicated { + return Err(Error::ReplicaPrepareExists { existing_message: format!("{:?}", existing_message), }); } @@ -50,7 +50,7 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| ReplicaMessageError::PrepareInvalidSignature { inner_err: err })?; + .map_err(|err| Error::ReplicaPrepareInvalidSignature(err))?; // ----------- Checking the contents of the message -------------- @@ -58,13 +58,13 @@ impl StateMachine { message .high_qc .verify(&consensus.validator_set, consensus.threshold()) - .map_err(|err| ReplicaMessageError::PrepareInvalidHighQC { inner_err: err })?; + .map_err(|err| Error::ReplicaPrepareInvalidHighQC(err))?; // 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(ReplicaMessageError::PrepareHighQCOfFutureView { + return Err(Error::ReplicaPrepareHighQCOfFutureView { high_qc_view: message.high_qc.message.view, current_view: message.view, }); @@ -82,7 +82,7 @@ impl StateMachine { let num_messages = self.prepare_message_cache.get(&message.view).unwrap().len(); if num_messages < consensus.threshold() { - return Err(ReplicaMessageError::PrepareNumReceivedBelowThreshold { + return Err(Error::ReplicaPrepareNumReceivedBelowThreshold { num_messages, threshold: consensus.threshold(), }); diff --git a/node/actors/consensus/src/leader/tests.rs b/node/actors/consensus/src/leader/tests.rs index 2ce9ca63..19949cfd 100644 --- a/node/actors/consensus/src/leader/tests.rs +++ b/node/actors/consensus/src/leader/tests.rs @@ -1,4 +1,4 @@ -use crate::{leader::error::ReplicaMessageError, testonly}; +use crate::{leader::error::Error, testonly}; use concurrency::ctx; use rand::{rngs::StdRng, Rng, SeedableRng}; use roles::validator; @@ -36,6 +36,6 @@ async fn replica_commit() { &consensus.inner, test_replica_msg.cast().unwrap() ), - Err(ReplicaMessageError::CommitMissingProposal) + 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..689b8296 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(|err| Error::LeaderCommitInvalidSignature(err))?; // ----------- 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(|err| Error::LeaderCommitInvalidJustification(err))?; // ----------- 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..204e2378 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(|err| Error::LeaderPrepareInvalidSignature(err))?; // ----------- 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(|err| Error::LeaderPrepareInvalidPrepareQC(err))?; // 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(|err| Error::LeaderPrepareInvalidHighQC(err))?; // 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 From f252a49908e8425942245a8eb98bbf67b69caaed Mon Sep 17 00:00:00 2001 From: moshababo Date: Thu, 12 Oct 2023 23:12:43 -0500 Subject: [PATCH 4/4] Remove redundant closures --- node/actors/consensus/src/leader/replica_commit.rs | 2 +- node/actors/consensus/src/leader/replica_prepare.rs | 4 ++-- node/actors/consensus/src/replica/leader_commit.rs | 4 ++-- node/actors/consensus/src/replica/leader_prepare.rs | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/node/actors/consensus/src/leader/replica_commit.rs b/node/actors/consensus/src/leader/replica_commit.rs index 3bfb3f54..9a1634d8 100644 --- a/node/actors/consensus/src/leader/replica_commit.rs +++ b/node/actors/consensus/src/leader/replica_commit.rs @@ -48,7 +48,7 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| Error::ReplicaCommitInvalidSignature(err))?; + .map_err(Error::ReplicaCommitInvalidSignature)?; // ----------- Checking the contents of the message -------------- diff --git a/node/actors/consensus/src/leader/replica_prepare.rs b/node/actors/consensus/src/leader/replica_prepare.rs index 862c250e..5a55be53 100644 --- a/node/actors/consensus/src/leader/replica_prepare.rs +++ b/node/actors/consensus/src/leader/replica_prepare.rs @@ -50,7 +50,7 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| Error::ReplicaPrepareInvalidSignature(err))?; + .map_err(Error::ReplicaPrepareInvalidSignature)?; // ----------- Checking the contents of the message -------------- @@ -58,7 +58,7 @@ impl StateMachine { message .high_qc .verify(&consensus.validator_set, consensus.threshold()) - .map_err(|err| Error::ReplicaPrepareInvalidHighQC(err))?; + .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 diff --git a/node/actors/consensus/src/replica/leader_commit.rs b/node/actors/consensus/src/replica/leader_commit.rs index 689b8296..352117c6 100644 --- a/node/actors/consensus/src/replica/leader_commit.rs +++ b/node/actors/consensus/src/replica/leader_commit.rs @@ -43,7 +43,7 @@ impl StateMachine { // Check the signature on the message. signed_message .verify() - .map_err(|err| Error::LeaderCommitInvalidSignature(err))?; + .map_err(Error::LeaderCommitInvalidSignature)?; // ----------- Checking the justification of the message -------------- @@ -51,7 +51,7 @@ impl StateMachine { message .justification .verify(&consensus.validator_set, consensus.threshold()) - .map_err(|err| Error::LeaderCommitInvalidJustification(err))?; + .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 204e2378..df479c3f 100644 --- a/node/actors/consensus/src/replica/leader_prepare.rs +++ b/node/actors/consensus/src/replica/leader_prepare.rs @@ -48,7 +48,7 @@ impl StateMachine { signed_message .verify() - .map_err(|err| Error::LeaderPrepareInvalidSignature(err))?; + .map_err(Error::LeaderPrepareInvalidSignature)?; // ----------- Checking the justification of the message -------------- @@ -56,7 +56,7 @@ impl StateMachine { message .justification .verify(&consensus.validator_set, consensus.threshold()) - .map_err(|err| Error::LeaderPrepareInvalidPrepareQC(err))?; + .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. @@ -89,7 +89,7 @@ impl StateMachine { highest_qc .verify(&consensus.validator_set, consensus.threshold()) - .map_err(|err| Error::LeaderPrepareInvalidHighQC(err))?; + .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