Import multiple authority set change blocks (#1808)

* core: implement logic for tracking dag of possible pending changes * core: move pending justifications dag to its own crate * core: remove unnecessary clone bounds on dag * core: request justifications in-order from the dag * core: dag: rename changes variables to node * core: dag: allow finalizing blocks not part of dag * core: dag: track best finalized number * core: dag: add more tests * core: sync: clean up pending justifications dag * core: dag: derive codec decode encode * core: dag: better error support * core: dag: add finalization guarded by predicate * core: grandpa: track multiple authority set changes in dag * core: dag: add pre-order iterator * core: grandpa: request justifications on startup * core: dag: rearrange order of definitions * core: rename util/dag to util/fork_tree * core: fork_tree: add docs * core: fork_tree: add more tests * core: fork_tree: fix issues found in tests * core: grandpa: fix authorities tests * core: grandpa: add docs for is_descendent_of * core: sync: add docs for PendingJustifications * core: sync: add test for justification requests across forks * core: sync: don't resend import or finality notifications in tests * core: grandpa: add test for importing multiple change blocks * core: grandpa: fix logic for checking if a block enacts a change * core: grandpa: fix authorities tests
2026-05-30 19:51:02 +00:00 · 2019-02-19 23:08:43 +00:00
parent c5d3da32f2
commit 21779b8cf2
16 changed files with 1322 additions and 273 deletions
@@ -5,6 +5,7 @@ authors = ["Parity Technologies <admin@parity.io>"]
 edition = "2018"

 [dependencies]
+fork-tree = { path = "../../core/util/fork-tree" }
 futures = "0.1"
 log = "0.4"
 parking_lot = "0.7.1"
@@ -16,6 +16,7 @@

 //! Utilities for dealing with authorities, authority sets, and handoffs.

+use fork_tree::ForkTree;
 use parking_lot::RwLock;
 use substrate_primitives::Ed25519AuthorityId;
 use grandpa::VoterSet;
@@ -38,7 +39,10 @@ impl<H, N> Clone for SharedAuthoritySet<H, N> {
 	}
 }

