Repository navigation
fix(ln-receive): key the claim/refund decision on the funded output, not an empty lockup - #341
Conversation
The lightning:BTC->arkade:BTC lockup address is public from the moment a client holds a quote, and anyone may pay it dust. Two paths keyed their decision on whether the WHOLE script was empty rather than on the recorded funded output, so a single stray dust output changed the outcome: - whenFunded searched for the client's claim preimage only when findLockups returned zero outputs. With dust present it waited to the refund deadline instead, even once the client's claim of the funded output was visible. - whenRefunding applied the EMPTY_LOCKUP_GRACE wait and the htlc-expiry (E) check only in the empty-script branch. With dust present it fell straight through to refund whatever findLockups returned and booked the row terminal refunded -- in the same tick as its single late-claim recheck, with no grace for the indexer's witness-fetch lag. A client who claimed the funded output just before the deadline then kept the payout while the held HTLC, now unreachable from terminal refunded, failed back to the payer: the solver lost payoutSats against a row that read as a clean refund. Both now gate on the recorded funded outpoint (arkade_lockup_txid/vout): whenFunded runs the preimage search whenever the funded output is no longer spendable regardless of residual dust; whenRefunding refunds only while the funded output is still present, and otherwise takes the same grace + E + needs-review path the empty branch already had, so the late recheck keeps running across ticks until the claim's witness becomes readable. Tests cover both gaps and fail on the pre-fix code.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #341
Verdict: no blocking issues. The fix is correct and the tests are well-targeted.
What the fix does
The old gate in both whenFunded and whenRefunding keyed on whether any output existed on the lockup script (outputs.length > 0 / outputs.length === 0). Because the lockup address is a public P2TR script hash, stray dust from any sender could make outputs.length > 0 permanently true, causing two distinct failures:
whenFunded(orchestrator.ts:1086): dust delayed claim detection to the refund deadline instead of running the preimage search as soon as the recorded output was gone.whenRefunding(orchestrator.ts:1292): dust routed the row into the live-output branch (arkade.refund(…, outputs)) when the recorded output had already been claimed, booking terminalrefundedin the same tick as the single late-recheck — leaving no grace for the indexer's witness-fetch lag.
Both gates are now keyed on fundedSpendable / fundedOutput (presence of the specific arkadeLockupTxid:arkadeLockupVout in findLockups's result). That is exactly the right discriminant.
Code observations
orchestrator.ts:1076-1079 — null guard moved to top of whenFunded
This is strictly better. In the old code the guard only triggered after outputs.length === 0; a corrupt funded row with no outpoint but with dust on the script would wait silently until the refund deadline. The new position fails fast regardless of script state. whenRefunding already had an equivalent guard at its own top (pre-existing, not part of this diff) so the fundedOutput lookup there is safe against null fields.
orchestrator.ts:1292 — fundedOutput replaces outputs.length === 0
The !fundedOutput branch correctly inherits the full EMPTY_LOCKUP_GRACE + htlcExpiresAt gate that the old empty-script branch already had. The arkade.refund(…, outputs) call in the live branch still passes the full outputs slice (not just fundedOutput), consistent with the existing convention of refunding whatever spendable outputs are present — no change there.
orchestrator.ts:1308-1312 — error message wording
'funded output gone during refunding with no matching claim found; needs review' is more precise than the old 'lockup empty during…'. Minor win for operator triage.
Test observations
settles a claim even when a stray dust output still sits on the lockup script (orchestrator.test.ts:1788)
Covers failure mode 1 cleanly. spentOutputs has the recorded output gone, claimOn+claimPreimage are set, and outputs has only dust. On the pre-fix code this test would have waited at fundedSpendable = true (old: outputs.length > 0) and returned false — now it correctly finds and settles the claim.
escalates rather than booking refunded when dust hides a funded output a claim already took (orchestrator.test.ts:1803)
Covers failure mode 2. Worth spelling out the tick trace so the invariants are clear for future readers:
- Tick 1 (
now = funded.refundLocktime):whenFundedseesfundedSpendable = false+ no preimage + deadline reached → transitions torefunding. Chains intowhenRefunding:fundedOutputis not found (only dust inoutputs),finalizePendingRefundreturns nothing, andthis.now() - row.updatedAt = 0 < EMPTY_LOCKUP_GRACE (120)→ returns false. Final state:refunding,refundCalls = 0. ✓ - Tick 2 (
claimOn+claimPreimageset):whenRefunding's late-claim recheck at the top finds the preimage →claimed→settled. ✓
The test does not exercise the full EMPTY_LOCKUP_GRACE exhaustion → htlcExpiresAt → store.fail path (the path where dust sits on the script AND the claim never becomes readable), but that path is covered by earlier tests in the suite (see the 'waits (does not escalate) when…' and surrounding cases).
No cross-repo impact
findLockups, arkadeLockupTxid/arkadeLockupVout, and both changed private methods are internal to solver-corridors. No public API, type, proto, or interface change is visible in the diff. SDK repos (ts-sdk, go-sdk, rust-sdk, dotnet-sdk) have no consumers of these symbols.
No issues requiring changes.
What
Two decisions on the
lightning:BTC->arkade:BTCreceive leg keyed on whether the whole lockupscript was empty, rather than on the recorded funded output. The lockup address is public and can
receive stray dust, so a residual dust output could change the outcome:
whenFundedsearched for the client's claim preimage only whenfindLockupsreturned zerooutputs. With an unrelated dust output still on the script, it waited until the refund deadline
instead of noticing that the funded output had already been claimed.
whenRefundingapplied theEMPTY_LOCKUP_GRACEwait and the HTLC-expiry (E) check only inthe empty-script branch. With dust present it went straight to refunding whatever
findLockupsreturned and booked the row terminal
refunded, in the same tick as its single late-claimrecheck — leaving no grace for the indexer's witness-fetch lag.
Fix
Both paths now key on the recorded funded outpoint (
arkade_lockup_txid/arkade_lockup_vout):whenFundedruns the preimage search whenever the funded output is no longer spendable,regardless of any residual dust.
whenRefundingrefunds only while the funded output is still present; otherwise it takes thesame grace +
E+ needs-review path the empty branch already had, so the late recheck keepsrunning across ticks until the claim's witness becomes readable.
The null-guard for a missing recorded outpoint moves up to the top of
whenFunded, since the newgate reads those fields.
Tests
Two regression tests in
test/receive/orchestrator.test.ts, both failing on the pre-fix code:settledonce the witness becomes readable) instead of booking terminalrefunded.Gates:
pnpm typecheck,pnpm test(6485),pnpm format:check,pnpm build— all green.