Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Pallet session new API #4609

Merged
merged 14 commits into from
Jan 20, 2020
Merged

Pallet session new API #4609

merged 14 commits into from
Jan 20, 2020

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jan 13, 2020

Context

Currently the Session pallet query new validator set for future session using the trait OnSessionEnding, this trait is:

        /// Handle the fact that a new session is planned, 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
	/// repeat, but it could be some time after in case we are staging authority set changes.
	fn on_session_ending(
		ending_index: SessionIndex,
		will_apply_at: SessionIndex
	) -> Option<Vec<ValidatorId>>;

Though the documentation doesn't say if will_apply_at is allowed to decrease between call, or must be equal or superior to the one in the previous call.

In order be able to make the number of session planned in the future dynamic I propose to decouple the ending of a session and the planning of a future session, thus this API:

/// A trait for managing creation of new validator set.
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.
	fn new_session(new_index: SessionIndex) -> Option<Vec<ValidatorId>>;
	/// End the session.
	///
	/// Because the session pallet can queue validator set the ending session can be lower than the
	/// last new session index.
	fn end_session(end_index: SessionIndex);
}

Done

  • removal of associated type SelectInitialValidatorSet in pallet_session:
    this is because the session 0 and 1 can simply be query using the new API SessionManager::new_session(0 and 1). Makine this trait obsolete.
  • modification of pallet-session-historical:
    as planned session are now final, we can just generate the proof for this session when it is planned, no need for CachedObsolete structure anymore.

Not Done

  • staking module still can only handle a session module with a delay of 1 session, and still has many bugs, this is meant to be fixed in Lazy payouts #4474
    its behavior must have not changed at all.

This was referenced Jan 13, 2020
) -> Option<Vec<ValidatorId>>;
/// The returned validator set, if any, will not be applied until `new_index`.
/// `new_index` must be strictly superior from previous call.
fn on_new_session(new_index: SessionIndex) -> Option<Vec<ValidatorId>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have to be interleaved with calls to on_session_ending?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily, if we want to increase or decrease dynamically the number of session queued in pallet-session we can call on_new_session multiple times or no time respectively.

frame/session/src/lib.rs Outdated Show resolved Hide resolved
@gui1117
Copy link
Contributor Author

gui1117 commented Jan 14, 2020

I wasn't aware of SessionHandler trait, this seems to duplicate with OnSessionEnding.

@gavofyork gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 15, 2020
@gui1117 gui1117 marked this pull request as ready for review January 15, 2020 16:39
Self::ensure_storage_upgraded();
Self::new_session(start_session - 1).map(|(new, _old)| new)
if new_index == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to provide the validator set for the first session, this doesn't change the behavior of staking module.

if new_index == 0 {
return <Module<T>>::select_validators().1
}
Self::new_session(new_index - 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new session will trigger the reward for the previous session and create a new one, this is wrong and is intented to be fixed by #4474
This PR doesn't the behavior of staking module

@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 15, 2020
@kianenigma
Copy link
Contributor

staking module still can only handle a session module with a delay of 1 session, and still has many bugs, this is meant to be fixed in #4474

One problem for offchain Phragmen that I will soon get to is that: I need staking to be able to know the current Validator set. Will this become possible by either of these two PRs? currently, Staking has only a CurrentElected storage, which is badly named and ironically points to the next validator set afaik.

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 16, 2020

The current validator set (currently producing and validating) is available with SessionInterface::validators
EDIT: in the PR gav-lazy-rewards you can also query ErasStaker::iter_prefix(ActiveEra::get())

pub struct Module<T: Trait> for enum Call where origin: T::Origin { }
pub struct Module<T: Trait> for enum Call where origin: T::Origin {
fn on_initialize(_n: T::BlockNumber) {
CachedObsolete::<T>::remove_all();
Copy link
Contributor

@rphmeier rphmeier Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that actually work? it's a regular map, which AFAIK is not iterable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is now prefixed with Module ++ StorageEntryName. So, it is iterable :)

@@ -385,7 +370,7 @@ mod tests {

assert_eq!(StoredRange::get(), Some((0, 100)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We started session 98, but 100 is in our stored range? what information do we have available about session 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have stored range [0; 100) and because we delay by one session in this test runtime, the validator of next session is available. so we have information of session 99 when we start session 98

@rphmeier
Copy link
Contributor

Thanks Guillaume! New API looking much cleaner.

@gui1117
Copy link
Contributor Author

gui1117 commented Jan 20, 2020

PR for polkadot update here paritytech/polkadot#782

(Also I'm not completely against keeping InitialValidatorSet trait, at some point I feel it might be better not to confuse the first validator set build for genesis config and the first new_session.

So instead of calling new_session(0) we just ask for the initial validator set.)

@gavofyork gavofyork merged commit 1f9d09d into master Jan 20, 2020
@gavofyork gavofyork deleted the gui-session-new-api branch January 20, 2020 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants