Repository navigation
fix: keep incoming request refreshes together - #1645
ben-kaufman wants to merge 1 commit into
Conversation
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryKeeps automatic incoming-request receipt and the following fresh read inside one queued SDK operation.
No actionable issues found. Tests were inspected, not run during this review. DiagramsequenceDiagram
participant App as Automatic refresh
participant Repo as Request repository
participant SDK as SDK service
participant Queue as Other queued work
App->>Repo: Refresh inbox or full state
opt Full refresh
Repo->>SDK: Drain pending messages
end
Repo->>SDK: Receive and observe together
activate SDK
SDK->>SDK: Check admission and identity
SDK->>SDK: Receive messages and track backup changes
SDK->>SDK: Report intake failures
SDK->>SDK: Recheck admission, then read state
SDK-->>Repo: Fresh requests and linked peers
deactivate SDK
Queue->>SDK: Next queued operation
Repo->>Repo: Check identity, proof, and action versions
Repo-->>App: Publish current requests
Reviews (1) · Last reviewed commit: "fix: keep incoming request refreshes tog..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Runs the automatic Background inbox receive and the fresh observeState read under one operationLock hold, and moves the two contact-driven forced refreshes to Background priority so a payment review defers them. Reviewed 8564ba2, full tier, against the stack base ce4db93 (#1639).
What I checked, and 6 candidates I ruled out
Read in full: the diffs of PaykitSdkService.kt, PaykitPaymentRequestRepo.kt, AppViewModel.kt, plus PaykitSdkOperationLock.kt, withPaykitBackupStateTracking, refreshPaykitKey, the old observeState and receivePrivateMessagesFromLinkedPeers
Call sites traced: refresh(mode, forceFresh, messagePriority, priority) and every refreshIncomingPaykitPaymentRequests caller in AppViewModel, to confirm only non-forced refreshes with Background on both priorities take the combined path
Not built here: Bitkit Android branches are not built on this box, so this is reasoned from the code and the tests in the diff. At review time build, build-local and lint were still pending
Ruled out
- Lock dropped between receive and observe:
withoutLockinsidewithLockonly re-checks wipe admission, it never releases the held lock, so queued work cannot run between the two calls. - Weaker identity check than the old
observeState: the old path matched the observation againstcachedPaykitKey; the new one matcheskey.publicKeyandobservation.publicKeyagainstexpectedIdentity, compareskeyGeneration, and checkscachedPaykitKey == keybefore observing, which is equivalent. - Wipe overtaking the held lock:
withWalletWipebumps the generation before it waits, so thewithoutLockadmission before observe throws the wipe error. - Review opening after receive loses the request: observe throws
OperationInProgress,completedRefreshGenerationwas already reset to -1 before the fetch, so the next poll is not skipped as fresh, and the received messages are already in SDK state. - Cancellation after receive skews backup accounting:
completeSdkCallfails after the SDK call returned, the trackingfinallyseessucceeded = falseand marks the backup dirty once, as on the old path. - Deferred contact-sync refresh drops presentation: on
OperationInProgressthe contact-sync and endpoint paths still runrefreshPaymentRequestTargets(force = true), and the next 10 s INBOX poll presents throughpresentNextIncomingPaykitPaymentRequest, covered byautomatic contact refresh defers without losing the next poll.
Merge confidence: 4/5, not built here and CI build and lint still pending; the ordering is covered by PaykitSdkServiceTest.kt and PaykitPaymentRequestRepoTest.kt.
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 8564ba2: Two Android API 35 wallets on staging Pubky and regtest. Videos run at 2× speed, with payer left and requester right.
Test 1: automatic payment request presentation ✅ Preparation cancellation, retry, sheet ownership and insufficient-funds rejection passed on 8564ba2 plus a timing hook.
I cancelled the 21,000-sat request while preparation was held, then released it: it stayed pending without reopening. Explicit retry showed the same disabled preparation sheet, then became ready in place. Receive retained ownership while a 5,000-sat request arrived; closing it presented that request. The drawer also retained ownership. Pending rows stayed unique, and opening/closing Receive did not reopen reviewed requests. A 1,000,000-sat request failed with insufficient Savings on both automatic presentation and explicit retry, without enabling payment or sending funds.
J1.2x.mp4
Test hook and deciding logs
A debug-only gate paused incoming preparation before real native resolution until explicit release. The callback hook present in this binary remained unarmed. The patch was not pushed. SDK results were not mocked.
diff --git a/app/src/main/java/to/bitkit/ui/MainActivity.kt b/app/src/main/java/to/bitkit/ui/MainActivity.kt
index 2f2b47d66..e6452d208 100644
--- a/app/src/main/java/to/bitkit/ui/MainActivity.kt
+++ b/app/src/main/java/to/bitkit/ui/MainActivity.kt
@@ -288,6 +288,10 @@ class MainActivity : FragmentActivity() {
}
private fun handleLaunchIntent(intent: Intent) {
+ if (to.bitkit.BuildConfig.DEBUG && intent.getBooleanExtra("review_hook_refresh", false)) { // test hook
+ intent.removeExtra("review_hook_refresh") // test hook
+ appViewModel.reviewHookRefreshContacts() // test hook
+ } // test hook
if (intent.getBooleanExtra(EXTRA_PAYKIT_SUBSCRIPTION_PAYMENT_DUE, false)) {
intent.removeExtra(EXTRA_PAYKIT_SUBSCRIPTION_PAYMENT_DUE)
appViewModel.onPaykitSubscriptionNotificationTapped(
diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
index 9655350fd..b439e8c3b 100644
--- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
+++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
@@ -914,6 +914,10 @@ class AppViewModel @Inject constructor(
}
}
+ fun reviewHookRefreshContacts() { // test hook
+ if (BuildConfig.DEBUG) viewModelScope.launch { pubkyRepo.refreshContacts() } // test hook
+ } // test hook
+
private suspend fun synchronizePaykitContacts(state: PaykitContactSyncState) {
if (!state.isPaykitEnabled || state.publicKey == null) {
isPaymentRequestIdentityActivating = true
@@ -943,6 +947,13 @@ class AppViewModel @Inject constructor(
)
}
+ if (BuildConfig.DEBUG && java.io.File(context.filesDir, "review-hook-arm").exists()) { // test hook
+ java.io.File(context.filesDir, "review-hook-arm").delete() // test hook
+ Logger.info("Held saved-contact callback before activation and request refresh", context = "ReviewHook") // test hook
+ while (!java.io.File(context.filesDir, "review-hook-release").exists()) delay(100) // test hook
+ java.io.File(context.filesDir, "review-hook-release").delete() // test hook
+ Logger.info("Released saved-contact callback", context = "ReviewHook") // test hook
+ } // test hook
isPaymentRequestIdentityActivating = true
val requestRefresh = try {
paykitPaymentRequestRepo.activate(state.publicKey)
@@ -956,6 +967,7 @@ class AppViewModel @Inject constructor(
refreshPriority = Priority.Background,
onlyWhileActive = true,
)
+ if (BuildConfig.DEBUG) Logger.info("Saved-contact refresh deferred='${result.exceptionOrNull() == PaykitPaymentRequestError.OperationInProgress}' pending='${paykitPaymentRequestRepo.pendingRequests.value.size}'", context = "ReviewHook") // test hook
if (!isPaymentRequestPollingStopped.value) refreshPaymentRequestTargets(force = true)
result
} finally {
@@ -1600,6 +1612,13 @@ class AppViewModel @Inject constructor(
}
if (reopenRetainedHardwarePayment(request, generation, preparation)) return true
val started = timeSource.markNow()
+ if (BuildConfig.DEBUG && java.io.File(context.filesDir, "prepare-hook-arm").exists()) { // test hook
+ java.io.File(context.filesDir, "prepare-hook-arm").delete() // test hook
+ Logger.info("Held incoming preparation before native resolution", context = "ReviewHook") // test hook
+ while (!java.io.File(context.filesDir, "prepare-hook-release").exists()) delay(100) // test hook
+ java.io.File(context.filesDir, "prepare-hook-release").delete() // test hook
+ Logger.info("Released incoming preparation", context = "ReviewHook") // test hook
+ } // test hook
val presentationResult = privatePaykitRepo.beginPaymentRequest(request)
val resolvedAfter = started.elapsedNow()
val result = finishIncomingPaymentRequestPreparation(request, generation, preparation, presentationResult)10-21 05:33:41.310 8856 8856 I APP : 2026-10-21 03:33:41.307 INFO [AppViewModel.kt:1617] Held incoming preparation before native resolution - ReviewHook
10-21 05:33:57.714 8856 8856 I APP : 2026-10-21 03:33:57.714 INFO [AppViewModel.kt:1620] Released incoming preparation - ReviewHook
10-21 05:34:30.090 8856 8856 I APP : 2026-10-21 03:34:30.090 INFO [AppViewModel.kt:1617] Held incoming preparation before native resolution - ReviewHook
10-21 05:34:47.823 8856 8856 I APP : 2026-10-21 03:34:47.823 INFO [AppViewModel.kt:1620] Released incoming preparation - ReviewHook
10-21 05:39:12.129 8856 8856 I APP : 2026-10-21 03:39:12.129 INFO [AppViewModel.kt:1617] Held incoming preparation before native resolution - ReviewHook
10-21 05:39:30.474 8856 8856 I APP : 2026-10-21 03:39:30.474 INFO [AppViewModel.kt:1620] Released incoming preparation - ReviewHook
Test 2: payment review refresh deferral ✅ The original 1,000-sat review survived a second request and cancellation, then paid exactly once on pristine 8564ba2.
The Savings confirmation and its 141-sat fee stayed intact for over 30 seconds and while a second 2,000-sat request arrived. I swiped, waited at payment PIN for over 30 seconds, then cancelled PIN and closed the sheet: Activity still showed only the funding credit. Ordinary foreground polling exposed the second request without restarting. I reopened the first, backgrounded/resumed and unlocked, verified the original amount and fee, then authorized one payment. “Bitcoin Sent” appeared; Activity contained one outgoing payment. The requester received 1,000 sats, and the original request recorded “Proof submitted.” The second remained “Waiting for payment.”
Both devices' LDK logs identify the same transaction, e0a32671329b7f7a9fb4202bfcdc6191b0d86d822e3268e1cbd72c0f7d15bbf7: recipient output 1,000 sats, change 98,859 sats and fee 141 sats. No hook was installed for this case.
J2.2x.mp4
Test 3: delayed saved-contact callback during review ✅ The actual Background refresh deferred, retained the original review, and later polling exposed the new request on 8564ba2 plus a callback hook.
With two warm pending rows, I held the saved-contact callback before activation and opened the original 21,000-sat confirmation. The requester sent a new 3,000-sat request. Releasing the callback left the original amount and ready swipe intact; the log confirmed the actual refresh returned the typed OperationInProgress deferral with both warm rows retained. After closing review unpaid, ordinary foreground synchronization presented the new 3,000-sat request without restarting. All three pending amounts remained correct; balance and Activity showed no outgoing payment.
M1.2x.mp4
Test hook and deciding logs
A debug-only entry point invoked the real saved-contact callback with the current repository snapshot and held it before activation until explicit release. The preparation gate remained unarmed. The patch was not pushed. SDK results were not mocked.
diff --git a/app/src/main/java/to/bitkit/ui/MainActivity.kt b/app/src/main/java/to/bitkit/ui/MainActivity.kt
index 2f2b47d66..e6452d208 100644
--- a/app/src/main/java/to/bitkit/ui/MainActivity.kt
+++ b/app/src/main/java/to/bitkit/ui/MainActivity.kt
@@ -288,6 +288,10 @@ class MainActivity : FragmentActivity() {
}
private fun handleLaunchIntent(intent: Intent) {
+ if (to.bitkit.BuildConfig.DEBUG && intent.getBooleanExtra("review_hook_refresh", false)) { // test hook
+ intent.removeExtra("review_hook_refresh") // test hook
+ appViewModel.reviewHookRefreshContacts() // test hook
+ } // test hook
if (intent.getBooleanExtra(EXTRA_PAYKIT_SUBSCRIPTION_PAYMENT_DUE, false)) {
intent.removeExtra(EXTRA_PAYKIT_SUBSCRIPTION_PAYMENT_DUE)
appViewModel.onPaykitSubscriptionNotificationTapped(
diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
index 9655350fd..01a4b4b1b 100644
--- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
+++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
@@ -914,6 +914,17 @@ class AppViewModel @Inject constructor(
}
}
+ fun reviewHookRefreshContacts() { // test hook
+ if (BuildConfig.DEBUG) viewModelScope.launch { // test hook
+ synchronizePaykitContacts(PaykitContactSyncState( // test hook
+ publicKey = pubkyRepo.publicKey.value, // test hook
+ contactKeys = pubkyRepo.contacts.value.map { it.publicKey }.toSet(), // test hook
+ contactsLoaded = pubkyRepo.contactsLoadVersion.value > 0L, // test hook
+ isPaykitEnabled = isPaykitEnabled.value, // test hook
+ )) // test hook
+ } // test hook
+ } // test hook
+
private suspend fun synchronizePaykitContacts(state: PaykitContactSyncState) {
if (!state.isPaykitEnabled || state.publicKey == null) {
isPaymentRequestIdentityActivating = true
@@ -943,6 +954,13 @@ class AppViewModel @Inject constructor(
)
}
+ if (BuildConfig.DEBUG && java.io.File(context.filesDir, "review-hook-arm").exists()) { // test hook
+ java.io.File(context.filesDir, "review-hook-arm").delete() // test hook
+ Logger.info("Held saved-contact callback before activation and request refresh", context = "ReviewHook") // test hook
+ while (!java.io.File(context.filesDir, "review-hook-release").exists()) delay(100) // test hook
+ java.io.File(context.filesDir, "review-hook-release").delete() // test hook
+ Logger.info("Released saved-contact callback", context = "ReviewHook") // test hook
+ } // test hook
isPaymentRequestIdentityActivating = true
val requestRefresh = try {
paykitPaymentRequestRepo.activate(state.publicKey)
@@ -956,6 +974,7 @@ class AppViewModel @Inject constructor(
refreshPriority = Priority.Background,
onlyWhileActive = true,
)
+ if (BuildConfig.DEBUG) Logger.info("Saved-contact refresh deferred='${result.exceptionOrNull() == PaykitPaymentRequestError.OperationInProgress}' pending='${paykitPaymentRequestRepo.pendingRequests.value.size}'", context = "ReviewHook") // test hook
if (!isPaymentRequestPollingStopped.value) refreshPaymentRequestTargets(force = true)
result
} finally {
@@ -1600,6 +1619,13 @@ class AppViewModel @Inject constructor(
}
if (reopenRetainedHardwarePayment(request, generation, preparation)) return true
val started = timeSource.markNow()
+ if (BuildConfig.DEBUG && java.io.File(context.filesDir, "prepare-hook-arm").exists()) { // test hook
+ java.io.File(context.filesDir, "prepare-hook-arm").delete() // test hook
+ Logger.info("Held incoming preparation before native resolution", context = "ReviewHook") // test hook
+ while (!java.io.File(context.filesDir, "prepare-hook-release").exists()) delay(100) // test hook
+ java.io.File(context.filesDir, "prepare-hook-release").delete() // test hook
+ Logger.info("Released incoming preparation", context = "ReviewHook") // test hook
+ } // test hook
val presentationResult = privatePaykitRepo.beginPaymentRequest(request)
val resolvedAfter = started.elapsedNow()
val result = finishIncomingPaymentRequestPreparation(request, generation, preparation, presentationResult)10-21 05:47:30.374 10711 10711 I APP : 2026-10-21 03:47:30.373 INFO [AppViewModel.kt:959] Held saved-contact callback before activation and request refresh - ReviewHook
10-21 05:48:53.697 10711 10711 I APP : 2026-10-21 03:48:53.697 INFO [AppViewModel.kt:962] Released saved-contact callback - ReviewHook
10-21 05:48:54.013 10711 10711 I APP : 2026-10-21 03:48:54.013 INFO [AppViewModel.kt:977] Saved-contact refresh deferred='true' pending='2' - ReviewHook
Test 4: pristine build and focused regressions ✅ The unmodified head built and all four focused suites passed, checked from Gradle and JUnit output.
assembleDevDebug and the complete PaykitSdkServiceTest, PaykitPaymentRequestRepoTest, PaykitPaymentRequestRepoSubscriptionTest and AppViewModelSendFlowTest suites passed with no failures, errors or skipped tests. These cover the receive/read lock boundary, receive-only backup accounting, cancellation, review/wipe/identity admission, pending-drain ordering, intake errors and polling recovery. The installed pristine APK's SHA-256 matched the build used for this check.
I reviewed the full ce4db93…8564ba2 range, including all changed production code, tests and journeys and the reached lock/admission helpers. No planned case was left unrun. Wipe, identity-change and cancellation interleavings were checked through source and automated tests rather than native fault injection; no base/head latency benchmark was run. Prior source report: Talos review.
Twin: synonymdev/bitkit-ios#1070 fix: keep Paykit inbox observation together at b1d65a7 matches the receive/read composition, Background contact refresh deferral and updated journeys in source. iOS was not device-tested in this Android review.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the PR diff against merge base ce4db93, at 8564ba2. This covers only the commits on this pull request, not the stacked parent #1639.
No new actionable code findings.
Automatic Background inbox refreshes receive linked-peer messages and read fresh request state under one operation-lock hold, after the existing full-refresh pending drain. Contact-sync and private-endpoint refreshes stay forced, but run at Background priority so an open payment review defers them; the next inbox poll still does the combined read. A deferred or failed read leaves the previous request list in place, and backup accounting stays tied to receive. The same production contract matches bitkit-ios#1070 at b1d65a7 for that composition and deferral only.
Validation: source and unit tests were inspected, not executed here. CI build and lint succeeded on this head.
Device testing: not performed in this review. A separate review reports Android runs on this head; those runs were not re-executed here.
Ready for device testing.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 8564ba2: no issues found. Reviewed this PR's own commit on top of #1639 (ce4db93). Reviewed as a Paykit sync change.
- No deadlock: the SDK lock isn't reentrant, and nothing inside the combined receive+observe hold takes it again.
- Stale-state checks: wipe, runtime and identity generations are checked before the lock, before receive and before observe. The key generation is checked against the observation, which is stricter than the old observe path.
- Narrow scope: only non-forced Background refreshes use the combined path. Forced and explicit refreshes keep the separate calls.
- Partial failure: if observe defers or fails after a successful receive, nothing is lost or shown twice. The refresh generation was already reset, so the next inbox poll still runs.
- No payment path touched.
- Parity: this side moves contact sync and endpoint refresh to Background. iOS moves one, because it has no forced refresh after endpoint refresh. See synonymdev/bitkit-ios#1070.
Device gate (partial): 8564ba2 — not run yet. Pending:
automatic-presentation.xmlandreview-refresh.xml;- time how long an incoming request takes to appear while a background publication is queued;
- re-check the resync time after closing review.
Device results will follow in a separate review.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approved at 8564ba2a1a96e4d1ac56c4b084143f619968d48a based on the current code review and recorded current-head device and regression evidence: presentation ownership, cancellation, delayed callback deferral, polling recovery and one confirmed payment. Rechecked head/base, CI, E2E and review threads before approval; no new blocking signal.
This branch has not been deployed
5 participants
Incoming requests can wait behind unrelated publishing after the inbox has received them. This PR keeps receipt and fresh request reads in one queued operation.
Stacked on #1639. Related to #1407 and #1419; native timing comparisons are still in progress.
Description
Out of Scope
None.
Design
N/A — no UI changes.
Preview
Native recording pending; no layout changes.
QA Notes
Journeys
automatic-presentation.xml— foreground resume receives each request once and keeps the current sheet intact.review-refresh.xml— a second request arriving during review leaves the first unchanged and becomes available after cancellation and ordinary synchronization.Manual Tests
Automated Checks
PaykitSdkServiceTest.kt— verifies receive/read ordering, receive-only backup accounting, cancellation, review and wipe admission, and no replay.PaykitPaymentRequestRepoTest.kt— keeps the pending drain before the pair, preserves intake errors and warm requests, and excludes forced or explicit refreshes.AppViewModelSendFlowTest.kt— covers contact and endpoint refresh events, typed deferral, and recovery through the next 10-second inbox poll.PaykitPaymentRequestRepoSubscriptionTest.kt— adapts the existing subscription fixture to the composed read without changing its assertions.Final source passed required compile, all 4,432 unit tests, Detekt/format and Android Lint, plus 769 affected caller, repository and SDK tests. Complete diagnostics were inspected; retained parent CloseGuard stack blocks have different ordering and suite attribution. Native comparison remains pending; these tests establish ordering and safeguards, not a measured speedup.