Remove superflous parameter overseer_enable_anyways and make parach…

…ain node type more explicit (#7617)

* Remove superflous parameter `overseer_enable_anyways`

We don't need this flag, as we don't need the overseer enabled when the
node isn't a collator or validator.

* Rename `IsCollator` to `IsParachainNode`

`IsParachainNode` is more expressive and also encapsulates the state of
the parachain node being a full node. Some functionality like the
overseer needs to run always when the node runs alongside a parachain
node. The parachain node needs the overseer to e.g. recover PoVs. Other
things like candidate validation or pvf checking are only required for
when the node is running as validator.

* FMT

* Fix CI

cli/src/command.rs

@@ -287,14 +287,13 @@ where
 		let task_manager = service::build_full(
 			config,
 			service::NewFullParams {
-				is_collator: service::IsCollator::No,
+				is_parachain_node: service::IsParachainNode::No,
 				grandpa_pause,
 				jaeger_agent,
 				telemetry_worker_handle: None,
 				node_version,
 				workers_path: cli.run.workers_path,
 				workers_names: None,
-				overseer_enable_anyways: false,
 				overseer_gen,
 				overseer_message_channel_capacity_override: cli
 					.run

node/core/approval-voting/src/lib.rs

@@ -2253,7 +2253,7 @@ where
 		//
 		// 1. This is not a local approval, as we don't store anything new in the approval entry.
 		// 2. The candidate is not newly approved, as we haven't altered the approval entry's
-		//	  approved flag with `mark_approved` above.
+		//    approved flag with `mark_approved` above.
 		// 3. The approver, if any, had already approved the candidate, as we haven't altered the
 		// bitfield.
 		if transition.is_local_approval() || newly_approved || !already_approved_by.unwrap_or(true)

node/core/pvf/execute-worker/src/lib.rs

@@ -55,8 +55,8 @@ use tokio::{io, net::UnixStream};
 //
 // There are quirks to that configuration knob:
 //
-// 1. It only limits the amount of stack space consumed by wasm but does not ensure nor check
-//    that the stack space is actually available.
+// 1. It only limits the amount of stack space consumed by wasm but does not ensure nor check that
+//    the stack space is actually available.
 //
 //    That means, if the calling thread has 1 MiB of stack space left and the wasm code consumes
 //    more, then the wasmtime limit will **not** trigger. Instead, the wasm code will hit the

node/network/approval-distribution/src/lib.rs

@@ -1319,13 +1319,13 @@ impl State {
 			}
 
 			// Here we're leaning on a few behaviors of assignment propagation:
-			//   1. At this point, the only peer we're aware of which has the approval
-			//      message is the source peer.
-			//   2. We have sent the assignment message to every peer in the required routing
-			//      which is aware of this block _unless_ the peer we originally received the
-			//      assignment from was part of the required routing. In that case, we've sent
-			//      the assignment to all aware peers in the required routing _except_ the original
-			//      source of the assignment. Hence the `in_topology_check`.
+			//   1. At this point, the only peer we're aware of which has the approval message is
+			//      the source peer.
+			//   2. We have sent the assignment message to every peer in the required routing which
+			//      is aware of this block _unless_ the peer we originally received the assignment
+			//      from was part of the required routing. In that case, we've sent the assignment
+			//      to all aware peers in the required routing _except_ the original source of the
+			//      assignment. Hence the `in_topology_check`.
 			//   3. Any randomly selected peers have been sent the assignment already.
 			let in_topology = topology
 				.map_or(false, |t| t.local_grid_neighbors().route_to_peer(required_routing, peer));

node/network/approval-distribution/src/tests.rs

@@ -463,8 +463,8 @@ fn delay_reputation_change() {
 /// <https://github.com/paritytech/polkadot/pull/2160#discussion_r547594835>
 ///
 /// 1. Send a view update that removes block B from their view.
-/// 2. Send a message from B that they incur `COST_UNEXPECTED_MESSAGE` for,
-///    but then they receive `BENEFIT_VALID_MESSAGE`.
+/// 2. Send a message from B that they incur `COST_UNEXPECTED_MESSAGE` for, but then they receive
+///    `BENEFIT_VALID_MESSAGE`.
 /// 3. Send all other messages related to B.
 #[test]
 fn spam_attack_results_in_negative_reputation_change() {

node/network/collator-protocol/src/lib.rs

@@ -37,7 +37,7 @@ use polkadot_node_network_protocol::{
 };
 use polkadot_primitives::CollatorPair;
 
-use polkadot_node_subsystem::{errors::SubsystemError, overseer, SpawnedSubsystem};
+use polkadot_node_subsystem::{errors::SubsystemError, overseer, DummySubsystem, SpawnedSubsystem};
 
 mod error;
 
@@ -82,6 +82,8 @@ pub enum ProtocolSide {
 		IncomingRequestReceiver<request_v1::CollationFetchingRequest>,
 		collator_side::Metrics,
 	),
+	/// No protocol side, just disable it.
+	None,
 }
 
 /// The collator protocol subsystem.
@@ -98,24 +100,22 @@ impl CollatorProtocolSubsystem {
 	pub fn new(protocol_side: ProtocolSide) -> Self {
 		Self { protocol_side }
 	}
-
-	async fn run<Context>(self, ctx: Context) -> std::result::Result<(), error::FatalError> {
-		match self.protocol_side {
-			ProtocolSide::Validator { keystore, eviction_policy, metrics } =>
-				validator_side::run(ctx, keystore, eviction_policy, metrics).await,
-			ProtocolSide::Collator(local_peer_id, collator_pair, req_receiver, metrics) =>
-				collator_side::run(ctx, local_peer_id, collator_pair, req_receiver, metrics).await,
-		}
-	}
 }
 
 #[overseer::subsystem(CollatorProtocol, error=SubsystemError, prefix=self::overseer)]
 impl<Context> CollatorProtocolSubsystem {
 	fn start(self, ctx: Context) -> SpawnedSubsystem {
-		let future = self
-			.run(ctx)
-			.map_err(|e| SubsystemError::with_origin("collator-protocol", e))
-			.boxed();
+		let future = match self.protocol_side {
+			ProtocolSide::Validator { keystore, eviction_policy, metrics } =>
+				validator_side::run(ctx, keystore, eviction_policy, metrics)
+					.map_err(|e| SubsystemError::with_origin("collator-protocol", e))
+					.boxed(),
+			ProtocolSide::Collator(local_peer_id, collator_pair, req_receiver, metrics) =>
+				collator_side::run(ctx, local_peer_id, collator_pair, req_receiver, metrics)
+					.map_err(|e| SubsystemError::with_origin("collator-protocol", e))
+					.boxed(),
+			ProtocolSide::None => return DummySubsystem.start(ctx),
+		};
 
 		SpawnedSubsystem { name: "collator-protocol-subsystem", future }
 	}

node/network/gossip-support/src/lib.rs

@@ -183,8 +183,7 @@ where
 	}
 
 	/// 1. Determine if the current session index has changed.
-	/// 2. If it has, determine relevant validators
-	///    and issue a connection request.
+	/// 2. If it has, determine relevant validators and issue a connection request.
 	async fn handle_active_leaves(
 		&mut self,
 		sender: &mut impl overseer::GossipSupportSenderTrait,

node/network/statement-distribution/src/responder.rs

@@ -62,8 +62,8 @@ pub async fn respond(
 		//
 		// 1. We want some requesters to have full data fast, rather then lots of them having them
 		//    late, as each requester having the data will help distributing it.
-		// 2. If we take too long, the requests timing out will not yet have had any data sent,
-		//    thus we wasted no bandwidth.
+		// 2. If we take too long, the requests timing out will not yet have had any data sent, thus
+		//    we wasted no bandwidth.
 		// 3. If the queue is full, requestes will get an immediate error instead of running in a
 		//    timeout, thus requesters can immediately try another peer and be faster.
 		//

node/service/src/lib.rs

@@ -627,7 +627,7 @@ where
 
 #[cfg(feature = "full-node")]
 pub struct NewFullParams<OverseerGenerator: OverseerGen> {
-	pub is_collator: IsCollator,
+	pub is_parachain_node: IsParachainNode,
 	pub grandpa_pause: Option<(u32, u32)>,
 	pub jaeger_agent: Option<std::net::SocketAddr>,
 	pub telemetry_worker_handle: Option<TelemetryWorkerHandle>,
@@ -638,7 +638,6 @@ pub struct NewFullParams<OverseerGenerator: OverseerGen> {
 	pub workers_path: Option<std::path::PathBuf>,
 	/// Optional custom names for the prepare and execute workers.
 	pub workers_names: Option<(String, String)>,
-	pub overseer_enable_anyways: bool,
 	pub overseer_gen: OverseerGenerator,
 	pub overseer_message_channel_capacity_override: Option<usize>,
 	#[allow(dead_code)]
@@ -657,32 +656,46 @@ pub struct NewFull {
 	pub backend: Arc<FullBackend>,
 }
 
-/// Is this node a collator?
+/// Is this node running as in-process node for a parachain node?
 #[cfg(feature = "full-node")]
 #[derive(Clone)]
-pub enum IsCollator {
-	/// This node is a collator.
-	Yes(CollatorPair),
-	/// This node is not a collator.
+pub enum IsParachainNode {
+	/// This node is running as in-process node for a parachain collator.
+	Collator(CollatorPair),
+	/// This node is running as in-process node for a parachain full node.
+	FullNode,
+	/// This node is not running as in-process node for a parachain node, aka a normal relay chain
+	/// node.
 	No,
 }
 
 #[cfg(feature = "full-node")]
-impl std::fmt::Debug for IsCollator {
+impl std::fmt::Debug for IsParachainNode {
 	fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
 		use sp_core::Pair;
 		match self {
-			IsCollator::Yes(pair) => write!(fmt, "Yes({})", pair.public()),
-			IsCollator::No => write!(fmt, "No"),
+			IsParachainNode::Collator(pair) => write!(fmt, "Collator({})", pair.public()),
+			IsParachainNode::FullNode => write!(fmt, "FullNode"),
+			IsParachainNode::No => write!(fmt, "No"),
 		}
 	}
 }
 
 #[cfg(feature = "full-node")]
-impl IsCollator {
-	/// Is this a collator?
+impl IsParachainNode {
+	/// Is this running alongside a collator?
 	fn is_collator(&self) -> bool {
-		matches!(self, Self::Yes(_))
+		matches!(self, Self::Collator(_))
+	}
+
+	/// Is this running alongside a full node?
+	fn is_full_node(&self) -> bool {
+		matches!(self, Self::FullNode)
+	}
+
+	/// Is this node running alongside a relay chain node?
+	fn is_running_alongside_parachain_node(&self) -> bool {
+		self.is_collator() || self.is_full_node()
 	}
 }
 
@@ -696,11 +709,6 @@ pub const AVAILABILITY_CONFIG: AvailabilityConfig = AvailabilityConfig {
 /// This is an advanced feature and not recommended for general use. Generally, `build_full` is
 /// a better choice.
 ///
-/// `overseer_enable_anyways` always enables the overseer, based on the provided
-/// `OverseerGenerator`, regardless of the role the node has. The relay chain selection (longest or
-/// disputes-aware) is still determined based on the role of the node. Likewise for authority
-/// discovery.
-///
 /// `workers_path` is used to get the path to the directory where auxiliary worker binaries reside.
 /// If not specified, the main binary's directory is searched first, then `/usr/lib/polkadot` is
 /// searched. If the path points to an executable rather then directory, that executable is used
@@ -709,14 +717,13 @@ pub const AVAILABILITY_CONFIG: AvailabilityConfig = AvailabilityConfig {
 pub fn new_full<OverseerGenerator: OverseerGen>(
 	mut config: Configuration,
 	NewFullParams {
-		is_collator,
+		is_parachain_node,
 		grandpa_pause,
 		jaeger_agent,
 		telemetry_worker_handle,
 		node_version,
 		workers_path,
 		workers_names,
-		overseer_enable_anyways,
 		overseer_gen,
 		overseer_message_channel_capacity_override,
 		malus_finality_delay: _malus_finality_delay,
@@ -768,8 +775,9 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 	let chain_spec = config.chain_spec.cloned_box();
 
 	let keystore = basics.keystore_container.local_keystore();
-	let auth_or_collator = role.is_authority() || is_collator.is_collator();
-	let pvf_checker_enabled = role.is_authority() && !is_collator.is_collator();
+	let auth_or_collator = role.is_authority() || is_parachain_node.is_collator();
+	// We only need to enable the pvf checker when this is a validator.
+	let pvf_checker_enabled = role.is_authority();
 
 	let select_chain = if auth_or_collator {
 		let metrics =
@@ -832,7 +840,12 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 	let peerset_protocol_names =
 		PeerSetProtocolNames::new(genesis_hash, config.chain_spec.fork_id());
 
-	if auth_or_collator || overseer_enable_anyways {
+	// If this is a validator or running alongside a parachain node, we need to enable the
+	// networking protocols.
+	//
+	// Collators and parachain full nodes require the collator and validator networking to send
+	// collations and to be able to recover PoVs.
+	if role.is_authority() || is_parachain_node.is_running_alongside_parachain_node() {
 		use polkadot_network_bridge::{peer_sets_info, IsAuthority};
 		let is_authority = if role.is_authority() { IsAuthority::Yes } else { IsAuthority::No };
 		for config in peer_sets_info(is_authority, &peerset_protocol_names) {
@@ -910,7 +923,7 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 		slot_duration_millis: slot_duration.as_millis() as u64,
 	};
 
-	let candidate_validation_config = if role.is_authority() && !is_collator.is_collator() {
+	let candidate_validation_config = if role.is_authority() {
 		let (prep_worker_path, exec_worker_path) =
 			workers::determine_workers_paths(workers_path, workers_names, node_version.clone())?;
 		log::info!("🚀 Using prepare-worker binary at: {:?}", prep_worker_path);
@@ -979,46 +992,50 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 	let overseer_client = client.clone();
 	let spawner = task_manager.spawn_handle();
 
-	let authority_discovery_service = if auth_or_collator || overseer_enable_anyways {
-		use futures::StreamExt;
-		use sc_network::{Event, NetworkEventStream};
+	let authority_discovery_service =
+		// We need the authority discovery if this node is either a validator or running alongside a parachain node.
+		// Parachains node require the authority discovery for finding relay chain validators for sending
+		// their PoVs or recovering PoVs.
+		if role.is_authority() || is_parachain_node.is_running_alongside_parachain_node() {
+			use futures::StreamExt;
+			use sc_network::{Event, NetworkEventStream};
 
-		let authority_discovery_role = if role.is_authority() {
-			sc_authority_discovery::Role::PublishAndDiscover(keystore_container.keystore())
+			let authority_discovery_role = if role.is_authority() {
+				sc_authority_discovery::Role::PublishAndDiscover(keystore_container.keystore())
+			} else {
+				// don't publish our addresses when we're not an authority (collator, cumulus, ..)
+				sc_authority_discovery::Role::Discover
+			};
+			let dht_event_stream =
+				network.event_stream("authority-discovery").filter_map(|e| async move {
+					match e {
+						Event::Dht(e) => Some(e),
+						_ => None,
+					}
+				});
+			let (worker, service) = sc_authority_discovery::new_worker_and_service_with_config(
+				sc_authority_discovery::WorkerConfig {
+					publish_non_global_ips: auth_disc_publish_non_global_ips,
+					// Require that authority discovery records are signed.
+					strict_record_validation: true,
+					..Default::default()
+				},
+				client.clone(),
+				network.clone(),
+				Box::pin(dht_event_stream),
+				authority_discovery_role,
+				prometheus_registry.clone(),
+			);
+
+			task_manager.spawn_handle().spawn(
+				"authority-discovery-worker",
+				Some("authority-discovery"),
+				Box::pin(worker.run()),
+			);
+			Some(service)
 		} else {
-			// don't publish our addresses when we're not an authority (collator, cumulus, ..)
-			sc_authority_discovery::Role::Discover
+			None
 		};
-		let dht_event_stream =
-			network.event_stream("authority-discovery").filter_map(|e| async move {
-				match e {
-					Event::Dht(e) => Some(e),
-					_ => None,
-				}
-			});
-		let (worker, service) = sc_authority_discovery::new_worker_and_service_with_config(
-			sc_authority_discovery::WorkerConfig {
-				publish_non_global_ips: auth_disc_publish_non_global_ips,
-				// Require that authority discovery records are signed.
-				strict_record_validation: true,
-				..Default::default()
-			},
-			client.clone(),
-			network.clone(),
-			Box::pin(dht_event_stream),
-			authority_discovery_role,
-			prometheus_registry.clone(),
-		);
-
-		task_manager.spawn_handle().spawn(
-			"authority-discovery-worker",
-			Some("authority-discovery"),
-			Box::pin(worker.run()),
-		);
-		Some(service)
-	} else {
-		None
-	};
 
 	let overseer_handle = if let Some(authority_discovery_service) = authority_discovery_service {
 		let (overseer, overseer_handle) = overseer_gen
@@ -1039,7 +1056,7 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 					dispute_req_receiver,
 					registry: prometheus_registry.as_ref(),
 					spawner,
-					is_collator,
+					is_parachain_node,
 					approval_voting_config,
 					availability_config: AVAILABILITY_CONFIG,
 					candidate_validation_config,
@@ -1332,11 +1349,6 @@ pub fn new_chain_ops(
 ///
 /// The actual "flavor", aka if it will use `Polkadot`, `Rococo` or `Kusama` is determined based on
 /// [`IdentifyVariant`] using the chain spec.
-///
-/// `overseer_enable_anyways` always enables the overseer, based on the provided
-/// `OverseerGenerator`, regardless of the role the node has. The relay chain selection (longest or
-/// disputes-aware) is still determined based on the role of the node. Likewise for authority
-/// discovery.
 #[cfg(feature = "full-node")]
 pub fn build_full<OverseerGenerator: OverseerGen>(
 	config: Configuration,