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

@@ -133,8 +133,7 @@ use types::{
 	verification::{Unverified, VerificationQueueInfo as BlockQueueInfo},
 };
 use types::data_format::DataFormat;
-use verification::{BlockQueue, Verifier};
+use verification::{self, BlockQueue};
-use verification;
 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<dyn Verifier<Client>>,
-
 	/// Queue containing pending blocks
 	pub block_queue: BlockQueue<Client>,
 
@@ -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());
 		}

ethcore/verification/src/canon_verifier.rs

@@ -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 <http://www.gnu.org/licenses/>.
-
-//! 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<C: BlockInfo + CallContract> Verifier<C> for CanonVerifier {
-	fn verify_block_family(
-		&self,
-		header: &Header,
-		parent: &Header,
-		engine: &dyn Engine,
-		do_full: Option<verification::FullFamilyParams<C>>,
-	) -> 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)
-	}
-}

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::verification::{FullFamilyParams, verify_block_family, verify_block_final};
-pub use self::verifier::Verifier;
 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<C: BlockInfo + CallContract>(v: VerifierType) -> Box<dyn Verifier<C>> {
-	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,
 		}
 	}
 }

ethcore/verification/src/noop_verifier.rs

@@ -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 <http://www.gnu.org/licenses/>.
-
-//! 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<C: BlockInfo + CallContract> Verifier<C> for NoopVerifier {
-	fn verify_block_family(
-		&self,
-		_: &Header,
-		_t: &Header,
-		_: &dyn Engine,
-		_: Option<verification::FullFamilyParams<C>>
-	) -> 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(())
-	}
-}

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<Self::Unverified, (Self::Input, Error)> {
-			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))
 			}

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<C: BlockInfo + CallContract>(
 	header: &Header,
 	parent: &Header,
 	engine: &dyn Engine,
-	do_full: Option<FullFamilyParams<C>>
+	params: FullFamilyParams<C>
 ) -> 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 &params.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)
+					let b = bc.block(&hash).expect("parent already known to be stored; qed");
-						.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,34 +279,62 @@ 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 {
+	Ok(())
+}
+
+/// 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;
@@ -299,12 +343,19 @@ pub(crate) fn verify_header_params(header: &Header, engine: &dyn Engine, is_full
 		.ok_or(BlockError::TimestampOverflow)?;
 
 	if timestamp > invalid_threshold {
-			return Err(From::from(BlockError::InvalidTimestamp(OutOfBounds { max: Some(max_time), min: None, found: timestamp }.into())))
+		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())))
+		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> {

ethcore/verification/src/verifier.rs

@@ -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 <http://www.gnu.org/licenses/>.
-
-//! 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<C>: 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<verification::FullFamilyParams<C>>
-	) -> 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>;
-}