PVF: Incorporate wasmtime version in worker version checks

[mrcnski]

Hardens the worker version check.

Let me know if anything is not clear.

Related

Closes #2398

[mrcnski]

I just removed this because the explanation is only ever shown when -- --nocapture is passed in, and even then it does not explain a rebuild most of the time. This code would have needed to be updated, and I didn't feel like updating un-useful code.

[s0me0ne-unkn0wn]

Looks good to me!

Two concerns:

1. Do we still need it at all if we're falling back to pruning all the artifacts?
2. Does the new artifact name always fit into MAX_PATH?

[mrcnski]

Thanks for the review!

Do we still need it at all if we're falling back to pruning all the artifacts?

Yes, actually this PR is still useful to make sure that we're running a version of the workers with the intended wasmtime version. We don't enforce constant rebuilds of the workers during development as it would be bad DevEx, but we should at least make sure that the wasmtime version between polkadot, prepare-worker, and execute-worker is always consistent, erroring if not. I don't think we have the same concerns here as in the other PR, namely CPU features, because I believe wasmtime/cranelift detect those at runtime.

Does the new artifact name always fit into MAX_PATH?

This PR doesn't change the artifact naming scheme (except to make sure that the wasmtime version portion is correct), but that will anyway be superseded by #2895. I did some reading on MAX_PATH anyway which was quite interesting 😛 Seems like this value is very large on any reasonable, modern Linux, so not a practical concern there, though it could be on Mac.

In any case, what would happen is, we get an error after preparation when renaming the temp compiled artifact to the final one. This would not result in a dispute, so I don't think we need to specifically handle this (which based on reading the above and here, seems full of pitfalls: "Pathnames are very evil, insecure and path_max is a lie and not even a constant (it might be different on different OS functions).").

[mrcnski]

To finish this we'd need to add back the wasmtime version which was removed (here). However, we should do it a different way than calling cargo tree, because it was resulting in errors at build time (possibly due to contention over the crates.io index).

[bkchr]

We don't enforce constant rebuilds of the workers during development as it would be bad DevEx, but we should at least make sure that the wasmtime version between polkadot, prepare-worker, and execute-worker is always consistent, erroring if not

Is this really a common issue that has happened once? We also don't bump wasmtime every week or so. So, not really sure we need this at all.

[mrcnski]

@bkchr Perhaps you're right. I thought this was a possible vector for disputes - but maybe fixing this is not worth the added complexity.

I can close if we don't want this, we can always re-open if it ever becomes an issue.

[bkchr]

On production we are guarded by the worker version check and that we delete the cache now on startup. So, we should not be able to run into any kind of problem with this?

[mrcnski]

@bkchr Since #1495 the worker version check only accounts for the node version. My full concern from the related issue is:

For example, say someone downloads the node and workers at a release version, and then switches to master for some bug fixes. He could be running an updated node without updating the workers, but the node version hasn't changed so the version check passes. But wasmtime may have been bumped since then.

