pvf-precheck: Candidate Validation Changes (#4409)

* pvf-precheck: Candidate Validation Changes

Co-authored-by: Chris Sosnin <chris125_@live.com>

* Rename `MockValidationBackend` and specialize it

* Add pre-check tests

Co-authored-by: Chris Sosnin <chris125_@live.com>

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

@@ -24,7 +24,7 @@
 #![warn(missing_docs)]
 
 use polkadot_node_core_pvf::{
-	InvalidCandidate as WasmInvalidCandidate, Pvf, ValidationError, ValidationHost,
+	InvalidCandidate as WasmInvalidCandidate, PrepareError, Pvf, ValidationError, ValidationHost,
 };
 use polkadot_node_primitives::{
 	BlockData, InvalidCandidate, PoV, ValidationResult, POV_BOMB_LIMIT, VALIDATION_CODE_BOMB_LIMIT,
@@ -32,7 +32,8 @@ use polkadot_node_primitives::{
 use polkadot_node_subsystem::{
 	errors::RuntimeApiError,
 	messages::{
-		CandidateValidationMessage, RuntimeApiMessage, RuntimeApiRequest, ValidationFailed,
+		CandidateValidationMessage, PreCheckOutcome, RuntimeApiMessage, RuntimeApiRequest,
+		ValidationFailed,
 	},
 	overseer, FromOverseer, OverseerSignal, SpawnedSubsystem, SubsystemContext, SubsystemError,
 	SubsystemResult, SubsystemSender,
@@ -194,6 +195,30 @@ where
 
 					ctx.spawn("validate-from-exhaustive", bg.boxed())?;
 				},
+				CandidateValidationMessage::PreCheck(
+					relay_parent,
+					validation_code_hash,
+					response_sender,
+				) => {
+					let bg = {
+						let mut sender = ctx.sender().clone();
+						let validation_host = validation_host.clone();
+
+						async move {
+							let precheck_result = precheck_pvf(
+								&mut sender,
+								validation_host,
+								relay_parent,
+								validation_code_hash,
+							)
+							.await;
+
+							let _ = response_sender.send(precheck_result);
+						}
+					};
+
+					ctx.spawn("candidate-validation-pre-check", bg.boxed())?;
+				},
 			},
 		}
 	}
@@ -235,6 +260,73 @@ where
 		})
 }
 
+async fn request_validation_code_by_hash<Sender>(
+	sender: &mut Sender,
+	relay_parent: Hash,
+	validation_code_hash: ValidationCodeHash,
+) -> Result<Option<ValidationCode>, RuntimeRequestFailed>
+where
+	Sender: SubsystemSender,
+{
+	let (tx, rx) = oneshot::channel();
+	runtime_api_request(
+		sender,
+		relay_parent,
+		RuntimeApiRequest::ValidationCodeByHash(validation_code_hash, tx),
+		rx,
+	)
+	.await
+}
+
+async fn precheck_pvf<Sender>(
+	sender: &mut Sender,
+	mut validation_backend: impl ValidationBackend,
+	relay_parent: Hash,
+	validation_code_hash: ValidationCodeHash,
+) -> PreCheckOutcome
+where
+	Sender: SubsystemSender,
+{
+	let validation_code =
+		match request_validation_code_by_hash(sender, relay_parent, validation_code_hash).await {
+			Ok(Some(code)) => code,
+			_ => {
+				// The reasoning why this is "failed" and not invalid is because we assume that
+				// during pre-checking voting the relay-chain will pin the code. In case the code
+				// actually is not there, we issue failed since this looks more like a bug. This
+				// leads to us abstaining.
+				tracing::warn!(
+					target: LOG_TARGET,
+					?relay_parent,
+					?validation_code_hash,
+					"precheck: requested validation code is not found on-chain!",
+				);
+				return PreCheckOutcome::Failed
+			},
+		};
+
+	let validation_code = match sp_maybe_compressed_blob::decompress(
+		&validation_code.0,
+		VALIDATION_CODE_BOMB_LIMIT,
+	) {
+		Ok(code) => Pvf::from_code(code.into_owned()),
+		Err(e) => {
+			tracing::debug!(target: LOG_TARGET, err=?e, "precheck: cannot decompress validation code");
+			return PreCheckOutcome::Invalid
+		},
+	};
+
+	match validation_backend.precheck_pvf(validation_code).await {
+		Ok(_) => PreCheckOutcome::Valid,
+		Err(prepare_err) => match prepare_err {
+			PrepareError::Prevalidation(_) |
+			PrepareError::Preparation(_) |
+			PrepareError::Panic(_) => PreCheckOutcome::Invalid,
+			PrepareError::TimedOut | PrepareError::DidNotMakeIt => PreCheckOutcome::Failed,
+		},
+	}
+}
+
 #[derive(Debug)]
 enum AssumptionCheckOutcome {
 	Matches(PersistedValidationData, ValidationCode),
@@ -487,6 +579,8 @@ trait ValidationBackend {
 		timeout: Duration,
 		params: ValidationParams,
 	) -> Result<WasmValidationResult, ValidationError>;
+
+	async fn precheck_pvf(&mut self, pvf: Pvf) -> Result<(), PrepareError>;
 }
 
 #[async_trait]
