Repository navigation
perf: search only loaded wallet accounts - #1084
Conversation
|
Milestone 3.2.0: it is stacked on #1070 (3.2.0) and is the iOS twin of synonymdev/bitkit-android#1656 (3.2.0). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Filters the account-zero address search to the address types LDK Node reports as loaded, reusing the same listOnchainWalletAccounts() call that already fed the companion accounts. Reviewed e8c835f, light tier on the stacked commit (base fix/paykit-inbox-observation, 10 lines of logic plus tests). Not built here: iOS does not build on Linux, so Run Tests is the gate and it was still pending when this was posted.
What I checked, and 4 candidates I ruled out
Read in full: AddressSearchCoordinator in Bitkit/Services/CoreService.swift, its caller findReceivingAddress and processOnchainPayment, LightningService.listOnchainWalletAccounts / addressInfosForType, AddressSearchCoordinatorTests.swift.
Traced into ldk-node at the pinned 0.7.0-rc.76 (e494969): Node::list_onchain_wallet_accounts returns wallet.loaded_keys(), and address_infos_for_account looks the same key up in that map, returning WalletNotFound when absent.
CI: SwiftFormat green; Run Tests pending.
Ruled out
- An unloaded account-zero type that holds a receive: before this change the derive call for it already threw (
WalletNotFoundmapped toWalletOperationFailed), the loop logged and broke, so it could never match. Skipping it up front returns the same result with fewer FFI round trips. - Listing fails: the
catchkeeps the original all-types primary list, covered bytestAccountListingFailureKeepsPrimaryFallbackfor both an app error andCancellationError. - Successful empty list: no derivation at all; every derive would have thrown anyway, and the next search re-reads the list (
testEmptyAccountListIsReadAgainForTheNextSearch). - Order and cache keys:
primaryAccountskeeps theprioritized(selected:)order because it filters, not rebuilds, and the key still depends only on type, keychain and account index.
Merge confidence: 4/5, no findings, but the change was not built or run here and Run Tests had not finished.
 [Medium impact] The PR appears safe to merge. SummaryLimits primary address searches to accounts returned by
