[keyring-controller] Atomic Operations by mikesposito · Pull Request #4192 · MetaMask/core

mikesposito · 2024-04-19T12:21:17Z

Explanation

This PR makes KeyringController operations atomic, with the addition of #persistOrRollback. Each of these operations will be executed mutually exclusively, and before each of them, a snapshot of the current keyrings state is kept and restored in case of errors in the operation execution.

More details on why this is needed here

This PR needs these changes to be merged first:

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/2382

Changelog

`@metamask/keyring-controller`

FIXED: All mutable operations are now atomic: each method will roll back keyring instances in case of errors

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate

packages/keyring-controller/src/KeyringController.ts

mikesposito · 2024-04-19T14:46:58Z

packages/keyring-controller/src/KeyringController.test.ts

                );
                expect(initialSeedWord).toBeDefined();
-                expect(initialState).toBe(controller.state);
+                expect(initialState).toStrictEqual(controller.state);


I wasn't able to understand why this started failing with these changes, but this was suggested by jest

mikesposito · 2024-04-22T08:19:48Z

We should remove return this.#getMemStore() from all function returns, since now it would return a stale version of the state (or alternatively, find a solution for those)

Edit: Refactored here #4199

mikesposito · 2024-04-22T10:09:57Z

packages/keyring-controller/src/KeyringController.ts

+    for (const serializedKeyring of serializedKeyrings) {
+      await this.#restoreKeyring(serializedKeyring);
+    }


Restoring keyrings without enforcing the order (await Promise.all()) always represented a risk, that most likely never turned into an actual bug for this reason:

While async operations are handed off from the main thread until they are resolved (e.g. net calls, encryption), deserialize operations on our default set of keyrings are marked as "async" but none of them is a real async operation, so, likely, they always get executed on the main thread synchronously (and sequentially) - async/await won't give JS multithreading powers.

We use a for loop instead of Promise.all here to also eliminate the risk, in case one of the keyrings used has a truly async operation in deserialize.

Hmm, init is also called during keyring restore, but since the HD and Simple keyrings don't have an asynchronous init (they don't have an init method at all), the above statement should remain valid

mikesposito · 2024-04-22T10:52:55Z

packages/keyring-controller/src/KeyringController.ts

+  /**
+   * Execute the given function after acquiring the controller lock
+   * and save the keyrings to state after it, or rollback to their
+   * previous state in case of error.
+   *
+   * @param fn - The function to execute.
+   * @returns The result of the function.
+   */
+  async #persistOrRollback<T>(fn: MutuallyExclusiveCallback<T>): Promise<T> {
+    return this.#withControllerLock(async ({ releaseLock }) => {
+      const currentSerializedKeyrings = await this.#getSerializedKeyrings();
+
+      try {
+        const callbackResult = await fn({ releaseLock });
+        // State is committed only if the operation is successful
+        await this.#updateVault();
+
+        return callbackResult;
+      } catch (e) {
+        // Keyrings are cleared and restored to their previous state
+        await this.#clearKeyrings({ skipStateUpdate: true });
+        await this.#restoreSerializedKeyrings(currentSerializedKeyrings);
+
+        throw e;
+      }
+    });
+  }
+


