Use strongly typed errors for block verification

matter-labs · Oct 27, 2023 · ca0b37c · ca0b37c
1 parent 9ca0bbc
commit ca0b37c
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 24 deletions.
diff --git a/node/Cargo.lock b/node/Cargo.lock
diff --git a/node/actors/sync_blocks/Cargo.toml b/node/actors/sync_blocks/Cargo.toml
@@ -8,6 +8,7 @@ license.workspace = true
 
 [dependencies]
 anyhow.workspace = true
+thiserror.workspace = true
 tracing.workspace = true
 
 concurrency = { path = "../../libs/concurrency" }

diff --git a/node/actors/sync_blocks/src/peers/mod.rs b/node/actors/sync_blocks/src/peers/mod.rs
@@ -11,7 +11,7 @@ use concurrency::{
 use network::io::{SyncBlocksInputMessage, SyncState};
 use roles::{
     node,
-    validator::{BlockNumber, FinalBlock},
+    validator::{BlockHeader, BlockNumber, FinalBlock, PayloadHash},
 };
 use std::{collections::HashMap, sync::Arc};
 use storage::WriteBlockStore;
@@ -332,25 +332,35 @@ impl PeerStates {
         Ok(None)
     }
 
-    fn validate_block(&self, block_number: BlockNumber, block: &FinalBlock) -> anyhow::Result<()> {
-        anyhow::ensure!(
-            block.header.number == block_number,
-            "Block does not have requested number"
-        );
-        anyhow::ensure!(
-            block.payload.hash() == block.header.payload,
-            "Block payload doesn't match the block header",
-        );
-        anyhow::ensure!(
-            block.header == block.justification.message.proposal,
-            "Quorum certificate proposal doesn't match the block header",
-        );
+    fn validate_block(
+        &self,
+        block_number: BlockNumber,
+        block: &FinalBlock,
+    ) -> Result<(), BlockValidationError> {
+        if block.header.number != block_number {
+            return Err(BlockValidationError::NumberMismatch {
+                requested: block_number,
+                got: block.header.number,
+            });
+        }
+        let payload_hash = block.payload.hash();
+        if payload_hash != block.header.payload {
+            return Err(BlockValidationError::HashMismatch {
+                header_hash: block.header.payload,
+                payload_hash,
+            });
+        }
+        if block.header != block.justification.message.proposal {
+            return Err(BlockValidationError::ProposalMismatch {
+                block_header: Box::new(block.header),
+                qc_header: Box::new(block.justification.message.proposal),
+            });
+        }
 
         block
             .justification
             .verify(&self.config.validator_set, self.config.consensus_threshold)
-            .context("Failed verifying quorum certificate")?;
-        Ok(())
+            .map_err(BlockValidationError::Justification)
     }
 
     #[instrument(level = "trace", skip(self, ctx))]
@@ -369,3 +379,31 @@ impl PeerStates {
         Ok(())
     }
 }
+
+/// Errors that can occur validating a `FinalBlock` received from a node.
+#[derive(Debug, thiserror::Error)]
+enum BlockValidationError {
+    #[error("block does not have requested number (requested: {requested}, got: {got})")]
+    NumberMismatch {
+        requested: BlockNumber,
+        got: BlockNumber,
+    },
+    #[error(
+        "block payload doesn't match the block header (hash in header: {header_hash:?}, \
+         payload hash: {payload_hash:?})"
+    )]
+    HashMismatch {
+        header_hash: PayloadHash,
+        payload_hash: PayloadHash,
+    },
+    #[error(
+        "quorum certificate proposal doesn't match the block header (block header: {block_header:?}, \
+         header in QC: {qc_header:?})"
+    )]
+    ProposalMismatch {
+        block_header: Box<BlockHeader>,
+        qc_header: Box<BlockHeader>,
+    },
+    #[error("failed verifying quorum certificate: {0:#?}")]
+    Justification(#[source] anyhow::Error),
+}
diff --git a/node/actors/sync_blocks/src/peers/tests.rs b/node/actors/sync_blocks/src/peers/tests.rs
@@ -860,25 +860,35 @@ async fn processing_invalid_blocks() {
     let (peer_states, _) = PeerStates::new(message_sender, storage, test_validators.test_config());
 
     let invalid_block = &test_validators.final_blocks[0];
-    assert!(peer_states
+    let err = peer_states
         .validate_block(BlockNumber(1), invalid_block)
-        .is_err());
+        .unwrap_err();
+    assert_matches!(
+        err,
+        BlockValidationError::NumberMismatch {
+            requested: BlockNumber(1),
+            got: BlockNumber(0),
+        }
+    );
 
     let mut invalid_block = test_validators.final_blocks[1].clone();
     invalid_block.justification = test_validators.final_blocks[0].justification.clone();
-    assert!(peer_states
+    let err = peer_states
         .validate_block(BlockNumber(1), &invalid_block)
-        .is_err());
+        .unwrap_err();
+    assert_matches!(err, BlockValidationError::ProposalMismatch { .. });
 
     let mut invalid_block = test_validators.final_blocks[1].clone();
     invalid_block.payload = validator::Payload(b"invalid".to_vec());
-    assert!(peer_states
+    let err = peer_states
         .validate_block(BlockNumber(1), &invalid_block)
-        .is_err());
+        .unwrap_err();
+    assert_matches!(err, BlockValidationError::HashMismatch { .. });
 
     let other_network = TestValidators::new(4, 2, rng);
     let invalid_block = &other_network.final_blocks[1];
-    assert!(peer_states
+    let err = peer_states
         .validate_block(BlockNumber(1), invalid_block)
-        .is_err());
+        .unwrap_err();
+    assert_matches!(err, BlockValidationError::Justification(_));
 }