Network sync refactoring (part 6) (#11940)

* Extract `NetworkKVProvider` trait in `sc-authority-discovery` and remove unnecessary dependency

* Extract `NetworkSyncForkRequest` trait in `sc-finality-grandpa`

* Relax requirements on `SyncOracle` trait, remove extra native methods from `NetworkService` that are already provided by trait impls

* Move `NetworkSigner` trait from `sc-authority-discovery` into `sc-network-common` and de-duplicate methods on `NetworkService`

* Move `NetworkKVProvider` trait from `sc-authority-discovery` into `sc-network-common` and de-duplicate methods on `NetworkService`

* Minimize `sc-authority-discovery` dependency on `sc-network`

* Move `NetworkSyncForkRequest` trait from `sc-finality-grandpa` to `sc-network-common` and de-duplicate methods in `NetworkService`

* Extract `NetworkStatusProvider` trait and de-duplicate methods on `NetworkService`

* Extract `NetworkPeers` trait and de-duplicate methods on `NetworkService`

* Extract `NetworkEventStream` trait and de-duplicate methods on `NetworkService`

* Move more methods from `NetworkService` into `NetworkPeers` trait

* Move `NetworkStateInfo` trait into `sc-network-common`

* Extract `NetworkNotification` trait and de-duplicate methods on `NetworkService`

* Extract `NetworkRequest` trait and de-duplicate methods on `NetworkService`

* Remove `NetworkService::local_peer_id()`, it is already provided by `NetworkStateInfo` impl

* Extract `NetworkTransaction` trait and de-duplicate methods on `NetworkService`

* Extract `NetworkBlock` trait and de-duplicate methods on `NetworkService`

* Remove dependencies on `NetworkService` from most of the methods of `sc-service`

* Address simple review comments

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

@@ -39,8 +39,9 @@ use futures::{
 	Stream,
 };
 
+use libp2p::{Multiaddr, PeerId};
 use sc_client_api::blockchain::HeaderBackend;
-use sc_network::{DhtEvent, Multiaddr, PeerId};
+use sc_network_common::protocol::event::DhtEvent;
 use sp_api::ProvideRuntimeApi;
 use sp_authority_discovery::{AuthorityDiscoveryApi, AuthorityId};
 use sp_runtime::traits::Block as BlockT;

substrate/client/authority-discovery/src/service.rs

@@ -25,7 +25,7 @@ use futures::{
 	SinkExt,
 };
 
-use sc_network::{Multiaddr, PeerId};
+use libp2p::{Multiaddr, PeerId};
 use sp_authority_discovery::AuthorityId;
 
 /// Service to interact with the [`crate::Worker`].

substrate/client/authority-discovery/src/tests.rs

@@ -87,12 +87,12 @@ fn get_addresses_and_authority_id() {
 fn cryptos_are_compatible() {
 	use sp_core::crypto::Pair;
 
-	let libp2p_secret = sc_network::Keypair::generate_ed25519();
+	let libp2p_secret = libp2p::identity::Keypair::generate_ed25519();
 	let libp2p_public = libp2p_secret.public();
 
 	let sp_core_secret = {
 		let libp2p_ed_secret = match libp2p_secret.clone() {
-			sc_network::Keypair::Ed25519(x) => x,
+			libp2p::identity::Keypair::Ed25519(x) => x,
 			_ => panic!("generate_ed25519 should have generated an Ed25519 key ¯\\_(ツ)_/¯"),
 		};
 		sp_core::ed25519::Pair::from_seed_slice(&libp2p_ed_secret.secret().as_ref()).unwrap()

substrate/client/authority-discovery/src/worker.rs

@@ -32,19 +32,22 @@ use std::{
 use futures::{channel::mpsc, future, stream::Fuse, FutureExt, Stream, StreamExt};
 
 use addr_cache::AddrCache;
-use async_trait::async_trait;
 use codec::Decode;
 use ip_network::IpNetwork;
 use libp2p::{
 	core::multiaddr,
 	multihash::{Multihash, MultihashDigest},
+	Multiaddr, PeerId,
 };
 use log::{debug, error, log_enabled};
 use prometheus_endpoint::{register, Counter, CounterVec, Gauge, Opts, U64};
 use prost::Message;
 use rand::{seq::SliceRandom, thread_rng};
 use sc_client_api::blockchain::HeaderBackend;
-use sc_network::{DhtEvent, ExHashT, Multiaddr, NetworkStateInfo, PeerId};
+use sc_network_common::{
+	protocol::event::DhtEvent,
+	service::{KademliaKey, NetworkDHTProvider, NetworkSigner, NetworkStateInfo, Signature},
+};
 use sp_api::ProvideRuntimeApi;
 use sp_authority_discovery::{
 	AuthorityDiscoveryApi, AuthorityId, AuthorityPair, AuthoritySignature,
@@ -136,7 +139,7 @@ pub struct Worker<Client, Network, Block, DhtEventStream> {
 	/// Queue of throttled lookups pending to be passed to the network.
 	pending_lookups: Vec<AuthorityId>,
 	/// Set of in-flight lookups.
-	in_flight_lookups: HashMap<sc_network::KademliaKey, AuthorityId>,
+	in_flight_lookups: HashMap<KademliaKey, AuthorityId>,
 
 	addr_cache: addr_cache::AddrCache,
 
@@ -464,10 +467,7 @@ where
 		}
 	}
 
-	fn handle_dht_value_found_event(
-		&mut self,
-		values: Vec<(sc_network::KademliaKey, Vec<u8>)>,
-	) -> Result<()> {
+	fn handle_dht_value_found_event(&mut self, values: Vec<(KademliaKey, Vec<u8>)>) -> Result<()> {
 		// Ensure `values` is not empty and all its keys equal.
 		let remote_key = single(values.iter().map(|(key, _)| key.clone()))
 			.map_err(|_| Error::ReceivingDhtValueFoundEventWithDifferentKeys)?
@@ -523,11 +523,11 @@ where
 				// properly signed by the owner of the PeerId
 
 				if let Some(peer_signature) = peer_signature {
-					let public_key =
-						sc_network::PublicKey::from_protobuf_encoding(&peer_signature.public_key)
-							.map_err(Error::ParsingLibp2pIdentity)?;
-					let signature =
-						sc_network::Signature { public_key, bytes: peer_signature.signature };
+					let public_key = libp2p::identity::PublicKey::from_protobuf_encoding(
+						&peer_signature.public_key,
+					)
+					.map_err(Error::ParsingLibp2pIdentity)?;
+					let signature = Signature { public_key, bytes: peer_signature.signature };
 
 					if !signature.verify(record, &remote_peer_id) {
 						return Err(Error::VerifyingDhtPayload)
@@ -590,55 +590,15 @@ where
 	}
 }
 
-pub trait NetworkSigner {
-	/// Sign a message in the name of `self.local_peer_id()`
-	fn sign_with_local_identity(
-		&self,
-		msg: impl AsRef<[u8]>,
-	) -> std::result::Result<sc_network::Signature, sc_network::SigningError>;
-}
-
 /// NetworkProvider provides [`Worker`] with all necessary hooks into the
 /// underlying Substrate networking. Using this trait abstraction instead of
-/// [`sc_network::NetworkService`] directly is necessary to unit test [`Worker`].
-#[async_trait]
-pub trait NetworkProvider: NetworkStateInfo + NetworkSigner {
-	/// Start putting a value in the Dht.
-	fn put_value(&self, key: sc_network::KademliaKey, value: Vec<u8>);
+/// `sc_network::NetworkService` directly is necessary to unit test [`Worker`].
+pub trait NetworkProvider: NetworkDHTProvider + NetworkStateInfo + NetworkSigner {}
 
-	/// Start getting a value from the Dht.
-	fn get_value(&self, key: &sc_network::KademliaKey);
-}
+impl<T> NetworkProvider for T where T: NetworkDHTProvider + NetworkStateInfo + NetworkSigner {}
 
-impl<B, H> NetworkSigner for sc_network::NetworkService<B, H>
-where
-	B: BlockT + 'static,
-	H: ExHashT,
-{
-	fn sign_with_local_identity(
-		&self,
-		msg: impl AsRef<[u8]>,
-	) -> std::result::Result<sc_network::Signature, sc_network::SigningError> {
-		self.sign_with_local_identity(msg)
-	}
-}
-
-#[async_trait::async_trait]
-impl<B, H> NetworkProvider for sc_network::NetworkService<B, H>
-where
-	B: BlockT + 'static,
-	H: ExHashT,
-{
-	fn put_value(&self, key: sc_network::KademliaKey, value: Vec<u8>) {
-		self.put_value(key, value)
-	}
-	fn get_value(&self, key: &sc_network::KademliaKey) {
-		self.get_value(key)
-	}
-}
-
-fn hash_authority_id(id: &[u8]) -> sc_network::KademliaKey {
-	sc_network::KademliaKey::new(&libp2p::multihash::Code::Sha2_256.digest(id).digest())
+fn hash_authority_id(id: &[u8]) -> KademliaKey {
+	KademliaKey::new(&libp2p::multihash::Code::Sha2_256.digest(id).digest())
 }
 
 // Makes sure all values are the same and returns it
@@ -685,7 +645,7 @@ async fn sign_record_with_authority_ids(
 	peer_signature: Option<schema::PeerSignature>,
 	key_store: &dyn CryptoStore,
 	keys: Vec<CryptoTypePublicPair>,
-) -> Result<Vec<(sc_network::KademliaKey, Vec<u8>)>> {
+) -> Result<Vec<(KademliaKey, Vec<u8>)>> {
 	let signatures = key_store
 		.sign_with_all(key_types::AUTHORITY_DISCOVERY, keys.clone(), &serialized_record)
 		.await

substrate/client/authority-discovery/src/worker/addr_cache.rs

@@ -16,9 +16,10 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use libp2p::core::multiaddr::{Multiaddr, Protocol};
-
-use sc_network::PeerId;
+use libp2p::{
+	core::multiaddr::{Multiaddr, Protocol},
+	PeerId,
+};
 use sp_authority_discovery::AuthorityId;
 use std::collections::{hash_map::Entry, HashMap, HashSet};
 

substrate/client/authority-discovery/src/worker/schema/tests.rs

@@ -21,9 +21,8 @@ mod schema_v1 {
 }
 
 use super::*;
-use libp2p::multiaddr::Multiaddr;
+use libp2p::{multiaddr::Multiaddr, PeerId};
 use prost::Message;
-use sc_network::PeerId;
 
 #[test]
 fn v2_decodes_v1() {
@@ -56,7 +55,7 @@ fn v2_decodes_v1() {
 
 #[test]
 fn v1_decodes_v2() {
-	let peer_secret = sc_network::Keypair::generate_ed25519();
+	let peer_secret = libp2p::identity::Keypair::generate_ed25519();
 	let peer_public = peer_secret.public();
 	let peer_id = peer_public.to_peer_id();
 	let multiaddress: Multiaddr =

substrate/client/authority-discovery/src/worker/tests.rs

@@ -22,7 +22,6 @@ use std::{
 	task::Poll,
 };
 
-use async_trait::async_trait;
 use futures::{
 	channel::mpsc::{self, channel},
 	executor::{block_on, LocalPool},
@@ -30,9 +29,10 @@ use futures::{
 	sink::SinkExt,
 	task::LocalSpawn,
 };
-use libp2p::{core::multiaddr, PeerId};
+use libp2p::{core::multiaddr, identity::Keypair, PeerId};
 use prometheus_endpoint::prometheus::default_registry;
 
+use sc_network_common::service::{KademliaKey, Signature, SigningError};
 use sp_api::{ApiRef, ProvideRuntimeApi};
 use sp_keystore::{testing::KeyStore, CryptoStore};
 use sp_runtime::traits::{Block as BlockT, NumberFor, Zero};
@@ -111,18 +111,18 @@ sp_api::mock_impl_runtime_apis! {
 
 #[derive(Debug)]
 pub enum TestNetworkEvent {
-	GetCalled(sc_network::KademliaKey),
-	PutCalled(sc_network::KademliaKey, Vec<u8>),
+	GetCalled(KademliaKey),
+	PutCalled(KademliaKey, Vec<u8>),
 }
 
 pub struct TestNetwork {
 	peer_id: PeerId,
-	identity: sc_network::Keypair,
+	identity: Keypair,
 	external_addresses: Vec<Multiaddr>,
 	// Whenever functions on `TestNetwork` are called, the function arguments are added to the
 	// vectors below.
-	pub put_value_call: Arc<Mutex<Vec<(sc_network::KademliaKey, Vec<u8>)>>>,
-	pub get_value_call: Arc<Mutex<Vec<sc_network::KademliaKey>>>,
+	pub put_value_call: Arc<Mutex<Vec<(KademliaKey, Vec<u8>)>>>,
+	pub get_value_call: Arc<Mutex<Vec<KademliaKey>>>,
 	event_sender: mpsc::UnboundedSender<TestNetworkEvent>,
 	event_receiver: Option<mpsc::UnboundedReceiver<TestNetworkEvent>>,
 }
@@ -136,7 +136,7 @@ impl TestNetwork {
 impl Default for TestNetwork {
 	fn default() -> Self {
 		let (tx, rx) = mpsc::unbounded();
-		let identity = sc_network::Keypair::generate_ed25519();
+		let identity = Keypair::generate_ed25519();
 		TestNetwork {
 			peer_id: identity.public().to_peer_id(),
 			identity,
@@ -153,21 +153,20 @@ impl NetworkSigner for TestNetwork {
 	fn sign_with_local_identity(
 		&self,
 		msg: impl AsRef<[u8]>,
-	) -> std::result::Result<sc_network::Signature, sc_network::SigningError> {
-		sc_network::Signature::sign_message(msg, &self.identity)
+	) -> std::result::Result<Signature, SigningError> {
+		Signature::sign_message(msg, &self.identity)
 	}
 }
 
-#[async_trait]
-impl NetworkProvider for TestNetwork {
-	fn put_value(&self, key: sc_network::KademliaKey, value: Vec<u8>) {
+impl NetworkDHTProvider for TestNetwork {
+	fn put_value(&self, key: KademliaKey, value: Vec<u8>) {
 		self.put_value_call.lock().unwrap().push((key.clone(), value.clone()));
 		self.event_sender
 			.clone()
 			.unbounded_send(TestNetworkEvent::PutCalled(key, value))
 			.unwrap();
 	}
-	fn get_value(&self, key: &sc_network::KademliaKey) {
+	fn get_value(&self, key: &KademliaKey) {
 		self.get_value_call.lock().unwrap().push(key.clone());
 		self.event_sender
 			.clone()
@@ -186,12 +185,16 @@ impl NetworkStateInfo for TestNetwork {
 	}
 }
 
-impl NetworkSigner for sc_network::Keypair {
+struct TestSigner<'a> {
+	keypair: &'a Keypair,
+}
+
+impl<'a> NetworkSigner for TestSigner<'a> {
 	fn sign_with_local_identity(
 		&self,
 		msg: impl AsRef<[u8]>,
-	) -> std::result::Result<sc_network::Signature, sc_network::SigningError> {
-		sc_network::Signature::sign_message(msg, self)
+	) -> std::result::Result<Signature, SigningError> {
+		Signature::sign_message(msg, self.keypair)
 	}
 }
 
@@ -200,7 +203,7 @@ async fn build_dht_event<Signer: NetworkSigner>(
 	public_key: AuthorityId,
 	key_store: &dyn CryptoStore,
 	network: Option<&Signer>,
-) -> Vec<(sc_network::KademliaKey, Vec<u8>)> {
+) -> Vec<(KademliaKey, Vec<u8>)> {
 	let serialized_record =
 		serialize_authority_record(serialize_addresses(addresses.into_iter())).unwrap();
 
@@ -313,7 +316,7 @@ fn publish_discover_cycle() {
 
 			let dht_event = {
 				let (key, value) = network.put_value_call.lock().unwrap().pop().unwrap();
-				sc_network::DhtEvent::ValueFound(vec![(key, value)])
+				DhtEvent::ValueFound(vec![(key, value)])
 			};
 
 			// Node B discovering node A's address.
@@ -469,7 +472,7 @@ fn dont_stop_polling_dht_event_stream_after_bogus_event() {
 				None,
 			)
 			.await;
-			sc_network::DhtEvent::ValueFound(kv_pairs)
+			DhtEvent::ValueFound(kv_pairs)
 		};
 		dht_event_tx.send(dht_event).await.expect("Channel has capacity of 1.");
 
@@ -487,7 +490,7 @@ fn dont_stop_polling_dht_event_stream_after_bogus_event() {
 struct DhtValueFoundTester {
 	pub remote_key_store: KeyStore,
 	pub remote_authority_public: sp_core::sr25519::Public,
-	pub remote_node_key: sc_network::Keypair,
+	pub remote_node_key: Keypair,
 	pub local_worker: Option<
 		Worker<
 			TestApi,
@@ -496,7 +499,7 @@ struct DhtValueFoundTester {
 				sp_runtime::generic::Header<u64, sp_runtime::traits::BlakeTwo256>,
 				substrate_test_runtime_client::runtime::Extrinsic,
 			>,
-			std::pin::Pin<Box<futures::channel::mpsc::Receiver<sc_network::DhtEvent>>>,
+			std::pin::Pin<Box<futures::channel::mpsc::Receiver<DhtEvent>>>,
 		>,
 	>,
 }
@@ -508,7 +511,7 @@ impl DhtValueFoundTester {
 			block_on(remote_key_store.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None))
 				.unwrap();
 
-		let remote_node_key = sc_network::Keypair::generate_ed25519();
+		let remote_node_key = Keypair::generate_ed25519();
 		Self { remote_key_store, remote_authority_public, remote_node_key, local_worker: None }
 	}
 
@@ -523,7 +526,7 @@ impl DhtValueFoundTester {
 	fn process_value_found(
 		&mut self,
 		strict_record_validation: bool,
-		values: Vec<(sc_network::KademliaKey, Vec<u8>)>,
+		values: Vec<(KademliaKey, Vec<u8>)>,
 	) -> Option<&HashSet<Multiaddr>> {
 		let (_dht_event_tx, dht_event_rx) = channel(1);
 		let local_test_api =
@@ -583,7 +586,7 @@ fn strict_accept_address_with_peer_signature() {
 		vec![addr.clone()],
 		tester.remote_authority_public.clone().into(),
 		&tester.remote_key_store,
-		Some(&tester.remote_node_key),
+		Some(&TestSigner { keypair: &tester.remote_node_key }),
 	));
 
 	let cached_remote_addresses = tester.process_value_found(true, kv_pairs);
@@ -598,12 +601,12 @@ fn strict_accept_address_with_peer_signature() {
 #[test]
 fn reject_address_with_rogue_peer_signature() {
 	let mut tester = DhtValueFoundTester::new();
-	let rogue_remote_node_key = sc_network::Keypair::generate_ed25519();
+	let rogue_remote_node_key = Keypair::generate_ed25519();
 	let kv_pairs = block_on(build_dht_event(
 		vec![tester.multiaddr_with_peer_id(1)],
 		tester.remote_authority_public.clone().into(),
 		&tester.remote_key_store,
-		Some(&rogue_remote_node_key),
+		Some(&TestSigner { keypair: &rogue_remote_node_key }),
 	));
 
 	let cached_remote_addresses = tester.process_value_found(false, kv_pairs);
@@ -621,7 +624,7 @@ fn reject_address_with_invalid_peer_signature() {
 		vec![tester.multiaddr_with_peer_id(1)],
 		tester.remote_authority_public.clone().into(),
 		&tester.remote_key_store,
-		Some(&tester.remote_node_key),
+		Some(&TestSigner { keypair: &tester.remote_node_key }),
 	));
 	// tamper with the signature
 	let mut record = schema::SignedAuthorityRecord::decode(kv_pairs[0].1.as_slice()).unwrap();
@@ -808,7 +811,7 @@ fn lookup_throttling() {
 					None,
 				)
 				.await;
-				sc_network::DhtEvent::ValueFound(kv_pairs)
+				DhtEvent::ValueFound(kv_pairs)
 			};
 			dht_event_tx.send(dht_event).await.expect("Channel has capacity of 1.");
 
@@ -822,7 +825,7 @@ fn lookup_throttling() {
 
 			// Make second one fail.
 			let remote_hash = network.get_value_call.lock().unwrap().pop().unwrap();
-			let dht_event = sc_network::DhtEvent::ValueNotFound(remote_hash);
+			let dht_event = DhtEvent::ValueNotFound(remote_hash);
 			dht_event_tx.send(dht_event).await.expect("Channel has capacity of 1.");
 
 			// Assert worker to trigger another lookup.