-impl<H, N> SharedAuthoritySet<H, N> {
+impl<H, N> SharedAuthoritySet<H, N>
+where H: PartialEq,
+	  N: Ord,
+{
 	/// The genesis authority set.
 	pub(crate) fn genesis(initial: Vec<(Ed25519AuthorityId, u64)>) -> Self {
 		SharedAuthoritySet {
@@ -53,9 +57,8 @@ impl<H, N> SharedAuthoritySet<H, N> {
 }

 impl<H: Eq, N> SharedAuthoritySet<H, N>
-where
-	N: Add<Output=N> + Ord + Clone + Debug,
-	H: Debug
+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> {
@@ -80,6 +83,7 @@ impl<H, N> From<AuthoritySet<H, N>> for SharedAuthoritySet<H, N> {
 }

 /// Status of the set after changes were applied.
+#[derive(Debug)]
 pub(crate) struct Status<H, N> {
 	/// Whether internal changes were made.
 	pub(crate) changed: bool,
@@ -93,16 +97,19 @@ pub(crate) struct Status<H, N> {
 pub(crate) struct AuthoritySet<H, N> {
 	current_authorities: Vec<(Ed25519AuthorityId, u64)>,
 	set_id: u64,
-	pending_changes: Vec<PendingChange<H, N>>,
+	pending_changes: ForkTree<H, N, PendingChange<H, N>>,
 }

-impl<H, N> AuthoritySet<H, N> {
+impl<H, N> AuthoritySet<H, N>
+where H: PartialEq,
+	  N: Ord,
+{
 	/// Get a genesis set with given authorities.
 	pub(crate) fn genesis(initial: Vec<(Ed25519AuthorityId, u64)>) -> Self {
 		AuthoritySet {
 			current_authorities: initial,
 			set_id: 0,
-			pending_changes: Vec::new(),
+			pending_changes: ForkTree::new(),
 		}
 	}

@@ -115,138 +122,131 @@ impl<H, N> AuthoritySet<H, N> {
 impl<H: Eq, N> AuthoritySet<H, N>
 where
 	N: Add<Output=N> + Ord + Clone + Debug,
-	H: Debug
+	H: Clone + Debug
 {
-	/// Note an upcoming pending transition. This makes sure that there isn't
-	/// already any pending change for the same chain. Multiple pending changes
-	/// are allowed but they must be signalled in different forks. The closure
-	/// should return an error if the pending change block is equal to or a
-	/// descendent of the given block.
-	pub(crate) fn add_pending_change<F, E: Debug>(
+	/// Note an upcoming pending transition. Multiple pending changes on the
+	/// same branch can be added as long as they don't overlap. This method
+	/// assumes that changes on the same branch will be added in-order. The
+	/// given function `is_descendent_of` should return `true` if the second
+	/// hash (target) is a descendent of the first hash (base).
+	pub(crate) fn add_pending_change<F, E>(
 		&mut self,
 		pending: PendingChange<H, N>,
-		is_equal_or_descendent_of: F,
-	) -> Result<(), E> where
-		F: Fn(&H) -> Result<(), E>,
+		is_descendent_of: &F,
+	) -> Result<(), fork_tree::Error<E>> where
+		F: Fn(&H, &H) -> Result<bool, E>,
+		E:  std::error::Error,
 	{
-		for change in self.pending_changes.iter() {
-			is_equal_or_descendent_of(&change.canon_hash)?;
-		}
+		let hash = pending.canon_hash.clone();
+		let number = pending.canon_height.clone();

-		// ordered first by effective number and then by signal-block number.
-		let key = (pending.effective_number(), pending.canon_height.clone());
-		let idx = self.pending_changes
-			.binary_search_by_key(&key, |change| (
-				change.effective_number(),
-				change.canon_height.clone(),
-			))
-			.unwrap_or_else(|i| i);
+		debug!(target: "afg", "Inserting potential set change signalled at block {:?} \
+							   (delayed by {:?} blocks).",
+			   (&number, &hash), pending.finalization_depth);

-		debug!(target: "afg", "Inserting potential set change at block {:?}.",
-			(&pending.canon_height, &pending.canon_hash));
+		self.pending_changes.import(
+			hash.clone(),
+			number.clone(),
+			pending,
+			is_descendent_of,
+		)?;

-		self.pending_changes.insert(idx, pending);
-
-		debug!(target: "afg", "There are now {} pending changes.", self.pending_changes.len());
+		debug!(target: "afg", "There are now {} alternatives for the next pending change (roots), \
+							   and a total of {} pending changes (across all forks).",
+			self.pending_changes.roots().count(),
+			self.pending_changes.iter().count(),
+		);

 		Ok(())
 	}

-	/// Inspect pending changes.
-	pub(crate) fn pending_changes(&self) -> &[PendingChange<H, N>] {
-		&self.pending_changes
+	/// Inspect pending changes. Pending changes in the tree are traversed in pre-order.
+	pub(crate) fn pending_changes(&self) -> impl Iterator<Item=&PendingChange<H, N>> {
+		self.pending_changes.iter().map(|(_, _, c)| c)
 	}

-	/// Get the earliest limit-block number, if any.
+	/// 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).
 	pub(crate) fn current_limit(&self) -> Option<N> {
-		self.pending_changes.get(0).map(|change| change.effective_number().clone())
+		self.pending_changes.roots()
+			.min_by_key(|&(_, _, c)| c.effective_number())
+			.map(|(_, _, c)| c.effective_number())
 	}

-	/// Apply or prune any pending transitions. Provide a closure that can be used to check for the
-	/// finalized block with given number.
+	/// Apply or prune any pending transitions. This method ensures that if
+	/// there are multiple changes in the same branch, finalizing this block
+	/// won't finalize past multiple transitions (i.e. transitions must be
+	/// finalized in-order). The given function `is_descendent_of` should return
+	/// `true` if the second hash (target) is a descendent of the first hash
+	/// (base).
 	///
-	/// When the set has changed, the return value will be `Ok(Some((H, N)))` which is the canonical
-	/// block where the set last changed.
-	pub(crate) fn apply_changes<F, E>(&mut self, just_finalized: N, mut canonical: F)
-		-> Result<Status<H, N>, E>
-		where F: FnMut(N) -> Result<Option<H>, E>
+	/// When the set has changed, the return value will be `Ok(Some((H, N)))`
+	/// which is the canonical block where the set last changed (i.e. the given
+	/// hash and number).
+	pub(crate) fn apply_changes<F, E>(
+		&mut self,
+		finalized_hash: H,
+		finalized_number: N,
+		is_descendent_of: &F,
+	) -> Result<Status<H, N>, fork_tree::Error<E>>
+	where F: Fn(&H, &H) -> Result<bool, E>,
+		  E: std::error::Error,
 	{
 		let mut status = Status {
 			changed: false,
 			new_set_block: None,
 		};
-		loop {
-			let remove_up_to = match self.pending_changes.first() {
-				None => break,
-				Some(change) => {
-					let effective_number = change.effective_number();
-					if effective_number > just_finalized { break }

-					// check if the block that signalled the change is canonical in
-					// our chain.
-					let canonical_hash = canonical(change.canon_height.clone())?;
-					let effective_hash = canonical(effective_number.clone())?;
+		match self.pending_changes.finalize_with_descendent_if(
+			&finalized_hash,
+			finalized_number.clone(),
+			is_descendent_of,
+			|change| change.effective_number() <= finalized_number
+		)? {
+			fork_tree::FinalizationResult::Changed(change) => {
+				status.changed = true;

-					debug!(target: "afg", "Evaluating potential set change at block {:?}. Our canonical hash is {:?}",
-						(&change.canon_height, &change.canon_hash), canonical_hash);
+				if let Some(change) = change {
+					info!(target: "finality", "Applying authority set change scheduled at block #{:?}",
+						  change.canon_height);

-					match (canonical_hash, effective_hash) {
-						(Some(canonical_hash), Some(effective_hash)) => {
-							if canonical_hash == change.canon_hash {
-								// apply this change: make the set canonical
-								info!(target: "finality", "Applying authority set change scheduled at block #{:?}",
-									  change.canon_height);
+					self.current_authorities = change.next_authorities;
+					self.set_id += 1;

-								self.current_authorities = change.next_authorities.clone();
-								self.set_id += 1;
-
-								status.new_set_block = Some((
-									effective_hash,
-									effective_number.clone(),
-								));
-
-								// discard all signalled changes since they're
-								// necessarily from other forks
-								self.pending_changes.len()
-							} else {
-								1 // prune out this entry; it's no longer relevant.
-							}
-						},
-						_ => 1, // prune out this entry; it's no longer relevant.
-					}
+					status.new_set_block = Some((
+						finalized_hash,
+						finalized_number,
+					));
 				}
-			};
-
-			let remove_up_to = ::std::cmp::min(remove_up_to, self.pending_changes.len());
-			self.pending_changes.drain(..remove_up_to);
-			status.changed = true; // always changed because we strip at least the first change.
+			},
+			fork_tree::FinalizationResult::Unchanged => {},
 		}

 		Ok(status)
 	}

 	/// Check whether the given finalized block number enacts any authority set
-	/// change (without triggering it). Provide a closure that can be used to
-	/// check for the canonical block with a given number.
-	pub fn enacts_change<F, E>(&self, just_finalized: N, mut canonical: F)
-		-> Result<bool, E>
-		where F: FnMut(N) -> Result<Option<H>, E>
+	/// change (without triggering it), ensuring that if there are multiple
+	/// changes in the same branch, finalizing this block won't finalize past
+	/// multiple transitions (i.e. transitions must be finalized in-order). The
+	/// given function `is_descendent_of` should return `true` if the second
+	/// hash (target) is a descendent of the first hash (base).
+	pub fn enacts_change<F, E>(
+		&self,
+		finalized_hash: H,
+		finalized_number: N,
+		is_descendent_of: &F,
+	) -> Result<bool, fork_tree::Error<E>>
+	where F: Fn(&H, &H) -> Result<bool, E>,
+		  E: std::error::Error,
 	{
-		for change in self.pending_changes.iter() {
-			if change.effective_number() > just_finalized { break };
-
-			if change.effective_number() == just_finalized {
-				// check if the block that signalled the change is canonical in
-				// our chain.
-				match canonical(change.canon_height.clone())? {
-					Some(ref canonical_hash) if *canonical_hash == change.canon_hash =>
-						return Ok(true),
-					_ => (),
-				}
-			}
-		}
-
-		Ok(false)
+		self.pending_changes.finalizes_any_with_descendent_if(
+			&finalized_hash,
+			finalized_number.clone(),
+			is_descendent_of,
+			|change| change.effective_number() == finalized_number
+		)
 	}
 }

@@ -278,16 +278,24 @@ impl<H, N: Add<Output=N> + Clone> PendingChange<H, N> {
 mod tests {
 	use super::*;

-	fn ignore_existing_changes<A>(_a: &A) -> Result<(), crate::Error> {
-		Ok(())
+	fn static_is_descendent_of<A>(value: bool)
+		-> impl Fn(&A, &A) -> Result<bool, std::io::Error>
+	{
+		move |_, _| Ok(value)
+	}
+
+	fn is_descendent_of<A, F>(f: F) -> impl Fn(&A, &A) -> Result<bool, std::io::Error>
+		where F: Fn(&A, &A) -> bool
+	{
+		move |base, hash| Ok(f(base, hash))
 	}

 	#[test]
-	fn changes_sorted_in_correct_order() {
+	fn changes_iterated_in_pre_order() {
 		let mut authorities = AuthoritySet {
 			current_authorities: Vec::new(),
 			set_id: 0,
-			pending_changes: Vec::new(),
+			pending_changes: ForkTree::new(),
 		};

 		let change_a = PendingChange {
@@ -300,7 +308,7 @@ mod tests {
 		let change_b = PendingChange {
 			next_authorities: Vec::new(),
 			finalization_depth: 0,
-			canon_height: 16,
+			canon_height: 5,
 			canon_hash: "hash_b",
 		};

@@ -311,11 +319,18 @@ mod tests {
 			canon_hash: "hash_c",
 		};

-		authorities.add_pending_change(change_a.clone(), ignore_existing_changes).unwrap();
-		authorities.add_pending_change(change_b.clone(), ignore_existing_changes).unwrap();
-		authorities.add_pending_change(change_c.clone(), ignore_existing_changes).unwrap();
+		authorities.add_pending_change(change_a.clone(), &static_is_descendent_of(false)).unwrap();
+		authorities.add_pending_change(change_b.clone(), &static_is_descendent_of(false)).unwrap();
+		authorities.add_pending_change(change_c.clone(), &is_descendent_of(|base, hash| match (*base, *hash) {
+			("hash_a", "hash_c") => true,
+			("hash_b", "hash_c") => false,
+			_ => unreachable!(),
+		})).unwrap();

-		assert_eq!(authorities.pending_changes, vec![change_a, change_c, change_b]);
+		assert_eq!(
+			authorities.pending_changes().collect::<Vec<_>>(),
+			vec![&change_b, &change_a, &change_c],
+		);
 	}

 	#[test]
@@ -323,12 +338,13 @@ mod tests {
 		let mut authorities = AuthoritySet {
 			current_authorities: Vec::new(),
 			set_id: 0,
-			pending_changes: Vec::new(),
+			pending_changes: ForkTree::new(),
 		};

 		let set_a = vec![([1; 32].into(), 5)];
 		let set_b = vec![([2; 32].into(), 5)];

+		// two competing changes at the same height on different forks
 		let change_a = PendingChange {
 			next_authorities: set_a.clone(),
 			finalization_depth: 10,
@@ -343,35 +359,54 @@ mod tests {
 			canon_hash: "hash_b",
 		};

-		authorities.add_pending_change(change_a.clone(), ignore_existing_changes).unwrap();
-		authorities.add_pending_change(change_b.clone(), ignore_existing_changes).unwrap();
+		authorities.add_pending_change(change_a.clone(), &static_is_descendent_of(true)).unwrap();
+		authorities.add_pending_change(change_b.clone(), &static_is_descendent_of(true)).unwrap();

-		authorities.apply_changes(10, |_| Err(())).unwrap();
-		assert!(authorities.current_authorities.is_empty());
+		assert_eq!(
+			authorities.pending_changes().collect::<Vec<_>>(),
+			vec![&change_b, &change_a],
+		);

-		authorities.apply_changes(15, |n| match n {
-			5 => Ok(Some("hash_a")),
-			15 => Ok(Some("hash_15_canon")),
-			_ => Err(()),
-		}).unwrap();
+		// finalizing "hash_c" won't enact the change signalled at "hash_a" but it will prune out "hash_b"
+		let status = authorities.apply_changes("hash_c", 11, &is_descendent_of(|base, hash| match (*base, *hash) {
+			("hash_a", "hash_c") => true,
+			("hash_b", "hash_c") => false,
+			_ => unreachable!(),
+		})).unwrap();
+
+		assert!(status.changed);
+		assert_eq!(status.new_set_block, None);
+		assert_eq!(
+			authorities.pending_changes().collect::<Vec<_>>(),
+			vec![&change_a],
+		);
+
+		// finalizing "hash_d" will enact the change signalled at "hash_a"
+		let status = authorities.apply_changes("hash_d", 15, &is_descendent_of(|base, hash| match (*base, *hash) {
+			("hash_a", "hash_d") => true,
+			_ => unreachable!(),
+		})).unwrap();
+
+		assert!(status.changed);
+		assert_eq!(status.new_set_block, Some(("hash_d", 15)));

 		assert_eq!(authorities.current_authorities, set_a);
 		assert_eq!(authorities.set_id, 1);
-		assert!(authorities.pending_changes.is_empty());
+		assert_eq!(authorities.pending_changes().count(), 0);
 	}

 	#[test]
-	fn disallow_multiple_changes_on_same_fork() {
+	fn disallow_multiple_changes_being_finalized_at_once() {
 		let mut authorities = AuthoritySet {
 			current_authorities: Vec::new(),
 			set_id: 0,
-			pending_changes: Vec::new(),
+			pending_changes: ForkTree::new(),
 		};

 		let set_a = vec![([1; 32].into(), 5)];
-		let set_b = vec![([2; 32].into(), 5)];
-		let set_c = vec![([3; 32].into(), 5)];
+		let set_c = vec![([2; 32].into(), 5)];

+		// two competing changes at the same height on different forks
 		let change_a = PendingChange {
 			next_authorities: set_a.clone(),
 			finalization_depth: 10,
@@ -379,65 +414,48 @@ mod tests {
 			canon_hash: "hash_a",
 		};

-		let change_b = PendingChange {
-			next_authorities: set_b.clone(),
-			finalization_depth: 10,
-			canon_height: 16,
-			canon_hash: "hash_b",
-		};
-
 		let change_c = PendingChange {
 			next_authorities: set_c.clone(),
 			finalization_depth: 10,
-			canon_height: 16,
+			canon_height: 30,
 			canon_hash: "hash_c",
 		};

-		let is_equal_or_descendent_of = |base, block| -> Result<(), ()> {
-			match (base, block) {
-				("hash_a", "hash_b") => return Err(()),
-				("hash_a", "hash_c") => return Ok(()),
-				("hash_c", "hash_b") => return Ok(()),
-				_ => unreachable!(),
-			}
-		};
+		authorities.add_pending_change(change_a.clone(), &static_is_descendent_of(true)).unwrap();
+		authorities.add_pending_change(change_c.clone(), &static_is_descendent_of(true)).unwrap();

-		authorities.add_pending_change(
-			change_a.clone(),
-			|base| is_equal_or_descendent_of(base, change_a.canon_hash),
-		).unwrap();
+		let is_descendent_of = is_descendent_of(|base, hash| match (*base, *hash) {
+			("hash_a", "hash_b") => true,
+			("hash_a", "hash_c") => true,
+			("hash_a", "hash_d") => true,

-		// change b is on the same chain has the unfinalized change a so it should error
-		assert!(
-			authorities.add_pending_change(
-				change_b.clone(),
-				|base| is_equal_or_descendent_of(base, change_b.canon_hash),
-			).is_err()
-		);
+			("hash_c", "hash_b") => false,
+			("hash_c", "hash_d") => true,

-		// change c is accepted because it's on a different fork
-		authorities.add_pending_change(
-			change_c.clone(),
-			|base| is_equal_or_descendent_of(base, change_c.canon_hash)
-		).unwrap();
+			("hash_b", "hash_c") => true,
+			_ => unreachable!(),
+		});

-		authorities.apply_changes(15, |n| match n {
-			5 => Ok(Some("hash_a")),
-			15 => Ok(Some("hash_a15")),
-			_ => Err(()),
-		}).unwrap();
+		// trying to finalize past `change_c` without finalizing `change_a` first
+		match authorities.apply_changes("hash_d", 40, &is_descendent_of) {
+			Err(fork_tree::Error::UnfinalizedAncestor) => {},
+			_ => unreachable!(),
+		}
+
+		let status = authorities.apply_changes("hash_b", 15, &is_descendent_of).unwrap();
+		assert!(status.changed);
+		assert_eq!(status.new_set_block, Some(("hash_b", 15)));

 		assert_eq!(authorities.current_authorities, set_a);
+		assert_eq!(authorities.set_id, 1);

-		// pending change c has been removed since it was on a different fork
-		// and can no longer be enacted
-		assert!(authorities.pending_changes.is_empty());
+		// after finalizing `change_a` it should be possible to finalize `change_c`
+		let status = authorities.apply_changes("hash_d", 40, &is_descendent_of).unwrap();
+		assert!(status.changed);
+		assert_eq!(status.new_set_block, Some(("hash_d", 40)));

-		// pending change b can now be added
-		authorities.add_pending_change(
-			change_b.clone(),
-			|base| is_equal_or_descendent_of(base, change_b.canon_hash),
-		).unwrap();
+		assert_eq!(authorities.current_authorities, set_c);
+		assert_eq!(authorities.set_id, 2);
 	}

 	#[test]
@@ -445,7 +463,7 @@ mod tests {
 		let mut authorities = AuthoritySet {
 			current_authorities: Vec::new(),
 			set_id: 0,
-			pending_changes: Vec::new(),
+			pending_changes: ForkTree::new(),
 		};

 		let set_a = vec![([1; 32].into(), 5)];
@@ -457,19 +475,21 @@ mod tests {
 			canon_hash: "hash_a",
 		};

-		authorities.add_pending_change(change_a.clone(), |_| Err(())).unwrap();
+		authorities.add_pending_change(change_a.clone(), &static_is_descendent_of(false)).unwrap();

-		let canonical = |n| match n {
-			5 => Ok(Some("hash_a")),
-			15 => Ok(Some("hash_a15")),
-			_ => Err(()),
-		};
+		let is_descendent_of = is_descendent_of(|base, hash| match (*base, *hash) {
+			("hash_a", "hash_b") => true,
+			("hash_a", "hash_c") => false,
+			_ => unreachable!(),
+		});

-		// there's an effective change triggered at block 15
-		assert!(authorities.enacts_change(15, canonical).unwrap());
+		// "hash_c" won't finalize the existing change since it isn't a descendent
+		assert!(!authorities.enacts_change("hash_c", 15, &is_descendent_of).unwrap());

-		// block 16 is already past the change, we assume 15 will be finalized
-		// first and enact the change
-		assert!(!authorities.enacts_change(16, canonical).unwrap());
+		// "hash_b" at depth 14 won't work either
+		assert!(!authorities.enacts_change("hash_b", 14, &is_descendent_of).unwrap());
+
+		// but it should work at depth 15 (change height + depth)
+		assert!(authorities.enacts_change("hash_b", 15, &is_descendent_of).unwrap());
 	}
 }
@@ -368,6 +368,7 @@ impl<Block: BlockT> From<GrandpaJustification<Block>> for JustificationOrCommit<
 /// Finalize the given block and apply any authority set changes. If an
 /// authority set change is enacted then a justification is created (if not
 /// given) and stored with the block when finalizing it.
+/// This method assumes that the block being finalized has already been imported.
 pub(crate) fn finalize_block<B, Block: BlockT<Hash=H256>, E, RA>(
 	client: &Client<B, E, Block, RA>,
 	authority_set: &SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
@@ -391,9 +392,11 @@ pub(crate) fn finalize_block<B, Block: BlockT<Hash=H256>, E, RA>(
 	let mut old_last_completed = None;

 	let mut consensus_changes = consensus_changes.lock();
-	let status = authority_set.apply_changes(number, |canon_number| {
-		canonical_at_height(client, (hash, number), canon_number)
-	})?;
+	let status = authority_set.apply_changes(
+		hash,
+		number,
+		&is_descendent_of(client, None),
+	).map_err(|e| Error::Safety(e.to_string()))?;

 	if status.changed {
 		// write new authority set state to disk.
@@ -599,3 +602,41 @@ pub(crate) fn canonical_at_height<B, E, Block: BlockT<Hash=H256>, RA>(

 	Ok(Some(current.hash()))
 }
+
+/// Returns a function for checking block ancestry, the returned function will
+/// return `true` if the given hash (second parameter) is a descendent of the
+/// base (first parameter). If the `current` parameter is defined, it should
+/// represent the current block `hash` and its `parent hash`, if given the
+/// function that's returned will assume that `hash` isn't part of the local DB
+/// yet, and all searches in the DB will instead reference the parent.
+pub fn is_descendent_of<'a, B, E, Block: BlockT<Hash=H256>, RA>(
+	client: &'a Client<B, E, Block, RA>,
+	current: Option<(&'a H256, &'a H256)>,
+) -> impl Fn(&H256, &H256) -> Result<bool, client::error::Error> + 'a
+where B: Backend<Block, Blake2Hasher>,
+	  E: CallExecutor<Block, Blake2Hasher> + Send + Sync,
+{
+	move |base, hash| {
+		if base == hash { return Ok(false); }
+
+		let mut hash = hash;
+		if let Some((current_hash, current_parent_hash)) = current {
+			if base == current_hash { return Ok(false); }
+			if hash == current_hash {
+				if base == current_parent_hash {
+					return Ok(true);
+				} else {
+					hash = current_parent_hash;
+				}
+			}
+		}
+
+		let tree_route = client::blockchain::tree_route(
+			client.backend().blockchain(),
+			BlockId::Hash(*hash),
+			BlockId::Hash(*base),
+		)?;
+
+		Ok(tree_route.common_block().hash == *base)
+	}
+}
@@ -38,7 +38,7 @@ use substrate_primitives::{H256, Ed25519AuthorityId, Blake2Hasher};
 use crate::{AUTHORITY_SET_KEY, Error};
 use crate::authorities::SharedAuthoritySet;
 use crate::consensus_changes::SharedConsensusChanges;
-use crate::environment::{canonical_at_height, finalize_block, ExitOrError, NewAuthoritySet};
+use crate::environment::{finalize_block, is_descendent_of, ExitOrError, NewAuthoritySet};
 use crate::justification::GrandpaJustification;

 /// A block-import handler for GRANDPA.
@@ -78,7 +78,8 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> JustificationImport<Block>
 		};

 		// request justifications for all pending changes for which change blocks have already been imported
-		for pending_change in self.authority_set.inner().read().pending_changes() {
+		let authorities = self.authority_set.inner().read();
+		for pending_change in authorities.pending_changes() {
 			if pending_change.effective_number() > chain_info.finalized_number &&
 				pending_change.effective_number() <= chain_info.best_number
 			{
@@ -128,7 +129,8 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 		use client::blockchain::HeaderBackend;

 		let hash = block.post_header().hash();
-		let number = block.header.number().clone();
+		let parent_hash = *block.header.parent_hash();
+		let number = *block.header.number();

 		// early exit if block already in chain, otherwise the check for
 		// authority changes will error when trying to re-import a change block
@@ -139,7 +141,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 		}

 		let maybe_change = self.api.runtime_api().grandpa_pending_change(
-			&BlockId::hash(*block.header.parent_hash()),
+			&BlockId::hash(parent_hash),
 			&block.header.digest().clone(),
 		);

@@ -151,39 +153,11 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 		// when we update the authorities, we need to hold the lock
 		// until the block is written to prevent a race if we need to restore
 		// the old authority set on error.
+		let is_descendent_of = is_descendent_of(&self.inner, Some((&hash, &parent_hash)));
 		let just_in_case = if let Some(change) = maybe_change {
-			let parent_hash = *block.header.parent_hash();
-
 			let mut authorities = self.authority_set.inner().write();
 			let old_set = authorities.clone();

-			let is_equal_or_descendent_of = |base: &Block::Hash| -> Result<(), ConsensusError> {
-				let error = || {
-					debug!(target: "afg", "rejecting change: {} is in the same chain as {}", hash, base);
-					Err(ConsensusErrorKind::ClientImport("Incorrect base hash".to_string()).into())
-				};
-
-				if *base == hash { return error(); }
-				if *base == parent_hash { return error(); }
-
-				let tree_route = blockchain::tree_route(
-					self.inner.backend().blockchain(),
-					BlockId::Hash(parent_hash),
-					BlockId::Hash(*base),
-				);
-
-				let tree_route = match tree_route {
-					Err(e) => return Err(ConsensusErrorKind::ClientImport(e.to_string()).into()),
-					Ok(route) => route,
-				};
-
-				if tree_route.common_block().hash == *base {
-					return error();
-				}
-
-				Ok(())
-			};
-
 			authorities.add_pending_change(
 				PendingChange {
 					next_authorities: change.next_authorities,
@@ -191,8 +165,8 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 					canon_height: number,
 					canon_hash: hash,
 				},
-				is_equal_or_descendent_of,
-			)?;
+				&is_descendent_of,
+			).map_err(|e| ConsensusError::from(ConsensusErrorKind::ClientImport(e.to_string())))?;

 			block.auxiliary.push((AUTHORITY_SET_KEY.to_vec(), Some(authorities.encode())));
 			Some((old_set, authorities))
@@ -226,9 +200,11 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 			}
 		};

-		let enacts_change = self.authority_set.inner().read().enacts_change(number, |canon_number| {
-			canonical_at_height(&self.inner, (hash, number), canon_number)
-		}).map_err(|e| ConsensusError::from(ConsensusErrorKind::ClientImport(e.to_string())))?;
+		let enacts_change = self.authority_set.inner().read().enacts_change(
+			hash,
+			number,
+			&is_descendent_of,
+		).map_err(|e| ConsensusError::from(ConsensusErrorKind::ClientImport(e.to_string())))?;

 		if !enacts_change && !enacts_consensus_change {
 			return Ok(import_result);
@@ -340,6 +316,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA>
 					Error::Network(error) => ConsensusErrorKind::ClientImport(error),
 					Error::Blockchain(error) => ConsensusErrorKind::ClientImport(error),
 					Error::Client(error) => ConsensusErrorKind::ClientImport(error.to_string()),
+					Error::Safety(error) => ConsensusErrorKind::ClientImport(error),
 					Error::Timer(error) => ConsensusErrorKind::ClientImport(error.to_string()),
 				}.into());
 			},
@@ -45,12 +45,12 @@
 //! logic is complex to compute because it requires looking arbitrarily far
 //! back in the chain.
 //!
-//! Instead, we keep track of a list of all signals we've seen so far,
-//! sorted ascending by the block number they would be applied at. We never vote
-//! on chains with number higher than the earliest handoff block number
-//! (this is num(signal) + N). When finalizing a block, we either apply or prune
-//! any signaled changes based on whether the signaling block is included in the
-//! newly-finalized chain.
+//! Instead, we keep track of a list of all signals we've seen so far (across
+//! all forks), sorted ascending by the block number they would be applied at.
+//! We never vote on chains with number higher than the earliest handoff block
+//! number (this is num(signal) + N). When finalizing a block, we either apply
+//! or prune any signaled changes based on whether the signaling block is
+//! included in the newly-finalized chain.

 use futures::prelude::*;
 use log::{debug, info, warn};
@@ -169,6 +169,8 @@ pub enum Error {
 	Blockchain(String),
 	/// Could not complete a round on disk.
 	Client(ClientError),
+	/// An invariant has been violated (e.g. not finalizing pending change blocks in-order)
+	Safety(String),
 	/// A timer failed to fire.
 	Timer(::tokio::timer::Error),
 }
@@ -416,7 +416,12 @@ fn run_to_completion(blocks: u64, net: Arc<Mutex<GrandpaTestNet>>, peers: &[Keyr
 		.map_err(|_| ());

 	let drive_to_completion = ::tokio::timer::Interval::new_interval(TEST_ROUTING_INTERVAL)
-		.for_each(move |_| { net.lock().route_fast(); Ok(()) })
+		.for_each(move |_| {
+			net.lock().send_import_notifications();
+			net.lock().send_finality_notifications();
+			net.lock().route_fast();
+			Ok(())
+		})
 		.map(|_| ())
 		.map_err(|_| ());

@@ -550,7 +555,7 @@ fn transition_3_voters_twice_1_observer() {
 		let set = AuthoritySet::<Hash, BlockNumber>::decode(&mut &set_raw[..]).unwrap();

 		assert_eq!(set.current(), (0, make_ids(peers_a).as_slice()));
-		assert_eq!(set.pending_changes().len(), 0);
+		assert_eq!(set.pending_changes().count(), 0);
 	}

 	{
@@ -636,7 +641,7 @@ fn transition_3_voters_twice_1_observer() {
 					let set = AuthoritySet::<Hash, BlockNumber>::decode(&mut &set_raw[..]).unwrap();

 					assert_eq!(set.current(), (2, make_ids(peers_c).as_slice()));
-					assert!(set.pending_changes().is_empty());
+					assert_eq!(set.pending_changes().count(), 0);
 				})
 		);
 		let voter = run_grandpa(
@@ -771,12 +776,71 @@ fn sync_justifications_on_change_blocks() {
 	}

 	// the last peer should get the justification by syncing from other peers
-	assert!(net.lock().peer(3).client().justification(&BlockId::Number(21)).unwrap().is_none());
 	while net.lock().peer(3).client().justification(&BlockId::Number(21)).unwrap().is_none() {
 		net.lock().route_fast();
 	}
 }

+#[test]
+fn finalizes_multiple_pending_changes_in_order() {
+	env_logger::init();
+
+	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
+	let peers_b = &[Keyring::Dave, Keyring::Eve, Keyring::Ferdie];
+	let peers_c = &[Keyring::Dave, Keyring::Alice, Keyring::Bob];
+
+	let all_peers = &[
+		Keyring::Alice, Keyring::Bob, Keyring::Charlie,
+		Keyring::Dave, Keyring::Eve, Keyring::Ferdie,
+	];
+	let genesis_voters = make_ids(peers_a);
+
+	// 6 peers, 3 of them are authorities and participate in grandpa from genesis
+	let api = TestApi::new(genesis_voters);
+	let transitions = api.scheduled_changes.clone();
+	let mut net = GrandpaTestNet::new(api, 6);
+
+	// add 20 blocks
+	net.peer(0).push_blocks(20, false);
+
+	// at block 21 we do add a transition which is instant
+	net.peer(0).generate_blocks(1, BlockOrigin::File, |builder| {
+		let block = builder.bake().unwrap();
+		transitions.lock().insert(*block.header.parent_hash(), ScheduledChange {
+			next_authorities: make_ids(peers_b),
+			delay: 0,
+		});
+		block
+	});
+
+	// add more blocks on top of it (until we have 25)
+	net.peer(0).push_blocks(4, false);
+
+	// at block 26 we add another which is enacted at block 30
+	net.peer(0).generate_blocks(1, BlockOrigin::File, |builder| {
+		let block = builder.bake().unwrap();
+		transitions.lock().insert(*block.header.parent_hash(), ScheduledChange {
+			next_authorities: make_ids(peers_c),
+			delay: 4,
+		});
+		block
+	});
+
+	// add more blocks on top of it (until we have 30)
+	net.peer(0).push_blocks(4, false);
+
+	net.sync();
+
+	// all peers imported both change blocks
+	for i in 0..6 {
+		assert_eq!(net.peer(i).client().info().unwrap().chain.best_number, 30,
+			"Peer #{} failed to sync", i);
+	}
+
+	let net = Arc::new(Mutex::new(net));
+	run_to_completion(30, net.clone(), all_peers);
+}
+
 #[test]
 fn doesnt_vote_on_the_tip_of_the_chain() {
 	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];