Repository navigation
Prioritize explicit contact linking with per-peer retries - #905
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Starts a per-peer interactive link retry when a contact is added or refreshed, ahead of the batched background preparation. Reviewed 87e7a34, full tier, reasoned from the code (iOS does not build on this reviewer's Linux host).
What I checked, and 3 candidates I ruled out
Read: the full diff of PrivatePaykitService+Contacts.swift, PrivatePaykitService.swift, +State, +Backup, ContactsManager.swift, AddContactView.swift, AppScene.swift, plus startPendingPrivateMessageDrainRetries, drainPendingPrivateMessageRetry and scheduleContactPreparation around it
Call sites traced: startExplicitContactLink from AddContactView and ContactsManager.refreshContactLink; PubkyPublicKeyFormat.normalized and matches; IdentityStatus.publicKey through paykit-ffi
CI: unit and integration tests still running at review time; validate green.
Ruled out
- Identity check with
==indrainPendingPrivateMessageRetrynever matching:expectedIdentityis stored normalized, and the FFI returnsIdentityStatus.publicKeyaspubkyplus lowercase z-base32 (to_app_key()inpaykit-ffi/src/sdk.rs), the same formnormalized()produces. - An unsaved or malformed key starting a link:
startExplicitContactLinknormalizes both keys, requires the local secret for the identity, andscheduleExplicitContactLinkrequires the key inknownSavedContactKeys. - Cleanup or profile deletion racing a new explicit retry:
scheduleExplicitContactLinkbails onisDeletingProfileandcleanupPendingKey, and the delayed task rechecks the generation and the identity before scheduling preparation.
Merge confidence: 4/5, CI has not finished, so the new PrivatePaykitServiceTests cases are unproven here.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Review: diff 10 files.
The open Android PR synonymdev/bitkit-android#1451 also adds a retry window for each contact and targets linked intake. It retries incomplete target discovery during that window. This iOS PR says its request manager retries only on a later refresh or lookup.
Findings:
1 inline (1 LOW)
QA:
Tests were not run for this review. The PR description lists only checks the author ran.
Reviewed by gpt-6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
| PubkyPublicKeyFormat.matches(completion.identity, pubkyProfile.publicKey), | ||
| contactsManager.contacts.contains(where: { PubkyPublicKeyFormat.matches($0.publicKey, completion.publicKey) }) | ||
| else { return } | ||
| paykitPaymentRequestManager.startEligibleTargetRefresh(publicKey: completion.publicKey) |
There was a problem hiding this comment.
LOW: No test covers the targeted refresh after a link completes.
The new contactLinkCompletedPublisher subscription has no test for targeted eligibility refresh. The service tests stub didLink, so they pass even if the subscription or identity guard stops working. Could we add a focused test that sends a completion and checks the active identity and saved contact gates?
There was a problem hiding this comment.
Agreed: the service tests do not exercise this SwiftUI subscription or its scene-level gates. eef1747 adds the mirrored save/background/resume/readiness journey and lists it unchecked in QA, but that is not a direct subscription test. Leaving this coverage point open; this follow-up does not add a test-only production abstraction or claim those gates are now tested.
There was a problem hiding this comment.
The journey exercises readiness after resume, but it can pass through the contact detail refresh without exercising this subscription. I am keeping the focused subscription-test coverage point open.
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 4 files.
New findings: 1 inline (1 LOW); the rest is in the review.
Android PR synonymdev/bitkit-android#1451 still implements the paired retry scheduling change, but the new iOS resume journey has no Android counterpart on its default branch or that PR.
QA:
No test ran because the PR description lists no test for reviewers to run; its Validation section only reports checks the author already ran.
Reviewed by gpt-6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Updated in 85421c1: due contacts reuse scheduling snapshots, with fresh post-mutation checks and invalidation after waits, lifecycle changes, preparation and identity changes. All 2,161 native tests passed with no introduced diagnostics. This reduces repeated SDK reads; paired device/Server linking performance remains unmeasured. |
There was a problem hiding this comment.
Verdict: ♻️ Comment
Reaudit: diff 6 files.
No new findings; the rest is in the review.
The paired Android PR synonymdev/bitkit-android#1451 also implements per-contact retries and now includes the identical resume journey at 8cc6c9e. The iOS snapshot changes preserve that shared behavior.
QA:
Tests wait for CI.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
03d7380 to
08f02ff
Compare
There was a problem hiding this comment.
Suggestion: 👍 Approve
Reaudit: diff 2 files.
No new findings; the rest is in the review.
The paired retry behavior and identical resume journey remain in synonymdev/bitkit-android#1451; these two files declare the iOS SwiftPM dependency.
QA:
Tests running.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Verdict: ✅ Approve
Tests for the review: Test 1 and journey J1 pass.
QA:
Tested on two iOS 26.5 simulators (iPhone 17 Pro).
🟢 Tests 1, J1
Test 1
On 08f02ff + test hook: admitted linking completed uncancelled while inactive, later work waited at the lifecycle gate, and the same retry resumed with one advancement.
1.mp4 |
test hook
Not pushed. Holds the first admitted link operation for 20 seconds before the SDK call, grants it a debug-only iOS background execution window, and logs cancellation, retry identity and pause/resume gates.
diff --git a/Bitkit/Services/PrivatePaykitService+Contacts.swift b/Bitkit/Services/PrivatePaykitService+Contacts.swift
index ea701316..8b1617d2 100644
--- a/Bitkit/Services/PrivatePaykitService+Contacts.swift
+++ b/Bitkit/Services/PrivatePaykitService+Contacts.swift
@@ -1,4 +1,7 @@
import Foundation
+#if DEBUG // test hook
+ import UIKit // test hook
+#endif // test hook
import Paykit
// MARK: - Saved Contacts
@@ -22,6 +25,22 @@ extension PrivatePaykitService {
var canPublish: () async -> Bool = { true }
}
+ #if DEBUG // test hook
+ private actor ContactLinkTestHold { // test hook
+ static let shared = ContactLinkTestHold() // test hook
+ private var held = false // test hook
+ func holdOnce(_ key: String) async { // test hook
+ guard ProcessInfo.processInfo.arguments.contains("-qa-link-hold"), !held else { return } // test hook
+ held = true // test hook
+ Logger.info("QA_LINK_HOLD admitted key=\(key) cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in // test hook
+ DispatchQueue.global().asyncAfter(deadline: .now() + 20) { continuation.resume() } // test hook
+ } // test hook
+ Logger.info("QA_LINK_HOLD released key=\(key) cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+ #endif // test hook
+
struct PrivateMessageDrainOperations {
let ensureLink: (String) async throws -> Void
let pendingOutbound: () async throws -> [String]
@@ -31,7 +50,29 @@ extension PrivatePaykitService {
static func live(readPriority: PaykitSdkOperationLock.Priority = .ordered) -> PrivateMessageDrainOperations {
PrivateMessageDrainOperations(
- ensureLink: { _ = try await PaykitSdkService.shared.ensureLinkWithPeer($0, priority: readPriority) },
+ ensureLink: { key in
+ #if DEBUG // test hook
+ let testBackgroundTask = await MainActor.run { // test hook
+ guard ProcessInfo.processInfo.arguments.contains("-qa-link-hold") else { return UIBackgroundTaskIdentifier.invalid } // test hook
+ return UIApplication.shared.beginBackgroundTask(withName: "QA link hold", expirationHandler: nil) // test hook
+ } // test hook
+ defer { // test hook
+ Task { @MainActor in // test hook
+ if testBackgroundTask != .invalid { UIApplication.shared.endBackgroundTask(testBackgroundTask) } // test hook
+ } // test hook
+ } // test hook
+ await ContactLinkTestHold.shared.holdOnce(key) // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_LINK_SDK start key=\(key) cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ } // test hook
+ defer { // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_LINK_SDK finished key=\(key) cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+ #endif // test hook
+ _ = try await PaykitSdkService.shared.ensureLinkWithPeer(key, priority: readPriority)
+ },
pendingOutbound: { try await PaykitSdkService.shared.pendingOutboundPrivateCounterparties(priority: readPriority) },
linkedPeers: { try await PaykitSdkService.shared.linkedPeers(priority: readPriority) },
processPending: { _ = try await PaykitSdkService.shared.processOutboundPrivateMessages(counterparty: $0, priority: readPriority) },
@@ -260,6 +301,11 @@ extension PrivatePaykitService {
}
func setBackgroundWorkPaused(_ paused: Bool) {
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_GATE paused=\(paused) generation=\(preparationGeneration)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
if paused != isBackgroundWorkPaused { messageSchedulingGeneration += 1 }
isBackgroundWorkPaused = paused
if !paused { resumeBackgroundWorkWaiters() }
@@ -269,9 +315,19 @@ extension PrivatePaykitService {
while isBackgroundWorkPaused, generation == preparationGeneration, !Task.isCancelled {
let id = UUID()
let (stream, continuation) = AsyncStream<Void>.makeStream()
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_GATE waiting generation=\(generation)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
backgroundWorkWaiters[id] = continuation
for await _ in stream {}
backgroundWorkWaiters.removeValue(forKey: id)
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_GATE resumed generation=\(generation) cancelled=\(Task.isCancelled)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
}
return generation == preparationGeneration && !Task.isCancelled
}
@@ -985,6 +1041,12 @@ extension PrivatePaykitService {
publicKey: String, reason: String, schedulingSnapshot: inout PrivateMessageSchedulingSnapshot?
) async {
guard let retry = pendingMessageDrainRetries[publicKey] else { return }
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_RETRY start key=\(publicKey) id=\(retry.id) index=\(retry.retryIndex)", context: "TestHook") // test hook
+ defer { Logger.info("QA_RETRY snapshot key=\(publicKey) id=\(retry.id)", context: "TestHook") } // test hook
+ } // test hook
+ #endif // test hook
let generation = preparationGeneration
var identityInspectionFailed = false
defer {
@@ -1075,6 +1137,11 @@ extension PrivatePaykitService {
} else {
pendingMessageDrainRetries[publicKey]?.completeAttempt(at: messageRetryOperations.now())
}
+ #if DEBUG // test hook
+ if ProcessInfo.processInfo.arguments.contains("-qa-link-hold") { // test hook
+ Logger.info("QA_RETRY settled key=\(publicKey) id=\(retry.id) nextIndex=\(pendingMessageDrainRetries[publicKey]?.retryIndex.description ?? "removed")", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
}
private func isMessageRetryCurrent(publicKey: String, id: UUID, generation: Int) -> Bool {[2026-10-09 02:53:35.533 UTC] INFOℹ️: QA_RETRY start key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y id=A4679FBE-C79A-4789-8F77-10D040F70791 index=0 - TestHook [PrivatePaykitService+Contacts.swift: drainPendingPrivateMessageRetry(publicKey:reason:schedulingSnapshot:) line: 1046]
[2026-10-09 02:53:37.479 UTC] INFOℹ️: QA_LINK_HOLD admitted key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y cancelled=false - TestHook [PrivatePaykitService+Contacts.swift: holdOnce(_:) line: 35]
[2026-10-09 02:53:40.290 UTC] INFOℹ️: QA_GATE paused=true generation=0 - TestHook [PrivatePaykitService+Contacts.swift: setBackgroundWorkPaused(_:) line: 306]
[2026-10-09 02:53:58.071 UTC] INFOℹ️: QA_LINK_HOLD released key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y cancelled=false - TestHook [PrivatePaykitService+Contacts.swift: holdOnce(_:) line: 39]
[2026-10-09 02:53:58.072 UTC] INFOℹ️: QA_LINK_SDK start key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y cancelled=false - TestHook [PrivatePaykitService+Contacts.swift: live(readPriority:) line: 66]
[2026-10-09 02:53:58.963 UTC] INFOℹ️: QA_LINK_SDK finished key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y cancelled=false - TestHook [PrivatePaykitService+Contacts.swift: live(readPriority:) line: 70]
[2026-10-09 02:53:58.965 UTC] INFOℹ️: QA_GATE waiting generation=0 - TestHook [PrivatePaykitService+Contacts.swift: waitForBackgroundWork(generation:) line: 320]
[2026-10-09 02:54:23.250 UTC] INFOℹ️: QA_GATE paused=false generation=0 - TestHook [PrivatePaykitService+Contacts.swift: setBackgroundWorkPaused(_:) line: 306]
[2026-10-09 02:54:23.251 UTC] INFOℹ️: QA_GATE resumed generation=0 cancelled=false - TestHook [PrivatePaykitService+Contacts.swift: waitForBackgroundWork(generation:) line: 328]
[2026-10-09 02:54:44.962 UTC] INFOℹ️: QA_RETRY settled key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y id=A4679FBE-C79A-4789-8F77-10D040F70791 nextIndex=1 - TestHook [PrivatePaykitService+Contacts.swift: drainPendingPrivateMessageRetry(publicKey:reason:schedulingSnapshot:) line: 1142]
[2026-10-09 02:54:47.972 UTC] INFOℹ️: QA_RETRY start key=pubkyodnhxup5eafu1jexet97s5o44im56gw3sx8tc7sw9zkizze9jo3y id=A4679FBE-C79A-4789-8F77-10D040F70791 index=1 - TestHook [PrivatePaykitService+Contacts.swift: drainPendingPrivateMessageRetry(publicKey:reason:schedulingSnapshot:) line: 1046]
Test J1
After saving and Home for 5 seconds, the contact offered Request Or Pay without adding it again; Request opened the amount screen and the contact appeared once.
J1.mp4 |
Tip
Test 1 worth a journey
Test 1
- Delete and re-add the staging peer, then save while the second wallet stays in the foreground.
- At the admitted debug hold before the real SDK call, press Home.
- While inactive, verify the SDK finishes uncancelled and later work waits at the lifecycle gate.
- Resume using the SpringBoard app icon; verify the same retry advances once.
Note
Ticked J1 and 1 in the PR’s QA list.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
3 participants
Summary
Stacked on #887 (
codex/paykit-rc71). Merge #887 first, then retarget this PR tomaster. #887 retains rc71; this PR adopts rc72.The current stacked diff includes lifecycle changes from already-merged #888 because #887 predates that merge. Those changes are already on
masterand will fall out of the diff when retargeted.Give each contact its own private-link retry schedule, so adding or refreshing a contact does not inherit another contact's backoff.
Backgrounding pauses subsequent work; admitted SDK operations finish normally. Identity changes, contact deletion, queue fairness, existing retry intervals and SDK safety waits remain protected. Uses published Paykit rc72 for phase-local handshake read reuse and typed link-observation failures. Request preparation keeps invalid link metadata pending for retry without resetting links or treating transport errors as corrupt state. No protocol or UI change.
Validation
Limits
This improves scheduling, not the cost of a Homeserver operation. Paired Bitkit-to-Server measurements remain outstanding; no end-to-end linking speedup is claimed. Target eligibility discovery coalesces requests but only retries a failed lookup on a later normal refresh or contact lookup.
QA Notes
Journeys
link-contact-after-resume.xml: save a contact, background/resume, and reach Request or Pay without re-adding it; mirrored in Android #1451. Not yet device-run on this final head.Manual Tests
Automated Checks
PrivatePaykitServiceTests.swift: scheduling snapshots share reads without authorizing mutations or surviving lifecycle, identity, preparation or timer invalidation.