Allow for customisation of chain selection systems (#2240)

* move SelectChain trait out of client

* Extend SelectChain, move longest chain implementation into it

* Bring SelectChain into service

* implement LongestChain SelectChain

* implement longest chain for node

* update Cargo.lock's

* in between erroring tests

* deprecate ::backend and ::import_lock

* Remove unneded space

Co-Authored-By: gnunicorn <ben.kampmann@googlemail.com>

* Remove unneded space

Co-Authored-By: gnunicorn <ben.kampmann@googlemail.com>

* Fixes test compilation

* remove todo

* re-enable client test

* add doc

* fixing tests

* Clarify SelectChain Interface, intended implementation and usage

* minor components cleanups

* minor cleanups

* Update lock files

* Implement cleaner interface for SelectChain

* addressing comments

* Updating tests

* bump node runtime impl version

* address grumbles

substrate/core/finality-grandpa/src/environment.rs

@@ -44,6 +44,8 @@ use crate::{
 	PrimaryPropose, SignedMessage, NewAuthoritySet, VoterCommand,
 };
 
+use consensus_common::SelectChain;
+
 use crate::authorities::SharedAuthoritySet;
 use crate::consensus_changes::SharedConsensusChanges;
 use crate::justification::GrandpaJustification;
@@ -262,8 +264,9 @@ impl<Block: BlockT> SharedVoterSetState<Block> {
 }
 
 /// The environment we run GRANDPA in.
-pub(crate) struct Environment<B, E, Block: BlockT, N: Network<Block>, RA> {
+pub(crate) struct Environment<B, E, Block: BlockT, N: Network<Block>, RA, SC> {
 	pub(crate) inner: Arc<Client<B, E, Block, RA>>,
+	pub(crate) select_chain: SC,
 	pub(crate) voters: Arc<VoterSet<AuthorityId>>,
 	pub(crate) config: Config,
 	pub(crate) authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
@@ -273,7 +276,7 @@ pub(crate) struct Environment<B, E, Block: BlockT, N: Network<Block>, RA> {
 	pub(crate) voter_set_state: SharedVoterSetState<Block>,
 }
 
-impl<B, E, Block: BlockT, N: Network<Block>, RA> Environment<B, E, Block, N, RA> {
+impl<B, E, Block: BlockT, N: Network<Block>, RA, SC> Environment<B, E, Block, N, RA, SC> {
 	/// Updates the voter set state using the given closure. The write lock is
 	/// held during evaluation of the closure and the environment's voter set
 	/// state is set to its result if successful.
@@ -289,12 +292,16 @@ impl<B, E, Block: BlockT, N: Network<Block>, RA> Environment<B, E, Block, N, RA>
 	}
 }
 
-impl<Block: BlockT<Hash=H256>, B, E, N, RA> grandpa::Chain<Block::Hash, NumberFor<Block>> for Environment<B, E, Block, N, RA> where
+impl<Block: BlockT<Hash=H256>, B, E, N, RA, SC>
+	grandpa::Chain<Block::Hash, NumberFor<Block>>
+for Environment<B, E, Block, N, RA, SC>
+where
 	Block: 'static,
 	B: Backend<Block, Blake2Hasher> + 'static,
 	E: CallExecutor<Block, Blake2Hasher> + 'static,
 	N: Network<Block> + 'static,
 	N::In: 'static,
+	SC: SelectChain<Block> + 'static,
 	NumberFor<Block>: BlockNumberOps,
 {
 	fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result<Vec<Block::Hash>, GrandpaError> {
@@ -317,7 +324,7 @@ impl<Block: BlockT<Hash=H256>, B, E, N, RA> grandpa::Chain<Block::Hash, NumberFo
 		let limit = self.authority_set.current_limit();
 		debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit);
 
-		match self.inner.best_containing(block, None) {
+		match self.select_chain.finality_target(block, None) {
 			Ok(Some(mut best_hash)) => {
 				let base_header = self.inner.header(&BlockId::Hash(block)).ok()?
 					.expect("Header known to exist after `best_containing` call; qed");
@@ -376,6 +383,7 @@ impl<Block: BlockT<Hash=H256>, B, E, N, RA> grandpa::Chain<Block::Hash, NumberFo
 	}
 }
 
+
 pub(crate) fn ancestry<B, Block: BlockT<Hash=H256>, E, RA>(
 	client: &Client<B, E, Block, RA>,
 	base: Block::Hash,
@@ -411,13 +419,17 @@ pub(crate) fn ancestry<B, Block: BlockT<Hash=H256>, E, RA>(
 	Ok(tree_route.retracted().iter().skip(1).map(|e| e.hash).collect())
 }
 
-impl<B, E, Block: BlockT<Hash=H256>, N, RA> voter::Environment<Block::Hash, NumberFor<Block>> for Environment<B, E, Block, N, RA> where
+impl<B, E, Block: BlockT<Hash=H256>, N, RA, SC>
+	voter::Environment<Block::Hash, NumberFor<Block>>
+for Environment<B, E, Block, N, RA, SC>
+where
 	Block: 'static,
 	B: Backend<Block, Blake2Hasher> + 'static,
 	E: CallExecutor<Block, Blake2Hasher> + 'static + Send + Sync,
 	N: Network<Block> + 'static + Send,
 	N::In: 'static + Send,
 	RA: 'static + Send + Sync,
+	SC: SelectChain<Block> + 'static,
 	NumberFor<Block>: BlockNumberOps,
 {
 	type Timer = Box<dyn Future<Item = (), Error = Self::Error> + Send>;

substrate/core/finality-grandpa/src/import.rs

@@ -28,6 +28,7 @@ use client::runtime_api::ApiExt;
 use consensus_common::{
 	BlockImport, Error as ConsensusError, ErrorKind as ConsensusErrorKind,
 	ImportBlock, ImportResult, JustificationImport, well_known_cache_keys,
+	SelectChain,
 };
 use fg_primitives::GrandpaApi;
 use runtime_primitives::Justification;
@@ -53,16 +54,17 @@ use crate::justification::GrandpaJustification;
 ///
 /// When using GRANDPA, the block import worker should be using this block import
 /// object.
-pub struct GrandpaBlockImport<B, E, Block: BlockT<Hash=H256>, RA, PRA> {
+pub struct GrandpaBlockImport<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> {
 	inner: Arc<Client<B, E, Block, RA>>,
+	select_chain: SC,
 	authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
 	send_voter_commands: mpsc::UnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
 	consensus_changes: SharedConsensusChanges<Block::Hash, NumberFor<Block>>,
 	api: Arc<PRA>,
 }
 
-impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> JustificationImport<Block>
-	for GrandpaBlockImport<B, E, Block, RA, PRA> where
+impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> JustificationImport<Block>
+	for GrandpaBlockImport<B, E, Block, RA, PRA, SC> where
 		NumberFor<Block>: grandpa::BlockNumberOps,
 		B: Backend<Block, Blake2Hasher> + 'static,
 		E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
@@ -70,6 +72,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> JustificationImport<Block>
 		RA: Send + Sync,
 		PRA: ProvideRuntimeApi,
 		PRA::Api: GrandpaApi<Block>,
+		SC: SelectChain<Block>,
 {
 	type Error = ConsensusError;
 
@@ -86,7 +89,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> JustificationImport<Block>
 				pending_change.effective_number() > chain_info.finalized_number &&
 				pending_change.effective_number() <= chain_info.best_number
 			{
-				let effective_block_hash = self.inner.best_containing(
+				let effective_block_hash = self.select_chain.finality_target(
 					pending_change.canon_hash,
 					Some(pending_change.effective_number()),
 				);
@@ -155,7 +158,9 @@ impl<'a, Block: 'a + BlockT> Drop for PendingSetChanges<'a, Block> {
 	}
 }
 
-impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> GrandpaBlockImport<B, E, Block, RA, PRA> where
+impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC>
+	GrandpaBlockImport<B, E, Block, RA, PRA, SC>
+where
 	NumberFor<Block>: grandpa::BlockNumberOps,
 	B: Backend<Block, Blake2Hasher> + 'static,
 	E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
@@ -371,8 +376,8 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> GrandpaBlockImport<B, E, Block, RA
 	}
 }
 
-impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
-	for GrandpaBlockImport<B, E, Block, RA, PRA> where
+impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> BlockImport<Block>
+	for GrandpaBlockImport<B, E, Block, RA, PRA, SC> where
 		NumberFor<Block>: grandpa::BlockNumberOps,
 		B: Backend<Block, Blake2Hasher> + 'static,
 		E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
@@ -504,16 +509,20 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 	}
 }
 
-impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> GrandpaBlockImport<B, E, Block, RA, PRA> {
+impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> 
+	GrandpaBlockImport<B, E, Block, RA, PRA, SC>
+{
 	pub(crate) fn new(
 		inner: Arc<Client<B, E, Block, RA>>,
+		select_chain: SC,
 		authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
 		send_voter_commands: mpsc::UnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
 		consensus_changes: SharedConsensusChanges<Block::Hash, NumberFor<Block>>,
 		api: Arc<PRA>,
-	) -> GrandpaBlockImport<B, E, Block, RA, PRA> {
+	) -> GrandpaBlockImport<B, E, Block, RA, PRA, SC> {
 		GrandpaBlockImport {
 			inner,
+			select_chain,
 			authority_set,
 			send_voter_commands,
 			consensus_changes,
@@ -522,12 +531,13 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> GrandpaBlockImport<B, E, Block, RA
 	}
 }
 
-impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> GrandpaBlockImport<B, E, Block, RA, PRA>
-	where
-		NumberFor<Block>: grandpa::BlockNumberOps,
-		B: Backend<Block, Blake2Hasher> + 'static,
-		E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
-		RA: Send + Sync,
+impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC>
+	GrandpaBlockImport<B, E, Block, RA, PRA, SC>
+where
+	NumberFor<Block>: grandpa::BlockNumberOps,
+	B: Backend<Block, Blake2Hasher> + 'static,
+	E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
+	RA: Send + Sync,
 {
 
 	/// Import a block justification and finalize the block.

substrate/core/finality-grandpa/src/lib.rs

@@ -52,6 +52,8 @@
 //! or prune any signaled changes based on whether the signaling block is
 //! included in the newly-finalized chain.
 #![forbid(warnings)]
+#![allow(deprecated)] // FIXME #2532: remove once the refactor is done https://github.com/paritytech/substrate/issues/2532
+
 use futures::prelude::*;
 use log::{debug, info, warn};
 use futures::sync::mpsc;
@@ -67,6 +69,7 @@ use runtime_primitives::traits::{
 use fg_primitives::GrandpaApi;
 use inherents::InherentDataProviders;
 use runtime_primitives::generic::BlockId;
+use consensus_common::SelectChain;
 use substrate_primitives::{ed25519, H256, Pair, Blake2Hasher};
 use substrate_telemetry::{telemetry, CONSENSUS_INFO, CONSENSUS_DEBUG, CONSENSUS_WARN};
 use serde_json;
@@ -285,24 +288,30 @@ impl<H, N> fmt::Display for CommandOrError<H, N> {
 	}
 }
 
-pub struct LinkHalf<B, E, Block: BlockT<Hash=H256>, RA> {
+pub struct LinkHalf<B, E, Block: BlockT<Hash=H256>, RA, SC> {
 	client: Arc<Client<B, E, Block, RA>>,
+	select_chain: SC,
 	persistent_data: PersistentData<Block>,
 	voter_commands_rx: mpsc::UnboundedReceiver<VoterCommand<Block::Hash, NumberFor<Block>>>,
 }
 
 /// Make block importer and link half necessary to tie the background voter
 /// to it.
-pub fn block_import<B, E, Block: BlockT<Hash=H256>, RA, PRA>(
+pub fn block_import<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC>(
 	client: Arc<Client<B, E, Block, RA>>,
-	api: Arc<PRA>
-) -> Result<(GrandpaBlockImport<B, E, Block, RA, PRA>, LinkHalf<B, E, Block, RA>), ClientError>
-	where
-		B: Backend<Block, Blake2Hasher> + 'static,
-		E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
-		RA: Send + Sync,
-		PRA: ProvideRuntimeApi,
-		PRA::Api: GrandpaApi<Block>,
+	api: Arc<PRA>,
+	select_chain: SC
+) -> Result<(
+		GrandpaBlockImport<B, E, Block, RA, PRA, SC>,
+		LinkHalf<B, E, Block, RA, SC>
+	), ClientError>
+where
+	B: Backend<Block, Blake2Hasher> + 'static,
+	E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
+	RA: Send + Sync,
+	PRA: ProvideRuntimeApi,
+	PRA::Api: GrandpaApi<Block>,
+	SC: SelectChain<Block>,
 {
 	use runtime_primitives::traits::Zero;
 
@@ -328,6 +337,7 @@ pub fn block_import<B, E, Block: BlockT<Hash=H256>, RA, PRA>(
 	Ok((
 		GrandpaBlockImport::new(
 			client.clone(),
+			select_chain.clone(),
 			persistent_data.authority_set.clone(),
 			voter_commands_tx,
 			persistent_data.consensus_changes.clone(),
@@ -335,6 +345,7 @@ pub fn block_import<B, E, Block: BlockT<Hash=H256>, RA, PRA>(
 		),
 		LinkHalf {
 			client,
+			select_chain,
 			persistent_data,
 			voter_commands_rx,
 		},
@@ -435,11 +446,11 @@ fn register_finality_tracker_inherent_data_provider<B, E, Block: BlockT<Hash=H25
 }
 
 /// Parameters used to run Grandpa.
-pub struct GrandpaParams<'a, B, E, Block: BlockT<Hash=H256>, N, RA, X> {
+pub struct GrandpaParams<'a, B, E, Block: BlockT<Hash=H256>, N, RA, SC, X> {
 	/// Configuration for the GRANDPA service.
 	pub config: Config,
 	/// A link to the block import worker.
-	pub link: LinkHalf<B, E, Block, RA>,
+	pub link: LinkHalf<B, E, Block, RA, SC>,
 	/// The Network instance.
 	pub network: N,
 	/// The inherent data providers.
@@ -452,14 +463,15 @@ pub struct GrandpaParams<'a, B, E, Block: BlockT<Hash=H256>, N, RA, X> {
 
 /// Run a GRANDPA voter as a task. Provide configuration and a link to a
 /// block import worker that has already been instantiated with `block_import`.
-pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
-	grandpa_params: GrandpaParams<B, E, Block, N, RA, X>,
+pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, SC, X>(
+	grandpa_params: GrandpaParams<B, E, Block, N, RA, SC, X>,
 ) -> ::client::error::Result<impl Future<Item=(),Error=()> + Send + 'static> where
 	Block::Hash: Ord,
 	B: Backend<Block, Blake2Hasher> + 'static,
 	E: CallExecutor<Block, Blake2Hasher> + Send + Sync + 'static,
 	N: Network<Block> + Send + Sync + 'static,
 	N::In: Send + 'static,
+	SC: SelectChain<Block> + 'static,
 	NumberFor<Block>: BlockNumberOps,
 	DigestFor<Block>: Encode,
 	RA: Send + Sync + 'static,
@@ -480,6 +492,7 @@ pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
 
 	let LinkHalf {
 		client,
+		select_chain,
 		persistent_data,
 		voter_commands_rx,
 	} = link;
@@ -513,6 +526,7 @@ pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
 	let initial_environment = Arc::new(Environment {
 		inner: client.clone(),
 		config: config.clone(),
+		select_chain: select_chain.clone(),
 		voters: Arc::new(voters),
 		network: network.clone(),
 		set_id: authority_set.set_id(),
@@ -599,6 +613,7 @@ pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
 		let client = client.clone();
 		let config = config.clone();
 		let network = network.clone();
+		let select_chain = select_chain.clone();
 		let authority_set = authority_set.clone();
 		let consensus_changes = consensus_changes.clone();
 
@@ -636,6 +651,7 @@ pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
 
 					let env = Arc::new(Environment {
 						inner: client,
+						select_chain,
 						config,
 						voters: Arc::new(new.authorities.into_iter().collect()),
 						set_id: new.set_id,
@@ -704,14 +720,15 @@ pub fn run_grandpa_voter<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
 }
 
 #[deprecated(since = "1.1", note = "Please switch to run_grandpa_voter.")]
-pub fn run_grandpa<B, E, Block: BlockT<Hash=H256>, N, RA, X>(
-	grandpa_params: GrandpaParams<B, E, Block, N, RA, X>,
+pub fn run_grandpa<B, E, Block: BlockT<Hash=H256>, N, RA, SC, X>(
+	grandpa_params: GrandpaParams<B, E, Block, N, RA, SC, X>,
 ) -> ::client::error::Result<impl Future<Item=(),Error=()> + Send + 'static> where
 	Block::Hash: Ord,
 	B: Backend<Block, Blake2Hasher> + 'static,
 	E: CallExecutor<Block, Blake2Hasher> + Send + Sync + 'static,
 	N: Network<Block> + Send + Sync + 'static,
 	N::In: Send + 'static,
+	SC: SelectChain<Block> + 'static,
 	NumberFor<Block>: BlockNumberOps,
 	DigestFor<Block>: Encode,
 	RA: Send + Sync + 'static,

substrate/core/finality-grandpa/src/observer.rs

@@ -24,6 +24,7 @@ use grandpa::{
 };
 use log::{debug, info, warn};
 
+use consensus_common::SelectChain;
 use client::{CallExecutor, Client, backend::Backend};
 use runtime_primitives::traits::{NumberFor, Block as BlockT};
 use substrate_primitives::{ed25519::Public as AuthorityId, H256, Blake2Hasher};
@@ -143,9 +144,9 @@ fn grandpa_observer<B, E, Block: BlockT<Hash=H256>, RA, S>(
 /// listening for and validating GRANDPA commits instead of following the full
 /// protocol. Provide configuration and a link to a block import worker that has
 /// already been instantiated with `block_import`.
-pub fn run_grandpa_observer<B, E, Block: BlockT<Hash=H256>, N, RA>(
+pub fn run_grandpa_observer<B, E, Block: BlockT<Hash=H256>, N, RA, SC>(
 	config: Config,
-	link: LinkHalf<B, E, Block, RA>,
+	link: LinkHalf<B, E, Block, RA, SC>,
 	network: N,
 	on_exit: impl Future<Item=(),Error=()> + Clone + Send + 'static,
 ) -> ::client::error::Result<impl Future<Item=(),Error=()> + Send + 'static> where
@@ -153,11 +154,13 @@ pub fn run_grandpa_observer<B, E, Block: BlockT<Hash=H256>, N, RA>(
 	E: CallExecutor<Block, Blake2Hasher> + Send + Sync + 'static,
 	N: Network<Block> + Send + Sync + 'static,
 	N::In: Send + 'static,
+	SC: SelectChain<Block> + 'static,
 	NumberFor<Block>: BlockNumberOps,
 	RA: Send + Sync + 'static,
 {
 	let LinkHalf {
 		client,
+		select_chain: _,
 		persistent_data,
 		voter_commands_rx,
 	} = link;

substrate/core/finality-grandpa/src/service_integration.rs

@@ -30,11 +30,13 @@ pub type BlockImportForService<F> = crate::GrandpaBlockImport<
         <F as ServiceFactory>::Block,
         <F as ServiceFactory>::RuntimeApi
     >,
+	<F as ServiceFactory>::SelectChain
 >;
 
 pub type LinkHalfForService<F> = crate::LinkHalf<
 	FullBackend<F>,
 	FullExecutor<F>,
 	<F as ServiceFactory>::Block,
-	<F as ServiceFactory>::RuntimeApi
+	<F as ServiceFactory>::RuntimeApi,
+	<F as ServiceFactory>::SelectChain
 >;

substrate/core/finality-grandpa/src/tests.rs

@@ -28,6 +28,7 @@ use client::{
 	BlockchainEvents, error::Result,
 	blockchain::Backend as BlockchainBackend,
 	runtime_api::{Core, RuntimeVersion, ApiExt},
+	LongestChain,
 };
 use test_client::{self, runtime::BlockNumber};
 use consensus_common::{BlockOrigin, ForkChoiceStrategy, ImportedAux, ImportBlock, ImportResult};
@@ -50,6 +51,7 @@ type PeerData =
 				test_client::Executor,
 				Block,
 				test_client::runtime::RuntimeApi,
+				LongestChain<test_client::Backend, Block>
 			>
 		>
 	>;
@@ -106,9 +108,15 @@ impl TestNetFactory for GrandpaTestNet {
 	fn make_block_import(&self, client: Arc<PeersClient>)
 		-> (SharedBlockImport<Block>, Option<SharedJustificationImport<Block>>, PeerData)
 	{
+		
+		let select_chain = LongestChain::new(
+			client.backend().clone(),
+			client.import_lock().clone()
+		);
 		let (import, link) = block_import(
 			client,
-			Arc::new(self.test_config.clone())
+			Arc::new(self.test_config.clone()),
+			select_chain,
 		).expect("Could not create block import for fresh peer.");
 		let shared_import = Arc::new(import);
 		(shared_import.clone(), Some(shared_import), Mutex::new(Some(link)))
@@ -679,6 +687,7 @@ fn transition_3_voters_twice_1_full_observer() {
 				link,
 			)
 		};
+
 		finality_notifications.push(
 			client.finality_notification_stream()
 				.take_while(|n| Ok(n.header.number() < &30))