Malus: improvements in dispute ancestor and suggest garbage candidate implementation

cli/src/cli.rs

@@ -98,6 +98,7 @@ pub struct ValidationWorkerCommand {
 
 #[allow(missing_docs)]
 #[derive(Debug, Parser)]
+#[cfg_attr(feature = "malus", derive(Clone))]
 pub struct RunCmd {
 	#[allow(missing_docs)]
 	#[clap(flatten)]

node/core/approval-voting/src/lib.rs

@@ -2277,6 +2277,7 @@ async fn launch_approval(
 					available_data.pov,
 					APPROVAL_EXECUTION_TIMEOUT,
 					val_tx,
+					candidate.commitments_hash,
 				)
 				.into(),
 			)

node/core/approval-voting/src/tests.rs

@@ -2403,7 +2403,7 @@ pub async fn handle_double_assignment_import(
 				assert_eq!(candidate_index, c_index);
 			},
 			AllMessages::CandidateValidation(
-				CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, timeout, tx),
+				CandidateValidationMessage::ValidateFromExhaustive(_, _, _, _, timeout, tx, _),
 			) if timeout == APPROVAL_EXECUTION_TIMEOUT => {
 				tx.send(Ok(ValidationResult::Valid(Default::default(), Default::default())))
 					.unwrap();

node/core/backing/src/lib.rs

@@ -31,8 +31,8 @@ use futures::{
 };
 
 use polkadot_node_primitives::{
-	AvailableData, PoV, SignedDisputeStatement, SignedFullStatement, Statement, ValidationResult,
-	BACKING_EXECUTION_TIMEOUT,
+	AvailableData, InvalidCandidate, PoV, SignedDisputeStatement, SignedFullStatement, Statement,
+	ValidationResult, BACKING_EXECUTION_TIMEOUT,
 };
 use polkadot_node_subsystem_util::{
 	self as util,
@@ -380,6 +380,7 @@ async fn request_candidate_validation(
 	sender: &mut JobSender<impl SubsystemSender>,
 	candidate: CandidateDescriptor,
 	pov: Arc<PoV>,
+	commitments_hash: Hash,
 ) -> Result<ValidationResult, Error> {
 	let (tx, rx) = oneshot::channel();
 
@@ -389,6 +390,7 @@ async fn request_candidate_validation(
 			pov,
 			BACKING_EXECUTION_TIMEOUT,
 			tx,
+			commitments_hash,
 		))
 		.await;
 
@@ -450,17 +452,23 @@ async fn validate_and_make_available(
 		},
 	};
 
+	let expected_commitments_hash = candidate.commitments_hash;
+
 	let v = {
 		let _span = span.as_ref().map(|s| {
 			s.child("request-validation")
 				.with_pov(&pov)
 				.with_para_id(candidate.descriptor().para_id)
 		});
-		request_candidate_validation(&mut sender, candidate.descriptor.clone(), pov.clone()).await?
+		request_candidate_validation(
+			&mut sender,
+			candidate.descriptor.clone(),
+			pov.clone(),
+			expected_commitments_hash,
+		)
+		.await?
 	};
 
-	let expected_commitments_hash = candidate.commitments_hash;
-
 	let res = match v {
 		ValidationResult::Valid(commitments, validation_data) => {
 			gum::debug!(
@@ -469,41 +477,39 @@ async fn validate_and_make_available(
 				"Validation successful",
 			);
 
-			// If validation produces a new set of commitments, we vote the candidate as invalid.
-			if commitments.hash() != expected_commitments_hash {
-				gum::debug!(
-					target: LOG_TARGET,
-					candidate_hash = ?candidate.hash(),
-					actual_commitments = ?commitments,
-					"Commitments obtained with validation don't match the announced by the candidate receipt",
-				);
-				Err(candidate)
-			} else {
-				let erasure_valid = make_pov_available(
-					&mut sender,
-					n_validators,
-					pov.clone(),
-					candidate.hash(),
-					validation_data,
-					candidate.descriptor.erasure_root,
-					span.as_ref(),
-				)
-				.await?;
+			let erasure_valid = make_pov_available(
+				&mut sender,
+				n_validators,
+				pov.clone(),
+				candidate.hash(),
+				validation_data,
+				candidate.descriptor.erasure_root,
+				span.as_ref(),
+			)
+			.await?;
 
-				match erasure_valid {
-					Ok(()) => Ok((candidate, commitments, pov.clone())),
-					Err(InvalidErasureRoot) => {
-						gum::debug!(
-							target: LOG_TARGET,
-							candidate_hash = ?candidate.hash(),
-							actual_commitments = ?commitments,
-							"Erasure root doesn't match the announced by the candidate receipt",
-						);
-						Err(candidate)
-					},
-				}
+			match erasure_valid {
+				Ok(()) => Ok((candidate, commitments, pov.clone())),
+				Err(InvalidErasureRoot) => {
+					gum::debug!(
+						target: LOG_TARGET,
+						candidate_hash = ?candidate.hash(),
+						actual_commitments = ?commitments,
+						"Erasure root doesn't match the announced by the candidate receipt",
+					);
+					Err(candidate)
+				},
 			}
 		},
+		ValidationResult::Invalid(InvalidCandidate::ComittmentsHashMismatch) => {
+			// If validation produces a new set of commitments, we vote the candidate as invalid.
+			gum::warn!(
+				target: LOG_TARGET,
+				candidate_hash = ?candidate.hash(),
+				"Validation yielded different commitments",
+			);
+			Err(candidate)
+		},
 		ValidationResult::Invalid(reason) => {
 			gum::debug!(
 				target: LOG_TARGET,

node/core/backing/src/tests.rs

@@ -336,8 +336,9 @@ fn backing_second_works() {
 					pov,
 					timeout,
 					tx,
+					commitments_hash
 				)
-			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && commitments_hash == candidate.commitments.hash() => {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -419,6 +420,8 @@ fn backing_works() {
 		.build();
 
 		let candidate_a_hash = candidate_a.hash();
+		let candidate_a_commitments_hash = candidate_a.commitments.hash();
+
 		let public1 = CryptoStore::sr25519_generate_new(
 			&*test_state.keystore,
 			ValidatorId::ID,
@@ -496,8 +499,9 @@ fn backing_works() {
 					pov,
 					timeout,
 					tx,
+					hash
 				)
-			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && hash == candidate_a_commitments_hash=> {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -594,6 +598,8 @@ fn backing_works_while_validation_ongoing() {
 		.build();
 
 		let candidate_a_hash = candidate_a.hash();
+		let candidate_a_commitments_hash = candidate_a.commitments.hash();
+
 		let public1 = CryptoStore::sr25519_generate_new(
 			&*test_state.keystore,
 			ValidatorId::ID,
@@ -690,8 +696,9 @@ fn backing_works_while_validation_ongoing() {
 					pov,
 					timeout,
 					tx,
+					hash
 				)
-			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && candidate_a_commitments_hash == hash => {
 				// we never validate the candidate. our local node
 				// shouldn't issue any statements.
 				std::mem::forget(tx);
@@ -799,6 +806,8 @@ fn backing_misbehavior_works() {
 		.build();
 
 		let candidate_a_hash = candidate_a.hash();
+		let candidate_a_commitments_hash = candidate_a.commitments.hash();
+
 		let public2 = CryptoStore::sr25519_generate_new(
 			&*test_state.keystore,
 			ValidatorId::ID,
@@ -864,8 +873,9 @@ fn backing_misbehavior_works() {
 					pov,
 					timeout,
 					tx,
+					hash
 				)
-			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
+			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && candidate_a_commitments_hash == hash => {
 				tx.send(Ok(
 					ValidationResult::Valid(CandidateCommitments {
 						head_data: expected_head_data.clone(),
@@ -1024,6 +1034,7 @@ fn backing_dont_second_invalid() {
 					pov,
 					timeout,
 					tx,
+					_
 				)
 			) if pov == pov && &c == candidate_a.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap();
@@ -1053,6 +1064,7 @@ fn backing_dont_second_invalid() {
 					pov,
 					timeout,
 					tx,
+					_
 				)
 			) if pov == pov && &c == candidate_b.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
 				tx.send(Ok(
@@ -1184,8 +1196,9 @@ fn backing_second_after_first_fails_works() {
 					pov,
 					timeout,
 					tx,
+					hash
 				)
-			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && hash == candidate.commitments.hash() => {
 				tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap();
 			}
 		);
@@ -1232,6 +1245,7 @@ fn backing_second_after_first_fails_works() {
 					pov,
 					_,
 					_,
+					_,
 				)
 			) => {
 				assert_eq!(&*pov, &pov_to_second);
@@ -1318,8 +1332,9 @@ fn backing_works_after_failed_validation() {
 					pov,
 					timeout,
 					tx,
+					hash
 				)
-			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT => {
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && hash == candidate.commitments.hash() => {
 				tx.send(Err(ValidationFailed("Internal test error".into()))).unwrap();
 			}
 		);
@@ -1696,8 +1711,9 @@ fn retry_works() {
 					pov,
 					timeout,
 					_tx,
+					hash,
 				)
-			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT
+			) if pov == pov && &c == candidate.descriptor() && timeout == BACKING_EXECUTION_TIMEOUT && hash == candidate.commitments.hash()
 		);
 		virtual_overseer
 	});