-
-
Notifications
You must be signed in to change notification settings - Fork 249
feat(user-storage-controller): use deferred promises cache for KDF operations #6736
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏 Let's make KDF work as fast as 🚀 And one day we'll merge #6097 too hopefully, I remember that PR eheh
@fabiobozzo lol we'll make sure to get it in the first RC after BIP-44! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, will approve after the comments are resolved
packages/profile-sync-controller/src/shared/encryption/encryption.test.ts
Outdated
Show resolved
Hide resolved
packages/profile-sync-controller/src/shared/encryption/encryption.test.ts
Show resolved
Hide resolved
import { SHARED_SALT } from '../shared/encryption/constants'; | ||
import { Env } from '../shared/env'; | ||
import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema'; | ||
import type { UserStorageGenericFeatureKey } from 'src/shared/storage-schema'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ../shared/storage-schema
? I guess src/...
probably works too
const results = await Promise.all(promises); | ||
expect(results).toHaveLength(numOperations); | ||
|
||
// Verify a sampling of results can be decrypted (testing all 25 would be slow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
// Verify a sampling of results can be decrypted (testing all 25 would be slow) | |
// Verify a sampling of results can be decrypted (testing 25 would be slow) |
## Explanation Release for `@metamask/profile-sync-controller` & `@metamask/multichain-account-service`. ```md ## [25.1.0] ### Changed - Use deferred promises for encryption/decryption KDF operations ([#6736](#6736)) - That will prevent duplicate KDF operations from being computed if one with the same options is already in progress. - For operations that already completed, we use the already existing cache. - Bump `@metamask/utils` from `^11.8.0` to `^11.8.1` ([#6708](#6708)) - Bump `@metamask/keyring-api` from `^20.1.0` to `^21.0.0` ([#6560](#6560)) - Bump `@metamask/keyring-internal-api` from `^8.1.0` to `^9.0.0` ([#6560](#6560)) - Strip `srpSessionData.token.accessToken` from state logs ([#6553](#6553)) - We haven't started using the `includeInStateLogs` metadata yet in clients, so this will have no functional impact. This change brings this metadata into alignment with the hard-coded state log generation performed by clients.today. - Add dependency on `@metamask/utils` ([#6553](#6553)) - Bump `@metamask/base-controller` from `^8.3.0` to `^8.4.0` ([#6632](#6632)) ``` ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## 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 communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Releases core 589.0.0, publishing @metamask/profile-sync-controller 25.1.0 and @metamask/multichain-account-service 1.3.0, and bumps dependent packages and changelogs. > > - **Release**: `@metamask/core-monorepo` to `589.0.0`. > - **Packages**: > - `@metamask/[email protected]` > - Use deferred promises for KDF operations; update deps and strip `srpSessionData.token.accessToken` from state logs. > - `@metamask/[email protected]` > - Add `{Btc, Trx}AccountProvider`; update compare links. > - **Deps Bumped**: > - Update references to `@metamask/profile-sync-controller` to `^25.1.0` in `account-tree-controller`, `notification-services-controller`, `subscription-controller`, and lockfile. > - Update references to `@metamask/multichain-account-service` to `^1.3.0` in `assets-controllers`, `account-tree-controller`, and lockfile. > - **Changelogs**: > - Add Unreleased note for `assets-controllers` reflecting `multichain-account-service` bump. > - Add `1.3.0` section and links in `multichain-account-service`. > - Add `25.1.0` section and links in `profile-sync-controller`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 88351ca. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Antonio Regadas <[email protected]>
Explanation
This improves startup performance on mobile by ~70%; going from an average of ~6s to an average of ~2s for the time it takes to derive the key.
References
Fixes: https://consensyssoftware.atlassian.net/browse/MUL-1100
Checklist
Note
Deduplicates concurrent scrypt KDF operations via a bounded promise cache, adds concurrency/eviction tests, and updates changelog.
src/shared/encryption
):encryption.ts
: Add deferred KDF promise cache to avoid duplicate scrypt derivations; cap size usingMAX_KDF_PROMISE_CACHE_SIZE
; add cache-key generation and#performKdfOperation
helper; minor refactors to usesalt
consistently.constants.ts
: AddMAX_KDF_PROMISE_CACHE_SIZE
.encryption.test.ts
: Add concurrent encrypt/decrypt tests (same/different passwords), load tests, and cache size eviction test.Written by Cursor Bugbot for commit 3b179d1. This will update automatically on new commits. Configure here.