diff --git a/Bitkit/Services/CoreService.swift b/Bitkit/Services/CoreService.swift index bafe61e12..831b7ba9d 100644 --- a/Bitkit/Services/CoreService.swift +++ b/Bitkit/Services/CoreService.swift @@ -210,7 +210,14 @@ class ActivityService { } /// Marks every unseen activity across all wallets as seen, each under its own wallet id. - func markAllUnseenActivitiesAsSeen() async { + /// + /// `startedBefore` limits the pass to activity that already existed at that time. The restore + /// sweep passes the moment the restore began, so a payment that genuinely arrives while the + /// restore is still running keeps its unseen state and still notifies the user. #588 + /// + /// Returns `false` when the pass did not complete, so callers can keep any suppression they hold. + @discardableResult + func markAllUnseenActivitiesAsSeen(startedBefore cutoff: UInt64? = nil) async -> Bool { let timestamp = UInt64(Date().timeIntervalSince1970) do { @@ -221,16 +228,23 @@ class ActivityService { let id: String let walletId: String let isSeen: Bool + let createdAt: UInt64 switch activity { case let .onchain(onchain): id = onchain.id walletId = onchain.walletId isSeen = onchain.seenAt != nil + createdAt = onchain.timestamp case let .lightning(lightning): id = lightning.id walletId = lightning.walletId isSeen = lightning.seenAt != nil + createdAt = lightning.timestamp + } + + if let cutoff, createdAt > cutoff { + continue } if !isSeen { @@ -244,8 +258,11 @@ class ActivityService { if didMarkAny { activitiesChangedSubject.send() } + + return true } catch { Logger.error("Failed to mark all activities as seen: \(error)", context: "ActivityService") + return false } } diff --git a/Bitkit/ViewModels/AppViewModel.swift b/Bitkit/ViewModels/AppViewModel.swift index 32ff7ac06..310d3748d 100644 --- a/Bitkit/ViewModels/AppViewModel.swift +++ b/Bitkit/ViewModels/AppViewModel.swift @@ -138,6 +138,12 @@ class AppViewModel: ObservableObject { private(set) var isQuickPayActive = false private var quickPayPaymentHash: String? + /// 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 = [] + /// 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? @@ -1192,6 +1198,62 @@ extension AppViewModel { // MARK: LDK Node Events extension AppViewModel { + /// Lifts the post-restore received-sheet suppression, once the activities the first post-restore + /// on-chain sync replayed have actually been marked seen. + /// + /// The flag must outlive the marking pass: clearing it up front reopens + /// `presentReceivedSheetForOnchainTransaction` while the pass is still running, and leaves it open + /// for good if the pass fails, so a historical tx can pop a "Received" sheet. #588 + @MainActor + func completePendingRestoreActivitySeen( + markAllSeen: (UInt64) async -> Bool = { cutoff in + await CoreService.shared.activity.markAllUnseenActivitiesAsSeen(startedBefore: cutoff) + } + ) async { + let restoreStartedAt = SettingsViewModel.shared.pendingRestoreActivitySeenSince + guard restoreStartedAt > 0 else { return } + guard await markAllSeen(restoreStartedAt) else { return } + SettingsViewModel.shared.pendingRestoreActivitySeenSince = 0 + } + + /// 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, _): @@ -1334,30 +1396,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 { @@ -1430,6 +1474,14 @@ 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 syncType == .onchainWallet { + Task { @MainActor in + await self.completePendingRestoreActivitySeen() + } + } + if MigrationsService.shared.needsPostMigrationSync { Task { @MainActor in try? await CoreService.shared.activity.syncLdkNodePayments(LightningService.shared.listPayments() ?? []) diff --git a/Bitkit/ViewModels/SettingsViewModel.swift b/Bitkit/ViewModels/SettingsViewModel.swift index 32d3df71e..b1b406b28 100644 --- a/Bitkit/ViewModels/SettingsViewModel.swift +++ b/Bitkit/ViewModels/SettingsViewModel.swift @@ -458,6 +458,24 @@ class SettingsViewModel: NSObject, ObservableObject { set { UserDefaults.standard.set(newValue, forKey: Self.pendingRestoreAddressTypePruneKey) } } + private static let pendingRestoreActivitySeenSinceKey = "pendingRestoreActivitySeenSince" + + /// When a seed restore began, or 0 when no restore is being suppressed. + /// + /// Set as the restore starts, before the node is started, so the replayed historical txs cannot + /// slip a "Received" sheet through ahead of the flag. Doubles as the cutoff for the sweep that + /// marks those txs seen, so a payment arriving mid-restore is not swept up with them. Cleared in + /// AppViewModel's syncCompleted(.onchainWallet) handler once that sweep succeeds. #588 + var pendingRestoreActivitySeenSince: UInt64 { + get { UInt64(UserDefaults.standard.double(forKey: Self.pendingRestoreActivitySeenSinceKey)) } + set { UserDefaults.standard.set(Double(newValue), forKey: Self.pendingRestoreActivitySeenSinceKey) } + } + + /// Whether replayed restore activity is still being suppressed. + var pendingRestoreActivitySeen: Bool { + pendingRestoreActivitySeenSince > 0 + } + /// After restore, disables monitoring for address types with zero balance. /// Keeps nativeSegwit as primary and monitored; only types with funds stay monitored. func pruneEmptyAddressTypesAfterRestore() async { diff --git a/Bitkit/Views/Onboarding/RestoreWalletView.swift b/Bitkit/Views/Onboarding/RestoreWalletView.swift index 3d960b6c5..de979d982 100644 --- a/Bitkit/Views/Onboarding/RestoreWalletView.swift +++ b/Bitkit/Views/Onboarding/RestoreWalletView.swift @@ -257,6 +257,11 @@ struct RestoreWalletView: View { // Prevent settings changes from triggering backups before the actual restore runs BackupService.shared.setRestoring(true) + // Suppress "Received" sheets for the historical txs the restore replays. Set here, before + // the node is started, because startup sync begins as soon as the wallet exists - setting + // it on the Get Started tap left a window where replayed txs could still pop a sheet. #588 + SettingsViewModel.shared.pendingRestoreActivitySeenSince = UInt64(Date().timeIntervalSince1970) + // When restoring a wallet, monitor all address types to catch any existing funds SettingsViewModel.shared.monitorAllAddressTypes() diff --git a/Bitkit/Views/Onboarding/WalletRestoreSuccess.swift b/Bitkit/Views/Onboarding/WalletRestoreSuccess.swift index 9d852fa69..9e0d90114 100644 --- a/Bitkit/Views/Onboarding/WalletRestoreSuccess.swift +++ b/Bitkit/Views/Onboarding/WalletRestoreSuccess.swift @@ -36,8 +36,13 @@ struct WalletRestoreSuccess: View { app.backupVerified = true wallet.isRestoringWallet = false - // Skip pruning if backup had explicit monitored address types let settings = SettingsViewModel.shared + + // Note: the "Received" sheet suppression for replayed historical txs is armed when the + // restore starts, in RestoreWalletView, not here - by this tap the node has already + // been syncing for a while. #588 + + // Skip pruning if backup had explicit monitored address types if !settings.restoredMonitoredTypesFromBackup { settings.pendingRestoreAddressTypePrune = true } diff --git a/BitkitTests/MarkAllUnseenActivitiesCutoffTests.swift b/BitkitTests/MarkAllUnseenActivitiesCutoffTests.swift new file mode 100644 index 000000000..aa5035dcf --- /dev/null +++ b/BitkitTests/MarkAllUnseenActivitiesCutoffTests.swift @@ -0,0 +1,103 @@ +@testable import Bitkit +import BitkitCore +import XCTest + +/// Regression cover for #588: the post-restore sweep marks the replayed historical activity as seen, +/// and must leave alone a payment that genuinely arrives while the restore is still running. +/// +/// Without the cutoff, such a payment was suppressed by `pendingRestoreActivitySeen` on arrival and +/// then marked seen by the sweep, so it never notified the user at all. +final class MarkAllUnseenActivitiesCutoffTests: XCTestCase { + private let testDbPath = NSTemporaryDirectory() + private let service = CoreService.shared.activity + + private let restoreStartedAt: UInt64 = 1_700_000_000 + + override func setUp() async throws { + try await super.setUp() + _ = try initDb(basePath: testDbPath) + try await Task.sleep(nanoseconds: 1_000_000_000) + } + + override func tearDown() async throws { + try await super.tearDown() + + let fileManager = FileManager.default + let dbPath = (testDbPath as NSString).appendingPathComponent("activity.db") + if fileManager.fileExists(atPath: dbPath) { + try fileManager.removeItem(atPath: dbPath) + } + } + + func testSweepMarksReplayedHistoryButSparesNewerActivity() async throws { + let replayed = "restore-replayed-history" + let arrivedDuringRestore = "arrived-mid-restore" + + try await service.insert(onchainActivity(id: replayed, txId: "old", timestamp: restoreStartedAt - 3600)) + try await service.insert( + onchainActivity(id: arrivedDuringRestore, txId: "new", timestamp: restoreStartedAt + 30) + ) + + let completed = await service.markAllUnseenActivitiesAsSeen(startedBefore: restoreStartedAt) + + let replayedSeenAt = try await seenAt(of: replayed) + let newerSeenAt = try await seenAt(of: arrivedDuringRestore) + + XCTAssertTrue(completed) + XCTAssertNotNil(replayedSeenAt, "replayed history should be marked seen by the sweep") + XCTAssertNil( + newerSeenAt, + "a payment that arrived during the restore must stay unseen, or it never notifies" + ) + } + + func testSweepWithoutACutoffStillMarksEverything() async throws { + let id = "no-cutoff" + try await service.insert(onchainActivity(id: id, txId: "any", timestamp: restoreStartedAt + 30)) + + let completed = await service.markAllUnseenActivitiesAsSeen() + + let markedSeenAt = try await seenAt(of: id) + + XCTAssertTrue(completed) + XCTAssertNotNil(markedSeenAt, "the post-migration caller passes no cutoff and expects a full sweep") + } + + // MARK: - Helpers + + private func seenAt(of id: String) async throws -> UInt64? { + guard case let .onchain(activity) = try await service.getActivity(id: id) else { + XCTFail("activity \(id) was not stored as on-chain") + return nil + } + return activity.seenAt + } + + private func onchainActivity(id: String, txId: String, timestamp: UInt64) -> Activity { + .onchain( + OnchainActivity( + walletId: WalletScope.default, + id: id, + txType: .received, + txId: txId, + value: 10000, + fee: 100, + feeRate: 1, + address: "bc1...", + confirmed: true, + timestamp: timestamp, + isBoosted: false, + boostTxIds: [], + isTransfer: false, + doesExist: true, + confirmTimestamp: nil, + channelId: nil, + transferTxId: nil, + contact: nil, + createdAt: nil, + updatedAt: nil, + seenAt: nil + ) + ) + } +} diff --git a/BitkitTests/RestoreActivitySeenSuppressionTests.swift b/BitkitTests/RestoreActivitySeenSuppressionTests.swift new file mode 100644 index 000000000..83f72feb8 --- /dev/null +++ b/BitkitTests/RestoreActivitySeenSuppressionTests.swift @@ -0,0 +1,86 @@ +@testable import Bitkit +import XCTest + +/// Regression cover for #588: the post-restore received-sheet suppression must outlive the pass that +/// marks the replayed activities as seen, and that pass must not sweep up payments that arrive while +/// the restore is still running. +/// +/// Clearing `pendingRestoreActivitySeenSince` up front reopened +/// `presentReceivedSheetForOnchainTransaction` while the marking pass was still running — and kept it +/// open when the pass failed — so a historical tx replayed by LDK could pop a "Received" sheet. +@MainActor +final class RestoreActivitySeenSuppressionTests: XCTestCase { + private let flagKey = "pendingRestoreActivitySeenSince" + private let restoreStartedAt: UInt64 = 1_700_000_000 + + override func setUp() { + super.setUp() + snapshotAppDefaults(flagKey) + } + + func testSuppressionHoldsUntilTheMarkingPassFinishes() async { + SettingsViewModel.shared.pendingRestoreActivitySeenSince = restoreStartedAt + let app = AppViewModel() + var flagDuringPass: Bool? + + await app.completePendingRestoreActivitySeen { _ in + flagDuringPass = SettingsViewModel.shared.pendingRestoreActivitySeen + return true + } + + XCTAssertEqual(flagDuringPass, true, "suppression was lifted before the activities were marked seen") + XCTAssertFalse(SettingsViewModel.shared.pendingRestoreActivitySeen) + } + + func testSuppressionIsKeptWhenTheMarkingPassFails() async { + SettingsViewModel.shared.pendingRestoreActivitySeenSince = restoreStartedAt + let app = AppViewModel() + + await app.completePendingRestoreActivitySeen { _ in false } + + XCTAssertTrue( + SettingsViewModel.shared.pendingRestoreActivitySeen, + "a failed marking pass must keep the restore suppression, or replayed txs pop a sheet" + ) + } + + func testMarkingPassIsSkippedWhenNoRestoreIsPending() async { + SettingsViewModel.shared.pendingRestoreActivitySeenSince = 0 + let app = AppViewModel() + var didRunPass = false + + await app.completePendingRestoreActivitySeen { _ in + didRunPass = true + return true + } + + XCTAssertFalse(didRunPass, "every on-chain sync would re-mark all activities seen") + XCTAssertFalse(SettingsViewModel.shared.pendingRestoreActivitySeen) + } + + /// The sweep is bounded by when the restore began, so a payment that genuinely arrives mid-restore + /// keeps its unseen state instead of being marked seen along with the replayed history. + func testMarkingPassIsBoundedByTheRestoreStartTime() async { + SettingsViewModel.shared.pendingRestoreActivitySeenSince = restoreStartedAt + let app = AppViewModel() + var passedCutoff: UInt64? + + await app.completePendingRestoreActivitySeen { cutoff in + passedCutoff = cutoff + return true + } + + XCTAssertEqual(passedCutoff, restoreStartedAt) + } + + /// The suppression is armed as the restore starts, not on the Get Started tap, because startup + /// sync begins as soon as the wallet exists. + func testSuppressionFlagIsDerivedFromTheStoredStartTime() { + SettingsViewModel.shared.pendingRestoreActivitySeenSince = restoreStartedAt + XCTAssertTrue(SettingsViewModel.shared.pendingRestoreActivitySeen) + XCTAssertEqual(SettingsViewModel.shared.pendingRestoreActivitySeenSince, restoreStartedAt) + + SettingsViewModel.shared.pendingRestoreActivitySeenSince = 0 + XCTAssertFalse(SettingsViewModel.shared.pendingRestoreActivitySeen) + } +} diff --git a/changelog.d/next/588.fixed.md b/changelog.d/next/588.fixed.md new file mode 100644 index 000000000..c75daf731 --- /dev/null +++ b/changelog.d/next/588.fixed.md @@ -0,0 +1 @@ +Incoming on-chain transactions that confirm before being seen in the mempool now show the received notification.