Split the Roles in three types (#5520)

* Split the Roles bitfield in three

* Forgot to include some changes

* Fix cli test

* More test fixes

* Oh God, merging master broke other tests

* Didn't run the doctests

* Address review

* I'm trying to fix the build blindly because it's taking a good hour to compile on my machine

* Address some review

* Also update the peerset's API to make sense

* Fix peerset tests

* Fix browser node

* client: distinguish between local and network authority

Co-authored-by: André Silva <andre.beat@gmail.com>

substrate/client/cli/src/commands/build_spec_cmd.rs

@@ -16,7 +16,7 @@
 
 use structopt::StructOpt;
 use log::info;
-use sc_network::config::build_multiaddr;
+use sc_network::config::{build_multiaddr, MultiaddrWithPeerId};
 use sc_service::{Configuration, ChainSpec};
 
 use crate::error;
@@ -60,11 +60,10 @@ impl BuildSpecCmd {
 		if spec.boot_nodes().is_empty() && !self.disable_default_bootnode {
 			let keys = config.network.node_key.into_keypair()?;
 			let peer_id = keys.public().into_peer_id();
-			let addr = build_multiaddr![
-				Ip4([127, 0, 0, 1]),
-				Tcp(30333u16),
-				P2p(peer_id)
-			];
+			let addr = MultiaddrWithPeerId {
+				multiaddr: build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(30333u16)],
+				peer_id,
+			};
 			spec.add_boot_node(addr)
 		}
 

substrate/client/cli/src/commands/check_block_cmd.rs

@@ -18,7 +18,7 @@ use std::fmt::Debug;
 use std::str::FromStr;
 use structopt::StructOpt;
 use sc_service::{
-	Configuration, ServiceBuilderCommand, Roles, ChainSpec,
+	Configuration, ServiceBuilderCommand, Role, ChainSpec,
 };
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 use sp_runtime::generic::BlockId;
@@ -93,7 +93,7 @@ impl CheckBlockCmd {
 		F: FnOnce(&str) -> Result<Box<dyn ChainSpec>, String>,
 	{
 		self.shared_params.update_config(&mut config, spec_factory, version)?;
-		self.import_params.update_config(&mut config, Roles::FULL, self.shared_params.dev)?;
+		self.import_params.update_config(&mut config, &Role::Full, self.shared_params.dev)?;
 		config.use_in_memory_keystore()?;
 
 		Ok(())

substrate/client/cli/src/commands/export_blocks_cmd.rs

@@ -22,7 +22,7 @@ use log::info;
 use structopt::StructOpt;
 use sc_service::{
 	Configuration, ServiceBuilderCommand, ChainSpec,
-	config::DatabaseConfig, Roles,
+	config::DatabaseConfig, Role,
 };
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
@@ -105,7 +105,7 @@ impl ExportBlocksCmd {
 		F: FnOnce(&str) -> Result<Box<dyn ChainSpec>, String>,
 	{
 		self.shared_params.update_config(&mut config, spec_factory, version)?;
-		self.pruning_params.update_config(&mut config, Roles::FULL, true)?;
+		self.pruning_params.update_config(&mut config, &Role::Full, true)?;
 		config.use_in_memory_keystore()?;
 
 		Ok(())

substrate/client/cli/src/commands/import_blocks_cmd.rs

@@ -20,7 +20,7 @@ use std::fs;
 use std::path::PathBuf;
 use structopt::StructOpt;
 use sc_service::{
-	Configuration, ServiceBuilderCommand, ChainSpec, Roles,
+	Configuration, ServiceBuilderCommand, ChainSpec, Role,
 };
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
@@ -95,7 +95,7 @@ impl ImportBlocksCmd {
 		F: FnOnce(&str) -> Result<Box<dyn ChainSpec>, String>,
 	{
 		self.shared_params.update_config(&mut config, spec_factory, version)?;
-		self.import_params.update_config(&mut config, Roles::FULL, self.shared_params.dev)?;
+		self.import_params.update_config(&mut config, &Role::Full, self.shared_params.dev)?;
 		config.use_in_memory_keystore()?;
 
 		Ok(())

substrate/client/cli/src/commands/revert_cmd.rs

@@ -17,7 +17,7 @@
 use std::fmt::Debug;
 use structopt::StructOpt;
 use sc_service::{
-	Configuration, ServiceBuilderCommand, ChainSpec, Roles,
+	Configuration, ServiceBuilderCommand, ChainSpec, Role,
 };
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 
@@ -71,7 +71,7 @@ impl RevertCmd {
 		F: FnOnce(&str) -> Result<Box<dyn ChainSpec>, String>,
 	{
 		self.shared_params.update_config(&mut config, spec_factory, version)?;
-		self.pruning_params.update_config(&mut config, Roles::FULL, true)?;
+		self.pruning_params.update_config(&mut config, &Role::Full, true)?;
 		config.use_in_memory_keystore()?;
 
 		Ok(())

substrate/client/cli/src/commands/runcmd.rs

@@ -23,8 +23,8 @@ use names::{Generator, Name};
 use regex::Regex;
 use chrono::prelude::*;
 use sc_service::{
-	AbstractService, Configuration, ChainSpec, Roles,
-	config::{KeystoreConfig, PrometheusConfig},
+	AbstractService, Configuration, ChainSpec, Role,
+	config::{MultiaddrWithPeerId, KeystoreConfig, PrometheusConfig},
 };
 use sc_telemetry::TelemetryEndpoints;
 
@@ -78,9 +78,10 @@ pub struct RunCmd {
 	/// available to relay to private nodes.
 	#[structopt(
 		long = "sentry",
-		conflicts_with_all = &[ "validator", "light" ]
+		conflicts_with_all = &[ "validator", "light" ],
+		parse(try_from_str)
 	)]
-	pub sentry: bool,
+	pub sentry: Vec<MultiaddrWithPeerId>,
 
 	/// Disable GRANDPA voter when running in validator mode, otherwise disable the GRANDPA observer.
 	#[structopt(long = "no-grandpa")]
@@ -329,18 +330,20 @@ impl RunCmd {
 		let keyring = self.get_keyring();
 		let is_dev = self.shared_params.dev;
 		let is_light = self.light;
-		let is_authority = (self.validator || self.sentry || is_dev || keyring.is_some())
+		let is_authority = (self.validator || is_dev || keyring.is_some())
 			&& !is_light;
 		let role =
 			if is_light {
-				sc_service::Roles::LIGHT
+				sc_service::Role::Light
 			} else if is_authority {
-				sc_service::Roles::AUTHORITY
+				sc_service::Role::Authority { sentry_nodes: self.network_config.sentry_nodes.clone() }
+			} else if !self.sentry.is_empty() {
+				sc_service::Role::Sentry { validators: self.sentry.clone() }
 			} else {
-				sc_service::Roles::FULL
+				sc_service::Role::Full
 			};
 
-		self.import_params.update_config(&mut config, role, is_dev)?;
+		self.import_params.update_config(&mut config, &role, is_dev)?;
 
 		config.name = match (self.name.as_ref(), keyring) {
 			(Some(name), _) => name.to_string(),
@@ -356,17 +359,14 @@ impl RunCmd {
 			));
 		}
 
-		// set sentry mode (i.e. act as an authority but **never** actively participate)
-		config.sentry_mode = self.sentry;
-
-		config.offchain_worker = match (&self.offchain_worker, role) {
-			(OffchainWorkerEnabled::WhenValidating, sc_service::Roles::AUTHORITY) => true,
+		config.offchain_worker = match (&self.offchain_worker, &role) {
+			(OffchainWorkerEnabled::WhenValidating, sc_service::Role::Authority { .. }) => true,
 			(OffchainWorkerEnabled::Always, _) => true,
 			(OffchainWorkerEnabled::Never, _) => false,
 			(OffchainWorkerEnabled::WhenValidating, _) => false,
 		};
 
-		config.roles = role;
+		config.role = role;
 		config.disable_grandpa = self.no_grandpa;
 
 		let client_id = config.client_id();
@@ -463,10 +463,10 @@ impl RunCmd {
 		info!("❤️  by {}, {}-{}", version.author, version.copyright_start_year, Local::today().year());
 		info!("📋 Chain specification: {}", config.expect_chain_spec().name());
 		info!("🏷  Node name: {}", config.name);
-		info!("👤 Roles: {}", config.display_role());
+		info!("👤 Role: {}", config.display_role());
 
-		match config.roles {
-			Roles::LIGHT => run_service_until_exit(
+		match config.role {
+			Role::Light => run_service_until_exit(
 				config,
 				new_light,
 			),
@@ -688,7 +688,7 @@ mod tests {
 			"test",
 			"test-id",
 			|| (),
-			vec!["boo".to_string()],
+			vec!["/ip4/127.0.0.1/tcp/30333/p2p/QmdSHZLmwEL5Axz5JvWNE2mmxU7qyd7xHBFpyUfktgAdg7".parse().unwrap()],
 			Some(TelemetryEndpoints::new(vec![("wss://foo/bar".to_string(), 42)])
 				.expect("provided url should be valid")),
 			None,

substrate/client/cli/src/params/import_params.rs

@@ -82,7 +82,7 @@ impl ImportParams {
 	pub fn update_config(
 		&self,
 		mut config: &mut Configuration,
-		role: sc_service::Roles,
+		role: &sc_service::Role,
 		is_dev: bool,
 	) -> error::Result<()> {
 		use sc_client_api::execution_extensions::ExecutionStrategies;

substrate/client/cli/src/params/network_configuration_params.rs

@@ -19,7 +19,7 @@ use std::iter;
 use std::net::Ipv4Addr;
 use structopt::StructOpt;
 use sc_network::{
-	config::{NonReservedPeerMode, TransportConfig}, multiaddr::Protocol,
+	config::{MultiaddrWithPeerId, NonReservedPeerMode, TransportConfig}, multiaddr::Protocol, Multiaddr,
 };
 use sc_service::Configuration;
 
@@ -30,12 +30,12 @@ use crate::params::node_key_params::NodeKeyParams;
 #[derive(Debug, StructOpt, Clone)]
 pub struct NetworkConfigurationParams {
 	/// Specify a list of bootnodes.
-	#[structopt(long = "bootnodes", value_name = "URL")]
-	pub bootnodes: Vec<String>,
+	#[structopt(long = "bootnodes", value_name = "ADDR")]
+	pub bootnodes: Vec<MultiaddrWithPeerId>,
 
 	/// Specify a list of reserved node addresses.
-	#[structopt(long = "reserved-nodes", value_name = "URL")]
-	pub reserved_nodes: Vec<String>,
+	#[structopt(long = "reserved-nodes", value_name = "ADDR")]
+	pub reserved_nodes: Vec<MultiaddrWithPeerId>,
 
 	/// Whether to only allow connections to/from reserved nodes.
 	///
@@ -47,14 +47,14 @@ pub struct NetworkConfigurationParams {
 	/// Specify a list of sentry node public addresses.
 	#[structopt(
 		long = "sentry-nodes",
-		value_name = "URL",
+		value_name = "ADDR",
 		conflicts_with_all = &[ "sentry" ]
 	)]
-	pub sentry_nodes: Vec<String>,
+	pub sentry_nodes: Vec<MultiaddrWithPeerId>,
 
 	/// Listen on this multiaddress.
 	#[structopt(long = "listen-addr", value_name = "LISTEN_ADDR")]
-	pub listen_addr: Vec<String>,
+	pub listen_addr: Vec<Multiaddr>,
 
 	/// Specify p2p protocol TCP port.
 	///
@@ -117,13 +117,7 @@ impl NetworkConfigurationParams {
 			config.network.non_reserved_mode = NonReservedPeerMode::Deny;
 		}
 
-		config.network.sentry_nodes.extend(self.sentry_nodes.clone());
-
-		for addr in self.listen_addr.iter() {
-			let addr = addr.parse().ok().ok_or(error::Error::InvalidListenMultiaddress)?;
-			config.network.listen_addresses.push(addr);
-		}
-
+		config.network.listen_addresses.extend(self.listen_addr.iter().cloned());
 		if config.network.listen_addresses.is_empty() {
 			let port = match self.port {
 				Some(port) => port,

substrate/client/cli/src/params/pruning_params.rs

@@ -36,7 +36,7 @@ impl PruningParams {
 	pub fn update_config(
 		&self,
 		mut config: &mut Configuration,
-		role: sc_service::Roles,
+		role: &sc_service::Role,
 		unsafe_pruning: bool,
 	) -> error::Result<()> {
 		// by default we disable pruning if the node is an authority (i.e.
@@ -45,10 +45,10 @@ impl PruningParams {
 		// unless `unsafe_pruning` is set.
 		config.pruning = match &self.pruning {
 			Some(ref s) if s == "archive" => PruningMode::ArchiveAll,
-			None if role == sc_service::Roles::AUTHORITY => PruningMode::ArchiveAll,
+			None if role.is_network_authority() => PruningMode::ArchiveAll,
 			None => PruningMode::default(),
 			Some(s) => {
-				if role == sc_service::Roles::AUTHORITY && !unsafe_pruning {
+				if role.is_network_authority() && !unsafe_pruning {
 					return Err(error::Error::Input(
 						"Validators should run with state pruning disabled (i.e. archive). \
 						You can ignore this check with `--unsafe-pruning`.".to_string()