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

fix: Truncate extremely long transaction histories #26291

Merged
merged 5 commits into from
Aug 1, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented 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.

Open in GitHub Codespaces

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

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.

@metamaskbot
Copy link
Collaborator

Builds ready [549146f]
Page Load Metrics (361 ± 338 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint782341023316
domContentLoaded9116212412
load442262361705338
domInteractive9116212412
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.14 KiB (0.06%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.99%. Comparing base (f05aba0) to head (cc9dbd5).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26291      +/-   ##
===========================================
+ Coverage    69.97%   69.99%   +0.03%     
===========================================
  Files         1409     1410       +1     
  Lines        49916    49958      +42     
  Branches     13788    13798      +10     
===========================================
+ Hits         34926    34968      +42     
  Misses       14990    14990              

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

@Gudahtt Gudahtt force-pushed the truncate-long-transaction-histories branch from 549146f to ced0861 Compare August 1, 2024 16:57
@Gudahtt Gudahtt marked this pull request as ready for review August 1, 2024 16:58
@Gudahtt Gudahtt requested a review from a team as a code owner August 1, 2024 16:58
@Gudahtt Gudahtt requested review from a team August 1, 2024 17:00
app/scripts/migrations/120.3.ts Outdated Show resolved Hide resolved
app/scripts/migrations/120.3.ts Outdated Show resolved Hide resolved
app/scripts/migrations/120.3.ts Outdated Show resolved Hide resolved
app/scripts/migrations/120.3.ts Outdated Show resolved Hide resolved
app/scripts/migrations/120.3.ts Show resolved Hide resolved
app/scripts/migrations/120.3.ts Outdated Show resolved Hide resolved
app/scripts/migrations/120.3.ts Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [ced0861]
Page Load Metrics (568 ± 491 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint652361274320
domContentLoaded10135363015
load4833095681022491
domInteractive10135363015
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.14 KiB (0.06%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt force-pushed the truncate-long-transaction-histories branch from e4906a8 to 47f8991 Compare August 1, 2024 18:00
@metamaskbot
Copy link
Collaborator

Builds ready [47f8991]
Page Load Metrics (323 ± 300 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6881414116479
domContentLoaded9107252211
load402013323624300
domInteractive9107252211
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.14 KiB (0.06%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

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 Gudahtt force-pushed the truncate-long-transaction-histories branch from 47f8991 to cc9dbd5 Compare August 1, 2024 19:34
Copy link

sonarcloud bot commented Aug 1, 2024

@Gudahtt Gudahtt merged commit be24e25 into develop Aug 1, 2024
77 of 78 checks passed
@Gudahtt Gudahtt deleted the truncate-long-transaction-histories branch August 1, 2024 19:52
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 1, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [cc9dbd5]
Page Load Metrics (314 ± 274 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint653511409545
domContentLoaded8114363316
load401724314571274
domInteractive8113363316
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.14 KiB (0.06%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-wallet-framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants