Remove ValidateFromChainState (#5707)

# Description This PR removes the `CandidateValidationMessage::ValidateFromChainState`, which was previously used by backing, but is no longer relevant since initial async backing implementation paritytech/polkadot#5557. Fixes #5643 ## Integration This change should not affect downstream projects since `ValidateFromChainState` was already unused. ## Review Notes - Removed all occurrences of `ValidateFromChainState`. - Moved utility functions, previously used in candidate validation tests and malus, exclusively to candidate validation tests as they are no longer used in malus. - Deleted the `polkadot_parachain_candidate_validation_validate_from_chain_state` metric from Prometheus. - Removed `Spawner` from `ReplaceValidationResult` in malus’ interceptors. - `fake_validation_error` was only used for `ValidateFromChainState` handling, while other cases directly used `InvalidCandidate::InvalidOutputs`. It has been replaced with `fake_validation_error`, with a fallback to `InvalidCandidate::InvalidOutputs`. - Updated overseer’s minimal example to replace `ValidateFromChainState` with `ValidateFromExhaustive`.
paritytech · Oct 1, 2024 · 1617852 · 1617852
1 parent 8279d10
commit 1617852
Show file tree

Hide file tree

Showing 12 changed files with 97 additions and 407 deletions.
diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs
@@ -46,8 +46,8 @@ use polkadot_primitives::{
 		DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT,
 	},
 	AuthorityDiscoveryId, CandidateCommitments, CandidateDescriptor, CandidateEvent,
-	CandidateReceipt, ExecutorParams, Hash, OccupiedCoreAssumption, PersistedValidationData,
-	PvfExecKind, PvfPrepKind, SessionIndex, ValidationCode, ValidationCodeHash, ValidatorId,
+	CandidateReceipt, ExecutorParams, Hash, PersistedValidationData, PvfExecKind, PvfPrepKind,
+	SessionIndex, ValidationCode, ValidationCodeHash, ValidatorId,
 };
 use sp_application_crypto::{AppCrypto, ByteArray};
 use sp_keystore::KeystorePtr;
@@ -83,8 +83,7 @@ const PVF_APPROVAL_EXECUTION_RETRY_DELAY: Duration = Duration::from_secs(3);
 const PVF_APPROVAL_EXECUTION_RETRY_DELAY: Duration = Duration::from_millis(200);
 
 // The task queue size is chosen to be somewhat bigger than the PVF host incoming queue size
-// to allow exhaustive validation messages to fall through in case the tasks are clogged with
-// `ValidateFromChainState` messages awaiting data from the runtime
+// to allow exhaustive validation messages to fall through in case the tasks are clogged
 const TASK_LIMIT: usize = 30;
 
 /// Configuration for the candidate validation subsystem
