-
Notifications
You must be signed in to change notification settings - Fork 11
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: use TranscriptRng
for random number generation
#109
feat: use TranscriptRng
for random number generation
#109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One optimisation question: how do you interpret this note in the docs? Since the witness data cannot be updated within the prove_with_rng
function (immutable reference), does the TranscriptRng need to be reinitialized multiple times within the function? It's not clear to me if they mean the TranscriptRng
should be kept up to date with the witness data or if each time it is used once to generate a random number a new instance is needed (seems doubtful/doesn't make sense, but if that is the case this is not done everywhere e.g. lines 524,525 etc).
Unfortunately yes. It's built from a clone of the current
The witness data is fixed at the start of proof generation and doesn't change. The only reason it needs to be added to each The linked documentation is a bit unclear when it advises reinitializing the That being said, I could have made the intent more clear by including better comments and placing the reinitializations more directly at the state changes. I'll make a small update to do so, and to include your other review suggestion. |
I made more changes than originally expected in order to make transcript and random number generator functionality more clear. In particular, the previous transcript helper functions have been moved into The prover and verifier transcript operations were correspondingly simplified, and each |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM - code is much clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi looking good, just some small changes, please.
After discussing with @hansieodendaal, it was agreed that it would be best to move the transcript RNG updates into The most recent commit does this. Transcript initialization returns a The verifier now also uses a |
TranscriptRng
for prover noncesTranscriptRng
for random number generation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
ACK
Recent work in #109 uses Merlin's `TranscriptRng` functionality to improve the handling of random number generation, and also refactors transcript operations for safer use through a `RangeProofTranscript` wrapper. A [suggestion](#109 (comment)) by @sdbondi recommends moving the `TranscriptRng` into `RangeProofTranscript` so the prover and verifier aren't responsible for it. This PR adds such a change. It also updates `RangeProofTranscript` to hold a mutable reference to the external random number generator. This ensures the prover and verifier can't accidentally use it instead of the `TranscriptRng`.
As noted in #108, it's possible to improve the robustness of prover nonces by using Merlin's
TranscriptRng
functionality.This PR moves all prover nonce generation to this design. In particular, it ensures that the most up-to-date state of the transcript is used each time the prover needs secure randomness (per the Merlin documentation). The verifier also uses this to produce its random weights.
It also takes the opportunity to refactor transcript operations so they are cleaner and likely safer to use.
Closes #108.