fix: add lsp refund address - #732
Conversation
jvsena42
left a comment
There was a problem hiding this comment.
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-nodenew_address_info_for_typeonOnchainWalletAccount::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_indexplus the<= Int32.maxguard 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)beforeisUsed, 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, insyncMonitoredTypesFromNode, andsetMonitoringrefuses to disable it — and sinceset_primary_address_typedemotes the previous primary into the monitored set, switching primary to taproot can't drop it either. Users who hadaddressTypesToMonitor = "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.nativeSegwitderivation and never re-derived fromselectedAddressType, 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 beforeorderClient.submit, withTask.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 explicitCodingKeysenum withdecodeIfPresentthroughout, andJSONDecoderignores keys absent fromCodingKeys— 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 isdecodeIfPresent-> nil ->clear(), pinned bytestOlderAppCacheWithoutRefundAddressDecodesAsNil. No version bump orBackupFieldMigrationentry needed; the envelope staysversion: 1with an optional field, same asquickPayLedgerandhwWalletNamesbefore it. - The new
throwsongetAppCacheData()/restoreAppCacheData()can only fire on a corrupt localDatablob or a failed UserDefaults read-back. Nothing writes that key except the store itself, so the abort paths inBackupServicearen'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 signedCreateOrderOptions. - Live integration test and CI.
integration-tests.ymladds the class to-only-testing,unit-tests.ymlto-skip-testing.Env.networkis hard-wired to.regtestunderisUnitTest, the testXCTSkips off-regtest, andregtestDepositFunds/regtestMineBlocksthrow 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()insetUpmatches the existingAddressTypeIntegrationTestspattern 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.
Fixes #728
Description
This PR:
Linked Issues/Tasks
Preview
QA Notes
Manual Tests
9cac70da-71fc-4c63-8da4-ddeb2d6518f6with its native SegWit refund address, the paired Trezor signed funding transaction66127374a905d1437eb761101c567c27bba4282094cc482363e09de10e2a7fd1, and the app reached Transfer Successful.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.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.99581b96ecfa768de10fc8518e162cb3867907a8fd0a295f81477c72a9c584f9.9cac70da-71fc-4c63-8da4-ddeb2d6518f6reachedexecuted/open; its internal refund destination was native SegWit index 2, independent of the Trezor funding account.Bitkit-PR728-Refundsimulator.node scripts/validate-translations.js: passed.