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

Validation: don't detect STDIN closing when running in process #1695

Merged
merged 10 commits into from
Sep 11, 2020
Merged

Validation: don't detect STDIN closing when running in process #1695

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
66a8654
Merge enums
cecton Sep 10, 2020
0cea17a
Move validation pool to execution mode
cecton Sep 10, 2020
f3620a5
Done todos
cecton Sep 10, 2020
6682600
Fix tests
cecton Sep 10, 2020
4efe84b
fix for wasm
cecton Sep 10, 2020
9197463
missing )
cecton Sep 10, 2020
a3cecf4
Update parachain/src/wasm_executor/mod.rs
cecton Sep 11, 2020
23fcec3
Update parachain/src/wasm_executor/validation_host.rs
cecton Sep 11, 2020
737bee1
WIP
cecton Sep 11, 2020
a6e238d
Re-enable wasmtime
cecton Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions network/src/protocol/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use polkadot_primitives::v0::{
GlobalValidationData, LocalValidationData, SigningContext,
PoVBlock, BlockData, ValidationCode,
};
use polkadot_validation::{SharedTable, TableRouter};
use polkadot_validation::{SharedTable, TableRouter, pipeline::ExecutionMode};

use av_store::Store as AvailabilityStore;
use sc_network_gossip::TopicNotification;
Expand Down Expand Up @@ -298,7 +298,7 @@ fn consensus_instances_cleaned_up() {
signing_context,
AvailabilityStore::new_in_memory(service.clone()),
None,
None,
ExecutionMode::InProcess,
));

pool.spawner().spawn_local(worker_task).unwrap();
Expand Down Expand Up @@ -329,7 +329,7 @@ fn collation_is_received_with_dropped_router() {
signing_context,
AvailabilityStore::new_in_memory(service.clone()),
None,
None,
ExecutionMode::InProcess,
));

pool.spawner().spawn_local(worker_task).unwrap();
Expand Down Expand Up @@ -489,7 +489,7 @@ fn fetches_pov_block_from_gossip() {
signing_context,
AvailabilityStore::new_in_memory(service.clone()),
None,
None,
ExecutionMode::InProcess,
));

let spawner = pool.spawner();
Expand Down
28 changes: 12 additions & 16 deletions node/core/candidate-validation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use polkadot_primitives::v1::{
};
use polkadot_parachain::wasm_executor::{
self, ValidationPool, ExecutionMode, ValidationError,
InvalidCandidate as WasmInvalidCandidate, ValidationExecutionMode,
InvalidCandidate as WasmInvalidCandidate,
};
use polkadot_parachain::primitives::{ValidationResult as WasmValidationResult, ValidationParams};

