forbid v1 descriptors with UMP signals

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

@@ -912,15 +912,10 @@ async fn validate_candidate_exhaustive(
 					// invalid.
 					Ok(ValidationResult::Invalid(InvalidCandidate::CommitmentsHashMismatch))
 				} else {
-					let core_index = candidate_receipt.descriptor.core_index();
-
-					match (core_index, exec_kind) {
+					match exec_kind {
 						// Core selectors are optional for V2 descriptors, but we still check the
 						// descriptor core index.
-						(
-							Some(_core_index),
-							PvfExecKind::Backing(_) | PvfExecKind::BackingSystemParas(_),
-						) => {
+						PvfExecKind::Backing(_) | PvfExecKind::BackingSystemParas(_) => {
 							let Some(claim_queue) = maybe_claim_queue else {
 								let error = "cannot fetch the claim queue from the runtime";
 								gum::warn!(
@@ -937,17 +932,17 @@ async fn validate_candidate_exhaustive(
 							{
 								gum::warn!(
 									target: LOG_TARGET,
-									?err,
 									candidate_hash = ?candidate_receipt.hash(),
-									"Candidate core index is invalid",
+									"Candidate core index is invalid: {}",
+									err
 								);
 								return Ok(ValidationResult::Invalid(
 									InvalidCandidate::InvalidCoreIndex,
 								))
 							}
 						},
 						// No checks for approvals and disputes
-						(_, _) => {},
+						_ => {},
 					}
 
 					Ok(ValidationResult::Valid(

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

@@ -30,8 +30,8 @@ use polkadot_node_subsystem_util::reexports::SubsystemContext;
 use polkadot_overseer::ActivatedLeaf;
 use polkadot_primitives::{
 	vstaging::{
-		CandidateDescriptorV2, ClaimQueueOffset, CoreSelector, MutateDescriptorV2, UMPSignal,
-		UMP_SEPARATOR,
+		CandidateDescriptorV2, CandidateDescriptorVersion, ClaimQueueOffset, CoreSelector,
+		MutateDescriptorV2, UMPSignal, UMP_SEPARATOR,
 	},
 	CandidateDescriptor, CoreIndex, GroupIndex, HeadData, Id as ParaId, OccupiedCoreAssumption,
 	SessionInfo, UpwardMessage, ValidatorId,
@@ -851,7 +851,7 @@ fn invalid_session_or_core_index() {
 	))
 	.unwrap();
 
-	// Validation doesn't fail for approvals, core/session index is not checked.
+	// Validation doesn't fail for disputes, core/session index is not checked.
 	assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => {
 		assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1]));
 		assert_eq!(outputs.upward_messages, commitments.upward_messages);
@@ -911,6 +911,69 @@ fn invalid_session_or_core_index() {
 		assert_eq!(outputs.hrmp_watermark, 0);
 		assert_eq!(used_validation_data, validation_data);
 	});
+
+	// Test that a v1 candidate that outputs the core selector UMP signal is invalid.
+	let descriptor_v1 = make_valid_candidate_descriptor(
+		ParaId::from(1_u32),
+		dummy_hash(),
+		dummy_hash(),
+		pov.hash(),
+		validation_code.hash(),
+		validation_result.head_data.hash(),
+		dummy_hash(),
+		sp_keyring::Sr25519Keyring::Ferdie,
+	);
+	let descriptor: CandidateDescriptorV2 = descriptor_v1.into();
+
+	perform_basic_checks(&descriptor, validation_data.max_pov_size, &pov, &validation_code.hash())
+		.unwrap();
+	assert_eq!(descriptor.version(), CandidateDescriptorVersion::V1);
+	let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() };
+
+	for exec_kind in
+		[PvfExecKind::Backing(dummy_hash()), PvfExecKind::BackingSystemParas(dummy_hash())]
+	{
+		let result = executor::block_on(validate_candidate_exhaustive(
+			Some(1),
+			MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())),
+			validation_data.clone(),
+			validation_code.clone(),
+			candidate_receipt.clone(),
+			Arc::new(pov.clone()),
+			ExecutorParams::default(),
+			exec_kind,
+			&Default::default(),
+			Some(Default::default()),
+		))
+		.unwrap();
+		assert_matches!(result, ValidationResult::Invalid(InvalidCandidate::InvalidCoreIndex));
+	}
+
+	// Validation doesn't fail for approvals and disputes, core/session index is not checked.
+	for exec_kind in [PvfExecKind::Approval, PvfExecKind::Dispute] {
+		let v = executor::block_on(validate_candidate_exhaustive(
+			Some(1),
+			MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result.clone())),
+			validation_data.clone(),
+			validation_code.clone(),
+			candidate_receipt.clone(),
+			Arc::new(pov.clone()),
+			ExecutorParams::default(),
+			exec_kind,
+			&Default::default(),
+			Default::default(),
+		))
+		.unwrap();
+
+		assert_matches!(v, ValidationResult::Valid(outputs, used_validation_data) => {
+			assert_eq!(outputs.head_data, HeadData(vec![1, 1, 1]));
+			assert_eq!(outputs.upward_messages, commitments.upward_messages);
+			assert_eq!(outputs.horizontal_messages, Vec::new());
+			assert_eq!(outputs.new_validation_code, Some(vec![2, 2, 2].into()));
+			assert_eq!(outputs.hrmp_watermark, 0);
+			assert_eq!(used_validation_data, validation_data);
+		});
+	}
 }
 
 #[test]
