Fix executor params classification and add test (#10194)

Closes #8054 

Executor parameters may be hashed in two ways: a full hash that changes
if any parameter changes, and a prep_hash, which changes only if the
parameter change will affect the prepared artifact (thus requiring
artifact recompilation).

As was discovered in #8054, the initial classification of parameters was
wrong, and some parameters that affect the artifact were not classified
as such, and vice versa.

This PR aims to fix that and to add a test that should prevent this from
happening in the future.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: s0me0ne-unkn0wn

Cargo.lock

@@ -43,6 +43,7 @@ seccompiler = { workspace = true }
 
 [dev-dependencies]
 assert_matches = { workspace = true }
+wat = { workspace = true }
 
 [target.'cfg(target_os = "linux")'.dev-dependencies]
 tempfile = { workspace = true }

polkadot/node/core/pvf/common/Cargo.toml

@@ -379,3 +379,119 @@ impl sp_core::traits::ReadRuntimeVersion for ReadRuntimeVersion {
 		}
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	#[test]
+	fn prep_hash_matches_artifact_effect_of_executor_params() {
+		use ExecutorParam::*;
+
+		// If you're adding a new ExecutorParam, please add it to the `cases` below.
+
+		let _coverage_check = |param: &ExecutorParam| match param {
+			MaxMemoryPages(_) => true,
+			StackLogicalMax(_) => true,
+			StackNativeMax(_) => true,
+			PrecheckingMaxMemory(_) => true,
+			PvfPrepTimeout(_, _) => true,
+			PvfExecTimeout(_, _) => true,
+			WasmExtBulkMemory => true,
+		};
+
+		// A minimal module with memory and an exported `validate_block` function.
+		let wat = r#"(module
+			(memory 1)
+			(func (export "validate_block") (param i32 i32))
+		)"#;
+		let wasm = wat::parse_str(wat).expect("wat parsing failed");
+		let blob = prevalidate(&wasm).expect("valid runtime blob");
+
+		let base = ExecutorParams::default();
+
+		let prepare_with = |params: &ExecutorParams| -> Vec<u8> {
+			prepare(blob.clone(), params).expect("prepare should succeed")
+		};
+
+		// Define pairs that toggle exactly one parameter.
+		let cases: Vec<(&str, ExecutorParams, ExecutorParams)> = vec![
+			(
+				"MaxMemoryPages",
+				base.clone(),
+				ExecutorParams::from(&[ExecutorParam::MaxMemoryPages(128)][..]),
+			),
+			(
+				"StackLogicalMax",
+				base.clone(),
+				ExecutorParams::from(
+					&[ExecutorParam::StackLogicalMax(DEFAULT_LOGICAL_STACK_MAX + 1)][..],
+				),
+			),
+			(
+				"StackNativeMax",
+				base.clone(),
+				ExecutorParams::from(
+					&[ExecutorParam::StackNativeMax(DEFAULT_NATIVE_STACK_MAX + 1024)][..],
+				),
+			),
+			(
+				"PrecheckingMaxMemory",
+				base.clone(),
+				ExecutorParams::from(&[ExecutorParam::PrecheckingMaxMemory(300 * 1024 * 1024)][..]),
+			),
+			(
+				"PvfPrepTimeout(Precheck)",
+				base.clone(),
+				ExecutorParams::from(
+					&[ExecutorParam::PvfPrepTimeout(polkadot_primitives::PvfPrepKind::Precheck, 1)]
+						[..],
+				),
+			),
+			(
+				"PvfPrepTimeout(Prepare)",
+				base.clone(),
+				ExecutorParams::from(
+					&[ExecutorParam::PvfPrepTimeout(polkadot_primitives::PvfPrepKind::Prepare, 2)]
+						[..],
+				),
+			),
+			(
+				"PvfExecTimeout(Backing)",
+				base.clone(),
+				ExecutorParams::from(
+					&[ExecutorParam::PvfExecTimeout(polkadot_primitives::PvfExecKind::Backing, 1)]
+						[..],
+				),
+			),
+			(
+				"PvfExecTimeout(Approval)",
+				base.clone(),
+				ExecutorParams::from(
+					&[ExecutorParam::PvfExecTimeout(polkadot_primitives::PvfExecKind::Approval, 2)]
+						[..],
+				),
+			),
+			(
+				"WasmExtBulkMemory",
+				base.clone(),
+				ExecutorParams::from(&[ExecutorParam::WasmExtBulkMemory][..]),
+			),
+		];
+
+		for (name, a, b) in cases.into_iter() {
+			let art_a = prepare_with(&a);
+			let art_b = prepare_with(&b);
+			let artifact_changed = art_a != art_b;
+			let prep_hash_changed = a.prep_hash() != b.prep_hash();
+			assert_eq!(
+				artifact_changed,
+				prep_hash_changed,
+				"ExecutorParam classification mismatch for {}: artifact_changed={}, prep_hash_changed={}",
+				name,
+				artifact_changed,
+				prep_hash_changed,
+			);
+		}
+	}
+}

polkadot/node/core/pvf/common/src/executor_interface.rs

