fix: add lsp refund address

[ovitrif]

Fixes #728

Description

This PR:

1. Supplies Blocktank channel orders with a reusable native SegWit refund address controlled by the active Bitkit wallet.
2. Rotates the address after local wallet activity records a payment, while validating restored address ownership and revealing the cached index to LDK before reuse.
3. Keeps native SegWit monitoring enabled so delayed Blocktank refund payments remain detectable after restart or restore.

Linked Issues/Tasks

• fix: pass refundOnchainAddress to createOrder #728
• Android counterpart: fix: add lsp refund address bitkit-android#1235

Preview

QA Notes

Manual Tests

• 1. Settings → Advanced → Address Type: Native SegWit monitoring stays enabled and clearly explains that it is required to detect Blocktank refund payments.
• 2. Standard and advanced Transfer to Spending orders: the app created and funded both orders while preserving the same cached refund address across an abandoned order and restart.
• 3. Restore the wallet metadata before an incoming payment: LDK restored the revealed native SegWit index from VSS, the restore pruning step retained native SegWit monitoring, and Bitkit detected a direct regtest deposit after restore.
• 4. Fund the order from a paired hardware wallet and sign on-device: a fresh internal Bitkit wallet submitted order 9cac70da-71fc-4c63-8da4-ddeb2d6518f6 with its native SegWit refund address, the paired Trezor signed funding transaction 66127374a905d1437eb761101c567c27bba4282094cc482363e09de10e2a7fd1, and the app reached Transfer Successful.
• 5. Let a paid, unclaimed staging order expire: confirm the created refund record targets the supplied address, then confirm broadcast after an authorized refund approval. Staging expiry is approximately 48 hours and payout requires backend approval.

Automated Checks

• BitkitTests/BlocktankRefundAddressProviderTests.swift: covers reusable allocation, rotation after recorded payment, concurrency, cancellation, ownership/index validation, failure blocking, and cross-platform metadata compatibility.
• BitkitTests/AddressTypeSettingsTests.swift: covers native SegWit startup and restore normalization.
• BitkitTests/BlocktankRefundAddressLiveIntegrationTests.swift: passed on staging/regtest. Five estimates allocated no address; real created, rejected, restarted, and restored orders reused index 0; a real 10,000 sat deposit was recorded in Core's local activity database; the next order rotated once to index 1 and the following order reused it.
• Live lifecycle order IDs: 5d1d1329-4f21-4cf3-9606-6d16728bc378, be83c116-cfe6-4439-92e1-853d9695ac87, 8a968b2f-55da-4a71-9101-90743a9a67b5, 9c67b9ab-e3a6-4825-bca7-3ee71a5f54bc, 8c443cee-9cba-4c08-b801-3cbc390123df, 074033dc-c081-4343-bb9c-f1eb27a1451b.
• Incoming deposit transaction: 99581b96ecfa768de10fc8518e162cb3867907a8fd0a295f81477c72a9c584f9.
• Hardware-funded order: 9cac70da-71fc-4c63-8da4-ddeb2d6518f6 reached executed / open; its internal refund destination was native SegWit index 2, independent of the Trezor funding account.
• Focused simulator suite: 47 tests passed.
• Exact branch build: built, installed, and launched on the dedicated Bitkit-PR728-Refund simulator.
• node scripts/validate-translations.js: passed.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; no concrete correctness, security, workflow, or repository-rule violation remains.

Summary

• Caches and validates the refund address and derivation index before reuse.
• Reveals restored indexes to LDK and rotates the cache after recorded address activity.
• Keeps native SegWit monitoring enabled across startup, settings changes, and restoration.
• Extends metadata backup/restore and adds unit and live integration coverage.
• Separates order submission behind an injectable client to test refund behavior without affecting fee estimates.

Diagram

sequenceDiagram
    participant UI as Transfer UI
    participant VM as BlocktankViewModel
    participant Provider as RefundAddressProvider
    participant Store as UserDefaults Cache
    participant LDK as LDK Wallet
    participant Core as Local Activity
    participant BT as Blocktank

    UI->>VM: createOrder(...)
    VM->>Provider: addressForOrder()
    Provider->>Store: load cached address/index
    alt Cached address exists
        Provider->>LDK: derive address at cached index
        Provider->>LDK: reveal receive addresses through index
        Provider->>Core: isAddressUsed(address)
        alt Address has recorded activity
            Provider->>LDK: allocate new native SegWit address
            Provider->>Store: save new address/index
        else Address is unused
            Provider-->>VM: reuse cached address
        end
    else No cached address
        Provider->>LDK: allocate new native SegWit address
        Provider->>Store: save address/index
    end
    VM->>BT: submit order with refundOnchainAddress
    BT-->>VM: channel order

Loading

[jvsena42]

