Use same fmt and clippy configs as in Substrate (#7611)

* Use same rustfmt.toml as Substrate

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* format format file

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Format with new config

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add Substrate Clippy config

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Print Clippy version in CI

Otherwise its difficult to reproduce locally.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Make fmt happy

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update node/core/pvf/src/error.rs

Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>

* Update node/core/pvf/src/error.rs

Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Tsvetomir Dimitrov <tsvetomir@parity.io>
This commit is contained in:
Oliver Tale-Yazdi
2023-08-14 16:29:29 +02:00
committed by GitHub
parent ac435c96cf
commit 342d720573
203 changed files with 1880 additions and 1504 deletions
+2 -1
View File
@@ -224,7 +224,8 @@ impl Artifacts {
.is_none());
}
/// Remove and retrieve the artifacts from the table that are older than the supplied Time-To-Live.
/// Remove and retrieve the artifacts from the table that are older than the supplied
/// Time-To-Live.
pub fn prune(&mut self, artifact_ttl: Duration) -> Vec<ArtifactId> {
let now = SystemTime::now();
+15 -14
View File
@@ -38,29 +38,30 @@ pub enum InvalidCandidate {
/// The worker has died during validation of a candidate. That may fall in one of the following
/// categories, which we cannot distinguish programmatically:
///
/// (a) Some sort of transient glitch caused the worker process to abort. An example would be that
/// the host machine ran out of free memory and the OOM killer started killing the processes,
/// and in order to save the parent it will "sacrifice child" first.
/// (a) Some sort of transient glitch caused the worker process to abort. An example would be
/// that the host machine ran out of free memory and the OOM killer started killing the
/// processes, and in order to save the parent it will "sacrifice child" first.
///
/// (b) The candidate triggered a code path that has lead to the process death. For example,
/// the PVF found a way to consume unbounded amount of resources and then it either exceeded
/// an `rlimit` (if set) or, again, invited OOM killer. Another possibility is a bug in
/// wasmtime allowed the PVF to gain control over the execution worker.
/// the PVF found a way to consume unbounded amount of resources and then it either
/// exceeded an `rlimit` (if set) or, again, invited OOM killer. Another possibility is a
/// bug in wasmtime allowed the PVF to gain control over the execution worker.
///
/// We attribute such an event to an *invalid candidate* in either case.
///
/// The rationale for this is that a glitch may lead to unfair rejecting candidate by a single
/// validator. If the glitch is somewhat more persistent the validator will reject all candidate
/// thrown at it and hopefully the operator notices it by decreased reward performance of the
/// validator. On the other hand, if the worker died because of (b) we would have better chances
/// to stop the attack.
/// validator. If the glitch is somewhat more persistent the validator will reject all
/// candidate thrown at it and hopefully the operator notices it by decreased reward
/// performance of the validator. On the other hand, if the worker died because of (b) we would
/// have better chances to stop the attack.
AmbiguousWorkerDeath,
/// PVF execution (compilation is not included) took more time than was allotted.
HardTimeout,
/// A panic occurred and we can't be sure whether the candidate is really invalid or some internal glitch occurred.
/// Whenever we are unsure, we can never treat an error as internal as we would abstain from voting. This is bad
/// because if the issue was due to the candidate, then all validators would abstain, stalling finality on the
/// chain. So we will first retry the candidate, and if the issue persists we are forced to vote invalid.
/// A panic occurred and we can't be sure whether the candidate is really invalid or some
/// internal glitch occurred. Whenever we are unsure, we can never treat an error as internal
/// as we would abstain from voting. This is bad because if the issue was due to the candidate,
/// then all validators would abstain, stalling finality on the chain. So we will first retry
/// the candidate, and if the issue persists we are forced to vote invalid.
Panic(String),
}
+2 -1
View File
@@ -419,7 +419,8 @@ fn spawn_extra_worker(queue: &mut Queue, job: ExecuteJob) {
/// beforehand. In such a way, a race condition is avoided: during the worker being spawned,
/// another job in the queue, with an incompatible execution environment, may become stale, and
/// the queue would have to kill a newly started worker and spawn another one.
/// Nevertheless, if the worker finishes executing the job, it becomes idle and may be used to execute other jobs with a compatible execution environment.
/// Nevertheless, if the worker finishes executing the job, it becomes idle and may be used to
/// execute other jobs with a compatible execution environment.
async fn spawn_worker_task(
program_path: PathBuf,
job: ExecuteJob,
@@ -74,8 +74,9 @@ pub enum Outcome {
/// PVF execution completed successfully and the result is returned. The worker is ready for
/// another job.
Ok { result_descriptor: ValidationResult, duration: Duration, idle_worker: IdleWorker },
/// The candidate validation failed. It may be for example because the wasm execution triggered a trap.
/// Errors related to the preparation process are not expected to be encountered by the execution workers.
/// The candidate validation failed. It may be for example because the wasm execution triggered
/// a trap. Errors related to the preparation process are not expected to be encountered by the
/// execution workers.
InvalidCandidate { err: String, idle_worker: IdleWorker },
/// An internal error happened during the validation. Such an error is most likely related to
/// some transient glitch.
@@ -95,7 +96,8 @@ pub enum Outcome {
/// Given the idle token of a worker and parameters of work, communicates with the worker and
/// returns the outcome.
///
/// NOTE: Not returning the idle worker token in `Outcome` will trigger the child process being killed.
/// NOTE: Not returning the idle worker token in `Outcome` will trigger the child process being
/// killed.
pub async fn start_work(
worker: IdleWorker,
artifact: ArtifactPathId,
+10 -9
View File
@@ -455,8 +455,8 @@ async fn handle_precheck_pvf(
ArtifactState::Preparing { waiting_for_response, num_failures: _ } =>
waiting_for_response.push(result_sender),
ArtifactState::FailedToProcess { error, .. } => {
// Do not retry failed preparation if another pre-check request comes in. We do not retry pre-checking,
// anyway.
// Do not retry failed preparation if another pre-check request comes in. We do not
// retry pre-checking, anyway.
let _ = result_sender.send(PrepareResult::Err(error.clone()));
},
}
@@ -470,8 +470,8 @@ async fn handle_precheck_pvf(
/// Handles PVF execution.
///
/// This will try to prepare the PVF, if a prepared artifact does not already exist. If there is already a
/// preparation job, we coalesce the two preparation jobs.
/// This will try to prepare the PVF, if a prepared artifact does not already exist. If there is
/// already a preparation job, we coalesce the two preparation jobs.
///
/// If the prepare job succeeded previously, we will enqueue an execute job right away.
///
@@ -521,7 +521,8 @@ async fn handle_execute_pvf(
"handle_execute_pvf: Re-queuing PVF preparation for prepared artifact with missing file."
);
// The artifact has been prepared previously but the file is missing, prepare it again.
// The artifact has been prepared previously but the file is missing, prepare it
// again.
*state = ArtifactState::Preparing {
waiting_for_response: Vec::new(),
num_failures: 0,
@@ -721,8 +722,8 @@ async fn handle_prepare_done(
pending_requests
{
if result_tx.is_canceled() {
// Preparation could've taken quite a bit of time and the requester may be not interested
// in execution anymore, in which case we just skip the request.
// Preparation could've taken quite a bit of time and the requester may be not
// interested in execution anymore, in which case we just skip the request.
continue
}
@@ -855,8 +856,8 @@ fn can_retry_prepare_after_failure(
return false
}
// Retry if the retry cooldown has elapsed and if we have already retried less than `NUM_PREPARE_RETRIES` times. IO
// errors may resolve themselves.
// Retry if the retry cooldown has elapsed and if we have already retried less than
// `NUM_PREPARE_RETRIES` times. IO errors may resolve themselves.
SystemTime::now() >= last_time_failed + PREPARE_FAILURE_COOLDOWN &&
num_failures <= NUM_PREPARE_RETRIES
}
+15 -15
View File
@@ -32,26 +32,26 @@
//! (a) PVF pre-checking. This takes the `Pvf` code and tries to prepare it (verify and
//! compile) in order to pre-check its validity.
//!
//! (b) PVF execution. This accepts the PVF [`params`][`polkadot_parachain::primitives::ValidationParams`]
//! and the `Pvf` code, prepares (verifies and compiles) the code, and then executes PVF
//! with the `params`.
//! (b) PVF execution. This accepts the PVF
//! [`params`][`polkadot_parachain::primitives::ValidationParams`] and the `Pvf` code, prepares
//! (verifies and compiles) the code, and then executes PVF with the `params`.
//!
//! (c) Heads up. This request allows to signal that the given PVF may be needed soon and that it
//! should be prepared for execution.
//!
//! The preparation results are cached for some time after they either used or was signaled in heads up.
//! All requests that depends on preparation of the same PVF are bundled together and will be executed
//! as soon as the artifact is prepared.
//! The preparation results are cached for some time after they either used or was signaled in heads
//! up. All requests that depends on preparation of the same PVF are bundled together and will be
//! executed as soon as the artifact is prepared.
//!
//! # Priority
//!
//! PVF execution requests can specify the [priority][`Priority`] with which the given request should
//! be handled. Different priority levels have different effects. This is discussed below.
//! PVF execution requests can specify the [priority][`Priority`] with which the given request
//! should be handled. Different priority levels have different effects. This is discussed below.
//!
//! Preparation started by a heads up signal always starts with the background priority. If there
//! is already a request for that PVF preparation under way the priority is inherited. If after heads
//! up, a new PVF execution request comes in with a higher priority, then the original task's priority
//! will be adjusted to match the new one if it's larger.
//! is already a request for that PVF preparation under way the priority is inherited. If after
//! heads up, a new PVF execution request comes in with a higher priority, then the original task's
//! priority will be adjusted to match the new one if it's larger.
//!
//! Priority can never go down, only up.
//!
@@ -63,11 +63,11 @@
//! dissimilar to actors. Each of such "processes" is a future task that contains an event loop that
//! processes incoming messages, potentially delegating sub-tasks to other "processes".
//!
//! Two of these processes are queues. The first one is for preparation jobs and the second one is for
//! execution. Both of the queues are backed by separate pools of workers of different kind.
//! Two of these processes are queues. The first one is for preparation jobs and the second one is
//! for execution. Both of the queues are backed by separate pools of workers of different kind.
//!
//! Preparation workers handle preparation requests by prevalidating and instrumenting PVF wasm code,
//! and then passing it into the compiler, to prepare the artifact.
//! Preparation workers handle preparation requests by prevalidating and instrumenting PVF wasm
//! code, and then passing it into the compiler, to prepare the artifact.
//!
//! ## Artifacts
//!
+2 -1
View File
@@ -85,7 +85,8 @@ impl Metrics {
#[cfg(any(target_os = "linux", feature = "jemalloc-allocator"))]
if let Some(tracker_stats) = memory_stats.memory_tracker_stats {
// We convert these stats from B to KB to match the unit of `ru_maxrss` from `getrusage`.
// We convert these stats from B to KB to match the unit of `ru_maxrss` from
// `getrusage`.
let max_resident_kb = (tracker_stats.resident / 1024) as f64;
let max_allocated_kb = (tracker_stats.allocated / 1024) as f64;
+9 -7
View File
@@ -61,9 +61,9 @@ pub enum ToPool {
/// Request the given worker to start working on the given code.
///
/// Once the job either succeeded or failed, a [`FromPool::Concluded`] message will be sent back.
/// It's also possible that the worker dies before handling the message in which case [`FromPool::Rip`]
/// will be sent back.
/// Once the job either succeeded or failed, a [`FromPool::Concluded`] message will be sent
/// back. It's also possible that the worker dies before handling the message in which case
/// [`FromPool::Rip`] will be sent back.
///
/// In either case, the worker is considered busy and no further `StartWork` messages should be
/// sent until either `Concluded` or `Rip` message is received.
@@ -237,8 +237,8 @@ fn handle_to_pool(
);
} else {
// idle token is present after spawn and after a job is concluded;
// the precondition for `StartWork` is it should be sent only if all previous work
// items concluded;
// the precondition for `StartWork` is it should be sent only if all previous
// work items concluded;
// thus idle token is Some;
// qed.
never!("unexpected absence of the idle token in prepare pool");
@@ -311,7 +311,8 @@ fn handle_mux(
match outcome {
Outcome::Concluded { worker: idle, result } =>
handle_concluded_no_rip(from_pool, spawned, worker, idle, result),
// Return `Concluded`, but do not kill the worker since the error was on the host side.
// Return `Concluded`, but do not kill the worker since the error was on the host
// side.
Outcome::CreateTmpFileErr { worker: idle, err } => handle_concluded_no_rip(
from_pool,
spawned,
@@ -319,7 +320,8 @@ fn handle_mux(
idle,
Err(PrepareError::CreateTmpFileErr(err)),
),
// Return `Concluded`, but do not kill the worker since the error was on the host side.
// Return `Concluded`, but do not kill the worker since the error was on the host
// side.
Outcome::RenameTmpFileErr { worker: idle, result: _, err } =>
handle_concluded_no_rip(
from_pool,
+3 -2
View File
@@ -96,8 +96,9 @@ impl WorkerData {
}
}
/// A queue structured like this is prone to starving, however, we don't care that much since we expect
/// there is going to be a limited number of critical jobs and we don't really care if background starve.
/// A queue structured like this is prone to starving, however, we don't care that much since we
/// expect there is going to be a limited number of critical jobs and we don't really care if
/// background starve.
#[derive(Default)]
struct Unscheduled {
normal: VecDeque<Job>,
@@ -247,8 +247,8 @@ where
let outcome = f(tmp_file.clone(), stream).await;
// The function called above is expected to move `tmp_file` to a new location upon success. However,
// the function may as well fail and in that case we should remove the tmp file here.
// The function called above is expected to move `tmp_file` to a new location upon success.
// However, the function may as well fail and in that case we should remove the tmp file here.
//
// In any case, we try to remove the file here so that there are no leftovers. We only report
// errors that are different from the `NotFound`.
+14 -12
View File
@@ -196,13 +196,15 @@ pub enum SpawnErr {
Handshake,
}
/// This is a representation of a potentially running worker. Drop it and the process will be killed.
/// This is a representation of a potentially running worker. Drop it and the process will be
/// killed.
///
/// A worker's handle is also a future that resolves when it's detected that the worker's process
/// has been terminated. Since the worker is running in another process it is obviously not
/// necessary to poll this future to make the worker run, it's only for termination detection.
///
/// This future relies on the fact that a child process's stdout `fd` is closed upon it's termination.
/// This future relies on the fact that a child process's stdout `fd` is closed upon it's
/// termination.
#[pin_project]
pub struct WorkerHandle {
child: process::Child,
@@ -240,15 +242,15 @@ impl WorkerHandle {
child_id,
stdout,
program: program.as_ref().to_path_buf(),
// We don't expect the bytes to be ever read. But in case we do, we should not use a buffer
// of a small size, because otherwise if the child process does return any data we will end up
// issuing a syscall for each byte. We also prefer not to do allocate that on the stack, since
// each poll the buffer will be allocated and initialized (and that's due `poll_read` takes &mut [u8]
// and there are no guarantees that a `poll_read` won't ever read from there even though that's
// unlikely).
// We don't expect the bytes to be ever read. But in case we do, we should not use a
// buffer of a small size, because otherwise if the child process does return any data
// we will end up issuing a syscall for each byte. We also prefer not to do allocate
// that on the stack, since each poll the buffer will be allocated and initialized (and
// that's due `poll_read` takes &mut [u8] and there are no guarantees that a `poll_read`
// won't ever read from there even though that's unlikely).
//
// OTOH, we also don't want to be super smart here and we could just afford to allocate a buffer
// for that here.
// OTOH, we also don't want to be super smart here and we could just afford to allocate
// a buffer for that here.
drop_box: vec![0; 8192].into_boxed_slice(),
})
}
@@ -280,8 +282,8 @@ impl futures::Future for WorkerHandle {
}
},
Err(err) => {
// The implementation is guaranteed to not to return `WouldBlock` and Interrupted. This
// leaves us with legit errors which we suppose were due to termination.
// The implementation is guaranteed to not to return `WouldBlock` and Interrupted.
// This leaves us with legit errors which we suppose were due to termination.
// Log the status code.
gum::debug!(