Repository navigation
feat: pubky swaps as paykit route - #182
Conversation
iroh reads network interfaces through Network.framework, so apps that link BitkitCore without it fail with undefined nw_* symbols.
|
Reviewed from the pubky-swap side at
One more, smaller: the edges use Converging with
|
|
@coreyphillips thanks for the careful read.
Fixed in 160d443:
Also in 160d443:
Agreed, and I'll take your change-output design: a change output back to the wallet absorbs the bumps, and the contact's output stays exact. The record then waits for the claim to confirm instead of turning
Agreed. The 10 minutes is a regtest demo bound. Mainnet needs a resume on app start plus a background wake when the lockup reaches depth, with the error screen only for a real failure. Listed under What it needs to ship, together with the app-killed and seed-restore cases from pubky/paykit-rs#229.
On identity: one seed-derived key keeps the user's Pubky unlinked from every provider. It also misses contact allowlists (coreyphillips/pubky-swap#88) and Encrypted Link recovery, as you say. That trade-off is better settled for
Right. The wildcard only labels the edge's family; the route itself always targets the contact's concrete identifier, for example
Agreed on one path: this PR's router and Paykit glue on top of |
coreyphillips
left a comment
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
needs changing before merge
- Restored or reinstalled wallet reuses payment hashes whose preimages are public (src/modules/pubky_swaps/store.rs:56-66). After a seed restore, a reinstall, a wipe, or on a second device with the same seed, the next swap reuses the payment hash of the first swap, and that preimage is already public.
store::reserve_index(src/modules/pubky_swaps/store.rs:59-61) returns 0 when<base>/pubky_swaps/next_indexis missing.keys::swap_keysis a pure function of seed and index (keys.rs:50-66, and its own test asserts the preimage repeats for the same index). The first swap's claim put that preimage in its witness: swap-commonbuild_htlc_spendpushes it at onchain.rs:287 at 8686702. Every Lightning hop that forwarded its hold-invoice payment also saw it at settlement. A provider (any contact that answers an offer) or a hop that knows the preimage can settle the new hold invoice without ever locking funds, and the wallet loses the debit. What I checked: the store default, the derivation, and the claim witness. Two things mitigate it in some cases. pubky-swap's own provider refuses a hash already in its store (ensure_reverse_hash_available, swap-provider/src/lib.rs:1828), so the same honest provider turns this into aninvalid_requestrefusal; a different or dishonest provider has no such check. I did not verify whether the app's LDK node refuses to re-pay a hash it paid before; that depends on its payment store surviving the restore and is outside this repo. A random per-swap nonce mixed into the derivation and kept in the record would remove the reuse and still keep secrets out of the record. So would an index that cannot repeat, such as one seeded from the clock. The index file is also written without fsync, which gives a smaller version of the same reuse after a power loss. - Claim is broadcast once at a fixed fee and reported Claimed, with mainnet allowed (src/modules/pubky_swaps/client.rs:525-538). A claim that does not confirm before the timeout loses the whole debit, even to an honest provider, and the exports accept
Network::Bitcoin.advance_blocking(client.rs:525-538) signs the claim withrecord.claim_fee_sat. That fee was fixed at route time from the app'sfee_rate_sat_vb, which the description says is 2 sat/vB in the Android caller. It broadcasts once and storesclaim_txid, and every later call returnsClaimedat line 458 without looking at the chain again. The pinned provider settles the hold invoice as soon as it sees the claim in Electrum history, which includes the mempool (swap-provider/src/reverse.rs:570-600 andfind_spendin swap-common chain/electrum.rs at 8686702). If that spend is not buried, it falls back to refunding. The claim signals RBF (ENABLE_RBF_NO_LOCKTIME), so oncetimeout_block_heightpasses, the provider's refund replaces a claim still sitting in the mempool. The invoice is then settled and the lock refunded. The description lists claim confirmation under what it needs to ship. Nothing in the code limits the exposure:route_paykit_paymentandcommit_routeaccept mainnet. Either land a re-sign/bump path that keeps watching until the claim is buried, or refuseNetwork::Bitcoinin these exports until it exists. - Seed restores reuse previously disclosed payment preimages (src/modules/pubky_swaps/store.rs:59-64). Restoring the seed without local swap state can reuse a preimage that an earlier claim already published.
reserve_indexrestarts at zero, andswap_keysderives the preimage only from the seed and index. I confirmed identical preimages and payment hashes before and after removingnext_indexin an isolated test. A provider knowing the old preimage can settle the new hold invoice without funding its lockup. - Unconfirmed claims become permanently terminal (src/modules/pubky_swaps/client.rs:535-538). A broadcast claim receives no further protection even if it never confirms. Saving
claim_txidmakes every lateradvancereturn before checking the chain. I confirmed this with mocked chain responses: an unconfirmed broadcast remainedClaimedwhen the chain became unavailable. If the claim stalls or is evicted, its public preimage lets the provider settle Lightning and later refund the lockup. The claim must remain monitored and bumpable while preserving the recipient's amount.
worth doing, does not block
- Commit accepts a Taproot contract that advance can never find (src/modules/pubky_swaps/client.rs:389-396).
commit_routeasks forSwapScript::P2wshbut never checks thataccept.script_typecame back as P2wsh. swap-common'sverify_contractsays the caller must do that check (validate.rs:487-488). A Taproot acceptance with a consistent tree passes verification (client.rs:389-396), and the Taproot address is returned to the app ashtlc_address.advance_blockingthen always rebuilds a P2WSH script (client.rs:468-474), never finds the lockup, and reportsAwaitingLockupuntil the provider refunds. Funds come back when the hold invoice is cancelled, but the wallet's Lightning payment stays locked until then. Reject anyscript_typeother than P2wsh before validating. - advance has no terminal state for a missing or already spent lockup (src/modules/pubky_swaps/client.rs:478-538). If the provider never funds,
advance_blockingreturnsAwaitingLockup { confirmations: 0 }forever, even aftertimeout_block_heighthas passed (client.rs:496-502). The app gets no signal to stop polling. If the app dies between the broadcast andstore::save(client.rs:535-537), the next call rebuilds the same claim and rebroadcasts it. The broadcast fails with already-known or inputs-spent, so every call returns aChainerror and the record never reachesClaimed. The same happens if the record file is lost after a successful claim. The script history already fetched at line 481 contains any spend of the outpoint. Checking it before broadcasting, and returningFailedonce the tip passes the timeout with no lockup, would close both. - Offers are not bound to the candidate that answered (src/modules/pubky_swaps/client.rs:222-235). The router keys offers and edges by the
provider_pkarrwritten inside the offer (client.rs:229, router.rs:70), not by the candidate whose iroh connection returned it.quote_edgethen connects to that claimed key (client.rs:142). So any contact can send the wallet's quote and commit traffic to an endpoint that was never in the candidate list. It can also name another candidate's key, which duplicates that provider's edge and spends the 3-quote fanout on it twice. The validators still apply, so this is not a theft path, but it undoes the restriction to the providers the caller passed. Drop offers wherebare_pubky(&offer.provider_pkarr) != provider. - Probing every contact through n0 DNS discloses the contact list (src/modules/pubky_swaps/transport.rs:30).
DirectClient::bindusespresets::N0(transport.rs:30). In iroh 1.2.0 that preset resolves endpoint ids over HTTPS and DNS through n0's servers, and publishes the wallet's own address there (endpoint/presets.rs:116-138).route_paykit_paymentprobes every candidate at once (client.rs:207-221), and the description says the candidates are usually the user's contacts. Each payment that needs a swap therefore sends n0 a burst of lookups for the user's whole contact list, from one stable swap identity and IP. The fan-out is also unbounded, so a large contact list opens that many concurrent connections on a phone. The PR's claim that iroh needs no homeserver is true, but it does depend on n0's infrastructure. Consider capping the candidate count and documenting or replacing the n0 address lookup. - Swap store ignores a second init_db path (src/modules/pubky_swaps/store.rs:36-41).
store::initsaves the base path in aOnceCelland ignores later calls (store.rs:39).init_dbreplaces the activity, Blocktank and Boltz connections on every call. After a secondinit_dbwith a different base path, swap records andnext_indexkeep going to the first directory while everything else moves. - Commit accepts contract types that advance cannot claim (src/modules/pubky_swaps/client.rs:388-396) [one reviewer only]. A provider can return
TaprootBoltzdespite the request selectingP2wsh, and commit still returns the invoice. I confirmed this with a valid Taproot acceptance and hold invoice in an isolated test.verify_contractvalidates the returned type, butadvance_blockingonly watches and spends P2WSH, so it cannot claim that lockup. - Expired unfunded swaps remain AwaitingLockup forever (src/modules/pubky_swaps/client.rs:496-508). An unfunded swap never becomes terminal after its timeout. The missing-lockup and insufficient-confirmation returns run before the timeout check. I reproduced an empty history at tip 2,000 with timeout 1,144, which still returned
AwaitingLockup. Callers waiting for a terminal status can poll forever. - Sync permanently preserves an unknown fee rate (src/modules/activity/implementation.rs:1093). An outgoing activity first stored with an unknown rate (
0) cannot acquire its rate through later syncs. I reproduced a preserving upsert of zero followed by a confirmed snapshot at 12 sat/vB: the absolute fee and confirmation updated, but the rate stayed zero. Preserve known positive rates while allowing an unknown rate to be filled. - The new dependency exceeds the declared Rust minimum (Cargo.toml:51). The pinned
iroh1.2.0 requires Rust 1.91, while this package still advertises Rust 1.90. Dependency validation rejects builds using the advertised minimum. Raise the package minimum or select a compatible dependency.
nits
- Quote cache never evicts (src/modules/pubky_swaps/client.rs:279-286).
QUOTES(client.rs:43) only drops an entry on a successful commit (line 428). Every routed payment the user abandons, and every expired quote, stays in memory for the life of the process. Pruning expired entries on insert would do.
the reviewers disagree, your call
- Quote ID collisions change the committed recipient (src/modules/pubky_swaps/client.rs:279-285). Objection: Real, but non-blocking rather than blocking: Real:
QUOTESis a process-global map keyed by the provider-chosenquote_id, so a second route with the same ID overwrites the first, and committing the first uses the second's address and debit. Not blocking: it needs two uncommitted routes in one process, the redirected recipient is one the wallet itself routed to, and the provider gains nothing. - Offers can redirect payments outside the provider list (src/modules/pubky_swaps/client.rs:227-229). Objection: Real, but non-blocking rather than blocking: Real: the queried
provideris dropped andoffers/edges are keyed byoffer.provider_pkarr, which is never compared with the peer that answered. A listed candidate can therefore steer the wallet to an unlisted provider, or overwrite another provider's offer. Not blocking: the HTLC doesn't depend on trusting the provider, the counterparty's quote and contract are still checked, and the review screen shows its real pubky.
|
@coreyphillips all four blocking points are addressed in 7621dfc, with iOS bindings in 64cdede.
Each swap's claim key and preimage now derive from the seed and a random 32-byte nonce, under new
I took the option to refuse mainnet.
Fixed:
Fixed. No lockup by the timeout is
Fixed: an offer whose
Candidates are capped at 30, in the caller's order, so configured providers come first. The n0 lookups are documented on the cap. Replacing n0 discovery belongs with moving negotiation onto Encrypted Links (pubky/paykit-rs#229).
Fixed: the store's base path moves with every
Not in this PR's diff; it comes from #166 on
Fixed:
Fixed: expired quotes are pruned on every insert.
Fixed: the cache is keyed by an id the wallet assigns to each route. The provider's quote id stays inside the cached entry.
Fixed with the offer binding above. |
Addressed in 7621dfc and 64cdede (reply: #182 (comment))
|
Pushed
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c8a7929f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An unsent saved claim waits for the lockup's confirmations again after a reorganization, and a claim of a lockup the provider replaced is built again for the new outpoint.
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P2). Adds Pubky Swaps as a Paykit route: signed paykit-cap quotes and contracts with delegated signer resolution, seed-derived swap keys with an owner_ref index, a reverse-swap claim driver over Electrum, and restore from provider listings. Reviewed 1fee62c, oversized tier (about 3,700 non-generated lines).
What I checked, and 6 candidates I ruled out
Read in full: pubky_swaps/keys.rs, claim.rs, cap.rs (non-test), delegation.rs, router.rs, store.rs, transport.rs, client.rs L106-1112, the paykit_route and pubky/profile.rs changes, Cargo.toml.
Upstream checked: swap-common at f45e204 (validate_quote, validate_accept, validate_hold_invoice, min_reverse_lockup_sat, claim_start_is_safe, swap-client/src/restore.rs).
Not read: client.rs tests and the live regtest test, lib.rs exports, generated bindings.
Build: not built here. talos-pr-run refused because it could not check the author's permission. This review is static.
CI: SwiftPM, manifests, paykit-route, wake tests green; Android/iOS bindgen and usdt pending at review time. No workflow runs cargo test modules::pubky_swaps.
Cold reader: did not finish within this run, so the finding is my own reading of L865-868, L1088 and claim::next_step.
Ruled out
- Unsigned or wrongly signed replies:
checkeralways pins the provider root key,transport::cap_replyrejects non-cap replies, and a delegated key is accepted only whendelegation::verify_chainchecks owner, generation and both signatures. - Lockup smaller than paid on restore: bounded by
min_reverse_lockup_satagainst the node's own payment, and the hash must derive from this seed. - Early preimage reveal from a provider's listing: a listed claim needs a settled payment and the lockup's confirmations (
claim.rsL125-136). - Refund mistaken for our claim: spends are classified by preimage in the witness, not by destination (
claim.rsL98). - Lost broadcast reply hiding a published preimage:
claim_publishedis saved before the send and cleared only on a protocol refusal of the first attempt. - Mainnet exposure with an unbumpable claim:
best_swap_routeandcommit_routeboth refuseNetwork::Bitcoin.
Merge confidence: 3/5, oversized tier ceiling; the remainder was not read and the PR was not built here.
|
@coreyphillips thanks. Since your approval at 4c8a792 I pushed fixes for three findings from Codex and Talos, so this PR needs your approval again:
I'll take both of your follow-ups in one PR after this merges: setting |
# Conflicts: # Cargo.lock # Cargo.toml # Package.swift # bindings/android/gradle.properties
|
@coreyphillips 9e4589c merges master (the wake client fixes from #195) into this PR. The only conflicts were the version strings, which keep 0.8.0-swaps.9. No swap code changed since the commits listed above, and CI is green. |
|
Merged: approved by @ben-kaufman on 4f3e215 and @coreyphillips, checks green, every review thread answered and resolved. Non-blocking items are tracked for one follow-up PR, together with #197. |
Builds on #193, merged.
Refs:
Description
pubky_swapsmodule so that a Paykit payment routes through a Pubky Swaps provider when no balance the wallet holds reaches an endpoint the contact published. The user never picks the swap; the router does.swap/1routes, following Paykit Capabilities §9.1. It runs the router's direct and service-leg check first. When no balance reaches the contact, it turns each provider's offer intoswap/1edges, pre-ranks them by indicative fee (base + ppm + onchain, enforced minimum), asks the cheapest three for a firm quote and picks the smallest debit for the exact amount the contact must receive. It returns whichever costs less in fees, the cheapest live service leg or the swap; a swap wins a tie.paykit-cap.*messages (Capabilities §10) and refuses anything unsigned:paykit-cap-v1anddirect-rpc-v1are quoted. The offer is unsigned, so it only says who to ask; the route is priced on the signed quote.CapClientchecks each reply. The quote'sterms_hashbinds this wallet, the provider, the request and the quote. The contract'scommit_hashbinds it to this wallet's commit. The hold invoice arrives sealed under a hash the contract signs.paykit_identity::resolve_signing_key; the tests hold records upstream's code wrote.owner_ref(Owners can now list their swaps with SwapListRequest, paged and scoped like status queries, and a swap-client restore helper rebuilds them from the listing for a wallet that has only its seed coreyphillips/pubky-swap#113). A restored wallet or a second device never repeats a published preimage.pubky_swaps_restorefor a wallet restored from its seed:swap-list-v1, all at once, derives their keys fromowner_ref, and checks the network, the hash and the contract against them.sent_paymentsholds a Lightning payment for its hash whose lockup is at leastmin_reverse_lockup_satof it, and no other record holds that hash. Finished swaps get none.pubky_swaps_advanceuntil it confirms, sends it again if it drops out of the mempool, and fails a swap whose lockup the provider refunded or never made before the timeout. A spend of the lockup is a claim only when its witness carries the preimage, so a claim from another device counts and a refund never does.CLAIM_ABORT_MARGIN(6) blocks away, and keeps asking about a commit the provider reports aspendinguntil it accepts or refuses.swap-commonvalidators.pubky-swap/direct/1at once, with 20 s to answer, and those that run no provider are skipped. iroh needs no homeserver on either side.pubky_swaps_route_paykit_payment,pubky_swaps_commit_route,pubky_swaps_advance,pubky_swaps_provider_edges,pubky_swaps_restore.This head is the prerelease
0.8.0-swaps.11. The app twins are synonymdev/bitkit-android#1732 and synonymdev/bitkit-ios#1170.Cross-platform impact
swap-common(git, coreyphillips/pubky-swap atf45e204, unaudited, which bringschacha20poly1305),iroh1.2.0 andelectrum-client0.25.ed25519-dalek2 becomes a direct dependency; it was already in the tree. The library size change against a release build is not measured yet.nw_*symbols.PaykitRouteRequesthasservice_legs, andPaykitRoutehas aServicecase (from feat: pay contacts via spark #193).PubkySwapRouteStatehasClaimBroadcast, andClaimedmeans the claim is confirmed.PubkySwapRouteQuote.quote_idis a route id the wallet assigns.pubky_swaps_restoretakes the wallet's sent Lightning payments, with whether each settled, and returns aPubkySwapRestoreReport.What it needs to ship
swap/1in Paykit:swap/1descriptor yet, so offers stay unsigned.paykit-cap.*over Encrypted Links needs the additive paykit-rs SDK API for extension kinds (Design review: Paykit Capabilities, a general capability and intent layer for Paykit peers (answers Paykit Intents question 8) pubky/paykit-rs#229, §17), and the compact signed contract (Fit signed paykit-cap contracts into the 1,000-byte Encrypted Links budget coreyphillips/pubky-swap#119) to fit its size limit.key_generationuntil the §8.1 envelope lands, so a delegated signer is checked against the generation published now.pubky_swaps_restorerebuilds them from providers' listings, within their retention window. An app restart between paying and claiming leaves the claim to a laterpubky_swaps_advancecall; the app does not resume it yet. Funds stay safe through the timeouts: the provider cancels its hold invoice, or refunds its own lock.confirm_or_bumpcan raise its fee until it confirms (up totimeout - CLAIM_ABORT_MARGIN) while the contact's output stays exact; until then, swap routes are refused on mainnet.sent_payments, and restore again once a pending one settles: a swap already on record then picks up a listed claim. Finished swaps get no record.feat/pubky-swap-boltzonce it builds without a local path.Preview
The Android twin's journey: a wallet with only a spending balance pays a contact who accepts only on-chain bitcoin. The router finds a Pubky Swaps provider among the wallet's contacts, the confirm screen shows the route, and the contact receives exactly 20,000 sats on chain (2x speed, regtest). This recording is from the twin's own bitkit-core build, before negotiation was signed; the signed path is proven at the bitkit-core level below.
swaps-route-android2x.mp4
QA Notes
Setup
f45e204:docker compose -f docker-compose.regtest.yml up -d(bitcoind, electrs, two LND nodes), then./scripts/setup-regtest-lnd.sh, which opens a balanced channel fromlnd-atolnd-b.lnd-a:cargo build --release -p swap-provider --features full, thenswap-provider --network regtest --directions submarine,reverse --min-amount 10000 --max-amount 1000000 --base-fee 100 --fee-ppm 2500 --confirmations 1 --wallet lnd --lnd-address https://127.0.0.1:10011 --lnd-cert <lnd-a tls.cert> --lnd-macaroon <lnd-a admin.macaroon> --electrum-url tcp://127.0.0.1:60001 --data-dir <dir> --no-homeserver, withPUBKY_SWAP_RECOVERY_PHRASE__FILEpointing at a throwaway BIP39 phrase. It prints its pubky.lnd-bis the Lightning-only payer.Automated Checks
cap.rstests: a quote signed by the provider is taken. These are refused:A committed swap's status without its contract asks for the commit again.
delegation.rstests on records written by pubky-swap'spaykit_identityatf45e204(its golden vector included): the chain resolves to the delegated key for generations 1 and 2. These are refused: a delegation for another generation, records for another identity, and a swapped key, routing key, missing signature or unknown field.transport.rstest: an unsigned reply to a signed request is refused.client.rslive test (ignored by default) to the full journey. It routes, commits and checks the signed contract, then runsPUBKY_SWAPS_PAY_CMD <invoice>alongside andPUBKY_SWAPS_MINE_CMDbetweenpubky_swaps_advancecalls untilClaimed, and checks the claim pays the contact exactly the amount asked. Run withPUBKY_SWAPS_PROVIDER=<pubky> PUBKY_SWAPS_ELECTRUM=tcp://127.0.0.1:60001 PUBKY_SWAPS_DESTINATION=<bcrt1 address> PUBKY_SWAPS_PAY_CMD='docker exec lnd-b lncli -n regtest --lnddir=/home/lnd/.lnd payinvoice --force' PUBKY_SWAPS_MINE_CMD='docker exec bitcoin bitcoin-cli -regtest -rpcport=43782 -rpcuser=polaruser -rpcpassword=polarpass -generate 1' cargo test --lib pubky_swaps::client::live_tests -- --ignored --nocapturecargo fmt --check,cargo clippy --lib --testsandcargo test --lib modules::pubky_swaps(41 tests): passlnd-bpaid the 20,986 sat hold invoice, which stayedIN_FLIGHTuntil the claim and thenSUCCEEDED.pubky_swaps_advancereportedAwaitingLockup,ClaimBroadcastand thenClaimed.btc-regtest-p2wpkhaddress.