Finish moving to check_inherents

consensus/src/evaluation.rs

@@ -20,7 +20,7 @@ use super::MAX_TRANSACTIONS_SIZE;
 
 use codec::{Decode, Encode};
 use node_runtime::{Block as GenericBlock};
-use node_primitives::{Hash, BlockNumber, Timestamp};
+use node_primitives::{Hash, BlockNumber};
 use runtime_primitives::traits::{Block as BlockT, Header as HeaderT, As};
 
 
@@ -52,7 +52,6 @@ error_chain! {
 /// upon any initial validity checks failing.
 pub fn evaluate_initial<Block: BlockT, Hash>(
 	proposal: &Block,
-	now: Timestamp,		// TODO: should be InherentData
 	parent_hash: &Hash,
 	parent_number: <<Block as BlockT>::Header as HeaderT>::Number,
 ) -> Result<()>
@@ -80,7 +79,7 @@ where
 		bail!(ErrorKind::WrongNumber(parent_number.as_() + 1, proposal.header.number));
 	}
 
-	// TODO: Should use 
+	// TODO: Should use
 
-	Ok(proposal)
+	Ok(())
 }

consensus/src/lib.rs

@@ -49,7 +49,7 @@ use client::{Client as SubstrateClient, CallExecutor};
 use client::runtime_api::{Core, BlockBuilder as BlockBuilderAPI, Miscellaneous, OldTxQueue};
 use codec::{Decode, Encode};
 use node_primitives::{AccountId, Timestamp, SessionKey, InherentData};
-use node_runtime::Runtime;
+use node_runtime::{block_builder_ext::{Error as BBEError, BlockBuilderExt}, Runtime};
 use primitives::{AuthorityId, ed25519, Blake2Hasher};
 use runtime_primitives::traits::{Block as BlockT, Hash as HashT, Header as HeaderT, As, BlockNumberToHash};
 use runtime_primitives::generic::{BlockId, Era};
@@ -88,6 +88,7 @@ pub trait AuthoringApi:
 	Send
 	+ Sync
 	+ BlockBuilderAPI<<Self as AuthoringApi>::Block, Error=<Self as AuthoringApi>::Error>
+	+ BlockBuilderExt<<Self as AuthoringApi>::Block, Error=<Self as AuthoringApi>::Error>
 	+ Core<<Self as AuthoringApi>::Block, AuthorityId, Error=<Self as AuthoringApi>::Error>
 	+ Miscellaneous<<Self as AuthoringApi>::Block, Error=<Self as AuthoringApi>::Error>
 	+ OldTxQueue<<Self as AuthoringApi>::Block, Error=<Self as AuthoringApi>::Error>
@@ -347,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());
@@ -358,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 vote_delays = {
-			let now = Instant::now();
+		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(BBEError::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 = {
 			// 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
 			};
@@ -398,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) {

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
+		}
+	}
+}

runtime/Cargo.toml

@@ -29,6 +29,7 @@ srml-staking = { git = "https://github.com/paritytech/substrate" }
 srml-system = { git = "https://github.com/paritytech/substrate" }
 srml-timestamp = { git = "https://github.com/paritytech/substrate" }
 srml-treasury = { git = "https://github.com/paritytech/substrate" }
+substrate-client = { git = "https://github.com/paritytech/substrate", optional = true }
 sr-version = { git = "https://github.com/paritytech/substrate" }
 node-primitives = { path = "../primitives" }
 
@@ -57,5 +58,6 @@ std = [
 	"node-primitives/std",
 	"serde_derive",
 	"serde/std",
-	"safe-mix/std"
+	"safe-mix/std",
+	"substrate-client"
 ]

runtime/src/lib.rs

@@ -58,7 +58,7 @@ extern crate sr_version as version;
 extern crate node_primitives;
 
 use rstd::prelude::*;
-use node_primitives::{AccountId, AccountIndex, Balance, BlockNumber, Hash, Index, SessionKey, Signature, InherentData};
+use node_primitives::{AccountId, AccountIndex, Balance, BlockNumber, Hash, Index, SessionKey, Signature, InherentData, Timestamp as TimestampType};
 use runtime_api::runtime::*;
 use runtime_primitives::ApplyResult;
 use runtime_primitives::transaction_validity::TransactionValidity;
