feat: add remote prover tests run on monitor by SantiagoPittella · Pull Request #1236 · 0xMiden/node

SantiagoPittella · 2025-09-19T21:58:55Z

Send periodical prove requests to the remote provers.

I ended up refactoring a bit the code for the sake of simplicity and type safety.

sergerad · 2025-09-22T20:13:07Z

+        .without_metadata_genesis()
+        .connect_lazy::<RemoteProverClient>();
+
+    let mut interval = tokio::time::interval(Duration::from_secs(30)); // Test every 30 seconds


Just wondering if we should make this relatively large - say 1 or 2 minutes? Particularly if we expect testnet/mainnet provers to be under significant load (I'm not sure how much load this request itself involves).

And it would be good to make this configurable but not required for this PR.

Yes, agree that the best is making this configurable. The configuration options is growing and I'm thinking that switching to clap might be a good idea, wdyt?

In the meantime, I changed it to 2 mins.

Yea clap sounds good. Main thing is to have everything configurable via env vars as always.

bobbinth · 2025-09-22T22:46:42Z

Not a review yet (I haven't looked at the code) - but curious how does the frontend look now? Specifically, what kind of info do we show for each proof "probe".

Also, how are we building the proof request? For this we need to have a transaction, right? Are we using a transaction against the faucet, or something else?

SantiagoPittella · 2025-09-23T15:08:19Z

This is the current frontend. For the failed request we are displaying the full error. Should I change that? The cards resize them based on the content of the largest one in the row. If we reduce the error messages, cards can be much smaller.

In the image an errors is shown. This is because of how we build the transaction, in this case, but could happen the same with blocks and batches. To build the test payload we have the following scenarios:

for transaction provers: we use the MockChain to create a P2ID note to be then used in a tx.
for block provers: we use the MockChain to create a ProposedBlock with no batches.
for batch provers: we use the MockChain and the same tx that we used for the tx prover, then we prove it with a local prover and use that ProvenTransaction to generate a ProposedBatch.

This approach has the benefit of being simple, but lacks compatibility across different versions. This is why using the deployed remote transaction prover is failing with the deserialization errors. I do think that supporting different versions might not be really useful and could be a bit tricky to maintain. An option that I thought of (which I'm not a fan) to fix this could be to add an endpoint to the provers that returns a valid transaction/batch/block to be proved, enabling us to support any remote prover version..

bobbinth · 2025-09-23T17:35:35Z

Looks really good!

For the failed request we are displaying the full error. Should I change that? The cards resize them based on the content of the largest one in the row. If we reduce the error messages, cards can be much smaller.

If not too difficult, I'd show a part of the error message and then an option to copy the rest of the message to clipboard. So, for example, we could show just "InvalidArgument" and then case a "copy" button next to it.

for transaction provers: we use the MockChain to create a P2ID note to be then used in a tx.

for block provers: we use the MockChain to create a ProposedBlock with no batches.

for batch provers: we use the MockChain and the same tx that we used for the tx prover, then we prove it with a local prover and use that ProvenTransaction to generate a ProposedBatch.

For now, let's just to transaction proofs - we can add other proof types later.

Regarding proof generation - I think using MockChain is fine for now, but I do wonder if in the longer term we'd want to build "real" proofs somehow. This could be done by deploying some "test wallet" and executing a real transaction against it. I'd leave this for the future though.

This approach has the benefit of being simple, but lacks compatibility across different versions. This is why using the deployed remote transaction prover is failing with the deserialization errors. I do think that supporting different versions might not be really useful and could be a bit tricky to maintain. An option that I thought of (which I'm not a fan) to fix this could be to add an endpoint to the provers that returns a valid transaction/batch/block to be proved, enabling us to support any remote prover version..

I think this is fine for now and I think it may be useful to see anyway that the status monitor is out of sync with the underlying network. So, I'd keep things as is.

SantiagoPittella · 2025-09-23T18:29:24Z

Done! I updated it to display just what's in the "status" field of a tonic error. The button copies the whole error with JSON format.

Regarding proof generation - I think using MockChain is fine for now, but I do wonder if in the longer term we'd want to build "real" proofs somehow. This could be done by deploying some "test wallet" and executing a real transaction against it. I'd leave this for the future though.

In the main issue of the network monitor, another feature is mentioned and is to send a real transaction to the network. Does that cover what you are mentioning? Because in that case we will be using a "real" transaction.

For now, let's just to transaction proofs - we can add other proof types later.

No problem, for the future: do we want to generate the proposed batch/block in any particular fashion?

SantiagoPittella · 2025-09-23T18:35:25Z

This is how it looks after removing the tests from non-transaction provers.

bobbinth · 2025-09-23T19:09:52Z

Done! I updated it to display just what's in the "status" field of a tonic error. The button copies the whole error with JSON format.

Nice! Another UI question: in the RPC card, is there not enough space to show the full genesis block hash? (it feels like it should almost fit there).

In the main issue of the network monitor, another feature is mentioned and is to send a real transaction to the network. Does that cover what you are mentioning? Because in that case we will be using a "real" transaction.

I think these are separate because this would test the full flow (not just the remote prover). But we could probably use the transaction we generate there to submit to the remote prover as well. This way, we'll test both things.

No problem, for the future: do we want to generate the proposed batch/block in any particular fashion?

Not sure yet. Maybe we use the "real" transaction discussed above to build a single-transaction batch, and then build a single-batch block. But also, this is fairly far away - so, things could change.

SantiagoPittella · 2025-09-24T18:03:37Z

Nice! Another UI question: in the RPC card, is there not enough space to show the full genesis block hash? (it feels like it should almost fit there).

I'm pasting an image of how it looks with this change (without fixing the card width), the part of the commitment that is being displayed is ~2/3 of the whole commitment. I think that it is too large, we can fit it but making the card that wide will make us to have a lot of free space. Having the short version + the copy button looks better IMO and functionality-wise is the same information.

bobbinth · 2025-09-25T05:42:05Z

I'm pasting an image of how it looks with this change (without fixing the card width), the part of the commitment that is being displayed is ~2/3 of the whole commitment. I think that it is too large, we can fit it but making the card that wide will make us to have a lot of free space. Having the short version + the copy button looks better IMO and functionality-wise is the same information.

Yes, agreed - let's keep the truncated version. One small suggestion: I'd probably prefix it with 0x.

Separately - what's outstanding on this PR? Or are we just waiting for more reviews on it?

SantiagoPittella · 2025-09-25T16:25:17Z

Separately - what's outstanding on this PR? Or are we just waiting for more reviews on it?

With this last thing addressed, this PR is ready.

bobbinth

A very shallow review from me - but looks good! Thank you!

SantiagoPittella force-pushed the santiagopittella-remote-prover-tests branch from 8b62ee3 to e5924b7 Compare September 19, 2025 21:59

Base automatically changed from santiagopittella-one-task-per-component to next September 19, 2025 22:01

feat: add remote prover tests run on monitor

90aff20

SantiagoPittella force-pushed the santiagopittella-remote-prover-tests branch from e5924b7 to 90aff20 Compare September 19, 2025 22:05

fix: remove unused struct

5214046

SantiagoPittella mentioned this pull request Sep 22, 2025

fix: add env var to enable OTel in network monitor #1237

Merged

SantiagoPittella requested review from Mirko-von-Leipzig, bobbinth, drahnr, igamigo and sergerad September 22, 2025 16:58

sergerad reviewed Sep 22, 2025

View reviewed changes

sergerad approved these changes Sep 22, 2025

View reviewed changes

review: update interval to 2 minutes

66f5baa

SantiagoPittella added 2 commits September 23, 2025 11:13

Merge branch 'next' into santiagopittella-remote-prover-tests

c8f04f3

review: add enable_otl field to MonitorConfig:

5b736df

review: display only the error code

b719b2d

SantiagoPittella added 2 commits September 23, 2025 15:36

review: remove non-transaction provers tests

782448e

chore: clean unused proposed block/batch generation

de7d7eb

Mirko-von-Leipzig reviewed Sep 25, 2025

View reviewed changes

Comment thread bin/network-monitor/src/remote_prover.rs Outdated

Mirko-von-Leipzig approved these changes Sep 25, 2025

View reviewed changes

review: add 0x prefix to commitment

5dd5526

SantiagoPittella and others added 2 commits September 25, 2025 09:00

feat: add faucet check in network monitor (#1240)

6aa8b1d

review: remove errors from spawn functions

f4b5716

bobbinth approved these changes Sep 25, 2025

View reviewed changes

bobbinth merged commit 881576e into next Sep 25, 2025
6 checks passed

bobbinth deleted the santiagopittella-remote-prover-tests branch September 25, 2025 20:52

Conversation

SantiagoPittella commented Sep 19, 2025

Uh oh!

sergerad Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

SantiagoPittella Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

SantiagoPittella Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

sergerad Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

bobbinth commented Sep 22, 2025

Uh oh!

SantiagoPittella commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbinth commented Sep 23, 2025

Uh oh!

SantiagoPittella commented Sep 23, 2025

Uh oh!

SantiagoPittella commented Sep 23, 2025

Uh oh!

bobbinth commented Sep 23, 2025

Uh oh!

SantiagoPittella commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbinth commented Sep 25, 2025

Uh oh!

Uh oh!

SantiagoPittella commented Sep 25, 2025

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SantiagoPittella commented Sep 23, 2025 •

edited

Loading

SantiagoPittella commented Sep 24, 2025 •

edited

Loading