fix: Compress transaction history upon update #4555
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gudahtt
force-pushed
the
compress-transaction-history
branch
3 times, most recently
from
365cd3a
to
a310bc9
Compare
matthewwalsh0
requested changes
Jul 25, 2024
Gudahtt
force-pushed
the
compress-transaction-history
branch
from
a1cd8d9
to
e0b19a1
Compare
matthewwalsh0
previously approved these changes
Jul 25, 2024
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
The TransactionController has been updated to compress transaction history if it exceeds the max transaction history size, which for now has been set to 100. Each time a new entry is added to a transaction history already at max size, we merge two entries to make room for the new one. Note that we never compress entries used for display in the transaction activity log, because compressing those entries might hide events that we want to display. If there are no non-displayed entries to compress, history is allowed to exceed the max size. This is a temporary solution to prevent unbounded growth of the transaction history. While technically it is still unbounded at this level because we don't strictly limit displayed history entries, we don't know of any cases where displayed entries can be repeated a significant number of times, so this will solve that problem in practice. Fixes #4549
…ove non-null assertions
…oving readability
…nd more consistent with other variables
This will be useful in writing a migration to eliminate huge transaction histories.
Gudahtt
force-pushed
the
compress-transaction-history
branch
from
b0e8a44
to
e3251bc
Compare
|
Rebased to resolve changelog conflict |
matthewwalsh0
approved these changes
Jul 26, 2024
AugmentedMode
pushed a commit
that referenced
this pull request
Jul 30, 2024
## Explanation The TransactionController has been updated to compress transaction history if it exceeds the max transaction history size, which for now has been set to 100. Each time a new entry is added to a transaction history already at max size, we merge two entries to make room for the new one. Note that we never compress entries used for display in the transaction activity log, because compressing those entries might hide events that we want to display. If there are no non-displayed entries to compress, history is allowed to exceed the max size. This is a temporary solution to prevent unbounded growth of the transaction history. While technically it is still unbounded at this level because we don't strictly limit displayed history entries, we don't know of any cases where displayed entries can be repeated a significant number of times, so this will solve that problem in practice. ## References Fixes #4549 ## Changelog For the `@metamask/transaction-controller` package: ```markdown ### Added - Add `DISPLAYED_TRANSACTION_HISTORY_PATHS` constant, representing the transaction history paths that may be used for display ([#4555](#4555)) - This was exported so that it might be used to ensure display logic and internal history logic remains in-sync. - Any paths listed here will have their timestamps preserved. Unlisted paths may be compressed by the controller to minimize history size, losing the timestamp. - Add `MAX_TRANSACTION_HISTORY_LENGTH` constant, representing the expected maximum size of the `history` property for a given transaction ([#4555](#4555)) - Note that this is not strictly enforced, the length may exceed this number of all entries are "displayed" entries, but we expect this to be extremely improbable in practice. ### Fixed - Prevent transaction history from growing endlessly in size ([#4555](#4555)) ``` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
Gudahtt
added a commit
that referenced
this pull request
Jul 30, 2024
The TransactionController has been updated to compress transaction history if it exceeds the max transaction history size, which for now has been set to 100. Each time a new entry is added to a transaction history already at max size, we merge two entries to make room for the new one. Note that we never compress entries used for display in the transaction activity log, because compressing those entries might hide events that we want to display. If there are no non-displayed entries to compress, history is allowed to exceed the max size. This is a temporary solution to prevent unbounded growth of the transaction history. While technically it is still unbounded at this level because we don't strictly limit displayed history entries, we don't know of any cases where displayed entries can be repeated a significant number of times, so this will solve that problem in practice. Fixes #4549 For the `@metamask/transaction-controller` package: ```markdown - Add `DISPLAYED_TRANSACTION_HISTORY_PATHS` constant, representing the transaction history paths that may be used for display ([#4555](#4555)) - This was exported so that it might be used to ensure display logic and internal history logic remains in-sync. - Any paths listed here will have their timestamps preserved. Unlisted paths may be compressed by the controller to minimize history size, losing the timestamp. - Add `MAX_TRANSACTION_HISTORY_LENGTH` constant, representing the expected maximum size of the `history` property for a given transaction ([#4555](#4555)) - Note that this is not strictly enforced, the length may exceed this number of all entries are "displayed" entries, but we expect this to be extremely improbable in practice. - Prevent transaction history from growing endlessly in size ([#4555](#4555)) ``` - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
6 tasks
Gudahtt
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
7 tasks
Gudahtt
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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
to MetaMask/metamask-extension
that referenced
this pull request
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.
](https://codespaces.new/MetaMask/metamask-extension/pull/26236?quickstart=1){kind=link}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
The TransactionController has been updated to compress transaction history if it exceeds the max transaction history size, which for now has been set to 100. Each time a new entry is added to a transaction history already at max size, we merge two entries to make room for the new one.
Note that we never compress entries used for display in the transaction activity log, because compressing those entries might hide events that we want to display. If there are no non-displayed entries to compress, history is allowed to exceed the max size.
This is a temporary solution to prevent unbounded growth of the transaction history. While technically it is still unbounded at this level because we don't strictly limit displayed history entries, we don't know of any cases where displayed entries can be repeated a significant number of times, so this will solve that problem in practice.
References
Fixes #4549
Changelog
For the
@metamask/transaction-controllerpackage:Checklist