From 04999b3bb1b06d2146f36d97b27b8e47f5288e06 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 13 Mar 2023 17:57:46 +0100 Subject: [PATCH 1/3] PVF: Document that preparation cannot lead to disputes --- node/core/candidate-validation/src/lib.rs | 2 +- node/core/pvf/src/error.rs | 6 ------ .../implementers-guide/src/node/utility/pvf-prechecker.md | 6 ++++++ roadmap/implementers-guide/src/pvf-prechecking.md | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index fdd6ff002dd1..7bc18dbe6a7a 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -638,7 +638,7 @@ where ))), Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => { // In principle if preparation of the `WASM` fails, the current candidate can not be the - // reason for that. So we can't say whether it is invalid or not in addition with + // reason for that. So we can't say whether it is invalid or not. In addition, with // pre-checking enabled only valid runtimes should ever get enacted, so we can be // reasonably sure that this is some local problem on the current node. Err(ValidationFailed(e)) diff --git a/node/core/pvf/src/error.rs b/node/core/pvf/src/error.rs index 3f642cd6ed24..5edf435e7190 100644 --- a/node/core/pvf/src/error.rs +++ b/node/core/pvf/src/error.rs @@ -120,12 +120,6 @@ impl From for ValidationError { fn from(error: PrepareError) -> Self { // Here we need to classify the errors into two errors: deterministic and non-deterministic. // See [`PrepareError::is_deterministic`]. - // - // We treat the deterministic errors as `InvalidCandidate`. Should those occur they could - // potentially trigger disputes. - // - // All non-deterministic errors are qualified as `InternalError`s and will not trigger - // disputes. if error.is_deterministic() { ValidationError::InvalidCandidate(InvalidCandidate::PrepareError(error.to_string())) } else { diff --git a/roadmap/implementers-guide/src/node/utility/pvf-prechecker.md b/roadmap/implementers-guide/src/node/utility/pvf-prechecker.md index 90193ec00e18..3cae12e65f33 100644 --- a/roadmap/implementers-guide/src/node/utility/pvf-prechecker.md +++ b/roadmap/implementers-guide/src/node/utility/pvf-prechecker.md @@ -35,6 +35,12 @@ Rejecting instead of abstaining is better in several ways: Also, if we only abstain, an attacker can specially craft a PVF wasm blob so that it will fail on e.g. 50% of the validators. In that case a supermajority will never be reached and the vote will repeat multiple times, most likely with the same result (since all votes are cleared on a session change). This is avoided by rejecting failed PVFs, and by only requiring 1/3 of validators to reject a PVF to reach a decision. +### Note on Disputes + +Having a pre-checking phase allows us to make certain assumptions later when preparing the PVF for execution. If a runtime passed pre-checking, then we know that the runtime should be valid, and therefore any issue during preparation for execution can be assumed to be a local problem on the current node. + +For this reason, even deterministic preparation errors should not trigger disputes. And since we do not dispute as a result of the pre-checking phase, as stated above, it should be impossible for preparation in general to result in disputes. + [overview]: ../../pvf-prechecking.md [Runtime API]: runtime-api.md [PVF pre-checking runtime API]: ../../runtime-api/pvf-prechecking.md diff --git a/roadmap/implementers-guide/src/pvf-prechecking.md b/roadmap/implementers-guide/src/pvf-prechecking.md index fd2ca12330bd..155d32d52898 100644 --- a/roadmap/implementers-guide/src/pvf-prechecking.md +++ b/roadmap/implementers-guide/src/pvf-prechecking.md @@ -46,7 +46,7 @@ The logic described above is implemented by the [paras] module. On the node-side, there is a PVF pre-checking [subsystem][pvf-prechecker-subsystem] that scans the chain for new PVFs via using [runtime APIs][pvf-runtime-api]. Upon finding a new PVF, the subsystem will initiate a PVF pre-checking request and wait for the result. Whenever the result is obtained, the subsystem will use the [runtime API][pvf-runtime-api] to submit a vote for the PVF. The vote is an unsigned transaction. The vote will be distributed via the gossip similarly to a normal transaction. Eventually a block producer will include the vote into the block where it will be handled by the [runtime][paras]. -## Pre-checking Summary +## Summary Parachains' and parathreads' validation function is described by a wasm module that we refer to as a PVF. @@ -54,7 +54,7 @@ In order to make the PVF usable for candidate validation it has to be registered As part of the registration process, it has to go through pre-checking. Pre-checking is a game of attempting preparation and reporting the results back on-chain. -We define preparation as a process that: validates the consistency of the wasm binary (aka prevalidation) and the compilation of the wasm module into machine code (refered to as artifact). +We define preparation as a process that: validates the consistency of the wasm binary (aka prevalidation) and the compilation of the wasm module into machine code (referred to as an artifact). Besides pre-checking, preparation can also be triggered by execution, since a compiled artifact is needed for the execution. If an artifact already exists, execution will skip preparation. If it does do preparation, execution uses a more lenient timeout than preparation, to avoid the situation where honest validators fail on valid, pre-checked PVFs. From 31d7a8c7c8b36710ba39bf7e7070ee91b91768f4 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 13 Mar 2023 18:07:06 +0100 Subject: [PATCH 2/3] Add warning for deterministic errors --- node/core/candidate-validation/src/lib.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 7bc18dbe6a7a..8ca4d821007c 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -623,7 +623,7 @@ where .await; if let Err(ref error) = result { - gum::info!(target: LOG_TARGET, ?para_id, ?error, "Failed to validate candidate",); + gum::info!(target: LOG_TARGET, ?para_id, ?error, "Failed to validate candidate"); } match result { @@ -640,7 +640,14 @@ where // In principle if preparation of the `WASM` fails, the current candidate can not be the // reason for that. So we can't say whether it is invalid or not. In addition, with // pre-checking enabled only valid runtimes should ever get enacted, so we can be - // reasonably sure that this is some local problem on the current node. + // reasonably sure that this is some local problem on the current node. However, as this + // particular error *seems* to indicate a deterministic error, we raise a warning. + gum::warn!( + target: LOG_TARGET, + ?para_id, + ?error, + "Deterministic error occurred during preparation (should have been ruled out by pre-checking phase)", + ); Err(ValidationFailed(e)) }, Ok(res) => From 22b1fb5b58eda7bf3110a223b2a69b0b4c28dcf1 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 13 Mar 2023 18:09:24 +0100 Subject: [PATCH 3/3] Fix warning --- node/core/candidate-validation/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 8ca4d821007c..a1c939b236ac 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -645,7 +645,7 @@ where gum::warn!( target: LOG_TARGET, ?para_id, - ?error, + ?e, "Deterministic error occurred during preparation (should have been ruled out by pre-checking phase)", ); Err(ValidationFailed(e))