PVF: Add test instructions

[mrcnski]

I was mentoring someone and figured I could copy/paste this info into a doc. This is especially needed now that we have multiple contributors. :) cc @jpserrat @eagr

[eagr]

Two things about pvf have been a bit confusing for me.

The *-intf.rs file names. I guess the "intf" part is shorthand for "interface"?

The terminology precheck / prepare / pre-validate. I thought they refer to different process, as they are different words. But the more I read the code and docs, the more confusing I got. Sometimes, they seem to refer the same thing, sometimes not. Not sure any more 😅.

[mrcnski]

Thanks @eagr, that is valuable feedback!

I've previously documented the terms we use. I added some links to the existing docs that are hopefully helpful - let me know.

Yeah, intf is short for "interface". These modules are in charge of interfacing with the workers. To me the naming makes sense, but I can see it being confusing. Any suggestions for improving it? 🤔

[eagr]

At first, I thought it defines the interfaces between components, that is, traits. Coz it's a common practice in languages with the interface feature. But they are not. So not only the name intf makes people wonder, even when you guess it right, it may give you a false assumption, which is not great.

I guess you don't want to just use worker.rs to avoid giving the assumption that it is a worker implementation, right? Maybe "work", in the sense of "work the worker". For comparison,

• worker-intf.rs
• worker-interface.rs
• worker.rs
• work.rs

[s0me0ne-unkn0wn]

I've previously documented the terms we use. I added some links to the existing docs that are hopefully helpful - let me know.

The doc is a bit ambiguous, though. (Not surprised, our docs are far from perfect).

PVF Prechecking: This is the process of initially checking the PVF when it is first added.

One could read it as "the very first time the PVF appears on chain, that is, during parachain onboarding only" (because the term "first added" is not defined and thus is ambiguous; known relevant processes are onboarding, upgrading, acceptance, rejection, encation, but not "addition"). It is not clear that the process takes place every time a parachain upgrades its STF.

As there is no prevalidation right now, preparation just consists of compilation.

Technically, that's not true. There is a prevalidate() function that gets called before preparation and that currently instantiates the RuntimeBlob object deserializing raw &[u8] code into parity_wasm::Module structure. During the deserialization, a lot of checks are made, including section ordering, function/code section consistency, etc. So, there is still a prevalidation phase, although it's not perfect and not complete.

[mrcnski]

I guess you don't want to just use worker.rs to avoid giving the assumption that it is a worker implementation, right? Maybe "work", in the sense of "work the worker". For comparison,

Hmm, good suggestions, but I still see potential for confusion. People may see worker.rs or work.rs and think that code belongs to the workers. Maybe worker-spawn is better?

[mrcnski]

The doc is a bit ambiguous, though. (Not surprised, our docs are far from perfect).

You can always raise an issue or PR to improve them! The docs were even more unclear when I joined, so I tried to improve them based on my notes, but I didn't have a great understanding back then. Thanks for the feedback, I'll make those changes here.

There is a prevalidate() function that gets called before preparation and that currently instantiates the RuntimeBlob object deserializing raw &[u8] code into parity_wasm::Module structure.

I see, in that case the comment in prevalidate is misleading (it literally says "this function" followed by "do nothing for now"). I'll revise it.

[eagr]

Maybe worker-spawn is better?

If it were up to me, I'd probably just use worker.rs in the sense of related to worker and call it a day. Name is hard. :)

[mrcnski]

It shouldn't be worker.rs because it's not worker code, but code that interfaces with workers. worker-interface.rs seems like the best alternative. There is maybe a potential for confusion with the concept of interfaces/traits, but it's still an improvement.

[alexggh]

Nit: Wouldn't this steps be better at the top of this repo ? And if we already have them can we just reference them, since they might change.

[mrcnski]

Yeah, there is a polkadot/doc/testing.md file. Looks much of this file can go there, and then this file can just reference that like you said.

[mrcnski]

This testing.md doc is pretty old and needs some love. I've never heard anyone mention Gurke or simnet, can that be removed? @sandreim

[mrcnski]

I removed this feature-ask because it should be in an issue, not in user-facing documentation.

[mrcnski]

Since this has been here for like three years, if it was an issue it would have been lost in the monorepo migration. So, I won't create it. :)

[mrcnski]

I removed this stuff because I couldn't find any reference to AllSubsystems in the codebase.

[mrcnski]

• testing.md rendered
• pvf/readme.md rendered

[mrcnski]

I raised a follow-up for full resuscitation of testing.md. (I'm pretty sure the old stuff can just be deleted, but it seemed best to leave it out of scope of this PR).
#2527