f: review comments

Author: joostjager

fuzz/Cargo.toml

@@ -26,7 +26,6 @@ lightning-persister = { path = "../lightning-persister", features = ["tokio"]}
 bech32 = "0.11.0"
 bitcoin = { version = "0.32.2", features = ["secp-lowmemory"] }
 tokio = { version = "~1.35", default-features = false, features = ["rt-multi-thread"] }
-uuid = { version = "1.3", features = ["v4"] }
 
 afl = { version = "0.12", optional = true }
 honggfuzz = { version = "0.5", optional = true, default-features = false }

fuzz/src/fs_store.rs

@@ -1,8 +1,8 @@
+use core::hash::{BuildHasher, Hasher};
 use lightning::util::persist::{KVStore, KVStoreSync};
 use lightning_persister::fs_store::FilesystemStore;
 use std::fs;
 use tokio::runtime::Runtime;
-use uuid::Uuid;
 
 use crate::utils::test_logger;
 
@@ -20,7 +20,8 @@ impl TempFilesystemStore {
 			std::env::temp_dir()
 		};
 
-		let random_folder_name = format!("fs_store_fuzz_{}", Uuid::new_v4());
+		let random_number = std::collections::hash_map::RandomState::new().build_hasher().finish();
+		let random_folder_name = format!("fs_store_fuzz_{:016x}", random_number);
 		temp_path.push(random_folder_name);
 
 		let inner = FilesystemStore::new(temp_path.clone());
@@ -46,10 +47,11 @@ async fn do_test_internal<Out: test_logger::Output>(data: &[u8], _out: Out) {
 		($len: expr) => {{
 			let slice_len = $len as usize;
 			if data.len() < read_pos + slice_len {
-				return;
+				None
+			} else {
+				read_pos += slice_len;
+				Some(&data[read_pos - slice_len..read_pos])
 			}
-			read_pos += slice_len;
-			&data[read_pos - slice_len..read_pos]
 		}};
 	}
 
@@ -72,7 +74,10 @@ async fn do_test_internal<Out: test_logger::Output>(data: &[u8], _out: Out) {
 
 	let mut handles = Vec::new();
 	loop {
-		let v = get_slice!(1)[0];
+		let v = match get_slice!(1) {
+			Some(b) => b[0],
+			None => break,
+		};
 		match v % 13 {
 			// Sync write
 			0 => {
@@ -104,7 +109,7 @@ async fn do_test_internal<Out: test_logger::Output>(data: &[u8], _out: Out) {
 			3 => {
 				_ = KVStoreSync::read(fs_store, primary_namespace, secondary_namespace, key);
 			},
-			// Async write
+			// Async write. Bias writes a bit.
 			4..=9 => {
 				let data_value = get_next_data_value();
 
@@ -127,8 +132,9 @@ async fn do_test_internal<Out: test_logger::Output>(data: &[u8], _out: Out) {
 			},
 			// Async remove
 			10 | 11 => {
+				let lazy = v == 10;
 				let fut =
-					KVStore::remove(fs_store, primary_namespace, secondary_namespace, key, v == 10);
+					KVStore::remove(fs_store, primary_namespace, secondary_namespace, key, lazy);
 
 				// Already set the current_data, even though writing hasn't finished yet. This supports the call-time
 				// ordering semantics.
@@ -143,9 +149,7 @@ async fn do_test_internal<Out: test_logger::Output>(data: &[u8], _out: Out) {
 					let _ = handle.await.unwrap();
 				}
 			},
-			_ => {
-				return;
-			},
+			_ => unreachable!(),
 		}
 
 		// If no more writes are pending, we can reliably see if the data is consistent.
@@ -160,6 +164,12 @@ async fn do_test_internal<Out: test_logger::Output>(data: &[u8], _out: Out) {
 			assert_eq!(0, fs_store.state_size());
 		}
 	}
+
+	// Always make sure that all async tasks are completed before returning. Otherwise the temporary storage dir could
+	// be removed, and then again recreated by unfinished tasks.
+	for handle in handles.drain(..) {
+		let _ = handle.await.unwrap();
+	}
 }
 
 /// Method that needs to be added manually, {name}_test

lightning-persister/src/fs_store.rs

@@ -198,7 +198,7 @@ impl FilesystemStoreInner {
 		let mut buf = Vec::new();
 
 		self.execute_locked_read(dest_file_path.clone(), || {
-			let mut f = fs::File::open(dest_file_path.clone())?;
+			let mut f = fs::File::open(dest_file_path)?;
 			f.read_to_end(&mut buf)?;
 			Ok(())
 		})?;
@@ -209,19 +209,21 @@ impl FilesystemStoreInner {
 	fn execute_locked_write<F: FnOnce() -> Result<(), lightning::io::Error>>(
 		&self, inner_lock_ref: Arc<RwLock<u64>>, dest_file_path: PathBuf, version: u64, callback: F,
 	) -> Result<(), lightning::io::Error> {
-		let mut last_written_version = inner_lock_ref.write().unwrap();
+		let res = {
+			let mut last_written_version = inner_lock_ref.write().unwrap();
 
-		// Check if we already have a newer version written/removed. This is used in async contexts to realize eventual
-		// consistency.
-		let is_stale_version = version <= *last_written_version;
+			// Check if we already have a newer version written/removed. This is used in async contexts to realize eventual
+			// consistency.
+			let is_stale_version = version <= *last_written_version;
 
-		// If the version is not stale, we execute the callback. Otherwise we can and must skip writing.
-		let res = if is_stale_version {
-			Ok(())
-		} else {
-			callback().map(|_| {
-				*last_written_version = version;
-			})
+			// If the version is not stale, we execute the callback. Otherwise we can and must skip writing.
+			if is_stale_version {
+				Ok(())
+			} else {
+				callback().map(|_| {
+					*last_written_version = version;
+				})
+			}
 		};
 
 		self.clean_locks(&inner_lock_ref, dest_file_path);
@@ -233,8 +235,10 @@ impl FilesystemStoreInner {
 		&self, dest_file_path: PathBuf, callback: F,
 	) -> Result<(), lightning::io::Error> {
 		let inner_lock_ref = self.get_inner_lock_ref(dest_file_path.clone());
-		let _guard = inner_lock_ref.read().unwrap();
-		let res = callback();
+		let res = {
+			let _guard = inner_lock_ref.read().unwrap();
+			callback()
+		};
 		self.clean_locks(&inner_lock_ref, dest_file_path);
 		res
 	}
@@ -245,7 +249,11 @@ impl FilesystemStoreInner {
 		// inner_lock_ref. The outer lock is obtained first, to avoid a new arc being cloned after we've already
 		// counted.
 		let mut outer_lock = self.locks.lock().unwrap();
-		if Arc::strong_count(&inner_lock_ref) == 2 {
+
+		let strong_count = Arc::strong_count(&inner_lock_ref);
+		debug_assert!(strong_count >= 2, "Unexpected FilesystemStore strong count");
+
+		if strong_count == 2 {
 			outer_lock.remove(&dest_file_path);
 		}
 	}