Skip to content

fix: add lsp refund address - #732

Open
ovitrif wants to merge 3 commits into
masterfrom
codex/728-lsp-refund-address
Open

fix: add lsp refund address#732
ovitrif wants to merge 3 commits into
masterfrom
codex/728-lsp-refund-address

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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

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.

@ovitrif
ovitrif marked this pull request as ready for review September 8, 2026 17:14
@ovitrif
ovitrif requested a review from pwltr September 8, 2026 17:14
@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown

RetriggerView in GreptileConfidence 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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Bitkit/Services/BlocktankRefundAddressProvider.swift Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: pass refundOnchainAddress to createOrder

2 participants