grandpa: make the VotingRule API async (#8101)

* grandpa: make the VotingRule api async

* grandpa: add docs to VotingRuleResult

* grandpa: formatting

* grandpa: use async blocks

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* grandpa: expose VotingRuleResult

* grandpa: revert some broken changes to async syntax

* grandpa: use finality-grandpa v0.14.0

* grandpa: bump impl_version

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

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

@@ -592,100 +592,6 @@ where
 	fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result<Vec<Block::Hash>, GrandpaError> {
 		ancestry(&self.client, base, block)
 	}
-
-	fn best_chain_containing(&self, block: Block::Hash) -> Option<(Block::Hash, NumberFor<Block>)> {
-		// NOTE: when we finalize an authority set change through the sync protocol the voter is
-		//       signaled asynchronously. therefore the voter could still vote in the next round
-		//       before activating the new set. the `authority_set` is updated immediately thus we
-		//       restrict the voter based on that.
-		if self.set_id != self.authority_set.set_id() {
-			return None;
-		}
-
-		let base_header = match self.client.header(BlockId::Hash(block)).ok()? {
-			Some(h) => h,
-			None => {
-				debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find base block", block);
-				return None;
-			}
-		};
-
-		// we refuse to vote beyond the current limit number where transitions are scheduled to
-		// occur.
-		// once blocks are finalized that make that transition irrelevant or activate it,
-		// we will proceed onwards. most of the time there will be no pending transition.
-		// the limit, if any, is guaranteed to be higher than or equal to the given base number.
-		let limit = self.authority_set.current_limit(*base_header.number());
-		debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit);
-
-		match self.select_chain.finality_target(block, None) {
-			Ok(Some(best_hash)) => {
-				let best_header = self.client.header(BlockId::Hash(best_hash)).ok()?
-					.expect("Header known to exist after `finality_target` call; qed");
-
-				// check if our vote is currently being limited due to a pending change
-				let limit = limit.filter(|limit| limit < best_header.number());
-				let target;
-
-				let target_header = if let Some(target_number) = limit {
-					let mut target_header = best_header.clone();
-
-					// walk backwards until we find the target block
-					loop {
-						if *target_header.number() < target_number {
-							unreachable!(
-								"we are traversing backwards from a known block; \
-								 blocks are stored contiguously; \
-								 qed"
-							);
-						}
-
-						if *target_header.number() == target_number {
-							break;
-						}
-
-						target_header = self.client.header(BlockId::Hash(*target_header.parent_hash())).ok()?
-							.expect("Header known to exist after `finality_target` call; qed");
-					}
-
-					target = target_header;
-					&target
-				} else {
-					// otherwise just use the given best as the target
-					&best_header
-				};
-
-				// restrict vote according to the given voting rule, if the
-				// voting rule doesn't restrict the vote then we keep the
-				// previous target.
-				//
-				// note that we pass the original `best_header`, i.e. before the
-				// authority set limit filter, which can be considered a
-				// mandatory/implicit voting rule.
-				//
-				// we also make sure that the restricted vote is higher than the
-				// round base (i.e. last finalized), otherwise the value
-				// returned by the given voting rule is ignored and the original
-				// target is used instead.
-				self.voting_rule
-					.restrict_vote(&*self.client, &base_header, &best_header, target_header)
-					.filter(|(_, restricted_number)| {
-						// we can only restrict votes within the interval [base, target]
-						restricted_number >= base_header.number() &&
-							restricted_number < target_header.number()
-					})
-					.or_else(|| Some((target_header.hash(), *target_header.number())))
-			},
-			Ok(None) => {
-				debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block);
-				None
-			}
-			Err(e) => {
-				debug!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e);
-				None
-			}
-		}
-	}
 }
 
 
