grandpa: fix early enactment of forced changes (#7321)

* grandpa: fix early enactment of forced authority set changes

* grandpa: add test for early enactment of forced changes

* grandpa: fix typo in log message

* grandpa: only allow one pending forced change per fork

* grandpa: fix tests

client/finality-grandpa/src/authorities.rs

@@ -32,14 +32,42 @@ use std::ops::Add;
 use std::sync::Arc;
 
 /// Error type returned on operations on the `AuthoritySet`.
-#[derive(Debug, derive_more::Display, derive_more::From)]
-pub enum Error<E> {
-	#[display("Invalid authority set, either empty or with an authority weight set to 0.")]
+#[derive(Debug, derive_more::Display)]
+pub enum Error<N, E> {
+	#[display(fmt = "Invalid authority set, either empty or with an authority weight set to 0.")]
 	InvalidAuthoritySet,
+	#[display(fmt = "Client error during ancestry lookup: {}", _0)]
+	Client(E),
+	#[display(fmt = "Duplicate authority set change.")]
+	DuplicateAuthoritySetChange,
+	#[display(fmt = "Multiple pending forced authority set changes are not allowed.")]
+	MultiplePendingForcedAuthoritySetChanges,
+	#[display(
+		fmt = "A pending forced authority set change could not be applied since it must be applied after \
+		 the pending standard change at #{}",
+		_0
+	)]
+	ForcedAuthoritySetChangeDependencyUnsatisfied(N),
 	#[display(fmt = "Invalid operation in the pending changes tree: {}", _0)]
 	ForkTree(fork_tree::Error<E>),
 }
 
