Repository navigation
Swaps: verify reverse swap lockup before claiming - #4894
Merged
kaloudis merged 12 commits intoOct 9, 2026
Merged
Conversation
![athena-zeusln[bot]](https://avatars.githubusercontent.com/in/4862658?s=60&v=4){kind=small}
There was a problem hiding this comment.
Athena Review
Pinned commit: 0fa0fdf
anthropic:claude-opus-5-5: completed openai:gpt-6-sol: completed
0 validated findings. This is a nonblocking COMMENT review, not an approval. No findings does not establish correctness.
Limitations:
- stores/SwapStore.ts: patch omitted by context bound
- utils/SwapUtils.test.ts: patch omitted by context bound
- utils/SwapUtils.ts: patch omitted by context bound
- views/Swaps/SwapDetails.tsx: patch omitted by context bound
- The patches for stores/SwapStore.ts, utils/SwapUtils.ts, utils/SwapUtils.test.ts and views/Swaps/SwapDetails.tsx were omitted. The core logic was not reviewable: verifyReverseLockup, createReverseSwap response verification, swap tree and lockup address derivation, amount comparison, and claim gating and retry in SwapDetails.
- Without SwapStore.ts, it cannot be confirmed whether the lockup lookup matches the txid of the provider-supplied hex. A provider could otherwise point at an unrelated confirmed transaction paying the same script.
- Without SwapStore.ts, it cannot be confirmed whether the 'unconfirmed'/'lockup-not-found' result for a fabricated lockup retries indefinitely, or whether it stops at the swap timeout.
- Not reviewable: whether the expected on-chain amount in createReverseSwap is derived from the reverseInfo fee fields exactly as the Swaps screen shows it, and whether a missing reverseInfo fails safely.
- Not reviewable: whether adding the UrlUtils import to SwapStore creates a runtime circular import with the Stores module graph. SwapStore.test.ts now mocks UrlUtils for this reason.
- The visible ClaimTransaction.ts change does not check that the decoded private key is 32 bytes. That behavior was already present before this diff, and its downstream impact was not visible.
- The patches for stores/SwapStore.ts, utils/SwapUtils.ts, and views/Swaps/SwapDetails.tsx were omitted, so the reverse-swap verification and claim flow could not be reviewed.
kaloudis
force-pushed
the
fix/reverse-swap-lockup-verification
branch
from
October 8, 2026 13:55
0fa0fdf to
114ebd0
Compare
Contributor
Author
|
Device test on the iOS 26.5 simulator (iPhone 17e), JS from this branch over Metro. The swap host was a local fake provider, and the user's mempool instance was set to a local fake Esplora. I opened SwapDetails with a reverse swap whose tree, keys and lockup address were built with this PR's helpers. Bug found and fixed in 49baf87. In the app, the buffer polyfill's Results after the fix:
Not covered:
While testing I also found that native claims request |
This was referenced Oct 8, 2026
A reverse swap claim sends the preimage to the provider's cooperative claim endpoint before the claim is broadcast, so the claim must not start until the lockup is known to exist and pay the swap. The claim was triggered by the provider's websocket update and built from the transaction hex in that update, with no independent check. At creation, after the invoice check, verify the rest of the provider's response against what we chose: the claim leaf must pay our key against our preimage hash, the refund leaf must pay the server key at the returned timeout, the lockup address must be the taproot output of exactly that tree (MuSig2 KeyAgg of [server, ours] tweaked by the tree, matching boltz-client), and onchainAmount must be above the amount the Swaps screen showed. A mismatch aborts before saving or paying. Before claiming, SwapStore.verifyReverseLockup re-checks the tree and address of the stored swap (covering swaps saved or rescued earlier), looks the lockup up by txid on the user's mempool instance, and only allows the claim once it is confirmed and pays the swap's output at least onchainAmount. Not yet seen, unconfirmed, or an unreachable mempool instance retry every 60s; a lockup that pays the wrong output or too little is refused. Tests pin KeyAgg to the BIP 327 vectors and the leaf scripts and lockup address to boltz-client v2.9.0's reverse swap vector.
boltz-client's FindVout claims the first output whose script after the 2-byte prefix equals the witness program, ignoring the witness version. checkLockupOutput checked the largest exact-match output instead, so a lockup with a small or v0 look-alike output ahead of a funded one passed while the claim built on the wrong output. Check the first match, and require it to be our exact v1 script holding the swap amount. verifyReverseLockup also let ECPair.fromPrivateKey throw on an invalid key. Nothing up the WebSocket handler catches it, so the swap screen stayed loading with no error. Return invalid/invalid-key instead.
In the app, the buffer polyfill's subarray returns a plain Uint8Array. bitcoinjs script.compile only treats Buffers as data, so the x-only keys compiled as OP_0, the rebuilt claim and refund leaves never matched the provider's, and every reverse swap failed the tree check at creation and before claiming. Node's subarray returns a Buffer, so Jest never saw it. Copy the slice into a Buffer, and cover the Uint8Array case.
Read the refund height from the committed script, including rescued swaps, and require six blocks of headroom against a fresh explorer tip. Check the exact native claim outpoint is unspent before every attempt. Keep unavailable observations retryable and recheck after fee/address resolution and native failures. Cover the deadline, outspend, script parsing, and view retry paths with regression tests.
getRescuableSwaps copied every field of the /swap/restore response into the stored swap, and the claim path paid out to a stored destinationAddress unchecked. A rescue host could set it, receive the lockup, and settle the user's hold invoice with the revealed preimage. Build rescued swaps from an allow-list of host fields (id, status, createdAt, and tree, keyIndex, lockupAddress, serverPublicKey, timeoutBlockHeight, amount, preimageHash from the details). Skip a reverse swap whose preimageHash does not match the preimage derived at its keyIndex. Swaps already rescued on a build with the preimage re-derivation may hold a host address. Mark addresses the wallet generates for a rescued swap with claimAddressFromWallet, and have SwapDetails claim through Swap.claimDestinationAddress, which ignores an unmarked address on an imported swap so a fresh wallet address is generated instead. Closes ZeusLN#4943
getRescuableSwaps skipped swaps it already had by checking IDs against SWAPS_KEY only, but the next fetchAndUpdateSwaps re-files rescued reverse swaps under REVERSE_SWAPS_KEY. Running the rescue again appended a second copy, and a swap rescued before the preimage was re-derived kept failing its claims with no preimage. Check both keys. For a reverse swap already stored without a preimage, attach the re-derived one in place, keeping its status and claim address, but only when the stored key is the one at the restored key index and any stored preimage hash matches. Collapse copies left by earlier runs to one, preferring the copy with a preimage, and store a swap listed twice in the response once. Closes ZeusLN#4821
verifyReverseSwapResponse accepted any key ecc.isPoint does, including the 65-byte uncompressed form. aggregateMusigKeys hashes the key bytes as given, while boltz-client's native MuSig2 hashes the compressed encoding, so an uncompressed key gave a different aggregate key than the claim signs for. The leaves still matched, since they use the x-only key. A host could fund the address derived from the raw bytes, pass verification, receive the preimage in the cooperative claim request, and leave the claim unsignable. Reject a server key that is not 33 bytes with an 02 or 03 prefix, and cover an uncompressed key with both the raw-bytes and the native lockup address.
Unmounting cleared only the recheck timer already pending, so a lookup still in flight scheduled a new one when it came back unavailable or unconfirmed. Final states (expired, failed, settled) and host errors closed the websocket but left the timer running, so rechecks kept going every 60 seconds. Stop rechecks on unmount, on every final state and on host errors, and don't schedule one or act on a result once stopped. A native claim already under way still finishes.
A rescued reverse swap has no on-chain amount, since the only source for one is the host, so verifyReverseLockup skipped the amount check. A host could lock up 1,000 sats against a 100,000-sat payment and still get the preimage that settles the whole payment. For a swap without an on-chain amount, find this wallet's own payment for the swap's hash (value_sat, amount_msat, or the invoice amount, which CLN needs for a payment that hasn't completed) and require the lockup to be at least that amount less 5% and 10,000 sats. If listing payments fails, retry. If the wallet has no such payment, return 'confirm-amount' with the lockup's value once every other check passes, and have SwapDetails ask the user to confirm it before claiming. Declining stops claims on the screen until the swap is opened again.
The SwapDetails tests mocked SwapUtils with swapWebSocketUrl only, so any helper added to it later is undefined there. Merged with ZeusLN#4944, which adds nativeSwapEndpoint, every test in the file failed. Spread the real module and override swapWebSocketUrl alone, and give the route a stored host with its /v2 suffix, as the app does, since the real helper needs one.
The stop flag added for teardown was only checked in the recheck handler, after createReverseClaimTransaction returned. A lookup still pending at unmount that came back ok went straight on to the native claim, starting a new claim after teardown. Pass the stop check into createReverseClaimTransaction and test it at the top of each attempt and again after each verification, right before the native call. A native claim already running still finishes.
The rescued-swap floor subtracted 5% and a fixed 10,000 sats from the wallet's own payment. On small payments the fixed allowance swallowed most of the amount: a 10,000-sat payment had a floor of 1 sat, so a 1,000-sat lockup was claimed without asking and the host kept 90%. Claim without asking only if the lockup is at least 90% of the payment. Below that, return 'confirm-amount' with both the lockup and the payment, and have SwapDetails show both amounts before the user decides. Declining stays safe: the hold invoice is cancelled at the swap's timeout and the payment comes back.
kaloudis
force-pushed
the
fix/reverse-swap-lockup-verification
branch
from
October 8, 2026 23:11
85d7074 to
ac3028c
Compare
shubhamkmr04
approved these changes
Oct 9, 2026
shubhamkmr04
left a comment
shubhamkmr04
left a comment
Contributor
There was a problem hiding this comment.
tACK ac3028c
Tested on regtest against a local Boltz stack (LND REST v0.21.1-beta), iOS 26.5 simulator and Android emulator. The final claim broadcast fails on regtest as expected (#4939); every check before it was verified.
- Reverse swap happy path: waits for the lockup to confirm, then claims.
- Bad provider responses are rejected before the invoice is shown: wrong lockup address, tampered claim leaf, on-chain amount below the quoted one, and fewer than 6 blocks to timeout (tampered with a local proxy).
- Claim-time deadline: a lockup within 6 blocks of its timeout is not claimed.
- Rescue: swaps from a real
/swap/restoreimport and claim. A host-sentdestinationAddressis ignored; the claim pays a fresh wallet address. - v13.2.2 repair: a swap rescued on v13.2.2 (no preimage) is repaired by rescuing again after upgrading, with no duplicate.
- Confirmation prompts: shown for a rescued swap paid from another wallet, and for a lockup under 90% of the payment. Cancel claims nothing; Claim proceeds.
- Payment lookup: finds the in-flight swap payment on LND REST. Not tested on embedded LND, LDK Node or CLN.
Non-blocking: findOwnPaymentAmount calls BackendUtils.getPayments() without params. Embedded LND only lists newest-first when reversed: true is passed, so in a wallet with more than 1000 payments the swap payment may not be found and the user gets an unnecessary confirmation prompt. Passing { maxPayments: 1000, reversed: true } would avoid that.
9 of 29 tasks
7 of 29 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Verify the reverse swap lockup independently before claiming.
Builds on #4893, which is now merged.
Closes #4943.
Closes #4821.
A reverse swap claim sends the preimage to the provider's cooperative claim endpoint (
/swap/reverse/{id}/claim, inside boltz-clientConstructTransaction) before the claim is broadcast. Previously the claim was triggered by the provider's websocket update and built from the transaction hex in that update, with no independent check that the lockup exists or pays the swap.At creation (
SwapStore.createReverseSwap, after the existing invoice check):[serverKey, ourKey], tweaked by the two-leaf treeonchainAmountabove the amount the Swaps screen showed the user they would receiveBefore every native claim attempt (
SwapStore.verifyReverseLockup, called fromSwapDetailsafter fee/address resolution and the retry delay):UrlUtils.getMempoolApiUrl), not the provideronchainAmountFindVout, using the transaction hex that native code will spend; require/tx/:txid/outspend/:voutto reportspent: false/blocks/tip/heightlast and require at least six blocks before the refund heightThe outspend check rejects both mempool and confirmed spends. It does not accept a later unspent output to the same address when the native claim would spend the first one. A missing, malformed, or unavailable outspend/tip response fails closed as retryable. An already-spent output or insufficient time before refund is invalid.
The six-block margin is a client safety policy to leave time for a unilateral claim if the provider refuses to co-sign. It is not a guarantee of confirmation within that window. A confirmed transaction alone is not sufficient: its output may already have been refunded, and an unspent output at the refund deadline can race the provider's refund.
Not-yet-seen or unconfirmed lockups and unavailable verification services show a notice and re-check every 60 seconds. Invalid lockups show
views.SwapDetails.lockupVerificationFailedand do not enter the native claim path. The client does not itself cancel the provider's hold invoice.The helpers use existing dependencies (bitcoinjs-lib, @bitcoinerlab/secp256k1, bignumber.js).
Rescued swap data from the host (
SwapStore.getRescuableSwaps,c83e1d798, #4943):/swap/restoreresponse into the stored swap, and the claim paid out to a storeddestinationAddressunchecked, so a rescue host could set the claim address, receive the lockup, and settle the hold invoice with the revealed preimage. The lockup checks above don't cover this, since the host can fund a valid lockup and still take it back through the claimid,statusandcreatedAt, plustree,keyIndex,lockupAddress,serverPublicKey,timeoutBlockHeight,amountandpreimageHashfrom the claim/refund details; a host-sentdestinationAddress,preimage,keys,refundPrivateKey,txidorlockupTransactionis droppedpreimageHashdoesn't match the preimage derived at itskeyIndexis skippedclaimAddressFromWallet, andSwapDetailsclaims throughSwap.claimDestinationAddress, which ignores an unmarked address on an imported swap. Swaps already rescued on a build with fix(swaps): re-derive the preimage when rescuing reverse swaps #4460's preimage re-derivation, which may hold a host address, get a fresh wallet address instead. A wallet-generated address stored by such a build is also replaced once, which uses up one extra addressRunning the rescue again (
SwapStore.getRescuableSwaps,9d2434951, #4821):SWAPS_KEYonly, butfetchAndUpdateSwapsre-files rescued reverse swaps underREVERSE_SWAPS_KEY, so a second run appended a duplicate. A swap rescued before fix(swaps): re-derive the preimage when rescuing reverse swaps #4460 has no preimage, and with this PR opening it fails verification withmissing-keys; running the rescue again was the obvious next step and repaired nothingclaimAddressFromWallet, only when its stored key is the one at the restoredkeyIndexand any storedpreimageHashmatchesc83e1d798moved intoSwapStore.buildRescuedSwapReview round fixes (
551563730,f27ffc959,4f6b8ba47, then82175bd2c,4d6e20276,85d70749fin the second round):verifyReverseSwapResponseaccepted a 65-byte uncompressed server key.aggregateMusigKeyshashes the bytes as given while boltz-client's MuSig2 hashes the compressed encoding, so a host could fund the raw-bytes address, pass verification, and receive the preimage while native signed for another output key. Only a 33-byte02/03key is accepted now (server-key-encoding).onchainAmount, so its lockup amount was not checked and a host could trade a small lockup for the preimage. For a swap without one,verifyReverseLockupnow finds this wallet's own payment for the swap's hash (value_sat,amount_msat, or the invoice amount, which CLN needs for a payment that hasn't completed) and claims without asking only if the lockup is at least 90% of that payment. The first version subtracted 5% and a fixed 10,000 sats, which set the floor for a 10,000-sat payment at 1 sat; the floor is now proportional. If payments can't be listed it retries. If the lockup is below 90% of the payment, or the wallet has no such payment, every other check still runs, thenSwapDetailsasks the user to confirm before claiming, showing the lockup amount and, when known, what the user paid. Declining stops claims on the screen until the swap is reopened; the hold invoice is then cancelled at the swap's timeout and the payment comes back.swap.expired,invoice.expired,transaction.failed,invoice.settled) and host errors closed the websocket but left the timer running. Rechecks now stop on all of them. The stop check is also passed intocreateReverseClaimTransactionand tested before every native attempt, so a verification that comes backokafter teardown no longer starts a claim; a native claim already under way still finishes.SwapDetails.test.tsxmockedSwapUtilswithswapWebSocketUrlonly, so merged with Swaps: strip /v2 from the swap host before native claim and refund calls #4944 (nativeSwapEndpoint) every test in the file failed. It now spreads the real module and gives the route a/v2host as the app does; all swap suites pass on a tree merged with Swaps: strip /v2 from the swap host before native claim and refund calls #4944.Behavior changes:
Known residuals:
onchainAmount; they are claimed without asking only at 90% or more of the wallet's own payment, and otherwise only after the user confirms the amountonchainAmount, which was never checked against what the user was shown, and use it instead of the payment floorZeusLN/lndmobile/swaps.go) still trusts its inputs; a JS check cannot make network observation and native broadcast atomicThis pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyTesting
If you modified or added testable logic, did you add new unit tests? For bug fixes, please include a test that would have caught the bug.
utils/SwapUtils.test.ts:49baf874bstores/SwapStore.reverseLockup.test.tsuses real crypto and mocked explorer/provider requests:New
views/Swaps/SwapDetails.test.tsx:stores/SwapStore.rescue.test.ts(rescue allow-list,c83e1d798):destinationAddress,claimAddressFromWallet,imported,preimage,keys,refundPrivateKey,txidandlockupTransaction, at the top level and inclaimDetails, are not stored; the fields the claim needs are keptpreimageHash(any case) is kept; a mismatched one is skipped while the other swaps in the response are still rescuedSwap.claimDestinationAddressfor a swap created on this device, an imported swap with a host address, and an imported swap with a wallet addressviews/Swaps/SwapDetails.test.tsx: a rescued swap with a stored host address claims to a wallet address; a swap created on this device claims to the address picked for it.Before the fix, 9 of these tests fail; they all pass now.
stores/SwapStore.rescue.test.ts(running the rescue again,9d2434951):REVERSE_SWAPS_KEYis not appended againBefore the fix, 5 of these fail; the other 4 check that nothing changes where it shouldn't.
Review round (
551563730,f27ffc959,4f6b8ba47):utils/SwapUtils.test.ts: the uncompressed form of the vector's server key is rejected both with the address derived from its raw bytes (which passed before) and with the native address;rescuedLockupFlooris 90% of the payment, rounded upstores/SwapStore.reverseLockup.test.ts: against a 100000-sat payment, lockups of 99100 and 90000 sats are claimed and 1000 and 89999 sats giveconfirm-amountwith both amounts; the same holds at 10000 sats (1000, 8999 ask; 9000 claims) and 25000 sats (22499 asks; 22500 claims); the amount is read fromvalue_sat,amount_msat, or the invoice; no payment record givesconfirm-amountwith the lockup value, only after the other checks pass; a confirmed amount is enforced; a failed payment listing isunavailable; a swap with anonchainAmountdoesn't list paymentsviews/Swaps/SwapDetails.test.tsx: no recheck after an unmount during a lookup; a scheduled recheck is cancelled on each final state and on a host error; the confirmation prompt claims with the confirmed amount once accepted, isn't shown twice, and stops claims once declinedviews/Swaps/SwapDetails.test.tsx(second round): a lookup in flight at unmount that comes backokstarts no native claim; the shortfall prompt shows what the user paidBefore the first-round fixes, 16 of these fail (2 key encoding, 6 teardown, 8 amount floor). Before the second round, the
ok-after-unmount case and 13 small-payment and prompt cases fail.Regression evidence: before implementing the deadline/outspend checks, the expanded store suite had 23 failures and 18 passes. Those checks now pass. This follow-up adds 47 tests across utility, store, and view suites.
Latest local validation (nested agent worktrees excluded from discovery):
Earlier iOS 26.5 simulator testing of
49baf874bis recorded in #4894 (comment). That exercised a local fake provider and fake Esplora, and found the Buffer polyfill bug. It does not validate this follow-up's new deadline/outspend checks on-device.Device testing still owed for this follow-up: both platforms, a fresh reverse swap, a reopened/rescued swap (including a check that a live
/swap/restoreresponse still produces a claimable swap through the field allow-list, that a swap rescued on v13.2.2 is repaired by running the rescue again, the payment amount lookup on each backend for a pending swap payment, and both confirmation prompts: a swap paid from another wallet, and a lockup below 90% of the payment), spent and near-expired lockups, explorer failures, and a successful native claim/broadcast. Native code currently supports mainnet/testnet parameters and broadcasts to mempool.space, so an end-to-end regtest claim is blocked by the existing native limitations.I have tested this follow-up on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
Locales
This follow-up reuses the PR's existing localized verification messages.
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther