Adds check_inherents function to the BlockerBuilder API (#912)

* Adds new `check_inherents` function to the `BlockBuilder` API

* Switch to `check_inherents` in `node-consensus`

* Remove `CheckedBlock`, because it is not required anymore

* Fixes after rebase

* Fixes compilation on stable

substrate/node/consensus/src/evaluation.rs

@@ -19,8 +19,8 @@
 use super::MAX_TRANSACTIONS_SIZE;
 
 use codec::{Decode, Encode};
-use node_runtime::{Block as GenericBlock, CheckedBlock};
-use node_primitives::{Hash, BlockNumber, Timestamp};
+use node_runtime::{Block as GenericBlock};
+use node_primitives::{Hash, BlockNumber};
 use runtime_primitives::traits::{Block as BlockT, Header as HeaderT, As};
 
 
@@ -30,10 +30,6 @@ error_chain! {
 			description("Proposal provided not a block."),
 			display("Proposal provided not a block."),
 		}
-		TimestampInFuture {
-			description("Proposal had timestamp too far in the future."),
-			display("Proposal had timestamp too far in the future."),
-		}
 		WrongParentHash(expected: Hash, got: Hash) {
 			description("Proposal had wrong parent hash."),
 			display("Proposal had wrong parent hash. Expected {:?}, got {:?}", expected, got),
@@ -56,19 +52,15 @@ error_chain! {
 /// upon any initial validity checks failing.
 pub fn evaluate_initial<Block: BlockT, Hash>(
 	proposal: &Block,
-	now: Timestamp,
 	parent_hash: &Hash,
 	parent_number: <<Block as BlockT>::Header as HeaderT>::Number,
-) -> Result<CheckedBlock>
+) -> Result<()>
 where
 	Hash: PartialEq<<<GenericBlock as BlockT>::Header as HeaderT>::Hash>,
 	Hash: Into<self::Hash> + Clone,
 {
-	const MAX_TIMESTAMP_DRIFT: Timestamp = 60;
-
 	let encoded = Encode::encode(proposal);
 	let proposal = GenericBlock::decode(&mut &encoded[..])
-		.and_then(|b| CheckedBlock::new(b).ok())
 		.ok_or_else(|| ErrorKind::BadProposalFormat)?;
 
 	let transactions_size = proposal.extrinsics.iter().fold(0, |a, tx| {
@@ -87,12 +79,5 @@ where
 		bail!(ErrorKind::WrongNumber(parent_number.as_() + 1, proposal.header.number));
 	}
 
-	let block_timestamp = proposal.timestamp();
-
-	// lenient maximum -- small drifts will just be delayed using a timer.
-	if block_timestamp > now + MAX_TIMESTAMP_DRIFT {
-		bail!(ErrorKind::TimestampInFuture)
-	}
-
-	Ok(proposal)
+	Ok(())
 }

substrate/node/consensus/src/lib.rs

@@ -48,7 +48,7 @@ use std::sync::Arc;
 use std::time::{self, Duration, Instant};
 
 use client::{Client as SubstrateClient, CallExecutor};
-use client::runtime_api::{Core, BlockBuilder as BlockBuilderAPI, Miscellaneous, OldTxQueue};
+use client::runtime_api::{Core, BlockBuilder as BlockBuilderAPI, Miscellaneous, OldTxQueue, BlockBuilderError};
 use codec::{Decode, Encode};
 use node_primitives::{AccountId, Timestamp, SessionKey, InherentData};
 use node_runtime::Runtime;
@@ -348,7 +348,6 @@ impl<C, A> bft::Proposer<<C as AuthoringApi>::Block> for Proposer<C, A> where
 
 		assert!(evaluation::evaluate_initial(
 			&substrate_block,
-			timestamp,
 			&self.parent_hash,
 			self.parent_number,
 		).is_ok());
@@ -359,34 +358,49 @@ impl<C, A> bft::Proposer<<C as AuthoringApi>::Block> for Proposer<C, A> where
 	fn evaluate(&self, unchecked_proposal: &<C as AuthoringApi>::Block) -> Self::Evaluate {
 		debug!(target: "bft", "evaluating block on top of parent ({}, {:?})", self.parent_number, self.parent_hash);
 
-		let current_timestamp = current_timestamp();
-
 		// do initial serialization and structural integrity checks.
-		let maybe_proposal = evaluation::evaluate_initial(
+		match evaluation::evaluate_initial(
 			unchecked_proposal,
-			current_timestamp,
 			&self.parent_hash,
 			self.parent_number,
-		);
-
-		let proposal = match maybe_proposal {
+		) {
 			Ok(p) => p,
 			Err(e) => {
 				// TODO: these errors are easily re-checked in runtime.
-				debug!(target: "bft", "Invalid proposal: {:?}", e);
+				debug!(target: "bft", "Invalid proposal (initial evaluation failed): {:?}", e);
+				return Box::new(future::ok(false));
+			}
+		};
+
+		let current_timestamp = current_timestamp();
+		let inherent = InherentData::new(
+			current_timestamp,
+			self.offline.read().reports(&self.validators)
+		);
+		let proposed_timestamp = match self.client.check_inherents(
+			&self.parent_id,
+			&unchecked_proposal,
+			&inherent
+		) {
+			Ok(Ok(())) => None,
+			Ok(Err(BlockBuilderError::TimestampInFuture(timestamp))) => Some(timestamp),
+			Ok(Err(e)) => {
+				debug!(target: "bft", "Invalid proposal (check_inherents): {:?}", e);
+				return Box::new(future::ok(false));
+			},
+			Err(e) => {
+				debug!(target: "bft", "Could not call into runtime: {:?}", e);
 				return Box::new(future::ok(false));
 			}
 		};
 
 		let vote_delays = {
-			let now = Instant::now();
-
 			// the duration until the given timestamp is current
-			let proposed_timestamp = ::std::cmp::max(self.minimum_timestamp, proposal.timestamp());
+			let proposed_timestamp = ::std::cmp::max(self.minimum_timestamp, proposed_timestamp.unwrap_or(0));
 			let timestamp_delay = if proposed_timestamp > current_timestamp {
 				let delay_s = proposed_timestamp - current_timestamp;
 				debug!(target: "bft", "Delaying evaluation of proposal for {} seconds", delay_s);
-				Some(now + Duration::from_secs(delay_s))
+				Some(Instant::now() + Duration::from_secs(delay_s))
 			} else {
 				None
 			};
@@ -399,13 +413,6 @@ impl<C, A> bft::Proposer<<C as AuthoringApi>::Block> for Proposer<C, A> where
 			}
 		};
 
-		// refuse to vote if this block says a validator is offline that we
-		// think isn't.
-		let offline = proposal.noted_offline();
-		if !self.offline.read().check_consistency(&self.validators[..], offline) {
-			return Box::new(futures::empty());
-		}
-
 		// evaluate whether the block is actually valid.
 		// TODO: is it better to delay this until the delays are finished?
 		let evaluated = match self.client.execute_block(&self.parent_id, &unchecked_proposal.clone()).map_err(Error::from) {

substrate/node/primitives/src/lib.rs

@@ -88,3 +88,13 @@ pub struct InherentData {
 	/// Indices of offline validators.
 	pub offline_indices: Vec<u32>,
 }
+
+impl InherentData {
+	/// Create a new `InherentData` instance.
+	pub fn new(timestamp: Timestamp, offline_indices: Vec<u32>) -> Self {
+		Self {
+			timestamp,
+			offline_indices
+		}
+	}
+}

substrate/node/runtime/src/checked_block.rs

@@ -1,94 +0,0 @@
-// Copyright 2018 Parity Technologies (UK) Ltd.
-// This file is part of Substrate.
-
-// Substrate 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.
-
-// Substrate 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 Substrate.  If not, see <http://www.gnu.org/licenses/>.
-
-//! Typesafe block interaction.
-
-use super::{Call, Block, TIMESTAMP_SET_POSITION, NOTE_OFFLINE_POSITION};
-use timestamp::Call as TimestampCall;
-use consensus::Call as ConsensusCall;
-
-/// Provides a type-safe wrapper around a structurally valid block.
-pub struct CheckedBlock {
-	inner: Block,
-	file_line: Option<(&'static str, u32)>,
-}
-
-impl CheckedBlock {
-	/// Create a new checked block. Fails if the block is not structurally valid.
-	pub fn new(block: Block) -> Result<Self, Block> {
-		let has_timestamp = block.extrinsics.get(TIMESTAMP_SET_POSITION as usize).map_or(false, |xt| {
-			!xt.is_signed() && match xt.function {
-				Call::Timestamp(TimestampCall::set(_)) => true,
-				_ => false,
-			}
-		});
-
-		if !has_timestamp { return Err(block) }
-
-		Ok(CheckedBlock {
-			inner: block,
-			file_line: None,
-		})
-	}
-
-	// Creates a new checked block, asserting that it is valid.
-	#[doc(hidden)]
-	pub fn new_unchecked(block: Block, file: &'static str, line: u32) -> Self {
-		CheckedBlock {
-			inner: block,
-			file_line: Some((file, line)),
-		}
-	}
-
-	/// Extract the timestamp from the block.
-	pub fn timestamp(&self) -> ::node_primitives::Timestamp {
-		let x = self.inner.extrinsics.get(TIMESTAMP_SET_POSITION as usize).and_then(|xt| match xt.function {
-			Call::Timestamp(TimestampCall::set(x)) => Some(x),
-			_ => None
-		});
-
-		match x {
-			Some(x) => x,
-			None => panic!("Invalid block asserted at {:?}", self.file_line),
-		}
-	}
-
-	/// Extract the noted missed proposal validator indices (if any) from the block.
-	pub fn noted_offline(&self) -> &[u32] {
-		self.inner.extrinsics.get(NOTE_OFFLINE_POSITION as usize).and_then(|xt| match xt.function {
-			Call::Consensus(ConsensusCall::note_offline(ref x)) => Some(&x[..]),
-			_ => None,
-		}).unwrap_or(&[])
-	}
-
-	/// Convert into inner block.
-	pub fn into_inner(self) -> Block { self.inner }
-}
-
-impl ::std::ops::Deref for CheckedBlock {
-	type Target = Block;
-
-	fn deref(&self) -> &Block { &self.inner }
-}
-
-/// Assert that a block is structurally valid. May lead to panic in the future
-/// in case it isn't.
-#[macro_export]
-macro_rules! assert_node_block {
-	($block: expr) => {
-		$crate::CheckedBlock::new_unchecked($block, file!(), line!())
-	}
-}

substrate/node/runtime/src/lib.rs

@@ -57,13 +57,13 @@ extern crate srml_treasury as treasury;
 extern crate sr_version as version;
 extern crate node_primitives;
 
-#[cfg(feature = "std")]
-mod checked_block;
-
 use rstd::prelude::*;
 use substrate_primitives::u32_trait::{_2, _4};
-use node_primitives::{AccountId, AccountIndex, Balance, BlockNumber, Hash, Index, SessionKey, Signature, InherentData};
-use runtime_api::runtime::*;
+use node_primitives::{
+	AccountId, AccountIndex, Balance, BlockNumber, Hash, Index,
+	SessionKey, Signature, InherentData, Timestamp as TimestampType
+};
+use runtime_api::{BlockBuilderError, runtime::*};
 use runtime_primitives::ApplyResult;
 use runtime_primitives::transaction_validity::TransactionValidity;
 use runtime_primitives::generic;
@@ -83,8 +83,6 @@ pub use balances::Call as BalancesCall;
 pub use runtime_primitives::{Permill, Perbill};
 pub use timestamp::BlockPeriod;
 pub use srml_support::StorageValue;
-#[cfg(any(feature = "std", test))]
-pub use checked_block::CheckedBlock;
 
 const TIMESTAMP_SET_POSITION: u32 = 0;
 const NOTE_OFFLINE_POSITION: u32 = 1;
@@ -271,7 +269,7 @@ impl_apis! {
 		}
 	}
 
-	impl BlockBuilder<Block, InherentData, UncheckedExtrinsic> for Runtime {
+	impl BlockBuilder<Block, InherentData, UncheckedExtrinsic, InherentData> for Runtime {
 		fn initialise_block(header: <Block as BlockT>::Header) {
 			Executive::initialise_block(&header)
 		}
@@ -298,6 +296,38 @@ impl_apis! {
 			inherent
 		}
 
+		fn check_inherents(block: Block, data: InherentData) -> Result<(), BlockBuilderError> {
+			// TODO: v1: should be automatically gathered
+
+			// Timestamp module...
+			const MAX_TIMESTAMP_DRIFT: TimestampType = 60;
+			let xt = block.extrinsics.get(TIMESTAMP_SET_POSITION as usize)
+				.ok_or_else(|| BlockBuilderError::Generic("No valid timestamp inherent in block".into()))?;
+			let t = match (xt.is_signed(), &xt.function) {
+				(false, Call::Timestamp(TimestampCall::set(t))) => t,
+				_ => return Err(BlockBuilderError::Generic("No valid timestamp inherent in block".into())),
+			};
+
+			if *t > data.timestamp + MAX_TIMESTAMP_DRIFT {
+				return Err(BlockBuilderError::TimestampInFuture(*t))
+			}
+
+			// Offline indices
+			let noted_offline =
+				block.extrinsics.get(NOTE_OFFLINE_POSITION as usize).and_then(|xt| match xt.function {
+					Call::Consensus(ConsensusCall::note_offline(ref x)) => Some(&x[..]),
+					_ => None,
+				}).unwrap_or(&[]);
+
+			noted_offline.iter().try_for_each(|n|
+				if !data.offline_indices.contains(n) {
+					Err(BlockBuilderError::Generic("Online node marked offline".into()))
+				} else {
+					Ok(())
+				}
+			)
+		}
+
 		fn random_seed() -> <Block as BlockT>::Hash {
 			System::random_seed()
 		}