more verbose assignment cert errors (#5433)

* more verbose assignment cert errors

* rename a variant to InvalidAssignmentKey

polkadot/node/core/approval-voting/src/criteria.rs

@@ -429,16 +429,30 @@ fn compute_relay_vrf_delay_assignments(
 
 /// Assignment invalid.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct InvalidAssignment;
+pub struct InvalidAssignment(pub(crate) InvalidAssignmentReason);
 
 impl std::fmt::Display for InvalidAssignment {
 	fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-		write!(f, "Invalid Assignment")
+		write!(f, "Invalid Assignment: {:?}", self.0)
 	}
 }
 
 impl std::error::Error for InvalidAssignment {}
 
+/// Failure conditions when checking an assignment cert.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum InvalidAssignmentReason {
+	ValidatorIndexOutOfBounds,
+	SampleOutOfBounds,
+	CoreIndexOutOfBounds,
+	InvalidAssignmentKey,
+	IsInBackingGroup,
+	VRFModuloCoreIndexMismatch,
+	VRFModuloOutputMismatch,
+	VRFDelayCoreIndexMismatch,
+	VRFDelayOutputMismatch,
+}
+
 /// Checks the crypto of an assignment cert. Failure conditions:
 ///   * Validator index out of bounds
 ///   * VRF signature check fails
@@ -458,16 +472,18 @@ pub(crate) fn check_assignment_cert(
 	assignment: &AssignmentCert,
 	backing_group: GroupIndex,
 ) -> Result<DelayTranche, InvalidAssignment> {
+	use InvalidAssignmentReason as Reason;
+
 	let validator_public = config
 		.assignment_keys
 		.get(validator_index.0 as usize)
-		.ok_or(InvalidAssignment)?;
+		.ok_or(InvalidAssignment(Reason::ValidatorIndexOutOfBounds))?;
 
 	let public = schnorrkel::PublicKey::from_bytes(validator_public.as_slice())
-		.map_err(|_| InvalidAssignment)?;
+		.map_err(|_| InvalidAssignment(Reason::InvalidAssignmentKey))?;
 
 	if claimed_core_index.0 >= config.n_cores {
-		return Err(InvalidAssignment)
+		return Err(InvalidAssignment(Reason::CoreIndexOutOfBounds))
 	}
 
 	// Check that the validator was not part of the backing group
@@ -476,14 +492,14 @@ pub(crate) fn check_assignment_cert(
 		is_in_backing_group(&config.validator_groups, validator_index, backing_group);
 
 	if is_in_backing {
-		return Err(InvalidAssignment)
+		return Err(InvalidAssignment(Reason::IsInBackingGroup))
 	}
 
 	let &(ref vrf_output, ref vrf_proof) = &assignment.vrf;
 	match assignment.kind {
 		AssignmentCertKind::RelayVRFModulo { sample } => {
 			if sample >= config.relay_vrf_modulo_samples {
-				return Err(InvalidAssignment)
+				return Err(InvalidAssignment(Reason::SampleOutOfBounds))
 			}
 
 			let (vrf_in_out, _) = public
@@ -493,18 +509,18 @@ pub(crate) fn check_assignment_cert(
 					&vrf_proof.0,
 					assigned_core_transcript(claimed_core_index),
 				)
-				.map_err(|_| InvalidAssignment)?;
+				.map_err(|_| InvalidAssignment(Reason::VRFModuloOutputMismatch))?;
 
 			// ensure that the `vrf_in_out` actually gives us the claimed core.
 			if relay_vrf_modulo_core(&vrf_in_out, config.n_cores) == claimed_core_index {
 				Ok(0)
 			} else {
-				Err(InvalidAssignment)
+				Err(InvalidAssignment(Reason::VRFModuloCoreIndexMismatch))
 			}
 		},
 		AssignmentCertKind::RelayVRFDelay { core_index } => {
 			if core_index != claimed_core_index {
-				return Err(InvalidAssignment)
+				return Err(InvalidAssignment(Reason::VRFDelayCoreIndexMismatch))
 			}
 
 			let (vrf_in_out, _) = public
@@ -513,7 +529,7 @@ pub(crate) fn check_assignment_cert(
 					&vrf_output.0,
 					&vrf_proof.0,
 				)
-				.map_err(|_| InvalidAssignment)?;
+				.map_err(|_| InvalidAssignment(Reason::VRFDelayOutputMismatch))?;
 
 			Ok(relay_vrf_delay_tranche(
 				&vrf_in_out,

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

@@ -1595,10 +1595,11 @@ fn check_and_import_assignment(
 		);
 
 		let tranche = match res {
-			Err(crate::criteria::InvalidAssignment) =>
+			Err(crate::criteria::InvalidAssignment(reason)) =>
 				return Ok((
 					AssignmentCheckResult::Bad(AssignmentCheckError::InvalidCert(
 						assignment.validator,
+						format!("{:?}", reason),
 					)),
 					Vec::new(),
 				)),

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

@@ -1007,8 +1007,11 @@ fn subsystem_rejects_bad_assignment_ok_criteria() {
 
 #[test]
 fn subsystem_rejects_bad_assignment_err_criteria() {
-	let assignment_criteria =
-		Box::new(MockAssignmentCriteria::check_only(move |_| Err(criteria::InvalidAssignment)));
+	let assignment_criteria = Box::new(MockAssignmentCriteria::check_only(move |_| {
+		Err(criteria::InvalidAssignment(
+			criteria::InvalidAssignmentReason::ValidatorIndexOutOfBounds,
+		))
+	}));
 	let config = HarnessConfigBuilder::default().assignment_criteria(assignment_criteria).build();
 	test_harness(config, |test_harness| async move {
 		let TestHarness { mut virtual_overseer, sync_oracle_handle: _sync_oracle_handle, .. } =
@@ -1045,7 +1048,10 @@ fn subsystem_rejects_bad_assignment_err_criteria() {
 
 		assert_eq!(
 			rx.await,
-			Ok(AssignmentCheckResult::Bad(AssignmentCheckError::InvalidCert(ValidatorIndex(0)))),
+			Ok(AssignmentCheckResult::Bad(AssignmentCheckError::InvalidCert(
+				ValidatorIndex(0),
+				"ValidatorIndexOutOfBounds".to_string(),
+			))),
 		);
 
 		virtual_overseer
@@ -2813,7 +2819,9 @@ fn pre_covers_dont_stall_approval() {
 		move |validator_index| match validator_index {
 			ValidatorIndex(0 | 1) => Ok(0),
 			ValidatorIndex(2) => Ok(1),
-			ValidatorIndex(_) => Err(criteria::InvalidAssignment),
+			ValidatorIndex(_) => Err(criteria::InvalidAssignment(
+				criteria::InvalidAssignmentReason::ValidatorIndexOutOfBounds,
+			)),
 		},
 	));