grandpa: guarantee that vote limit is never lower than vote base (#4563)

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

@@ -51,9 +51,10 @@ impl<H: Eq, N> SharedAuthoritySet<H, N>
 where N: Add<Output=N> + Ord + Clone + Debug,
 	  H: Clone + Debug
 {
-	/// Get the earliest limit-block number, if any.
-	pub(crate) fn current_limit(&self) -> Option<N> {
-		self.inner.read().current_limit()
+	/// Get the earliest limit-block number that's higher or equal to the given
+	/// min number, if any.
+	pub(crate) fn current_limit(&self, min: N) -> Option<N> {
+		self.inner.read().current_limit(min)
 	}
 
 	/// Get the current set ID. This is incremented every time the set changes.
@@ -224,10 +225,13 @@ where
 
 	/// Get the earliest limit-block number, if any. If there are pending changes across
 	/// different forks, this method will return the earliest effective number (across the
-	/// different branches). Only standard changes are taken into account for the current
+	/// different branches) that is higher or equal to the given min number.
+	///
+	/// Only standard changes are taken into account for the current
 	/// limit, since any existing forced change should preclude the voter from voting.
-	pub(crate) fn current_limit(&self) -> Option<N> {
+	pub(crate) fn current_limit(&self, min: N) -> Option<N> {
 		self.pending_standard_changes.roots()
+			.filter(|&(_, _, c)| c.effective_number() >= min)
 			.min_by_key(|&(_, _, c)| c.effective_number())
 			.map(|(_, _, c)| c.effective_number())
 	}
@@ -445,6 +449,51 @@ mod tests {
 		move |base, hash| Ok(f(base, hash))
 	}
 
+	#[test]
+	fn current_limit_filters_min() {
+		let mut authorities = AuthoritySet {
+			current_authorities: Vec::new(),
+			set_id: 0,
+			pending_standard_changes: ForkTree::new(),
+			pending_forced_changes: Vec::new(),
+		};
+
+		let change = |height| {
+			PendingChange {
+				next_authorities: Vec::new(),
+				delay: 0,
+				canon_height: height,
+				canon_hash: height.to_string(),
+				delay_kind: DelayKind::Finalized,
+			}
+		};
+
+		let is_descendent_of = static_is_descendent_of(false);
+
+		authorities.add_pending_change(change(1), &is_descendent_of).unwrap();
+		authorities.add_pending_change(change(2), &is_descendent_of).unwrap();
+
+		assert_eq!(
+			authorities.current_limit(0),
+			Some(1),
+		);
+
+		assert_eq!(
+			authorities.current_limit(1),
+			Some(1),
+		);
+
+		assert_eq!(
+			authorities.current_limit(2),
+			Some(2),
+		);
+
+		assert_eq!(
+			authorities.current_limit(3),
+			None,
+		);
+	}
+
 	#[test]
 	fn changes_iterated_in_pre_order() {
 		let mut authorities = AuthoritySet {

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

@@ -432,32 +432,26 @@ where
 			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.
-		let limit = self.authority_set.current_limit();
+		// 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 base_header = self.client.header(&BlockId::Hash(block)).ok()?
-					.expect("Header known to exist after `best_containing` call; qed");
-
-				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 best_header = self.client.header(&BlockId::Hash(best_hash)).ok()?
-					.expect("Header known to exist after `best_containing` call; qed");
+					.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());
@@ -481,7 +475,7 @@ where
 						}
 
 						target_header = self.client.header(&BlockId::Hash(*target_header.parent_hash())).ok()?
-							.expect("Header known to exist after `best_containing` call; qed");
+							.expect("Header known to exist after `finality_target` call; qed");
 					}
 
 					target = target_header;