*: Refactor authority discovery (key mngmt, runtime API) (#3955)

* {core,srml}/authority-discovery: Move generic to specific session keys

* {srml,core}/authority-discovery: Verify signature outside of runtime

Given that the `core/authority-discovery` uses concrete authority
identifiers and signatures, one can verify a signature with the
authority discovery within `core`. Given the above, the `verify` runtime
api is obsolete and thus removed.

* *: Add authority discovery to the set of session keys

* *: Sign authority discovery DHT payload with keystore instead of runtime

Instead of calling a runtime function to sign a dht payload, which then
invokes the keystore, pass the keystore to the authority discovery
module and use it directly.

* core/authority-discovery: Give libp2p Kademlia time to start up

* core/authority-discovery: Move authorities priority group name to const

* node/runtime/src/lib.rs: Bump runtime spec version

* *: Fix lints and node/testing test failures

* *: Fix formatting

* core/authority-discovery: Box dht event channel in unit tests

* node/cli/src/service.rs: Fix future import

* node/cli/src/service.rs: Replace unwrap by expect with proof

* node/cli/src/chain_spec: Remove TODO for testnet key generation

* core/authority-discovery/src/lib: Remove scale encoding TODOs

* srml/authority-discovery: Make comment a doc comment

* core/authority-discovery: Remove unused StreamExt import

* node/runtime: Bump impl version to debug CI

* Test ci.

* Change the line width to 100.

* Revert "Change the line width to 100."

This reverts commit edff1f855bc71e0418bf3a967f81a35591d882e3.

* Fix a check for polkadot to work on forked repos.

* Revert "node/runtime: Bump impl version to debug CI"

This reverts commit 1a90903b4c929bc55a9e0a538af34b50b7f65139.

* Revert "Test ci."

This reverts commit a2c9df574e645158f77cd2b3d4d9355bcae33aab.

* Cargo.lock: Fix wrong lock file merge

* srml/authority-discovery: Keep track of new validator set not upcoming

* core/authority-discovery: Document key retrieval functions

substrate/srml/authority-discovery/Cargo.toml

@@ -6,24 +6,25 @@ edition = "2018"
 
 [dependencies]
 app-crypto = { package = "substrate-application-crypto", path = "../../core/application-crypto", default-features = false }
+authority-discovery-primitives = { package = "substrate-authority-discovery-primitives", path = "../../core/authority-discovery/primitives", default-features = false }
 codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false, features = ["derive"] }
 primitives = { package = "substrate-primitives",  path = "../../core/primitives", default-features = false }
 rstd = { package = "sr-std", path = "../../core/sr-std", default-features = false }
-serde = { version = "1.0.101", optional = true }
 runtime-io = { package = "sr-io", path = "../../core/sr-io", default-features = false }
+serde = { version = "1.0.101", optional = true }
 session = { package = "srml-session", path = "../session", default-features = false }
 sr-primitives = { path = "../../core/sr-primitives", default-features = false }
 support = { package = "srml-support", path = "../support", default-features = false }
 system = { package = "srml-system", path = "../system", default-features = false }
 
 [dev-dependencies]
