Skip to content

refactor: Add async to proving-service #990

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.

Sign up for GitHub

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

Closed
sergerad wants to merge 1 commit into from
Closed

refactor: Add async to proving-service #990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,12 @@ BUILD_PROTO=BUILD_PROTO=1

.PHONY: clippy
clippy: ## Runs Clippy with configs
cargo clippy --locked --all-targets --all-features --workspace --exclude miden-proving-service -- -D warnings # miden-tx async feature on.
cargo clippy --locked --all-targets --all-features -p miden-proving-service -- -D warnings # miden-tx async feature off.
cargo clippy --locked --all-targets --all-features --workspace


.PHONY: fix
fix: ## Runs Fix with configs
cargo fix --allow-staged --allow-dirty --all-targets --all-features --workspace --exclude miden-proving-service # miden-tx async feature on.
cargo fix --allow-staged --allow-dirty --all-targets --all-features -p miden-proving-service # miden-tx async feature off.
cargo fix --allow-staged --allow-dirty --all-targets --all-features --workspace


.PHONY: format
Expand Down Expand Up @@ -65,22 +63,19 @@ book: ## Builds the book & serves documentation site

.PHONY: test
test: ## Runs all tests
cargo nextest run --all-features --workspace --exclude miden-proving-service # miden-tx async feature on.
cargo nextest run --all-features -p miden-proving-service # miden-tx async feature off.
cargo nextest run --all-features --workspace

# --- checking ------------------------------------------------------------------------------------

.PHONY: check
check: ## Check all targets and features for errors without code generation
${BUILD_PROTO} cargo check --all-features --all-targets --locked --workspace --exclude miden-proving-service # miden-tx async feature on.
${BUILD_PROTO} cargo check --all-features --all-targets --locked -p miden-proving-service # miden-tx async feature off
${BUILD_PROTO} cargo check --all-features --all-targets --locked --workspace

# --- building ------------------------------------------------------------------------------------

.PHONY: build
build: ## Builds all crates and re-builds ptotobuf bindings for proto crates
${BUILD_PROTO} cargo build --locked --workspace --exclude miden-proving-service # miden-tx async feature on.
${BUILD_PROTO} cargo build --locked -p miden-proving-service # miden-tx async feature off
${BUILD_PROTO} cargo build --locked --workspace
${BUILD_PROTO} cargo build --locked -p miden-proving-service-client --target wasm32-unknown-unknown --no-default-features # no-std compatible build

# --- installing ----------------------------------------------------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions bin/proving-service/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ miden-block-prover = { workspace = true }
miden-lib = { workspace = true }
miden-node-utils = { workspace = true }
miden-objects = { features = ["std"], workspace = true }
miden-tx = { features = ["std"], workspace = true }
miden-tx = { features = ["async", "std"], workspace = true }
miden-tx-batch-prover = { features = ["std"], workspace = true }
opentelemetry = { features = ["metrics", "trace"], version = "0.30" }
opentelemetry-otlp = { features = ["grpc-tonic"], version = "0.30" }
Expand Down Expand Up @@ -62,7 +62,7 @@ uuid = { features = ["v4"], version = "1.16" }
[dev-dependencies]
miden-lib = { features = ["testing"], workspace = true }
miden-objects = { features = ["testing"], workspace = true }
miden-testing = { workspace = true }
miden-testing = { features = ["async"], workspace = true }
miden-tx = { features = ["testing"], workspace = true }
[build-dependencies]
miette = { features = ["fancy"], version = "7.5" }
Expand Down
5 changes: 3 additions & 2 deletions bin/proving-service/src/api/prover.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ impl ProverRpcApi {
fields(id = tracing::field::Empty),
err
)]
pub fn prove_tx(
pub async fn prove_tx(
&self,
transaction_witness: TransactionWitness,
) -> Result<Response<ProvingResponse>, tonic::Status> {
Expand All @@ -73,6 +73,7 @@ impl ProverRpcApi {
.try_lock()
.map_err(|_| Status::resource_exhausted("Server is busy handling another request"))?
.prove(transaction_witness)
.await
Copy link
Collaborator Author

@sergerad sergerad Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>>

Copy link
Collaborator Author

@sergerad sergerad Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@sergerad sergerad Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@sergerad sergerad Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@igamigo igamigo Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@sergerad sergerad Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Collaborator Author

@sergerad sergerad Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,

Copy link
Collaborator Author

@sergerad sergerad Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made PR for winter anywho facebook/winterfell#385

.map_err(internal_error)?;

// Record the transaction_id in the current tracing span
Expand Down Expand Up @@ -161,7 +162,7 @@ impl ProverApi for ProverRpcApi {
match request.get_ref().proof_type() {
ProofType::Transaction => {
let tx_witness = request.into_inner().try_into().map_err(invalid_argument)?;
self.prove_tx(tx_witness)
self.prove_tx(tx_witness).await
},
ProofType::Batch => {
let proposed_batch = request.into_inner().try_into().map_err(invalid_argument)?;
Expand Down
2 changes: 1 addition & 1 deletion bin/proving-service/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ mod test {
.tx_script(tx_script)
.build();

let executed_transaction = tx_context.execute().unwrap();
let executed_transaction = tx_context.execute().await.unwrap();

let transaction_witness = TransactionWitness::from(executed_transaction);

Expand Down
Loading