Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From the client's perspective, we took some more time to investigate what was needed to make the
Client
beSend+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
clientFor the
Client
to be send and sync, its elements also must meet this requirements.In miden-client these elements are:
NodeRpcClient
trait and its implementationsStore
trait and its implementationsBut also, the client internally uses other objects that also need to be changed:
TransactionAuthenticator
trait and its implementationsDataStore
trait and its implementationsThe 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 internalRng
).One would think that this is the only thing needed but there's another problem:
Send
futuresOne of the restrictions of tokio tasks is that all the data held inside must implement
Send
. With the changes above, theClient
could be instantiated inside a tokio task without issues. But it also means that any object theClient
creates/generates must also beSend
.Apparently, the futures generated by the client's async functions were not
Send
. The cause was the way we used theasync_trait
crate. When you add(?Send)
to the attribute, the generated futures will not implementSend
. This meant that we had to remove the(?Send)
from all theasync_trait
definitions, these included:NodeRpcClient
andStore
inmiden-client
TransactionProver
inmiden-base
.maybe-async-trait
The definition of the
TransactionProver
trait doesn't useasync_trait
butmaybe_async_trait
which always uses?Send
. To fix this, we added a newasync-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. Bothmiden-base
andmiden-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: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.