From c4aa0c900851cf4ea89e0152a5bd5469fc2f7f5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 15 Oct 2018 19:23:49 +0200 Subject: [PATCH] Finish moving to `check_inherents` --- consensus/src/evaluation.rs | 7 ++- consensus/src/lib.rs | 50 ++++++++++++--------- primitives/src/lib.rs | 10 +++++ runtime/Cargo.toml | 4 +- runtime/src/lib.rs | 88 ++++++++++++++++++++++++++++++------- 5 files changed, 117 insertions(+), 42 deletions(-) diff --git a/consensus/src/evaluation.rs b/consensus/src/evaluation.rs index 72a90cdb0..729ce62f0 100644 --- a/consensus/src/evaluation.rs +++ b/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( proposal: &Block, - now: Timestamp, // TODO: should be InherentData parent_hash: &Hash, parent_number: <::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(()) } diff --git a/consensus/src/lib.rs b/consensus/src/lib.rs index 4fbb233ae..e986589a4 100644 --- a/consensus/src/lib.rs +++ b/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<::Block, Error=::Error> + + BlockBuilderExt<::Block, Error=::Error> + Core<::Block, AuthorityId, Error=::Error> + Miscellaneous<::Block, Error=::Error> + OldTxQueue<::Block, Error=::Error> @@ -347,7 +348,6 @@ impl bft::Proposer<::Block> for Proposer where assert!(evaluation::evaluate_initial( &substrate_block, - timestamp, &self.parent_hash, self.parent_number, ).is_ok()); @@ -358,34 +358,49 @@ impl bft::Proposer<::Block> for Proposer where fn evaluate(&self, unchecked_proposal: &::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 bft::Proposer<::Block> for Proposer 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) { diff --git a/primitives/src/lib.rs b/primitives/src/lib.rs index 4bd4d4774..c6802c716 100644 --- a/primitives/src/lib.rs +++ b/primitives/src/lib.rs @@ -88,3 +88,13 @@ pub struct InherentData { /// Indices of offline validators. pub offline_indices: Vec, } + +impl InherentData { + /// Create a new `InherentData` instance. + pub fn new(timestamp: Timestamp, offline_indices: Vec) -> Self { + Self { + timestamp, + offline_indices + } + } +} diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index de84c7034..4ee1fb1f9 100644 --- a/runtime/Cargo.toml +++ b/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" ] diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 60659ea8b..1fa70bbd7 100644 --- a/runtime/src/lib.rs +++ b/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; /// Executive: handles dispatch to the various modules. pub type Executive = executive::Executive; + +//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 { + fn check_inherents(block: Block, data: InherentData) -> Result<(), Error>; + } + } + + #[cfg(feature = "std")] + impl BlockBuilderExt for client::Client where + B: client::backend::Backend, + E: client::CallExecutor, + Block: BlockT, + { + type Error = client::error::Error; + + fn check_inherents( + &self, + at: &BlockId, + block: &Block, + data: &InherentData + ) -> Result, 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 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() -> ::Hash { + System::random_seed() + } + } + + impl BlockBuilderExt 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(()) + } + ) } }