One LOW inline, dev/QA-facing only. Not blocking.

No gating here — BlocktankViewModel.createOrder is the live Transfer-to-Spending path, so every release user who buys a channel now goes through BlocktankRefundAddressProvider.addressForOrder(). I reviewed it accordingly, and the backup changes as a shipped-feature concern rather than under the Paykit migration exemption.

Checked and clean:

• Derivation and ownership. allocate -> newAddressInfoForType(.nativeSegwit) -> ldk-node new_address_info_for_type on OnchainWalletAccount::account_zero — the seed wallet's BIP84 account 0 external keychain, never a watch-only or Trezor account. The reuse path re-derives at the cached index and requires both address and index equality, so a cached value from another seed, passphrase or network can never be handed to Blocktank. validate_derivation_index plus the <= Int32.max guard exclude hardened indexes. On the hardware-funded path the refund goes to the internal hot wallet, which the PR description states — funds stay user-controlled.
• Gap limit and scanning. Every reuse calls revealReceiveAddresses(to: index, .nativeSegwit) before isUsed, and allocation reveals too; ldk-node persists BDK changesets to VSS, so the revealed index survives restore. Even if that upload were lost, a stop-gap of 20 covers a single-index refund. nativeSegwit is force-monitored at node build, on settings restore, after post-restore prune, in syncMonitoredTypesFromNode, and setMonitoring refuses to disable it — and since set_primary_address_type demotes the previous primary into the monitored set, switching primary to taproot can't drop it either. Users who had addressTypesToMonitor = "taproot" under the old rule are handled by the static normaliser at node setup, which triggers a full scan for the newly added type.
• Address-type preference after an order. The refund address is persisted as {address, index} with an explicit .nativeSegwit derivation and never re-derived from selectedAddressType, so changing the preference later alters neither the stored value nor what is monitored.
• Ordering and partial failure. save(generated) — which re-reads and verifies — runs before orderClient.submit, with Task.checkCancellation() bracketing resolution. A crash between save and submit leaves an unused cached address the next order reuses. A crash after submit leaves a remote order whose refund address is already revealed in BDK and persisted to VSS, so a refund is still detected even with no local order record. Concurrent callers coalesce onto one in-flight task.
• Backup encoding, traced both directions. Newer backup decoded by an older build: the pre-PR AppCacheData.init(from:) uses an explicit CodingKeys enum with decodeIfPresent throughout, and JSONDecoder ignores keys absent from CodingKeys — so the new field is silently skipped and no other field is disturbed. The old build's next upload rewrites the envelope without it; after re-upgrade that decodes to nil, clear() runs, and a fresh address is allocated. The previously supplied address stays owned, revealed and monitored, so a late refund still lands — only reuse is lost. Older backup on a newer build is decodeIfPresent -> nil -> clear(), pinned by testOlderAppCacheWithoutRefundAddressDecodesAsNil. No version bump or BackupFieldMigration entry needed; the envelope stays version: 1 with an optional field, same as quickPayLedger and hwWalletNames before it.
• The new throws on getAppCacheData()/restoreAppCacheData() can only fire on a corrupt local Data blob or a failed UserDefaults read-back. Nothing writes that key except the store itself, so the abort paths in BackupService aren't reachable from any shipped input.
• Key material. Only a public bech32 address and a 32-bit derivation index are added — no xpub, no private material — through the unchanged seed-derived VSS encryption path.
• Trust boundaries. The refund address flows client -> server only; no new Blocktank-supplied value is rendered or used as a path or URL. Good catch removing the options: \(options) debug log that dumped the signed CreateOrderOptions.
• Live integration test and CI. integration-tests.yml adds the class to -only-testing, unit-tests.yml to -skip-testing. Env.network is hard-wired to .regtest under isUnitTest, the test XCTSkips off-regtest, and regtestDepositFunds/regtestMineBlocks throw unless regtest. No secrets introduced, endpoints are the existing staging ones, and the test only creates unpaid staging orders and takes a 10k-sat regtest deposit — it never signs or broadcasts a spend. wipeEntireKeychain() in setUp matches the existing AddressTypeIntegrationTests pattern and touches only the test-host keychain.

Cross-repo parity with synonymdev/bitkit-android#1235: I'd initially assumed Android had no backup coverage for this — that's wrong, and worth recording. Android's AppCacheData is the metadata envelope (BackupRepo builds it from cacheStore.data.first()), so the one-line field addition carries it with no payload edit; you needed explicit plumbing only because iOS assembles that struct field-by-field from UserDefaults. Wire format matches ({address, index}), and Android's index: Long vs your UInt32 is compatible since both clamp to 0...Int32.max before saving. Both decoders tolerate the field being absent or unknown, so metadata restores decode in either direction.

