fix: notify confirmed-only onchain receives - #1299
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
CI:
|
|
Settled the Checked the spec against the branch. The step order is receive unconfirmed, CPFP boost, restore from seed, reinstall, The e2e PR adds
|
|
Correction on the e2e link: the companion PR is now synonymdev/bitkit-e2e-tests#231. Same commit, on a branch named |
|
CI is green on 24ab459. The rerun resolved the e2e branch to Merge order: #231 first (or together), otherwise this PR is green only while the matching e2e branch exists. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 24ab459b9. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — Device clock off by more than an hour keeps every confirmed-only receive silent
Checked: Traced both entry paths at the PR head: LightningRepo invokes the service handler sequentially and only then emits to AppViewModel, so in the background the service claims first and posts one notification while AppViewModel's later call hits isPresentationClaimed and skips; in the foreground the service's canPresent (App.currentActivity == null) fails and AppViewModel claims, so one sheet. Dedupe across process death does not depend on the in-memory presentationClaims set: markOnchainActivityAsSeen writes seenAt into the Core activity DB via present -> recordPresentation, and CoreService.shouldShowReceivedSheet returns false on seenAt != null, so a mempool-presented tx that confirms after a restart (foreground or background path) resolves to Skip; ldk-node calls eventHandled before onEvent so events are not replayed either. Sends, CPFP children and channel-funding/order payments carry a negative net amount_sats (ldk-node get_tx_details = received - sent) and exit on amountSats <= 0 before any activity write; coop-close and sweep transactions run the same findClosedChannelForTransaction / isTransfer / channelId checks as the existing mempool path, with lightningService.balances read live from node.listBalances(). ldk-node rc.66 updates the payment store before returning wallet events, so handleOnchainTransactionConfirmed finds the payment and creates the activity before shouldShowReceivedSheet runs. Restore, migration and the one-hour block-timestamp guard are evaluated before any state is written, and the greptile thread on the duplicate confirmed update is resolved by applyConfirmationIfMissing. iOS twin synonymdev/bitkit-ios#588 is open.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 2a6b9dd68. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — grep -A3 "pkg=to.bitkit.dev" never prints the notification title the next step verifies; LOW — Cross-platform row says not ported, but the iOS twin fixes the same behaviour without the journeys
Checked: Round 3 (2a6b9dd, new journeys): Round 3 delta is journeys only (three onchain-receive journeys, the suite README and two rows in journeys/README.md). Every testTag the journeys name exists at head: Receive (TabBar.kt:156), ReceiveScreen (ReceiveSheet.kt:144), ShowDetails and ReceiveOnchainAddress (ReceiveQrScreen.kt:427/658), ReceivedTransaction and ReceivedTransactionButton (NewTransactionSheet.kt:161/194), MoneyText (Money.kt). The preconditions are reachable with repo tooling: ./lsp at the root wraps the blocktank-api script, deposit returns the txid the log grep needs, Env.walletSyncIntervalSecs is 10, and LightningService logs every event as LDK event fired: <json> at debug level under the APP tag with the event class name as type. The staging regtest tip timestamp was checked against the wall clock (about 5 minutes apart), so the one-hour MAX_CONFIRMED_ONLY_AGE guard does not make the confirmed-only journeys false-fail today. The background flow matches the code: App.currentActivity clears on stop, LightningNodeService claims and posts Payment Received / Received %s and caches the sheet, AppViewModel's later call skips on the claim, and ContentView ON_START calls consumePaymentReceivedInBackground, so tapping the auto-cancel notification opens the received sheet once. The mempool-then-confirmed journey would catch a second sheet if the txid claim or seenAt dedupe regressed.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head c67782eb6. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Round 4 (e2be5d9, c67782e): both journey LOWs are fixed. Round 4 covers two journey-only commits that answer my earlier LOWs. e2be5d9: the new grep matches what AOSP NotificationRecord prints inside the extras block under --noredact (android.title=String (Payment Received), android.text=String (Received ...)). The receive notification is posted through Context.pushNotification (Notifications.kt:84-86) with plain Strings from ReceivedNotificationContent.build (R.string.notification__received__title = "Payment Received", notification__received__body_amount = "Received %s"), so the extras class is String and both patterns hit. The foreground-service notification uses app_name ("Bitkit Regtest" on dev, build.gradle.kts:205) and notification__service__body, so it matches neither pattern and the title count is exact. c67782e: the cross-platform row now says the port is pending in synonymdev/bitkit-ios#588 and records both adaptations (restore-flag replay guard instead of the one-hour window, no foreground-service counterpart for the background notification journey), which is what I asked for. No new findings.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ✅ Approve
Review: diff 13 files.
Findings:
N/A
Audit:
Audited - no findings.
Coverage:
QA: journeys and manual tests await green CI checks
Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
|
@jvsena42 ci failed |
|
Looked at it: infra, not the change. The That shard has failed the same way on several PRs in this batch this afternoon. Re-running the failed jobs now. The real e2e issue this PR did cause is already handled separately in synonymdev/bitkit-e2e-tests#231, which dismisses the received sheet in |
|
@jvsena42 maybe we can sync with base branch to retrigger ci |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 7 files.
Findings:
N/A
Audit:
Already done in comment.
QA: emu-1 redroid container on Android 15, dev build 2.4.1 (188)
-
✅ passed: deposit of 21 797 sats mined in one command; one
OnchainTransactionConfirmedand noOnchainTransactionReceivedfor the txid, sheet showed once and stayed away for 15 s after dismissal.Test 1:
Regtest, app on Home → Receive → Savings address → deposit and mine 1 block in one command: received sheet…1.mp4
-
✅ passed: mempool-only deposit of 14 797 sats; sheet appeared at t=8s with one
OnchainTransactionReceivedand no confirmed event.Test 2:
**2a.** `regression:` Home → deposit to a Savings address without mining: received sheet shows; dismiss it.2.mp4
-
✅ passed: mining that deposit produced the confirmed event with no second sheet over 36 s and no "Payment Received" notification, with
POST_NOTIFICATIONSgranted.Test 3:
**2b.** mine 1 block: no second sheet and no "Payment Received" notification.3.mp4
-
✅ passed:
pm clearand a 12-word restore brought back 3 activities and 44 564 sats with no sheet for 40 s during the restore, after Get Started and after a relaunch, and no notification.Test 4:
`regression:` Restore a wallet with old onchain deposits: no received sheets or notifications for them.4.mp4
-
✅ passed: background deposit of 7 970 sats mined in one command posted exactly one "Payment Received" notification; tapping it opened the received sheet and dismissing it returned Home with no second sheet.
Test 5:
**3b.** tap the notification: app opens on the received sheet; dismiss shows Home with no second sheet.5.mp4
Note
The restored node in item 4 logged only SyncCompleted and replayed no OnchainTransactionConfirmed for the three txids, so that item proves the silence rather than the restore guard NotifyPaymentReceivedHandlerTest.kt covers. The three new journeys say the Receive tab row carries no test tag, but this head exposes Tab-savings and Tab-spending.
Tip
Test 4 worth a journey:
- Fund the wallet with a few onchain deposits and let them confirm
- Note the Savings balance on Home
- Wipe the app data and relaunch
- Accept the terms, skip the intro and choose Restore
- Type the 12-word recovery phrase and tap Restore
- Tap Get Started when the wallet is restored
- Watch Home for at least 40 seconds: no received sheet appears
- Force-stop, relaunch and watch again: still no sheet
- Check the notification shade: no Payment Received notification
Coverage:
QA: 6 of 6 manual tests passed
Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
|
QA reviewed on d23597d. Reviewed the confirmed-only receive path against FindingsHIGH
|
@piotr-iohk could you update your skill for using a "require change" comment type for cases like this? |
|
Agreed, this was a real bug, and the fix now matches bitkit-ios#588. Rule (same as iOS):
Commits: 83e20fa (the rule) and ee50a0c. The second one clears the flag only if marking the activities seen succeeds. Otherwise a failed write would leave the whole restored history unseen with the hold already lifted, which is this same bug. iOS clears the flag without checking; Android retries on the next on-chain sync. The other Android-only guards (restore in progress, migration, 1-hour age window) are unchanged. Tests are in On e2e: bitkit-e2e-tests#231 is closed without merging, as you asked. I haven't done a device run of your restore → mine scenario on this revision yet. It's listed unchecked in the PR's QA notes. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
563899a to
2926fd7
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
🐞 potential bug · 5e83e77 · https://github.com/synonymdev/bitkit-android/actions/runs/35903670272/job/107352366182 E2E failure triaged as potential bug · @boost_1 failed 3/3: Payment Received sheet for 100000 sats covers HeaderMenu after restore and mine; the same spec passed on 2926fd7 before the restore |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…irmed-onchain-receive # Conflicts: # journeys/README.md
…irmed-onchain-receive # Conflicts: # journeys/README.md
|
QA reviewed on 3ebff42. Android device, Pixel 6 emulator, local regtest, CI APK bitkit-e2e-apk_5029 from run 35983947634. Passed
Head is now 57da489. Those commits are mnemonic wrapping, not this receive path, so they were not in this run. iOS twin bitkit-ios#588 must be undrafted and brought to parity with this PR before the pair is approved. Android still has the one-hour block-time guard and the background notification. iOS shows the in-app sheet only. |
Port the Android confirmed-only guards from synonymdev/bitkit-android#1299: a confirmed event opens the received sheet only when its block time is within one hour of the device clock and no migration is running, so the confirmations a full wallet scan replays for old txs stay silent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 3 files.
No new findings; the rest is in the review.
Coverage:
QA: waits for the other reviewers' approval, or @ovi-reviewer test
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
addressed - reaudit confirmed
|
@ovi-reviewer test |
There was a problem hiding this comment.
Verdict: ✅ Approve
Tests for the review: Tests 2a, 2b, 3a and 3b pass.
QA:
Tested on Android 15 emulator
Tests 1, 4 already done in review.
Test 5 already done in review.
Test 2a ✅ passed
evidence
2a.mp4 |
 |
