Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 38 additions & 30 deletions runtime/parachains/src/inclusion/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,11 +524,20 @@ 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) => {
// 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;
Expand All @@ -545,32 +554,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(),
Expand Down Expand Up @@ -952,6 +935,9 @@ 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 }
Expand All @@ -967,10 +953,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!(
Expand Down Expand Up @@ -1014,7 +1022,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
Expand Down
3 changes: 2 additions & 1 deletion runtime/parachains/src/paras_inherent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, _>(
Expand All @@ -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)
Copy link
Copy Markdown
Contributor

@drahnr drahnr Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct, we'd want to filter out all of them, not only the outer error. Missing a para chain header is also a good enough reason to drop the candidate iiuc

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time connecting your comment to the code here. Can you explain in more detail?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now only considering the outer error of the nested result, I'd argue that an error of the inner result type is also a good enough reason to discard the backed candidate iiuc. This can be deferred to a follow up PR, since as you correctly stated, behavior does not change compared to current master

Copy link
Copy Markdown
Contributor

@rphmeier rphmeier Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that an error of the inner result type is also a good enough reason to discard the backed candidate iiuc

Ok, fair. We should probably discard all backed candidates when encountering this error. It means something is seriously wrong and governance will probably need to step in. The on-chain logic is to ignore all the candidates anyway for the same reasons.

Copy link
Copy Markdown
Member

@eskimor eskimor Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can change that in the next release/new PR.

.is_err()
},
&scheduled[..],
Expand Down