+impl<N, E> From<fork_tree::Error<E>> for Error<N, E> {
+	fn from(err: fork_tree::Error<E>) -> Error<N, E> {
+		match err {
+			fork_tree::Error::Client(err) => Error::Client(err),
+			fork_tree::Error::Duplicate => Error::DuplicateAuthoritySetChange,
+			err => Error::ForkTree(err),
+		}
+	}
+}
+
+impl<N, E: std::error::Error> From<E> for Error<N, E> {
+	fn from(err: E) -> Error<N, E> {
+		Error::Client(err)
+	}
+}
+
 /// A shared authority set.
 pub struct SharedAuthoritySet<H, N> {
 	inner: Arc<RwLock<AuthoritySet<H, N>>>,
@@ -111,14 +139,20 @@ pub(crate) struct AuthoritySet<H, N> {
 	/// a given branch
 	pub(crate) pending_standard_changes: ForkTree<H, N, PendingChange<H, N>>,
 	/// Pending forced changes across different forks (at most one per fork).
-	/// Forced changes are enacted on block depth (not finality), for this reason
-	/// only one forced change should exist per fork.
+	/// Forced changes are enacted on block depth (not finality), for this
+	/// reason only one forced change should exist per fork. When trying to
+	/// apply forced changes we keep track of any pending standard changes that
+	/// they may depend on, this is done by making sure that any pending change
+	/// that is an ancestor of the forced changed and its effective block number
+	/// is lower than the last finalized block (as signaled in the forced
+	/// change) must be applied beforehand.
 	pending_forced_changes: Vec<PendingChange<H, N>>,
 }
 
 impl<H, N> AuthoritySet<H, N>
-where H: PartialEq,
-	  N: Ord,
+where
+	H: PartialEq,
+	N: Ord,
 {
 	// authority sets must be non-empty and all weights must be greater than 0
 	fn invalid_authority_list(authorities: &AuthorityList) -> bool {
@@ -180,7 +214,7 @@ where
 		&self,
 		best_hash: &H,
 		is_descendent_of: &F,
-	) -> Result<Option<(H, N)>, fork_tree::Error<E>>
+	) -> Result<Option<(H, N)>, Error<N, E>>
 	where
 		F: Fn(&H, &H) -> Result<bool, E>,
 		E: std::error::Error,
@@ -219,7 +253,8 @@ where
 		&mut self,
 		pending: PendingChange<H, N>,
 		is_descendent_of: &F,
-	) -> Result<(), Error<E>> where
+	) -> Result<(), Error<N, E>>
+	where
 		F: Fn(&H, &H) -> Result<bool, E>,
 		E: std::error::Error,
 	{
@@ -250,16 +285,18 @@ where
 		&mut self,
 		pending: PendingChange<H, N>,
 		is_descendent_of: &F,
-	) -> Result<(), Error<E>> where
+	) -> Result<(), Error<N, E>>
+	where
 		F: Fn(&H, &H) -> Result<bool, E>,
 		E: std::error::Error,
 	{
-		for change in self.pending_forced_changes.iter() {
-			if change.canon_hash == pending.canon_hash ||
-				is_descendent_of(&change.canon_hash, &pending.canon_hash)
-					.map_err(fork_tree::Error::Client)?
-			{
-				return Err(fork_tree::Error::UnfinalizedAncestor.into());
+		for change in &self.pending_forced_changes {
+			if change.canon_hash == pending.canon_hash {
+				return Err(Error::DuplicateAuthoritySetChange);
+			}
+
+			if is_descendent_of(&change.canon_hash, &pending.canon_hash)? {
+				return Err(Error::MultiplePendingForcedAuthoritySetChanges);
 			}
 		}
 
@@ -293,7 +330,8 @@ where
 		&mut self,
 		pending: PendingChange<H, N>,
 		is_descendent_of: &F,
-	) -> Result<(), Error<E>> where
+	) -> Result<(), Error<N, E>>
+	where
 		F: Fn(&H, &H) -> Result<bool, E>,
 		E: std::error::Error,
 	{
@@ -341,52 +379,92 @@ where
 	///
 	/// These transitions are always forced and do not lead to justifications
 	/// which light clients can follow.
+	///
+	/// Forced changes can only be applied after all pending standard changes
+	/// that it depends on have been applied. If any pending standard change
+	/// exists that is an ancestor of a given forced changed and which effective
+	/// block number is lower than the last finalized block (as defined by the
+	/// forced change), then the forced change cannot be applied. An error will
+	/// be returned in that case which will prevent block import.
 	pub(crate) fn apply_forced_changes<F, E>(
 		&self,
 		best_hash: H,
 		best_number: N,
 		is_descendent_of: &F,
 		initial_sync: bool,
-	) -> Result<Option<(N, Self)>, E>
-		where F: Fn(&H, &H) -> Result<bool, E>,
+	) -> Result<Option<(N, Self)>, Error<N, E>>
+	where
+		F: Fn(&H, &H) -> Result<bool, E>,
+		E: std::error::Error,
 	{
 		let mut new_set = None;
 
-		for change in self.pending_forced_changes.iter()
+		for change in self
+			.pending_forced_changes
+			.iter()
 			.take_while(|c| c.effective_number() <= best_number) // to prevent iterating too far
 			.filter(|c| c.effective_number() == best_number)
 		{
 			// check if the given best block is in the same branch as
 			// the block that signaled the change.
 			if change.canon_hash == best_hash || is_descendent_of(&change.canon_hash, &best_hash)? {
+				let median_last_finalized = match change.delay_kind {
+					DelayKind::Best {
+						ref median_last_finalized,
+					} => median_last_finalized.clone(),
+					_ => unreachable!(
+						"pending_forced_changes only contains forced changes; forced changes have delay kind Best; qed."
+					),
+				};
+
+				// check if there's any pending standard change that we depend on
+				for (_, _, standard_change) in self.pending_standard_changes.roots() {
+					if standard_change.effective_number() <= median_last_finalized
+						&& is_descendent_of(&standard_change.canon_hash, &change.canon_hash)?
+					{
+						log::info!(target: "afg",
+							"Not applying authority set change forced at block #{:?}, due to pending standard change at block #{:?}",
+							change.canon_height,
+							standard_change.effective_number(),
+						);
+
+						return Err(
+							Error::ForcedAuthoritySetChangeDependencyUnsatisfied(
+								standard_change.effective_number()
+							)
+						);
+					}
+				}
+
 				// apply this change: make the set canonical
-				afg_log!(initial_sync,
+				afg_log!(
+					initial_sync,
 					"👴 Applying authority set change forced at block #{:?}",
 					change.canon_height,
 				);
-				telemetry!(CONSENSUS_INFO; "afg.applying_forced_authority_set_change";
+
+				telemetry!(
+					CONSENSUS_INFO;
+					"afg.applying_forced_authority_set_change";
 					"block" => ?change.canon_height
 				);
 
-				let median_last_finalized = match change.delay_kind {
-					DelayKind::Best { ref median_last_finalized } => median_last_finalized.clone(),
-					_ => unreachable!("pending_forced_changes only contains forced changes; forced changes have delay kind Best; qed."),
-				};
-
-				new_set = Some((median_last_finalized, AuthoritySet {
-					current_authorities: change.next_authorities.clone(),
-					set_id: self.set_id + 1,
-					pending_standard_changes: ForkTree::new(), // new set, new changes.
-					pending_forced_changes: Vec::new(),
-				}));
+				new_set = Some((
+					median_last_finalized,
+					AuthoritySet {
+						current_authorities: change.next_authorities.clone(),
+						set_id: self.set_id + 1,
+						pending_standard_changes: ForkTree::new(), // new set, new changes.
+						pending_forced_changes: Vec::new(),
+					},
+				));
 
 				break;
 			}
-
-			// we don't wipe forced changes until another change is
-			// applied
 		}
 
+		// we don't wipe forced changes until another change is applied, hence
+		// why we return a new set instead of mutating.
 		Ok(new_set)
 	}
 
@@ -406,7 +484,8 @@ where
 		finalized_number: N,
 		is_descendent_of: &F,
 		initial_sync: bool,
-	) -> Result<Status<H, N>, Error<E>> where
+	) -> Result<Status<H, N>, Error<N, E>>
+	where
 		F: Fn(&H, &H) -> Result<bool, E>,
 		E: std::error::Error,
 	{
@@ -429,12 +508,11 @@ where
 					Vec::new(),
 				);
 
-				// we will keep all forced change for any later blocks and that are a
-				// descendent of the finalized block (i.e. they are from this fork).
+				// we will keep all forced changes for any later blocks and that are a
+				// descendent of the finalized block (i.e. they are part of this branch).
 				for change in pending_forced_changes {
 					if change.effective_number() > finalized_number &&
-						is_descendent_of(&finalized_hash, &change.canon_hash)
-							.map_err(fork_tree::Error::Client)?
+						is_descendent_of(&finalized_hash, &change.canon_hash)?
 					{
 						self.pending_forced_changes.push(change)
 					}
@@ -479,7 +557,8 @@ where
 		finalized_hash: H,
 		finalized_number: N,
 		is_descendent_of: &F,
-	) -> Result<Option<bool>, Error<E>> where
+	) -> Result<Option<bool>, Error<N, E>>
+	where
 		F: Fn(&H, &H) -> Result<bool, E>,
 		E: std::error::Error,
 	{
@@ -928,7 +1007,13 @@ mod tests {
 		};
 
 		authorities.add_pending_change(change_a, &static_is_descendent_of(false)).unwrap();
-		authorities.add_pending_change(change_b, &static_is_descendent_of(false)).unwrap();
+		authorities.add_pending_change(change_b.clone(), &static_is_descendent_of(false)).unwrap();
+
+		// no duplicates are allowed
+		assert!(matches!(
+			authorities.add_pending_change(change_b, &static_is_descendent_of(false)),
+			Err(Error::DuplicateAuthoritySetChange)
+		));
 
 		// there's an effective change triggered at block 15 but not a standard one.
 		// so this should do nothing.
@@ -953,9 +1038,11 @@ mod tests {
 			Err(Error::MultiplePendingForcedAuthoritySetChanges)
 		));
 
-		// too early.
+		// let's try and apply the forced changes.
+		// too early and there's no forced changes to apply.
 		assert!(
-			authorities.apply_forced_changes("hash_a10", 10, &static_is_descendent_of(true), false)
+			authorities
+				.apply_forced_changes("hash_a10", 10, &static_is_descendent_of(true), false)
 				.unwrap()
 				.is_none()
 		);
@@ -1379,26 +1466,11 @@ mod tests {
 		add_pending_change(15, "C3", true);
 		add_pending_change(20, "D", true);
 
-		println!(
-			"pending_changes: {:?}",
-			authorities
-				.pending_changes()
-				.map(|c| c.canon_hash)
-				.collect::<Vec<_>>()
-		);
-
 		// applying the standard change at A should not prune anything
 		// other then the change that was applied
 		authorities
 			.apply_standard_changes("A", 5, &is_descendent_of, false)
 			.unwrap();
-		println!(
-			"pending_changes: {:?}",
-			authorities
-				.pending_changes()
-				.map(|c| c.canon_hash)
-				.collect::<Vec<_>>()
-		);
 
 		assert_eq!(authorities.pending_changes().count(), 6);