@@ -520,6 +614,17 @@ impl ValidationBackend for ValidationHost {
 
 		validation_result
 	}
+
+	async fn precheck_pvf(&mut self, pvf: Pvf) -> Result<(), PrepareError> {
+		let (tx, rx) = oneshot::channel();
+		if let Err(_) = self.precheck_pvf(pvf, tx).await {
+			return Err(PrepareError::DidNotMakeIt)
+		}
+
+		let precheck_result = rx.await.or(Err(PrepareError::DidNotMakeIt))?;
+
+		precheck_result
+	}
 }
 
 /// Does basic checks of a candidate. Provide the encoded PoV-block. Returns `Ok` if basic checks

polkadot/node/core/candidate-validation/src/tests.rs

@@ -17,6 +17,7 @@
 use super::*;
 use assert_matches::assert_matches;
 use futures::executor;
+use polkadot_node_core_pvf::PrepareError;
 use polkadot_node_subsystem::messages::AllMessages;
 use polkadot_node_subsystem_test_helpers as test_helpers;
 use polkadot_node_subsystem_util::reexports::SubsystemContext;
@@ -326,18 +327,18 @@ fn check_does_not_match() {
 	executor::block_on(test_fut);
 }
 
-struct MockValidatorBackend {
+struct MockValidateCandidateBackend {
 	result: Result<WasmValidationResult, ValidationError>,
 }
 
-impl MockValidatorBackend {
+impl MockValidateCandidateBackend {
 	fn with_hardcoded_result(result: Result<WasmValidationResult, ValidationError>) -> Self {
 		Self { result }
 	}
 }
 
 #[async_trait]
-impl ValidationBackend for MockValidatorBackend {
+impl ValidationBackend for MockValidateCandidateBackend {
 	async fn validate_candidate(
 		&mut self,
 		_raw_validation_code: Vec<u8>,
@@ -346,6 +347,10 @@ impl ValidationBackend for MockValidatorBackend {
 	) -> Result<WasmValidationResult, ValidationError> {
 		self.result.clone()
 	}
+
+	async fn precheck_pvf(&mut self, _pvf: Pvf) -> Result<(), PrepareError> {
+		unreachable!()
+	}
 }
 
 #[test]
@@ -380,7 +385,7 @@ fn candidate_validation_ok_is_ok() {
 	};
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Ok(validation_result)),
+		MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)),
 		validation_data.clone(),
 		validation_code,
 		descriptor,
@@ -421,9 +426,9 @@ fn candidate_validation_bad_return_is_invalid() {
 	assert!(check.is_ok());
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Err(ValidationError::InvalidCandidate(
+		MockValidateCandidateBackend::with_hardcoded_result(Err(
-			WasmInvalidCandidate::AmbiguousWorkerDeath,
+			ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbiguousWorkerDeath),
-		))),
+		)),
 		validation_data,
 		validation_code,
 		descriptor,
@@ -457,9 +462,9 @@ fn candidate_validation_timeout_is_internal_error() {
 	assert!(check.is_ok());
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Err(ValidationError::InvalidCandidate(
+		MockValidateCandidateBackend::with_hardcoded_result(Err(
-			WasmInvalidCandidate::HardTimeout,
+			ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout),
-		))),
+		)),
 		validation_data,
 		validation_code,
 		descriptor,
@@ -492,9 +497,9 @@ fn candidate_validation_code_mismatch_is_invalid() {
 	assert_matches!(check, Err(InvalidCandidate::CodeHashMismatch));
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Err(ValidationError::InvalidCandidate(
+		MockValidateCandidateBackend::with_hardcoded_result(Err(
-			WasmInvalidCandidate::HardTimeout,
+			ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout),
-		))),
+		)),
 		validation_data,
 		validation_code,
 		descriptor,
