[keyring-controller] Lock controller mutex on write operations

[mikesposito]

Explanation

Since all changes to the state of any keyring held by KeyringController should result in an update to the vault, we need to ensure that what we save into the vault is the result of the latest executed operation. In other words, we need to:

1. Ensure that each operation that changes the state is mutually exclusive. That is, if an operation is made of multiple steps, these steps are guaranteed to be executed before any other operation is started
2. Ensure that each operation is persisted to the vault before another operation can start

Visually:

sequenceDiagram
  create participant Client
  create participant aPublicMethod()
  Client->>aPublicMethod(): Calls a method that changes state
  create participant ControllerMutex
  aPublicMethod()->>ControllerMutex: Acquire Controller Lock
  loop Controller Lock
    ControllerMutex-->ControllerMutex: Wait controller to be available
  end
  rect rgb(230,245,255)
    note right of aPublicMethod(): Exclusive Controller OP
    participant KeyringController State
    destroy KeyringController State
    aPublicMethod()->>KeyringController State: Update Controller State
    create participant VaultMutex
    aPublicMethod()->>VaultMutex: Acquire Vault Lock
    loop Vault Lock
      VaultMutex-->VaultMutex: Wait vault to be available
    end
    rect rgb(190,225,235)
      note right of aPublicMethod(): Exclusive Vault OP
      participant Vault
      destroy Vault
      aPublicMethod()->>Vault: Persist state changes
    end
    destroy VaultMutex
    VaultMutex-->>aPublicMethod(): Release Vault lock
  end
    destroy ControllerMutex
    ControllerMutex-->>aPublicMethod(): Release Controller lock
    destroy aPublicMethod()
    aPublicMethod()-->>Client: Return value

To do this, this PR adds the KeyringController.#withControllerLock helper function, to be used on any write operation that can potentially mutate the controller state (e.g. any function that calls this.persistAllKeyrings at the end).

References

• Related to [keyring-controller] Update state in single call when persisting or unlocking #4154

Changelog

@metamask/keyring-controller

• CHANGED: KeyringController method calls that change state are now mutually exclusive

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]

This should ensure that all calls to #updateVault are done after locking on the controller mutex, enforcing mutually exclusive operations in mutable methods

[mcmire]

Makes sense, but it might be worth encoding the knowledge that we have about the bug somewhere, perhaps in the docs for withControllerLock or somewhere else.

[mcmire]

Is it worth explaining why this method exists and what problem it is intended to solve?

[mikesposito]

Definitely! added some details in ab38237

[Gudahtt]

I was thinking that this would cause a deadlock if anyone listened for state change events, then called a write operation in response, e.g.

messenger.subscribe('KeyringController:stateChange',(state) => {
 messenger.call('KeyringController:persistAllKeyrings');
});

But no, this should be OK because all write operations are async. The triggered operation would just wait for the first to complete. Is that correct?

[mikesposito]

I was thinking that this would cause a deadlock if anyone listened for state change events, then called a write operation in response, e.g.

messenger.subscribe('KeyringController:stateChange',(state) => {
 messenger.call('KeyringController:persistAllKeyrings');
});

But no, this should be OK because all write operations are async. The triggered operation would just wait for the first to complete. Is that correct?

@Gudahtt In your example, if messenger.call is called with await then it would cause a deadlock. But I would say that it is a "feature" since that particular call would probably cause an invalid state to persist, so the controller is acting as a safeguard against bad usage.

This is also part of the reason why methods and actions like persistAllKeyrings are being deprecated in upcoming PRs related to withKeyring: if we remove the necessity of doing something like your example by providing a safe alternative, we can easily avoid both deadlocks and corrupted states

[Gudahtt]

Hmm. Event publishing is synchronous, so I do not see how an await could cause a deadlock. The code making the state update would not await async operations, as state updates themselves are synchronous.

[mikesposito]

In this example test case the persistAllKeyrings call is executed before the end of submitPassword, even if the event publishing is synchronous and the method call isn't. I think that with the introduction of locks, that test case would cause a deadlock since it would await on a locked mutex

[legobeat]

I was thinking that this would cause a deadlock if anyone listened for state change events, then called a write operation in response, e.g.

messenger.subscribe('KeyringController:stateChange',(state) => {
 messenger.call('KeyringController:persistAllKeyrings');
});

But no, this should be OK because all write operations are async. The triggered operation would just wait for the first to complete. Is that correct?

@Gudahtt In your example, if messenger.call is called with await then it would cause a deadlock. But I would say that it is a "feature" since that particular call would probably cause an invalid state to persist, so the controller is acting as a safeguard against bad usage.

This is also part of the reason why methods and actions like persistAllKeyrings are being deprecated in upcoming PRs related to withKeyring: if we remove the necessity of doing something like your example by providing a safe alternative, we can easily avoid both deadlocks and corrupted states