@@ -733,6 +639,14 @@ where
 	NumberFor<Block>: BlockNumberOps,
 {
 	type Timer = Pin<Box<dyn Future<Output = Result<(), Self::Error>> + Send + Sync>>;
+	type BestChain = Pin<
+		Box<
+			dyn Future<Output = Result<Option<(Block::Hash, NumberFor<Block>)>, Self::Error>>
+				+ Send
+				+ Sync
+		>,
+	>;
+
 	type Id = AuthorityId;
 	type Signature = AuthoritySignature;
 
@@ -747,6 +661,119 @@ where
 
 	type Error = CommandOrError<Block::Hash, NumberFor<Block>>;
 
+	fn best_chain_containing(&self, block: Block::Hash) -> Self::BestChain {
+		let find_best_chain = || {
+			// NOTE: when we finalize an authority set change through the sync protocol the voter is
+			//       signaled asynchronously. therefore the voter could still vote in the next round
+			//       before activating the new set. the `authority_set` is updated immediately thus we
+			//       restrict the voter based on that.
+			if self.set_id != self.authority_set.set_id() {
+				return None;
+			}
+
+			let base_header = match self.client.header(BlockId::Hash(block)).ok()? {
+				Some(h) => h,
+				None => {
+					debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find base block", block);
+					return None;
+				}
+			};
+
+			// we refuse to vote beyond the current limit number where transitions are scheduled to
+			// occur.
+			// once blocks are finalized that make that transition irrelevant or activate it,
+			// we will proceed onwards. most of the time there will be no pending transition.
+			// the limit, if any, is guaranteed to be higher than or equal to the given base number.
+			let limit = self.authority_set.current_limit(*base_header.number());
+			debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit);
+
+			match self.select_chain.finality_target(block, None) {
+				Ok(Some(best_hash)) => {
+					let best_header = self
+						.client
+						.header(BlockId::Hash(best_hash))
+						.ok()?
+						.expect("Header known to exist after `finality_target` call; qed");
+
+					// check if our vote is currently being limited due to a pending change
+					let limit = limit.filter(|limit| limit < best_header.number());
+
+					if let Some(target_number) = limit {
+						let mut target_header = best_header.clone();
+
+						// walk backwards until we find the target block
+						loop {
+							if *target_header.number() < target_number {
+								unreachable!(
+									"we are traversing backwards from a known block; \
+									 blocks are stored contiguously; \
+									 qed"
+								);
+							}
+
+							if *target_header.number() == target_number {
+								break;
+							}
+
+							target_header = self
+								.client
+								.header(BlockId::Hash(*target_header.parent_hash()))
+								.ok()?
+								.expect("Header known to exist after `finality_target` call; qed");
+						}
+
+						Some((base_header, best_header, target_header))
+					} else {
+						// otherwise just use the given best as the target
+						Some((base_header, best_header.clone(), best_header))
+					}
+				}
+				Ok(None) => {
+					debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block);
+					None
+				}
+				Err(e) => {
+					debug!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e);
+					None
+				}
+			}
+		};
+
+		if let Some((base_header, best_header, target_header)) = find_best_chain() {
+			// restrict vote according to the given voting rule, if the
+			// voting rule doesn't restrict the vote then we keep the
+			// previous target.
+			//
+			// note that we pass the original `best_header`, i.e. before the
+			// authority set limit filter, which can be considered a
+			// mandatory/implicit voting rule.
+			//
+			// we also make sure that the restricted vote is higher than the
+			// round base (i.e. last finalized), otherwise the value
+			// returned by the given voting rule is ignored and the original
+			// target is used instead.
+			let rule_fut = self.voting_rule.restrict_vote(
+				self.client.clone(),
+				&base_header,
+				&best_header,
+				&target_header,
+			);
+
+			Box::pin(async move {
+				Ok(rule_fut
+					.await
+					.filter(|(_, restricted_number)| {
+						// we can only restrict votes within the interval [base, target]
+						restricted_number >= base_header.number()
+							&& restricted_number < target_header.number()
+					})
+					.or_else(|| Some((target_header.hash(), *target_header.number()))))
+			})
+		} else {
+			Box::pin(future::ok(None))
+		}
+	}
+
 	fn round_data(
 		&self,
 		round: RoundNumber,

substrate/client/finality-grandpa/src/justification.rs

@@ -217,8 +217,4 @@ impl<Block: BlockT> finality_grandpa::Chain<Block::Hash, NumberFor<Block>> for A
 
 		Ok(route)
 	}