@@ -534,7 +539,7 @@ fn compressed_code_works() {
 	};
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Ok(validation_result)),
+		MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)),
 		validation_data,
 		validation_code,
 		descriptor,
@@ -574,7 +579,7 @@ fn code_decompression_failure_is_invalid() {
 	};
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Ok(validation_result)),
+		MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)),
 		validation_data,
 		validation_code,
 		descriptor,
@@ -615,7 +620,7 @@ fn pov_decompression_failure_is_invalid() {
 	};
 
 	let v = executor::block_on(validate_candidate_exhaustive(
-		MockValidatorBackend::with_hardcoded_result(Ok(validation_result)),
+		MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)),
 		validation_data,
 		validation_code,
 		descriptor,
@@ -626,3 +631,166 @@ fn pov_decompression_failure_is_invalid() {
 
 	assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure)));
 }
+
+struct MockPreCheckBackend {
+	result: Result<(), PrepareError>,
+}
+
+impl MockPreCheckBackend {
+	fn with_hardcoded_result(result: Result<(), PrepareError>) -> Self {
+		Self { result }
+	}
+}
+
+#[async_trait]
+impl ValidationBackend for MockPreCheckBackend {
+	async fn validate_candidate(
+		&mut self,
+		_raw_validation_code: Vec<u8>,
+		_timeout: Duration,
+		_params: ValidationParams,
+	) -> Result<WasmValidationResult, ValidationError> {
+		unreachable!()
+	}
+
+	async fn precheck_pvf(&mut self, _pvf: Pvf) -> Result<(), PrepareError> {
+		self.result.clone()
+	}
+}
+
+#[test]
+fn precheck_works() {
+	let relay_parent = [3; 32].into();
+	let validation_code = ValidationCode(vec![3; 16]);
+	let validation_code_hash = validation_code.hash();
+
+	let pool = TaskExecutor::new();
+	let (mut ctx, mut ctx_handle) =
+		test_helpers::make_subsystem_context::<AllMessages, _>(pool.clone());
+
+	let (check_fut, check_result) = precheck_pvf(
+		ctx.sender(),
+		MockPreCheckBackend::with_hardcoded_result(Ok(())),
+		relay_parent,
+		validation_code_hash,
+	)
+	.remote_handle();
+
+	let test_fut = async move {
+		assert_matches!(
+			ctx_handle.recv().await,
+			AllMessages::RuntimeApi(RuntimeApiMessage::Request(
+				rp,
+				RuntimeApiRequest::ValidationCodeByHash(
+					vch,
+					tx
+				),
+			)) => {
+				assert_eq!(vch, validation_code_hash);
+				assert_eq!(rp, relay_parent);
+
+				let _ = tx.send(Ok(Some(validation_code.clone())));
+			}
+		);
+		assert_matches!(check_result.await, PreCheckOutcome::Valid);
+	};
+
+	let test_fut = future::join(test_fut, check_fut);
+	executor::block_on(test_fut);
+}
+
+#[test]
+fn precheck_invalid_pvf_blob_compression() {
+	let relay_parent = [3; 32].into();
+
+	let raw_code = vec![2u8; VALIDATION_CODE_BOMB_LIMIT + 1];
+	let validation_code =
+		sp_maybe_compressed_blob::compress(&raw_code, VALIDATION_CODE_BOMB_LIMIT + 1)
+			.map(ValidationCode)
+			.unwrap();
+	let validation_code_hash = validation_code.hash();
+
+	let pool = TaskExecutor::new();
+	let (mut ctx, mut ctx_handle) =
+		test_helpers::make_subsystem_context::<AllMessages, _>(pool.clone());
+
+	let (check_fut, check_result) = precheck_pvf(
+		ctx.sender(),
+		MockPreCheckBackend::with_hardcoded_result(Ok(())),
+		relay_parent,
+		validation_code_hash,
+	)
+	.remote_handle();
+
+	let test_fut = async move {
+		assert_matches!(
+			ctx_handle.recv().await,
+			AllMessages::RuntimeApi(RuntimeApiMessage::Request(
+				rp,
+				RuntimeApiRequest::ValidationCodeByHash(
+					vch,
+					tx
+				),
+			)) => {
+				assert_eq!(vch, validation_code_hash);
+				assert_eq!(rp, relay_parent);
+
+				let _ = tx.send(Ok(Some(validation_code.clone())));
+			}
+		);
+		assert_matches!(check_result.await, PreCheckOutcome::Invalid);
+	};
+
+	let test_fut = future::join(test_fut, check_fut);
+	executor::block_on(test_fut);
+}
+
+#[test]
+fn precheck_properly_classifies_outcomes() {
+	let inner = |prepare_result, precheck_outcome| {
+		let relay_parent = [3; 32].into();
+		let validation_code = ValidationCode(vec![3; 16]);
+		let validation_code_hash = validation_code.hash();
+
+		let pool = TaskExecutor::new();
+		let (mut ctx, mut ctx_handle) =
+			test_helpers::make_subsystem_context::<AllMessages, _>(pool.clone());
+
+		let (check_fut, check_result) = precheck_pvf(
+			ctx.sender(),
+			MockPreCheckBackend::with_hardcoded_result(prepare_result),
+			relay_parent,
+			validation_code_hash,
+		)
+		.remote_handle();
+
+		let test_fut = async move {
+			assert_matches!(
+				ctx_handle.recv().await,
+				AllMessages::RuntimeApi(RuntimeApiMessage::Request(
+					rp,
+					RuntimeApiRequest::ValidationCodeByHash(
+						vch,
+						tx
+					),
+				)) => {
+					assert_eq!(vch, validation_code_hash);
+					assert_eq!(rp, relay_parent);
+
+					let _ = tx.send(Ok(Some(validation_code.clone())));
+				}
+			);
+			assert_eq!(check_result.await, precheck_outcome);
+		};
+
+		let test_fut = future::join(test_fut, check_fut);
+		executor::block_on(test_fut);
+	};
+
+	inner(Err(PrepareError::Prevalidation("foo".to_owned())), PreCheckOutcome::Invalid);
+	inner(Err(PrepareError::Preparation("bar".to_owned())), PreCheckOutcome::Invalid);
+	inner(Err(PrepareError::Panic("baz".to_owned())), PreCheckOutcome::Invalid);
+
+	inner(Err(PrepareError::TimedOut), PreCheckOutcome::Failed);
+	inner(Err(PrepareError::DidNotMakeIt), PreCheckOutcome::Failed);
+}

