Unverified Commit 7f97b50a authored by Arkadiy Paronyan's avatar Arkadiy Paronyan Committed by GitHub
Browse files

Sort out validation errors (#1516)

* Sort out validation errors

* Typo

* Fixed wasm/android build

* Fixed bad merge
parent 5679bc89
Pipeline #102493 passed with stages
in 25 minutes and 44 seconds
......@@ -305,7 +305,7 @@ impl CandidateBackingJob {
}
}
}
ValidationResult::Invalid => {
ValidationResult::Invalid(_reason) => {
// no need to issue a statement about this if we aren't seconding it.
//
// there's an infinite amount of garbage out there. no need to acknowledge
......@@ -497,7 +497,7 @@ impl CandidateBackingJob {
Err(()) => Statement::Invalid(candidate_hash),
}
}
ValidationResult::Invalid => {
ValidationResult::Invalid(_reason) => {
Statement::Invalid(candidate_hash)
}
};
......@@ -826,6 +826,7 @@ mod tests {
messages::RuntimeApiRequest,
ActiveLeavesUpdate, FromOverseer, OverseerSignal,
};
use polkadot_node_primitives::InvalidCandidate;
use sp_keyring::Sr25519Keyring;
use std::collections::HashMap;
......@@ -1461,7 +1462,7 @@ mod tests {
tx,
)
) if pov == pov && &c == candidate_a.descriptor() => {
tx.send(Ok(ValidationResult::Invalid)).unwrap();
tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap();
}
);
......@@ -1597,7 +1598,7 @@ mod tests {
tx,
)
) if pov == pov && &c == candidate.descriptor() => {
tx.send(Ok(ValidationResult::Invalid)).unwrap();
tx.send(Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn))).unwrap();
}
);
......@@ -1729,7 +1730,7 @@ mod tests {
tx,
)
) if pov == pov && &c == candidate.descriptor() => {
tx.send(Err(ValidationFailed)).unwrap();
tx.send(Err(ValidationFailed("Internal test error".into()))).unwrap();
}
);
......
......@@ -28,12 +28,13 @@ use polkadot_subsystem::messages::{
AllMessages, CandidateValidationMessage, RuntimeApiMessage, ValidationFailed, RuntimeApiRequest,
};
use polkadot_subsystem::errors::RuntimeApiError;
use polkadot_node_primitives::{ValidationResult, ValidationOutputs};
use polkadot_node_primitives::{ValidationResult, ValidationOutputs, InvalidCandidate};
use polkadot_primitives::v1::{
ValidationCode, OmittedValidationData, PoV, CandidateDescriptor, LocalValidationData,
GlobalValidationData, OccupiedCoreAssumption, Hash, validation_data_hash,
};
use polkadot_parachain::wasm_executor::{self, ValidationPool, ExecutionMode};
use polkadot_parachain::wasm_executor::{self, ValidationPool, ExecutionMode, ValidationError,
InvalidCandidate as WasmInvalidCandidate};
use polkadot_parachain::primitives::{ValidationResult as WasmValidationResult, ValidationParams};
use parity_scale_codec::Encode;
......@@ -241,7 +242,7 @@ async fn spawn_validate_from_chain_state(
e,
);
return Ok(Err(ValidationFailed));
return Ok(Err(ValidationFailed("Error making API request".into())));
}
}
};
......@@ -264,7 +265,7 @@ async fn spawn_validate_from_chain_state(
).await;
}
AssumptionCheckOutcome::DoesNotMatch => {},
AssumptionCheckOutcome::BadRequest => return Ok(Err(ValidationFailed)),
AssumptionCheckOutcome::BadRequest => return Ok(Err(ValidationFailed("Bad request".into()))),
}
match check_assumption_validation_data(
......@@ -285,13 +286,13 @@ async fn spawn_validate_from_chain_state(
).await;
}
AssumptionCheckOutcome::DoesNotMatch => {},
AssumptionCheckOutcome::BadRequest => return Ok(Err(ValidationFailed)),
AssumptionCheckOutcome::BadRequest => return Ok(Err(ValidationFailed("Bad request".into()))),
}
// 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 validation_data_hash in the descriptor
// is not based on the relay parent and is thus invalid.
Ok(Ok(ValidationResult::Invalid))
Ok(Ok(ValidationResult::Invalid(InvalidCandidate::BadParent)))
}
async fn spawn_validate_exhaustive(
......@@ -321,52 +322,52 @@ async fn spawn_validate_exhaustive(
rx.await.map_err(Into::into)
}
/// Does basic checks of a candidate. Provide the encoded PoV-block. Returns `true` if basic checks
/// are passed, false otherwise.
fn passes_basic_checks(
/// Does basic checks of a candidate. Provide the encoded PoV-block. Returns `Ok` if basic checks
/// are passed, `Err` otherwise.
fn perform_basic_checks(
candidate: &CandidateDescriptor,
max_block_data_size: Option<u64>,
pov: &PoV,
) -> bool {
) -> Result<(), InvalidCandidate> {
let encoded_pov = pov.encode();
let hash = pov.hash();
if let Some(max_size) = max_block_data_size {
if encoded_pov.len() as u64 > max_size {
return false;
return Err(InvalidCandidate::ParamsTooLarge(encoded_pov.len() as u64));
}
}
if hash != candidate.pov_hash {
return false;
return Err(InvalidCandidate::HashMismatch);
}
if let Err(()) = candidate.check_collator_signature() {
return false;
return Err(InvalidCandidate::BadSignature);
}
true
Ok(())
}
/// Check the result of Wasm execution against the constraints given by the relay-chain.
///
/// Returns `true` if checks pass, false otherwise.
/// Returns `Ok(())` if checks pass, error otherwise.
fn check_wasm_result_against_constraints(
global_validation_data: &GlobalValidationData,
_local_validation_data: &LocalValidationData,
result: &WasmValidationResult,
) -> bool {
) -> Result<(), InvalidCandidate> {
if result.head_data.0.len() > global_validation_data.max_head_data_size as _ {
return false
return Err(InvalidCandidate::HeadDataTooLarge(result.head_data.0.len() as u64))
}
if let Some(ref code) = result.new_validation_code {
if code.0.len() > global_validation_data.max_code_size as _ {
return false
return Err(InvalidCandidate::NewCodeTooLarge(code.0.len() as u64))
}
}
true
Ok(())
}
trait ValidationBackend {
......@@ -377,7 +378,7 @@ trait ValidationBackend {
validation_code: &ValidationCode,
params: ValidationParams,
spawn: S,
) -> Result<WasmValidationResult, wasm_executor::Error>;
) -> Result<WasmValidationResult, ValidationError>;
}
struct RealValidationBackend;
......@@ -390,7 +391,7 @@ impl ValidationBackend for RealValidationBackend {
validation_code: &ValidationCode,
params: ValidationParams,
spawn: S,
) -> Result<WasmValidationResult, wasm_executor::Error> {
) -> Result<WasmValidationResult, ValidationError> {
let execution_mode = pool.as_ref()
.map(ExecutionMode::Remote)
.unwrap_or(ExecutionMode::Local);
......@@ -415,8 +416,8 @@ fn validate_candidate_exhaustive<B: ValidationBackend, S: SpawnNamed + 'static>(
pov: Arc<PoV>,
spawn: S,
) -> Result<ValidationResult, ValidationFailed> {
if !passes_basic_checks(&descriptor, None, &*pov) {
return Ok(ValidationResult::Invalid);
if let Err(e) = perform_basic_checks(&descriptor, None, &*pov) {
return Ok(ValidationResult::Invalid(e))
}
let OmittedValidationData { global_validation, local_validation } = omitted_validation;
......@@ -431,26 +432,36 @@ fn validate_candidate_exhaustive<B: ValidationBackend, S: SpawnNamed + 'static>(
};
match B::validate(backend_arg, &validation_code, params, spawn) {
Err(wasm_executor::Error::BadReturn) => Ok(ValidationResult::Invalid),
Err(_) => Err(ValidationFailed),
Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::Timeout)) =>
Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)),
Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::ParamsTooLarge(l))) =>
Ok(ValidationResult::Invalid(InvalidCandidate::ParamsTooLarge(l as u64))),
Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::CodeTooLarge(l))) =>
Ok(ValidationResult::Invalid(InvalidCandidate::CodeTooLarge(l as u64))),
Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::BadReturn)) =>
Ok(ValidationResult::Invalid(InvalidCandidate::BadReturn)),
Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::WasmExecutor(e))) =>
Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e.to_string()))),
Err(ValidationError::InvalidCandidate(WasmInvalidCandidate::ExternalWasmExecutor(e))) =>
Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e.to_string()))),
Err(ValidationError::Internal(e)) => Err(ValidationFailed(e.to_string())),
Ok(res) => {
let passes_post_checks = check_wasm_result_against_constraints(
let post_check_result = check_wasm_result_against_constraints(
&global_validation,
&local_validation,
&res,
);
Ok(if passes_post_checks {
ValidationResult::Valid(ValidationOutputs {
Ok(match post_check_result {
Ok(()) => ValidationResult::Valid(ValidationOutputs {
head_data: res.head_data,
global_validation_data: global_validation,
local_validation_data: local_validation,
upward_messages: res.upward_messages,
fees: 0,
new_validation_code: res.new_validation_code,
})
} else {
ValidationResult::Invalid
}),
Err(e) => ValidationResult::Invalid(e),
})
}
}
......@@ -469,7 +480,7 @@ mod tests {
struct MockValidationBackend;
struct MockValidationArg {
result: Result<WasmValidationResult, wasm_executor::Error>,
result: Result<WasmValidationResult, ValidationError>,
}
impl ValidationBackend for MockValidationBackend {
......@@ -480,7 +491,7 @@ mod tests {
_validation_code: &ValidationCode,
_params: ValidationParams,
_spawn: S,
) -> Result<WasmValidationResult, wasm_executor::Error> {
) -> Result<WasmValidationResult, ValidationError> {
arg.result
}
}
......@@ -795,7 +806,7 @@ mod tests {
descriptor.pov_hash = pov.hash();
collator_sign(&mut descriptor, Sr25519Keyring::Alice);
assert!(passes_basic_checks(&descriptor, Some(1024), &pov));
assert!(perform_basic_checks(&descriptor, Some(1024), &pov).is_ok());
let validation_result = WasmValidationResult {
head_data: HeadData(vec![1, 1, 1]),
......@@ -808,7 +819,7 @@ mod tests {
&omitted_validation.global_validation,
&omitted_validation.local_validation,
&validation_result,
));
).is_ok());
let v = validate_candidate_exhaustive::<MockValidationBackend, _>(
MockValidationArg { result: Ok(validation_result) },
......@@ -845,7 +856,7 @@ mod tests {
descriptor.pov_hash = pov.hash();
collator_sign(&mut descriptor, Sr25519Keyring::Alice);
assert!(passes_basic_checks(&descriptor, Some(1024), &pov));
assert!(perform_basic_checks(&descriptor, Some(1024), &pov).is_ok());
let validation_result = WasmValidationResult {
head_data: HeadData(vec![1, 1, 1]),
......@@ -858,10 +869,14 @@ mod tests {
&omitted_validation.global_validation,
&omitted_validation.local_validation,
&validation_result,
));
).is_ok());
let v = validate_candidate_exhaustive::<MockValidationBackend, _>(
MockValidationArg { result: Err(wasm_executor::Error::BadReturn) },
MockValidationArg {
result: Err(ValidationError::InvalidCandidate(
WasmInvalidCandidate::BadReturn
))
},
omitted_validation.clone(),
vec![1, 2, 3].into(),
descriptor,
......@@ -869,7 +884,7 @@ mod tests {
TaskExecutor::new(),
).unwrap();
assert_matches!(v, ValidationResult::Invalid);
assert_matches!(v, ValidationResult::Invalid(InvalidCandidate::BadReturn));
}
......@@ -889,7 +904,7 @@ mod tests {
descriptor.pov_hash = pov.hash();
collator_sign(&mut descriptor, Sr25519Keyring::Alice);
assert!(passes_basic_checks(&descriptor, Some(1024), &pov));
assert!(perform_basic_checks(&descriptor, Some(1024), &pov).is_ok());
let validation_result = WasmValidationResult {
head_data: HeadData(vec![1, 1, 1]),
......@@ -902,10 +917,14 @@ mod tests {
&omitted_validation.global_validation,
&omitted_validation.local_validation,
&validation_result,
));
).is_ok());
let v = validate_candidate_exhaustive::<MockValidationBackend, _>(
MockValidationArg { result: Err(wasm_executor::Error::Timeout) },
MockValidationArg {
result: Err(ValidationError::InvalidCandidate(
WasmInvalidCandidate::Timeout
))
},
omitted_validation.clone(),
vec![1, 2, 3].into(),
descriptor,
......@@ -913,6 +932,6 @@ mod tests {
TaskExecutor::new(),
);
assert_matches!(v, Err(ValidationFailed));
assert_matches!(v, Ok(ValidationResult::Invalid(InvalidCandidate::Timeout)));
}
}
......@@ -129,13 +129,38 @@ pub struct ValidationOutputs {
pub new_validation_code: Option<ValidationCode>,
}
/// Candidate invalidity details
#[derive(Debug)]
pub enum InvalidCandidate {
/// Failed to execute.`validate_block`. This includes function panicking.
ExecutionError(String),
/// Execution timeout.
Timeout,
/// Validation input is over the limit.
ParamsTooLarge(u64),
/// Code size is over the limit.
CodeTooLarge(u64),
/// Validation function returned invalid data.
BadReturn,
/// Invalid relay chain parent.
BadParent,
/// POV hash does not match.
HashMismatch,
/// Bad collator signature.
BadSignature,
/// Output code is too large
NewCodeTooLarge(u64),
/// Head-data is over the limit.
HeadDataTooLarge(u64),
}
/// Result of the validation of the candidate.
#[derive(Debug)]
pub enum ValidationResult {
/// Candidate is valid. The validation process yields these outputs.
Valid(ValidationOutputs),
/// Candidate is invalid.
Invalid,
Invalid(InvalidCandidate),
}
impl std::convert::TryFrom<FromTableMisbehavior> for MisbehaviorReport {
......
......@@ -87,9 +87,9 @@ impl CandidateBackingMessage {
}
}
/// Blanket error for validation failing.
/// Blanket error for validation failing for internal reasons.
#[derive(Debug)]
pub struct ValidationFailed;
pub struct ValidationFailed(pub String);
/// Messages received by the Validation subsystem.
///
......
......@@ -69,9 +69,18 @@ pub enum ExecutionMode<'a> {
RemoteTest(&'a ValidationPool),
}
/// Error type for the wasm executor
#[derive(Debug, derive_more::Display, derive_more::From)]
pub enum Error {
/// Candidate validation error.
pub enum ValidationError {
/// Validation failed due to internal reasons. The candidate might still be valid.
Internal(InternalError),
/// Candidate is invalid.
InvalidCandidate(InvalidCandidate),
}
/// Error type that indicates invalid candidate.
#[derive(Debug, derive_more::Display, derive_more::From)]
pub enum InvalidCandidate {
/// Wasm executor error.
#[display(fmt = "WASM executor error: {:?}", _0)]
WasmExecutor(sc_executor::error::Error),
......@@ -82,30 +91,37 @@ pub enum Error {
/// Code size it too large.
#[display(fmt = "WASM code is {} bytes, max allowed is {}", _0, MAX_CODE_MEM)]
CodeTooLarge(usize),
/// Bad return data or type.
/// Error decoding returned data.
#[display(fmt = "Validation function returned invalid data.")]
BadReturn,
#[display(fmt = "Validation function timeout.")]
Timeout,
#[display(fmt = "External WASM execution error: {}", _0)]
ExternalWasmExecutor(String),
}
/// Host error during candidate validation. This does not indicate an invalid candidate.
#[derive(Debug, derive_more::Display, derive_more::From)]
pub enum InternalError {
#[display(fmt = "IO error: {}", _0)]
Io(std::io::Error),
#[display(fmt = "System error: {}", _0)]
System(Box<dyn std::error::Error + Send>),
#[display(fmt = "WASM worker error: {}", _0)]
External(String),
#[display(fmt = "Shared memory error: {}", _0)]
#[cfg(not(any(target_os = "android", target_os = "unknown")))]
SharedMem(shared_memory::SharedMemError),
#[display(fmt = "WASM worker error: {}", _0)]
WasmWorker(String),
}
impl std::error::Error for Error {
impl std::error::Error for ValidationError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
Error::WasmExecutor(ref err) => Some(err),
Error::Io(ref err) => Some(err),
Error::System(ref err) => Some(&**err),
ValidationError::Internal(InternalError::Io(ref err)) => Some(err),
ValidationError::Internal(InternalError::System(ref err)) => Some(&**err),
#[cfg(not(any(target_os = "android", target_os = "unknown")))]
Error::SharedMem(ref err) => Some(err),
ValidationError::Internal(InternalError::SharedMem(ref err)) => Some(err),
ValidationError::InvalidCandidate(InvalidCandidate::WasmExecutor(ref err)) => Some(err),
_ => None,
}
}
......@@ -119,7 +135,7 @@ pub fn validate_candidate(
params: ValidationParams,
options: ExecutionMode<'_>,
spawner: impl SpawnNamed + 'static,
) -> Result<ValidationResult, Error> {
) -> Result<ValidationResult, ValidationError> {
match options {
ExecutionMode::Local => {
validate_candidate_internal(validation_code, &params.encode(), spawner)
......@@ -133,15 +149,19 @@ pub fn validate_candidate(
pool.validate_candidate(validation_code, params, true)
},
#[cfg(any(target_os = "android", target_os = "unknown"))]
ExecutionMode::Remote(pool) =>
Err(Error::System(Box::<dyn std::error::Error + Send + Sync>::from(
"Remote validator not available".to_string()
) as Box<_>)),
ExecutionMode::Remote(_pool) =>
Err(ValidationError::Internal(InternalError::System(
Box::<dyn std::error::Error + Send + Sync>::from(
"Remote validator not available".to_string()
) as Box<_>
))),
#[cfg(any(target_os = "android", target_os = "unknown"))]
ExecutionMode::RemoteTest(pool) =>
Err(Error::System(Box::<dyn std::error::Error + Send + Sync>::from(
"Remote validator not available".to_string()
) as Box<_>)),
ExecutionMode::RemoteTest(_pool) =>
Err(ValidationError::Internal(InternalError::System(
Box::<dyn std::error::Error + Send + Sync>::from(
"Remote validator not available".to_string()
) as Box<_>
))),
}
}
......@@ -155,7 +175,7 @@ pub fn validate_candidate_internal(
validation_code: &[u8],
encoded_call_data: &[u8],
spawner: impl SpawnNamed + 'static,
) -> Result<ValidationResult, Error> {
) -> Result<ValidationResult, ValidationError> {
let mut extensions = Extensions::new();
extensions.register(sp_core::traits::TaskExecutorExt::new(spawner));
......@@ -175,9 +195,10 @@ pub fn validate_candidate_internal(
encoded_call_data,
&mut ext,
sp_core::traits::MissingHostFunctions::Allow,
)?;
).map_err(|e| ValidationError::InvalidCandidate(e.into()))?;
ValidationResult::decode(&mut &res[..]).map_err(|_| Error::BadReturn.into())
ValidationResult::decode(&mut &res[..])
.map_err(|_| ValidationError::InvalidCandidate(InvalidCandidate::BadReturn).into())
}
/// The validation externalities that will panic on any storage related access. They just provide
......
......@@ -19,7 +19,8 @@
use std::{process, env, sync::Arc, sync::atomic};
use codec::{Decode, Encode};
use crate::primitives::{ValidationParams, ValidationResult};
use super::{validate_candidate_internal, Error, MAX_CODE_MEM, MAX_RUNTIME_MEM};
use super::{validate_candidate_internal, ValidationError, InvalidCandidate, InternalError,
MAX_CODE_MEM, MAX_RUNTIME_MEM};
use shared_memory::{SharedMem, SharedMemConf, EventState, WriteLockable, EventWait, EventSet};
use parking_lot::Mutex;
use log::{debug, trace};
......@@ -88,7 +89,7 @@ impl ValidationPool {
validation_code: &[u8],
params: ValidationParams,
test_mode: bool,
) -> Result<ValidationResult, Error> {
) -> Result<ValidationResult, ValidationError> {
for host in self.hosts.iter() {
if let Some(mut host) = host.try_lock() {
return host.validate_candidate(validation_code, params, test_mode);
......@@ -165,7 +166,10 @@ pub fn run_worker(mem_id: &str) -> Result<(), String> {
match result {
Ok(r) => ValidationResultHeader::Ok(r),
Err(e) => ValidationResultHeader::Error(e.to_string()),
Err(ValidationError::Internal(e)) =>
ValidationResultHeader::Error(WorkerValidationError::InternalError(e.to_string())),
Err(ValidationError::InvalidCandidate(e)) =>
ValidationResultHeader::Error(WorkerValidationError::ValidationError(e.to_string())),
}
};
let mut data: &mut[u8] = &mut **slice;
......@@ -186,9 +190,15 @@ struct ValidationHeader {
}
#[derive(Encode, Decode, Debug)]
pub enum ValidationResultHeader {
enum WorkerValidationError {
InternalError(String),
ValidationError(String),
}
#[derive(Encode, Decode, Debug)]
enum ValidationResultHeader {
Ok(ValidationResult),
Error(String),
Error(WorkerValidationError),
}
unsafe impl Send for ValidationHost {}
......@@ -209,7 +219,7 @@ impl Drop for ValidationHost {
}
impl ValidationHost {
fn create_memory() -> Result<SharedMem, Error> {
fn create_memory() -> Result<SharedMem, InternalError> {
let mem_size = MAX_RUNTIME_MEM + MAX_CODE_MEM + 1024;
let mem_config = SharedMemConf::default()
.set_size(mem_size)
......@@ -221,7 +231,7 @@ impl ValidationHost {
Ok(mem_config.create()?)
}
fn start_worker(&mut self, test_mode: bool) -> Result<(), Error> {
fn start_worker(&mut self, test_mode: bool) -> Result<(), InternalError> {
if let Some(ref mut worker) = self.worker {
// Check if still alive
if let Ok(None) = worker.try_wait() {
......@@ -257,9 +267,9 @@ impl ValidationHost {
validation_code: &[u8],
params: ValidationParams,
test_mode: bool,
) -> Result<ValidationResult, Error> {
) -> Result<ValidationResult, ValidationError> {
if validation_code.len() > MAX_CODE_MEM {
return Err(Error::CodeTooLarge(validation_code.len()));
return Err(ValidationError::InvalidCandidate(InvalidCandidate::CodeTooLarge(validation_code.len())));
}
// First, check if need to spawn the child process
self.start_worker(test_mode)?;
......@@ -267,7 +277,8 @@ impl ValidationHost {
.expect("memory is always `Some` after `start_worker` completes successfully");
{
// Put data in shared mem
let data: &mut[u8] = &mut **memory.wlock_as_slice(0)?;
let data: &mut[u8] = &mut **memory.wlock_as_slice(0)
.map_err(|e|ValidationError::Internal(e.into()))?;
let (mut header_buf, rest) = data.split_at_mut(1024);
let (code, rest) = rest.split_at_mut(MAX_CODE_MEM);
let (code, _) = code.split_at_mut(validation_code.len());
......@@ -275,7 +286,7 @@ impl ValidationHost {