GRANDPA links set IDs to sessions. (#3472)

* introduce some type aliases for round and set-id

* overhaul session "changed" flag and document better

* do_initialize in BABE when getting new session

* grandpa module tracks set IDs

* update runtime versions

* doc comment -> comment

* Include docs fixes from Gav

Co-Authored-By: Gavin Wood <gavin@parity.io>

* some more review changes

* fix srml-grandpa compilation

substrate/srml/babe/src/lib.rs

@@ -445,6 +445,8 @@ impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
 	fn on_new_session<'a, I: 'a>(_changed: bool, validators: I, queued_validators: I)
 		where I: Iterator<Item=(&'a T::AccountId, AuthorityId)>
 	{
+		Self::do_initialize();
+
 		// Update epoch index
 		let epoch_index = EpochIndex::get()
 			.checked_add(1)

substrate/srml/grandpa/src/lib.rs

@@ -33,7 +33,8 @@ pub use substrate_finality_grandpa_primitives as fg_primitives;
 use rstd::prelude::*;
 use codec::{self as codec, Encode, Decode, Error};
 use srml_support::{
-	decl_event, decl_storage, decl_module, dispatch::Result, storage::StorageValue
+	decl_event, decl_storage, decl_module, dispatch::Result,
+	storage::StorageValue, storage::StorageMap,
 };
 use sr_primitives::{
 	generic::{DigestItem, OpaqueDigestItemId}, traits::Zero,
@@ -43,7 +44,7 @@ use sr_staking_primitives::{
 	SessionIndex,
 	offence::{Offence, Kind},
 };
-use fg_primitives::{ScheduledChange, ConsensusLog, GRANDPA_ENGINE_ID};
+use fg_primitives::{GRANDPA_ENGINE_ID, ScheduledChange, ConsensusLog, SetId, RoundNumber};
 pub use fg_primitives::{AuthorityId, AuthorityWeight};
 use system::{ensure_signed, DigestOf};
 
@@ -65,7 +66,7 @@ pub struct OldStoredPendingChange<N> {
 	/// The delay in blocks until it will be applied.
 	pub delay: N,
 	/// The next authority set.
-	pub next_authorities: Vec<(AuthorityId, u64)>,
+	pub next_authorities: Vec<(AuthorityId, AuthorityWeight)>,
 }
 
 /// A stored pending change.
@@ -76,7 +77,7 @@ pub struct StoredPendingChange<N> {
 	/// The delay in blocks until it will be applied.
 	pub delay: N,
 	/// The next authority set.
-	pub next_authorities: Vec<(AuthorityId, u64)>,
+	pub next_authorities: Vec<(AuthorityId, AuthorityWeight)>,
 	/// If defined it means the change was forced and the given block number
 	/// indicates the median last finalized block when the change was signaled.
 	pub forced: Option<N>,
@@ -127,7 +128,7 @@ pub enum StoredState<N> {
 decl_event!(
 	pub enum Event {
 		/// New authority set has been applied.
-		NewAuthorities(Vec<(AuthorityId, u64)>),
+		NewAuthorities(Vec<(AuthorityId, AuthorityWeight)>),
 		/// Current authority set has been paused.
 		Paused,
 		/// Current authority set has been resumed.
@@ -151,6 +152,13 @@ decl_storage! {
 
 		/// `true` if we are currently stalled.
 		Stalled get(stalled): Option<(T::BlockNumber, T::BlockNumber)>;
+
+		/// The number of changes (both in terms of keys and underlying economic responsibilities)
+		/// in the "set" of Grandpa validators from genesis.
+		CurrentSetId get(current_set_id) build(|_| fg_primitives::SetId::default()): SetId;
+
+		/// A mapping from grandpa set ID to the index of the *most recent* session for which its members were responsible.
+		SetIdSession get(session_for_set): map SetId => Option<SessionIndex>;
 	}
 	add_extra_genesis {
 		config(authorities): Vec<(AuthorityId, AuthorityWeight)>;
@@ -243,7 +251,7 @@ decl_module! {
 
 impl<T: Trait> Module<T> {
 	/// Get the current set of authorities, along with their respective weights.
-	pub fn grandpa_authorities() -> Vec<(AuthorityId, u64)> {
+	pub fn grandpa_authorities() -> Vec<(AuthorityId, AuthorityWeight)> {
 		Authorities::get()
 	}
 
@@ -292,7 +300,7 @@ impl<T: Trait> Module<T> {
 	/// No change should be signaled while any change is pending. Returns
 	/// an error if a change is already pending.
 	pub fn schedule_change(
-		next_authorities: Vec<(AuthorityId, u64)>,
+		next_authorities: Vec<(AuthorityId, AuthorityWeight)>,
 		in_blocks: T::BlockNumber,
 		forced: Option<T::BlockNumber>,
 	) -> Result {
@@ -337,29 +345,34 @@ 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>
 	{
@@ -367,7 +380,9 @@ impl<T: Trait> Module<T> {
 	}
 }
 
-impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
+impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T>
+	where T: session::Trait
+{
 	type Key = AuthorityId;
 
 	fn on_genesis_session<'a, I: 'a>(validators: I)
@@ -380,18 +395,27 @@ impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
 	fn on_new_session<'a, I: 'a>(changed: bool, validators: I, _queued_validators: I)
 		where I: Iterator<Item=(&'a T::AccountId, AuthorityId)>
 	{
-		// instant changes
-		if changed {
+		// Always issue a change if `session` says that the validators have changed.
+		// Even if their session keys are the same as before, the underyling economic
+		// identities have changed.
+		let current_set_id = if changed {
 			let next_authorities = validators.map(|(_, k)| (k, 1)).collect::<Vec<_>>();
-			let last_authorities = <Module<T>>::grandpa_authorities();
-			if next_authorities != last_authorities {
-				if let Some((further_wait, median)) = <Stalled<T>>::take() {
-					let _ = Self::schedule_change(next_authorities, further_wait, Some(median));
-				} else {
-					let _ = Self::schedule_change(next_authorities, Zero::zero(), None);
-				}
+			if let Some((further_wait, median)) = <Stalled<T>>::take() {
+				let _ = Self::schedule_change(next_authorities, further_wait, Some(median));
+			} else {
+				let _ = Self::schedule_change(next_authorities, Zero::zero(), None);
 			}
-		}
+			CurrentSetId::mutate(|s| { *s += 1; *s })
+		} else {
+			// nothing's changed, neither economic conditions nor session keys. update the pointer
+			// of the current set.
+			Self::current_set_id()
+		};
+
+		// if we didn't issue a change, we update the mapping to note that the current
+		// set corresponds to the latest equivalent session (i.e. now).
+		let session_index = <session::Module<T>>::current_index();
+		SetIdSession::insert(current_set_id, &session_index);
 	}
 
 	fn on_disabled(i: usize) {
@@ -412,8 +436,8 @@ impl<T: Trait> finality_tracker::OnFinalizationStalled<T::BlockNumber> for Modul
 #[derive(Copy, Clone, PartialOrd, Ord, Eq, PartialEq, Encode, Decode)]
 struct GrandpaTimeSlot {
 	// The order of these matters for `derive(Ord)`.
-	set_id: u64,
-	round: u64,
+	set_id: SetId,
+	round: RoundNumber,
 }
 
 // TODO [slashing]: Integrate this.

substrate/srml/session/src/lib.rs

@@ -170,6 +170,12 @@ impl<
 pub trait OnSessionEnding<ValidatorId> {
 	/// Handle the fact that the session is ending, and optionally provide the new validator set.
 	///
+	/// Even if the validator-set is the same as before, if any underlying economic
+	/// conditions have changed (i.e. stake-weights), the new validator set must be returned.
+	/// This is necessary for consensus engines making use of the session module to
+	/// issue a validator-set change so misbehavior can be provably associated with the new
+	/// economic conditions as opposed to the old.
+	///
 	/// `ending_index` is the index of the currently ending session.
 	/// The returned validator set, if any, will not be applied until `will_apply_at`.
 	/// `will_apply_at` is guaranteed to be at least `ending_index + 1`, since session indices don't
@@ -192,7 +198,11 @@ pub trait SessionHandler<ValidatorId> {
 	/// should provide the same validator set.
 	fn on_genesis_session<Ks: OpaqueKeys>(validators: &[(ValidatorId, Ks)]);
 
-	/// Session set has changed; act appropriately.
+	/// Session set has changed; act appropriately. Note that this can be called
+	/// before initialization of your module.
+	///
+	/// `changed` is true whenever any of the session keys or underlying economic
+	/// identities or weightings behind those keys has changed.
 	fn on_new_session<Ks: OpaqueKeys>(
 		changed: bool,
 		validators: &[(ValidatorId, Ks)],
@@ -217,11 +227,19 @@ pub trait OneSessionHandler<ValidatorId> {
 	fn on_genesis_session<'a, I: 'a>(validators: I)
 		where I: Iterator<Item=(&'a ValidatorId, Self::Key)>, ValidatorId: 'a;
 
-	/// Session set has changed; act appropriately.
+	/// Session set has changed; act appropriately. Note that this can be called
+	/// before initialization of your module.
+	///
+	/// `changed` is true when at least one of the session keys
+	/// or the underlying economic identities/distribution behind one the
+	/// session keys has changed, false otherwise.
+	///
+	/// The `validators` are the validators of the incoming session, and `queued_validators`
+	/// will follow.
 	fn on_new_session<'a, I: 'a>(
-		_changed: bool,
-		_validators: I,
-		_queued_validators: I
+		changed: bool,
+		validators: I,
+		queued_validators: I,
 	) where I: Iterator<Item=(&'a ValidatorId, Self::Key)>, ValidatorId: 'a;
 
 
@@ -341,10 +359,8 @@ decl_storage! {
 		/// Current index of the session.
 		CurrentIndex get(current_index): SessionIndex;
 
-		/// True if anything has changed in this session.
-		Changed: bool;
-
-		/// Queued keys changed.
+		/// True if the underlying economic identities or weighting behind the validators
+		/// has changed in the queued validator set.
 		QueuedChanged: bool;
 
 		/// The queued keys for the next session. When the next session begins, these keys
@@ -443,13 +459,10 @@ decl_module! {
 
 			Self::do_set_keys(&who, keys)?;
 
-			// Something changed.
-			Changed::put(true);
-
 			Ok(())
 		}
 
-		/// Called when a block is finalized. Will rotate session if it is the last
+		/// Called when a block is initialized. Will rotate session if it is the last
 		/// block of the current session.
 		fn on_initialize(n: T::BlockNumber) {
 			if T::ShouldEndSession::should_end_session(n) {
@@ -467,7 +480,6 @@ impl<T: Trait> Module<T> {
 		let session_index = CurrentIndex::get();
 
 		let changed = QueuedChanged::get();
-		let mut next_changed = Changed::take();
 
 		// Get queued session keys and validators.
 		let session_keys = <QueuedKeys<T>>::get();
@@ -479,12 +491,16 @@ impl<T: Trait> Module<T> {
 		let applied_at = session_index + 2;
 
 		// Get next validator set.
-		let maybe_validators = T::OnSessionEnding::on_session_ending(session_index, applied_at);
-		let next_validators = if let Some(validators) = maybe_validators {
-			next_changed = true;
-			validators
+		let maybe_next_validators = T::OnSessionEnding::on_session_ending(session_index, applied_at);
+		let (next_validators, next_identities_changed)
+			= if let Some(validators) = maybe_next_validators
+		{
+			// NOTE: as per the documentation on `OnSessionEnding`, we consider
+			// the validator set as having changed even if the validators are the
+			// same as before, as underlying economic conditions may have changed.
+			(validators, true)
 		} else {
-			<Validators<T>>::get()
+			(<Validators<T>>::get(), false)
 		};
 
 		// Increment session index.
@@ -492,9 +508,34 @@ impl<T: Trait> Module<T> {
 		CurrentIndex::put(session_index);
 
 		// Queue next session keys.
-		let queued_amalgamated = next_validators.into_iter()
-			.map(|a| { let k = Self::load_keys(&a).unwrap_or_default(); (a, k) })
-			.collect::<Vec<_>>();
+		let (queued_amalgamated, next_changed) = {
+			// until we are certain there has been a change, iterate the prior
+			// validators along with the current and check for changes
+			let mut changed = next_identities_changed;
+
+			let mut now_session_keys = session_keys.iter();
+			let mut check_next_changed = |keys: &T::Keys| {
+				if changed { return }
+				// since a new validator set always leads to `changed` starting
+				// as true, we can ensure that `now_session_keys` and `next_validators`
+				// have the same length. this function is called once per iteration.
+				if let Some(&(_, ref old_keys)) = now_session_keys.next() {
+					if old_keys != keys {
+						changed = true;
+						return
+					}
+				}
+			};
+			let queued_amalgamated = next_validators.into_iter()
+				.map(|a| {
+					let k = Self::load_keys(&a).unwrap_or_default();
+					check_next_changed(&k);
+					(a, k)
+				})
+				.collect::<Vec<_>>();
+
+			(queued_amalgamated, changed)
+		};
 
 		<QueuedKeys<T>>::put(queued_amalgamated.clone());
 		QueuedChanged::put(next_changed);
@@ -503,13 +544,16 @@ impl<T: Trait> Module<T> {
 		Self::deposit_event(Event::NewSession(session_index));
 
 		// Tell everyone about the new session keys.
-		T::SessionHandler::on_new_session::<T::Keys>(changed, &session_keys, &queued_amalgamated);
+		T::SessionHandler::on_new_session::<T::Keys>(
+			changed,
+			&session_keys,
+			&queued_amalgamated,
+		);
 	}
 
 	/// Disable the validator of index `i`.
 	pub fn disable_index(i: usize) {
 		T::SessionHandler::on_disabled(i);
-		Changed::put(true);
 	}
 
 	/// Disable the validator identified by `c`. (If using with the staking module, this would be
@@ -554,8 +598,6 @@ impl<T: Trait> Module<T> {
 				let key_data = old_keys.get_raw(id);
 				Self::clear_key_owner(id, key_data);
 			}
-
-			Changed::put(true);
 		}
 	}
 
@@ -668,8 +710,6 @@ mod tests {
 			Session::on_free_balance_zero(&1);
 			assert_eq!(Session::load_keys(&1), None);
 			assert_eq!(Session::key_owner(id, UintAuthorityId(1).get_raw(id)), None);
-
-			assert!(Changed::get());
 		})
 	}
 
@@ -816,9 +856,16 @@ mod tests {
 			initialize_block(6);
 			assert!(!session_changed());
 
+			// changing the keys of a validator leads to change.
+			assert_ok!(Session::set_keys(Origin::signed(69), UintAuthorityId(69).into(), vec![]));
 			force_new_session();
 			initialize_block(7);
 			assert!(session_changed());
+
+			// while changing the keys of a non-validator does not.
+			force_new_session();
+			initialize_block(7);
+			assert!(!session_changed());
 		});
 	}
 

substrate/srml/session/src/mock.rs

@@ -44,6 +44,7 @@ impl_outer_origin! {
 }
 
 thread_local! {
+	pub static VALIDATORS: RefCell<Vec<u64>> = RefCell::new(vec![1, 2, 3]);
 	pub static NEXT_VALIDATORS: RefCell<Vec<u64>> = RefCell::new(vec![1, 2, 3]);
 	pub static AUTHORITIES: RefCell<Vec<UintAuthorityId>> =
 		RefCell::new(vec![UintAuthorityId(1), UintAuthorityId(2), UintAuthorityId(3)]);
@@ -51,6 +52,7 @@ thread_local! {
 	pub static SESSION_LENGTH: RefCell<u64> = RefCell::new(2);
 	pub static SESSION_CHANGED: RefCell<bool> = RefCell::new(false);
 	pub static TEST_SESSION_CHANGED: RefCell<bool> = RefCell::new(false);
+	pub static DISABLED: RefCell<bool> = RefCell::new(false);
 }
 
 pub struct TestShouldEndSession;
@@ -76,14 +78,24 @@ impl SessionHandler<u64> for TestSessionHandler {
 				.collect()
 		);
 	}
-	fn on_disabled(_validator_index: usize) {}
+	fn on_disabled(_validator_index: usize) {
+		DISABLED.with(|l| *l.borrow_mut() = true)
+	}
 }
 
 pub struct TestOnSessionEnding;
 impl OnSessionEnding<u64> for TestOnSessionEnding {
 	fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option<Vec<u64>> {
 		if !TEST_SESSION_CHANGED.with(|l| *l.borrow()) {
-			Some(NEXT_VALIDATORS.with(|l| l.borrow().clone()))
+			VALIDATORS.with(|v| {
+				let mut v = v.borrow_mut();
+				*v = NEXT_VALIDATORS.with(|l| l.borrow().clone());
+				Some(v.clone())
+			})
+		} else if DISABLED.with(|l| std::mem::replace(&mut *l.borrow_mut(), false)) {
+			// If there was a disabled validator, underlying conditions have changed
+			// so we return `Some`.
+			Some(VALIDATORS.with(|v| v.borrow().clone()))
 		} else {
 			None
 		}
@@ -92,16 +104,13 @@ impl OnSessionEnding<u64> for TestOnSessionEnding {
 
 #[cfg(feature = "historical")]
 impl crate::historical::OnSessionEnding<u64, u64> for TestOnSessionEnding {
-	fn on_session_ending(_: SessionIndex, _: SessionIndex)
+	fn on_session_ending(ending_index: SessionIndex, will_apply_at: SessionIndex)
 		-> Option<(Vec<u64>, Vec<(u64, u64)>)>
 	{
-		if !TEST_SESSION_CHANGED.with(|l| *l.borrow()) {
-			let last_validators = Session::validators();
-			let last_identifications = last_validators.into_iter().map(|v| (v, v)).collect();
-			Some((NEXT_VALIDATORS.with(|l| l.borrow().clone()), last_identifications))
-		} else {
-			None
-		}
+		let pair_with_ids = |vals: &[u64]| vals.iter().map(|&v| (v, v)).collect::<Vec<_>>();
+		<Self as OnSessionEnding<_>>::on_session_ending(ending_index, will_apply_at)
+			.map(|vals| (pair_with_ids(&vals), vals))
+			.map(|(ids, vals)| (vals, ids))
 	}
 }
 

substrate/srml/staking/src/lib.rs

@@ -1355,6 +1355,10 @@ impl<T: Trait> Module<T> {
 			// Set the new validator set in sessions.
 			<CurrentElected<T>>::put(&elected_stashes);
 
+			// In order to keep the property required by `n_session_ending`
+			// that we must return the new validator set even if it's the same as the old,
+			// as long as any underlying economic conditions have changed, we don't attempt
+			// to do any optimization where we compare against the prior set.
 			(slot_stake, Some(elected_stashes))
 		} else {
 			// There were not enough candidates for even our minimal level of functionality.