Repository navigation
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
|
|
@ovitrif #105 is merged as coreyphillips/pubky-swap#113 ( |
|
@coreyphillips thanks. Done as you described, in cab1972, with iOS bindings in 0f93ce0 and the pin moved to
That closes both same-second points from your review. |
coreyphillips
left a comment
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked f07bccb. The two same-second points are closed: a random 32-byte index, an owner_ref that round-trips, and no clock index left. Restore checks payment_hash_hex and verify_contract before it saves, and skips swaps already on record. cargo test --lib modules::pubky_swaps passes (23).
Two fixes before merge:
- A restored swap whose claim the provider saw can get stuck.
restored_recordsaves the provider'sclaim_observed_txid_hexas a published claim with noclaim_tx_hex(client.rs#L765-L781). If that claim then leaves the mempool,next_stepreturnsRebroadcast(claim.rs#L108-L109) andadvance_blockinghas nothing to send (client.rs#L563-L567). The wallet never claims, while the preimage is already public and the provider can refund at the timeout. Suggest: with no saved claim hex, build and broadcast a fresh claim (no window check, the preimage is out), plus a test. list_swapshas no page limit (client.rs#L706-L727). A provider that keeps returningnext_cursorstopspubky_swaps_restorefrom returning, and the providers after it are never asked. Suggest a page cap, or stopping on a cursor seen before.
Minor: store::save(&record)? (client.rs#L697) ends the whole restore on one write error.
Pin: keep 1801c80 in this PR. Bump it with the owner-side wake work, which needs SwapWakeCapability from d5f1ce5. swap-common only gains ed25519-dalek 2 and chacha20poly1305 0.10 by then.
|
@coreyphillips all three are fixed in 3d36a27.
It now stops on a cursor it already saw, or after 100 pages, logs which, and keeps what it read, so the providers after it are still asked.
A failed save now logs and skips that swap; the rest of the restore goes on.
Kept. It moves with the owner-side wake work.
|
ovitrif
changed the title
|
Retargeted onto #193, which now holds the Paykit router and its Spark legs, so that paying contacts via Spark can ship without waiting for pubky/paykit-rs#229. This PR now carries only Pubky Swaps:
|
# Conflicts: # bindings/ios/bitkitcore.swift
# Conflicts: # Cargo.lock # Cargo.toml # Package.swift # bindings/android/gradle.properties
# Conflicts: # Cargo.lock # Cargo.toml # Package.swift # bindings/android/gradle.properties
…bky-swaps-route # Conflicts: # bindings/ios/bitkitcore.swift # src/modules/paykit_route/router.rs # src/modules/paykit_route/types.rs
…bky-swaps-route # Conflicts: # Cargo.lock # Cargo.toml # Package.swift # bindings/android/gradle.properties
Stacked on #193
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 trusted 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.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_restore: for a wallet restored from its seed, it lists each provider's swaps (swap-list-v1), derives their keys fromowner_ref, checks them against each contract, and rebuilds the records. Restored swaps claim to the wallet's own payout address, since the listing doesn't carry the contact's.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.CLAIM_ABORT_MARGIN(6) blocks away, and keeps asking about a swap request 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.The apps that use this are synonymdev/bitkit-android#1732 and synonymdev/bitkit-ios#1170, both on the prerelease
0.8.0-swaps.2or its predecessor0.8.0-swaps.1.Cross-platform impact
swap-common(git, coreyphillips/pubky-swap at1801c80, unaudited),iroh1.2.0 andelectrum-client0.25. The library size change against a release build is not measured yet.cargo checkpasses foraarch64-apple-ios.nw_*symbols.PubkySwapRouteStategainsClaimBroadcast, andClaimednow means the claim is confirmed. Both apps'default/elsebranches keep compiling, but they wait for a block after the claim before showing success.PubkySwapRouteQuote.quote_idis a route id the wallet assigns.pubky_swaps_restore.What it needs to ship
swap/1in Paykit: Capability Descriptors published by providers, andpaykit-cap.*negotiation over Encrypted Links. That 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). pubky-swap signs nothing atmain, so quotes and contracts are not attributable yet.pubky_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.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).
swaps-route-android2x.mp4
QA Notes
Setup
8686702, built withcargo build --release -p swap-provider --features full, with one local change so it answers over iroh without signing in to a homeserver:swap-provider/src/lib.rs, inrun).swap-provider --network regtest --directions submarine,reverse --min-amount 10000 --max-amount 1000000 --base-fee 100 --fee-ppm 2500 --confirmations 1 --lnd-address https://127.0.0.1:23009 --lnd-cert <bitkit-docker>/lnd/tls.cert --lnd-macaroon <bitkit-docker>/lnd/data/chain/bitcoin/regtest/admin.macaroon --electrum-url tcp://127.0.0.1:60001 --data-dir <dir>, withPUBKY_SWAP_RECOVERY_PHRASE__FILEandPUBKY_SWAP_WALLET_MNEMONIC__FILEpointing at two throwaway BIP39 phrases. Fund the provider's first BIP84 address and mine six blocks.--doctormust pass.Automated Checks
keys.rsandrouter.rsunit tests: seed derivation, edge ranking, fee formula, endpoint preferenceclient.rslive test (ignored by default): route, quote, commit, contract and hold-invoice checks against a running regtest provider. Run withPUBKY_SWAPS_PROVIDER=<pubky> PUBKY_SWAPS_ELECTRUM=tcp://127.0.0.1:60001 PUBKY_SWAPS_DESTINATION=<bcrt1 address> cargo test --lib pubky_swaps::client::live_tests -- --ignoredcargo fmt --check,cargo clippy --locked --lib --tests,cargo test --lib modules::pubky_swaps,cargo check --lib --target aarch64-apple-ios: pass3fd9e9d, with the provider found only among the wallet's contacts: the provider loggedhold invoice accepted,HTLC fundedandclient claimed; hold invoice settled, and the claim paid exactly 20,000 sats to the contact's address (one input from the HTLC, one output)