Do not validate a candidate in candidate selection (#1912)

* Do not validate a candidate in candidate selection

The candidate selection subsystem should not validate a candidate, as
this is done by the backing subsystem on a `Second` request. Otherwise
we validate one candidate twice.

* Update candidate-selection.md

polkadot/Cargo.lock

@@ -5017,7 +5017,6 @@ version = "0.1.0"
 dependencies = [
  "futures 0.3.5",
  "log 0.4.11",
- "polkadot-node-primitives",
  "polkadot-node-subsystem",
  "polkadot-node-subsystem-util",
  "polkadot-primitives",

polkadot/node/core/candidate-selection/Cargo.toml

@@ -9,7 +9,6 @@ futures = "0.3.5"
 log = "0.4.11"
 thiserror = "1.0.21"
 polkadot-primitives = { path = "../../../primitives" }
-polkadot-node-primitives = { path = "../../primitives" }
 polkadot-node-subsystem = { path = "../../subsystem" }
 polkadot-node-subsystem-util = { path = "../../subsystem-util" }
 

polkadot/node/core/candidate-selection/src/lib.rs

@@ -23,22 +23,16 @@ use futures::{
 	channel::{mpsc, oneshot},
 	prelude::*,
 };
-use polkadot_node_primitives::ValidationResult;
 use polkadot_node_subsystem::{
 	errors::{ChainApiError, RuntimeApiError},
-	messages::{
+	messages::{AllMessages, CandidateBackingMessage, CandidateSelectionMessage, CollatorProtocolMessage},
-		AllMessages, CandidateBackingMessage, CandidateSelectionMessage,
-		CandidateValidationMessage, CollatorProtocolMessage,
-	},
 };
 use polkadot_node_subsystem_util::{
 	self as util, delegated_subsystem, JobTrait, ToJobTrait,
 	metrics::{self, prometheus},
 };
-use polkadot_primitives::v1::{
+use polkadot_primitives::v1::{CandidateReceipt, CollatorId, Hash, Id as ParaId, PoV};
-	CandidateDescriptor, CandidateReceipt, CollatorId, Hash, Id as ParaId, PoV,
+use std::{convert::TryFrom, pin::Pin};
-};
-use std::{convert::TryFrom, pin::Pin, sync::Arc};
 use thiserror::Error;
 
 const TARGET: &'static str = "candidate_selection";
@@ -89,7 +83,6 @@ impl From<CandidateSelectionMessage> for ToJob {
 
 #[derive(Debug)]
 enum FromJob {
-	Validation(CandidateValidationMessage),
 	Backing(CandidateBackingMessage),
 	Collator(CollatorProtocolMessage),
 }
@@ -97,7 +90,6 @@ enum FromJob {
 impl From<FromJob> for AllMessages {
 	fn from(from_job: FromJob) -> AllMessages {
 		match from_job {
-			FromJob::Validation(msg) => AllMessages::CandidateValidation(msg),
 			FromJob::Backing(msg) => AllMessages::CandidateBacking(msg),
 			FromJob::Collator(msg) => AllMessages::CollatorProtocol(msg),
 		}
@@ -109,7 +101,6 @@ impl TryFrom<AllMessages> for FromJob {
 
 	fn try_from(msg: AllMessages) -> Result<Self, Self::Error> {
 		match msg {
-			AllMessages::CandidateValidation(msg) => Ok(FromJob::Validation(msg)),
 			AllMessages::CandidateBacking(msg) => Ok(FromJob::Backing(msg)),
 			AllMessages::CollatorProtocol(msg) => Ok(FromJob::Collator(msg)),
 			_ => Err(()),
@@ -230,25 +221,6 @@ impl CandidateSelectionJob {
 					}
 				};
 
-			let pov = Arc::new(pov);
-
-			if !candidate_is_valid(
-				candidate_receipt.descriptor.clone(),
-				pov.clone(),
-				self.sender.clone(),
-			)
-			.await
-			{
-				return;
-			}
-
-			let pov = if let Ok(pov) = Arc::try_unwrap(pov) {
-				pov
-			} else {
-				log::warn!(target: TARGET, "Arc unwrapping is expected to succeed, the other fns should have already run to completion by now.");
-				return;
-			};
-
 			match second_candidate(
 				relay_parent,
 				candidate_receipt,
@@ -317,34 +289,6 @@ async fn get_collation(
 	rx.await.map_err(Into::into)
 }
 
-// find out whether a candidate is valid or not
-async fn candidate_is_valid(
-	candidate_descriptor: CandidateDescriptor,
-	pov: Arc<PoV>,
-	sender: mpsc::Sender<FromJob>,
-) -> bool {
-	candidate_is_valid_inner(candidate_descriptor, pov, sender).await.unwrap_or(false)
-}
-
-// find out whether a candidate is valid or not, with a worse interface
-// the external interface is worse, but the internal implementation is easier
-async fn candidate_is_valid_inner(
-	candidate_descriptor: CandidateDescriptor,
-	pov: Arc<PoV>,
-	mut sender: mpsc::Sender<FromJob>,
-) -> Result<bool, Error> {
-	let (tx, rx) = oneshot::channel();
-	sender
-		.send(FromJob::Validation(
-			CandidateValidationMessage::ValidateFromChainState(candidate_descriptor, pov, tx),
-		))
-		.await?;
-	Ok(matches!(
-		rx.await,
-		Ok(Ok(ValidationResult::Valid(_, _)))
-	))
-}
-
 async fn second_candidate(
 	relay_parent: Hash,
 	candidate_receipt: CandidateReceipt,
@@ -444,8 +388,9 @@ delegated_subsystem!(CandidateSelectionJob((), Metrics) <- ToJob as CandidateSel
 mod tests {
 	use super::*;
 	use futures::lock::Mutex;
-	use polkadot_primitives::v1::{BlockData, HeadData, PersistedValidationData, ValidationOutputs};
+	use polkadot_primitives::v1::BlockData;
 	use sp_core::crypto::Public;
+	use std::sync::Arc;
 
 	fn test_harness<Preconditions, TestBuilder, Test, Postconditions>(
 		preconditions: Preconditions,
@@ -476,30 +421,6 @@ mod tests {
 		postconditions(job, job_result);
 	}
 
-	fn default_validation_outputs_and_data() -> (ValidationOutputs, polkadot_primitives::v1::PersistedValidationData) {
-		let head_data: Vec<u8> = (0..32).rev().cycle().take(256).collect();
-		let parent_head_data = head_data
-			.iter()
-			.copied()
-			.map(|x| x.saturating_sub(1))
-			.collect();
-
-		(
-			ValidationOutputs {
-				head_data: HeadData(head_data),
-				upward_messages: Vec::new(),
-				new_validation_code: None,
-				processed_downward_messages: 0,
-			},
-			PersistedValidationData {
-				parent_head: HeadData(parent_head_data),
-				block_number: 123,
-				hrmp_mqc_heads: Vec::new(),
-				dmq_mqc_head: Default::default(),
-			},
-		)
-	}
-
 	/// when nothing is seconded so far, the collation is fetched and seconded
 	#[test]
 	fn fetches_and_seconds_a_collation() {
@@ -547,21 +468,6 @@ mod tests {
 								.send((candidate_receipt.clone(), pov.clone()))
 								.unwrap();
 						}
-						FromJob::Validation(
-							CandidateValidationMessage::ValidateFromChainState(
-								got_candidate_descriptor,
-								got_pov,
-								return_sender,
-							),
-						) => {
-							assert_eq!(got_candidate_descriptor, candidate_receipt.descriptor);
-							assert_eq!(got_pov.as_ref(), &pov);
-
-							let (outputs, data) = default_validation_outputs_and_data();
-							return_sender
-								.send(Ok(ValidationResult::Valid(outputs, data)))
-								.unwrap();
-						}
 						FromJob::Backing(CandidateBackingMessage::Second(
 							got_relay_parent,
 							got_candidate_receipt,

polkadot/roadmap/implementers-guide/src/node/backing/candidate-selection.md

@@ -16,7 +16,6 @@ Input: [`CandidateSelectionMessage`](../../types/overseer-protocol.md#candidate-
 
 Output:
 
-- Validation requests to Validation subsystem
 - [`CandidateBackingMessage`](../../types/overseer-protocol.md#candidate-backing-message)`::Second`
 - Peer set manager: report peers (collators who have misbehaved)