Check executor params coherence (#1774)

Co-authored-by: Marcin S <marcin@realemail.net>

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 {

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,
@@ -42,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 = 256 * 1024 * 1024;
-
 // 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
@@ -73,8 +73,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
@@ -106,8 +106,9 @@ pub fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result<Semantics, S
 	for p in par.iter() {
 		match p {
 			ExecutorParam::MaxMemoryPages(max_pages) =>
-				sem.heap_alloc_strategy =
-					HeapAllocStrategy::Dynamic { maximum_pages: Some(*max_pages) },
+				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,

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<Handshake> {
 	let handshake_enc = framed_recv_blocking(stream)?;

polkadot/primitives/src/lib.rs

@@ -35,19 +35,19 @@ 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, 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,
+	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,
+	OccupiedCore, OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry,
 	PersistedValidationData, PvfCheckStatement, PvfExecTimeoutKind, PvfPrepTimeoutKind,
 	RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels,
 	RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex,

polkadot/primitives/src/v6/executor_params.rs

@@ -26,28 +26,83 @@ 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};
+
+/// 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;
+
+/// 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 `PvfPrepTimeoutKind` and `PvfExecTimeoutKind` 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 {
 	/// Maximum number of memory pages (64KiB bytes per page) the executor can allocate.
+	/// A valid value lies within (0, 65536].
 	#[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 [[`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
+	/// Executor machine stack size limit, in bytes.
+	/// If `StackLogicalMax` is also present, a valid value should not fall below
+	/// 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
+	/// pre-checking, in bytes.
+	/// Valid max. memory ranges from [`PRECHECK_MEM_MAX_LO`] to [`PRECHECK_MEM_MAX_HI`].
 	#[codec(index = 4)]
 	PrecheckingMaxMemory(u64),
-	/// PVF preparation timeouts, millisec
+	/// PVF preparation timeouts, in millisecond.
+	/// Always ensure that `precheck_timeout` < `lenient_timeout`.
+	/// When absent, the default values will be used.
 	#[codec(index = 5)]
 	PvfPrepTimeout(PvfPrepTimeoutKind, u64),
-	/// PVF execution timeouts, millisec
+	/// PVF execution timeouts, in millisecond.
+	/// Always ensure that `backing_timeout` < `approval_timeout`.
+	/// When absent, the default values will be used.
 	#[codec(index = 6)]
 	PvfExecTimeout(PvfExecTimeoutKind, u64),
 	/// Enables WASM bulk memory proposal
@@ -55,6 +110,17 @@ 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.
+	OutsideLimit(&'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.
 ///
 /// This type is produced by [`ExecutorParams::hash`].
@@ -130,6 +196,126 @@ impl ExecutorParams {
 		}
 		None
 	}
+
+	/// Check params coherence.
+	pub fn check_consistency(&self) -> Result<(), ExecutorParamError> {
+		use ExecutorParam::*;
+		use ExecutorParamError::*;
+
+		let mut seen = BTreeMap::<&str, u64>::new();
+
+		macro_rules! check {
+			($param:ident, $val:expr $(,)?) => {
+				if seen.contains_key($param) {
+					return Err(DuplicatedParam($param))
+				}
+				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))
+				}
+				if $out_of_limit {
+					return Err(OutsideLimit($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",
+				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",
+			};
+
+			match *param {
+				MaxMemoryPages(val) => {
+					check!(param_ident, val, val == 0 || val > MEMORY_PAGES_MAX,);
+				},
+
+				StackLogicalMax(val) => {
+					check!(param_ident, val, val < LOGICAL_MAX_LO || val > LOGICAL_MAX_HI,);
+				},
+
+				StackNativeMax(val) => {
+					check!(param_ident, val);
+				},
+
+				PrecheckingMaxMemory(val) => {
+					check!(
+						param_ident,
+						val,
+						val < PRECHECK_MEM_MAX_LO || val > PRECHECK_MEM_MAX_HI,
+					);
+				},
+
+				PvfPrepTimeout(_, val) => {
+					check!(param_ident, val);
+				},
+
+				PvfExecTimeout(_, val) => {
+					check!(param_ident, val);
+				},
+
+				WasmExtBulkMemory => {
+					check!(param_ident, 1);
+				},
+			}
+		}
+
+		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"))
+			}
+		}
+
+		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",
+				))
+			}
+		}
+
+		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",
+				))
+			}
+		}
+
+		Ok(())
+	}
 }
 
 impl Deref for ExecutorParams {

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::{

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<BlockNumber> {
 	MaxHrmpInboundChannelsExceeded,
 	/// `minimum_backing_votes` is set to zero.
 	ZeroMinimumBackingVotes,
+	/// `executor_params` are inconsistent.
+	InconsistentExecutorParams { inner: ExecutorParamError },
 }
 
 impl<BlockNumber> HostConfiguration<BlockNumber>
@@ -432,6 +435,10 @@ where
 			return Err(ZeroMinimumBackingVotes)
 		}
 
+		if let Err(inner) = self.executor_params.check_consistency() {
+			return Err(InconsistentExecutorParams { inner })
+		}
+
 		Ok(())
 	}