Should the Client be Send+Sync?

[tomyrd]

Stemming from #849 but it seems to be a recurring issue.

Context

Currently the Client is not Send+Sync which means that it is only able to be used in single-threaded controlled accesses. There was a previous experimental attempt (#738) which spanned multiple repositories, but it seemed to work in making the client Send+Sync.

Main points

I feel like there are two main discussions that somewhat clash with each other:

Dev experience

A lot of projects nowadays use asynchronous tokio contexts to deal with concurrency. As discussed in the mentioned PR:

One of the restrictions of tokio tasks is that all the data held inside must implement Send.

Which means that our current implementation, a lot of devs won't be able to use the SDK in their projects. At least not out of the box.

There are workarounds like managing access of the client behind a channel or instantiating a new client for every use (although this won't solve the next issue discussed). These workarounds fall on the client's users which isn't ideal and makes the client more cumbersome to use which is something we are trying to avoid (see #854).

Another problem with these workarounds is that the users may introduce concurrency problems anyway. If two separate instances of the client that use the same store try to execute a transaction against the same account then we also have the same problem. The database manager will deal with the concurrent accesses but we are not sure how this will happen (especially with different implementations of Store).

Concurrency in the client

Conceptually it doesn't make sense for the client to allow concurrent access. The actor model in Miden's protocol means that for concurrent transactions only one will go through and the others will fail, even if they access totally separate information. But maybe for the sake of improving usability and dev experience we want to support it.

If we allow concurrent access to the same account in the client we can just return an error for subsequent transactions on the same account state, it will be up to the user to deal with these errors.
There's another option that will open once we finish with #814. We could let multiple transactions go through and the client will discard the transaction chains that don't go through. The user will have to deal with transactions that went through but later got discarded.

Final thoughts

I think one of the current objectives of the client is to make it as easy to use as possible, and making the necessary changes (even if they span multiple repositories) may be worthwhile as it will be easier for users to create different kinds of projects with the Client.

[sergerad]

Conceptually it doesn't make sense for the client to allow concurrent access.

Please bear with me as I pick up on context. Would this concurrent access problem not be solved by making the relevant associated fns of the Client &mut self? I'm assuming there is a meaningful subset of the Client functionality that would be &self (like the chain tracking logic I mentioned in #849).

Pseudocode for illustration:

impl Client {
    pub fn get_head(&self) -> Result
    pub fn post_transaction(&mut self) -> Result
}

I would expect Send+Sync only to be a problem if we have rc, refcell, etc under the hood.

[tomyrd]

This could work, but maybe we also want some kind of native, more flexible way of dealing with this. For example, we could have per account locks with some logic behind them or other ideas that might come up.

Then there's the issue of just making the Client send+sync (independently of how we deal with concurrency) and fixing the compilation errors that might come up. As shown in #738 this isn't as straightforward. So we want to be sure before continuing down this line.

[sergerad]

Agreed that Send+Sync is a separate issue to the account-level concurrency one. I think the ergonomics of the Client are significantly impacted by the former. I am keen to pick up where you left off on #738 and see where I get to.

[igamigo]

Recently, there's been discussions for making upstream components Send. While introducing generics should solve the issue, some of the missing pieces are related to trait objects (0xMiden/miden-vm#1993) not having the correct bounds. However, adding the bounds does not entirely solve the problem because this implies enforcing some futures to be Send as well. This enforcement bubbles up all the way to the webclient, and more specifically, its implementation of TransactionAuthenticator: Because the idea is to be able to reach out to the store (ie, indexed DB) and this means using a js_sys::Promise which is not Send, it could not be used in a future that is expected to be Send. This also conflicts with some of the requirements of the node, where we want to be able to have a TransactionExecutor that is Send+Sync.

So if we want to make the client Send+Sync and keep the dynamic objects, a similar problem occurs. We could not enforce Client being Send because we want it to also be usable from browsers, and the web store and web authenticator implementations will have this as a limitation. Generics should solve this, but they were already once removed to make for a cleaner interface. One potential alternative to this would be to do something like this and enforce bounds based on the environment.