Allow Staking tests to run with session length other than 1 (parityte…

…ch#7719)

* fix periodic session

* Allow staking tests to run with session lengths other than 1.

* Update frame/staking/src/mock.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* Fix all tests with session length 5.

* Test for active != current

* Better doc

* Update frame/staking/src/lib.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* also set the timestamp properly.

* trigger CI

* Revert "trigger CI"

This reverts commit 0f254944cdad848aa6e63bd8a618db95447a8e68.

* Update frame/staking/src/lib.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

frame/session/src/lib.rs

@@ -17,8 +17,8 @@
 
 //! # Session Module
 //!
-//! The Session module allows validators to manage their session keys, provides a function for changing
-//! the session length, and handles session rotation.
+//! The Session module allows validators to manage their session keys, provides a function for
+//! changing the session length, and handles session rotation.
 //!
 //! - [`session::Config`](./trait.Config.html)
 //! - [`Call`](./enum.Call.html)
@@ -29,34 +29,39 @@
 //! ### Terminology
 //! <!-- Original author of paragraph: @gavofyork -->
 //!
-//! - **Session:** A session is a period of time that has a constant set of validators. Validators can only join
-//! or exit the validator set at a session change. It is measured in block numbers. The block where a session is
-//! ended is determined by the `ShouldEndSession` trait. When the session is ending, a new validator set
-//! can be chosen by `OnSessionEnding` implementations.
-//! - **Session key:** A session key is actually several keys kept together that provide the various signing
-//! functions required by network authorities/validators in pursuit of their duties.
-//! - **Validator ID:** Every account has an associated validator ID. For some simple staking systems, this
-//! may just be the same as the account ID. For staking systems using a stash/controller model,
-//! the validator ID would be the stash account ID of the controller.
+//! - **Session:** A session is a period of time that has a constant set of validators. Validators
+//!   can only join or exit the validator set at a session change. It is measured in block numbers.
+//!   The block where a session is ended is determined by the `ShouldEndSession` trait. When the
+//!   session is ending, a new validator set can be chosen by `OnSessionEnding` implementations.
+//!
+//! - **Session key:** A session key is actually several keys kept together that provide the various
+//!   signing functions required by network authorities/validators in pursuit of their duties.
+//! - **Validator ID:** Every account has an associated validator ID. For some simple staking
+//!   systems, this may just be the same as the account ID. For staking systems using a
+//!   stash/controller model, the validator ID would be the stash account ID of the controller.
+//!
 //! - **Session key configuration process:** Session keys are set using `set_keys` for use not in
-//! the next session, but the session after next. They are stored in `NextKeys`, a mapping between
-//! the caller's `ValidatorId` and the session keys provided. `set_keys` allows users to set their
-//! session key prior to being selected as validator.
-//! It is a public call since it uses `ensure_signed`, which checks that the origin is a signed account.
-//! As such, the account ID of the origin stored in `NextKeys` may not necessarily be associated with
-//! a block author or a validator. The session keys of accounts are removed once their account balance is zero.
+//!   the next session, but the session after next. They are stored in `NextKeys`, a mapping between
+//!   the caller's `ValidatorId` and the session keys provided. `set_keys` allows users to set their
+//!   session key prior to being selected as validator. It is a public call since it uses
+//!   `ensure_signed`, which checks that the origin is a signed account. As such, the account ID of
+//!   the origin stored in `NextKeys` may not necessarily be associated with a block author or a
+//!   validator. The session keys of accounts are removed once their account balance is zero.
+//!
 //! - **Session length:** This pallet does not assume anything about the length of each session.
-//! Rather, it relies on an implementation of `ShouldEndSession` to dictate a new session's start.
-//! This pallet provides the `PeriodicSessions` struct for simple periodic sessions.
-//! - **Session rotation configuration:** Configure as either a 'normal' (rewardable session where rewards are
-//! applied) or 'exceptional' (slashable) session rotation.
+//!   Rather, it relies on an implementation of `ShouldEndSession` to dictate a new session's start.
+//!   This pallet provides the `PeriodicSessions` struct for simple periodic sessions.
+//!
+//! - **Session rotation configuration:** Configure as either a 'normal' (rewardable session where
+//!   rewards are applied) or 'exceptional' (slashable) session rotation.
+//!
 //! - **Session rotation process:** At the beginning of each block, the `on_initialize` function
-//! queries the provided implementation of `ShouldEndSession`. If the session is to end the newly
-//! activated validator IDs and session keys are taken from storage and passed to the
-//! `SessionHandler`. The validator set supplied by `SessionManager::new_session` and the corresponding session
-//! keys, which may have been registered via `set_keys` during the previous session, are written
-//! to storage where they will wait one session before being passed to the `SessionHandler`
-//! themselves.
+//!   queries the provided implementation of `ShouldEndSession`. If the session is to end the newly
+//!   activated validator IDs and session keys are taken from storage and passed to the
+//!   `SessionHandler`. The validator set supplied by `SessionManager::new_session` and the
+//!   corresponding session keys, which may have been registered via `set_keys` during the previous
+//!   session, are written to storage where they will wait one session before being passed to the
+//!   `SessionHandler` themselves.
 //!
 //! ### Goals
 //!
@@ -75,21 +80,22 @@
 //! ### Public Functions
 //!
 //! - `rotate_session` - Change to the next session. Register the new authority set. Queue changes
-//! for next session rotation.
+//!   for next session rotation.
 //! - `disable_index` - Disable a validator by index.
 //! - `disable` - Disable a validator by Validator ID
 //!
 //! ## Usage
 //!
 //! ### Example from the FRAME
 //!
-//! The [Staking pallet](../pallet_staking/index.html) uses the Session pallet to get the validator set.
+//! The [Staking pallet](../pallet_staking/index.html) uses the Session pallet to get the validator
+//! set.
 //!
 //! ```
 //! use pallet_session as session;
 //!
 //! fn validators<T: pallet_session::Config>() -> Vec<<T as pallet_session::Config>::ValidatorId> {
-//!	<pallet_session::Module<T>>::validators()
+//! <pallet_session::Module<T>>::validators()
 //! }
 //! # fn main(){}
 //! ```
@@ -166,18 +172,18 @@ impl<
 					period.saturating_sub(block_after_last_session)
 				)
 			} else {
-				Zero::zero()
+				now
 			}
 		} else {
 			offset
 		})
 	}
 
 	fn weight(_now: BlockNumber) -> Weight {
-		// Weight note: `estimate_next_session_rotation` has no storage reads and trivial computational overhead.
-		// There should be no risk to the chain having this weight value be zero for now.
-		// However, this value of zero was not properly calculated, and so it would be reasonable
-		// to come back here and properly calculate the weight of this function.
+		// Weight note: `estimate_next_session_rotation` has no storage reads and trivial
+		// computational overhead. There should be no risk to the chain having this weight value be
+		// zero for now. However, this value of zero was not properly calculated, and so it would be
+		// reasonable to come back here and properly calculate the weight of this function.
 		0
 	}
 }
