Move crypto checks in the approval-distribution

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>

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(

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

@@ -581,9 +581,14 @@ pub(crate) async fn handle_new_head<
 			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,
+			session_info: session_info.clone(),
+			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";
 
@@ -1878,14 +1877,39 @@ async fn distribution_messages_for_activation<Sender: SubsystemSender<RuntimeApi
 		distribution_message_span.add_string_tag("block-hash", &block_hash.to_string());
 		distribution_message_span
 			.add_string_tag("parent-hash", &block_entry.parent_hash().to_string());
+		let ExtendedSessionInfo { ref session_info, .. } = match get_extended_session_info(
+			session_info_provider,
+			sender,
+			block_entry.block_hash(),
+			block_entry.session(),
+		)
+		.await
+		{
+			Some(i) => i,
+			None => continue,
+		};
 		approval_meta.push(BlockApprovalMeta {
 			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(),
+			session_info: session_info.clone(),
 		});
+
 		let mut signatures_queued = HashSet::new();
 		for (core_index, candidate_hash) in block_entry.candidates() {
 			let _candidate_span =
@@ -2155,35 +2179,38 @@ async fn handle_from_overseer<
 			vec![Action::Conclude]
 		},
 		FromOrchestra::Communication { msg } => match msg {
-			ApprovalVotingMessage::CheckAndImportAssignment(a, claimed_cores, res) => {
+			ApprovalVotingMessage::CheckAndImportAssignment(a, claimed_cores, tranche) => {
 				let (check_outcome, actions) = check_and_import_assignment(
 					sender,
 					state,
 					db,
 					session_info_provider,
 					a,
 					claimed_cores,
+					tranche,
 				)
 				.await?;
-				let _ = res.send(check_outcome);
-
+				if let AssignmentCheckResult::Bad(err) = check_outcome {
+					gum::error!(target: LOG_TARGET, ?err, "Unexpected fail to check and import assignment");
+				}
 				actions
 			},
-			ApprovalVotingMessage::CheckAndImportApproval(a, res) =>
-				check_and_import_approval(
+			ApprovalVotingMessage::CheckAndImportApproval(a) => {
+				let result = check_and_import_approval(
 					sender,
 					state,
 					db,
 					session_info_provider,
 					metrics,
 					a,
-					|r| {
-						let _ = res.send(r);
-					},
 					&wakeups,
 				)
-				.await?
-				.0,
+				.await?;
+				if let ApprovalCheckResult::Bad(err) = result.1 {
+					gum::error!(target: LOG_TARGET, ?err, "Unexpected fail to check and import approval");
+				}
+				result.0
+			},
 			ApprovalVotingMessage::ApprovedAncestor(target, lower_bound, res) => {
 				let mut approved_ancestor_span = state
 					.spans
@@ -2744,6 +2771,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>,
@@ -2877,42 +2905,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;
@@ -3002,23 +2994,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
@@ -3072,67 +3062,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 {
@@ -3196,9 +3130,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)]

polkadot/node/network/approval-distribution/Cargo.toml

@@ -17,6 +17,8 @@ polkadot-node-subsystem = { path = "../../subsystem" }
 polkadot-node-subsystem-util = { path = "../../subsystem-util" }
 polkadot-primitives = { path = "../../../primitives" }
 polkadot-node-jaeger = { path = "../../jaeger" }
+polkadot-node-core-approval-voting = { path = "../../core/approval-voting"}
+
 rand = "0.8"
 itertools = "0.11"