Expand Down Expand Up @@ -75,7 +75,7 @@ async fn run(
)
-> SubsystemResult<()>
{
let pool = ValidationPool::new(ValidationExecutionMode::ExternalProcessSelfHost);
let execution_mode = ExecutionMode::ExternalProcessSelfHost(ValidationPool::new());

loop {
match ctx.recv().await? {
Expand All @@ -90,7 +90,7 @@ async fn run(
) => {
let res = spawn_validate_from_chain_state(
&mut ctx,
Some(pool.clone()),
execution_mode.clone(),
descriptor,
pov,
spawn.clone(),
Expand All @@ -110,7 +110,7 @@ async fn run(
) => {
let res = spawn_validate_exhaustive(
&mut ctx,
Some(pool.clone()),
execution_mode.clone(),
omitted_validation,
validation_code,
descriptor,
Expand Down Expand Up @@ -217,7 +217,7 @@ async fn check_assumption_validation_data(

async fn spawn_validate_from_chain_state(
ctx: &mut impl SubsystemContext<Message = CandidateValidationMessage>,
validation_pool: Option<ValidationPool>,
execution_mode: ExecutionMode,
descriptor: CandidateDescriptor,
pov: Arc<PoV>,
spawn: impl SpawnNamed + 'static,
Expand Down Expand Up @@ -258,7 +258,7 @@ async fn spawn_validate_from_chain_state(
AssumptionCheckOutcome::Matches(omitted_validation, validation_code) => {
return spawn_validate_exhaustive(
ctx,
validation_pool,
execution_mode,
omitted_validation,
validation_code,
descriptor,
Expand All @@ -279,7 +279,7 @@ async fn spawn_validate_from_chain_state(
AssumptionCheckOutcome::Matches(omitted_validation, validation_code) => {
return spawn_validate_exhaustive(
ctx,
validation_pool,
execution_mode,
omitted_validation,
validation_code,
descriptor,
Expand All @@ -299,7 +299,7 @@ async fn spawn_validate_from_chain_state(

async fn spawn_validate_exhaustive(
ctx: &mut impl SubsystemContext<Message = CandidateValidationMessage>,
validation_pool: Option<ValidationPool>,
execution_mode: ExecutionMode,
omitted_validation: OmittedValidationData,
validation_code: ValidationCode,
descriptor: CandidateDescriptor,
Expand All @@ -309,7 +309,7 @@ async fn spawn_validate_exhaustive(
let (tx, rx) = oneshot::channel();
let fut = async move {
let res = validate_candidate_exhaustive::<RealValidationBackend, _>(
validation_pool,
execution_mode,
omitted_validation,
validation_code,
descriptor,
Expand Down Expand Up @@ -386,22 +386,18 @@ trait ValidationBackend {
struct RealValidationBackend;

impl ValidationBackend for RealValidationBackend {
type Arg = Option<ValidationPool>;
type Arg = ExecutionMode;

fn validate<S: SpawnNamed + 'static>(
pool: Option<ValidationPool>,
execution_mode: ExecutionMode,
validation_code: &ValidationCode,
params: ValidationParams,
spawn: S,
) -> Result<WasmValidationResult, ValidationError> {
let execution_mode = pool.as_ref()
.map(ExecutionMode::Remote)
.unwrap_or(ExecutionMode::Local);

wasm_executor::validate_candidate(
&validation_code.0,
params,
execution_mode,
&execution_mode,
spawn,
)
}
Expand Down
50 changes: 34 additions & 16 deletions parachain/src/wasm_executor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@
//! Assuming the parameters are correct, this module provides a wrapper around
//! a WASM VM for re-execution of a parachain candidate.

use std::any::{TypeId, Any};
use std::{any::{TypeId, Any}, path::PathBuf};
use crate::primitives::{ValidationParams, ValidationResult};
use codec::{Decode, Encode};
use sp_core::{storage::ChildInfo, traits::{CallInWasm, SpawnNamed}};
use sp_externalities::Extensions;
use sp_wasm_interface::HostFunctions as _;

#[cfg(not(any(target_os = "android", target_os = "unknown")))]
pub use validation_host::{run_worker, ValidationPool, EXECUTION_TIMEOUT_SEC, ValidationExecutionMode};
pub use validation_host::{run_worker, ValidationPool, EXECUTION_TIMEOUT_SEC, WORKER_ARGS};

mod validation_host;

Expand Down Expand Up @@ -58,16 +58,29 @@ pub fn run_worker(_: &str) -> Result<(), String> {
Err("Cannot run validation worker on this platform".to_string())
}

/// WASM code execution mode.
///
/// > Note: When compiling for WASM, the `Remote` variants are not available.
pub enum ExecutionMode<'a> {
/// Execute in-process. The execution can not be interrupted or aborted.
Local,
/// Remote execution in a spawned process.
Remote(&'a ValidationPool),
/// The execution mode for the `ValidationPool`.
#[derive(Clone)]
#[cfg_attr(not(any(target_os = "android", target_os = "unknown")), derive(Debug))]
pub enum ExecutionMode {
/// The validation worker is ran in a thread inside the same process.
InProcess,
/// The validation worker is ran using the process' executable and the subcommand `validation-worker` is passed
/// following by the address of the shared memory.
ExternalProcessSelfHost(ValidationPool),
/// The validation worker is ran using the command provided and the argument provided. The address of the shared
/// memory is added at the end of the arguments.
ExternalProcessCustomHost {
/// Validation pool.
pool: ValidationPool,
/// Path to the validation worker. The file must exists and be executable.
binary: PathBuf,
/// List of arguments passed to the validation worker. The address of the shared memory will be automatically
/// added after the arguments.
args: Vec<String>,
},
}


#[derive(Debug, derive_more::Display, derive_more::From)]
/// Candidate validation error.
pub enum ValidationError {
Expand Down Expand Up @@ -132,19 +145,24 @@ impl std::error::Error for ValidationError {
pub fn validate_candidate(
validation_code: &[u8],
params: ValidationParams,
options: ExecutionMode<'_>,
execution_mode: &ExecutionMode,
spawner: impl SpawnNamed + 'static,
) -> Result<ValidationResult, ValidationError> {
match options {
ExecutionMode::Local => {
match execution_mode {
ExecutionMode::InProcess => {
validate_candidate_internal(validation_code, &params.encode(), spawner)
},
#[cfg(not(any(target_os = "android", target_os = "unknown")))]
ExecutionMode::Remote(pool) => {
ExecutionMode::ExternalProcessSelfHost(pool) => {
pool.validate_candidate(validation_code, params)
},
#[cfg(not(any(target_os = "android", target_os = "unknown")))]
ExecutionMode::ExternalProcessCustomHost { pool, binary, args } => {
let args: Vec<&str> = args.iter().map(|x| x.as_str()).collect();
pool.validate_candidate_custom(validation_code, params, binary, &args)
},
#[cfg(any(target_os = "android", target_os = "unknown"))]
ExecutionMode::Remote(_pool) =>
ExecutionMode::ExternalProcessSelfHost(_) | ExecutionMode::ExternalProcessCustomHost { .. } =>
Err(ValidationError::Internal(InternalError::System(
Box::<dyn std::error::Error + Send + Sync>::from(
"Remote validator not available".to_string()
Expand All @@ -166,7 +184,7 @@ pub fn validate_candidate_internal(
) -> Result<ValidationResult, ValidationError> {
let executor = sc_executor::WasmExecutor::new(
#[cfg(not(any(target_os = "android", target_os = "unknown")))]
sc_executor::WasmExecutionMethod::Compiled,
sc_executor::WasmExecutionMethod::Interpreted,
Copy link
Member

@ordian ordian Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

may I ask why it was changed?

not sure how much of that code will be retained given #1609

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had compilation issue. Will revert it to see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous backport I had to remove the feature wasmtime because the CI failed like this: https://gitlab.parity.io/parity/polkadot/-/jobs/635768

And now it's failing because of:

error[E0599]: no variant or associated item named `Compiled` found for enum `WasmExecutionMethod` in the current scope
   --> parachain/src/wasm_executor/mod.rs:187:37
    |
187 |         sc_executor::WasmExecutionMethod::Compiled,
    |                                           ^^^^^^^^ variant or associated item not found in `WasmExecutionMethod`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `polkadot-parachain`.

So I talked to @bkchr and apparently I can ignore the failing test for now

cecton marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(any(target_os = "android", target_os = "unknown"))]
Default::default(),
// TODO: Make sure we don't use more than 1GB: https://github.com/paritytech/polkadot/issues/699
Expand Down
Loading