From 4ee5ff2f90017a4036544e53912cc563009b450a Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 1 Aug 2024 17:22:08 -0230 Subject: [PATCH] fix: Truncate extremely long transaction histories (#26291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Transaction histories over 100 entries long have been truncated to 100 entries. Transaction histories are not expected to be that large in normal circumstances, but we have found cases of users with transactions stuck in error loops that result in extremely long histories, causing significant performance issues and crashes. This is a partial solution to that problem. We still need to prevent history from growing again. This is accomplished in `@metamask/transaction-controller@35.1.0`, but that update will be included in a later PR because there are some other updates blocking it. This migration is added first to make it easier to cherry-pick into v12.0.1. The migration has been set as number 120.3 because we want to cherry- pick it into v12.0.1, and using this number avoids needing to re-order migrations. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26291?quickstart=1) ## **Related issues** Relates to #9372 ## **Manual testing steps** * Checkout v11.15.6 and create a dev build (unfortunately the repro steps don't work on MV3 due to snow being enabled even in dev builds, so we're using a pre-MV3 build) * Install the dev build from the `dist/chrome` directory and proceed through onboarding * Make a single transaction * Any chain, any transaction, as long as it's approved. We don't even need to wait for it to settle. * Run this command in the background console to add a huge history to the transaction controller and restart the extension: ``` chrome.storage.local.get( null, (state) => { state.data.TransactionController.transactions[0].history.push( ...[...Array(100000).keys()].map(() => [ { value: '[ethjs-rpc] rpc error with payload {"id":[number],"jsonrpc":"2.0","params":["0x"],"method":"eth_getTransactionReceipt"} { "code": -32603, "message": "Internal JSON-RPC error.", "data": { "code": -32602, "message": "invalid argument 0: hex string has length 0, want 64 for common.Hash", "cause": null }, "stack": ...', path: '/warning/error', op: 'replace' }, { note: 'transactions/pending-tx-tracker#event: tx:warning', value: 'There was a problem loading this transaction.', path: '/warning/message', op: 'replace' }, ]), ); chrome.storage.local.set(state, () => window.location.reload()); } ); ``` * The extension should become extremely slow * Disable the extension * Switch to this branch and create a dev build * Enable and reload the extension * The extension should no longer be slow ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/migrations/120.3.test.ts | 253 +++++++++++++++++++++++++++ app/scripts/migrations/120.3.ts | 114 ++++++++++++ app/scripts/migrations/index.js | 1 + 3 files changed, 368 insertions(+) create mode 100644 app/scripts/migrations/120.3.test.ts create mode 100644 app/scripts/migrations/120.3.ts diff --git a/app/scripts/migrations/120.3.test.ts b/app/scripts/migrations/120.3.test.ts new file mode 100644 index 000000000000..11bef64a3422 --- /dev/null +++ b/app/scripts/migrations/120.3.test.ts @@ -0,0 +1,253 @@ +import { cloneDeep } from 'lodash'; +import { migrate, version } from './120.3'; + +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + +const oldVersion = 120.2; + +describe('migration #120.3', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('updates the version metadata', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + + const newStorage = await migrate(oldStorage); + + expect(newStorage.meta).toStrictEqual({ version }); + }); + + it('returns state unchanged if TransactionController state is missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('reports error and returns state unchanged if TransactionController state is invalid', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: 'invalid', + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + `Migration ${version}: Invalid TransactionController state of type 'string'`, + ); + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('returns state unchanged if transactions are missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: {}, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('reports error and returns state unchanged if transactions property is invalid', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: 'invalid', + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + `Migration ${version}: Invalid TransactionController transactions state of type 'string'`, + ); + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('reports error and returns state unchanged if there is an invalid transaction', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: [ + {}, // empty object is valid for the purposes of this migration + 'invalid', + {}, + ], + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + `Migration ${version}: Invalid transaction of type 'string'`, + ); + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('reports error and returns state unchanged if there is a transaction with an invalid history property', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: [ + {}, // empty object is valid for the purposes of this migration + { + history: 'invalid', + }, + {}, + ], + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + `Migration ${version}: Invalid transaction history of type 'string'`, + ); + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('returns state unchanged if there are no transactions', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: [], + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('returns state unchanged if there are no transactions with history', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: [{}, {}, {}], + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('returns state unchanged if there are no transactions with history exceeding max size', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: [ + { + history: [...Array(99).keys()], + }, + { + history: [...Array(100).keys()], + }, + { + history: [], + }, + ], + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual(oldStorageDataClone); + }); + + it('trims histories exceeding max size', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + PreferencesController: {}, + TransactionController: { + transactions: [ + { + history: [...Array(99).keys()], + }, + { + history: [...Array(100).keys()], + }, + { + history: [...Array(101).keys()], + }, + { + history: [...Array(1000).keys()], + }, + ], + }, + }, + }; + const oldStorageDataClone = cloneDeep(oldStorage.data); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual({ + ...oldStorageDataClone, + TransactionController: { + transactions: [ + { + history: [...Array(99).keys()], + }, + { + history: [...Array(100).keys()], + }, + { + history: [...Array(100).keys()], + }, + { + history: [...Array(100).keys()], + }, + ], + }, + }); + }); +}); diff --git a/app/scripts/migrations/120.3.ts b/app/scripts/migrations/120.3.ts new file mode 100644 index 000000000000..139e6f5221c2 --- /dev/null +++ b/app/scripts/migrations/120.3.ts @@ -0,0 +1,114 @@ +import { RuntimeObject, hasProperty, isObject } from '@metamask/utils'; +import { cloneDeep } from 'lodash'; +import log from 'loglevel'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 120.3; + +const MAX_TRANSACTION_HISTORY_LENGTH = 100; + +/** + * This migration trims the size of any large transaction histories. This will + * result in some loss of information, but the impact is minor. The lost data + * is only used in the "Activity log" on the transaction details page. + * + * @param originalVersionedData - Versioned MetaMask extension state, exactly + * what we persist to dist. + * @param originalVersionedData.meta - State metadata. + * @param originalVersionedData.meta.version - The current state version. + * @param originalVersionedData.data - The persisted MetaMask state, keyed by + * controller. + * @returns Updated versioned MetaMask extension state. + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record): void { + if (!hasProperty(state, 'TransactionController')) { + log.warn(`Migration ${version}: Missing TransactionController state`); + return; + } else if (!isObject(state.TransactionController)) { + global.sentry?.captureException( + `Migration ${version}: Invalid TransactionController state of type '${typeof state.TransactionController}'`, + ); + return; + } + + const transactionControllerState = state.TransactionController; + + if (!hasProperty(transactionControllerState, 'transactions')) { + log.warn( + `Migration ${version}: Missing TransactionController transactions`, + ); + return; + } else if (!Array.isArray(transactionControllerState.transactions)) { + global.sentry?.captureException( + `Migration ${version}: Invalid TransactionController transactions state of type '${typeof transactionControllerState.transactions}'`, + ); + return; + } + + const { transactions } = transactionControllerState; + const validTransactions = transactions.filter(isObject); + if (transactions.length !== validTransactions.length) { + const invalidTransaction = transactions.find( + (transaction) => !isObject(transaction), + ); + global.sentry?.captureException( + `Migration ${version}: Invalid transaction of type '${typeof invalidTransaction}'`, + ); + return; + } + + const validHistoryTransactions = validTransactions.filter( + hasValidTransactionHistory, + ); + if (validHistoryTransactions.length !== validTransactions.length) { + const invalidTransaction = validTransactions.find( + (transaction) => !hasValidTransactionHistory(transaction), + ); + global.sentry?.captureException( + `Migration ${version}: Invalid transaction history of type '${typeof invalidTransaction?.history}'`, + ); + return; + } + + for (const transaction of validHistoryTransactions) { + if ( + transaction.history && + transaction.history.length > MAX_TRANSACTION_HISTORY_LENGTH + ) { + transaction.history = transaction.history.slice( + 0, + MAX_TRANSACTION_HISTORY_LENGTH, + ); + } + } +} + +/** + * Check whether the given object has a valid `history` property, or no `history` + * property. We just check that it's an array, we don't validate the contents. + * + * @param transaction - The object to validate. + * @returns True if the given object was valid, false otherwise. + */ +function hasValidTransactionHistory( + transaction: RuntimeObject, +): transaction is RuntimeObject & { + history: undefined | unknown[]; +} { + return ( + !hasProperty(transaction, 'history') || Array.isArray(transaction.history) + ); +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 6f28fefb6b89..f7dc7b16026c 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -133,6 +133,7 @@ const migrations = [ require('./120'), require('./120.1'), require('./120.2'), + require('./120.3'), require('./121'), require('./122'), require('./123'),