feat: use remote batch prover #701

SantiagoPittella · 2025-02-17T21:28:29Z

This PR replaces the local batch prover with the remote one.

It adds the remote_batch_prover endpoint to the configuration file, that will be used at node startup to create a RemoteBatchProver.

At the moment, I removed the miden-faucet package from certain makefile commands because of an error regarding the mix of async and non async usage of miden-base dependencies.

The formatting of the Cargo.toml is a bit odd after adding some dependencies, but it is compliant with taplo. I'm investigating what can be causing the issue.

Mirko-von-Leipzig

The code seems good; I'm just unhappy with requiring an external service out-of-process by default. Feels pretty unergonomic

.github/workflows/test.yml

Cargo.lock

Makefile

crates/block-producer/src/batch_builder/mod.rs

bobbinth

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

bobbinth · 2025-02-22T11:41:41Z

crates/block-producer/src/config.rs

        f.write_fmt(format_args!(
-            "{{ endpoint: \"{}\", store_url: \"{}\" }}",
-            self.endpoint, self.store_url
+            "{{ endpoint: \"{}\", store_url: \"{}\", remote_batch_prover: \"{}\" }}",
+            self.endpoint,
+            self.store_url,
+            self.remote_batch_prover
+                .as_ref()
+                .map_or("None".to_string(), |url| url.to_string())
        ))


nit: is there a better way to write this out? e.g., using multiple statements instead of a single statement.

crates/block-producer/src/config.rs

bobbinth · 2025-02-22T11:45:14Z

crates/block-producer/src/batch_builder/mod.rs

+    pub fn new_local(security_level: u32) -> Self {
+        warn!(target: COMPONENT, "Using local batch prover");
+        Self::Local(LocalBatchProver::new(security_level))
+    }
+
+    pub fn new_remote(endpoint: impl Into<String>) -> Self {
+        warn!(target: COMPONENT, "Using remote batch prover");
+        Self::Remote(RemoteBatchProver::new(endpoint))
+    }


Question: why do we use warn! here?

I was using it with debug purposes (I using Ghostty and not having search scrollback is not playing out nice), to distinguish these logs easily. I'm replacing it with info.

bobbinth · 2025-02-22T11:46:44Z

crates/block-producer/src/batch_builder/mod.rs

