goblin: serialize proof-address allocation, atomic persist, refuse corrupt registry

The per-sale proof-address allocator did an unlocked read-modify-write of the
persisted counter, so two concurrent mints could read the same next index and
hand out the same derivation index, an address reuse that mis-addresses a
payment proof. Serialize the whole load-bump-persist behind a process-wide
lock so every index is unique.

Persist the counter atomically: write a sibling temp file in the same
directory, flush, then rename over the target, so a crash mid-write cannot
leave a torn file.

Refuse a corrupt registry: a file that exists but fails to read or parse now
returns an error instead of silently resetting the counter to 1 (which would
re-hand-out already-minted indices and reuse an address). Only a missing file
defaults to the fresh-wallet counter. Adds a concurrency test asserting unique
indices across racing threads, plus missing-vs-corrupt coverage.

src/wallet/proof_addrs.rs

@@ -29,6 +29,14 @@
 
 use serde_derive::{Deserialize, Serialize};
 use std::path::PathBuf;
+use std::sync::Mutex;
+
+/// Serializes the whole load-bump-persist in [`allocate`] across threads. The
+/// counter lives in a file, so without this two concurrent mints would each
+/// read the same `next`, hand out the SAME derivation index, and mis-address a
+/// payment proof (money path). This process-wide lock makes allocation one
+/// atomic step so every index handed out is unique.
+static ALLOC_LOCK: Mutex<()> = Mutex::new(());
 
 /// Highest allocatable proof-address derivation index. MUST equal the wallet
 /// receive path's scan bound (`address::MAX_PROOF_ADDRESS_INDEX` in the
