-
Notifications
You must be signed in to change notification settings - Fork 174
[bls-signatures] Add support for custom payloads for bls PoP #452
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
Conversation
502c3a0 to
6ba99e8
Compare
| /// possession signing and verification functions. See the | ||
| /// [standard](https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-bls-signature-05#section-4.2.3). | ||
| pub const POP_DST: &[u8] = b"BLS_SIG_BLS12381G1_XMD:SHA-256_SSWU_RO_POP_"; | ||
| pub const POP_DST: &[u8] = b"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"; |
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.
I also realized that this domain separation tag was written for G1 while we hash the message on G2 for PoP, so I fixed this.
zz-sol
left a comment
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.
| payload: Option<&[u8]>, | ||
| ) -> G2Projective { | ||
| if let Some(bytes) = payload { | ||
| G2Projective::hash_to_curve(bytes, POP_DST, &[]) |
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.
nits: this means bytes = Some(&[]) and bytes=None have different outputs. Is this behavior acceptable? Or should bytes=None also hash the public key?
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.
Yeah, I was thinking that this is acceptable.
I originally made the function take in prefix: Option<&[u8]> instead. It would then concatenate prefix || pubkey_bytes and then use this as the payload. This is probably the safer API because the public key will always be hashed. But if someone wants to add a suffix instead of prefix pubkey_bytes || suffix, for example, they cannot use this API. This is why I made the function to take in paylod: Option<&[u8]> instead.
I think hashing on bytes = Some(&[]) is not any safer than hashing any other string that does not have the public key bytes in it anyway, so there doesn't seem to be a justification to specialize for this case. Of course, if you think specializing addressing the case Some(&[]) to hash the public key bytes instead, then let me know 😄
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.
sounds good to me
Problem
Currently, the PoP generation API in the crate doesn't support custom payload. To support solana-foundation/solana-improvement-documents#387 where custom message is used for domain separation, we need the interface for the caller to generate and verify PoP on custom payload.
Summary of Changes
I added a field
payload: Option<&[u8]>to the PoP generation and verification functions. If this field isNone, then the functions just use the BLS public key as bytes for the payload. If this field isSome(...), then it uses the provided bytes to generate the message for PoP.