grandpa: allow noting that the set has stalled (#6725)

* grandpa: remove unused methods to convert digest

* grandpa: add root extrinsic for scheduling forced change

* grandpa: add benchmark for schedule_forced_change

* grandpa: don't take authority weight in schedule_forced_change

* grandpa: add const for default forced change delay

* grandpa: adjust weights after benchmark on ref hardware

* grandpa: fix cleanup of forced changes on standard change application

* grandpa: replace schedule_forced_change with note_stalled

* grandpa: always trigger a session change when the set is stalled

* grandpa: fix bug on set id mutation after failed scheduled change

* grandpa: take delay as parameter in note_stalled

* grandpa: fix tests

* grandpa: fix cleanup of forced changes

* grandpa: add test for forced changes cleanup

* grandpa: add test for session rotation set id

* grandpa: add test for scheduling of forced changes on new session

Author: andresilva

src/benchmarking.rs

@@ -19,8 +19,9 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use super::*;
+use super::{*, Module as Grandpa};
 use frame_benchmarking::benchmarks;
+use frame_system::RawOrigin;
 use sp_core::H256;
 
 benchmarks! {
@@ -62,6 +63,15 @@ benchmarks! {
 	} verify {
 		assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2));
 	}
+
+	note_stalled {
+		let delay = 1000.into();
+		let best_finalized_block_number = 1.into();
+
+	}: _(RawOrigin::Root, delay, best_finalized_block_number)
+	verify {
+		assert!(Grandpa::<T>::stalled().is_some());
+	}
 }
 
 #[cfg(test)]
