Instantiating Miden Client in Backend Causes Route Handlers to Fail

[OgnjenCBS]

Feature description

The problem occurs when instantiation of Miden Client is attempted in a backend server (Axum). The core issue lies in the fact that the SEND and SYNC traits are not implemented for several types in the Miden Client, so any usage of created async functions in Miden SDK for Orderbook will not be able to be awaited on the server side.

1. Handler-Level Instantiation: If I instantiate the client within a handler function, the handler function itself cannot be used as a part of a route due to the lack of SEND and SYNC implementation
2. Application-Level Instantiation: If I instantiate at the application level and make it part of the application state, none of the handlers would work since Miden Client is now a part of the state.

This is an example of the code that will produce an error

pub async fn create_limit_order(
    Path(pair): Path<String>,
    State(controller): State<ExchangeModelController>,
    Json(payload): Json<CreateLimitOrderRequest>,
) {
    let mut client = create_client().await;

    let order_creator = AccountId::from_hex(&payload.order_creator).unwrap();

    // Example of using the client for creating a limit order
    // Uncommenting this block leads to errors
    // match payload.side {
    //     Side::Bid => {
    //         let offered_asset = AccountId::from_hex(&payload.price_acc_id).unwrap();
    //         let requested_asset = AccountId::from_hex(&payload.quantity_acc_id).unwrap();

    //         let lo = limit_order::LimitOrder::create_limit_order(
    //             &mut client,
    //             None,
    //             order_creator,
    //             offered_asset,
    //             payload.price,
    //             requested_asset,
    //             payload.quantity,
    //         )
    //         .await
    //         .unwrap();

    //         lo.submit_limit_order(&mut client).await;
    //     }
    //     Side::Ask => {
    //         let offered_asset = AccountId::from_hex(&payload.quantity_acc_id).unwrap();
    //         let requested_asset = AccountId::from_hex(&payload.price_acc_id).unwrap();

    //         let lo = limit_order::LimitOrder::create_limit_order(
    //             &mut client,
    //             None,
    //             order_creator,
    //             offered_asset,
    //             payload.price,
    //             requested_asset,
    //             payload.quantity,
    //         )
    //         .await
    //         .unwrap();

    //         lo.submit_limit_order(&mut client).await;
    //     }
    // }

    // Update the orderbook
    let mut exchange = controller.get_orderbook_mut(&pair).unwrap();
    let orderbook = exchange.get_mut(&pair).unwrap();

    orderbook.add_limit_order(payload.quantity, payload.side, payload.price);
}

Most of the errors will show in this form