@@ -57,20 +65,61 @@ impl Default for ProofAddrRegistry {
 /// persisted BEFORE the index is returned, so a crash after allocation burns
 /// the index rather than ever reusing it.
 pub fn allocate(path: &PathBuf) -> Result<u32, String> {
-	let mut reg: ProofAddrRegistry = std::fs::read_to_string(path)
-		.ok()
-		.and_then(|raw| serde_json::from_str(&raw).ok())
-		.unwrap_or_default();
+	// One allocation at a time. Without this the load-bump-persist below is a
+	// racy read-modify-write and two concurrent mints hand out the same index.
+	let _guard = ALLOC_LOCK
+		.lock()
+		.map_err(|_| "proof-address allocator lock poisoned".to_string())?;
+	let mut reg = load_registry(path)?;
 	let index = reg.next;
 	if index > MAX_PROOF_ADDRESS_INDEX {
 		return Err("proof address space exhausted".to_string());
 	}
 	reg.next = index + 1;
-	let raw = serde_json::to_string(&reg).map_err(|e| e.to_string())?;
-	std::fs::write(path, raw).map_err(|e| e.to_string())?;
+	persist_registry(path, &reg)?;
 	Ok(index)
 }
 
+/// Load the persisted counter. A MISSING file is the fresh-wallet case and
+/// defaults to `{ver:1, next:1}`. A file that EXISTS but cannot be read or
+/// parsed is REFUSED (`Err`) rather than silently reset: resetting the counter
+/// to 1 would re-hand-out already-minted indices and reuse an address, so a
+/// corrupt registry must stop minting, not quietly start over. Unknown extra
+/// fields are ignored (forward-compat), so only genuinely malformed content
+/// refuses.
+fn load_registry(path: &PathBuf) -> Result<ProofAddrRegistry, String> {
+	match std::fs::read_to_string(path) {
+		Ok(raw) => serde_json::from_str(&raw)
+			.map_err(|e| format!("proof-address registry is corrupt, refusing to allocate: {e}")),
+		Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(ProofAddrRegistry::default()),
+		Err(e) => Err(format!("proof-address registry unreadable: {e}")),
+	}
+}
+
+/// Persist the counter atomically: write a sibling temp file in the SAME
+/// directory, flush it, then rename it over the target. The rename is atomic, so
+/// a crash mid-write can never leave a half-written (and then corrupt-refused)
+/// registry, and a reader never sees a torn file.
+fn persist_registry(path: &PathBuf, reg: &ProofAddrRegistry) -> Result<(), String> {
+	use std::io::Write;
+	let raw = serde_json::to_string(reg).map_err(|e| e.to_string())?;
+	let suffix = std::time::SystemTime::now()
+		.duration_since(std::time::UNIX_EPOCH)
+		.map(|d| d.as_nanos())
+		.unwrap_or(0);
+	// Sibling of the target (same directory, so the rename stays on one volume).
+	let tmp = path.with_extension(format!("tmp-{}-{}", std::process::id(), suffix));
+	{
+		let mut f = std::fs::File::create(&tmp).map_err(|e| e.to_string())?;
+		f.write_all(raw.as_bytes()).map_err(|e| e.to_string())?;
+		let _ = f.sync_all();
+	}
+	std::fs::rename(&tmp, path).map_err(|e| {
+		let _ = std::fs::remove_file(&tmp);
+		e.to_string()
+	})
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;
@@ -110,13 +159,58 @@ mod tests {
 	}
 
 	#[test]
-	fn corrupt_registry_restarts_at_one() {
-		// A corrupt counter must not brick minting; restarting at 1 can re-hand
-		// out an index, which at worst reuses an address (same as the old
-		// single-address world) — never a fund risk.
+	fn corrupt_registry_refuses() {
+		// A corrupt counter must REFUSE rather than silently restart at 1:
+		// restarting would re-hand-out an already-minted index and reuse an
+		// address (a mis-addressed payment proof), so a corrupt registry stops
+		// minting instead of quietly starting over. Only a MISSING file defaults.
 		let path = tmpfile("corrupt");
 		std::fs::write(&path, "not json").unwrap();
+		assert!(allocate(&path).is_err(), "corrupt registry must refuse");
+		let _ = std::fs::remove_file(&path);
+	}
+
+	#[test]
+	fn missing_file_defaults_but_present_survives() {
+		// A missing file is the fresh-wallet case: default and allocate 1.
+		let path = tmpfile("missing");
 		assert_eq!(allocate(&path).unwrap(), 1);
+		// After the first allocate the file exists and the counter persisted.
+		assert_eq!(allocate(&path).unwrap(), 2);
+		let _ = std::fs::remove_file(&path);
+	}
+
+	#[test]
+	fn concurrent_allocation_hands_out_unique_indices() {
+		use std::collections::HashSet;
+		// Many threads racing on one shared registry must never hand out the same
+		// index twice, which is exactly what the allocator lock prevents.
+		let path = tmpfile("concurrent");
+		let threads = 8usize;
+		let per_thread = 6usize;
+		let mut handles = Vec::new();
+		for _ in 0..threads {
+			let p = path.clone();
+			handles.push(std::thread::spawn(move || {
+				let mut got = Vec::with_capacity(per_thread);
+				for _ in 0..per_thread {
+					got.push(allocate(&p).unwrap());
+				}
+				got
+			}));
+		}
+		let mut all = Vec::new();
+		for h in handles {
+			all.extend(h.join().unwrap());
+		}
+		let total = threads * per_thread;
+		assert_eq!(all.len(), total);
+		let unique: HashSet<u32> = all.iter().copied().collect();
+		assert_eq!(unique.len(), total, "every allocated index must be unique");
+		// Counter started at 1, so after `total` allocations `next` == 1 + total.
+		let reg: ProofAddrRegistry =
+			serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap();
+		assert_eq!(reg.next, 1 + total as u32);
 		let _ = std::fs::remove_file(&path);
 	}
 }

src/wallet/wallet.rs

@@ -1140,6 +1140,10 @@ impl Wallet {
 	/// payment-proof slate is addressed to and signs the proof with the
 	/// matching key. Returns `(index, address)`.
 	pub fn mint_proof_address(&self) -> Result<(u32, String), String> {
+		// `allocate` is called before we take the wallet instance lock, but that is
+		// safe: the allocator serializes its whole read-modify-write behind its own
+		// process-wide lock, so two concurrent mints can never be handed the same
+		// index regardless of the instance-lock ordering here.
 		let index =
 			crate::wallet::proof_addrs::allocate(&self.get_config().get_proof_addrs_path())?;
 		let r_inst = self.instance.as_ref().read();