Set uncles inherent (#3317)

* Include uncles

* Filter missing uncles

* Moved inherent registration to a new crate

* Ignore invalid inherent encoding

substrate/Cargo.lock

@@ -3738,6 +3738,7 @@ dependencies = [
  "sr-std 2.0.0",
  "srml-support 2.0.0",
  "srml-system 2.0.0",
+ "substrate-inherents 2.0.0",
  "substrate-primitives 2.0.0",
 ]
 
@@ -4457,6 +4458,7 @@ dependencies = [
  "substrate-consensus-babe-primitives 2.0.0",
  "substrate-consensus-common 2.0.0",
  "substrate-consensus-slots 2.0.0",
+ "substrate-consensus-uncles 2.0.0",
  "substrate-executor 2.0.0",
  "substrate-inherents 2.0.0",
  "substrate-keyring 2.0.0",
@@ -4553,6 +4555,19 @@ dependencies = [
  "substrate-test-runtime-client 2.0.0",
 ]
 
+[[package]]
+name = "substrate-consensus-uncles"
+version = "2.0.0"
+dependencies = [
+ "log 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)",
+ "sr-primitives 2.0.0",
+ "srml-authorship 0.1.0",
+ "substrate-client 2.0.0",
+ "substrate-consensus-common 2.0.0",
+ "substrate-inherents 2.0.0",
+ "substrate-primitives 2.0.0",
+]
+
 [[package]]
 name = "substrate-executor"
 version = "2.0.0"

substrate/Cargo.toml

@@ -28,6 +28,7 @@ members = [
 	"core/consensus/common",
 	"core/consensus/rhd",
 	"core/consensus/slots",
+	"core/consensus/uncles",
 	"core/executor",
 	"core/executor/runtime-test",
 	"core/finality-grandpa",

substrate/core/client/src/client.rs

@@ -170,6 +170,13 @@ pub trait BlockBody<Block: BlockT> {
 	) -> error::Result<Option<Vec<<Block as BlockT>::Extrinsic>>>;
 }
 
+/// Provide a list of potential uncle headers for a given block.
+pub trait ProvideUncles<Block: BlockT> {
+	/// Gets the uncles of the block with `target_hash` going back `max_generation` ancestors.
+	fn uncles(&self, target_hash: Block::Hash, max_generation: NumberFor<Block>)
+		-> error::Result<Vec<Block::Header>>;
+}
+
 /// Client info
 #[derive(Debug)]
 pub struct ClientInfo<Block: BlockT> {
@@ -1329,7 +1336,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			ancestor_hash = *current.parent_hash();
 			ancestor = load_header(ancestor_hash)?;
 		}
-
+		trace!("Collected {} uncles", uncles.len());
 		Ok(uncles)
 	}
 
@@ -1353,6 +1360,20 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 	}
 }
 
+impl<B, E, Block, RA> ProvideUncles<Block> for Client<B, E, Block, RA> where
+	B: backend::Backend<Block, Blake2Hasher>,
+	E: CallExecutor<Block, Blake2Hasher>,
+	Block: BlockT<Hash=H256>,
+{
+	fn uncles(&self, target_hash: Block::Hash, max_generation: NumberFor<Block>) -> error::Result<Vec<Block::Header>> {
+		Ok(Client::uncles(self, target_hash, max_generation)?
+			.into_iter()
+			.filter_map(|hash| Client::header(self, &BlockId::Hash(hash)).unwrap_or(None))
+			.collect()
+		)
+	}
+}
+
 impl<B, E, Block, RA> ChainHeaderBackend<Block> for Client<B, E, Block, RA> where
 	B: backend::Backend<Block, Blake2Hasher>,
 	E: CallExecutor<Block, Blake2Hasher> + Send + Sync,

substrate/core/client/src/lib.rs

@@ -115,7 +115,7 @@ pub use crate::client::{
 	new_in_mem,
 	BlockBody, BlockStatus, ImportNotifications, FinalityNotifications, BlockchainEvents,
 	BlockImportNotification, Client, ClientInfo, ExecutionStrategies, FinalityNotification,
-	LongestChain, BlockOf,
+	LongestChain, BlockOf, ProvideUncles,
 	utils,
 };
 #[cfg(feature = "std")]

substrate/core/consensus/babe/Cargo.toml

@@ -22,6 +22,7 @@ keystore = { package = "substrate-keystore", path = "../../keystore" }
 srml-babe = { path = "../../../srml/babe" }
 client = { package = "substrate-client", path = "../../client" }
 consensus-common = { package = "substrate-consensus-common", path = "../common" }
