From 5da96b65afc1b60bb9e50aa179b7fdec5c7be790 Mon Sep 17 00:00:00 2001 From: jeremytsng Date: Mon, 31 Aug 2026 15:05:33 +0800 Subject: [PATCH] fix(bitcoin-wallet-snap): repair drifted wallets with a one-time full rescan A full scan runs only once in an account's life, at creation, so a wallet whose funds landed outside the revealed set stays wrong forever. The first regular sync after this update schedules one full scan per existing account, gated by a rescanV1 state marker set after scheduling so a crash retries with duplicate scans instead of silently skipping the repair. Repair scans emit a Scan Discovered Missed Transactions tracking event for each transaction routine sync did not know about, which measures whether the coverage fixes hold in the field. --- packages/bitcoin-wallet-snap/CHANGELOG.md | 5 + .../bitcoin-wallet-snap/src/entities/snap.ts | 1 + .../src/handlers/CronHandler.test.ts | 125 ++++++++++++++++++ .../src/handlers/CronHandler.ts | 76 +++++++++-- 4 files changed, 193 insertions(+), 14 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 1aca0c3a5..39fe8ce2d 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Run a one-time full scan of every existing account after the update, so funds on previously unwatched addresses are found ([#226](https://github.com/MetaMask/internal-snaps/pull/226)) +- Emit a `Scan Discovered Missed Transactions` tracking event when the repair scan finds transactions that routine sync did not know about ([#226](https://github.com/MetaMask/internal-snaps/pull/226)) + ### Changed - Split the chain `stopGap` configuration into `{ discovery: 5, scan: 20 }` so account discovery keeps the cheap probe while full account scans use the BIP44 gap limit ([#224](https://github.com/MetaMask/internal-snaps/pull/224)) diff --git a/packages/bitcoin-wallet-snap/src/entities/snap.ts b/packages/bitcoin-wallet-snap/src/entities/snap.ts index 386167a5f..b8eaa26d1 100644 --- a/packages/bitcoin-wallet-snap/src/entities/snap.ts +++ b/packages/bitcoin-wallet-snap/src/entities/snap.ts @@ -51,6 +51,7 @@ export const TrackingSnapEvent = { TransactionReceived: 'Transaction Received', TransactionReorged: 'Transaction Reorged', TransactionSubmitted: 'Transaction Submitted', + ScanDiscoveredMissedTransactions: 'Scan Discovered Missed Transactions', } as const; export type TrackingSnapEvent = diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 224e9bf12..a42c6b451 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -32,6 +32,9 @@ describe('CronHandler', () => { clientVersion: '1.0.0', platformVersion: '1.0.0', }); + // Default the one-time rescan to already-done so existing tests don't + // trigger background-event scheduling. + mockSnapClient.getState.mockResolvedValue(true); }); describe('synchronizeAccounts', () => { @@ -148,6 +151,63 @@ describe('CronHandler', () => { mockSnapClient.emitAccountBalancesUpdatedEvent, ).toHaveBeenCalledWith([mockAccounts[0]]); }); + + describe('one-time rescan repair', () => { + beforeEach(() => { + (getSelectedAccounts as jest.Mock).mockResolvedValue([ + 'account-1', + 'account-2', + ]); + mockAccountUseCases.list.mockResolvedValue(mockAccounts); + mockAccountUseCases.synchronize.mockResolvedValue({ + account: mockAccount1, + transactionsToNotify: [], + }); + }); + + it('schedules one full scan per existing account and marks the repair done when not yet run', async () => { + mockSnapClient.getState.mockResolvedValue(null); + + await handler.route(request); + + expect(mockSnapClient.getState).toHaveBeenCalledWith('rescanV1'); + expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledTimes( + mockAccounts.length, + ); + expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ + duration: 'PT5S', + method: CronMethod.FullScanAccount, + params: { accountId: 'account-1', trackMissed: true }, + }); + expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ + duration: 'PT5S', + method: CronMethod.FullScanAccount, + params: { accountId: 'account-2', trackMissed: true }, + }); + expect(mockSnapClient.setState).toHaveBeenCalledWith('rescanV1', true); + + // Scheduling happens before the state is marked done. + const scheduleOrder = + mockSnapClient.scheduleBackgroundEvent.mock.invocationCallOrder[0]; + const setStateOrder = + mockSnapClient.setState.mock.invocationCallOrder[0]; + expect(scheduleOrder).toBeLessThan(setStateOrder as number); + + // The normal sync flow still runs afterwards. + expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); + }); + + it('does not schedule or update state when the repair already ran', async () => { + mockSnapClient.getState.mockResolvedValue(true); + + await handler.route(request); + + expect(mockSnapClient.getState).toHaveBeenCalledWith('rescanV1'); + expect(mockSnapClient.scheduleBackgroundEvent).not.toHaveBeenCalled(); + expect(mockSnapClient.setState).not.toHaveBeenCalled(); + expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); + }); + }); }); describe('refreshRates', () => { @@ -349,5 +409,70 @@ describe('CronHandler', () => { await expect(handler.route(request)).rejects.toThrow(error); }); + + describe('trackMissed', () => { + const buildTx = (txid: string): WalletTx => + mock({ + txid: mock({ toString: () => txid }), + }); + + const trackMissedRequest = { + method: CronMethod.FullScanAccount, + params: { accountId: 'account-1', trackMissed: true }, + } as unknown as JsonRpcRequest; + + it('passes trackMissed through from the route params', async () => { + mockAccountUseCases.get.mockResolvedValue(mockAccount); + mockAccountUseCases.fullScan.mockResolvedValue({ + account: mockAccount, + transactionsToNotify: [], + }); + mockAccount.listTransactions.mockReturnValue([]); + + await handler.route(trackMissedRequest); + + expect(mockAccountUseCases.get).toHaveBeenCalledWith('account-1'); + // Called once for the before-scan set and once for the after-scan set, + // proving trackMissed was honored. + expect(mockAccount.listTransactions).toHaveBeenCalledTimes(2); + }); + + it('emits a tracking event only for transactions discovered by the scan', async () => { + const txBefore = buildTx('txid-existing'); + const txNew = buildTx('txid-new'); + + mockAccountUseCases.get.mockResolvedValue(mockAccount); + mockAccountUseCases.fullScan.mockResolvedValue({ + account: mockAccount, + transactionsToNotify: [], + }); + mockAccount.listTransactions + .mockReturnValueOnce([txBefore]) + .mockReturnValueOnce([txBefore, txNew]); + + await handler.route(trackMissedRequest); + + expect(mockSnapClient.emitTrackingEvent).toHaveBeenCalledTimes(1); + expect(mockSnapClient.emitTrackingEvent).toHaveBeenCalledWith( + 'Scan Discovered Missed Transactions', + mockAccount, + txNew, + 'cron', + ); + }); + + it('never emits the tracking event when trackMissed is false or undefined', async () => { + mockAccountUseCases.get.mockResolvedValue(mockAccount); + mockAccountUseCases.fullScan.mockResolvedValue({ + account: mockAccount, + transactionsToNotify: [], + }); + mockAccount.listTransactions.mockReturnValue([buildTx('txid-new')]); + + await handler.route(request); + + expect(mockSnapClient.emitTrackingEvent).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index bf772ec76..4891faac4 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -1,9 +1,10 @@ import { getSelectedAccounts } from '@metamask/keyring-snap-sdk'; -import type { JsonRpcRequest, SnapsProvider } from '@metamask/snaps-sdk'; -import { array, assert, object, string } from 'superstruct'; +import type { Json, JsonRpcRequest, SnapsProvider } from '@metamask/snaps-sdk'; +import { array, assert, boolean, object, optional, string } from 'superstruct'; import { InexistentMethodError, SynchronizationError } from '../entities'; -import type { SnapClient, SyncResult } from '../entities'; +import type { BitcoinAccount, SnapClient, SyncResult } from '../entities'; +import { TrackingSnapEvent } from '../entities'; import type { SendFlowUseCases, AccountUseCases } from '../use-cases'; export const CronMethod = { @@ -25,6 +26,7 @@ export const SyncSelectedAccountsRequest = object({ export const FullScanAccountRequest = object({ accountId: string(), + trackMissed: optional(boolean()), }); export class CronHandler { @@ -70,7 +72,7 @@ export class CronHandler { } case CronMethod.FullScanAccount: { assert(params, FullScanAccountRequest); - return this.fullScanAccount(params.accountId); + return this.fullScanAccount(params.accountId, params.trackMissed); } default: throw new InexistentMethodError(`Method not found: ${method}`); @@ -78,6 +80,18 @@ export class CronHandler { } async synchronizeAccounts(): Promise { + if ((await this.#snapClient.getState('rescanV1')) !== true) { + const allAccounts = await this.#accountsUseCases.list(); + for (const account of allAccounts) { + await this.#snapClient.scheduleBackgroundEvent({ + duration: 'PT5S', + method: CronMethod.FullScanAccount, + params: { accountId: account.id, trackMissed: true }, + }); + } + await this.#snapClient.setState('rescanV1', true); + } + const selectedAccounts: Set = new Set( await getSelectedAccounts(this.#snap), ); @@ -92,11 +106,27 @@ export class CronHandler { ), ); - const successfulResults: SyncResult[] = []; + await this.#finishSync( + accounts, + results, + 'Account synchronization failures', + ); + } - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const errors: Record = {}; + /** + * Aggregate settled sync results, emit events for successes, and throw for failures. + * + * @param accounts - The accounts that were synchronized, in the same order as `results`. + * @param results - The settled synchronization results. + * @param message - The error message to use if any synchronization failed. + */ + async #finishSync( + accounts: BitcoinAccount[], + results: PromiseSettledResult[], + message: string, + ): Promise { + const successfulResults: SyncResult[] = []; + const errors: Record = {}; results.forEach((result, index) => { if (result.status === 'fulfilled') { @@ -104,7 +134,7 @@ export class CronHandler { } else { const id = accounts[index]?.id; if (id) { - errors[id] = result.reason; + errors[id] = String(result.reason); } } }); @@ -112,10 +142,7 @@ export class CronHandler { await this.#emitSyncEvents(successfulResults); if (Object.keys(errors).length > 0) { - throw new SynchronizationError( - 'Account synchronization failures', - errors, - ); + throw new SynchronizationError(message, errors); } } @@ -183,10 +210,31 @@ export class CronHandler { } } - async fullScanAccount(accountId: string): Promise { + async fullScanAccount( + accountId: string, + trackMissed?: boolean, + ): Promise { const account = await this.#accountsUseCases.get(accountId); + + const before = trackMissed + ? new Set(account.listTransactions().map((tx) => tx.txid.toString())) + : undefined; + const result = await this.#accountsUseCases.fullScan(account); + if (before) { + for (const tx of account.listTransactions()) { + if (!before.has(tx.txid.toString())) { + await this.#snapClient.emitTrackingEvent( + TrackingSnapEvent.ScanDiscoveredMissedTransactions, + account, + tx, + 'cron', + ); + } + } + } + await this.#emitSyncEvents([result]); } }