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

paras: Add runtime events for PVF pre-checking #4683

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Jan 10, 2022

In this PR, paras module emit runtime events on certain PVF pre-checking
related conditions.

Specifically, there are 3 new events in the paras module:

  1. PvfCheckStarted
  2. PvfCheckAccepted
  3. PvfCheckRejected

All of those have identifiers for the parachain that triggered the PVF
pre-checking and the validation code that goes through the pre-checking.

The mechanics of those are as follows. Each time a new PVF is added, be
it due to onboarding or upgrading, the PvfCheckStarted will be
triggered. If another parachain triggers a pre-checking process for the
validation code which is already being pre-checked, another
PvfCheckStarted event will be triggered with the corresponding para
id.

When the PVF pre-checking voting for a PVF was finished, several
PvfCheckAccepted/Rejected events will be triggered: one for each para id that
was subscribed to this check (i.e. was a "cause" for it).

If the PVF pre-checking is disabled, then one can still expect these
events to be fired. Since insta PVF approval is syncronous, the
PvfCheckStarted will be followed by the PvfCheckAccepted with the
same validation code and para id.

If somebody is interested in following validation code changes for a PVF
of a parachain, they would need to subscribe to those events. I did not
supply the topics for the events, since I am not sure if that's needed
or will be used, but they can be added later if needed.

@pepyakin
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 10, 2022
@pepyakin pepyakin added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Jan 10, 2022
@@ -467,6 +477,12 @@ pub mod pallet {
NewHeadNoted(ParaId),
/// A para has been queued to execute pending actions. `para_id`
ActionQueued(ParaId, SessionIndex),
/// The given para either initiated or subscribed to a PVF check for the given validation
/// code. `code_hash` `para_id`
PvfCheckStarted(ValidationCodeHash, ParaId),
Copy link
Member

Choose a reason for hiding this comment

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

FYI: We started now slowly migrating to named event arguments. Which doesn't require these comments on what is what .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up!

Comment on lines 1364 to 1365
weight += T::DbWeight::get().reads_writes(3, 2);
Self::deposit_event(Event::PvfCheckConcluded(*code_hash, cause.para_id(), true));
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should be fixed in Substrate? So that deposit_event returns the weight instead of you are required to calculate this here manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know. I did not want to open up this can of worms though. There are plenty examples of this, even in the polkadot repo, e.g.:

Self::deposit_event(Event::<T>::CandidateIncluded(

And it is not limited to deposit_event though. One of the worst cases IMO is the Currency trait where you can't even assume what the concrete implementation would do.

IMO this problem should be addressed as a whole, holistically.

Anyway, I do not think that problem is within the scope of this particular PR (unless, you propose to remove the weight accounting for this case to make it inline with other cases found elsewhere). I see a solution should be first discussed in an issue in Substrate (cc @shawntabrizi), not in this thread. As soon as we have a solution though, I'd be down to migrate this code (and the rest of polkadot's for that matter) to the new mechanism.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I also don't wanted you to fix this as part of this pr.

PvfCheckStarted(ValidationCodeHash, ParaId),
/// The PVF pre-checking for the given validation code was concluded with the given outcome
/// (either accepted or rejected). `code_hash` `para_id` `accepted`
PvfCheckConcluded(ValidationCodeHash, ParaId, bool),
Copy link
Member

Choose a reason for hiding this comment

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

IMO we could also just split this up into two variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your reasoning here?

Copy link
Member

Choose a reason for hiding this comment

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

You directly see the output based on the event name.

Copy link
Contributor Author

@pepyakin pepyakin Jan 12, 2022

Choose a reason for hiding this comment

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

Ok, don't feel about this strongly. Here is the fix. Commit message and PR description also fixed.

In this PR, paras module emit runtime events on certain PVF pre-checking
related conditions.

Specifically, there are 3 new events in the paras module:

1. PvfCheckStarted
2. PvfCheckAccepted
3. PvfCheckRejected

All of those have identifiers for the parachain that triggered the PVF
pre-checking and the validation code that goes through the pre-checking.

The mechanics of those are as follows. Each time a new PVF is added, be
it due to onboarding or upgrading, the `PvfCheckStarted` will be
triggered. If another parachain triggers a pre-checking process for the
validation code which is already being pre-checked, another
`PvfCheckStarted` event will be triggered with the corresponding para
id.

When the PVF pre-checking voting for a PVF was finished, several
`PvfCheckAccepted/Rejected` events will be triggered: one for each para id that
was subscribed to this check (i.e. was a "cause" for it).

If the PVF pre-checking is disabled, then one can still expect these
events to be fired. Since insta PVF approval is syncronous, the
`PvfCheckStarted` will be followed by the `PvfCheckAccepted` with the
same validation code and para id.

If somebody is interested in following validation code changes for a PVF
of a parachain, they would need to subscribe to those events. I did not
supply the topics for the events, since I am not sure if that's needed
or will be used, but they can be added later if needed.
@pepyakin pepyakin merged commit bbaf400 into master Jan 13, 2022
@pepyakin pepyakin deleted the pep-pvf-runtime-events branch January 13, 2022 17:44
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
In this PR, paras module emit runtime events on certain PVF pre-checking
related conditions.

Specifically, there are 3 new events in the paras module:

1. PvfCheckStarted
2. PvfCheckAccepted
3. PvfCheckRejected

All of those have identifiers for the parachain that triggered the PVF
pre-checking and the validation code that goes through the pre-checking.

The mechanics of those are as follows. Each time a new PVF is added, be
it due to onboarding or upgrading, the `PvfCheckStarted` will be
triggered. If another parachain triggers a pre-checking process for the
validation code which is already being pre-checked, another
`PvfCheckStarted` event will be triggered with the corresponding para
id.

When the PVF pre-checking voting for a PVF was finished, several
`PvfCheckAccepted/Rejected` events will be triggered: one for each para id that
was subscribed to this check (i.e. was a "cause" for it).

If the PVF pre-checking is disabled, then one can still expect these
events to be fired. Since insta PVF approval is syncronous, the
`PvfCheckStarted` will be followed by the `PvfCheckAccepted` with the
same validation code and para id.

If somebody is interested in following validation code changes for a PVF
of a parachain, they would need to subscribe to those events. I did not
supply the topics for the events, since I am not sure if that's needed
or will be used, but they can be added later if needed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants