Do not re-prepare PVFs if not needed

polkadot/node/core/pvf/src/artifacts.rs

@@ -58,7 +58,7 @@ use crate::{host::PrecheckResultSender, worker_interface::WORKER_DIR_PREFIX};
 use always_assert::always;
 use polkadot_node_core_pvf_common::{error::PrepareError, prepare::PrepareStats, pvf::PvfPrepData};
 use polkadot_parachain_primitives::primitives::ValidationCodeHash;
-use polkadot_primitives::ExecutorParamsHash;
+use polkadot_primitives::ExecutorParamsPrepHash;
 use std::{
 	collections::HashMap,
 	fs,
@@ -85,22 +85,27 @@ pub fn generate_artifact_path(cache_path: &Path) -> PathBuf {
 	artifact_path
 }
 
-/// Identifier of an artifact. Encodes a code hash of the PVF and a hash of executor parameter set.
+/// Identifier of an artifact. Encodes a code hash of the PVF and a hash of preparation-related
+///  executor parameter set.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct ArtifactId {
 	pub(crate) code_hash: ValidationCodeHash,
-	pub(crate) executor_params_hash: ExecutorParamsHash,
+	pub(crate) executor_params_prep_hash: ExecutorParamsPrepHash,
 }
 
 impl ArtifactId {
 	/// Creates a new artifact ID with the given hash.
-	pub fn new(code_hash: ValidationCodeHash, executor_params_hash: ExecutorParamsHash) -> Self {
-		Self { code_hash, executor_params_hash }
+	pub fn new(
+		code_hash: ValidationCodeHash,
+		executor_params_prep_hash: ExecutorParamsPrepHash,
+	) -> Self {
+		Self { code_hash, executor_params_prep_hash }
 	}
 
-	/// Returns an artifact ID that corresponds to the PVF with given executor params.
+	/// Returns an artifact ID that corresponds to the PVF with given preparation-related
+	/// executor parameters.
 	pub fn from_pvf_prep_data(pvf: &PvfPrepData) -> Self {
-		Self::new(pvf.code_hash(), pvf.executor_params().hash())
+		Self::new(pvf.code_hash(), pvf.executor_params().prep_hash())
 	}
 }
 

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

@@ -26,7 +26,7 @@ use polkadot_node_core_pvf::{
 	ValidationHost, JOB_TIMEOUT_WALL_CLOCK_FACTOR,
 };
 use polkadot_parachain_primitives::primitives::{BlockData, ValidationParams, ValidationResult};
-use polkadot_primitives::{ExecutorParam, ExecutorParams};
+use polkadot_primitives::{ExecutorParam, ExecutorParams, PvfExecKind, PvfPrepKind};
 
 use std::{io::Write, time::Duration};
 use tokio::sync::Mutex;
@@ -559,3 +559,73 @@ async fn nonexistent_cache_dir() {
 		.await
 		.unwrap();
 }
+
+// Checks the the artifact is not re-prepared when the executor environment parameters change
+// in a way not affecting the preparation
+#[tokio::test]
+async fn artifact_does_not_reprepare_on_non_meaningful_exec_parameter_change() {
+	let host = TestHost::new_with_config(|cfg| {
+		cfg.prepare_workers_hard_max_num = 1;
+	})
+	.await;
+	let cache_dir = host.cache_dir.path();
+
+	let set1 = ExecutorParams::default();
+	let set2 =
+		ExecutorParams::from(&[ExecutorParam::PvfExecTimeout(PvfExecKind::Backing, 2500)][..]);
+
+	let _stats = host.precheck_pvf(halt::wasm_binary_unwrap(), set1).await.unwrap();
+
+	let md1 = {
+		let mut cache_dir: Vec<_> = std::fs::read_dir(cache_dir).unwrap().collect();
+		assert_eq!(cache_dir.len(), 2);
+		let mut artifact_path = cache_dir.pop().unwrap().unwrap();
+		if artifact_path.path().is_dir() {
+			artifact_path = cache_dir.pop().unwrap().unwrap();
+		}
+		std::fs::metadata(artifact_path.path()).unwrap()
+	};
+
+	// FS times are not monotonical so we wait 2 secs here to be sure that the creation time of the
+	// second attifact will be different
+	tokio::time::sleep(Duration::from_secs(2)).await;
+
+	let _stats = host.precheck_pvf(halt::wasm_binary_unwrap(), set2).await.unwrap();
+
+	let md2 = {
+		let mut cache_dir: Vec<_> = std::fs::read_dir(cache_dir).unwrap().collect();
+		assert_eq!(cache_dir.len(), 2);
+		let mut artifact_path = cache_dir.pop().unwrap().unwrap();
+		if artifact_path.path().is_dir() {
+			artifact_path = cache_dir.pop().unwrap().unwrap();
+		}
+		std::fs::metadata(artifact_path.path()).unwrap()
+	};
+
+	assert_eq!(md1.created().unwrap(), md2.created().unwrap());
+}
+
+// Checks if the artifact is re-prepared if the re-preparation is needed by the nature of
+// the execution environment parameters change
+#[tokio::test]
+async fn artifact_does_reprepare_on_meaningful_exec_parameter_change() {
+	let host = TestHost::new_with_config(|cfg| {
+		cfg.prepare_workers_hard_max_num = 1;
+	})
+	.await;
+	let cache_dir = host.cache_dir.path();
+
+	let set1 = ExecutorParams::default();
+	let set2 =
+		ExecutorParams::from(&[ExecutorParam::PvfPrepTimeout(PvfPrepKind::Prepare, 60000)][..]);
+
+	let _stats = host.precheck_pvf(halt::wasm_binary_unwrap(), set1).await.unwrap();
+	let cache_dir_contents: Vec<_> = std::fs::read_dir(cache_dir).unwrap().collect();
+
+	assert_eq!(cache_dir_contents.len(), 2);
+
+	let _stats = host.precheck_pvf(halt::wasm_binary_unwrap(), set2).await.unwrap();
+	let cache_dir_contents: Vec<_> = std::fs::read_dir(cache_dir).unwrap().collect();
+
+	assert_eq!(cache_dir_contents.len(), 3); // new artifact has been added
+}

polkadot/primitives/src/lib.rs

@@ -44,7 +44,7 @@ pub use v7::{
 	CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CollatorId,
 	CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex,
 	CoreState, DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, EncodeAs,
-	ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash,
+	ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, ExecutorParamsPrepHash,
 	ExplicitDisputeStatement, GroupIndex, GroupRotationInfo, Hash, HashT, HeadData, Header,
 	HorizontalMessages, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage, IndexedVec,
 	InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, NodeFeatures,

polkadot/primitives/src/v7/executor_params.rs

@@ -152,13 +152,42 @@ impl sp_std::fmt::LowerHex for ExecutorParamsHash {
 	}
 }
 
+/// Unit type wrapper around [`type@Hash`] that represents a hash of preparation-related
+/// executor parameters.
+///
+/// This type is produced by [`ExecutorParams::prep_hash`].
+#[derive(Clone, Copy, Encode, Decode, Hash, Eq, PartialEq, PartialOrd, Ord, TypeInfo)]
+pub struct ExecutorParamsPrepHash(Hash);
+
+impl sp_std::fmt::Display for ExecutorParamsPrepHash {
+	fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result {
+		self.0.fmt(f)
+	}
+}
+
+impl sp_std::fmt::Debug for ExecutorParamsPrepHash {
+	fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result {
+		write!(f, "{:?}", self.0)
+	}
+}
+
+impl sp_std::fmt::LowerHex for ExecutorParamsPrepHash {
+	fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result {
+		sp_std::fmt::LowerHex::fmt(&self.0, f)
+	}
+}
+
 /// # Deterministically serialized execution environment semantics
 /// Represents an arbitrary semantics of an arbitrary execution environment, so should be kept as
 /// abstract as possible.
+//
 // ADR: For mandatory entries, mandatoriness should be enforced in code rather than separating them
 // into individual fields of the structure. Thus, complex migrations shall be avoided when adding
 // new entries and removing old ones. At the moment, there's no mandatory parameters defined. If
 // 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.
 #[derive(
 	Clone, Debug, Default, Encode, Decode, PartialEq, Eq, TypeInfo, Serialize, Deserialize,
 )]