+uncles = { package = "substrate-consensus-uncles", path = "../uncles" }
 slots = { package = "substrate-consensus-slots", path = "../slots"  }
 sr-primitives = {  path = "../../sr-primitives" }
 fork-tree = { path = "../../utils/fork-tree" }

substrate/core/consensus/babe/src/lib.rs

@@ -67,6 +67,7 @@ use client::{
 	block_builder::api::BlockBuilder as BlockBuilderApi,
 	blockchain::{self, HeaderBackend, ProvideCache}, BlockchainEvents, CallExecutor, Client,
 	runtime_api::ApiExt, error::Result as ClientResult, backend::{AuxStore, Backend},
+	ProvideUncles,
 	utils::is_descendent_of,
 };
 use fork_tree::ForkTree;
@@ -182,9 +183,9 @@ pub fn start_babe<B, C, SC, E, I, SO, Error, H>(BabeParams {
 	consensus_common::Error,
 > where
 	B: BlockT<Header=H>,
-	C: ProvideRuntimeApi + ProvideCache<B>,
+	C: ProvideRuntimeApi + ProvideCache<B> + ProvideUncles<B> + Send + Sync + 'static,
 	C::Api: BabeApi<B>,
-	SC: SelectChain<B>,
+	SC: SelectChain<B> + 'static,
 	E::Proposer: Proposer<B, Error=Error>,
 	<E::Proposer as Proposer<B>>::Create: Unpin + Send + 'static,
 	H: Header<Hash=B::Hash>,
@@ -203,6 +204,11 @@ pub fn start_babe<B, C, SC, E, I, SO, Error, H>(BabeParams {
 		keystore,
 	};
 	register_babe_inherent_data_provider(&inherent_data_providers, config.0.slot_duration())?;
+	uncles::register_uncles_inherent_data_provider(
+		client.clone(),
+		select_chain.clone(),
+		&inherent_data_providers,
+	)?;
 	Ok(slots::start_slot_worker(
 		config.0,
 		select_chain,

substrate/core/consensus/uncles/Cargo.toml

@@ -0,0 +1,15 @@
+[package]
+name = "substrate-consensus-uncles"
+version = "2.0.0"
+authors = ["Parity Technologies <admin@parity.io>"]
+description = "Generic uncle inclusion utilities for consensus"
+edition = "2018"
+
+[dependencies]
+client = { package = "substrate-client", path = "../../client" }
+primitives = { package = "substrate-primitives", path = "../../primitives" }
+sr-primitives = {  path = "../../sr-primitives" }
+srml-authorship = {  path = "../../../srml/authorship" }
+consensus_common = { package = "substrate-consensus-common", path = "../common" }
+inherents = { package = "substrate-inherents", path = "../../inherents" }
+log = "0.4"

substrate/core/consensus/uncles/src/lib.rs

@@ -0,0 +1,66 @@
+// Copyright 2019 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/>.
+
+//! Uncles functionality for Substrate.
+//!
+#![deny(warnings)]
+#![forbid(unsafe_code, missing_docs)]
+
+use consensus_common::SelectChain;
+use inherents::{InherentDataProviders};
+use log::warn;
+use client::ProvideUncles;
+use sr_primitives::traits::{Block as BlockT, Header};
+use std::sync::Arc;
+
+/// Maximum uncles generations we may provide to the runtime.
+const MAX_UNCLE_GENERATIONS: u32 = 8;
+
+/// Register uncles inherent data provider, if not registered already.
+pub fn register_uncles_inherent_data_provider<B, C, SC>(
+	client: Arc<C>,
+	select_chain: SC,
+	inherent_data_providers: &InherentDataProviders,
+) -> Result<(), consensus_common::Error> where
+	B: BlockT,
+	C: ProvideUncles<B> + Send + Sync + 'static,
+	SC: SelectChain<B> + 'static,
+{
+	if !inherent_data_providers.has_provider(&srml_authorship::INHERENT_IDENTIFIER) {
+		inherent_data_providers
+			.register_provider(srml_authorship::InherentDataProvider::new(move || {
+				{
+					let chain_head = match select_chain.best_chain() {
+						Ok(x) => x,
+						Err(e) => {
+							warn!(target: "uncles", "Unable to get chain head: {:?}", e);
+							return Vec::new();
+						}
+					};
+					match client.uncles(chain_head.hash(), MAX_UNCLE_GENERATIONS.into()) {
+						Ok(uncles) => uncles,
+						Err(e) => {
+							warn!(target: "uncles", "Unable to get uncles: {:?}", e);
+							Vec::new()
+						}
+					}
+				}
+			}))
+		.map_err(|err| consensus_common::Error::InherentData(err.into()))?;
+	}
+	Ok(())
+}
+

substrate/node/runtime/src/lib.rs

@@ -80,8 +80,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to equal spec_version. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 131,
+	spec_version: 134,
-	impl_version: 133,
+	impl_version: 134,
 	apis: RUNTIME_API_VERSIONS,
 };
 
@@ -179,7 +179,7 @@ impl timestamp::Trait for Runtime {
 }
 
 parameter_types! {
-	pub const UncleGenerations: u64 = 0;
+	pub const UncleGenerations: u64 = 5;
 }
 
 impl authorship::Trait for Runtime {
@@ -421,7 +421,7 @@ construct_runtime!(
 		System: system::{Module, Call, Storage, Config, Event},
 		Babe: babe::{Module, Call, Storage, Config, Inherent(Timestamp)},
 		Timestamp: timestamp::{Module, Call, Storage, Inherent},
-		Authorship: authorship::{Module, Call, Storage},
+		Authorship: authorship::{Module, Call, Storage, Inherent},
 		Indices: indices,
 		Balances: balances,
 		Staking: staking::{default, OfflineWorker},

substrate/srml/authorship/Cargo.toml

@@ -8,6 +8,7 @@ edition = "2018"
 [dependencies]
 primitives = { package = "substrate-primitives",  path = "../../core/primitives", default-features = false }
 codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false, features = ["derive"] }
+inherents = { package = "substrate-inherents", path = "../../core/inherents", default-features = false }
 rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false }
 sr-primitives = { path = "../../core/sr-primitives", default-features = false }
 srml-support = { path = "../support", default-features = false }
@@ -19,6 +20,7 @@ default = ["std"]
 std = [
 	"codec/std",
 	"primitives/std",
+	"inherents/std",
 	"sr-primitives/std",
 	"rstd/std",
 	"srml-support/std",

substrate/srml/authorship/src/lib.rs

@@ -20,7 +20,7 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use rstd::prelude::*;
+use rstd::{result, prelude::*};
 use rstd::collections::btree_set::BTreeSet;
 use srml_support::{decl_module, decl_storage, for_each_tuple, StorageValue};
 use srml_support::traits::{FindAuthor, VerifySeal, Get};
@@ -29,6 +29,61 @@ use codec::{Encode, Decode};
 use system::ensure_none;
 use sr_primitives::traits::{SimpleArithmetic, Header as HeaderT, One, Zero};
 use sr_primitives::weights::SimpleDispatchInfo;
+use inherents::{
+	RuntimeString, InherentIdentifier, ProvideInherent,
+	InherentData, MakeFatalError,
+};
+
+/// The identifier for the `uncles` inherent.
+pub const INHERENT_IDENTIFIER: InherentIdentifier = *b"uncles00";
+
+/// Auxiliary trait to extract uncles inherent data.
+pub trait UnclesInherentData<H: Decode> {
+	/// Get uncles.
+	fn uncles(&self) -> Result<Vec<H>, RuntimeString>;
+}
+
+impl<H: Decode> UnclesInherentData<H> for InherentData {
+	fn uncles(&self) -> Result<Vec<H>, RuntimeString> {
+		Ok(self.get_data(&INHERENT_IDENTIFIER)?.unwrap_or_default())
+	}
+}
+
+/// Provider for inherent data.
+#[cfg(feature = "std")]
+pub struct InherentDataProvider<F, H> {
+	inner: F,
+	_marker: std::marker::PhantomData<H>,
+}
+
+#[cfg(feature = "std")]
+impl<F, H> InherentDataProvider<F, H> {
+	pub fn new(uncles_oracle: F) -> Self {
+		InherentDataProvider { inner: uncles_oracle, _marker: Default::default() }
+	}
+}
+
+#[cfg(feature = "std")]
+impl<F, H: Encode + std::fmt::Debug> inherents::ProvideInherentData for InherentDataProvider<F, H>
+where F: Fn() -> Vec<H>
+{
+	fn inherent_identifier(&self) -> &'static InherentIdentifier {
+		&INHERENT_IDENTIFIER
+	}
+
+	fn provide_inherent_data(&self, inherent_data: &mut InherentData) -> Result<(), RuntimeString> {
+		let uncles = (self.inner)();
+		if !uncles.is_empty() {
+			inherent_data.put_data(INHERENT_IDENTIFIER, &uncles)
+		} else {
+			Ok(())
+		}
+	}
+
+	fn error_to_string(&self, _error: &[u8]) -> Option<String> {
+		Some(format!("no further information"))
+	}
+}
 
 pub trait Trait: system::Trait {
 	/// Find the author of a block.
@@ -101,16 +156,16 @@ pub trait FilterUncle<Header, Author> {
 
 	/// Do additional filtering on a seal-checked uncle block, with the accumulated
 	/// filter.
-	fn filter_uncle(header: &Header, acc: Self::Accumulator)
+	fn filter_uncle(header: &Header, acc: &mut Self::Accumulator)
-		-> Result<(Option<Author>, Self::Accumulator), &'static str>;
+		-> Result<Option<Author>, &'static str>;
 }
 
 impl<H, A> FilterUncle<H, A> for () {
 	type Accumulator = ();
-	fn filter_uncle(_: &H, acc: Self::Accumulator)
+	fn filter_uncle(_: &H, _acc: &mut Self::Accumulator)
-		-> Result<(Option<A>, Self::Accumulator), &'static str>
+		-> Result<Option<A>, &'static str>
 	{
-		Ok((None, acc))
+		Ok(None)
 	}
 }
 
@@ -124,10 +179,10 @@ impl<Header, Author, T: VerifySeal<Header, Author>> FilterUncle<Header, Author>
 {
 	type Accumulator = ();
 
-	fn filter_uncle(header: &Header, _acc: ())
+	fn filter_uncle(header: &Header, _acc: &mut ())
-		-> Result<(Option<Author>, ()), &'static str>
+		-> Result<Option<Author>, &'static str>
 	{
-		T::verify_seal(header).map(|author| (author, ()))
+		T::verify_seal(header)
 	}
 }
 
@@ -147,8 +202,8 @@ where
 {
 	type Accumulator = BTreeSet<(Header::Number, Author)>;
 
-	fn filter_uncle(header: &Header, mut acc: Self::Accumulator)
+	fn filter_uncle(header: &Header, acc: &mut Self::Accumulator)
-		-> Result<(Option<Author>, Self::Accumulator), &'static str>
+		-> Result<Option<Author>, &'static str>
 	{
 		let author = T::verify_seal(header)?;
 		let number = header.number();
@@ -159,7 +214,7 @@ where
 			}
 		}
 
-		Ok((author, acc))
+		Ok(author)
 	}
 }
 
@@ -256,6 +311,39 @@ impl<T: Trait> Module<T> {
 	fn verify_and_import_uncles(new_uncles: Vec<T::Header>) -> DispatchResult {
 		let now = <system::Module<T>>::block_number();
 
+		let mut uncles = <Self as Store>::Uncles::get();
+		uncles.push(UncleEntryItem::InclusionHeight(now));
+
+		let mut acc: <T::FilterUncle as FilterUncle<_, _>>::Accumulator = Default::default();
+
+		for uncle in new_uncles {
+			let prev_uncles = uncles.iter().filter_map(|entry|
+				match entry {
+					UncleEntryItem::InclusionHeight(_) => None,
+					UncleEntryItem::Uncle(h, _) => Some(h),
+				});
+			let author = Self::verify_uncle(&uncle, prev_uncles, &mut acc)?;
+			let hash = uncle.hash();
+
+			T::EventHandler::note_uncle(
+				author.clone().unwrap_or_default(),
+				now - uncle.number().clone(),
+			);
+			uncles.push(UncleEntryItem::Uncle(hash, author));
+		}
+
+		<Self as Store>::Uncles::put(&uncles);
+		Ok(())
+	}
+
+	fn verify_uncle<'a, I: IntoIterator<Item=&'a T::Hash>>(
+		uncle: &T::Header,
+		existing_uncles: I,
+		accumulator: &mut <T::FilterUncle as FilterUncle<T::Header, T::AccountId>>::Accumulator,
+	) -> Result<Option<T::AccountId>, &'static str>
+	{
+		let now = <system::Module<T>>::block_number();
+
 		let (minimum_height, maximum_height) = {
 			let uncle_generations = T::UncleGenerations::get();
 			let min = if now >= uncle_generations {
@@ -267,12 +355,6 @@ impl<T: Trait> Module<T> {
 			(min, now)
 		};
 
-		let mut uncles = <Self as Store>::Uncles::get();
-		uncles.push(UncleEntryItem::InclusionHeight(now));
-
-		let mut acc: <T::FilterUncle as FilterUncle<_, _>>::Accumulator = Default::default();
-
-		for uncle in new_uncles {
 		let hash = uncle.hash();
 
 		if uncle.number() < &One::one() {
@@ -280,7 +362,7 @@ impl<T: Trait> Module<T> {
 		}
 
 		if uncle.number() > &maximum_height {
-				return Err("uncles too high in chain");
+			return Err("uncle is too high in chain");
 		}
 
 		{
@@ -295,27 +377,60 @@ impl<T: Trait> Module<T> {
 			return Err("uncle not recent enough to be included");
 		}
 
-			let duplicate = uncles.iter().find(|entry| match entry {
+		let duplicate = existing_uncles.into_iter().find(|h| **h == hash).is_some();
-				UncleEntryItem::InclusionHeight(_) => false,
-				UncleEntryItem::Uncle(h, _) => h == &hash,
-			}).is_some();
-
 		let in_chain = <system::Module<T>>::block_hash(uncle.number()) == hash;
 
-			if duplicate || in_chain { return Err("uncle already included") }
+		if duplicate || in_chain {
-
+			return Err("uncle already included")
-			// check uncle validity.
-			let (author, temp_acc) = T::FilterUncle::filter_uncle(&uncle, acc)?;
-			acc = temp_acc;
-
-			T::EventHandler::note_uncle(
-				author.clone().unwrap_or_default(),
-				now - uncle.number().clone(),
-			);
-			uncles.push(UncleEntryItem::Uncle(hash, author));
 		}
 
-		<Self as Store>::Uncles::put(&uncles);
+		// check uncle validity.
+		T::FilterUncle::filter_uncle(&uncle, accumulator)
+	}
+}
+
+impl<T: Trait> ProvideInherent for Module<T> {
+	type Call = Call<T>;
+	type Error = MakeFatalError<()>;
+	const INHERENT_IDENTIFIER: InherentIdentifier = INHERENT_IDENTIFIER;
+
+	fn create_inherent(data: &InherentData) -> Option<Self::Call> {
+		let uncles = data.uncles().unwrap_or_default();
+		let mut set_uncles = Vec::new();
+
+		if !uncles.is_empty() {
+			let prev_uncles = <Self as Store>::Uncles::get();
+			let mut existing_hashes: Vec<_> = prev_uncles.into_iter().filter_map(|entry|
+				match entry {
+					UncleEntryItem::InclusionHeight(_) => None,
+					UncleEntryItem::Uncle(h, _) => Some(h),
+				}
+			).collect();
+
+			let mut acc: <T::FilterUncle as FilterUncle<_, _>>::Accumulator = Default::default();
+
+			for uncle in uncles {
+				match Self::verify_uncle(&uncle, &existing_hashes, &mut acc) {
+					Ok(_) => {
+						let hash = uncle.hash();
+						set_uncles.push(uncle);
+						existing_hashes.push(hash);
+					}
+					Err(_) => {
+						// skip this uncle
+					}
+				}
+			}
+		}
+
+		if set_uncles.is_empty() {
+			None
+		} else {
+			Some(Call::set_uncles(set_uncles))
+		}
+	}
+
+	fn check_inherent(_call: &Self::Call, _data: &InherentData) -> result::Result<(), Self::Error> {
 		Ok(())
 	}
 }
@@ -601,7 +716,7 @@ mod tests {
 		let author_a = 42;
 		let author_b = 43;
 
-		let mut acc: Option<<Filter as FilterUncle<Header, u64>>::Accumulator> = Some(Default::default());
+		let mut acc: <Filter as FilterUncle<Header, u64>>::Accumulator = Default::default();
 		let header_a1 = seal_header(
 			create_header(1, Default::default(), [1; 32].into()),
 			author_a,
@@ -621,13 +736,7 @@ mod tests {
 		);
 
 		let mut check_filter = move |uncle| {
-			match Filter::filter_uncle(uncle, acc.take().unwrap()) {
+			Filter::filter_uncle(uncle, &mut acc)
-				Ok((author, a)) => {
-					acc = Some(a);
-					Ok(author)
-				}
-				Err(e) => Err(e),
-			}
 		};
 
 		// same height, different author is OK.

substrate/srml/finality-tracker/src/lib.rs

@@ -244,15 +244,16 @@ impl<T: Trait> ProvideInherent for Module<T> {
 	const INHERENT_IDENTIFIER: InherentIdentifier = INHERENT_IDENTIFIER;
 
 	fn create_inherent(data: &InherentData) -> Option<Self::Call> {
-		let final_num =
+		if let Ok(final_num) = data.finalized_number() {
-			data.finalized_number().expect("Gets and decodes final number inherent data");
-
 			// make hint only when not same as last to avoid bloat.
 			Self::recent_hints().last().and_then(|last| if last == &final_num {
 				None
 			} else {
 				Some(Call::final_hint(final_num))
 			})
+		} else {
+			None
+		}
 	}
 
 	fn check_inherent(_call: &Self::Call, _data: &InherentData) -> result::Result<(), Self::Error> {