Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add possibility to inject non-authorities session-keys in genesis #5078

Merged
merged 23 commits into from
Jul 29, 2024
Merged

add possibility to inject non-authorities session-keys in genesis #5078

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
80719e9
add possibility to inject non-authorities session-keys in genesis
girazoki Jul 19, 2024
1e5c9b2
just single cloned
girazoki Jul 19, 2024
8a019db
changes
girazoki Jul 22, 2024
2d4b35b
prdoc
girazoki Jul 22, 2024
5eefddc
improve commets
girazoki Jul 22, 2024
91c5402
more comments
girazoki Jul 22, 2024
3c5f073
Apply suggestions from code review
bkchr Jul 22, 2024
b4ae933
Merge branch 'master' into girazoki-genesis-non-validator-keys
bkchr Jul 22, 2024
802a7f0
make prdoc hppy
girazoki Jul 23, 2024
d6be583
Merge remote-tracking branch 'origin/master' into girazoki-genesis-no…
girazoki Jul 23, 2024
c1f345f
make prdoc happy again
girazoki Jul 23, 2024
4ea650c
add default for tests
girazoki Jul 23, 2024
707df69
more fixes
girazoki Jul 26, 2024
7a8b4fd
more fixes
girazoki Jul 26, 2024
93dbfbf
fix all chain-spec
girazoki Jul 26, 2024
718b924
fix leftover collect
girazoki Jul 26, 2024
3153156
Merge branch 'master' into girazoki-genesis-non-validator-keys
bkchr Jul 26, 2024
1ce0602
Merge branch 'master' into girazoki-genesis-non-validator-keys
bkchr Jul 26, 2024
32e4f80
fmt
girazoki Jul 26, 2024
0fd97e2
Merge branch 'master' into girazoki-genesis-non-validator-keys
bkchr Jul 28, 2024
1420621
fix failing test
girazoki Jul 29, 2024
395e9b6
prdoc
girazoki Jul 29, 2024
e585289
Merge branch 'master' into girazoki-genesis-non-validator-keys
bkchr Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/test-utils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ impl<Runtime: BasicParachainRuntime> ExtBuilder<Runtime> {
.assimilate_storage(&mut t)
.unwrap();

pallet_session::GenesisConfig::<Runtime> { keys: self.keys }
pallet_session::GenesisConfig::<Runtime> { keys: self.keys, ..Default::default() }
.assimilate_storage(&mut t)
.unwrap();

Expand Down
10 changes: 10 additions & 0 deletions prdoc/pr_5078.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
title: Add possibility to inject non-authorities session-keys in genesis

doc:
- audience: Runtime Dev
description: |
Allows to inject a set of registered session-keys in pallet-session that are not
part of the first initial set of validators
crates:
- name: frame-session
bump: minor
bkchr marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 7 additions & 2 deletions substrate/frame/session/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,10 @@ pub mod pallet {
#[pallet::genesis_config]
#[derive(frame_support::DefaultNoBound)]
pub struct GenesisConfig<T: Config> {
/// Initial list of validator at genesis representing by their `(AccountId, ValidatorId, Keys)`.
pub keys: Vec<(T::AccountId, T::ValidatorId, T::Keys)>,
girazoki marked this conversation as resolved.
Show resolved Hide resolved
/// List of (AccountId, ValidatorId, Keys) that will be registered at genesis, but not as active validators.
pub non_authority_keys: Vec<(T::AccountId, T::ValidatorId, T::Keys)>,
Copy link
Member

Choose a reason for hiding this comment

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

IMO is worth being explicit that these keys (together with keys) will end up in the NextKeys set. I.e. keys that are registered as candidates for session after next session (session # 2).

And technically these are still authorities (well, authority candidates). So maybe a more appropriate name? Don't know registered_candidates? But naming is hard so I may be biased :-D

Copy link
Contributor Author

@girazoki girazoki Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, non_authority_keys are not necessarily authorities, whether they will be authorities it will be decided by SessionManager (even in the session # 2). Just for having keys registered they will not be authorities, unless they SessionManager decides to select them.

As for the naming, I am fine with anything as I am not too biased around it.

Copy link
Member

@davxy davxy Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well.

  1. I agree that these are not necessarily actual authorities, indeed I put in bold that are candidates.
  2. This is not my opinion, if I've not overlooked something, this is just a fact. The inner_set_keys is called, which calls put_keys, which stores your non_authority_keys in NextKeys set, which are the candidates registered

So I still think is worth being explicit about what is the usage of this non_authority_keys list. Otherwise as a user the doc is not super clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah got it. Yeah I can add more clarity on that for sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright so I added a comment saying that these keys are valid at least until session 2 (maybe valid is not the best word here?), but basically my intention is to say that if there is a session-key change, the first session that would pick these new keys is session number 3

}

#[pallet::genesis_build]
Expand All @@ -446,7 +449,9 @@ pub mod pallet {
}
});

for (account, val, keys) in self.keys.iter().cloned() {
for (account, val, keys) in
self.keys.iter().chain(self.non_authority_keys.iter()).cloned()
{
Pallet::<T>::inner_set_keys(&val, keys)
.expect("genesis config must not contain duplicates; qed");
if frame_system::Pallet::<T>::inc_consumers_without_limit(&account).is_err() {
Expand Down Expand Up @@ -676,7 +681,7 @@ impl<T: Config> Pallet<T> {
let mut now_session_keys = session_keys.iter();
let mut check_next_changed = |keys: &T::Keys| {
if changed {
return
return;
}
// since a new validator set always leads to `changed` starting
// as true, we can ensure that `now_session_keys` and `next_validators`
Expand Down