Bounded Validators / Authorities in New Sessions #8640

shawntabrizi · 2021-04-20T01:53:03Z

This adds a limit to the number of validators and authorities that can be set by the session handler.

frame/aura/src/lib.rs

kianenigma · 2021-04-20T13:31:51Z

bin/node/runtime/src/lib.rs

@@ -332,6 +332,7 @@ parameter_types! {
 	pub const ExpectedBlockTime: Moment = MILLISECS_PER_BLOCK;
 	pub const ReportLongevity: u64 =
 		BondingDuration::get() as u64 * SessionsPerEra::get() as u64 * EpochDuration::get();
+	pub const MaxAuthorities: u32 = 100;


Probably we should document that this should be the upper bound of what Staking::validator_count is ever going to be.

frame/aura/src/lib.rs

frame/authority-discovery/src/lib.rs

frame/im-online/src/lib.rs

kianenigma

Only major issue: We are silently dropping authorities at session listener pallets (aura, babe, grandpa etc.) and I think we should not.

Pushed a truncating_from as you asked for, should help also with my only comment. Basically, we should use this instead of a combination of .take and force_from to ensure we get the logs somewhere at least.

shawntabrizi · 2021-04-21T18:13:42Z

Part of paritytech/polkadot-sdk#323

frame/session/src/lib.rs

kianenigma · 2021-04-22T05:46:13Z

frame/aura/src/lib.rs

@@ -165,7 +187,7 @@ impl<T: Config> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
 	type Public = T::AuthorityId;
 }

-impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
+impl<T: Config> OneSessionHandler<T::AccountId, T::MaxAuthorities> for Pallet<T> {


I think this is very very good because now OneSessionHandler<_, X> is a different trait than OneSessionHandler<_, Y> and we should get some good compile time checks, ensuring that we can't faff with them:

if babe implements OneSessionHandler<_, 10> and aura implements OneSessionHandler<_, 20>, I think if you then say impl session::Config { type SessionHandlers = (babe, aura) } we should get an error 🤔

But the missing point is:

If session's MaxAuthority is 100

and babe and aura's MaxAuthority is 50

I think we should prevent this at compile time but I don't it in the current state of the PR.

oh no we have this guarantee as well, because of

type SessionHandler: SessionHandler<Self::ValidatorId, Self::MaxValidators>;

Yes, the point here is that there should be a single global config of MaxAuthorities / MaxValidators in the runtime, and this should be shared with all of the pallets which are using a session handler.

Anything else would result in a compile error, which ensures that these systems are working together correctly.

I dont think it would ever be that session and babe would have different max authorities, but maybe @andresilva could give insight

Yeah, we should have an uniform number of authorities throughout the whole runtime.

kianenigma

So far LGTM

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

andresilva

Overall the changes LGTM but I feel like we could maximize the utility of this by spreading the typed sized bound to more places, this is so that we can avoid truncating_from which I think diminishes the value of this change. I believe the two main causes for the back and forth between vec and bounded vec are:

SessionManager still uses regular vecs - I didn't look much deeply but I think it's probably not too hard to migrate it to BoundedVec as well.
The SessionHandler/OneSessionHandler methods use slices / iterators.
- SessionHandler methods are probably not too hard to convert to BoundedVec as well.
- OneSessionHandler I'm not sure what we should do. Ideally we would have a BoundedIterator trait so that we don't lose the size type information. A weaker alternative would be some extension method on iterators that allows us to essentialy do a .collect_bounded<N>() where we get back a BoundedVec<_, N> (basically just hiding the truncating_from).

andresilva · 2021-04-22T11:22:22Z

frame/session/src/lib.rs

+		// Note this should never truncate because queued keys is also bounded to `MaxValidators`,
+		// but we do so defensively.


If BoundedVec add a map method we could use it here and guarantee at compile-time that we're getting a BoundedVec with the same size bound.

andresilva · 2021-04-22T11:25:33Z

frame/session/src/lib.rs

+			let validators = BoundedVec::<T::ValidatorId, T::MaxValidators>::truncating_from(
+				validators,
+				Some("Session Rotate Session Maybe Next Validators"),
+			);


Wouldn't it make sense that SessionManager is also parameterized on MaxValidators and SessionManager::new_session returns a BoundedVec instead?

andresilva · 2021-04-22T11:31:47Z

frame/aura/src/lib.rs

@@ -165,7 +187,7 @@ impl<T: Config> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
 	type Public = T::AuthorityId;
 }

-impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
+impl<T: Config> OneSessionHandler<T::AccountId, T::MaxAuthorities> for Pallet<T> {


Yeah, we should have an uniform number of authorities throughout the whole runtime.

shawntabrizi · 2021-06-17T22:50:07Z

stale

shawntabrizi added 4 commits April 19, 2021 21:39

update pallet

2ae0c01

update and add tests

fb14e40

Fix typo One -> On

fcf8809

fix node build

0b21a93

shawntabrizi requested review from andresilva and kianenigma as code owners April 20, 2021 01:53

github-actions bot added A0-please_review Pull request needs code review. A7-needspolkadotpr labels Apr 20, 2021

use types in session handler

530c4a7

shawntabrizi changed the title ~~Bounded Aura~~ Bounded Validators / Authorities in New Sessions Apr 20, 2021

shawntabrizi added 4 commits April 20, 2021 00:38

fix compile

3885e7a

fix test compile

41e9820

bound authority discovery

307e969

bound babe

a8a68cd

shawntabrizi added B7-runtimenoteworthy D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited C1-low PR touches the given topic and has a low impact on builders. labels Apr 20, 2021

shawntabrizi added 3 commits April 20, 2021 01:27

some compile fixes

6359f64

fix session benchmark compile

8f6ae0c

make pallet offences benchmarks compile

5d74819