diff --git a/core/archipelago/src/api/rpc/wallet.rs b/core/archipelago/src/api/rpc/wallet.rs index a4ca5600..02a938ea 100644 --- a/core/archipelago/src/api/rpc/wallet.rs +++ b/core/archipelago/src/api/rpc/wallet.rs @@ -442,6 +442,7 @@ impl RpcHandler { "claimed_count": outcome.claimed_count, "received_sats": outcome.received_sats, "failed_count": outcome.failed_count, + "dropped_count": outcome.dropped_count, })) } diff --git a/core/archipelago/src/wallet/minibits.rs b/core/archipelago/src/wallet/minibits.rs index 7471e10f..8f6f2030 100644 --- a/core/archipelago/src/wallet/minibits.rs +++ b/core/archipelago/src/wallet/minibits.rs @@ -138,14 +138,14 @@ pub struct MinibitsState { pub server_nostr_pubkey: String, #[serde(default)] pub created_at: String, - /// Raw NIP-04-encrypted claim tokens fetched from `/claim` but not yet - /// successfully redeemed. A claim is consumed server-side the instant - /// `/claim` returns it, so it is stashed here *before* decrypt/redeem is + /// Raw NIP-04-encrypted claim tokens fetched from `/claim` (or a relay DM) + /// but not yet successfully redeemed. A claim is consumed server-side the + /// instant it's fetched, so it is stashed here *before* decrypt/redeem is /// attempted — a local failure (mint briefly down, bad cached server key, /// process crash mid-loop) then retries next poll instead of losing the /// coins outright. #[serde(default)] - pub pending_claims: Vec, + pub pending_claims: Vec, /// Unix timestamp of the newest Nostr DM we've already pulled into /// `pending_claims` (see `fetch_relay_dms`). Nostr events never expire /// from relays, so without this watermark every poll would re-fetch and @@ -155,6 +155,64 @@ pub struct MinibitsState { pub last_dm_seen_at: u64, } +/// A claim token queued for redeem, plus how many times redeem has already +/// been tried. Deserializes from either shape: the pre-existing plain string +/// (a node's `minibits.json` written before this field existed) becomes +/// `attempts: 0`, so upgrading never drops or resets an operator's queued +/// claims (see the module's data-preservation invariant). +#[derive(Debug, Clone, Serialize)] +pub struct PendingClaim { + pub token: String, + #[serde(default)] + pub attempts: u32, +} + +impl<'de> Deserialize<'de> for PendingClaim { + fn deserialize(deserializer: D) -> std::result::Result + where + D: serde::Deserializer<'de>, + { + #[derive(Deserialize)] + #[serde(untagged)] + enum Repr { + Legacy(String), + Full { + token: String, + #[serde(default)] + attempts: u32, + }, + } + Ok(match Repr::deserialize(deserializer)? { + Repr::Legacy(token) => PendingClaim { token, attempts: 0 }, + Repr::Full { token, attempts } => PendingClaim { token, attempts }, + }) + } +} + +/// A claim is dropped (not retried again) after this many failed decrypt/redeem +/// attempts — a backstop against a token that fails for a permanent reason +/// `is_already_redeemed` doesn't catch (e.g. a corrupt payload), which would +/// otherwise retry forever and keep `failed_count` stuck non-zero. +const MAX_CLAIM_ATTEMPTS: u32 = 20; + +/// True when `ecash::receive_token` failed because the token was already +/// redeemed (mint error 11001, see `mint_client::describe_mint_error_code`) — +/// a terminal condition, not a reason to retry. Seen after a state-file +/// watermark rewind replays an already-swept relay DM, or (pre-`STATE_LOCK`) +/// after a race let two polls redeem the same claim. +fn is_already_redeemed(err: &anyhow::Error) -> bool { + err.to_string().contains(super::mint_client::ALREADY_REDEEMED_MSG) +} + +/// Serializes every in-flight Minibits state read-modify-write +/// (`lnaddress`'s registration/token-refresh and `claim_and_redeem`'s fetch/ +/// redeem cycle) so two callers can never race on `minibits.json`. Without +/// this, two overlapping claim polls can each load the same +/// `last_dm_seen_at`, fetch the same DMs, and last-writer-wins on save — +/// rewinding the watermark and/or duplicating a pending token into a double +/// redeem. +static STATE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + /// A fresh Nostr keypair + seedHash derived from the node's ecash phrase. struct MinibitsIdentity { keys: nostr_sdk::Keys, @@ -184,19 +242,28 @@ struct ProfileRecord { pubkey: String, } +/// Parse a Minibits `{"error": {"name": ..., "message": ...}}` response body, +/// when present. +fn parse_minibits_error(body: &str) -> Option<(String, String)> { + let v: serde_json::Value = serde_json::from_str(body).ok()?; + let err = v.get("error")?; + let name = err.get("name").and_then(|n| n.as_str()).unwrap_or("ERROR"); + let msg = err.get("message").and_then(|m| m.as_str()).unwrap_or(""); + Some((name.to_string(), msg.to_string())) +} + /// Turn a non-2xx Minibits response into a readable error, surfacing the /// server's `error.name`/`error.message` when present. fn minibits_error(status: reqwest::StatusCode, body: &str) -> anyhow::Error { - if let Ok(v) = serde_json::from_str::(body) { - if let Some(err) = v.get("error") { - let name = err.get("name").and_then(|n| n.as_str()).unwrap_or("ERROR"); - let msg = err.get("message").and_then(|m| m.as_str()).unwrap_or(""); - return anyhow!("Minibits API error {status}: {name} {msg}"); - } + if let Some((name, msg)) = parse_minibits_error(body) { + return anyhow!("Minibits API error {status}: {name} {msg}"); } + // Truncate on a char boundary, not a byte index — `body` is + // server-controlled and a multi-byte UTF-8 character straddling byte 180 + // would otherwise panic inside this RPC handler's own error path. anyhow!( "Minibits API error {status}: {}", - &body[..body.len().min(180)] + body.chars().take(180).collect::() ) } @@ -234,6 +301,15 @@ async fn load_state(data_dir: &Path) -> Result> { /// Write the state file 0600 — it holds a bearer JWT. Same sensitivity class as /// the ecash files it sits beside, so it gets the same owner-only mode. +/// +/// Writes via a temp file + rename rather than truncating `minibits.json` in +/// place: a disk-full write hitting the truncate-then-write path destroyed +/// this exact file on archy-x250-pa3, 2026-09-08. `pending_claims` entries +/// sourced from `/claim` are unrecoverable once consumed server-side (unlike +/// relay DMs, which stay on the relay), so losing that field to a partial +/// write is not something the existing corrupt-file self-heal can undo — this +/// makes the write itself atomic instead. `STATE_LOCK` (held by every caller) +/// makes a fixed temp filename safe: only one writer runs at a time. async fn save_state(data_dir: &Path, state: &MinibitsState) -> Result<()> { let path = state_path(data_dir); if let Some(parent) = path.parent() { @@ -243,17 +319,22 @@ async fn save_state(data_dir: &Path, state: &MinibitsState) -> Result<()> { } let content = serde_json::to_string_pretty(state) .context("Failed to serialize the Minibits profile")?; - fs::write(&path, content) + let tmp_path = path.with_extension("json.tmp"); + fs::write(&tmp_path, content) .await - .with_context(|| format!("Failed to write {}", path.display()))?; + .with_context(|| format!("Failed to write {}", tmp_path.display()))?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + fs::set_permissions(&tmp_path, std::fs::Permissions::from_mode(0o600)) .await - .with_context(|| format!("Failed to chmod 0600 {}", path.display()))?; + .with_context(|| format!("Failed to chmod 0600 {}", tmp_path.display()))?; } + + fs::rename(&tmp_path, &path) + .await + .with_context(|| format!("Failed to move {} into place", tmp_path.display()))?; Ok(()) } @@ -395,8 +476,14 @@ async fn register_profile( .context("Minibits profile response was not the expected shape")?; return Ok(rec); } - // Name collision → draw another. Anything else is fatal. - let is_taken = body.contains("ALREADY_EXISTS") || body.contains("already"); + // Name collision → draw another. Anything else is fatal. Match on the + // structured error name, not a raw substring: an unrelated failure + // whose free-text message happens to contain the word "already" + // (e.g. a rate-limit or session message) must not burn one of the + // 6 retry attempts here. + let is_taken = parse_minibits_error(&body) + .map(|(name, _)| name.eq_ignore_ascii_case("ALREADY_EXISTS")) + .unwrap_or(false); if is_taken { warn!("Minibits name '{wallet_id}' taken, retrying (attempt {attempt})"); last_err = Some(minibits_error(status, &body)); @@ -463,6 +550,11 @@ pub async fn lnaddress(data_dir: &Path) -> Result { let (phrase, seed) = ecash_phrase(data_dir).await?; let identity = derive_identity(&phrase, &seed)?; + // Hold the same lock a claim poll uses: both read-modify-write + // `minibits.json`, and this call registers the profile the first time — + // it must not race a poll that's mid-save. + let _guard = STATE_LOCK.lock().await; + let mut state = match load_state(data_dir).await? { Some(st) => st, None => { @@ -511,26 +603,20 @@ pub struct ClaimOutcome { /// dropped. Non-zero here means real, unswept value the operator should /// know about. pub failed_count: usize, + /// Claims permanently given up on this poll: either recognized as + /// already redeemed elsewhere (harmless — the value was already swept), + /// or a decrypt/redeem failure that hit `MAX_CLAIM_ATTEMPTS`. The latter + /// case means real value that was lost; the former does not. + pub dropped_count: usize, } const NO_CLAIMS: ClaimOutcome = ClaimOutcome { claimed_count: 0, received_sats: 0, failed_count: 0, + dropped_count: 0, }; -/// Make sure the Minibits mint is on the accepted-mints allow-list. -/// -/// `ecash::receive_token` checks the raw accepted-mints file directly (not -/// the more lenient `ecash::is_mint_trusted`, which always trusts the default -/// mint) — so an operator who edited their accepted-mints list (e.g. via the -/// `streaming.configure-mints` RPC) and dropped the default mint would -/// otherwise cause every Minibits claim to fail *after* the claim was already -/// consumed server-side, permanently losing those coins with nothing but a -/// log line to show for it. The Minibits Lightning address is inherently -/// backed by this one mint — registering it already implies trusting the -/// mint — so self-heal the allow-list here rather than let that combination -/// silently strand funds. /// Fetch NIP-04 DM (kind 4) events addressed to `our_pubkey` newer than /// `since`, from `CLAIM_RELAY_URLS`. Returns each event's raw (still /// encrypted) content plus its `created_at`, newest last. This — not @@ -613,6 +699,19 @@ pub async fn claim_and_redeem(data_dir: &Path) -> Result { if network == EcashNetwork::Testnet { return Ok(NO_CLAIMS); } + + // The UI polls this every 8s, and a single poll (auth + `/claim` + relay + // fetch + redeem loop) can run well past that — so a second poll starting + // before the first finishes is expected, not exceptional. Skip it rather + // than queue: both copies would otherwise load the same + // `last_dm_seen_at`, fetch/redeem the same claims, and last-writer-wins on + // save (rewinding the watermark and/or double-redeeming). Nothing is lost + // by skipping — relay DMs persist and get picked up next tick. + let Ok(_guard) = STATE_LOCK.try_lock() else { + debug!("Minibits: a claim poll is already in flight, skipping this tick"); + return Ok(NO_CLAIMS); + }; + ensure_mint_accepted(data_dir, &network.default_mint()).await?; let client = reqwest::Client::builder() @@ -663,7 +762,10 @@ pub async fn claim_and_redeem(data_dir: &Path) -> Result { Ok(claims) => { for claim in &claims { match claim.get("token").and_then(|t| t.as_str()) { - Some(t) => state.pending_claims.push(t.to_string()), + Some(t) => state.pending_claims.push(PendingClaim { + token: t.to_string(), + attempts: 0, + }), None => warn!("Minibits claim had no 'token' field; skipping"), } } @@ -687,7 +789,10 @@ pub async fn claim_and_redeem(data_dir: &Path) -> Result { warn!("Minibits: ignoring claim DM from unexpected pubkey {author}"); continue; } - state.pending_claims.push(content); + state.pending_claims.push(PendingClaim { + token: content, + attempts: 0, + }); state.last_dm_seen_at = state.last_dm_seen_at.max(created_at); } @@ -704,13 +809,23 @@ pub async fn claim_and_redeem(data_dir: &Path) -> Result { let to_process = std::mem::take(&mut state.pending_claims); let mut redeemed = 0usize; let mut sats = 0u64; + let mut dropped = 0usize; let mut still_pending = Vec::new(); - for enc in &to_process { - let decoded = match nip04::decrypt(identity.keys.secret_key(), &server_pk, enc) { + for mut claim in to_process { + let decoded = match nip04::decrypt(identity.keys.secret_key(), &server_pk, &claim.token) { Ok(d) => d, Err(e) => { - warn!("Minibits claim could not be decrypted ({e}); will retry next poll"); - still_pending.push(enc.clone()); + claim.attempts += 1; + if claim.attempts >= MAX_CLAIM_ATTEMPTS { + warn!( + "Minibits: dropping a claim after {} failed decrypt attempts ({e})", + claim.attempts + ); + dropped += 1; + } else { + warn!("Minibits claim could not be decrypted ({e}); will retry next poll"); + still_pending.push(claim); + } continue; } }; @@ -720,9 +835,26 @@ pub async fn claim_and_redeem(data_dir: &Path) -> Result { sats += got; info!("Minibits: redeemed a claimed payment ({got} sats)"); } + // Terminal: the token was already swept (a watermark-rewind + // replay of an old DM, or — pre-`STATE_LOCK` — a race's double + // redeem). Retrying changes nothing, so drop it instead of + // leaving `failed_count` stuck non-zero forever. + Err(e) if is_already_redeemed(&e) => { + info!("Minibits: a claimed token was already redeemed elsewhere; dropping it ({e})"); + dropped += 1; + } Err(e) => { - warn!("Minibits claim decrypted but failed to redeem ({e}); will retry next poll"); - still_pending.push(enc.clone()); + claim.attempts += 1; + if claim.attempts >= MAX_CLAIM_ATTEMPTS { + warn!( + "Minibits: dropping a claim after {} failed redeem attempts ({e})", + claim.attempts + ); + dropped += 1; + } else { + warn!("Minibits claim decrypted but failed to redeem ({e}); will retry next poll"); + still_pending.push(claim); + } } } } @@ -731,13 +863,61 @@ pub async fn claim_and_redeem(data_dir: &Path) -> Result { state.pending_claims = still_pending; save_state(data_dir, &state).await?; - Ok(ClaimOutcome { claimed_count: redeemed, received_sats: sats, failed_count }) + Ok(ClaimOutcome { + claimed_count: redeemed, + received_sats: sats, + failed_count, + dropped_count: dropped, + }) } #[cfg(test)] mod tests { use super::*; + #[tokio::test] + async fn state_lock_rejects_a_second_concurrent_holder() { + // Regression guard for the overlapping-claim race: `claim_and_redeem` + // must skip rather than run when another call already holds + // `STATE_LOCK`, or two polls can race on `minibits.json`. + let _guard = STATE_LOCK.lock().await; + assert!(STATE_LOCK.try_lock().is_err()); + } + + #[test] + fn pending_claim_migrates_from_the_legacy_plain_string_shape() { + // A node's existing `minibits.json` (written before `attempts` + // existed) has `pending_claims` as a plain string array — it must + // load as `attempts: 0`, not fail or drop the queued claim. + let legacy: PendingClaim = serde_json::from_str("\"encrypted-token\"").unwrap(); + assert_eq!(legacy.token, "encrypted-token"); + assert_eq!(legacy.attempts, 0); + + let current: PendingClaim = + serde_json::from_str(r#"{"token":"encrypted-token","attempts":3}"#).unwrap(); + assert_eq!(current.token, "encrypted-token"); + assert_eq!(current.attempts, 3); + } + + #[test] + fn minibits_error_does_not_panic_on_a_multibyte_boundary() { + // Reproduces the byte-slice panic: a multi-byte UTF-8 character + // straddling byte 180 of a server-controlled error body must not + // crash the RPC handler's own error path. + let body = format!("{}{}", "x".repeat(179), "€".repeat(10)); + let err = minibits_error(reqwest::StatusCode::BAD_REQUEST, &body); + assert!(err.to_string().contains("Minibits API error")); + } + + #[test] + fn is_already_redeemed_matches_only_the_specific_mint_error() { + let redeemed = anyhow!("Could not receive this ecash: {}", super::super::mint_client::ALREADY_REDEEMED_MSG); + assert!(is_already_redeemed(&redeemed)); + + let unrelated = anyhow!("Could not receive this ecash: mint unreachable"); + assert!(!is_already_redeemed(&unrelated)); + } + #[test] fn derived_nostr_key_matches_the_nip06_vector() { // The Minibits app derives its Nostr key at m/44'/1237'/0'/0/0 with an diff --git a/core/archipelago/src/wallet/mint_client.rs b/core/archipelago/src/wallet/mint_client.rs index 2da75edd..a591874a 100644 --- a/core/archipelago/src/wallet/mint_client.rs +++ b/core/archipelago/src/wallet/mint_client.rs @@ -71,10 +71,16 @@ pub struct MintResult { /// keyset codes shared by NUT-02/03/04/05 — the codes a swap/melt/mint call /// can actually hit. Returns `None` for anything else (e.g. Lightning/quote /// codes in the 20000s) so the caller falls back to the mint's own `detail`. +/// Text of the NUT error-code-11001 translation, exposed so callers that +/// received an `anyhow::Error` from a receive/redeem path (e.g. Minibits +/// claim replay) can recognize an already-spent token as terminal rather than +/// retrying it forever. +pub const ALREADY_REDEEMED_MSG: &str = "This ecash has already been redeemed — it can't be claimed twice."; + fn describe_mint_error_code(code: i64) -> Option<&'static str> { Some(match code { 10001 => "The mint rejected these coins as invalid.", - 11001 => "This ecash has already been redeemed — it can't be claimed twice.", + 11001 => ALREADY_REDEEMED_MSG, 11002 => "This ecash is already being redeemed elsewhere — try again in a moment.", 11003 => "The mint already issued new coins for this exact request — there's nothing left to redeem.", 11004 => "This request is still being processed by the mint — try again in a moment.", diff --git a/neode-ui/src/components/ReceiveBitcoinModal.vue b/neode-ui/src/components/ReceiveBitcoinModal.vue index 550c27e2..e64fd42f 100644 --- a/neode-ui/src/components/ReceiveBitcoinModal.vue +++ b/neode-ui/src/components/ReceiveBitcoinModal.vue @@ -242,6 +242,10 @@ const lnClaimedSats = ref(0) // operator should see it rather than have it be a silent, unbounded wait. const lnPendingClaims = ref(0) let lnClaimTimer: ReturnType | null = null +// A poll can outlast the 8s interval (backend auth + relay fetch + redeem +// loop) — without this, the next tick fires on top of it and both calls hit +// the backend's `minibits.json` at once. +let lnPollInFlight = false async function loadLnAddress() { if (lnAddress.value || lnAddressLoading.value) return @@ -281,6 +285,8 @@ async function pollLnClaims() { stopLnClaimPoll() return } + if (lnPollInFlight) return + lnPollInFlight = true try { const res = await rpcClient.call<{ received_sats?: number; failed_count?: number }>({ method: 'wallet.ecash-lnaddress-claim', @@ -292,6 +298,8 @@ async function pollLnClaims() { lnPendingClaims.value = res?.failed_count || 0 } catch { // Transient poll failure (offline, mint busy) — keep polling. + } finally { + lnPollInFlight = false } } diff --git a/neode-ui/src/components/__tests__/ReceiveBitcoinModal.test.ts b/neode-ui/src/components/__tests__/ReceiveBitcoinModal.test.ts index 3a9b4986..5443a8ce 100644 --- a/neode-ui/src/components/__tests__/ReceiveBitcoinModal.test.ts +++ b/neode-ui/src/components/__tests__/ReceiveBitcoinModal.test.ts @@ -71,3 +71,59 @@ describe('ReceiveBitcoinModal — ecash tab click', () => { wrapper.unmount() }) }) + +// Regression guard for the overlapping-claim race: a single +// wallet.ecash-lnaddress-claim call can outlast the 8s poll interval (backend +// auth + relay fetch + redeem loop), and a second call firing on top of it +// raced on the backend's minibits.json (see minibits.rs STATE_LOCK). +describe('ReceiveBitcoinModal — ecash claim poll', () => { + it('does not start a second claim poll while one is still in flight', async () => { + vi.useFakeTimers() + let resolveClaim: (v: unknown) => void = () => {} + vi.mocked(rpcClient.call).mockImplementation((args: unknown) => { + const method = (args as { method?: string })?.method + if (method === 'wallet.ecash-lnaddress') { + return Promise.resolve({ address: 'someone@minibits.cash' } as never) + } + if (method === 'wallet.ecash-lnaddress-claim') { + return new Promise((resolve) => { + resolveClaim = resolve + }) as never + } + return Promise.resolve({} as never) + }) + + const wrapper = mount(ReceiveBitcoinModal, { + props: { show: true }, + attachTo: document.body, + }) + await flushPromises() + + const tabs = Array.from(document.body.querySelectorAll('button')) + const ecashTab = tabs.find((b) => b.textContent?.toLowerCase().includes('ecash')) + ecashTab!.dispatchEvent(new Event('click', { bubbles: true })) + await flushPromises() + + const claimCalls = () => + vi + .mocked(rpcClient.call) + .mock.calls.filter(([a]) => (a as { method?: string })?.method === 'wallet.ecash-lnaddress-claim').length + + await vi.advanceTimersByTimeAsync(8000) + expect(claimCalls()).toBe(1) + + // Second tick fires while the first claim call is still unresolved. + await vi.advanceTimersByTimeAsync(8000) + expect(claimCalls()).toBe(1) + + resolveClaim({ received_sats: 0, failed_count: 0 }) + await flushPromises() + + // Once the in-flight call finishes, the next tick is free to poll again. + await vi.advanceTimersByTimeAsync(8000) + expect(claimCalls()).toBe(2) + + wrapper.unmount() + vi.useRealTimers() + }) +})