@@ -186,17 +192,17 @@ impl<
 pub trait SessionManager<ValidatorId> {
 	/// Plan a new session, 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.
-	/// The returned validator set, if any, will not be applied until `new_index`.
-	/// `new_index` is strictly greater than from previous call.
+	/// 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. The returned validator set, if any, will not be applied until `new_index`. `new_index`
+	/// is strictly greater than from previous call.
 	///
 	/// The first session start at index 0.
 	///
-	/// `new_session(session)` is guaranteed to be called before `end_session(session-1)`.
+	/// `new_session(session)` is guaranteed to be called before `end_session(session-1)`. In other
+	/// words, a new session must always be planned before an ongoing one can be finished.
 	fn new_session(new_index: SessionIndex) -> Option<Vec<ValidatorId>>;
 	/// End the session.
 	///
@@ -205,7 +211,7 @@ pub trait SessionManager<ValidatorId> {
 	fn end_session(end_index: SessionIndex);
 	/// Start the session.
 	///
-	/// The session start to be used for validation
+	/// The session start to be used for validation.
 	fn start_session(start_index: SessionIndex);
 }
 
@@ -242,7 +248,7 @@ pub trait SessionHandler<ValidatorId> {
 
 	/// A notification for end of the session.
 	///
-	/// Note it is triggered before any `SessionManager::end_session` handlers,
+	/// Note it is triggered before any [`SessionManager::end_session`] handlers,
 	/// so we can still affect the validator set.
 	fn on_before_session_ending() {}
 

frame/session/src/tests.rs

@@ -252,31 +252,32 @@ fn session_changed_flag_works() {
 
 #[test]
 fn periodic_session_works() {
-	struct Period;
-	struct Offset;
 
-	impl Get<u64> for Period {
-		fn get() -> u64 { 10 }
+	frame_support::parameter_types! {
+		const Period: u64 = 10;
+		const Offset: u64 = 3;
 	}
 
-	impl Get<u64> for Offset {
-		fn get() -> u64 { 3 }
-	}
-
-
 	type P = PeriodicSessions<Period, Offset>;
 
-	for i in 0..3 {
+	for i in 0u64..3 {
 		assert!(!P::should_end_session(i));
+		assert_eq!(P::estimate_next_session_rotation(i).unwrap(), 3);
 	}
 
-	assert!(P::should_end_session(3));
+	assert!(P::should_end_session(3u64));
+	assert_eq!(P::estimate_next_session_rotation(3u64).unwrap(), 3);
 
-	for i in (1..10).map(|i| 3 + i) {
+	for i in (1u64..10).map(|i| 3 + i) {
 		assert!(!P::should_end_session(i));
+		assert_eq!(P::estimate_next_session_rotation(i).unwrap(), 13);
 	}
 
-	assert!(P::should_end_session(13));
+	assert!(P::should_end_session(13u64));
+	assert_eq!(P::estimate_next_session_rotation(13u64).unwrap(), 13);
+
+	assert!(!P::should_end_session(14u64));
+	assert_eq!(P::estimate_next_session_rotation(14u64).unwrap(), 23);
 }
 
 #[test]

frame/staking/src/lib.rs

@@ -952,11 +952,14 @@ decl_storage! {
 
 		/// The active era information, it holds index and start.
 		///
-		/// The active era is the era currently rewarded.
-		/// Validator set of this era must be equal to `SessionInterface::validators`.
+		/// The active era is the era being currently rewarded. Validator set of this era must be
+		/// equal to [`SessionInterface::validators`].
 		pub ActiveEra get(fn active_era): Option<ActiveEraInfo>;
 
 		/// The session index at which the era start for the last `HISTORY_DEPTH` eras.
+		///
+		/// Note: This tracks the starting session (i.e. session index when era start being active)
+		/// for the eras in `[CurrentEra - HISTORY_DEPTH, CurrentEra]`.
 		pub ErasStartSessionIndex get(fn eras_start_session_index):
 			map hasher(twox_64_concat) EraIndex => Option<SessionIndex>;
 
@@ -2630,14 +2633,17 @@ impl<T: Config> Module<T> {
 	/// Start a session potentially starting an era.
 	fn start_session(start_session: SessionIndex) {
 		let next_active_era = Self::active_era().map(|e| e.index + 1).unwrap_or(0);
+		// This is only `Some` when current era has already progressed to the next era, while the
+		// active era is one behind (i.e. in the *last session of the active era*, or *first session
+		// of the new current era*, depending on how you look at it).
 		if let Some(next_active_era_start_session_index) =
 			Self::eras_start_session_index(next_active_era)
 		{
 			if next_active_era_start_session_index == start_session {
 				Self::start_era(start_session);
 			} else if next_active_era_start_session_index < start_session {
-				// This arm should never happen, but better handle it than to stall the
-				// staking pallet.
+				// This arm should never happen, but better handle it than to stall the staking
+				// pallet.
 				frame_support::print("Warning: A session appears to have been skipped.");
 				Self::start_era(start_session);
 			}
@@ -2893,9 +2899,11 @@ impl<T: Config> Module<T> {
 	/// Self votes are added and nominations before the most recent slashing span are ignored.
 	///
 	/// No storage item is updated.
-	pub fn do_phragmen<Accuracy: PerThing>(iterations: usize)
-	-> Option<PrimitiveElectionResult<T::AccountId, Accuracy>>
-		where ExtendedBalance: From<InnerOf<Accuracy>>
+	pub fn do_phragmen<Accuracy: PerThing>(
+		iterations: usize,
+	) -> Option<PrimitiveElectionResult<T::AccountId, Accuracy>>
+	where
+		ExtendedBalance: From<InnerOf<Accuracy>>,
 	{
 		let weight_of = Self::slashable_balance_of_fn();
 		let mut all_nominators: Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)> = Vec::new();
@@ -2928,7 +2936,11 @@ impl<T: Config> Module<T> {
 
 		if all_validators.len() < Self::minimum_validator_count().max(1) as usize {
 			// If we don't have enough candidates, nothing to do.
-			log!(error, "💸 Chain does not have enough staking candidates to operate. Era {:?}.", Self::current_era());
+			log!(
+				warn,
+				"💸 Chain does not have enough staking candidates to operate. Era {:?}.",
+				Self::current_era()
+			);
 			None
 		} else {
 			seq_phragmen::<_, Accuracy>(
@@ -3090,12 +3102,30 @@ impl<T: Config> Module<T> {
 /// some session can lag in between the newest session planned and the latest session started.
 impl<T: Config> pallet_session::SessionManager<T::AccountId> for Module<T> {
 	fn new_session(new_index: SessionIndex) -> Option<Vec<T::AccountId>> {
+		frame_support::debug::native::trace!(
+			target: LOG_TARGET,
+			"[{}] planning new_session({})",
+			<frame_system::Module<T>>::block_number(),
+			new_index
+		);
 		Self::new_session(new_index)
 	}
 	fn start_session(start_index: SessionIndex) {
+		frame_support::debug::native::trace!(
+			target: LOG_TARGET,
+			"[{}] starting start_session({})",
+			<frame_system::Module<T>>::block_number(),
+			start_index
+		);
 		Self::start_session(start_index)
 	}
 	fn end_session(end_index: SessionIndex) {
+		frame_support::debug::native::trace!(
+			target: LOG_TARGET,
+			"[{}] ending end_session({})",
+			<frame_system::Module<T>>::block_number(),
+			end_index
+		);
 		Self::end_session(end_index)
 	}
 }