Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Ensure the listen addresses are consistent with the transport #6436

Merged
merged 12 commits into from
Jun 23, 2020
5 changes: 4 additions & 1 deletion client/network/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ pub enum Error {
second_id: PeerId,
},
/// Prometheus metrics error.
Prometheus(prometheus_endpoint::PrometheusError)
Prometheus(prometheus_endpoint::PrometheusError),
/// Invalid configuration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this pattern of using Strings for error messages is common in Substrate, but to me it is a bad programming practice nonetheless.
Please change to an enum, or at least mention what the string is.

Suggested change
/// Invalid configuration.
/// Invalid configuration. Contains a human-readable error message.

Copy link
Contributor

@tomaka tomaka Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Error::DuplicateBootnode is normally part of InvalidConfiguration as well.
I know I'm probably being annoying, but I'm trying to remove quick hacks from sc-network as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all!! This is fine, I will improve it! Thanks for the feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomaka voila. I made the error more "generic". It's a bit less descriptive but I think it's still good. I think it would have been weird to have 2 different errors to make the distinction in the human readable text.

InvalidConfiguration(String),
}

// Make `Debug` use the `Display` implementation.
Expand All @@ -65,6 +67,7 @@ impl std::error::Error for Error {
Error::Client(ref err) => Some(err),
Error::DuplicateBootnode { .. } => None,
Error::Prometheus(ref err) => Some(err),
Error::InvalidConfiguration(_) => None,
}
}
}
55 changes: 55 additions & 0 deletions client/network/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,20 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
/// for the network processing to advance. From it, you can extract a `NetworkService` using
/// `worker.service()`. The `NetworkService` can be shared through the codebase.
pub fn new(params: Params<B, H>) -> Result<NetworkWorker<B, H>, Error> {
// Ensure the listen addresses are consistent with the transport.
ensure_addresses_consistent_with_transport(
params.network_config.listen_addresses.iter(),
&params.network_config.transport,
)?;
ensure_addresses_consistent_with_transport(
params.network_config.boot_nodes.iter().map(|x| &x.multiaddr),
&params.network_config.transport,
)?;
ensure_addresses_consistent_with_transport(
params.network_config.reserved_nodes.iter().map(|x| &x.multiaddr),
&params.network_config.transport,
)?;

let (to_worker, from_worker) = tracing_unbounded("mpsc_network_worker");

if let Some(path) = params.network_config.net_config_path {
Expand Down Expand Up @@ -1447,3 +1461,44 @@ impl<'a, B: BlockT, H: ExHashT> Link<B> for NetworkLink<'a, B, H> {
}
}
}

fn ensure_addresses_consistent_with_transport<'a>(
addresses: impl Iterator<Item = &'a Multiaddr>,
transport: &TransportConfig,
) -> Result<(), Error> {
if matches!(transport, TransportConfig::MemoryOnly) {
let invalid_addresses: Vec<_> = addresses
.filter(|x| x.iter()
.any(|y| !matches!(y, libp2p::core::multiaddr::Protocol::Memory(_)))
)
.map(|x| x.to_string())
.collect();

if !invalid_addresses.is_empty() {
return Err(Error::InvalidConfiguration(format!(
"The following network listen addresses are invalid because the transport \
is set on {:?}: {}",
TransportConfig::MemoryOnly,
invalid_addresses.join(", "),
)));
}
} else {
let invalid_addresses: Vec<_> = addresses
.filter(|x| x.iter()
.any(|y| matches!(y, libp2p::core::multiaddr::Protocol::Memory(_)))
)
.map(|x| x.to_string())
.collect();

if !invalid_addresses.is_empty() {
return Err(Error::InvalidConfiguration(format!(
"The following network listen addresses are invalid because the transport \
is not set on {:?}: {}",
TransportConfig::MemoryOnly,
invalid_addresses.join(", "),
)));
}
}

Ok(())
}
91 changes: 91 additions & 0 deletions client/network/src/service/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

use crate::{config, Event, NetworkService, NetworkWorker};

use libp2p::PeerId;
use futures::prelude::*;
use sp_runtime::traits::{Block as BlockT, Header as _};
use std::{sync::Arc, time::Duration};
Expand Down Expand Up @@ -138,6 +139,7 @@ fn build_nodes_one_proto()

