fix: add lsp refund address - #1235
Conversation
This comment has been minimized.
This comment has been minimized.
jvsena42
left a comment
There was a problem hiding this comment.
No findings. Clean at the HIGH/MEDIUM bar.
Reviewed at head f9b1f6b against full files, the ldk-node fork (crates/bdk-wallet-aggregate), and the iOS twin. No gating on the core path — BlocktankRepo.createOrder is reached from Transfer -> Spending in release builds, so I treated this as live user-facing code and kept upgrade/restore paths in scope.
Checked and clean:
- Derivation and ownership.
newAddressInfoForType(P2WPKH)->node.onchainPayment().newAddressInfoForType(NATIVE_SEGWIT)-> forkget_new_address_info_for_account(OnchainWalletAccount::account_zero(P2WPKH))->reveal_next_address(External)+wallet.persist(persister). So it's account-0 external P2WPKH of the node's own seed, and the reveal is persisted to the VSS-backed KV store before the address is returned. Not a watch-only or Trezor account — those go throughaccount_index != 0. - Reuse check. The re-derive at
:370-372usespeek_addresson the same account-0 key and requires both index and address to match, so a foreign pointer can't pass. The subsequent re-reveal through the index means BDK includes the script in sync even if the cursor were somehow lower. - The P2WPKH wallet is guaranteed loaded when these calls run: it's forced into settings before
setAddressTypesToMonitor,set_primary_address_typedemotes the old primary to monitored,validateDisableMonitoringrejects P2WPKH,syncMonitoredTypesFromNodere-adds it, and the post-restore prune skips it.createOrderalso gates onnodeId != nullbefore touching the mutex. - Address-type preference after an order. The pointer is persisted as
(address, index)and always re-derived against P2WPKH regardless ofselectedAddressType; switching primary to Taproot keeps the P2WPKH account loaded, and rollback also passes throughSettingsStore.update, which re-applies the invariant. - Reuse and privacy. The receive flow and the refund allocation both advance the same persisted BDK external cursor, so the refund address is never handed out as a receive address. Paykit private reservations use the same cursor. Reuse across orders is by design and rotates on
isAddressUsed. - Network correctness. Flavors have distinct application ids (separate
app_cache.json),vssStoreIdPrefixscopes backups per network, and the derive-and-compare would reject atb1/bc1mismatch anyway. Worth noting this is exactly why the cross-network cache issue I filed on the iOS twin can't happen here. - Persistence ordering.
getBlocktankRefundAddress()->cacheStore.update(DataStoreupdateDatacompletes the disk write before returning) -> only thencoreService.blocktank.newOrder. A crash or cancellation before submission leaves a persisted unused pointer that's reused next time; a failednewOrderreuses it too, which the repeated-orders test pins.ensureActive()plusrunSuspendCatchingpreserve cancellation, and concurrent calls serialise onrefundAddressMutex. Estimates never allocate. - Backup and restore. The metadata envelope embeds the whole cache, so adding
blocktankRefundAddresstoAppCacheDatacarries it in every metadata backup with no payload edit — which is why removing the duplicate collector was right. Restore'sresetBip21()clears only bip21/bolt11/onchainAddress, so the pointer survives. Even without the pointer, the refund lands on a revealed, persisted account-0 P2WPKH index that a restored node syncs, with P2WPKH monitoring forced on at build — I found no un-sweepable path. - Trust boundaries and key material. No Blocktank-supplied value is consumed by the new code; the address flows client -> server only. The order log line no longer dumps the full
options(which included the signature). Only a public address and an index are persisted and backed up.
Cross-repo parity with synonymdev/bitkit-ios#732 — the backup coverage is present here, just differently shaped, which corrects my initial assumption that Android had none. iOS needed explicit BackupPayloads / SettingsBackupConfig / BackupService edits because its cache keys live in UserDefaults behind an allow-list; yours falls out of the whole-cache envelope. JSON shape is identical on both (blocktankRefundAddress: {address, index}), Android tolerates unknown keys and range-checks the index, and settings backups from either platform pass through withRequiredNativeSegwitMonitoring() — so restores decode in both directions. Ownership-mismatch handling matches iOS exactly.
One residual I couldn't turn into a finding, recorded in case it's useful: if the BDK reveal cursor were ever lost while the metadata pointer survived, newAddress() could hand out index N before the next order re-reveals it. The cursor lives in the VSS-backed node KV store and I couldn't construct a restore path where that happens, so there's nothing to fix — noting it only because it's the one shape that would break the no-reuse property.
Fixes #1229
Counterpart: synonymdev/bitkit-ios#732
This PR:
Description
The refund address is allocated once through LDK, stored only after node persistence succeeds, and reused across unpaid or failed order attempts. Estimates do not allocate addresses. Concurrent order requests share the same unused destination, while lookup, reveal, ownership, cache-write, and cancellation failures stop before Blocktank submission.
Preview
QA Notes
Manual Tests
ae58765a-f228-4452-ac42-b960b34c855a; pointer index2.2across an app restart.faa81e71-a9ca-4de9-860a-fabde5b4ec26.POST /channelsused the internal Bitkit wallet pointer at index1, distinct from the external Trezor address.55b1d296-82b9-4545-bf91-981a22d1f6b2;orderExpiresAt=2026-09-10T16:38:44.238Z.activity.db; the next order rotates the pointer exactly once.68875bfdbf1822b13e2452b3d097ff51015fe50bb4d19794361449954e2d137d; rotated order9c5a0405-a65b-4dba-974b-696ba82d0089.Automated Checks
BlocktankRepoTest.kt: covers one persisted allocation, unpaid and failed reuse across restarts, payment-driven rotation, concurrency, allocation-free estimates, ownership validation, reveal, persistence, failure, and cancellation gates.AppCacheDataTest.ktandBackupRepoTest.kt: cover the optional cross-platform JSON shape and metadata backup/restore pointer.LightningRepoTest.kt,SettingsDataTest.kt, andAddressTypePreferenceViewModelTest.kt: cover native SegWit normalization, disable protection, post-restore retention, and localized feedback.POST /channelsrequest.