grandpa: pass the actual best block to voting rules

client/finality-grandpa/src/environment.rs

@@ -1174,7 +1174,7 @@ where
 	let base_header = match client.header(block)? {
 		Some(h) => h,
 		None => {
-			debug!(
+			warn!(
 				target: LOG_TARGET,
 				"Encountered error finding best chain containing {:?}: couldn't find base block",
 				block,
@@ -1194,75 +1194,82 @@ where
 		"Finding best chain containing block {:?} with number limit {:?}", block, limit
 	);
 
-	let result = match select_chain.finality_target(block, None).await {
-		Ok(best_hash) => {
-			let best_header = client
-				.header(best_hash)?
-				.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 (base_header, best_header, 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 = client
-						.header(*target_header.parent_hash())?
-						.expect("Header known to exist after `finality_target` call; qed");
-				}
-
-				(base_header, best_header, target_header)
-			} else {
-				// otherwise just use the given best as the target
-				(base_header, best_header.clone(), best_header)
-			};
+	let mut target_header = match select_chain.finality_target(block, None).await {
+		Ok(target_hash) => client
+			.header(target_hash)?
+			.expect("Header known to exist after `finality_target` call; qed"),
+		Err(err) => {
+			warn!(
+				target: LOG_TARGET,
+				"Encountered error finding best chain containing {:?}: couldn't find target block: {}",
+				block,
+				err,
+			);
 
-			// 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.
-			voting_rule
-				.restrict_vote(client.clone(), &base_header, &best_header, &target_header)
-				.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())))
+			return Ok(None)
 		},
-		Err(e) => {
+	};
+
+	// NOTE: this is purposefully done after `finality_target` to prevent a case
+	// where in-between these two requests there is a block import and
+	// `finality_target` returns something higher than `best_chain`.
+	let best_header = match select_chain.best_chain().await {
+		Ok(best_header) => best_header,
+		Err(err) => {
 			warn!(
 				target: LOG_TARGET,
-				"Encountered error finding best chain containing {:?}: {}", block, e
+				"Encountered error finding best chain containing {:?}: couldn't find best block: {}",
+				block,
+				err,
 			);
-			None
+
+			return Ok(None)
 		},
 	};
 
-	Ok(result)
+	if target_header.number() > best_header.number() {
+		return Err(Error::Safety(
+			"SelectChain returned a finality target higher than its best block".into(),
+		))
+	}
+
+	// check if our vote is currently being limited due to a pending change,
+	// in which case we will restrict our target header to the given limit
+	if let Some(target_number) = limit.filter(|limit| limit < target_header.number()) {
+		// 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 = client
+				.header(*target_header.parent_hash())?
+				.expect("Header known to exist after `finality_target` call; qed");
+		}
+	}
+
+	// restrict vote according to the given voting rule, if the voting rule
+	// doesn't restrict the vote then we keep the previous target.
+	//
+	// 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.
+	Ok(voting_rule
+		.restrict_vote(client.clone(), &base_header, &best_header, &target_header)
+		.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()))))
 }
 
 /// Finalize the given block and apply any authority set changes. If an

client/finality-grandpa/src/tests.rs

@@ -20,6 +20,7 @@
 
 use super::*;
 use assert_matches::assert_matches;
+use async_trait::async_trait;
 use environment::HasVoted;
 use futures_timer::Delay;
 use parking_lot::{Mutex, RwLock};
@@ -33,8 +34,7 @@ use sc_network_test::{
 	PeersFullClient, TestClient, TestNetFactory,
 };
 use sp_api::{ApiRef, ProvideRuntimeApi};
