frame/authority-discovery: Have authorities() return both current and next (#6788)

* frame/authority-discovery: Have authorities() return both current and next

Authority address lookups on the DHT happen periodically (every 10
mintues) and are rather slow (~10 seconds).

In order to smooth the transition period between two sessions, have the
runtime module return both the current as well as the next authority
set. Thereby the client authority module will:

1. Publish its addresses one session in advance.

2. Prefetch the addresses of authorities of the next session in advance.

* frame/authority-discovery: Deduplicate authority ids

* frame/authority-discovery: Don't dedup on_genesis authorities

* frame/authority-discovery: Remove mut and sort on comparison in tests

* frame/authority-discovery: Use BTreeSet for deduplication

substrate/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::<HashMap<_, _>>()
 		};
 
-		// 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::<HashSet<_>>();
 
 		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::<HashSet<_>>();
 
-		let intersection = local_pub_keys.intersection(&current_authorities)
+		let intersection = local_pub_keys.intersection(&authorities)
 			.cloned()
 			.map(std::convert::Into::into)
 			.collect();

substrate/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<T: Trait> as AuthorityDiscovery {
-		/// Keys of the current authority set.
+		/// Keys of the current and next authority set.
 		Keys get(fn keys): Vec<AuthorityId>;
 	}
 	add_extra_genesis {
@@ -47,7 +47,7 @@ decl_module! {
 }
 
 impl<T: Trait> Module<T> {
-	/// Retrieve authority identifiers of the current authority set.
+	/// Retrieve authority identifiers of the current and next authority set.
 	pub fn authorities() -> Vec<AuthorityId> {
 		Keys::get()
 	}
@@ -71,17 +71,17 @@ impl<T: Trait> pallet_session::OneSessionHandler<T::AccountId> for Module<T> {
 	where
 		I: Iterator<Item = (&'a T::AccountId, Self::Key)>,
 	{
-		let keys = authorities.map(|x| x.1).collect::<Vec<_>>();
-		Self::initialize_keys(&keys);
+		Self::initialize_keys(&authorities.map(|x| x.1).collect::<Vec<_>>());
 	}
 
-	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<Item = (&'a T::AccountId, Self::Key)>,
 	{
-		// 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::<Vec<_>>());
+			let keys = validators.chain(queued_validators).map(|x| x.1).collect::<BTreeSet<_>>();
+			Keys::put(keys.into_iter().collect::<Vec<_>>());
 		}
 	}
 
@@ -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<AuthorityId> = vec![0, 1].into_iter()
+		let mut first_authorities: Vec<AuthorityId> = 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::<Vec<(&AuthorityId, AuthorityId)> >();
+
+		let mut 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)
 			.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::<Vec<(&AuthorityId, AuthorityId)> >();
 
 		// 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::<Vec<AuthorityId>>();
+			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());
 		});
 	}
 }

substrate/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<AuthorityId>;
 	}
 }