refactor: Add async to proving-service

[sergerad]

Closes #978.

WIP

Might not work or might require changes to upstream crate that has maybe_async macro.

[sergerad]

@Mirko-von-Leipzig the root cause seems to be that this future is not Send. I have confirmed that the Prover (&self here) is Send + Sync. So IIUC the issue is that the future coming out of the maybe_async trait from base is not Send:

the trait std::marker::Send is not implemented for dyn Future<Output = Result<ProvenTransaction, TransactionProverError>>

[sergerad]

I tried modifying base locally to fix node but the issue is that the maybe_async macro always adds this: #[async_trait::async_trait(?Send)].

So there seems to be no way of having Send futures with maybe_async.

I would love to get rid of maybe_async altogether or at least refactor our usage of it (additive feature) so that we can integrate with base in a way that completely avoids it if we need.

Does anyone know the root cause of having maybe_async and how realistic it could be to remove it?

cc @igamigo @tomyrd

[sergerad]

Actually the above is contrary to the docs which say you can do maybe_async(Send). But I can't get past this despite using that variant:

method prove has an incompatible type for trait
expected signature fn(&'life0 proving_service::tx_prover::RemoteTransactionProver, miden_objects::transaction::TransactionWitness)
-> core::pin::Pin<alloc::boxed::Box<(dyn core::future::Future<Output = core::result::Result<miden_objects::transaction::ProvenTransaction, miden_tx::TransactionProverError>> + 'async_trait)>>

found signature fn(&'life0 proving_service::tx_prover::RemoteTransactionProver, miden_objects::transaction::TransactionWitness)
-> core::pin::Pin<alloc::boxed::Box<(dyn core::future::Future<Output = core::result::Result<miden_objects::transaction::ProvenTransaction, miden_tx::TransactionProverError>> + core::marker::Send + 'async_trait)>> (rustc E0053)

Which is saying the non Send is expected, but Send is found.

[sergerad]

Ah its because we are using https://crates.io/crates/winter-maybe-async

not https://docs.rs/maybe-async/latest/maybe_async/.

Could we use the latter instead?

[igamigo]

I am not familiar with the origins of the crate but I do know that @tomyrd was able to remove the ?Send marker and make downstream dependencies compatible (or at least the ones that go down to miden-client). Though maybe it's also used somewhere else and there are other side effects.

[sergerad]

AFAICT the winter crate will always add ?Send whereas the other crate will allow you to avoid ?Send.

[Mirko-von-Leipzig]

Omg I tried this same path and couldn't understand wth was going on. Never occurred to me that its using a custom maybe-async..

[sergerad]

I updated the macro impl to allow for Send futures here.

I can confirm that got rid of the intial issue. But there is a plethora of similar issues blocking progress. All caused by structs from miden-prover and miden-tx etc not being Sync due to trait objects without the bound. For example:

pub struct TransactionHost<A> {
    /// Advice provider which is used to provide non-deterministic inputs to the transaction
    /// runtime.
    adv_provider: A,

    /// MAST store which contains the code required to execute account code functions.
    mast_store: Arc<dyn MastForestStore>,

[sergerad]

Made PR for winter anywho facebook/winterfell#385

[igamigo]

Might not work or might require changes to upstream crate that has maybe_async macro.

Yes, I think this will be the case. The main problem is the ?Send marker which makes the future not Send (as tomyrd explains here and elsewhere; we discussed this a couple times before)

EDIT: the comment had not appeared while I was writing this but that's basically it!