supranational / supra_seal

Apache License 2.0
32 stars 21 forks source link

API to enable Groth parameter caching #3

Closed vmx closed 1 year ago

vmx commented 1 year ago

Currently in rust-fil-proofs we cache the Groth parameters. It would make sense to do the same for the SupraSeal C2. If I understand the current API, it's only possible to pass in a parameter file before proving, which will then be used for the next proof. You then later reset it. If you have another proof, with different parameters, you need to set it again.

It would be great to have some API, so that parameters need to processed only once and can then be re-used (similar to the current Rust code).

vmx commented 1 year ago

What if the SRS would just be a pointer that is passed into supraseal_c2::generate_groth16_proof? This way the Rust side could decide when to generate it.

dot-asm commented 1 year ago

This was supposedly resolved in 53beb38. The interface has Clone and Drop traits so that you don't need to treat the object as alien. If there is need to ~pick~ peek into the bulk data, then it's possible to arrange access through borrowed non-mutable slices. Well, the verifier key could, if not should, be returned by value.

vmx commented 1 year ago

Thanks, that looks good. Is SRS thread safe? If yes, it would be great to have Sync implemented.

dot-asm commented 1 year ago

Is SRS thread safe?

Trick question. The SRS itself is, but calling generate_groth16_proof_c isn't. "Isn't" in the sense that calling it simultaneously from different threads is bound to confuse CUDA to failure. Well, if it was possible to steer each generate_groth16_proof_c toward distinct GPUs, it would be possible to execute them simultaneously. Yes, with the same SRS. Well, since the question is specifically about SRS, then the answer is unequivocal yes.

If yes, it would be great to have Sync implemented.

You mean the marker simply added, right? I mean there is nothing to actually implement, right?

dot-asm commented 1 year ago

calling it [generate_groth16_proof_c] simultaneously from different threads is bound to confuse CUDA to failure.

I suppose it would be appropriate to serialize the execution of the CUDA part of the subroutine in question. Just in case :-)

dot-asm commented 1 year ago

If yes, it would be great to have Sync implemented.

You mean the marker simply added, right?

On the second thought, why not insist on cloning instead? I mean lack of Sync will make you clone, right? And the thing is that it might be preferred. Indeed, SRSs are supposed to be long-lived and you're likely to have hard time expressing its lifetime without cloning. Or is it wrong way to think about it?

On a related note. Related to referencing SRS. Is there a way to prevent application code from taking a mutable reference?

vmx commented 1 year ago

You mean the marker simply added, right? I mean there is nothing to actually implement, right?

Yes, correct.

The reason I ask for it is, that our current code base has a trait for parameters, which requires the underlying things to be Sync. So easiest for me would be, if SRS is thread safe, to just implement that trait and be done.

vmx commented 1 year ago

Is there a way to prevent application code from taking a mutable reference?

I don't think so. Though in case of SRS I'd say it's kind of immutable. Do don't really have any methods to mutate it. Or I maybe misunderstood your question.

vmx commented 1 year ago

I mean lack of Sync will make you clone, right? And the thing is that it might be preferred. Indeed, SRSs are supposed to be long-lived and you're likely to have hard time expressing its lifetime without cloning. Or is it wrong way to think about it?

Yes you would clone instead. (~Though also clone would need to be implemented~ Clone is already implemented.) I don't think expressing its longevity is a problem. You just create it once and then pass on the references to it.

vmx commented 1 year ago

As you mentioned that SRS is thread safe, also having the Send marker would be great. Then I can use it in a less hacky way on my side.

dot-asm commented 1 year ago

Is there a way to prevent application code from taking a mutable reference?

I don't think so. Though in case of SRS I'd say it's kind of immutable. Do don't really have any methods to mutate it. Or I maybe misunderstood your question.

Yes, there are no mutating methods, and the compiler prevents dereferencing thanks to the lack of Deref and Copy traits. But I've wished to take it to the ultimate level and have the compiler prevent the user from even writing &mut srs:-) Oh well, if there is no way, then there is no way:-)

I've added the Sync trait, but keep in mind that it's not safe to simultaneously call the generate_groth16_proof_c from multiple threads yet...

vmx commented 1 year ago

I've added the Sync trait, but keep in mind that it's not safe to simultaneously call the generate_groth16_proof_c from multiple threads yet...

I also don't think I need that. This wasn't supported in our code base anyway.

I checked again why I need it. We store our caches via lazy_static and that requires things to be thread safe (unless you do wrap then).

Thanks for adding Sync, please also add Send, which I also need for the lazy_static reason.

dot-asm commented 1 year ago

please also add Send

Fixed.

vmx commented 1 year ago

The API for the parameters seems to work as expected, thanks a lot!