Remove superflous parameter overseer_enable_anyways and make parachain node type more explicit

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,