@@ -175,6 +204,28 @@ impl ExecutorParams {
 		ExecutorParamsHash(BlakeTwo256::hash(&self.encode()))
 	}
 
+	/// Returns hash of preparation-related executor parameters
+	pub fn prep_hash(&self) -> ExecutorParamsPrepHash {
+		use ExecutorParam::*;
+
+		let mut enc = b"prep".to_vec();
+
+		self.0
+			.iter()
+			.flat_map(|param| match param {
+				MaxMemoryPages(..) => None,
+				StackLogicalMax(..) => Some(param),
+				StackNativeMax(..) => None,
+				PrecheckingMaxMemory(..) => None,
+				PvfPrepTimeout(..) => Some(param),
+				PvfExecTimeout(..) => None,
+				WasmExtBulkMemory => Some(param),
+			})
+			.for_each(|p| enc.extend(p.encode()));
+
+		ExecutorParamsPrepHash(BlakeTwo256::hash(&enc))
+	}
+
 	/// Returns a PVF preparation timeout, if any
 	pub fn pvf_prep_timeout(&self, kind: PvfPrepKind) -> Option<Duration> {
 		for param in &self.0 {
@@ -336,3 +387,51 @@ impl From<&[ExecutorParam]> for ExecutorParams {
 		ExecutorParams(arr.to_vec())
 	}
 }
+
+// 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.
+// See also `prep_hash()` comments.
+#[test]
+fn ensure_prep_hash_changes() {
+	use ExecutorParam::*;
+	let ep = ExecutorParams::from(
+		&[
+			MaxMemoryPages(0),
+			StackLogicalMax(0),
+			StackNativeMax(0),
+			PrecheckingMaxMemory(0),
+			PvfPrepTimeout(PvfPrepKind::Precheck, 0),
+			PvfPrepTimeout(PvfPrepKind::Prepare, 0),
+			PvfExecTimeout(PvfExecKind::Backing, 0),
+			PvfExecTimeout(PvfExecKind::Approval, 0),
+			WasmExtBulkMemory,
+		][..],
+	);
+
+	for p in ep.iter() {
+		let (ep1, ep2) = match p {
+			MaxMemoryPages(_) => continue,
+			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)][..]),
+			),
+			PvfExecTimeout(_, _) => continue,
+			WasmExtBulkMemory =>
+				(ExecutorParams::default(), ExecutorParams::from(&[WasmExtBulkMemory][..])),
+		};
+
+		assert_ne!(ep1.prep_hash(), ep2.prep_hash());
+	}
+}

polkadot/primitives/src/v7/mod.rs

@@ -62,7 +62,9 @@ pub mod executor_params;
 pub mod slashing;
 
 pub use async_backing::AsyncBackingParams;
-pub use executor_params::{ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash};
+pub use executor_params::{
+	ExecutorParam, ExecutorParamError, ExecutorParams, ExecutorParamsHash, ExecutorParamsPrepHash,
+};
 
 mod metrics;
 pub use metrics::{

prdoc/pr_4211.prdoc

@@ -0,0 +1,15 @@
+title: "Re-prepare PVF artifacts only if needed"
+
+doc:
+  - audience: Node Dev
+    description: |
+      When a change in the executor environment parameters can not affect the prepared artifact,
+      it is preserved without recompilation and used for future executions. That mitigates
+      situations where every unrelated executor parameter change resulted in re-preparing every
+      artifact on every validator, causing a significant finality lag.
+
+crates:
+  - name: polkadot-node-core-pvf
+    bump: minor
+  - name: polkadot-primitives
+    bump: minor