Test 2b ✅ passed
evidence
2b.mp4 |
 |
Test 3a ✅ passed
evidence
3a.mp4 |
 |
Test 3b ✅ passed
evidence
3b.mp4 |
 |  |
Tip
Worth a journey
Test 2a
- Open Receive and select Savings.
- Show the address details and return Home.
- Deposit 14 797 sats without mining and verify the received sheet.
- Dismiss the sheet.
Test 2b
- Mine one block after dismissing the mempool received sheet.
- Verify no second sheet appears and no Payment Received notification posts.
Test 3a
- Enable Background Payments and keep Bitkit active in background.
- Open Receive, select Savings, and read its address.
- Send the app Home, then deposit 7 970 sats and mine one block in one command.
- Verify exactly one Payment Received notification with the amount.
Test 3b
- Open the Payment Received notification.
- Verify the received sheet shows 7 970 sats.
- Dismiss it and verify Home stays visible without another sheet.
Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner) · wrong <why> (owner)
3 participants
Fixes #797
Related to #1275
E2E companion: synonymdev/bitkit-e2e-tests#231 (same branch name, so CI here runs against it)
This PR shows the received sheet and notification for onchain deposits that the wallet first sees already confirmed, without a prior mempool event.
Description
pendingRestoreActivitySeen, ported with the same setting name; it fixes the restored-wallet sheet that opened over the Boost Fee screen in the local e2e@boost_1. Persisted in the settings DataStore so it survives an app kill between the restore and the first sync.Out of Scope
NotifyPaymentReceivedHandler: a confirmed-only deposit found more than one hour after its block (for example, the app was killed while it confirmed and reopened later) stays silent, while a deposit still in the mempool at reopen always shows.AppViewModel: the post-restore hold is lifted by the foregroundSyncCompleted(ONCHAIN_WALLET)handler only, as on iOS. If the app is killed within the ~10 s between Get Started and that first sync, the hold stays until the app is next foregrounded and an onchain sync completes; background notifications for new deposits are held until then.AppViewModel: other receive side effects keyed to the mempool event, such as closing an open Receive sheet and the Paykit contact attachment and private address rotation, still run only onOnchainTransactionReceived. On device the Savings address did rotate after a confirmed-only receive.OnchainTransactionReceived.LightningNodeService: after a process restart, a recent confirmation of a tx already seen through its mempool event still runs the handler's short retry delay (about 1.4 s) before skipping.@boost_1 - Can do CPFPin the local e2e suite restores from seed and then mines the boosted receive. Under the restore rule above that confirmation no longer opens the received sheet: the mine lands either while the hold is still on, or after the first post-restore sync marked the restored activity seen. The companion spec'sdismissReceivedPaymentIfShown()tolerates the sheet's absence, so it still passes, but it should stop expecting the sheet: test: dismiss confirmed-only receive sheet in boost_1 bitkit-e2e-tests#231.NotifyPaymentReceivedHandler: the one-hour window is measured against the device clock, so a device whose clock is off by more than an hour never shows a confirmed-only receive, and ldk-node emitsOnchainTransactionConfirmedonce, so it cannot be recovered later. The mempool path is unaffected. A clock-independent reference, such as the timestamp of the tip block from the sync that produced the event, is left for a follow-up.handleOnchainTransactionReceived/handleOnchainTransactionConfirmed) so the activity list stays correct, where iOS writes activities elsewhere; and Android marks the activities seen before clearing the flag, iOS clears the flag first and marks them in a detached task.Design
Reuses the existing received sheet with no visual change: Payment received event
Preview
The wallet's primary display is USD, so fiat leads on device; the button label is chosen at random. Both are existing behaviour.
Deposit and mine in one command while on Home; the received sheet appears after the next sync:
confirmed-only-foreground.mp4
QA Notes
Manual Tests
OnchainTransactionReceivedfor the txid.regression:Home → deposit to a Savings address without mining: received sheet shows; dismiss it.regression:Restore a wallet with old onchain deposits: no received sheets or notifications for them.@boost_1finding). Not run on device in this revision; covered by the unit tests below.The device run used the
journeys/onchain-receivesuite, committed in this PR atjourneys/onchain-receive/(confirmed-only-received-sheet.xml,mempool-then-confirmed-single-sheet.xml,confirmed-only-background-notification.xml, plus thesuite
README.mdand thejourneys/README.mdrows). The same diff inline:journeys/onchain-receive
Automated Checks
NotifyPaymentReceivedHandlerTest.ktcovers a recent confirmed-only receive showing the sheet and the notification, timestamps inside and outside the one-hour window in both directions, received-then-confirmed showing once, negative amounts, the restore and migration skips, and the confirmed activity update being applied once and skipped when it is already stored.NotifyPaymentReceivedHandlerTest.ktmoves the existing onchain cases to the new handler dependencies.LightningNodeServiceTest.ktcovers a background confirmed event reaching the handler and posting exactly one notification, and a handler skip posting none.AppViewModelSendFlowTest.ktcovers the confirmed event updating the activity and then showing the sheet, and no handler call during a migration.NotifyPaymentReceivedHandlerTest.ktcovers the hold skipping the mempool and the confirmed-only paths while still writing the activity, a historical activity already marked seen after restore skipping, and a new deposit notifying again once the hold is lifted;AppViewModelSendFlowTest.ktcovers the firstSyncCompleted(ONCHAIN_WALLET)marking unseen activities seen before clearing the flag, a lightning sync leaving both untouched, and an onchain sync without a pending restore doing nothing;WalletViewModelTest.ktcovers Get Started setting the flag with and without the address-type prune. With the handler hold removed, the three hold tests fail withexpected:<Skip> but was:<ShowSheet(...)>; with the sync step removed, the sync test fails withWanted but not invoked: activityRepo.markAllUnseenActivitiesAsSeen().just compile,just test,just lint.