Reviews (1) · Last reviewed commit: "perf: search only loaded wallet accounts" · Reviewed by Greptile |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against merge base b1d65a7, at e8c835f. GitHub’s base e16d13bc is ahead of that merge base; CoreService.swift and AddressSearchCoordinatorTests.swift are unchanged there, and the PR merges cleanly.
No new actionable code findings.
Account-zero search keeps address types returned by listOnchainWalletAccounts(). LDK Node 0.7.0-rc.76 (e494969) uses that list as the loaded-key set, and address_infos_for fails with WalletNotFound for any other account. Account 0 surfaces that as WalletOperationFailed, which the existing batch loop already stops on. A listing throw, including CancellationError, still searches every primary type and skips companions. A successful empty list derives nothing and is read again on the next search. Selected-type order, receive-before-change, primary-before-companion, cache keys, and the 1000-index window are unchanged. The loaded-account filter matches bitkit-android#1656 at b6a6078. Companion ordering on iOS remains index, then address-type name.
Validation: Run Tests and SwiftFormat passed on e8c835f. AddressSearchCoordinatorTests belongs to the synchronized BitkitTests target, which that workflow runs. This review did not rebuild or rerun the suite.
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.
Verdict: ✅ Approve
Posted as a comment: not every planned case passed.
Findings: None. The loaded-account list and address derivation use the same wallet map in the pinned LDK implementation. Filtering preserves selected-type priority, primary-before-companion ordering, cache keys, search bounds, and existing error outcomes.
QA for b7561c0:
Check 1: exact-head build ✅ The unmodified head builds for arm64 iOS Simulator with Xcode 27.0.
I ran xcodebuild build against this head. No application launch or device journey was performed.
Check 2: automated coverage ✅ Current-head CI passes the address-search tests and SwiftFormat.
I inspected the Run Tests log: AddressSearchCoordinatorTests passes loaded-only lookup, ordering, scoped caches, bounded ownership lookup, empty-list refresh, and listing/derivation error cases. SwiftFormat also passes at b7561c0. I did not rerun the suite. The workflow retains its integration exclusions and unrelated UI skips.
No device run: the QA notes request no journeys or manual cases, and this optimization preserves user-visible behavior. Native timing was not measured.
Twin: synonymdev/bitkit-android#1656 perf: skip unloaded accounts in address search matches loaded-account selection at b6a6078; each platform retains its existing cancellation handling.
Full review of 7cd096f..b7561c0, including both changed files and the reached lookup/LDK paths. No changed runtime path was driven on a device. This agrees with the earlier code review, with the build and CI checks above verified at the current head.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
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 reassessment of the complete PR diff against merge base 7cd096f4, at b7561c08. The merge base changed since the saved assessment, so this pass inherited no prior coverage.
No new actionable code findings.
Account-zero search retains the address types in a successful listOnchainWalletAccounts() result and keeps selected-type order. LDK Node 0.7.0-rc.76 (e494969) uses that list as the loaded wallet map; addressInfosForAccount fails for any other account, and account 0 becomes WalletOperationFailed, which the existing batch loop already stops on. A listing throw still searches every primary type and skips companions. A successful empty list derives nothing and is read again on the next search. Receive-before-change, primary-before-companion, cache keys, and the 1000-index window stay as they are.
bitkit-android#1656 at b6a6078 applies the same loaded-primary filter. iOS still sorts companions by account index, then address-type name. Android aborts when listing is cancelled; iOS keeps the existing primary fallback. Production listing returns the loaded accounts, or throws nodeNotSetup before the queue, so that cancellation path does not drop a derivable address.
Validation: Run Tests passed on b7561c08 and ran AddressSearchCoordinatorTests (9 tests, 0 failures). SwiftFormat passed on the same commit. This review did not rebuild or rerun the suite.
Device testing: not performed in this review. Ready for device testing.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
Approved at b7561c08efd33a7e3f958aee576b276098d8b818 based on the current code review and build and address-search regression evidence. This behavior-preserving optimization has no required manual/device cases; native timing remains unmeasured. Rechecked head/base, checks and review threads before approval: unit tests and SwiftFormat pass. Separate integration/E2E runs are absent for this stacked head.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- Skipping unloaded accounts can't produce a false "not ours". ldk-node's
listOnchainWalletAccountsreturns the aggregate wallet's loaded keys, andaddress_infos_for_accountlooks the type up in the same map under the same lock, so every type the filter drops already threwWalletNotFoundon master and was skipped. Node-not-set-up (nodeNotSetupbefore the queue), node-stopped, address-type switch and wallet switch trace to master's outcome. - The filter compares
LDKNode.AddressTypevalues directly and keeps the order ofprioritized(selected:)(LDKNode+AddressType.swift:10-16). - A type changed between the listing and the derive loop costs at most one search: the activity shows "Loading..." and inbound no-contact activities are re-resolved on the next sync.
- Twin synonymdev/bitkit-android#1656 applies the same filter and fallback.
Performance-relevant. The gate counts, per inbound on-chain payment to a non-current address, the Skipping '<type>' … address search batch '0' warnings (CoreService.swift:2009-2013) and LDK's Failed to get address infos for account lines, on this head and on master: expected 6 → 0 with only native segwit loaded.
Device gate (partial): e9180b4 — not run yet; pending the log-count measurement above against master. Device results will follow in a separate review.
|
Current-head CI follow-up at This repeats the previously recorded intermittent preparation expectation timeout, not the separate extra-route assertion; its cause remains unestablished. Requested one unchanged-head failed-job retry after the source/log comparison. No code or assertions changed. The older successful retry is not being rerun, and this new failure is retained as evidence rather than called fixed. Attempt 2 result — passed on retryThe single unchanged-head retry passed all 2,921 unit tests, with 43 UI cases / zero failures / four existing skips. The previously failing expectation passed. Complete workflow logs were inspected; all 573 warning diagnostics match passing parent This remains an intermittent failure with an unestablished cause, not a code fix. Evidence has been added to #1096 without equating it to the older extra-route assertion. Do not rerun this successful job. |
There was a problem hiding this comment.
Verdict:
Findings: None. The address-search implementation, its tests and the dependency pins are unchanged since the previous reviewed head. Loaded-only filtering retains selected-type priority, primary-before-companion ordering, cache scope, bounds and listing-failure fallback.
QA for 2b3b7aa:
Check 1: exact-head simulator build ✅ The unmodified head builds for arm64 iOS Simulator with Xcode 27.0.
I built the Bitkit scheme, including its embedded extensions. No app launch or device journey was performed.
Check 2: automated regression evidence ✅ Exact-head CI passes the address-search tests and the changed lifecycle regression classes.
I inspected the successful retry log. All nine AddressSearchCoordinatorTests passed, covering loaded-only calls, ordering, scoped caches, bounded searches, fresh empty-list handling, current-address lookup and error fallback. The changed scanner, Send presentation, Quick Pay, USDT protection and sheet regression classes also ran successfully. The full run passed 2,921 unit tests; its UI lane reported 43 cases with zero failures and four skips. SwiftFormat passed at this head. I did not rerun these suites.
The first-attempt lifecycle timeout passed on retry; that does not establish a fix for synonymdev/bitkit-ios#1096 bug: send confirmation lifecycle tests fail intermittently on master. The workflow excludes six integration suites, and migration E2E jobs were skipped.
Check 3: delta and source checks ✅ The delta introduces no new actionable code finding.
I verified contiguous ancestry, reviewed the merged scanner, Send presentation, Quick Pay and USDT protection changes, and rechecked the two-file PR diff against 974be754. git diff --check passed. Translation validation reported zero errors and 15 warnings. These are source and automated checks, not device evidence.
The merged changes include:
Test 1: native unloaded-account log comparison ⏭️ The requested comparison remains unrun.
The earlier review requested a native comparison of unloaded-account warnings for an inbound payment to a non-current address. No report proves that case. No device run was performed: the PR's QA notes request no manual cases or journeys for this behavior-preserving optimization. Native log reduction and timing remain unmeasured; no measured performance benefit is claimed.
Twin: synonymdev/bitkit-android#1656 perf: skip unloaded accounts in address search at 42fdcf9 matches loaded-primary selection and listing-error fallback; each platform retains its existing cancellation handling.
Delta review of ff28af1..2b3b7aa, covering all 49 changed files, including changes merged through synonymdev/bitkit-ios#1070 fix: keep Paykit inbox observation together, plus the two-file PR diff against 974be754. No changed runtime path was driven on a device; the merged parent journeys were not exercised by these checks.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
…it-loaded-account-address-search
There was a problem hiding this comment.
Verdict:
Findings: None. The address-search implementation, regression tests and dependency pins are unchanged since the previous review. I also checked the merged app-lock correction: active request screens retain their state while hidden, and actual dismissal still clears it.
QA for b142985:
Check 1: exact-head simulator build ✅ The unmodified head builds for arm64 iOS Simulator with Xcode 27.0.
I built the Bitkit scheme and its embedded extensions. No app launch or device journey was performed.
Check 2: automated regression evidence ✅ Current-tree CI passes the address-search and app-lock preservation regressions.
I inspected the Run Tests log. CI tested merge 24ceb37c, whose tree is identical to b142985. All nine AddressSearchCoordinatorTests passed, covering loaded-only calls, ordering, scoped caches, bounded searches, empty-list refresh, current-address lookup and error fallback. The new testAppLockKeepsOtherRequestScreensUntilDismissal passed, alongside the existing app-lock remount and retention regressions. I did not rerun these suites.
SwiftFormat passed. The workflow retains six integration-suite exclusions and four UI skips; migration E2E jobs were skipped. This pass does not establish a fix for synonymdev/bitkit-ios#1096 bug: send confirmation lifecycle tests fail intermittently on master.
Check 3: delta and source checks ✅ The four-file delta introduces no actionable code finding.
I verified contiguous ancestry and reviewed the app-lock cleanup guard, the hosted regression, changelog and updated journey. The new guard preserves the current request outside retained Bitcoin confirmation; actual dismissal still cancels setup and clears state. I rechecked the two-file PR diff against 7a3cc5a; loaded-only filtering retains priority, cache scope, search bounds and listing-failure fallback. git diff --check passed.
Test 1: native unloaded-account log comparison ⏭️ The requested comparison remains unrun.
The earlier review requested a native comparison of unloaded-account warnings for an inbound payment to a non-current address. No report proves that measurement. No device run was performed: this optimization's QA notes request no manual cases or journeys. Native log reduction and timing remain unmeasured; no measured performance benefit is claimed.
Twin: synonymdev/bitkit-android#1656 perf: skip unloaded accounts in address search at 42fdcf9 matches loaded-primary selection and listing-error fallback; each platform retains its existing cancellation handling.
Delta review of 2b3b7aa..b142985, covering all four changed files and reached paths, plus the two-file PR diff against 7a3cc5a. No changed runtime path was driven on a device. The merged app-lock journey remains unexecuted; its hosted regression is automated evidence only.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
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:
Findings: None.
QA for b4a6ea5:
Check 1: simulator build ✅ The head builds for arm64 iOS Simulator.
Built the Bitkit scheme and its embedded extensions.
Check 2: regression evidence ✅ The address-search implementation and its tests are unchanged since the previous reviewed head.
The previous review cites the passing address-search regression suite. The current-head CI run also passed; its UI lane retains four prerequisite skips.
Test 1: native unloaded-account log comparison ⏭️ The requested warning reduction remains unverified.
On a disposable regtest wallet, an inbound payment to an older receive address reached address derivation on both this head and base ac364f0. The app logs showed zero unloaded-account warnings on each; the base found the loaded account before the unloaded types, so this did not reproduce the requested baseline. The head recording has visible stutters. Native timing and the predicted warning reduction remain unmeasured.
Head: https://github.com/user-attachments/assets/691a85e9-dc79-4142-985e-eac65313c393
Base: https://github.com/user-attachments/assets/b05c171f-b2c7-4311-858c-8d93df049384
This PR's QA notes request no device journeys or manual cases for the behavior-preserving optimization; Test 1 addresses the reviewer's separate request. The merged parent changes were reviewed but their device journeys were not driven here.
Twin: synonymdev/bitkit-android#1656 perf: skip unloaded accounts in address search matches loaded-primary filtering and listing-failure fallback at 42fdcf9; cancellation handling remains platform-specific.
Delta review of b142985..b4a6ea5, including the 88 changed files and the unchanged address-search paths.
Reviewer model: Unknown
Reasoning effort: Not exposed
Reviewed by gpt-6.1-terra-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: Re-reviewed the full PR at 54e8820 after the merge base changed since the saved assessment. No coverage baseline was supplied, so this pass covers the whole diff from merge base ac364f01.
No new actionable code findings.
Loaded-account filtering uses one listOnchainWalletAccounts() result. At ldk-node f05161ed, that list is the loaded wallet map and includes the primary account-0 wallet; derivation of a missing wallet already fails. Selected-type order, receive-before-change, and primary-before-companion order stay in place. A listing failure still searches every primary type, and a successful empty list is read again on the next search. A recreated confirmation uses the send flow's isSubmittingPayment flag, set inside performPayment, to stay busy and to skip a second authorization while that payment is still in flight.
The shared address-search contract matches bitkit-android#1656 at 42fdcf9e: loaded primary types, the listing-failure fallback, and primary-before-companion order. That comparison is limited to the address-search contract. The unlock busy state is iOS confirmation lifecycle behavior.
Validation: unit-tests and SwiftFormat passed on 54e8820. This review did not execute those checks or a device. Earlier review threads on older heads left no open code finding for this head.
Device testing: not performed in this review.
Ready for device testing.
Suggested additional test cases
- Platform: iOS. Setup: PIN required for payments, biometrics off, and an initial subscription payment that has already started submitting. Action: lock and unlock the app so payment confirmation is recreated during that submission. Expected: no second PIN prompt, the confirmation stays busy, and the original payment completes once.
Reviewer model: grok-4.7
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None in b4a6ea5..54e8820.
QA for 54e8820: Xcode 27.0, iPhone 17 Pro simulator, iOS 26.5; disposable wallet and staging regtest funds.
Native unloaded-account warning comparison ✅ Both builds resolved the same non-current change address; logs measured three unloaded-account warnings/errors on base and zero on head.
I sent separate 100,000-sat receipts to the same revealed native change address on head and base ac364f0, with only native SegWit account 0 loaded. Both displayed the confirmed receipt and resolved its address correctly. For each lookup, base logged three Skipping … receive address search batch 0 warnings and three corresponding LDK Wallet not found errors; head logged zero of each.
This exercises the requested native comparison. The native-change match ends the search before unloaded change types, accounting for the measured 3 → 0 rather than the predicted full-traversal six. Native timing remains unmeasured.
Head 54e8820 |
Base ac364f0 |
|---|---|
native-head-2x.2x.mp4 |
native-base-2x.2x.mp4 |
Receipt and activity detail on each build, 2×. Warning counts and resolved addresses were checked in transaction-scoped app/native logs.
Test hook
Not pushed. Derives a native change address without advancing its cursor, exposes it for funding, and logs the real lookup boundaries and result. The same patch was applied to head and base.
diff --git a/Bitkit/BitkitApp.swift b/Bitkit/BitkitApp.swift
index 961935aa..fdb9bc2b 100644
--- a/Bitkit/BitkitApp.swift
+++ b/Bitkit/BitkitApp.swift
@@ -204,6 +204,20 @@ struct BitkitApp: App {
} else {
ContentView()
.preferredColorScheme(.dark)
+ .task { // test hook
+ #if DEBUG // test hook
+ guard CommandLine.arguments.contains("-QA_NATIVE_ADDRESS") else { return } // test hook
+ for _ in 0 ..< 300 { // test hook
+ if let infos = try? await LightningService.shared.addressInfosForType(.nativeSegwit, keychain: .internal, startIndex: 0, count: 1), let address = infos.first?.address { // test hook
+ try? address.write(to: Env.appStorageUrl.appendingPathComponent("v4-1084-change-address"), atomically: true, encoding: .utf8) // test hook
+ let accounts = (try? await LightningService.shared.listOnchainWalletAccounts()) ?? [] // test hook
+ Logger.info("QA loaded accounts: \(accounts)", context: "Review1084") // test hook
+ return // test hook
+ } // test hook
+ try? await Task.sleep(for: .seconds(1)) // test hook
+ } // test hook
+ #endif // test hook
+ } // test hook
}
}
}
diff --git a/Bitkit/Services/CoreService.swift b/Bitkit/Services/CoreService.swift
index d8add9d4..5ee8b179 100644
--- a/Bitkit/Services/CoreService.swift
+++ b/Bitkit/Services/CoreService.swift
@@ -1219,12 +1219,19 @@ class ActivityService {
let currentWalletAddress = UserDefaults.standard.string(forKey: "onchainAddress") ?? ""
let selectedAddressType = LDKNode.AddressType.fromStorage(UserDefaults.standard.string(forKey: "selectedAddressType"))
- return try await addressSearchCoordinator.runAddressSearch(
+ #if DEBUG // test hook
+ if CommandLine.arguments.contains("-QA_NATIVE_ADDRESS") { Logger.info("QA search begins \(txid)", context: "Review1084") } // test hook
+ #endif // test hook
+ let qaResult = try await addressSearchCoordinator.runAddressSearch( // test hook
details: details,
value: value,
currentWalletAddress: currentWalletAddress,
selectedAddressType: selectedAddressType
)
+ #if DEBUG // test hook
+ if CommandLine.arguments.contains("-QA_NATIVE_ADDRESS") { Logger.info("QA resolved \(txid) to \(qaResult ?? "nil")", context: "Review1084") } // test hook
+ #endif // test hook
+ return qaResult // test hook
}
func getActivity(id: String, walletId: String = WalletScope.default) async throws -> Activity? {Base ac364f0:
INFOℹ️: QA search begins 209bc89492a36c54f6127980e2b9effe620d7d07b69b05682ce5e34d7d5a90db
ERROR❌: Failed to get address infos for account OnchainWalletAccount { address_type: Legacy, account_index: 0 } keychain External at indexes 0..200: Wallet not found
WARN⚠️: Skipping legacy account 0 receive address search batch 0: AppError(message: "Failed to perform wallet operation", debugMessage: Optional("Failed to conduct wallet operation."), paymentFailureReason: nil, underlyingError: Optional(LDKNode.NodeError.WalletOperationFailed(message: "Failed to conduct wallet operation.")))
ERROR❌: Failed to get address infos for account OnchainWalletAccount { address_type: NestedSegwit, account_index: 0 } keychain External at indexes 0..200: Wallet not found
WARN⚠️: Skipping nestedSegwit account 0 receive address search batch 0: AppError(message: "Failed to perform wallet operation", debugMessage: Optional("Failed to conduct wallet operation."), paymentFailureReason: nil, underlyingError: Optional(LDKNode.NodeError.WalletOperationFailed(message: "Failed to conduct wallet operation.")))
ERROR❌: Failed to get address infos for account OnchainWalletAccount { address_type: Taproot, account_index: 0 } keychain External at indexes 0..200: Wallet not found
WARN⚠️: Skipping taproot account 0 receive address search batch 0: AppError(message: "Failed to perform wallet operation", debugMessage: Optional("Failed to conduct wallet operation."), paymentFailureReason: nil, underlyingError: Optional(LDKNode.NodeError.WalletOperationFailed(message: "Failed to conduct wallet operation.")))
INFOℹ️: QA resolved 209bc89492a36c54f6127980e2b9effe620d7d07b69b05682ce5e34d7d5a90db to bcrt1qzkja4t45ec20yhvyntpykfyere2hrgl2m2mp7t
Head 54e8820:
INFOℹ️: QA search begins 9d438fa79aa457c2f0bee13b0d6fea0154857b1dae9e42079b365695cd3fdc78
INFOℹ️: QA resolved 9d438fa79aa457c2f0bee13b0d6fea0154857b1dae9e42079b365695cd3fdc78 to bcrt1qzkja4t45ec20yhvyntpykfyere2hrgl2m2mp7t
Unlock during an initial subscription payment ✅ The recreated confirmation stayed busy, requested no second payment PIN and completed the original native send once after release.
With payment PIN enabled and biometrics off, I authorized a 1,000-sat payment, held it after durable native preparation, backgrounded the app, then entered only app-unlock PIN. The automatic confirmation returned to its busy overlay; the log confirmed its duplicate authorization entry was ignored. Releasing the hold reached success.
Both executions passed. Each added exactly one recipient transaction containing a 1,000-sat output and a 143-sat fee, verified by decoded transactions and recipient history. The video shows the shorter second execution; the earlier outgoing activity belongs to the first.
subscription-2x.2x.mp4
Payment authorization, app unlock, retained busy state and completion, 2×.
A debug-only hook staged isInitialSubscriptionPayment on an ordinary regtest payment and held its real native dispatch until explicit release. PIN authorization, the retained submission flag, the guard and native broadcast outcome remained active. Subscription creation and proof exchange were not exercised. This covers the automatic re-entry case suggested in the current-head QA review; the parent's manual-swipe report remains separate attributed evidence.
Test hook
Not pushed. Stages the initial-subscription context, logs authorization entry and holds the real operation after durable preparation until explicit release.
diff --git a/Bitkit/Services/OnchainSendAttemptService.swift b/Bitkit/Services/OnchainSendAttemptService.swift
index eefaac4f..418426aa 100644
--- a/Bitkit/Services/OnchainSendAttemptService.swift
+++ b/Bitkit/Services/OnchainSendAttemptService.swift
@@ -666,6 +666,17 @@ actor OnchainSendAttemptService {
throw OnchainSendAttemptError.preDispatch(preDispatchError)
}
+ #if DEBUG // test hook
+ if CommandLine.arguments.contains("-QA_HOLD_DISPATCH") { // test hook
+ let gate = Env.appStorageUrl.appendingPathComponent("v4-1084-release-dispatch") // test hook
+ let evidence = "txid: \(prepared.txid)\namount: \(prepared.recipientAmountSats)\naddress: \(address)\n" // test hook
+ try evidence.write(to: Env.appStorageUrl.appendingPathComponent("v4-1084-held.yaml"), atomically: true, encoding: .utf8) // test hook
+ Logger.info("QA dispatch held after authorization and durable preparation", context: "Review1084") // test hook
+ while !FileManager.default.fileExists(atPath: gate.path) { try await Task.sleep(for: .milliseconds(200)) } // test hook
+ Logger.info("QA dispatch released", context: "Review1084") // test hook
+ } // test hook
+ #endif // test hook
+
if let winner = try winningResult(attemptId: attemptId) {
return winner
}
diff --git a/Bitkit/Views/Wallets/Send/SendConfirmationView.swift b/Bitkit/Views/Wallets/Send/SendConfirmationView.swift
index 564d721c..85bdb9fd 100644
--- a/Bitkit/Views/Wallets/Send/SendConfirmationView.swift
+++ b/Bitkit/Views/Wallets/Send/SendConfirmationView.swift
@@ -405,6 +405,12 @@ struct SendConfirmationView: View {
}
guard preparingRequest == nil, !returnToAmountReviewIfNeeded() else { return }
ensureSendAmountFromScannedInvoicesIfNeeded()
+ #if DEBUG // test hook
+ if CommandLine.arguments.contains("-QA_INITIAL_SUBSCRIPTION"), app.contactPaymentContext == nil { // test hook
+ app.contactPaymentContext = ContactPaymentContext(publicKey: "pubkytest", isInitialSubscriptionPayment: true) // test hook
+ Logger.info("QA staged initial subscription context", context: "Review1084") // test hook
+ } // test hook
+ #endif // test hook
if app.contactPaymentContext?.isInitialSubscriptionPayment == true, !shouldAutomaticallyPay {
showManualConfirmation(reason: "not automatic when opened")
}
@@ -807,7 +813,12 @@ struct SendConfirmationView: View {
}
private func submitPayment(isAutomatic: Bool = false) async throws {
- guard !isSubmittingPayment else { return }
+ guard !isSubmittingPayment else { // test hook
+ #if DEBUG // test hook
+ if CommandLine.arguments.contains("-QA_INITIAL_SUBSCRIPTION") { Logger.info("QA duplicate subscription authorization ignored", context: "Review1084") } // test hook
+ #endif // test hook
+ return // test hook
+ } // test hook
guard preparingRequest == nil, walletSwitchContext == nil else { throw CancellationError() }
if isAutomatic, isFeeRateMissing {
guard !Self.automaticPaymentAwaitsFeeRate(isFeeRateMissing: isFeeRateMissing, feeRateLoadFailed: wallet.feeRateLoadFailed) else {
@@ -889,6 +900,9 @@ struct SendConfirmationView: View {
throw CancellationError()
}
} else {
+ #if DEBUG // test hook
+ if CommandLine.arguments.contains("-QA_INITIAL_SUBSCRIPTION") { Logger.info("QA payment PIN requested", context: "Review1084") } // test hook
+ #endif // test hook
let shouldProceed = await requestPinCheck()
guard shouldProceed else {
throw CancellationError()QA payment PIN requested
QA dispatch held after authorization and durable preparation
QA duplicate subscription authorization ignored
QA dispatch released
Confirmation lifecycle and swipe regressions ✅ The focused suites passed on the unmodified head, including both new regressions; results were checked in the Xcode result bundle.
The selected address-search, confirmation lifecycle and swipe suites passed: 35 tests, zero failures or skips. Both new regressions also passed in a recorded replay using the original unmodified test products. They check retained busy state, no new authorization after recreation, unchanged navigation and ownership, alongside the existing idle-entry and cancellation cases.
regressions-2x.2x.mp4
Recorded replay of the two new hosted regressions, 2×; assertions were checked in test results.
Check: build and review scope ✅ The unmodified head builds and the contiguous four-file delta has no actionable code finding.
I reviewed b4a6ea5..54e8820, all four changed files and reached submission, app-lock, PIN, swipe and completion callers, with an independent reviewer. The address-search implementation, tests and dependency pins are unchanged since the previous review. I rechecked the pinned LDK loaded-wallet contract and built this head before adding test hooks.
No planned case remains unrun. Biometric, hardware and USDT payment paths were not driven; native latency and the inherited parent journeys received no new measurement or full journey execution.
Twin: bitkit-android#1656 perf: skip unloaded accounts in address search, checked at 42fdcf9, matches loaded-primary filtering and listing-failure fallback. The confirmation recreation fix is iOS-specific.
Reviewed by gpt-6.1-sol-high
The base branch was changed.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at f9a969a5 after the comparison base changed and no coverage baseline was supplied. This pass covers the complete diff against merge base b23f305a.
No new actionable code findings.
Primary address search now keeps only account-0 types present in listOnchainWalletAccounts(), in selected-type order, then companion accounts. At ldk-node 1976754 (0.7.0-rc.78), that list is the loaded wallet map and includes the primary account-0 wallet; deriving a missing account-0 wallet already fails, so dropped types were not previously matchable. A listing failure still searches every primary type and skips companions. A successful empty list is read again on the next search. findReceivingAddress still uses the result only to store an inbound activity address.
The loaded-primary filter, listing-failure fallback, and primary-before-companion order match bitkit-android#1656 at 42fdcf9e. That is a contract comparison, not a review of the Android PR. Shipped Android 02e6ae44 still probes every account-0 type.
GitHub reports Run Tests and SwiftFormat succeeded on this head. This review did not execute those checks or inspect their logs. AddressSearchCoordinatorTests was read as source. Device testing: not performed in this review.
Reviewer model: grok-4.7
Reasoning effort: high
|
@ben-kaufman Merging this PR: @piotr-iohk approved head f9a969a, checks are green and no threads are open. It has no manual cases; the native comparison on 54e8820 still covers it, since only master merges came after. It merges before the 3.1.0 cut, so it ships in 3.1.0. |
|
@ben-kaufman Merged, as it was approved and green. |
|
Placing this PR in 3.1.0: it merged before the 3.1.0 cut, so it ships in 3.1.0. |
This PR uses the existing loaded-account list to avoid address derivation for unloaded primary wallet types. Based on master after #1070 merged.
Description
Out of Scope
None.
Design
N/A — no UI changes.
Preview
N/A — no user-visible behaviour change.
QA Notes
Setup
N/A — no device journeys or manual cases are requested for this change. Automated coverage is listed below; the shared journey guide remains the device-test reference.
Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
AddressSearchCoordinatorTests.swift— distinguishes successful empty lists from failure, observes newly loaded accounts on the next search, and preserves listing-failure fallback.AddressSearchCoordinatorTests.swift— checks loaded-only calls, search order, cache scope, bounded ownership lookup and unchanged cancellation-error outcomes.Evidence
Both exact-base negative controls failed on the expected unloaded/empty-account assertions.
Final integration validation
Integration with master
4530ff7batb7561c08retains both USDT-warning cancellation and the payment-review callback. Bottom #1025: all-target build passed (19 targets), 378 focused tests passed, 0 failures/skips. One combined #1070 + #1081 + #1084 + #1085 composition, tree8f1e72d3from #10707cd096f4plus #10818967dff8, #1084b7561c08and #1085310e689a, covers the stack and those sibling changes: all-target build passed (19 targets) and 2,779 tests passed, 0 failures/skips using the six existing CI exclusions. These are not eight independent full-suite runs.All eight heads passed SwiftFormat. Translation checks report 0 errors and 15 warnings, all matching master by key, language, file and occurrence count, including
security__use_pin. Final toolchain/runtime and complete diagnostic disposition: Xcode 26.1 / iOS 27.0 simulator, serial tests; complete diagnostics inspected, with six SDK-construction QoS warnings and retained fixture/appearance messages matching baseline evidence; no introduced finding. Final-head CI after publication: pending.Gaps
Loaded-account selection is covered by deterministic tests. No native timing improvement is claimed.
The final Android 206 / iOS 215 comparison cohort was blocked before financial authorization, with no debit. It supplies no final payment-latency result or native benefit claim. Native USDT-warning, PIN and biometric flows have not been verified by this integration validation.
October 26 master integration
Published integration head
3f5ec6bb0e9ee64c3442d7306a01694f6ad60ed2incorporates actual master2ce210252be8ae7ea130d2422a2fa704a191ef7dthrough ordinary signed merges, preserving the stacked base and prior child changes. The two parent conflict resolutions retain review-state ownership, root-safe subscription Back navigation, one outer USDT status task, prompt recipient-scoped proof reconciliation and the terminal-refresh exit. No local assertion/timeout weakening or new test suppression was introduced.Validation: two 19-target builds and two canonical hosted unit suites passed: 2,910 on the parent and 2,937 on the combined tree, zero failures/skips with the exact six CI integration exclusions. This PR is covered by the combined stack/sibling tree
f471e8a6(2,937 units), including this branch; these are two suites, not eight independent runs. Full SwiftFormat0.64 passed on all eight final trees (0/844); translations had zero errors and 15 warnings matching execution on complete actual-master source. Independent frozen integration review found no actionable issue. Local environment: Xcode26.1/SDK26.1, iOS26.5 simulator.Complete diagnostics retained: two App Intents notices per build; parent1,082/combined1,099 runtime scan records, including four/six SDK/Pubky/DHT QoS stacks, fault fixtures and hosted appearance/identity notices. Source comparisons are not exact-base runtime replay or warning-free evidence. New-head hosted CI follows publication. Master's #1130 retired Blocktank skips and E2E285 channel/migration omissions remain coverage gaps, not fixes for original #1109. Historical #1054 preparation/UI-launch causes remain unresolved. Existing QA case IDs and native gaps remain: no new payment, funded USDT, physical hardware, latency or merchant-unlock journey was run by this integration.
October 26 lifecycle integration and camera fixture
Published head
b4a6ea597c97ae926397bb7f9d1399bc6fa02a00includes approved and merged #1098 through masterb8b7f3d9902de5669c025bb839232c1d7e98d2df. The overlapping lifecycle test retains one owned switch task per current context, holds resumed preparation until confirmation is visible, and keeps the exact-one-success assertion, cancellation/context guards and retained presentation/app-lock behavior. Seven ordinary signed merges preserve stacked bases and prior child changes; #1084/#1085 also retain merged #1081 through #1070.The existing unfunded USDT UI case now resets camera permission before launch, explicitly waits for the camera alert, allows it and waits for dismissal before manual entry. This adds six test-only lines and two assertions; no test, assertion or timeout was removed or weakened. Production camera behavior and the merged Receive blank-band checks are preserved.
Final validation passed two 19-target builds, 2,913 parent canonical units and 2,940 units on combined tree
db653be56e228dec55f4e6c0fd57e42e7d8475c2, with zero failures/skips and the exact six CI integration exclusions. This branch is included in that combined composition. These are two suites, not seven independent runs. The combined unfunded UI case passed, including both Receive blank-band swipes and explicit camera handling, using runner-only loopback endpoint settings. Full SwiftFormat 0.64 passed on all seven final trees (0/844); translations are unchanged. Independent final review found no actionable issue. All 151 distinct commits across local current/prospective base ranges verified cryptographically; all 129 distinct commits across the final published GitHub ranges report verified=true/reason=valid.Complete diagnostics remain retained: parent 1,107/combined 1,125 broad records, including four/six SDK/Pubky/DHT QoS stacks, App Intents notices, hosted appearance and fault-fixture messages. The combined shared-keychain -25300 publication diagnostic has unchanged emitter/test source; its exact runtime cause remains unestablished. Source comparison is not exact-base runtime replay or warning-free evidence.
The old Receive-dismissal failure in #1070 run 40394372963 did not reproduce in the controlled original-head run; that run passed and used XCTest's default camera interruption handler. The camera-fixture comparison passed with an identical 157-file production app bundle. Neither result establishes the Receive failure's cause or repair. Local Xcode 26.1/SDK 26.1/iOS 26.5 differs from hosted CI; the completed hosted results are recorded below. The daemon launch issue #1110, original hardware-transfer issue #1109 and earlier coverage gaps remain. No new payment, funded USDT, physical-hardware, final latency or merchant-unlock result is claimed. João, Piotr and Ovi remain requested; existing pending requests are preserved without duplicate notification.
October 26 hosted CI confirmation
For published head
b4a6ea597c97ae926397bb7f9d1399bc6fa02a00, unit/UI run 40412985828 passed on attempt 1: 2,939 unit tests, zero failures/skips; 43 UI executions, 39 passed and four existing prerequisite skips, zero failures. SwiftFormat 40412985894 passed: 0/844 files require formatting, 84 skipped. These workflows tested GitHub's PR merge commit using Xcode 16.4/SDK 18.5 on iOS 26.2.Complete workflow diagnostics were inspected. The 571 duplicate-symbol records match the retained passing #1025 run 40376172874; workflow/cache-action sources match actual master. This is retained-log/source comparison, not an exact-base runtime replay or warning-free result.
No manual CI rerun was requested. Historical Receive-dismissal/daemon-launch causes and original hardware-transfer issue #1109 remain unresolved; existing skipped/deferred coverage and final Android 206/iOS 215 payment/unlock measurement remain incomplete. Existing human review requests are preserved.
October 29 inherited app-lock guard
Published 54e8820b carries the four-file R1 correction from #1039 at aae380fc: a recreated confirmation uses the retained submission flag for its busy state and refuses another authorization while the original payment is in flight. This branch retains its existing Core 0.8.0-rc10, LDK 0.7.0-rc.77, Paykit 0.1.1-rc1 and VSS 0.5.23 pins, stacked base and prior history.
On this exact branch candidate, Xcode 27.1 / SDK 27.1 / iOS 26.5 passed the complete selected unit suite: 2,941 passed, 0 failed, 0 skipped, with the six existing unit-workflow integration exclusions. Both new confirmation regression tests passed. Full SwiftFormat 0.64.0 passed: 0/844 files require formatting; 84 existing exclusions. The private package copy was corrected before this branch's first invocation; the sibling #1054's initial build-path failure remains retained. This branch's first full test run passed with unchanged source and dependency pins.
Complete diagnostics were inspected: two AppIntents metadata notices, five native SDK/Pubky/mainline DHT construction QoS warnings, and retained fault-fixture/host output. Forty-five emitter/test files were compared with this branch's pre-fix head; the warning-emitting methods in the two changed files are unchanged. No introduced actionable finding was found. This is source and retained-evidence comparison, not an exact-base runtime replay or warning-free claim. The identical production patch reuses the two independent R1 source reviews and guard negative control from #1039, with the older child callers inspected separately.
All 14 commits in the actual current-parent PR range verified cryptographically locally and report GitHub verified=true / reason=valid after the normal push. Hosted unit/UI run 40849989525 passed on merge
bf91ba9einto actual parentd9566ad8: 2,980 passed executions, 0 failed, 4 explicit fixture skips (2,951 unique passing definitions; one parameterized test has 30 runs). Both new confirmation tests passed, and hosted SwiftFormat passed 0/844. Full artifact and nested diagnostics were audited; the 286 duplicate native-symbol warning set matches retained history. A native keyboard queue timeout in the unchanged, passing tags-resume lifecycle test is retained. This is a separate Xcode 16.4/iOS 26.2 result using the same committed package pins as the locally validated branch.Existing authored QA cases keep their recorded status and documented prerequisites. This propagation does not supply new native timing or controlled-fixture execution on the child. The external native R1 report on #1039 reports two successful controlled regtest executions at aae380f; it is attributed parent evidence, not a device run on this child. No new financial journey, binary adoption or final Android206/iOS215 latency/unlock result is claimed.
October 30 current master merge and hosted validation
Piotr merged master
b23f305ainto this branch asf9a969a5. The PR now targets master after #1070 merged. The published tree3babc953exactly matches the previously built prospective composition. All 15 commits in the actual-base GitHub PR range report verified=true / reason=valid. These were externally published commits; this follow-up makes no new local pre-push verification claim.Fresh canonical CI tested merge
0006f6db, with parentsb23f305a+f9a969a5:611509ee; 13 executable cases eventually passed across two trials, with one explicit hardware-transfer skip. Trial 1 had 12 passes and one real failure in@multi_address_4: the 45-second timed-sheet prerequisite could see neither its description nor the expected home controls. Trial 2 reran that one body successfully and skipped the 12 current-run completed bodies plus the hardware case; trial 3 was skipped. This is not one full passing trial. One exit-1 annotation is retained. Failure PNG/video were saved on the runner but not uploaded after recovery; the actual failed screen/probe-error trace is unavailable. No introduced source defect or causal repair was established. Producer upload, API artifact and consumer download digests agree.Complete logs, annotations and nested test diagnostics were inspected once per result. Retained unit/UI observations include 571 native duplicate-symbol warnings, 48 NaN messages, four layout notices and one keyboard SDK timeout. CoreService's modified diagnostic labels/forms are individually accounted for, not classified as unchanged wholesale. Integration also retained NodeNotRunning/batch-search tails, startup-metric and symbolication errors, and the host version-command broken pipe. E2E retained 82 stale-element warnings, five native collector restarts, two recovered address-feedback retries, conditional helper observations and npm's 76 vulnerability notices. No runtime-clean, security-clean or performance claim follows.
Current pins are Core 0.8.0-rc11, LDK 0.7.0-rc.78, Paykit 0.1.1-rc2, VSS 0.5.26-rc1; lock SHA-256
aa3182b3a410d9738665bb4c864ce971e9c186302d7500bfa4964b07dda0b006. Unit/integration used Xcode 16.4/SDK 18.5 on iOS 26.2; the E2E app build used Xcode 26.2/SDK 26.2. No older runtime pass was transferred. Prior cancelled-run HeaderMenu failures and the inherited BoostSuccessToast timeout remain historical unresolved causes. Existing native comparison deferrals and the blocked final Android206/iOS215 payment/unlock measurement remain; no new field latency, funded native journey or SDK release/adoption is claimed.Existing named reviewers and pending requests are retained. Current-head comments and pending requests are not human approvals.
The new first-trial UI prerequisite failure remains a diagnostic acceptance gap: screenshot/video/native UI state or probe-error trace is needed to distinguish app state from helper/driver failure. Eventual CI success does not establish overall repair completion. No assertion, guard or timeout was weakened, and no manual CI retry was requested.
Master advanced to
04a0a1f8after these jobs. A bounded comparison found its manual-Lightning recovery changes do not change this PR's address-search inputs; the repository's strict up-to-date status policy is false. These results retain their exactb23f305acomposition and do not claim to execute that newer master or its added recovery test.Models used
gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:xhigh)gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:xhigh)gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:high)