From aa442f743eccaf12df9f43c9e0d5741a850fe196 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 17:13:39 +0200 Subject: [PATCH 1/9] Make SessionInfo include all authorities of the current session. --- node/network/gossip-support/src/lib.rs | 1 - node/subsystem-util/src/runtime/mod.rs | 2 +- primitives/src/v1/mod.rs | 12 +++++ runtime/parachains/src/session_info.rs | 13 ++++-- runtime/parachains/src/util.rs | 64 +++++++++++++++++++++++--- 5 files changed, 79 insertions(+), 13 deletions(-) diff --git a/node/network/gossip-support/src/lib.rs b/node/network/gossip-support/src/lib.rs index 1cdfaf4f5d9e..8a6d08b27280 100644 --- a/node/network/gossip-support/src/lib.rs +++ b/node/network/gossip-support/src/lib.rs @@ -54,7 +54,6 @@ const BACKOFF_DURATION: Duration = Duration::from_secs(5); /// should be fine: /// /// https://github.com/paritytech/substrate/blob/fc49802f263529160635471c8a17888846035f5d/client/authority-discovery/src/lib.rs#L88 -/// const LOW_CONNECTIVITY_WARN_DELAY: Duration = Duration::from_secs(600); /// The Gossip Support subsystem. diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index e1ac19e0e8fc..f086d31c661c 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -199,7 +199,7 @@ impl RuntimeInfo { /// Build `ValidatorInfo` for the current session. /// /// - /// Returns: `None` if not a validator. + /// Returns: `None` if not a parachain validator. async fn get_validator_info(&self, session_info: &SessionInfo) -> Result { if let Some(our_index) = self.get_our_index(&session_info.validators).await { // Get our group index: diff --git a/primitives/src/v1/mod.rs b/primitives/src/v1/mod.rs index dec17ca915a7..a063d267ac28 100644 --- a/primitives/src/v1/mod.rs +++ b/primitives/src/v1/mod.rs @@ -892,11 +892,23 @@ pub enum CandidateEvent { #[cfg_attr(feature = "std", derive(PartialEq, Default, MallocSizeOf))] pub struct SessionInfo { /// Validators in canonical ordering. + /// + /// NOTE: There might be more authorities in the current session, than validators participating + /// in parachain consensus. See + /// [max_validators](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). pub validators: Vec, /// Validators' authority discovery keys for the session in canonical ordering. + /// + /// NOTE: The first `validators.len()` entries will match the corresponding validators in + /// `validators`, afterwards any remaining authorities can be found. (Authorities not + /// participating in parachain consensus - see + /// [max_validators](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148) #[cfg_attr(feature = "std", ignore_malloc_size_of = "outside type")] pub discovery_keys: Vec, /// The assignment keys for validators. + /// + /// NOTE: For assignment keys the same rule applies as for discovery keys. (First + /// `validators.len()` keys, will correspond to validators in `validators`. pub assignment_keys: Vec, /// Validators in shuffled ordering - these are the validator groups as produced /// by the `Scheduler` module for the session and are typically referred to by diff --git a/runtime/parachains/src/session_info.rs b/runtime/parachains/src/session_info.rs index 3e18b16b9aab..a828f14b78d8 100644 --- a/runtime/parachains/src/session_info.rs +++ b/runtime/parachains/src/session_info.rs @@ -19,7 +19,10 @@ //! //! See https://w3f.github.io/parachain-implementers-guide/runtime/session_info.html. -use crate::{configuration, paras, scheduler, shared, util::take_active_subset}; +use crate::{ + configuration, paras, scheduler, shared, + util::take_active_subset_and_inactive, +}; use frame_support::{pallet_prelude::*, traits::OneSessionHandler}; use primitives::v1::{AssignmentId, AuthorityDiscoveryId, SessionIndex, SessionInfo}; use sp_std::vec::Vec; @@ -120,8 +123,8 @@ impl Pallet { // create a new entry in `Sessions` with information about the current session let new_session_info = SessionInfo { validators, // these are from the notification and are thus already correct. - discovery_keys: take_active_subset(&active_set, &discovery_keys), - assignment_keys: take_active_subset(&active_set, &assignment_keys), + discovery_keys: take_active_subset_and_inactive(&active_set, &discovery_keys), + assignment_keys: take_active_subset_and_inactive(&active_set, &assignment_keys), validator_groups, n_cores, zeroth_delay_tranche_width, @@ -357,11 +360,11 @@ mod tests { assert_eq!(session.validators, validators); assert_eq!( session.discovery_keys, - take_active_subset(&active_set, &unscrambled_discovery), + take_active_subset_and_inactive(&active_set, &unscrambled_discovery), ); assert_eq!( session.assignment_keys, - take_active_subset(&active_set, &unscrambled_assignment), + take_active_subset_and_inactive(&active_set, &unscrambled_assignment), ); }) } diff --git a/runtime/parachains/src/util.rs b/runtime/parachains/src/util.rs index 4f9d1e588ba1..41b352151f61 100644 --- a/runtime/parachains/src/util.rs +++ b/runtime/parachains/src/util.rs @@ -18,7 +18,7 @@ //! on all modules. use primitives::v1::{Id as ParaId, PersistedValidationData, ValidatorIndex}; -use sp_std::vec::Vec; +use sp_std::{collections::btree_set::BTreeSet, vec::Vec}; use crate::{configuration, hrmp, paras}; @@ -41,15 +41,47 @@ pub fn make_persisted_validation_data( }) } -/// Take the active subset of a set containing all validators. -pub fn take_active_subset(active_validators: &[ValidatorIndex], set: &[T]) -> Vec { - let subset: Vec<_> = active_validators +/// Take an active subset of a set containing all validators. +/// +/// First item in pair will be all items in set have indeces found in the `activer` indices set (in +/// the order of the `active` vec, the second item will contain the rest, in the original order. +/// +/// split_active_subset(active.into_iter().collect(), all).0 == take_active_subset(active, all) +pub fn split_active_subset(active: &[ValidatorIndex], all: &[T]) -> (Vec, Vec) { + let active_set: BTreeSet<_> = active.iter().cloned().collect(); + // active result has ordering of active set. + let active_result = take_active_subset(active, all); + // inactive result preserves original ordering of `all`. + let inactive_result = all .iter() - .filter_map(|i| set.get(i.0 as usize)) + .enumerate() + .filter(|(i, _)| !active_set.contains(&ValidatorIndex(*i as _))) + .map(|(_, v)| v) .cloned() .collect(); - if subset.len() != active_validators.len() { + if active_result.len() != active.len() { + log::warn!( + target: "runtime::parachains", + "Took active validators from set with wrong size.", + ); + } + + (active_result, inactive_result) +} + +/// Uses `split_active_subset` and concatenates the inactive to the active vec. +pub fn take_active_subset_and_inactive(active: &[ValidatorIndex], all: &[T]) -> Vec { + let (mut a, mut i) = split_active_subset(active, all); + a.append(&mut i); + a +} + +/// Take the active subset of a set containing all validators. +pub fn take_active_subset(active: &[ValidatorIndex], set: &[T]) -> Vec { + let subset: Vec<_> = active.iter().filter_map(|i| set.get(i.0 as usize)).cloned().collect(); + + if subset.len() != active.len() { log::warn!( target: "runtime::parachains", "Took active validators from set with wrong size", @@ -58,3 +90,23 @@ pub fn take_active_subset(active_validators: &[ValidatorIndex], set: & subset } + +#[cfg(test)] +mod tests { + + use sp_std::vec::Vec; + + use crate::util::{split_active_subset, take_active_subset}; + use primitives::v1::ValidatorIndex; + + #[test] + fn take_active_subset_is_compatible_with_split_active_subset() { + let active: Vec<_> = vec![ValidatorIndex(1), ValidatorIndex(7), ValidatorIndex(3)]; + let validators = vec![9, 1, 6, 7, 4, 5, 2, 3, 0, 8]; + let (selected, unselected) = split_active_subset(&active, &validators); + let selected2 = take_active_subset(&active, &validators); + assert_eq!(selected, selected2); + assert_eq!(unselected, vec![9, 6, 4, 5, 2, 0, 8]); + assert_eq!(selected, vec![1, 3, 7]); + } +} From a069c9fe72ac96c8ff4d94a47afc6523898874ac Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 17:16:40 +0200 Subject: [PATCH 2/9] Add missing import. --- runtime/parachains/src/session_info.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/runtime/parachains/src/session_info.rs b/runtime/parachains/src/session_info.rs index a828f14b78d8..1aba459b1601 100644 --- a/runtime/parachains/src/session_info.rs +++ b/runtime/parachains/src/session_info.rs @@ -172,14 +172,10 @@ impl OneSessionHandler for Pal #[cfg(test)] mod tests { use super::*; - use crate::{ - configuration::HostConfiguration, - initializer::SessionChangeNotification, - mock::{ + use crate::{configuration::HostConfiguration, initializer::SessionChangeNotification, mock::{ new_test_ext, Configuration, MockGenesisConfig, Origin, ParasShared, SessionInfo, System, Test, - }, - }; + }, util::take_active_subset}; use keyring::Sr25519Keyring; use primitives::v1::{BlockNumber, ValidatorId, ValidatorIndex}; From 118b7465179d26fef31f07429ab50c12ccd317cc Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 17:37:06 +0200 Subject: [PATCH 3/9] Only take subset for assignment keys. --- primitives/src/v1/mod.rs | 7 +++++-- runtime/parachains/src/session_info.rs | 13 +++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/primitives/src/v1/mod.rs b/primitives/src/v1/mod.rs index a063d267ac28..a2546c9a3263 100644 --- a/primitives/src/v1/mod.rs +++ b/primitives/src/v1/mod.rs @@ -907,8 +907,11 @@ pub struct SessionInfo { pub discovery_keys: Vec, /// The assignment keys for validators. /// - /// NOTE: For assignment keys the same rule applies as for discovery keys. (First - /// `validators.len()` keys, will correspond to validators in `validators`. + /// NOTE: There might be more authorities in the current session, than validators participating + /// in parachain consensus. See + /// [max_validators](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). + /// + /// Therefore: assignment_keys.len() == validators.len() && validators.len() <= discovery_keys.len(). pub assignment_keys: Vec, /// Validators in shuffled ordering - these are the validator groups as produced /// by the `Scheduler` module for the session and are typically referred to by diff --git a/runtime/parachains/src/session_info.rs b/runtime/parachains/src/session_info.rs index 1aba459b1601..35423324da8d 100644 --- a/runtime/parachains/src/session_info.rs +++ b/runtime/parachains/src/session_info.rs @@ -21,7 +21,7 @@ use crate::{ configuration, paras, scheduler, shared, - util::take_active_subset_and_inactive, + util::{take_active_subset, take_active_subset_and_inactive}, }; use frame_support::{pallet_prelude::*, traits::OneSessionHandler}; use primitives::v1::{AssignmentId, AuthorityDiscoveryId, SessionIndex, SessionInfo}; @@ -124,7 +124,7 @@ impl Pallet { let new_session_info = SessionInfo { validators, // these are from the notification and are thus already correct. discovery_keys: take_active_subset_and_inactive(&active_set, &discovery_keys), - assignment_keys: take_active_subset_and_inactive(&active_set, &assignment_keys), + assignment_keys: take_active_subset(&active_set, &assignment_keys), validator_groups, n_cores, zeroth_delay_tranche_width, @@ -172,10 +172,15 @@ impl OneSessionHandler for Pal #[cfg(test)] mod tests { use super::*; - use crate::{configuration::HostConfiguration, initializer::SessionChangeNotification, mock::{ + use crate::{ + configuration::HostConfiguration, + initializer::SessionChangeNotification, + mock::{ new_test_ext, Configuration, MockGenesisConfig, Origin, ParasShared, SessionInfo, System, Test, - }, util::take_active_subset}; + }, + util::take_active_subset, + }; use keyring::Sr25519Keyring; use primitives::v1::{BlockNumber, ValidatorId, ValidatorIndex}; From 00d61701b8fcac3145b9a8da7b6774228d487ebd Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 17:39:02 +0200 Subject: [PATCH 4/9] Fix typo. --- runtime/parachains/src/util.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/util.rs b/runtime/parachains/src/util.rs index 41b352151f61..f6d4aeaccd7b 100644 --- a/runtime/parachains/src/util.rs +++ b/runtime/parachains/src/util.rs @@ -43,7 +43,7 @@ pub fn make_persisted_validation_data( /// Take an active subset of a set containing all validators. /// -/// First item in pair will be all items in set have indeces found in the `activer` indices set (in +/// First item in pair will be all items in set have indeces found in the `active` indices set (in /// the order of the `active` vec, the second item will contain the rest, in the original order. /// /// split_active_subset(active.into_iter().collect(), all).0 == take_active_subset(active, all) From 3b4ef5309d52ace5357a0f300acd61c6d52ab9f1 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 17:54:26 +0200 Subject: [PATCH 5/9] Make spellcheck happy. --- primitives/src/v1/mod.rs | 9 ++++++--- runtime/parachains/src/util.rs | 4 +++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/primitives/src/v1/mod.rs b/primitives/src/v1/mod.rs index a2546c9a3263..3b892ec916f0 100644 --- a/primitives/src/v1/mod.rs +++ b/primitives/src/v1/mod.rs @@ -902,16 +902,19 @@ pub struct SessionInfo { /// NOTE: The first `validators.len()` entries will match the corresponding validators in /// `validators`, afterwards any remaining authorities can be found. (Authorities not /// participating in parachain consensus - see - /// [max_validators](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148) + /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148) #[cfg_attr(feature = "std", ignore_malloc_size_of = "outside type")] pub discovery_keys: Vec, /// The assignment keys for validators. /// /// NOTE: There might be more authorities in the current session, than validators participating /// in parachain consensus. See - /// [max_validators](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). + /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). /// - /// Therefore: assignment_keys.len() == validators.len() && validators.len() <= discovery_keys.len(). + /// Therefore: + /// ```ignore + /// assignment_keys.len() == validators.len() && validators.len() <= discovery_keys.len() + /// ``` pub assignment_keys: Vec, /// Validators in shuffled ordering - these are the validator groups as produced /// by the `Scheduler` module for the session and are typically referred to by diff --git a/runtime/parachains/src/util.rs b/runtime/parachains/src/util.rs index f6d4aeaccd7b..f3e5623d7ac5 100644 --- a/runtime/parachains/src/util.rs +++ b/runtime/parachains/src/util.rs @@ -46,7 +46,9 @@ pub fn make_persisted_validation_data( /// First item in pair will be all items in set have indeces found in the `active` indices set (in /// the order of the `active` vec, the second item will contain the rest, in the original order. /// -/// split_active_subset(active.into_iter().collect(), all).0 == take_active_subset(active, all) +/// ```ignore +/// split_active_subset(active.into_iter().collect(), all).0 == take_active_subset(active, all) +/// ``` pub fn split_active_subset(active: &[ValidatorIndex], all: &[T]) -> (Vec, Vec) { let active_set: BTreeSet<_> = active.iter().cloned().collect(); // active result has ordering of active set. From 2e3777a1d7ac6881729dee3ad5b0202e8877cdc8 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 17:58:00 +0200 Subject: [PATCH 6/9] Really. --- primitives/src/v1/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/src/v1/mod.rs b/primitives/src/v1/mod.rs index 3b892ec916f0..680ac55a7d5f 100644 --- a/primitives/src/v1/mod.rs +++ b/primitives/src/v1/mod.rs @@ -895,7 +895,7 @@ pub struct SessionInfo { /// /// NOTE: There might be more authorities in the current session, than validators participating /// in parachain consensus. See - /// [max_validators](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). + /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). pub validators: Vec, /// Validators' authority discovery keys for the session in canonical ordering. /// From a1519ddbeeb71d798b80675e4ae351755c3bd67a Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 8 Sep 2021 18:05:15 +0200 Subject: [PATCH 7/9] Fix test. --- runtime/parachains/src/session_info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/session_info.rs b/runtime/parachains/src/session_info.rs index 35423324da8d..93f0ba736577 100644 --- a/runtime/parachains/src/session_info.rs +++ b/runtime/parachains/src/session_info.rs @@ -365,7 +365,7 @@ mod tests { ); assert_eq!( session.assignment_keys, - take_active_subset_and_inactive(&active_set, &unscrambled_assignment), + take_active_subset(&active_set, &unscrambled_assignment), ); }) } From 65d1faa8f79579613c34dca0fedf41ee635e1231 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 9 Sep 2021 11:29:25 +0200 Subject: [PATCH 8/9] More clear documentation. --- primitives/src/v1/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/primitives/src/v1/mod.rs b/primitives/src/v1/mod.rs index 680ac55a7d5f..c261bf9b69e4 100644 --- a/primitives/src/v1/mod.rs +++ b/primitives/src/v1/mod.rs @@ -893,14 +893,16 @@ pub enum CandidateEvent { pub struct SessionInfo { /// Validators in canonical ordering. /// - /// NOTE: There might be more authorities in the current session, than validators participating + /// NOTE: There might be more authorities in the current session, than `validators` participating /// in parachain consensus. See /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). + /// + /// `SessionInfo::validators` will be limited to to `max_validators` when set. pub validators: Vec, /// Validators' authority discovery keys for the session in canonical ordering. /// /// NOTE: The first `validators.len()` entries will match the corresponding validators in - /// `validators`, afterwards any remaining authorities can be found. (Authorities not + /// `validators`, afterwards any remaining authorities can be found. This is any authorities not /// participating in parachain consensus - see /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148) #[cfg_attr(feature = "std", ignore_malloc_size_of = "outside type")] From 086648813035cb31204592535162c424b3dbc0f1 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 9 Sep 2021 11:39:12 +0200 Subject: [PATCH 9/9] Update comments in `SessionInfo`. --- .../src/runtime/session_info.md | 48 +++++++++++++------ 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/roadmap/implementers-guide/src/runtime/session_info.md b/roadmap/implementers-guide/src/runtime/session_info.md index a37f61af08d7..5ee63ab5a903 100644 --- a/roadmap/implementers-guide/src/runtime/session_info.md +++ b/roadmap/implementers-guide/src/runtime/session_info.md @@ -8,27 +8,47 @@ Helper structs: ```rust struct SessionInfo { - // validators in canonical ordering. These are the public keys used for backing, - // dispute participation, and approvals. + /// Validators in canonical ordering. + /// + /// NOTE: There might be more authorities in the current session, than `validators` participating + /// in parachain consensus. See + /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). + /// + /// `SessionInfo::validators` will be limited to to `max_validators` when set. validators: Vec, - // validators' authority discovery keys for the session in canonical ordering. - discovery_keys: Vec, - // The assignment keys for validators. + /// Validators' authority discovery keys for the session in canonical ordering. + /// + /// NOTE: The first `validators.len()` entries will match the corresponding validators in + /// `validators`, afterwards any remaining authorities can be found. This is any authorities not + /// participating in parachain consensus - see + /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148) + #[cfg_attr(feature = "std", ignore_malloc_size_of = "outside type")] + discovery_keys: Vec, + /// The assignment keys for validators. + /// + /// NOTE: There might be more authorities in the current session, than validators participating + /// in parachain consensus. See + /// [`max_validators`](https://github.com/paritytech/polkadot/blob/a52dca2be7840b23c19c153cf7e110b1e3e475f8/runtime/parachains/src/configuration.rs#L148). + /// + /// Therefore: + /// ```ignore + /// assignment_keys.len() == validators.len() && validators.len() <= discovery_keys.len() + /// ``` assignment_keys: Vec, - // validators in shuffled ordering - these are the validator groups as produced - // by the `Scheduler` module for the session and are typically referred to by - // `GroupIndex`. + /// Validators in shuffled ordering - these are the validator groups as produced + /// by the `Scheduler` module for the session and are typically referred to by + /// `GroupIndex`. validator_groups: Vec>, - // The number of availability cores used by the protocol during this session. + /// The number of availability cores used by the protocol during this session. n_cores: u32, - // the zeroth delay tranche width. + /// The zeroth delay tranche width. zeroth_delay_tranche_width: u32, - // The number of samples we do of relay_vrf_modulo. + /// The number of samples we do of `relay_vrf_modulo`. relay_vrf_modulo_samples: u32, - // The number of delay tranches in total. + /// The number of delay tranches in total. n_delay_tranches: u32, - // How many slots (BABE / SASSAFRAS) must pass before an assignment is considered a - // no-show. + /// How many slots (BABE / SASSAFRAS) must pass before an assignment is considered a + /// no-show. no_show_slots: u32, /// The number of validators needed to approve a block. needed_approvals: u32,