Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send+Sync Client test #738

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Send+Sync Client test #738

wants to merge 1 commit into from

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented Feb 17, 2025

From the client's perspective, we took some more time to investigate what was needed to make the Client be Send+Sync by default. I have an experimental branch up, but the changes don't ensure the correct and synchronous access to accounts, it just aims to fix compiling errors in concurrent contexts. As @bobbinth mentioned, additional work is needed to synchronize changes to accounts.

Necessary changes

The necessary changes bubble up to the winterfell codebase.
The changes in these repos fall into one of two categories:

Send+Sync client

For the Client to be send and sync, its elements also must meet this requirements.
In miden-client these elements are:

  • The NodeRpcClient trait and its implementations
  • The Store trait and its implementations
    But also, the client internally uses other objects that also need to be changed:
  • The TransactionAuthenticator trait and its implementations
  • The DataStore trait and its implementations

The necessary changes for this to happen are quite straightforward. We just need to specify :Send+Sync on the traits/implementations and enforce it on the generic types (like internal Rng).

One would think that this is the only thing needed but there's another problem:

Send futures

One of the restrictions of tokio tasks is that all the data held inside must implement Send. With the changes above, the Client could be instantiated inside a tokio task without issues. But it also means that any object the Client creates/generates must also be Send.

Apparently, the futures generated by the client's async functions were not Send. The cause was the way we used the async_trait crate. When you add (?Send) to the attribute, the generated futures will not implement Send. This meant that we had to remove the (?Send) from all the async_trait definitions, these included:

  • NodeRpcClient and Store in miden-client
  • TransactionProver in miden-base.

maybe-async-trait

The definition of the TransactionProver trait doesn't use async_trait but maybe_async_trait which always uses ?Send. To fix this, we added a new async-send feature that removes the (?Send) from the definition when enabled.

tonic-web-wasm-client

The futures generated by the web tonic client code are not Send, this meant that the removal of ?Send generated compilation errors. Both miden-base and miden-client failed to build for wasm, I couldn't find a way to fix this. The workaround was to add the ?Send only when building for wasm, this was done with the following lines:

#[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)]
#[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))]

This means that the wasm client's futures won't be Send but maybe this is not a big drawback and can be tackled in the future if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant