Skip to content
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.

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

feat: transaction prover service #881

Merged
merged 37 commits into from
Sep 26, 2024

Conversation

SantiagoPittella
Copy link
Collaborator

closes #860

@bobbinth
Copy link
Contributor

Thank you! Not a review - just a couple of small comments:

  • Could we move the code related to TransactionWitness serialization into a different PR? We can probably merge that PR pretty quickly and it would also reduce the complexity of this PR.
  • I would put the miden-tx-prover crate under bin/tx-prover (we can also move bench-tx into the same directory in a different PR later on).

@SantiagoPittella SantiagoPittella force-pushed the santiagopittella-transaction-prover-service branch 2 times, most recently from 243d3cd to debb0e7 Compare September 20, 2024 14:11
@SantiagoPittella SantiagoPittella marked this pull request as ready for review September 20, 2024 14:11
@SantiagoPittella
Copy link
Collaborator Author

I've removed thing related to RemoteTransactionProver from this PR to minimize the scope.

@SantiagoPittella
Copy link
Collaborator Author

I added an unimplemented for the async feature because the TransactionProver trait in that case is async, and it was not compiling because I use a LocalTransactionProver, which has an Rc which is not thread safe.
I also tried to avoid that lib from compiling when using cargo build --lib but did not find anything that would stop it, since --exclude <package> doesn't work when using --lib.

@SantiagoPittella SantiagoPittella force-pushed the santiagopittella-transaction-prover-service branch from 791208a to 568ffb5 Compare September 20, 2024 18:11
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good. I left some comments inline.

Cargo.toml Outdated Show resolved Hide resolved
bin/miden-tx-prover/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file could be quite a bit simpler. The main reason we have most of this complexity in miden-node is because .proto files are not in the crate directory and so we need to copy them otherwise building on crates.io` doesn't work.

But here, things are pretty standard - so, we should be able to avoid many complexity here including the need for BUILD_PROTO environment variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll check this.

bin/miden-tx-prover/src/bin.rs Outdated Show resolved Hide resolved
bin/miden-tx-prover/src/server/mod.rs Outdated Show resolved Hide resolved
bin/miden-tx-prover/src/server/mod.rs Outdated Show resolved Hide resolved
bin/miden-tx-prover/src/domain/mod.rs Outdated Show resolved Hide resolved
bin/miden-tx-prover/Cargo.toml Outdated Show resolved Hide resolved
bin/miden-tx-prover/Cargo.toml Outdated Show resolved Hide resolved
bin/miden-tx-prover/src/server/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some more comments inline.

bin/tx-prover/Cargo.toml Outdated Show resolved Hide resolved
bin/tx-prover/Cargo.toml Outdated Show resolved Hide resolved
bin/tx-prover/Cargo.toml Outdated Show resolved Hide resolved
bin/tx-prover/src/server/mod.rs Show resolved Hide resolved
bin/tx-prover/src/server/mod.rs Outdated Show resolved Hide resolved
miden-tx/src/errors/mod.rs Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
bin/tx-prover/README.md Outdated Show resolved Hide resolved
bin/tx-prover/README.md Outdated Show resolved Hide resolved
bin/tx-prover/README.md Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few more small comments inline. Once these are addressed, we can merge.

bin/tx-prover/Cargo.toml Outdated Show resolved Hide resolved
bin/tx-prover/build.rs Outdated Show resolved Hide resolved
bin/tx-prover/src/server/mod.rs Show resolved Hide resolved
miden-tx/src/errors/mod.rs Outdated Show resolved Hide resolved
@SantiagoPittella
Copy link
Collaborator Author

SantiagoPittella commented Sep 25, 2024

Regarding:

Let's use Arc instead of Rc.

I'm on it, internally TransactionMastStore uses a RefCell that I'm changing to RwLock in order to support sharing between threads.

cc @bobbinth

@igamigo
Copy link
Collaborator

igamigo commented Sep 25, 2024

Using RwLock as-is is likely not be a possibility due to the std requirement.
EDIT: Didn't know miden_lib was using parking_lot!

@SantiagoPittella
Copy link
Collaborator Author

Using RwLock as-is is likely not be a possibility due to the std requirement.

I'm using the RwLock exported by miden_lib which uses the parking_lots version instead of the std one.

@bobbinth
Copy link
Contributor

I'm on it, internally TransactionMastStore uses a RefCell that I'm changing to RwLock in order to support sharing between threads.

Yeah, we could use RwLock from miden-core crate (we have our own no-std implementation there which is based on spinlock). But I wonder if the simpler approach would have been to do something like:

pub struct RpcApi {
    prover: Mutex<Arc<LocalTransactionProver>>,
}

Or would that not work for some reason?

@SantiagoPittella
Copy link
Collaborator Author

I'm on it, internally TransactionMastStore uses a RefCell that I'm changing to RwLock in order to support sharing between threads.

Yeah, we could use RwLock from miden-core crate (we have our own no-std implementation there which is based on spinlock). But I wonder if the simpler approach would have been to do something like:

pub struct RpcApi {
    prover: Mutex<Arc<LocalTransactionProver>>,
}

Or would that not work for some reason?

That wouldn't solve the issue of RefCell<BTreeMap<RpoDigest, std::sync::Arc<MastForest>>> not implenting Sync. I'm not sure on how to solve that without using RwLock or adding some unsafe impl Sync.

Regarding RwLock: yes, I'm using the miden-lib version which is a re-export of the miden-core one.

Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

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

LGTM!

@bobbinth
Copy link
Contributor

That wouldn't solve the issue of RefCell<BTreeMap<RpoDigest, std::sync::Arc<MastForest>>> not implenting Sync. I'm not sure on how to solve that without using RwLock or adding some unsafe impl Sync.

Thinking more about this, I wonder if the previous approach of using unsafe impl Sync was better. Basically, LocalTransactionProver, as it stands now, is not meant to be shared between threads. Doing so in RpcApi is fine because we put it behind Mutex. So, rather than making LocalTransactionProver sharable between threads, it makes sense to manually indicate that RpcApi is sharable. What do you think?

@SantiagoPittella
Copy link
Collaborator Author

SantiagoPittella commented Sep 26, 2024

Thinking more about this, I wonder if the previous approach of using unsafe impl Sync was better. Basically, LocalTransactionProver, as it stands now, is not meant to be shared between threads. Doing so in RpcApi is fine because we put it behind Mutex. So, rather than making LocalTransactionProver sharable between threads, it makes sense to manually indicate that RpcApi is sharable. What do you think?

Yes, I agree with that. Making LocalTransactionProver thread safe is unneeded. I prefer having an explicit unsafe impl for the RpcApi which is much obvious why it needs to be Sync + Send (also, this way we don't introduce an unneeded breaking change too).
I'm removing the commit that introduces that change. If you realize there is an ulterior motive for using Arc and RwLock though let me know and I can re-introduce them.

@SantiagoPittella SantiagoPittella force-pushed the santiagopittella-transaction-prover-service branch from 85828b8 to 1cbb27d Compare September 26, 2024 12:53
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

Comment on lines +31 to +34
// We need to implement Send and Sync for the generated code to be able to use the prover in the
// shared context.
unsafe impl Send for RpcApi {}
unsafe impl Sync for RpcApi {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll merge as is, but ideally, the comment should explain why it is OK to implement Send and Sync.

@bobbinth bobbinth merged commit 148258b into next Sep 26, 2024
8 checks passed
@bobbinth bobbinth deleted the santiagopittella-transaction-prover-service branch September 26, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants