paras: Add runtime events for PVF pre-checking #4683
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
pepyakin
added
B7-runtimenoteworthy
C1-low
| @@ -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), | |||
There was a problem hiding this comment.
FYI: We started now slowly migrating to named event arguments. Which doesn't require these comments on what is what .
There was a problem hiding this comment.
Thanks for the heads up!
runtime/parachains/src/paras/mod.rs
Outdated
| weight += T::DbWeight::get().reads_writes(3, 2); | ||
| Self::deposit_event(Event::PvfCheckConcluded(*code_hash, cause.para_id(), true)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.:
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.
There was a problem hiding this comment.
Yeah, I also don't wanted you to fix this as part of this pr.
runtime/parachains/src/paras/mod.rs
Outdated
| 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), |
There was a problem hiding this comment.
IMO we could also just split this up into two variants.
There was a problem hiding this comment.
What's your reasoning here?
There was a problem hiding this comment.
You directly see the output based on the event name.
There was a problem hiding this comment.
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
force-pushed
the
pep-pvf-runtime-events
branch
from
9537c19
to
bec1cc0
Compare
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.
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:
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
PvfCheckStartedwill betriggered. If another parachain triggers a pre-checking process for the
validation code which is already being pre-checked, another
PvfCheckStartedevent will be triggered with the corresponding paraid.
When the PVF pre-checking voting for a PVF was finished, several
PvfCheckAccepted/Rejectedevents will be triggered: one for each para id thatwas 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
PvfCheckStartedwill be followed by thePvfCheckAcceptedwith thesame 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.