diff --git a/client/authority-discovery/src/worker.rs b/client/authority-discovery/src/worker.rs index 232e59d08dd78..629ea4fb2f423 100644 --- a/client/authority-discovery/src/worker.rs +++ b/client/authority-discovery/src/worker.rs @@ -99,7 +99,7 @@ pub enum Role { /// /// 2. **Discovers other authorities** /// -/// 1. Retrieves the current set of authorities. +/// 1. Retrieves the current and next set of authorities. /// /// 2. Starts DHT queries for the ids of the authorities. /// @@ -447,7 +447,7 @@ where .collect::>() }; - // Check if the event origins from an authority in the current authority set. + // Check if the event origins from an authority in the current or next authority set. let authority_id: &AuthorityId = authorities .get(&remote_key) .ok_or(Error::MatchingHashedAuthorityIdWithAuthorityId)?; @@ -514,12 +514,12 @@ where Ok(()) } - /// Retrieve our public keys within the current authority set. + /// Retrieve our public keys within the current and next authority set. // // A node might have multiple authority discovery keys within its keystore, e.g. an old one and - // one for the upcoming session. In addition it could be participating in the current authority - // set with two keys. The function does not return all of the local authority discovery public - // keys, but only the ones intersecting with the current authority set. + // one for the upcoming session. In addition it could be participating in the current and (/ or) + // next authority set with two keys. The function does not return all of the local authority + // discovery public keys, but only the ones intersecting with the current or next authority set. fn get_own_public_keys_within_authority_set( key_store: &BareCryptoStorePtr, client: &Client, @@ -530,14 +530,14 @@ where .collect::>(); let id = BlockId::hash(client.info().best_hash); - let current_authorities = client.runtime_api() + let authorities = client.runtime_api() .authorities(&id) .map_err(Error::CallingRuntime)? .into_iter() .map(std::convert::Into::into) .collect::>(); - let intersection = local_pub_keys.intersection(¤t_authorities) + let intersection = local_pub_keys.intersection(&authorities) .cloned() .map(std::convert::Into::into) .collect(); diff --git a/frame/authority-discovery/src/lib.rs b/frame/authority-discovery/src/lib.rs index 55e32b21dcb94..d584838ecbedd 100644 --- a/frame/authority-discovery/src/lib.rs +++ b/frame/authority-discovery/src/lib.rs @@ -23,7 +23,7 @@ // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] -use sp_std::prelude::*; +use sp_std::{collections::btree_set::BTreeSet, prelude::*}; use frame_support::{decl_module, decl_storage}; use sp_authority_discovery::AuthorityId; @@ -32,7 +32,7 @@ pub trait Trait: frame_system::Trait + pallet_session::Trait {} decl_storage! { trait Store for Module as AuthorityDiscovery { - /// Keys of the current authority set. + /// Keys of the current and next authority set. Keys get(fn keys): Vec; } add_extra_genesis { @@ -47,7 +47,7 @@ decl_module! { } impl Module { - /// Retrieve authority identifiers of the current authority set. + /// Retrieve authority identifiers of the current and next authority set. pub fn authorities() -> Vec { Keys::get() } @@ -71,17 +71,17 @@ impl pallet_session::OneSessionHandler for Module { where I: Iterator, { - let keys = authorities.map(|x| x.1).collect::>(); - Self::initialize_keys(&keys); + Self::initialize_keys(&authorities.map(|x| x.1).collect::>()); } - fn on_new_session<'a, I: 'a>(changed: bool, validators: I, _queued_validators: I) + fn on_new_session<'a, I: 'a>(changed: bool, validators: I, queued_validators: I) where I: Iterator, { - // Remember who the authorities are for the new session. + // Remember who the authorities are for the new and next session. if changed { - Keys::put(validators.map(|x| x.1).collect::>()); + let keys = validators.chain(queued_validators).map(|x| x.1).collect::>(); + Keys::put(keys.into_iter().collect::>()); } } @@ -192,12 +192,13 @@ mod tests { } #[test] - fn authorities_returns_current_authority_set() { - // The whole authority discovery module ignores account ids, but we still need it for - // `pallet_session::OneSessionHandler::on_new_session`, thus its safe to use the same value everywhere. + fn authorities_returns_current_and_next_authority_set() { + // The whole authority discovery module ignores account ids, but we still need them for + // `pallet_session::OneSessionHandler::on_new_session`, thus its safe to use the same value + // everywhere. let account_id = AuthorityPair::from_seed_slice(vec![10; 32].as_ref()).unwrap().public(); - let first_authorities: Vec = vec![0, 1].into_iter() + let mut first_authorities: Vec = vec![0, 1].into_iter() .map(|i| AuthorityPair::from_seed_slice(vec![i; 32].as_ref()).unwrap().public()) .map(AuthorityId::from) .collect(); @@ -206,12 +207,21 @@ mod tests { .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 second_authorities_and_account_ids: Vec<(&AuthorityId, AuthorityId)> = second_authorities.clone() + let second_authorities_and_account_ids = second_authorities.clone() .into_iter() .map(|id| (&account_id, id)) + .collect:: >(); + + let mut third_authorities: Vec = vec![4, 5].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 third_authorities_and_account_ids = third_authorities.clone() + .into_iter() + .map(|id| (&account_id, id)) + .collect:: >(); // Build genesis. let mut t = frame_system::GenesisConfig::default() @@ -233,23 +243,55 @@ mod tests { AuthorityDiscovery::on_genesis_session( first_authorities.iter().map(|id| (id, id.clone())) ); - assert_eq!(first_authorities, AuthorityDiscovery::authorities()); + first_authorities.sort(); + let mut authorities_returned = AuthorityDiscovery::authorities(); + authorities_returned.sort(); + assert_eq!(first_authorities, authorities_returned); // When `changed` set to false, the authority set should not be updated. AuthorityDiscovery::on_new_session( false, second_authorities_and_account_ids.clone().into_iter(), - vec![].into_iter(), + third_authorities_and_account_ids.clone().into_iter(), + ); + let mut authorities_returned = AuthorityDiscovery::authorities(); + authorities_returned.sort(); + assert_eq!( + first_authorities, + authorities_returned, + "Expected authority set not to change as `changed` was set to false.", ); - assert_eq!(first_authorities, AuthorityDiscovery::authorities()); // When `changed` set to true, the authority set should be updated. AuthorityDiscovery::on_new_session( true, second_authorities_and_account_ids.into_iter(), - vec![].into_iter(), + third_authorities_and_account_ids.clone().into_iter(), + ); + let mut second_and_third_authorities = second_authorities.iter() + .chain(third_authorities.iter()) + .cloned() + .collect::>(); + second_and_third_authorities.sort(); + assert_eq!( + second_and_third_authorities, + AuthorityDiscovery::authorities(), + "Expected authority set to contain both the authorities of the new as well as the \ + next session." + ); + + // 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(), + ); + third_authorities.sort(); + assert_eq!( + third_authorities, + AuthorityDiscovery::authorities(), + "Expected authority set to be deduplicated." ); - assert_eq!(second_authorities, AuthorityDiscovery::authorities()); }); } } diff --git a/primitives/authority-discovery/src/lib.rs b/primitives/authority-discovery/src/lib.rs index 8903a7f383755..0ae47c9758ee6 100644 --- a/primitives/authority-discovery/src/lib.rs +++ b/primitives/authority-discovery/src/lib.rs @@ -45,9 +45,9 @@ sp_api::decl_runtime_apis! { /// The authority discovery api. /// /// This api is used by the `client/authority-discovery` module to retrieve identifiers - /// of the current authority set. + /// of the current and next authority set. pub trait AuthorityDiscoveryApi { - /// Retrieve authority identifiers of the current authority set. + /// Retrieve authority identifiers of the current and next authority set. fn authorities() -> Vec; } }