Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Accept PVF code hashes in validation host #3655

Open
slumber wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Accept PVF code hashes in validation host #3655

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
2880898
Refactor runtime API requests
slumber Aug 17, 2021
2c6a031
Try to use cached compiled PVF
slumber Aug 17, 2021
da26bf0
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Aug 17, 2021
162be9b
fmt
slumber Aug 17, 2021
427bc1a
Refactor runtime API requests
slumber Aug 19, 2021
7311c27
Introduce PvfPreimage
slumber Aug 19, 2021
094d164
Reliable error handling
slumber Aug 19, 2021
50fcfca
Improve candidate validation readability
slumber Aug 22, 2021
2c39c37
Send correct hash to the PVF host
slumber Aug 22, 2021
8d40599
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Aug 22, 2021
c7cf9f5
Test validation by hash feature
slumber Aug 23, 2021
2d0d047
Remove extra comma
slumber Aug 30, 2021
828af04
Rename
slumber Oct 25, 2021
d2a2dbf
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Oct 25, 2021
8da537a
Rework candidate validation to use new runtime API endpoint
slumber Oct 25, 2021
bec11a3
fmt
slumber Oct 25, 2021
33560ca
Remove extra line
slumber Oct 26, 2021
0a93b6a
Get rid of unreachable
slumber Oct 27, 2021
e58b1ec
Remove mutable result
slumber Oct 27, 2021
0e9b0fe
Log the error
slumber Oct 27, 2021
a11d53a
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Dec 17, 2021
e4edd44
Resolve precheck todo
slumber Jan 18, 2022
9c91729
Host tests
slumber Jan 19, 2022
cb82bd4
Update implementers guide
slumber Jan 19, 2022
fb7e4e7
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Jan 19, 2022
c38eb81
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Jan 21, 2022
2a48e29
Remove unnecessary log message
slumber Feb 14, 2022
24c043c
Improve naming
slumber Feb 14, 2022
c3cf617
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Feb 14, 2022
249fc28
Revert error handling
slumber Feb 14, 2022
47e9e7e
Merge remote-tracking branch 'origin/master' into slumber-use-cached-pvf
slumber Mar 30, 2022
0a847ab
Replace tracing usage
slumber Mar 30, 2022
bccbbc3
Explain force enacting
slumber Apr 1, 2022
3668cfa
Merge branch 'master' into slumber-use-cached-pvf
mrcnski Oct 11, 2022
32680ca
Fix leftover errors from merge
mrcnski Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 129 additions & 108 deletions node/core/candidate-validation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ use polkadot_node_subsystem::{
use polkadot_node_subsystem_util::metrics::{self, prometheus};
use polkadot_parachain::primitives::{ValidationParams, ValidationResult as WasmValidationResult};
use polkadot_primitives::v1::{
CandidateCommitments, CandidateDescriptor, Hash, OccupiedCoreAssumption,
PersistedValidationData, ValidationCode, ValidationCodeHash,
BlockNumber, CandidateCommitments, CandidateDescriptor, Hash, PersistedValidationData,
ValidationCode, ValidationCodeHash,
};

use parity_scale_codec::Encode;
Expand Down Expand Up @@ -171,15 +171,17 @@ where
response_sender,
) => {
let bg = {
let mut sender = ctx.sender().clone();
let metrics = metrics.clone();
let validation_host = validation_host.clone();

async move {
let _timer = metrics.time_validate_from_exhaustive();
let res = validate_candidate_exhaustive(
&mut sender,
validation_host,
persisted_validation_data,
validation_code,
Some(validation_code),
descriptor,
pov,
timeout,
Expand All @@ -199,6 +201,7 @@ where
}
}

#[derive(Debug)]
struct RuntimeRequestFailed;

async fn runtime_api_request<T, Sender>(
Expand Down Expand Up @@ -235,95 +238,50 @@ where
})
}

#[derive(Debug)]
enum AssumptionCheckOutcome {
Matches(PersistedValidationData, ValidationCode),
DoesNotMatch,
BadRequest,
}