@@ -1407,7 +1470,7 @@ fn compressed_code_works() {
 		ExecutorParams::default(),
 		PvfExecKind::Backing(dummy_hash()),
 		&Default::default(),
-		Default::default(),
+		Some(Default::default()),
 	));
 
 	assert_matches!(v, Ok(ValidationResult::Valid(_, _)));

polkadot/primitives/src/vstaging/mod.rs

@@ -505,6 +505,10 @@ pub enum CommittedCandidateReceiptError {
 	/// Currenly only one such message is allowed.
 	#[cfg_attr(feature = "std", error("Too many UMP signals"))]
 	TooManyUMPSignals,
+	/// If the parachain runtime started sending core selectors, v1 descriptors are no longer
+	/// allowed.
+	#[cfg_attr(feature = "std", error("Version 1 receipt does not support core selectors"))]
+	CoreSelectorWithV1Decriptor,
 }
 
 macro_rules! impl_getter {
@@ -603,15 +607,25 @@ impl<H: Copy> CommittedCandidateReceiptV2<H> {
 		&self,
 		cores_per_para: &TransposedClaimQueue,
 	) -> Result<(), CommittedCandidateReceiptError> {
+		let maybe_core_selector = self.commitments.core_selector()?;
+
 		match self.descriptor.version() {
-			// Don't check v1 descriptors.
-			CandidateDescriptorVersion::V1 => return Ok(()),
+			CandidateDescriptorVersion::V1 => {
+				// If the parachain runtime started sending core selectors, v1 descriptors are no
+				// longer allowed.
+				if maybe_core_selector.is_some() {
+					return Err(CommittedCandidateReceiptError::CoreSelectorWithV1Decriptor)
+				} else {
+					// Nothing else to check for v1 descriptors.
+					return Ok(())
+				}
+			},
 			CandidateDescriptorVersion::V2 => {},
 			CandidateDescriptorVersion::Unknown =>
 				return Err(CommittedCandidateReceiptError::UnknownVersion(self.descriptor.version)),
 		}
 
-		let (maybe_core_index_selector, cq_offset) = self.commitments.core_selector()?.map_or_else(
+		let (maybe_core_index_selector, cq_offset) = maybe_core_selector.map_or_else(
 			|| (None, ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET)),
 			|(sel, off)| (Some(sel), off),
 		);
@@ -1207,8 +1221,7 @@ mod tests {
 		assert_eq!(new_ccr.hash(), v2_ccr.hash());
 	}
 
-	// Only check descriptor `core_index` field of v2 descriptors. If it is v1, that field
-	// will be garbage.
+	// V1 descriptors are forbidden once the parachain runtime started sending UMP signals.
 	#[test]
 	fn test_v1_descriptors_with_ump_signal() {
 		let mut ccr = dummy_old_committed_candidate_receipt();
@@ -1234,9 +1247,12 @@ mod tests {
 		cq.insert(CoreIndex(0), vec![v1_ccr.descriptor.para_id()].into());
 		cq.insert(CoreIndex(1), vec![v1_ccr.descriptor.para_id()].into());
 
-		assert!(v1_ccr.check_core_index(&transpose_claim_queue(cq)).is_ok());
-
 		assert_eq!(v1_ccr.descriptor.core_index(), None);
+
+		assert_eq!(
+			v1_ccr.check_core_index(&transpose_claim_queue(cq)),
+			Err(CommittedCandidateReceiptError::CoreSelectorWithV1Decriptor)
+		);
 	}
 
 	#[test]

prdoc/pr_7127.prdoc

@@ -0,0 +1,9 @@
+title: 'Forbid v1 descriptors with UMP signals'
+doc:
+- audience: [Runtime Dev, Node Dev]
+  description: Adds a check that parachain candidates do not send out UMP signals with v1 descriptors.
+crates:
+- name: polkadot-node-core-candidate-validation
+  bump: minor
+- name: polkadot-primitives
+  bump: major