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

Switch inner product proofs to Merlin for Fiat-Shamir transform #39

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rozbb
Copy link
Collaborator

@rozbb rozbb commented Aug 22, 2021

Previously, the way F-S transcripts were done was manually with a hash function and to_bytes! calls on the transcript values. I switched everything to use the Merlin transcript constructor instead. The changes have a few benefits:

  1. Proofs are sound under composition. Transcripts can now be passed in to sub-protocols, and all the proofs in this crate now have domain separators. In addition, each value in the transcript is marked with a label, which was previously not done.
  2. The code is cleaner. D is no longer a type parameter for any of the proof structs because it's now fixed to be a Keccak backend. I also added helper functions append_serializable and challenge_scalar to clean up the proof code itself.
  3. It's possibly faster. I'm not sure how to_bytes! works, but CanonicalSerialize uses compressed representation by default. So if that's not what to_bytes! does, then the new way is faster.

One downside: the API has changed. Every prove and verify function now takes a transcript: &mut merlin::Transcript. This is necessary though if you want to allow sound composability.

Let me know if anything looks not quite right

@rozbb
Copy link
Collaborator Author

rozbb commented Aug 22, 2021

Ope I forgot to update ip_proofs/applications. Doing now.

@rozbb
Copy link
Collaborator Author

rozbb commented Aug 22, 2021

Side note: this obsoletes the other two pending PRs, as those dependencies no longer exist

@Pratyush
Copy link
Member

Hey Michael, thanks for the great PR!

Quick question: do you think it is worthwhile to integrate this as is, or to use ark-sponge to provide a sponge interface? I ask because if there is ever a need to recursively verify IPPs, then you want to use a circuit-friendly sponge, which merlin does not provide.

@rozbb
Copy link
Collaborator Author

rozbb commented Aug 23, 2021

Oh this is an interesting point I hadn't considered. The long and the short of it is: ark_sponge::CryptographicSponge doesn't expose enough functionality to implement STROBE at the moment, but it's not far off. In particular, if you don't want the TranscriptRng functionality in Merlin, then the only extra thing STROBE needs is to be able to force the underlying permute() function to run. That change looks like it'd be enough to allow for the META_AD, AD, and PRF operations, which is all that's necessary for Transcript::append_message() and Transcript::challenge_bytes().

So the downside of these changes is that, if you wanted to write an IPP protocol in a circuit, you'd have no way of doing it now. Versus previously, you could theoretically replace Blake2b with Blake2s and use the crypto-primitives version of Blake2s for your circuit.

That said, for now I think it's worth it to go with the current, non-parameterized transcript method, at the very least because of the soundness issue. And then going forward, the next steps would be to implement a CryptographicSponge::permute() method (this is a very easy change), then implement a parameterized Transcript<S: CryptographicSponge> using the above subset of STROBE, then eventually make a TranscriptVar<S: CryptographicSpongeVar>.

@Pratyush
Copy link
Member

Thanks for the analysis! That would probably be helpful for other SNARK-related protocols too. So I'll merge this with the current design, and if you'd like we can collaborate on extending sponge to integrate functionality to work with STROBE as well.

@rozbb
Copy link
Collaborator Author

rozbb commented Aug 24, 2021

Went down a rabbithole again and left a PR on ark-sponge. I think after this PR and the further changes mentioned therein, it wouldn't be hard at all to make a transcript functionality.

@Pratyush
Copy link
Member

This went really stale lol. We can merge this if you're inclined to update it, but obviously no pressure

@rozbb
Copy link
Collaborator Author

rozbb commented Oct 27, 2023

I 100% forgot I wrote this. Looks like a slog lol. Feel free to close and I'll resubmit when I get some time. Obv this is a necessary change before ripp gets used in any serious context.

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.

None yet

2 participants