async fn check_assumption_validation_data<Sender>(
async fn request_validation_code_by_hash<Sender>(
sender: &mut Sender,
descriptor: &CandidateDescriptor,
assumption: OccupiedCoreAssumption,
) -> AssumptionCheckOutcome
) -> Result<Option<ValidationCode>, RuntimeRequestFailed>
where
Sender: SubsystemSender,
{
let validation_data = {
let (tx, rx) = oneshot::channel();
let d = runtime_api_request(
sender,
descriptor.relay_parent,
RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx),
rx,
)
.await;

match d {
Ok(None) | Err(RuntimeRequestFailed) => return AssumptionCheckOutcome::BadRequest,
Ok(Some(d)) => d,
}
};

let persisted_validation_data_hash = validation_data.hash();

if descriptor.persisted_validation_data_hash == persisted_validation_data_hash {
let (code_tx, code_rx) = oneshot::channel();
let validation_code = runtime_api_request(
sender,
descriptor.relay_parent,
RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, code_tx),
code_rx,
)
.await;

match validation_code {
Ok(None) | Err(RuntimeRequestFailed) => AssumptionCheckOutcome::BadRequest,
Ok(Some(v)) => AssumptionCheckOutcome::Matches(validation_data, v),
}
} else {
AssumptionCheckOutcome::DoesNotMatch
}
let (tx, rx) = oneshot::channel();
runtime_api_request(
sender,
descriptor.relay_parent,
RuntimeApiRequest::ValidationCodeByHash(descriptor.validation_code_hash, tx),
rx,
)
.await
}

async fn find_assumed_validation_data<Sender>(
async fn request_assumed_validation_data<Sender>(
sender: &mut Sender,
descriptor: &CandidateDescriptor,
) -> AssumptionCheckOutcome
) -> Result<
Option<(PersistedValidationData<Hash, BlockNumber>, ValidationCodeHash)>,
RuntimeRequestFailed,
>
where
Sender: SubsystemSender,
{
// The candidate descriptor has a `persisted_validation_data_hash` which corresponds to
// one of up to two possible values that we can derive from the state of the
// relay-parent. We can fetch these values by getting the persisted validation data
// based on the different `OccupiedCoreAssumption`s.

const ASSUMPTIONS: &[OccupiedCoreAssumption] = &[
OccupiedCoreAssumption::Included,
OccupiedCoreAssumption::TimedOut,
// `TimedOut` and `Free` both don't perform any speculation and therefore should be the same
// for our purposes here. In other words, if `TimedOut` matched then the `Free` must be
// matched as well.
];

// Consider running these checks in parallel to reduce validation latency.
for assumption in ASSUMPTIONS {
let outcome = check_assumption_validation_data(sender, descriptor, *assumption).await;

match outcome {
AssumptionCheckOutcome::Matches(_, _) => return outcome,
AssumptionCheckOutcome::BadRequest => return outcome,
AssumptionCheckOutcome::DoesNotMatch => continue,
}
}

AssumptionCheckOutcome::DoesNotMatch
let (tx, rx) = oneshot::channel();
runtime_api_request(
sender,
descriptor.relay_parent,
RuntimeApiRequest::AssumedValidationData(
descriptor.para_id,
descriptor.persisted_validation_data_hash,
tx,
),
rx,
)
.await
}

