From 636b7f09ab450a41b9f9f1de5f3765361abddc17 Mon Sep 17 00:00:00 2001 From: Lldenaurois Date: Sun, 16 Jan 2022 22:39:33 +0100 Subject: [PATCH 1/6] Move PersistedValidationData check into --- runtime/parachains/src/inclusion/mod.rs | 54 ++++++++++---------- runtime/parachains/src/paras_inherent/mod.rs | 3 +- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/runtime/parachains/src/inclusion/mod.rs b/runtime/parachains/src/inclusion/mod.rs index c2baa94b9335..d6f9088b5d03 100644 --- a/runtime/parachains/src/inclusion/mod.rs +++ b/runtime/parachains/src/inclusion/mod.rs @@ -526,6 +526,7 @@ impl Pallet { if let FullCheck::Yes = full_check { check_ctx.verify_backed_candidate( parent_hash, + parent_storage_root, candidate_idx, backed_candidate, )?; @@ -545,32 +546,6 @@ impl Pallet { ); } - { - // this should never fail because the para is registered - let persisted_validation_data = - match crate::util::make_persisted_validation_data::( - 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::::ValidationDataHashMismatch, - ); - } - ensure!( >::get(¶_id).is_none() && >::get(¶_id).is_none(), @@ -967,10 +942,37 @@ impl CandidateCheckContext { pub(crate) fn verify_backed_candidate( &self, parent_hash: ::Hash, + parent_storage_root: T::Hash, candidate_idx: usize, backed_candidate: &BackedCandidate<::Hash>, ) -> Result<(), Error> { let para_id = backed_candidate.descriptor().para_id; + let now = >::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::( + 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(()) + }, + }; + + let expected = persisted_validation_data.hash(); + + ensure!( + expected == backed_candidate.descriptor().persisted_validation_data_hash, + Error::::ValidationDataHashMismatch, + ); + } // we require that the candidate is in the context of the parent block. ensure!( diff --git a/runtime/parachains/src/paras_inherent/mod.rs b/runtime/parachains/src/paras_inherent/mod.rs index 1645f74ee804..74d58fe0feea 100644 --- a/runtime/parachains/src/paras_inherent/mod.rs +++ b/runtime/parachains/src/paras_inherent/mod.rs @@ -710,6 +710,7 @@ impl Pallet { let scheduled = >::scheduled(); let relay_parent_number = now - One::one(); + let parent_storage_root = parent_header.state_root().clone(); let check_ctx = CandidateCheckContext::::new(now, relay_parent_number); let backed_candidates = sanitize_backed_candidates::( @@ -725,7 +726,7 @@ impl Pallet { // 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[..], From 97a58cf5022acf4e07a9e09805ed37a41c40d303 Mon Sep 17 00:00:00 2001 From: Lldenaurois Date: Sun, 16 Jan 2022 23:20:00 +0100 Subject: [PATCH 2/6] Address feedback --- runtime/parachains/src/inclusion/mod.rs | 24 ++++++++++++-------- runtime/parachains/src/paras_inherent/mod.rs | 24 ++++++++++++++------ 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/runtime/parachains/src/inclusion/mod.rs b/runtime/parachains/src/inclusion/mod.rs index d6f9088b5d03..20412ccad17b 100644 --- a/runtime/parachains/src/inclusion/mod.rs +++ b/runtime/parachains/src/inclusion/mod.rs @@ -524,12 +524,20 @@ impl Pallet { 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) => { + // 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; @@ -927,6 +935,7 @@ pub(crate) struct CandidateCheckContext { relay_parent_number: T::BlockNumber, } +pub(crate) struct FailedToCreatePVD; impl CandidateCheckContext { pub(crate) fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self { Self { config: >::config(), now, relay_parent_number } @@ -945,7 +954,7 @@ impl CandidateCheckContext { parent_storage_root: T::Hash, candidate_idx: usize, backed_candidate: &BackedCandidate<::Hash>, - ) -> Result<(), Error> { + ) -> Result, Error> { let para_id = backed_candidate.descriptor().para_id; let now = >::block_number(); let relay_parent_number = now - One::one(); @@ -958,12 +967,7 @@ impl CandidateCheckContext { 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(()) - }, + None => return Ok(Err(FailedToCreatePVD)), }; let expected = persisted_validation_data.hash(); @@ -1016,7 +1020,7 @@ impl CandidateCheckContext { ); Err(err.strip_into_dispatch_err::())?; }; - 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 index 74d58fe0feea..920fd7914597 100644 --- a/runtime/parachains/src/paras_inherent/mod.rs +++ b/runtime/parachains/src/paras_inherent/mod.rs @@ -721,13 +721,23 @@ impl Pallet { -> bool { // never include a concluded-invalid candidate concluded_invalid_disputes.contains(&backed_candidate.hash()) || - // Instead of checking the candidates with code upgrades twice - // move the checking up here and skip it in the training wheels fallback. - // That way we avoid possible duplicate checks while assuring all - // backed candidates fine to pass on. - check_ctx - .verify_backed_candidate(parent_hash, parent_storage_root, candidate_idx, backed_candidate) - .is_err() + // Instead of checking the candidates with code upgrades twice + // move the checking up here and skip it in the training wheels fallback. + // That way we avoid possible duplicate checks while assuring all + // backed candidates fine to pass on. + // + // NOTE: this is the only place where we check the relay-parent. + { + match check_ctx.verify_backed_candidate( + parent_hash, + parent_storage_root, + candidate_idx, + backed_candidate, + ) { + Err(_) | Ok(Err(_)) => true, + Ok(Ok(_)) => false, + } + } }, &scheduled[..], ); From 46118d924f3c49e7760fdf75d449d0a66425450f Mon Sep 17 00:00:00 2001 From: Lldenaurois Date: Mon, 17 Jan 2022 22:20:45 +0100 Subject: [PATCH 3/6] Remove incorrect comment --- runtime/parachains/src/paras_inherent/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/runtime/parachains/src/paras_inherent/mod.rs b/runtime/parachains/src/paras_inherent/mod.rs index 920fd7914597..7b41ec88dc0e 100644 --- a/runtime/parachains/src/paras_inherent/mod.rs +++ b/runtime/parachains/src/paras_inherent/mod.rs @@ -725,8 +725,6 @@ impl Pallet { // move the checking up here and skip it in the training wheels fallback. // That way we avoid possible duplicate checks while assuring all // backed candidates fine to pass on. - // - // NOTE: this is the only place where we check the relay-parent. { match check_ctx.verify_backed_candidate( parent_hash, From 1a1ed335e39958950da1566c40a9b2b40a263b68 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 21 Jan 2022 17:44:36 -0600 Subject: [PATCH 4/6] Update runtime/parachains/src/inclusion/mod.rs --- runtime/parachains/src/inclusion/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/runtime/parachains/src/inclusion/mod.rs b/runtime/parachains/src/inclusion/mod.rs index 20412ccad17b..449a55c4b666 100644 --- a/runtime/parachains/src/inclusion/mod.rs +++ b/runtime/parachains/src/inclusion/mod.rs @@ -935,6 +935,8 @@ pub(crate) struct CandidateCheckContext { 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 CandidateCheckContext { pub(crate) fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self { From 44a2c5dbe22b9d028fe22767b023e21819c9b2b8 Mon Sep 17 00:00:00 2001 From: Andronik Date: Thu, 27 Jan 2022 19:47:23 +0100 Subject: [PATCH 5/6] fmt --- runtime/parachains/src/inclusion/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/inclusion/mod.rs b/runtime/parachains/src/inclusion/mod.rs index 449a55c4b666..61afd14afcca 100644 --- a/runtime/parachains/src/inclusion/mod.rs +++ b/runtime/parachains/src/inclusion/mod.rs @@ -935,7 +935,7 @@ pub(crate) struct CandidateCheckContext { relay_parent_number: T::BlockNumber, } -/// An error indicating that creating Persisted Validation Data failed +/// An error indicating that creating Persisted Validation Data failed /// while checking a candidate's validity. pub(crate) struct FailedToCreatePVD; impl CandidateCheckContext { From f5f6a90decc8902ac38c15bfaaf684636c0c4e65 Mon Sep 17 00:00:00 2001 From: Lldenaurois Date: Thu, 27 Jan 2022 20:10:12 +0100 Subject: [PATCH 6/6] Add logging --- runtime/parachains/src/inclusion/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/runtime/parachains/src/inclusion/mod.rs b/runtime/parachains/src/inclusion/mod.rs index 61afd14afcca..41d06d2272be 100644 --- a/runtime/parachains/src/inclusion/mod.rs +++ b/runtime/parachains/src/inclusion/mod.rs @@ -531,6 +531,12 @@ impl Pallet { 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. @@ -938,6 +944,7 @@ pub(crate) struct CandidateCheckContext { /// An error indicating that creating Persisted Validation Data failed /// while checking a candidate's validity. pub(crate) struct FailedToCreatePVD; + impl CandidateCheckContext { pub(crate) fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self { Self { config: >::config(), now, relay_parent_number }