fix: decouple payout authorization from sync - #1338
ben-kaufman wants to merge 2 commits into
Conversation
 |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
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):
- The only change is dropping
node.syncWallets()after the reveal insetAccountTracking. The add, reveal and remove-on-reveal-failure sequence, the lock scope and the error mapping are unchanged. - The removed sync only moved the account's first full scan earlier. Address index state does not depend on it: revealed indices are re-applied by
reconcileWatchOnlyAccountson every start and refresh. - Background sync (
onchainWalletSyncIntervalSecs = 10) readsadditional_wallet_accounts()on every tick. A derived account's first sync in a process is a full scan with the 1000 stop gap. A failure leaves it unmarked, so the next tick retries it. Payouts are picked up within one tick. - The claim payload is version, account index, address type and xpub. No address is handed to the counterparty, so local sync state cannot cause address reuse or an untracked payout address.
- Reconciliation cannot remove the new account:
beginAuthorizationholdslifecycleCoordinator.withLockwhile it adds the account and persistsAuthorizingwith tracking enabled, and Authorizing+tracking counts as desired. After a crash mid-approval, the account reloads, is re-revealed and is scanned on the first tick. - This change removes a rollback trigger and adds none.
apply_wallet_sync_resultsfolds any account's error into the overall result, which is how a timeout on an existing account rolled back the fresh one. - Upgrade: no persisted format change, so v2.5.0 users only gain the fix. The flow requires a Pubky identity.
Parity with synonymdev/bitkit-ios#794: same change, same background sync config and same lock/reconcile guarantees.
Out of scope and pre-existing: the startup reconcileWatchOnlyAccounts() still runs syncWallets() inside the lifecycle mutex, so a startup Electrum timeout can still delay a concurrent beginAuthorization.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA reviewed on b3fb622.
QA review
Reviewed the full PR diff against its merge base, at b3fb622.
No new actionable code findings.
beginAuthorization still adds the watch-only account and reveals receive indexes 0...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. 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 and wallet events include that account, so savings (ActivitySavings) and home activity update after that sync rather than during authorization.
Unchanged, and out of scope in this PR: startup reconcileWatchOnlyAccounts() still calls syncWallets() while holding the lifecycle lock, so an Electrum timeout there can still delay a concurrent authorization.
The same sync removal and wallet-leg.xml update are in bitkit-ios#794 at 186c99e. iOS keeps id where Android uses testTag.
CI build on this revision passed, including testDevDebugUnitTest. I inspected WatchOnlyAccountRepoTest and WatchOnlyAccountLifecycleCoordinatorTest and did not re-run them. 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 7 files.
Pair PR synonymdev/bitkit-ios#794: equivalent.
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 7 files.
Pair PR synonymdev/bitkit-ios#794: equivalent.
Audit:
Awaits QA.
QA:
Tested on two Android 16 emulators (Pixel 10 Pro)
Test 1
evidence
1.mp4 |
 |
log
09-29 00:34:38.973 E APP [PubkyAuthApprovalViewModel.kt:67] Failed to parse auth request [AppError='code=protocol_error, context=only Pubky grant auth URLs are supported']
09-29 00:38:04.534 E APP [PubkyAuthApprovalViewModel.kt:67] Failed to parse auth request [AppError='code=protocol_error, context=invalid Pubky auth URL: Missing query parameter cid']
Test J1
evidence
J1.mp4 |
 |
log
Error: Activity not started, unable to resolve Intent { act=android.intent.action.VIEW dat=pubkyauth://signin/... }
09-29 00:34:37.938 D APP [AppViewModel.kt:5499] Received deeplink 'pubkyauth://signin'
09-29 00:34:38.973 E APP [PubkyAuthApprovalViewModel.kt:67] Failed to parse auth request [AppError='code=protocol_error, context=only Pubky grant auth URLs are supported']
Warning
The pinned Paykit Server sends a signin URL this app cannot authorize. Watch-only setup, payout, and periodic seller sync are unverified. Both tests need a fixture that returns grant URLs.
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.
Verdict: ✅ Approve
Retest for the review: journey J1 failed on our test setup, not the app; Test 1 passes now.
QA:
Tested on two Android 16 emulators (Pixel 10 Pro); iOS 26.5 simulator (iPhone 17 Pro), regtest
Test 1 ✅ passed
evidence
1.mp4 |
 |  |  |  |  |  |
Test J1
evidence
J1.mp4 |
 |  |  |  |  |  |  |  |  |  |  |  |  |  |
log
2026-09-29 00:03:14.291 DEBUG [AppViewModel.kt:1333] LDK-node event received in AppViewModel: {"type":"SyncCompleted","syncType":"ONCHAIN_WALLET","syncedBlockHeight":110} - AppViewModel
2026-09-29 00:03:15.031 INFO [CoreService.kt:721] Synced 1 payments successfully, 0 failed - ActivityService
Warning
The Locks fixture cannot create a marketplace purchase: its frontend session returns HTTP 401 and its connect flow uses legacy Pubky sign-in. The buyer Payment Request and marketplace completion remain unverified. A separate active-seller payout check confirmed periodic synchronization.
Tip
Worth a journey
Test 1
- Create a fresh wallet and a Bitkit Pubky identity.
- Enable Paykit UI in Dev Settings.
- Open a fresh payout setup grant while Electrum is unavailable.
- Approve the watch-only consent and authorize the Paykit scopes.
- Verify Authorization Successful.
- Restore Electrum and receive a payout to the claimed account.
- Verify Savings, received activity, confirmation, and transaction id.
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)
This branch has not been deployed
4 participants
Closes #1354
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: iOS 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
The timeout regression failed against the original code and passes with this fix. Android staging UI replay and the complete updated marketplace journey remain unverified.
Automated Checks
WatchOnlyAccountRepoTest.kt— adds authorization success despite a sync timeout, retains rollback when address revelation fails, and keeps tracking restoration independent of network sync.WatchOnlyAccountLifecycleCoordinatorTest.kt— verifies reconciliation cannot remove the account while local tracking and authorization state are being established.