polkadot/node/subsystem-types/src/messages.rs

@@ -93,6 +93,26 @@ impl BoundToRelayParent for CandidateBackingMessage {
 #[error("Validation failed with {0:?}")]
 pub struct ValidationFailed(pub String);
 
+/// The outcome of the candidate-validation's PVF pre-check request.
+#[derive(Debug, PartialEq)]
+pub enum PreCheckOutcome {
+	/// The PVF has been compiled successfully within the given constraints.
+	Valid,
+	/// The PVF could not be compiled. This variant is used when the candidate-validation subsystem
+	/// can be sure that the PVF is invalid. To give a couple of examples: a PVF that cannot be
+	/// decompressed or that does not represent a structurally valid WebAssembly file.
+	Invalid,
+	/// This variant is used when the PVF cannot be compiled but for other reasons that are not
+	/// included into [`PreCheckOutcome::Invalid`]. This variant can indicate that the PVF in
+	/// question is invalid, however it is not necessary that PVF that received this judgement
+	/// is invalid.
+	///
+	/// For example, if during compilation the preparation worker was killed we cannot be sure why
+	/// it happened: because the PVF was malicious made the worker to use too much memory or its
+	/// because the host machine is under severe memory pressure and it decided to kill the worker.
+	Failed,
+}
+
 /// Messages received by the Validation subsystem.
 ///
 /// ## Validation Requests
@@ -137,6 +157,17 @@ pub enum CandidateValidationMessage {
 		Duration,
 		oneshot::Sender<Result<ValidationResult, ValidationFailed>>,
 	),
+	/// Try to compile the given validation code and send back
+	/// the outcome.
+	///
+	/// The validation code is specified by the hash and will be queried from the runtime API at the
+	/// given relay-parent.
+	PreCheck(
+		// Relay-parent
+		Hash,
+		ValidationCodeHash,
+		oneshot::Sender<PreCheckOutcome>,
+	),
 }
 
 impl CandidateValidationMessage {
@@ -145,6 +176,7 @@ impl CandidateValidationMessage {
 		match self {
 			Self::ValidateFromChainState(_, _, _, _) => None,
 			Self::ValidateFromExhaustive(_, _, _, _, _, _) => None,
+			Self::PreCheck(relay_parent, _, _) => Some(*relay_parent),
 		}
 	}
 }

polkadot/scripts/gitlab/lingua.dic

@@ -124,6 +124,7 @@ isolate/BG
 iterable
 jaeger/MS
 js
+judgement
 keccak256/M
 keypair/MS
 keystore/MS