From e237da08c1f286763afeeb970e3b711f55a1f630 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Tue, 3 Oct 2023 12:33:30 +0800 Subject: [PATCH 1/8] check executor params coherence --- polkadot/primitives/src/v6/executor_params.rs | 157 +++++++++++++++++- .../runtime/parachains/src/configuration.rs | 11 +- 2 files changed, 163 insertions(+), 5 deletions(-) diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 6fbf3037fd6c..b0d7bdab06f4 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -28,26 +28,38 @@ use scale_info::TypeInfo; use serde::{Deserialize, Serialize}; use sp_std::{ops::Deref, time::Duration, vec, vec::Vec}; +const MEMORY_PAGES_LIMIT: u32 = 65536; +const SHADOW_STACK_PAGES: u32 = 32; + /// 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]. #[codec(index = 1)] MaxMemoryPages(u32), - /// Wasm logical stack size limit (max. number of Wasm values on stack) + /// 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 + /// Executor machine stack size limit, in bytes. + /// If `StackLogicalMax` is also present, a valid value lies within + /// [128 * logical_max, 512 * logical_max]. #[codec(index = 3)] StackNativeMax(u32), /// Max. amount of memory the preparation worker is allowed to use during - /// pre-checking, in bytes + /// pre-checking, in bytes. + /// A valid max memory ranges from 256MB to 16GB. #[codec(index = 4)] PrecheckingMaxMemory(u64), /// PVF preparation timeouts, millisec + /// If both `PvfPrepTimeoutKind::Precheck` and `PvfPrepTimeoutKind::Lenient` are present, + /// ensure that `precheck` < `lenient`. #[codec(index = 5)] PvfPrepTimeout(PvfPrepTimeoutKind, u64), /// PVF execution timeouts, millisec + /// If both `PvfExecTimeoutKind::Backing` and `PvfExecTimeoutKind::Approval` are present, + /// ensure that `backing` < `approval`. #[codec(index = 6)] PvfExecTimeout(PvfExecTimeoutKind, u64), /// Enables WASM bulk memory proposal @@ -55,6 +67,13 @@ pub enum ExecutorParam { WasmExtBulkMemory, } +#[derive(Debug)] +pub enum ExecutorParamError { + DuplicatedParam(&'static str), + LimitExceeded(&'static str), + IncompatibleValues(&'static str, &'static str) +} + /// Unit type wrapper around [`type@Hash`] that represents an execution parameter set hash. /// /// This type is produced by [`ExecutorParams::hash`]. @@ -130,6 +149,138 @@ impl ExecutorParams { } 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 { + use ExecutorParam::*; + use ExecutorParamError::*; + + let mut errors = Vec::with_capacity(8); + + let mut max_pages = false; + let mut logical_max: Option = None; + let mut native_max: Option = None; + let mut pvf_mem_max = false; + let mut pvf_prep_precheck: Option = None; + let mut pvf_prep_lenient: Option = None; + let mut pvf_exec_backing: Option = None; + let mut pvf_exec_approval: Option = None; + let mut enable_bulk_mem = false; + + for param in &self.0 { + let param_ident = match *param { + MaxMemoryPages(_) => "MaxMemoryPages", + StackLogicalMax(_) => "StackLogicalMax", + StackNativeMax(_) => "StackNativeMax", + PrecheckingMaxMemory(_) => "PrecheckingMaxMemory", + PvfPrepTimeout(kind, _) => match kind { + PvfPrepTimeoutKind::Precheck => "PvfPrepTimeoutKind::Precheck", + PvfPrepTimeoutKind::Lenient => "PvfPrepTimeoutKind::Lenient", + }, + PvfExecTimeout(kind, _) => match kind { + PvfExecTimeoutKind::Backing => "PvfExecTimeoutKind::Backing", + PvfExecTimeoutKind::Approval => "PvfExecTimeoutKind::Approval", + }, + 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)); + } + } + + 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)); + } + } + + StackNativeMax(max) => if native_max.is_some() { + errors.push(DuplicatedParam(param_ident)); + } else { + native_max = Some(max); + } + + // 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)); + } + } + + // 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); + } + } + + // 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); + } + } + + WasmExtBulkMemory => if enable_bulk_mem { + errors.push(DuplicatedParam(param_ident)); + } else { + enable_bulk_mem = true; + } + } + + // 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(precheck), Some(lenient)) = (pvf_prep_precheck, pvf_prep_lenient) { + if precheck >= lenient { + errors.push(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")); + } + } + } + + errors + } } impl Deref for ExecutorParams { diff --git a/polkadot/runtime/parachains/src/configuration.rs b/polkadot/runtime/parachains/src/configuration.rs index f53f986a553f..be357d3ff3f1 100644 --- a/polkadot/runtime/parachains/src/configuration.rs +++ b/polkadot/runtime/parachains/src/configuration.rs @@ -26,8 +26,9 @@ use polkadot_parachain_primitives::primitives::{ MAX_HORIZONTAL_MESSAGE_NUM, MAX_UPWARD_MESSAGE_NUM, }; use primitives::{ - AsyncBackingParams, Balance, ExecutorParams, SessionIndex, LEGACY_MIN_BACKING_VOTES, - MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, + AsyncBackingParams, Balance, ExecutorParamError, ExecutorParams, SessionIndex, + LEGACY_MIN_BACKING_VOTES, MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE, MAX_POV_SIZE, + ON_DEMAND_DEFAULT_QUEUE_MAX_SIZE, }; use sp_runtime::{traits::Zero, Perbill}; use sp_std::prelude::*; @@ -348,6 +349,8 @@ pub enum InconsistentError { MaxHrmpInboundChannelsExceeded, /// `minimum_backing_votes` is set to zero. ZeroMinimumBackingVotes, + /// `executor_params` are incoherent. + ExecutorParamErrors(Vec), } impl HostConfiguration @@ -432,6 +435,10 @@ where return Err(ZeroMinimumBackingVotes) } + if let errors = self.executor_params.check_consistency() && errors.len() > 0 { + return Err(ExecutorParamErrors(errors)) + } + Ok(()) } From 6c8cdfdf9919e1587b7b38889c86fc4fe482d1be Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 4 Oct 2023 02:33:56 +0800 Subject: [PATCH 2/8] overhaul ExecutorParams::check_consistency() --- .../node/core/pvf/common/src/executor_intf.rs | 2 +- polkadot/primitives/src/lib.rs | 2 +- polkadot/primitives/src/v6/executor_params.rs | 183 ++++++++---------- polkadot/primitives/src/v6/mod.rs | 73 +++---- .../runtime/parachains/src/configuration.rs | 40 ++-- 5 files changed, 147 insertions(+), 153 deletions(-) diff --git a/polkadot/node/core/pvf/common/src/executor_intf.rs b/polkadot/node/core/pvf/common/src/executor_intf.rs index 79839149ebdf..e27ff3adf4f8 100644 --- a/polkadot/node/core/pvf/common/src/executor_intf.rs +++ b/polkadot/node/core/pvf/common/src/executor_intf.rs @@ -107,7 +107,7 @@ pub fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result 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, diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index 5adb6d253134..116d5bce2e07 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -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, diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index b0d7bdab06f4..4805b07b8019 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -26,16 +26,19 @@ 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). @@ -43,13 +46,13 @@ pub enum ExecutorParam { #[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 { + /// Check params coherence. + pub fn check_consistency(&self) -> Result<(), ExecutorParamError> { 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 = None; - let mut native_max: Option = None; - let mut pvf_mem_max = false; - let mut pvf_prep_precheck: Option = None; - let mut pvf_prep_lenient: Option = None; - let mut pvf_exec_backing: Option = None; - let mut pvf_exec_approval: Option = 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(()) } } diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index cf9008355178..5e651fa899fb 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -62,7 +62,7 @@ pub mod executor_params; pub mod slashing; pub use async_backing::AsyncBackingParams; -pub use executor_params::{ExecutorParam, ExecutorParams, ExecutorParamsHash}; +pub use executor_params::{ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash}; mod metrics; pub use metrics::{ @@ -750,11 +750,11 @@ pub fn check_candidate_backing + Clone + Encode>( validator_lookup: impl Fn(usize) -> Option, ) -> Result { if backed.validator_indices.len() != group_len { - return Err(()) + return Err(()); } if backed.validity_votes.len() > group_len { - return Err(()) + return Err(()); } // this is known, even in runtime, to be blake2-256. @@ -775,12 +775,12 @@ pub fn check_candidate_backing + Clone + Encode>( if sig.verify(&payload[..], &validator_id) { signed += 1; } else { - return Err(()) + return Err(()); } } if signed != backed.validity_votes.len() { - return Err(()) + return Err(()); } Ok(signed) @@ -854,10 +854,10 @@ impl GroupRotationInfo { /// `core_index` should be less than `cores`, which is capped at `u32::max()`. pub fn group_for_core(&self, core_index: CoreIndex, cores: usize) -> GroupIndex { if self.group_rotation_frequency == 0 { - return GroupIndex(core_index.0) + return GroupIndex(core_index.0); } if cores == 0 { - return GroupIndex(0) + return GroupIndex(0); } let cores = sp_std::cmp::min(cores, u32::MAX as usize); @@ -876,10 +876,10 @@ impl GroupRotationInfo { /// `core_index` should be less than `cores`, which is capped at `u32::max()`. pub fn core_for_group(&self, group_index: GroupIndex, cores: usize) -> CoreIndex { if self.group_rotation_frequency == 0 { - return CoreIndex(group_index.0) + return CoreIndex(group_index.0); } if cores == 0 { - return CoreIndex(0) + return CoreIndex(0); } let cores = sp_std::cmp::min(cores, u32::MAX as usize); @@ -911,15 +911,15 @@ impl GroupRotationInfo { /// is 10 and the rotation frequency is 5, this should return 15. pub fn next_rotation_at(&self) -> N { let cycle_once = self.now + self.group_rotation_frequency; - cycle_once - - (cycle_once.saturating_sub(self.session_start_block) % self.group_rotation_frequency) + cycle_once + - (cycle_once.saturating_sub(self.session_start_block) % self.group_rotation_frequency) } /// Returns the block number of the last rotation before or including the current block. If the /// current block is 10 and the rotation frequency is 5, this should return 10. pub fn last_rotation_at(&self) -> N { - self.now - - (self.now.saturating_sub(self.session_start_block) % self.group_rotation_frequency) + self.now + - (self.now.saturating_sub(self.session_start_block) % self.group_rotation_frequency) } } @@ -1218,8 +1218,9 @@ impl ConsensusLog { digest_item: &runtime_primitives::DigestItem, ) -> Result, parity_scale_codec::Error> { match digest_item { - runtime_primitives::DigestItem::Consensus(id, encoded) if id == &POLKADOT_ENGINE_ID => - Ok(Some(Self::decode(&mut &encoded[..])?)), + runtime_primitives::DigestItem::Consensus(id, encoded) if id == &POLKADOT_ENGINE_ID => { + Ok(Some(Self::decode(&mut &encoded[..])?)) + }, _ => Ok(None), } } @@ -1248,23 +1249,27 @@ impl DisputeStatement { /// Get the payload data for this type of dispute statement. pub fn payload_data(&self, candidate_hash: CandidateHash, session: SessionIndex) -> Vec { match *self { - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) => - ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload(), + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) => { + ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload() + }, DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded( inclusion_parent, )) => CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext { session_index: session, parent_hash: inclusion_parent, }), - DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) => + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) => { CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext { session_index: session, parent_hash: inclusion_parent, - }), - DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) => - ApprovalVote(candidate_hash).signing_payload(session), - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) => - ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload(), + }) + }, + DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) => { + ApprovalVote(candidate_hash).signing_payload(session) + }, + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) => { + ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload() + }, } } @@ -1304,11 +1309,11 @@ impl DisputeStatement { /// Statement is backing statement. pub fn is_backing(&self) -> bool { match *self { - Self::Valid(ValidDisputeStatementKind::BackingSeconded(_)) | - Self::Valid(ValidDisputeStatementKind::BackingValid(_)) => true, - Self::Valid(ValidDisputeStatementKind::Explicit) | - Self::Valid(ValidDisputeStatementKind::ApprovalChecking) | - Self::Invalid(_) => false, + Self::Valid(ValidDisputeStatementKind::BackingSeconded(_)) + | Self::Valid(ValidDisputeStatementKind::BackingValid(_)) => true, + Self::Valid(ValidDisputeStatementKind::Explicit) + | Self::Valid(ValidDisputeStatementKind::ApprovalChecking) + | Self::Invalid(_) => false, } } } @@ -1480,10 +1485,12 @@ impl ValidityAttestation { signing_context: &SigningContext, ) -> Vec { match *self { - ValidityAttestation::Implicit(_) => - (CompactStatement::Seconded(candidate_hash), signing_context).encode(), - ValidityAttestation::Explicit(_) => - (CompactStatement::Valid(candidate_hash), signing_context).encode(), + ValidityAttestation::Implicit(_) => { + (CompactStatement::Seconded(candidate_hash), signing_context).encode() + }, + ValidityAttestation::Explicit(_) => { + (CompactStatement::Valid(candidate_hash), signing_context).encode() + }, } } } @@ -1561,7 +1568,7 @@ impl parity_scale_codec::Decode for CompactStatement { ) -> Result { let maybe_magic = <[u8; 4]>::decode(input)?; if maybe_magic != BACKING_STATEMENT_MAGIC { - return Err(parity_scale_codec::Error::from("invalid magic string")) + return Err(parity_scale_codec::Error::from("invalid magic string")); } Ok(match CompactStatementInner::decode(input)? { diff --git a/polkadot/runtime/parachains/src/configuration.rs b/polkadot/runtime/parachains/src/configuration.rs index be357d3ff3f1..96647b8b42c8 100644 --- a/polkadot/runtime/parachains/src/configuration.rs +++ b/polkadot/runtime/parachains/src/configuration.rs @@ -349,8 +349,8 @@ pub enum InconsistentError { MaxHrmpInboundChannelsExceeded, /// `minimum_backing_votes` is set to zero. ZeroMinimumBackingVotes, - /// `executor_params` are incoherent. - ExecutorParamErrors(Vec), + /// `executor_params` are inconsistent. + InconsistentExecutorParams { inner: ExecutorParamError }, } impl HostConfiguration @@ -366,77 +366,77 @@ where use InconsistentError::*; if self.group_rotation_frequency.is_zero() { - return Err(ZeroGroupRotationFrequency) + return Err(ZeroGroupRotationFrequency); } if self.paras_availability_period.is_zero() { - return Err(ZeroParasAvailabilityPeriod) + return Err(ZeroParasAvailabilityPeriod); } if self.no_show_slots.is_zero() { - return Err(ZeroNoShowSlots) + return Err(ZeroNoShowSlots); } if self.max_code_size > MAX_CODE_SIZE { - return Err(MaxCodeSizeExceedHardLimit { max_code_size: self.max_code_size }) + return Err(MaxCodeSizeExceedHardLimit { max_code_size: self.max_code_size }); } if self.max_head_data_size > MAX_HEAD_DATA_SIZE { return Err(MaxHeadDataSizeExceedHardLimit { max_head_data_size: self.max_head_data_size, - }) + }); } if self.max_pov_size > MAX_POV_SIZE { - return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size }) + return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size }); } if self.minimum_validation_upgrade_delay <= self.paras_availability_period { return Err(MinimumValidationUpgradeDelayLessThanChainAvailabilityPeriod { minimum_validation_upgrade_delay: self.minimum_validation_upgrade_delay.clone(), paras_availability_period: self.paras_availability_period.clone(), - }) + }); } if self.validation_upgrade_delay <= 1.into() { return Err(ValidationUpgradeDelayIsTooLow { validation_upgrade_delay: self.validation_upgrade_delay.clone(), - }) + }); } if self.max_upward_message_size > crate::inclusion::MAX_UPWARD_MESSAGE_SIZE_BOUND { return Err(MaxUpwardMessageSizeExceeded { max_message_size: self.max_upward_message_size, - }) + }); } if self.hrmp_max_message_num_per_candidate > MAX_HORIZONTAL_MESSAGE_NUM { return Err(MaxHorizontalMessageNumExceeded { max_message_num: self.hrmp_max_message_num_per_candidate, - }) + }); } if self.max_upward_message_num_per_candidate > MAX_UPWARD_MESSAGE_NUM { return Err(MaxUpwardMessageNumExceeded { max_message_num: self.max_upward_message_num_per_candidate, - }) + }); } if self.hrmp_max_parachain_outbound_channels > crate::hrmp::HRMP_MAX_OUTBOUND_CHANNELS_BOUND { - return Err(MaxHrmpOutboundChannelsExceeded) + return Err(MaxHrmpOutboundChannelsExceeded); } if self.hrmp_max_parachain_inbound_channels > crate::hrmp::HRMP_MAX_INBOUND_CHANNELS_BOUND { - return Err(MaxHrmpInboundChannelsExceeded) + return Err(MaxHrmpInboundChannelsExceeded); } if self.minimum_backing_votes.is_zero() { - return Err(ZeroMinimumBackingVotes) + return Err(ZeroMinimumBackingVotes); } - if let errors = self.executor_params.check_consistency() && errors.len() > 0 { - return Err(ExecutorParamErrors(errors)) + if let Err(inner) = self.executor_params.check_consistency() { + return Err(InconsistentExecutorParams { inner }); } Ok(()) @@ -1240,7 +1240,7 @@ impl Pallet { // No pending configuration changes, so we're done. if pending_configs.is_empty() { - return SessionChangeOutcome { prev_config, new_config: None } + return SessionChangeOutcome { prev_config, new_config: None }; } let (mut past_and_present, future) = pending_configs @@ -1354,7 +1354,7 @@ impl Pallet { "Configuration change rejected due to invalid configuration: {:?}", e, ); - return Err(Error::::InvalidNewValue.into()) + return Err(Error::::InvalidNewValue.into()); } else { // The configuration was already broken, so we can as well proceed with the update. // You cannot break something that is already broken. From 072af16eb8264d348f1144815bc7bda8a7177ddf Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 4 Oct 2023 12:33:33 +0800 Subject: [PATCH 3/8] check timeouts against defaults --- polkadot/primitives/src/v6/executor_params.rs | 74 ++++++++++++++----- 1 file changed, 55 insertions(+), 19 deletions(-) diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 4805b07b8019..e6ec6965aadf 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -46,8 +46,8 @@ pub enum ExecutorParam { #[codec(index = 2)] StackLogicalMax(u32), /// Executor machine stack size limit, in bytes. - /// If `StackLogicalMax` is also present, a valid value ranges from 128 * `logical_max` - /// to 512 * `logical_max`. + /// If `StackLogicalMax` is also present, a valid value should not fall below + /// 128 * `logical_max`. #[codec(index = 3)] StackNativeMax(u32), /// Max. amount of memory the preparation worker is allowed to use during @@ -55,14 +55,14 @@ pub enum ExecutorParam { /// Valid max. memory ranges from 256MB to 16GB. #[codec(index = 4)] PrecheckingMaxMemory(u64), - /// PVF preparation timeouts, millisec - /// If both `PvfPrepTimeoutKind::Precheck` and `PvfPrepTimeoutKind::Lenient` are present, - /// ensure that `precheck` < `lenient`. + /// PVF preparation timeouts, in millisecond. + /// Always ensure that `precheck_timeout` < `lenient_timeout`. + /// If not set, the default values will be used, 60,000 and 360,000 respectively. #[codec(index = 5)] PvfPrepTimeout(PvfPrepTimeoutKind, u64), - /// PVF execution timeouts, millisec - /// If both `PvfExecTimeoutKind::Backing` and `PvfExecTimeoutKind::Approval` are present, - /// ensure that `backing` < `approval`. + /// PVF execution timeouts, in millisecond. + /// Always ensure that `backing_timeout` < `approval_timeout`. + /// If not set, the default values will be used, 2,000 and 12,000 respectively. #[codec(index = 6)] PvfExecTimeout(PvfExecTimeoutKind, u64), /// Enables WASM bulk memory proposal @@ -172,6 +172,7 @@ impl ExecutorParams { seen.insert($param, $val as u64); }; + // should check existence before range ($param:ident, $val:expr, $out_of_limit:expr $(,)?) => { if seen.contains_key($param) { return Err(DuplicatedParam($param)); @@ -231,7 +232,8 @@ impl ExecutorParams { }, WasmExtBulkMemory => { - check!(param_ident, 0); + // 1 is a dummy for inserting the key into the map + check!(param_ident, 1); }, } @@ -243,26 +245,60 @@ impl ExecutorParams { } } - if let (Some(precheck), Some(lenient)) = - (seen.get("PvfPrepTimeoutKind::Precheck"), seen.get("PvfPrepTimeoutKind::Lenient")) - { - if *precheck >= *lenient { + match ( + seen.get("PvfPrepTimeoutKind::Precheck"), + seen.get("PvfPrepTimeoutKind::Lenient"), + ) { + (Some(precheck), Some(lenient)) if *precheck >= *lenient => { return Err(IncompatibleValues( "PvfPrepTimeoutKind::Precheck", "PvfPrepTimeoutKind::Lenient", )); - } + }, + + (Some(precheck), None) if *precheck >= 360000 => { + return Err(IncompatibleValues( + "PvfPrepTimeoutKind::Precheck", + "PvfPrepTimeoutKind::Lenient default", + )); + }, + + (None, Some(lenient)) if *lenient <= 60000 => { + return Err(IncompatibleValues( + "PvfPrepTimeoutKind::Precheck default", + "PvfPrepTimeoutKind::Lenient", + )); + }, + + (_, _) => {}, } - if let (Some(backing), Some(approval)) = - (seen.get("PvfExecTimeoutKind::Backing"), seen.get("PvfExecTimeoutKind::Approval")) - { - if *backing >= *approval { + match ( + seen.get("PvfExecTimeoutKind::Backing"), + seen.get("PvfExecTimeoutKind::Approval"), + ) { + (Some(backing), Some(approval)) if *backing >= *approval => { return Err(IncompatibleValues( "PvfExecTimeoutKind::Backing", "PvfExecTimeoutKind::Approval", )); - } + }, + + (Some(backing), None) if *backing >= 12000 => { + return Err(IncompatibleValues( + "PvfExecTimeoutKind::Backing", + "PvfExecTimeoutKind::Approval default", + )); + }, + + (None, Some(approval)) if *approval <= 2000 => { + return Err(IncompatibleValues( + "PvfExecTimeoutKind::Backing default", + "PvfExecTimeoutKind::Approval", + )); + }, + + (_, _) => {}, } } From b7b972bd8ecfb0e4416fc4852d7111828b831d05 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Thu, 5 Oct 2023 22:37:34 +0800 Subject: [PATCH 4/8] fmt --- .../node/core/pvf/common/src/executor_intf.rs | 5 +- polkadot/primitives/src/lib.rs | 10 +-- polkadot/primitives/src/v6/executor_params.rs | 24 +++---- polkadot/primitives/src/v6/mod.rs | 71 +++++++++---------- .../runtime/parachains/src/configuration.rs | 34 ++++----- 5 files changed, 69 insertions(+), 75 deletions(-) diff --git a/polkadot/node/core/pvf/common/src/executor_intf.rs b/polkadot/node/core/pvf/common/src/executor_intf.rs index e27ff3adf4f8..6b854f31f10d 100644 --- a/polkadot/node/core/pvf/common/src/executor_intf.rs +++ b/polkadot/node/core/pvf/common/src/executor_intf.rs @@ -106,8 +106,9 @@ pub fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result - sem.heap_alloc_strategy = - HeapAllocStrategy::Dynamic { maximum_pages: Some((*max_pages).saturating_add(DEFAULT_HEAP_PAGES_ESTIMATE)) }, + sem.heap_alloc_strategy = 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, diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index 116d5bce2e07..a2cf4aafc43e 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -43,11 +43,11 @@ pub use v6::{ CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, EncodeAs, - ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, ExplicitDisputeStatement, GroupIndex, - GroupRotationInfo, Hash, HashT, HeadData, Header, HorizontalMessages, HrmpChannelId, Id, - InboundDownwardMessage, InboundHrmpMessage, IndexedVec, InherentData, - InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, OccupiedCore, - OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, + ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, + ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, + HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, + InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, + OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry, PersistedValidationData, PvfCheckStatement, PvfExecTimeoutKind, PvfPrepTimeoutKind, RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels, RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex, diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index e6ec6965aadf..8fc0225e2bd0 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -138,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)) } } } @@ -150,7 +150,7 @@ 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)) } } } @@ -167,7 +167,7 @@ impl ExecutorParams { macro_rules! check { ($param:ident, $val:expr $(,)?) => { if seen.contains_key($param) { - return Err(DuplicatedParam($param)); + return Err(DuplicatedParam($param)) } seen.insert($param, $val as u64); }; @@ -175,10 +175,10 @@ impl ExecutorParams { // should check existence before range ($param:ident, $val:expr, $out_of_limit:expr $(,)?) => { if seen.contains_key($param) { - return Err(DuplicatedParam($param)); + return Err(DuplicatedParam($param)) } if $out_of_limit { - return Err(LimitExceeded($param)); + return Err(LimitExceeded($param)) } seen.insert($param, $val as u64); }; @@ -241,7 +241,7 @@ impl ExecutorParams { if let (Some(lm), Some(nm)) = (seen.get("StackLogicalMax"), seen.get("StackNativeMax")) { if *nm < 128 * *lm { - return Err(IncompatibleValues("StackLogicalMax", "StackNativeMax")); + return Err(IncompatibleValues("StackLogicalMax", "StackNativeMax")) } } @@ -253,21 +253,21 @@ impl ExecutorParams { return Err(IncompatibleValues( "PvfPrepTimeoutKind::Precheck", "PvfPrepTimeoutKind::Lenient", - )); + )) }, (Some(precheck), None) if *precheck >= 360000 => { return Err(IncompatibleValues( "PvfPrepTimeoutKind::Precheck", "PvfPrepTimeoutKind::Lenient default", - )); + )) }, (None, Some(lenient)) if *lenient <= 60000 => { return Err(IncompatibleValues( "PvfPrepTimeoutKind::Precheck default", "PvfPrepTimeoutKind::Lenient", - )); + )) }, (_, _) => {}, @@ -281,21 +281,21 @@ impl ExecutorParams { return Err(IncompatibleValues( "PvfExecTimeoutKind::Backing", "PvfExecTimeoutKind::Approval", - )); + )) }, (Some(backing), None) if *backing >= 12000 => { return Err(IncompatibleValues( "PvfExecTimeoutKind::Backing", "PvfExecTimeoutKind::Approval default", - )); + )) }, (None, Some(approval)) if *approval <= 2000 => { return Err(IncompatibleValues( "PvfExecTimeoutKind::Backing default", "PvfExecTimeoutKind::Approval", - )); + )) }, (_, _) => {}, diff --git a/polkadot/primitives/src/v6/mod.rs b/polkadot/primitives/src/v6/mod.rs index 5e651fa899fb..9371b3db406b 100644 --- a/polkadot/primitives/src/v6/mod.rs +++ b/polkadot/primitives/src/v6/mod.rs @@ -750,11 +750,11 @@ pub fn check_candidate_backing + Clone + Encode>( validator_lookup: impl Fn(usize) -> Option, ) -> Result { if backed.validator_indices.len() != group_len { - return Err(()); + return Err(()) } if backed.validity_votes.len() > group_len { - return Err(()); + return Err(()) } // this is known, even in runtime, to be blake2-256. @@ -775,12 +775,12 @@ pub fn check_candidate_backing + Clone + Encode>( if sig.verify(&payload[..], &validator_id) { signed += 1; } else { - return Err(()); + return Err(()) } } if signed != backed.validity_votes.len() { - return Err(()); + return Err(()) } Ok(signed) @@ -854,10 +854,10 @@ impl GroupRotationInfo { /// `core_index` should be less than `cores`, which is capped at `u32::max()`. pub fn group_for_core(&self, core_index: CoreIndex, cores: usize) -> GroupIndex { if self.group_rotation_frequency == 0 { - return GroupIndex(core_index.0); + return GroupIndex(core_index.0) } if cores == 0 { - return GroupIndex(0); + return GroupIndex(0) } let cores = sp_std::cmp::min(cores, u32::MAX as usize); @@ -876,10 +876,10 @@ impl GroupRotationInfo { /// `core_index` should be less than `cores`, which is capped at `u32::max()`. pub fn core_for_group(&self, group_index: GroupIndex, cores: usize) -> CoreIndex { if self.group_rotation_frequency == 0 { - return CoreIndex(group_index.0); + return CoreIndex(group_index.0) } if cores == 0 { - return CoreIndex(0); + return CoreIndex(0) } let cores = sp_std::cmp::min(cores, u32::MAX as usize); @@ -911,15 +911,15 @@ impl GroupRotationInfo { /// is 10 and the rotation frequency is 5, this should return 15. pub fn next_rotation_at(&self) -> N { let cycle_once = self.now + self.group_rotation_frequency; - cycle_once - - (cycle_once.saturating_sub(self.session_start_block) % self.group_rotation_frequency) + cycle_once - + (cycle_once.saturating_sub(self.session_start_block) % self.group_rotation_frequency) } /// Returns the block number of the last rotation before or including the current block. If the /// current block is 10 and the rotation frequency is 5, this should return 10. pub fn last_rotation_at(&self) -> N { - self.now - - (self.now.saturating_sub(self.session_start_block) % self.group_rotation_frequency) + self.now - + (self.now.saturating_sub(self.session_start_block) % self.group_rotation_frequency) } } @@ -1218,9 +1218,8 @@ impl ConsensusLog { digest_item: &runtime_primitives::DigestItem, ) -> Result, parity_scale_codec::Error> { match digest_item { - runtime_primitives::DigestItem::Consensus(id, encoded) if id == &POLKADOT_ENGINE_ID => { - Ok(Some(Self::decode(&mut &encoded[..])?)) - }, + runtime_primitives::DigestItem::Consensus(id, encoded) if id == &POLKADOT_ENGINE_ID => + Ok(Some(Self::decode(&mut &encoded[..])?)), _ => Ok(None), } } @@ -1249,27 +1248,23 @@ impl DisputeStatement { /// Get the payload data for this type of dispute statement. pub fn payload_data(&self, candidate_hash: CandidateHash, session: SessionIndex) -> Vec { match *self { - DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) => { - ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload() - }, + DisputeStatement::Valid(ValidDisputeStatementKind::Explicit) => + ExplicitDisputeStatement { valid: true, candidate_hash, session }.signing_payload(), DisputeStatement::Valid(ValidDisputeStatementKind::BackingSeconded( inclusion_parent, )) => CompactStatement::Seconded(candidate_hash).signing_payload(&SigningContext { session_index: session, parent_hash: inclusion_parent, }), - DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) => { + DisputeStatement::Valid(ValidDisputeStatementKind::BackingValid(inclusion_parent)) => CompactStatement::Valid(candidate_hash).signing_payload(&SigningContext { session_index: session, parent_hash: inclusion_parent, - }) - }, - DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) => { - ApprovalVote(candidate_hash).signing_payload(session) - }, - DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) => { - ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload() - }, + }), + DisputeStatement::Valid(ValidDisputeStatementKind::ApprovalChecking) => + ApprovalVote(candidate_hash).signing_payload(session), + DisputeStatement::Invalid(InvalidDisputeStatementKind::Explicit) => + ExplicitDisputeStatement { valid: false, candidate_hash, session }.signing_payload(), } } @@ -1309,11 +1304,11 @@ impl DisputeStatement { /// Statement is backing statement. pub fn is_backing(&self) -> bool { match *self { - Self::Valid(ValidDisputeStatementKind::BackingSeconded(_)) - | Self::Valid(ValidDisputeStatementKind::BackingValid(_)) => true, - Self::Valid(ValidDisputeStatementKind::Explicit) - | Self::Valid(ValidDisputeStatementKind::ApprovalChecking) - | Self::Invalid(_) => false, + Self::Valid(ValidDisputeStatementKind::BackingSeconded(_)) | + Self::Valid(ValidDisputeStatementKind::BackingValid(_)) => true, + Self::Valid(ValidDisputeStatementKind::Explicit) | + Self::Valid(ValidDisputeStatementKind::ApprovalChecking) | + Self::Invalid(_) => false, } } } @@ -1485,12 +1480,10 @@ impl ValidityAttestation { signing_context: &SigningContext, ) -> Vec { match *self { - ValidityAttestation::Implicit(_) => { - (CompactStatement::Seconded(candidate_hash), signing_context).encode() - }, - ValidityAttestation::Explicit(_) => { - (CompactStatement::Valid(candidate_hash), signing_context).encode() - }, + ValidityAttestation::Implicit(_) => + (CompactStatement::Seconded(candidate_hash), signing_context).encode(), + ValidityAttestation::Explicit(_) => + (CompactStatement::Valid(candidate_hash), signing_context).encode(), } } } @@ -1568,7 +1561,7 @@ impl parity_scale_codec::Decode for CompactStatement { ) -> Result { let maybe_magic = <[u8; 4]>::decode(input)?; if maybe_magic != BACKING_STATEMENT_MAGIC { - return Err(parity_scale_codec::Error::from("invalid magic string")); + return Err(parity_scale_codec::Error::from("invalid magic string")) } Ok(match CompactStatementInner::decode(input)? { diff --git a/polkadot/runtime/parachains/src/configuration.rs b/polkadot/runtime/parachains/src/configuration.rs index 96647b8b42c8..a65fda370488 100644 --- a/polkadot/runtime/parachains/src/configuration.rs +++ b/polkadot/runtime/parachains/src/configuration.rs @@ -366,77 +366,77 @@ where use InconsistentError::*; if self.group_rotation_frequency.is_zero() { - return Err(ZeroGroupRotationFrequency); + return Err(ZeroGroupRotationFrequency) } if self.paras_availability_period.is_zero() { - return Err(ZeroParasAvailabilityPeriod); + return Err(ZeroParasAvailabilityPeriod) } if self.no_show_slots.is_zero() { - return Err(ZeroNoShowSlots); + return Err(ZeroNoShowSlots) } if self.max_code_size > MAX_CODE_SIZE { - return Err(MaxCodeSizeExceedHardLimit { max_code_size: self.max_code_size }); + return Err(MaxCodeSizeExceedHardLimit { max_code_size: self.max_code_size }) } if self.max_head_data_size > MAX_HEAD_DATA_SIZE { return Err(MaxHeadDataSizeExceedHardLimit { max_head_data_size: self.max_head_data_size, - }); + }) } if self.max_pov_size > MAX_POV_SIZE { - return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size }); + return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size }) } if self.minimum_validation_upgrade_delay <= self.paras_availability_period { return Err(MinimumValidationUpgradeDelayLessThanChainAvailabilityPeriod { minimum_validation_upgrade_delay: self.minimum_validation_upgrade_delay.clone(), paras_availability_period: self.paras_availability_period.clone(), - }); + }) } if self.validation_upgrade_delay <= 1.into() { return Err(ValidationUpgradeDelayIsTooLow { validation_upgrade_delay: self.validation_upgrade_delay.clone(), - }); + }) } if self.max_upward_message_size > crate::inclusion::MAX_UPWARD_MESSAGE_SIZE_BOUND { return Err(MaxUpwardMessageSizeExceeded { max_message_size: self.max_upward_message_size, - }); + }) } if self.hrmp_max_message_num_per_candidate > MAX_HORIZONTAL_MESSAGE_NUM { return Err(MaxHorizontalMessageNumExceeded { max_message_num: self.hrmp_max_message_num_per_candidate, - }); + }) } if self.max_upward_message_num_per_candidate > MAX_UPWARD_MESSAGE_NUM { return Err(MaxUpwardMessageNumExceeded { max_message_num: self.max_upward_message_num_per_candidate, - }); + }) } if self.hrmp_max_parachain_outbound_channels > crate::hrmp::HRMP_MAX_OUTBOUND_CHANNELS_BOUND { - return Err(MaxHrmpOutboundChannelsExceeded); + return Err(MaxHrmpOutboundChannelsExceeded) } if self.hrmp_max_parachain_inbound_channels > crate::hrmp::HRMP_MAX_INBOUND_CHANNELS_BOUND { - return Err(MaxHrmpInboundChannelsExceeded); + return Err(MaxHrmpInboundChannelsExceeded) } if self.minimum_backing_votes.is_zero() { - return Err(ZeroMinimumBackingVotes); + return Err(ZeroMinimumBackingVotes) } if let Err(inner) = self.executor_params.check_consistency() { - return Err(InconsistentExecutorParams { inner }); + return Err(InconsistentExecutorParams { inner }) } Ok(()) @@ -1240,7 +1240,7 @@ impl Pallet { // No pending configuration changes, so we're done. if pending_configs.is_empty() { - return SessionChangeOutcome { prev_config, new_config: None }; + return SessionChangeOutcome { prev_config, new_config: None } } let (mut past_and_present, future) = pending_configs @@ -1354,7 +1354,7 @@ impl Pallet { "Configuration change rejected due to invalid configuration: {:?}", e, ); - return Err(Error::::InvalidNewValue.into()); + return Err(Error::::InvalidNewValue.into()) } else { // The configuration was already broken, so we can as well proceed with the update. // You cannot break something that is already broken. From 7412c3a7cad2bf7dae49a79337554aa5aacaf390 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Thu, 5 Oct 2023 23:13:29 +0800 Subject: [PATCH 5/8] upstream constants --- .../node/core/candidate-validation/src/lib.rs | 11 +- .../node/core/pvf/common/src/executor_intf.rs | 11 +- polkadot/primitives/src/lib.rs | 18 +-- polkadot/primitives/src/v6/executor_params.rs | 116 +++++++++--------- 4 files changed, 77 insertions(+), 79 deletions(-) diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 3281fcc59a15..21a7121d47bd 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/polkadot/node/core/candidate-validation/src/lib.rs @@ -44,6 +44,10 @@ use polkadot_parachain_primitives::primitives::{ ValidationParams, ValidationResult as WasmValidationResult, }; use polkadot_primitives::{ + executor_params::{ + DEFAULT_APPROVAL_EXECUTION_TIMEOUT, DEFAULT_BACKING_EXECUTION_TIMEOUT, + DEFAULT_LENIENT_PREPARATION_TIMEOUT, DEFAULT_PRECHECK_PREPARATION_TIMEOUT, + }, CandidateCommitments, CandidateDescriptor, CandidateReceipt, ExecutorParams, Hash, OccupiedCoreAssumption, PersistedValidationData, PvfExecTimeoutKind, PvfPrepTimeoutKind, ValidationCode, ValidationCodeHash, @@ -83,13 +87,6 @@ const PVF_APPROVAL_EXECUTION_RETRY_DELAY: Duration = Duration::from_secs(3); #[cfg(test)] const PVF_APPROVAL_EXECUTION_RETRY_DELAY: Duration = Duration::from_millis(200); -// Default PVF timeouts. Must never be changed! Use executor environment parameters in -// `session_info` pallet to adjust them. See also `PvfTimeoutKind` docs. -const DEFAULT_PRECHECK_PREPARATION_TIMEOUT: Duration = Duration::from_secs(60); -const DEFAULT_LENIENT_PREPARATION_TIMEOUT: Duration = Duration::from_secs(360); -const DEFAULT_BACKING_EXECUTION_TIMEOUT: Duration = Duration::from_secs(2); -const DEFAULT_APPROVAL_EXECUTION_TIMEOUT: Duration = Duration::from_secs(12); - /// Configuration for the candidate validation subsystem #[derive(Clone)] pub struct Config { diff --git a/polkadot/node/core/pvf/common/src/executor_intf.rs b/polkadot/node/core/pvf/common/src/executor_intf.rs index 6b854f31f10d..bbcf8e9edd49 100644 --- a/polkadot/node/core/pvf/common/src/executor_intf.rs +++ b/polkadot/node/core/pvf/common/src/executor_intf.rs @@ -16,7 +16,10 @@ //! Interface to the Substrate Executor -use polkadot_primitives::{ExecutorParam, ExecutorParams}; +use polkadot_primitives::{ + executor_params::{DEFAULT_LOGICAL_STACK_MAX, DEFAULT_NATIVE_STACK_MAX}, + ExecutorParam, ExecutorParams, +}; use sc_executor_common::{ error::WasmError, runtime_blob::RuntimeBlob, @@ -43,7 +46,7 @@ const DEFAULT_HEAP_PAGES_ESTIMATE: u32 = 32; const EXTRA_HEAP_PAGES: u32 = 2048; /// The number of bytes devoted for the stack during wasm execution of a PVF. -pub const NATIVE_STACK_MAX: u32 = 256 * 1024 * 1024; +pub const NATIVE_STACK_MAX: u32 = DEFAULT_NATIVE_STACK_MAX; // VALUES OF THE DEFAULT CONFIGURATION SHOULD NEVER BE CHANGED // They are used as base values for the execution environment parametrization. @@ -73,8 +76,8 @@ pub const DEFAULT_CONFIG: Config = Config { // also increase the native 256x. This hopefully should preclude wasm code from reaching // the stack limit set by the wasmtime. deterministic_stack_limit: Some(DeterministicStackLimit { - logical_max: 65536, - native_stack_max: NATIVE_STACK_MAX, + logical_max: DEFAULT_LOGICAL_STACK_MAX, + native_stack_max: DEFAULT_NATIVE_STACK_MAX, }), canonicalize_nans: true, // Rationale for turning the multi-threaded compilation off is to make the preparation time diff --git a/polkadot/primitives/src/lib.rs b/polkadot/primitives/src/lib.rs index a2cf4aafc43e..4ba8b8b031fc 100644 --- a/polkadot/primitives/src/lib.rs +++ b/polkadot/primitives/src/lib.rs @@ -35,15 +35,15 @@ pub mod runtime_api; // Primitives requiring versioning must not be exported and must be referred by an exact version. pub use v6::{ async_backing, byzantine_threshold, check_candidate_backing, collator_signature_payload, - effective_minimum_backing_votes, metric_definitions, slashing, supermajority_threshold, - well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, AccountId, AccountIndex, - AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, AuthorityDiscoveryId, - AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, BlockId, BlockNumber, - CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, CandidateIndex, - CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CollatorId, - CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex, - CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, EncodeAs, - ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, + effective_minimum_backing_votes, executor_params, metric_definitions, slashing, + supermajority_threshold, well_known_keys, AbridgedHostConfiguration, AbridgedHrmpChannel, + AccountId, AccountIndex, AccountPublic, ApprovalVote, AssignmentId, AsyncBackingParams, + AuthorityDiscoveryId, AvailabilityBitfield, BackedCandidate, Balance, BlakeTwo256, Block, + BlockId, BlockNumber, CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, + CandidateIndex, CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, + CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, + CoreIndex, CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, + EncodeAs, ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header, HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec, InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce, diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 8fc0225e2bd0..362005eccf2b 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -28,12 +28,38 @@ use scale_info::TypeInfo; use serde::{Deserialize, Serialize}; use sp_std::{collections::btree_map::BTreeMap, ops::Deref, time::Duration, vec, vec::Vec}; +/// Default maximum number of wasm values allowed for the stack during execution of a PVF. +pub const DEFAULT_LOGICAL_STACK_MAX: u32 = 65536; +/// Default maximum number of bytes devoted for the stack during execution of a PVF. +pub const DEFAULT_NATIVE_STACK_MAX: u32 = 256 * 1024 * 1024; + 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; +// Default PVF timeouts. Must never be changed! Use executor environment parameters to adjust them. +// See also `PvfTimeoutKind` docs. + +/// Default PVF preparation timeout for prechecking requests. +pub const DEFAULT_PRECHECK_PREPARATION_TIMEOUT: Duration = Duration::from_secs(60); +/// Default PVF preparation timeout for execution requests. +pub const DEFAULT_LENIENT_PREPARATION_TIMEOUT: Duration = Duration::from_secs(360); +/// Default PVF execution timeout for backing. +pub const DEFAULT_BACKING_EXECUTION_TIMEOUT: Duration = Duration::from_secs(2); +/// Default PVF execution timeout for approval or disputes. +pub const DEFAULT_APPROVAL_EXECUTION_TIMEOUT: Duration = Duration::from_secs(12); + +const DEFAULT_PRECHECK_PREPARATION_TIMEOUT_MS: u64 = + DEFAULT_PRECHECK_PREPARATION_TIMEOUT.as_millis() as u64; +const DEFAULT_LENIENT_PREPARATION_TIMEOUT_MS: u64 = + DEFAULT_LENIENT_PREPARATION_TIMEOUT.as_millis() as u64; +const DEFAULT_BACKING_EXECUTION_TIMEOUT_MS: u64 = + DEFAULT_BACKING_EXECUTION_TIMEOUT.as_millis() as u64; +const DEFAULT_APPROVAL_EXECUTION_TIMEOUT_MS: u64 = + DEFAULT_APPROVAL_EXECUTION_TIMEOUT.as_millis() as u64; + /// The different executor parameters for changing the execution environment semantics. #[derive(Clone, Debug, Encode, Decode, PartialEq, Eq, TypeInfo, Serialize, Deserialize)] pub enum ExecutorParam { @@ -232,73 +258,45 @@ impl ExecutorParams { }, WasmExtBulkMemory => { - // 1 is a dummy for inserting the key into the map check!(param_ident, 1); }, } + } - // FIXME is it valid if only one is present? - if let (Some(lm), Some(nm)) = (seen.get("StackLogicalMax"), seen.get("StackNativeMax")) - { - if *nm < 128 * *lm { - return Err(IncompatibleValues("StackLogicalMax", "StackNativeMax")) - } + if let (Some(lm), Some(nm)) = ( + seen.get("StackLogicalMax").or(Some(&(DEFAULT_LOGICAL_STACK_MAX as u64))), + seen.get("StackNativeMax").or(Some(&(DEFAULT_NATIVE_STACK_MAX as u64))), + ) { + if *nm < 128 * *lm { + return Err(IncompatibleValues("StackLogicalMax", "StackNativeMax")) } + } - match ( - seen.get("PvfPrepTimeoutKind::Precheck"), - seen.get("PvfPrepTimeoutKind::Lenient"), - ) { - (Some(precheck), Some(lenient)) if *precheck >= *lenient => { - return Err(IncompatibleValues( - "PvfPrepTimeoutKind::Precheck", - "PvfPrepTimeoutKind::Lenient", - )) - }, - - (Some(precheck), None) if *precheck >= 360000 => { - return Err(IncompatibleValues( - "PvfPrepTimeoutKind::Precheck", - "PvfPrepTimeoutKind::Lenient default", - )) - }, - - (None, Some(lenient)) if *lenient <= 60000 => { - return Err(IncompatibleValues( - "PvfPrepTimeoutKind::Precheck default", - "PvfPrepTimeoutKind::Lenient", - )) - }, - - (_, _) => {}, + if let (Some(precheck), Some(lenient)) = ( + seen.get("PvfPrepTimeoutKind::Precheck") + .or(Some(&DEFAULT_PRECHECK_PREPARATION_TIMEOUT_MS)), + seen.get("PvfPrepTimeoutKind::Lenient") + .or(Some(&DEFAULT_LENIENT_PREPARATION_TIMEOUT_MS)), + ) { + if *precheck >= *lenient { + return Err(IncompatibleValues( + "PvfPrepTimeoutKind::Precheck", + "PvfPrepTimeoutKind::Lenient", + )) } + } - match ( - seen.get("PvfExecTimeoutKind::Backing"), - seen.get("PvfExecTimeoutKind::Approval"), - ) { - (Some(backing), Some(approval)) if *backing >= *approval => { - return Err(IncompatibleValues( - "PvfExecTimeoutKind::Backing", - "PvfExecTimeoutKind::Approval", - )) - }, - - (Some(backing), None) if *backing >= 12000 => { - return Err(IncompatibleValues( - "PvfExecTimeoutKind::Backing", - "PvfExecTimeoutKind::Approval default", - )) - }, - - (None, Some(approval)) if *approval <= 2000 => { - return Err(IncompatibleValues( - "PvfExecTimeoutKind::Backing default", - "PvfExecTimeoutKind::Approval", - )) - }, - - (_, _) => {}, + if let (Some(backing), Some(approval)) = ( + seen.get("PvfExecTimeoutKind::Backing") + .or(Some(&DEFAULT_BACKING_EXECUTION_TIMEOUT_MS)), + seen.get("PvfExecTimeoutKind::Approval") + .or(Some(&DEFAULT_APPROVAL_EXECUTION_TIMEOUT_MS)), + ) { + if *backing >= *approval { + return Err(IncompatibleValues( + "PvfExecTimeoutKind::Backing", + "PvfExecTimeoutKind::Approval", + )) } } From 5e2b0d30dd770c3b001524af247b2ddec3122881 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 11 Oct 2023 01:45:52 +0800 Subject: [PATCH 6/8] as per suggestions --- polkadot/primitives/src/v6/executor_params.rs | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 362005eccf2b..05ceccc7e645 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -33,14 +33,19 @@ pub const DEFAULT_LOGICAL_STACK_MAX: u32 = 65536; /// Default maximum number of bytes devoted for the stack during execution of a PVF. pub const DEFAULT_NATIVE_STACK_MAX: u32 = 256 * 1024 * 1024; -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 limit of [`ExecutorParam::MaxMemoryPages`]. +pub const MEMORY_PAGES_MAX: u32 = 65536; +/// The lower bound of [`ExecutorParam::StackLogicalMax`]. +pub const LOGICAL_MAX_LO: u32 = 1024; +/// The upper bound of [`ExecutorParam::StackLogicalMax`]. +pub const LOGICAL_MAX_HI: u32 = 2 * 65536; +/// The lower bound of [`ExecutorParam::PrecheckingMaxMemory`]. +pub const PRECHECK_MEM_MAX_LO: u64 = 256 * 1024 * 1024; +/// The upper bound of [`ExecutorParam::PrecheckingMaxMemory`]. +pub const PRECHECK_MEM_MAX_HI: u64 = 16 * 1024 * 1024 * 1024; // Default PVF timeouts. Must never be changed! Use executor environment parameters to adjust them. -// See also `PvfTimeoutKind` docs. +// See also `PvfPrepTimeoutKind` and `PvfExecTimeoutKind` docs. /// Default PVF preparation timeout for prechecking requests. pub const DEFAULT_PRECHECK_PREPARATION_TIMEOUT: Duration = Duration::from_secs(60); @@ -68,27 +73,36 @@ pub enum ExecutorParam { #[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]. + /// A valid value lies within [[`LOGICAL_MAX_LO`], [`LOGICAL_MAX_HI`]]. + /// + /// For WebAssembly, the stack limit is subject to implementations, meaning that it may vary on + /// different platforms. However, we want execution to be deterministic across machines of + /// different architectures, including failures like stack overflow. For deterministic + /// overflow, we rely on a **logical** limit, the maximum number of values allowed to be pushed + /// on the stack. #[codec(index = 2)] StackLogicalMax(u32), /// Executor machine stack size limit, in bytes. /// If `StackLogicalMax` is also present, a valid value should not fall below - /// 128 * `logical_max`. + /// 128 * `StackLogicalMax`. + /// + /// For deterministic overflow, `StackLogicalMax` should be reached before the native stack is + /// exhausted. #[codec(index = 3)] StackNativeMax(u32), /// Max. amount of memory the preparation worker is allowed to use during /// pre-checking, in bytes. - /// Valid max. memory ranges from 256MB to 16GB. + /// Valid max. memory ranges from [`PRECHECK_MEM_MAX_LO`] to [`PRECHECK_MEM_MAX_HI`]. #[codec(index = 4)] PrecheckingMaxMemory(u64), /// PVF preparation timeouts, in millisecond. /// Always ensure that `precheck_timeout` < `lenient_timeout`. - /// If not set, the default values will be used, 60,000 and 360,000 respectively. + /// When absent, the default values will be used. #[codec(index = 5)] PvfPrepTimeout(PvfPrepTimeoutKind, u64), /// PVF execution timeouts, in millisecond. /// Always ensure that `backing_timeout` < `approval_timeout`. - /// If not set, the default values will be used, 2,000 and 12,000 respectively. + /// When absent, the default values will be used. #[codec(index = 6)] PvfExecTimeout(PvfExecTimeoutKind, u64), /// Enables WASM bulk memory proposal @@ -102,7 +116,7 @@ pub enum ExecutorParamError { /// A param is duplicated. DuplicatedParam(&'static str), /// A param value exceeds its limitation. - LimitExceeded(&'static str), + OutsideLimit(&'static str), /// Two param values are incompatible or senseless when put together. IncompatibleValues(&'static str, &'static str), } @@ -204,7 +218,7 @@ impl ExecutorParams { return Err(DuplicatedParam($param)) } if $out_of_limit { - return Err(LimitExceeded($param)) + return Err(OutsideLimit($param)) } seen.insert($param, $val as u64); }; From 2287dfdda0c6c96bc076fed48079d00bb54a704b Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 11 Oct 2023 02:00:24 +0800 Subject: [PATCH 7/8] sorry clippy, should've known better --- polkadot/primitives/src/v6/executor_params.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/primitives/src/v6/executor_params.rs b/polkadot/primitives/src/v6/executor_params.rs index 05ceccc7e645..8ea0889bd23b 100644 --- a/polkadot/primitives/src/v6/executor_params.rs +++ b/polkadot/primitives/src/v6/executor_params.rs @@ -244,7 +244,7 @@ impl ExecutorParams { match *param { MaxMemoryPages(val) => { - check!(param_ident, val, val <= 0 || val > MEMORY_PAGES_MAX,); + check!(param_ident, val, val == 0 || val > MEMORY_PAGES_MAX,); }, StackLogicalMax(val) => { From c1c0f4cafef12f31a74f85ab74d8eb29bc4a122a Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Thu, 12 Oct 2023 04:06:33 +0800 Subject: [PATCH 8/8] remove duplicated constant --- polkadot/node/core/pvf/common/src/executor_intf.rs | 3 --- polkadot/node/core/pvf/execute-worker/src/lib.rs | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/polkadot/node/core/pvf/common/src/executor_intf.rs b/polkadot/node/core/pvf/common/src/executor_intf.rs index bbcf8e9edd49..508a12998fc9 100644 --- a/polkadot/node/core/pvf/common/src/executor_intf.rs +++ b/polkadot/node/core/pvf/common/src/executor_intf.rs @@ -45,9 +45,6 @@ use std::any::{Any, TypeId}; const DEFAULT_HEAP_PAGES_ESTIMATE: u32 = 32; const EXTRA_HEAP_PAGES: u32 = 2048; -/// The number of bytes devoted for the stack during wasm execution of a PVF. -pub const NATIVE_STACK_MAX: u32 = DEFAULT_NATIVE_STACK_MAX; - // VALUES OF THE DEFAULT CONFIGURATION SHOULD NEVER BE CHANGED // They are used as base values for the execution environment parametrization. // To overwrite them, add new ones to `EXECUTOR_PARAMS` in the `session_info` pallet and perform diff --git a/polkadot/node/core/pvf/execute-worker/src/lib.rs b/polkadot/node/core/pvf/execute-worker/src/lib.rs index 9d7bfdf28669..5ab73669e68c 100644 --- a/polkadot/node/core/pvf/execute-worker/src/lib.rs +++ b/polkadot/node/core/pvf/execute-worker/src/lib.rs @@ -27,7 +27,6 @@ use parity_scale_codec::{Decode, Encode}; use polkadot_node_core_pvf_common::{ error::InternalValidationError, execute::{Handshake, Response}, - executor_intf::NATIVE_STACK_MAX, framed_recv_blocking, framed_send_blocking, worker::{ cpu_time_monitor_loop, stringify_panic_payload, @@ -36,6 +35,7 @@ use polkadot_node_core_pvf_common::{ }, }; use polkadot_parachain_primitives::primitives::ValidationResult; +use polkadot_primitives::executor_params::DEFAULT_NATIVE_STACK_MAX; use std::{ os::unix::net::UnixStream, path::PathBuf, @@ -69,7 +69,7 @@ use tokio::io; // // Typically on Linux the main thread gets the stack size specified by the `ulimit` and // typically it's configured to 8 MiB. Rust's spawned threads are 2 MiB. OTOH, the -// NATIVE_STACK_MAX is set to 256 MiB. Not nearly enough. +// DEFAULT_NATIVE_STACK_MAX is set to 256 MiB. Not nearly enough. // // Hence we need to increase it. The simplest way to fix that is to spawn a thread with the desired // stack limit. @@ -78,7 +78,7 @@ use tokio::io; // // The default Rust thread stack limit 2 MiB + 256 MiB wasm stack. /// The stack size for the execute thread. -pub const EXECUTE_THREAD_STACK_SIZE: usize = 2 * 1024 * 1024 + NATIVE_STACK_MAX as usize; +pub const EXECUTE_THREAD_STACK_SIZE: usize = 2 * 1024 * 1024 + DEFAULT_NATIVE_STACK_MAX as usize; fn recv_handshake(stream: &mut UnixStream) -> io::Result { let handshake_enc = framed_recv_blocking(stream)?;