From 89239be003bd64a17644d521d46c0d3aa6e3ca34 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Sun, 17 Mar 2024 13:59:15 +0200 Subject: [PATCH 1/5] Fix kusama validators getting 0 backing rewards the first session they enter active set Signed-off-by: Alexandru Gheorghe --- .../frame/authority-discovery/src/lib.rs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/substrate/frame/authority-discovery/src/lib.rs b/substrate/frame/authority-discovery/src/lib.rs index aa57378934af..c631060e1ea3 100644 --- a/substrate/frame/authority-discovery/src/lib.rs +++ b/substrate/frame/authority-discovery/src/lib.rs @@ -144,19 +144,22 @@ impl OneSessionHandler for Pallet { ); Keys::::put(bounded_keys); + } - let next_keys = queued_validators.map(|x| x.1).collect::>(); + // `changed` represents if queued_validators changed in the previous session not in the + // current one, so it does not make sense to guard updating of next_keys by it. + // see: https://github.com/paritytech/polkadot-sdk/blob/02e1a7f476d7d7c67153e975ab9a1bdc02ffea12/substrate/frame/session/src/lib.rs#L613 + let next_keys = queued_validators.map(|x| x.1).collect::>(); - 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::::put(next_bounded_keys); - } + NextKeys::::put(next_bounded_keys); } fn on_disabled(_i: u32) { From 6295f2045585346c71b8057aa951f0c6a7371394 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Mon, 18 Mar 2024 08:43:23 +0200 Subject: [PATCH 2/5] Update substrate/frame/authority-discovery/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/frame/authority-discovery/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/frame/authority-discovery/src/lib.rs b/substrate/frame/authority-discovery/src/lib.rs index c631060e1ea3..ef24e6418d89 100644 --- a/substrate/frame/authority-discovery/src/lib.rs +++ b/substrate/frame/authority-discovery/src/lib.rs @@ -147,8 +147,7 @@ impl OneSessionHandler for Pallet { } // `changed` represents if queued_validators changed in the previous session not in the - // current one, so it does not make sense to guard updating of next_keys by it. - // see: https://github.com/paritytech/polkadot-sdk/blob/02e1a7f476d7d7c67153e975ab9a1bdc02ffea12/substrate/frame/session/src/lib.rs#L613 + // current one. let next_keys = queued_validators.map(|x| x.1).collect::>(); let next_bounded_keys = WeakBoundedVec::<_, T::MaxAuthorities>::force_from( From 7d76ba6e5d84df262ac36ed16f175d7b07f7e5ec Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Mon, 18 Mar 2024 09:02:18 +0200 Subject: [PATCH 3/5] Fix unittests Signed-off-by: Alexandru Gheorghe --- .../frame/authority-discovery/src/lib.rs | 44 ++++++++++++++----- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/substrate/frame/authority-discovery/src/lib.rs b/substrate/frame/authority-discovery/src/lib.rs index ef24e6418d89..ed9240d99e8d 100644 --- a/substrate/frame/authority-discovery/src/lib.rs +++ b/substrate/frame/authority-discovery/src/lib.rs @@ -272,7 +272,7 @@ mod tests { .map(|id| (&account_id, id)) .collect::>(); - let mut third_authorities: Vec = vec![4, 5] + let third_authorities: Vec = vec![4, 5] .into_iter() .map(|i| AuthorityPair::from_seed_slice(vec![i; 32].as_ref()).unwrap().public()) .map(AuthorityId::from) @@ -284,6 +284,18 @@ mod tests { .map(|id| (&account_id, id)) .collect::>(); + let mut fourth_authorities: Vec = 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::>(); + // Build genesis. let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -312,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::>(); + 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::>(); - 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." @@ -339,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." ); From fbe24ee285a08b9aa0a93eb3e3c780c022f5dc00 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Mon, 18 Mar 2024 09:10:23 +0200 Subject: [PATCH 4/5] Improve documentation for `on_new_session` Signed-off-by: Alexandru Gheorghe --- substrate/frame/session/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 7d8128dbc1fe..17b6aa7a4640 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -285,7 +285,11 @@ pub trait SessionHandler { /// 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( changed: bool, validators: &[(ValidatorId, Ks)], From 0f2b7b1e3cfece79809c019c96ef1afc8d426e8d Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Mon, 18 Mar 2024 13:21:33 +0200 Subject: [PATCH 5/5] Add prdoc Signed-off-by: Alexandru Gheorghe --- prdoc/pr_3722.prdoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 prdoc/pr_3722.prdoc diff --git a/prdoc/pr_3722.prdoc b/prdoc/pr_3722.prdoc new file mode 100644 index 000000000000..7e2d7d38795b --- /dev/null +++ b/prdoc/pr_3722.prdoc @@ -0,0 +1,13 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Fix kusama 0 backing rewards when entering active set + +doc: + - audience: Runtime Dev + description: | + This PR fixes getting 0 backing rewards the first session when + a node enters the active set. + +crates: + - name: pallet-authority-discovery