Add descriptive block validation errors

Author: bamrith

Cargo.lock

@@ -35,7 +35,7 @@ use monad_consensus_types::{
     block::{
         BlockPolicy, BlockRange, ConsensusBlockHeader, ConsensusFullBlock, OptimisticPolicyCommit,
     },
-    block_validator::{BlockValidationError, BlockValidator},
+    block_validator::BlockValidator,
     checkpoint::{Checkpoint, LockedEpoch, RootInfo},
     metrics::Metrics,
     no_endorsement::{FreshProposalCertificate, NoEndorsement},
@@ -583,74 +583,16 @@ where
             body,
             Some(author_pubkey),
             &self.config.chain_config,
+            self.metrics,
         ) {
             Ok(block) => block,
-            Err(BlockValidationError::SystemTxnError) => {
-                warn!(
-                    ?block_round,
-                    ?block_author,
-                    ?seq_num,
-                    "dropping proposal, system transaction validation failed"
-                );
-                self.metrics.consensus_events.failed_txn_validation += 1;
-                return None;
-            }
-            Err(BlockValidationError::TxnError) => {
-                warn!(
-                    ?block_round,
-                    ?block_author,
-                    ?seq_num,
-                    "dropping proposal, transaction validation failed"
-                );
-                self.metrics.consensus_events.failed_txn_validation += 1;
-                return None;
-            }
-            Err(BlockValidationError::RandaoError) => {
-                warn!(
-                    ?block_round,
-                    ?block_author,
-                    ?seq_num,
-                    "dropping proposal, randao validation failed"
-                );
-                self.metrics
-                    .consensus_events
-                    .failed_verify_randao_reveal_sig += 1;
-                return None;
-            }
-            Err(BlockValidationError::HeaderPayloadMismatchError) => {
-                // TODO: this is malicious behaviour?
-                warn!(
-                    ?block_round,
-                    ?block_author,
-                    ?seq_num,
-                    "dropping proposal, header payload mismatch"
-                );
-                return None;
-            }
-            Err(BlockValidationError::PayloadError) => {
-                warn!(
-                    ?block_round,
-                    ?block_author,
-                    ?seq_num,
-                    "dropping proposal, payload validation failed"
-                );
-                return None;
-            }
-            Err(BlockValidationError::HeaderError) => {
-                warn!(
-                    ?block_round,
-                    ?block_author,
-                    ?seq_num,
-                    "dropping proposal, header validation failed"
-                );
-                return None;
-            }
-            Err(BlockValidationError::TimestampError) => {
+            Err(err) => {
                 warn!(
                     ?block_round,
                     ?block_author,
                     ?seq_num,
-                    "dropping proposal, timestamp validation failed"
+                    ?err,
+                    "dropping proposal, block validation failed"
                 );
                 return None;
             }

monad-consensus-state/src/lib.rs

@@ -36,7 +36,6 @@ use monad_validator::signature_collection::SignatureCollection;
 use serde::Serialize;
 
 use crate::{
-    block_validator::BlockValidationError,
     checkpoint::RootInfo,
     payload::{ConsensusBlockBody, ConsensusBlockBodyId, RoundSignature},
     quorum_certificate::QuorumCertificate,
@@ -533,6 +532,11 @@ where
 {
 }
 
+#[derive(Debug)]
+pub enum ConsensusFullBlockError {
+    HeaderPayloadMismatch,
+}
+
 impl<ST, SCT, EPT> ConsensusFullBlock<ST, SCT, EPT>
 where
     ST: CertificateSignatureRecoverable,
@@ -542,9 +546,9 @@ where
     pub fn new(
         header: ConsensusBlockHeader<ST, SCT, EPT>,
         body: ConsensusBlockBody<EPT>,
-    ) -> Result<Self, BlockValidationError> {
+    ) -> Result<Self, ConsensusFullBlockError> {
         if body.get_id() != header.block_body_id {
-            return Err(BlockValidationError::HeaderPayloadMismatchError);
+            return Err(ConsensusFullBlockError::HeaderPayloadMismatch);
         }
         Ok(Self { header, body })
     }

monad-consensus-types/src/block.rs

@@ -29,25 +29,13 @@ use monad_validator::signature_collection::{SignatureCollection, SignatureCollec
 
 use crate::{
     block::{
-        BlockPolicy, ConsensusBlockHeader, ConsensusFullBlock, PassthruBlockPolicy,
-        PassthruWrappedBlock,
+        BlockPolicy, ConsensusBlockHeader, ConsensusFullBlock, ConsensusFullBlockError,
+        PassthruBlockPolicy, PassthruWrappedBlock,
     },
+    metrics::Metrics,
     payload::ConsensusBlockBody,
 };
 
-// TODO these are eth-specific types... we could make these an associated type of BlockValidator if
-// we care enough
-#[derive(Debug)]
-pub enum BlockValidationError {
-    SystemTxnError,
-    TxnError,
-    RandaoError,
-    HeaderError,
-    PayloadError,
-    HeaderPayloadMismatchError,
-    TimestampError,
-}
-
 #[auto_impl(Box)]
 pub trait BlockValidator<ST, SCT, EPT, BPT, SBT, CCT, CRT>
 where
@@ -59,6 +47,8 @@ where
     CCT: ChainConfig<CRT>,
     CRT: ChainRevision,
 {
+    type BlockValidationError: Debug;
+
     // TODO it would be less jank if the BLS pubkey was included in the block payload.
     //
     // It's weird that we need to pass in the expected author's BLS pubkey just to validate the
@@ -75,12 +65,26 @@ where
         body: ConsensusBlockBody<EPT>,
         author_pubkey: Option<&SignatureCollectionPubKeyType<SCT>>,
         chain_config: &CCT,
-    ) -> Result<BPT::ValidatedBlock, BlockValidationError>;
+        metrics: &mut Metrics,
+    ) -> Result<BPT::ValidatedBlock, Self::BlockValidationError>;
 }
 
 #[derive(Copy, Clone, Default, Debug, PartialEq, Eq)]
 pub struct MockValidator;
 
+#[derive(Debug)]
+pub enum MockBlockValidationError {
+    HeaderPayloadMismatch,
+}
+
+impl From<ConsensusFullBlockError> for MockBlockValidationError {
+    fn from(value: ConsensusFullBlockError) -> Self {
+        match value {
+            ConsensusFullBlockError::HeaderPayloadMismatch => Self::HeaderPayloadMismatch,
+        }
+    }
+}
+
 impl<ST, SCT, EPT>
     BlockValidator<
         ST,
@@ -96,12 +100,15 @@ where
     SCT: SignatureCollection<NodeIdPubKey = CertificateSignaturePubKey<ST>>,
     EPT: ExecutionProtocol,
 {
+    type BlockValidationError = MockBlockValidationError;
+
     fn validate(
         &self,
         header: ConsensusBlockHeader<ST, SCT, EPT>,
         body: ConsensusBlockBody<EPT>,
         _author_pubkey: Option<&SignatureCollectionPubKeyType<SCT>>,
         _chain_config: &MockChainConfig,
+        _metrics: &mut Metrics,
     ) -> Result<
         <PassthruBlockPolicy as BlockPolicy<
             ST,
@@ -111,7 +118,7 @@ where
             MockChainConfig,
             MockChainRevision,
         >>::ValidatedBlock,
-        BlockValidationError,
+        Self::BlockValidationError,
     > {
         let full_block = ConsensusFullBlock::new(header, body)?;
         Ok(PassthruWrappedBlock(full_block))

monad-consensus-types/src/block_validator.rs

@@ -12,7 +12,6 @@ bench = false
 monad-chain-config = { workspace = true }
 monad-consensus-types = { workspace = true }
 monad-crypto = { workspace = true }
-monad-eth-txpool-types = { workspace = true }
 monad-eth-types = { workspace = true }
 monad-state-backend = { workspace = true }
 monad-system-calls = { workspace = true }
@@ -24,6 +23,7 @@ alloy-consensus = { workspace = true }
 alloy-eips = { workspace = true }
 alloy-primitives = { workspace = true }
 itertools = { workspace = true }
+serde = { workspace = true }
 sorted_vector_map = { workspace = true }
 tracing = { workspace = true }