-use sp_blockchain::Result;
-use sp_consensus::BlockOrigin;
+use sp_consensus::{BlockOrigin, Error as ConsensusError, SelectChain};
 use sp_core::H256;
 use sp_finality_grandpa::{
 	AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof, GRANDPA_ENGINE_ID,
@@ -200,11 +200,50 @@ sp_api::mock_impl_runtime_apis! {
 }
 
 impl GenesisAuthoritySetProvider<Block> for TestApi {
-	fn get(&self) -> Result<AuthorityList> {
+	fn get(&self) -> sp_blockchain::Result<AuthorityList> {
 		Ok(self.genesis_authorities.clone())
 	}
 }
 
+/// A mock `SelectChain` that allows the user to set the return values for each
+/// method. After the `SelectChain` methods are called the pending value is
+/// discarded and another call to set new values must be performed.
+#[derive(Clone, Default)]
+struct MockSelectChain {
+	leaves: Arc<Mutex<Option<Vec<Hash>>>>,
+	best_chain: Arc<Mutex<Option<<Block as BlockT>::Header>>>,
+	finality_target: Arc<Mutex<Option<Hash>>>,
+}
+
+impl MockSelectChain {
+	fn set_best_chain(&self, best: <Block as BlockT>::Header) {
+		*self.best_chain.lock() = Some(best);
+	}
+
+	fn set_finality_target(&self, target: Hash) {
+		*self.finality_target.lock() = Some(target);
+	}
+}
+
+#[async_trait]
+impl SelectChain<Block> for MockSelectChain {
+	async fn leaves(&self) -> Result<Vec<Hash>, ConsensusError> {
+		Ok(self.leaves.lock().take().unwrap())
+	}
+
+	async fn best_chain(&self) -> Result<<Block as BlockT>::Header, ConsensusError> {
+		Ok(self.best_chain.lock().take().unwrap())
+	}
+
+	async fn finality_target(
+		&self,
+		_target_hash: Hash,
+		_maybe_max_number: Option<NumberFor<Block>>,
+	) -> Result<Hash, ConsensusError> {
+		Ok(self.finality_target.lock().take().unwrap())
+	}
+}
+
 const TEST_GOSSIP_DURATION: Duration = Duration::from_millis(500);
 
 fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList {
@@ -1291,21 +1330,16 @@ async fn voter_catches_up_to_latest_round_when_behind() {
 	future::select(test, drive_to_completion).await;
 }
 
-type TestEnvironment<N, VR> = Environment<
-	substrate_test_runtime_client::Backend,
-	Block,
-	TestClient,
-	N,
-	LongestChain<substrate_test_runtime_client::Backend, Block>,
-	VR,
->;
+type TestEnvironment<N, SC, VR> =
+	Environment<substrate_test_runtime_client::Backend, Block, TestClient, N, SC, VR>;
 
-fn test_environment<N, VR>(
+fn test_environment_with_select_chain<N, VR, SC>(
 	link: &TestLinkHalf,
 	keystore: Option<SyncCryptoStorePtr>,
 	network_service: N,
+	select_chain: SC,
 	voting_rule: VR,
-) -> TestEnvironment<N, VR>
+) -> TestEnvironment<N, SC, VR>
 where
 	N: NetworkT<Block>,
 	VR: VotingRule<Block, TestClient>,
@@ -1330,7 +1364,7 @@ where
 		authority_set: authority_set.clone(),
 		config: config.clone(),
 		client: link.client.clone(),
-		select_chain: link.select_chain.clone(),
+		select_chain,
 		set_id: authority_set.set_id(),
 		voter_set_state: set_state.clone(),
 		voters: Arc::new(authority_set.current_authorities()),
@@ -1343,6 +1377,25 @@ where
 	}
 }
 
+fn test_environment<N, VR>(
+	link: &TestLinkHalf,
+	keystore: Option<SyncCryptoStorePtr>,
+	network_service: N,
+	voting_rule: VR,
+) -> TestEnvironment<N, LongestChain<substrate_test_runtime_client::Backend, Block>, VR>
+where
+	N: NetworkT<Block>,
+	VR: VotingRule<Block, TestClient>,
+{
+	test_environment_with_select_chain(
+		link,
+		keystore,
+		network_service,
+		link.select_chain.clone(),
+		voting_rule,
+	)
+}
+
 #[tokio::test]
 async fn grandpa_environment_respects_voting_rules() {
 	use finality_grandpa::voter::Environment;
@@ -1455,6 +1508,77 @@ async fn grandpa_environment_respects_voting_rules() {
 	);
 }
 
+#[tokio::test]
+async fn grandpa_environment_passes_actual_best_block_to_voting_rules() {
+	// NOTE: this is a "regression" test since initially we were not passing the
+	// best block to the voting rules
+	use finality_grandpa::voter::Environment;
+
+	let peers = &[Ed25519Keyring::Alice];
+	let voters = make_ids(peers);
+
+	let mut net = GrandpaTestNet::new(TestApi::new(voters), 1, 0);
+	let peer = net.peer(0);
+	let network_service = peer.network_service().clone();
+	let link = peer.data.lock().take().unwrap();
+	let client = peer.client().as_client().clone();
+	let select_chain = MockSelectChain::default();
+
+	// add 42 blocks
+	peer.push_blocks(42, false);
+
+	// create an environment with a voting rule that always restricts votes to
+	// before the best block by 5 blocks
+	let env = test_environment_with_select_chain(
+		&link,
+		None,
+		network_service.clone(),
+		select_chain.clone(),
+		voting_rule::BeforeBestBlockBy(5),
+	);
+
+	// both best block and finality target are pointing to the same latest block,
+	// therefore we must restrict our vote on top of the given target (#21)
+	let hashof21 = client.expect_block_hash_from_id(&BlockId::Number(21)).unwrap();
+	select_chain.set_best_chain(client.expect_header(hashof21).unwrap());
+	select_chain.set_finality_target(client.expect_header(hashof21).unwrap().hash());
+
+	assert_eq!(
+		env.best_chain_containing(peer.client().info().finalized_hash)
+			.await
+			.unwrap()
+			.unwrap()
+			.1,
+		16,
+	);
+
+	// the returned finality target is already 11 blocks from the best block,
+	// therefore there should be no further restriction by the voting rule
+	let hashof10 = client.expect_block_hash_from_id(&BlockId::Number(10)).unwrap();
+	select_chain.set_best_chain(client.expect_header(hashof21).unwrap());
+	select_chain.set_finality_target(client.expect_header(hashof10).unwrap().hash());
+
+	assert_eq!(
+		env.best_chain_containing(peer.client().info().finalized_hash)
+			.await
+			.unwrap()
+			.unwrap()
+			.1,
+		10,
+	);
+
+	// returning a finality target that's higher than the best block is an
+	// inconsistent state that should be handled
+	let hashof42 = client.expect_block_hash_from_id(&BlockId::Number(42)).unwrap();
+	select_chain.set_best_chain(client.expect_header(hashof21).unwrap());
+	select_chain.set_finality_target(client.expect_header(hashof42).unwrap().hash());
+
+	assert_matches!(
+		env.best_chain_containing(peer.client().info().finalized_hash).await,
+		Err(CommandOrError::Error(Error::Safety(_)))
+	);
+}
+
 #[tokio::test]
 async fn grandpa_environment_never_overwrites_round_voter_state() {
 	use finality_grandpa::voter::Environment;