From 2880898ec6a8c0a3af7ef5dcb43e7bb1725704d1 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Tue, 17 Aug 2021 22:43:13 +0300 Subject: [PATCH 01/26] Refactor runtime API requests --- node/core/candidate-validation/src/lib.rs | 201 ++++++++++++-------- node/core/candidate-validation/src/tests.rs | 126 ++++++------ 2 files changed, 195 insertions(+), 132 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 17b625e67670..f5acf459f579 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -97,6 +97,45 @@ where } } +trait FromRuntimeApi: Sized { + fn request( + descriptor: &CandidateDescriptor, + assumption: OccupiedCoreAssumption, + ) -> (RuntimeApiRequest, oneshot::Receiver, RuntimeApiError>>); + + fn validate_hash(&self, descriptor: &CandidateDescriptor) -> bool; +} + +impl FromRuntimeApi for ValidationCode { + fn validate_hash(&self, _descriptor: &CandidateDescriptor) -> bool { + // Computing the code hash might be expensive, instead, rely + // on the [`PersistedValidationData`] hash. + true + } + + fn request( + descriptor: &CandidateDescriptor, + assumption: OccupiedCoreAssumption, + ) -> (RuntimeApiRequest, oneshot::Receiver, RuntimeApiError>>) { + let (tx, rx) = oneshot::channel(); + (RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, tx), rx) + } +} + +impl FromRuntimeApi for PersistedValidationData { + fn validate_hash(&self, descriptor: &CandidateDescriptor) -> bool { + self.hash() == descriptor.persisted_validation_data_hash + } + + fn request( + descriptor: &CandidateDescriptor, + assumption: OccupiedCoreAssumption, + ) -> (RuntimeApiRequest, oneshot::Receiver, RuntimeApiError>>) { + let (tx, rx) = oneshot::channel(); + (RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx), rx) + } +} + async fn run( mut ctx: Context, metrics: Metrics, @@ -114,8 +153,8 @@ where loop { match ctx.recv().await? { - FromOverseer::Signal(OverseerSignal::ActiveLeaves(_)) => {}, - FromOverseer::Signal(OverseerSignal::BlockFinalized(..)) => {}, + FromOverseer::Signal(OverseerSignal::ActiveLeaves(_)) => {} + FromOverseer::Signal(OverseerSignal::BlockFinalized(..)) => {} FromOverseer::Signal(OverseerSignal::Conclude) => return Ok(()), FromOverseer::Communication { msg } => match msg { CandidateValidationMessage::ValidateFromChainState( @@ -138,10 +177,10 @@ where Ok(x) => { metrics.on_validation_event(&x); let _ = response_sender.send(x); - }, + } Err(e) => return Err(e), } - }, + } CandidateValidationMessage::ValidateFromExhaustive( persisted_validation_data, validation_code, @@ -170,10 +209,10 @@ where "Requester of candidate validation dropped", ) } - }, + } Err(e) => return Err(e), } - }, + } }, } } @@ -195,73 +234,58 @@ where } #[derive(Debug)] -enum AssumptionCheckOutcome { - Matches(PersistedValidationData, ValidationCode), +enum AssumptionCheckOutcome { + Matches(T, OccupiedCoreAssumption), DoesNotMatch, BadRequest, } -async fn check_assumption_validation_data( +async fn check_assumption_validation_data( ctx: &mut Context, descriptor: &CandidateDescriptor, assumption: OccupiedCoreAssumption, -) -> SubsystemResult +) -> SubsystemResult> where Context: SubsystemContext, Context: overseer::SubsystemContext, + T: FromRuntimeApi, { - let validation_data = { - let (tx, rx) = oneshot::channel(); - let d = runtime_api_request( - ctx, - descriptor.relay_parent, - RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx), - rx, - ) - .await?; + let data: T = { + let (request, rx) = ::request(descriptor, assumption); + let data = runtime_api_request(ctx, descriptor.relay_parent, request, rx).await?; - match d { + match data { Ok(None) | Err(_) => return Ok(AssumptionCheckOutcome::BadRequest), - Ok(Some(d)) => d, + Ok(Some(data)) => data, } }; - let persisted_validation_data_hash = validation_data.hash(); - - SubsystemResult::Ok( - if descriptor.persisted_validation_data_hash == persisted_validation_data_hash { - let (code_tx, code_rx) = oneshot::channel(); - let validation_code = runtime_api_request( - ctx, - descriptor.relay_parent, - RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, code_tx), - code_rx, - ) - .await?; - - match validation_code { - Ok(None) | Err(_) => AssumptionCheckOutcome::BadRequest, - Ok(Some(v)) => AssumptionCheckOutcome::Matches(validation_data, v), - } - } else { - AssumptionCheckOutcome::DoesNotMatch - }, - ) + SubsystemResult::Ok(if data.validate_hash(descriptor) { + AssumptionCheckOutcome::Matches(data, assumption) + } else { + AssumptionCheckOutcome::DoesNotMatch + }) } -async fn find_assumed_validation_data( +async fn find_assumed_validation_data( ctx: &mut Context, descriptor: &CandidateDescriptor, -) -> SubsystemResult + checked_assumption: Option, +) -> SubsystemResult> where Context: SubsystemContext, Context: overseer::SubsystemContext, + T: FromRuntimeApi, { // 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. + if let Some(assumption) = checked_assumption { + return Ok(check_assumption_validation_data(ctx, descriptor, assumption).await?); + } + const ASSUMPTIONS: &[OccupiedCoreAssumption] = &[ OccupiedCoreAssumption::Included, OccupiedCoreAssumption::TimedOut, @@ -295,18 +319,32 @@ where Context: SubsystemContext, Context: overseer::SubsystemContext, { - let (validation_data, validation_code) = - match find_assumed_validation_data(ctx, &descriptor).await? { - AssumptionCheckOutcome::Matches(validation_data, validation_code) => - (validation_data, validation_code), + let (validation_data, valid_assumption) = + match find_assumed_validation_data(ctx, &descriptor, None).await? { + AssumptionCheckOutcome::Matches(validation_data, valid_assumption) => { + (validation_data, valid_assumption) + } AssumptionCheckOutcome::DoesNotMatch => { // 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(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))) - }, - AssumptionCheckOutcome::BadRequest => - return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))); + } + AssumptionCheckOutcome::BadRequest => { + return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))) + } + }; + + let validation_code = + match find_assumed_validation_data(ctx, &descriptor, Some(valid_assumption)).await? { + AssumptionCheckOutcome::Matches(validation_code, _) => validation_code, + AssumptionCheckOutcome::DoesNotMatch => { + // Code hash is not validated, should be unreachable. + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))); + } + AssumptionCheckOutcome::BadRequest => { + return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))) + } }; let validation_result = validate_candidate_exhaustive( @@ -329,10 +367,13 @@ where ) .await? { - Ok(true) => {}, - Ok(false) => return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::InvalidOutputs))), - Err(_) => - return Ok(Err(ValidationFailed("Check Validation Outputs: Bad request".into()))), + Ok(true) => {} + Ok(false) => { + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::InvalidOutputs))) + } + Err(_) => { + return Ok(Err(ValidationFailed("Check Validation Outputs: Bad request".into()))) + } } } @@ -355,7 +396,7 @@ async fn validate_candidate_exhaustive( &*pov, &validation_code, ) { - return Ok(Ok(ValidationResult::Invalid(e))) + return Ok(Ok(ValidationResult::Invalid(e))); } let raw_validation_code = match sp_maybe_compressed_blob::decompress( @@ -367,8 +408,8 @@ async fn validate_candidate_exhaustive( tracing::debug!(target: LOG_TARGET, err=?e, "Invalid validation code"); // If the validation code is invalid, the candidate certainly is. - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))) - }, + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))); + } }; let raw_block_data = @@ -378,8 +419,10 @@ async fn validate_candidate_exhaustive( tracing::debug!(target: LOG_TARGET, err=?e, "Invalid PoV code"); // If the PoV is invalid, the candidate certainly is. - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure))) - }, + return Ok(Ok(ValidationResult::Invalid( + InvalidCandidate::PoVDecompressionFailure, + ))); + } }; let params = ValidationParams { @@ -404,16 +447,19 @@ async fn validate_candidate_exhaustive( let result = match result { Err(ValidationError::InternalError(e)) => Err(ValidationFailed(e)), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout)) => - Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WorkerReportedError(e))) => - Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e))), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbigiousWorkerDeath)) => + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout)) => { + Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)) + } + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WorkerReportedError(e))) => { + Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e))) + } + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbigiousWorkerDeath)) => { Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError( "ambigious worker death".to_string(), - ))), + ))) + } - Ok(res) => + Ok(res) => { if res.head_data.hash() != descriptor.para_head { Ok(ValidationResult::Invalid(InvalidCandidate::ParaHeadHashMismatch)) } else { @@ -426,7 +472,8 @@ async fn validate_candidate_exhaustive( hrmp_watermark: res.hrmp_watermark, }; Ok(ValidationResult::Valid(outputs, persisted_validation_data)) - }, + } + } }; Ok(result) @@ -461,7 +508,7 @@ impl ValidationBackend for &'_ mut ValidationHost { return Err(ValidationError::InternalError(format!( "cannot send pvf to the validation host: {:?}", err - ))) + ))); } let validation_result = rx @@ -485,19 +532,19 @@ fn perform_basic_checks( let encoded_pov_size = pov.encoded_size(); if encoded_pov_size > max_pov_size as usize { - return Err(InvalidCandidate::ParamsTooLarge(encoded_pov_size as u64)) + return Err(InvalidCandidate::ParamsTooLarge(encoded_pov_size as u64)); } if pov_hash != candidate.pov_hash { - return Err(InvalidCandidate::PoVHashMismatch) + return Err(InvalidCandidate::PoVHashMismatch); } if validation_code_hash != candidate.validation_code_hash { - return Err(InvalidCandidate::CodeHashMismatch) + return Err(InvalidCandidate::CodeHashMismatch); } if let Err(()) = candidate.check_collator_signature() { - return Err(InvalidCandidate::BadSignature) + return Err(InvalidCandidate::BadSignature); } Ok(()) @@ -521,13 +568,13 @@ impl Metrics { match event { Ok(ValidationResult::Valid(_, _)) => { metrics.validation_requests.with_label_values(&["valid"]).inc(); - }, + } Ok(ValidationResult::Invalid(_)) => { metrics.validation_requests.with_label_values(&["invalid"]).inc(); - }, + } Err(_) => { metrics.validation_requests.with_label_values(&["validation failure"]).inc(); - }, + } } } } diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index 9c5cd34f6b95..01370bec21c0 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -38,9 +38,8 @@ fn collator_sign(descriptor: &mut CandidateDescriptor, collator: Sr25519Keyring) } #[test] -fn correctly_checks_included_assumption() { +fn correctly_checks_included_assumption_for_data() { let validation_data: PersistedValidationData = Default::default(); - let validation_code: ValidationCode = vec![1, 2, 3].into(); let persisted_validation_data_hash = validation_data.hash(); let relay_parent = [2; 32].into(); @@ -54,9 +53,12 @@ fn correctly_checks_included_assumption() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = - check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::Included) - .remote_handle(); + let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( + &mut ctx, + &candidate, + OccupiedCoreAssumption::Included, + ) + .remote_handle(); let test_fut = async move { assert_matches!( @@ -76,6 +78,39 @@ fn correctly_checks_included_assumption() { } ); + assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(d, a) => { + assert_eq!(d, validation_data); + assert_eq!(a, OccupiedCoreAssumption::Included); + }); + }; + + let test_fut = future::join(test_fut, check_fut); + executor::block_on(test_fut); +} + +#[test] +fn correctly_checks_included_assumption_for_code() { + let validation_code: ValidationCode = vec![1, 2, 3].into(); + + let relay_parent = [2; 32].into(); + let para_id = 5.into(); + + let mut candidate = CandidateDescriptor::default(); + candidate.relay_parent = relay_parent; + candidate.para_id = para_id; + + let pool = TaskExecutor::new(); + let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + + let (check_code_fut, check_code_result) = + check_assumption_validation_data::<_, ValidationCode>( + &mut ctx, + &candidate, + OccupiedCoreAssumption::Included, + ) + .remote_handle(); + + let test_fut = async move { assert_matches!( ctx_handle.recv().await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( @@ -89,20 +124,19 @@ fn correctly_checks_included_assumption() { } ); - assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(o, v) => { - assert_eq!(o, validation_data); - assert_eq!(v, validation_code); + assert_matches!(check_code_result.await.unwrap(), AssumptionCheckOutcome::Matches(c, a) => { + assert_eq!(c, validation_code); + assert_eq!(a, OccupiedCoreAssumption::Included); }); }; - let test_fut = future::join(test_fut, check_fut); + let test_fut = future::join(test_fut, check_code_fut); executor::block_on(test_fut); } #[test] fn correctly_checks_timed_out_assumption() { let validation_data: PersistedValidationData = Default::default(); - let validation_code: ValidationCode = vec![1, 2, 3].into(); let persisted_validation_data_hash = validation_data.hash(); let relay_parent = [2; 32].into(); @@ -116,9 +150,12 @@ fn correctly_checks_timed_out_assumption() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = - check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::TimedOut) - .remote_handle(); + let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( + &mut ctx, + &candidate, + OccupiedCoreAssumption::TimedOut, + ) + .remote_handle(); let test_fut = async move { assert_matches!( @@ -138,22 +175,9 @@ fn correctly_checks_timed_out_assumption() { } ); - assert_matches!( - ctx_handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - rp, - RuntimeApiRequest::ValidationCode(p, OccupiedCoreAssumption::TimedOut, tx) - )) => { - assert_eq!(rp, relay_parent); - assert_eq!(p, para_id); - - let _ = tx.send(Ok(Some(validation_code.clone()))); - } - ); - - assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(o, v) => { - assert_eq!(o, validation_data); - assert_eq!(v, validation_code); + assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(d, a) => { + assert_eq!(d, validation_data); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); }); }; @@ -176,9 +200,12 @@ fn check_is_bad_request_if_no_validation_data() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = - check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::Included) - .remote_handle(); + let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( + &mut ctx, + &candidate, + OccupiedCoreAssumption::Included, + ) + .remote_handle(); let test_fut = async move { assert_matches!( @@ -220,28 +247,14 @@ fn check_is_bad_request_if_no_validation_code() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = - check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::TimedOut) - .remote_handle(); + let (check_fut, check_result) = check_assumption_validation_data::<_, ValidationCode>( + &mut ctx, + &candidate, + OccupiedCoreAssumption::TimedOut, + ) + .remote_handle(); let test_fut = async move { - assert_matches!( - ctx_handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - rp, - RuntimeApiRequest::PersistedValidationData( - p, - OccupiedCoreAssumption::TimedOut, - tx - ), - )) => { - assert_eq!(rp, relay_parent); - assert_eq!(p, para_id); - - let _ = tx.send(Ok(Some(validation_data.clone()))); - } - ); - assert_matches!( ctx_handle.recv().await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( @@ -276,9 +289,12 @@ fn check_does_not_match() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = - check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::Included) - .remote_handle(); + let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( + &mut ctx, + &candidate, + OccupiedCoreAssumption::Included, + ) + .remote_handle(); let test_fut = async move { assert_matches!( From 2c6a031b522f64c10e1b488fce1604a2b4cea3b6 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Tue, 17 Aug 2021 23:39:46 +0300 Subject: [PATCH 02/26] Try to use cached compiled PVF --- Cargo.lock | 2 + node/core/candidate-validation/Cargo.toml | 1 + node/core/candidate-validation/src/lib.rs | 110 +++++++++++-------- node/core/candidate-validation/src/tests.rs | 49 ++++++--- node/core/pvf/Cargo.toml | 1 + node/core/pvf/src/error.rs | 2 + node/core/pvf/src/host.rs | 112 +++++++++++++------- node/core/pvf/tests/it/main.rs | 3 +- node/primitives/src/lib.rs | 2 + 9 files changed, 185 insertions(+), 97 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index defe3f11307d..1810361e38fb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6289,6 +6289,7 @@ version = "0.9.9" dependencies = [ "assert_matches", "async-trait", + "either", "futures 0.3.16", "parity-scale-codec", "polkadot-node-core-pvf", @@ -6426,6 +6427,7 @@ dependencies = [ "assert_matches", "async-process", "async-std", + "either", "futures 0.3.16", "futures-timer 3.0.2", "hex-literal", diff --git a/node/core/candidate-validation/Cargo.toml b/node/core/candidate-validation/Cargo.toml index 6e388a99466d..9542f7ad187e 100644 --- a/node/core/candidate-validation/Cargo.toml +++ b/node/core/candidate-validation/Cargo.toml @@ -6,6 +6,7 @@ edition = "2018" [dependencies] async-trait = "0.1.42" +either = "1.6" futures = "0.3.15" tracing = "0.1.26" diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index f5acf459f579..f3b46d5fc225 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -38,7 +38,9 @@ use polkadot_node_subsystem::{ SubsystemResult, }; use polkadot_node_subsystem_util::metrics::{self, prometheus}; -use polkadot_parachain::primitives::{ValidationParams, ValidationResult as WasmValidationResult}; +use polkadot_parachain::primitives::{ + ValidationCodeHash, ValidationParams, ValidationResult as WasmValidationResult, +}; use polkadot_primitives::v1::{ CandidateCommitments, CandidateDescriptor, Hash, OccupiedCoreAssumption, PersistedValidationData, ValidationCode, @@ -46,6 +48,7 @@ use polkadot_primitives::v1::{ use parity_scale_codec::Encode; +use either::Either; use futures::{channel::oneshot, prelude::*}; use std::{path::PathBuf, sync::Arc}; @@ -193,7 +196,7 @@ where let res = validate_candidate_exhaustive( &mut validation_host, persisted_validation_data, - validation_code, + Some(validation_code), descriptor, pov, &metrics, @@ -319,7 +322,7 @@ where Context: SubsystemContext, Context: overseer::SubsystemContext, { - let (validation_data, valid_assumption) = + let (validation_data, valid_assumption): (PersistedValidationData, OccupiedCoreAssumption) = match find_assumed_validation_data(ctx, &descriptor, None).await? { AssumptionCheckOutcome::Matches(validation_data, valid_assumption) => { (validation_data, valid_assumption) @@ -335,28 +338,39 @@ where } }; - let validation_code = - match find_assumed_validation_data(ctx, &descriptor, Some(valid_assumption)).await? { - AssumptionCheckOutcome::Matches(validation_code, _) => validation_code, - AssumptionCheckOutcome::DoesNotMatch => { - // Code hash is not validated, should be unreachable. - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))); - } - AssumptionCheckOutcome::BadRequest => { - return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))) - } - }; - - let validation_result = validate_candidate_exhaustive( - validation_host, - validation_data, - validation_code, + let mut validation_result = validate_candidate_exhaustive( + &mut validation_host.clone(), + validation_data.clone(), + None, descriptor.clone(), - pov, + pov.clone(), metrics, ) .await; + if let Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound))) = validation_result { + let validation_code = + match find_assumed_validation_data(ctx, &descriptor, Some(valid_assumption)).await? { + AssumptionCheckOutcome::Matches(validation_code, _) => validation_code, + AssumptionCheckOutcome::DoesNotMatch => { + // Code hash is not validated, should be unreachable. + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))); + } + AssumptionCheckOutcome::BadRequest => { + return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))) + } + }; + validation_result = validate_candidate_exhaustive( + validation_host, + validation_data, + Some(validation_code), + descriptor.clone(), + pov, + metrics, + ) + .await; + } + if let Ok(Ok(ValidationResult::Valid(ref outputs, _))) = validation_result { let (tx, rx) = oneshot::channel(); match runtime_api_request( @@ -383,7 +397,7 @@ where async fn validate_candidate_exhaustive( mut validation_backend: impl ValidationBackend, persisted_validation_data: PersistedValidationData, - validation_code: ValidationCode, + validation_code: Option, descriptor: CandidateDescriptor, pov: Arc, metrics: &Metrics, @@ -394,22 +408,33 @@ async fn validate_candidate_exhaustive( &descriptor, persisted_validation_data.max_pov_size, &*pov, - &validation_code, + validation_code.as_ref(), ) { return Ok(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"); - - // If the validation code is invalid, the candidate certainly is. - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))); - } + let validation_code = if let Some(validation_code) = validation_code { + Either::Left( + 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"); + + // If the validation code is invalid, the candidate certainly is. + return Ok(Ok(ValidationResult::Invalid( + InvalidCandidate::CodeDecompressionFailure, + ))); + } + } + .to_vec(), + ) + } else { + // In case validation code is not provided, ask the backend to obtain + // it from the cache using the hash. + Either::Right(ValidationCodeHash::from(descriptor.persisted_validation_data_hash)) }; let raw_block_data = @@ -432,9 +457,7 @@ 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(), params) - .await; + let result = validation_backend.validate_candidate(validation_code, params).await; if let Err(ref e) = result { tracing::debug!( @@ -458,6 +481,9 @@ async fn validate_candidate_exhaustive( "ambigious worker death".to_string(), ))) } + Err(ValidationError::ArtifactNotFound) => { + Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound)) + } Ok(res) => { if res.head_data.hash() != descriptor.para_head { @@ -483,7 +509,7 @@ async fn validate_candidate_exhaustive( trait ValidationBackend { async fn validate_candidate( &mut self, - raw_validation_code: Vec, + validation_code: Either, ValidationCodeHash>, params: ValidationParams, ) -> Result; } @@ -492,13 +518,13 @@ trait ValidationBackend { impl ValidationBackend for &'_ mut ValidationHost { async fn validate_candidate( &mut self, - raw_validation_code: Vec, + validation_code: Either, ValidationCodeHash>, params: ValidationParams, ) -> Result { let (tx, rx) = oneshot::channel(); if let Err(err) = self .execute_pvf( - Pvf::from_code(raw_validation_code), + validation_code.map_left(Pvf::from_code), params.encode(), polkadot_node_core_pvf::Priority::Normal, tx, @@ -525,10 +551,10 @@ fn perform_basic_checks( candidate: &CandidateDescriptor, max_pov_size: u32, pov: &PoV, - validation_code: &ValidationCode, + validation_code: Option<&ValidationCode>, ) -> Result<(), InvalidCandidate> { let pov_hash = pov.hash(); - let validation_code_hash = validation_code.hash(); + let validation_code_hash = validation_code.map(ValidationCode::hash); let encoded_pov_size = pov.encoded_size(); if encoded_pov_size > max_pov_size as usize { @@ -539,7 +565,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) { return Err(InvalidCandidate::CodeHashMismatch); } diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index 01370bec21c0..4e1664bb8988 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -16,6 +16,7 @@ use super::*; use assert_matches::assert_matches; +use either::Either; use futures::executor; use polkadot_node_subsystem::messages::AllMessages; use polkadot_node_subsystem_test_helpers as test_helpers; @@ -335,7 +336,7 @@ impl MockValidatorBackend { impl ValidationBackend for MockValidatorBackend { async fn validate_candidate( &mut self, - _raw_validation_code: Vec, + _raw_validation_code: Either, ValidationCodeHash>, _params: ValidationParams, ) -> Result { self.result.clone() @@ -356,8 +357,12 @@ fn candidate_validation_ok_is_ok() { descriptor.validation_code_hash = validation_code.hash(); collator_sign(&mut descriptor, Sr25519Keyring::Alice); - let check = - perform_basic_checks(&descriptor, validation_data.max_pov_size, &pov, &validation_code); + let check = perform_basic_checks( + &descriptor, + validation_data.max_pov_size, + &pov, + Some(&validation_code), + ); assert!(check.is_ok()); let validation_result = WasmValidationResult { @@ -372,7 +377,7 @@ fn candidate_validation_ok_is_ok() { let v = executor::block_on(validate_candidate_exhaustive( MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data.clone(), - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), @@ -402,8 +407,12 @@ fn candidate_validation_bad_return_is_invalid() { descriptor.validation_code_hash = validation_code.hash(); collator_sign(&mut descriptor, Sr25519Keyring::Alice); - let check = - perform_basic_checks(&descriptor, validation_data.max_pov_size, &pov, &validation_code); + let check = perform_basic_checks( + &descriptor, + validation_data.max_pov_size, + &pov, + Some(&validation_code), + ); assert!(check.is_ok()); let v = executor::block_on(validate_candidate_exhaustive( @@ -411,7 +420,7 @@ fn candidate_validation_bad_return_is_invalid() { WasmInvalidCandidate::AmbigiousWorkerDeath, ))), validation_data, - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), @@ -434,8 +443,12 @@ fn candidate_validation_timeout_is_internal_error() { descriptor.validation_code_hash = validation_code.hash(); collator_sign(&mut descriptor, Sr25519Keyring::Alice); - let check = - perform_basic_checks(&descriptor, validation_data.max_pov_size, &pov, &validation_code); + let check = perform_basic_checks( + &descriptor, + validation_data.max_pov_size, + &pov, + Some(&validation_code), + ); assert!(check.is_ok()); let v = executor::block_on(validate_candidate_exhaustive( @@ -443,7 +456,7 @@ fn candidate_validation_timeout_is_internal_error() { WasmInvalidCandidate::HardTimeout, ))), validation_data, - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), @@ -465,8 +478,12 @@ fn candidate_validation_code_mismatch_is_invalid() { descriptor.validation_code_hash = ValidationCode(vec![1; 16]).hash(); collator_sign(&mut descriptor, Sr25519Keyring::Alice); - let check = - perform_basic_checks(&descriptor, validation_data.max_pov_size, &pov, &validation_code); + let check = perform_basic_checks( + &descriptor, + validation_data.max_pov_size, + &pov, + Some(&validation_code), + ); assert_matches!(check, Err(InvalidCandidate::CodeHashMismatch)); let v = executor::block_on(validate_candidate_exhaustive( @@ -474,7 +491,7 @@ fn candidate_validation_code_mismatch_is_invalid() { WasmInvalidCandidate::HardTimeout, ))), validation_data, - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), @@ -514,7 +531,7 @@ fn compressed_code_works() { let v = executor::block_on(validate_candidate_exhaustive( MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data, - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), @@ -554,7 +571,7 @@ fn code_decompression_failure_is_invalid() { let v = executor::block_on(validate_candidate_exhaustive( MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data, - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), @@ -595,7 +612,7 @@ fn pov_decompression_failure_is_invalid() { let v = executor::block_on(validate_candidate_exhaustive( MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data, - validation_code, + Some(validation_code), descriptor, Arc::new(pov), &Default::default(), diff --git a/node/core/pvf/Cargo.toml b/node/core/pvf/Cargo.toml index f66b6f3afcc2..477d76190384 100644 --- a/node/core/pvf/Cargo.toml +++ b/node/core/pvf/Cargo.toml @@ -13,6 +13,7 @@ always-assert = "0.1" async-std = { version = "1.8.0", features = ["attributes"] } async-process = "1.1.0" assert_matches = "1.4.0" +either = "1.6" futures = "0.3.15" futures-timer = "3.0.2" libc = "0.2.98" diff --git a/node/core/pvf/src/error.rs b/node/core/pvf/src/error.rs index f0ba95515054..370e7ffce7a0 100644 --- a/node/core/pvf/src/error.rs +++ b/node/core/pvf/src/error.rs @@ -21,6 +21,8 @@ pub enum ValidationError { InvalidCandidate(InvalidCandidate), /// This error is raised due to inability to serve the request. InternalError(String), + /// Provided validation code hash is not present in the artifacts cache. + ArtifactNotFound, } /// A description of an error raised during executing a PVF and can be attributed to the combination diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index 720a8a521729..326a5b8b73f0 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -26,11 +26,12 @@ use crate::{ }; use always_assert::never; use async_std::path::{Path, PathBuf}; +use either::Either; use futures::{ channel::{mpsc, oneshot}, Future, FutureExt, SinkExt, StreamExt, }; -use polkadot_parachain::primitives::ValidationResult; +use polkadot_parachain::primitives::{ValidationCodeHash, ValidationResult}; use std::{ collections::HashMap, time::{Duration, SystemTime}, @@ -55,7 +56,7 @@ impl ValidationHost { /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. pub async fn execute_pvf( &mut self, - pvf: Pvf, + pvf: Either, params: Vec, priority: Priority, result_tx: ResultSender, @@ -81,8 +82,15 @@ impl ValidationHost { } enum ToHost { - ExecutePvf { pvf: Pvf, params: Vec, priority: Priority, result_tx: ResultSender }, - HeadsUp { active_pvfs: Vec }, + ExecutePvf { + pvf: Either, + params: Vec, + priority: Priority, + result_tx: ResultSender, + }, + HeadsUp { + active_pvfs: Vec, + }, } /// Configuration for the validation host. @@ -368,10 +376,10 @@ async fn handle_to_host( result_tx, ) .await?; - }, + } ToHost::HeadsUp { active_pvfs } => { handle_heads_up(artifacts, prepare_queue, active_pvfs).await?; - }, + } } Ok(()) @@ -383,12 +391,15 @@ async fn handle_execute_pvf( prepare_queue: &mut mpsc::Sender, execute_queue: &mut mpsc::Sender, awaiting_prepare: &mut AwaitingPrepare, - pvf: Pvf, + pvf: Either, params: Vec, priority: Priority, result_tx: ResultSender, ) -> Result<(), Fatal> { - let artifact_id = pvf.as_artifact_id(); + let artifact_id = match &pvf { + Either::Left(pvf) => pvf.as_artifact_id(), + Either::Right(hash) => ArtifactId::new(*hash), + }; if let Some(state) = artifacts.artifact_state_mut(&artifact_id) { match state { @@ -404,7 +415,7 @@ async fn handle_execute_pvf( }, ) .await?; - }, + } ArtifactState::Preparing => { send_prepare( prepare_queue, @@ -413,18 +424,23 @@ async fn handle_execute_pvf( .await?; awaiting_prepare.add(artifact_id, params, result_tx); - }, + } } } else { - // Artifact is unknown: register it and enqueue a job with the corresponding priority and - // - artifacts.insert_preparing(artifact_id.clone()); - send_prepare(prepare_queue, prepare::ToQueue::Enqueue { priority, pvf }).await?; + if let Either::Left(pvf) = pvf { + // Artifact is unknown: register it and enqueue a job with the corresponding priority and + // + artifacts.insert_preparing(artifact_id.clone()); + send_prepare(prepare_queue, prepare::ToQueue::Enqueue { priority, pvf }).await?; - awaiting_prepare.add(artifact_id, params, result_tx); + awaiting_prepare.add(artifact_id, params, result_tx); + } else { + // Expect another request with PVF provided. + let _ = result_tx.send(Err(ValidationError::ArtifactNotFound)); + }; } - return Ok(()) + return Ok(()); } async fn handle_heads_up( @@ -440,11 +456,11 @@ async fn handle_heads_up( match state { ArtifactState::Prepared { last_time_needed, .. } => { *last_time_needed = now; - }, + } ArtifactState::Preparing => { // Already preparing. We don't need to send a priority amend either because // it can't get any lower than the background. - }, + } } } else { // The artifact is unknown: register it and put a background job into the prepare queue. @@ -477,8 +493,8 @@ async fn handle_prepare_done( // thus the artifact cannot be unknown, only preparing; // qed. never!("an unknown artifact was prepared: {:?}", artifact_id); - return Ok(()) - }, + return Ok(()); + } Some(ArtifactState::Prepared { .. }) => { // before sending request to prepare, the artifact is inserted with `preparing` state; // the requests are deduplicated for the same artifact id; @@ -486,8 +502,8 @@ async fn handle_prepare_done( // thus the artifact cannot be prepared, only preparing; // qed. never!("the artifact is already prepared: {:?}", artifact_id); - return Ok(()) - }, + return Ok(()); + } Some(state @ ArtifactState::Preparing) => state, }; @@ -499,7 +515,7 @@ async fn handle_prepare_done( 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. - continue + continue; } send_execute( @@ -551,7 +567,7 @@ async fn sweeper_task(mut sweeper_rx: mpsc::Receiver) { None => break, Some(condemned) => { let _ = async_std::fs::remove_file(condemned).await; - }, + } } } } @@ -743,7 +759,7 @@ mod tests { } if let Poll::Ready(r) = futures::poll!(&mut *fut) { - break r + break r; } if futures::poll!(&mut *task).is_ready() { @@ -816,9 +832,14 @@ mod tests { .await; let (result_tx, _result_rx) = oneshot::channel(); - host.execute_pvf(Pvf::from_discriminator(1), vec![], Priority::Critical, result_tx) - .await - .unwrap(); + host.execute_pvf( + Either::Left(Pvf::from_discriminator(1)), + vec![], + Priority::Critical, + result_tx, + ) + .await + .unwrap(); run_until( &mut test.run, @@ -838,13 +859,18 @@ mod tests { let mut host = test.host_handle(); let (result_tx, result_rx_pvf_1_1) = oneshot::channel(); - host.execute_pvf(Pvf::from_discriminator(1), b"pvf1".to_vec(), Priority::Normal, result_tx) - .await - .unwrap(); + host.execute_pvf( + Either::Left(Pvf::from_discriminator(1)), + b"pvf1".to_vec(), + Priority::Normal, + result_tx, + ) + .await + .unwrap(); let (result_tx, result_rx_pvf_1_2) = oneshot::channel(); host.execute_pvf( - Pvf::from_discriminator(1), + Either::Left(Pvf::from_discriminator(1)), b"pvf1".to_vec(), Priority::Critical, result_tx, @@ -853,9 +879,14 @@ mod tests { .unwrap(); let (result_tx, result_rx_pvf_2) = oneshot::channel(); - host.execute_pvf(Pvf::from_discriminator(2), b"pvf2".to_vec(), Priority::Normal, result_tx) - .await - .unwrap(); + host.execute_pvf( + Either::Left(Pvf::from_discriminator(2)), + b"pvf2".to_vec(), + Priority::Normal, + result_tx, + ) + .await + .unwrap(); assert_matches!( test.poll_and_recv_to_prepare_queue().await, @@ -923,9 +954,14 @@ mod tests { let mut host = test.host_handle(); let (result_tx, result_rx) = oneshot::channel(); - host.execute_pvf(Pvf::from_discriminator(1), b"pvf1".to_vec(), Priority::Normal, result_tx) - .await - .unwrap(); + host.execute_pvf( + Either::Left(Pvf::from_discriminator(1)), + b"pvf1".to_vec(), + Priority::Normal, + result_tx, + ) + .await + .unwrap(); assert_matches!( test.poll_and_recv_to_prepare_queue().await, diff --git a/node/core/pvf/tests/it/main.rs b/node/core/pvf/tests/it/main.rs index 57fea2e2ca34..c3330042cf1f 100644 --- a/node/core/pvf/tests/it/main.rs +++ b/node/core/pvf/tests/it/main.rs @@ -15,6 +15,7 @@ // along with Polkadot. If not, see . use async_std::sync::Mutex; +use either::Either; use parity_scale_codec::Encode as _; use polkadot_node_core_pvf::{ start, Config, InvalidCandidate, Pvf, ValidationError, ValidationHost, @@ -63,7 +64,7 @@ impl TestHost { .lock() .await .execute_pvf( - Pvf::from_code(code.into()), + Either::Left(Pvf::from_code(code.into())), params.encode(), polkadot_node_core_pvf::Priority::Normal, result_tx, diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 19ee14f05acf..067ad94d6bb0 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -165,6 +165,8 @@ pub enum InvalidCandidate { ParaHeadHashMismatch, /// Validation code hash does not match. CodeHashMismatch, + /// Validation code not found in cache. + CodeNotFound, } /// Result of the validation of the candidate. From 162be9b976e43ba506dba7d87d49cad18603c32b Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 18 Aug 2021 00:20:09 +0300 Subject: [PATCH 03/26] fmt --- node/core/candidate-validation/src/lib.rs | 101 ++++++++++------------ node/core/pvf/src/host.rs | 26 +++--- 2 files changed, 57 insertions(+), 70 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 5179f395592a..d1de99e2a709 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -154,8 +154,8 @@ where loop { match ctx.recv().await? { - FromOverseer::Signal(OverseerSignal::ActiveLeaves(_)) => {} - FromOverseer::Signal(OverseerSignal::BlockFinalized(..)) => {} + FromOverseer::Signal(OverseerSignal::ActiveLeaves(_)) => {}, + FromOverseer::Signal(OverseerSignal::BlockFinalized(..)) => {}, FromOverseer::Signal(OverseerSignal::Conclude) => return Ok(()), FromOverseer::Communication { msg } => match msg { CandidateValidationMessage::ValidateFromChainState( @@ -178,10 +178,10 @@ where Ok(x) => { metrics.on_validation_event(&x); let _ = response_sender.send(x); - } + }, Err(e) => return Err(e), } - } + }, CandidateValidationMessage::ValidateFromExhaustive( persisted_validation_data, validation_code, @@ -211,10 +211,10 @@ where "Requester of candidate validation dropped", ) } - } + }, Err(e) => return Err(e), } - } + }, }, } } @@ -285,7 +285,7 @@ where // based on the different `OccupiedCoreAssumption`s. if let Some(assumption) = checked_assumption { - return Ok(check_assumption_validation_data(ctx, descriptor, assumption).await?); + return Ok(check_assumption_validation_data(ctx, descriptor, assumption).await?) } const ASSUMPTIONS: &[OccupiedCoreAssumption] = &[ @@ -323,18 +323,16 @@ where { let (validation_data, valid_assumption): (PersistedValidationData, OccupiedCoreAssumption) = match find_assumed_validation_data(ctx, &descriptor, None).await? { - AssumptionCheckOutcome::Matches(validation_data, valid_assumption) => { - (validation_data, valid_assumption) - } + AssumptionCheckOutcome::Matches(validation_data, valid_assumption) => + (validation_data, valid_assumption), AssumptionCheckOutcome::DoesNotMatch => { // 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(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))); - } - AssumptionCheckOutcome::BadRequest => { - return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))) - } + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))) + }, + AssumptionCheckOutcome::BadRequest => + return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), }; let mut validation_result = validate_candidate_exhaustive( @@ -353,11 +351,10 @@ where AssumptionCheckOutcome::Matches(validation_code, _) => validation_code, AssumptionCheckOutcome::DoesNotMatch => { // Code hash is not validated, should be unreachable. - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))); - } - AssumptionCheckOutcome::BadRequest => { - return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))) - } + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))) + }, + AssumptionCheckOutcome::BadRequest => + return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), }; validation_result = validate_candidate_exhaustive( validation_host, @@ -380,13 +377,10 @@ where ) .await? { - Ok(true) => {} - Ok(false) => { - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::InvalidOutputs))) - } - Err(_) => { - return Ok(Err(ValidationFailed("Check Validation Outputs: Bad request".into()))) - } + Ok(true) => {}, + Ok(false) => return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::InvalidOutputs))), + Err(_) => + return Ok(Err(ValidationFailed("Check Validation Outputs: Bad request".into()))), } } @@ -417,7 +411,7 @@ async fn validate_candidate_exhaustive( &*pov, validation_code_hash.as_ref(), ) { - return Ok(Ok(ValidationResult::Invalid(e))); + return Ok(Ok(ValidationResult::Invalid(e))) } let validation_code = if let Some(validation_code) = validation_code { @@ -433,8 +427,8 @@ async fn validate_candidate_exhaustive( // If the validation code is invalid, the candidate certainly is. return Ok(Ok(ValidationResult::Invalid( InvalidCandidate::CodeDecompressionFailure, - ))); - } + ))) + }, } .to_vec(), ) @@ -451,10 +445,8 @@ async fn validate_candidate_exhaustive( tracing::debug!(target: LOG_TARGET, err=?e, "Invalid PoV code"); // If the PoV is invalid, the candidate certainly is. - return Ok(Ok(ValidationResult::Invalid( - InvalidCandidate::PoVDecompressionFailure, - ))); - } + return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure))) + }, }; let params = ValidationParams { @@ -477,22 +469,18 @@ async fn validate_candidate_exhaustive( let result = match result { Err(ValidationError::InternalError(e)) => Err(ValidationFailed(e)), - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout)) => { - Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)) - } - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WorkerReportedError(e))) => { - Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e))) - } - Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbigiousWorkerDeath)) => { + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::HardTimeout)) => + Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WorkerReportedError(e))) => + Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e))), + Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::AmbigiousWorkerDeath)) => Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError( "ambigious worker death".to_string(), - ))) - } - Err(ValidationError::ArtifactNotFound) => { - Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound)) - } + ))), + Err(ValidationError::ArtifactNotFound) => + Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound)), - Ok(res) => { + Ok(res) => if res.head_data.hash() != descriptor.para_head { Ok(ValidationResult::Invalid(InvalidCandidate::ParaHeadHashMismatch)) } else { @@ -505,8 +493,7 @@ async fn validate_candidate_exhaustive( hrmp_watermark: res.hrmp_watermark, }; Ok(ValidationResult::Valid(outputs, persisted_validation_data)) - } - } + }, }; Ok(result) @@ -541,7 +528,7 @@ impl ValidationBackend for &'_ mut ValidationHost { return Err(ValidationError::InternalError(format!( "cannot send pvf to the validation host: {:?}", err - ))); + ))) } let validation_result = rx @@ -564,19 +551,19 @@ fn perform_basic_checks( let encoded_pov_size = pov.encoded_size(); if encoded_pov_size > max_pov_size as usize { - return Err(InvalidCandidate::ParamsTooLarge(encoded_pov_size as u64)); + return Err(InvalidCandidate::ParamsTooLarge(encoded_pov_size as u64)) } if pov_hash != candidate.pov_hash { - return Err(InvalidCandidate::PoVHashMismatch); + return Err(InvalidCandidate::PoVHashMismatch) } if let Some(false) = validation_code_hash.map(|hash| *hash == candidate.validation_code_hash) { - return Err(InvalidCandidate::CodeHashMismatch); + return Err(InvalidCandidate::CodeHashMismatch) } if let Err(()) = candidate.check_collator_signature() { - return Err(InvalidCandidate::BadSignature); + return Err(InvalidCandidate::BadSignature) } Ok(()) @@ -600,13 +587,13 @@ impl Metrics { match event { Ok(ValidationResult::Valid(_, _)) => { metrics.validation_requests.with_label_values(&["valid"]).inc(); - } + }, Ok(ValidationResult::Invalid(_)) => { metrics.validation_requests.with_label_values(&["invalid"]).inc(); - } + }, Err(_) => { metrics.validation_requests.with_label_values(&["validation failure"]).inc(); - } + }, } } } diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index c8eef05cda7a..2f895e683e82 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -376,10 +376,10 @@ async fn handle_to_host( result_tx, ) .await?; - } + }, ToHost::HeadsUp { active_pvfs } => { handle_heads_up(artifacts, prepare_queue, active_pvfs).await?; - } + }, } Ok(()) @@ -415,7 +415,7 @@ async fn handle_execute_pvf( }, ) .await?; - } + }, ArtifactState::Preparing => { send_prepare( prepare_queue, @@ -424,7 +424,7 @@ async fn handle_execute_pvf( .await?; awaiting_prepare.add(artifact_id, params, result_tx); - } + }, } } else { if let Either::Left(pvf) = pvf { @@ -440,7 +440,7 @@ async fn handle_execute_pvf( }; } - return Ok(()); + return Ok(()) } async fn handle_heads_up( @@ -456,11 +456,11 @@ async fn handle_heads_up( match state { ArtifactState::Prepared { last_time_needed, .. } => { *last_time_needed = now; - } + }, ArtifactState::Preparing => { // Already preparing. We don't need to send a priority amend either because // it can't get any lower than the background. - } + }, } } else { // The artifact is unknown: register it and put a background job into the prepare queue. @@ -493,8 +493,8 @@ async fn handle_prepare_done( // thus the artifact cannot be unknown, only preparing; // qed. never!("an unknown artifact was prepared: {:?}", artifact_id); - return Ok(()); - } + return Ok(()) + }, Some(ArtifactState::Prepared { .. }) => { // before sending request to prepare, the artifact is inserted with `preparing` state; // the requests are deduplicated for the same artifact id; @@ -502,8 +502,8 @@ async fn handle_prepare_done( // thus the artifact cannot be prepared, only preparing; // qed. never!("the artifact is already prepared: {:?}", artifact_id); - return Ok(()); - } + return Ok(()) + }, Some(state @ ArtifactState::Preparing) => state, }; @@ -514,7 +514,7 @@ async fn handle_prepare_done( 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. - continue; + continue } send_execute( @@ -778,7 +778,7 @@ mod tests { } if let Poll::Ready(r) = futures::poll!(&mut *fut) { - break r; + break r } if futures::poll!(&mut *task).is_ready() { From 427bc1a4e0918b069632c56a7e862d46a54bb66f Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Thu, 19 Aug 2021 11:32:29 +0300 Subject: [PATCH 04/26] Refactor runtime API requests --- node/core/candidate-validation/src/lib.rs | 145 ++++++++------------ node/core/candidate-validation/src/tests.rs | 101 +++----------- 2 files changed, 79 insertions(+), 167 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index d1de99e2a709..abc2ed9e2f01 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -98,45 +98,6 @@ where } } -trait FromRuntimeApi: Sized { - fn request( - descriptor: &CandidateDescriptor, - assumption: OccupiedCoreAssumption, - ) -> (RuntimeApiRequest, oneshot::Receiver, RuntimeApiError>>); - - fn validate_hash(&self, descriptor: &CandidateDescriptor) -> bool; -} - -impl FromRuntimeApi for ValidationCode { - fn validate_hash(&self, _descriptor: &CandidateDescriptor) -> bool { - // Computing the code hash might be expensive, instead, rely - // on the [`PersistedValidationData`] hash. - true - } - - fn request( - descriptor: &CandidateDescriptor, - assumption: OccupiedCoreAssumption, - ) -> (RuntimeApiRequest, oneshot::Receiver, RuntimeApiError>>) { - let (tx, rx) = oneshot::channel(); - (RuntimeApiRequest::ValidationCode(descriptor.para_id, assumption, tx), rx) - } -} - -impl FromRuntimeApi for PersistedValidationData { - fn validate_hash(&self, descriptor: &CandidateDescriptor) -> bool { - self.hash() == descriptor.persisted_validation_data_hash - } - - fn request( - descriptor: &CandidateDescriptor, - assumption: OccupiedCoreAssumption, - ) -> (RuntimeApiRequest, oneshot::Receiver, RuntimeApiError>>) { - let (tx, rx) = oneshot::channel(); - (RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx), rx) - } -} - async fn run( mut ctx: Context, metrics: Metrics, @@ -236,25 +197,48 @@ where } #[derive(Debug)] -enum AssumptionCheckOutcome { - Matches(T, OccupiedCoreAssumption), +enum AssumptionCheckOutcome { + Matches(PersistedValidationData), DoesNotMatch, BadRequest, } -async fn check_assumption_validation_data( +async fn request_validation_code_by_hash( + ctx: &mut Context, + descriptor: &CandidateDescriptor, +) -> SubsystemResult, RuntimeApiError>> +where + Context: SubsystemContext, + Context: overseer::SubsystemContext, +{ + let (tx, rx) = oneshot::channel(); + runtime_api_request( + ctx, + descriptor.relay_parent, + RuntimeApiRequest::ValidationCodeByHash(descriptor.validation_code_hash, tx), + rx, + ) + .await +} + +async fn check_assumption_validation_data( ctx: &mut Context, descriptor: &CandidateDescriptor, assumption: OccupiedCoreAssumption, -) -> SubsystemResult> +) -> SubsystemResult where Context: SubsystemContext, Context: overseer::SubsystemContext, - T: FromRuntimeApi, { - let data: T = { - let (request, rx) = ::request(descriptor, assumption); - let data = runtime_api_request(ctx, descriptor.relay_parent, request, rx).await?; + let validation_data = { + let (tx, rx) = oneshot::channel(); + let data = runtime_api_request( + ctx, + descriptor.relay_parent, + RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx), + rx, + ) + .await?; match data { Ok(None) | Err(_) => return Ok(AssumptionCheckOutcome::BadRequest), @@ -262,32 +246,30 @@ where } }; - SubsystemResult::Ok(if data.validate_hash(descriptor) { - AssumptionCheckOutcome::Matches(data, assumption) - } else { - AssumptionCheckOutcome::DoesNotMatch - }) + let persisted_validation_data_hash = validation_data.hash(); + + SubsystemResult::Ok( + if descriptor.persisted_validation_data_hash == persisted_validation_data_hash { + AssumptionCheckOutcome::Matches(validation_data) + } else { + AssumptionCheckOutcome::DoesNotMatch + }, + ) } -async fn find_assumed_validation_data( +async fn find_assumed_validation_data( ctx: &mut Context, descriptor: &CandidateDescriptor, - checked_assumption: Option, -) -> SubsystemResult> +) -> SubsystemResult where Context: SubsystemContext, Context: overseer::SubsystemContext, - T: FromRuntimeApi, { // 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. - if let Some(assumption) = checked_assumption { - return Ok(check_assumption_validation_data(ctx, descriptor, assumption).await?) - } - const ASSUMPTIONS: &[OccupiedCoreAssumption] = &[ OccupiedCoreAssumption::Included, OccupiedCoreAssumption::TimedOut, @@ -301,7 +283,7 @@ where let outcome = check_assumption_validation_data(ctx, descriptor, *assumption).await?; match outcome { - AssumptionCheckOutcome::Matches(_, _) => return Ok(outcome), + AssumptionCheckOutcome::Matches(_) => return Ok(outcome), AssumptionCheckOutcome::BadRequest => return Ok(outcome), AssumptionCheckOutcome::DoesNotMatch => continue, } @@ -321,19 +303,17 @@ where Context: SubsystemContext, Context: overseer::SubsystemContext, { - let (validation_data, valid_assumption): (PersistedValidationData, OccupiedCoreAssumption) = - match find_assumed_validation_data(ctx, &descriptor, None).await? { - AssumptionCheckOutcome::Matches(validation_data, valid_assumption) => - (validation_data, valid_assumption), - AssumptionCheckOutcome::DoesNotMatch => { - // 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(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))) - }, - AssumptionCheckOutcome::BadRequest => - return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), - }; + let validation_data = match find_assumed_validation_data(ctx, &descriptor).await? { + AssumptionCheckOutcome::Matches(validation_data) => validation_data, + AssumptionCheckOutcome::DoesNotMatch => { + // 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(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))) + }, + AssumptionCheckOutcome::BadRequest => + return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), + }; let mut validation_result = validate_candidate_exhaustive( &mut validation_host.clone(), @@ -346,16 +326,13 @@ where .await; if let Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound))) = validation_result { - let validation_code = - match find_assumed_validation_data(ctx, &descriptor, Some(valid_assumption)).await? { - AssumptionCheckOutcome::Matches(validation_code, _) => validation_code, - AssumptionCheckOutcome::DoesNotMatch => { - // Code hash is not validated, should be unreachable. - return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent))) - }, - AssumptionCheckOutcome::BadRequest => - return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), - }; + let validation_code = match request_validation_code_by_hash(ctx, &descriptor).await? { + Ok(Some(validation_code)) => validation_code, + Ok(None) => + return Ok(Err(ValidationFailed("Runtime API didn't return validation code".into()))), + Err(err) => return Ok(Err(ValidationFailed(err.to_string()))), + }; + validation_result = validate_candidate_exhaustive( validation_host, validation_data, diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index dd9258929bfa..bd4c0987ac0a 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -54,12 +54,9 @@ fn correctly_checks_included_assumption_for_data() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( - &mut ctx, - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); + let (check_fut, check_result) = + check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::Included) + .remote_handle(); let test_fut = async move { assert_matches!( @@ -79,9 +76,8 @@ fn correctly_checks_included_assumption_for_data() { } ); - assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(d, a) => { + assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(d) => { assert_eq!(d, validation_data); - assert_eq!(a, OccupiedCoreAssumption::Included); }); }; @@ -89,52 +85,6 @@ fn correctly_checks_included_assumption_for_data() { executor::block_on(test_fut); } -#[test] -fn correctly_checks_included_assumption_for_code() { - let validation_code: ValidationCode = vec![1, 2, 3].into(); - - let relay_parent = [2; 32].into(); - let para_id = 5.into(); - - let mut candidate = CandidateDescriptor::default(); - candidate.relay_parent = relay_parent; - candidate.para_id = para_id; - - let pool = TaskExecutor::new(); - let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - - let (check_code_fut, check_code_result) = - check_assumption_validation_data::<_, ValidationCode>( - &mut ctx, - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); - - let test_fut = async move { - assert_matches!( - ctx_handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - rp, - RuntimeApiRequest::ValidationCode(p, OccupiedCoreAssumption::Included, tx) - )) => { - assert_eq!(rp, relay_parent); - assert_eq!(p, para_id); - - let _ = tx.send(Ok(Some(validation_code.clone()))); - } - ); - - assert_matches!(check_code_result.await.unwrap(), AssumptionCheckOutcome::Matches(c, a) => { - assert_eq!(c, validation_code); - assert_eq!(a, OccupiedCoreAssumption::Included); - }); - }; - - let test_fut = future::join(test_fut, check_code_fut); - executor::block_on(test_fut); -} - #[test] fn correctly_checks_timed_out_assumption() { let validation_data: PersistedValidationData = Default::default(); @@ -151,12 +101,9 @@ fn correctly_checks_timed_out_assumption() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( - &mut ctx, - &candidate, - OccupiedCoreAssumption::TimedOut, - ) - .remote_handle(); + let (check_fut, check_result) = + check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::TimedOut) + .remote_handle(); let test_fut = async move { assert_matches!( @@ -176,9 +123,8 @@ fn correctly_checks_timed_out_assumption() { } ); - assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(d, a) => { + assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::Matches(d) => { assert_eq!(d, validation_data); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); }); }; @@ -201,12 +147,9 @@ fn check_is_bad_request_if_no_validation_data() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( - &mut ctx, - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); + let (check_fut, check_result) = + check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::Included) + .remote_handle(); let test_fut = async move { assert_matches!( @@ -248,28 +191,23 @@ fn check_is_bad_request_if_no_validation_code() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data::<_, ValidationCode>( - &mut ctx, - &candidate, - OccupiedCoreAssumption::TimedOut, - ) - .remote_handle(); + let (check_fut, check_result) = + request_validation_code_by_hash(&mut ctx, &candidate).remote_handle(); let test_fut = async move { assert_matches!( ctx_handle.recv().await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( rp, - RuntimeApiRequest::ValidationCode(p, OccupiedCoreAssumption::TimedOut, tx) + RuntimeApiRequest::ValidationCodeByHash(_, tx) )) => { assert_eq!(rp, relay_parent); - assert_eq!(p, para_id); let _ = tx.send(Ok(None)); } ); - assert_matches!(check_result.await.unwrap(), AssumptionCheckOutcome::BadRequest); + assert_matches!(check_result.await.unwrap(), Ok(None)); }; let test_fut = future::join(test_fut, check_fut); @@ -290,12 +228,9 @@ fn check_does_not_match() { let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data::<_, PersistedValidationData>( - &mut ctx, - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); + let (check_fut, check_result) = + check_assumption_validation_data(&mut ctx, &candidate, OccupiedCoreAssumption::Included) + .remote_handle(); let test_fut = async move { assert_matches!( From 7311c270fcfa87793e35276ab32095118b68f5e1 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Thu, 19 Aug 2021 13:13:40 +0300 Subject: [PATCH 05/26] Introduce PvfPreimage --- Cargo.lock | 2 - node/core/candidate-validation/Cargo.toml | 1 - node/core/candidate-validation/src/lib.rs | 92 +++++++++-------- node/core/candidate-validation/src/tests.rs | 3 +- node/core/pvf/Cargo.toml | 1 - node/core/pvf/src/host.rs | 103 ++++++++------------ node/core/pvf/src/prepare/queue.rs | 20 ++-- node/core/pvf/src/pvf.rs | 32 ++++-- node/core/pvf/tests/it/main.rs | 3 +- node/primitives/src/lib.rs | 2 - 10 files changed, 135 insertions(+), 124 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c0f952556b41..77a864ddcd2e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6119,7 +6119,6 @@ version = "0.9.9" dependencies = [ "assert_matches", "async-trait", - "either", "futures 0.3.16", "parity-scale-codec", "polkadot-node-core-pvf", @@ -6257,7 +6256,6 @@ dependencies = [ "assert_matches", "async-process", "async-std", - "either", "futures 0.3.16", "futures-timer 3.0.2", "hex-literal", diff --git a/node/core/candidate-validation/Cargo.toml b/node/core/candidate-validation/Cargo.toml index 9542f7ad187e..6e388a99466d 100644 --- a/node/core/candidate-validation/Cargo.toml +++ b/node/core/candidate-validation/Cargo.toml @@ -6,7 +6,6 @@ edition = "2018" [dependencies] async-trait = "0.1.42" -either = "1.6" futures = "0.3.15" tracing = "0.1.26" diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index abc2ed9e2f01..9555b546d6cf 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -46,7 +46,6 @@ use polkadot_primitives::v1::{ use parity_scale_codec::Encode; -use either::Either; use futures::{channel::oneshot, prelude::*}; use std::{path::PathBuf, sync::Arc}; @@ -325,23 +324,29 @@ where ) .await; - if let Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound))) = validation_result { - let validation_code = match request_validation_code_by_hash(ctx, &descriptor).await? { - Ok(Some(validation_code)) => validation_code, - Ok(None) => - return Ok(Err(ValidationFailed("Runtime API didn't return validation code".into()))), - Err(err) => return Ok(Err(ValidationFailed(err.to_string()))), - }; - - validation_result = validate_candidate_exhaustive( - validation_host, - validation_data, - Some(validation_code), - descriptor.clone(), - pov, - metrics, - ) - .await; + // In case preimage for the supplied code hash was not found by the + // validation host, request the code from Runtime API and try again. + if let Ok(Err(ref err)) = validation_result { + if err.0.as_str() == "Code not found" { + let validation_code = match request_validation_code_by_hash(ctx, &descriptor).await? { + Ok(Some(validation_code)) => validation_code, + Ok(None) => + return Ok(Err(ValidationFailed( + "Runtime API didn't return validation code".into(), + ))), + Err(err) => return Ok(Err(ValidationFailed(err.to_string()))), + }; + + validation_result = validate_candidate_exhaustive( + validation_host, + validation_data, + Some(validation_code), + descriptor.clone(), + pov, + metrics, + ) + .await; + } } if let Ok(Ok(ValidationResult::Valid(ref outputs, _))) = validation_result { @@ -361,6 +366,16 @@ where } } + if let Ok(Err(ref err)) = validation_result { + if err.0.as_str() == "Code not found" { + tracing::error!( + target: LOG_TARGET, + para_id = ?descriptor.para_id, + "Failed to validate candidate: validation code not found in cache" + ); + } + } + validation_result } @@ -392,27 +407,24 @@ async fn validate_candidate_exhaustive( } let validation_code = if let Some(validation_code) = validation_code { - Either::Left( - 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"); - - // If the validation code is invalid, the candidate certainly is. - return Ok(Ok(ValidationResult::Invalid( - InvalidCandidate::CodeDecompressionFailure, - ))) - }, - } - .to_vec(), - ) + 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, "Invalid validation code"); + + // If the validation code is invalid, the candidate certainly is. + return Ok(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. - Either::Right(ValidationCodeHash::from(descriptor.persisted_validation_data_hash)) + Pvf::Hash(ValidationCodeHash::from(descriptor.persisted_validation_data_hash)) }; let raw_block_data = @@ -455,7 +467,7 @@ async fn validate_candidate_exhaustive( "ambigious worker death".to_string(), ))), Err(ValidationError::ArtifactNotFound) => - Ok(ValidationResult::Invalid(InvalidCandidate::CodeNotFound)), + Err(ValidationFailed("Code not found".to_string())), Ok(res) => if res.head_data.hash() != descriptor.para_head { @@ -480,7 +492,7 @@ async fn validate_candidate_exhaustive( trait ValidationBackend { async fn validate_candidate( &mut self, - validation_code: Either, ValidationCodeHash>, + validation_code: Pvf, params: ValidationParams, ) -> Result; } @@ -489,13 +501,13 @@ trait ValidationBackend { impl ValidationBackend for &'_ mut ValidationHost { async fn validate_candidate( &mut self, - validation_code: Either, ValidationCodeHash>, + validation_code: Pvf, params: ValidationParams, ) -> Result { let (tx, rx) = oneshot::channel(); if let Err(err) = self .execute_pvf( - validation_code.map_left(Pvf::from_code), + validation_code, params.encode(), polkadot_node_core_pvf::Priority::Normal, tx, diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index bd4c0987ac0a..326d6550425b 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -16,7 +16,6 @@ use super::*; use assert_matches::assert_matches; -use either::Either; use futures::executor; use polkadot_node_subsystem::messages::AllMessages; use polkadot_node_subsystem_test_helpers as test_helpers; @@ -271,7 +270,7 @@ impl MockValidatorBackend { impl ValidationBackend for MockValidatorBackend { async fn validate_candidate( &mut self, - _raw_validation_code: Either, ValidationCodeHash>, + _raw_validation_code: Pvf, _params: ValidationParams, ) -> Result { self.result.clone() diff --git a/node/core/pvf/Cargo.toml b/node/core/pvf/Cargo.toml index 3efd3555624d..adf0171d8187 100644 --- a/node/core/pvf/Cargo.toml +++ b/node/core/pvf/Cargo.toml @@ -13,7 +13,6 @@ always-assert = "0.1" async-std = { version = "1.8.0", features = ["attributes"] } async-process = "1.1.0" assert_matches = "1.4.0" -either = "1.6" futures = "0.3.15" futures-timer = "3.0.2" libc = "0.2.99" diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index 2f895e683e82..dc95e3f43b0e 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -22,16 +22,17 @@ use crate::{ artifacts::{ArtifactId, ArtifactPathId, ArtifactState, Artifacts}, - execute, prepare, Priority, Pvf, ValidationError, LOG_TARGET, + execute, prepare, + pvf::{Pvf, PvfPreimage}, + Priority, ValidationError, LOG_TARGET, }; use always_assert::never; use async_std::path::{Path, PathBuf}; -use either::Either; use futures::{ channel::{mpsc, oneshot}, Future, FutureExt, SinkExt, StreamExt, }; -use polkadot_parachain::primitives::{ValidationCodeHash, ValidationResult}; +use polkadot_parachain::primitives::ValidationResult; use std::{ collections::HashMap, time::{Duration, SystemTime}, @@ -56,7 +57,7 @@ impl ValidationHost { /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. pub async fn execute_pvf( &mut self, - pvf: Either, + pvf: Pvf, params: Vec, priority: Priority, result_tx: ResultSender, @@ -73,7 +74,7 @@ impl ValidationHost { /// situations this function should return immediately. /// /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. - pub async fn heads_up(&mut self, active_pvfs: Vec) -> Result<(), String> { + pub async fn heads_up(&mut self, active_pvfs: Vec) -> Result<(), String> { self.to_host_tx .send(ToHost::HeadsUp { active_pvfs }) .await @@ -82,15 +83,8 @@ impl ValidationHost { } enum ToHost { - ExecutePvf { - pvf: Either, - params: Vec, - priority: Priority, - result_tx: ResultSender, - }, - HeadsUp { - active_pvfs: Vec, - }, + ExecutePvf { pvf: Pvf, params: Vec, priority: Priority, result_tx: ResultSender }, + HeadsUp { active_pvfs: Vec }, } /// Configuration for the validation host. @@ -391,15 +385,12 @@ async fn handle_execute_pvf( prepare_queue: &mut mpsc::Sender, execute_queue: &mut mpsc::Sender, awaiting_prepare: &mut AwaitingPrepare, - pvf: Either, + pvf: Pvf, params: Vec, priority: Priority, result_tx: ResultSender, ) -> Result<(), Fatal> { - let artifact_id = match &pvf { - Either::Left(pvf) => pvf.as_artifact_id(), - Either::Right(hash) => ArtifactId::new(*hash), - }; + let artifact_id = pvf.as_artifact_id(); if let Some(state) = artifacts.artifact_state_mut(&artifact_id) { match state { @@ -427,11 +418,11 @@ async fn handle_execute_pvf( }, } } else { - if let Either::Left(pvf) = pvf { + if let Pvf::Preimage(inner) = pvf { // Artifact is unknown: register it and enqueue a job with the corresponding priority and // artifacts.insert_preparing(artifact_id.clone()); - send_prepare(prepare_queue, prepare::ToQueue::Enqueue { priority, pvf }).await?; + send_prepare(prepare_queue, prepare::ToQueue::Enqueue { priority, pvf: inner }).await?; awaiting_prepare.add(artifact_id, params, result_tx); } else { @@ -446,7 +437,7 @@ async fn handle_execute_pvf( async fn handle_heads_up( artifacts: &mut Artifacts, prepare_queue: &mut mpsc::Sender, - active_pvfs: Vec, + active_pvfs: Vec, ) -> Result<(), Fatal> { let now = SystemTime::now(); @@ -623,12 +614,12 @@ mod tests { } /// Creates a new PVF which artifact id can be uniquely identified by the given number. - fn artifact_id(descriminator: u32) -> ArtifactId { - Pvf::from_discriminator(descriminator).as_artifact_id() + fn artifact_id(discriminator: u32) -> ArtifactId { + Pvf::from_discriminator(discriminator).as_artifact_id() } - fn artifact_path(descriminator: u32) -> PathBuf { - artifact_id(descriminator).path(&PathBuf::from(std::env::temp_dir())).to_owned() + fn artifact_path(discriminator: u32) -> PathBuf { + artifact_id(discriminator).path(&PathBuf::from(std::env::temp_dir())).to_owned() } struct Builder { @@ -787,6 +778,14 @@ mod tests { } } + /// Creates a new PVF which artifact id can be uniquely identified by the given number. + fn pvf(discriminator: u32) -> PvfPreimage { + match Pvf::from_discriminator(discriminator) { + Pvf::Preimage(inner) => inner, + Pvf::Hash(_) => unreachable!(), + } + } + #[async_std::test] async fn shutdown_on_handle_drop() { let test = Builder::default().build(); @@ -811,7 +810,7 @@ mod tests { let mut test = builder.build(); let mut host = test.host_handle(); - host.heads_up(vec![Pvf::from_discriminator(1)]).await.unwrap(); + host.heads_up(vec![pvf(1)]).await.unwrap(); let to_sweeper_rx = &mut test.to_sweeper_rx; run_until( @@ -825,7 +824,7 @@ mod tests { // Extend TTL for the first artifact and make sure we don't receive another file removal // request. - host.heads_up(vec![Pvf::from_discriminator(1)]).await.unwrap(); + host.heads_up(vec![pvf(1)]).await.unwrap(); test.poll_ensure_to_sweeper_is_empty().await; } @@ -834,7 +833,7 @@ mod tests { let mut test = Builder::default().build(); let mut host = test.host_handle(); - host.heads_up(vec![Pvf::from_discriminator(1)]).await.unwrap(); + host.heads_up(vec![pvf(1)]).await.unwrap(); // Run until we receive a prepare request. let prepare_q_rx = &mut test.to_prepare_queue_rx; @@ -851,14 +850,9 @@ mod tests { .await; let (result_tx, _result_rx) = oneshot::channel(); - host.execute_pvf( - Either::Left(Pvf::from_discriminator(1)), - vec![], - Priority::Critical, - result_tx, - ) - .await - .unwrap(); + host.execute_pvf(Pvf::from_discriminator(1), vec![], Priority::Critical, result_tx) + .await + .unwrap(); run_until( &mut test.run, @@ -878,18 +872,13 @@ mod tests { let mut host = test.host_handle(); let (result_tx, result_rx_pvf_1_1) = oneshot::channel(); - host.execute_pvf( - Either::Left(Pvf::from_discriminator(1)), - b"pvf1".to_vec(), - Priority::Normal, - result_tx, - ) - .await - .unwrap(); + host.execute_pvf(Pvf::from_discriminator(1), b"pvf1".to_vec(), Priority::Normal, result_tx) + .await + .unwrap(); let (result_tx, result_rx_pvf_1_2) = oneshot::channel(); host.execute_pvf( - Either::Left(Pvf::from_discriminator(1)), + Pvf::from_discriminator(1), b"pvf1".to_vec(), Priority::Critical, result_tx, @@ -898,14 +887,9 @@ mod tests { .unwrap(); let (result_tx, result_rx_pvf_2) = oneshot::channel(); - host.execute_pvf( - Either::Left(Pvf::from_discriminator(2)), - b"pvf2".to_vec(), - Priority::Normal, - result_tx, - ) - .await - .unwrap(); + host.execute_pvf(Pvf::from_discriminator(2), b"pvf2".to_vec(), Priority::Normal, result_tx) + .await + .unwrap(); assert_matches!( test.poll_and_recv_to_prepare_queue().await, @@ -973,14 +957,9 @@ mod tests { let mut host = test.host_handle(); let (result_tx, result_rx) = oneshot::channel(); - host.execute_pvf( - Either::Left(Pvf::from_discriminator(1)), - b"pvf1".to_vec(), - Priority::Normal, - result_tx, - ) - .await - .unwrap(); + host.execute_pvf(Pvf::from_discriminator(1), b"pvf1".to_vec(), Priority::Normal, result_tx) + .await + .unwrap(); assert_matches!( test.poll_and_recv_to_prepare_queue().await, diff --git a/node/core/pvf/src/prepare/queue.rs b/node/core/pvf/src/prepare/queue.rs index 7240152a9417..21647f7601e9 100644 --- a/node/core/pvf/src/prepare/queue.rs +++ b/node/core/pvf/src/prepare/queue.rs @@ -17,7 +17,7 @@ //! A queue that handles requests for PVF preparation. use super::pool::{self, Worker}; -use crate::{artifacts::ArtifactId, Priority, Pvf, LOG_TARGET}; +use crate::{artifacts::ArtifactId, pvf::PvfPreimage, Priority, LOG_TARGET}; use always_assert::{always, never}; use async_std::path::PathBuf; use futures::{channel::mpsc, stream::StreamExt as _, Future, SinkExt}; @@ -31,7 +31,7 @@ pub enum ToQueue { /// Note that it is incorrect to enqueue the same PVF again without first receiving the /// [`FromQueue::Prepared`] response. In case there is a need to bump the priority, use /// [`ToQueue::Amend`]. - Enqueue { priority: Priority, pvf: Pvf }, + Enqueue { priority: Priority, pvf: PvfPreimage }, /// Amends the priority for the given [`ArtifactId`] if it is running. If it's not, then it's noop. Amend { priority: Priority, artifact_id: ArtifactId }, } @@ -73,7 +73,7 @@ slotmap::new_key_type! { pub struct Job; } struct JobData { /// The priority of this job. Can be bumped. priority: Priority, - pvf: Pvf, + pvf: PvfPreimage, worker: Option, } @@ -211,7 +211,11 @@ async fn handle_to_queue(queue: &mut Queue, to_queue: ToQueue) -> Result<(), Fat Ok(()) } -async fn handle_enqueue(queue: &mut Queue, priority: Priority, pvf: Pvf) -> Result<(), Fatal> { +async fn handle_enqueue( + queue: &mut Queue, + priority: Priority, + pvf: PvfPreimage, +) -> Result<(), Fatal> { tracing::debug!( target: LOG_TARGET, validation_code_hash = ?pvf.code_hash, @@ -512,14 +516,18 @@ pub fn start( #[cfg(test)] mod tests { use super::*; + use crate::pvf::Pvf; use assert_matches::assert_matches; use futures::{future::BoxFuture, FutureExt}; use slotmap::SlotMap; use std::task::Poll; /// Creates a new PVF which artifact id can be uniquely identified by the given number. - fn pvf(descriminator: u32) -> Pvf { - Pvf::from_discriminator(descriminator) + fn pvf(discriminator: u32) -> PvfPreimage { + match Pvf::from_discriminator(discriminator) { + Pvf::Preimage(inner) => inner, + Pvf::Hash(_) => unreachable!(), + } } async fn run_until( diff --git a/node/core/pvf/src/pvf.rs b/node/core/pvf/src/pvf.rs index 901cc1c70d6e..4461de96348c 100644 --- a/node/core/pvf/src/pvf.rs +++ b/node/core/pvf/src/pvf.rs @@ -23,34 +23,54 @@ use std::{fmt, sync::Arc}; /// /// Should be cheap to clone. #[derive(Clone)] -pub struct Pvf { +pub struct PvfPreimage { pub(crate) code: Arc>, pub(crate) code_hash: ValidationCodeHash, } -impl fmt::Debug for Pvf { +impl fmt::Debug for PvfPreimage { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Pvf {{ code, code_hash: {:?} }}", self.code_hash) } } +impl PvfPreimage { + pub(crate) fn as_artifact_id(&self) -> ArtifactId { + ArtifactId::new(self.code_hash) + } +} + +/// An enum that either contains full preimage of validation function along +/// with its hash (see [`PvfPreimage`]) or the hash only. +#[derive(Clone, Debug)] +pub enum Pvf { + /// Hash-preimage of the validation function, contains both the code + /// and the hash itself. + Preimage(PvfPreimage), + /// Hash of the validation function without its validation code. + Hash(ValidationCodeHash), +} + impl Pvf { /// Returns an instance of the PVF out of the given PVF code. pub fn from_code(code: Vec) -> Self { let code = Arc::new(code); let code_hash = blake2_256(&code).into(); - Self { code, code_hash } + Self::Preimage(PvfPreimage { code, code_hash }) } /// Creates a new PVF which artifact id can be uniquely identified by the given number. #[cfg(test)] pub(crate) fn from_discriminator(num: u32) -> Self { - let descriminator_buf = num.to_le_bytes().to_vec(); - Pvf::from_code(descriminator_buf) + let discriminator_buf = num.to_le_bytes().to_vec(); + Pvf::from_code(discriminator_buf) } /// Returns the artifact ID that corresponds to this PVF. pub(crate) fn as_artifact_id(&self) -> ArtifactId { - ArtifactId::new(self.code_hash) + match self { + Pvf::Preimage(ref inner) => inner.as_artifact_id(), + Pvf::Hash(code_hash) => ArtifactId::new(*code_hash), + } } } diff --git a/node/core/pvf/tests/it/main.rs b/node/core/pvf/tests/it/main.rs index c3330042cf1f..57fea2e2ca34 100644 --- a/node/core/pvf/tests/it/main.rs +++ b/node/core/pvf/tests/it/main.rs @@ -15,7 +15,6 @@ // along with Polkadot. If not, see . use async_std::sync::Mutex; -use either::Either; use parity_scale_codec::Encode as _; use polkadot_node_core_pvf::{ start, Config, InvalidCandidate, Pvf, ValidationError, ValidationHost, @@ -64,7 +63,7 @@ impl TestHost { .lock() .await .execute_pvf( - Either::Left(Pvf::from_code(code.into())), + Pvf::from_code(code.into()), params.encode(), polkadot_node_core_pvf::Priority::Normal, result_tx, diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 067ad94d6bb0..19ee14f05acf 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -165,8 +165,6 @@ pub enum InvalidCandidate { ParaHeadHashMismatch, /// Validation code hash does not match. CodeHashMismatch, - /// Validation code not found in cache. - CodeNotFound, } /// Result of the validation of the candidate. From 094d1647415f82c0a79c6496cb9041cae23a8019 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Thu, 19 Aug 2021 17:44:02 +0300 Subject: [PATCH 06/26] Reliable error handling --- node/core/candidate-validation/src/lib.rs | 107 ++++++++++++-------- node/core/candidate-validation/src/tests.rs | 28 +++++ node/core/pvf/src/lib.rs | 2 +- 3 files changed, 91 insertions(+), 46 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 9555b546d6cf..13a17af3e1f5 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -152,6 +152,7 @@ where let _timer = metrics.time_validate_from_exhaustive(); let res = validate_candidate_exhaustive( + &mut ctx, &mut validation_host, persisted_validation_data, Some(validation_code), @@ -314,41 +315,17 @@ where return Ok(Err(ValidationFailed("Assumption Check: Bad request".into()))), }; - let mut validation_result = validate_candidate_exhaustive( - &mut validation_host.clone(), - validation_data.clone(), + let validation_result = validate_candidate_exhaustive( + ctx, + validation_host, + validation_data, None, descriptor.clone(), - pov.clone(), + pov, metrics, ) .await; - // In case preimage for the supplied code hash was not found by the - // validation host, request the code from Runtime API and try again. - if let Ok(Err(ref err)) = validation_result { - if err.0.as_str() == "Code not found" { - let validation_code = match request_validation_code_by_hash(ctx, &descriptor).await? { - Ok(Some(validation_code)) => validation_code, - Ok(None) => - return Ok(Err(ValidationFailed( - "Runtime API didn't return validation code".into(), - ))), - Err(err) => return Ok(Err(ValidationFailed(err.to_string()))), - }; - - validation_result = validate_candidate_exhaustive( - validation_host, - validation_data, - Some(validation_code), - descriptor.clone(), - pov, - metrics, - ) - .await; - } - } - if let Ok(Ok(ValidationResult::Valid(ref outputs, _))) = validation_result { let (tx, rx) = oneshot::channel(); match runtime_api_request( @@ -366,27 +343,22 @@ where } } - if let Ok(Err(ref err)) = validation_result { - if err.0.as_str() == "Code not found" { - tracing::error!( - target: LOG_TARGET, - para_id = ?descriptor.para_id, - "Failed to validate candidate: validation code not found in cache" - ); - } - } - validation_result } -async fn validate_candidate_exhaustive( +async fn validate_candidate_exhaustive( + ctx: &mut Context, mut validation_backend: impl ValidationBackend, persisted_validation_data: PersistedValidationData, validation_code: Option, descriptor: CandidateDescriptor, pov: Arc, metrics: &Metrics, -) -> SubsystemResult> { +) -> SubsystemResult> +where + Context: SubsystemContext, + Context: overseer::SubsystemContext, +{ let _timer = metrics.time_validate_candidate_exhaustive(); let validation_code_hash = validation_code.as_ref().map(ValidationCode::hash); @@ -445,7 +417,7 @@ async fn validate_candidate_exhaustive( relay_parent_storage_root: persisted_validation_data.relay_parent_storage_root, }; - let result = validation_backend.validate_candidate(validation_code, params).await; + let mut result = validation_backend.validate_candidate(validation_code, params.clone()).await; if let Err(ref e) = result { tracing::debug!( @@ -453,6 +425,43 @@ async fn validate_candidate_exhaustive( error = ?e, "Failed to validate candidate", ); + + // In case preimage for the supplied code hash was not found by the + // validation host, request the code from Runtime API and try again. + if let &ValidationError::ArtifactNotFound = e { + tracing::debug!( + target: LOG_TARGET, + "Validation host failed to find artifact by provided hash", + ); + + let validation_code = match request_validation_code_by_hash(ctx, &descriptor).await? { + Ok(Some(validation_code)) => validation_code, + Ok(None) => + return Ok(Err(ValidationFailed( + "Runtime API didn't return validation code by hash".into(), + ))), + Err(err) => return Ok(Err(ValidationFailed(err.to_string()))), + }; + + let validation_code = Pvf::from_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"); + + // If the validation code is invalid, the candidate certainly is. + return Ok(Ok(ValidationResult::Invalid( + InvalidCandidate::CodeDecompressionFailure, + ))) + }, + } + .to_vec(), + ); + result = validation_backend.validate_candidate(validation_code, params).await; + } } let result = match result { @@ -466,9 +475,17 @@ async fn validate_candidate_exhaustive( Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError( "ambigious worker death".to_string(), ))), - Err(ValidationError::ArtifactNotFound) => - Err(ValidationFailed("Code not found".to_string())), - + Err(ValidationError::ArtifactNotFound) => { + // The code was supplied on the second attempt, this + // error should be unreachable. + tracing::error!( + target: LOG_TARGET, + "Unexpected error received from the validation host" + ); + Err(ValidationFailed( + "Validation host failed to find artifact even though it was supplied".to_string(), + )) + }, Ok(res) => if res.head_data.hash() != descriptor.para_head { Ok(ValidationResult::Invalid(InvalidCandidate::ParaHeadHashMismatch)) diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index 326d6550425b..b0ca40c1d079 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -279,6 +279,9 @@ impl ValidationBackend for MockValidatorBackend { #[test] fn candidate_validation_ok_is_ok() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -309,6 +312,7 @@ fn candidate_validation_ok_is_ok() { }; let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data.clone(), Some(validation_code), @@ -331,6 +335,9 @@ fn candidate_validation_ok_is_ok() { #[test] fn candidate_validation_bad_return_is_invalid() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -350,6 +357,7 @@ fn candidate_validation_bad_return_is_invalid() { assert!(check.is_ok()); let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Err(ValidationError::InvalidCandidate( WasmInvalidCandidate::AmbigiousWorkerDeath, ))), @@ -367,6 +375,9 @@ fn candidate_validation_bad_return_is_invalid() { #[test] fn candidate_validation_timeout_is_internal_error() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -386,6 +397,7 @@ fn candidate_validation_timeout_is_internal_error() { assert!(check.is_ok()); let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Err(ValidationError::InvalidCandidate( WasmInvalidCandidate::HardTimeout, ))), @@ -402,6 +414,9 @@ fn candidate_validation_timeout_is_internal_error() { #[test] fn candidate_validation_code_mismatch_is_invalid() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; @@ -421,6 +436,7 @@ fn candidate_validation_code_mismatch_is_invalid() { assert_matches!(check, Err(InvalidCandidate::CodeHashMismatch)); let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Err(ValidationError::InvalidCandidate( WasmInvalidCandidate::HardTimeout, ))), @@ -438,6 +454,9 @@ fn candidate_validation_code_mismatch_is_invalid() { #[test] fn compressed_code_works() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; let head_data = HeadData(vec![1, 1, 1]); @@ -463,6 +482,7 @@ fn compressed_code_works() { }; let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data, Some(validation_code), @@ -477,6 +497,9 @@ fn compressed_code_works() { #[test] fn code_decompression_failure_is_invalid() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; let pov = PoV { block_data: BlockData(vec![1; 32]) }; let head_data = HeadData(vec![1, 1, 1]); @@ -503,6 +526,7 @@ fn code_decompression_failure_is_invalid() { }; let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data, Some(validation_code), @@ -517,6 +541,9 @@ fn code_decompression_failure_is_invalid() { #[test] fn pov_decompression_failure_is_invalid() { + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + let validation_data = PersistedValidationData { max_pov_size: POV_BOMB_LIMIT as u32, ..Default::default() }; let head_data = HeadData(vec![1, 1, 1]); @@ -544,6 +571,7 @@ fn pov_decompression_failure_is_invalid() { }; let v = executor::block_on(validate_candidate_exhaustive( + &mut ctx, MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), validation_data, Some(validation_code), diff --git a/node/core/pvf/src/lib.rs b/node/core/pvf/src/lib.rs index bd72a5e1ed08..08c4f2617f66 100644 --- a/node/core/pvf/src/lib.rs +++ b/node/core/pvf/src/lib.rs @@ -93,7 +93,7 @@ pub use sp_tracing; pub use error::{InvalidCandidate, ValidationError}; pub use priority::Priority; -pub use pvf::Pvf; +pub use pvf::{Pvf, PvfPreimage}; pub use host::{start, Config, ValidationHost}; From 50fcfcac638c3eb0720127824b5cc2020fdc759b Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 23 Aug 2021 00:32:10 +0300 Subject: [PATCH 07/26] Improve candidate validation readability --- node/core/candidate-validation/src/lib.rs | 45 ++++++++++------------- node/core/pvf/src/host.rs | 2 +- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 13a17af3e1f5..5f7ee1392ab3 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -385,7 +385,7 @@ where ) { Ok(code) => code, Err(e) => { - tracing::debug!(target: LOG_TARGET, err=?e, "Invalid validation code"); + tracing::debug!(target: LOG_TARGET, err=?e, "Code decompression failed"); // If the validation code is invalid, the candidate certainly is. return Ok(Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure))) @@ -443,23 +443,21 @@ where Err(err) => return Ok(Err(ValidationFailed(err.to_string()))), }; - let validation_code = Pvf::from_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"); - - // If the validation code is invalid, the candidate certainly is. - return Ok(Ok(ValidationResult::Invalid( - InvalidCandidate::CodeDecompressionFailure, - ))) - }, - } - .to_vec(), - ); + 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(Ok(ValidationResult::Invalid( + InvalidCandidate::CodeDecompressionFailure, + ))) + }, + }; + let validation_code = Pvf::from_code(raw_code.to_vec()); result = validation_backend.validate_candidate(validation_code, params).await; } } @@ -478,13 +476,10 @@ where Err(ValidationError::ArtifactNotFound) => { // The code was supplied on the second attempt, this // error should be unreachable. - tracing::error!( - target: LOG_TARGET, - "Unexpected error received from the validation host" - ); - Err(ValidationFailed( - "Validation host failed to find artifact even though it was supplied".to_string(), - )) + let error_message = + "Validation host failed to find artifact even though it was supplied"; + tracing::error!(target: LOG_TARGET, error_message,); + Err(ValidationFailed(error_message.to_string())) }, Ok(res) => if res.head_data.hash() != descriptor.para_head { diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index dc95e3f43b0e..6bf9b5a63a11 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -428,7 +428,7 @@ async fn handle_execute_pvf( } else { // Expect another request with PVF provided. let _ = result_tx.send(Err(ValidationError::ArtifactNotFound)); - }; + } } return Ok(()) From 2c39c3783cff26693715ae9d3d9a4a439be0bd61 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 23 Aug 2021 00:33:17 +0300 Subject: [PATCH 08/26] Send correct hash to the PVF host --- 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 5f7ee1392ab3..21e98186c461 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -396,7 +396,7 @@ where } 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.persisted_validation_data_hash)) + Pvf::Hash(ValidationCodeHash::from(descriptor.validation_code_hash)) }; let raw_block_data = From c7cf9f5fc5970ace8ade57761ca18c30b00fd657 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 23 Aug 2021 11:16:38 +0300 Subject: [PATCH 09/26] Test validation by hash feature --- node/core/candidate-validation/src/tests.rs | 69 ++++++++++++++++++++- 1 file changed, 67 insertions(+), 2 deletions(-) diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index b0ca40c1d079..4eb36a0c71d1 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -270,13 +270,78 @@ impl MockValidatorBackend { impl ValidationBackend for MockValidatorBackend { async fn validate_candidate( &mut self, - _raw_validation_code: Pvf, + raw_validation_code: Pvf, _params: ValidationParams, ) -> Result { - self.result.clone() + match raw_validation_code { + Pvf::Preimage(_) => self.result.clone(), + Pvf::Hash(_) => Err(ValidationError::ArtifactNotFound), + } } } +#[test] +fn check_runtime_validation_code_request() { + let metrics = Default::default(); + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; + + let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let validation_code = ValidationCode(vec![2; 16]); + let validation_code_hash = validation_code.hash(); + + let head_data = HeadData(vec![1, 1, 1]); + let mut descriptor = CandidateDescriptor::default(); + descriptor.pov_hash = pov.hash(); + descriptor.para_head = head_data.hash(); + descriptor.validation_code_hash = validation_code_hash; + collator_sign(&mut descriptor, Sr25519Keyring::Alice); + + let validation_result = WasmValidationResult { + head_data, + new_validation_code: Some(vec![2, 2, 2].into()), + upward_messages: Vec::new(), + horizontal_messages: Vec::new(), + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + + let pool = TaskExecutor::new(); + let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context(pool.clone()); + + let (check_fut, check_result) = validate_candidate_exhaustive( + &mut ctx, + MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), + validation_data.clone(), + None, + descriptor, + Arc::new(pov), + &metrics, + ) + .remote_handle(); + + let test_fut = async move { + assert_matches!( + ctx_handle.recv().await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _, + RuntimeApiRequest::ValidationCodeByHash( + validation_code_hash, + tx + ), + )) => { + assert_eq!(validation_code_hash, validation_code.hash()); + + let _ = tx.send(Ok(Some(validation_code))); + } + ); + + assert_matches!(check_result.await.unwrap(), Ok(ValidationResult::Valid(_, _))); + }; + + let test_fut = future::join(test_fut, check_fut); + executor::block_on(test_fut); +} + #[test] fn candidate_validation_ok_is_ok() { let pool = TaskExecutor::new(); From 2d0d047a365cb3ee9e4b0654dab8f6533b595c10 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 30 Aug 2021 11:54:31 +0300 Subject: [PATCH 10/26] Remove extra comma --- 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 a0de70434ca6..454f8f42f455 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -490,7 +490,7 @@ where // error should be unreachable. let error_message = "Validation host failed to find artifact even though it was supplied"; - tracing::error!(target: LOG_TARGET, error_message,); + tracing::error!(target: LOG_TARGET, error_message); Err(ValidationFailed(error_message.to_string())) }, Ok(res) => From 828af049e2a12f34905f262338487a9ee4eb6554 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 25 Oct 2021 19:26:56 +0300 Subject: [PATCH 11/26] Rename --- node/core/pvf/src/host.rs | 10 +++++----- node/core/pvf/src/lib.rs | 2 +- node/core/pvf/src/prepare/queue.rs | 14 +++++--------- node/core/pvf/src/pvf.rs | 12 ++++++------ 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index 1bc6a007d521..4479e72727c8 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -25,7 +25,7 @@ use crate::{ execute, metrics::Metrics, prepare, - pvf::{Pvf, PvfPreimage}, + pvf::{Pvf, PvfCode}, Priority, ValidationError, LOG_TARGET, }; use always_assert::never; @@ -76,7 +76,7 @@ impl ValidationHost { /// situations this function should return immediately. /// /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. - pub async fn heads_up(&mut self, active_pvfs: Vec) -> Result<(), String> { + pub async fn heads_up(&mut self, active_pvfs: Vec) -> Result<(), String> { self.to_host_tx .send(ToHost::HeadsUp { active_pvfs }) .await @@ -86,7 +86,7 @@ impl ValidationHost { enum ToHost { ExecutePvf { pvf: Pvf, params: Vec, priority: Priority, result_tx: ResultSender }, - HeadsUp { active_pvfs: Vec }, + HeadsUp { active_pvfs: Vec }, } /// Configuration for the validation host. @@ -442,7 +442,7 @@ async fn handle_execute_pvf( async fn handle_heads_up( artifacts: &mut Artifacts, prepare_queue: &mut mpsc::Sender, - active_pvfs: Vec, + active_pvfs: Vec, ) -> Result<(), Fatal> { let now = SystemTime::now(); @@ -784,7 +784,7 @@ mod tests { } /// Creates a new PVF which artifact id can be uniquely identified by the given number. - fn pvf(discriminator: u32) -> PvfPreimage { + fn pvf(discriminator: u32) -> PvfCode { match Pvf::from_discriminator(discriminator) { Pvf::Preimage(inner) => inner, Pvf::Hash(_) => unreachable!(), diff --git a/node/core/pvf/src/lib.rs b/node/core/pvf/src/lib.rs index 3f645dbfdd3f..6e04e79e3fc7 100644 --- a/node/core/pvf/src/lib.rs +++ b/node/core/pvf/src/lib.rs @@ -94,7 +94,7 @@ pub use sp_tracing; pub use error::{InvalidCandidate, ValidationError}; pub use priority::Priority; -pub use pvf::{Pvf, PvfPreimage}; +pub use pvf::{Pvf, PvfCode}; pub use host::{start, Config, ValidationHost}; pub use metrics::Metrics; diff --git a/node/core/pvf/src/prepare/queue.rs b/node/core/pvf/src/prepare/queue.rs index d8b61c616230..2cfecdfbdffc 100644 --- a/node/core/pvf/src/prepare/queue.rs +++ b/node/core/pvf/src/prepare/queue.rs @@ -17,7 +17,7 @@ //! A queue that handles requests for PVF preparation. use super::pool::{self, Worker}; -use crate::{artifacts::ArtifactId, metrics::Metrics, pvf::PvfPreimage, Priority, LOG_TARGET}; +use crate::{artifacts::ArtifactId, metrics::Metrics, pvf::PvfCode, Priority, LOG_TARGET}; use always_assert::{always, never}; use async_std::path::PathBuf; use futures::{channel::mpsc, stream::StreamExt as _, Future, SinkExt}; @@ -31,7 +31,7 @@ pub enum ToQueue { /// Note that it is incorrect to enqueue the same PVF again without first receiving the /// [`FromQueue::Prepared`] response. In case there is a need to bump the priority, use /// [`ToQueue::Amend`]. - Enqueue { priority: Priority, pvf: PvfPreimage }, + Enqueue { priority: Priority, pvf: PvfCode }, /// Amends the priority for the given [`ArtifactId`] if it is running. If it's not, then it's noop. Amend { priority: Priority, artifact_id: ArtifactId }, } @@ -73,7 +73,7 @@ slotmap::new_key_type! { pub struct Job; } struct JobData { /// The priority of this job. Can be bumped. priority: Priority, - pvf: PvfPreimage, + pvf: PvfCode, worker: Option, } @@ -215,11 +215,7 @@ async fn handle_to_queue(queue: &mut Queue, to_queue: ToQueue) -> Result<(), Fat Ok(()) } -async fn handle_enqueue( - queue: &mut Queue, - priority: Priority, - pvf: PvfPreimage, -) -> Result<(), Fatal> { +async fn handle_enqueue(queue: &mut Queue, priority: Priority, pvf: PvfCode) -> Result<(), Fatal> { tracing::debug!( target: LOG_TARGET, validation_code_hash = ?pvf.code_hash, @@ -532,7 +528,7 @@ mod tests { use std::task::Poll; /// Creates a new PVF which artifact id can be uniquely identified by the given number. - fn pvf(discriminator: u32) -> PvfPreimage { + fn pvf(discriminator: u32) -> PvfCode { match Pvf::from_discriminator(discriminator) { Pvf::Preimage(inner) => inner, Pvf::Hash(_) => unreachable!(), diff --git a/node/core/pvf/src/pvf.rs b/node/core/pvf/src/pvf.rs index 4461de96348c..5d8cc9049405 100644 --- a/node/core/pvf/src/pvf.rs +++ b/node/core/pvf/src/pvf.rs @@ -23,30 +23,30 @@ use std::{fmt, sync::Arc}; /// /// Should be cheap to clone. #[derive(Clone)] -pub struct PvfPreimage { +pub struct PvfCode { pub(crate) code: Arc>, pub(crate) code_hash: ValidationCodeHash, } -impl fmt::Debug for PvfPreimage { +impl fmt::Debug for PvfCode { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Pvf {{ code, code_hash: {:?} }}", self.code_hash) } } -impl PvfPreimage { +impl PvfCode { pub(crate) fn as_artifact_id(&self) -> ArtifactId { ArtifactId::new(self.code_hash) } } /// An enum that either contains full preimage of validation function along -/// with its hash (see [`PvfPreimage`]) or the hash only. +/// with its hash (see [`PvfCode`]) or the hash only. #[derive(Clone, Debug)] pub enum Pvf { /// Hash-preimage of the validation function, contains both the code /// and the hash itself. - Preimage(PvfPreimage), + Preimage(PvfCode), /// Hash of the validation function without its validation code. Hash(ValidationCodeHash), } @@ -56,7 +56,7 @@ impl Pvf { pub fn from_code(code: Vec) -> Self { let code = Arc::new(code); let code_hash = blake2_256(&code).into(); - Self::Preimage(PvfPreimage { code, code_hash }) + Self::Preimage(PvfCode { code, code_hash }) } /// Creates a new PVF which artifact id can be uniquely identified by the given number. From 8da537a942cded53e088c3bf0c6c876797f5e531 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 25 Oct 2021 22:13:04 +0300 Subject: [PATCH 12/26] Rework candidate validation to use new runtime API endpoint --- node/core/candidate-validation/src/lib.rs | 121 +++------ node/core/candidate-validation/src/tests.rs | 272 +++++++++++--------- 2 files changed, 190 insertions(+), 203 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 7185ef7be7e2..2ee639595840 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -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; @@ -238,13 +238,6 @@ where }) } -#[derive(Debug)] -enum AssumptionCheckOutcome { - Matches(PersistedValidationData), - DoesNotMatch, - BadRequest, -} - async fn request_validation_code_by_hash( sender: &mut Sender, descriptor: &CandidateDescriptor, @@ -262,76 +255,33 @@ where .await } -async fn check_assumption_validation_data( +async fn request_assumed_validation_data( sender: &mut Sender, descriptor: &CandidateDescriptor, - assumption: OccupiedCoreAssumption, -) -> AssumptionCheckOutcome +) -> Result< + Option<(PersistedValidationData, ValidationCodeHash)>, + RuntimeRequestFailed, +> where Sender: SubsystemSender, { - let validation_data = { - let (tx, rx) = oneshot::channel(); - let data = runtime_api_request( - sender, - descriptor.relay_parent, - RuntimeApiRequest::PersistedValidationData(descriptor.para_id, assumption, tx), - rx, - ) - .await; - - match data { - Ok(None) | Err(RuntimeRequestFailed) => return AssumptionCheckOutcome::BadRequest, - Ok(Some(data)) => data, - } - }; - - let persisted_validation_data_hash = validation_data.hash(); - - if descriptor.persisted_validation_data_hash == persisted_validation_data_hash { - AssumptionCheckOutcome::Matches(validation_data) - } else { - AssumptionCheckOutcome::DoesNotMatch - } -} - -async fn find_assumed_validation_data( - sender: &mut Sender, - descriptor: &CandidateDescriptor, -) -> AssumptionCheckOutcome -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: &mut Sender, - validation_host: ValidationHost, + validation_host: impl ValidationBackend, descriptor: CandidateDescriptor, pov: Arc, timeout: Duration, @@ -340,17 +290,25 @@ async fn validate_from_chain_state( where Sender: SubsystemSender, { - let validation_data = match find_assumed_validation_data(sender, &descriptor).await { - AssumptionCheckOutcome::Matches(validation_data) => validation_data, - AssumptionCheckOutcome::DoesNotMatch => { - // 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())), - }; + 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)) + }, + 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, @@ -477,6 +435,7 @@ where 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? return Err(ValidationFailed( "Runtime API didn't return validation code by hash".into(), )), diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index b251a1fc58d7..4ae662d4ebbf 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -39,7 +39,7 @@ fn collator_sign(descriptor: &mut CandidateDescriptor, collator: Sr25519Keyring) } #[test] -fn correctly_checks_included_assumption_for_data() { +fn correctly_finds_assumed_validation_data() { let validation_data: PersistedValidationData = Default::default(); let persisted_validation_data_hash = validation_data.hash(); @@ -55,83 +55,29 @@ fn correctly_checks_included_assumption_for_data() { let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data( - ctx.sender(), - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); - - let test_fut = async move { - assert_matches!( - ctx_handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - rp, - RuntimeApiRequest::PersistedValidationData( - p, - OccupiedCoreAssumption::Included, - tx - ), - )) => { - assert_eq!(rp, relay_parent); - assert_eq!(p, para_id); - - let _ = tx.send(Ok(Some(validation_data.clone()))); - } - ); - - assert_matches!(check_result.await, AssumptionCheckOutcome::Matches(d) => { - assert_eq!(d, validation_data); - }); - }; - - let test_fut = future::join(test_fut, check_fut); - executor::block_on(test_fut); -} - -#[test] -fn correctly_checks_timed_out_assumption() { - let validation_data: PersistedValidationData = Default::default(); - - let persisted_validation_data_hash = validation_data.hash(); - let relay_parent = [2; 32].into(); - let para_id = 5.into(); - - let mut candidate = CandidateDescriptor::default(); - candidate.relay_parent = relay_parent; - candidate.persisted_validation_data_hash = persisted_validation_data_hash; - candidate.para_id = para_id; - - let pool = TaskExecutor::new(); - let (mut ctx, mut ctx_handle) = - test_helpers::make_subsystem_context::(pool.clone()); - - let (check_fut, check_result) = check_assumption_validation_data( - ctx.sender(), - &candidate, - OccupiedCoreAssumption::TimedOut, - ) - .remote_handle(); + let (check_fut, check_result) = + request_assumed_validation_data(ctx.sender(), &candidate).remote_handle(); let test_fut = async move { assert_matches!( ctx_handle.recv().await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( rp, - RuntimeApiRequest::PersistedValidationData( + RuntimeApiRequest::AssumedValidationData( p, - OccupiedCoreAssumption::TimedOut, + hash, tx ), )) => { assert_eq!(rp, relay_parent); assert_eq!(p, para_id); + assert_eq!(hash, persisted_validation_data_hash); - let _ = tx.send(Ok(Some(validation_data.clone()))); + let _ = tx.send(Ok(Some((validation_data.clone(), Default::default())))); } ); - assert_matches!(check_result.await, AssumptionCheckOutcome::Matches(d) => { + assert_matches!(check_result.await, Ok(Some((d, _))) => { assert_eq!(d, validation_data); }); }; @@ -141,7 +87,7 @@ fn correctly_checks_timed_out_assumption() { } #[test] -fn check_is_bad_request_if_no_validation_data() { +fn check_is_none_if_no_validation_data() { let validation_data: PersistedValidationData = Default::default(); let persisted_validation_data_hash = validation_data.hash(); let relay_parent = [2; 32].into(); @@ -156,21 +102,17 @@ fn check_is_bad_request_if_no_validation_data() { let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data( - ctx.sender(), - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); + let (check_fut, check_result) = + request_assumed_validation_data(ctx.sender(), &candidate).remote_handle(); let test_fut = async move { assert_matches!( ctx_handle.recv().await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( rp, - RuntimeApiRequest::PersistedValidationData( + RuntimeApiRequest::AssumedValidationData( p, - OccupiedCoreAssumption::Included, + _, tx ), )) => { @@ -181,45 +123,6 @@ fn check_is_bad_request_if_no_validation_data() { } ); - assert_matches!(check_result.await, AssumptionCheckOutcome::BadRequest); - }; - - let test_fut = future::join(test_fut, check_fut); - executor::block_on(test_fut); -} - -#[test] -fn check_is_bad_request_if_no_validation_code() { - let validation_data: PersistedValidationData = Default::default(); - let persisted_validation_data_hash = validation_data.hash(); - let relay_parent = [2; 32].into(); - let para_id = 5.into(); - - let mut candidate = CandidateDescriptor::default(); - candidate.relay_parent = relay_parent; - candidate.persisted_validation_data_hash = persisted_validation_data_hash; - candidate.para_id = para_id; - - let pool = TaskExecutor::new(); - let (mut ctx, mut ctx_handle) = - test_helpers::make_subsystem_context::(pool.clone()); - - let (check_fut, check_result) = - request_validation_code_by_hash(ctx.sender(), &candidate).remote_handle(); - - let test_fut = async move { - assert_matches!( - ctx_handle.recv().await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - rp, - RuntimeApiRequest::ValidationCodeByHash(_, tx) - )) => { - assert_eq!(rp, relay_parent); - - let _ = tx.send(Ok(None)); - } - ); - assert_matches!(check_result.await, Ok(None)); }; @@ -228,8 +131,7 @@ fn check_is_bad_request_if_no_validation_code() { } #[test] -fn check_does_not_match() { - let validation_data: PersistedValidationData = Default::default(); +fn check_runtime_api_error() { let relay_parent = [2; 32].into(); let para_id = 5.into(); @@ -242,32 +144,28 @@ fn check_does_not_match() { let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); - let (check_fut, check_result) = check_assumption_validation_data( - ctx.sender(), - &candidate, - OccupiedCoreAssumption::Included, - ) - .remote_handle(); + let (check_fut, check_result) = + request_assumed_validation_data(ctx.sender(), &candidate).remote_handle(); let test_fut = async move { assert_matches!( ctx_handle.recv().await, AllMessages::RuntimeApi(RuntimeApiMessage::Request( rp, - RuntimeApiRequest::PersistedValidationData( + RuntimeApiRequest::AssumedValidationData( p, - OccupiedCoreAssumption::Included, + _, tx ), )) => { assert_eq!(rp, relay_parent); assert_eq!(p, para_id); - let _ = tx.send(Ok(Some(validation_data.clone()))); + let _ = tx.send(Err(RuntimeApiError::from(String::new()))); } ); - assert_matches!(check_result.await, AssumptionCheckOutcome::DoesNotMatch); + assert_matches!(check_result.await, Err(RuntimeRequestFailed)); }; let test_fut = future::join(test_fut, check_fut); @@ -676,3 +574,133 @@ fn pov_decompression_failure_is_invalid() { assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::PoVDecompressionFailure))); } + +#[test] +fn requests_code_from_api_if_not_found() { + let pool = TaskExecutor::new(); + let (mut ctx, mut ctx_handle) = + test_helpers::make_subsystem_context::(pool.clone()); + + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; + + let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let head_data = HeadData(vec![1, 1, 1]); + let validation_code = ValidationCode(vec![2; 16]); + let validation_code_hash = validation_code.hash(); + + let mut descriptor = CandidateDescriptor::default(); + descriptor.pov_hash = pov.hash(); + descriptor.para_head = head_data.hash(); + descriptor.validation_code_hash = validation_code.hash(); + collator_sign(&mut descriptor, Sr25519Keyring::Alice); + + let validation_result = WasmValidationResult { + head_data, + new_validation_code: Some(vec![2, 2, 2].into()), + upward_messages: Vec::new(), + horizontal_messages: Vec::new(), + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + + let metrics = Default::default(); + + let (check_fut, check_result) = validate_candidate_exhaustive( + ctx.sender(), + MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), + validation_data, + None, + descriptor, + Arc::new(pov), + Duration::from_secs(0), + &metrics, + ) + .remote_handle(); + + let test_fut = async move { + assert_matches!( + ctx_handle.recv().await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _rp, + RuntimeApiRequest::ValidationCodeByHash( + code_hash, + tx + ), + )) => { + assert_eq!(code_hash, validation_code_hash); + + let _ = tx.send(Ok(Some(validation_code))); + } + ); + + assert_matches!(check_result.await, Ok(ValidationResult::Valid(_, _))); + }; + + let test_fut = future::join(test_fut, check_fut); + executor::block_on(test_fut); +} + +#[test] +fn code_hash_mismatch_error() { + let pool = TaskExecutor::new(); + let (mut ctx, mut ctx_handle) = + test_helpers::make_subsystem_context::(pool.clone()); + + let validation_data = PersistedValidationData { max_pov_size: 1024, ..Default::default() }; + + let pov = PoV { block_data: BlockData(vec![1; 32]) }; + let head_data = HeadData(vec![1, 1, 1]); + let validation_code = ValidationCode(vec![2; 16]); + + let mut descriptor = CandidateDescriptor::default(); + descriptor.pov_hash = pov.hash(); + descriptor.para_head = head_data.hash(); + descriptor.validation_code_hash = validation_code.hash(); + collator_sign(&mut descriptor, Sr25519Keyring::Alice); + + let validation_result = WasmValidationResult { + head_data, + new_validation_code: Some(vec![2, 2, 2].into()), + upward_messages: Vec::new(), + horizontal_messages: Vec::new(), + processed_downward_messages: 0, + hrmp_watermark: 0, + }; + + let metrics = Default::default(); + + let (check_fut, check_result) = validate_from_chain_state( + ctx.sender(), + MockValidatorBackend::with_hardcoded_result(Ok(validation_result)), + descriptor, + Arc::new(pov), + Duration::from_secs(0), + &metrics, + ) + .remote_handle(); + + let test_fut = async move { + assert_matches!( + ctx_handle.recv().await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _rp, + RuntimeApiRequest::AssumedValidationData( + _p, + _hash, + tx + ), + )) => { + let code_hash = ValidationCodeHash::from(Hash::zero()); + let _ = tx.send(Ok(Some((validation_data, code_hash)))); + } + ); + + assert_matches!( + check_result.await, + Ok(ValidationResult::Invalid(InvalidCandidate::CodeHashMismatch)) + ); + }; + + let test_fut = future::join(test_fut, check_fut); + executor::block_on(test_fut); +} From bec11a30e0abac8c1b2f580e17fd7600348673d2 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 25 Oct 2021 22:33:06 +0300 Subject: [PATCH 13/26] fmt --- 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 2ee639595840..c8ed5a13feeb 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -435,7 +435,7 @@ where 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? + // TODO: code not found by hash by the runtime, is this candidate invalid? return Err(ValidationFailed( "Runtime API didn't return validation code by hash".into(), )), From 33560cab09c015a34e6c9a320cd36986e85d672b Mon Sep 17 00:00:00 2001 From: Chris Sosnin <48099298+slumber@users.noreply.github.com> Date: Tue, 26 Oct 2021 13:58:14 +0300 Subject: [PATCH 14/26] Remove extra line Co-authored-by: Bernhard Schuster --- node/core/pvf/src/host.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index c16f0f8b2de0..a5a44461ce62 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -447,7 +447,6 @@ async fn handle_execute_pvf( } else { if let Pvf::Preimage(code) = pvf { // Artifact is unknown: register it and enqueue a job with the corresponding priority and - // artifacts.insert_preparing(artifact_id.clone()); send_prepare(prepare_queue, prepare::ToQueue::Enqueue { priority, pvf: code }).await?; From 0a93b6a5c4372efaeb932f8b5684e545693744ef Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 27 Oct 2021 12:09:21 +0300 Subject: [PATCH 15/26] Get rid of unreachable --- node/core/pvf/src/host.rs | 17 +++++++---------- node/core/pvf/src/prepare/queue.rs | 6 +----- node/core/pvf/src/pvf.rs | 27 +++++++++++++++++---------- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index a5a44461ce62..64949654eb09 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -661,7 +661,7 @@ mod tests { /// Creates a new PVF which artifact id can be uniquely identified by the given number. fn artifact_id(discriminator: u32) -> ArtifactId { - Pvf::from_discriminator(discriminator).as_artifact_id() + PvfCode::from_discriminator(discriminator).as_artifact_id() } fn artifact_path(discriminator: u32) -> PathBuf { @@ -826,10 +826,7 @@ mod tests { /// Creates a new PVF which artifact id can be uniquely identified by the given number. fn pvf(discriminator: u32) -> PvfCode { - match Pvf::from_discriminator(discriminator) { - Pvf::Preimage(inner) => inner, - Pvf::Hash(_) => unreachable!(), - } + PvfCode::from_discriminator(discriminator) } #[async_std::test] @@ -897,7 +894,7 @@ mod tests { let (result_tx, _result_rx) = oneshot::channel(); host.execute_pvf( - Pvf::from_discriminator(1), + Pvf::Preimage(PvfCode::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, vec![], Priority::Critical, @@ -925,7 +922,7 @@ mod tests { let (result_tx, result_rx_pvf_1_1) = oneshot::channel(); host.execute_pvf( - Pvf::from_discriminator(1), + Pvf::Preimage(PvfCode::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf1".to_vec(), Priority::Normal, @@ -936,7 +933,7 @@ mod tests { let (result_tx, result_rx_pvf_1_2) = oneshot::channel(); host.execute_pvf( - Pvf::from_discriminator(1), + Pvf::Preimage(PvfCode::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf1".to_vec(), Priority::Critical, @@ -947,7 +944,7 @@ mod tests { let (result_tx, result_rx_pvf_2) = oneshot::channel(); host.execute_pvf( - Pvf::from_discriminator(2), + Pvf::Preimage(PvfCode::from_discriminator(2)), TEST_EXECUTION_TIMEOUT, b"pvf2".to_vec(), Priority::Normal, @@ -1023,7 +1020,7 @@ mod tests { let (result_tx, result_rx) = oneshot::channel(); host.execute_pvf( - Pvf::from_discriminator(1), + Pvf::Preimage(PvfCode::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf1".to_vec(), Priority::Normal, diff --git a/node/core/pvf/src/prepare/queue.rs b/node/core/pvf/src/prepare/queue.rs index 49e65b6099e2..5ee6ac1d8be7 100644 --- a/node/core/pvf/src/prepare/queue.rs +++ b/node/core/pvf/src/prepare/queue.rs @@ -530,7 +530,6 @@ pub fn start( #[cfg(test)] mod tests { use super::*; - use crate::pvf::Pvf; use assert_matches::assert_matches; use futures::{future::BoxFuture, FutureExt}; use slotmap::SlotMap; @@ -538,10 +537,7 @@ mod tests { /// Creates a new PVF which artifact id can be uniquely identified by the given number. fn pvf(discriminator: u32) -> PvfCode { - match Pvf::from_discriminator(discriminator) { - Pvf::Preimage(inner) => inner, - Pvf::Hash(_) => unreachable!(), - } + PvfCode::from_discriminator(discriminator) } async fn run_until( diff --git a/node/core/pvf/src/pvf.rs b/node/core/pvf/src/pvf.rs index 5d8cc9049405..9dd29d528659 100644 --- a/node/core/pvf/src/pvf.rs +++ b/node/core/pvf/src/pvf.rs @@ -28,6 +28,22 @@ pub struct PvfCode { pub(crate) code_hash: ValidationCodeHash, } +impl PvfCode { + /// Returns an instance of the PVF out of the given PVF code. + pub fn from_code(code: Vec) -> Self { + let code = Arc::new(code); + let code_hash = blake2_256(&code).into(); + Self { code, code_hash } + } + + /// Creates a new PVF which artifact id can be uniquely identified by the given number. + #[cfg(test)] + pub(crate) fn from_discriminator(num: u32) -> Self { + let discriminator_buf = num.to_le_bytes().to_vec(); + Self::from_code(discriminator_buf) + } +} + impl fmt::Debug for PvfCode { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Pvf {{ code, code_hash: {:?} }}", self.code_hash) @@ -54,16 +70,7 @@ pub enum Pvf { impl Pvf { /// Returns an instance of the PVF out of the given PVF code. pub fn from_code(code: Vec) -> Self { - let code = Arc::new(code); - let code_hash = blake2_256(&code).into(); - Self::Preimage(PvfCode { code, code_hash }) - } - - /// Creates a new PVF which artifact id can be uniquely identified by the given number. - #[cfg(test)] - pub(crate) fn from_discriminator(num: u32) -> Self { - let discriminator_buf = num.to_le_bytes().to_vec(); - Pvf::from_code(discriminator_buf) + Self::Preimage(PvfCode::from_code(code)) } /// Returns the artifact ID that corresponds to this PVF. From e58b1ec9388ec802cf593cea97fbc3cb83f04dc2 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 27 Oct 2021 12:22:09 +0300 Subject: [PATCH 16/26] Remove mutable result --- node/core/candidate-validation/src/lib.rs | 32 ++++++++++++----------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index c8ed5a13feeb..e7cb3f81a813 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -413,20 +413,13 @@ where relay_parent_storage_root: persisted_validation_data.relay_parent_storage_root, }; - let mut result = validation_backend + let result = match validation_backend .validate_candidate(validation_code, timeout, params.clone()) - .await; - - if let Err(ref e) = result { - tracing::debug!( - target: LOG_TARGET, - error = ?e, - "Failed to validate candidate", - ); - - // In case preimage for the supplied code hash was not found by the - // validation host, request the code from Runtime API and try again. - if let &ValidationError::ArtifactNotFound = e { + .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", @@ -455,8 +448,17 @@ where }, }; let validation_code = Pvf::from_code(raw_code.to_vec()); - result = validation_backend.validate_candidate(validation_code, timeout, params).await; - } + validation_backend.validate_candidate(validation_code, timeout, params).await + }, + result => result, + }; + + if let Err(ref e) = result { + tracing::debug!( + target: LOG_TARGET, + error = ?e, + "Failed to validate candidate", + ); } match result { From 0e9b0fe32923fdbe951b8ea5fbbd8ff4bd26718e Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 27 Oct 2021 12:25:03 +0300 Subject: [PATCH 17/26] Log the error --- node/core/candidate-validation/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index e7cb3f81a813..a994c0435514 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -475,10 +475,11 @@ where Err(ValidationError::ArtifactNotFound) => { // The code was supplied on the second attempt, this // error should be unreachable. - let error_message = - "Validation host failed to find artifact even though it was supplied"; - tracing::error!(target: LOG_TARGET, error_message); - Err(ValidationFailed(error_message.to_string())) + 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"); + Err(err) }, Ok(res) => if res.head_data.hash() != descriptor.para_head { From e4edd445f472a2610b25f1aaee525f9e16ed95fe Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Tue, 18 Jan 2022 18:56:29 +0300 Subject: [PATCH 18/26] Resolve precheck todo --- node/core/candidate-validation/src/lib.rs | 13 +++++++---- node/core/candidate-validation/src/tests.rs | 4 ++-- node/core/pvf/src/host.rs | 26 ++++++--------------- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 2494fd3b74e5..0ade416c7961 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -24,7 +24,8 @@ #![warn(missing_docs)] use polkadot_node_core_pvf::{ - InvalidCandidate as WasmInvalidCandidate, PrepareError, Pvf, ValidationError, ValidationHost, + InvalidCandidate as WasmInvalidCandidate, PrepareError, Pvf, PvfCode, ValidationError, + ValidationHost, }; use polkadot_node_primitives::{ BlockData, InvalidCandidate, PoV, ValidationResult, POV_BOMB_LIMIT, VALIDATION_CODE_BOMB_LIMIT, @@ -314,6 +315,10 @@ async fn precheck_pvf( where Sender: SubsystemSender, { + // Even though the PVF host is capable of looking for code by its hash, we still + // request the validation code from the runtime right away. The reasoning is that + // the host is only expected to have the PVF cached in case there was a transient error + // that allows precheck retrying, otherwise the Runtime API request is necessary. let validation_code = match request_validation_code_by_hash(sender, relay_parent, validation_code_hash).await { Ok(Some(code)) => code, @@ -336,7 +341,7 @@ where &validation_code.0, VALIDATION_CODE_BOMB_LIMIT, ) { - Ok(code) => Pvf::from_code(code.into_owned()), + Ok(code) => PvfCode::from_code(code.into_owned()), Err(e) => { tracing::debug!(target: LOG_TARGET, err=?e, "precheck: cannot decompress validation code"); return PreCheckOutcome::Invalid @@ -586,7 +591,7 @@ trait ValidationBackend { params: ValidationParams, ) -> Result; - async fn precheck_pvf(&mut self, pvf: Pvf) -> Result<(), PrepareError>; + async fn precheck_pvf(&mut self, pvf: PvfCode) -> Result<(), PrepareError>; } #[async_trait] @@ -621,7 +626,7 @@ impl ValidationBackend for ValidationHost { validation_result } - async fn precheck_pvf(&mut self, pvf: Pvf) -> Result<(), PrepareError> { + async fn precheck_pvf(&mut self, pvf: PvfCode) -> Result<(), PrepareError> { let (tx, rx) = oneshot::channel(); if let Err(_) = self.precheck_pvf(pvf, tx).await { return Err(PrepareError::DidNotMakeIt) diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index 33065b724a34..963fa23b8f9c 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -204,7 +204,7 @@ impl ValidationBackend for MockValidateCandidateBackend { } } - async fn precheck_pvf(&mut self, _pvf: Pvf) -> Result<(), PrepareError> { + async fn precheck_pvf(&mut self, _pvf: PvfCode) -> Result<(), PrepareError> { unreachable!() } } @@ -788,7 +788,7 @@ impl ValidationBackend for MockPreCheckBackend { unreachable!() } - async fn precheck_pvf(&mut self, _pvf: Pvf) -> Result<(), PrepareError> { + async fn precheck_pvf(&mut self, _pvf: PvfCode) -> Result<(), PrepareError> { self.result.clone() } } diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index 024fcc31393a..4ff63518b22b 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -62,7 +62,7 @@ impl ValidationHost { /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. pub async fn precheck_pvf( &mut self, - pvf: Pvf, + pvf: PvfCode, result_tx: PrepareResultSender, ) -> Result<(), String> { self.to_host_tx @@ -108,7 +108,7 @@ impl ValidationHost { enum ToHost { PrecheckPvf { - pvf: Pvf, + pvf: PvfCode, result_tx: PrepareResultSender, }, ExecutePvf { @@ -432,13 +432,9 @@ async fn handle_to_host( async fn handle_precheck_pvf( artifacts: &mut Artifacts, prepare_queue: &mut mpsc::Sender, - pvf: Pvf, + pvf: PvfCode, result_sender: PrepareResultSender, ) -> Result<(), Fatal> { - let pvf = match pvf { - Pvf::Code(code) => code, - Pvf::Hash(_) => todo!(), - }; let artifact_id = pvf.as_artifact_id(); if let Some(state) = artifacts.artifact_state_mut(&artifact_id) { @@ -1032,9 +1028,7 @@ mod tests { // First, test a simple precheck request. let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(Pvf::Code(PvfCode::from_discriminator(1)), result_tx) - .await - .unwrap(); + host.precheck_pvf(PvfCode::from_discriminator(1), result_tx).await.unwrap(); // The queue received the prepare request. assert_matches!( @@ -1055,9 +1049,7 @@ mod tests { let mut precheck_receivers = Vec::new(); for _ in 0..3 { let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(Pvf::Code(PvfCode::from_discriminator(2)), result_tx) - .await - .unwrap(); + host.precheck_pvf(PvfCode::from_discriminator(2), result_tx).await.unwrap(); precheck_receivers.push(result_rx); } // Received prepare request. @@ -1107,9 +1099,7 @@ mod tests { ); let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(Pvf::Code(PvfCode::from_discriminator(1)), result_tx) - .await - .unwrap(); + host.precheck_pvf(PvfCode::from_discriminator(1), result_tx).await.unwrap(); // Suppose the preparation failed, the execution queue is empty and both // "clients" receive their results. @@ -1131,9 +1121,7 @@ mod tests { let mut precheck_receivers = Vec::new(); for _ in 0..3 { let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(Pvf::Code(PvfCode::from_discriminator(2)), result_tx) - .await - .unwrap(); + host.precheck_pvf(PvfCode::from_discriminator(2), result_tx).await.unwrap(); precheck_receivers.push(result_rx); } From 9c91729697dc6131b87bc9547b0ec28e6793cd7d Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 19 Jan 2022 13:08:28 +0300 Subject: [PATCH 19/26] Host tests --- node/core/pvf/src/host.rs | 60 +++++++++++++++++++++++++++++++++++++++ node/core/pvf/src/pvf.rs | 8 ++++++ 2 files changed, 68 insertions(+) diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index 4ff63518b22b..ca939e13eaa8 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -1185,4 +1185,64 @@ mod tests { test.poll_ensure_to_execute_queue_is_empty().await; } + + #[async_std::test] + async fn artifact_cache() { + let mut test = Builder::default().build(); + let mut host = test.host_handle(); + + let pvf_code = Pvf::Code(PvfCode::from_discriminator(1)); + let pvf_hash = Pvf::Hash(pvf_code.hash()); + + // First, ensure that we receive an `ArtifactNotFound` error when sending + // an unknown code hash. + let (result_tx, result_rx) = oneshot::channel(); + host.execute_pvf( + pvf_hash.clone(), + TEST_EXECUTION_TIMEOUT, + Vec::new(), + Priority::Normal, + result_tx, + ) + .await + .unwrap(); + + test.poll_ensure_to_execute_queue_is_empty().await; + assert_matches!( + result_rx.now_or_never().unwrap().unwrap(), + Err(ValidationError::ArtifactNotFound) + ); + + // Supply the code and retry the request. + let (result_tx, _result_rx) = oneshot::channel(); + host.execute_pvf( + pvf_code, // Code. + TEST_EXECUTION_TIMEOUT, + Vec::new(), + Priority::Normal, + result_tx, + ) + .await + .unwrap(); + + assert_matches!( + test.poll_and_recv_to_prepare_queue().await, + prepare::ToQueue::Enqueue { .. } + ); + + let (result_tx, result_rx) = oneshot::channel(); + host.execute_pvf( + pvf_hash, // Hash. + TEST_EXECUTION_TIMEOUT, + Vec::new(), + Priority::Normal, + result_tx, + ) + .await + .unwrap(); + + test.poll_ensure_to_execute_queue_is_empty().await; + // The execution is queued, no error expected. + assert_matches!(result_rx.now_or_never(), None); + } } diff --git a/node/core/pvf/src/pvf.rs b/node/core/pvf/src/pvf.rs index ed363f4e7003..9a9ebee4ccc7 100644 --- a/node/core/pvf/src/pvf.rs +++ b/node/core/pvf/src/pvf.rs @@ -73,6 +73,14 @@ impl Pvf { Self::Code(PvfCode::from_code(code)) } + /// Returns the validation code hash of the given PVF. + pub fn hash(&self) -> ValidationCodeHash { + match self { + Pvf::Code(code) => code.code_hash, + Pvf::Hash(hash) => *hash, + } + } + /// Returns the artifact ID that corresponds to this PVF. pub(crate) fn as_artifact_id(&self) -> ArtifactId { match self { From cb82bd476fa7958daed499d5af22cfc3cb08c84f Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 19 Jan 2022 16:39:27 +0300 Subject: [PATCH 20/26] Update implementers guide --- roadmap/implementers-guide/src/node/subsystems-and-jobs.md | 4 ++-- .../src/node/utility/candidate-validation.md | 6 +++--- roadmap/implementers-guide/src/types/overseer-protocol.md | 7 +++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/roadmap/implementers-guide/src/node/subsystems-and-jobs.md b/roadmap/implementers-guide/src/node/subsystems-and-jobs.md index bffee1ffd2b1..ab9854242187 100644 --- a/roadmap/implementers-guide/src/node/subsystems-and-jobs.md +++ b/roadmap/implementers-guide/src/node/subsystems-and-jobs.md @@ -90,8 +90,8 @@ digraph { cand_sel -> cand_back [arrowhead = "onormal", label = "Second"] cand_sel -> coll_prot [arrowhead = "onormal", label = "ReportCollator"] - cand_val -> runt_api [arrowhead = "diamond", label = "Request::PersistedValidationData"] - cand_val -> runt_api [arrowhead = "diamond", label = "Request::ValidationCode"] + cand_val -> runt_api [arrowhead = "diamond", label = "Request::AssumedValidationData"] + cand_val -> runt_api [arrowhead = "diamond", label = "Request::ValidationCodeByHash"] cand_val -> runt_api [arrowhead = "diamond", label = "Request::CheckValidationOutputs"] coll_gen -> coll_prot [arrowhead = "onormal", label = "DistributeCollation"] diff --git a/roadmap/implementers-guide/src/node/utility/candidate-validation.md b/roadmap/implementers-guide/src/node/utility/candidate-validation.md index c34672368c32..dc4d3469def4 100644 --- a/roadmap/implementers-guide/src/node/utility/candidate-validation.md +++ b/roadmap/implementers-guide/src/node/utility/candidate-validation.md @@ -28,11 +28,11 @@ For a [`CandidateValidationMessage`][CVM]`::ValidateFromExhaustive`, these param For a [`CandidateValidationMessage`][CVM]`::ValidateFromChainState`, some more work needs to be done. Due to the uncertainty of Availability Cores (implemented in the [`Scheduler`](../../runtime/scheduler.md) module of the runtime), a candidate at a particular relay-parent and for a particular para may have two different valid validation-data to be executed under depending on what is assumed to happen if the para is occupying a core at the onset of the new block. This is encoded as an `OccupiedCoreAssumption` in the runtime API. -The way that we can determine which assumption the candidate is meant to be executed under is simply to do an exhaustive check of both possibilities based on the state of the relay-parent. First we fetch the validation data under the assumption that the block occupying becomes available. If the `validation_data_hash` of the `CandidateDescriptor` matches this validation data, we use that. Otherwise, if the `validation_data_hash` matches the validation data fetched under the `TimedOut` assumption, we use that. Otherwise, we return a `ValidationResult::Invalid` response and conclude. +For this reason this subsystem uses a convenient Runtime API endpoint — [`AssumedValidationData`](../../types/overseer-protocol.md#runtime-api-message). It accepts two parameters: parachain ID and the expected hash of the validation data, — the one we obtain from the `CandidateDescriptor`. Then the runtime tries to construct the validation data for the given parachain first without force enacting the core and, if its hash doesn't match the expected one, tries again with it. In case of a match the API returns the constructed validation data along with the corresponding validation code hash, or `None` otherwise. The latter means that the validation data hash in the descriptor is not based on the relay parent and thus given candidate is invalid. -Then, we can fetch the validation code from the runtime based on which type of candidate this is. This gives us all the parameters. The descriptor and PoV come from the request itself, and the other parameters have been derived from the state. +The validation backend, the one that is responsible for actually compiling and executing wasm code, keeps an artifact cache. This allows the subsystem to attempt the validation by the code hash obtained earlier. If the code with the given hash is missing though, we will have to perform another request necessary to obtain the validation function: `ValidationCodeByHash`. -> TODO: This would be a great place for caching to avoid making lots of runtime requests. That would need a job, though. +This gives us all the parameters. The descriptor and PoV come from the request itself, and the other parameters have been derived from the state. ### Execution of the Parachain Wasm diff --git a/roadmap/implementers-guide/src/types/overseer-protocol.md b/roadmap/implementers-guide/src/types/overseer-protocol.md index 61a874697835..eb58f9dd7ad7 100644 --- a/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -690,6 +690,13 @@ enum RuntimeApiRequest { OccupiedCoreAssumption, ResponseChannel>, ), + /// Get the persisted validation data for a particular para along with the current validation code + /// hash, matching the data hash against an expected one. + AssumedValidationData( + ParaId, + Hash, + RuntimeApiSender>, + ), /// Sends back `true` if the commitments pass all acceptance criteria checks. CheckValidationOutputs( ParaId, From 2a48e298537dfe0d6552359dc9af39bfe8e687ae Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 14 Feb 2022 11:42:17 +0300 Subject: [PATCH 21/26] Remove unnecessary log message --- 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 0ade416c7961..3bbc2197d363 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -560,7 +560,7 @@ where 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"); + tracing::error!(target: LOG_TARGET, error = ?err); Err(err) }, Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => From 24c043c056fd352bbd76ec38fc34b7467fef80e6 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 14 Feb 2022 12:24:23 +0300 Subject: [PATCH 22/26] Improve naming --- node/core/candidate-validation/src/lib.rs | 20 ++++---- node/core/candidate-validation/src/tests.rs | 12 ++--- node/core/pvf/src/host.rs | 52 ++++++++++----------- node/core/pvf/src/lib.rs | 2 +- node/core/pvf/src/prepare/queue.rs | 16 ++++--- node/core/pvf/src/pvf.rs | 33 +++++++------ node/core/pvf/tests/it/main.rs | 4 +- 7 files changed, 71 insertions(+), 68 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 3bbc2197d363..218b0f5239c8 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -24,8 +24,8 @@ #![warn(missing_docs)] use polkadot_node_core_pvf::{ - InvalidCandidate as WasmInvalidCandidate, PrepareError, Pvf, PvfCode, ValidationError, - ValidationHost, + InvalidCandidate as WasmInvalidCandidate, PrepareError, PvfDescriptor, PvfPreimage, + ValidationError, ValidationHost, }; use polkadot_node_primitives::{ BlockData, InvalidCandidate, PoV, ValidationResult, POV_BOMB_LIMIT, VALIDATION_CODE_BOMB_LIMIT, @@ -341,7 +341,7 @@ where &validation_code.0, VALIDATION_CODE_BOMB_LIMIT, ) { - Ok(code) => PvfCode::from_code(code.into_owned()), + Ok(code) => PvfPreimage::from_code(code.into_owned()), Err(e) => { tracing::debug!(target: LOG_TARGET, err=?e, "precheck: cannot decompress validation code"); return PreCheckOutcome::Invalid @@ -467,11 +467,11 @@ where }, } .to_vec(); - Pvf::from_code(raw_code) + PvfDescriptor::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)) + PvfDescriptor::Hash(ValidationCodeHash::from(descriptor.validation_code_hash)) }; let raw_block_data = @@ -529,7 +529,7 @@ where return Ok(ValidationResult::Invalid(InvalidCandidate::CodeDecompressionFailure)) }, }; - let validation_code = Pvf::from_code(raw_code.to_vec()); + let validation_code = PvfDescriptor::from_code(raw_code.to_vec()); validation_backend.validate_candidate(validation_code, timeout, params).await }, result => result, @@ -586,19 +586,19 @@ where trait ValidationBackend { async fn validate_candidate( &mut self, - validation_code: Pvf, + validation_code: PvfDescriptor, timeout: Duration, params: ValidationParams, ) -> Result; - async fn precheck_pvf(&mut self, pvf: PvfCode) -> Result<(), PrepareError>; + async fn precheck_pvf(&mut self, pvf: PvfPreimage) -> Result<(), PrepareError>; } #[async_trait] impl ValidationBackend for ValidationHost { async fn validate_candidate( &mut self, - validation_code: Pvf, + validation_code: PvfDescriptor, timeout: Duration, params: ValidationParams, ) -> Result { @@ -626,7 +626,7 @@ impl ValidationBackend for ValidationHost { validation_result } - async fn precheck_pvf(&mut self, pvf: PvfCode) -> Result<(), PrepareError> { + async fn precheck_pvf(&mut self, pvf: PvfPreimage) -> Result<(), PrepareError> { let (tx, rx) = oneshot::channel(); if let Err(_) = self.precheck_pvf(pvf, tx).await { return Err(PrepareError::DidNotMakeIt) diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index 963fa23b8f9c..73d5b95678df 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -194,17 +194,17 @@ impl MockValidateCandidateBackend { impl ValidationBackend for MockValidateCandidateBackend { async fn validate_candidate( &mut self, - raw_validation_code: Pvf, + raw_validation_code: PvfDescriptor, _timeout: Duration, _params: ValidationParams, ) -> Result { match raw_validation_code { - Pvf::Code(_) => self.result.clone(), - Pvf::Hash(_) => Err(ValidationError::ArtifactNotFound), + PvfDescriptor::Preimage(_) => self.result.clone(), + PvfDescriptor::Hash(_) => Err(ValidationError::ArtifactNotFound), } } - async fn precheck_pvf(&mut self, _pvf: PvfCode) -> Result<(), PrepareError> { + async fn precheck_pvf(&mut self, _pvf: PvfPreimage) -> Result<(), PrepareError> { unreachable!() } } @@ -781,14 +781,14 @@ impl MockPreCheckBackend { impl ValidationBackend for MockPreCheckBackend { async fn validate_candidate( &mut self, - _raw_validation_code: Pvf, + _raw_validation_code: PvfDescriptor, _timeout: Duration, _params: ValidationParams, ) -> Result { unreachable!() } - async fn precheck_pvf(&mut self, _pvf: PvfCode) -> Result<(), PrepareError> { + async fn precheck_pvf(&mut self, _pvf: PvfPreimage) -> Result<(), PrepareError> { self.result.clone() } } diff --git a/node/core/pvf/src/host.rs b/node/core/pvf/src/host.rs index ca939e13eaa8..1c274459d886 100644 --- a/node/core/pvf/src/host.rs +++ b/node/core/pvf/src/host.rs @@ -25,7 +25,7 @@ use crate::{ execute, metrics::Metrics, prepare, - pvf::{Pvf, PvfCode}, + pvf::{PvfDescriptor, PvfPreimage}, PrepareResult, Priority, ValidationError, LOG_TARGET, }; use always_assert::never; @@ -62,7 +62,7 @@ impl ValidationHost { /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. pub async fn precheck_pvf( &mut self, - pvf: PvfCode, + pvf: PvfPreimage, result_tx: PrepareResultSender, ) -> Result<(), String> { self.to_host_tx @@ -80,7 +80,7 @@ impl ValidationHost { /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. pub async fn execute_pvf( &mut self, - pvf: Pvf, + pvf: PvfDescriptor, execution_timeout: Duration, params: Vec, priority: Priority, @@ -98,7 +98,7 @@ impl ValidationHost { /// situations this function should return immediately. /// /// Returns an error if the request cannot be sent to the validation host, i.e. if it shut down. - pub async fn heads_up(&mut self, active_pvfs: Vec) -> Result<(), String> { + pub async fn heads_up(&mut self, active_pvfs: Vec) -> Result<(), String> { self.to_host_tx .send(ToHost::HeadsUp { active_pvfs }) .await @@ -108,18 +108,18 @@ impl ValidationHost { enum ToHost { PrecheckPvf { - pvf: PvfCode, + pvf: PvfPreimage, result_tx: PrepareResultSender, }, ExecutePvf { - pvf: Pvf, + pvf: PvfDescriptor, execution_timeout: Duration, params: Vec, priority: Priority, result_tx: ResultSender, }, HeadsUp { - active_pvfs: Vec, + active_pvfs: Vec, }, } @@ -432,7 +432,7 @@ async fn handle_to_host( async fn handle_precheck_pvf( artifacts: &mut Artifacts, prepare_queue: &mut mpsc::Sender, - pvf: PvfCode, + pvf: PvfPreimage, result_sender: PrepareResultSender, ) -> Result<(), Fatal> { let artifact_id = pvf.as_artifact_id(); @@ -463,7 +463,7 @@ async fn handle_execute_pvf( prepare_queue: &mut mpsc::Sender, execute_queue: &mut mpsc::Sender, awaiting_prepare: &mut AwaitingPrepare, - pvf: Pvf, + pvf: PvfDescriptor, execution_timeout: Duration, params: Vec, priority: Priority, @@ -495,7 +495,7 @@ async fn handle_execute_pvf( }, } } else { - if let Pvf::Code(code) = pvf { + if let PvfDescriptor::Preimage(code) = pvf { // Artifact is unknown: register it and enqueue a job with the corresponding priority and artifacts.insert_preparing(artifact_id.clone(), Vec::new()); send_prepare(prepare_queue, prepare::ToQueue::Enqueue { priority, pvf: code }).await?; @@ -513,7 +513,7 @@ async fn handle_execute_pvf( async fn handle_heads_up( artifacts: &mut Artifacts, prepare_queue: &mut mpsc::Sender, - active_pvfs: Vec, + active_pvfs: Vec, ) -> Result<(), Fatal> { let now = SystemTime::now(); @@ -717,7 +717,7 @@ mod tests { /// Creates a new PVF which artifact id can be uniquely identified by the given number. fn artifact_id(discriminator: u32) -> ArtifactId { - PvfCode::from_discriminator(discriminator).as_artifact_id() + PvfPreimage::from_discriminator(discriminator).as_artifact_id() } fn artifact_path(discriminator: u32) -> PathBuf { @@ -881,8 +881,8 @@ mod tests { } /// Creates a new PVF which artifact id can be uniquely identified by the given number. - fn pvf(discriminator: u32) -> PvfCode { - PvfCode::from_discriminator(discriminator) + fn pvf(discriminator: u32) -> PvfPreimage { + PvfPreimage::from_discriminator(discriminator) } #[async_std::test] @@ -934,7 +934,7 @@ mod tests { let (result_tx, result_rx_pvf_1_1) = oneshot::channel(); host.execute_pvf( - Pvf::Code(PvfCode::from_discriminator(1)), + PvfDescriptor::Preimage(PvfPreimage::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf1".to_vec(), Priority::Normal, @@ -945,7 +945,7 @@ mod tests { let (result_tx, result_rx_pvf_1_2) = oneshot::channel(); host.execute_pvf( - Pvf::Code(PvfCode::from_discriminator(1)), + PvfDescriptor::Preimage(PvfPreimage::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf1".to_vec(), Priority::Critical, @@ -956,7 +956,7 @@ mod tests { let (result_tx, result_rx_pvf_2) = oneshot::channel(); host.execute_pvf( - Pvf::Code(PvfCode::from_discriminator(2)), + PvfDescriptor::Preimage(PvfPreimage::from_discriminator(2)), TEST_EXECUTION_TIMEOUT, b"pvf2".to_vec(), Priority::Normal, @@ -1028,7 +1028,7 @@ mod tests { // First, test a simple precheck request. let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(PvfCode::from_discriminator(1), result_tx).await.unwrap(); + host.precheck_pvf(PvfPreimage::from_discriminator(1), result_tx).await.unwrap(); // The queue received the prepare request. assert_matches!( @@ -1049,7 +1049,7 @@ mod tests { let mut precheck_receivers = Vec::new(); for _ in 0..3 { let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(PvfCode::from_discriminator(2), result_tx).await.unwrap(); + host.precheck_pvf(PvfPreimage::from_discriminator(2), result_tx).await.unwrap(); precheck_receivers.push(result_rx); } // Received prepare request. @@ -1084,7 +1084,7 @@ mod tests { // Send PVF for the execution and request the prechecking for it. let (result_tx, result_rx_execute) = oneshot::channel(); host.execute_pvf( - Pvf::Code(PvfCode::from_discriminator(1)), + PvfDescriptor::Preimage(PvfPreimage::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf2".to_vec(), Priority::Critical, @@ -1099,7 +1099,7 @@ mod tests { ); let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(PvfCode::from_discriminator(1), result_tx).await.unwrap(); + host.precheck_pvf(PvfPreimage::from_discriminator(1), result_tx).await.unwrap(); // Suppose the preparation failed, the execution queue is empty and both // "clients" receive their results. @@ -1121,13 +1121,13 @@ mod tests { let mut precheck_receivers = Vec::new(); for _ in 0..3 { let (result_tx, result_rx) = oneshot::channel(); - host.precheck_pvf(PvfCode::from_discriminator(2), result_tx).await.unwrap(); + host.precheck_pvf(PvfPreimage::from_discriminator(2), result_tx).await.unwrap(); precheck_receivers.push(result_rx); } let (result_tx, _result_rx_execute) = oneshot::channel(); host.execute_pvf( - Pvf::Code(PvfCode::from_discriminator(2)), + PvfDescriptor::Preimage(PvfPreimage::from_discriminator(2)), TEST_EXECUTION_TIMEOUT, b"pvf2".to_vec(), Priority::Critical, @@ -1162,7 +1162,7 @@ mod tests { let (result_tx, result_rx) = oneshot::channel(); host.execute_pvf( - Pvf::Code(PvfCode::from_discriminator(1)), + PvfDescriptor::Preimage(PvfPreimage::from_discriminator(1)), TEST_EXECUTION_TIMEOUT, b"pvf1".to_vec(), Priority::Normal, @@ -1191,8 +1191,8 @@ mod tests { let mut test = Builder::default().build(); let mut host = test.host_handle(); - let pvf_code = Pvf::Code(PvfCode::from_discriminator(1)); - let pvf_hash = Pvf::Hash(pvf_code.hash()); + let pvf_code = PvfDescriptor::Preimage(PvfPreimage::from_discriminator(1)); + let pvf_hash = PvfDescriptor::Hash(pvf_code.hash()); // First, ensure that we receive an `ArtifactNotFound` error when sending // an unknown code hash. diff --git a/node/core/pvf/src/lib.rs b/node/core/pvf/src/lib.rs index 72cf8e94fef8..28b69acb9364 100644 --- a/node/core/pvf/src/lib.rs +++ b/node/core/pvf/src/lib.rs @@ -94,7 +94,7 @@ pub use sp_tracing; pub use error::{InvalidCandidate, PrepareError, PrepareResult, ValidationError}; pub use priority::Priority; -pub use pvf::{Pvf, PvfCode}; +pub use pvf::{PvfDescriptor, PvfPreimage}; pub use host::{start, Config, ValidationHost}; pub use metrics::Metrics; diff --git a/node/core/pvf/src/prepare/queue.rs b/node/core/pvf/src/prepare/queue.rs index bf6cd569cd8c..c6ca6c14a1aa 100644 --- a/node/core/pvf/src/prepare/queue.rs +++ b/node/core/pvf/src/prepare/queue.rs @@ -18,7 +18,7 @@ use super::pool::{self, Worker}; use crate::{ - artifacts::ArtifactId, metrics::Metrics, pvf::PvfCode, PrepareResult, Priority, LOG_TARGET, + artifacts::ArtifactId, metrics::Metrics, pvf::PvfPreimage, PrepareResult, Priority, LOG_TARGET, }; use always_assert::{always, never}; use async_std::path::PathBuf; @@ -32,7 +32,7 @@ pub enum ToQueue { /// /// Note that it is incorrect to enqueue the same PVF again without first receiving the /// [`FromQueue`] response. - Enqueue { priority: Priority, pvf: PvfCode }, + Enqueue { priority: Priority, pvf: PvfPreimage }, } /// A response from queue. @@ -77,7 +77,7 @@ slotmap::new_key_type! { pub struct Job; } struct JobData { /// The priority of this job. Can be bumped. priority: Priority, - pvf: PvfCode, + pvf: PvfPreimage, worker: Option, } @@ -212,7 +212,11 @@ async fn handle_to_queue(queue: &mut Queue, to_queue: ToQueue) -> Result<(), Fat Ok(()) } -async fn handle_enqueue(queue: &mut Queue, priority: Priority, pvf: PvfCode) -> Result<(), Fatal> { +async fn handle_enqueue( + queue: &mut Queue, + priority: Priority, + pvf: PvfPreimage, +) -> Result<(), Fatal> { tracing::debug!( target: LOG_TARGET, validation_code_hash = ?pvf.code_hash, @@ -487,8 +491,8 @@ mod tests { use std::task::Poll; /// Creates a new PVF which artifact id can be uniquely identified by the given number. - fn pvf(discriminator: u32) -> PvfCode { - PvfCode::from_discriminator(discriminator) + fn pvf(discriminator: u32) -> PvfPreimage { + PvfPreimage::from_discriminator(discriminator) } async fn run_until( diff --git a/node/core/pvf/src/pvf.rs b/node/core/pvf/src/pvf.rs index 9a9ebee4ccc7..ff7ae09a1d12 100644 --- a/node/core/pvf/src/pvf.rs +++ b/node/core/pvf/src/pvf.rs @@ -23,12 +23,12 @@ use std::{fmt, sync::Arc}; /// /// Should be cheap to clone. #[derive(Clone)] -pub struct PvfCode { +pub struct PvfPreimage { pub(crate) code: Arc>, pub(crate) code_hash: ValidationCodeHash, } -impl PvfCode { +impl PvfPreimage { /// Returns an instance of the PVF out of the given PVF code. pub fn from_code(code: Vec) -> Self { let code = Arc::new(code); @@ -44,48 +44,47 @@ impl PvfCode { } } -impl fmt::Debug for PvfCode { +impl fmt::Debug for PvfPreimage { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Pvf {{ code, code_hash: {:?} }}", self.code_hash) } } -impl PvfCode { +impl PvfPreimage { pub(crate) fn as_artifact_id(&self) -> ArtifactId { ArtifactId::new(self.code_hash) } } -/// An enum that either contains full preimage of validation function along -/// with its hash (see [`PvfCode`]) or the hash only. +/// An enum that either contains full preimage of the validation function +/// (see [`PvfPreimage`]) or the hash only. #[derive(Clone, Debug)] -pub enum Pvf { - /// Hash-preimage of the validation function, contains both the code - /// and the hash itself. - Code(PvfCode), - /// Hash of the validation function without its validation code. +pub enum PvfDescriptor { + /// Hash-preimage of the validation function, carries the full bytecode. + Preimage(PvfPreimage), + /// Hash of the validation function. Hash(ValidationCodeHash), } -impl Pvf { +impl PvfDescriptor { /// Returns an instance of the PVF out of the given PVF code. pub fn from_code(code: Vec) -> Self { - Self::Code(PvfCode::from_code(code)) + Self::Preimage(PvfPreimage::from_code(code)) } /// Returns the validation code hash of the given PVF. pub fn hash(&self) -> ValidationCodeHash { match self { - Pvf::Code(code) => code.code_hash, - Pvf::Hash(hash) => *hash, + Self::Preimage(code) => code.code_hash, + Self::Hash(hash) => *hash, } } /// Returns the artifact ID that corresponds to this PVF. pub(crate) fn as_artifact_id(&self) -> ArtifactId { match self { - Pvf::Code(ref inner) => inner.as_artifact_id(), - Pvf::Hash(code_hash) => ArtifactId::new(*code_hash), + Self::Preimage(ref inner) => inner.as_artifact_id(), + Self::Hash(code_hash) => ArtifactId::new(*code_hash), } } } diff --git a/node/core/pvf/tests/it/main.rs b/node/core/pvf/tests/it/main.rs index bf0983d50874..bda85e5412d8 100644 --- a/node/core/pvf/tests/it/main.rs +++ b/node/core/pvf/tests/it/main.rs @@ -17,7 +17,7 @@ use async_std::sync::Mutex; use parity_scale_codec::Encode as _; use polkadot_node_core_pvf::{ - start, Config, InvalidCandidate, Metrics, Pvf, ValidationError, ValidationHost, + start, Config, InvalidCandidate, Metrics, PvfDescriptor, ValidationError, ValidationHost, }; use polkadot_parachain::primitives::{BlockData, ValidationParams, ValidationResult}; use std::time::Duration; @@ -65,7 +65,7 @@ impl TestHost { .lock() .await .execute_pvf( - Pvf::from_code(code.into()), + PvfDescriptor::from_code(code.into()), TEST_EXECUTION_TIMEOUT, params.encode(), polkadot_node_core_pvf::Priority::Normal, From 249fc283027eb476ec39c97c453386b0715fa4d7 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Mon, 14 Feb 2022 20:32:21 +0300 Subject: [PATCH 23/26] Revert error handling --- node/core/candidate-validation/src/lib.rs | 15 +++++++++++++-- node/primitives/src/lib.rs | 2 -- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 218b0f5239c8..a66e389bd858 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -501,6 +501,7 @@ where // validation host, request the code from Runtime API and try again. tracing::debug!( target: LOG_TARGET, + validation_code_hash = ?descriptor.validation_code_hash, "Validation host failed to find artifact by provided hash", ); @@ -512,8 +513,18 @@ where .await { Ok(Some(validation_code)) => validation_code, - Ok(None) => - return Ok(ValidationResult::Invalid(InvalidCandidate::MissingValidationCode)), + Ok(None) => { + // This path can only happen when validating candidate from chain state. + // Earlier we obtained the PVF hash from the runtime and verified + // that it matches with the one in candidate descriptor. + // As a result, receiving `None` is unexpected and should hint on a major + // bug in Runtime API. + return Err(ValidationFailed( + "Runtime API returned `None` for the validation \ + code with a known hash" + .into(), + )) + }, Err(_) => return Err(ValidationFailed("Runtime API request failed".into())), }; diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 7a1b402f2fad..fd2268ab6023 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -231,8 +231,6 @@ pub enum InvalidCandidate { ParaHeadHashMismatch, /// Validation code hash does not match. CodeHashMismatch, - /// The corresponding validation code is not present in the Runtime storage. - MissingValidationCode, } /// Result of the validation of the candidate. From 0a847ab721e9c376efd9f095099b280ca5239828 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Wed, 30 Mar 2022 21:51:09 +0300 Subject: [PATCH 24/26] Replace tracing usage --- node/core/candidate-validation/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index b2d12ae7d8a1..d7a59fb6d268 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -499,7 +499,7 @@ where 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!( + gum::debug!( target: LOG_TARGET, validation_code_hash = ?descriptor.validation_code_hash, "Validation host failed to find artifact by provided hash", @@ -534,7 +534,7 @@ where ) { Ok(code) => code, Err(e) => { - tracing::debug!(target: LOG_TARGET, err=?e, "Code decompression failed"); + gum::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)) @@ -571,7 +571,7 @@ where let err = ValidationFailed( "Validation host failed to find artifact even though it was supplied".to_string(), ); - tracing::error!(target: LOG_TARGET, error = ?err); + gum::error!(target: LOG_TARGET, error = ?err); Err(err) }, Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::PrepareError(e))) => From bccbbc3b63f2c1aed97495d56ff126e20d079f35 Mon Sep 17 00:00:00 2001 From: Chris Sosnin Date: Fri, 1 Apr 2022 23:15:44 +0300 Subject: [PATCH 25/26] Explain force enacting --- .../implementers-guide/src/node/utility/candidate-validation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roadmap/implementers-guide/src/node/utility/candidate-validation.md b/roadmap/implementers-guide/src/node/utility/candidate-validation.md index 184880f7c370..404b18c09fb3 100644 --- a/roadmap/implementers-guide/src/node/utility/candidate-validation.md +++ b/roadmap/implementers-guide/src/node/utility/candidate-validation.md @@ -32,7 +32,7 @@ For a [`CandidateValidationMessage`][CVM]`::ValidateFromExhaustive`, these param For a [`CandidateValidationMessage`][CVM]`::ValidateFromChainState`, some more work needs to be done. Due to the uncertainty of Availability Cores (implemented in the [`Scheduler`](../../runtime/scheduler.md) module of the runtime), a candidate at a particular relay-parent and for a particular para may have two different valid validation-data to be executed under depending on what is assumed to happen if the para is occupying a core at the onset of the new block. This is encoded as an `OccupiedCoreAssumption` in the runtime API. -For this reason this subsystem uses a convenient Runtime API endpoint — [`AssumedValidationData`](../../types/overseer-protocol.md#runtime-api-message). It accepts two parameters: parachain ID and the expected hash of the validation data, — the one we obtain from the `CandidateDescriptor`. Then the runtime tries to construct the validation data for the given parachain first without force enacting the core and, if its hash doesn't match the expected one, tries again with it. In case of a match the API returns the constructed validation data along with the corresponding validation code hash, or `None` otherwise. The latter means that the validation data hash in the descriptor is not based on the relay parent and thus given candidate is invalid. +For this reason this subsystem uses a convenient Runtime API endpoint — [`AssumedValidationData`](../../types/overseer-protocol.md#runtime-api-message). It accepts two parameters: parachain ID and the expected hash of the validation data, — the one we obtain from the `CandidateDescriptor`. Then the runtime tries to construct the validation data for the given parachain first under the assumption that the block occupying the core didn't advance the para, i.e. it didn't reach the availability, if the data hash doesn't match the expected one, tries again with force enacting the core, temporarily updating the state as if the block had been deemed available. In case of a match the API returns the constructed validation data along with the corresponding validation code hash, or `None` otherwise. The latter means that the validation data hash in the descriptor is not based on the relay parent and thus given candidate is invalid. The validation backend, the one that is responsible for actually compiling and executing wasm code, keeps an artifact cache. This allows the subsystem to attempt the validation by the code hash obtained earlier. If the code with the given hash is missing though, we will have to perform another request necessary to obtain the validation function: `ValidationCodeByHash`. From 32680ca6a7456f79cd8e6e40f4111be20baa6fa3 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Tue, 11 Oct 2022 16:38:07 -0400 Subject: [PATCH 26/26] Fix leftover errors from merge Putting these changes in their own commit, to make it clear that there were extra changes on top of the merge. --- node/core/candidate-validation/src/lib.rs | 11 ++-- node/core/candidate-validation/src/tests.rs | 56 +++++++++++++-------- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/node/core/candidate-validation/src/lib.rs b/node/core/candidate-validation/src/lib.rs index 50000caa8f1a..f538f3a4929f 100644 --- a/node/core/candidate-validation/src/lib.rs +++ b/node/core/candidate-validation/src/lib.rs @@ -42,7 +42,7 @@ use polkadot_node_subsystem::{ use polkadot_parachain::primitives::{ValidationParams, ValidationResult as WasmValidationResult}; use polkadot_primitives::v2::{ BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateReceipt, Hash, - OccupiedCoreAssumption, PersistedValidationData, ValidationCode, ValidationCodeHash, + PersistedValidationData, ValidationCode, ValidationCodeHash, }; use parity_scale_codec::Encode; @@ -286,7 +286,7 @@ async fn request_assumed_validation_data( RuntimeRequestFailed, > where - Sender: SubsystemSender, + Sender: SubsystemSender, { let (tx, rx) = oneshot::channel(); runtime_api_request( @@ -437,12 +437,13 @@ async fn validate_candidate_exhaustive( metrics: &Metrics, ) -> Result where - Sender: SubsystemSender, + Sender: SubsystemSender, { let _timer = metrics.time_validate_candidate_exhaustive(); + let descriptor = &candidate_receipt.descriptor; let validation_code_hash = validation_code.as_ref().map(ValidationCode::hash); - let para_id = candidate_receipt.descriptor.para_id.clone(); + let para_id = descriptor.para_id.clone(); gum::debug!( target: LOG_TARGET, ?validation_code_hash, @@ -451,7 +452,7 @@ where ); if let Err(e) = perform_basic_checks( - &candidate_receipt.descriptor, + &descriptor, persisted_validation_data.max_pov_size, &*pov, validation_code_hash.as_ref(), diff --git a/node/core/candidate-validation/src/tests.rs b/node/core/candidate-validation/src/tests.rs index 05e043e66f0e..21e8398eef31 100644 --- a/node/core/candidate-validation/src/tests.rs +++ b/node/core/candidate-validation/src/tests.rs @@ -238,6 +238,10 @@ fn check_runtime_validation_code_request() { hrmp_watermark: 0, }; + let commitments = make_commitments_from_validation_result(&validation_result); + + let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; + let pool = TaskExecutor::new(); let (mut ctx, mut ctx_handle) = test_helpers::make_subsystem_context::(pool.clone()); @@ -247,7 +251,7 @@ fn check_runtime_validation_code_request() { MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data.clone(), None, - descriptor, + candidate_receipt, Arc::new(pov), Duration::from_secs(0), &metrics, @@ -317,14 +321,7 @@ fn candidate_validation_ok_is_ok() { hrmp_watermark: 0, }; - let commitments = CandidateCommitments { - head_data: validation_result.head_data.clone(), - upward_messages: validation_result.upward_messages.clone(), - horizontal_messages: validation_result.horizontal_messages.clone(), - new_validation_code: validation_result.new_validation_code.clone(), - processed_downward_messages: validation_result.processed_downward_messages, - hrmp_watermark: validation_result.hrmp_watermark, - }; + let commitments = make_commitments_from_validation_result(&validation_result); let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; @@ -333,7 +330,6 @@ fn candidate_validation_ok_is_ok() { MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data.clone(), Some(validation_code), - validation_code, candidate_receipt, Arc::new(pov), Duration::from_secs(0), @@ -390,7 +386,6 @@ fn candidate_validation_bad_return_is_invalid() { )), validation_data, Some(validation_code), - validation_code, candidate_receipt, Arc::new(pov), Duration::from_secs(0), @@ -480,10 +475,15 @@ fn candidate_validation_commitment_hash_mismatch_is_invalid() { hrmp_watermark: 12345, }; + let pool = TaskExecutor::new(); + let (mut ctx, mut _ctx_handle) = + test_helpers::make_subsystem_context::(pool.clone()); + let result = executor::block_on(validate_candidate_exhaustive( + ctx.sender(), MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data, - validation_code, + Some(validation_code), candidate_receipt, Arc::new(pov), Duration::from_secs(0), @@ -579,14 +579,7 @@ fn compressed_code_works() { hrmp_watermark: 0, }; - let commitments = CandidateCommitments { - head_data: validation_result.head_data.clone(), - upward_messages: validation_result.upward_messages.clone(), - horizontal_messages: validation_result.horizontal_messages.clone(), - new_validation_code: validation_result.new_validation_code.clone(), - processed_downward_messages: validation_result.processed_downward_messages, - hrmp_watermark: validation_result.hrmp_watermark, - }; + let commitments = make_commitments_from_validation_result(&validation_result); let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; @@ -741,6 +734,10 @@ fn requests_code_from_api_if_not_found() { hrmp_watermark: 0, }; + let commitments = make_commitments_from_validation_result(&validation_result); + + let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: commitments.hash() }; + let metrics = Default::default(); let (check_fut, check_result) = validate_candidate_exhaustive( @@ -748,7 +745,7 @@ fn requests_code_from_api_if_not_found() { MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), validation_data, None, - descriptor, + candidate_receipt, Arc::new(pov), Duration::from_secs(0), &metrics, @@ -809,12 +806,14 @@ fn code_hash_mismatch_error() { hrmp_watermark: 0, }; + let candidate_receipt = CandidateReceipt { descriptor, commitments_hash: Hash::zero() }; + let metrics = Default::default(); let (check_fut, check_result) = validate_from_chain_state( ctx.sender(), MockValidateCandidateBackend::with_hardcoded_result(Ok(validation_result)), - descriptor, + candidate_receipt, Arc::new(pov), Duration::from_secs(0), &metrics, @@ -1009,3 +1008,16 @@ fn precheck_properly_classifies_outcomes() { inner(Err(PrepareError::TimedOut), PreCheckOutcome::Failed); inner(Err(PrepareError::DidNotMakeIt), PreCheckOutcome::Failed); } + +fn make_commitments_from_validation_result( + validation_result: &WasmValidationResult, +) -> CandidateCommitments { + CandidateCommitments { + head_data: validation_result.head_data.clone(), + upward_messages: validation_result.upward_messages.clone(), + horizontal_messages: validation_result.horizontal_messages.clone(), + new_validation_code: validation_result.new_validation_code.clone(), + processed_downward_messages: validation_result.processed_downward_messages, + hrmp_watermark: validation_result.hrmp_watermark, + } +}