Merge pull request #186 from pragma-org/jeluard/rules-validation-errors

chore: make sure rules validation returns a structured result

Author: jeluard

.gitignore

@@ -38,3 +38,6 @@ coverage/
 
 # Cardano node support files
 /cardano-node-config/
+
+# llvm-cov generated file
+lcov.info

Cargo.toml

@@ -7,7 +7,7 @@ authors = ["Amaru Maintainers <[email protected]>"]
 repository = "https://github.com/pragma-org/amaru"
 homepage = "https://github.com/pragma-org/amaru"
 documentation = "https://docs.rs/amaru"
-rust-version = "1.84"                                    # ⚠️ Also change in .cargo/rust-toolchain.toml
+rust-version = "1.88"                                    # ⚠️ Also change in .cargo/rust-toolchain.toml
 
 [workspace]
 members = ["crates/*", "simulation/*"]

Makefile

@@ -110,10 +110,7 @@ build-examples: ## Build all examples
 	@for dir in $(wildcard examples/*/.); do \
 		if [ -f $$dir/Makefile ]; then \
 			echo "Building $$dir"; \
-			$(MAKE) -C $$dir; \
-			if [ $$? != "0" ]; then \
-				exit $$?; \
-			fi; \
+			$(MAKE) -C $$dir || exit; \
 		fi; \
 	done
 

crates/amaru-consensus/src/consensus/chain_selection.rs

@@ -497,8 +497,10 @@ pub(crate) mod tests {
             chain_selector.select_roll_forward(&alice, *header);
         });
 
+        #[allow(clippy::double_ended_iterator_last)]
         let result = chain2
             .iter()
+            // TODO looks like this test relies on the fact that `select_roll_forward` is called on every element. `map` might not be correct then
             .map(|header| chain_selector.select_roll_forward(&bob, *header))
             .last();
 
@@ -533,7 +535,7 @@ pub(crate) mod tests {
         let result = chain1
             .iter()
             .map(|header| chain_selector.select_roll_forward(&alice, *header))
-            .last();
+            .next_back();
 
         assert_eq!(ForwardChainSelection::NoChange, result.unwrap());
     }

crates/amaru-ledger/src/lib.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#![feature(try_trait_v2)]
+
 use amaru_kernel::Point;
 use tracing::Span;
 

crates/amaru-ledger/src/rules.rs

@@ -128,8 +128,10 @@ pub(crate) mod tests {
     use super::*;
     use crate::{
         context::assert::{AssertPreparationContext, AssertValidationContext},
-        rules,
-        rules::block::{InvalidBlock, InvalidBlockHeader},
+        rules::{
+            self,
+            block::{BlockValidation, InvalidBlockDetails},
+        },
         tests::{fake_input, fake_output},
     };
     use amaru_kernel::protocol_parameters::ProtocolParameters;
@@ -174,12 +176,12 @@ pub(crate) mod tests {
         prepare_block(&mut ctx, &block);
 
         let results = rules::block::execute(
-            AssertValidationContext::from(ctx),
+            &mut AssertValidationContext::from(ctx),
             ProtocolParameters::default(),
-            block,
+            &block,
         );
 
-        assert!(results.is_ok())
+        assert!(matches!(results, BlockValidation::Valid));
     }
 
     #[test]
@@ -200,14 +202,12 @@ pub(crate) mod tests {
 
         prepare_block(&mut ctx, &block);
 
-        assert!(
-            rules::block::execute(AssertValidationContext::from(ctx), pp, block).is_err_and(|e| {
-                matches!(
-                    e,
-                    InvalidBlock::Header(InvalidBlockHeader::SizeTooBig { .. })
-                )
-            })
-        )
+        let results = rules::block::execute(&mut AssertValidationContext::from(ctx), pp, &block);
+
+        assert!(matches!(
+            results,
+            BlockValidation::Invalid(InvalidBlockDetails::HeaderSizeTooBig { .. })
+        ))
     }
 
     macro_rules! fixture_context {

crates/amaru-ledger/src/rules/block.rs

@@ -16,76 +16,109 @@ pub mod body_size;
 pub mod ex_units;
 pub mod header_size;
 
-pub use crate::rules::block::{
-    body_size::InvalidBlockSize, ex_units::InvalidExUnits, header_size::InvalidBlockHeader,
-};
 use crate::{
     context::ValidationContext,
     rules::{transaction, transaction::InvalidTransaction},
     state::FailedTransactions,
 };
 use amaru_kernel::{
-    protocol_parameters::ProtocolParameters, AuxiliaryData, HasExUnits, Hash, MintedBlock,
+    protocol_parameters::ProtocolParameters, AuxiliaryData, ExUnits, HasExUnits, Hash, MintedBlock,
     OriginalHash, StakeCredential, TransactionPointer,
 };
-use std::ops::Deref;
-use thiserror::Error;
+use std::ops::{ControlFlow, Deref, FromResidual, Try};
 use tracing::{instrument, Level};
 
-#[derive(Debug, Error)]
-pub enum InvalidBlock {
-    #[error("Invalid block's size: {0}")]
-    Size(#[from] InvalidBlockSize),
-
-    #[error("Invalid block's execution units: {0}")]
-    ExUnits(#[from] InvalidExUnits),
-
-    #[error("Invalid block header: {0}")]
-    Header(#[from] InvalidBlockHeader),
-
-    #[error(
-        "Invalid transaction (hash: {transaction_hash}, index: {transaction_index}): {violation} "
-    )]
+#[derive(Debug)]
+pub enum InvalidBlockDetails {
+    BlockSizeMismatch {
+        supplied: usize,
+        actual: usize,
+    },
+    TooManyExUnits {
+        provided: ExUnits,
+        max: ExUnits,
+    },
+    HeaderSizeTooBig {
+        supplied: usize,
+        max: usize,
+    },
     Transaction {
         transaction_hash: Hash<32>,
         transaction_index: u32,
         violation: InvalidTransaction,
     },
-
-    // TODO: This error shouldn't exist, it's a placeholder for better error handling in less straight forward cases
-    #[error("Uncategorized error: {0}")]
     UncategorizedError(String),
 }
 
+#[derive(Debug)]
+pub enum BlockValidation {
+    Valid,
+    Invalid(InvalidBlockDetails),
+}
+
+impl Try for BlockValidation {
+    type Output = ();
+    type Residual = InvalidBlockDetails;
+
+    fn from_output((): Self::Output) -> Self {
+        BlockValidation::Valid
+    }
+
+    fn branch(self) -> ControlFlow<Self::Residual, Self::Output> {
+        match self {
+            BlockValidation::Valid => ControlFlow::Continue(()),
+            BlockValidation::Invalid(e) => ControlFlow::Break(e),
+        }
+    }
+}
+
+impl FromResidual for BlockValidation {
+    fn from_residual(residual: InvalidBlockDetails) -> Self {
+        BlockValidation::Invalid(residual)
+    }
+}
+
+impl From<BlockValidation> for Result<(), InvalidBlockDetails> {
+    fn from(validation: BlockValidation) -> Self {
+        match validation {
+            BlockValidation::Valid => Ok(()),
+            BlockValidation::Invalid(e) => Err(e),
+        }
+    }
+}
+
 #[instrument(level = Level::TRACE, skip_all)]
 pub fn execute<C: ValidationContext<FinalState = S>, S: From<C>>(
-    mut context: C,
+    context: &mut C,
     protocol_params: ProtocolParameters,
-    block: MintedBlock<'_>,
-) -> Result<S, InvalidBlock> {
+    block: &MintedBlock<'_>,
+) -> BlockValidation {
     header_size::block_header_size_valid(block.header.raw_cbor(), &protocol_params)?;
 
-    body_size::block_body_size_valid(&block.header.header_body, &block)?;
+    body_size::block_body_size_valid(block)?;
 
     ex_units::block_ex_units_valid(block.ex_units(), &protocol_params)?;
 
-    let failed_transactions = FailedTransactions::from_block(&block);
+    let failed_transactions = FailedTransactions::from_block(block);
 
     let witness_sets = block.transaction_witness_sets.deref().to_vec();
 
-    let transactions = block.transaction_bodies.to_vec();
+    let transactions = block.transaction_bodies.deref().to_vec();
 
     // using `zip` here instead of enumerate as it is safer to cast from u32 to usize than usize to u32
     // Realistically, we're never gonna hit the u32 limit with the number of transactions in a block (a boy can dream)
     for (i, transaction) in (0u32..).zip(transactions.into_iter()) {
         let transaction_hash = transaction.original_hash();
 
-        let witness_set = witness_sets
-            .get(i as usize)
-            .ok_or(InvalidBlock::UncategorizedError(format!(
-                "Missing witness set for transaction index {}",
-                i
-            )))?;
+        let witness_set = match witness_sets.get(i as usize) {
+            Some(witness_set) => witness_set,
+            None => {
+                return BlockValidation::Invalid(InvalidBlockDetails::UncategorizedError(format!(
+                    "Witness set not found for transaction index {}",
+                    i
+                )));
+            }
+        };
 
         let auxiliary_data: Option<&AuxiliaryData> = block
             .auxiliary_data_set
@@ -108,21 +141,22 @@ pub fn execute<C: ValidationContext<FinalState = S>, S: From<C>>(
             transaction_index: i as usize, // From u32
         };
 
-        transaction::execute(
-            &mut context,
+        if let Err(err) = transaction::execute(
+            context,
             &protocol_params,
             pointer,
             !failed_transactions.has(i),
             transaction,
             witness_set,
             auxiliary_data,
-        )
-        .map_err(|err| InvalidBlock::Transaction {
-            transaction_hash,
-            transaction_index: i,
-            violation: err,
-        })?;
+        ) {
+            return BlockValidation::Invalid(InvalidBlockDetails::Transaction {
+                transaction_hash,
+                transaction_index: i,
+                violation: err,
+            });
+        }
     }
 
-    Ok(context.into())
+    BlockValidation::Valid
 }

crates/amaru-ledger/src/rules/block/body_size.rs

@@ -12,31 +12,24 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use amaru_kernel::{to_cbor, HeaderBody, MintedBlock};
-use thiserror::Error;
+use amaru_kernel::{to_cbor, MintedBlock};
 
-#[derive(Debug, Error)]
-pub enum InvalidBlockSize {
-    #[error("block body size mismatch: supplied {supplied}, actual {actual}")]
-    SizeMismatch { supplied: usize, actual: usize },
-}
+use super::{BlockValidation, InvalidBlockDetails};
 
 /// This validation checks that the purported block body size matches the actual block body size.
 /// The validation of the bounds happens in the networking layer
-pub fn block_body_size_valid(
-    block_header: &HeaderBody,
-    block: &MintedBlock<'_>,
-) -> Result<(), InvalidBlockSize> {
+pub fn block_body_size_valid(block: &MintedBlock<'_>) -> BlockValidation {
+    let block_header = &block.header.header_body;
     let bh_size = block_header.block_body_size as usize;
     let actual_block_size = calculate_block_body_size(block);
 
     if bh_size != actual_block_size {
-        Err(InvalidBlockSize::SizeMismatch {
+        BlockValidation::Invalid(InvalidBlockDetails::BlockSizeMismatch {
             supplied: bh_size,
             actual: actual_block_size,
         })
     } else {
-        Ok(())
+        BlockValidation::Valid
     }
 }
 
@@ -58,7 +51,7 @@ mod tests {
     use amaru_kernel::{include_cbor, MintedBlock};
     use test_case::test_case;
 
-    use super::InvalidBlockSize;
+    use crate::rules::block::InvalidBlockDetails;
 
     macro_rules! fixture {
         ($number:literal) => {
@@ -70,12 +63,11 @@ mod tests {
     }
 
     #[test_case(fixture!("2667660"); "valid")]
-    #[test_case(fixture!("2667660", "invalid_block_body_size") =>
-        matches Err(InvalidBlockSize::SizeMismatch {supplied, actual})
+    #[test_case(fixture!("2667660", "invalid_block_body_size") => 
+        matches Err(InvalidBlockDetails::BlockSizeMismatch {supplied, actual})
             if supplied == 0 && actual == 3411;
-        "block body size mismatch"
-    )]
-    fn test_block_size(block: MintedBlock<'_>) -> Result<(), InvalidBlockSize> {
-        super::block_body_size_valid(&block.header.header_body, &block)
+    "block body size mismatch")]
+    fn test_block_size(block: MintedBlock<'_>) -> Result<(), InvalidBlockDetails> {
+        super::block_body_size_valid(&block).into()
     }
 }