@@ -77,7 +77,6 @@ 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;
@@ -187,6 +186,55 @@ pub type CheckedExtrinsic = generic::CheckedExtrinsic<AccountId, Index, Call>;
 /// Executive: handles dispatch to the various modules.
 pub type Executive = executive::Executive<Runtime, Block, Context, Balances, AllModules>;
 
+
+//TODO: Move upstream into `BlockBuilder`.
+pub mod block_builder_ext {
+	#[cfg(feature = "std")]
+	extern crate substrate_client as client;
+	#[cfg(feature = "std")]
+	extern crate parity_codec;
+
+	use super::BlockT;
+	use rstd::borrow::Cow;
+	#[cfg(feature = "std")]
+	use substrate_primitives::Blake2Hasher;
+	#[cfg(feature = "std")]
+	use runtime_primitives::generic::BlockId;
+
+	#[derive(Encode, Decode, Debug)]
+	pub enum Error {
+		Generic(Cow<'static, str>),
+		TimestampInFuture(u64),
+	}
+
+	decl_apis! {
+		pub trait BlockBuilderExt<Block: BlockT> {
+			fn check_inherents<InherentData>(block: Block, data: InherentData) -> Result<(), Error>;
+		}
+	}
+
+	#[cfg(feature = "std")]
+	impl<B, E, Block> BlockBuilderExt<Block> for client::Client<B, E, Block> where
+		B: client::backend::Backend<Block, Blake2Hasher>,
+		E: client::CallExecutor<Block, Blake2Hasher>,
+		Block: BlockT,
+	{
+		type Error = client::error::Error;
+
+		fn check_inherents<InherentData: parity_codec::Encode + parity_codec::Decode>(
+			&self,
+			at: &BlockId<Block>,
+			block: &Block,
+			data: &InherentData
+		) -> Result<Result<(), Error>, Self::Error> {
+			self.call_api_at(at, "check_inherents", &(block, data))
+		}
+	}
+
+}
+use block_builder_ext::runtime::BlockBuilderExt;
+use block_builder_ext::Error as BBEError;
+
 impl_apis! {
 	impl Core<Block, SessionKey> for Runtime {
 		fn version() -> RuntimeVersion {
@@ -236,34 +284,42 @@ impl_apis! {
 			inherent
 		}
 
-		fn check_inherents(block: Block, data: InherentData) -> Result<(), &'static str> {
+		fn random_seed() -> <Block as BlockT>::Hash {
+			System::random_seed()
+		}
+	}
+
+	impl BlockBuilderExt<Block, InherentData> for Runtime {
+		fn check_inherents(block: Block, data: InherentData) -> Result<(), BBEError> {
 			// TODO: v1: should be automatically gathered
 
 			// Timestamp module...
-			const MAX_TIMESTAMP_DRIFT: Timestamp = 60;
+			const MAX_TIMESTAMP_DRIFT: TimestampType = 60;
 			let xt = block.extrinsics.get(TIMESTAMP_SET_POSITION as usize)
-				.ok_or("No valid timestamp inherent in block")?;
-			let t = match (xt.is_signed(), xt.function) {
+				.ok_or_else(|| BBEError::Generic("No valid timestamp inherent in block".into()))?;
+			let t = match (xt.is_signed(), &xt.function) {
 				(false, Call::Timestamp(TimestampCall::set(t))) => t,
-				_ => bail!("No valid timestamp inherent in block"),
+				_ => return Err(BBEError::Generic("No valid timestamp inherent in block".into())),
 			};
-			if t > now + MAX_TIMESTAMP_DRIFT {
-				bail!("Timestamp in future")
+
+			if *t > data.timestamp + MAX_TIMESTAMP_DRIFT {
+				return Err(BBEError::TimestampInFuture(*t))
 			}
 
 			// Offline indices
 			let noted_offline =
-				self.inner.extrinsics.get(NOTE_OFFLINE_POSITION as usize).and_then(|xt| match xt.function {
+				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(&[]);
-			for i in noted_offline {
-				if data.offline_indices.iter().find(i).is_none() {
-					bail!("Online node marked offline")
-				}
-			}
 
-			Ok(())
+			noted_offline.iter().try_for_each(|n|
+				if !data.offline_indices.contains(n) {
+					Err(BBEError::Generic("Online node marked offline".into()))
+				} else {
+					Ok(())
+				}
+			)
 		}
 	}