Reduce log size for long series' of tx retries #9372
Assignees
Labels
area-performance
Issues relating to slowness of app, cpu usage, and/or blank screens.
area-transactions
Sev1-high
High severity; partial loss of service with severe impact upon users, with no workaround.
team-confirmations
Push issues to confirmations team
type-bug
Comments
danfinlay
added
area-performance
danfinlay
changed the title
|
Closing in favour of MetaMask/core#4549 |
|
Actually, I'll leave this open to represent the work of bringing the fix for MetaMask/core#4549 into the extension |
This was referenced Jul 24, 2024
Gudahtt
added
type-bug
Sev1-high
|
Recategorizing this as a sev1 bug because it has been causing severe performance degradations and crashes for users. |
Gudahtt
added a commit
that referenced
this issue
Jul 25, 2024
The `@metamask/transaction-controller` package has been updated from v32 to v34. The `@metamask/user-operation-controller` package had to be bumped as well to avoid new peer dependency warnings. Primarily the only breaking changes in these ranges were peer dependency updates and an update to the minimum supported Node.js version. The new peer dependencies did not introduce any new warnings, they are all met. This update has resolved some pre-existing peer dependency warnings. The only functional change required was this update to the TransactionController in v33.0.0: ``` - **BREAKING:** The `TransactionController` messenger must now allow the `AccountsController:getSelectedAccount` action ([#4244](MetaMask/core#4244)) ... - **BREAKING:** Remove `getSelectedAddress` option from `TransactionController` ([#4244](MetaMask/core#4244)) - The AccountsController is used to get the currently selected address automatically. ``` That change has been applied. See the full changelogs here: * [`@metamask/transaction-controller`](https://github.com/MetaMask/core/blob/%40metamask/transaction-controller%4034.0.0/packages/transaction-controller/CHANGELOG.md) * [`@metamask/user-operation-controller`](https://github.com/MetaMask/core/blob/%40metamask/user-operation-controller%4013.0.0/packages/user-operation-controller/CHANGELOG.md) This helps unblock #9372
7 tasks
Gudahtt
added a commit
that referenced
this issue
Jul 25, 2024
This resolves a peer dependency warning; v10.0.1 of the `@metamask/ens-controller` was requesting an older version of the `@metamask/network-controller` package. The versions are aligned with this update. The only breaking changes were to this peer dependency, and to the minimum supported Node.js version. See here for the full changelog: https://github.com/MetaMask/core/blob/%40metamask/ens-controller%4012.0.0/packages/ens-controller/CHANGELOG.md This helps to unblock #9372
7 tasks
Gudahtt
added a commit
that referenced
this issue
Jul 25, 2024
## **Description** The `@metamask/transaction-controller` package has been updated from v32 to v34. The `@metamask/user-operation-controller` package had to be bumped as well to avoid new peer dependency warnings. Primarily the only breaking changes in these ranges were peer dependency updates and an update to the minimum supported Node.js version. The new peer dependencies did not introduce any new warnings, they are all met. This update has resolved some pre-existing peer dependency warnings. The only functional change required was this update to the TransactionController in v33.0.0: ``` - **BREAKING:** The `TransactionController` messenger must now allow the `AccountsController:getSelectedAccount` action ([#4244](MetaMask/core#4244)) ... - **BREAKING:** Remove `getSelectedAddress` option from `TransactionController` ([#4244](MetaMask/core#4244)) - The AccountsController is used to get the currently selected address automatically. ``` That change has been applied. See the full changelogs here: * [`@metamask/transaction-controller`](https://github.com/MetaMask/core/blob/%40metamask/transaction-controller%4034.0.0/packages/transaction-controller/CHANGELOG.md) * [`@metamask/user-operation-controller`](https://github.com/MetaMask/core/blob/%40metamask/user-operation-controller%4013.0.0/packages/user-operation-controller/CHANGELOG.md) [](https://codespaces.new/MetaMask/metamask-extension/pull/26124?quickstart=1) ## **Related issues** This helps unblock #9372 ## **Manual testing steps** N/A ## **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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Gudahtt
added a commit
that referenced
this issue
Jul 25, 2024
## **Description** This resolves a peer dependency warning; v10.0.1 of the `@metamask/ens-controller` was requesting an older version of the `@metamask/network-controller` package. The versions are aligned with this update. The only breaking changes were to this peer dependency, and to the minimum supported Node.js version. See here for the full changelog: https://github.com/MetaMask/core/blob/%40metamask/ens-controller%4012.0.0/packages/ens-controller/CHANGELOG.md [](https://codespaces.new/MetaMask/metamask-extension/pull/26127?quickstart=1) ## **Related issues** This helps to unblock #9372 ## **Manual testing steps** N/A ## **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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Gudahtt
added a commit
that referenced
this issue
Jul 25, 2024
A number of core packages have been updated to the versions included in [core release v171](https://github.com/MetaMask/core/releases/tag/v171.0.0). The updated packages are: * `@metamask/assets-controllers@35.0.0` * No breaking changes beyond peer dependency updates * `@metamask/ens-controller@3.0.0` * No breaking changes beyond peer dependency updates * `@metamask/gas-fee-controller@19.0.0` * No breaking changes beyond peer dependency updates * `@metamask/network-controller@20.0.0` * The `providerConfig` state has been removed. * The `networksMetadata` state is now always keyed by network client ID, never by an RPC URL. * `@metamask/queued-request-controller@3.0.0` * No breaking changes beyond peer dependency updates * `@metamask/selected-network-controller@16.0.0` * No breaking changes beyond peer dependency updates * `@metamask/transaction-controller@35.0.0` * No breaking changes beyond peer dependency updates * `@metamask/user-operation-controller@14.0.0` * No breaking changes beyond peer dependency updates The only significant breaking change is to the `@metamask/network-controller` package, the removal of `providerConfig`. The existing patch on v19 of `@metamask/network-controller` has been ported to v20. This helps to unblock #9372.
Gudahtt
added a commit
that referenced
this issue
Jul 30, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Jul 30, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Jul 30, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Jul 30, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Jul 30, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Jul 31, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Jul 31, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
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. Relates to #9372
7 tasks
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
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. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
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. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
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. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
## **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. [](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.
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
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. [](https://codespaces.new/MetaMask/metamask-extension/pull/26291?quickstart=1) Relates to #9372 * 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 N/A - [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. - [ ] 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.
Gudahtt
added a commit
that referenced
this issue
Aug 1, 2024
Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. Relates to #9372
Gudahtt
added a commit
that referenced
this issue
Aug 5, 2024
## **Description** Compress transaction history down to 100 entries and truncate any existing transaction historys to just 100 entries. This should fix reports we've seen from production of extremely poor performance and crashes caused by long transaction histories, and will also prevent this problem from happening again. Transaction history has been truncated because in the extreme cases, it would be prohibitively expensive to compress. The downside is that some state of how the transaction has changed may be lost. But this is unlikely to impact users because we only show a limited number of events from the transaction history in our UI, and these events are more likely to be at the beginning of long transaction histories. Even if a displayed event is lost, the impact on the UI is minimal (it's only shown on the transaction details page under "Activity log", and only for informational purposes). For details on how the transaction compression works, and how it prevents history size from growing unbouned, see MetaMask/core#4555 The transaction controller change has been applied using a patch. The patch was generated from the core repository branch `patch/transaction-controller-v32-compress-history`. It will be made on `develop` at a later date because it is blocked by other controller updates at the moment. The truncation is performed by a migration that was added in #26291 and cherry-picked into this branch [](https://codespaces.new/MetaMask/metamask-extension/pull/26236?quickstart=1) ## **Related issues** Relates to #9372 ## **Manual testing steps** To test the migration: * 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, () => chrome.runtime.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 To test that the compression is working, we would want to get a transaction in a "stuck pending" state where an error is captured each iteration. I am not yet sure how to do that unfortunately. ## **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.
dawnseeker8
pushed a commit
that referenced
this issue
Aug 12, 2024
](https://codespaces.new/MetaMask/metamask-extension/pull/26124?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/26127?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/26291?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/26236?quickstart=1){kind=link}
## **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. [](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.
|
Hi @Gudahtt , do you know if this issue can be closed now? |
|
This is still open because the fix is not in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
A user reported bad performance and provided state logs.
Their state logs show one tx in particular as the clear culprit.
The tx had a series of errors, each marked as a warning (probably should have marked as failed on first err):
A few issues here:
rpc error with payloadwarning message should record the returned error, not the params that got the error./warning/errorlog entries in a row, we should concatenate this list, so it has a max size (this one grew to a tremendous size).This also resembles another user who recently reported a performance problem who also had a large tx history.
The text was updated successfully, but these errors were encountered: