diff --git a/app/src/main/java/to/bitkit/data/SettingsStore.kt b/app/src/main/java/to/bitkit/data/SettingsStore.kt index 12220c64cc..e11f9e8750 100644 --- a/app/src/main/java/to/bitkit/data/SettingsStore.kt +++ b/app/src/main/java/to/bitkit/data/SettingsStore.kt @@ -58,7 +58,10 @@ class SettingsStore @Inject constructor( store.updateData { current -> // The received-sheet hold is armed before the backup is read, so it has to survive the // settings the backup brings with it - otherwise the replayed history raises sheets. - data.copy(pendingRestoreActivitySeenSince = current.pendingRestoreActivitySeenSince) + data.copy( + pendingRestoreActivitySeenSince = current.pendingRestoreActivitySeenSince, + restoreSyncedBlockHeight = current.restoreSyncedBlockHeight, + ) } val monitored = data.addressTypesToMonitor @@ -212,6 +215,14 @@ data class SettingsData( * them. Cleared by the first on-chain sync completion whose sweep succeeds. */ val pendingRestoreActivitySeenSince: Long = 0, + /** + * Chain tip of the first on-chain sync after the latest seed restore, or 0 when none completed. + * + * Everything confirmed at or below it was already on chain when the restore scanned the wallet, so it outlives + * [pendingRestoreActivitySeenSince]: LDK events are handled concurrently, and a later rescan replays those + * confirmations too, so their received sheets must stay silent however late they are handled. + */ + val restoreSyncedBlockHeight: Long = 0, ) { val pendingRestoreActivitySeen: Boolean get() = pendingRestoreActivitySeenSince > 0 } diff --git a/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceived.kt b/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceived.kt index 61cb0737ac..468ac7eac1 100644 --- a/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceived.kt +++ b/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceived.kt @@ -17,13 +17,14 @@ sealed interface NotifyPaymentReceived { /** * An incoming onchain transaction. [confirmationTime] is the block timestamp in seconds since the - * UNIX epoch, set when the wallet first saw the transaction already confirmed without a prior - * mempool event. + * UNIX epoch and [blockHeight] the confirming block, both set when the wallet first saw the + * transaction already confirmed without a prior mempool event. */ data class Onchain( val txid: String, val details: TransactionDetails, val confirmationTime: ULong? = null, + val blockHeight: UInt? = null, override val includeNotification: Boolean = false, ) : Command { val isConfirmedOnly: Boolean get() = confirmationTime != null @@ -47,6 +48,7 @@ sealed interface NotifyPaymentReceived { txid = event.txid, details = event.details, confirmationTime = event.confirmationTime, + blockHeight = event.blockHeight, includeNotification = includeNotification, ) diff --git a/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandler.kt b/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandler.kt index 26552f880b..d4abab8103 100644 --- a/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandler.kt +++ b/app/src/main/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandler.kt @@ -4,6 +4,7 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext +import to.bitkit.data.SettingsData import to.bitkit.data.SettingsStore import to.bitkit.di.IoDispatcher import to.bitkit.ext.nowMillis @@ -112,16 +113,19 @@ class NotifyPaymentReceivedHandler @Inject constructor( } private suspend fun shouldShowOnchain(command: NotifyPaymentReceived.Command.Onchain): Boolean { + // One snapshot for both restore checks: the sweep lifts the hold and records the restore tip in the same + // update, so a snapshot either still holds the receive or already knows which blocks are history. + val settings = settingsStore.data.first() if (command.isConfirmedOnly) { if (command.details.amountSats <= 0) return false - if (!canShowConfirmedOnly(command)) return false + if (!canShowConfirmedOnly(command, settings)) return false applyConfirmationIfMissing(command) } else { activityRepo.handleOnchainTransactionReceived(command.txid, command.details) if (command.details.amountSats <= 0) return false } - if (settingsStore.data.first().pendingRestoreActivitySeen) { + if (settings.pendingRestoreActivitySeen) { Logger.debug("Skipping onchain receive '${command.txid}' until the first sync after restore", context = TAG) return false } @@ -142,25 +146,31 @@ class NotifyPaymentReceivedHandler @Inject constructor( activityRepo.handleOnchainTransactionConfirmed(command.txid, command.details) } - private suspend fun canShowConfirmedOnly(command: NotifyPaymentReceived.Command.Onchain): Boolean { - val confirmationTime = command.confirmationTime ?: return false - if (backupRepo.isRestoring.value) { - Logger.debug("Skipping confirmed-only receive '${command.txid}' during restore", context = TAG) - return false - } - if (migrationService.isShowingMigrationLoading.value || migrationService.needsPostMigrationSync()) { - Logger.debug("Skipping confirmed-only receive '${command.txid}' during migration", context = TAG) - return false - } + private suspend fun canShowConfirmedOnly( + command: NotifyPaymentReceived.Command.Onchain, + settings: SettingsData, + ): Boolean { + val skipReason = confirmedOnlySkipReason(command, settings) ?: return true + Logger.debug("Skipping confirmed-only receive '${command.txid}' $skipReason", context = TAG) + return false + } + + private suspend fun confirmedOnlySkipReason( + command: NotifyPaymentReceived.Command.Onchain, + settings: SettingsData, + ): String? { + val confirmationTime = command.confirmationTime ?: return "without a confirmation time" + val blockHeight = command.blockHeight ?: return "without a block height" val age = nowMillis(clock).milliseconds - confirmationTime.toLong().seconds - if (age.absoluteValue > MAX_CONFIRMED_ONLY_AGE) { - Logger.debug( - "Skipping confirmed-only receive '${command.txid}' confirmed at '$confirmationTime'", - context = TAG, - ) - return false + return when { + blockHeight.toLong() <= settings.restoreSyncedBlockHeight -> + "at height '$blockHeight', scanned by the restore" + backupRepo.isRestoring.value -> "during restore" + migrationService.isShowingMigrationLoading.value || migrationService.needsPostMigrationSync() -> + "during migration" + age.absoluteValue > MAX_CONFIRMED_ONLY_AGE -> "confirmed at '$confirmationTime'" + else -> null } - return true } private suspend fun markAsSeen(command: NotifyPaymentReceived.Command) { diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt index 8c4581b76b..5c160b396b 100644 --- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt +++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt @@ -401,6 +401,7 @@ class AppViewModel @Inject constructor( registerSheet(highBalanceSheet) } private var isCompletingMigration = false + private var isCompletingRestoreHold = false private var addressValidationJob: Job? = null private var lastPrivatePaykitContactKeys: Set = emptySet() private val isPaykitEnabled = settingsStore.isPaykitEnabled @@ -1452,7 +1453,7 @@ class AppViewModel @Inject constructor( } private suspend fun handleSyncCompleted(event: Event.SyncCompleted) { - if (event.syncType == SyncType.ONCHAIN_WALLET) completePendingRestoreActivitySeen() + if (event.syncType == SyncType.ONCHAIN_WALLET) completePendingRestoreActivitySeen(event.syncedBlockHeight) val isShowingLoading = migrationService.isShowingMigrationLoading.value val isRestoringRemote = migrationService.isRestoringFromRNRemoteBackup.value @@ -1483,13 +1484,27 @@ class AppViewModel @Inject constructor( } } - private suspend fun completePendingRestoreActivitySeen() { - val restoreStartedAt = settingsStore.data.first().pendingRestoreActivitySeenSince - if (restoreStartedAt <= 0) return - Logger.info("Marking activities replayed by the first sync after restore as seen", context = TAG) - // Bounded by the restore start so a payment arriving mid-restore keeps its unseen state. - activityRepo.markAllUnseenActivitiesAsSeen(startedBefore = restoreStartedAt.toULong()).onSuccess { - settingsStore.update { settings -> settings.copy(pendingRestoreActivitySeenSince = 0) } + private suspend fun completePendingRestoreActivitySeen(syncedBlockHeight: UInt) { + // Claimed before the first suspension, so a later sync completing while this sweep runs cannot record its + // higher tip and silence a new receive confirmed in between. + if (isCompletingRestoreHold) return + isCompletingRestoreHold = true + try { + val restoreStartedAt = settingsStore.data.first().pendingRestoreActivitySeenSince + if (restoreStartedAt <= 0) return + Logger.info("Marking activities replayed by the first sync after restore as seen", context = TAG) + // Bounded by the restore start so a payment arriving mid-restore keeps its unseen state. + activityRepo.markAllUnseenActivitiesAsSeen(startedBefore = restoreStartedAt.toULong()).onSuccess { + settingsStore.update { settings -> + if (!settings.pendingRestoreActivitySeen) return@update settings + settings.copy( + pendingRestoreActivitySeenSince = 0, + restoreSyncedBlockHeight = syncedBlockHeight.toLong(), + ) + } + } + } finally { + isCompletingRestoreHold = false } } diff --git a/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt index 7ed466d66d..8106dd3b25 100644 --- a/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt +++ b/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt @@ -568,7 +568,9 @@ class WalletViewModel @Inject constructor( backupRepo.setRestorePending(true) // Same reason for the received-sheet hold: by the time the user taps continue the node has // already been replaying historical transactions for a while. - settingsStore.update { it.copy(pendingRestoreActivitySeenSince = nowTimestamp().epochSecond) } + settingsStore.update { + it.copy(pendingRestoreActivitySeenSince = nowTimestamp().epochSecond, restoreSyncedBlockHeight = 0) + } walletRepo.restoreWallet( mnemonic = mnemonic, @@ -576,6 +578,8 @@ class WalletViewModel @Inject constructor( ).onFailure { // Nothing reaches restoreFromBackup when the wallet was never created, so release here. backupRepo.setRestorePending(false) + // No node starts, so no sync would ever lift the received-sheet hold. + settingsStore.update { it.copy(pendingRestoreActivitySeenSince = 0) } ToastEventBus.send(it) } } diff --git a/app/src/test/java/to/bitkit/androidServices/LightningNodeServiceTest.kt b/app/src/test/java/to/bitkit/androidServices/LightningNodeServiceTest.kt index a8fc569efe..a536f6e1d3 100644 --- a/app/src/test/java/to/bitkit/androidServices/LightningNodeServiceTest.kt +++ b/app/src/test/java/to/bitkit/androidServices/LightningNodeServiceTest.kt @@ -490,6 +490,7 @@ class LightningNodeServiceTest : BaseUnitTest() { txid = "confirmed_txid", details = details, confirmationTime = 0uL, + blockHeight = 100u, includeNotification = true, ) verify(notifyPaymentReceivedHandler).invoke(expectedCommand) diff --git a/app/src/test/java/to/bitkit/data/SettingsStoreTest.kt b/app/src/test/java/to/bitkit/data/SettingsStoreTest.kt index f780584f8e..2ef7d2e34c 100644 --- a/app/src/test/java/to/bitkit/data/SettingsStoreTest.kt +++ b/app/src/test/java/to/bitkit/data/SettingsStoreTest.kt @@ -97,4 +97,14 @@ class SettingsStoreTest : BaseUnitTest() { assertEquals(restoreStartedAt, sut.data.first().pendingRestoreActivitySeenSince) assertTrue(sut.data.first().pendingRestoreActivitySeen) } + + @Test + fun `restoring settings keeps the restore tip recorded on this device`() = test { + sut.update { it.copy(restoreSyncedBlockHeight = 900L) } + val backup = SettingsBackupV1(createdAt = 0L, settings = SettingsData(restoreSyncedBlockHeight = 5L)) + + assertTrue(sut.restoreFromBackup(backup).isSuccess) + + assertEquals(900L, sut.data.first().restoreSyncedBlockHeight) + } } diff --git a/app/src/test/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandlerTest.kt b/app/src/test/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandlerTest.kt index 533adde3bb..a76ca21675 100644 --- a/app/src/test/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandlerTest.kt +++ b/app/src/test/java/to/bitkit/domain/commands/NotifyPaymentReceivedHandlerTest.kt @@ -49,6 +49,7 @@ class NotifyPaymentReceivedHandlerTest : BaseUnitTest() { /** Stands in for the epoch second a seed restore began. */ private const val RESTORE_STARTED_AT = 1_700_000_000L + private const val RESTORE_SYNCED_HEIGHT = 900u } private val context: Context = mock() @@ -583,6 +584,43 @@ class NotifyPaymentReceivedHandlerTest : BaseUnitTest() { verify(activityRepo, never()).markOnchainActivityAsSeen("txidHistorical", WalletScope.default) } + @Test + fun `confirmed-only onchain receive scanned by the restore returns Skip after the hold lifts`() = test { + // #1342: LDK events are handled concurrently, so a replayed confirmation can reach the handler after the + // first sync already lifted the hold, and a later rescan replays it again. + settingsData.value = SettingsData(restoreSyncedBlockHeight = RESTORE_SYNCED_HEIGHT.toLong()) + val details = TransactionDetails(amountSats = 5000L, inputs = emptyList(), outputs = emptyList()) + whenever(activityRepo.shouldShowReceivedSheet(any(), any())).thenReturn(true) + val command = confirmedCommand( + txid = "txidHistorical", + details = details, + age = Duration.ZERO, + blockHeight = RESTORE_SYNCED_HEIGHT, + ) + + val result = sut(command).getOrThrow() + + assertEquals(NotifyPaymentReceived.Result.Skip, result) + verify(activityRepo, never()).shouldShowReceivedSheet(any(), any()) + } + + @Test + fun `confirmed-only onchain receive above the restore tip shows the sheet`() = test { + settingsData.value = SettingsData(restoreSyncedBlockHeight = RESTORE_SYNCED_HEIGHT.toLong()) + val details = TransactionDetails(amountSats = 5000L, inputs = emptyList(), outputs = emptyList()) + whenever(activityRepo.shouldShowReceivedSheet(any(), any())).thenReturn(true) + val command = confirmedCommand( + txid = "txidNew", + details = details, + age = Duration.ZERO, + blockHeight = RESTORE_SYNCED_HEIGHT + 1u, + ) + + val result = sut(command).getOrThrow() + + assertTrue(result is NotifyPaymentReceived.Result.ShowSheet) + } + @Test fun `from maps a confirmed onchain event to a confirmed-only command`() { val details = TransactionDetails(amountSats = 5000L, inputs = emptyList(), outputs = emptyList()) @@ -601,6 +639,7 @@ class NotifyPaymentReceivedHandlerTest : BaseUnitTest() { txid = "txidMapped", details = details, confirmationTime = 1_700_000_000uL, + blockHeight = 100u, includeNotification = true, ), command, @@ -623,10 +662,12 @@ class NotifyPaymentReceivedHandlerTest : BaseUnitTest() { details: TransactionDetails, age: Duration, includeNotification: Boolean = false, + blockHeight: UInt = RESTORE_SYNCED_HEIGHT + 1u, ) = NotifyPaymentReceived.Command.Onchain( txid = txid, details = details, confirmationTime = (NOW - age).epochSeconds.toULong(), + blockHeight = blockHeight, includeNotification = includeNotification, ) } diff --git a/app/src/test/java/to/bitkit/ui/WalletViewModelTest.kt b/app/src/test/java/to/bitkit/ui/WalletViewModelTest.kt index 12838a406b..ab775dcb10 100644 --- a/app/src/test/java/to/bitkit/ui/WalletViewModelTest.kt +++ b/app/src/test/java/to/bitkit/ui/WalletViewModelTest.kt @@ -225,6 +225,17 @@ class WalletViewModelTest : BaseUnitTest() { assertTrue(settingsData.value.pendingRestoreActivitySeenSince > 0) } + @Test + fun `restoreWallet should release the received sheet hold when the restore fails`() = test { + whenever(walletRepo.restoreWallet(any(), anyOrNull())).thenReturn(Result.failure(AppError("restore failed"))) + val settingsData = stubSettingsUpdate() + + sut.restoreWallet("test_mnemonic", null) + advanceUntilIdle() + + assertFalse(settingsData.value.pendingRestoreActivitySeen) + } + @Test fun `addTagToSelected should call walletRepo addTagToSelected`() = test { sut.addTagToSelected("test_tag") diff --git a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt index 148c33909b..1739776e7a 100644 --- a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt +++ b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt @@ -4501,6 +4501,7 @@ class AppViewModelSendFlowTest : BaseUnitTest() { txid = "confirmed-txid", details = details, confirmationTime = 0uL, + blockHeight = 100u, ) inOrder(activityRepo, notifyPaymentReceivedHandler) { verify(activityRepo).handleOnchainTransactionConfirmed("confirmed-txid", details) @@ -4525,6 +4526,7 @@ class AppViewModelSendFlowTest : BaseUnitTest() { verify(settingsStore).update(any()) } assertFalse(settingsData.value.pendingRestoreActivitySeen) + assertEquals(100L, settingsData.value.restoreSyncedBlockHeight) } @Test @@ -4538,6 +4540,29 @@ class AppViewModelSendFlowTest : BaseUnitTest() { verify(activityRepo).markAllUnseenActivitiesAsSeen(eq(RESTORE_STARTED_AT.toULong())) assertTrue(settingsData.value.pendingRestoreActivitySeen) + assertEquals(0L, settingsData.value.restoreSyncedBlockHeight) + } + + @Test + fun `a later onchain sync during the restore sweep does not overwrite the restore tip`() = test { + settingsData.value = SettingsData(pendingRestoreActivitySeenSince = RESTORE_STARTED_AT) + val sweep = CompletableDeferred() + whenever { activityRepo.markAllUnseenActivitiesAsSeen(eq(RESTORE_STARTED_AT.toULong())) } + .doSuspendableAnswer { + sweep.await() + Result.success(Unit) + } + + emitNodeEvent(Event.SyncCompleted(syncType = SyncType.ONCHAIN_WALLET, syncedBlockHeight = 100u)) + runCurrent() + emitNodeEvent(Event.SyncCompleted(syncType = SyncType.ONCHAIN_WALLET, syncedBlockHeight = 101u)) + runCurrent() + sweep.complete(Unit) + advanceUntilIdle() + + verify(activityRepo).markAllUnseenActivitiesAsSeen(eq(RESTORE_STARTED_AT.toULong())) + assertEquals(100L, settingsData.value.restoreSyncedBlockHeight) + assertFalse(settingsData.value.pendingRestoreActivitySeen) } @Test diff --git a/changelog.d/next/1342.fixed.md b/changelog.d/next/1342.fixed.md new file mode 100644 index 0000000000..b953b4416f --- /dev/null +++ b/changelog.d/next/1342.fixed.md @@ -0,0 +1 @@ +A historical on-chain payment no longer shows a Received sheet after restoring a wallet from its recovery phrase. diff --git a/journeys/onchain-receive/README.md b/journeys/onchain-receive/README.md index 69476d5890..fe6d651e8e 100644 --- a/journeys/onchain-receive/README.md +++ b/journeys/onchain-receive/README.md @@ -9,13 +9,15 @@ it in the mempool produces only the confirmed event. Both events go through the background. A confirmed-only receive is shown only when its block timestamp is within one hour of the device -clock and no restore or migration is running. After a seed restore, Get Started sets -`pendingRestoreActivitySeen`, which holds every onchain received sheet and notification until the +clock and no restore or migration is running. A seed restore sets +`pendingRestoreActivitySeen` as it starts, which holds every onchain received sheet and notification until the first onchain sync completes; that sync marks all unseen activities as seen and clears the flag, so the transactions it discovered stay silent when they later confirm while new deposits notify again. The same rule ships on iOS in bitkit-ios#588. A full scan after a restore also replays old -confirmations, which the one-hour window keeps silent. Neither case can be driven on a funded -device; both are covered by `NotifyPaymentReceivedHandlerTest.kt` and `AppViewModelSendFlowTest.kt`. +confirmations, which the one-hour window keeps silent. Because LDK events are handled concurrently, a +replayed confirmation can reach the handler after the hold is lifted, so that sync also records its +chain tip and confirmed-only receives at or below it stay silent (#1342). The restore journey needs a +throwaway emulator, since it wipes the app and restores a public test seed. ## Preconditions diff --git a/journeys/onchain-receive/restore-recent-receive-stays-silent.xml b/journeys/onchain-receive/restore-recent-receive-stays-silent.xml new file mode 100644 index 0000000000..69ffe7835b --- /dev/null +++ b/journeys/onchain-receive/restore-recent-receive-stays-silent.xml @@ -0,0 +1,31 @@ + + + Covers issue #1342. A deposit confirmed within the last hour, restored from its seed, must not show + the received sheet: its confirmation is inside the one-hour window, and LDK events are handled + concurrently, so it could reach the handler after the first sync had lifted the restore hold. The + first onchain sync after a restore records its chain tip, and confirmed-only receives at or below + it stay silent. A deposit mined after the restore still shows the sheet. + + Precondition: a throwaway emulator (both halves wipe the app with `adb shell pm clear`; never run + this on a device holding a wallet you need). The wallet is the public BIP39 test vector + "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about", so + other developers may have funded it on the shared regtest chain; only the deposit made here is + asserted on. Enter the words with the host-clipboard paste described in + restore-wallet/paste-seed-fragment.xml, never with `adb shell input text`. Run the whole journey + within the hour, or the one-hour window alone keeps the sheet silent and the run proves nothing. + + + Run: adb shell pm clear to.bitkit.dev, launch to.bitkit.dev, tap "Continue", "SkipIntro", "RestoreWallet" and "MultipleDevices-button" + Paste the test vector into "Word-0" as in restore-wallet/paste-seed-fragment.xml, tap "RestoreButton", then "GetStartedButton", and dismiss any intro sheet until the home screen shows (testTag "TotalBalance") + Tap Receive (testTag "Receive"), tap the "Savings" receive tab, tap "Show Details" (testTag "ShowDetails"), read the address from testTag "ReceiveOnchainAddress", and press back to the home screen + Run in one command: ./lsp POST /regtest/chain/deposit '{"address":"<savings addr>","amountSat":21342}' && ./lsp POST /regtest/chain/mine '{"count":1}' + Verify within 30s the received sheet (testTag "ReceivedTransaction") shows 21 342 sats, then tap "ReceivedTransactionButton" + Run: adb shell pm clear to.bitkit.dev, launch to.bitkit.dev and restore the same test vector the same way, tap "GetStartedButton" and dismiss any intro sheet + Wait 60s on the home screen + Verify the received sheet (testTag "ReceivedTransaction") never appeared and the home screen shows "TotalBalance" + Run: adb logcat -d -s APP:V | grep "Skipping.*<txid>" + Verify the log skipped the deposit's txid, as "scanned by the restore", "until the first sync after restore" or "during restore" — which one depends on whether its confirmation was handled before or after the restore hold lifted + Read a new address as above and run in one command: ./lsp POST /regtest/chain/deposit '{"address":"<savings addr>","amountSat":21343}' && ./lsp POST /regtest/chain/mine '{"count":1}' + Verify within 30s the received sheet (testTag "ReceivedTransaction") shows 21 343 sats + +