Fix some unjustified disputes (#6103)

* Fix indentation + add warning on participation errors. * Don't vote invalid on internal errors. * Don't dispute on code compression error. * Remove CodeDecompressionError * Candidate not invalid if PVF preparation fails. Instead: Report error. * Fix malus * Add clarifying comment. * cargo fmt * Fix indentation.
paritytech · Jan 22, 2023 · 11e1453 · 11e1453
1 parent 6d26b1c
commit 11e1453
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 63 deletions.
diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs
@@ -527,8 +527,9 @@ async fn validate_candidate_exhaustive(
 		Err(e) => {
 			gum::info!(target: LOG_TARGET, ?para_id, err=?e, "Invalid candidate (validation code)");
 
-			// If the validation code is invalid, the candidate certainly is.
-			return Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))
+			// Code already passed pre-checking, if decompression fails now this most likley means
+			// some local corruption happened.
+			return Err(ValidationFailed("Code decompression failed".to_string()))
 		},
 	};
 
@@ -560,7 +561,6 @@ async fn validate_candidate_exhaustive(
 
 	match result {
 		Err(ValidationError::InternalError(e)) => Err(ValidationFailed(e)),
-
 		Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout)) =>
 			Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)),
 		Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WorkerReportedError(e))) =>
@@ -569,9 +569,13 @@ async fn validate_candidate_exhaustive(
 			Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(
 				"ambiguous worker death".to_string(),
 			))),
-		Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) =>
-			Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e))),
-
+		Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => {
+			// In principle if preparation of the `WASM` fails, the current candidate can not 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.
+			Err(ValidationFailed(e))
+		},
 		Ok(res) =>
 			if res.head_data.hash() != candidate_receipt.descriptor.para_head {
 				gum::info!(target: LOG_TARGET, ?para_id, "Invalid candidate (para_head)");

diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs
@@ -799,7 +799,7 @@ fn compressed_code_works() {
 }
 
 #[test]
-fn code_decompression_failure_is_invalid() {
+fn code_decompression_failure_is_error() {
 	let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() };
 	let pov = PoV { block_data: BlockData(vec![1; 32]) };
 	let head_data = HeadData(vec![1, 1, 1]);
@@ -842,7 +842,7 @@ fn code_decompression_failure_is_invalid() {
 		&Default::default(),
 	));
 
-	assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure)));
+	assert_matches!(v, Err(_));
 }
 
 #[test]

diff --git a/node/core/dispute-coordinator/src/initialized.rs b/node/core/dispute-coordinator/src/initialized.rs
@@ -198,59 +198,62 @@ impl Initialized {
 			gum::trace!(target: LOG_TARGET, "Waiting for message");
 			let mut overlay_db = OverlayedBackend::new(backend);
 			let default_confirm = Box::new(|| Ok(()));
-			let confirm_write =
-				match MuxedMessage::receive(ctx, &mut self.participation_receiver).await? {
-					MuxedMessage::Participation(msg) => {
-						gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation");
-						let ParticipationStatement {
-							session,
+			let confirm_write = match MuxedMessage::receive(ctx, &mut self.participation_receiver)
+				.await?
+			{
+				MuxedMessage::Participation(msg) => {
+					gum::trace!(target: LOG_TARGET, "MuxedMessage::Participation");
+					let ParticipationStatement {
+						session,
+						candidate_hash,
+						candidate_receipt,
+						outcome,
+					} = self.participation.get_participation_result(ctx, msg).await?;
+					if let Some(valid) = outcome.validity() {
+						gum::trace!(
+							target: LOG_TARGET,
+							?session,
+							?candidate_hash,
+							?valid,
+							"Issuing local statement based on participation outcome."
+						);
+						self.issue_local_statement(
+							ctx,
+							&mut overlay_db,
 							candidate_hash,
 							candidate_receipt,
-							outcome,
-						} = self.participation.get_participation_result(ctx, msg).await?;
-						if let Some(valid) = outcome.validity() {
-							gum::trace!(
-								target: LOG_TARGET,
-								?session,
-								?candidate_hash,
-								?valid,
-								"Issuing local statement based on participation outcome."
-							);
-							self.issue_local_statement(
-								ctx,
-								&mut overlay_db,
-								candidate_hash,
-								candidate_receipt,
-								session,
-								valid,
-								clock.now(),
-							)
-							.await?;
-						}
+							session,
+							valid,
+							clock.now(),
+						)
+						.await?;
+					} else {
+						gum::warn!(target: LOG_TARGET, ?outcome, "Dispute participation failed");
+					}
+					default_confirm
+				},
+				MuxedMessage::Subsystem(msg) => match msg {
+					FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()),
+					FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => {
+						gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves");
+						self.process_active_leaves_update(
+							ctx,
+							&mut overlay_db,
+							update,
+							clock.now(),
+						)
+						.await?;
 						default_confirm
 					},
-					MuxedMessage::Subsystem(msg) => match msg {
-						FromOrchestra::Signal(OverseerSignal::Conclude) => return Ok(()),
-						FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update)) => {
-							gum::trace!(target: LOG_TARGET, "OverseerSignal::ActiveLeaves");
-							self.process_active_leaves_update(
-								ctx,
-								&mut overlay_db,
-								update,
-								clock.now(),
-							)
-							.await?;
-							default_confirm
-						},
-						FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => {
-							gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized");
-							self.scraper.process_finalized_block(&n);
-							default_confirm
-						},
-						FromOrchestra::Communication { msg } =>
-							self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?,
+					FromOrchestra::Signal(OverseerSignal::BlockFinalized(_, n)) => {
+						gum::trace!(target: LOG_TARGET, "OverseerSignal::BlockFinalized");
+						self.scraper.process_finalized_block(&n);
+						default_confirm
 					},
-				};
+					FromOrchestra::Communication { msg } =>
+						self.handle_incoming(ctx, &mut overlay_db, msg, clock.now()).await?,
+				},
+			};
 
 			if !overlay_db.is_empty() {
 				let ops = overlay_db.into_write_ops();

diff --git a/node/core/dispute-coordinator/src/participation/mod.rs b/node/core/dispute-coordinator/src/participation/mod.rs
@@ -373,7 +373,7 @@ async fn participate(
 				err,
 			);
 
-			send_result(&mut result_sender, req, ParticipationOutcome::Invalid).await;
+			send_result(&mut result_sender, req, ParticipationOutcome::Error).await;
 		},
 
 		Ok(Ok(ValidationResult::Invalid(invalid))) => {

diff --git a/node/malus/src/variants/common.rs b/node/malus/src/variants/common.rs
@@ -62,8 +62,6 @@ pub enum FakeCandidateValidationError {
 	ParamsTooLarge,
 	/// Code size is over the limit.
 	CodeTooLarge,
-	/// Code does not decompress correctly.
-	CodeDecompressionFailure,
 	/// PoV does not decompress correctly.
 	POVDecompressionFailure,
 	/// Validation function returned invalid data.
@@ -89,8 +87,6 @@ impl Into<InvalidCandidate> for FakeCandidateValidationError {
 			FakeCandidateValidationError::Timeout => InvalidCandidate::Timeout,
 			FakeCandidateValidationError::ParamsTooLarge => InvalidCandidate::ParamsTooLarge(666),
 			FakeCandidateValidationError::CodeTooLarge => InvalidCandidate::CodeTooLarge(666),
-			FakeCandidateValidationError::CodeDecompressionFailure =>
-				InvalidCandidate::CodeDecompressionFailure,
 			FakeCandidateValidationError::POVDecompressionFailure =>
 				InvalidCandidate::PoVDecompressionFailure,
 			FakeCandidateValidationError::BadReturn => InvalidCandidate::BadReturn,

diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs
@@ -237,8 +237,6 @@ pub enum InvalidCandidate {
 	ParamsTooLarge(u64),
 	/// Code size is over the limit.
 	CodeTooLarge(u64),
-	/// Code does not decompress correctly.
-	CodeDecompressionFailure,
 	/// PoV does not decompress correctly.
 	PoVDecompressionFailure,
 	/// Validation function returned invalid data.