Refactor ValidationError (#2406)

polkadot/node/core/pvf/src/error.rs

@@ -19,31 +19,44 @@ use polkadot_node_core_pvf_common::error::{InternalValidationError, PrepareError
 /// A error raised during validation of the candidate.
 #[derive(Debug, Clone)]
 pub enum ValidationError {
-	/// The error was raised because the candidate is invalid.
+	/// Deterministic preparation issue. In practice, most of the problems should be caught by
+	/// prechecking, so this may be a sign of internal conditions.
 	///
-	/// Whenever we are unsure if the error was due to the candidate or not, we must vote invalid.
-	InvalidCandidate(InvalidCandidate),
-	/// Some internal error occurred.
-	InternalError(InternalValidationError),
+	/// In principle if preparation of the `WASM` fails, the current candidate cannot be the
+	/// reason for that. So we can't say whether it is invalid or not. In addition, with
+	/// pre-checking enabled only valid runtimes should ever get enacted, so we can be
+	/// reasonably sure that this is some local problem on the current node. However, as this
+	/// particular error *seems* to indicate a deterministic error, we raise a warning.
+	Preparation(PrepareError),
+	/// The error was raised because the candidate is invalid. Should vote against.
+	Invalid(InvalidCandidate),
+	/// Possibly transient issue that may resolve after retries. Should vote against when retries
+	/// fail.
+	PossiblyInvalid(PossiblyInvalidError),
+	/// Preparation or execution issue caused by an internal condition. Should not vote against.
+	Internal(InternalValidationError),
 }
 
 /// A description of an error raised during executing a PVF and can be attributed to the combination
 /// of the candidate [`polkadot_parachain_primitives::primitives::ValidationParams`] and the PVF.
 #[derive(Debug, Clone)]
 pub enum InvalidCandidate {
-	/// PVF preparation ended up with a deterministic error.
-	PrepareError(String),
 	/// The candidate is reported to be invalid by the execution worker. The string contains the
 	/// error message.
 	WorkerReportedInvalid(String),
+	/// PVF execution (compilation is not included) took more time than was allotted.
+	HardTimeout,
+}
+
+/// Possibly transient issue that may resolve after retries.
+#[derive(Debug, Clone)]
+pub enum PossiblyInvalidError {
 	/// The worker process (not the job) has died during validation of a candidate.
 	///
 	/// It's unlikely that this is caused by malicious code since workers spawn separate job
 	/// processes, and those job processes are sandboxed. But, it is possible. We retry in this
 	/// case, and if the error persists, we assume it's caused by the candidate and vote against.
 	AmbiguousWorkerDeath,
-	/// PVF execution (compilation is not included) took more time than was allotted.
-	HardTimeout,
 	/// The job process (not the worker) has died for one of the following reasons:
 	///
 	/// (a) A seccomp violation occurred, most likely due to an attempt by malicious code to
@@ -68,7 +81,7 @@ pub enum InvalidCandidate {
 
 impl From<InternalValidationError> for ValidationError {
 	fn from(error: InternalValidationError) -> Self {
-		Self::InternalError(error)
+		Self::Internal(error)
 	}
 }
 
@@ -77,9 +90,9 @@ impl From<PrepareError> for ValidationError {
 		// Here we need to classify the errors into two errors: deterministic and non-deterministic.
 		// See [`PrepareError::is_deterministic`].
 		if error.is_deterministic() {
-			Self::InvalidCandidate(InvalidCandidate::PrepareError(error.to_string()))
+			Self::Preparation(error)
 		} else {
-			Self::InternalError(InternalValidationError::NonDeterministicPrepareError(error))
+			Self::Internal(InternalValidationError::NonDeterministicPrepareError(error))
 		}
 	}
 }

polkadot/node/core/pvf/src/execute/queue.rs

@@ -22,7 +22,7 @@ use crate::{
 	host::ResultSender,
 	metrics::Metrics,
 	worker_intf::{IdleWorker, WorkerHandle},
-	InvalidCandidate, ValidationError, LOG_TARGET,
+	InvalidCandidate, PossiblyInvalidError, ValidationError, LOG_TARGET,
 };
 use futures::{
 	channel::mpsc,
@@ -342,27 +342,27 @@ fn handle_job_finish(
 		},
 		Outcome::InvalidCandidate { err, idle_worker } => (
 			Some(idle_worker),
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::WorkerReportedInvalid(err))),
+			Err(ValidationError::Invalid(InvalidCandidate::WorkerReportedInvalid(err))),
 			None,
 		),
-		Outcome::InternalError { err } => (None, Err(ValidationError::InternalError(err)), None),
+		Outcome::InternalError { err } => (None, Err(ValidationError::Internal(err)), None),
 		// Either the worker or the job timed out. Kill the worker in either case. Treated as
 		// definitely-invalid, because if we timed out, there's no time left for a retry.
 		Outcome::HardTimeout =>
-			(None, Err(ValidationError::InvalidCandidate(InvalidCandidate::HardTimeout)), None),
+			(None, Err(ValidationError::Invalid(InvalidCandidate::HardTimeout)), None),
 		// "Maybe invalid" errors (will retry).
 		Outcome::WorkerIntfErr => (
 			None,
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath)),
+			Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)),
 			None,
 		),
 		Outcome::JobDied { err } => (
 			None,
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousJobDeath(err))),
+			Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousJobDeath(err))),
 			None,
 		),
 		Outcome::JobError { err } =>
