From d804d2e126d0be33e4d6c77f7380a10aefb77443 Mon Sep 17 00:00:00 2001 From: 2ro <17595647+2ro@users.noreply.github.com> Date: Mon, 6 Jul 2026 07:49:25 -0400 Subject: [PATCH] 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 | 114 ++++++++++++++++++++++++++++++++++---- src/wallet/wallet.rs | 4 ++ 2 files changed, 108 insertions(+), 10 deletions(-) diff --git a/src/wallet/proof_addrs.rs b/src/wallet/proof_addrs.rs index 05df08c8..2f7791ef 100644 --- a/src/wallet/proof_addrs.rs +++ b/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 { - 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(®).map_err(|e| e.to_string())?; - std::fs::write(path, raw).map_err(|e| e.to_string())?; + persist_registry(path, ®)?; 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 { + 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 = 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); } } diff --git a/src/wallet/wallet.rs b/src/wallet/wallet.rs index bbd31ca9..386b8e60 100644 --- a/src/wallet/wallet.rs +++ b/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();