From 1d10db31845020bf2f7f34e5f52401d50a4193ca Mon Sep 17 00:00:00 2001
From: Max Inden <mail@max-inden.de>
Date: Wed, 2 Sep 2020 17:20:51 +0200
Subject: [PATCH] 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
---
.../client/authority-discovery/src/worker.rs | 16 ++--
.../frame/authority-discovery/src/lib.rs | 80 ++++++++++++++-----
.../primitives/authority-discovery/src/lib.rs | 4 +-
3 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs
index 232e59d08dd..629ea4fb2f4 100644
--- a/substrate/client/authority-discovery/src/worker.rs
+++ b/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(¤t_authorities)
+ let intersection = local_pub_keys.intersection(&authorities)
.cloned()
.map(std::convert::Into::into)
.collect();
diff --git a/substrate/frame/authority-discovery/src/lib.rs b/substrate/frame/authority-discovery/src/lib.rs
index 55e32b21dcb..d584838ecbe 100644
--- a/substrate/frame/authority-discovery/src/lib.rs
+++ b/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());
});
}
}
diff --git a/substrate/primitives/authority-discovery/src/lib.rs b/substrate/primitives/authority-discovery/src/lib.rs
index 8903a7f3837..0ae47c9758e 100644
--- a/substrate/primitives/authority-discovery/src/lib.rs
+++ b/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>;
}
}
--
GitLab