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

client/authority-discovery/src/lib.rs

@@ -34,7 +34,7 @@
 //!
 //! 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.
 //!
@@ -426,7 +426,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)?;
@@ -492,12 +492,13 @@ 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 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 and next authority
+	// set.
 	fn get_own_public_keys_within_authority_set(
 		key_store: &BareCryptoStorePtr,
 		client: &Client,
@@ -508,14 +509,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();

frame/authority-discovery/src/lib.rs

@@ -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,24 @@ 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<_>>();
+		let mut keys = authorities.map(|x| x.1).collect::<Vec<_>>();
+		keys.sort();
+		keys.dedup();
+
 		Self::initialize_keys(&keys);
 	}
 
-	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 mut keys = validators.chain(queued_validators).map(|x| x.1).collect::<Vec<_>>();
+			keys.sort();
+			keys.dedup();
+
+			Keys::put(keys);
 		}
 	}
 
@@ -192,12 +199,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 +214,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 +250,51 @@ mod tests {
 			AuthorityDiscovery::on_genesis_session(
 				first_authorities.iter().map(|id| (id, id.clone()))
 			);
+			first_authorities.sort();
 			assert_eq!(first_authorities, AuthorityDiscovery::authorities());
 
 			// 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(),
+			);
+			assert_eq!(
+				first_authorities,
+				AuthorityDiscovery::authorities(),
+				"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());
 		});
 	}
 }

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>;
 	}
 }