From 48d62f85c78183817a9e96ba40b2038bd69e883a Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 13 Apr 2023 16:40:34 +0400 Subject: [PATCH 1/2] remove `block_announce_config` from `Params` and treat it instead as any other `Notifications` protocol Refs #13542 --- client/network/src/config.rs | 3 --- client/network/src/protocol.rs | 21 ++++++--------------- client/network/src/service.rs | 8 ++------ client/network/test/src/lib.rs | 4 +++- client/service/src/builder.rs | 7 +++++-- 5 files changed, 16 insertions(+), 27 deletions(-) diff --git a/client/network/src/config.rs b/client/network/src/config.rs index e00bfac79f650..1af466933d502 100644 --- a/client/network/src/config.rs +++ b/client/network/src/config.rs @@ -722,9 +722,6 @@ pub struct Params { /// Registry for recording prometheus metrics to. pub metrics_registry: Option, - /// Block announce protocol configuration - pub block_announce_config: NonDefaultSetConfig, - /// TX channel for direct communication with `SyncingEngine` and `Protocol`. pub tx: TracingUnboundedSender>, diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index a7e6f36ef6215..d96c2ba44417e 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -42,7 +42,6 @@ use sp_runtime::traits::Block as BlockT; use std::{ collections::{HashMap, HashSet, VecDeque}, future::Future, - iter, pin::Pin, task::Poll, }; @@ -104,7 +103,6 @@ impl Protocol { pub fn new( roles: Roles, network_config: &config::NetworkConfiguration, - block_announces_protocol: config::NonDefaultSetConfig, tx: TracingUnboundedSender>, ) -> error::Result<(Self, sc_peerset::PeersetHandle, Vec<(PeerId, Multiaddr)>)> { let mut known_addresses = Vec::new(); @@ -162,21 +160,12 @@ impl Protocol { let behaviour = { Notifications::new( peerset, - // NOTE: Block announcement protocol is still very much hardcoded into `Protocol`. - // This protocol must be the first notification protocol given to - // `Notifications` - iter::once(notifications::ProtocolConfig { - name: block_announces_protocol.notifications_protocol.clone(), - fallback_names: block_announces_protocol.fallback_names.clone(), - handshake: block_announces_protocol.handshake.as_ref().unwrap().to_vec(), - max_notification_size: block_announces_protocol.max_notification_size, - }) - .chain(network_config.extra_sets.iter().map(|s| notifications::ProtocolConfig { + network_config.extra_sets.iter().map(|s| notifications::ProtocolConfig { name: s.notifications_protocol.clone(), fallback_names: s.fallback_names.clone(), handshake: s.handshake.as_ref().map_or(roles.encode(), |h| (*h).to_vec()), max_notification_size: s.max_notification_size, - })), + }), ) }; @@ -184,8 +173,10 @@ impl Protocol { pending_messages: VecDeque::new(), peerset_handle: peerset_handle.clone(), behaviour, - notification_protocols: iter::once(block_announces_protocol.notifications_protocol) - .chain(network_config.extra_sets.iter().map(|s| s.notifications_protocol.clone())) + notification_protocols: network_config + .extra_sets + .iter() + .map(|s| s.notifications_protocol.clone()) .collect(), bad_handshake_substreams: Default::default(), peers: HashMap::new(), diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 9708b24d29b52..e9de3ca3c797d 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -224,12 +224,8 @@ where local_peer_id.to_base58(), ); - let (protocol, peerset_handle, mut known_addresses) = Protocol::new( - From::from(¶ms.role), - ¶ms.network_config, - params.block_announce_config, - params.tx, - )?; + let (protocol, peerset_handle, mut known_addresses) = + Protocol::new(From::from(¶ms.role), ¶ms.network_config, params.tx)?; // List of multiaddresses that we know in the network. let mut boot_node_ids = HashSet::new(); diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index f85d6ed63c247..1be315e1ce3ba 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -918,6 +918,9 @@ where let sync_service_import_queue = Box::new(sync_service.clone()); let sync_service = Arc::new(sync_service.clone()); + // This protocol MUST BE the first notification protocol given to `Notifications` + network_config.extra_sets.insert(0, block_announce_config); + let genesis_hash = client.hash(Zero::zero()).ok().flatten().expect("Genesis block exists; qed"); let network = NetworkWorker::new(sc_network::config::Params { @@ -930,7 +933,6 @@ where protocol_id, fork_id, metrics_registry: None, - block_announce_config, tx, request_response_protocol_configs: [ block_request_protocol_config, diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 415fa03a10020..9a086efb54a75 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -887,7 +887,6 @@ where protocol_id: protocol_id.clone(), fork_id: config.chain_spec.fork_id().map(ToOwned::to_owned), metrics_registry: config.prometheus_config.as_ref().map(|config| config.registry.clone()), - block_announce_config, tx, request_response_protocol_configs: request_response_protocol_configs .into_iter() @@ -901,6 +900,9 @@ where .collect::>(), }; + // This protocol MUST BE the first notification protocol given to `Notifications` + network_params.network_config.extra_sets.insert(0, block_announce_config); + // crate transactions protocol and add it to the list of supported protocols of `network_params` let transactions_handler_proto = sc_network_transactions::TransactionsHandlerPrototype::new( protocol_id.clone(), @@ -911,10 +913,11 @@ where .expect("Genesis block exists; qed"), config.chain_spec.fork_id(), ); + network_params .network_config .extra_sets - .insert(0, transactions_handler_proto.set_config()); + .insert(1, transactions_handler_proto.set_config()); let has_bootnodes = !network_params.network_config.boot_nodes.is_empty(); let network_mut = sc_network::NetworkWorker::new(network_params)?; From 6f2b3fb4523c6370269a4a9503e852bfc32aa5a8 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 13 Apr 2023 16:55:59 +0400 Subject: [PATCH 2/2] fix network test --- client/network/test/src/service.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/network/test/src/service.rs b/client/network/test/src/service.rs index 5871860a7c4a6..f2889480cc5ad 100644 --- a/client/network/test/src/service.rs +++ b/client/network/test/src/service.rs @@ -114,7 +114,7 @@ impl TestNetworkBuilder { |v| v.clone(), ); - let network_config = self.config.unwrap_or(config::NetworkConfiguration { + let mut network_config = self.config.unwrap_or(config::NetworkConfiguration { extra_sets: vec![config::NonDefaultSetConfig { notifications_protocol: PROTOCOL_NAME.into(), fallback_names: Vec::new(), @@ -196,6 +196,8 @@ impl TestNetworkBuilder { rx, ) .unwrap(); + // This protocol MUST BE the first notification protocol given to `Notifications` + network_config.extra_sets.insert(0, block_announce_config); let mut link = self.link.unwrap_or(Box::new(chain_sync_service.clone())); let genesis_hash = client.hash(Zero::zero()).ok().flatten().expect("Genesis block exists; qed"); @@ -203,7 +205,6 @@ impl TestNetworkBuilder { substrate_test_runtime_client::runtime::Block, substrate_test_runtime_client::runtime::Hash, >::new(config::Params:: { - block_announce_config, role: config::Role::Full, executor: Box::new(|f| { tokio::spawn(f);