let (node2, events_stream2) = build_test_full_node(config::NetworkConfiguration {
notifications_protocols: vec![(ENGINE_ID, From::from(&b"/foo"[..]))],
listen_addresses: vec![],
reserved_nodes: vec![config::MultiaddrWithPeerId {
multiaddr: listen_addr,
peer_id: node1.local_peer_id().clone(),
Expand Down Expand Up @@ -342,3 +344,92 @@ fn lots_of_incoming_peers_works() {
future::join_all(background_tasks_to_wait).await
});
}

#[test]
#[should_panic(expected = "the transport is set on MemoryOnly")]
fn ensure_listen_addresses_consistent_with_transport_memory() {
let listen_addr = config::build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(0_u16)];

let _ = build_test_full_node(config::NetworkConfiguration {
listen_addresses: vec![listen_addr.clone()],
transport: config::TransportConfig::MemoryOnly,
.. config::NetworkConfiguration::new("test-node", "test-client", Default::default(), None)
});
}

#[test]
#[should_panic(expected = "the transport is not set on MemoryOnly")]
fn ensure_listen_addresses_consistent_with_transport_not_memory() {
let listen_addr = config::build_multiaddr![Memory(rand::random::<u64>())];

let _ = build_test_full_node(config::NetworkConfiguration {
listen_addresses: vec![listen_addr.clone()],
.. config::NetworkConfiguration::new("test-node", "test-client", Default::default(), None)
});
}

#[test]
#[should_panic(expected = "the transport is set on MemoryOnly")]
fn ensure_boot_node_addresses_consistent_with_transport_memory() {
let listen_addr = config::build_multiaddr![Memory(rand::random::<u64>())];
let boot_node = config::MultiaddrWithPeerId {
multiaddr: config::build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(0_u16)],
peer_id: PeerId::random(),
};

let _ = build_test_full_node(config::NetworkConfiguration {
listen_addresses: vec![listen_addr.clone()],
transport: config::TransportConfig::MemoryOnly,
boot_nodes: vec![boot_node],
.. config::NetworkConfiguration::new("test-node", "test-client", Default::default(), None)
});
}

#[test]
#[should_panic(expected = "the transport is not set on MemoryOnly")]
fn ensure_boot_node_addresses_consistent_with_transport_not_memory() {
let listen_addr = config::build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(0_u16)];
let boot_node = config::MultiaddrWithPeerId {
multiaddr: config::build_multiaddr![Memory(rand::random::<u64>())],
peer_id: PeerId::random(),
};

let _ = build_test_full_node(config::NetworkConfiguration {
listen_addresses: vec![listen_addr.clone()],
boot_nodes: vec![boot_node],
.. config::NetworkConfiguration::new("test-node", "test-client", Default::default(), None)
});
}

#[test]
#[should_panic(expected = "the transport is set on MemoryOnly")]
fn ensure_reserved_node_addresses_consistent_with_transport_memory() {
let listen_addr = config::build_multiaddr![Memory(rand::random::<u64>())];
let reserved_node = config::MultiaddrWithPeerId {
multiaddr: config::build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(0_u16)],
peer_id: PeerId::random(),
};

let _ = build_test_full_node(config::NetworkConfiguration {
listen_addresses: vec![listen_addr.clone()],
transport: config::TransportConfig::MemoryOnly,
reserved_nodes: vec![reserved_node],
.. config::NetworkConfiguration::new("test-node", "test-client", Default::default(), None)
});
}

#[test]
#[should_panic(expected = "the transport is not set on MemoryOnly")]
fn ensure_reserved_node_addresses_consistent_with_transport_not_memory() {
let listen_addr = config::build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(0_u16)];
let reserved_node = config::MultiaddrWithPeerId {
multiaddr: config::build_multiaddr![Memory(rand::random::<u64>())],
peer_id: PeerId::random(),
};

let _ = build_test_full_node(config::NetworkConfiguration {
listen_addresses: vec![listen_addr.clone()],
reserved_nodes: vec![reserved_node],
.. config::NetworkConfiguration::new("test-node", "test-client", Default::default(), None)
});
}