core: grandpa limits votes to 3/4 of the unfinalized chain (#1554)

* core: grandpa limits votes to 3/4 of the unfinalized chain

* core: test grandpa limited votes

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

@@ -95,7 +95,7 @@ use consensus_common::{BlockImport, JustificationImport, Error as ConsensusError
 use runtime_primitives::Justification;
 use runtime_primitives::traits::{
 	NumberFor, Block as BlockT, Header as HeaderT, DigestFor, ProvideRuntimeApi, Hash as HashT,
-	DigestItemFor, DigestItem, As, Zero,
+	DigestItemFor, DigestItem, As, One, Zero,
 };
 use fg_primitives::GrandpaApi;
 use runtime_primitives::generic::BlockId;
@@ -422,16 +422,57 @@ 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, limit) {
+		match self.inner.best_containing(block, None) {
-			Ok(Some(hash)) => {
+			Ok(Some(mut best_hash)) => {
-				let header = self.inner.header(&BlockId::Hash(hash)).ok()?
+				let base_header = self.inner.header(&BlockId::Hash(block)).ok()?
 					.expect("Header known to exist after `best_containing` call; qed");
 
-				Some((hash, header.number().clone()))
+				if let Some(limit) = limit {
+					// this is a rare case which might cause issues,
+					// might be better to return the header itself.
+					if *base_header.number() > limit {
+						debug!(target: "afg", "Encountered error finding best chain containing {:?} with limit {:?}: target block is after limit",
+							block,
+							limit,
+						);
+						return None;
+					}
+				}
+
+				let mut best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()?
+					.expect("Header known to exist after `best_containing` call; qed");
+
+				// we target a vote towards 3/4 of the unfinalized chain (rounding up)
+				let target = {
+					let two = NumberFor::<Block>::one() + One::one();
+					let three = two + One::one();
+					let four = three + One::one();
+
+					let diff = *best_header.number() - *base_header.number();
+					let diff = ((diff * three) + two) / four;
+
+					*base_header.number() + diff
+				};
+
+				// unless our vote is currently being limited due to a pending change
+				let target = limit.map(|limit| limit.min(target)).unwrap_or(target);
+
+				// walk backwards until we find the target block
+				loop {
+					if *best_header.number() < target { unreachable!(); }
+					if *best_header.number() == target {
+						return Some((best_hash, *best_header.number()));
+					}
+
+					best_hash = *best_header.parent_hash();
+					best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()?
+						.expect("Header known to exist after `best_containing` call; qed");
+				}
+			},
+			Ok(None) => {
+				debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block);
+				None
 			}
-			// Ok(None) can be returned when `block` is after `limit`. That might cause issues.
-			// might be better to return the header itself in this (rare) case.
-			Ok(None) => None,
 			Err(e) => {
 				debug!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e);
 				None

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

@@ -329,11 +329,16 @@ fn make_ids(keys: &[Keyring]) -> Vec<(Ed25519AuthorityId, u64)> {
 		.collect()
 }
 
-fn run_to_completion(blocks: u64, net: Arc<Mutex<GrandpaTestNet>>, peers: &[Keyring]) {
+fn run_to_completion(blocks: u64, net: Arc<Mutex<GrandpaTestNet>>, peers: &[Keyring]) -> u64 {
+	use parking_lot::RwLock;
+
 	let mut finality_notifications = Vec::new();
 	let mut runtime = current_thread::Runtime::new().unwrap();
 
+	let highest_finalized = Arc::new(RwLock::new(0));
+
 	for (peer_id, key) in peers.iter().enumerate() {
+		let highest_finalized = highest_finalized.clone();
 		let (client, link) = {
 			let mut net = net.lock();
 			// temporary needed for some reason
@@ -345,7 +350,13 @@ fn run_to_completion(blocks: u64, net: Arc<Mutex<GrandpaTestNet>>, peers: &[Keyr
 		};
 		finality_notifications.push(
 			client.finality_notification_stream()
-				.take_while(|n| Ok(n.header.number() < &blocks))
+				.take_while(move |n| {
+					let mut highest_finalized = highest_finalized.write();
+					if *n.header.number() > *highest_finalized {
+						*highest_finalized = *n.header.number();
+					}
+					Ok(n.header.number() < &blocks)
+				})
 				.for_each(|_| Ok(()))
 		);
 		fn assert_send<T: Send>(_: &T) { }
@@ -378,6 +389,10 @@ fn run_to_completion(blocks: u64, net: Arc<Mutex<GrandpaTestNet>>, peers: &[Keyr
 		.map_err(|_| ());
 
 	runtime.block_on(wait_for.select(drive_to_completion).map_err(|_| ())).unwrap();
+
+	let highest_finalized = *highest_finalized.read();
+
+	highest_finalized
 }
 
 #[test]
@@ -683,8 +698,6 @@ fn consensus_changes_works() {
 
 #[test]
 fn sync_justifications_on_change_blocks() {
-	::env_logger::init();
-
 	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
 	let peers_b = &[Keyring::Alice, Keyring::Bob];
 	let voters = make_ids(peers_b);
@@ -731,3 +744,28 @@ fn sync_justifications_on_change_blocks() {
 		net.lock().sync_steps(100);
 	}
 }
+
+#[test]
+fn doesnt_vote_on_the_tip_of_the_chain() {
+	::env_logger::init();
+
+	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
+	let voters = make_ids(peers_a);
+	let api = TestApi::new(voters);
+	let mut net = GrandpaTestNet::new(api, 3);
+
+	// add 100 blocks
+	net.peer(0).push_blocks(100, false);
+	net.sync();
+
+	for i in 0..3 {
+		assert_eq!(net.peer(i).client().info().unwrap().chain.best_number, 100,
+			"Peer #{} failed to sync", i);
+	}
+
+	let net = Arc::new(Mutex::new(net));
+	let highest = run_to_completion(75, net.clone(), peers_a);
+
+	// the highest block to be finalized will be 3/4 deep in the unfinalized chain
+	assert_eq!(highest, 75);
+}