Repository navigation
perf: skip unloaded accounts in address search - #1656
ben-kaufman wants to merge 2 commits into
Conversation
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryAddress searches now read the loaded accounts once, skip unloaded primary types, and reuse the same list for companion accounts.
No actionable issues found. Reviews (1) · Last reviewed commit: "perf: skip unloaded accounts in address ..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Lists the loaded onchain wallet accounts once, before the primary search instead of after it, and skips account-0 address types that are not loaded, so searchReceivingAddressWithLdk stops asking for addresses it cannot get. Reviewed b6a6078, full tier, code only (see below).
What I checked, and 3 candidates I ruled out
Read in full: ActivityService.searchReceivingAddressWithLdk and its helpers in CoreService.kt, LightningService.listOnchainWalletAccounts / addressInfosForType, the new tests in ActivityServicePaykitContactsTest.kt. Diffed against the base codex/paykit-inbox-observation (#1645), not master.
ldk-node v0.7.0-rc.76 (the version pinned in libs.versions.toml): list_onchain_wallet_accounts returns loaded_keys() of the wallet aggregate, all keys, account 0 included. address_infos_for on a key with no wallet returns WalletNotFound, which fetchAddressSearchBatch already turned into a skipped type. So filtering by the loaded set removes exactly the lookups that used to fail, and changes no result.
Ruled out
- A loaded primary type missing from the filter:
loaded_keys()takes every key inwallets, andtoBitkitAddressType()covers all fourLdkAddressTypevariants, so a loaded account-0 type always passes. - Listing failure drops the primary search:
loadedPrimaryTypes == nullkeeps every type, the same as before, andrunSuspendCatchingrethrowsCancellationException, which the new cancellation test covers. - An empty list while the node is stopped: the node being null throws
NodeNotSetup, which takes the fallback path. A stopped but set-up node answersNotRunninginaddress_infos_for_accounteither way, so skipping the derivation loses nothing.
Not run here: on the Bitkit track an auto review does not build bitkit-android branches. This review is reasoned from the code and from CI. build, build-local and lint were still pending when I posted it.
Merge confidence: 4/5, no findings but CI (build, lint) had not finished at review time.
|
Milestone 3.2.0: it is stacked on #1645 (3.2.0), the same Paykit performance track. |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
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 8564ba2, at b6a6078.
No new actionable code findings.
The loaded-account filter does not drop a receiving address LDK can derive. In LDK Node v0.7.0-rc.76 (e494969), list_onchain_wallet_accounts is the loaded wallet set, including account 0, and address_infos_for_account fails for a key outside that set. A listing failure still searches every primary type, cancellation still leaves the activity unwritten, and the current-address shortcut still returns before the list. Selected-type, receive-before-change, and primary-before-companion order stay as they were. A null result still falls through to private reserved-address resolution.
Bitkit iOS AddressSearchCoordinator at c5e33a31 still attempts every account-0 type and uses the account list only for companions. Those extra attempts fail for unloaded wallets, so the resolved address matches.
This review inspected the new ActivityServicePaykitContactsTest cases and did not execute them. build and lint succeeded on this revision.
Ready for device testing.
Device testing: not performed in this review.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: I found no actionable defects.
QA for b6a6078:
Check 1: Development APK build ✅ The app built successfully at the reviewed head.
Ran :app:assembleDevDebug from the pinned source checkout and confirmed the APK outputs.
Test 1: Address resolution regressions ✅ All focused automated tests passed.
Ran :app:testDevDebugUnitTest --tests to.bitkit.services.ActivityServicePaykitContactsTest; the JUnit report confirms no failures or skips. This covers loaded-primary selection, search priority, companion indexes, listing-failure fallback, cancellation, newly loaded accounts on the next search, and the current-address shortcut.
No device run: the QA notes list no journeys or manual cases, and this change has no user-visible behavior change. Native timing was not measured.
Pair PR: synonymdev/bitkit-ios#1084 perf: search only loaded wallet accounts matches the loaded-account filtering, listing-failure fallback and primary-before-companion priority.
Reviewed the full two-file diff and the address-resolution code it reaches. No device case drove the native lookup path.
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.
Approved at b6a6078be969d8fa43f82edffc78f8d861ffdaf5 based on the code review and build and focused regression evidence. This behavior-preserving optimization has no required manual/device cases; native timing remains unmeasured. Rechecked head/base, CI, E2E and review threads before approval; no new blocking signal.
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".
listOnchainWalletAccountsreturns the aggregate wallet's loaded keys, andaddress_infos_for_accountlooks the type up in that same map, so every type the filter now drops already threwWalletNotFoundon master, and the loop already treated that as "skip type". Node-not-built, node-stopped, address-type switch and wallet switch all trace to master's outcome. LdkAddressType.toBitkitAddressType()(LightningService.kt:131-136) is exhaustive and maps 1:1 ontoALL_ADDRESS_TYPES, so a loaded type always survives the filter.- The current-address shortcut still runs before the listing. A listing failure falls back to searching all types (old companion behaviour kept). Cancellation still propagates through
runSuspendCatching, and the new test pins that no activity is written. resolveAddressForInboundPaymentorder is unchanged: pre-activity metadata, then LDK search, then the private reserved address. Cache keys, receive-before-change and batch window are unchanged.- Twin synonymdev/bitkit-ios#1084 applies the same filter and fallback.
Performance-relevant (fewer native calls on the inbound-payment path). The gate counts, per inbound on-chain payment to a non-current address, the Skipping '<type>' … address search batch '0' warnings (CoreService.kt:1139-1143) 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. It also checks that the activity shows the address, not Loading..., with Taproot selected and a receive to an earlier native-segwit address.
Device gate (partial): b6a6078 — not run yet; pending the log-count measurement above against master. Device results will follow in a separate review.
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — an inherited manual-payment retry defect is reproducible in the requested delta.
Findings:
- MEDIUM — Release the unsent request’s reservation when an earlier payment blocks it.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:5939— inherited from the merged parent. When an unresolved on-chain payment A blocks a different incoming request B, B has already reserved a manual allowance attempt.LightningRepo.sendOnChainrejects B beforebeforeSendAttempt, but this branch records B asUNKNOWN. Its journal has neither a payment hash nor a transaction ID, so recovery cannot settle it; retrying B then fails withPaymentAlreadyRecorded, including after A completes. Fail B’s reservation when dispatch never began and the blocker belongs to a different request.
QA for 42fdcf9:
Check 1: Development APK build ✅ The reviewed head and the unfiltered parent both built from source.
Ran :app:assembleDevDebug at 42fdcf9 and bad5ae8, then installed their APKs for the comparison.
Test 1: Address resolution regressions ✅ The focused regression suite passed with LDK `0.7.0-rc.77`.
Ran :app:testDevDebugUnitTest --tests to.bitkit.services.ActivityServicePaykitContactsTest. The JUnit report confirms no failures or skips. Coverage includes loaded-primary selection, ordering, scoped companion indexes, listing failure, cancellation, per-search freshness and the current-address shortcut.
Test 2: Earlier segwit receive with Taproot selected ✅ The receiving address remained correct, while failed unloaded lookups fell from two per search to zero, checked in app and native logs.
Created and confirmed a 12,000-sat regtest receipt to the earlier native-segwit address with Taproot selected and native segwit monitored. Replayed the same transaction and wallet state on the unfiltered parent bad5ae8 and the reviewed head. The stored activity’s receiving address matched the funded address on both builds; the current receiving address was Taproot. The stored field was checked directly, separately from the transaction outputs shown in the recordings.
The parent logged 18 skipped lookups and 18 corresponding native errors across nine single-payment synchronizations: one legacy and one nested-segwit lookup per search. The head logged zero across seven single-payment synchronizations. Six failures are not expected in this configuration: selecting Taproot loads its wallet, and the native-segwit receive match returns before the change-address loop. Native latency was not measured.
Unfiltered parent bad5ae8 |
Reviewed head 42fdcf9 |
|---|---|
native-base-final.2x.mp4 |
native-head-final.2x.mp4 |
Both recordings show refresh and the same confirmed transaction’s details, at 2× speed.
Test 3: Blocked manual request releases its reservation ❌ A request blocked before dispatch was recorded as uncertain instead of failed.
Extended the existing blocked Shop recovery follows original request through success unit test with a temporary test-only reservation stub and assertion. The original recovery assertions still ran. After the earlier request resolved, the new assertion expected finishManualPayment("attempt-b", false, null) for the unsent request; the actual call supplied null for the outcome. Production source was unchanged, and the test-only edit was restored.
This confirms the finding in the inherited manual-payment code. The recovery path cannot resolve an unknown journal entry without a payment hash or transaction ID. No manual-payment device case was driven.
No planned case remains unrun. The device comparison follows the requested native check, with the lookup expectation adjusted to the actual loaded wallets and early match.
Twin: synonymdev/bitkit-ios#1084 perf: search only loaded wallet accounts matches the loaded-account filter, listing-failure fallback and primary-before-companion order.
Reviewed the contiguous delta b6a6078..42fdcf9, including inherited parent changes, plus the earlier address-search review’s open scope. The two-file address-search change is unchanged from the previous review. Other inherited UI and allowance journeys were reviewed in code and were not driven on a device in this pass.
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
This branch has not been deployed
5 participants
This PR avoids failed native lookups for unloaded wallet accounts when resolving an onchain activity’s receiving address. Stacked on #1645 (
codex/paykit-inbox-observation).Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A
QA Notes
Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
ActivityServicePaykitContactsTest.kt— covers loaded-primary priority, listing failure and cancellation, empty-list handling, fresh lookup on the next search, and the current-address shortcut.ActivityServicePaykitContactsTest.kt— verifies companion resolution retains scoped indexes and primary-before-companion order without deriving unloaded primary types.Validation passed: Kotlin compilation, 33 focused tests, all 4,437 unit tests, Detekt/format and Android Lint. An exact-base negative control executed the companion-resolution test and failed on the unwanted unloaded-account lookup. Complete diagnostics were inspected; no introduced findings.