Seems like a viable concern to me (I've talked to a validator who was running on master), but let me what you think.

[bkchr]

In general, wouldn't it fail on loading the wasm file from the disk if the wasmtime version changed? I mean this shouldn't be a reason for a dispute?

[s0me0ne-unkn0wn]

In general, wouldn't it fail on loading the wasm file from the disk if the wasmtime version changed? I mean this shouldn't be a reason for a dispute?

Currently, it does not hold AFAICT. execute_artifact is still returning Result<Vec<u8>, String>, so it does not distinguish between error types, whether it be runtime construction, instantiation, or anything else, it will report invalid candidate in any case, and that will result in a dispute. We mitigated that in #2895, but the described "screwed up node upgrade" scenario still can trigger that, as it was in March 2023 on Polkadot.

There was an intent in #661 to solve that in a more straightforward way, but it has drowned in discussions, although, TBH, I think that way is superior to constantly increasing the number of checks over artifacts, workers, node version, and everything. Also, I'm not sure if we should put a lot of effort into it right now, considering the transition to PolkaVM.

[bkchr]

There was an intent in #661 to solve that in a more straightforward way, but it has drowned in discussions, although, TBH, I think that way is superior to constantly increasing the number of checks over artifacts, workers, node version, and everything.

I mean that we can create a dispute because of a faulty disc, sounds really bad to me. (I didn't jumped into the issue and read the discussion) Should it be really that complicated to make at least some simple distinction between actual execution errors and other errors?

[s0me0ne-unkn0wn]

I mean that we can create a dispute because of a faulty disc, sounds really bad to me. (I didn't jumped into the issue and read the discussion) Should it be really that complicated to make at least some simple distinction between actual execution errors and other errors?

Yes, I do agree that's no good. The complexity of implementation highly depends on what we're considering to be a transient error (and that's what all those discussions are about). The least complex approach discussed is like this: considering we check for successful runtime construction during PVF pre-checking, we can consider failed runtime construction during execution a transient problem (corrupted artifact, wrong Wastime version, etc.), and in that case, we could try to re-prepare the artifact from scratch and try to execute it again. That sounds like a solid strategy to me.

However, right now, after shifting our priorities for 2024, we lack the capacity to implement that. СС @eskimor in case we want to push it. Also, maybe some of our external contributors will want to work on it? CC @eagr @jpserrat @maksimryndin

[maksimryndin]

I mean that we can create a dispute because of a faulty disc, sounds really bad to me. (I didn't jumped into the issue and read the discussion) Should it be really that complicated to make at least some simple distinction between actual execution errors and other errors?

Yes, I do agree that's no good. The complexity of implementation highly depends on what we're considering to be a transient error (and that's what all those discussions are about). The least complex approach discussed is like this: considering we check for successful runtime construction during PVF pre-checking, we can consider failed runtime construction during execution a transient problem (corrupted artifact, wrong Wastime version, etc.), and in that case, we could try to re-prepare the artifact from scratch and try to execute it again. That sounds like a solid strategy to me.

However, right now, after shifting our priorities for 2024, we lack the capacity to implement that. СС @eskimor in case we want to push it. Also, maybe some of our external contributors will want to work on it? CC @eagr @jpserrat @maksimryndin

@s0me0ne-unkn0wn I can try to tackle it but I need some time to understand all the discussions :)

[s0me0ne-unkn0wn]

@maksimryndin don't hesitate to ask questions, and also jump on call/chat with me to discuss!

I'll try to formulate a separate issue for public discussion later today.

[maksimryndin]

This PR doesn't change the artifact naming scheme (except to make sure that the wasmtime version portion is correct), but that will anyway be superseded by #2895. I did some reading on MAX_PATH anyway which was quite interesting 😛 Seems like this value is very large on any reasonable, modern Linux, so not a practical concern there, though it could be on Mac.

Thanks @mrcnski for interesting insights on PATH_MAX!
Curious, when I first tried to build a substrate template node on the Ubuntu 22 QEMU VM with an encrypted home directory (ecryptfs), I encountered a build error "Filename is too long" or so. I found that ecryptfs limits filenames and paths. So I moved a substrate directory from home to the root to shorten the path. I'm not sure, it is relevant to actual validator setups, but a fun fact to tell :)

[maksimryndin]

@maksimryndin don't hesitate to ask questions, and also jump on call/chat with me to discuss!

I'll try to formulate a separate issue for public discussion later today.

@s0me0ne-unkn0wn @bkchr @jpserrat @eagr Let me summarize what I've understood so far :)

Problem
Inconsistency between node's wasmtime version and workers' one as a consequence of separate building of binaries

Initial solution
Check version of wasmtime via baked in generated version

More general solution (after discussions in this and related PRs, merge of #2895) for a more general problem of raising disputes for transient local errors
execute_interface should distinguish between different kind of errors. Some errors should be considered transient (like wasmtime versions discrepancy or an artifact corruption, other local issues) and not lead to unnecessary disputes about a candidate. Key assumption here is that an artifact is pre-checked (see #661). In this case the artifact should be re-prepared (important note from Marcin Since that process runs untrusted code, malicious code can return any error they want, so we can't treat any error returned by that process as an internal/local error.). Other errors (considered non-transient) proceed the usual way leading to disputes.

I believe the following is also related
artifact file integrity #2399; #677

Now I am going to understand the logic of workers interaction more

[s0me0ne-unkn0wn]

@maksimryndin I've just raised #3139 with a hopefully exhaustive description of my vision of the problem. Feel free to ask questions there!

[s0me0ne-unkn0wn]

Closing as superseded by #3187