async fn validate_from_chain_state<Sender>(
sender: &mut Sender,
validation_host: ValidationHost,
validation_host: impl ValidationBackend,
descriptor: CandidateDescriptor,
pov: Arc<PoV>,
timeout: Duration,
Expand All @@ -332,24 +290,31 @@ async fn validate_from_chain_state<Sender>(
where
Sender: SubsystemSender,
{
let (validation_data, validation_code) =
match find_assumed_validation_data(sender, &descriptor).await {
AssumptionCheckOutcome::Matches(validation_data, validation_code) =>
(validation_data, validation_code),
AssumptionCheckOutcome::DoesNotMatch => {
let (validation_data, validation_code_hash) =
match request_assumed_validation_data(sender, &descriptor).await {
Ok(Some((data, code_hash))) => (data, code_hash),
Ok(None) => {
// TODO: update comment.
// If neither the assumption of the occupied core having the para included or the assumption
// of the occupied core timing out are valid, then the persisted_validation_data_hash in the descriptor
// is not based on the relay parent and is thus invalid.
return Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))
},
AssumptionCheckOutcome::BadRequest =>
return Err(ValidationFailed("Assumption Check: Bad request".into())),
Err(_) =>
return Err(ValidationFailed(
"Failed to request persisted validation data from the Runtime API".into(),
)),
};

if descriptor.validation_code_hash != validation_code_hash {
return Ok(ValidationResult::Invalid(InvalidCandidate::CodeHashMismatch))
}

let validation_result = validate_candidate_exhaustive(
sender,
validation_host,
validation_data,
validation_code,
None,
Comment thread
slumber marked this conversation as resolved.
descriptor.clone(),
pov,
timeout,
Expand Down Expand Up @@ -377,18 +342,22 @@ where
validation_result
}

async fn validate_candidate_exhaustive(
async fn validate_candidate_exhaustive<Sender>(
sender: &mut Sender,
mut validation_backend: impl ValidationBackend,
persisted_validation_data: PersistedValidationData,
validation_code: ValidationCode,
validation_code: Option<ValidationCode>,
descriptor: CandidateDescriptor,
pov: Arc<PoV>,
timeout: Duration,
metrics: &Metrics,
) -> Result<ValidationResult, ValidationFailed> {
) -> Result<ValidationResult, ValidationFailed>
where
Sender: SubsystemSender,
{
let _timer = metrics.time_validate_candidate_exhaustive();

let validation_code_hash = validation_code.hash();
let validation_code_hash = validation_code.as_ref().map(ValidationCode::hash);
tracing::debug!(
target: LOG_TARGET,
?validation_code_hash,
Expand All @@ -400,22 +369,30 @@ async fn validate_candidate_exhaustive(
&descriptor,
persisted_validation_data.max_pov_size,
&*pov,
&validation_code_hash,
validation_code_hash.as_ref(),
) {
return Ok(ValidationResult::Invalid(e))
}

let raw_validation_code = match sp_maybe_compressed_blob::decompress(
&validation_code.0,
VALIDATION_CODE_BOMB_LIMIT,
) {
Ok(code) => code,
Err(e) => {
tracing::debug!(target: LOG_TARGET, err=?e, "Invalid validation code");
let validation_code = if let Some(validation_code) = validation_code {
let raw_code = match sp_maybe_compressed_blob::decompress(
&validation_code.0,
VALIDATION_CODE_BOMB_LIMIT,
) {
Ok(code) => code,
Err(e) => {
tracing::debug!(target: LOG_TARGET, err=?e, "Code decompression failed");

// If the validation code is invalid, the candidate certainly is.
return Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))
},
// If the validation code is invalid, the candidate certainly is.
return Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))
},
}
.to_vec();
Pvf::from_code(raw_code)
} else {
// In case validation code is not provided, ask the backend to obtain
// it from the cache using the hash.
Pvf::Hash(ValidationCodeHash::from(descriptor.validation_code_hash))
};

let raw_block_data =
Expand All @@ -436,9 +413,45 @@ async fn validate_candidate_exhaustive(
relay_parent_storage_root: persisted_validation_data.relay_parent_storage_root,
};

let result = validation_backend
.validate_candidate(raw_validation_code.to_vec(), timeout, params)
.await;
let result = match validation_backend
.validate_candidate(validation_code, timeout, params.clone())
.await
{
Err(ValidationError::ArtifactNotFound) => {
// In case preimage for the supplied code hash was not found by the
// validation host, request the code from Runtime API and try again.
tracing::debug!(
target: LOG_TARGET,
"Validation host failed to find artifact by provided hash",
);

let validation_code = match request_validation_code_by_hash(sender, &descriptor).await {
Ok(Some(validation_code)) => validation_code,
Ok(None) =>
// TODO: code not found by hash by the runtime, is this candidate invalid?
Comment thread
slumber marked this conversation as resolved.
Outdated
return Err(ValidationFailed(
"Runtime API didn't return validation code by hash".into(),
)),
Err(_) => return Err(ValidationFailed("Runtime API request failed".into())),
};

let raw_code = match sp_maybe_compressed_blob::decompress(
&validation_code.0,
VALIDATION_CODE_BOMB_LIMIT,
) {
Ok(code) => code,
Err(e) => {
tracing::debug!(target: LOG_TARGET, err=?e, "Code decompression failed");

// If the validation code is invalid, the candidate certainly is.
return Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))
},
};
let validation_code = Pvf::from_code(raw_code.to_vec());
validation_backend.validate_candidate(validation_code, timeout, params).await
},
result => result,
};

if let Err(ref e) = result {
tracing::debug!(
Expand All @@ -459,7 +472,15 @@ async fn validate_candidate_exhaustive(
Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(
"ambigious worker death".to_string(),
))),

Err(ValidationError::ArtifactNotFound) => {
// The code was supplied on the second attempt, this
// error should be unreachable.
let err = ValidationFailed(
"Validation host failed to find artifact even though it was supplied".to_string(),
);
tracing::error!(target: LOG_TARGET, error = ?err, "Unexpected error reported by the validation backend");
Comment thread
slumber marked this conversation as resolved.
Outdated
Err(err)
Comment on lines +478 to +579

@drahnr drahnr Nov 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho there are too many strings involved and ValidationFailed should become a proper error enum, but that's out of scope for this PR. I also don't see a point to print the error here, the caller should take care of printing the Err(err) once there are variants to distinguish them by.

@slumber slumber Nov 1, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this could be replaced with unreachable, but since we try to avoid panicking the error simply gets logged. Caller (as I understand the component that sent the validation request?) should never receive this error, so we handle it here. Though I totally agree on the part of turning strings into enum.

The PR is on ice due to dependency on the recent runtime upgrade, so probably I could fix ValidationFailed too, but I wonder how would anybody handle variants? Isn't it a kind of internal error that means something went wrong and we should skip the candidate?

@drahnr drahnr Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What my comment boils down to, is that


/// Blanket error for validation failing for internal reasons.
#[derive(Debug, Error)]
#[error("Validation failed with {0:?}")]
pub struct ValidationFailed(pub String);

exists which is an error antipattern, and makes the code less legible than it could be. It should be an error enum with explicit variants and annotations using thiserror.

},
Ok(res) =>
if res.head_data.hash() != descriptor.para_head {
Ok(ValidationResult::Invalid(InvalidCandidate::ParaHeadHashMismatch))
Expand All @@ -481,7 +502,7 @@ async fn validate_candidate_exhaustive(
trait ValidationBackend {
async fn validate_candidate(
&mut self,
raw_validation_code: Vec<u8>,
validation_code: Pvf,
timeout: Duration,
params: ValidationParams,
) -> Result<WasmValidationResult, ValidationError>;
Expand All @@ -491,14 +512,14 @@ trait ValidationBackend {
impl ValidationBackend for ValidationHost {
async fn validate_candidate(
&mut self,
raw_validation_code: Vec<u8>,
validation_code: Pvf,
Comment thread
drahnr marked this conversation as resolved.
Outdated
timeout: Duration,
params: ValidationParams,
) -> Result<WasmValidationResult, ValidationError> {
let (tx, rx) = oneshot::channel();
if let Err(err) = self
.execute_pvf(
Pvf::from_code(raw_validation_code),
validation_code,
timeout,
params.encode(),
polkadot_node_core_pvf::Priority::Normal,
Expand Down Expand Up @@ -526,7 +547,7 @@ fn perform_basic_checks(
candidate: &CandidateDescriptor,
max_pov_size: u32,
pov: &PoV,
validation_code_hash: &ValidationCodeHash,
validation_code_hash: Option<&ValidationCodeHash>,
) -> Result<(), InvalidCandidate> {
let pov_hash = pov.hash();

Expand All @@ -539,7 +560,7 @@ fn perform_basic_checks(
return Err(InvalidCandidate::PoVHashMismatch)
}

if *validation_code_hash != candidate.validation_code_hash {
if let Some(false) = validation_code_hash.map(|hash| *hash == candidate.validation_code_hash) {
Comment thread
pepyakin marked this conversation as resolved.
Comment thread
slumber marked this conversation as resolved.
return Err(InvalidCandidate::CodeHashMismatch)
}

Expand Down
Loading