-
-	fn best_chain_containing(&self, _block: Block::Hash) -> Option<(Block::Hash, NumberFor<Block>)> {
-		None
-	}
 }

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

@@ -127,7 +127,8 @@ pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream};
 pub use import::GrandpaBlockImport;
 pub use justification::GrandpaJustification;
 pub use voting_rule::{
-	BeforeBestBlockBy, ThreeQuartersOfTheUnfinalizedChain, VotingRule, VotingRulesBuilder
+	BeforeBestBlockBy, ThreeQuartersOfTheUnfinalizedChain, VotingRule, VotingRuleResult,
+	VotingRulesBuilder,
 };
 pub use finality_grandpa::voter::report;
 pub use finality_proof::{prove_warp_sync, WarpSyncFragmentCache};

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

@@ -57,11 +57,6 @@ impl<'a, Block, Client> finality_grandpa::Chain<Block::Hash, NumberFor<Block>>
 	fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result<Vec<Block::Hash>, GrandpaError> {
 		environment::ancestry(&self.client, base, block)
 	}
-
-	fn best_chain_containing(&self, _block: Block::Hash) -> Option<(Block::Hash, NumberFor<Block>)> {
-		// only used by voter
-		None
-	}
 }
 
 fn grandpa_observer<BE, Block: BlockT, Client, S, F>(

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

@@ -1355,7 +1355,7 @@ where
 
 #[test]
 fn grandpa_environment_respects_voting_rules() {
-	use finality_grandpa::Chain;
+	use finality_grandpa::voter::Environment;
 
 	let peers = &[Ed25519Keyring::Alice];
 	let voters = make_ids(peers);
@@ -1390,25 +1390,25 @@ fn grandpa_environment_respects_voting_rules() {
 
 	// the unrestricted environment should just return the best block
 	assert_eq!(
-		unrestricted_env.best_chain_containing(
+		futures::executor::block_on(unrestricted_env.best_chain_containing(
 			peer.client().info().finalized_hash
-		).unwrap().1,
+		)).unwrap().unwrap().1,
 		21,
 	);
 
 	// both the other environments should return block 16, which is 3/4 of the
 	// way in the unfinalized chain
 	assert_eq!(
-		three_quarters_env.best_chain_containing(
+		futures::executor::block_on(three_quarters_env.best_chain_containing(
 			peer.client().info().finalized_hash
-		).unwrap().1,
+		)).unwrap().unwrap().1,
 		16,
 	);
 
 	assert_eq!(
-		default_env.best_chain_containing(
+		futures::executor::block_on(default_env.best_chain_containing(
 			peer.client().info().finalized_hash
-		).unwrap().1,
+		)).unwrap().unwrap().1,
 		16,
 	);
 
@@ -1417,18 +1417,18 @@ fn grandpa_environment_respects_voting_rules() {
 
 	// the 3/4 environment should propose block 21 for voting
 	assert_eq!(
-		three_quarters_env.best_chain_containing(
+		futures::executor::block_on(three_quarters_env.best_chain_containing(
 			peer.client().info().finalized_hash
-		).unwrap().1,
+		)).unwrap().unwrap().1,
 		21,
 	);
 
 	// while the default environment will always still make sure we don't vote
 	// on the best block (2 behind)
 	assert_eq!(
-		default_env.best_chain_containing(
+		futures::executor::block_on(default_env.best_chain_containing(
 			peer.client().info().finalized_hash
-		).unwrap().1,
+		)).unwrap().unwrap().1,
 		19,
 	);
 
@@ -1439,9 +1439,9 @@ fn grandpa_environment_respects_voting_rules() {
 	// best block, there's a hard rule that we can't cast any votes lower than
 	// the given base (#21).
 	assert_eq!(
-		default_env.best_chain_containing(
+		futures::executor::block_on(default_env.best_chain_containing(
 			peer.client().info().finalized_hash
-		).unwrap().1,
+		)).unwrap().unwrap().1,
 		21,
 	);
 }

substrate/client/finality-grandpa/src/voting_rule.rs

@@ -22,14 +22,22 @@
 //! restrictions that are taken into account by the GRANDPA environment when
 //! selecting a finality target to vote on.
 
+use std::future::Future;
 use std::sync::Arc;
+use std::pin::Pin;
+
+use dyn_clone::DynClone;
 
 use sc_client_api::blockchain::HeaderBackend;
 use sp_runtime::generic::BlockId;
 use sp_runtime::traits::{Block as BlockT, Header, NumberFor, One, Zero};
 
+/// A future returned by a `VotingRule` to restrict a given vote, if any restriction is necessary.
+pub type VotingRuleResult<Block> =
+	Pin<Box<dyn Future<Output = Option<(<Block as BlockT>::Hash, NumberFor<Block>)>> + Send + Sync>>;
+
 /// A trait for custom voting rules in GRANDPA.
-pub trait VotingRule<Block, B>: Send + Sync where
+pub trait VotingRule<Block, B>: DynClone + Send + Sync where
 	Block: BlockT,
 	B: HeaderBackend<Block>,
 {
@@ -47,11 +55,11 @@ pub trait VotingRule<Block, B>: Send + Sync where
 	/// execution of voting rules wherein `current_target <= best_target`.
 	fn restrict_vote(
 		&self,
-		backend: &B,
+		backend: Arc<B>,
 		base: &Block::Header,
 		best_target: &Block::Header,
 		current_target: &Block::Header,
-	) -> Option<(Block::Hash, NumberFor<Block>)>;
+	) -> VotingRuleResult<Block>;
 }
 
 impl<Block, B> VotingRule<Block, B> for () where
@@ -60,12 +68,12 @@ impl<Block, B> VotingRule<Block, B> for () where
 {
 	fn restrict_vote(
 		&self,
-		_backend: &B,
+		_backend: Arc<B>,
 		_base: &Block::Header,
 		_best_target: &Block::Header,
 		_current_target: &Block::Header,
-	) -> Option<(Block::Hash, NumberFor<Block>)> {
-		None
+	) -> VotingRuleResult<Block> {
+		Box::pin(async { None })
 	}
 }
 
@@ -80,15 +88,15 @@ impl<Block, B> VotingRule<Block, B> for BeforeBestBlockBy<NumberFor<Block>> wher
 {
 	fn restrict_vote(
 		&self,
-		backend: &B,
+		backend: Arc<B>,
 		_base: &Block::Header,
 		best_target: &Block::Header,
 		current_target: &Block::Header,
-	) -> Option<(Block::Hash, NumberFor<Block>)> {
+	) -> VotingRuleResult<Block> {
 		use sp_arithmetic::traits::Saturating;
 
 		if current_target.number().is_zero() {
-			return None;
+			return Box::pin(async { None });
 		}
 
 		// find the target number restricted by this rule
@@ -96,21 +104,24 @@ impl<Block, B> VotingRule<Block, B> for BeforeBestBlockBy<NumberFor<Block>> wher
 
 		// our current target is already lower than this rule would restrict
 		if target_number >= *current_target.number() {
-			return None;
+			return Box::pin(async { None });
 		}
 
+		let current_target = current_target.clone();
+
 		// find the block at the given target height
-		find_target(
-			backend,
-			target_number,
-			current_target,
-		)
+		Box::pin(std::future::ready(find_target(
+			&*backend,
+			target_number.clone(),
+			&current_target,
+		)))
 	}
 }
 
 /// A custom voting rule that limits votes towards 3/4 of the unfinalized chain,
 /// using the given `base` and `best_target` to figure where the 3/4 target
 /// should fall.
+#[derive(Clone)]
 pub struct ThreeQuartersOfTheUnfinalizedChain;
 
 impl<Block, B> VotingRule<Block, B> for ThreeQuartersOfTheUnfinalizedChain where
@@ -119,11 +130,11 @@ impl<Block, B> VotingRule<Block, B> for ThreeQuartersOfTheUnfinalizedChain where
 {
 	fn restrict_vote(
 		&self,
-		backend: &B,
+		backend: Arc<B>,
 		base: &Block::Header,
 		best_target: &Block::Header,
 		current_target: &Block::Header,
-	) -> Option<(Block::Hash, NumberFor<Block>)> {
+	) -> VotingRuleResult<Block> {
 		// target a vote towards 3/4 of the unfinalized chain (rounding up)
 		let target_number = {
 			let two = NumberFor::<Block>::one() + One::one();
@@ -138,15 +149,15 @@ impl<Block, B> VotingRule<Block, B> for ThreeQuartersOfTheUnfinalizedChain where
 
 		// our current target is already lower than this rule would restrict
 		if target_number >= *current_target.number() {
-			return None;
+			return Box::pin(async { None });
 		}
 
 		// find the block at the given target height
-		find_target(
-			backend,
+		Box::pin(std::future::ready(find_target(
+			&*backend,
 			target_number,
 			current_target,
-		)
+		)))
 	}
 }
 
@@ -195,37 +206,42 @@ impl<B, Block> Clone for VotingRules<B, Block> {
 
 impl<Block, B> VotingRule<Block, B> for VotingRules<Block, B> where
 	Block: BlockT,
-	B: HeaderBackend<Block>,
+	B: HeaderBackend<Block> + 'static,
 {
 	fn restrict_vote(
 		&self,
-		backend: &B,
+		backend: Arc<B>,
 		base: &Block::Header,
 		best_target: &Block::Header,
 		current_target: &Block::Header,
-	) -> Option<(Block::Hash, NumberFor<Block>)> {
-		let restricted_target = self.rules.iter().fold(
-			current_target.clone(),
-			|current_target, rule| {
-				rule.restrict_vote(
-					backend,
-					base,
-					best_target,
-					&current_target,
-				)
+	) -> VotingRuleResult<Block> {
+		let rules = self.rules.clone();
+		let base = base.clone();
+		let best_target = best_target.clone();
+		let current_target = current_target.clone();
+
+		Box::pin(async move {
+			let mut restricted_target = current_target.clone();
+
+			for rule in rules.iter() {
+				if let Some(header) = rule
+					.restrict_vote(backend.clone(), &base, &best_target, &restricted_target)
+					.await
 					.and_then(|(hash, _)| backend.header(BlockId::Hash(hash)).ok())
 					.and_then(std::convert::identity)
-					.unwrap_or(current_target)
-			},
-		);
+				{
+					restricted_target = header;
+				}
+			}
 
-		let restricted_hash = restricted_target.hash();
+			let restricted_hash = restricted_target.hash();
 
-		if restricted_hash != current_target.hash() {
-			Some((restricted_hash, *restricted_target.number()))
-		} else {
-			None
-		}
+			if restricted_hash != current_target.hash() {
+				Some((restricted_hash, *restricted_target.number()))
+			} else {
+				None
+			}
+		})
 	}
 }
 
@@ -237,7 +253,7 @@ pub struct VotingRulesBuilder<Block, B> {
 
 impl<Block, B> Default for VotingRulesBuilder<Block, B> where
 	Block: BlockT,
-	B: HeaderBackend<Block>,
+	B: HeaderBackend<Block> + 'static,
 {
 	fn default() -> Self {
 		VotingRulesBuilder::new()
@@ -248,7 +264,7 @@ impl<Block, B> Default for VotingRulesBuilder<Block, B> where
 
 impl<Block, B> VotingRulesBuilder<Block, B> where
 	Block: BlockT,
-	B: HeaderBackend<Block>,
+	B: HeaderBackend<Block> + 'static,
 {
 	/// Return a new voting rule builder using the given backend.
 	pub fn new() -> Self {
@@ -285,14 +301,15 @@ impl<Block, B> VotingRulesBuilder<Block, B> where
 impl<Block, B> VotingRule<Block, B> for Box<dyn VotingRule<Block, B>> where
 	Block: BlockT,
 	B: HeaderBackend<Block>,
+	Self: Clone,
 {
 	fn restrict_vote(
 		&self,
-		backend: &B,
+		backend: Arc<B>,
 		base: &Block::Header,
 		best_target: &Block::Header,
 		current_target: &Block::Header,
-	) -> Option<(Block::Hash, NumberFor<Block>)> {
+	) -> VotingRuleResult<Block> {
 		(**self).restrict_vote(backend, base, best_target, current_target)
 	}
 }