Make `Client` shareable between threads

[igamigo]

What should be done?

As a consequence of recent changes in how the store is handled between the client and TransactionExecutor, Client is not Send and so it cannot be shared between threads.

This is seen in the faucet site, where in the last revision a Client is instantiated on each request for now, instead of being acquired through a lock.

We should use Arc instead of Rc. This will likely imply changes in TransactionExecutor as well.

How should it be done?

Change Rc to Arc, both here and in the TransactionExecutor.

When is this task done?

When Client can be shared between threads and users like the faucet can use a single instance across multiple requests.

Additional context

https://github.com/0xPolygonMiden/miden-node/pull/360

[tomyrd]

There's a problem with this change in miden-base. The TransactionAuthenticator is one of the elements stored in a Rc because it's shared between the Client and the TransactionExecutor. We can't store the authenticator in an Arc because it's not Sync+Send this is because it contains a RefCell which is not thread safe. The RefCell is needed because we need a mutable reference to the rng (fn get_falcon_signature( ..., rng: &mut R)) from inside a method that is not mean't to mutate the authenticator (fn get_signature(&self, ...)) This makes the problem of changing Rc to Arc a little more difficult as we haven't found a nice solution without compromises.

Some possible solutions I found:

• Store the rng inside an Arc<Mutex<R>>. This works but the standard Mutex in rust is std and we want the authenticator to be no_std. So we need to use an external crate that implements a no_std mutex.
• Give the responsability of storing the rng to the authenticator user. This wouldn't require a Mutex and the authenticator would become Sync+Send. The problem is that some interfaces would change (fn get_signature(&self, rng: &mut R, ...)) and the authenticator user is burdened with the additional responsability of creating/storing a rng it doesn't use and having to pass it as a parameter.

cc @bobbinth @mFragaBA

[bobbinth]

A couple of preliminary thoughts:

• I think we need to enforce in miden-base that TransactionExecutor is Send+Sync - this way, we'd avoid issues like this in the future.
• For TransactionAuthenticator, I wonder if there is a way to treat it somewhat similar to how we treat DataStore. That is, change the TransactionExecutor to look something like this:

pub struct TransactionExecutor<D, A> {
    data_store: D,
    authenticator: Option<A>,
    compiler: TransactionCompiler,
    exec_options: ExecutionOptions,
}

This would imply two things:

1. First, wen'd also need to change the type of authenticator in TransactionHost to Option<T>.
2. Second, we'll need to make sure that TransactionAuthenticator implies Clone and that the underlying objects are cheap to clone.

The last point is the one that I have the most doubts about. Basically, the assumption that we'd be making is that cloning the authenticator will not result in two authenticators generating the same randomness. But maybe if we document it well enough, it may be OK.

[tomyrd]

I think we need to enforce in miden-base that TransactionExecutor is Send+Sync - this way, we'd avoid issues like this in the future.

In this case we would have the same problem as before. The TransactionExecutor can't be Send+Sync because the BasicAuthenticator is not Send+Sync (it contains a RefCell).

[mFragaBA]

quick summary of the current state of this:

• the current Client is not Send, and the two things blocking it to be so are:
  • The store: since we track an Rc<S> where S is the store type it cannot be shared. One way of solving this would be by wrapping the store in an Arc<Mutex<_>> and make sure that we don't get any deadlocks. Another ways is to use what we saw in the node, where they use deadpool_sqlite and keep a Pool of connections instead of a Connection which can be shared.
  • The store authenticator (the one which implements TransactionAuthenticator on the client side). First, because it is behind an Rc but if we replace it with an Arc we still have some issues because of the RefCell use to store its rng. An alternative we came up to is to asume the StoreAuthenticator is available with std only, we use a Mutex instead of a RefCell (there are some other external no_std mutex crates, but we leave that choice to the user of the library), and we make Send + Sync a requirement for TransactionAuthenticator. Lastly, we'd add some validation tests like the ones done with ProvenTransaction
• If the client is Send then we can wrap it over an Arc<Mutex<_>>
  • while we're making it sendable we shouldn't add this within the client's codebase.
  • I had some concerns regarding whether an Arc<Mutex<Client>> because things such as proving take a while, so having a client shared like that might incurr in stagnation

[mFragaBA]

I made some test branches for miden node, base and client. All are named mFragaBA-make-client-sendable

[bobbinth]

Thinking about this a bit more, I'm wondering if making the Client shareable between threads is a good idea. Specifically, it seems like this would require pushing the need to handle interior mutability in multi-threaded context deep down into the client "stack" (e.g., authenticator, rng etc. would also need to handle this).

Maybe instead we say that Client is explicitly not sharable between threads and if it does need to be shared, users can manually wrap it in Arc<Mutex<_>> and pass that around.

Or, maybe for convenience we can provide something like SharableClient which would look as follows:

pub struct SharableClient(Arc<Mutex<Client>>);

impl SharableClient {
    pub fn lock(&self) -> Result<&Client, SomeError> {
        self.0.lock()
    }
}

Assuming the above works, the main downside that I see is that any interaction with SharableClient would require locking it first, but maybe that's acceptable.

[tomyrd]

Closing this issue. As it was discussed above, making the Client shareable maybe isn't the best idea right now as it requires changing too much of the internal logic to allow for concurrency.

[bobbinth]

I would maybe still keep this open (just at a much lower priority) - unless we think that something like SharableClient is not really useful to expose. What do you guys think?

[tomyrd]

I'm sorry, I forgot to adress this comment. I feel like wrapping the Client in an Arc+Mutex (like the SharableClient) is something the library user can choose and implement themselves, it wouldn't be too much trouble and they may decide which libraries to use (like if they want it to be no_std). Maybe we can reopen the issue just to look for ways to make the Client Send+Sync in the future?