PVF: fix unshare "could not create temporary directory"; refactor (#2663)

Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>

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

@@ -201,13 +201,20 @@ impl Artifacts {
 	/// Create an empty table and populate it with valid artifacts as [`ArtifactState::Prepared`],
 	/// if any. The existing caches will be checked by their file name to determine whether they are
 	/// valid, e.g., matching the current node version. The ones deemed invalid will be pruned.
+	///
+	/// Create the cache directory on-disk if it doesn't exist.
 	pub async fn new_and_prune(cache_path: &Path) -> Self {
 		let mut artifacts = Self { inner: HashMap::new() };
-		artifacts.insert_and_prune(cache_path).await;
+		let _ = artifacts.insert_and_prune(cache_path).await.map_err(|err| {
+			gum::error!(
+				target: LOG_TARGET,
+				"could not initialize artifacts cache: {err}",
+			)
+		});
 		artifacts
 	}
 
-	async fn insert_and_prune(&mut self, cache_path: &Path) {
+	async fn insert_and_prune(&mut self, cache_path: &Path) -> Result<(), String> {
 		async fn is_corrupted(path: &Path) -> bool {
 			let checksum = match tokio::fs::read(path).await {
 				Ok(bytes) => blake3::hash(&bytes),
@@ -237,24 +244,16 @@ impl Artifacts {
 			artifacts: &mut Artifacts,
 			entry: &tokio::fs::DirEntry,
 			cache_path: &Path,
-		) {
+		) -> Result<(), String> {
 			let file_type = entry.file_type().await;
 			let file_name = entry.file_name();
 
 			match file_type {
 				Ok(file_type) =>
 					if !file_type.is_file() {
-						return
+						return Ok(())
 					},
-				Err(err) => {
+				Err(err) => return Err(format!("unable to get file type for {file_name:?}: {err}")),
-					gum::warn!(
-						target: LOG_TARGET,
-						?err,
-						"unable to get file type for {:?}",
-						file_name,
-					);
-					return
-				},
 			}
 
 			if let Some(file_name) = file_name.to_str() {
@@ -262,73 +261,51 @@ impl Artifacts {
 				let path = cache_path.join(file_name);
 
 				if id.is_none() || is_corrupted(&path).await {
-					gum::warn!(
-						target: LOG_TARGET,
-						"discarding invalid artifact {:?}",
-						&path,
-					);
 					let _ = tokio::fs::remove_file(&path).await;
-					return
+					return Err(format!("invalid artifact {path:?}, file deleted"))
 				}
 
-				if let Some(id) = id {
+				let id = id.expect("checked is_none() above; qed");
-					gum::debug!(
+				gum::debug!(
-						target: LOG_TARGET,
-						"reusing existing {:?} for node version v{}",
-						&path,
-						NODE_VERSION,
-					);
-					artifacts.insert_prepared(id, path, SystemTime::now(), Default::default());
-				}
-			} else {
-				gum::warn!(
 					target: LOG_TARGET,
-					"non-Unicode file name {:?} found in {:?}",
+					"reusing existing {:?} for node version v{}",
-					file_name,
+					&path,
-					cache_path,
+					NODE_VERSION,
 				);
+				artifacts.insert_prepared(id, path, SystemTime::now(), Default::default());
+
+				Ok(())
+			} else {
+				Err(format!("non-Unicode file name {file_name:?} found in {cache_path:?}"))
 			}
 		}
 
 		// Make sure that the cache path directory and all its parents are created.
 		if let Err(err) = tokio::fs::create_dir_all(cache_path).await {
 			if err.kind() != io::ErrorKind::AlreadyExists {
-				gum::error!(
+				return Err(format!("failed to create dir {cache_path:?}: {err}"))
-					target: LOG_TARGET,
-					?err,
-					"failed to create dir {:?}",
-					cache_path,
-				);
-				return
 			}
 		}
 
-		let mut dir = match tokio::fs::read_dir(cache_path).await {
+		let mut dir = tokio::fs::read_dir(cache_path)
-			Ok(dir) => dir,
+			.await
-			Err(err) => {
+			.map_err(|err| format!("failed to read dir {cache_path:?}: {err}"))?;
-				gum::error!(
-					target: LOG_TARGET,
-					?err,
-					"failed to read dir {:?}",
-					cache_path,
-				);
-				return
-			},
-		};
 
 		loop {
 			match dir.next_entry().await {
-				Ok(Some(entry)) => insert_or_prune(self, &entry, cache_path).await,
+				Ok(Some(entry)) =>
-				Ok(None) => break,
+					if let Err(err) = insert_or_prune(self, &entry, cache_path).await {
-				Err(err) => {
+						gum::warn!(
-					gum::warn!(
+							target: LOG_TARGET,
-						target: LOG_TARGET,
+							?cache_path,
-						?err,
+							"could not insert entry {:?} into the artifact cache: {}",
-						"error processing artifacts in {:?}",
+							entry,
-						cache_path,
+							err,
-					);
+						)
-					break
+					},
-				},
+				Ok(None) => return Ok(()),
+				Err(err) =>
+					return Err(format!("error processing artifacts in {cache_path:?}: {err}")),
 			}
 		}
 	}

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

@@ -217,6 +217,9 @@ pub async fn start(
 ) -> SubsystemResult<(ValidationHost, impl Future<Output = ()>)> {
 	gum::debug!(target: LOG_TARGET, ?config, "starting PVF validation host");
 
+	// Make sure the cache is initialized before doing anything else.
+	let artifacts = Artifacts::new_and_prune(&config.cache_path).await;
+
 	// Run checks for supported security features once per host startup. If some checks fail, warn
 	// if Secure Validator Mode is disabled and return an error otherwise.
 	let security_status = match security::check_security_status(&config).await {
@@ -260,8 +263,6 @@ pub async fn start(
 	let run_sweeper = sweeper_task(to_sweeper_rx);
 
 	let run_host = async move {
-		let artifacts = Artifacts::new_and_prune(&config.cache_path).await;
-
 		run(Inner {
 			cleanup_pulse_interval: Duration::from_secs(3600),
 			artifact_ttl: Duration::from_secs(3600 * 24),

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

@@ -445,3 +445,21 @@ async fn all_security_features_work() {
 		}
 	);
 }
+
+// Regression test to make sure the unshare-pivot-root capability does not depend on the PVF
+// artifacts cache existing.
+#[cfg(all(feature = "ci-only-tests", target_os = "linux"))]
+#[tokio::test]
+async fn nonexistant_cache_dir() {
+	let host = TestHost::new_with_config(|cfg| {
+		cfg.cache_path = cfg.cache_path.join("nonexistant_cache_dir");
+	})
+	.await;
+
+	assert!(host.security_status().await.can_unshare_user_namespace_and_change_root);
+
+	let _stats = host
+		.precheck_pvf(::adder::wasm_binary_unwrap(), Default::default())
+		.await
+		.unwrap();
+}

prdoc/pr_2663-fix-could-not-create-temporary-drectory.prdoc

@@ -0,0 +1,17 @@
+title: "PVF: fix unshare 'could not create temporary directory'"
+
+doc:
+  - audience: Node Operator
+    description: |
+      For validators: fixes the potential warning/error:
+      "Cannot unshare user namespace and change root, which are Linux-specific kernel security features: could not create a temporary directory in "/tmp/.tmpIcLriO".
+
+migrations:
+  db: []
+
+  runtime: []
+
+crates:
+  - name: polkadot-node-core-pvf
+
+host_functions: []