From 4cbfb1fb2e2c745e2797a741cbdf51c1cae53f0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 17:33:45 +0100 Subject: [PATCH 01/16] grandpa: remove unused methods to convert digest --- frame/grandpa/src/lib.rs | 36 ------------------------------------ 1 file changed, 36 deletions(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index c903e081e7260..a97133fd7c3c1 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -580,42 +580,6 @@ impl Module { } } -impl Module { - /// Attempt to extract a GRANDPA log from a generic digest. - pub fn grandpa_log(digest: &DigestOf) -> Option> { - let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID); - digest.convert_first(|l| l.try_to::>(id)) - } - - /// Attempt to extract a pending set-change signal from a digest. - pub fn pending_change(digest: &DigestOf) - -> Option> - { - 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) - -> Option<(T::BlockNumber, ScheduledChange)> - { - 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) - -> Option - { - 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) - -> Option - { - Self::grandpa_log(digest).and_then(|signal| signal.try_into_resume()) - } -} - impl sp_runtime::BoundToRuntimeAppPublic for Module { type Public = AuthorityId; } From b19cbb220d317672cb89ae27f37868252e604a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 17:35:21 +0100 Subject: [PATCH 02/16] grandpa: add root extrinsic for scheduling forced change --- frame/grandpa/src/lib.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index a97133fd7c3c1..ea9d025500478 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -43,9 +43,9 @@ 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 sp_runtime::{ - generic::{DigestItem, OpaqueDigestItemId}, + generic::DigestItem, traits::Zero, DispatchResult, KeyTypeId, }; @@ -205,7 +205,7 @@ decl_storage! { State get(fn state): StoredState = StoredState::Live; /// Pending change: (signaled at, scheduled change). - PendingChange: Option>; + PendingChange get(fn pending_change): Option>; /// next block number where we can force a change. NextForced get(fn next_forced): Option; @@ -280,6 +280,21 @@ decl_module! { )?; } + #[weight = weight_for::schedule_forced_change::(next_authorities.len())] + fn schedule_forced_change( + origin, + next_authorities: AuthorityList, + best_finalized_block_number: T::BlockNumber, + ) { + ensure_root(origin)?; + + Self::schedule_change( + next_authorities, + 1000.into(), + Some(best_finalized_block_number), + )?; + } + fn on_finalize(block_number: T::BlockNumber) { // check for scheduled pending authority set changes if let Some(pending_change) = >::get() { @@ -295,7 +310,7 @@ decl_module! { )) } else { Self::deposit_log(ConsensusLog::ScheduledChange( - ScheduledChange{ + ScheduledChange { delay: pending_change.delay, next_authorities: pending_change.next_authorities.clone(), } @@ -377,6 +392,13 @@ mod weight_for { // fetching set id -> session index mappings .saturating_add(T::DbWeight::get().reads(2)) } + + pub fn schedule_forced_change(next_authorities: usize) -> Weight { + (20 * WEIGHT_PER_MICROS) + .saturating_add((50 * WEIGHT_PER_NANOS).saturating_mul(next_authorities as u64)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(2)) + } } impl Module { From 1c3430af578f96943eb261dc69dfa5801bd5359c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 17:35:40 +0100 Subject: [PATCH 03/16] grandpa: add benchmark for schedule_forced_change --- frame/grandpa/src/benchmarking.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index 18f6f62fa44ed..44469cecf2f99 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -19,8 +19,10 @@ #![cfg_attr(not(feature = "std"), no_std)] -use super::*; +use super::{*, Module as Grandpa}; use frame_benchmarking::benchmarks; +use frame_system::RawOrigin; +use sp_application_crypto::Public; use sp_core::H256; benchmarks! { @@ -62,6 +64,21 @@ benchmarks! { } verify { assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2)); } + + schedule_forced_change { + let n in 0 .. 1000; + + let next_authorities = (0..n) + .map(|n| (AuthorityId::from_slice(&n.to_be_bytes().repeat(8)), 1)) + .collect(); + + let best_finalized_block_number = 1.into(); + + }: _(RawOrigin::Root, next_authorities, best_finalized_block_number) + verify { + let pending_change = Grandpa::::pending_change().unwrap(); + assert!(pending_change.forced.is_some()) + } } #[cfg(test)] @@ -74,6 +91,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::()); + assert_ok!(test_benchmark_schedule_forced_change::()); }) } From a5481985fdff8afa3077a985d00feefe5b230786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 18:21:50 +0100 Subject: [PATCH 04/16] grandpa: don't take authority weight in schedule_forced_change --- frame/grandpa/src/benchmarking.rs | 2 +- frame/grandpa/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index 44469cecf2f99..59cc0accf4387 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -69,7 +69,7 @@ benchmarks! { let n in 0 .. 1000; let next_authorities = (0..n) - .map(|n| (AuthorityId::from_slice(&n.to_be_bytes().repeat(8)), 1)) + .map(|n| AuthorityId::from_slice(&n.to_be_bytes().repeat(8))) .collect(); let best_finalized_block_number = 1.into(); diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index ea9d025500478..ee2f2c3f528ed 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -283,13 +283,13 @@ decl_module! { #[weight = weight_for::schedule_forced_change::(next_authorities.len())] fn schedule_forced_change( origin, - next_authorities: AuthorityList, + next_authorities: Vec, best_finalized_block_number: T::BlockNumber, ) { ensure_root(origin)?; Self::schedule_change( - next_authorities, + next_authorities.into_iter().map(|k| (k, 1)).collect(), 1000.into(), Some(best_finalized_block_number), )?; From 4f9a6e3acf6b8cbd6bafa173efd540118b298d31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 18:24:45 +0100 Subject: [PATCH 05/16] grandpa: add const for default forced change delay --- frame/grandpa/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index ee2f2c3f528ed..57b5a82701995 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -66,6 +66,10 @@ pub use equivocation::{ HandleEquivocation, }; +/// The default number of blocks a forced change is delayed by, +/// i.e. it will be activated at `block_signal_number + delay`. +pub const FORCED_CHANGE_DELAY: u32 = 1000; + pub trait Trait: frame_system::Trait { /// The event type of this module. type Event: From + Into<::Event>; @@ -290,7 +294,7 @@ decl_module! { Self::schedule_change( next_authorities.into_iter().map(|k| (k, 1)).collect(), - 1000.into(), + FORCED_CHANGE_DELAY.into(), Some(best_finalized_block_number), )?; } From 704432889722d308569a59865284e03bac79216a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 18:26:11 +0100 Subject: [PATCH 06/16] grandpa: adjust weights after benchmark on ref hardware --- frame/grandpa/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index 57b5a82701995..5499a445278b5 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -398,8 +398,8 @@ mod weight_for { } pub fn schedule_forced_change(next_authorities: usize) -> Weight { - (20 * WEIGHT_PER_MICROS) - .saturating_add((50 * WEIGHT_PER_NANOS).saturating_mul(next_authorities as u64)) + (18 * WEIGHT_PER_MICROS) + .saturating_add((55 * WEIGHT_PER_NANOS).saturating_mul(next_authorities as u64)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(2)) } From b345df6e6df6d79c39f0e4fb4549525bc0e28ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 23 Jul 2020 18:53:03 +0100 Subject: [PATCH 07/16] grandpa: fix cleanup of forced changes on standard change application --- client/finality-grandpa/src/authorities.rs | 25 +++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index b4cb254864df7..7ee2e58a5bef6 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -423,9 +423,28 @@ where fork_tree::FinalizationResult::Changed(change) => { status.changed = true; - // if we are able to finalize any standard change then we can - // discard all pending forced changes (on different forks) - self.pending_forced_changes.clear(); + // NOTE: the grandpa runtime module guarantees that there are no + // overlaping pending changes emitted, either forced or standard, + // and taking into account the enactment delay. therefore, an + // invariant of this module is that there is always at most one + // forced change per fork (the first must have been enacted before + // the next one is emitted). + let mut pending_forced_change = None; + for change in &self.pending_forced_changes { + // we will keep a forced change for this fork, which must be for + // a later block otherwise it would have been enacted already, + // and must be a descendent of the finalized block. + if change.effective_number() > finalized_number && + is_descendent_of(&finalized_hash, &change.canon_hash) + .map_err(fork_tree::Error::Client)? + { + pending_forced_change = Some(change.clone()); + break; + } + } + + // collect the single forced change into a vec (if any) + self.pending_forced_changes = pending_forced_change.into_iter().collect(); if let Some(change) = change { afg_log!(initial_sync, From b69b5630586bfa98c08a3dc89fefde78ddcb7063 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 00:41:17 +0100 Subject: [PATCH 08/16] grandpa: replace schedule_forced_change with note_stalled --- frame/grandpa/src/benchmarking.rs | 16 ++++------------ frame/grandpa/src/lib.rs | 26 ++++++++++++-------------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index 59cc0accf4387..cacfcd1e06587 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -22,7 +22,6 @@ use super::{*, Module as Grandpa}; use frame_benchmarking::benchmarks; use frame_system::RawOrigin; -use sp_application_crypto::Public; use sp_core::H256; benchmarks! { @@ -65,19 +64,12 @@ benchmarks! { assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2)); } - schedule_forced_change { - let n in 0 .. 1000; - - let next_authorities = (0..n) - .map(|n| AuthorityId::from_slice(&n.to_be_bytes().repeat(8))) - .collect(); - + note_stalled { let best_finalized_block_number = 1.into(); - }: _(RawOrigin::Root, next_authorities, best_finalized_block_number) + }: _(RawOrigin::Root, best_finalized_block_number) verify { - let pending_change = Grandpa::::pending_change().unwrap(); - assert!(pending_change.forced.is_some()) + assert!(Grandpa::::stalled().is_some()); } } @@ -91,7 +83,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::()); - assert_ok!(test_benchmark_schedule_forced_change::()); + assert_ok!(test_benchmark_note_stalled::()); }) } diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index 5499a445278b5..6f9205ba16b05 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -44,6 +44,7 @@ use frame_support::{ Parameter, }; use frame_system::{ensure_none, ensure_root, ensure_signed}; +use pallet_finality_tracker::OnFinalizationStalled; use sp_runtime::{ generic::DigestItem, traits::Zero, @@ -284,19 +285,18 @@ decl_module! { )?; } - #[weight = weight_for::schedule_forced_change::(next_authorities.len())] - fn schedule_forced_change( + /// Note that the current authority set of the GRANDPA finality gadget + /// has stalled. This will trigger a forced authority set change after + /// 1000 blocks, and 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::()] + fn note_stalled( origin, - next_authorities: Vec, best_finalized_block_number: T::BlockNumber, ) { ensure_root(origin)?; - Self::schedule_change( - next_authorities.into_iter().map(|k| (k, 1)).collect(), - FORCED_CHANGE_DELAY.into(), - Some(best_finalized_block_number), - )?; + Self::on_stalled(FORCED_CHANGE_DELAY.into(), best_finalized_block_number) } fn on_finalize(block_number: T::BlockNumber) { @@ -397,11 +397,9 @@ mod weight_for { .saturating_add(T::DbWeight::get().reads(2)) } - pub fn schedule_forced_change(next_authorities: usize) -> Weight { - (18 * WEIGHT_PER_MICROS) - .saturating_add((55 * WEIGHT_PER_NANOS).saturating_mul(next_authorities as u64)) - .saturating_add(T::DbWeight::get().reads(2)) - .saturating_add(T::DbWeight::get().writes(2)) + pub fn note_stalled() -> Weight { + (3 * WEIGHT_PER_MICROS) + .saturating_add(T::DbWeight::get().writes(1)) } } @@ -653,7 +651,7 @@ impl pallet_session::OneSessionHandler for Module } } -impl pallet_finality_tracker::OnFinalizationStalled for Module { +impl OnFinalizationStalled for Module { 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 From 7c8133b43324b877d34482c8272a8558178fb5cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 00:41:58 +0100 Subject: [PATCH 09/16] grandpa: always trigger a session change when the set is stalled --- frame/grandpa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index 6f9205ba16b05..d2ce33c30c6e7 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -626,7 +626,7 @@ impl pallet_session::OneSessionHandler for Module // 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 || >::exists() { let next_authorities = validators.map(|(_, k)| (k, 1)).collect::>(); if let Some((further_wait, median)) = >::take() { let _ = Self::schedule_change(next_authorities, further_wait, Some(median)); From dee2e266ed1fcfaada78a3ec2a5590a5677c2a78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 00:49:25 +0100 Subject: [PATCH 10/16] grandpa: fix bug on set id mutation after failed scheduled change --- frame/grandpa/src/lib.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index d2ce33c30c6e7..ddc7d9cdf6546 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -628,12 +628,24 @@ impl pallet_session::OneSessionHandler for Module // identities have changed. let current_set_id = if changed || >::exists() { let next_authorities = validators.map(|(_, k)| (k, 1)).collect::>(); - if let Some((further_wait, median)) = >::take() { - let _ = Self::schedule_change(next_authorities, further_wait, Some(median)); + + let res = if let Some((further_wait, median)) = >::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. From b13f6b509509b6a85a8ccf44a4db89d6102303b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 12:09:57 +0100 Subject: [PATCH 11/16] grandpa: take delay as parameter in note_stalled --- frame/grandpa/src/lib.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index ddc7d9cdf6546..961c099460752 100644 --- a/frame/grandpa/src/lib.rs +++ b/frame/grandpa/src/lib.rs @@ -67,10 +67,6 @@ pub use equivocation::{ HandleEquivocation, }; -/// The default number of blocks a forced change is delayed by, -/// i.e. it will be activated at `block_signal_number + delay`. -pub const FORCED_CHANGE_DELAY: u32 = 1000; - pub trait Trait: frame_system::Trait { /// The event type of this module. type Event: From + Into<::Event>; @@ -285,18 +281,22 @@ decl_module! { )?; } - /// Note that the current authority set of the GRANDPA finality gadget - /// has stalled. This will trigger a forced authority set change after - /// 1000 blocks, and the GRANDPA voters will start the new authority set - /// using the given finalized block as base. Only callable by root. + /// 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::()] fn note_stalled( origin, + delay: T::BlockNumber, best_finalized_block_number: T::BlockNumber, ) { ensure_root(origin)?; - Self::on_stalled(FORCED_CHANGE_DELAY.into(), best_finalized_block_number) + Self::on_stalled(delay, best_finalized_block_number) } fn on_finalize(block_number: T::BlockNumber) { From 41cf58180ca10a554fd76d7acabb33a25c9b673b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 12:09:31 +0100 Subject: [PATCH 12/16] grandpa: fix tests --- frame/grandpa/src/benchmarking.rs | 3 +- frame/grandpa/src/mock.rs | 27 +++++++------- frame/grandpa/src/tests.rs | 59 +++++++++++++++++++------------ 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index cacfcd1e06587..048f99fff7a9b 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -65,9 +65,10 @@ benchmarks! { } note_stalled { + let delay = 1000.into(); let best_finalized_block_number = 1.into(); - }: _(RawOrigin::Root, best_finalized_block_number) + }: _(RawOrigin::Root, delay, best_finalized_block_number) verify { assert!(Grandpa::::stalled().is_some()); } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 6291a2f82f102..684712df7d078 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/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); diff --git a/frame/grandpa/src/tests.rs b/frame/grandpa/src/tests.rs index f4b353c0fa0c6..c5cc2d0f4bf9a 100644 --- a/frame/grandpa/src/tests.rs +++ b/frame/grandpa/src/tests.rs @@ -342,14 +342,15 @@ fn report_equivocation_current_set_works() { start_era(1); let authorities = Grandpa::grandpa_authorities(); + let validators = Session::validators(); - // make sure that all authorities have the same balance - for i in 0..authorities.len() { - assert_eq!(Balances::total_balance(&(i as u64)), 10_000_000); - assert_eq!(Staking::slashable_balance_of(&(i as u64)), 10_000); + // make sure that all validators have the same balance + for validator in &validators { + assert_eq!(Balances::total_balance(validator), 10_000_000); + assert_eq!(Staking::slashable_balance_of(validator), 10_000); assert_eq!( - Staking::eras_stakers(1, i as u64), + Staking::eras_stakers(1, validator), pallet_staking::Exposure { total: 10_000, own: 10_000, @@ -388,11 +389,12 @@ fn report_equivocation_current_set_works() { start_era(2); // check that the balance of 0-th validator is slashed 100%. - assert_eq!(Balances::total_balance(&0), 10_000_000 - 10_000); - assert_eq!(Staking::slashable_balance_of(&0), 0); + let equivocation_validator_id = validators[equivocation_authority_index]; + assert_eq!(Balances::total_balance(&equivocation_validator_id), 10_000_000 - 10_000); + assert_eq!(Staking::slashable_balance_of(&equivocation_validator_id), 0); assert_eq!( - Staking::eras_stakers(2, 0), + Staking::eras_stakers(2, equivocation_validator_id), pallet_staking::Exposure { total: 0, own: 0, @@ -401,12 +403,16 @@ fn report_equivocation_current_set_works() { ); // check that the balances of all other validators are left intact. - for i in 1..authorities.len() { - assert_eq!(Balances::total_balance(&(i as u64)), 10_000_000); - assert_eq!(Staking::slashable_balance_of(&(i as u64)), 10_000); + for validator in &validators { + if *validator == equivocation_validator_id { + continue; + } + + assert_eq!(Balances::total_balance(validator), 10_000_000); + assert_eq!(Staking::slashable_balance_of(validator), 10_000); assert_eq!( - Staking::eras_stakers(2, i as u64), + Staking::eras_stakers(2, validator), pallet_staking::Exposure { total: 10_000, own: 10_000, @@ -425,6 +431,7 @@ fn report_equivocation_old_set_works() { start_era(1); let authorities = Grandpa::grandpa_authorities(); + let validators = Session::validators(); let equivocation_authority_index = 0; let equivocation_key = &authorities[equivocation_authority_index].0; @@ -436,12 +443,12 @@ fn report_equivocation_old_set_works() { start_era(2); // make sure that all authorities have the same balance - for i in 0..authorities.len() { - assert_eq!(Balances::total_balance(&(i as u64)), 10_000_000); - assert_eq!(Staking::slashable_balance_of(&(i as u64)), 10_000); + for validator in &validators { + assert_eq!(Balances::total_balance(validator), 10_000_000); + assert_eq!(Staking::slashable_balance_of(validator), 10_000); assert_eq!( - Staking::eras_stakers(2, i as u64), + Staking::eras_stakers(2, validator), pallet_staking::Exposure { total: 10_000, own: 10_000, @@ -474,11 +481,13 @@ fn report_equivocation_old_set_works() { start_era(3); // check that the balance of 0-th validator is slashed 100%. - assert_eq!(Balances::total_balance(&0), 10_000_000 - 10_000); - assert_eq!(Staking::slashable_balance_of(&0), 0); + let equivocation_validator_id = validators[equivocation_authority_index]; + + assert_eq!(Balances::total_balance(&equivocation_validator_id), 10_000_000 - 10_000); + assert_eq!(Staking::slashable_balance_of(&equivocation_validator_id), 0); assert_eq!( - Staking::eras_stakers(3, 0), + Staking::eras_stakers(3, equivocation_validator_id), pallet_staking::Exposure { total: 0, own: 0, @@ -487,12 +496,16 @@ fn report_equivocation_old_set_works() { ); // check that the balances of all other validators are left intact. - for i in 1..authorities.len() { - assert_eq!(Balances::total_balance(&(i as u64)), 10_000_000); - assert_eq!(Staking::slashable_balance_of(&(i as u64)), 10_000); + for validator in &validators { + if *validator == equivocation_validator_id { + continue; + } + + assert_eq!(Balances::total_balance(validator), 10_000_000); + assert_eq!(Staking::slashable_balance_of(validator), 10_000); assert_eq!( - Staking::eras_stakers(3, i as u64), + Staking::eras_stakers(3, validator), pallet_staking::Exposure { total: 10_000, own: 10_000, From d0f3bede8a1bed241e9bb7087f734592cb019f61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 16:31:38 +0100 Subject: [PATCH 13/16] grandpa: fix cleanup of forced changes --- client/finality-grandpa/src/authorities.rs | 25 ++++++++-------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 7ee2e58a5bef6..c41f3b2b1fc94 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -423,29 +423,22 @@ where fork_tree::FinalizationResult::Changed(change) => { status.changed = true; - // NOTE: the grandpa runtime module guarantees that there are no - // overlaping pending changes emitted, either forced or standard, - // and taking into account the enactment delay. therefore, an - // invariant of this module is that there is always at most one - // forced change per fork (the first must have been enacted before - // the next one is emitted). - let mut pending_forced_change = None; - for change in &self.pending_forced_changes { - // we will keep a forced change for this fork, which must be for - // a later block otherwise it would have been enacted already, - // and must be a descendent of the finalized block. + let pending_forced_changes = std::mem::replace( + &mut self.pending_forced_changes, + Vec::new(), + ); + + for change in pending_forced_changes { + // 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). if change.effective_number() > finalized_number && is_descendent_of(&finalized_hash, &change.canon_hash) .map_err(fork_tree::Error::Client)? { - pending_forced_change = Some(change.clone()); - break; + self.pending_forced_changes.push(change) } } - // collect the single forced change into a vec (if any) - self.pending_forced_changes = pending_forced_change.into_iter().collect(); - if let Some(change) = change { afg_log!(initial_sync, "👴 Applying authority set change scheduled at block #{:?}", From e988f1b699787b9ef43fc6df3a803931945e6fd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 16:37:27 +0100 Subject: [PATCH 14/16] grandpa: add test for forced changes cleanup --- client/finality-grandpa/src/authorities.rs | 124 ++++++++++++++++++++- 1 file changed, 122 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index c41f3b2b1fc94..117f5cad5e3d2 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -428,9 +428,9 @@ 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). for change in pending_forced_changes { - // 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). if change.effective_number() > finalized_number && is_descendent_of(&finalized_hash, &change.canon_hash) .map_err(fork_tree::Error::Client)? @@ -1175,4 +1175,124 @@ mod tests { Err(Error::InvalidAuthoritySet) )); } + + #[test] + fn cleans_up_stale_forced_changes_when_applying_standard_change() { + let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; + + let mut authorities = AuthoritySet { + current_authorities: current_authorities.clone(), + set_id: 0, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }; + + let new_set = current_authorities.clone(); + + // Create the following pending changes tree: + // + // [#C3] + // / + // /- (#C2) + // / + // (#A) - (#B) - [#C1] + // \ + // (#C0) - [#D] + // + // () - Standard change + // [] - Forced change + + let is_descendent_of = { + let hashes = vec!["B", "C0", "C1", "C2", "C3", "D"]; + is_descendent_of(move |base, hash| match (*base, *hash) { + ("B", "B") => false, // required to have the simpler case below + ("A", b) | ("B", b) => hashes.iter().any(|h| *h == b), + ("C0", "D") => true, + _ => false, + }) + }; + + let mut add_pending_change = |canon_height, canon_hash, forced| { + let change = PendingChange { + next_authorities: new_set.clone(), + delay: 0, + canon_height, + canon_hash, + delay_kind: if forced { + DelayKind::Best { + median_last_finalized: 0, + } + } else { + DelayKind::Finalized + }, + }; + + authorities + .add_pending_change(change, &is_descendent_of) + .unwrap(); + }; + + add_pending_change(5, "A", false); + add_pending_change(10, "B", false); + add_pending_change(15, "C0", false); + add_pending_change(15, "C1", true); + add_pending_change(15, "C2", false); + add_pending_change(15, "C3", true); + add_pending_change(20, "D", true); + + println!( + "pending_changes: {:?}", + authorities + .pending_changes() + .map(|c| c.canon_hash) + .collect::>() + ); + + // 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::>() + ); + + assert_eq!(authorities.pending_changes().count(), 6); + + // same for B + authorities + .apply_standard_changes("B", 10, &is_descendent_of, false) + .unwrap(); + + assert_eq!(authorities.pending_changes().count(), 5); + + let authorities2 = authorities.clone(); + + // finalizing C2 should clear all forced changes + authorities + .apply_standard_changes("C2", 15, &is_descendent_of, false) + .unwrap(); + + assert_eq!(authorities.pending_forced_changes.len(), 0); + + // finalizing C0 should clear all forced changes but D + let mut authorities = authorities2; + authorities + .apply_standard_changes("C0", 15, &is_descendent_of, false) + .unwrap(); + + assert_eq!(authorities.pending_forced_changes.len(), 1); + assert_eq!( + authorities + .pending_forced_changes + .first() + .unwrap() + .canon_hash, + "D" + ); + } } From 8c737b82dab2fa0cc593f68d0f5dbc8c54dcda28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 17:08:43 +0100 Subject: [PATCH 15/16] grandpa: add test for session rotation set id --- frame/grandpa/src/tests.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/frame/grandpa/src/tests.rs b/frame/grandpa/src/tests.rs index c5cc2d0f4bf9a..45d5d4c00f64b 100644 --- a/frame/grandpa/src/tests.rs +++ b/frame/grandpa/src/tests.rs @@ -780,3 +780,40 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() { ); }); } + +#[test] +fn on_new_session_doesnt_start_new_set_if_schedule_change_failed() { + use pallet_session::OneSessionHandler; + + new_test_ext(vec![(1, 1), (2, 1), (3, 1)]).execute_with(|| { + assert_eq!(Grandpa::current_set_id(), 0); + + // starting a new era should lead to a change in the session + // validators and trigger a new set + start_era(1); + assert_eq!(Grandpa::current_set_id(), 1); + + // we schedule a change delayed by 2 blocks, this should make it so that + // when we try to rotate the session at the beginning of the era we will + // fail to schedule a change (there's already one pending), so we should + // not increment the set id. + Grandpa::schedule_change(to_authorities(vec![(1, 1)]), 2, None); + start_era(2); + assert_eq!(Grandpa::current_set_id(), 1); + + // everything should go back to normal after. + start_era(3); + assert_eq!(Grandpa::current_set_id(), 2); + + // session rotation might also fail to schedule a change if it's for a + // forced change (i.e. grandpa is stalled) and it is too soon. + >::put(1000); + >::put((30, 1)); + + // NOTE: we cannot go through normal era rotation since having `Stalled` + // defined will also trigger a new set (regardless of whether the + // session validators changed) + Grandpa::on_new_session(true, std::iter::empty(), std::iter::empty()); + assert_eq!(Grandpa::current_set_id(), 2); + }); +} From 7b0c4b2e5eef4f16e685a0be005033c7aed1931e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 24 Jul 2020 17:18:37 +0100 Subject: [PATCH 16/16] grandpa: add test for scheduling of forced changes on new session --- frame/grandpa/src/tests.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/frame/grandpa/src/tests.rs b/frame/grandpa/src/tests.rs index 45d5d4c00f64b..9eca2cc381371 100644 --- a/frame/grandpa/src/tests.rs +++ b/frame/grandpa/src/tests.rs @@ -28,6 +28,7 @@ use frame_support::{ traits::{Currency, OnFinalize}, }; use frame_system::{EventRecord, Phase}; +use pallet_session::OneSessionHandler; use sp_core::H256; use sp_keyring::Ed25519Keyring; use sp_runtime::testing::Digest; @@ -783,8 +784,6 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() { #[test] fn on_new_session_doesnt_start_new_set_if_schedule_change_failed() { - use pallet_session::OneSessionHandler; - new_test_ext(vec![(1, 1), (2, 1), (3, 1)]).execute_with(|| { assert_eq!(Grandpa::current_set_id(), 0); @@ -797,7 +796,7 @@ fn on_new_session_doesnt_start_new_set_if_schedule_change_failed() { // when we try to rotate the session at the beginning of the era we will // fail to schedule a change (there's already one pending), so we should // not increment the set id. - Grandpa::schedule_change(to_authorities(vec![(1, 1)]), 2, None); + Grandpa::schedule_change(to_authorities(vec![(1, 1)]), 2, None).unwrap(); start_era(2); assert_eq!(Grandpa::current_set_id(), 1); @@ -817,3 +816,29 @@ fn on_new_session_doesnt_start_new_set_if_schedule_change_failed() { assert_eq!(Grandpa::current_set_id(), 2); }); } + +#[test] +fn always_schedules_a_change_on_new_session_when_stalled() { + new_test_ext(vec![(1, 1), (2, 1), (3, 1)]).execute_with(|| { + start_era(1); + + assert!(Grandpa::pending_change().is_none()); + assert_eq!(Grandpa::current_set_id(), 1); + + // if the session handler reports no change then we should not schedule + // any pending change + Grandpa::on_new_session(false, std::iter::empty(), std::iter::empty()); + + assert!(Grandpa::pending_change().is_none()); + assert_eq!(Grandpa::current_set_id(), 1); + + // if grandpa is stalled then we should **always** schedule a forced + // change on a new session + >::put((10, 1)); + Grandpa::on_new_session(false, std::iter::empty(), std::iter::empty()); + + assert!(Grandpa::pending_change().is_some()); + assert!(Grandpa::pending_change().unwrap().forced.is_some()); + assert_eq!(Grandpa::current_set_id(), 2); + }); +}