Serializing and restoring (in case it's needed) should be the most reliable way to roll back all keyrings to a previous state.

An alternative would have been something like:

const backup = this.#keyrings.slice(); // -- this.#keyrings = backup

But this doesn't work for several reasons:

The shallow copies of keyring instances in backup would be different instances than the original ones (which is ok); but if a keyring instance has a class variable with an array of objects, the shallow copy of that instance would have a class variable array with references to the same objects of the original. This is the case with the HD Keyring, so a rollback would not work there because changing the originals would also change the backups

An eventual QRKeyring event subscription from the controller would be left invalid in case of rollback, since the controller would be subscribed to the original instance instead of the shallow copy

Having two living instances of a keyring could create unwanted scenarios (e.g. two keyring instances that communicate with the same device)

Shallow copies could not be initialized properly

mikesposito · 2024-04-22T11:07:47Z

packages/keyring-controller/src/KeyringController.ts

  async persistAllKeyrings(): Promise<boolean> {
-    return this.#withControllerLock(async () => this.#updateVault());
+    return this.#persistOrRollback(async () => true);
  }


This ensures that all vault update operations triggered by clients can also fail safely

packages/keyring-controller/src/KeyringController.ts

## Explanation This PR is an intermediate refactor needed for #4192. Since operations are eventually rolled back, the internal method will not be able to return the last controller state, risking returning a stale one. Moreover, function returns make more sense now. This PR needs these changes to be merged **first**: - [x] #4182  ## References  ### `@metamask/keyring-controller` - **BREAKING**: Change various `KeyringController` methods so they no longer return the controller state - Changed `addNewAccount` return type to `Promise<string>` - Changed `addNewAccountWithoutUpdate` return type to `Promise<string>` - Changed `createNewVaultAndKeychain` return type to `Promise<void>` - Changed `createNewVaultAndRestore` return type to `Promise<void>` - Changed `importAccountWithStrategy` return type to `Promise<string>` - Changed `removeAccount` return type to `Promise<void>` - Changed `setLocked` return type to `Promise<void>` - Changed `submitEncryptionKey` return type to `Promise<void>` - Changed `submitPassword` return type to `Promise<void>` ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

packages/keyring-controller/src/KeyringController.ts

packages/keyring-controller/src/KeyringController.test.ts

Gudahtt

LGTM!

mikesposito · 2024-04-26T11:39:06Z

packages/keyring-controller/src/KeyringController.ts

+        return await fn({ releaseLock });
+      } catch (e) {
+        // Keyrings and password are restored to their previous state
+        await this.#restoreSerializedKeyrings(currentSerializedKeyrings);


I'm realising now that this probably does not restore the QR keyring event subscription, because that's a specific step of submitPassword and submitEncryptionKey 🤔

We need to move the subscription to the internal restore method, or in #newKeyring

Probably it makes sense for that to go in the internal restore method

Moving it in #newKeyring allowed me to also remove some superfluous code, since the subscription was also created in #addQRKeyring: 36347ad

I see, right because #newKeyring is called inside of the restore method. Makes sense!

Need to preserve QR keyring events

mikesposito · 2024-04-26T12:01:46Z

packages/keyring-controller/src/KeyringController.ts

-    const accounts = await qrKeyring.getAccounts();
-    await this.#checkForDuplicate(KeyringTypes.qr, accounts);
-
-    this.#keyrings.push(qrKeyring as unknown as EthKeyring<Json>);


This is also done in #newKeyring

mikesposito · 2024-04-26T12:02:08Z

packages/keyring-controller/src/KeyringController.ts

-      // getAccounts also validates the accounts for some keyrings
-      await keyring.getAccounts();
-      this.#keyrings.push(keyring);


This is also done in #newKeyring

mikesposito · 2024-04-26T12:02:31Z

packages/keyring-controller/src/KeyringController.ts

-  async #newKeyring(
-    type: string,
-    data: unknown,
-    persist = false,


persist is now always true, so we can remove the last argument

## Explanation Part of `KeyringController` responsibilies is ensuring each operation is [mutually exclusive](#4182) and [atomic](#4192), updating keyring instances and the vault (or rolling them back) in the same mutex lock. However, the ability of clients to have direct access to a keyring instance represents a loophole, as with the current implementation they don’t have to comply with the rules enforced by the controller: we should provide a way for clients to interact with a keyring instance through safeguards provided by KeyringController. The current behavior is this one: 1. Client obtains a keyring instance through `getKeyringForAccount` 2. Client interacts with the instance 3. Client calls `persistAllKeyrings` We should, instead, have something like this: 1. Client calls a `withKeyring` method, passing a _keyring selector_ and a callback 2. KeyringController selects the keyring instance and calls the callback with it, after locking the controller operation mutex 3. Client interacts with the keyring instance safely, inside the callback 4. KeyringController, after the callback execution, internally updates the vault or rolls back changes in case of error, and then releases the mutex lock ## References  * Fixes #4198 * Related to #4192 ## Changelog  ### `@metamask/keyring-controller` - **ADDED**: Added `withKeyring` method - **DEPRECATED**: Deprecated `persistAllKeyrings` method - Use `withKeyring` instead ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch 2 times, most recently from 234fcdf to 13dfb62 Compare April 19, 2024 13:00

mikesposito commented Apr 19, 2024

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Outdated Show resolved Hide resolved

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch from 77261a2 to 5819589 Compare April 19, 2024 14:11

mikesposito commented Apr 19, 2024

View reviewed changes

mikesposito mentioned this pull request Apr 19, 2024

[keyring-controller] Avoid calling mutable public functions internally #4181

Merged

3 tasks

mikesposito force-pushed the feat/keyring-controller-lock-v2 branch from 0d7ee99 to b336151 Compare April 20, 2024 09:50

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch from 0435c12 to 152823a Compare April 20, 2024 10:05

mikesposito mentioned this pull request Apr 20, 2024

[keyring-controller] Add withKeyring method #4197

Merged

3 tasks

mikesposito force-pushed the feat/keyring-controller-lock-v2 branch from 029f2c0 to ab38237 Compare April 22, 2024 08:43

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch from 81d5e29 to 6d13430 Compare April 22, 2024 08:59

mikesposito mentioned this pull request Apr 22, 2024

[keyring-controller] Remove #getMemState returns #4199

Merged

4 tasks

mikesposito changed the base branch from feat/keyring-controller-lock-v2 to refactor/remove-get-mem-state April 22, 2024 09:19

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch 2 times, most recently from 808ec49 to 871c5fe Compare April 22, 2024 09:32

mikesposito changed the title ~~[keyring-controller]: Atomic Operations~~ [keyring-controller] Atomic Operations Apr 22, 2024

mikesposito commented Apr 22, 2024

View reviewed changes

mikesposito mentioned this pull request Apr 23, 2024

[keyring-controller] Lock controller mutex on write operations #4182

Merged

3 tasks

Gudahtt reviewed Apr 23, 2024

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Show resolved Hide resolved

mikesposito force-pushed the refactor/remove-get-mem-state branch 2 times, most recently from dc3165d to d9b5134 Compare April 24, 2024 15:56

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch from 871c5fe to 6bc84be Compare April 24, 2024 16:02

Base automatically changed from refactor/remove-get-mem-state to main April 24, 2024 17:13

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch from 6bc84be to c3e6a34 Compare April 24, 2024 17:14

mikesposito marked this pull request as ready for review April 24, 2024 17:14

mikesposito requested a review from a team April 24, 2024 17:14

Gudahtt reviewed Apr 25, 2024

View reviewed changes

packages/keyring-controller/src/KeyringController.ts Show resolved Hide resolved

packages/keyring-controller/src/KeyringController.ts Show resolved Hide resolved

Gudahtt reviewed Apr 25, 2024

View reviewed changes

packages/keyring-controller/src/KeyringController.test.ts Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Apr 26, 2024

View reviewed changes

mikesposito commented Apr 26, 2024

View reviewed changes

Gudahtt approved these changes Apr 26, 2024

View reviewed changes

mikesposito added 17 commits April 26, 2024 14:22

feat: add #asAtomicOperation

355b792

docs: add jsdoc

5a16c2e

refactor: execute onSuccess after releasing lock

a3739f2

refactor: persistAllKeyrings let the wrapper update vault or rollback

4da133c

refactor: remove #asAtomicOperation options

7eae86b

test: add test case

dac93e7

refactor: rename to #persistOrRollback

011b10a

refactor: rollback keyrings from serialized array

0e79e1a

test: set jest coverage threshold

2b46a94

refactor: roll back password

54b3eeb

refactor: split into #withRollback and #persistOrRollback

5a15dd7

test: adjust coverage threshold

60a274b

refactor: move #clearKeyrings into #restoreSerializedKeyrings

82f124e

test: use exportAccount instead of getAccounts

1410d2c

remove leftover #updateVault call

c4c03de

docs: add MutuallyExclusiveCallback tsdoc

0abb9b0

fix: preserve qr subscription

20fd344

mikesposito force-pushed the feat/keyring-controller-atomic-operations branch from 36347ad to 20fd344 Compare April 26, 2024 12:22

mikesposito merged commit 5536420 into main Apr 26, 2024

mikesposito deleted the feat/keyring-controller-atomic-operations branch April 26, 2024 12:39

This was referenced Apr 29, 2024

[keyring-controller] Mitigate run conditions with >^16.x MetaMask/metamask-extension#24276

Closed

[keyring-controller] Mitigate run conditions with >^16.x MetaMask/metamask-mobile#9432

Closed

Uh oh!

Conversation

mikesposito commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Changelog

@metamask/keyring-controller

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesposito commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikesposito Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesposito Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

mikesposito Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesposito Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mikesposito commented Apr 19, 2024 •

edited

Loading

`@metamask/keyring-controller`

mikesposito commented Apr 22, 2024 •

edited

Loading

mikesposito Apr 22, 2024 •

edited

Loading

mikesposito Apr 22, 2024 •

edited

Loading

mikesposito Apr 26, 2024 •

edited

Loading

mikesposito Apr 26, 2024 •

edited

Loading