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

move session handler traits to frame-support #8649

Closed
wants to merge 2 commits into from

Conversation

kianenigma
Copy link
Contributor

Will help with paritytech/statemint#43.

Context

For a simple parachain like statemint, we are not going to use staking or session, as they are too complicated. But we still need to use pallet-aura. The only easy and sensible way to communicate with pallet-aura is to register it in the list of SessionHandler somewhere. This is exactly how relay chains work.

Now, I want a new pallet like parachain-selection to act like the combination of staking and session for a parachain, and select some collators in some simple way every N block and report them to pallet-aura.

All of this works, but it has two flaws:

  1. SessionHandler trait was defined in pallet-session and in order to reuse it I had to import it, which was not good.
  2. I can imagine why SessionHandler was defined in pallet-session: its API is exposing queued_validators which is specific to pallet-session. And what I have done now is breaking this. I am not happy with this but in short term I think it is okay and I will make a follow up issue with further details. Essentially, we need to convert SessionHandler trait to something more generic, so that other chains can write their own custom pallet-session, but still be able to use our aura (and grandpa, babe, authority-discovery pallets). This will need to also change the OneSessionHandler.

If it makes anyone happier, I can think of something like this, we could make the queued_validator an Option and that's already a good step, but tbh I felt like it doesn't make a big difference at this point and a big revamp is needed.


I also removed a bunch of useless TestSessionHandlers here and there.

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 21, 2021
@gui1117
Copy link
Contributor

gui1117 commented Apr 21, 2021

If it makes anyone happier, I can think of something like this, we could make the queued_validator an Option and that's already a good step, but tbh I felt like it doesn't make a big difference at this point and a big revamp is needed.

It would be great for Babe to ensure at compile-time that at least one session is queued.
Maybe we can create different trait depending on the number of session queued.

We could create trait SessionHandler and SessionHandlerWithQueued, aura would implement both, babe only the latter.

Or maybe we can solve it with some const like:

trait SessionHandler<const QUEUED: usize> { .. }

impl<T: SessionHandler<0>> SessionHandler<1> for T { .. }
impl<T: SessionHandler<1>> SessionHandler<2> for T { .. }

struct Babe;
impl SessionHandler<1> for Babe { .. }

@gui1117
Copy link
Contributor

gui1117 commented Apr 21, 2021

Now, I want a new pallet like parachain-selection to act like the combination of staking and session for a parachain, and select some collators in some simple way every N block and report them to pallet-aura.

moving the trait to support won't solve the fact that the trait is enforcing queueing 1 session. And if the session is queued by one then I don't see a need to implement it differently than pallet-session.
Why not replacing only staking by another pallet and keeping the use pallet-session in parachain ?
A pallet implementing SessionManager could solve the usecase no ?

I'm not opposed to move it to support. But I feel like if we are going to queue the sessions by 1 then pallet-session should fit our need and only another SessionManager implementation is needed.

@kianenigma
Copy link
Contributor Author

A pallet implementing SessionManager could solve the usecase no ?

I like this idea as well, but I have to double-check. I was worried that pallet-session would be too much for a parachain and bring in too much functionality.

@kianenigma kianenigma added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 21, 2021
@kianenigma
Copy link
Contributor Author

@thiolliere in that case I will rework this PR to move SesseionManager out of pallet-session to avoid the dependency, and I agree that is slightly better. SesseionManager is a more generic trait and exposes less of the underlying session pallet complexity.

@kianenigma
Copy link
Contributor Author

not a big deal for now

@kianenigma kianenigma closed this Apr 26, 2021
@bkchr bkchr deleted the kiz-extract-session-handler-stuff branch May 1, 2021 06:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants