perf(multichain-account-service)!: avoid excessive state syncing #6654
+1,432
−1,019
Conversation
This file contains hidden or 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
…to wallets and groups
…tead of getAccountByAddress which iterates through the whole of internal accounts in the AccountsController
hmalik88
commented
Sep 18, 2025
| accountsList, | ||
| ); | ||
| // we cast here because we know that the accounts are BIP-44 compatible | ||
| return internalAccounts as Bip44Account<KeyringAccount>[]; |
Contributor
Author
There was a problem hiding this comment.
Although the getAccounts's return type is (InternalAccount | undefined)[], we're sure to get back all the accounts we want since the accounts list will never be stale.
…accountAdded and accountRemoved handling, it is dead code
hmalik88
commented
Sep 19, 2025
| MultichainAccountWallet<Bip44Account<KeyringAccount>> | ||
| >; | ||
|
|
||
| readonly #accountIdToContext: Map< |
Contributor
Author
There was a problem hiding this comment.
Decided to get rid of this mapping since it was only being used for handling the accountRemoved and accountAdded events, removing this gets rid of a large loop in the init function as well. If there's a particular need for this data at the client level, we can always add this back in.
Contributor
There was a problem hiding this comment.
We'll have to add this back, this is now being used on the extension:
…handle createNewVaultAndKeychain and createNewVaultAndRestore code paths
…s, remove redundant state assignment, use assert to ensure wallet existence after creation
…ble with new changes
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts
Show resolved
Hide resolved
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| for (const account of accounts) { | ||
| this.accounts.add(account); | ||
| } | ||
| } |
There was a problem hiding this comment.
Provider accounts set not cleared on re-initialization
Medium Severity
The init() method in BaseBip44AccountProvider only adds account IDs to the internal accounts Set without ever clearing it. When MultichainAccountService.init() is called multiple times (e.g., during app restart or unlock cycles while keeping the same service instance), providers accumulate stale account IDs from previous initializations. Since providers are created once in the constructor and reused, the accounts set grows unbounded.
This could cause getAccounts() to request accounts that no longer exist in the AccountsController, and the as unknown as Account[] type assertion masks any undefined entries in the returned array.
Additional Locations (1)
ccharly
changed the title
| for (const account of accounts) { | ||
| this.accounts.add(account); | ||
| } | ||
| } |
There was a problem hiding this comment.
Provider accounts set accumulates without clearing on reinit
Medium Severity
The init method in BaseBip44AccountProvider only adds account IDs to the internal accounts Set without clearing it first. When MultichainAccountService.init() is called multiple times (e.g., during wallet lock/unlock cycles), #wallets is cleared but providers are reused with the same instances. This causes account IDs from previous initializations to accumulate in the provider's Set. If an account were ever removed between init calls, the stale ID would remain, potentially causing getAccounts() to include undefined entries when those IDs are fetched from AccountsController.
Additional Locations (1)
| this.#providerToAccounts.set(provider, accounts); | ||
| for (const accountId of accounts) { | ||
| this.#accountToProvider.set(accountId, provider); | ||
| } |
There was a problem hiding this comment.
Redundant map updates in group alignment method
Low Severity
The alignAccounts method updates the #providerToAccounts and #accountToProvider maps twice: first inline during the Promise.allSettled callback (lines 292-295), and then again via this.update(groupState) at line 331. The update method calls #setState which clears entries for each provider via #clearAccountToProviderState and re-sets the same values. This is redundant work that adds complexity without changing behavior.
Additional Locations (1)
ccharly
reviewed
Jan 27, 2026
packages/multichain-account-service/src/MultichainAccountService.ts
Outdated
Show resolved
Hide resolved
packages/multichain-account-service/src/MultichainAccountService.ts
Outdated
Show resolved
Hide resolved
packages/multichain-account-service/src/MultichainAccountService.ts
Outdated
Show resolved
Hide resolved
Contributor
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| for (const account of accounts) { | ||
| this.accounts.add(account); | ||
| } | ||
| } |
There was a problem hiding this comment.
Provider state not cleared on re-initialization
Medium Severity
The init method in BaseBip44AccountProvider only adds account IDs to the internal accounts Set without clearing it first. The service's init method calls this.#wallets.clear() before re-initializing, but providers are constructed once and reused. When init is called multiple times (e.g., on unlock), stale account IDs persist in the provider's Set. Since AccountsController:getAccounts returns (InternalAccount | undefined)[] for missing IDs, and getAccounts() casts this without filtering, the returned array may contain undefined values, causing potential runtime errors downstream.
Additional Locations (1)
| this.#providerToAccounts.set(provider, accounts); | ||
| for (const accountId of accounts) { | ||
| this.#accountToProvider.set(accountId, provider); | ||
| } |
There was a problem hiding this comment.
Redundant state setting in alignAccounts method
Low Severity
In alignAccounts, the #providerToAccounts and #accountToProvider maps are set twice: first inside the Promise.allSettled callback (lines 292-295), then again via this.update(groupState) at line 331. The update method calls #setState which clears and re-sets the same data. The first setting inside the callback is redundant since update will overwrite it anyway.
Additional Locations (1)
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
MultichainAccountServicecreateMultichainAccountWallet) that handles import/restore/new vault.ServiceStateindex in one pass and passes state slices to wallets/groups (cuts repeated controller scans/calls).initpath and removed deadaccountIdToContextmapping.MultichainAccountWalletinitnow consumes a pre-sliced wallet state (entropySource → groups → providerName → ids) instead of querying providers.MultichainAccountGroupinitregisters account IDs per provider and fills reverse maps; callsprovider.addAccounts(ids)to keep providers in sync.getAccountIds()for direct access to underlying IDs.BaseBip44AccountProvideraddAccounts(ids: string[]), enabling providers to track their own account ID lists.getAccounts()paths rely on known IDs (plural lookups) rather than scanning the full controller list.EvmAccountProvidergetAccount(s)) for create/discover (removesPerformance Analysis
When fully aligned$g = n / p$ .$g = max(f(p))$ , where $f(p)$ is the number of accounts associated with a provider.
When accounts are not fully aligned then
Consider two scenarios:
General formulas
For Scenario 2, the formulas are as follows:
Before this refactor, the number of loops can be represented$n * p * (1 + w + g)$ , which with $p = 4$ , becomes $n^2 + 4n(1 + w)$ .
Before this refactor, the number of controller calls can be represented as$1 + w + g$ , which with $p = 4$ , becomes $1 + w + n/4$ .
After this refactor, the number of loops can be represented by$n * p$ , which with $p = 4$ , becomes $4n$ .
After this refactor, the number of calls is just$1$ .
For Scenario 1, the formulas are entirely dependent on the breakdown of the number of accounts each provider has amongst the$n$ accounts, let's consider a scenario where Solana has $n/2$ , Ethereum has $n/8$ , Bitcoin has $n/4$ and Tron has $n/8$ , the formulas would be as follows:
Before this refactor, the number of loops in the alignment process can be represented as$(p * g) + (n * e)$ , which with $p=4$ and $g = n/2$ , becomes $2n + 3n^2/8$ . Therefore the number of loops for initialization + alignment in this scenario with $p = 4$ and $g = n/2$ , becomes $(19/8)n^2 + (4w + 6)n$ .
Before this refactor, the number of controller calls in the alignment process can be represented as$e$ , which becomes $3n/8$ . Therefore the number of controller calls for initialization + alignment in this scenario with $p = 4$ , becomes $1 + w + 5n/8$ .
After this refactor, the number of loops in the alignment process can be represented as$p * g$ , which becomes $2n$ . Therefore, the number of loops for initialization + alignment in this scenario with $p = 4$ and $g = n/2$ , becomes $6n$ .
After this refactor, the number of controller calls in the alignment process can be represented as$e$ which becomes $3n/8$ . Therefore, the number of controller calls for initialization + alignment in this scenario with $p = 4$ and $g = n/2$ , becomes $1 + 3n/8$ .
In short, previous
initperformance for loops and controller calls was quadratic and linear, respectively. After, it is linear and constant.Performance Charts
Below are charts that show performance (loops and controller calls)$n = 0$ -> $n = 256$ for Scenario 1 and 2 with $w = 2$ , respectively:
References
MultichainAccountServicechanges metamask-extension#38265multichain-account-servicemetamask-mobile#25040Checklist
Note
Major performance/architecture refactor across multichain account service.
ServiceStatebuilt once, passed toMultichainAccountWallet.initandMultichainAccountGroup.init/update; removes per-provider scanning and legacy reverse mapscreateMultichainAccountWalletsupportingimport/create/restore; addsremoveMultichainAccountWalletactionalignAccounts; improved partial‑failure aggregation/warnings; provider enable/disable respected viaAccountProviderWrapperBaseBip44AccountProvidernow tracks account IDs internally (init, ID set),getAccounts/getAccountuse controller ID lookups instead of global scansEvmAccountProvideruses deterministic IDs (getUUIDFromAddressOfNormalAccount) andgetAccount(s); similar ID tracking added to BTC/TRX/SOL providersWritten by Cursor Bugbot for commit 91e7f90. This will update automatically on new commits. Configure here.