Repository navigation
Release a pool spend reservation once it provably never landed - #167
Conversation
The native wallet reserved a shield's coin before posting it and could never release it again. Cloudflare's rate limit answered the post with a non-JSON 429 the journal could not read, so the entry stayed; the native journal had no grace window, so a 404 never counted as proof the spend never arrived; and a shield never appears among an account's spent nullifiers, so the landed-batch rule could not judge it and waited for a batch an idle pool never makes. The coin stayed reserved and the "unresolved outcomes" warning never cleared. - A 429 without the pool's JSON envelope is a refusal in front of the pool (Cloudflare's edge limit, the OHTTP gateway's shed), so the entry is released; a 429 that carries another envelope stays uncertain. - The native journal gets a 180-second grace, sized to its longest in-flight post through Tor; the sealed browser path keeps 900. - A reservation with a coin is judged by the Ark indexer's view of that coin: still unspent past the pool's one-hour retention means the deposit cannot have landed, so it is released without waiting for a new batch. Swept, unrolled or otherwise spent coins stay reserved. - Native now reconciles on every wallet tick while anything is pending, through a reconcile round shared with the browser path. PoolSpendJournal.tla models client, edge and pool with a mutation config for each release guard.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 2ab1839.
The analysis below covers protocol correctness of the deposit-coin release path, the 429 edge-refusal semantic, the new grace wiring for native, and the TLA model. No protocol-critical issues found.
429 edge-refusal classification (src/native/mutations.ts, preAdmissionRefusal)
The rewritten predicate is correct. Three distinct outcomes map cleanly to three code paths:
| Body shape | typeof result?.error |
Action |
|---|---|---|
Non-JSON / no error field |
not 'string' |
edge refusal → release ✓ |
JSON with error string, mismatched message |
'string', condition false |
uncertain → keep ✓ |
Pool's exact {code,error} |
'string', condition true |
pool refusal → release ✓ |
The fix that makes this work: catch{result=undefined;} instead of catch{return;}. Previously a non-JSON 429 body exited before the predicate saw the response; now result is undefined, typeof undefined?.error !== 'string' is true, and the entry is released. The result?.code guard on line 516 (result?.code==='fee-missing') is a concurrent defensive fix — previously result.code would have thrown if result were null.
Operational assumption worth monitoring: the predicate treats every 429 without a JSON error string as written by a hop that had not forwarded the request. That holds for today's topology (Cloudflare edge + OHTTP admit(), both decide before forwarding), but a future proxy that answered 429 after forwarding without a JSON envelope would cause incorrect releases. The code comment documents this clearly. If the network path ever changes, this classification needs to be revisited.
The test at tests/tor-mutations.test.ts covers all three cases: the new test at the top of the 429 block validates edge-refusal for six non-envelope bodies; the existing 'generic 400, double-spend, unknown codes, mismatched status and 5xx remain uncertain' test covers [429,{code:'stale-anchor',…}] and [429,{code:'admission-rate-limit',error:'Proxy busy.'}] (mismatched message) staying uncertain. Coverage is complete.
Deposit coin release path (src/native/mutations.ts, settleLandedMutations; src/shielded/wallet.ts, settledDeposit)
Logic trace (the critical path):
landedDeposit returns string (txid, our batch spent the coin) | false (coin exists, unspent) | undefined (coin absent, swept, unrolled, or spent by a foreign party). settledDeposit maps this to true | false | undefined.
In settleLandedMutations, for a deposit entry (row.coin !== undefined):
deposited(row.coin) = true→verdict = true→return false(remove; landed) ✓deposited(row.coin) = undefined→verdict = undefined→return true(keep; uncertain) ✓deposited(row.coin) = false→verdict = false→ falls through:at - row.createdAt <= TRACKED_MS + graceMs→return true(keep; inside window) ✓- past window →
return row.coin === undefined && …→false(remove; coin unspent past retention, definitely never landed) ✓
The row.coin === undefined guard on the final return is specifically correct: it prevents note spends (no coin) from being released before the batch-count rule can speak, while allowing coin-keyed deposits to exit on the coin proof alone.
settledDeposit input validation, src/shielded/wallet.ts: The vout regex /^\d{1,10}$/ matches up to ten decimal digits, which includes values above 0xffffffff (4 294 967 295, the Bitcoin max vout). Values above that bound would reach landedDeposit with an out-of-range vout, which the indexer would return as an empty result (→ undefined). All outpoints stored in the journal are written by parseBody, which enforces vout <= 0xffffffff, so the over-range case is unreachable from live storage. Worth tightening to /^\d{1,9}$/ (nine digits covers the full range 0–999 999 999 < 4 294 967 296) or adding an explicit Number(parts[1]) <= 0xffffffff guard to match the parse-time invariant.
Native grace wiring (src/native/mutations.ts, NATIVE_GRACE_MS)
NATIVE_GRACE_MS = 180_000 ms (3 minutes). The 404-based release window in observeNativeResponse is createdAt + graceMs < now < createdAt + TRACKED_MS − graceMs, i.e., 3 minutes to 57 minutes after creation. For a POST that lives at most 120 s (90 s connect + 30 s idle per session.rs), 180 s provides margin. The settle release requires at − row.createdAt > TRACKED_MS + graceMs = 3 780 000 ms ≈ 63 minutes, consistent with the pool's one-hour tracking window plus grace.
Previously current() built the journal without graceMs, so settleLandedMutations returned immediately on the native path (if(graceMs===undefined)return). The fix supplies {graceMs:NATIVE_GRACE_MS}.
reconcileJournal and tick wiring (src/native/mutations.ts; app/src/RollupWallet.tsx)
reconcileJournal early-exits on !pending() before touching the network or the wallet — no I/O when nothing is pending. When pending deposits exist, it calls Promise.all(pendingPoolOutpoints.map(settledDeposit)). An indexer error for any coin rejects the whole Promise, propagates as a journalError, and keeps all reservations. This is the safe failure mode.
The tick useEffect in RollupWallet.tsx previously ran a synchronous localStorage read on the native path and routed errors to setPoolError (which could displace startup errors). The new async path routes journal errors to setJournalError correctly, matching the behaviour the existing tests require. sessionFetch added to the dependency array is stable (useState initial value) and correct.
The PrivateTransport.reconcile deduplication (reconciling ??= …) still functions: reconcileJournal is called through it on the browser private path, so concurrent ticks coalesce into one reconcile.
TLA model (spec/tla/PoolSpendJournal.tla)
The model is well-structured. CanLand, SettleProven, and RefusalProven correspond directly to the code guards. All seven mutation configurations break a distinct invariant as expected:
_Graceand_TrackedWindow→NoDoublePayment(a 404 at the wrong time releases a live entry)_ForeignRefusal→NoDoublePayment(a forwarded 429 releases)_EdgeRefusal→NoStrandedReservation(without edge-refusal handling, reservations strand)_Retention→NoDoublePayment(releasing before the pool forgets the spend)_CoinProof→NoStrandedReservation(without coin-based proof, deposits wait forever)_CoinSpentCheck→NoDoublePayment(nullifier history is blind to deposits)
The BreakCoinSpentCheck configuration directly validates why the original nullifier-based path was wrong for shields: a deposit of ours never produces a history row, so settled(nullifiers) always returns false, meaning any deposit with no admission would be incorrectly released once the batch count advances.
The pre-existing lagging-replica hazard for note spends (a frozen replica serving a stale count while another operator batches) is correctly identified, documented as a follow-up, and not touched by this change. The deposit path does not depend on the batch-count rule.
Cross-repo impact
JournalWallet, reconcileJournal, and NATIVE_GRACE_MS are new exports from src/native/mutations.ts. The PrivateTransport.reconcile interface now requires settledDeposit on the wallet argument. No SDK repo (ts-sdk, go-sdk, rust-sdk, dotnet-sdk) consumes these paths — the journal is an internal client concern. No cross-repo breakage.
Summary
Logic is sound. The three-part cause of the stuck state is correctly addressed: the non-JSON 429 body is now classified, the native journal now carries a grace, and deposit coins are judged by the indexer independently of nullifier history. The TLA model confirms the safety invariants hold and each guard earns its place.
One minor item worth fixing before merge: settledDeposit's vout regex (src/shielded/wallet.ts) admits out-of-range values that are unreachable from live storage but could confuse a future reader — tightening it to /^\d{1,9}$/ or adding an explicit bound check would close the gap.
|
Thanks. On the vout bound: it is unreachable today, since every outpoint in the journal passed |
Reported from an Android phone: after a faucet send to the Ark address, the automatic shield failed with a 429 and the wallet has shown "Some payments have unresolved outcomes…" ever since, with the 50,000 sats stuck as unshielded. Eight hours later it had not cleared, and on the current build it never would.
Causes, all three needed for the stuck state
error code: 1015.observeNativeResponseparsed the body as JSON insidetry{…}catch{return;}, so a non-JSON refusal was never judged, andpreAdmissionRefusalaccepted only the pool's exact admission-limit message. A private POST is never retried (pool.ts), so the entry stayed.current()insrc/native/mutations.ts) was built withoutgraceMs, so a 404 for the spend id never counted, andsettleLandedMutationsreturned at once. The browser private path got both in feat: run the browser wallet through the Nostr carrier, behind ?private=1 #147 and feat: quieter, less linkable pool requests from the private wallet #156; native never did.Changes
NATIVE_GRACE_MS= 180 s, sized to the native POST's longest life (90 s connect, 30 s idle, no relay holding a sealed copy); the sealed browser path keeps 900 s.coinis judged byShieldedWallet.settledDeposit, which asks the Ark indexer about that coin through the existinglandedDeposit: still unspent past the pool's one-hour retention and the grace means the deposit cannot land, so it is released without waiting for a batch; our own batch spending it means it landed; swept, unrolled or spent elsewhere stays reserved.reconcileJournalis shared by the native and browser private paths, and native now reconciles on every wallet tick while anything is pending (about once a minute), not only at open.How the reported entry clears on this build: at open the id check still answers 404 with no effect (it is past the one-hour window, by design). The first tick after the wallet is ready asks the indexer for the reserved coin, finds it unspent, and releases the entry; the warning clears and the next auto-shield tick posts the shield again, within about a minute.
Model. New
spec/tla/PoolSpendJournal.tla(client, edge, pool; refused, lost and admitted posts; reconcile; settle) withNoDoublePaymentandNoStrandedReservation; base and simulation configs green, one mutation config per release guard. It also found a pre-existing hazard in the batch-count rule for note spends with a lagging replica, recorded in the README as a follow-up; the deposit path does not depend on it.Validation, local, rebased on main
9686db5:tor-mutations,tor-content,private-transport,app-private-build,shielded-walletandwallet-balance-noticepass 241 of 241;tsc --noEmitis clean;node tools/check-native-model.mjsmatches 112 configurations (104 plus 8). Each release guard was mutated and caught: exact-message-only 429s (entry never freed), every 429 released (a foreign 429 wrongly freed), no native grace, shields judged by nullifiers, shields still waiting for a batch, no retention bound, and a coin spent elsewhere read as never landed. Full suite before the rebase: 1885 tests, 1884 pass, the one knownLinux publication…failure under Git Bash tar.Only a device can confirm: that the real journal row on the phone loads under this build, that the Tor indexer read for that coin succeeds, and that the auto-shield re-posts. The claim most worth doubting: that every 429 without a JSON
errorwas answered before reaching the pool app; a future proxy that answered 429 after forwarding would break it, and the alternative strands funds instead.