Skip to content

fix: notify on-chain receives that skip mempool - #588

Merged
jvsena42 merged 15 commits into
synonymdev:masterfrom
CypherPoet:fix/onchain-confirmed-received-sheet
Sep 25, 2026
Merged

jvsena42 merged 15 commits into
synonymdev:masterfrom
CypherPoet:fix/onchain-confirmed-received-sheet

Conversation

@CypherPoet

@CypherPoet CypherPoet commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #455
Related: synonymdev/bitkit-android#797, Android twin synonymdev/bitkit-android#1299
Also fixes on iOS the restore case reported in synonymdev/bitkit-android#1342 (Android fix: synonymdev/bitkit-android#1343)

This PR shows the received sheet for on-chain deposits the wallet first sees already confirmed, without a prior mempool event.

Description

  • Routes onchainTransactionConfirmed to the same received-sheet flow as onchainTransactionReceived, so a deposit mined before any wallet sync saw it in the mempool is no longer silent.
  • Keeps one sheet per deposit: the txid is reserved on the MainActor before any await, and the persisted seenAt still blocks a mempool-first tx from showing again when it confirms.
  • Shows a confirmed-only receive only when its block timestamp is within one hour of the device clock and no migration is running (AppViewModel.shouldPresentConfirmedOnlyReceive, matching Android's MAX_CONFIRMED_ONLY_AGE and migration skip). A full wallet scan, as after a migration or when an address type starts being monitored, replays confirmations for old txs; those stay silent.
  • Holds every on-chain received sheet after a seed restore. RestoreWalletView sets pendingRestoreActivitySeenSince before the node starts; the first on-chain syncCompleted marks activities that existed before the restore began as seen, then clears the flag. The flag is cleared only when that pass succeeds, and the cutoff keeps a payment arriving mid-restore unseen so it still notifies. Same rule as Android.
  • Records the chain tip of that first sync (restoreSyncedBlockHeight) and skips confirmed-only sheets at or below it, so a later rescan or a late-handled event for a pre-restore tx confirmed within the hour stays silent ([Bug]: historical on-chain receive sheet stays up after mnemonic restore bitkit-android#1342). Receives that arrive while the sweep runs are held and presented once it succeeds; whatever the restore scan itself emitted is dropped as history, since a payment arriving mid-scan looks the same as a replayed unconfirmed one. A failed restore clears the hold.
  • markAllUnseenActivitiesAsSeen returns whether the pass completed and takes an optional startedBefore cutoff; the post-migration caller keeps its full sweep.
  • Adds ReceiveOnchainAddress / ReceiveLightningAddress identifiers to CopyAddressCard, matching the Android test tags, and ports the onchain-receive journeys.

Out of Scope

  • AppViewModel: a reorg that replaces a block at or below the restore tip with one paying the wallet a tx it never saw in the mempool shows no sheet; SyncCompleted reports only a height, so the replacement block cannot be told apart. A tx seen in the mempool first still notifies.
  • AppViewModel: the one-hour window uses the device clock, so a device off by more than an hour never shows a confirmed-only receive. ldk-node emits the confirmed event once, so it cannot be recovered. Same limitation as Android; the mempool path is unaffected.
  • AppViewModel: a confirmed-only deposit found more than one hour after its block (app closed while it confirmed) stays silent.
  • Background notifications: Android also posts a "Payment Received" notification from its foreground node service. iOS has no foreground node service, so confirmed-only-background-notification.xml is not ported (recorded in journeys/README.md).
  • Android additionally skips confirmed-only receives while a backup restore runs; on iOS the restore hold covers the same window.

Design

Reuses the existing received sheet with no visual change: Payment received event

Preview

bitkit-455-before-after.mp4

Regtest deposit mined immediately, so the tx goes straight to confirmed:

build received events confirmed events received sheet shown
master (before) 0 1 0
this PR (after) 0 1 1

QA Notes

Journeys

  • new confirmed-only-received-sheet.xml — a deposit mined before any sync shows the received sheet once, with only the confirmed event logged
  • new mempool-then-confirmed-single-sheet.xml — a mempool-first deposit shows one sheet and no second one when it confirms
  • new restore-recent-receive-stays-silent.xml — a deposit confirmed within the hour stays silent after restoring its seed, and a deposit mined after the restore still shows the sheet

Manual Tests

  • Update from the React Native app with on-chain history → no received sheet for migrated transactions during the post-migration sync — an installed React Native Bitkit to migrate from not in Capabilities

Automated Checks

  • added ConfirmedOnlyReceiveGuardTests.swift — confirmed-only receives show inside the one-hour window in both directions and skip outside it, during a migration, or at or below the restore tip
  • added RestoreActivitySeenSuppressionTests.swift — the restore hold outlives the marking pass, survives a failed pass, is bounded by the restore start, is skipped when no restore is pending, records the restore tip, drops receives from the restore scan, replays receives held during the sweep, and runs one sweep at a time
  • added MarkAllUnseenActivitiesCutoffTests.swift — against a real DB, the sweep marks activity older than the cutoff and leaves newer activity unseen; without a cutoff it marks everything

🤖 Generated with Claude Code

CypherPoet added 2 commits June 8, 2026 11:46
When a transaction is confirmed before the wallet sees it unconfirmed, only onchainTransactionConfirmed fires. The received sheet was wired solely to onchainTransactionReceived, so straight-to-confirmed receives showed no notification. Both handlers now share one check-and-show helper. Fixes synonymdev#455.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3df9c525b9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
@ovitrif ovitrif added this to the 2.4.0 milestone Jun 8, 2026
Routing both onchainTransactionReceived and onchainTransactionConfirmed through the shared presenter means two tasks can run for one txid. The seen-check and mark were not atomic across awaits, so both events could present the sheet. Reserve the txid synchronously on the MainActor before any await so only the first event presents it. Addresses the PR synonymdev#588 review.
@ovitrif

ovitrif commented Jun 9, 2026 •

Copy link
Copy Markdown
Collaborator

Hint: As a general note, we expect PR authors to Resolve the bot comments conversation thread after addressing them, via either GitHub web UI or directly via AI agents 😉.

I know some teams might get some members flamey over this, not the case with us.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

Got failure from the e2e test. After CPFP boost and subsequent restoration of the wallet we got the notification of previously received transaction in UI, pls see recording:

boost_-_Boost_boost_1_-_Can_do_CPFP-2026-06-10T21-22-29-359Z.mp4

Attaching also all artifacts that include simulator logs.
e2e-artifacts_e2e_1528.zip

On a wallet restore the activity store is rebuilt without seenAt, so the
initial on-chain sync replays onchainTransactionConfirmed for historical
receives. The shared presenter then showed the "Received" sheet for an old
transaction, which covered the activity list and timed out the CPFP restore
e2e. Set a pendingRestoreActivitySeen flag on the restore success screen, have
the presenter bail while it is set, and on the first post-restore on-chain
syncCompleted mark all unseen activities as seen and clear the flag. New
straight-to-confirmed receives still notify once the flag clears, so synonymdev#455 stays
fixed. Addresses the PR synonymdev#588 review.
@CypherPoet

Copy link
Copy Markdown
Contributor Author

Got failure from the e2e test. After CPFP boost and subsequent restoration of the wallet we got the notification of previously received transaction in UI, pls see recording:

boost_-Boost_boost_1-_Can_do_CPFP-2026-06-10T21-22-29-359Z.mp4
Attaching also all artifacts that include simulator logs. e2e-artifacts_e2e_1528.zip

@piotr-iohk I did some more digging on this, and it definitely seems to be an issue.

Root cause: on a wallet restore the activity store is rebuilt with no seenAt timestamps. During the
initial on-chain sync, LDK replays onchainTransactionConfirmed for historical (already received)
transactions. The new shared presenter then sees a positive amount, an unseen activity, and
shouldShowReceivedSheet == true, so it pops the "Received" sheet for an old transaction. That sheet
covered the activity list, which is why @boost_1 - Can do CPFP timed out after the restore step. A
normal launch was never affected (persisted seenAt already marks historical transactions as seen), and
migration restores already call markAllUnseenActivitiesAsSeen() on sync completion. Standard seed
restore had no equivalent.

Fix: a short-lived suppression flag that mirrors the existing migration handling, scoped to seed restore.

  • Set pendingRestoreActivitySeen when the user taps Get Started on the restore success screen.
  • While it is set, the received-sheet presenter returns early, so replayed historical confirmations do
    not notify.
  • On the first post-restore on-chain syncCompleted, mark all unseen activities as seen and clear the flag.

After the flag clears, genuinely new straight-to-confirmed receives notify again, so #455 stays fixed.

Verified on regtest:

  • Receive + confirm 100k, back up, wipe, restore: the historical receive lands in the activity list with
    no "Received" sheet, and the list is reachable (repro of the e2e failure, now passing).
  • A new deposit after restore still shows the "Received" sheet once (flag cleared correctly).
  • [Bug]: no received transaction notification when tx confirms without being seen in mempool #455 still holds: a new straight-to-confirmed receive notifies; mempool-first receives notify once and
    not again on confirmation.

The @boost_1 e2e should pass now.

@piotr-iohk piotr-iohk modified the milestones: 2.4.0, 2.5.0 Jul 7, 2026
@jvsena42 jvsena42 self-assigned this Jul 29, 2026

@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 overall is good, no issues found in the tests

Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
@ovitrif ovitrif modified the milestones: 2.5.0, 2.6.0 Sep 9, 2026
@jvsena42
jvsena42 marked this pull request as draft September 10, 2026 10:12
@jvsena42
jvsena42 marked this pull request as ready for review September 23, 2026 12:06
@jvsena42
jvsena42 requested review from a team, piotr-iohk and pwltr and removed request for a team September 23, 2026 12:07
@greptile-apps

greptile-apps Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5

The PR is not yet safe to merge because restore failures can leave receive notifications persistently disabled, and sync completion can clear suppression before historical activities are persisted.

Findings

  1. P1 Restore Failure Leaves Hold ▶
  2. P1 Sync Clears Hold Too Early ▶

Summary

This PR routes recent confirmed-only on-chain deposits through the received-sheet flow, adds in-session transaction deduplication, and suppresses historical restore activity using a persisted restore-start cutoff.

  • Adds confirmed-only receive eligibility checks and regression coverage.
  • Marks pre-restore activities seen after the first on-chain sync while preserving newer activity.
  • Adds receive-address accessibility identifiers and on-chain receive journeys.
  • The updated restore cleanup still has two ordering/lifecycle gaps that can either suppress legitimate notifications indefinitely or surface replayed historical deposits.

Diagram

sequenceDiagram
    participant LDK
    participant LightningService
    participant ActivityDB
    participant AppViewModel
    participant Sheet

    LDK->>LightningService: historical transaction event
    LightningService-->>ActivityDB: launch unstructured persistence task
    LDK->>AppViewModel: syncCompleted(onchainWallet)
    AppViewModel->>ActivityDB: mark pre-restore unseen activities
    ActivityDB-->>AppViewModel: pass completed
    AppViewModel->>AppViewModel: clear restore suppression
    Note over ActivityDB,AppViewModel: Transaction persistence may still be pending
    ActivityDB-->>ActivityDB: persist historical activity as unseen
    LDK->>AppViewModel: delayed confirmed event
    AppViewModel->>Sheet: historical "Received" sheet may appear
Loading

Reviews (2) · Last reviewed commit: "Merge remote-tracking branch 'origin/mas..."

Comment thread Bitkit/Views/Onboarding/WalletRestoreSuccess.swift Outdated
Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jvsena42
jvsena42 marked this pull request as draft September 23, 2026 17:18
jvsena42 and others added 3 commits September 24, 2026 13:03
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>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ed-received-sheet

# Conflicts:
#	journeys/README.md
@jvsena42
jvsena42 self-requested a review September 24, 2026 16:11
@jvsena42
jvsena42 marked this pull request as ready for review September 24, 2026 16:11
jvsena42
jvsena42 previously approved these changes Sep 24, 2026
@jvsena42
jvsena42 requested review from a team, piotr-iohk and pwltr and removed request for a team September 24, 2026 16:12
Comment thread Bitkit/Views/Onboarding/RestoreWalletView.swift
Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
Comment thread Bitkit/ViewModels/AppViewModel.swift Outdated
@piotr-iohk

Copy link
Copy Markdown
Collaborator

QA reviewed on 33cf753.

Agree with the open comment on the restore hold: a new mempool receive during seed restore returns before the txid is reserved, the cutoff sweep leaves it unseen, and clearing the hold does not present it.

This head also makes a late confirmation of that same receive silent. shouldPresentConfirmedOnlyReceive skips a confirmed event whose block time is more than an hour from the device clock, so the later confirm is not a second chance.

The one-hour window and the migration skip otherwise match merged bitkit-android#1299. The background notification journey is correctly not ported. Run Tests passed. No simulator run in this pass. The React Native migration case is still unchecked.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

@jvsena42 Android nightly on master 44c82ed reproduced a historical Received Bitcoin sheet after mnemonic restore. Filed as synonymdev/bitkit-android#1342.

This PR has the same one-hour window and the same restore hold. A receive confirmed within the last hour can leave that sheet up if the event is handled after the hold is cleared. Not reproduced on iOS.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jvsena42

Copy link
Copy Markdown
Member

@piotr-iohk thanks — fixed here in a84277ad: the first post-restore on-chain sync records its tip, and confirmed-only sheets at or below it are skipped, so a historical receive confirmed within the hour stays silent however late its event is handled. Android fix to follow on synonymdev/bitkit-android#1342.

jvsena42 and others added 2 commits September 25, 2026 07:18
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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.

Since my last pass: 6c8c252 ports the restore journey and e86d0ae adds the hold log line it greps for. a84277a answers the three earlier threads. A restore failure now clears the hold, the restore tip is recorded only after markAllSeen succeeds, and receives arriving during the sweep are held and replayed. One LOW refinement is on the replay thread: held receives are dropped after a failed sweep. Not independently verified.

Checked and clean:

  • Mempool-then-confirmed double sheet: blocked in-session by receivedSheetInFlightTxids and across relaunch by persisted seenAt. ldk-node writes the payment store before emitting the events, so the activity exists when it's marked seen.
  • SyncCompleted{OnchainWallet} is emitted only after a successful sync that covers every monitored address type (ldk-node 0.7.0-rc.66), so the recorded tip only silences blocks the restore scanned.
  • Sends, self-transfers, channel/transfer txs and value-preserving RBF are filtered by amountSats <= 0, shouldShowReceivedSheet and boostTxIds, the same as the mempool path.
  • Cutoff sweep: restored confirmed history gets the block timestamp, so it's swept; a mid-restore mempool receive keeps a later timestamp and stays unseen.
  • The hold keys survive the backup's settings restore because SettingsBackupConfig doesn't list them, and AppReset.wipe clears them.
  • Kill mid-hold: the first onchain sync after relaunch sweeps with the original cutoff.
  • The confirmed-only guard matches Android's canShowConfirmedOnly: migration skip, 1h window, tip guard.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

Update from the React Native app with on-chain history → no received sheet for migrated transactions during the post-migration sync — an installed React Native Bitkit to migrate from not in Capabilities

Ran manually ✅

Screen.Recording.2026-09-25.at.14.44.21.mov

@jvsena42
jvsena42 enabled auto-merge September 25, 2026 14:10
@ovitrif ovitrif removed this from the 2.6.0 milestone Sep 25, 2026
@jvsena42
jvsena42 merged commit 38f1ad2 into synonymdev:master Sep 25, 2026
14 checks passed
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.

[Bug]: no received transaction notification when tx confirms without being seen in mempool

5 participants