[3 / 5] Move crypto checks in the approval-distribution

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

@@ -254,7 +254,7 @@ impl<'a> From<&'a SessionInfo> for Config {
 }
 
 /// A trait for producing and checking assignments. Used to mock.
-pub(crate) trait AssignmentCriteria {
+pub trait AssignmentCriteria {
 	fn compute_assignments(
 		&self,
 		keystore: &LocalKeystore,
@@ -276,7 +276,7 @@ pub(crate) trait AssignmentCriteria {
 	) -> Result<DelayTranche, InvalidAssignment>;
 }
 
-pub(crate) struct RealAssignmentCriteria;
+pub struct RealAssignmentCriteria;
 
 impl AssignmentCriteria for RealAssignmentCriteria {
 	fn compute_assignments(
@@ -614,7 +614,7 @@ fn compute_relay_vrf_delay_assignments(
 
 /// Assignment invalid.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct InvalidAssignment(pub(crate) InvalidAssignmentReason);
+pub struct InvalidAssignment(pub InvalidAssignmentReason);
 
 impl std::fmt::Display for InvalidAssignment {
 	fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
@@ -626,7 +626,7 @@ impl std::error::Error for InvalidAssignment {}
 
 /// Failure conditions when checking an assignment cert.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub(crate) enum InvalidAssignmentReason {
+pub enum InvalidAssignmentReason {
 	ValidatorIndexOutOfBounds,
 	SampleOutOfBounds,
 	CoreIndexOutOfBounds,

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

@@ -574,9 +574,13 @@ pub(crate) async fn handle_new_head<Context, B: Backend>(
 			hash: block_hash,
 			number: block_header.number,
 			parent_hash: block_header.parent_hash,
-			candidates: included_candidates.iter().map(|(hash, _, _, _)| *hash).collect(),
+			candidates: included_candidates
+				.iter()
+				.map(|(hash, _, core_index, group_index)| (*hash, *core_index, *group_index))
+				.collect(),
 			slot,
 			session: session_index,
+			vrf_story: relay_vrf_story,
 		});
 
 		imported_candidates.push(BlockImportedCandidates {

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

@@ -55,9 +55,8 @@ use polkadot_node_subsystem_util::{
 };
 use polkadot_primitives::{
 	ApprovalVoteMultipleCandidates, ApprovalVotingParams, BlockNumber, CandidateHash,
-	CandidateIndex, CandidateReceipt, CoreIndex, DisputeStatement, ExecutorParams, GroupIndex,
-	Hash, PvfExecKind, SessionIndex, SessionInfo, ValidDisputeStatementKind, ValidatorId,
-	ValidatorIndex, ValidatorPair, ValidatorSignature,
+	CandidateIndex, CandidateReceipt, CoreIndex, ExecutorParams, GroupIndex, Hash, PvfExecKind,
+	SessionIndex, SessionInfo, ValidatorId, ValidatorIndex, ValidatorPair, ValidatorSignature,
 };
 use sc_keystore::LocalKeystore;
 use sp_application_crypto::Pair;
@@ -91,7 +90,7 @@ use schnellru::{ByLength, LruMap};
 
 use approval_checking::RequiredTranches;
 use bitvec::{order::Lsb0, vec::BitVec};
-use criteria::{AssignmentCriteria, RealAssignmentCriteria};
+pub use criteria::{AssignmentCriteria, Config as AssignmentConfig, RealAssignmentCriteria};
 use persisted_entries::{ApprovalEntry, BlockEntry, CandidateEntry};
 use time::{slot_number_to_tick, Clock, ClockExt, DelayedApprovalTimer, SystemClock, Tick};
 
@@ -123,7 +122,7 @@ const APPROVAL_CHECKING_TIMEOUT: Duration = Duration::from_secs(120);
 const WAIT_FOR_SIGS_TIMEOUT: Duration = Duration::from_millis(500);
 const APPROVAL_CACHE_SIZE: u32 = 1024;
 
-const TICK_TOO_FAR_IN_FUTURE: Tick = 20; // 10 seconds.
+pub const TICK_TOO_FAR_IN_FUTURE: Tick = 20; // 10 seconds.
 const APPROVAL_DELAY: Tick = 2;
 pub(crate) const LOG_TARGET: &str = "parachain::approval-voting";
 
@@ -1607,9 +1606,21 @@ async fn distribution_messages_for_activation<Context>(
 			hash: block_hash,
 			number: block_entry.block_number(),
 			parent_hash: block_entry.parent_hash(),
-			candidates: block_entry.candidates().iter().map(|(_, c_hash)| *c_hash).collect(),
+			candidates: block_entry
+				.candidates()
+				.iter()
+				.flat_map(|(core_index, c_hash)| {
+					let candidate = db.load_candidate_entry(c_hash).ok().flatten();
+					candidate
+						.and_then(|entry| {
+							entry.approval_entry(&block_hash).map(|entry| entry.backing_group())
+						})
+						.and_then(|group_index| Some((*c_hash, *core_index, group_index)))
+				})
+				.collect(),
 			slot: block_entry.slot(),
 			session: block_entry.session(),
+			vrf_story: block_entry.relay_vrf_story(),
 		});
 		let mut signatures_queued = HashSet::new();
 		for (core_index, candidate_hash) in block_entry.candidates() {
@@ -1872,35 +1883,47 @@ async fn handle_from_overseer<Context>(
 			vec![Action::Conclude]
 		},
 		FromOrchestra::Communication { msg } => match msg {
-			ApprovalVotingMessage::CheckAndImportAssignment(a, claimed_cores, res) => {
+			ApprovalVotingMessage::CheckAndImportAssignment(a, claimed_cores, tranche, tx) => {
 				let (check_outcome, actions) = check_and_import_assignment(
 					ctx.sender(),
 					state,
 					db,
 					session_info_provider,
 					a,
 					claimed_cores,
+					tranche,
 				)
 				.await?;
-				let _ = res.send(check_outcome);
-
+				// approval-distribution already sanity checks the assignment is valid and expected,
+				// so this import should never fail if it does it means there is a bug in the
+				// code.
+				if let AssignmentCheckResult::Bad(ref err) = check_outcome {
+					gum::error!(target: LOG_TARGET, ?err, "Unexpected fail to check and import assignment");
+				}
+				let _ = tx.map(|tx| tx.send(check_outcome));
 				actions
 			},
-			ApprovalVotingMessage::CheckAndImportApproval(a, res) =>
-				check_and_import_approval(
+			ApprovalVotingMessage::CheckAndImportApproval(a, tx) => {
+				let result = check_and_import_approval(
 					ctx.sender(),
 					state,
 					db,
 					session_info_provider,
 					metrics,
 					a,
-					|r| {
-						let _ = res.send(r);
-					},
 					&wakeups,
 				)
-				.await?
-				.0,
+				.await?;
+				// approval-distribution already sanity checks the vote is valid and expected,
+				// so this import should never fail if it does it means there is a bug in the
+				// code.
+				if let ApprovalCheckResult::Bad(ref err) = result.1 {
+					gum::error!(target: LOG_TARGET, ?err, "Unexpected fail to check and import approval");
+				}
+				let _ = tx.map(|tx| tx.send(result.1));
+
+				result.0
+			},
 			ApprovalVotingMessage::ApprovedAncestor(target, lower_bound, res) => {
 				let mut approved_ancestor_span = state
 					.spans
@@ -2384,7 +2407,12 @@ fn schedule_wakeup_action(
 				last_assignment_tick.map(|l| l + APPROVAL_DELAY).filter(|t| t > &tick_now),
 				next_no_show,
 			)
-			.map(|tick| Action::ScheduleWakeup { block_hash, block_number, candidate_hash, tick })
+			.map(|tick| Action::ScheduleWakeup {
+				block_hash,
+				block_number,
+				candidate_hash,
+				tick,
+			})
 		},
 		RequiredTranches::Pending { considered, next_no_show, clock_drift, .. } => {
 			// select the minimum of `next_no_show`, or the tick of the next non-empty tranche
@@ -2446,6 +2474,7 @@ async fn check_and_import_assignment<Sender>(
 	session_info_provider: &mut RuntimeInfo,
 	assignment: IndirectAssignmentCertV2,
 	candidate_indices: CandidateBitfield,
+	tranche: DelayTranche,
 ) -> SubsystemResult<(AssignmentCheckResult, Vec<Action>)>
 where
 	Sender: SubsystemSender<RuntimeApiMessage>,
@@ -2514,8 +2543,6 @@ where
 		))
 	}
 
-	// The Compact VRF modulo assignment cert has multiple core assignments.
-	let mut backing_groups = Vec::new();
 	let mut claimed_core_indices = Vec::new();
 	let mut assigned_candidate_hashes = Vec::new();
 
@@ -2551,7 +2578,7 @@ where
 			format!("{:?}", jaeger::hash_to_trace_identifier(assigned_candidate_hash.0)),
 		);
 
-		let approval_entry = match candidate_entry.approval_entry_mut(&assignment.block_hash) {
+		let _approval_entry = match candidate_entry.approval_entry_mut(&assignment.block_hash) {
 			Some(a) => a,
 			None =>
 				return Ok((
@@ -2563,7 +2590,6 @@ where
 				)),
 		};
 
-		backing_groups.push(approval_entry.backing_group());
 		claimed_core_indices.push(claimed_core_index);
 		assigned_candidate_hashes.push(assigned_candidate_hash);
 	}
@@ -2579,42 +2605,6 @@ where
 		))
 	}
 
-	// Check the assignment certificate.
-	let res = state.assignment_criteria.check_assignment_cert(
-		claimed_core_indices
-			.clone()
-			.try_into()
-			.expect("Checked for null assignment above; qed"),
-		assignment.validator,
-		&criteria::Config::from(session_info),
-		block_entry.relay_vrf_story(),
-		&assignment.cert,
-		backing_groups,
-	);
-
-	let tranche = match res {
-		Err(crate::criteria::InvalidAssignment(reason)) =>
-			return Ok((
-				AssignmentCheckResult::Bad(AssignmentCheckError::InvalidCert(
-					assignment.validator,
-					format!("{:?}", reason),
-				)),
-				Vec::new(),
-			)),
-		Ok(tranche) => {
-			let current_tranche =
-				state.clock.tranche_now(state.slot_duration_millis, block_entry.slot());
-
-			let too_far_in_future = current_tranche + TICK_TOO_FAR_IN_FUTURE as DelayTranche;
-
-			if tranche >= too_far_in_future {
-				return Ok((AssignmentCheckResult::TooFarInFuture, Vec::new()))
-			}
-
-			tranche
-		},
-	};
-
 	let mut actions = Vec::new();
 	let res = {
 		let mut is_duplicate = true;
@@ -2704,23 +2694,21 @@ where
 	Ok((res, actions))
 }
 
-async fn check_and_import_approval<T, Sender>(
+async fn check_and_import_approval<Sender>(
 	sender: &mut Sender,
 	state: &mut State,
 	db: &mut OverlayedBackend<'_, impl Backend>,
 	session_info_provider: &mut RuntimeInfo,
 	metrics: &Metrics,
 	approval: IndirectSignedApprovalVoteV2,
-	with_response: impl FnOnce(ApprovalCheckResult) -> T,
 	wakeups: &Wakeups,
-) -> SubsystemResult<(Vec<Action>, T)>
+) -> SubsystemResult<(Vec<Action>, ApprovalCheckResult)>
 where
 	Sender: SubsystemSender<RuntimeApiMessage>,
 {
 	macro_rules! respond_early {
 		($e: expr) => {{
-			let t = with_response($e);
-			return Ok((Vec::new(), t))
+			return Ok((Vec::new(), $e))
 		}};
 	}
 	let mut span = state
@@ -2774,67 +2762,11 @@ where
 		),
 	);
 
-	{
-		let session_info = match get_session_info(
-			session_info_provider,
-			sender,
-			approval.block_hash,
-			block_entry.session(),
-		)
-		.await
-		{
-			Some(s) => s,
-			None => {
-				respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::UnknownSessionIndex(
-					block_entry.session()
-				),))
-			},
-		};
-
-		let pubkey = match session_info.validators.get(approval.validator) {
-			Some(k) => k,
-			None => respond_early!(ApprovalCheckResult::Bad(
-				ApprovalCheckError::InvalidValidatorIndex(approval.validator),
-			)),
-		};
-
-		gum::trace!(
-			target: LOG_TARGET,
-			"Received approval for num_candidates {:}",
-			approval.candidate_indices.count_ones()
-		);
-
-		let candidate_hashes: Vec<CandidateHash> =
-			approved_candidates_info.iter().map(|candidate| candidate.1).collect();
-		// Signature check:
-		match DisputeStatement::Valid(
-			ValidDisputeStatementKind::ApprovalCheckingMultipleCandidates(candidate_hashes.clone()),
-		)
-		.check_signature(
-			&pubkey,
-			if let Some(candidate_hash) = candidate_hashes.first() {
-				*candidate_hash
-			} else {
-				respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::InvalidValidatorIndex(
-					approval.validator
-				),))
-			},
-			block_entry.session(),
-			&approval.signature,
-		) {
-			Err(_) => {
-				gum::error!(
-					target: LOG_TARGET,
-					"Error while checking signature {:}",
-					approval.candidate_indices.count_ones()
-				);
-				respond_early!(ApprovalCheckResult::Bad(ApprovalCheckError::InvalidSignature(
-					approval.validator
-				),))
-			},
-			Ok(()) => {},
-		};
-	}
+	gum::trace!(
+		target: LOG_TARGET,
+		"Received approval for num_candidates {:}",
+		approval.candidate_indices.count_ones()
+	);
 
 	let mut actions = Vec::new();
 	for (approval_candidate_index, approved_candidate_hash) in approved_candidates_info {
@@ -2898,9 +2830,7 @@ where
 	}
 
 	// importing the approval can be heavy as it may trigger acceptance for a series of blocks.
-	let t = with_response(ApprovalCheckResult::Accepted);
-
-	Ok((actions, t))
+	Ok((actions, ApprovalCheckResult::Accepted))
 }
 
 #[derive(Debug)]