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.

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

Fix kusama validators getting 0 backing rewards the first session they enter the active set #3722

Merged
merged 5 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
66 changes: 44 additions & 22 deletions substrate/frame/authority-discovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,19 +144,21 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
);

Keys::<T>::put(bounded_keys);
}

let next_keys = queued_validators.map(|x| x.1).collect::<Vec<_>>();
// `changed` represents if queued_validators changed in the previous session not in the
// current one.
let next_keys = queued_validators.map(|x| x.1).collect::<Vec<_>>();

let next_bounded_keys = WeakBoundedVec::<_, T::MaxAuthorities>::force_from(
next_keys,
Some(
"Warning: The session has more queued validators than expected. \
A runtime configuration adjustment may be needed.",
),
);
let next_bounded_keys = WeakBoundedVec::<_, T::MaxAuthorities>::force_from(
next_keys,
Some(
"Warning: The session has more queued validators than expected. \
A runtime configuration adjustment may be needed.",
),
);

NextKeys::<T>::put(next_bounded_keys);
}
NextKeys::<T>::put(next_bounded_keys);
}

fn on_disabled(_i: u32) {
Expand Down Expand Up @@ -270,7 +272,7 @@ mod tests {
.map(|id| (&account_id, id))
.collect::<Vec<(&AuthorityId, AuthorityId)>>();

let mut third_authorities: Vec<AuthorityId> = vec![4, 5]
let third_authorities: Vec<AuthorityId> = vec![4, 5]
.into_iter()
.map(|i| AuthorityPair::from_seed_slice(vec![i; 32].as_ref()).unwrap().public())
.map(AuthorityId::from)
Expand All @@ -282,6 +284,18 @@ mod tests {
.map(|id| (&account_id, id))
.collect::<Vec<(&AuthorityId, AuthorityId)>>();

let mut fourth_authorities: Vec<AuthorityId> = vec![6, 7]
.into_iter()
.map(|i| AuthorityPair::from_seed_slice(vec![i; 32].as_ref()).unwrap().public())
.map(AuthorityId::from)
.collect();
// Needed for `pallet_session::OneSessionHandler::on_new_session`.
let fourth_authorities_and_account_ids = fourth_authorities
.clone()
.into_iter()
.map(|id| (&account_id, id))
.collect::<Vec<(&AuthorityId, AuthorityId)>>();

// Build genesis.
let mut t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();

Expand Down Expand Up @@ -310,25 +324,33 @@ mod tests {
third_authorities_and_account_ids.clone().into_iter(),
);
let authorities_returned = AuthorityDiscovery::authorities();
let mut first_and_third_authorities = first_authorities
.iter()
.chain(third_authorities.iter())
.cloned()
.collect::<Vec<AuthorityId>>();
first_and_third_authorities.sort();

assert_eq!(
first_authorities, authorities_returned,
first_and_third_authorities, authorities_returned,
"Expected authority set not to change as `changed` was set to false.",
);

// When `changed` set to true, the authority set should be updated.
AuthorityDiscovery::on_new_session(
true,
second_authorities_and_account_ids.into_iter(),
third_authorities_and_account_ids.clone().into_iter(),
third_authorities_and_account_ids.into_iter(),
fourth_authorities_and_account_ids.clone().into_iter(),
);
let mut second_and_third_authorities = second_authorities

let mut third_and_fourth_authorities = third_authorities
.iter()
.chain(third_authorities.iter())
.chain(fourth_authorities.iter())
.cloned()
.collect::<Vec<AuthorityId>>();
second_and_third_authorities.sort();
third_and_fourth_authorities.sort();
assert_eq!(
second_and_third_authorities,
third_and_fourth_authorities,
AuthorityDiscovery::authorities(),
"Expected authority set to contain both the authorities of the new as well as the \
next session."
Expand All @@ -337,12 +359,12 @@ mod tests {
// With overlapping authority sets, `authorities()` should return a deduplicated set.
AuthorityDiscovery::on_new_session(
true,
third_authorities_and_account_ids.clone().into_iter(),
third_authorities_and_account_ids.clone().into_iter(),
fourth_authorities_and_account_ids.clone().into_iter(),
fourth_authorities_and_account_ids.clone().into_iter(),
);
third_authorities.sort();
fourth_authorities.sort();
assert_eq!(
third_authorities,
fourth_authorities,
AuthorityDiscovery::authorities(),
"Expected authority set to be deduplicated."
);
Expand Down
6 changes: 5 additions & 1 deletion substrate/frame/session/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,11 @@ pub trait SessionHandler<ValidatorId> {
/// before initialization of your pallet.
///
/// `changed` is true whenever any of the session keys or underlying economic
/// identities or weightings behind those keys has changed.
/// identities or weightings behind `validators` keys has changed. `queued_validators`
/// could change without `validators` changing. Example of possible sequent calls:
/// Session N: on_new_session(false, unchanged_validators, unchanged_queued_validators)
/// Session N + 1: on_new_session(false, unchanged_validators, new_queued_validators)
/// Session N + 2: on_new_session(true, new_queued_validators, new_queued_validators)
fn on_new_session<Ks: OpaqueKeys>(
changed: bool,
validators: &[(ValidatorId, Ks)],
Expand Down
Loading