-			(None, Err(ValidationError::InvalidCandidate(InvalidCandidate::JobError(err))), None),
+			(None, Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::JobError(err))), None),
 	};
 
 	queue.metrics.execute_finished();

polkadot/node/core/pvf/src/host.rs

@@ -873,7 +873,7 @@ fn pulse_every(interval: std::time::Duration) -> impl futures::Stream<Item = ()>
 #[cfg(test)]
 pub(crate) mod tests {
 	use super::*;
-	use crate::InvalidCandidate;
+	use crate::PossiblyInvalidError;
 	use assert_matches::assert_matches;
 	use futures::future::BoxFuture;
 	use polkadot_node_core_pvf_common::{
@@ -1211,27 +1211,27 @@ pub(crate) mod tests {
 		);
 
 		result_tx_pvf_1_1
-			.send(Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath)))
+			.send(Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)))
 			.unwrap();
 		assert_matches!(
 			result_rx_pvf_1_1.now_or_never().unwrap().unwrap(),
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath))
+			Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath))
 		);
 
 		result_tx_pvf_1_2
-			.send(Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath)))
+			.send(Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)))
 			.unwrap();
 		assert_matches!(
 			result_rx_pvf_1_2.now_or_never().unwrap().unwrap(),
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath))
+			Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath))
 		);
 
 		result_tx_pvf_2
-			.send(Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath)))
+			.send(Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)))
 			.unwrap();
 		assert_matches!(
 			result_rx_pvf_2.now_or_never().unwrap().unwrap(),
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath))
+			Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath))
 		);
 	}
 
@@ -1337,7 +1337,7 @@ pub(crate) mod tests {
 		assert_matches!(result_rx.now_or_never().unwrap().unwrap(), Err(PrepareError::TimedOut));
 		assert_matches!(
 			result_rx_execute.now_or_never().unwrap().unwrap(),
-			Err(ValidationError::InternalError(_))
+			Err(ValidationError::Internal(_))
 		);
 
 		// Reversed case: first send multiple precheck requests, then ask for an execution.
@@ -1479,7 +1479,7 @@ pub(crate) mod tests {
 
 		// The result should contain the error.
 		let result = test.poll_and_recv_result(result_rx).await;
-		assert_matches!(result, Err(ValidationError::InternalError(_)));
+		assert_matches!(result, Err(ValidationError::Internal(_)));
 
 		// Submit another execute request. We shouldn't try to prepare again, yet.
 		let (result_tx_2, result_rx_2) = oneshot::channel();
@@ -1498,7 +1498,7 @@ pub(crate) mod tests {
 
 		// The result should contain the original error.
 		let result = test.poll_and_recv_result(result_rx_2).await;
-		assert_matches!(result, Err(ValidationError::InternalError(_)));
+		assert_matches!(result, Err(ValidationError::Internal(_)));
 
 		// Pause for enough time to reset the cooldown for this failed prepare request.
 		futures_timer::Delay::new(PREPARE_FAILURE_COOLDOWN).await;
@@ -1538,11 +1538,11 @@ pub(crate) mod tests {
 		// Send an error for the execution here, just so we can check the result receiver is still
 		// alive.
 		result_tx_3
-			.send(Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath)))
+			.send(Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath)))
 			.unwrap();
 		assert_matches!(
 			result_rx_3.now_or_never().unwrap().unwrap(),
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::AmbiguousWorkerDeath))
+			Err(ValidationError::PossiblyInvalid(PossiblyInvalidError::AmbiguousWorkerDeath))
 		);
 	}
 
@@ -1581,10 +1581,7 @@ pub(crate) mod tests {
 
 		// The result should contain the error.
 		let result = test.poll_and_recv_result(result_rx).await;
-		assert_matches!(
-			result,
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::PrepareError(_)))
-		);
+		assert_matches!(result, Err(ValidationError::Preparation(_)));
 
 		// Submit another execute request.
 		let (result_tx_2, result_rx_2) = oneshot::channel();
@@ -1603,10 +1600,7 @@ pub(crate) mod tests {
 
 		// The result should contain the original error.
 		let result = test.poll_and_recv_result(result_rx_2).await;
-		assert_matches!(
-			result,
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::PrepareError(_)))
-		);
+		assert_matches!(result, Err(ValidationError::Preparation(_)));
 
 		// Pause for enough time to reset the cooldown for this failed prepare request.
 		futures_timer::Delay::new(PREPARE_FAILURE_COOLDOWN).await;
@@ -1628,10 +1622,7 @@ pub(crate) mod tests {
 
 		// The result should still contain the original error.
 		let result = test.poll_and_recv_result(result_rx_3).await;
-		assert_matches!(
-			result,
-			Err(ValidationError::InvalidCandidate(InvalidCandidate::PrepareError(_)))
-		);
+		assert_matches!(result, Err(ValidationError::Preparation(_)));
 	}
 
 	// Test that multiple heads-up requests trigger preparation retries if the first one failed.

polkadot/node/core/pvf/src/lib.rs

@@ -103,7 +103,7 @@ mod worker_intf;
 #[cfg(feature = "test-utils")]
 pub mod testing;
 
-pub use error::{InvalidCandidate, ValidationError};
+pub use error::{InvalidCandidate, PossiblyInvalidError, ValidationError};
 pub use host::{start, Config, ValidationHost, EXECUTE_BINARY_NAME, PREPARE_BINARY_NAME};
 pub use metrics::Metrics;
 pub use priority::Priority;