-
Notifications
You must be signed in to change notification settings - Fork 0
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
Zero Knowledge Proofs #2
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.
Looks mostly good, most of the comments are stylistic except for the challenge scalar issue in CiphertextValidityProof
pkg/encryption/elgamal/common.go
Outdated
@@ -12,6 +14,11 @@ import ( | |||
// H_STRING H is a random point on the elliptic curve that is unrelated to G. | |||
const H_STRING = "gPt25pi0eDphSiXWu0BIeIvyVATCtwhslTqfqvNhW2c" | |||
|
|||
// GenerateKey generates a new ECDSA key pair. | |||
func GenerateKey() (*ecdsa.PrivateKey, error) { |
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.
This seems to be useful more as a testing function? Should we still export this?
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.
If we do want to export it we could consider renaming it since it may be misleading
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.
This is used for testing, but across multiple packages.
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.
Gotcha, any way to rename this or move this to some test utils file?
This is exposed to user as elgamal.GenerateKey(), which might be confusing (since it might suggest the generation of a el gamal Private Key or el gamal Public Key). Maybe just rename to GenerateEcdsaKey?
pkg/zkproofs/range.go
Outdated
} | ||
|
||
// VerifyRangeProof verifies the range proof for the given ciphertext | ||
func VerifyRangeProof(proof *RangeProof, ciphertext *elgamal.Ciphertext) (bool, error) { |
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.
Similar comment as above, we could consider adding the upperBound to this method as well, since it should be up to the verifier to determine what upper bound they want to check for (just that the associated proof should not work if it's not meant for that upper bound)
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.
Added
@mj850 addressed most comments. Ptal |
// Step 0: Recompute the challenge using the Fiat-Shamir heuristic. | ||
transcript := NewProofTranscript() | ||
transcript.AppendMessage("C1", proof.Commitment1.ToAffineCompressed()) | ||
transcript.AppendMessage("C2", proof.Commitment2.ToAffineCompressed()) |
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.
Think we want to add ct.C and ct.D to the challenge as well so the proof can only be used as for the specific ciphertext.
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.
Done
// the proof cannot be for some other set of values. | ||
transcript := NewProofTranscript() | ||
transcript.AppendMessage("C1", Commitment1.ToAffineCompressed()) | ||
transcript.AppendMessage("C2", Commitment2.ToAffineCompressed()) |
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.
Think we need to add ciphertext.C and ciphertext.D to the challenge transcript here as well to bind the proof to the specific ciphertext.
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.
Done
@@ -0,0 +1,39 @@ | |||
package zkproofs |
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.
Nit: Should we rename this file to proof_transcript.go
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.
Done
@mj850 addressed comments, please have another look |
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.
Approved!
Still have minor nit in pkg/encryption/elgamal/common.go, lmk what you think there
This PR contains the following zero knowledge proofs:
Tests: