feat: split miden-tx-prover into miden-proving-service and miden-remote-provers #1047

SantiagoPittella · 2025-01-03T17:16:07Z

This approach uses a single protobuf file placed in miden-remote-provers/proto/ and replaced the name of RemoteTransactionProverError with RemoteProverError in order of being used by other types of provers.

It uses the feature tx-prover to activate the RemoteTransactionProver, this is needed because we don't want to trigger the async feature for the whole workspace by default.

The miden-proving-service now only generates the file for the std version of the api, since it doesn't support WASM.

miden-remote-provers/src/generated/std/api.rs

miden-remote-provers/src/lib.rs

miden-remote-provers/Cargo.toml

bin/proving-service/src/generated/std/api.rs

…-service

SantiagoPittella · 2025-01-13T22:15:18Z

I addressed the comments and changed a bit the README s. Also rebased because it had tons of conflicts.

cc @bobbinth

bobbinth

Thank you! Looks good. Not a full review, but I left some comments inline.

Cargo.toml

bin/proving-service/Cargo.toml

bin/proving-service/src/generated/std/mod.rs

bobbinth · 2025-01-14T09:17:21Z

bin/proving-service/src/generated/std/api.rs

I think this file still contains the generated client code. Do we actually need it?

I leave the client code here because we use it in the test placed in main.rs.

bin/proving-service/build.rs

miden-remote-provers/src/generated/std/mod.rs

miden-remote-provers/src/lib.rs

…-repo-approach

…rovers

igamigo

Looks good overall, left some comments about the file structure and make target organization mostly, but other than that I think this is ready to be merged

igamigo · 2025-01-14T19:03:26Z

miden-remote-provers/Cargo.toml

+[package]
+name = "miden-remote-provers"
+version = "0.1.0"
+description = "client library that provides a client for the remote provers"


nit: Is it worth changing the name to miden-remote-prover-clients? Maybe it's too much of a mouthful but it sounds more descriptive at a first glance (miden-remote-provers sounds more ambiguous in comparison). cc @bobbinth since I think he suggested these names

I'm not a big fan of miden-remote-prover-clients, maybe something like miden-prover-clients?

miden-prover-client may be fine, but also maybe miden-proving-service-client is better? (it immediately indicates that this is a "companion" crate for miden-proving-service crate).

miden-remote-provers/README.md

bin/proving-service/build.rs

Makefile

bin/proving-service/build.rs

miden-remote-provers/README.md

Co-authored-by: igamigo <[email protected]>

…-repo-approach

igamigo

LGTM! I left a couple more nits, but I think this is good to merge already if we can test it works well on the client.

miden-remote-provers/src/lib.rs

miden-remote-provers/build.rs

…-repo-approach

SantiagoPittella · 2025-01-15T20:34:53Z

I'm merging it in order to merge other "child" PRs on the client, I can address any further comment in a followup PR.

bobbinth

Looks good! Thank you. I know this PR is merged, but I left some comments inline which I think we should address in a follow-up PR (before we can release these crates).

Also, it seems like we are doing some things in the proving service crate purely for testing reasons (e.g., building the client part of the protobuf service). I wonder if there is a better way to handle these.

bobbinth · 2025-01-16T21:41:58Z

bin/proving-service/Cargo.toml

-async = ["miden-tx/async"]
-default = ["std"]
-std = ["miden-objects/std", "miden-tx/std", "dep:tokio", "dep:tonic-web", "dep:tokio-stream", "dep:axum",  "dep:tracing", "dep:tracing-subscriber", "tonic/transport"]
+default = ["concurrent"]
 testing = ["miden-objects/testing", "miden-lib/testing", "miden-tx/testing"]


Let's check if we still need the testing feature.

bobbinth · 2025-01-16T21:46:57Z

bin/proving-service/README.md

 Note that for the prover worker you might need to enable the `testing` feature in case the transactions were executed with reduced proof-of-work requirements (or otherwise, the proving process will fail). This step will also generate the necessary protobuf-related files. You can achieve that by generating the binary with:

 ```bash
-make install-tx-prover-testing
+make install-proving-service-testing
 ```


This should no longer be needed.

bobbinth · 2025-01-16T21:49:38Z

bin/proving-service/README.md

 | ------------ | ------------------------------------------------------------------------------------------------------------|
 | `std`        | Enable usage of Rust's `std`, use `--no-default-features` for `no-std` support.                             |
 | `concurrent` | Enables concurrent code to speed up runtime execution.                                                      |
