[WIP] Pvf host prechecking support #4101
Conversation
github-actions
bot
added
the
A3-in_progress
| @@ -22,7 +22,7 @@ use std::{fmt, sync::Arc}; | |||
| /// A struct that carries code of a parachain validation function and it's hash. | |||
| /// | |||
| /// Should be cheap to clone. | |||
| #[derive(Clone)] | |||
| #[derive(Clone, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Why do we need PartialEq and Eq impls here?
The implementation for PartialEq will be derived based on Vec's which implies byte-wise comparsion, which is really not efficient and also not required since we have the hash at hand.
There was a problem hiding this comment.
ToPool implements it (for testing purpose I guess), one of events now contains Pvf, that's why I derived it. A subject to change of course.
| @@ -265,6 +291,7 @@ async fn run( | |||
| cleanup_pulse_interval, | |||
| artifact_ttl, | |||
| mut artifacts, | |||
| mut precheck_statuses, | |||
There was a problem hiding this comment.
I think passing this struct as a parameter doesn't really make sense: the only thing caller does is just creates a new instance.
Let's move it inside the body making it a local.
| /// Failed to precheck the PVF due to the time limit. | ||
| TimedOut, | ||
| /// Failed to precheck the PVF due to the memory limit. | ||
| MemoryLimitReached, |
There was a problem hiding this comment.
Are we using this anywhere?
There was a problem hiding this comment.
Nope, I don't know yet how to catch this kind of error, even though there's a memory limit in executor configuration, I don't see any public interface that tells you preparation exceeded it
There was a problem hiding this comment.
ah, I think that's irrelevant.
The memory limit in the executor is only for the execution. There is no option to limit the memory amount consumed by the compilation. That's actually one of the reasons we do it in a separate process in the first place.
| use std::collections::hash_map::Entry; | ||
|
|
||
| if let Some(_) = artifacts.artifact_state(&pvf.as_artifact_id()) { | ||
| let _ = result_sender.send(Ok(())); |
There was a problem hiding this comment.
As the things stand now, we only note whether we processed the artifact or not in artifacts. We do not record anything regarding the preparation outcome. It will also return Some in case the artifact is going through preparation.
Thus this will incorrectly report an Ok when the artifact is in fact preparing or prepared with an error.
There was a problem hiding this comment.
If a PVF actually got to the point of preparing, doesn't it mean it's considered prechecked?
There was a problem hiding this comment.
Well, kind of.
That is, under the current model, the owner of the PVF validation host will only send a command to execute PVF, if the PVF was approved on-chain. That assumption is a bit too fragile to rely upon though IMO.
| }, | ||
| } | ||
| }) | ||
| .await | ||
| } | ||
|
|
||
| pub async fn start_precheck_work( |
There was a problem hiding this comment.
Why can't we get away with only single code path?
From a glance, I can see that the pre-checking code path doesn't persist the resulting artifact. Which is acceptable, but not necessary.
It's important that it's not the otherway around actually: e.g. assuming we run preparation with parallel_compilation on, if pre-checking reused the result then its result will be skewed.
There was a problem hiding this comment.
Code path is the same for them, the actual worker process accepts both types of tasks, it's that futures attached to the worker stream are different, trying to fit them in a single function would be a complete mess.
From the point of memory consumption it makes no difference, on the other hand having two methods handling separate tasks improves readability.
Probably I didn't quite understand you?
|
Closed in favor of #4123 |
Resolves #3857