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

[Merged by Bors] - Update blst to official crate and incorporate subgroup changes #1979

Closed
wants to merge 1 commit into from

Conversation

sean-sn
Copy link
Contributor

@sean-sn sean-sn commented Nov 26, 2020

Issue Addressed

Move to latest official version of blst (v0.3.1). Incorporate all the subgroup check API changes.

Proposed Changes

Update Cargo.toml to use official blst crate 0.3.1
Modifications to blst.rs wrapper for subgroup check API changes

Additional Info

The overall subgroup check methodology is public keys should be check for validity using key_validate() at time of first seeing them. This will check for infinity and in group. Those keys can then be cached for future usage. All calls into blst set the pk_validate boolean to false to indicate there is no need for on the fly checking of public keys in the library. Additionally the public keys are supposed to be validated for proof of possession outside of blst.

For signatures the subgroup check can be done at time of deserialization, prior to being used in aggregation or verification, or in the blst aggregation or verification functions themselves. In the interface wrapper the call to subgroup_check has been left for one instance, although that could be moved into the
verify_multiple_aggregate_signatures() call if wanted. Checking beforehand does save some compute resources in the scenario a bad signature is received. Elsewhere the subgroup check is being done inside the higher level operations. See comments in the code.

All checks on signature are done for subgroup only. There are no checks for infinity. The rationale is an aggregate signature could technically equal infinity. If any individual signature was infinity (invalid) then it would fail at time of verification. A loss of compute resources, although safety would be preserved.

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I just have one query, and then we should be ready to merge

crypto/bls/src/impls/blst.rs Show resolved Hide resolved
Copy link
Member

@kirk-baird kirk-baird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!!

Happy with all these changes :)

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Kirk having cleared up my confusion I'm happy to merge now.

Although I would also like a blst 0.3.2 with this PR included supranational/blst#45 ;)

@paulhauner
Copy link
Member

I understand we're not going to merge this until after v1.0.1 is released. As such, I'll add blocked.

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you @sean-sn!

bors r+

bors bot pushed a commit that referenced this pull request Nov 28, 2020
## Issue Addressed

Move to latest official version of blst (v0.3.1).  Incorporate all the subgroup check API changes.

## Proposed Changes

Update Cargo.toml to use official blst crate 0.3.1
Modifications to blst.rs wrapper for subgroup check API changes

## Additional Info

The overall subgroup check methodology is public keys should be check for validity using key_validate() at time of first seeing them.  This will check for infinity and in group.  Those keys can then be cached for future usage.  All calls into blst set the pk_validate boolean to false to indicate there is no need for on the fly checking of public keys in the library.  Additionally the public keys are supposed to be validated for proof of possession outside of blst.

For signatures the subgroup check can be done at time of deserialization, prior to being used in aggregation or verification, or in the blst aggregation or verification functions themselves.  In the interface wrapper the call to subgroup_check has been left for one instance, although that could be moved into the 
verify_multiple_aggregate_signatures() call if wanted.  Checking beforehand does save some compute resources in the scenario a bad signature is received.  Elsewhere the subgroup check is being done inside the higher level operations.  See comments in the code.

All checks on signature are done for subgroup only.  There are no checks for infinity.  The rationale is an aggregate signature could technically equal infinity.  If any individual signature was infinity (invalid) then it would fail at time of verification.  A loss of compute resources, although safety would be preserved.
@bors bors bot changed the title Update blst to official crate and incorporate subgroup changes [Merged by Bors] - Update blst to official crate and incorporate subgroup changes Nov 28, 2020
@bors bors bot closed this Nov 28, 2020
@michaelsproul michaelsproul added the consensus An issue/PR that touches consensus code, such as state_processing or block verification. label Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0 consensus An issue/PR that touches consensus code, such as state_processing or block verification. crypto An issue/PR that touches cryptography code. ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants