perf(tron-wallet-snap): account-lifecycle fixes from createAccounts/import profiling - #149
Open
hmalik88 wants to merge 8 commits into
Open
perf(tron-wallet-snap): account-lifecycle fixes from createAccounts/import profiling#149hmalik88 wants to merge 8 commits into
hmalik88 wants to merge 8 commits into
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
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.
Account-lifecycle performance fixes from profiling account creation and bulk import
Context: how we found these
While investigating batch account creation performance (MUL-2015), we instrumented the snap two ways: a baseline branch with per-step timings inside
createAccounts(state read, entropy fetch, derivation, merge, re-read), and a branch wrapping every snap handler invocation with duration logs. We then measured warm single account creation and a 151-account SRP import on a local Flask build with the snapfile:-linked into the extension.Three findings from that campaign drive this PR:
createAccountsmade five sequential extension RPC round trips, two of which were avoidable: the post-merge state re-read existed only becausemergeKeyringAccountsreturnedvoid, and the existing-accounts read and entropy fetch ran sequentially despite being independent.Bip44Discoverpath fetched entropy twice per call; once at the full derivation path just to compute an address for the on-chain activity check, then again at the coin-type path to create the account.onSynchronizeSelectedAccountsrunning 3× concurrently at onboarding and 7× concurrently during the import, which is identical work, since the handler takes no parameters.The same instrumentation also showed where the import time doesn't go: MetaMask imports all accounts as a single
Bip44DeriveIndexRangecall, so per-callcreateAccountsoverhead barely registers on import (snap-side total ~3.5s either way). The import window is instead dominated by extension-side RPC queueing and post-import background work (tracked separately).Changes
1. Fewer extension RPC round trips in
keyring_createAccounts(5 → at most 4)AccountsRepository.mergeKeyringAccountsnow returns the merge result ({ merged, added }) instead ofvoid, socreateAccountsresolves persisted accounts; including conflict winners when a concurrent writer got there first, directly from the merge instead of re-reading state afterwards.snap_getBip32Entropyfetch now run in parallel. Timings confirm the state read is fully hidden behind the entropy fetch.readAndEntropyMs,deriveMs,mergeMs,totalMs) so future regressions are visible. Values are stringified because live objects become unexpandable in DevTools once the snap execution environment is torn down.snap_getBip32Entropyis now called even when all requested indices already exist (+~5ms on that path). It only occurs on idempotent retries, and no new permissions are required.2. Single entropy fetch during BIP-44 discovery (2 → 1)
Discovery now creates the coin-type deriver once, derives the activity-check address locally from the cached change node, and reuses the same deriver for account creation. This is address-equivalent: both the old full-path fetch and the deriver produce
m/44'/195'/0'/0/i. New tests pin the single-fetch behavior and assert the probed address is the one persisted.3. Coalesce concurrent account synchronization runs
Root cause of the 3×/7× duplicate syncs: every
setSelectedAccountscall schedules aPT1Sbackground event with no dedupe against already-pending ones, stacking on thePT60Scronjob and post-transaction refreshes. During onboarding/import the extension callssetSelectedAccountsrepeatedly, so several identical sync runs fire within the same second. Each duplicate run fans out accounts × active networks of TronGrid requests, then duplicates state writes andAccountAssetListUpdated/AccountBalancesUpdated/AccountTransactionsUpdatedkeyring events back to the extension, when the extension RPC queue is most contended.Fix:
AccountsService.synchronizeis wrapped in a keyed in-flight guard: concurrent calls for the same account set share one run; the next call after settlement starts a fresh one. All sync entry points (cronjob,SynchronizeSelectedAccounts,SynchronizeAccounts,SynchronizeAccount) funnel through this method. Tron's sync already swallows partial failures viaPromise.allSettled, so sharing a run does not change error semantics.The guard is a new
InFlightCoalescerclass in@metamask/snap-networks-utils, exported from a new./dedupeentry point (same subpath-export pattern as./logger), will be used to fix a similar issue in the Bitcoin snap in a follow-up PR.4. Remove the unreachable v1 account-creation path
AccountsService.createwas not exposed by any handler, the keyring handler only routeskeyring_createAccounts(the extension uses the batch method even for single manual adds), so it was dead code. It was also a latent bug: it emitted the v1AccountCreatedlifecycle event, which keyring-v2 clients reject (this snap declaresendowment:keyring.capabilities, making it v2), and its rollback-on-emission-failure would then delete the just-persisted account. Removed along with its private dependency chain, which nothing else used:deriveAccount, the lowest-unused-index helpers (utils/getLowestUnusedIndex.ts), and theCreateAccountOptionstype (services/accounts/types.ts). Note that the three data-update events (AccountAssetListUpdated/AccountBalancesUpdated/AccountTransactionsUpdated) are still supported for v2 snaps and remain untouched.Measurements
Local Flask build, snap
file:-linked into the extension:Warm phase profile for reference: state read ~4 ms, entropy fetch 7–13 ms, derivation ~0.8 ms/account, merge 3–9 ms quiet (25–33 ms under extension-side contention, where all phases inflate together 3–5×).
Where the bulk-import time actually goes
Phase timings for the single bulk
createAccountscall ({from: 1, to: 151}, 151 accounts created) during a 152-account SRP import, same build, with MetaMetrics on vs off:Snap compute is ~100 ms either way; >95% of the call is extension RPC queue wait, which is why the round-trip reduction in this PR is the right shape of fix even though it cannot rescue import latency by itself. The on/off comparison isolates the MetaMetrics proof-of-ownership fan-out (~150 concurrent per-account
signProofOfOwnershiprequests, fired by the profile-metrics controller as the imported accounts get registered client-side) at ~4.1 s of queue pressure on this one call; most visibly on the persist, which takes 15 ms on a quiet queue. The remaining ~3.1 s on the first round trip is cross-snap import contention (Solana/Bitcoin snaps importing the same range concurrently over the same transport). Both are tracked as follow-ups.The extension also re-issues the same bulk range once (~3–10 s later); the idempotent path returns
created: 0in ~25 ms.Testing
InFlightCoalescerunit tests at 100% coverage (sharing, settlement reset, key independence, rejection propagation).create/deriveAccountpaths were deleted with them; full suites pass for both packages.References
N/A
Checklist