-| `async`      | Enables the `RemoteTransactionProver` struct, that implements an async version of `TransactionProver` trait.|
 | `testing`    | Enables testing utilities and reduces proof-of-work requirements to speed up tests' runtimes.               |


I think std feature is no longer relevant. Also, we should be able to remove the testing feature as mentioned in other comments.

bobbinth · 2025-01-16T21:54:04Z

bin/proving-service/Cargo.toml

+tonic-web = { version = "0.12" }
+tracing = { version = "0.1" }
+tracing-opentelemetry = "0.28"
+tracing-subscriber = { version = "0.3", features = ["fmt",  "json",  "env-filter"] }
 uuid = { version = "1.11", features = ["v4"] }
 winter-maybe-async = { version = "0.10" }


I'm not sure we need winter-maybe-async dependency here. What is it used for? Also, let'd make sure we only have dependencies that are required.

bobbinth · 2025-01-16T21:54:59Z

bin/proving-service/Cargo.toml

+pingora = { version = "0.4", features = [ "lb" ] }
+pingora-core = "0.4"
+pingora-proxy = "0.4"
+pingora-limits = "0.4"
 reqwest = { version = "0.11" }


I think the latest version of request is now 0.12 - can we update to it?

bobbinth · 2025-01-16T22:15:08Z

miden-remote-provers/src/prover.rs

+        let mut client = self
+            .client
+            .write()
+            .take()
+            .ok_or_else(|| TransactionProverError::other("client should be connected"))?;


