-
Notifications
You must be signed in to change notification settings - Fork 1.6k
PVF: Document that preparation cannot lead to disputes #6873
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! And it's really good to have someone on the team who cares about documentation updates :)
Sidenote: While reading this, one more corner case came to mind. What if a PVF is pre-checked in one session and gets enacted in another session? Executor parameters may change in between. There is not much we can do about that, but it's worth considering.
@s0me0ne-unkn0wn I thought about that also. If executor params can affect preparation, then it would invalidate the assumptions documented here. Probably we would have to do pre-checking again when changing any parameter that can affect the runtime. Is there already an issue for this? |
What executor params could be controlled? |
@burdges current list is like that:
Planned but not yet implemented:
|
We can do that, but what's next? We cannon un-enact a PVF. Consider the situation when it's the first and only PVF for a parachain just onboarded.
Just created paritytech/polkadot-sdk#694, let's continue there. |
I suppose Wasm stack limit is a stack used by Wasm code, while native stack limit is used by the Wasm engine, our host calls, etc., yes? Aside form the two existing and planned preperation limits then these all sound more determined by the block than by the runtime, so not a new problem and not sure why they'd require rebuilding. As for the two existing and planned preperation limits, are we instrumented to record the current max during preperation? I suppose no as code is rarely instrumented like this, but if so then we could just deactivate the PVF if the recorded limit gets invalidated on chain. Anyways our problem is a preperation limit change could make preparing an already valid PVF impossible, which gives an adversary some control over who can check the PVF. Importantly, we should not enact a limit change immediately, just like we do not enact a PVF change immediately. Assuming all validators do rebuild, then yeah we'd prepare to unenact the PVF but governance should've time and ability to revert the limit change. Isn't this enough? We could even have the vote block the limit change, so then governance should manually boot any fat parachains before reducing limits. Is that simpler? It's also kinda shitty that all validators must rebuild just to check a preperation limit violation. |
We could (not saying we should) don't accept execution environment parameter updates if pre-checking fails for a PVF that was valid before. So changing parameters would also be pre-checked, by re pre-checking all PVFs before enacting them. I actually think this is absolute overkill and does very likely more harm than good, but in that particular case we "could" do something. Realistically, I think we have to accept a small risk and live with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrcnski !
@burdges Just to answer your question, we do have instrumentation for preparation memory usage and time taken. Agree with the rest of your post. |
* master: (50 commits) Issue 4393: Correcting Unnecessary Use of Arc (#6882) Companion for #13287 (#6655) timestamp ci job logs (#6890) Release parachain host API v4 (#6885) polkadot companion: #13128 (Pools commission) (#6264) companion for #13555 (#6842) Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600) Bump bumpalo from 3.8.0 to 3.12.0 (#6599) Bump git2 from 0.16.0 to 0.16.1 (#6601) Council as SpendOrigin (#6877) PVF: Document that preparation cannot lead to disputes (#6873) Sync versions with current release (0.9.39) (#6875) Companion for paritytech/substrate#13592 (#6869) Update orchestra to the recent version (#6854) Delete unused Cargo.lock (#6870) Remove use of Store trait (#6835) Companion for Substrate #13564 (#6845) Adding Dispute Participation Metrics (#6838) Update `substrate` to 48e7cb1 (#6851) Move PVF timeouts to executor environment parameters (#6823) ...
…slashing-client * ao-past-session-slashing-runtime: (23 commits) Issue 4393: Correcting Unnecessary Use of Arc (#6882) Companion for #13287 (#6655) timestamp ci job logs (#6890) Release parachain host API v4 (#6885) polkadot companion: #13128 (Pools commission) (#6264) companion for #13555 (#6842) Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600) Bump bumpalo from 3.8.0 to 3.12.0 (#6599) Bump git2 from 0.16.0 to 0.16.1 (#6601) Council as SpendOrigin (#6877) PVF: Document that preparation cannot lead to disputes (#6873) Sync versions with current release (0.9.39) (#6875) Companion for paritytech/substrate#13592 (#6869) Update orchestra to the recent version (#6854) Delete unused Cargo.lock (#6870) Remove use of Store trait (#6835) Companion for Substrate #13564 (#6845) Adding Dispute Participation Metrics (#6838) Update `substrate` to 48e7cb1 (#6851) Move PVF timeouts to executor environment parameters (#6823) ...
PULL REQUEST
Overview
Since the changes in #6103, preparation should not be able to result in disputes.