-
Notifications
You must be signed in to change notification settings - Fork 158
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
Refactor/break kzg srs #378
Conversation
// encode the ProvingKey | ||
enc := {{ .CurvePackage }}.NewEncoder(w) | ||
|
||
toEncode := []interface{}{ |
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.
no need for slice and for loop, just enc.Encode(pk.G1)
return enc.BytesWritten(), nil | ||
} | ||
|
||
// WriteRawTo writes binary encoding of Proof to w without point compression |
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.
Proof -> VerifyingKey
func (srs *SRS) WriteTo(w io.Writer) (int64, error) { | ||
// encode the SRS | ||
// encode the VerifyingKey |
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.
bad comment.
Here I would rather call srs.Pk.WriteTo, srs.Vk.WriteTo, easier to maintain for future changes.
There is a duplicate point but it's not a crazy overhead.
dec := {{ .CurvePackage }}.NewDecoder(r) | ||
|
||
toDecode := []interface{}{ | ||
&pk.G1, |
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.
just dec.Decode(&pk.G1)
&srs.G2[0], | ||
&srs.G2[1], | ||
&srs.G1, | ||
&srs.Vk.G2[0], |
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.
same srs.Pk.ReadFrom...
srs.Vk.ReadFrom(...)
Could you modify
to
in BatchVerifyMultiPoints in the kzg.go files (e.g. l.419 for bls377) ? (Spotted by @kevaundray ) |
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.
minor comment on godoc format but lgtm 👍
// SRS stores the result of the MPC | ||
type SRS struct { | ||
// Proving and Verifying keys together constitute the SRS (result of the MPC) | ||
type ProvingKey struct { |
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.
godoc format: struct doc should be // ProvingKey ...
(else it isn't render properly in godoc)
The KZG SRS breaks up naturally into a prover component (G1[:]) and a verifier component (G1[0], G2[0:2]), which only have one G1 point in common (which is usually standardized and can be omitted anyway).
Treating it as a single object results in very long Plonk verifying keys for example, which can be problematic in Smart Contracts.
This PR implements the break-up.
WARNING: I've removed pretty much all pass-by-pointers I encountered. In case some of that was performance-critical, lmk and I'll add it back in.