feat: transaction prover service

[SantiagoPittella]

closes #860

[bobbinth]

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]

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

[SantiagoPittella]

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.

[bobbinth]

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.

Do we actually need it with async feature? Maybe sync version would be sufficient here?

Also, we need to implement some strategy of handling multiple requests. As far as I can tell, right now it'll try to execute all incoming requests rather than queueing them.

[SantiagoPittella]

Answering the questions related to the "async" feature and the unimplemented:

When using make build-async, the command is the following:

.PHONY: build-async
build-async: ## Build with the `async` feature enabled (only libraries)
    ${BUILD_PROTO} cargo build --lib --release --features async

This causes an error when compiling the bin/miden-tx-prover/.../lib.rs since we are using the TransactionProver trait, which is maybe_async, and the LocalTransactionProver which can not be async. 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 doesn't work when using --lib.

I added the feature flag to simply do nothing there when using the feature async, but I'm open to any other idea.

[bobbinth]

This causes an error when compiling the bin/miden-tx-prover/.../lib.rs since we are using the TransactionProver trait, which is maybe_async, and the LocalTransactionProver which can not be async. 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 doesn't work when using --lib.

If we make miden-tx-prover a pure binary (i.e., no lib component, similar to how bench-tx works), would it solve the issue?

[SantiagoPittella]

This causes an error when compiling the bin/miden-tx-prover/.../lib.rs since we are using the TransactionProver trait, which is maybe_async, and the LocalTransactionProver which can not be async. 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 doesn't work when using --lib.

If we make miden-tx-prover a pure binary (i.e., no lib component, similar to how bench-tx works), would it solve the issue?

This causes an error when compiling the bin/miden-tx-prover/.../lib.rs since we are using the TransactionProver trait, which is maybe_async, and the LocalTransactionProver which can not be async. 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 doesn't work when using --lib.

If we make miden-tx-prover a pure binary (i.e., no lib component, similar to how bench-tx works), would it solve the issue?

Yes, it did solve the issue.

[SantiagoPittella]

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]

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]

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]

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]

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.

[bobbinth]

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]

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.