Derivation, ownership check, reveal, rotate-on-used, persist-before-submit, and the forced nativeSegwit monitoring invariant are all present and same-shaped on both platforms. Concurrency differs in shape only — Android serialises on a Mutex, you coalesce onto one Task.

[jvsena42]

Re-reviewed after the network-namespacing commit (9b6478de). Everything I raised earlier holds up at head — derivation/ownership fails closed, reveal-before-isUsed, persist-before-submit, forced nativeSegwit monitoring across node build / restore / prune / setMonitoring, the additive decodeIfPresent backup field, and the per-network metadata store. No LSP-supplied value is rendered or used in a path.

One real bug in the new migration code, inline. It's LOW for funds — the predicate fails closed, so a wrong-network address is never handed to Blocktank and the ownership check still guards reuse — but it's why Run Tests is red on this head, so it blocks merge either way.

Also checked and clean: the try inserted into the metadata-restore closure (BackupService.swift:258) can't realistically strand the rest of the restore, since save() only throws on a UserDefaults read-back mismatch and the invalidCache path needs a legacy value matching no HRP, which newAddressInfoForType(.nativeSegwit) cannot produce. addressTypeStateFromUserDefaults writing to UserDefaults (LightningService.swift:1306-1309) is fine for all three callers — _addressTypesToMonitor is @AppStorage so it observes the write. BlocktankRefundAddressStore.key is in appStateKeys and KVO-observed, so a save marks .metadata backup required. createOrder cancellation is safe: Task.checkCancellation() brackets addressForOrder() and the coalesced inner task is unstructured, so a cancelled caller can't leave a half-saved cache.

Doesn't apply to the Android twin (synonymdev/bitkit-android#1235) — each network is a separate application id there and the diff has no legacy-key migration.

addressed - re-requested review

[jvsena42]

The network-classification finding is closed, and Run Tests is green again.

matchingNetworks(for:) is now the explicit HRP classifier at BlocktankRefundAddressProvider.swift:118-130 — bc1q → [.bitcoin], bcrt1q → [.regtest], tb1q → [.testnet, .signet], else [] — and the provider no longer references the shared matchesAddressFormat, so PrivatePaykitAddressReservationStore and SettingsViewModel keep their loose semantics. Both consumers work now: load():65 sees [.regtest] == [.regtest] and migrates, saving the scoped key at :67 before removing the legacy one at :68; clear():113 removes a bcrt1q legacy value on a regtest store and leaves it on a bitcoin store. testnet/signet correctly never migrate.

It wasn't papered over: testLegacyCacheMigratesOnlyWhenPrefixIdentifiesNetwork asserts the stronger case for both .bitcoin and .regtest, and the three new tests pin the remaining branches. One assertion was dropped in the rewrite — a bcrt1q legacy value loaded on a .bitcoin store returns nil and writes no scoped key — but that path is trivially correct by reading, so it's a nit at most.

On the failing e2e-tests-staging - multi_address_2_regtest: most likely unrelated to this PR, though I can't prove it from the artifacts.

Attempt 1 was Appium failing to create a session (infra). Attempts 2 and 3 were identical: all four address types funded, order created, confirm screen reached, GRAB swiped, then transferSavingsToSpending waited ~6.5 min / 11 mined blocks and ~TransferSuccess never appeared. Both screenshots show SettingUpView parked on "Processing Payment" — updateOrder returning 0 because order.state2 == .created, i.e. the LSP never registered the payment.

Why I lean unrelated:

• Run 34378599511 on this same branch at b4776962 passed this shard, swipe → TransferSuccess with no wait loop. git diff b4776962..9ecf5787 is formatting only — brace placement and try placement in tests — so functionally identical code completed the whole refund-address → pay → channel → close flow.
• The only PR code on this path is addressForOrder() inside createOrder, which had already succeeded; the fee screen was displayed and the swipe happened. payOrder → lightningService.send is untouched, and a throw there would have shown an error rather than navigating to SettingUpView.
• The shard's baseline is poor: 12 multi_address_2_regtest failures in the last 120 runs, the passing run above itself needed 3 attempts, and codex/713-paykit-interop failed the same shard 35 minutes later. Staging regtest looked unhealthy in that window.

The honest caveat: this exact TransferSuccess signature appears in no other multi_address_2 failure in that 120-run sample, and simulator.log doesn't capture the app's Logger, so I couldn't confirm the funding tx was broadcast. Suggest a re-run — and if it recurs at this step on this branch, the evidence needed is the app's own log file (order id + LN setup step lines), not more e2e retries.

Parity: both twins now carry the skip-used loop with the same shape, so nothing to mirror.

[piotr-iohk]

Staging E2E reds on transfer-to-spending are Blocktank staging (payment confirmed, order stays created) — not this PR. Tracked in #bitkit-backend.