@@ -155,30 +154,6 @@ where
 	S: SubsystemSender<RuntimeApiMessage>,
 {
 	match msg {
-		CandidateValidationMessage::ValidateFromChainState {
-			candidate_receipt,
-			pov,
-			executor_params,
-			exec_kind,
-			response_sender,
-			..
-		} => async move {
-			let _timer = metrics.time_validate_from_chain_state();
-			let res = validate_from_chain_state(
-				&mut sender,
-				validation_host,
-				candidate_receipt,
-				pov,
-				executor_params,
-				exec_kind,
-				&metrics,
-			)
-			.await;
-
-			metrics.on_validation_event(&res);
-			let _ = response_sender.send(res);
-		}
-		.boxed(),
 		CandidateValidationMessage::ValidateFromExhaustive {
 			validation_data,
 			validation_code,
@@ -657,170 +632,6 @@ where
 	}
 }
 
-#[derive(Debug)]
-enum AssumptionCheckOutcome {
-	Matches(PersistedValidationData, ValidationCode),
-	DoesNotMatch,
-	BadRequest,
-}
-
-async fn check_assumption_validation_data<Sender>(
-	sender: &mut Sender,
-	descriptor: &CandidateDescriptor,
-	assumption: OccupiedCoreAssumption,
-) -> AssumptionCheckOutcome
-where
-	Sender: SubsystemSender<RuntimeApiMessage>,
-{
-	let validation_data = {
-		let (tx, rx) = oneshot::channel();
-		let d = runtime_api_request(
-			sender,
-			descriptor.relay_parent,
-			RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx),
-			rx,
-		)
-		.await;
-
-		match d {
-			Ok(None) | Err(RuntimeRequestFailed) => return AssumptionCheckOutcome::BadRequest,
-			Ok(Some(d)) => d,
-		}
-	};
-
-	let persisted_validation_data_hash = validation_data.hash();
-
-	if descriptor.persisted_validation_data_hash == persisted_validation_data_hash {
-		let (code_tx, code_rx) = oneshot::channel();
-		let validation_code = runtime_api_request(
-			sender,
-			descriptor.relay_parent,
-			RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, code_tx),
-			code_rx,
-		)
-		.await;
-
-		match validation_code {
-			Ok(None) | Err(RuntimeRequestFailed) => AssumptionCheckOutcome::BadRequest,
-			Ok(Some(v)) => AssumptionCheckOutcome::Matches(validation_data, v),
-		}
-	} else {
-		AssumptionCheckOutcome::DoesNotMatch
-	}
-}
-
-async fn find_assumed_validation_data<Sender>(
-	sender: &mut Sender,
-	descriptor: &CandidateDescriptor,
-) -> AssumptionCheckOutcome
-where
-	Sender: SubsystemSender<RuntimeApiMessage>,
-{
-	// The candidate descriptor has a `persisted_validation_data_hash` which corresponds to
-	// one of up to two possible values that we can derive from the state of the
-	// relay-parent. We can fetch these values by getting the persisted validation data
-	// based on the different `OccupiedCoreAssumption`s.
-
-	const ASSUMPTIONS: &[OccupiedCoreAssumption] = &[
-		OccupiedCoreAssumption::Included,
-		OccupiedCoreAssumption::TimedOut,
-		// `TimedOut` and `Free` both don't perform any speculation and therefore should be the
-		// same for our purposes here. In other words, if `TimedOut` matched then the `Free` must
-		// be matched as well.
-	];
-
-	// Consider running these checks in parallel to reduce validation latency.
-	for assumption in ASSUMPTIONS {
-		let outcome = check_assumption_validation_data(sender, descriptor, *assumption).await;
-
-		match outcome {
-			AssumptionCheckOutcome::Matches(_, _) => return outcome,
-			AssumptionCheckOutcome::BadRequest => return outcome,
-			AssumptionCheckOutcome::DoesNotMatch => continue,
-		}
-	}
-
-	AssumptionCheckOutcome::DoesNotMatch
-}
-
-/// Returns validation data for a given candidate.
-pub async fn find_validation_data<Sender>(
-	sender: &mut Sender,
-	descriptor: &CandidateDescriptor,
-) -> Result<Option<(PersistedValidationData, ValidationCode)>, ValidationFailed>
-where
-	Sender: SubsystemSender<RuntimeApiMessage>,
-{
-	match find_assumed_validation_data(sender, &descriptor).await {
-		AssumptionCheckOutcome::Matches(validation_data, validation_code) =>
-			Ok(Some((validation_data, validation_code))),
-		AssumptionCheckOutcome::DoesNotMatch => {
-			// If neither the assumption of the occupied core having the para included or the
-			// assumption of the occupied core timing out are valid, then the
-			// persisted_validation_data_hash in the descriptor is not based on the relay parent and
-			// is thus invalid.
-			Ok(None)
-		},
-		AssumptionCheckOutcome::BadRequest =>
-			Err(ValidationFailed("Assumption Check: Bad request".into())),
-	}
-}
-
-async fn validate_from_chain_state<Sender>(
-	sender: &mut Sender,
-	validation_host: ValidationHost,
-	candidate_receipt: CandidateReceipt,
-	pov: Arc<PoV>,
-	executor_params: ExecutorParams,
-	exec_kind: PvfExecKind,
-	metrics: &Metrics,
-) -> Result<ValidationResult, ValidationFailed>
-where
-	Sender: SubsystemSender<RuntimeApiMessage>,
-{
-	let mut new_sender = sender.clone();
-	let (validation_data, validation_code) =
-		match find_validation_data(&mut new_sender, &candidate_receipt.descriptor).await? {
-			Some((validation_data, validation_code)) => (validation_data, validation_code),
-			None => return Ok(ValidationResult::Invalid(InvalidCandidate::BadParent)),
-		};
-
-	let validation_result = validate_candidate_exhaustive(
-		validation_host,
-		validation_data,
-		validation_code,
-		candidate_receipt.clone(),
-		pov,
-		executor_params,
-		exec_kind,
-		metrics,
-	)
-	.await;
-
-	if let Ok(ValidationResult::Valid(ref outputs, _)) = validation_result {
-		let (tx, rx) = oneshot::channel();
-		match runtime_api_request(
-			sender,
-			candidate_receipt.descriptor.relay_parent,
-			RuntimeApiRequest::CheckValidationOutputs(
-				candidate_receipt.descriptor.para_id,
-				outputs.clone(),
-				tx,
-			),
-			rx,
-		)
-		.await
-		{
-			Ok(true) => {},
-			Ok(false) => return Ok(ValidationResult::Invalid(InvalidCandidate::InvalidOutputs)),
-			Err(RuntimeRequestFailed) =>
-				return Err(ValidationFailed("Check Validation Outputs: Bad request".into())),
-		}
-	}
-
-	validation_result
-}
-
 async fn validate_candidate_exhaustive(
 	mut validation_backend: impl ValidationBackend + Send,
 	persisted_validation_data: PersistedValidationData,

diff --git a/polkadot/node/core/candidate-validation/src/metrics.rs b/polkadot/node/core/candidate-validation/src/metrics.rs
@@ -20,7 +20,6 @@ use polkadot_node_metrics::metrics::{self, prometheus};
 #[derive(Clone)]
 pub(crate) struct MetricsInner {
 	pub(crate) validation_requests: prometheus::CounterVec<prometheus::U64>,
-	pub(crate) validate_from_chain_state: prometheus::Histogram,
 	pub(crate) validate_from_exhaustive: prometheus::Histogram,
 	pub(crate) validate_candidate_exhaustive: prometheus::Histogram,
 }
@@ -46,13 +45,6 @@ impl Metrics {
 		}
 	}
 
