Grandpa validator set handoff justification (#1190)

* core: make block justification optional

* runtime: update wasm binaries

* core: optionally pass justification on finalize_block

* finality-grandpa: add channel to trigger authority set changes

this will allow the `BlockImport` to trigger an authority set change when
importing a change block that provides a justification (when syncing)

* finality-grandpa: move finalize_block to free function

* finality-grandpa: add GrandpaOracle for auth set liveness checking

this will be used by `BlockImport` to check whether the authority set for a
given block is still live, if the authority set isn't live then importing a
change block requires a justification.

* finality-grandpa: store justification on finalized transition blocks

* finality-grandpa: check justification on authority set change blocks

* finality-grandpa: poll grandpa liveness oracle every 10 seconds

* finality-grandpa: spawn grandpa oracle in service setup

* core: support multiple subscriptions per consensus gossip topic

* finality-grandpa: create and verify justifications

* finality-grandpa: update to local branch of grandpa

* finality-grandpa: update to finality-grandpa v0.5.0

* finality-grandpa: move grandpa oracle code

* finality-grandpa: fix canonality check

* finality-grandpa: clean up error handling

* finality-grandpa: fix canonical_at_height

* finality-grandpa: fix tests

* runtime: update wasm binaries

* core: add tests for finalizing block with justification

* finality-grandpa: improve validation of justifications

* core: remove unused IncompleteJustification block import error

* core: test multiple subscribers for same consensus gossip topic

* Revert "finality-grandpa: improve validation of justifications"

This reverts commit 51eb2c58c2219801e876af6d6c9371bdd9ff2477.

* finality-grandpa: fix commit validation

* finality-grandpa: fix commit ancestry validation

* finality-grandpa: use grandpa v0.5.1

* finality-grandpa: add docs

* finality-grandpa: fix failing test

* finality-grandpa: only allow a pending authority set change per fork

* finality-grandpa: fix validator set transition test

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

@@ -20,6 +20,7 @@ use parking_lot::RwLock;
 use substrate_primitives::AuthorityId;
 
 use std::cmp::Ord;
+use std::collections::HashMap;
 use std::fmt::Debug;
 use std::ops::Add;
 use std::sync::Arc;
@@ -63,6 +64,11 @@ where
 	pub(crate) fn set_id(&self) -> u64 {
 		self.inner.read().set_id
 	}
+
+	/// Get the current authorities and their weights (for the current set ID).
+	pub(crate) fn current_authorities(&self) -> HashMap<AuthorityId, u64> {
+		self.inner.read().current_authorities.iter().cloned().collect()
+	}
 }
 
 impl<H, N> From<AuthoritySet<H, N>> for SharedAuthoritySet<H, N> {
@@ -109,8 +115,22 @@ where
 	N: Add<Output=N> + Ord + Clone + Debug,
 	H: Debug
 {
-	/// Note an upcoming pending transition.
-	pub(crate) fn add_pending_change(&mut self, pending: PendingChange<H, N>) {
+	/// 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>(
+		&mut self,
+		pending: PendingChange<H, N>,
+		is_equal_or_descendent_of: F,
+	) -> Result<(), E> where
+		F: Fn(&H) -> Result<(), E>,
+	{
+		for change in self.pending_changes.iter() {
+			is_equal_or_descendent_of(&change.canon_hash)?;
+		}
+
 		// 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
@@ -121,6 +141,8 @@ where
 			.unwrap_or_else(|i| i);
 
 		self.pending_changes.insert(idx, pending);
+
+		Ok(())
 	}
 
 	/// Inspect pending changes.
@@ -141,7 +163,7 @@ where
 	/// 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<H, E>
+		where F: FnMut(N) -> Result<Option<H>, E>
 	{
 		let mut status = Status {
 			changed: false,
@@ -156,30 +178,35 @@ where
 
 					// check if the block that signalled the change is canonical in
 					// our chain.
-					let canonical_at_height = canonical(change.canon_height.clone())?;
+					let canonical_hash = canonical(change.canon_height.clone())?;
+					let effective_hash = canonical(effective_number.clone())?;
+
 					debug!(target: "afg", "Evaluating potential set change at block {:?}. Our canonical hash is {:?}",
-						(&change.canon_height, &change.canon_hash), canonical_at_height);
+						(&change.canon_height, &change.canon_hash), canonical_hash);
 
-					if canonical_at_height == change.canon_hash {
-						// apply this change: make the set canonical
-						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.clone();
-						self.set_id += 1;
+								self.current_authorities = change.next_authorities.clone();
+								self.set_id += 1;
 
-						status.new_set_block = Some((
-							canonical(effective_number.clone())?,
-							effective_number.clone(),
-						));
+								status.new_set_block = Some((
+									effective_hash,
+									effective_number.clone(),
+								));
 
-						// discard any signalled changes
-						// that happened before or equal to the effective number of the change.
-						self.pending_changes.iter()
-							.take_while(|c| c.canon_height <= effective_number)
-							.count()
-					} else {
-						1 // prune out this entry; it's no longer relevant.
+								// 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.
 					}
 				}
 			};
@@ -191,6 +218,28 @@ where
 
 		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>
+	{
+		for change in self.pending_changes.iter() {
+			if change.effective_number() > just_finalized { break };
+
+			// 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)
+	}
 }
 
 /// A pending change to the authority set.
@@ -221,6 +270,10 @@ impl<H, N: Add<Output=N> + Clone> PendingChange<H, N> {
 mod tests {
 	use super::*;
 
+	fn ignore_existing_changes<A>(_a: &A) -> Result<(), ::Error> {
+		Ok(())
+	}
+
 	#[test]
 	fn changes_sorted_in_correct_order() {
 		let mut authorities = AuthoritySet {
@@ -250,9 +303,9 @@ mod tests {
 			canon_hash: "hash_c",
 		};
 
-		authorities.add_pending_change(change_a.clone());
-		authorities.add_pending_change(change_b.clone());
-		authorities.add_pending_change(change_c.clone());
+		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();
 
 		assert_eq!(authorities.pending_changes, vec![change_a, change_c, change_b]);
 	}
@@ -282,15 +335,15 @@ mod tests {
 			canon_hash: "hash_b",
 		};
 
-		authorities.add_pending_change(change_a.clone());
-		authorities.add_pending_change(change_b.clone());
+		authorities.add_pending_change(change_a.clone(), ignore_existing_changes).unwrap();
+		authorities.add_pending_change(change_b.clone(), ignore_existing_changes).unwrap();
 
 		authorities.apply_changes(10, |_| Err(())).unwrap();
 		assert!(authorities.current_authorities.is_empty());
 
 		authorities.apply_changes(15, |n| match n {
-			5 => Ok("hash_a"),
-			15 => Ok("hash_15_canon"),
+			5 => Ok(Some("hash_a")),
+			15 => Ok(Some("hash_15_canon")),
 			_ => Err(()),
 		}).unwrap();
 
@@ -300,7 +353,7 @@ mod tests {
 	}
 
 	#[test]
-	fn apply_many_changes_at_once() {
+	fn disallow_multiple_changes_on_same_fork() {
 		let mut authorities = AuthoritySet {
 			current_authorities: Vec::new(),
 			set_id: 0,
@@ -318,11 +371,10 @@ mod tests {
 			canon_hash: "hash_a",
 		};
 
-		// will be ignored because it was signalled when change_a still pending.
 		let change_b = PendingChange {
 			next_authorities: set_b.clone(),
 			finalization_depth: 10,
-			canon_height: 15,
+			canon_height: 16,
 			canon_hash: "hash_b",
 		};
 
@@ -333,20 +385,50 @@ mod tests {
 			canon_hash: "hash_c",
 		};
 
-		authorities.add_pending_change(change_a.clone());
-		authorities.add_pending_change(change_b.clone());
-		authorities.add_pending_change(change_c.clone());
+		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.apply_changes(26, |n| match n {
-			5 => Ok("hash_a"),
-			15 => Ok("hash_b"),
-			16 => Ok("hash_c"),
-			26 => Ok("hash_26"),
+		authorities.add_pending_change(
+			change_a.clone(),
+			|base| is_equal_or_descendent_of(base, change_a.canon_hash),
+		).unwrap();
+
+		// 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()
+		);
+
+		// 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();
+
+		authorities.apply_changes(15, |n| match n {
+			5 => Ok(Some("hash_a")),
+			15 => Ok(Some("hash_a15")),
 			_ => Err(()),
 		}).unwrap();
 
-		assert_eq!(authorities.current_authorities, set_c);
-		assert_eq!(authorities.set_id, 2); // has been bumped only twice
+		assert_eq!(authorities.current_authorities, set_a);
+
+		// 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());
+
+		// 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();
 	}
 }