+        Self::Remote(RemoteBatchProver::new(endpoint))
+    }
+
+    pub async fn prove(&self, proposed_batch: ProposedBatch) -> Result<ProvenBatch, BuildBatchError> {


We should probably instrument this function, no? And then maybe we don't need the info! statements inside?

bobbinth · 2025-02-22T11:50:09Z

bin/node/src/config.rs

@@ -27,6 +27,7 @@ struct NormalizedRpcConfig {
 struct NormalizedBlockProducerConfig {
    endpoint: Url,
    verify_tx_proofs: bool,
+    remote_batch_prover: Option<Url>,


I would probably name this: batch_prover_url.

bobbinth · 2025-02-22T11:51:16Z

crates/utils/src/config.rs

@@ -10,6 +10,7 @@ pub const DEFAULT_NODE_RPC_PORT: u16 = 57291;
 pub const DEFAULT_BLOCK_PRODUCER_PORT: u16 = 48046;
 pub const DEFAULT_STORE_PORT: u16 = 28943;
 pub const DEFAULT_FAUCET_SERVER_PORT: u16 = 8080;
+pub const DEFAULT_BATCH_PROVER_PORT: u16 = 8082;


How did we come up with this default port value?

I was using it to match the value that i was using in the Makefile target that was initializing the worker. Since now we have a local prover coexisting with the remote, this default value is not needed anymore since we are defaulting to local. I'm removing the constant.

bobbinth · 2025-02-22T12:01:49Z

crates/block-producer/src/server.rs

@@ -90,6 +97,7 @@ impl BlockProducer {
            store,
            rpc_listener,
            chain_tip,
+            remote_batch_prover,


I wonder whether instead of making remote_batch_prover a field in BlockProducer, we should just pass it to the BatchBuilder. Specifically, we could have BatchBuilder::new() constructor which looks something like this:

impl BatchBuilder { pub fn new(prover_url: Option<Url>) -> Self { ... } }

Then, how to construct the prover would be the internal concern of the BatchBuilder, and here, we'd just do something like:

BatchBuilder::new(config.batch_prover_url)

bobbinth · 2025-02-22T12:07:54Z

crates/block-producer/src/batch_builder/mod.rs

+    pub async fn run(
+        self,
+        mempool: SharedMempool,
+        store: StoreClient,
+        batch_prover: BatchProver,
+    ) {


Related to the previous comment, if we make batch_prover a field of BatchBuilder, we won't need to pass it here.

Also, not from this PR, but we have a slight inconsistency here between batch and block builders: BlockBuilder takes the store as a construction parameter, but BatchBuilder takes it via the run method. Should we also move store into the constructor for BatchBuilder? @Mirko-von-Leipzig - what do you think?

I've been meaning to relook at these as part of #676.

Its also semi-related to properly decoupling components, i.e. a block-producer should be able to startup independently from the store.

I think we can align them now; but we should take a closer look in the medium term. Bit of a non-answer I know 🙃

SantiagoPittella · 2025-02-24T20:10:49Z

crates/block-producer/Cargo.toml

Not loving how I break the formatting, but since I introduced a multi line dependency taplo is formatting this way.

You could try fiddle with some of these: https://taplo.tamasfe.dev/configuration/formatter-options.html.

I imagine it would look nicer as a field per line once it exceeds the column limit.

But in the end if nothing looks good then it is what it is :) At least its consistent.

SantiagoPittella · 2025-02-24T20:15:43Z

@bobbinth I ended up rebasing with Philipp's branch. It does not use any of the features provided in his PR, but the changes the changes for it to work with the latest in miden base.

~~I'm currently running the client integration tests against this branch.~~ Just ended up the run, it works 👌🏼

Mirko-von-Leipzig · 2025-02-25T14:33:20Z

@SantiagoPittella I think you need to rebase to next because that PR was merged.

docs: add PR number to Changelog remove remote prover from CI and makefile remove remote prover from README feat: create BatchProver enum, make remote proving optional update to latest changes in miden-base fix: udpate with miden-base latest review: use multiple statements to print the configs review: replace warn! with info! in prover init review: add instrumentation to batch prover review: rename remote_batch_prover to batch_prover_url review: remove default batch prover url constant review: move the batch_prover field from BatchProducer to BatchBuilder fix: rollback changes in Makefile docs: update CHANGELOG, add change to readme fix: make it compatible with miden-base

Mirko-von-Leipzig

Some minor doc nits, but LGTM

Mirko-von-Leipzig · 2025-02-26T07:41:39Z

crates/block-producer/src/batch_builder/mod.rs

+    /// The batch prover to use.
+    ///
+    /// If not provided, a local batch prover is used.
+    batch_prover: BatchProver,


The optional conversion happens in new so lets document it there only.

Suggested change

/// The batch prover to use.

///

/// If not provided, a local batch prover is used.

batch_prover: BatchProver,

/// The batch prover to use.

batch_prover: BatchProver,

Mirko-von-Leipzig · 2025-02-26T07:43:14Z

crates/block-producer/src/batch_builder/mod.rs

+    /// Creates a new [`BatchBuilder`] with the given batch prover URL.
+    ///
+    /// If no URL is provided, a local batch prover is used.
+    pub fn new(batch_prover_url: Option<Url>) -> Self {


Suggested change

/// Creates a new [`BatchBuilder`] with the given batch prover URL.

///

/// If no URL is provided, a local batch prover is used.

pub fn new(batch_prover_url: Option<Url>) -> Self {

/// Creates a new [`BatchBuilder`] with the given batch prover URL.

///

/// Defaults to [`BatchProver::Local`] is no URL is provided.

pub fn new(batch_prover_url: Option<Url>) -> Self {

Mirko-von-Leipzig reviewed Feb 18, 2025

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

Cargo.lock Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

crates/block-producer/src/batch_builder/mod.rs Outdated Show resolved Hide resolved

Mirko-von-Leipzig mentioned this pull request Feb 20, 2025

Use tower trace layer #698

Draft

This was referenced Feb 21, 2025

Remote block and batch provers #715

Open

Use remote batch prover #716

Open

bobbinth reviewed Feb 22, 2025

View reviewed changes

Mirko-von-Leipzig mentioned this pull request Feb 23, 2025

Remove verify-tx-proofs config item #718

Open

SantiagoPittella force-pushed the santiagopittella-use-remote-batch-prover branch from 38ad61d to d03caff Compare February 24, 2025 20:07

SantiagoPittella changed the base branch from next to pgackst-migrate-block-prover February 24, 2025 20:08

SantiagoPittella commented Feb 24, 2025

View reviewed changes

SantiagoPittella requested a review from bobbinth February 24, 2025 20:23

Base automatically changed from pgackst-migrate-block-prover to next February 25, 2025 06:40

SantiagoPittella force-pushed the santiagopittella-use-remote-batch-prover branch from 99be9e4 to c0e0b04 Compare February 25, 2025 15:18

SantiagoPittella force-pushed the santiagopittella-use-remote-batch-prover branch from c0e0b04 to 7b80750 Compare February 25, 2025 18:22

SantiagoPittella requested a review from Mirko-von-Leipzig February 25, 2025 18:26

Mirko-von-Leipzig approved these changes Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use remote batch prover #701

feat: use remote batch prover #701

SantiagoPittella commented Feb 17, 2025

Mirko-von-Leipzig left a comment

bobbinth left a comment

bobbinth Feb 22, 2025

bobbinth Feb 22, 2025

SantiagoPittella Feb 24, 2025

bobbinth Feb 22, 2025

bobbinth Feb 22, 2025

bobbinth Feb 22, 2025

SantiagoPittella Feb 24, 2025

bobbinth Feb 22, 2025

bobbinth Feb 22, 2025

Mirko-von-Leipzig Feb 23, 2025

SantiagoPittella Feb 24, 2025

Mirko-von-Leipzig Feb 26, 2025

SantiagoPittella commented Feb 24, 2025 •

edited

Loading

Mirko-von-Leipzig commented Feb 25, 2025

Mirko-von-Leipzig left a comment

Mirko-von-Leipzig Feb 26, 2025

Mirko-von-Leipzig Feb 26, 2025

feat: use remote batch prover #701

Are you sure you want to change the base?

feat: use remote batch prover #701

Conversation

SantiagoPittella commented Feb 17, 2025

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

SantiagoPittella commented Feb 24, 2025 • edited Loading

Mirko-von-Leipzig commented Feb 25, 2025

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoPittella commented Feb 24, 2025 •

edited

Loading