diff --git a/packages/bitcoin-wallet-snap/package.json b/packages/bitcoin-wallet-snap/package.json index 366de4da3..07468943c 100644 --- a/packages/bitcoin-wallet-snap/package.json +++ b/packages/bitcoin-wallet-snap/package.json @@ -73,6 +73,7 @@ "ts-jest": "^29.4.14", "typescript": "~5.8.3", "uuid": "^14.0.0", + "webpack": "5.107.2", "wif": "^5.0.0" }, "engines": { diff --git a/packages/bitcoin-wallet-snap/snap.config.ts b/packages/bitcoin-wallet-snap/snap.config.ts index 7828286f4..6237b2112 100644 --- a/packages/bitcoin-wallet-snap/snap.config.ts +++ b/packages/bitcoin-wallet-snap/snap.config.ts @@ -1,6 +1,9 @@ +/* eslint-disable import-x/no-nodejs-modules -- Snap configuration executes in Node.js. */ import type { SnapConfig } from '@metamask/snaps-cli'; +import { merge } from '@metamask/snaps-cli'; import { config as dotenv } from 'dotenv'; import { resolve } from 'path'; +import { ProvidePlugin } from 'webpack'; dotenv(); @@ -29,6 +32,16 @@ const config: SnapConfig = { }, polyfills: true, experimental: { wasm: true }, + // `createFetchPolicy` (cockatiel) times calls with `performance.now()`, + // which snaps don't have. + customizeWebpackConfig: (webpackConfig) => + merge(webpackConfig, { + plugins: [ + new ProvidePlugin({ + performance: '@metamask/snap-networks-utils/performance-shim', + }), + ], + }), preinstalled: { removable: false, hidden: true, diff --git a/packages/bitcoin-wallet-snap/src/index.ts b/packages/bitcoin-wallet-snap/src/index.ts index df0b97e1b..9e6fb5eeb 100644 --- a/packages/bitcoin-wallet-snap/src/index.ts +++ b/packages/bitcoin-wallet-snap/src/index.ts @@ -40,7 +40,7 @@ const { config } = configProvider; // Infra layer const snapClient = new SnapClientAdapter(logger, config.encrypt); -const chainClient = new EsploraClientAdapter(config.chain); +const chainClient = new EsploraClientAdapter(config.chain, logger); const assetRatesClient = new PriceApiClientAdapter(config.priceApi); const translator = new LocalTranslatorAdapter(); const middleware = new HandlerMiddleware(logger, snapClient, translator); diff --git a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts index 3c31994fa..4e03169ba 100644 --- a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts @@ -5,7 +5,10 @@ import { mock } from 'jest-mock-extended'; import type { BitcoinAccount, ChainConfig } from '../entities'; import { EsploraClientAdapter, + FULL_SCAN_TIMEOUT_MS, SENDER_REQUEST_TIMEOUT_MS, + SYNC_TIMEOUT_MS, + withOperationTimeout, } from './EsploraClientAdapter'; jest.mock('@metamask/bitcoindevkit', () => ({ @@ -69,6 +72,162 @@ describe('EsploraClientAdapter', () => { }); }); + describe('operation timeouts (v3: a stalled request must not keep the sync guard stuck)', () => { + afterEach(() => { + jest.useRealTimers(); + }); + + it('withOperationTimeout resolves with the result and clears its timer', async () => { + jest.useFakeTimers(); + + expect(await withOperationTimeout(Promise.resolve(42), 1000, 'Op')).toBe( + 42, + ); + expect(jest.getTimerCount()).toBe(0); + }); + + it('withOperationTimeout rejects with a TimeoutError when the operation stalls', async () => { + jest.useFakeTimers(); + const stalled = new Promise(() => undefined); + + const result = withOperationTimeout(stalled, 1000, 'Op'); + const caught = result.catch((error: unknown) => error); + await jest.advanceTimersByTimeAsync(1000); + expect(await caught).toMatchObject({ + name: 'TimeoutError', + message: 'Op timed out after 1000 ms', + }); + expect(jest.getTimerCount()).toBe(0); + }); + + it('fails a sync whose request never settles after SYNC_TIMEOUT_MS, without applying anything', async () => { + jest.useFakeTimers(); + const { adapter, mockEsploraClient, account } = setupTest(); + mockEsploraClient.sync.mockReturnValue(new Promise(() => undefined)); + + const result = adapter.sync(account); + const caught = result.catch((error: unknown) => error); + await jest.advanceTimersByTimeAsync(SYNC_TIMEOUT_MS); + expect(await caught).toMatchObject({ + message: 'Failed to synchronize account', + cause: expect.objectContaining({ name: 'TimeoutError' }), + }); + expect(account.applyUpdate).not.toHaveBeenCalled(); + }); + + it('fails a full scan whose requests never settle after FULL_SCAN_TIMEOUT_MS', async () => { + jest.useFakeTimers(); + const { adapter, mockEsploraClient, account } = setupTest(); + mockEsploraClient.full_scan.mockReturnValue(new Promise(() => undefined)); + + const result = adapter.fullScan(account); + let settled = false; + result.catch(() => undefined).finally(() => (settled = true)); + await jest.advanceTimersByTimeAsync(SYNC_TIMEOUT_MS); + expect(settled).toBe(false); + + const caught = result.catch((error: unknown) => error); + await jest.advanceTimersByTimeAsync( + FULL_SCAN_TIMEOUT_MS - SYNC_TIMEOUT_MS, + ); + expect(await caught).toMatchObject({ + message: 'Failed to perform initial full scan', + cause: expect.objectContaining({ name: 'TimeoutError' }), + }); + expect(account.applyUpdate).not.toHaveBeenCalled(); + }); + }); + + describe('read retries', () => { + const setupRetryTest = (): { + adapter: EsploraClientAdapter; + client: ReturnType>; + account: BitcoinAccount; + } => { + const client = mock(); + jest.mocked(EsploraClient).mockImplementation(() => client); + const config = { + parallelRequests: 5, + maxRetries: 3, + stopGap: { discovery: 5, scan: 20 }, + url: { + bitcoin: 'https://bitcoin.example', + testnet: 'https://testnet.example', + testnet4: 'https://testnet4.example', + signet: 'https://signet.example', + regtest: 'https://regtest.example', + }, + } as unknown as ChainConfig; + const account = mock({ network: 'bitcoin' }); + account.startFullScan.mockReturnValue(mock()); + return { + adapter: new EsploraClientAdapter(config), + client, + account, + }; + }; + + beforeEach(() => { + jest.useFakeTimers({ doNotFake: ['nextTick', 'queueMicrotask'] }); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('retries a scan that fails with a network error once', async () => { + const { adapter, client, account } = setupRetryTest(); + const update = { update: true }; + client.full_scan + .mockRejectedValueOnce(new TypeError('Failed to fetch')) + .mockResolvedValue(update as never); + + const pending = adapter.fullScan(account); + await jest.runAllTimersAsync(); + await pending; + + expect(client.full_scan).toHaveBeenCalledTimes(2); + expect(account.applyUpdate).toHaveBeenCalledWith(update); + }); + + it('fails once the retry also fails with a network error', async () => { + const { adapter, client, account } = setupRetryTest(); + client.full_scan.mockRejectedValue(new TypeError('Failed to fetch')); + + const pending = adapter + .fullScan(account) + .catch((error: unknown) => error); + await jest.runAllTimersAsync(); + expect(await pending).toMatchObject({ + message: 'Failed to perform initial full scan', + }); + + // 1 attempt + 1 retry. + expect(client.full_scan).toHaveBeenCalledTimes(2); + expect(account.applyUpdate).not.toHaveBeenCalled(); + }); + + it('never retries a broadcast', async () => { + const { adapter, client } = setupRetryTest(); + client.broadcast.mockRejectedValue(new TypeError('Failed to fetch')); + const transaction = + mock[1]>(); + transaction.compute_txid.mockReturnValue({ + toString: () => 'txid', + } as never); + + const pending = adapter + .broadcast('bitcoin', transaction) + .catch((error: unknown) => error); + await jest.runAllTimersAsync(); + expect(await pending).toMatchObject({ + message: 'Failed to broadcast transaction', + }); + + expect(client.broadcast).toHaveBeenCalledTimes(1); + }); + }); + describe('getTransactionSenders', () => { const mockFetch = jest.fn(); let abortController: AbortController; @@ -124,28 +283,44 @@ describe('EsploraClientAdapter', () => { ); }); - it('rejects and evicts the cache when the request times out', async () => { - const { adapter } = setupTest(); - mockFetch.mockImplementation( - (_url: unknown, init?: { signal?: AbortSignal }) => - new Promise((_resolve, reject) => { - init?.signal?.addEventListener('abort', () => - reject(new Error('TimeoutError')), - ); - }), - ); - - const pending = adapter.getTransactionSenders('bitcoin', 'txid'); - abortController.abort(); - - await expect(pending).rejects.toThrow('TimeoutError'); - - // The failure is not cached, so the next lookup retries. - mockFetch.mockRejectedValue(new Error('TimeoutError')); - await expect( - adapter.getTransactionSenders('bitcoin', 'txid'), - ).rejects.toThrow('TimeoutError'); - expect(txCallCount()).toBe(2); + it('retries a timed-out request under the fetch policy, then rejects and evicts the cache', async () => { + jest.useFakeTimers({ doNotFake: ['nextTick', 'queueMicrotask'] }); + try { + const { adapter } = setupTest(); + // The signal is already aborted, so every attempt times out. + abortController.abort(); + mockFetch.mockImplementation( + async (_url: unknown, init?: { signal?: AbortSignal }) => { + if (init?.signal?.aborted) { + throw new Error('TimeoutError'); + } + return okResponse({ vin: [] }); + }, + ); + + const first = adapter + .getTransactionSenders('bitcoin', 'txid') + .catch((error: unknown) => error); + await jest.runAllTimersAsync(); + expect(await first).toMatchObject({ + message: 'Failed to fetch transaction', + }); + // 1 attempt + 3 policy retries. + const attempts = txCallCount(); + expect(attempts).toBe(4); + + // The failure is not cached, so the next lookup fetches again. + const second = adapter + .getTransactionSenders('bitcoin', 'txid') + .catch((error: unknown) => error); + await jest.runAllTimersAsync(); + expect(await second).toMatchObject({ + message: 'Failed to fetch transaction', + }); + expect(txCallCount()).toBeGreaterThan(attempts); + } finally { + jest.useRealTimers(); + } }); it('skips inputs without a resolved prevout (e.g. coinbase)', async () => { @@ -201,9 +376,9 @@ describe('EsploraClientAdapter', () => { expect(txCallCount()).toBe(1); }); - it('throws and does not cache the failure for a non-2xx response', async () => { + it('throws without retrying and does not cache the failure for a non-retryable non-2xx response', async () => { const { adapter } = setupTest(); - mockFetch.mockResolvedValue({ ok: false, status: 429 }); + mockFetch.mockResolvedValue({ ok: false, status: 404 }); await expect( adapter.getTransactionSenders('bitcoin', 'txid'), @@ -213,5 +388,27 @@ describe('EsploraClientAdapter', () => { ).rejects.toThrow('Failed to fetch transaction'); expect(txCallCount()).toBe(2); }); + + it('retries a transient non-2xx response and returns the recovered result', async () => { + jest.useFakeTimers({ doNotFake: ['nextTick', 'queueMicrotask'] }); + try { + const { adapter } = setupTest(); + mockFetch + .mockResolvedValueOnce({ ok: false, status: 503 }) + .mockResolvedValueOnce( + okResponse({ + vin: [{ prevout: { scriptpubkey_address: 'bc1qsender1' } }], + }), + ); + + const pending = adapter.getTransactionSenders('bitcoin', 'txid'); + await jest.runAllTimersAsync(); + + expect(await pending).toStrictEqual(['bc1qsender1']); + expect(txCallCount()).toBe(2); + } finally { + jest.useRealTimers(); + } + }); }); }); diff --git a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts index 3ae7b6ad4..d5f96a2a6 100644 --- a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts @@ -4,6 +4,13 @@ import type { Transaction, } from '@metamask/bitcoindevkit'; import { EsploraClient } from '@metamask/bitcoindevkit'; +import { BtcScope } from '@metamask/keyring-api'; +import type { FetchPolicy, Logger } from '@metamask/snap-networks-utils'; +import { + createFetchPolicy, + createFetchPolicyError, + executeWithFetchPolicy, +} from '@metamask/snap-networks-utils'; import { ExternalServiceError } from '../entities'; import type { @@ -51,37 +58,154 @@ function toEsploraRestUrl(url: string): string { */ export const SENDER_REQUEST_TIMEOUT_MS = 10_000; +/** + * Upper bound on a whole sync (all its requests and retries). + * The WASM client sets no timeout on its requests: a request stalled by a + * suspended or broken connection could otherwise never settle, keeping the + * coalesced cron sync (and every later cron run joining it) stuck. + */ +export const SYNC_TIMEOUT_MS = 60_000; + +/** + * Upper bound on a whole full scan, for the same reason as + * {@link SYNC_TIMEOUT_MS}: it also releases the single-flight scan guard. + */ +export const FULL_SCAN_TIMEOUT_MS = 120_000; + +/** + * Rejects with a `TimeoutError` (a transient failure for the fetch policies) + * if the operation does not settle in time. The abandoned operation's result + * is discarded: it is never applied to the wallet. + * + * @param operation - The operation. + * @param timeoutMs - The timeout, in milliseconds. + * @param label - Name of the operation, for the error message. + * @returns The operation result. + */ +export async function withOperationTimeout( + operation: Promise, + timeoutMs: number, + label: string, +): Promise { + let timer: ReturnType | undefined; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + const error = new Error(`${label} timed out after ${timeoutMs} ms`); + error.name = 'TimeoutError'; + reject(error); + }, timeoutMs); + }); + try { + return await Promise.race([operation, timeout]); + } finally { + clearTimeout(timer); + } +} + +/** + * One Esplora endpoint: the WASM client, the REST base URL, and the fetch + * policies guarding them. + */ +type EsploraEndpoint = { + label: 'primary'; + client: EsploraClient; + restUrl: string; + /** Guards `full_scan` / `sync` (the WASM client already retries each request). */ + scanPolicy: FetchPolicy; + /** Guards REST lookups and fee estimates. */ + restPolicy: FetchPolicy; +}; + +const NETWORKS: Network[] = [ + 'bitcoin', + 'testnet', + 'testnet4', + 'signet', + 'regtest', +]; + +const CHAIN_IDS: Record = { + bitcoin: BtcScope.Mainnet, + testnet: BtcScope.Testnet, + testnet4: BtcScope.Testnet4, + signet: BtcScope.Signet, + regtest: BtcScope.Regtest, +}; + +const noopLogger: Pick = { + warn: () => undefined, +}; + export class EsploraClientAdapter implements BlockchainClient { // Should be a Repository but we don't support custom networks so we can save in memory from config values - readonly #clients: Record; + readonly #endpoints: Record; readonly #config: ChainConfig; - readonly #restUrls: Record; + readonly #logger: Pick; // Funding addresses are resolved while mapping transactions, which runs on // every sync/event emission. Cache by network + txid so a receive is only // looked up once instead of on each notification. readonly #sendersCache = new Map>(); - constructor(config: ChainConfig) { - this.#clients = { - bitcoin: new EsploraClient(config.url.bitcoin, config.maxRetries), - testnet: new EsploraClient(config.url.testnet, config.maxRetries), - testnet4: new EsploraClient(config.url.testnet4, config.maxRetries), - signet: new EsploraClient(config.url.signet, config.maxRetries), - regtest: new EsploraClient(config.url.regtest, config.maxRetries), - }; + constructor(config: ChainConfig, logger: Pick = noopLogger) { + this.#config = config; + this.#logger = logger; + + const endpoints = {} as Record; + for (const network of NETWORKS) { + endpoints[network] = this.#createEndpoint( + network, + config.url[network], + 'primary', + ); + } + this.#endpoints = endpoints; + } - this.#restUrls = { - bitcoin: toEsploraRestUrl(config.url.bitcoin), - testnet: toEsploraRestUrl(config.url.testnet), - testnet4: toEsploraRestUrl(config.url.testnet4), - signet: toEsploraRestUrl(config.url.signet), - regtest: toEsploraRestUrl(config.url.regtest), + #createEndpoint( + network: Network, + url: string, + label: EsploraEndpoint['label'], + ): EsploraEndpoint { + const chainIdCaip = CHAIN_IDS[network]; + return { + label, + client: new EsploraClient(url, this.#config.maxRetries), + restUrl: toEsploraRestUrl(url), + scanPolicy: createFetchPolicy({ + service: `esplora_${label}_scan`, + chainIdCaip, + logger: this.#logger, + maxRetries: 1, + }), + restPolicy: createFetchPolicy({ + service: `esplora_${label}_rest`, + chainIdCaip, + logger: this.#logger, + }), }; + } - this.#config = config; + /** + * Runs a read against the endpoint under its fetch policy (bounded retries + * with backoff on transient failures, circuit breaker). + * + * @param network - The network. + * @param policy - Which of the endpoint's policies guards the call. + * @param operation - The read, given the endpoint to use. + * @returns The read result. + */ + async #read( + network: Network, + policy: 'scanPolicy' | 'restPolicy', + operation: (endpoint: EsploraEndpoint) => Promise, + ): Promise { + const endpoint = this.#endpoints[network]; + return executeWithFetchPolicy(endpoint[policy], async () => + operation(endpoint), + ); } async fullScan( @@ -93,11 +217,17 @@ export class EsploraClientAdapter implements BlockchainClient { mode === 'discovery' ? this.#config.stopGap.discovery : this.#config.stopGap.scan; - const request = account.startFullScan(); - const update = await this.#clients[account.network].full_scan( - request, - stopGap, - this.#config.parallelRequests, + // The WASM client consumes the request, so each attempt builds its own. + const update = await withOperationTimeout( + this.#read(account.network, 'scanPolicy', async ({ client }) => + client.full_scan( + account.startFullScan(), + stopGap, + this.#config.parallelRequests, + ), + ), + FULL_SCAN_TIMEOUT_MS, + 'Full scan', ); account.applyUpdate(update); } catch (error) { @@ -111,10 +241,12 @@ export class EsploraClientAdapter implements BlockchainClient { async sync(account: BitcoinAccount): Promise { try { - const request = account.startSync(); - const update = await this.#clients[account.network].sync( - request, - this.#config.parallelRequests, + const update = await withOperationTimeout( + this.#read(account.network, 'scanPolicy', async ({ client }) => + client.sync(account.startSync(), this.#config.parallelRequests), + ), + SYNC_TIMEOUT_MS, + 'Sync', ); account.applyUpdate(update); } catch (error) { @@ -128,7 +260,8 @@ export class EsploraClientAdapter implements BlockchainClient { async broadcast(network: Network, transaction: Transaction): Promise { try { - await this.#clients[network].broadcast(transaction); + // Never retried: a retry could broadcast twice. + await this.#endpoints[network].client.broadcast(transaction); } catch (error) { throw new ExternalServiceError( `Failed to broadcast transaction`, @@ -140,7 +273,9 @@ export class EsploraClientAdapter implements BlockchainClient { async getFeeEstimates(network: Network): Promise { try { - return await this.#clients[network].get_fee_estimates(); + return await this.#read(network, 'restPolicy', async ({ client }) => + client.get_fee_estimates(), + ); } catch (error) { throw new ExternalServiceError( `Failed to fetch fee estimates`, @@ -193,19 +328,32 @@ export class EsploraClientAdapter implements BlockchainClient { network: Network, txid: string, ): Promise { - const response = await fetch(`${this.#restUrls[network]}/tx/${txid}`, { - signal: AbortSignal.timeout(SENDER_REQUEST_TIMEOUT_MS), - }); - - if (!response.ok) { - throw new ExternalServiceError(`Failed to fetch transaction`, { + let transaction: EsploraTx; + try { + transaction = await this.#read( network, - txid, - status: response.status, - }); + 'restPolicy', + async ({ restUrl }) => { + const response = await fetch(`${restUrl}/tx/${txid}`, { + signal: AbortSignal.timeout(SENDER_REQUEST_TIMEOUT_MS), + }); + if (!response.ok) { + throw createFetchPolicyError(`HTTP ${response.status}`, { + httpStatus: response.status, + }); + } + return (await response.json()) as EsploraTx; + }, + ); + } catch (error) { + const status = (error as { httpStatus?: number }).httpStatus; + throw new ExternalServiceError( + `Failed to fetch transaction`, + status === undefined ? { network, txid } : { network, txid, status }, + error, + ); } - const transaction = (await response.json()) as EsploraTx; // Self-transfers and consolidations can repeat the same funding address; // dedupe while preserving input order. const senders = [ diff --git a/yarn.lock b/yarn.lock index b745a1aa1..84c11b7b9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2279,6 +2279,7 @@ __metadata: ts-jest: "npm:^29.4.14" typescript: "npm:~5.8.3" uuid: "npm:^14.0.0" + webpack: "npm:5.107.2" wif: "npm:^5.0.0" languageName: unknown linkType: soft