Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check executor params coherence #1774

Merged
merged 9 commits into from
Oct 13, 2023
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
overhaul ExecutorParams::check_consistency()
eagr committed Oct 3, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 6c8cdfdf9919e1587b7b38889c86fc4fe482d1be
2 changes: 1 addition & 1 deletion polkadot/node/core/pvf/common/src/executor_intf.rs
Original file line number Diff line number Diff line change
@@ -107,7 +107,7 @@ pub fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result<Semantics, S
match p {
ExecutorParam::MaxMemoryPages(max_pages) =>
sem.heap_alloc_strategy =
HeapAllocStrategy::Dynamic { maximum_pages: Some(*max_pages) },
HeapAllocStrategy::Dynamic { maximum_pages: Some((*max_pages).saturating_add(DEFAULT_HEAP_PAGES_ESTIMATE)) },
ExecutorParam::StackLogicalMax(slm) => stack_limit.logical_max = *slm,
ExecutorParam::StackNativeMax(snm) => stack_limit.native_stack_max = *snm,
ExecutorParam::WasmExtBulkMemory => sem.wasm_bulk_memory = true,
2 changes: 1 addition & 1 deletion polkadot/primitives/src/lib.rs
Original file line number Diff line number Diff line change
@@ -43,7 +43,7 @@ pub use v6::{
CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CollatorId,
CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex,
CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, EncodeAs,
ExecutorParam, ExecutorParams, ExecutorParamsHash, ExplicitDisputeStatement, GroupIndex,
ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, ExplicitDisputeStatement, GroupIndex,
GroupRotationInfo, Hash, HashT, HeadData, Header, HorizontalMessages, HrmpChannelId, Id,
InboundDownwardMessage, InboundHrmpMessage, IndexedVec, InherentData,
InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, OccupiedCore,
183 changes: 85 additions & 98 deletions polkadot/primitives/src/v6/executor_params.rs
Original file line number Diff line number Diff line change
@@ -26,30 +26,33 @@ use parity_scale_codec::{Decode, Encode};
use polkadot_core_primitives::Hash;
use scale_info::TypeInfo;
use serde::{Deserialize, Serialize};
use sp_std::{ops::Deref, time::Duration, vec, vec::Vec};
use sp_std::{collections::btree_map::BTreeMap, ops::Deref, time::Duration, vec, vec::Vec};

const MEMORY_PAGES_LIMIT: u32 = 65536;
const SHADOW_STACK_PAGES: u32 = 32;
const MEMORY_PAGES_MAX: u32 = 65536;
const LOGICAL_MAX_LO: u32 = 1024;
const LOGICAL_MAX_HI: u32 = 2 * 65536;
const PRECHECK_MEM_MAX_LO: u64 = 256 * 1024 * 1024;
const PRECHECK_MEM_MAX_HI: u64 = 16 * 1024 * 1024 * 1024;

/// The different executor parameters for changing the execution environment semantics.
#[derive(Clone, Debug, Encode, Decode, PartialEq, Eq, TypeInfo, Serialize, Deserialize)]
pub enum ExecutorParam {
/// Maximum number of memory pages (64KiB bytes per page) the executor can allocate.
/// A valid value lies within (0, 65536 + 32].
/// A valid value lies within (0, 65536].
#[codec(index = 1)]
MaxMemoryPages(u32),
/// Wasm logical stack size limit (max. number of Wasm values on stack).
/// A valid value lies within [1024, 2 * 65536].
#[codec(index = 2)]
StackLogicalMax(u32),
/// Executor machine stack size limit, in bytes.
/// If `StackLogicalMax` is also present, a valid value lies within
/// [128 * logical_max, 512 * logical_max].
/// If `StackLogicalMax` is also present, a valid value ranges from 128 * `logical_max`
/// to 512 * `logical_max`.
#[codec(index = 3)]
StackNativeMax(u32),
/// Max. amount of memory the preparation worker is allowed to use during
/// pre-checking, in bytes.
/// A valid max memory ranges from 256MB to 16GB.
/// Valid max. memory ranges from 256MB to 16GB.
#[codec(index = 4)]
PrecheckingMaxMemory(u64),
/// PVF preparation timeouts, millisec
@@ -67,11 +70,15 @@ pub enum ExecutorParam {
WasmExtBulkMemory,
}

/// Possible inconsistencies of executor params.
#[derive(Debug)]
pub enum ExecutorParamError {
/// A param is duplicated.
DuplicatedParam(&'static str),
/// A param value exceeds its limitation.
LimitExceeded(&'static str),
IncompatibleValues(&'static str, &'static str)
/// Two param values are incompatible or senseless when put together.
IncompatibleValues(&'static str, &'static str),
}

/// Unit type wrapper around [`type@Hash`] that represents an execution parameter set hash.
@@ -131,7 +138,7 @@ impl ExecutorParams {
for param in &self.0 {
if let ExecutorParam::PvfPrepTimeout(k, timeout) = param {
if kind == *k {
return Some(Duration::from_millis(*timeout))
return Some(Duration::from_millis(*timeout));
}
}
}
@@ -143,35 +150,41 @@ impl ExecutorParams {
for param in &self.0 {
if let ExecutorParam::PvfExecTimeout(k, timeout) = param {
if kind == *k {
return Some(Duration::from_millis(*timeout))
return Some(Duration::from_millis(*timeout));
}
}
}
None
}

// FIXME Should it be
// pub fn check_consistency(&self) -> Result<(), ExecutorParamError>, which would be simpler.
// I guess it depends on whether they could be considered "warnings".

/// Check params coherence
pub fn check_consistency(&self) -> Vec<ExecutorParamError> {
/// Check params coherence.
pub fn check_consistency(&self) -> Result<(), ExecutorParamError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have this logic right in configuration.rs? This file seems to be mainly defining a bunch of types with not much actual logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it would be nice to if we could put the checks in one place. But that may require opening up the internal of ExecutorParams, do you think it's worth it? @mrcnski

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean exactly, but I think it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was thinking that we might need to make the internal state public in order to do the checks in another crate, like pub struct ExecutorParams(pub Vec<ExecutorParam>). Then it's possible that other crates may rely on the internal state which is not great.

I just realized that it comes with an iterator :)

But still, for the sake of separation of concerns, I think it wouldn't be a bad idea that the type does its own checks. Like, what if the checks are needed else where (other than configurations.rs)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. :)

use ExecutorParam::*;
use ExecutorParamError::*;

let mut errors = Vec::with_capacity(8);
let mut seen = BTreeMap::<&str, u64>::new();

let mut max_pages = false;
let mut logical_max: Option<u32> = None;
let mut native_max: Option<u32> = None;
let mut pvf_mem_max = false;
let mut pvf_prep_precheck: Option<u64> = None;
let mut pvf_prep_lenient: Option<u64> = None;
let mut pvf_exec_backing: Option<u64> = None;
let mut pvf_exec_approval: Option<u64> = None;
let mut enable_bulk_mem = false;
macro_rules! check {
($param:ident, $val:expr $(,)?) => {
if seen.contains_key($param) {
return Err(DuplicatedParam($param));
}
seen.insert($param, $val as u64);
};

($param:ident, $val:expr, $out_of_limit:expr $(,)?) => {
if seen.contains_key($param) {
return Err(DuplicatedParam($param));
}
if $out_of_limit {
return Err(LimitExceeded($param));
}
seen.insert($param, $val as u64);
};
}

for param in &self.0 {
// should ensure to be unique
let param_ident = match *param {
MaxMemoryPages(_) => "MaxMemoryPages",
StackLogicalMax(_) => "StackLogicalMax",
@@ -188,98 +201,72 @@ impl ExecutorParams {
WasmExtBulkMemory => "WasmExtBulkMemory",
};

// FIXME report each kind of duplication only once
match *param {
MaxMemoryPages(max) => if max_pages {
errors.push(DuplicatedParam(param_ident));
} else {
max_pages = true;
if max <= 0 || max > MEMORY_PAGES_LIMIT + SHADOW_STACK_PAGES {
errors.push(LimitExceeded(param_ident));
}
}
MaxMemoryPages(val) => {
check!(param_ident, val, val <= 0 || val > MEMORY_PAGES_MAX,);
},

StackLogicalMax(max) => if logical_max.is_some() {
errors.push(DuplicatedParam(param_ident));
} else {
logical_max = Some(max);
if max < 1024 || max > 2 * 65536 {
errors.push(LimitExceeded(param_ident));
}
}
StackLogicalMax(val) => {
check!(param_ident, val, val < LOGICAL_MAX_LO || val > LOGICAL_MAX_HI,);
},

StackNativeMax(max) => if native_max.is_some() {
errors.push(DuplicatedParam(param_ident));
} else {
native_max = Some(max);
}
StackNativeMax(val) => {
check!(param_ident, val);
},

// FIXME upper bound
PrecheckingMaxMemory(max) => if pvf_mem_max {
errors.push(DuplicatedParam(param_ident));
} else {
pvf_mem_max = true;
if max < 256 * 1024 * 1024 || max > 16 * 1024 * 1024 * 1024 {
errors.push(LimitExceeded(param_ident));
}
}
PrecheckingMaxMemory(val) => {
check!(
param_ident,
val,
val < PRECHECK_MEM_MAX_LO || val > PRECHECK_MEM_MAX_HI,
);
},

// FIXME upper bounds
PvfPrepTimeout(kind, timeout) => match kind {
PvfPrepTimeoutKind::Precheck => if pvf_prep_precheck.is_some() {
errors.push(DuplicatedParam(param_ident));
} else {
pvf_prep_precheck = Some(timeout);
}
PvfPrepTimeoutKind::Lenient => if pvf_prep_lenient.is_some() {
errors.push(DuplicatedParam(param_ident));
} else {
pvf_prep_lenient = Some(timeout);
}
}
PvfPrepTimeout(_, val) => {
check!(param_ident, val);
},

// FIXME upper bounds
PvfExecTimeout(kind, timeout) => match kind {
PvfExecTimeoutKind::Backing => if pvf_exec_backing.is_some() {
errors.push(DuplicatedParam(param_ident));
} else {
pvf_exec_backing = Some(timeout);
}
PvfExecTimeoutKind::Approval => if pvf_exec_approval.is_some() {
errors.push(DuplicatedParam(param_ident));
} else {
pvf_exec_approval = Some(timeout);
}
}
PvfExecTimeout(_, val) => {
check!(param_ident, val);
},

WasmExtBulkMemory => if enable_bulk_mem {
errors.push(DuplicatedParam(param_ident));
} else {
enable_bulk_mem = true;
}
WasmExtBulkMemory => {
check!(param_ident, 0);
},
}

// FIXME is it valid if only one is present?
if let (Some(lm), Some(nm)) = (logical_max, native_max) {
if nm < 128 * lm || nm > 512 * lm {
errors.push(IncompatibleValues("StackLogicalMax", "StackNativeMax"));
if let (Some(lm), Some(nm)) = (seen.get("StackLogicalMax"), seen.get("StackNativeMax"))
{
if *nm < 128 * *lm {
return Err(IncompatibleValues("StackLogicalMax", "StackNativeMax"));
}
}

if let (Some(precheck), Some(lenient)) = (pvf_prep_precheck, pvf_prep_lenient) {
if precheck >= lenient {
errors.push(IncompatibleValues("PvfPrepTimeoutKind::Precheck", "PvfPrepTimeoutKind::Lenient"));
if let (Some(precheck), Some(lenient)) =
(seen.get("PvfPrepTimeoutKind::Precheck"), seen.get("PvfPrepTimeoutKind::Lenient"))
{
if *precheck >= *lenient {
return Err(IncompatibleValues(
"PvfPrepTimeoutKind::Precheck",
"PvfPrepTimeoutKind::Lenient",
));
}
}

if let (Some(backing), Some(approval)) = (pvf_exec_backing, pvf_exec_approval) {
if backing >= approval {
errors.push(IncompatibleValues("PvfExecTimeoutKind::Backing", "PvfExecTimeoutKind::Approval"));
if let (Some(backing), Some(approval)) =
(seen.get("PvfExecTimeoutKind::Backing"), seen.get("PvfExecTimeoutKind::Approval"))
{
if *backing >= *approval {
return Err(IncompatibleValues(
"PvfExecTimeoutKind::Backing",
"PvfExecTimeoutKind::Approval",
));
}
}
}

errors
Ok(())
}
}

Loading