In either case, perhaps warranted to add an explicit test-case with that to capture current behavior?

[mikesposito]

Ah I see what you are saying. It would wait for the lock to be released, and just executed afterwards 🤔

[Gudahtt]

With the introduction of this new lock, is the vault mutex still useful? It now seems redundant to me.

Not a blocker - we can remove it in a later PR, but if we can remove it we should, because it would simplify the controller a great deal and eliminate the chances of a new deadlock involving that mutex being introduced in the future.

[Gudahtt]

Hmm, it still protects internally against two vault accesses in the same controller operation. I would say that it is a protection against bad controller development, while the controller mutex is a protection against bad controller usage

Hmm. It would protect against two concurrent write operations to the vault I guess, as sequential changes would be fine.

I get the impression that the lock was originally added to prevent concurrent vault operations caused by concurrent calls to write operations, rather than internal calls. I see how it could be a useful protection measure internally, but it comes at a non-trivial cost in complexity and deadlock risk, and I don't see a reason to suspect concurrent write operations are especially likely to occur. We have no similar protection against concurrent keyring updates, which would be just as problematic.

[mikesposito]

There are a few private methods that are write operations but have no lock, because they are intended to be called from a method that is locked. Thoughts on adding a guard to these to ensure they're always called from within a lock? e.g.

if (!this.#controllerOperationMutex.isLocked()) {
  throw new Error(KeyringControllerError.ControllerLockRequired);
}

(just like the one in #withVaultLock)

The methods I saw that would qualify here are #addQRKeyring, #createNewVaultWithKeyring, #createKeyringWithFirstAccount, #newKeyring, #restoreKeyring, #removeEmptyKeyrings, and #setUnlocked

Good point! I added assertions for all those methods 9780406

[mikesposito]

I get the impression that the lock was originally added to prevent concurrent vault operations caused by concurrent calls to write operations, rather than internal calls. I see how it could be a useful protection measure internally, but it comes at a non-trivial cost in complexity and deadlock risk, and I don't see a reason to suspect concurrent write operations are especially likely to occur. We have no similar protection against concurrent keyring updates, which would be just as problematic.

True, there's a high cost in terms of additional complexity. Perhaps we can remove the vault lock before #4192

[Gudahtt]

Good point! I added assertions for all those methods 9780406

Thanks!

It looks like there is still one method missing this assertion: #createKeyringWithFirstAccount. Thoughts on adding it there as well?

[Gudahtt]

One pending non-blocking suggestion (about adding one last lock assertion), but overall this LGTM!

[Gudahtt]

We could consider preserving async on getAccounts, to avoid this breaking change. But I am OK with this changing, it's very easy to accommodate this change.

[mikesposito]

Good point, it's very easy to accommodate but we'll have to refactor a lot of code already when releasing the complete set of atomic/exclusive operations on clients. Since it's functionally equivalent I'd say that we can come back at this later and remove the async.

I reverted the async removal part in 14ab92e

[mikesposito]

Hmm, I'm thinking that if we intend to release #4192 on clients along with these changes, then we'll have to release #4199 too - which includes a bunch of breaking changes already, so changing getAccounts too should not change much in terms of update complexity since we'll have to release a major version anyway

[Gudahtt]

Agreed! That is why I was ambivalent about this

[mcmire]

It makes sense that some methods have steps that need to be performed atomically and exclusively — they can only run as a whole once the mutex is unlocked — but it's interesting to me that we would go the other way and restrict some methods from being performed outside of a mutex (they have to be a step in an atomic method). Why is that? Or am I not understanding this correctly?

[mikesposito]

The main reason for that is to avoid the addition of methods in the future that could unintentionally call these methods internally without locking: if it's true that all KeyringController mutable operations must be atomic, then should be fair to assert that all mutable methods (including # methods) must be behind the mutex.

[mcmire]

I notice that for removeAccount, the accountRemoved is emitted after the lock is released, but in this case the lock event is emitted before the lock is released. Do these need to behave the same or does it matter?

[mikesposito]

Good point! I moved that out of the lock in removeAccount for two reasons mainly:

1. I initially had a deadlock concern for side effects running while the mutex was still locked, but this has been proven not to be the case
2. In the subsequent PR for atomic operations removeAccount will be wrapped in this.#persistOrRollback, and since the state and vault update will be delegated to the wrapper, this function would emit the :accountRemoved event before removing the account from the state, so a consumer listening would receive the event while having the controller on its original state
3. (Related to (2)) setLocked does not persist to the vault, it just clears the keyrings array, so it will not be wrapped in the this.#persistOrRollback function

[mikesposito]

It looks like there is still one method missing this assertion: #createKeyringWithFirstAccount. Thoughts on adding it there as well?

Good point! I also added it to #clearKeyrings. With these two we should have all methods interacting with this.#keyrings covered now