@@ -74,6 +84,7 @@ mod tests {
 	fn test_benchmarks() {
 		new_test_ext(vec![(1, 1), (2, 1), (3, 1)]).execute_with(|| {
 			assert_ok!(test_benchmark_check_equivocation_proof::<Test>());
+			assert_ok!(test_benchmark_note_stalled::<Test>());
 		})
 	}
 

src/lib.rs

@@ -43,9 +43,10 @@ use frame_support::{
 	decl_error, decl_event, decl_module, decl_storage, storage, traits::KeyOwnerProofSystem,
 	Parameter,
 };
-use frame_system::{ensure_none, ensure_signed, DigestOf};
+use frame_system::{ensure_none, ensure_root, ensure_signed};
+use pallet_finality_tracker::OnFinalizationStalled;
 use sp_runtime::{
-	generic::{DigestItem, OpaqueDigestItemId},
+	generic::DigestItem,
 	traits::Zero,
 	DispatchResult, KeyTypeId,
 };
@@ -205,7 +206,7 @@ decl_storage! {
 		State get(fn state): StoredState<T::BlockNumber> = StoredState::Live;
 
 		/// Pending change: (signaled at, scheduled change).
-		PendingChange: Option<StoredPendingChange<T::BlockNumber>>;
+		PendingChange get(fn pending_change): Option<StoredPendingChange<T::BlockNumber>>;
 
 		/// next block number where we can force a change.
 		NextForced get(fn next_forced): Option<T::BlockNumber>;
@@ -280,6 +281,24 @@ decl_module! {
 			)?;
 		}
 
+		/// Note that the current authority set of the GRANDPA finality gadget has
+		/// stalled. This will trigger a forced authority set change at the beginning
+		/// of the next session, to be enacted `delay` blocks after that. The delay
+		/// should be high enough to safely assume that the block signalling the
+		/// forced change will not be re-orged (e.g. 1000 blocks). The GRANDPA voters
+		/// will start the new authority set using the given finalized block as base.
+		/// Only callable by root.
+		#[weight = weight_for::note_stalled::<T>()]
+		fn note_stalled(
+			origin,
+			delay: T::BlockNumber,
+			best_finalized_block_number: T::BlockNumber,
+		) {
+			ensure_root(origin)?;
+
+			Self::on_stalled(delay, best_finalized_block_number)
+		}
+
 		fn on_finalize(block_number: T::BlockNumber) {
 			// check for scheduled pending authority set changes
 			if let Some(pending_change) = <PendingChange<T>>::get() {
@@ -295,7 +314,7 @@ decl_module! {
 						))
 					} else {
 						Self::deposit_log(ConsensusLog::ScheduledChange(
-							ScheduledChange{
+							ScheduledChange {
 								delay: pending_change.delay,
 								next_authorities: pending_change.next_authorities.clone(),
 							}
@@ -377,6 +396,11 @@ mod weight_for {
 			// fetching set id -> session index mappings
 			.saturating_add(T::DbWeight::get().reads(2))
 	}
+
+	pub fn note_stalled<T: super::Trait>() -> Weight {
+		(3 * WEIGHT_PER_MICROS)
+			.saturating_add(T::DbWeight::get().writes(1))
+	}
 }
 
 impl<T: Trait> Module<T> {
@@ -580,42 +604,6 @@ impl<T: Trait> Module<T> {
 	}
 }
 
-impl<T: Trait> Module<T> {
-	/// Attempt to extract a GRANDPA log from a generic digest.
-	pub fn grandpa_log(digest: &DigestOf<T>) -> Option<ConsensusLog<T::BlockNumber>> {
-		let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID);
-		digest.convert_first(|l| l.try_to::<ConsensusLog<T::BlockNumber>>(id))
-	}
-
-	/// Attempt to extract a pending set-change signal from a digest.
-	pub fn pending_change(digest: &DigestOf<T>)
-		-> Option<ScheduledChange<T::BlockNumber>>
-	{
-		Self::grandpa_log(digest).and_then(|signal| signal.try_into_change())
-	}
-
-	/// Attempt to extract a forced set-change signal from a digest.
-	pub fn forced_change(digest: &DigestOf<T>)
-		-> Option<(T::BlockNumber, ScheduledChange<T::BlockNumber>)>
-	{
-		Self::grandpa_log(digest).and_then(|signal| signal.try_into_forced_change())
-	}
-
-	/// Attempt to extract a pause signal from a digest.
-	pub fn pending_pause(digest: &DigestOf<T>)
-		-> Option<T::BlockNumber>
-	{
-		Self::grandpa_log(digest).and_then(|signal| signal.try_into_pause())
-	}
-
-	/// Attempt to extract a resume signal from a digest.
-	pub fn pending_resume(digest: &DigestOf<T>)
-		-> Option<T::BlockNumber>
-	{
-		Self::grandpa_log(digest).and_then(|signal| signal.try_into_resume())
-	}
-}
-
 impl<T: Trait> sp_runtime::BoundToRuntimeAppPublic for Module<T> {
 	type Public = AuthorityId;
 }
@@ -638,14 +626,26 @@ impl<T: Trait> pallet_session::OneSessionHandler<T::AccountId> for Module<T>
 		// Always issue a change if `session` says that the validators have changed.
 		// Even if their session keys are the same as before, the underlying economic
 		// identities have changed.
-		let current_set_id = if changed {
+		let current_set_id = if changed || <Stalled<T>>::exists() {
 			let next_authorities = validators.map(|(_, k)| (k, 1)).collect::<Vec<_>>();
-			if let Some((further_wait, median)) = <Stalled<T>>::take() {
-				let _ = Self::schedule_change(next_authorities, further_wait, Some(median));
+
+			let res = if let Some((further_wait, median)) = <Stalled<T>>::take() {
+				Self::schedule_change(next_authorities, further_wait, Some(median))
+			} else {
+				Self::schedule_change(next_authorities, Zero::zero(), None)
+			};
+
+			if res.is_ok() {
+				CurrentSetId::mutate(|s| {
+					*s += 1;
+					*s
+				})
 			} else {
-				let _ = Self::schedule_change(next_authorities, Zero::zero(), None);
+				// either the session module signalled that the validators have changed
+				// or the set was stalled. but since we didn't successfully schedule
+				// an authority set change we do not increment the set id.
+				Self::current_set_id()
 			}
-			CurrentSetId::mutate(|s| { *s += 1; *s })
 		} else {
 			// nothing's changed, neither economic conditions nor session keys. update the pointer
 			// of the current set.
@@ -663,7 +663,7 @@ impl<T: Trait> pallet_session::OneSessionHandler<T::AccountId> for Module<T>
 	}
 }
 
-impl<T: Trait> pallet_finality_tracker::OnFinalizationStalled<T::BlockNumber> for Module<T> {
+impl<T: Trait> OnFinalizationStalled<T::BlockNumber> for Module<T> {
 	fn on_stalled(further_wait: T::BlockNumber, median: T::BlockNumber) {
 		// when we record old authority sets, we can use `pallet_finality_tracker::median`
 		// to figure out _who_ failed. until then, we can't meaningfully guard

src/mock.rs

@@ -365,23 +365,18 @@ pub fn new_test_ext_raw_authorities(authorities: AuthorityList) -> sp_io::TestEx
 }
 
 pub fn start_session(session_index: SessionIndex) {
-	let mut parent_hash = System::parent_hash();
-
 	for i in Session::current_index()..session_index {
+		System::on_finalize(System::block_number());
+		Session::on_finalize(System::block_number());
 		Staking::on_finalize(System::block_number());
-		System::set_block_number((i + 1).into());
-		Timestamp::set_timestamp(System::block_number() * 6000);
+		Grandpa::on_finalize(System::block_number());
 
-		// In order to be able to use `System::parent_hash()` in the tests
-		// we need to first get it via `System::finalize` and then set it
-		// the `System::initialize`. However, it is needed to be taken into
-		// consideration that finalizing will prune some data in `System`
-		// storage including old values `BlockHash` if that reaches above
-		// `BlockHashCount` capacity.
-		if System::block_number() > 1 {
+		let parent_hash = if System::block_number() > 1 {
 			let hdr = System::finalize();
-			parent_hash = hdr.hash();
-		}
+			hdr.hash()
+		} else {
+			System::parent_hash()
+		};
 
 		System::initialize(
 			&(i as u64 + 1),
@@ -390,9 +385,13 @@ pub fn start_session(session_index: SessionIndex) {
 			&Default::default(),
 			Default::default(),
 		);
+		System::set_block_number((i + 1).into());
+		Timestamp::set_timestamp(System::block_number() * 6000);
 
-		Session::on_initialize(System::block_number());
 		System::on_initialize(System::block_number());
+		Session::on_initialize(System::block_number());
+		Staking::on_initialize(System::block_number());
+		Grandpa::on_initialize(System::block_number());
 	}
 
 	assert_eq!(Session::current_index(), session_index);