-babe-primitives = { package = "substrate-consensus-babe-primitives", path = "../../core/consensus/babe/primitives", default-features = false }
 sr-staking-primitives = { path = "../../core/sr-staking-primitives", default-features = false }
 
 [features]
 default = ["std"]
 std = [
 	"app-crypto/std",
+	"authority-discovery-primitives/std",
 	"codec/std",
 	"primitives/std",
 	"rstd/std",

substrate/srml/authority-discovery/src/lib.rs

@@ -17,34 +17,25 @@
 //! # Authority discovery module.
 //!
 //! This module is used by the `core/authority-discovery` to retrieve the
-//! current set of authorities, learn its own authority id as well as sign and
-//! verify messages to and from other authorities.
-//!
-//! ## Dependencies
-//!
-//! This module depends on an externally defined session key type, specified via
-//! `Trait::AuthorityId` in the respective node runtime implementation.
+//! current set of authorities.
 
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use app_crypto::RuntimeAppPublic;
-use codec::FullCodec;
 use rstd::prelude::*;
 use support::{decl_module, decl_storage};
+use authority_discovery_primitives::AuthorityId;
 
 /// The module's config trait.
-pub trait Trait: system::Trait + session::Trait {
-	type AuthorityId: RuntimeAppPublic + Default + FullCodec + PartialEq;
-}
+pub trait Trait: system::Trait + session::Trait {}
 
 decl_storage! {
 	trait Store for Module<T: Trait> as AuthorityDiscovery {
-		/// The current set of keys that may issue a heartbeat.
-		Keys get(fn keys): Vec<T::AuthorityId>;
+		/// Keys of the current authority set.
+		Keys get(fn keys): Vec<AuthorityId>;
 	}
 	add_extra_genesis {
-		config(keys): Vec<T::AuthorityId>;
+		config(keys): Vec<AuthorityId>;
 		build(|config| Module::<T>::initialize_keys(&config.keys))
 	}
 }
@@ -55,79 +46,42 @@ decl_module! {
 }
 
 impl<T: Trait> Module<T> {
-	/// Returns own authority identifier iff it is part of the current authority
-	/// set, otherwise this function returns None. The restriction might be
-	/// softened in the future in case a consumer needs to learn own authority
-	/// identifier.
-	fn authority_id() -> Option<T::AuthorityId> {
-		let authorities = Keys::<T>::get();
-
-		let local_keys = T::AuthorityId::all();
-
-		authorities.into_iter().find_map(|authority| {
-			if local_keys.contains(&authority) {
-				Some(authority)
-			} else {
-				None
-			}
-		})
-	}
-
 	/// Retrieve authority identifiers of the current authority set.
-	pub fn authorities() -> Vec<T::AuthorityId> {
-		Keys::<T>::get()
+	pub fn authorities() -> Vec<AuthorityId> {
+		Keys::get()
 	}
 
-	/// Sign the given payload with the private key corresponding to the given authority id.
-	pub fn sign(
-		payload: &Vec<u8>,
-	) -> Option<(
-		<<T as Trait>::AuthorityId as RuntimeAppPublic>::Signature,
-		T::AuthorityId,
-	)> {
-		let authority_id = Module::<T>::authority_id()?;
-		authority_id.sign(payload).map(|s| (s, authority_id))
-	}
-
-	/// Verify the given signature for the given payload with the given
-	/// authority identifier.
-	pub fn verify(
-		payload: &Vec<u8>,
-		signature: <<T as Trait>::AuthorityId as RuntimeAppPublic>::Signature,
-		authority_id: T::AuthorityId,
-	) -> bool {
-		authority_id.verify(payload, &signature)
-	}
-
-	fn initialize_keys(keys: &[T::AuthorityId]) {
+	fn initialize_keys(keys: &[AuthorityId]) {
 		if !keys.is_empty() {
-			assert!(Keys::<T>::get().is_empty(), "Keys are already initialized!");
-			Keys::<T>::put(keys);
+			assert!(Keys::get().is_empty(), "Keys are already initialized!");
+			Keys::put(keys);
 		}
 	}
 }
 
 impl<T: Trait> sr_primitives::BoundToRuntimeAppPublic for Module<T> {
-	type Public = T::AuthorityId;
+	type Public = AuthorityId;
 }
 
 impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
-	type Key = T::AuthorityId;
+	type Key = AuthorityId;
 
-	fn on_genesis_session<'a, I: 'a>(validators: I)
+	fn on_genesis_session<'a, I: 'a>(authorities: I)
 	where
 		I: Iterator<Item = (&'a T::AccountId, Self::Key)>,
 	{
-		let keys = validators.map(|x| x.1).collect::<Vec<_>>();
+		let keys = authorities.map(|x| x.1).collect::<Vec<_>>();
 		Self::initialize_keys(&keys);
 	}
 
-	fn on_new_session<'a, I: 'a>(_changed: bool, _validators: I, next_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.
-		Keys::<T>::put(next_validators.map(|x| x.1).collect::<Vec<_>>());
+		if changed {
+			Keys::put(validators.map(|x| x.1).collect::<Vec<_>>());
+		}
 	}
 
 	fn on_disabled(_i: usize) {
@@ -138,8 +92,9 @@ impl<T: Trait> session::OneSessionHandler<T::AccountId> for Module<T> {
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use authority_discovery_primitives::{AuthorityPair};
 	use app_crypto::Pair;
-	use primitives::{testing::KeyStore, crypto::key_types, sr25519, H256, traits::KeystoreExt};
+	use primitives::{crypto::key_types, H256};
 	use runtime_io::TestExternalities;
 	use sr_primitives::{
 		testing::{Header, UintAuthorityId}, traits::{ConvertInto, IdentityLookup, OpaqueKeys},
@@ -152,11 +107,7 @@ mod tests {
 
 	#[derive(Clone, Eq, PartialEq)]
 	pub struct Test;
-	impl Trait for Test {
-		type AuthorityId = babe_primitives::AuthorityId;
-	}
-
-	type AuthorityId = babe_primitives::AuthorityId;
+	impl Trait for Test {}
 
 	pub struct TestOnSessionEnding;
 	impl session::OnSessionEnding<AuthorityId> for TestOnSessionEnding {
@@ -237,126 +188,79 @@ mod tests {
 	}
 
 	#[test]
-	fn authority_id_fn_returns_intersection_of_current_authorities_and_keys_in_key_store() {
-		// Create keystore and generate key.
-		let key_store = KeyStore::new();
-		key_store
-			.write()
-			.sr25519_generate_new(key_types::BABE, None)
-			.expect("Generates key.");
+	fn authorities_returns_current_authority_set() {
+		// The whole authority discovery module ignores account ids, but we still need it for
+		// `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();
 
-		// Retrieve key to later check if we got the right one.
-		let public_key = key_store
-			.read()
-			.sr25519_public_keys(key_types::BABE)
-			.pop()
-			.unwrap();
-		let authority_id = AuthorityId::from(public_key);
-
-		// Build genesis.
-		let mut t = system::GenesisConfig::default()
-			.build_storage::<Test>()
-			.unwrap();
-
-		GenesisConfig::<Test> {
-			keys: vec![authority_id.clone()],
-		}
-		.assimilate_storage(&mut t)
-		.unwrap();
-
-		// Create externalities.
-		let mut externalities = TestExternalities::new(t);
-		externalities.register_extension(KeystoreExt(key_store));
-
-		externalities.execute_with(|| {
-			assert_eq!(
-				authority_id,
-				AuthorityDiscovery::authority_id().expect("Retrieving public key.")
-			);
-		});
-	}
-
-	#[test]
-	fn authority_id_fn_does_not_return_key_outside_current_authority_set() {
-		// Create keystore and generate key.
-		let key_store = KeyStore::new();
-		key_store
-			.write()
-			.sr25519_generate_new(key_types::BABE, None)
-			.expect("Generates key.");
-
-		// Build genesis.
-		let mut t = system::GenesisConfig::default()
-			.build_storage::<Test>()
-			.unwrap();
-
-		// Generate random authority set.
-		let keys = vec![(); 5]
-			.iter()
-			.map(|_x| sr25519::Pair::generate_with_phrase(None).0.public())
+		let 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();
 
-		GenesisConfig::<Test> { keys: keys }
-			.assimilate_storage(&mut t)
-			.unwrap();
+		// Needed for `session::OneSessionHandler::on_new_session`.
+		let first_authorities_and_account_ids: Vec<(&AuthorityId, AuthorityId)> = first_authorities.clone()
+			.into_iter()
+			.map(|id| (&account_id, id))
+			.collect();
 
-		// Create externalities.
-		let mut externalities = TestExternalities::new(t);
-		externalities.register_extension(KeystoreExt(key_store));
+		let second_authorities: Vec<AuthorityId> = vec![2, 3].into_iter()
+			.map(|i| AuthorityPair::from_seed_slice(vec![i; 32].as_ref()).unwrap().public())
+			.map(AuthorityId::from)
+			.collect();
 
-		externalities.execute_with(|| {
-			assert_eq!(None, AuthorityDiscovery::authority_id());
-		});
-	}
-
-	#[test]
-	fn sign_and_verify_workflow() {
-		// Create keystore and generate key.
-		let key_store = KeyStore::new();
-		key_store
-			.write()
-			.sr25519_generate_new(key_types::BABE, None)
-			.expect("Generates key.");
-
-		// Retrieve key to later check if we got the right one.
-		let public_key = key_store
-			.read()
-			.sr25519_public_keys(key_types::BABE)
-			.pop()
-			.unwrap();
-		let authority_id = AuthorityId::from(public_key);
+		// Needed for `session::OneSessionHandler::on_new_session`.
+		let second_authorities_and_account_ids: Vec<(&AuthorityId, AuthorityId)> = second_authorities.clone()
+			.into_iter()
+			.map(|id| (&account_id, id))
+			.collect();
 
 		// Build genesis.
 		let mut t = system::GenesisConfig::default()
 			.build_storage::<Test>()
 			.unwrap();
 
-		GenesisConfig::<Test> {
-			keys: vec![authority_id.clone()],
+		GenesisConfig {
+			keys: vec![],
 		}
-		.assimilate_storage(&mut t)
+		.assimilate_storage::<Test>(&mut t)
 		.unwrap();
 
 		// Create externalities.
 		let mut externalities = TestExternalities::new(t);
-		externalities.register_extension(KeystoreExt(key_store));
 
 		externalities.execute_with(|| {
-			let payload = String::from("test payload").into_bytes();
-			let (sig, authority_id) = AuthorityDiscovery::sign(&payload).expect("signature");
+			use session::OneSessionHandler;
 
-			assert!(AuthorityDiscovery::verify(
-				&payload,
-				sig.clone(),
-				authority_id.clone(),
-			));
+			AuthorityDiscovery::on_genesis_session(
+				first_authorities.iter().map(|id| (id, id.clone()))
+			);
+			assert_eq!(
+				first_authorities,
+				AuthorityDiscovery::authorities()
+			);
 
-			assert!(!AuthorityDiscovery::verify(
-				&String::from("other payload").into_bytes(),
-				sig,
-				authority_id,
-			))
+			// 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(),
+			);
+			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(),
+			);
+			assert_eq!(
+				second_authorities,
+				AuthorityDiscovery::authorities()
+			);
 		});
 	}
 }