the trait Syncis not implemented for(dyn miden_tx::prover::TransactionProver + 'static), which is required by impl Future<Output = ()>: Send``

the trait Sendis not implemented for(dyn miden_tx::prover::TransactionProver + 'static), which is required by impl Future<Output = ()>: Send``

Environment:

miden-client: version 0.6.0
miden-objects: version 0.6.0

axum: version 0.7.9
tokio: version 1.0

Why is this feature needed?

No response

[phklive]

@Mirko-von-Leipzig @igamigo could you guys give it a look, I know that you have a lot of exp with Rust and multi-threading.

If not you could you re-route to someone that can give a hand, thanks.

[tomyrd]

I've been working on this, there's a version of the Client in the tomyrd-send-sync-client branch (PR #659) that should be Send+Sync (this is the latest version of the client, so there will be some interfaces changes from v0.6) and should fix the mentioned issue.

There might another problem with that version though. Specifically, all the async methods of the client will return Futures that are not Send. This might not seem as a problem, but tokio tasks require that all of the data held is Send. This might or might not be a problem in the original issue, I don't have enough context to determine this.

Even with a full Send + Sync client, we haven't tested enough with the client to know if the behavior would be correct. The current client is not meant to be used concurrently, there are problems that might arise with concurrent use (like race conditions when using the same account at the same time). If the access to the client is controlled this might not be an issue.

A possible workaround in this case is to create new instances of the client that all share the same sqlite database, this means that every client would share the data and be undistinguishable from each other. In this case an external lock could be used to make sure the client is only accessed by one thread at a time.

[bobbinth]

I don't have the full context, but this reminded me of the discussion we had a while back in #358 (comment). Would the SharableClient approach work here?

[phklive]

Written by the Keom team:

Keom<>Miden Asynchronous - Multithreading Problem

Problem exposure

While developing the Keom backend, I encountered a significant issue with the Orderbook SDK that we have created and its integration into the Axum-based API. Specifically, many types within the Miden Client do not implement Send or Sync traits, which creates a roadblock when attempting to await functions in handler routes.

Context

Axum relies on Tokio as the asynchronous runtime. Tokio, by default, uses a multithreaded runtime, requiring that all futures implement the Send trait to ensure they can be safely moved across threads.
Additionally, shared state in Axum often needs to be Sync to allow concurrent access from multiple threads

The Orderbook SDK that we created contains functions and data structures integral to managing orderbook operations. However, many of these types do not implement Send or Sync.

This limitation prevents these types and functions from being used in Axum routes since the handler functions are expected to operate seamlessly in Tokio’s multithreaded environment.

After opening a github issue, core developers proposed different solutions. I have tried implementing those solutions as trying to implement some of the solutions that I thought would work. In the next section, I will explain each and one of them.

Tomyrd-send-sync-client Branch

The issue arises because the new client introduces significant changes, including new methods and removal of old ones. While attempting to refactor the SDK to align with version 0.7, I encountered several challenges due to the lack of examples or documentation for implementing certain features. For instance, the AccountTemplate type no longer exists, which left me unsure about how to create faucets and user accounts under the new implementation.
The updated client branch introduces breaking changes to our existing codebase. While this could potentially be resolved, it would require time and effort, together with back-and-forth communication with the Miden team for clarification and support.

As Tomyrd stated: “This is the latest version of the client, so there will be some interface changes from v0.6”

This turned out to be a bigger issue than expected.

SharableClient

Initially, I assumed that SharableClient was something provided by the Miden Client itself. However, after searching for it, I realized that this is not the case. Upon reading through a related discussion, I discovered that SharableClient needs to be implemented by the developer working on the application.
I attempted to implement this by wrapping the Client in an Arc and a Mutex, which allowed the client to be shared across threads. However, this approach introduced a new issue: The value needs to be locked and unwrapped in a thread where functions from a client are intended to be used. When attempting to use those functions, the process propagated the requirement for Sync and Send implementations from the lowest-level Miden Client types all the way up to my handler function, creating the same cascade of compatibility problems.

Making the whole application single-threaded

The initial idea was to make the entire application run on a single thread, eliminating the need for Send and Sync (Single-threaded Tokio). After introducing the necessary macro change for the main function, the problem still persisted, which was confusing. Why would Send and Sync still be required in a single-threaded environment? This had me raise the question on the Tokio Discord server.

I received two responses. The first suggested using the current-thread flavor of Tokio’s runtime along with LocalSet to remove the need for Send (LocalSet). I attempted to wrap app instantiation in the main function within a LocalSet, but it didn’t give me the results I was hoping for. Honestly, I can’t explain why this approach failed, as it goes beyond my expertise.

The second suggestion was to create custom mpsc channels to handle communication. In this approach, the sender would be given to the handler, and the receiver would be a thread. While this method might promise, implementing would require more time to explore the best approach and understanding a problem to its fundamentals, as this is not a type of problem I frequently encounter

@tomyrd @igamigo @bobbinth

[tomyrd]

I resumed working on this issue today, I hope to bring a solution in the coming days.
We might need to rethink our approach on concurrency in the Client. Mainly because, due to Miden's actor based nature, it doesn't make much sense to try to access an account concurrently.

In the meantime, I think there's a possible workaround that might work with the current Client in a context like Axum. The idea is that you don't share the client, you instantiate a new one each time you need it (creating a new client isn't a compute heavy action). It is important that each new client uses the same Store (same filepath) so they all share the same data.
To avoid concurrent accesses and race conditions on shared data, an empty Arc<Mutex<()>> should be used as a "client lock" (i.e. you only use the new instantiated Client if you have acquired the empty lock).

[bobbinth]

I would actually like to understand the use case for this a bit better - what exactly are we trying to do here? I can think of a couple of scenarios (but not sure if this is an exhaustive list):

1. The front-end accepts requests concurrently and we need to apply them to the same account on the beck-end.
2. The front-end accepts requests concurrently but they are all meant for different accounts.

If it is the first case, all requests should be synchronized before they get to the client. That is the client runs in a single thread and the front end puts it behind a mutex or something like that for the duration of the entire request. If this creates latency issues (because we need to both execute and prove the transactions before we can release the client), we'll need to re-think how transaction submitting process works. For example, people may want to execute transactions, apply them to the local DB w/o generating proofs. The proofs can then be generated concurrently and transactions submitted to the chain as the proofs are generated (this model is much more complicated than what we do now because we need to handle potential state rollbacks if transactions get discarded by the node).

If it is the second case, then one solution is to instantiate a separate client for each account because the current client is not really set up to handle concurrent access to the accounts it manages. Though, we could try to explore how to enable something like this.

@OgnjenCBS, @phklive - which (if any) of the above scenarios are we trying to handle?