fix: decouple payout authorization from sync - #794
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
No findings.
Checked (ldk-node 0.7.0-rc.66):
setWatchOnlyAccountTrackingstill adds the account if absent, reveals up towatchOnlyAccountHighestPreRevealedAddressIndex, and removes the account and rethrows if the reveal fails. Only the post-revealsyncWallets()is gone.- Background sync runs every 10s (
Env.swift) and picks up accounts added at runtime. A derived account's first sync in a process is a full scan, and a failed scan is retried on the next tick. WatchOnlyAccountClaimCodec.encodesends only the xpub, so no address that depends on sync state reaches the counterparty.beginSetupAuthorizationpersists Authorizing with tracking enabled underlifecycleCoordinator.withLock.reconcileTrackingtakes the same lock and only removes accounts that are managed but not desired. After a crash mid-approval, the account reloads on the next start.- The failure paths in
PubkyService.approveAuthRequestare unchanged: cancel unless the companion approval was delivered, thenmarkActive. - Reachable only with a stored Pubky identity. v2.5.0 already has the sync, so upgrading users only gain the fix.
Parity with synonymdev/bitkit-android#1338: same change. The only difference is that this side carries no test update; the existing tests mock tracking and never covered the sync.
pwltr
left a comment
pwltr
left a comment
There was a problem hiding this comment.
Reviewed 186c99e1de204f044632e10c662cd0bf98f77627, tracing payout authorization through account tracking, persisted authorization state, rollback and subsequent synchronization.
No actionable code findings. Local account registration and address revelation remain in place; the full-wallet syncWallets() call is removed from authorization preparation. The journey's earlier foreground-sync coverage concern is addressed in the current source.
Executed CI checks, including local E2E, pass. The author reports 50 focused simulator tests passing, but those tests mock account tracking and do not reproduce the native Electrum timeout.
The complete revised wallet-leg.xml journey and a deterministic Electrum-only outage have not been replayed on this head. Device verification of authorization during that outage and later periodic payout detection remains pending.
There was a problem hiding this comment.
QA reviewed on 186c99e.
QA review
Reviewed the full PR diff against its merge base, at 186c99e.
No new actionable code findings.
beginSetupAuthorization still registers the watch-only account and reveals receive indexes through 999 before the claim is sent. syncWallets() is no longer called on that path, so an Electrum timeout there would no longer remove the new account. A failed address reveal still removes an account this step just added. WatchOnlyAccountClaimCodec.encode sends the account index and xpub only, so the claim does not depend on sync state. In ldk-node 0.7.0-rc.66, background on-chain sync (interval 10s) reads derived accounts on each tick. A derived account's first successful scan is a full scan with the configured stop gap of 1000; a failed scan leaves it unmarked, so a later tick retries it. Aggregate on-chain balance includes that account, and the wallet refreshes on sync completion, balance change, and on-chain receive events, so savings (ActivitySavings) and home activity update after that sync rather than during authorization. Foreground wallet.sync() runs when the scene becomes active and on pull-to-refresh, not on a timer while the app stays in the foreground. The updated wallet-leg.xml keeps the seller on Home and awake until the payout appears, which addresses the earlier foreground-sync coverage note.
Unchanged, and out of scope in this PR: startup and foreground LightningService.sync() still call syncWallets() after reconciliation returns. That call is outside the add/rollback path, so an Electrum timeout there fails the sync and leaves the new account in place. It can still delay a concurrent authorization waiting on the LDK queue.
The same sync removal and wallet-leg.xml update are in bitkit-android#1338 at b3fb622. iOS keeps id where Android uses testTag. Android also asserts in unit tests that syncWallets() is not called; these iOS tests mock tracking above LightningService and do not execute this path.
CI tests, integration tests, and local e2e on this revision passed. The PR reports 50 focused simulator tests passing. I inspected WatchOnlyAccountServiceTests and did not re-run them; they mock account tracking and do not reproduce the native Electrum timeout. The runtime checks are the updated journeys/pubky-marketplace/wallet-leg.xml and the Electrum-failure authorization case in the PR. No Appium spec covers this marketplace leg.
Device testing: not performed in this review.
Ready for device testing.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 5 files.
Equivalent to synonymdev/bitkit-android#1338.
Audit:
Awaits QA.
Coverage:
QA: waits for the other reviewers' approval, or @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
|
@ovi-reviewer test |
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 5 files.
Pair PR synonymdev/bitkit-android#1338: equivalent.
Audit:
Awaits QA.
QA:
Tested on two iOS 26.5 simulators (iPhone 17 Pro), regtest
Test 1
evidence
1.mp4 |
 |  |  |  |
Test J1
evidence
J1.mp4 |
 |  |  |
Warning
The seller account lacked the proof needed for J1, and a same-chain marketplace purchase could not be completed. The Electrum settings screen would not save an unreachable host, leaving the selective outage and payout recovery in Test 1 unverified.
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 5 files.
Pair PR synonymdev/bitkit-android#1338: equivalent.
Audit:
Awaits QA.
QA:
Tested on two iOS 26.5 simulators (iPhone 17 Pro), regtest
Test 1
evidence
1.mp4 |
 |  |  |  |  |
log
ERROR: Incremental sync of on-chain wallet failed: Connection refused (os error 61)
INFO: Sync completed: onchainWallet at height 202513
Test J1
evidence
J1.mp4 |
 |  |  |  |  |
log
INFO: Registered derived on-chain wallet account OnchainWalletAccount { address_type: NativeSegwit, account_index: 1 }
DEBUG: Hiding sheet pubkyAuthApproval reason: PubkyAuthApprovalSheet.swift:222 successContent
Note
“Authorization Successful” remained visible for 30 seconds while Electrum connections failed. On-chain sync completed after the connection was restored.
Warning
No payout was produced on the app's regtest chain, and the fixture provided no seller xpub or proof of spending authority. Payout receipt and the remaining marketplace purchase steps remain unverified.
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
|
Verified locally (iOS seller + Android buyer, payments-env regtest) Cross-device marketplace purchase against this PR’s payout path:
Confirms periodic payout detection after authorization, without needing a sync during auth prep. Counterpart run also exercised Android #1338 as the buyer. |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA pass — periodic payout path verified locally (iOS seller / Android buyer) on payments-env regtest.
5 participants
Closes #810
This PR prevents a wallet-sync timeout from aborting payout authorization. Bitkit registers the watch-only account and prepares its receiving addresses before sending authorization; existing periodic synchronization retrieves its transaction history.
Counterpart: Android PR.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A — no visual changes.
QA Notes
Journeys
wallet-leg.xml— authorization succeeds and the seller sees the payout while continuously active on its own device, without a foreground transition or manual refresh. Capture lifecycle and sync logs to verify the interval.Manual Tests
Observed on staging: the original build timed out syncing an existing account and removed the newly added account. With this fix, local registration/revelation took 0.65 seconds and native authorization succeeded. The subsequent server reconnect still returned HTTP 422. A deterministic Electrum outage was not re-injected, and the complete updated marketplace journey has not been replayed on this head.
Automated Checks
WatchOnlyAccountServiceTests.swiftandPubkyAuthApprovalSheetTests.swift— 50 tests passed using an isolated iOS simulator with the E2E network/regtest build; these mock account tracking, so they do not directly reproduce the native Electrum timeout.