Question: why do we need to do take() here now? (I don't think this was needed before, right?)

I replaced the .take with a .as_ref and then cloning the client.

This change was needed because of a clippy error. We didn't ran the linter earlier because it was only enabled when using the "async" feature and it was disabled in the Makefile. Now that we have clippy for no-std, the linter is throwing an error because the MutexGuard was held through an .await call.

bobbinth · 2025-01-16T22:16:59Z

miden-remote-provers/src/lib.rs

+/// Contains the protobuf definitions
+pub const PROTO_MESSAGES: &str = include_str!("../../proto/api.proto");


I would change the doc comment to something like: "Protobuf definitions for the Miden proving service".

Also nit: maybe we should change the constant name to something like SERVICE_PROTO?

bobbinth · 2025-01-16T22:23:57Z

miden-remote-provers/src/lib.rs

@@ -0,0 +1,35 @@
+extern crate alloc;


Should this not be more like this:

#![no_std] #[cfg(feature = "std")] extern crate std;

bobbinth · 2025-01-16T22:26:31Z

Makefile

@@ -23,7 +23,7 @@ clippy: ## Runs Clippy with configs

 .PHONY: clippy-no-std
 clippy-no-std: ## Runs Clippy with configs
-	cargo clippy --no-default-features --target wasm32-unknown-unknown --workspace --lib -- -D warnings
+	cargo clippy --no-default-features --target wasm32-unknown-unknown --workspace --lib --exclude miden-proving-service --features tx-prover -- -D warnings


nit: could we swap --features and --exclude places? i.e., so that it looks like:

cargo clippy --no-default-features --target wasm32-unknown-unknown --workspace --lib --features tx-prover --exclude miden-proving-service -- -D warnings

And do the same for other commands below.

bobbinth · 2025-01-16T22:28:41Z

Makefile

+.PHONY: install-proving-service-testing
+install-proving-service-testing: ## Install proving service's CLI intended for testing purposes
+	cargo install --path bin/proving-service --bin miden-proving-service --locked --features concurrent,testing


As I mentioned in one of the other comments, I don't think this will be needed any longer.

SantiagoPittella mentioned this pull request Jan 3, 2025

Refactor miden-tx-prover features #975

Closed

bobbinth reviewed Jan 3, 2025

View reviewed changes

miden-remote-provers/src/generated/std/api.rs Outdated Show resolved Hide resolved

bobbinth reviewed Jan 3, 2025

View reviewed changes

miden-remote-provers/src/lib.rs Show resolved Hide resolved

bobbinth reviewed Jan 3, 2025

View reviewed changes

miden-remote-provers/Cargo.toml Outdated Show resolved Hide resolved

bobbinth reviewed Jan 3, 2025

View reviewed changes

bin/proving-service/src/generated/std/api.rs Outdated Show resolved Hide resolved

bobbinth requested a review from igamigo January 13, 2025 17:11

SantiagoPittella force-pushed the santiagopittella-split-tx-prover-crates-same-repo-approach branch from 02fb98a to ac627ee Compare January 13, 2025 22:09

feat: split miden-tx-prover in miden-remote-provers and miden-proving…

62d5605

…-service

SantiagoPittella force-pushed the santiagopittella-split-tx-prover-crates-same-repo-approach branch from ac627ee to 62d5605 Compare January 13, 2025 22:15

SantiagoPittella marked this pull request as ready for review January 13, 2025 22:15

fix: docs

3d4b591

bobbinth reviewed Jan 14, 2025

View reviewed changes

SantiagoPittella mentioned this pull request Jan 14, 2025

Rename objects to miden-objects and use crates/ dir #1062

Closed

SantiagoPittella added 5 commits January 14, 2025 11:57

review: remove std feature from binary, make concurrent default

18e333c

review: add thiserror to remote-provers

8cdfd35

review: remove std dir in binary

2b048fd

Merge branch 'next' into santiagopittella-split-tx-prover-crates-same…

eb9b975

…-repo-approach

fix: exclude proving service from no-std clippy, fix lint in remote p…

46ef873

…rovers

igamigo reviewed Jan 14, 2025

View reviewed changes

SantiagoPittella and others added 6 commits January 15, 2025 11:38

review: fix typo

2a66321

Co-authored-by: igamigo <[email protected]>

review: remove build-remote-provers target from makefile

d7a636a

review: move proto files to workspace dir

a451aca

review: improve clients readme

cbf3af2

Merge branch 'next' into santiagopittella-split-tx-prover-crates-same…

8fac579

…-repo-approach

chore: update cargo lock

ec167ee

SantiagoPittella requested review from bobbinth and igamigo January 15, 2025 16:11

igamigo approved these changes Jan 15, 2025

View reviewed changes

miden-remote-provers/src/lib.rs Show resolved Hide resolved

miden-remote-provers/src/lib.rs Show resolved Hide resolved

miden-remote-provers/build.rs Show resolved Hide resolved

Merge branch 'next' into santiagopittella-split-tx-prover-crates-same…

9a54c60

…-repo-approach

SantiagoPittella merged commit 0e65f4f into next Jan 15, 2025
11 checks passed

SantiagoPittella deleted the santiagopittella-split-tx-prover-crates-same-repo-approach branch January 15, 2025 20:35

SantiagoPittella mentioned this pull request Jan 16, 2025

follow-up: build generated files to publish in crates.io #1072

Closed

bobbinth mentioned this pull request Jan 16, 2025

Move RemoteTransactionProver out of bin #924

Closed

bobbinth reviewed Jan 16, 2025

View reviewed changes

This was referenced Jan 17, 2025

follow-up(miden-proving-service): address missing comments #1078

Merged

Split tx-prover dependencies (binary and library) #1001

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: split miden-tx-prover into miden-proving-service and miden-remote-provers #1047

feat: split miden-tx-prover into miden-proving-service and miden-remote-provers #1047

SantiagoPittella commented Jan 3, 2025

SantiagoPittella commented Jan 13, 2025

bobbinth left a comment

bobbinth Jan 14, 2025

SantiagoPittella Jan 14, 2025

igamigo left a comment

igamigo Jan 14, 2025 •

edited

Loading

SantiagoPittella Jan 15, 2025

bobbinth Jan 16, 2025

igamigo left a comment

SantiagoPittella commented Jan 15, 2025

bobbinth left a comment

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

SantiagoPittella Jan 17, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

bobbinth Jan 16, 2025

		/// Contains the protobuf definitions
		pub const PROTO_MESSAGES: &str = include_str!("../../proto/api.proto");

feat: split miden-tx-prover into miden-proving-service and miden-remote-provers #1047

feat: split miden-tx-prover into miden-proving-service and miden-remote-provers #1047

Conversation

SantiagoPittella commented Jan 3, 2025

SantiagoPittella commented Jan 13, 2025

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo left a comment

Choose a reason for hiding this comment

igamigo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo left a comment

Choose a reason for hiding this comment

SantiagoPittella commented Jan 15, 2025

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Jan 14, 2025 •

edited

Loading