From db1ea1dcd8361c6b53a136842398464da363447b Mon Sep 17 00:00:00 2001 From: Marek Kotewicz Date: Tue, 12 Nov 2019 15:05:49 +0100 Subject: [PATCH] simplify verification (#11249) * simplify verifier, remove NoopVerifier * simplify verifier by removing Verifier trait and its only implementation * remove unused imports * fixed verification test failing to compile --- ethcore/src/client/client.rs | 17 +-- ethcore/verification/src/canon_verifier.rs | 50 ------- ethcore/verification/src/lib.rs | 24 +--- ethcore/verification/src/noop_verifier.rs | 50 ------- ethcore/verification/src/queue/kind.rs | 7 +- ethcore/verification/src/verification.rs | 146 +++++++++++++++------ ethcore/verification/src/verifier.rs | 46 ------- 7 files changed, 117 insertions(+), 223 deletions(-) delete mode 100644 ethcore/verification/src/canon_verifier.rs delete mode 100644 ethcore/verification/src/noop_verifier.rs delete mode 100644 ethcore/verification/src/verifier.rs diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index d29c8ac6f..bdaaedebc 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -133,8 +133,7 @@ use types::{ verification::{Unverified, VerificationQueueInfo as BlockQueueInfo}, }; use types::data_format::DataFormat; -use verification::{BlockQueue, Verifier}; -use verification; +use verification::{self, BlockQueue}; use verification::queue::kind::BlockLike; use vm::{CreateContractAddress, EnvInfo, LastHashes}; @@ -162,9 +161,6 @@ struct Importer { /// Lock used during block import pub import_lock: Mutex<()>, // FIXME Maybe wrap the whole `Importer` instead? - /// Used to verify blocks - pub verifier: Box>, - /// Queue containing pending blocks pub block_queue: BlockQueue, @@ -271,7 +267,6 @@ impl Importer { Ok(Importer { import_lock: Mutex::new(()), - verifier: verification::new(config.verifier_type.clone()), block_queue, miner, ancient_verifier: AncientVerifier::new(engine.clone()), @@ -389,15 +384,15 @@ impl Importer { let chain = client.chain.read(); // Verify Block Family - let verify_family_result = self.verifier.verify_block_family( + let verify_family_result = verification::verify_block_family( &header, &parent, engine, - Some(verification::FullFamilyParams { + verification::FullFamilyParams { block: &block, block_provider: &**chain, client - }), + }, ); if let Err(e) = verify_family_result { @@ -405,7 +400,7 @@ impl Importer { return Err(e); }; - let verify_external_result = self.verifier.verify_block_external(&header, engine); + let verify_external_result = engine.verify_block_external(&header); if let Err(e) = verify_external_result { warn!(target: "client", "Stage 4 block verification failed for #{} ({})\nError: {:?}", header.number(), header.hash(), e); return Err(e.into()); @@ -446,7 +441,7 @@ impl Importer { } // Final Verification - if let Err(e) = self.verifier.verify_block_final(&header, &locked_block.header) { + if let Err(e) = verification::verify_block_final(&header, &locked_block.header) { warn!(target: "client", "Stage 5 block verification failed for #{} ({})\nError: {:?}", header.number(), header.hash(), e); return Err(e.into()); } diff --git a/ethcore/verification/src/canon_verifier.rs b/ethcore/verification/src/canon_verifier.rs deleted file mode 100644 index 336428caf..000000000 --- a/ethcore/verification/src/canon_verifier.rs +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright 2015-2019 Parity Technologies (UK) Ltd. -// This file is part of Parity Ethereum. - -// Parity Ethereum is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Parity Ethereum is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Parity Ethereum. If not, see . - -//! Canonical verifier. - -use call_contract::CallContract; -use client_traits::BlockInfo; -use engine::Engine; -use common_types::{ - header::Header, - errors::EthcoreError as Error, -}; -use super::Verifier; -use super::verification; - -/// A canonical verifier -- this does full verification. -pub struct CanonVerifier; - -impl Verifier for CanonVerifier { - fn verify_block_family( - &self, - header: &Header, - parent: &Header, - engine: &dyn Engine, - do_full: Option>, - ) -> Result<(), Error> { - verification::verify_block_family(header, parent, engine, do_full) - } - - fn verify_block_final(&self, expected: &Header, got: &Header) -> Result<(), Error> { - verification::verify_block_final(expected, got) - } - - fn verify_block_external(&self, header: &Header, engine: &dyn Engine) -> Result<(), Error> { - engine.verify_block_external(header) - } -} diff --git a/ethcore/verification/src/lib.rs b/ethcore/verification/src/lib.rs index 1c07e714e..5edaace7f 100644 --- a/ethcore/verification/src/lib.rs +++ b/ethcore/verification/src/lib.rs @@ -16,8 +16,6 @@ //! Block verification utilities. -use call_contract::CallContract; -use client_traits::BlockInfo; // The MallocSizeOf derive looks for this in the root use parity_util_mem as malloc_size_of; @@ -25,20 +23,13 @@ use parity_util_mem as malloc_size_of; pub mod verification; #[cfg(not(feature = "bench" ))] mod verification; -mod verifier; pub mod queue; -mod canon_verifier; -mod noop_verifier; #[cfg(any(test, feature = "bench" ))] pub mod test_helpers; -pub use self::verification::FullFamilyParams; -pub use self::verifier::Verifier; +pub use self::verification::{FullFamilyParams, verify_block_family, verify_block_final}; pub use self::queue::{BlockQueue, Config as QueueConfig}; -use self::canon_verifier::CanonVerifier; -use self::noop_verifier::NoopVerifier; - /// Verifier type. #[derive(Debug, PartialEq, Clone)] pub enum VerifierType { @@ -46,17 +37,6 @@ pub enum VerifierType { Canon, /// Verifies block normally, but skips seal verification. CanonNoSeal, - /// Does not verify block at all. - /// Used in tests. - Noop, -} - -/// Create a new verifier based on type. -pub fn new(v: VerifierType) -> Box> { - match v { - VerifierType::Canon | VerifierType::CanonNoSeal => Box::new(CanonVerifier), - VerifierType::Noop => Box::new(NoopVerifier), - } } impl VerifierType { @@ -64,7 +44,7 @@ impl VerifierType { pub fn verifying_seal(&self) -> bool { match *self { VerifierType::Canon => true, - VerifierType::Noop | VerifierType::CanonNoSeal => false, + VerifierType::CanonNoSeal => false, } } } diff --git a/ethcore/verification/src/noop_verifier.rs b/ethcore/verification/src/noop_verifier.rs deleted file mode 100644 index 149f27988..000000000 --- a/ethcore/verification/src/noop_verifier.rs +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright 2015-2019 Parity Technologies (UK) Ltd. -// This file is part of Parity Ethereum. - -// Parity Ethereum is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Parity Ethereum is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Parity Ethereum. If not, see . - -//! No-op verifier. - -use call_contract::CallContract; -use client_traits::BlockInfo; -use common_types::{ - header::Header, - errors::EthcoreError as Error -}; -use engine::Engine; -use super::{verification, Verifier}; - -/// A no-op verifier -- this will verify everything it's given immediately. -#[allow(dead_code)] -pub struct NoopVerifier; - -impl Verifier for NoopVerifier { - fn verify_block_family( - &self, - _: &Header, - _t: &Header, - _: &dyn Engine, - _: Option> - ) -> Result<(), Error> { - Ok(()) - } - - fn verify_block_final(&self, _expected: &Header, _got: &Header) -> Result<(), Error> { - Ok(()) - } - - fn verify_block_external(&self, _header: &Header, _engine: &dyn Engine) -> Result<(), Error> { - Ok(()) - } -} diff --git a/ethcore/verification/src/queue/kind.rs b/ethcore/verification/src/queue/kind.rs index cc4d1167a..b7642cd0d 100644 --- a/ethcore/verification/src/queue/kind.rs +++ b/ethcore/verification/src/queue/kind.rs @@ -152,7 +152,7 @@ pub mod headers { header::Header, errors::EthcoreError as Error, }; - use crate::verification::verify_header_params; + use crate::verification::{verify_header_params, verify_header_time}; use ethereum_types::{H256, U256}; @@ -171,7 +171,10 @@ pub mod headers { type Verified = Header; fn create(input: Self::Input, engine: &dyn Engine, check_seal: bool) -> Result { - match verify_header_params(&input, engine, true, check_seal) { + let res = verify_header_params(&input, engine, check_seal) + .and_then(|_| verify_header_time(&input)); + + match res { Ok(_) => Ok(input), Err(err) => Err((input, err)) } diff --git a/ethcore/verification/src/verification.rs b/ethcore/verification/src/verification.rs index 3f06c39ad..4f2c055a4 100644 --- a/ethcore/verification/src/verification.rs +++ b/ethcore/verification/src/verification.rs @@ -29,7 +29,7 @@ use rlp::Rlp; use triehash::ordered_trie_root; use unexpected::{Mismatch, OutOfBounds}; -use blockchain::*; +use blockchain::BlockProvider; use call_contract::CallContract; use client_traits::BlockInfo; use engine::Engine; @@ -46,7 +46,8 @@ use time_utils::CheckedSystemTime; /// Phase 1 quick block verification. Only does checks that are cheap. Operates on a single block pub fn verify_block_basic(block: &Unverified, engine: &dyn Engine, check_seal: bool) -> Result<(), Error> { - verify_header_params(&block.header, engine, true, check_seal)?; + verify_header_params(&block.header, engine, check_seal)?; + verify_header_time(&block.header)?; verify_block_integrity(block)?; if check_seal { @@ -54,7 +55,7 @@ pub fn verify_block_basic(block: &Unverified, engine: &dyn Engine, check_seal: b } for uncle in &block.uncles { - verify_header_params(uncle, engine, false, check_seal)?; + verify_header_params(uncle, engine, check_seal)?; if check_seal { engine.verify_block_basic(uncle)?; } @@ -123,16 +124,11 @@ pub fn verify_block_family( header: &Header, parent: &Header, engine: &dyn Engine, - do_full: Option> + params: FullFamilyParams ) -> Result<(), Error> { // TODO: verify timestamp verify_parent(&header, &parent, engine)?; engine.verify_block_family(&header, &parent)?; - - let params = match do_full { - Some(x) => x, - None => return Ok(()), - }; verify_uncles(params.block, params.block_provider, engine)?; for tx in ¶ms.block.transactions { @@ -165,8 +161,7 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn match bc.block_details(&hash) { Some(details) => { excluded.insert(details.parent); - let b = bc.block(&hash) - .expect("parent already known to be stored; qed"); + let b = bc.block(&hash).expect("parent already known to be stored; qed"); excluded.extend(b.uncle_hashes()); hash = details.parent; } @@ -195,10 +190,18 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn let depth = if header.number() > uncle.number() { header.number() - uncle.number() } else { 0 }; if depth > MAX_UNCLE_AGE as u64 { - return Err(From::from(BlockError::UncleTooOld(OutOfBounds { min: Some(header.number() - depth), max: Some(header.number() - 1), found: uncle.number() }))); + return Err(From::from(BlockError::UncleTooOld(OutOfBounds { + min: Some(header.number() - depth), + max: Some(header.number() - 1), + found: uncle.number() + }))); } else if depth < 1 { - return Err(From::from(BlockError::UncleIsBrother(OutOfBounds { min: Some(header.number() - depth), max: Some(header.number() - 1), found: uncle.number() }))); + return Err(From::from(BlockError::UncleIsBrother(OutOfBounds { + min: Some(header.number() - depth), + max: Some(header.number() - 1), + found: uncle.number() + }))); } // cB @@ -211,7 +214,8 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn // cB.p^7 -------------/ // cB.p^8 let mut expected_uncle_parent = header.parent_hash().clone(); - let uncle_parent = bc.block_header_data(&uncle.parent_hash()).ok_or_else(|| Error::from(BlockError::UnknownUncleParent(uncle.parent_hash().clone())))?; + let uncle_parent = bc.block_header_data(&uncle.parent_hash()) + .ok_or_else(|| Error::from(BlockError::UnknownUncleParent(*uncle.parent_hash())))?; for _ in 0..depth { match bc.block_details(&expected_uncle_parent) { Some(details) => { @@ -237,22 +241,34 @@ fn verify_uncles(block: &PreverifiedBlock, bc: &dyn BlockProvider, engine: &dyn /// Phase 4 verification. Check block information against transaction enactment results, pub fn verify_block_final(expected: &Header, got: &Header) -> Result<(), Error> { if expected.state_root() != got.state_root() { - return Err(From::from(BlockError::InvalidStateRoot(Mismatch { expected: *expected.state_root(), found: *got.state_root() }))) + return Err(From::from(BlockError::InvalidStateRoot(Mismatch { + expected: *expected.state_root(), + found: *got.state_root() + }))) } if expected.gas_used() != got.gas_used() { - return Err(From::from(BlockError::InvalidGasUsed(Mismatch { expected: *expected.gas_used(), found: *got.gas_used() }))) + return Err(From::from(BlockError::InvalidGasUsed(Mismatch { + expected: *expected.gas_used(), + found: *got.gas_used() + }))) } if expected.log_bloom() != got.log_bloom() { - return Err(From::from(BlockError::InvalidLogBloom(Box::new(Mismatch { expected: *expected.log_bloom(), found: *got.log_bloom() })))) + return Err(From::from(BlockError::InvalidLogBloom(Box::new(Mismatch { + expected: *expected.log_bloom(), + found: *got.log_bloom() + })))) } if expected.receipts_root() != got.receipts_root() { - return Err(From::from(BlockError::InvalidReceiptsRoot(Mismatch { expected: *expected.receipts_root(), found: *got.receipts_root() }))) + return Err(From::from(BlockError::InvalidReceiptsRoot(Mismatch { + expected: *expected.receipts_root(), + found: *got.receipts_root() + }))) } Ok(()) } /// Check basic header parameters. -pub(crate) fn verify_header_params(header: &Header, engine: &dyn Engine, is_full: bool, check_seal: bool) -> Result<(), Error> { +pub(crate) fn verify_header_params(header: &Header, engine: &dyn Engine, check_seal: bool) -> Result<(), Error> { if check_seal { let expected_seal_fields = engine.seal_fields(header); if header.seal().len() != expected_seal_fields { @@ -263,48 +279,83 @@ pub(crate) fn verify_header_params(header: &Header, engine: &dyn Engine, is_full } if header.number() >= From::from(BlockNumber::max_value()) { - return Err(From::from(BlockError::RidiculousNumber(OutOfBounds { max: Some(From::from(BlockNumber::max_value())), min: None, found: header.number() }))) + return Err(From::from(BlockError::RidiculousNumber(OutOfBounds { + max: Some(From::from(BlockNumber::max_value())), + min: None, + found: header.number() + }))) } if header.gas_used() > header.gas_limit() { - return Err(From::from(BlockError::TooMuchGasUsed(OutOfBounds { max: Some(*header.gas_limit()), min: None, found: *header.gas_used() }))); + return Err(From::from(BlockError::TooMuchGasUsed(OutOfBounds { + max: Some(*header.gas_limit()), + min: None, + found: *header.gas_used() + }))); } let min_gas_limit = engine.params().min_gas_limit; if header.gas_limit() < &min_gas_limit { - return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: Some(min_gas_limit), max: None, found: *header.gas_limit() }))); + return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { + min: Some(min_gas_limit), + max: None, + found: *header.gas_limit() + }))); } if let Some(limit) = engine.maximum_gas_limit() { if header.gas_limit() > &limit { - return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: *header.gas_limit() }))); + return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { + min: None, + max: Some(limit), + found: *header.gas_limit() + }))); } } let maximum_extra_data_size = engine.maximum_extra_data_size(); if header.number() != 0 && header.extra_data().len() > maximum_extra_data_size { - return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { min: None, max: Some(maximum_extra_data_size), found: header.extra_data().len() }))); + return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { + min: None, + max: Some(maximum_extra_data_size), + found: header.extra_data().len() + }))); } if let Some(ref ext) = engine.machine().ethash_extensions() { if header.number() >= ext.dao_hardfork_transition && header.number() <= ext.dao_hardfork_transition + 9 && header.extra_data()[..] != b"dao-hard-fork"[..] { - return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { min: None, max: None, found: 0 }))); + return Err(From::from(BlockError::ExtraDataOutOfBounds(OutOfBounds { + min: None, + max: None, + found: 0 + }))); } } - if is_full { - const ACCEPTABLE_DRIFT: Duration = Duration::from_secs(15); - // this will resist overflow until `year 2037` - let max_time = SystemTime::now() + ACCEPTABLE_DRIFT; - let invalid_threshold = max_time + ACCEPTABLE_DRIFT * 9; - let timestamp = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(header.timestamp())) - .ok_or(BlockError::TimestampOverflow)?; + Ok(()) +} - if timestamp > invalid_threshold { - return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: Some(max_time), min: None, found: timestamp }.into()))) - } +/// A header verification step that should be done for new block headers, but not for uncles. +pub(crate) fn verify_header_time(header: &Header) -> Result<(), Error> { + const ACCEPTABLE_DRIFT: Duration = Duration::from_secs(15); + // this will resist overflow until `year 2037` + let max_time = SystemTime::now() + ACCEPTABLE_DRIFT; + let invalid_threshold = max_time + ACCEPTABLE_DRIFT * 9; + let timestamp = CheckedSystemTime::checked_add(UNIX_EPOCH, Duration::from_secs(header.timestamp())) + .ok_or(BlockError::TimestampOverflow)?; - if timestamp > max_time { - return Err(From::from(BlockError::TemporarilyInvalid(OutOfBounds { max: Some(max_time), min: None, found: timestamp }.into()))) - } + if timestamp > invalid_threshold { + return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { + max: Some(max_time), + min: None, + found: timestamp + }.into()))) + } + + if timestamp > max_time { + return Err(From::from(BlockError::TemporarilyInvalid(OutOfBounds { + max: Some(max_time), + min: None, + found: timestamp + }.into()))) } Ok(()) @@ -326,18 +377,29 @@ fn verify_parent(header: &Header, parent: &Header, engine: &dyn Engine) -> Resul return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: None, min: Some(min), found }.into()))) } if header.number() != parent.number() + 1 { - return Err(From::from(BlockError::InvalidNumber(Mismatch { expected: parent.number() + 1, found: header.number() }))); + return Err(From::from(BlockError::InvalidNumber(Mismatch { + expected: parent.number() + 1, + found: header.number() + }))); } if header.number() == 0 { - return Err(BlockError::RidiculousNumber(OutOfBounds { min: Some(1), max: None, found: header.number() }).into()); + return Err(BlockError::RidiculousNumber(OutOfBounds { + min: Some(1), + max: None, + found: header.number() + }).into()); } let parent_gas_limit = *parent.gas_limit(); let min_gas = parent_gas_limit - parent_gas_limit / gas_limit_divisor; let max_gas = parent_gas_limit + parent_gas_limit / gas_limit_divisor; if header.gas_limit() <= &min_gas || header.gas_limit() >= &max_gas { - return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: Some(min_gas), max: Some(max_gas), found: *header.gas_limit() }))); + return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { + min: Some(min_gas), + max: Some(max_gas), + found: *header.gas_limit() + }))); } Ok(()) @@ -448,7 +510,7 @@ mod tests { block_provider: bc as &dyn BlockProvider, client: &client, }; - verify_block_family(&block.header, &parent, engine, Some(full_params)) + verify_block_family(&block.header, &parent, engine, full_params) } fn unordered_test(bytes: &[u8], engine: &dyn Engine) -> Result<(), Error> { diff --git a/ethcore/verification/src/verifier.rs b/ethcore/verification/src/verifier.rs deleted file mode 100644 index 982eab68f..000000000 --- a/ethcore/verification/src/verifier.rs +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2015-2019 Parity Technologies (UK) Ltd. -// This file is part of Parity Ethereum. - -// Parity Ethereum is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Parity Ethereum is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Parity Ethereum. If not, see . - -//! A generic verifier trait. - -use call_contract::CallContract; -use client_traits::BlockInfo; -use common_types::{ - header::Header, - errors::EthcoreError as Error, -}; -use engine::Engine; - -use super::verification; - -/// Should be used to verify blocks. -pub trait Verifier: Send + Sync - where C: BlockInfo + CallContract -{ - /// Verify a block relative to its parent and uncles. - fn verify_block_family( - &self, - header: &Header, - parent: &Header, - engine: &dyn Engine, - do_full: Option> - ) -> Result<(), Error>; - - /// Do a final verification check for an enacted header vs its expected counterpart. - fn verify_block_final(&self, expected: &Header, got: &Header) -> Result<(), Error>; - /// Verify a block, inspecting external state. - fn verify_block_external(&self, header: &Header, engine: &dyn Engine) -> Result<(), Error>; -}