-
Notifications
You must be signed in to change notification settings - Fork 3
fix: notify on-chain receives that skip mempool #588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
CypherPoet
wants to merge
5
commits into
synonymdev:master
Choose a base branch
from
CypherPoet:fix/onchain-confirmed-received-sheet
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+75
−24
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f31d0ce
fix: notify on-chain receives that skip mempool
3df9c52
chore: rename changelog fragment
bd07776
fix: guard received sheet against duplicate presentation
479aac1
fix: suppress received sheet for restored historical receives
dd589eb
Merge remote-tracking branch 'origin/master' into fix/onchain-confirm…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,12 @@ class AppViewModel: ObservableObject { | |
| private var pendingPaymentHashes: Set<String> = [] | ||
| private var pendingContactPaymentContexts: [String: ContactPaymentContext] = [:] | ||
|
|
||
| /// Txids for which a received-sheet presentation has already been started this session. | ||
| /// The received and confirmed LDK events for the same tx each call the presenter, so this | ||
| /// reserves the txid synchronously on the MainActor (before any await) to guarantee the sheet | ||
| /// is presented at most once and avoid a double-notification race. See issue #455. | ||
| private var receivedSheetInFlightTxids: Set<String> = [] | ||
|
|
||
| /// When a payment that was shown on the pending screen succeeds or fails, this is set so SendPendingScreen can navigate. | ||
| /// Consumed by SendPendingScreen via consumeSendSheetPendingResolution. | ||
| @Published var sendSheetPendingResolution: SendSheetPendingResolution? | ||
|
|
@@ -812,6 +818,44 @@ extension AppViewModel { | |
| // MARK: LDK Node Events | ||
|
|
||
| extension AppViewModel { | ||
| /// Shows the "received" sheet for an incoming on-chain tx, unless it was already shown. | ||
| /// Used by both the received (mempool) and confirmed (straight-to-confirmed) LDK events so a | ||
| /// tx that skips the mempool still notifies the user. See issue #455. | ||
| private func presentReceivedSheetForOnchainTransaction(txid: String, amountSats: Int64) { | ||
| guard amountSats > 0 else { return } | ||
|
|
||
| // During a restore replay, LDK re-fires confirmed events for historical (already-received) txs. | ||
| // Suppress the sheet for the whole restore window; the first post-restore on-chain sync marks | ||
| // those activities seen and clears this flag, after which genuinely-new receives notify again. #588 | ||
| guard !SettingsViewModel.shared.pendingRestoreActivitySeen else { return } | ||
|
|
||
| // Reserve the txid synchronously on the MainActor (no await between check and insert) so the | ||
| // received and confirmed events for the same tx can't both pass the seen-check and present the | ||
| // sheet twice. The persisted seenAt still handles cross-launch dedup; this closes the in-session | ||
| // concurrency race. | ||
| guard receivedSheetInFlightTxids.insert(txid).inserted else { return } | ||
|
|
||
| let sats = UInt64(amountSats) | ||
|
|
||
| Task { | ||
| // 500ms delay so the activity is written to the DB before the dedup/filter checks read it. | ||
| try? await Task.sleep(nanoseconds: 500_000_000) | ||
|
|
||
| if await CoreService.shared.activity.isOnchainActivitySeen(txid: txid) { | ||
| return | ||
| } | ||
|
|
||
| let shouldShow = await CoreService.shared.activity.shouldShowReceivedSheet(txid: txid, value: sats) | ||
| guard shouldShow else { return } | ||
|
|
||
| await CoreService.shared.activity.markOnchainActivityAsSeen(txid: txid) | ||
|
|
||
| await MainActor.run { | ||
| sheetViewModel.showSheet(.receivedTx, data: ReceivedTxSheetDetails(type: .onchain, sats: sats)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func handleLdkNodeEvent(_ event: Event) { | ||
| switch event { | ||
| case let .paymentReceived(paymentId, _, amountMsat, _): | ||
|
|
@@ -922,30 +966,12 @@ extension AppViewModel { | |
| // MARK: New Onchain Transaction Events | ||
|
|
||
| case let .onchainTransactionReceived(txid, details): | ||
| // Show notification for incoming transactions | ||
| if details.amountSats > 0 { | ||
| let sats = UInt64(abs(Int64(details.amountSats))) | ||
|
|
||
| Task { | ||
| // Show sheet for new transactions or replacements with value changes | ||
| try? await Task.sleep(nanoseconds: 500_000_000) // 500ms delay | ||
|
|
||
| if await CoreService.shared.activity.isOnchainActivitySeen(txid: txid) { | ||
| return | ||
| } | ||
|
|
||
| let shouldShow = await CoreService.shared.activity.shouldShowReceivedSheet(txid: txid, value: sats) | ||
| guard shouldShow else { return } | ||
|
|
||
| await CoreService.shared.activity.markOnchainActivityAsSeen(txid: txid) | ||
|
|
||
| await MainActor.run { | ||
| sheetViewModel.showSheet(.receivedTx, data: ReceivedTxSheetDetails(type: .onchain, sats: sats)) | ||
| } | ||
| } | ||
| } | ||
| case let .onchainTransactionConfirmed(txid, _, blockHeight, _, _): | ||
| // Show notification for incoming transactions seen in the mempool | ||
| presentReceivedSheetForOnchainTransaction(txid: txid, amountSats: details.amountSats) | ||
| case let .onchainTransactionConfirmed(txid, _, blockHeight, _, details): | ||
| Logger.info("Transaction confirmed: \(txid) at block \(blockHeight)") | ||
| // Also notify when a tx goes straight to confirmed without a prior received event | ||
| presentReceivedSheetForOnchainTransaction(txid: txid, amountSats: details.amountSats) | ||
| case let .onchainTransactionReplaced(txid, conflicts): | ||
| Logger.info("Transaction replaced: \(txid) by \(conflicts.count) conflict(s)") | ||
| Task { | ||
|
|
@@ -1018,6 +1044,15 @@ extension AppViewModel { | |
| } | ||
| } | ||
|
|
||
| // After a seed restore, the first on-chain sync has now discovered the historical txs. | ||
| // Mark them seen so they don't pop a "Received" sheet, and lift the restore suppression. #588 | ||
| if SettingsViewModel.shared.pendingRestoreActivitySeen, syncType == .onchainWallet { | ||
| SettingsViewModel.shared.pendingRestoreActivitySeen = false | ||
| Task { @MainActor in | ||
| await CoreService.shared.activity.markAllUnseenActivitiesAsSeen() | ||
| } | ||
| } | ||
|
Comment on lines
+1050
to
+1054
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could move this flag it inside the Task, after the await, so it is cleared after the activities are actually marked as seen. Medium, couldn't reproduce a regression |
||
|
|
||
| if MigrationsService.shared.needsPostMigrationSync { | ||
| Task { @MainActor in | ||
| try? await CoreService.shared.activity.syncLdkNodePayments(LightningService.shared.listPayments() ?? []) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Incoming on-chain transactions that confirm before being seen in the mempool now show the received notification. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.