From 89c7847c6d25eca45afd21154cb9a99be537b77a Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Fri, 15 May 2020 15:25:25 +0200 Subject: [PATCH] Refactor the validators to ensure that each validation step only completes once. --- applications/tari_base_node/src/builder.rs | 2 +- .../core/src/validation/block_validators.rs | 70 +++++++++++++------ base_layer/core/tests/block_validation.rs | 4 +- .../chain_storage_tests/chain_storage.rs | 4 +- base_layer/core/tests/node_service.rs | 2 +- base_layer/core/tests/node_state_machine.rs | 4 +- 6 files changed, 55 insertions(+), 31 deletions(-) diff --git a/applications/tari_base_node/src/builder.rs b/applications/tari_base_node/src/builder.rs index c8dfaa18a8..7f2a1e47a8 100644 --- a/applications/tari_base_node/src/builder.rs +++ b/applications/tari_base_node/src/builder.rs @@ -467,7 +467,7 @@ where let factories = CryptoFactories::default(); let validators = Validators::new( FullConsensusValidator::new(rules.clone(), factories.clone()), - StatelessBlockValidator::new(&rules.consensus_constants()), + StatelessBlockValidator::new(rules.clone(), factories.clone()), AccumDifficultyValidator {}, ); let db_config = BlockchainDatabaseConfig { diff --git a/base_layer/core/src/validation/block_validators.rs b/base_layer/core/src/validation/block_validators.rs index 19955fd06e..542bfbb5a4 100644 --- a/base_layer/core/src/validation/block_validators.rs +++ b/base_layer/core/src/validation/block_validators.rs @@ -45,28 +45,29 @@ pub const LOG_TARGET: &str = "c::val::block_validators"; /// This validator tests whether a candidate block is internally consistent #[derive(Clone)] pub struct StatelessBlockValidator { - consensus_constants: ConsensusConstants, + rules: ConsensusManager, + factories: CryptoFactories, } impl StatelessBlockValidator { - pub fn new(consensus_constants: &ConsensusConstants) -> Self { - Self { - consensus_constants: consensus_constants.clone(), - } + pub fn new(rules: ConsensusManager, factories: CryptoFactories) -> Self { + Self { rules, factories } } } impl StatelessValidation for StatelessBlockValidator { /// The consensus checks that are done (in order of cheapest to verify to most expensive): + /// 1. Is there precisely one Coinbase output and is it correctly defined? /// 1. Is the block weight of the block under the prescribed limit? /// 1. Does it contain only unique inputs and outputs? /// 1. Where all the rules for the spent outputs followed? /// 1. Was cut through applied in the block? + /// 1. Is the accounting correct? fn validate(&self, block: &Block) -> Result<(), ValidationError> { let block_id = format!("block #{} ({})", block.header.height, block.hash().to_hex()); - check_coinbase_output(block, &self.consensus_constants)?; + check_coinbase_output(block, &self.rules.consensus_constants())?; trace!(target: LOG_TARGET, "SV - Coinbase output is ok for {} ", &block_id); - check_block_weight(block, &self.consensus_constants)?; + check_block_weight(block, &self.rules.consensus_constants())?; trace!(target: LOG_TARGET, "SV - Block weight is ok for {} ", &block_id); check_duplicate_transactions_inputs(block)?; trace!( @@ -83,6 +84,9 @@ impl StatelessValidation for StatelessBlockValidator { target: LOG_TARGET, "{} has PASSED stateless VALIDATION check.", &block_id ); + + check_accounting_balance(block, self.rules.clone(), &self.factories)?; + trace!(target: LOG_TARGET, "SV - accounting balance correct for {}", &block_id); Ok(()) } } @@ -111,22 +115,6 @@ impl Validation for FullConsensusValidator { /// 1. Is the achieved difficulty of this block >= the target difficulty for this block? fn validate(&self, block: &Block, db: &B) -> Result<(), ValidationError> { let block_id = format!("block #{} ({})", block.header.height, block.hash().to_hex()); - check_coinbase_output(block, &self.rules.consensus_constants())?; - trace!(target: LOG_TARGET, "FCV - Coinbase output ok for {}", &block_id); - check_block_weight(block, &self.rules.consensus_constants())?; - trace!(target: LOG_TARGET, "FCV - Block weight ok for {}", &block_id); - check_duplicate_transactions_inputs(block)?; - trace!( - target: LOG_TARGET, - "FCV - No duplicate inputs or outputs for {} ", - &block_id - ); - check_cut_through(block)?; - trace!(target: LOG_TARGET, "FCV - Cut-though correct for {}", &block_id); - block.check_stxo_rules().map_err(BlockValidationError::from)?; - trace!(target: LOG_TARGET, "FCV - STxO rules correct for {}", &block_id); - check_accounting_balance(block, self.rules.clone(), &self.factories)?; - trace!(target: LOG_TARGET, "FCV - accounting balance correct for {}", &block_id); check_inputs_are_utxos(block, db)?; trace!(target: LOG_TARGET, "FCV - All inputs are valid for {}", &block_id); check_mmr_roots(block, db)?; @@ -147,6 +135,38 @@ impl Validation for FullConsensusValidator { } } +/// This validator tests whether a candidate block is internally consistent, BUT it does not check internal accounting +/// as some tests use odd values. +#[derive(Clone)] +pub struct MockStatelessBlockValidator { + rules: ConsensusManager, + factories: CryptoFactories, +} + +impl MockStatelessBlockValidator { + pub fn new(rules: ConsensusManager, factories: CryptoFactories) -> Self { + Self { rules, factories } + } +} + +impl StatelessValidation for MockStatelessBlockValidator { + /// The consensus checks that are done (in order of cheapest to verify to most expensive): + /// 1. Is there precisely one Coinbase output and is it correctly defined? + /// 1. Is the block weight of the block under the prescribed limit? + /// 1. Does it contain only unique inputs and outputs? + /// 1. Where all the rules for the spent outputs followed? + /// 1. Was cut through applied in the block? + fn validate(&self, block: &Block) -> Result<(), ValidationError> { + check_coinbase_output(block, &self.rules.consensus_constants())?; + check_block_weight(block, &self.rules.consensus_constants())?; + // Check that the inputs are are allowed to be spent + block.check_stxo_rules().map_err(BlockValidationError::from)?; + check_cut_through(block)?; + + Ok(()) + } +} + //------------------------------------- Block validator helper functions -------------------------------------// fn check_accounting_balance( block: &Block, @@ -154,6 +174,10 @@ fn check_accounting_balance( factories: &CryptoFactories, ) -> Result<(), ValidationError> { + if block.header.height == 0 { + // Gen block does not need to be checked for this. + return Ok(()); + } let offset = &block.header.total_kernel_offset; let total_coinbase = rules.calculate_coinbase_and_fees(block); block diff --git a/base_layer/core/tests/block_validation.rs b/base_layer/core/tests/block_validation.rs index 41a66e0c0b..860fc10d11 100644 --- a/base_layer/core/tests/block_validation.rs +++ b/base_layer/core/tests/block_validation.rs @@ -37,8 +37,8 @@ fn test_genesis_block() { let rules = ConsensusManagerBuilder::new(network).build(); let backend = MemoryDatabase::::default(); let validators = Validators::new( - FullConsensusValidator::new(rules.clone(), factories), - StatelessBlockValidator::new(&rules.consensus_constants()), + FullConsensusValidator::new(rules.clone(), factories.clone()), + StatelessBlockValidator::new(rules.clone(), factories), AccumDifficultyValidator {}, ); let db = BlockchainDatabase::new(backend, &rules, validators, BlockchainDatabaseConfig::default()).unwrap(); diff --git a/base_layer/core/tests/chain_storage_tests/chain_storage.rs b/base_layer/core/tests/chain_storage_tests/chain_storage.rs index 45ce144bc3..b4e7f2667c 100644 --- a/base_layer/core/tests/chain_storage_tests/chain_storage.rs +++ b/base_layer/core/tests/chain_storage_tests/chain_storage.rs @@ -60,7 +60,7 @@ use tari_core::{ txn_schema, validation::{ accum_difficulty_validators::MockAccumDifficultyValidator, - block_validators::StatelessBlockValidator, + block_validators::{MockStatelessBlockValidator, StatelessBlockValidator}, mocks::MockValidator, }, }; @@ -873,7 +873,7 @@ fn invalid_block() { .build(); let validators = Validators::new( MockValidator::new(true), - StatelessBlockValidator::new(&consensus_manager.consensus_constants()), + MockStatelessBlockValidator::new(consensus_manager.clone(), factories.clone()), MockAccumDifficultyValidator {}, ); let db = create_lmdb_database(&temp_path, MmrCacheConfig::default()).unwrap(); diff --git a/base_layer/core/tests/node_service.rs b/base_layer/core/tests/node_service.rs index dc1d3cf705..b1c3dc64ca 100644 --- a/base_layer/core/tests/node_service.rs +++ b/base_layer/core/tests/node_service.rs @@ -507,7 +507,7 @@ fn propagate_and_forward_invalid_block() { .with_consensus_constants(consensus_constants) .with_block(block0.clone()) .build(); - let stateless_block_validator = StatelessBlockValidator::new(&rules.consensus_constants()); + let stateless_block_validator = StatelessBlockValidator::new(rules.clone(), factories.clone()); let mock_validator = MockValidator::new(true); let mock_accum_difficulty_validator = MockAccumDifficultyValidator {}; let (mut alice_node, rules) = BaseNodeBuilder::new(network) diff --git a/base_layer/core/tests/node_state_machine.rs b/base_layer/core/tests/node_state_machine.rs index a076c5a7a5..5272df7134 100644 --- a/base_layer/core/tests/node_state_machine.rs +++ b/base_layer/core/tests/node_state_machine.rs @@ -65,7 +65,7 @@ use tari_core::{ transactions::types::CryptoFactories, validation::{ accum_difficulty_validators::MockAccumDifficultyValidator, - block_validators::StatelessBlockValidator, + block_validators::{MockStatelessBlockValidator, StatelessBlockValidator}, mocks::MockValidator, }, }; @@ -605,7 +605,7 @@ fn test_sync_peer_banning() { .with_consensus_constants(consensus_constants) .with_block(prev_block.clone()) .build(); - let stateless_block_validator = StatelessBlockValidator::new(consensus_manager.consensus_constants()); + let stateless_block_validator = MockStatelessBlockValidator::new(consensus_manager.clone(), factories.clone()); let mock_validator = MockValidator::new(true); // Create base nodes let alice_node_identity = random_node_identity();