-	/// Provide a timer for `validate_from_chain_state` which observes on drop.
-	pub fn time_validate_from_chain_state(
-		&self,
-	) -> Option<metrics::prometheus::prometheus::HistogramTimer> {
-		self.0.as_ref().map(|metrics| metrics.validate_from_chain_state.start_timer())
-	}
-
 	/// Provide a timer for `validate_from_exhaustive` which observes on drop.
 	pub fn time_validate_from_exhaustive(
 		&self,
@@ -83,13 +75,6 @@ impl metrics::Metrics for Metrics {
 				)?,
 				registry,
 			)?,
-			validate_from_chain_state: prometheus::register(
-				prometheus::Histogram::with_opts(prometheus::HistogramOpts::new(
-					"polkadot_parachain_candidate_validation_validate_from_chain_state",
-					"Time spent within `candidate_validation::validate_from_chain_state`",
-				))?,
-				registry,
-			)?,
 			validate_from_exhaustive: prometheus::register(
 				prometheus::Histogram::with_opts(prometheus::HistogramOpts::new(
 					"polkadot_parachain_candidate_validation_validate_from_exhaustive",

diff --git a/polkadot/node/core/candidate-validation/src/tests.rs b/polkadot/node/core/candidate-validation/src/tests.rs
@@ -25,7 +25,8 @@ use polkadot_node_subsystem::messages::AllMessages;
 use polkadot_node_subsystem_util::reexports::SubsystemContext;
 use polkadot_overseer::ActivatedLeaf;
 use polkadot_primitives::{
-	CoreIndex, GroupIndex, HeadData, Id as ParaId, SessionInfo, UpwardMessage, ValidatorId,
+	CoreIndex, GroupIndex, HeadData, Id as ParaId, OccupiedCoreAssumption, SessionInfo,
+	UpwardMessage, ValidatorId,
 };
 use polkadot_primitives_test_helpers::{
 	dummy_collator, dummy_collator_signature, dummy_hash, make_valid_candidate_descriptor,
@@ -34,6 +35,58 @@ use sp_core::{sr25519::Public, testing::TaskExecutor};
 use sp_keyring::Sr25519Keyring;
 use sp_keystore::{testing::MemoryKeystore, Keystore};
 
+#[derive(Debug)]
+enum AssumptionCheckOutcome {
+	Matches(PersistedValidationData, ValidationCode),
+	DoesNotMatch,
+	BadRequest,
+}
+
+async fn check_assumption_validation_data<Sender>(
+	sender: &mut Sender,
+	descriptor: &CandidateDescriptor,
+	assumption: OccupiedCoreAssumption,
+) -> AssumptionCheckOutcome
+where
+	Sender: SubsystemSender<RuntimeApiMessage>,
+{
+	let validation_data = {
+		let (tx, rx) = oneshot::channel();
+		let d = runtime_api_request(
+			sender,
+			descriptor.relay_parent,
+			RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx),
+			rx,
+		)
+		.await;
+
+		match d {
+			Ok(None) | Err(RuntimeRequestFailed) => return AssumptionCheckOutcome::BadRequest,
+			Ok(Some(d)) => d,
+		}
+	};
+
+	let persisted_validation_data_hash = validation_data.hash();
+
+	if descriptor.persisted_validation_data_hash == persisted_validation_data_hash {
+		let (code_tx, code_rx) = oneshot::channel();
+		let validation_code = runtime_api_request(
+			sender,
+			descriptor.relay_parent,
+			RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, code_tx),
+			code_rx,
+		)
+		.await;
+
+		match validation_code {
+			Ok(None) | Err(RuntimeRequestFailed) => AssumptionCheckOutcome::BadRequest,
+			Ok(Some(v)) => AssumptionCheckOutcome::Matches(validation_data, v),
+		}
+	} else {
+		AssumptionCheckOutcome::DoesNotMatch
+	}
+}
+
 #[test]
 fn correctly_checks_included_assumption() {
 	let validation_data: PersistedValidationData = Default::default();

diff --git a/polkadot/node/malus/src/variants/back_garbage_candidate.rs b/polkadot/node/malus/src/variants/back_garbage_candidate.rs
@@ -67,12 +67,10 @@ impl OverseerGen for BackGarbageCandidates {
 		RuntimeClient: RuntimeApiSubsystemClient + ChainApiBackend + AuxStore + 'static,
 		Spawner: 'static + SpawnNamed + Clone + Unpin,
 	{
-		let spawner = args.spawner.clone();
 		let validation_filter = ReplaceValidationResult::new(
 			FakeCandidateValidation::BackingAndApprovalValid,
 			FakeCandidateValidationError::InvalidOutputs,
 			f64::from(self.percentage),
-			SpawnGlue(spawner),
 		);
 
 		validator_overseer_builder(