Throw an error if a bootnode is registered with two different peer ids (#3891)

* Throw an error if a bootnode is registered with two different peer ids

* Rename error

* Fix compilation :(

* Review feedback

substrate/core/cli/src/lib.rs

@@ -269,22 +269,24 @@ pub struct ParseAndPrepareRun<'a, RP> {
 
 impl<'a, RP> ParseAndPrepareRun<'a, RP> {
 	/// Runs the command and runs the main client.
-	pub fn run<C, G, E, S, Exit, RS>(
+	pub fn run<C, G, CE, S, Exit, RS, E>(
 		self,
 		spec_factory: S,
 		exit: Exit,
 		run_service: RS,
 	) -> error::Result<()>
-	where S: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
+	where
+		S: FnOnce(&str) -> Result<Option<ChainSpec<G, CE>>, String>,
+		E: Into<error::Error>,
 		RP: StructOpt + Clone,
 		C: Default,
 		G: RuntimeGenesis,
-		E: ChainSpecExtension,
+		CE: ChainSpecExtension,
 		Exit: IntoExit,
-		RS: FnOnce(Exit, RunCmd, RP, Configuration<C, G, E>) -> Result<(), String>
+		RS: FnOnce(Exit, RunCmd, RP, Configuration<C, G, CE>) -> Result<(), E>
 	{
 		let config = create_run_node_config(
-			self.params.left.clone(), spec_factory, self.impl_name, self.version
+			self.params.left.clone(), spec_factory, self.impl_name, self.version,
 		)?;
 
 		run_service(exit, self.params.left, self.params.right, config).map_err(Into::into)
@@ -633,7 +635,7 @@ fn fill_config_keystore_password<C, G, E>(
 }
 
 fn create_run_node_config<C, G, E, S>(
-	cli: RunCmd, spec_factory: S, impl_name: &'static str, version: &VersionInfo
+	cli: RunCmd, spec_factory: S, impl_name: &'static str, version: &VersionInfo,
 ) -> error::Result<Configuration<C, G, E>>
 where
 	C: Default,

substrate/core/network/src/error.rs

@@ -18,16 +18,42 @@
 
 use client;
 
+use libp2p::{PeerId, Multiaddr};
+
+use std::fmt;
+
 /// Result type alias for the network.
 pub type Result<T> = std::result::Result<T, Error>;
 
 /// Error type for the network.
-#[derive(Debug, derive_more::Display, derive_more::From)]
+#[derive(derive_more::Display, derive_more::From)]
 pub enum Error {
 	/// Io error
 	Io(std::io::Error),
 	/// Client error
 	Client(client::error::Error),
+	/// The same bootnode (based on address) is registered with two different peer ids.
+	#[display(
+		fmt = "The same bootnode (`{}`) is registered with two different peer ids: `{}` and `{}`",
+		address,
+		first_id,
+		second_id,
+	)]
+	DuplicateBootnode {
+		/// The address of the bootnode.
+		address: Multiaddr,
+		/// The first peer id that was found for the bootnode.
+		first_id: PeerId,
+		/// The second peer id that was found for the bootnode.
+		second_id: PeerId,
+	},
+}
+
+// Make `Debug` use the `Display` implementation.
+impl fmt::Debug for Error {
+	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+		fmt::Display::fmt(self, f)
+	}
 }
 
 impl std::error::Error for Error {
@@ -35,6 +61,7 @@ impl std::error::Error for Error {
 		match self {
 			Error::Io(ref err) => Some(err),
 			Error::Client(ref err) => Some(err),
+			Error::DuplicateBootnode { .. } => None,
 		}
 	}
 }

substrate/core/network/src/service.rs

@@ -52,14 +52,11 @@ use crate::protocol::specialization::NetworkSpecialization;
 use crate::protocol::sync::SyncState;
 
 /// Minimum Requirements for a Hash within Networking
-pub trait ExHashT:
-	::std::hash::Hash + Eq + ::std::fmt::Debug + Clone + Send + Sync + 'static
-{
-}
+pub trait ExHashT: std::hash::Hash + Eq + std::fmt::Debug + Clone + Send + Sync + 'static {}
+
 impl<T> ExHashT for T where
-	T: ::std::hash::Hash + Eq + ::std::fmt::Debug + Clone + Send + Sync + 'static
-{
-}
+	T: std::hash::Hash + Eq + std::fmt::Debug + Clone + Send + Sync + 'static
+{}
 
 /// Transaction pool interface
 pub trait TransactionPool<H: ExHashT, B: BlockT>: Send + Sync {
@@ -152,6 +149,23 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkWorker
 			}
 		}
 
+		// Check for duplicate bootnodes.
+		known_addresses.iter()
+			.try_for_each(|(peer_id, addr)|
+				if let Some(other) = known_addresses
+					.iter()
+					.find(|o| o.1 == *addr && o.0 != *peer_id)
+				{
+					Err(Error::DuplicateBootnode {
+						address: addr.clone(),
+						first_id: peer_id.clone(),
+						second_id: other.0.clone(),
+					})
+				} else {
+					Ok(())
+				}
+			)?;
+
 		// Initialize the reserved peers.
 		for reserved in params.network_config.reserved_nodes.iter() {
 			if let Ok((peer_id, addr)) = parse_str_addr(reserved) {
@@ -553,8 +567,9 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkServic
 	}
 }
 
-impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT>
-	::consensus::SyncOracle for NetworkService<B, S, H> {
+impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> consensus::SyncOracle
+	for NetworkService<B, S, H>
+{
 	fn is_major_syncing(&mut self) -> bool {
 		NetworkService::is_major_syncing(self)
 	}
@@ -564,8 +579,9 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT>
 	}
 }
 
-impl<'a, B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT>
-	::consensus::SyncOracle for &'a NetworkService<B, S, H> {
+impl<'a, B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> consensus::SyncOracle
+	for &'a NetworkService<B, S, H>
+{
 	fn is_major_syncing(&mut self) -> bool {
 		NetworkService::is_major_syncing(self)
 	}

substrate/node-template/src/cli.rs

@@ -29,15 +29,15 @@ pub fn run<I, T, E>(args: I, exit: E, version: VersionInfo) -> error::Result<()>
 			match config.roles {
 				ServiceRoles::LIGHT => run_until_exit(
 					runtime,
-				 	service::new_light(config).map_err(|e| format!("{:?}", e))?,
+					service::new_light(config)?,
 					exit
 				),
 				_ => run_until_exit(
 					runtime,
-					service::new_full(config).map_err(|e| format!("{:?}", e))?,
+					service::new_full(config)?,
 					exit
 				),
-			}.map_err(|e| format!("{:?}", e))
+			}
 		}),
 		ParseAndPrepare::BuildSpec(cmd) => cmd.run(load_spec),
 		ParseAndPrepare::ExportBlocks(cmd) => cmd.run_with_builder(|config: Config<_>|

substrate/node-template/src/main.rs

@@ -10,7 +10,7 @@ mod cli;
 
 pub use substrate_cli::{VersionInfo, IntoExit, error};
 
-fn main() {
+fn main() -> Result<(), cli::error::Error> {
 	let version = VersionInfo {
 		name: "Substrate Node",
 		commit: env!("VERGEN_SHA_SHORT"),
@@ -21,8 +21,5 @@ fn main() {
 		support_url: "support.anonymous.an",
 	};
 
-	if let Err(e) = cli::run(::std::env::args(), cli::Exit, version) {
-		eprintln!("Fatal error: {}\n\n{:?}", e, e);
-		std::process::exit(1)
-	}
+	cli::run(std::env::args(), cli::Exit, version)
 }

substrate/node/cli/src/lib.rs

@@ -172,15 +172,15 @@ pub fn run<I, T, E>(args: I, exit: E, version: cli::VersionInfo) -> error::Resul
 			match config.roles {
 				ServiceRoles::LIGHT => run_until_exit(
 					runtime,
-					service::new_light(config).map_err(|e| format!("{:?}", e))?,
+					service::new_light(config)?,
 					exit
 				),
 				_ => run_until_exit(
 					runtime,
-					service::new_full(config).map_err(|e| format!("{:?}", e))?,
+					service::new_full(config)?,
 					exit
 				),
-			}.map_err(|e| format!("{:?}", e))
+			}
 		}),
 		ParseAndPrepare::BuildSpec(cmd) => cmd.run(load_spec),
 		ParseAndPrepare::ExportBlocks(cmd) => cmd.run_with_builder(|config: Config<_, _>|

substrate/node/src/main.rs

@@ -43,7 +43,7 @@ impl cli::IntoExit for Exit {
 	}
 }
 
-fn main() {
+fn main() -> Result<(), cli::error::Error> {
 	let version = VersionInfo {
 		name: "Substrate Node",
 		commit: env!("VERGEN_SHA_SHORT"),
@@ -54,8 +54,5 @@ fn main() {
 		support_url: "https://github.com/paritytech/substrate/issues/new",
 	};
 
-	if let Err(e) = cli::run(::std::env::args(), Exit, version) {
-		eprintln!("Fatal error: {}\n\n{:?}", e, e);
-		std::process::exit(1)
-	}
+	cli::run(std::env::args(), Exit, version)
 }