@@ -29,8 +29,7 @@ use polkadot_node_primitives::{PoV, POV_BOMB_LIMIT};
 use polkadot_node_subsystem::messages::PvfExecKind;
 use polkadot_parachain_primitives::primitives::{BlockData, ValidationResult};
 use polkadot_primitives::{
-	ExecutorParam, ExecutorParams, Hash, PersistedValidationData,
-	PvfExecKind as RuntimePvfExecKind, PvfPrepKind,
+	ExecutorParam, ExecutorParams, Hash, PersistedValidationData, PvfExecKind as RuntimePvfExecKind,
 };
 use sp_core::H256;
 
@@ -766,8 +765,7 @@ async fn artifact_does_reprepare_on_meaningful_exec_parameter_change() {
 	let cache_dir = host.cache_dir.path();
 
 	let set1 = ExecutorParams::default();
-	let set2 =
-		ExecutorParams::from(&[ExecutorParam::PvfPrepTimeout(PvfPrepKind::Prepare, 60000)][..]);
+	let set2 = ExecutorParams::from(&[ExecutorParam::MaxMemoryPages(128)][..]);
 
 	let _stats = host
 		.precheck_pvf(test_parachain_halt::wasm_binary_unwrap(), set1)

polkadot/node/core/pvf/tests/it/main.rs

@@ -199,7 +199,7 @@ impl core::fmt::LowerHex for ExecutorParamsPrepHash {
 // they show up, they must be clearly documented as mandatory ones.
 //
 // !!! Any new parameter that does not affect the prepared artifact must be added to the exclusion
-// !!! list in `prep_hash()` to avoid unneccessary artifact rebuilds.
+// !!! list in `prep_hash()` to avoid unnecessary artifact rebuilds.
 #[derive(
 	Clone,
 	Debug,
@@ -226,7 +226,8 @@ impl ExecutorParams {
 		ExecutorParamsHash(BlakeTwo256::hash(&self.encode()))
 	}
 
-	/// Returns hash of preparation-related executor parameters
+	/// Returns hash of preparation-related executor parameters (those affecting the artifact
+	/// produced on the preparation step).
 	pub fn prep_hash(&self) -> ExecutorParamsPrepHash {
 		use ExecutorParam::*;
 
@@ -235,11 +236,11 @@ impl ExecutorParams {
 		self.0
 			.iter()
 			.flat_map(|param| match param {
-				MaxMemoryPages(..) => None,
+				MaxMemoryPages(..) => Some(param),
 				StackLogicalMax(..) => Some(param),
 				StackNativeMax(..) => None,
 				PrecheckingMaxMemory(..) => None,
-				PvfPrepTimeout(..) => Some(param),
+				PvfPrepTimeout(..) => None,
 				PvfExecTimeout(..) => None,
 				WasmExtBulkMemory => Some(param),
 			})
@@ -413,7 +414,7 @@ impl From<&[ExecutorParam]> for ExecutorParams {
 // This test ensures the hash generated by `prep_hash()` changes if any preparation-related
 // executor parameter changes. If you're adding a new executor parameter, you must add it into
 // this test, and if changing that parameter may not affect the artifact produced on the
-// preparation step, it must be added to the list of exlusions in `pre_hash()` as well.
+// preparation step, it must be added to the list of exclusions in `pre_hash()` as well.
 // See also `prep_hash()` comments.
 #[test]
 fn ensure_prep_hash_changes() {
@@ -434,21 +435,17 @@ fn ensure_prep_hash_changes() {
 
 	for p in ep.iter() {
 		let (ep1, ep2) = match p {
-			MaxMemoryPages(_) => continue,
+			MaxMemoryPages(_) => (
+				ExecutorParams::from(&[MaxMemoryPages(1)][..]),
+				ExecutorParams::from(&[MaxMemoryPages(2)][..]),
+			),
 			StackLogicalMax(_) => (
 				ExecutorParams::from(&[StackLogicalMax(1)][..]),
 				ExecutorParams::from(&[StackLogicalMax(2)][..]),
 			),
 			StackNativeMax(_) => continue,
 			PrecheckingMaxMemory(_) => continue,
-			PvfPrepTimeout(PvfPrepKind::Precheck, _) => (
-				ExecutorParams::from(&[PvfPrepTimeout(PvfPrepKind::Precheck, 1)][..]),
-				ExecutorParams::from(&[PvfPrepTimeout(PvfPrepKind::Precheck, 2)][..]),
-			),
-			PvfPrepTimeout(PvfPrepKind::Prepare, _) => (
-				ExecutorParams::from(&[PvfPrepTimeout(PvfPrepKind::Prepare, 1)][..]),
-				ExecutorParams::from(&[PvfPrepTimeout(PvfPrepKind::Prepare, 2)][..]),
-			),
+			PvfPrepTimeout(_, _) => continue,
 			PvfExecTimeout(_, _) => continue,
 			WasmExtBulkMemory =>
 				(ExecutorParams::default(), ExecutorParams::from(&[WasmExtBulkMemory][..])),

polkadot/primitives/src/v9/executor_params.rs

@@ -0,0 +1,13 @@
+title: Fix executor parameters classification
+doc:
+- audience: Node Dev
+  description: |-
+    Executor parameters may be hashed in two ways: a full hash that changes if any parameter changes, and a prep_hash, which changes only if the parameter change will affect the prepared artifact (thus requiring artifact recompilation).
+    This PR aims to fix a bug in the initial classification and to add a test that should prevent those bugs from happening in the future.
+crates:
+- name: polkadot-node-core-pvf-common
+  bump: patch
+- name: polkadot-node-core-pvf
+  bump: patch
+- name: polkadot-primitives
+  bump: patch