Refactor check_validation_outputs (#4727)

* Move PersistedValidationData check into * Address feedback * Remove incorrect comment * Update runtime/parachains/src/inclusion/mod.rs * fmt * Add logging Co-authored-by: Robert Habermeier <rphmeier@gmail.com> Co-authored-by: Andronik <write@reusable.software>
paritytech · Jan 28, 2022 · e3985d4 · e3985d4
1 parent f0041c4
commit e3985d4
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 31 deletions.
diff --git a/runtime/parachains/src/inclusion/mod.rs b/runtime/parachains/src/inclusion/mod.rs
@@ -524,11 +524,26 @@ impl<T: Config> Pallet<T> {
 				candidates.iter().enumerate()
 			{
 				if let FullCheck::Yes = full_check {
-					check_ctx.verify_backed_candidate(
+					match check_ctx.verify_backed_candidate(
 						parent_hash,
+						parent_storage_root,
 						candidate_idx,
 						backed_candidate,
-					)?;
+					)? {
+						Err(FailedToCreatePVD) => {
+							log::debug!(
+								target: LOG_TARGET,
+								"Failed to create PVD for candidate {} on relay parent {:?}",
+								candidate_idx,
+								parent_hash,
+							);
+							// We don't want to error out here because it will
+							// brick the relay-chain. So we return early without
+							// doing anything.
+							return Ok(ProcessedCandidates::default())
+						},
+						Ok(rpn) => rpn,
+					}
 				}
 
 				let para_id = backed_candidate.descriptor().para_id;
@@ -545,32 +560,6 @@ impl<T: Config> Pallet<T> {
 							);
 						}
 
-						{
-							// this should never fail because the para is registered
-							let persisted_validation_data =
-								match crate::util::make_persisted_validation_data::<T>(
-									para_id,
-									relay_parent_number,
-									parent_storage_root,
-								) {
-									Some(l) => l,
-									None => {
-										// We don't want to error out here because it will
-										// brick the relay-chain. So we return early without
-										// doing anything.
-										return Ok(ProcessedCandidates::default())
-									},
-								};
-
-							let expected = persisted_validation_data.hash();
-
-							ensure!(
-								expected ==
-									backed_candidate.descriptor().persisted_validation_data_hash,
-								Error::<T>::ValidationDataHashMismatch,
-							);
-						}
-
 						ensure!(
 							<PendingAvailability<T>>::get(&para_id).is_none() &&
 								<PendingAvailabilityCommitments<T>>::get(&para_id).is_none(),
@@ -952,6 +941,10 @@ pub(crate) struct CandidateCheckContext<T: Config> {
 	relay_parent_number: T::BlockNumber,
 }
 
+/// An error indicating that creating Persisted Validation Data failed
+/// while checking a candidate's validity.
+pub(crate) struct FailedToCreatePVD;
+
 impl<T: Config> CandidateCheckContext<T> {
 	pub(crate) fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self {
 		Self { config: <configuration::Pallet<T>>::config(), now, relay_parent_number }
@@ -967,10 +960,32 @@ impl<T: Config> CandidateCheckContext<T> {
 	pub(crate) fn verify_backed_candidate(
 		&self,
 		parent_hash: <T as frame_system::Config>::Hash,
+		parent_storage_root: T::Hash,
 		candidate_idx: usize,
 		backed_candidate: &BackedCandidate<<T as frame_system::Config>::Hash>,
-	) -> Result<(), Error<T>> {
+	) -> Result<Result<(), FailedToCreatePVD>, Error<T>> {
 		let para_id = backed_candidate.descriptor().para_id;
+		let now = <frame_system::Pallet<T>>::block_number();
+		let relay_parent_number = now - One::one();
+
+		{
+			// this should never fail because the para is registered
+			let persisted_validation_data = match crate::util::make_persisted_validation_data::<T>(
+				para_id,
+				relay_parent_number,
+				parent_storage_root,
+			) {
+				Some(l) => l,
+				None => return Ok(Err(FailedToCreatePVD)),
+			};
+
+			let expected = persisted_validation_data.hash();
+
+			ensure!(
+				expected == backed_candidate.descriptor().persisted_validation_data_hash,
+				Error::<T>::ValidationDataHashMismatch,
+			);
+		}
 
 		// we require that the candidate is in the context of the parent block.
 		ensure!(
@@ -1014,7 +1029,7 @@ impl<T: Config> CandidateCheckContext<T> {
 			);
 			Err(err.strip_into_dispatch_err::<T>())?;
 		};
-		Ok(())
+		Ok(Ok(()))
 	}
 
 	/// Check the given outputs after candidate validation on whether it passes the acceptance

diff --git a/runtime/parachains/src/paras_inherent/mod.rs b/runtime/parachains/src/paras_inherent/mod.rs
@@ -710,6 +710,7 @@ impl<T: Config> Pallet<T> {
 			let scheduled = <scheduler::Pallet<T>>::scheduled();
 
 			let relay_parent_number = now - One::one();
+			let parent_storage_root = parent_header.state_root().clone();
 
 			let check_ctx = CandidateCheckContext::<T>::new(now, relay_parent_number);
 			let backed_candidates = sanitize_backed_candidates::<T, _>(
@@ -725,7 +726,7 @@ impl<T: Config> Pallet<T> {
 							// That way we avoid possible duplicate checks while assuring all
 							// backed candidates fine to pass on.
 							check_ctx
-								.verify_backed_candidate(parent_hash, candidate_idx, backed_candidate)
+								.verify_backed_candidate(parent_hash, parent_storage_root, candidate_idx, backed_candidate)
 								.is_err()
 				},
 				&scheduled[..],