From ca0b37ccc8eaabd700c53916d263b7eb19ee2fe7 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Fri, 27 Oct 2023 13:07:58 +0300 Subject: [PATCH] Use strongly typed errors for block verification --- node/Cargo.lock | 1 + node/actors/sync_blocks/Cargo.toml | 1 + node/actors/sync_blocks/src/peers/mod.rs | 70 +++++++++++++++++----- node/actors/sync_blocks/src/peers/tests.rs | 26 +++++--- 4 files changed, 74 insertions(+), 24 deletions(-) diff --git a/node/Cargo.lock b/node/Cargo.lock index f81a3c58..86f18a03 100644 --- a/node/Cargo.lock +++ b/node/Cargo.lock @@ -1955,6 +1955,7 @@ dependencies = [ "storage", "tempfile", "test-casing", + "thiserror", "tokio", "tracing", "utils", diff --git a/node/actors/sync_blocks/Cargo.toml b/node/actors/sync_blocks/Cargo.toml index 999b7ec6..5657f754 100644 --- 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 index 7eb70009..f81cf469 100644 --- 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, + qc_header: Box, + }, + #[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 index 3b588169..cadbf1f5 100644 --- 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(_)); }