Skip to content

perf: search only loaded wallet accounts - #1084

Open
ben-kaufman wants to merge 14 commits into
fix/paykit-inbox-observationfrom
fix/paykit-loaded-account-address-search
Open

ben-kaufman wants to merge 14 commits into
fix/paykit-inbox-observationfrom
fix/paykit-loaded-account-address-search

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

This PR uses the existing loaded-account list to avoid address derivation for unloaded primary wallet types. Stacked on #1070.

Description

  • Filters primary searches while preserving selected-type priority, primary-before-companion order, search bounds and cache keys.
  • Reuses the same list for companions and retains the existing all-primary fallback when listing fails.

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

  • added AddressSearchCoordinatorTests.swift — distinguishes successful empty lists from failure, observes newly loaded accounts on the next search, and preserves listing-failure fallback.
  • updated 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 4530ff7b at b7561c08 retains 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, tree 8f1e72d3 from #1070 7cd096f4 plus #1081 8967dff8, #1084 b7561c08 and #1085 310e689a, 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 3f5ec6bb0e9ee64c3442d7306a01694f6ad60ed2 incorporates actual master 2ce210252be8ae7ea130d2422a2fa704a191ef7d through 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 b4a6ea597c97ae926397bb7f9d1399bc6fa02a00 includes approved and merged #1098 through master b8b7f3d9902de5669c025bb839232c1d7e98d2df. 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 bf91ba9e into actual parent d9566ad8: 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.

Models used

  • Planning/scoping: gpt-6-astra (reasoning: xhigh), gpt-6.1-sol (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh), gpt-6.1-sol (reasoning: xhigh)
  • Review: gpt-6-astra (reasoning: xhigh), gpt-6.1-sol (reasoning: high)

@ben-kaufman
ben-kaufman marked this pull request as ready for review October 21, 2026 15:17
@ovitrif ovitrif added this to the 3.2.0 milestone Oct 21, 2026
@ovitrif

ovitrif commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

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 talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (WalletNotFound mapped to WalletOperationFailed), 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 catch keeps the original all-types primary list, covered by testAccountListingFailureKeepsPrimaryFallback for both an app error and CancellationError.
  • 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: primaryAccounts keeps the prioritized(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.

@greptile-apps

greptile-apps Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

Limits primary address searches to accounts returned by listAccounts().

  • Address searches derive primary addresses only for loaded wallet types.

Reviews (1) · Last reviewed commit: "perf: search only loaded wallet accounts" · Reviewed by Greptile

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code scan: no findings.

Checked and clean:

  • Skipping unloaded accounts can't produce a false "not ours". ldk-node's listOnchainWalletAccounts returns the aggregate wallet's loaded keys, and address_infos_for_account looks the type up in the same map under the same lock, so every type the filter drops already threw WalletNotFound on master and was skipped. Node-not-set-up (nodeNotSetup before the queue), node-stopped, address-type switch and wallet switch trace to master's outcome.
  • The filter compares LDKNode.AddressType values directly and keeps the order of prioritized(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.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 22, 2026 19:09
@ben-kaufman

ben-kaufman commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

The current-head test run had one failure among 2,881 unit tests: testPendingWalletSwitchResumesAfterTags exceeded its three-second wait for “Second send preparation suspended.” All 43 UI cases completed with zero failures and four existing skips.

I compared the failing head ff28af1e with its exact parent 02986d70: the lifecycle test and send view/model sources are byte-identical. That parent’s run passed 2,879 unit tests and 43 UI cases (four skips). All 571 warning diagnostics in the failed run exactly match the passing parent. The two-test difference is this PR’s address-search coverage, which passed.

One unchanged-head retry of the failed job is now requested. The intermittent lifecycle timeout’s cause remains unestablished; a retry pass will not establish that it is fixed. No test weakening or production change was made.

Retry result: attempt 2 passed on unchanged head ff28af1e: 2,881 unit tests with zero failures, and 43 UI cases with zero failures/four existing skips. The previously failing lifecycle case passed. All 571 warning diagnostics exactly match the passing parent after inspecting the complete retry log. No further retry is needed. The intermittent timeout's underlying cause remains unestablished; this pass is not a code fix.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings: None. The address-search implementation and its tests are unchanged since the previous review. I checked the new LDK Node 0.7.0-rc.77 pin: account listing and address derivation still use the same loaded-wallet map. Selected-type priority, primary-before-companion order, cache scope, search bounds and listing-failure fallback remain intact.

QA for ff28af1:

Check 1: exact-head simulator build ✅ The unmodified head builds for arm64 iOS Simulator with Xcode 27.0.

I ran xcodebuild build for the Bitkit scheme at this head. The app and its embedded extensions built successfully. No app launch or device journey was performed.

Check 2: automated regression coverage ✅ Current-head CI passes all nine address-search tests and SwiftFormat.

I inspected the successful retry log: loaded-only derivation, primary/companion ordering, scoped caches, search boundaries, empty-list refresh, the current-address shortcut and error fallback all passed. The full run passed 2,881 unit tests; its 43 UI cases completed with zero failures and four skips. SwiftFormat also passed at this head. I did not rerun these suites.

The first-attempt lifecycle timeout passed on the unchanged-head retry; its underlying cause remains unresolved. The workflow retains six integration-suite exclusions, and migration E2E checks were skipped.

Check 3: delta and native lookup contract ✅ The pinned LDK implementation still supports loaded-only filtering.

I verified contiguous ancestry from b7561c0, compared the unchanged address-search files, and traced listing and derivation through the pinned LDK implementation. This was source inspection, not a native performance measurement.

No device run: this behavior-preserving optimization requests no manual cases or journeys. The reviewer's native log-count comparison remains unmeasured, as does native timing; no measured performance benefit is claimed.

Twin: synonymdev/bitkit-android#1656 perf: skip unloaded accounts in address search matches loaded-account selection and listing-failure fallback at 42fdcf9; each platform retains its existing cancellation behavior.

Delta review of b7561c0..ff28af1, including merged changes from synonymdev/bitkit-ios#1070 fix: keep Paykit inbox observation together, plus a recheck of the two-file diff against 02986d70. No changed runtime path was driven on a device; device coverage of the dependency's features is outside 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

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@ben-kaufman
ben-kaufman requested a review from ovitrif October 23, 2026 07:39
@ben-kaufman

ben-kaufman commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Current-head CI follow-up at 2b3b7aa2 (run 39919146272, attempt 1): 2,921 unit tests, one failure in testPendingWalletSwitchResumesAfterTags, awaiting “Second send preparation suspended” for three seconds. The 43 UI cases had no failures and four existing skips. The lifecycle test is byte-identical to current parent 974be754, whose 39919145993 passed 2,919 units and the UI lane. This PR changes only address-account search and its tests; those tests passed. Complete logs were inspected; all 573 warning diagnostics match the passing parent.

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 retry

The 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 974be754. A separate xcodebuild -version broken-pipe exception appeared during Swift-package cache setup (the version pipeline uses head -1); that action continued successfully and the test job passed.

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.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ⚠️ Not verified — code review is clean; the requested native log-count comparison remains unrun.

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

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ⚠️ Not verified — no code findings; the requested native log comparison remains unrun.

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

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ⚠️ Not verified — the native warning reduction remains unproven.

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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants