Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Refactor check_validation_outputs #4727

Merged
merged 9 commits into from
Jan 28, 2022
Merged

Refactor check_validation_outputs #4727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d247b75
Move PersistedValidationData check into
Lldenaurois Jan 16, 2022
de8235d
Address feedback
Lldenaurois Jan 16, 2022
6371754
Remove incorrect comment
Lldenaurois Jan 17, 2022
9c7b108
Merge remote-tracking branch 'parity/master' into refactor_check_vali…
Lldenaurois Jan 17, 2022
cfd1223
Merge remote-tracking branch 'parity/master' into refactor_check_vali…
Lldenaurois Jan 21, 2022
765461d
Update runtime/parachains/src/inclusion/mod.rs
rphmeier Jan 21, 2022
675dde3
Merge branch 'master' into refactor_check_validation_outputs
ordian Jan 27, 2022
6158f51
fmt
ordian Jan 27, 2022
b798f91
Add logging
Lldenaurois Jan 27, 2022
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
75 changes: 45 additions & 30 deletions runtime/parachains/src/inclusion/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
eskimor marked this conversation as resolved.
Show resolved Hide resolved
},
Ok(rpn) => rpn,
}
}

let para_id = backed_candidate.descriptor().para_id;
Expand All @@ -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(),
Expand Down Expand Up @@ -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;
rphmeier marked this conversation as resolved.
Show resolved Hide resolved

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 +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!(
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion runtime/parachains/src/paras_inherent/mod.rs
View file
Open in desktop
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
Contributor

@drahnr drahnr Jan 26, 2022 •
edited
Loading

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
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
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
Contributor

@rphmeier rphmeier Jan 28, 2022 •
edited
Loading

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
Member

@eskimor eskimor Jan 28, 2022 •
edited
Loading

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