PVF: include polkadot's version into the artifact path

[mrcnski]

ISSUE

Overview

Compilation artifact filenames should contain the polkadot version. This way if there is a node upgrade, an old version can't override an artifact for the new node.

[mrcnski]

We may also want to include a hash per run; see here:

paritytech/polkadot#6861 (comment)

[mrcnski]

We may also want to include a hash per run; see here:

#6861 (comment)

Note this is only if we don't want artifacts to persist across restarts. But I think most of the issues with reusing artifacts across restarts would be addressed by this issue and by #677. See #685

[eagr]

I presume no one is working on this right? @s0me0ne-unkn0wn @mrcnski

I dug around the code a bit. On node startup, all existing artifacts will be nuked. Then a host will spin up a worker to compile the PVF and it writes to a tmp file on completion on success. Later, the host will be notified and rename the tmp file to the dest. But the artifact is only executed if it is marked Prepared. Please correct me if I got it wrong.

I could imagine there will be some corner cases, like a host restarting during PVF compilation and purging the caches before compilation is completed. But even that wouldn’t be a problem if the host doesn’t deem the artifact prepared, which doesn’t seem to be the case.

Please help me understand why are these needed. Like in what scenario this could otherwise be a problem? Or is this some kind of precaution?

[s0me0ne-unkn0wn]

@eagr, the initial idea comes from an incident we had some time ago. Worker binaries were not separate then and spawned from the main polkadot binary. There was a validator whose operator upgraded his node in the wrong way. He overwrote the polkadot binary with a new version and didn't restart the node. There was an artifact in cache generated by the old node version. The new node came with a new version of Wasmtime that introduced some breaking changes, and that new version of Wasmtime wasn't able to execute artifacts produced by older versions. So, when it came to the execution, the node spawned an execution worker from the new polkadot binary, Wasmtime refused to execute the old artifact, validation failed, and the validator started a dispute. As the dispute obviously concluded invalid, the validator might be slashed.

That's some pre-history of why we wanted to include the node version into the artifact path initially. The workers have been split out since then, and we have a version control between node and workers, so this measure does not make as much sense for that very problem now. But now there're discussions on preserving artifacts between runs. Preparation is a very resource-heavy process, and in the light of on-demand parachains upcoming, it would be good to cut on resource usage, so if artifacts are reusable, why not reuse them? But we must ensure they're really reusable, that is, produced with a proper node version.

Hope it helps, and maybe @mrcnski might add something, I'm not sure I remember all the background and reasoning behind this issue.

[eagr]

But now there're discussions on preserving artifacts between runs.

With that change, these additions definitely make sense now. Thanks for the elaboration! @s0me0ne-unkn0wn

[mrcnski]

Good question @eagr and great answer @s0me0ne-unkn0wn! Thanks!