From 69af87a1996d331e18815c9d8a0551d4032e0220 Mon Sep 17 00:00:00 2001 From: Andronik Date: Wed, 4 May 2022 14:45:11 +0200 Subject: [PATCH] more verbose assignment cert errors (#5433) * more verbose assignment cert errors * rename a variant to InvalidAssignmentKey --- .../node/core/approval-voting/src/criteria.rs | 38 +++++++++++++------ polkadot/node/core/approval-voting/src/lib.rs | 3 +- .../node/core/approval-voting/src/tests.rs | 16 ++++++-- polkadot/node/subsystem-types/src/messages.rs | 4 +- 4 files changed, 43 insertions(+), 18 deletions(-) diff --git a/polkadot/node/core/approval-voting/src/criteria.rs b/polkadot/node/core/approval-voting/src/criteria.rs index a2bd0889f0..d7bedc9b0a 100644 --- a/polkadot/node/core/approval-voting/src/criteria.rs +++ b/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 { + 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, diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index c3892a160e..a67cb4bbaa 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/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(), )), diff --git a/polkadot/node/core/approval-voting/src/tests.rs b/polkadot/node/core/approval-voting/src/tests.rs index 165b66828a..9199da4f98 100644 --- a/polkadot/node/core/approval-voting/src/tests.rs +++ b/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, + )), }, )); diff --git a/polkadot/node/subsystem-types/src/messages.rs b/polkadot/node/subsystem-types/src/messages.rs index b4eef7c8c1..9394416d32 100644 --- a/polkadot/node/subsystem-types/src/messages.rs +++ b/polkadot/node/subsystem-types/src/messages.rs @@ -810,8 +810,8 @@ pub enum AssignmentCheckError { InvalidCandidateIndex(CandidateIndex), #[error("Invalid candidate {0}: {1:?}")] InvalidCandidate(CandidateIndex, CandidateHash), - #[error("Invalid cert: {0:?}")] - InvalidCert(ValidatorIndex), + #[error("Invalid cert: {0:?}, reason: {1}")] + InvalidCert(ValidatorIndex, String), #[error("Internal state mismatch: {0:?}, {1:?}")] Internal(Hash, CandidateHash), }