Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch: Compress transaction history #26236

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jul 30, 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

Open in GitHub Codespaces

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

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.

Copy link

socket-security bot commented Jul 30, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@metamask/gas-fee-controller@17.0.0, npm/@metamask/transaction-controller@32.0.0

View full report↗︎

@Gudahtt Gudahtt force-pushed the compress-transaction-history-v12 branch 3 times, most recently from e8cc763 to c344dc4 Compare July 30, 2024 22:20
Base automatically changed from Version-v12.0.0 to master July 30, 2024 22:58
@Gudahtt Gudahtt force-pushed the compress-transaction-history-v12 branch from c344dc4 to d4ec2f9 Compare July 31, 2024 16:49
@Gudahtt Gudahtt changed the base branch from master to Version-v12.0.1 July 31, 2024 16:50
@Gudahtt Gudahtt force-pushed the compress-transaction-history-v12 branch 2 times, most recently from ec306dd to dd4cd6f Compare August 1, 2024 15:02
Gudahtt added 2 commits August 1, 2024 17:24
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)

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.
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 Gudahtt force-pushed the compress-transaction-history-v12 branch from dd4cd6f to 179086a Compare August 1, 2024 19:55
@Gudahtt Gudahtt marked this pull request as ready for review August 1, 2024 20:01
@Gudahtt Gudahtt requested a review from a team as a code owner August 1, 2024 20:01
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.64%. Comparing base (c880e07) to head (7005ca2).
Report is 7 commits behind head on Version-v12.0.1.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           Version-v12.0.1   #26236      +/-   ##
===================================================
+ Coverage            65.61%   65.64%   +0.03%     
===================================================
  Files                 1368     1369       +1     
  Lines                54600    54642      +42     
  Branches             14183    14193      +10     
===================================================
+ Hits                 35825    35867      +42     
  Misses               18775    18775              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [179086a]
Page Load Metrics (52 ± 9 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint67276924723
domContentLoaded96914157
load4111552189
domInteractive96914157

Comment on lines 59 to 60
const validTransactions =
transactionControllerState.transactions.filter(isObject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: under which circumstance transactions is not an object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should always be objects. This is meant to validate that the state is not corrupted somehow. We validate everything that the migration assumes, and in this case we are assuming it's an object so we validate that first.

app/scripts/migrations/120.2.ts Outdated Show resolved Hide resolved
@danjm
Copy link
Contributor

danjm commented Aug 2, 2024

There is both a 120.2 and 120.3 is this PR. The code is almost identical between the two... I assume this is not intentional?

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 2, 2024

There is both a 120.2 and 120.3 is this PR. The code is almost identical between the two... I assume this is not intentional?

Correct, that was a mistake. 120.2 was the old copy, 120.3 was cherry-picked from develop. The old one has been deleted in 497d64f

I'm leaving a gap here for 120.2 because we still need to cherry-pick that in as well

@Gudahtt Gudahtt requested a review from DDDDDanica August 2, 2024 12:19
@Gudahtt Gudahtt added team-extension-platform needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Aug 2, 2024
@Gudahtt Gudahtt requested a review from danjm August 2, 2024 12:19
@metamaskbot
Copy link
Collaborator

Builds ready [7005ca2]
Page Load Metrics (47 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint611057984
domContentLoaded9181021
load40754784
domInteractive9181021

…ransaction-history-v12

* origin/Version-v12.0.1:
  Cherry pick obsolete state migration (#26330)
  cherry pick: undefined market data selector (#26264) (#26329)
  Cherry-Pick: "fix: issue where `setNetworkClientIdForDomain` was called without checking whether the origin was eligible for setting its own network (#26323)" (#26328)
  fix: Fix GitHub release description (#26247) (#26279)
  fix: add guard for draft tx in send validation (#26295)
  fix: handle send transaction API errors (#26253)
  fix: Cherry-pick persistence fix to RC (#26305)
@danjm
Copy link
Contributor

danjm commented Aug 5, 2024

I have successfully manually tested this. If the transaction history reaches length 100, then further additions to the history do not increase the length, but the latest additions are correctly added (whether those are errors or otherwise)

To test, I made 100 edits to gas price, and then confirmed the transaction. I also did the same, but with some code in the transaction controller edited, such that the next resubmission after first submission would fail.

@Gudahtt Gudahtt merged commit 014b56c into Version-v12.0.1 Aug 5, 2024
72 of 73 checks passed
@Gudahtt Gudahtt deleted the compress-transaction-history-v12 branch August 5, 2024 17:56
@github-actions github-actions bot locked and limited conversation to collaborators Aug 5, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 5, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [9661d84]
Page Load Metrics (54 ± 9 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint70173862110
domContentLoaded96613126
load4212354189
domInteractive96613126

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants