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

Adding Verification Function for Aggregate BLS Signatures #240

Merged
merged 14 commits into from
Feb 28, 2020
Merged

Conversation

RajarupanSampanthan
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Added a basic verification function for BLS aggregate signatures
  • Added a simple test

Issues with pull Request :

  • This function can be made more convenient to use if it takes in the CIDs as a argument instead of the actual data. I was advised by Austin to do this, but I wasn't sure exactly how to do so.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Make sure to run make lint to make sure code is formatted and linted. Looks like you're on the right track though! I'll have a more in depth check once CI passes

crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
@claassistantio
Copy link

claassistantio commented Feb 24, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

few small changes

crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
crypto/src/signature.rs Show resolved Hide resolved
crypto/src/signature.rs Outdated Show resolved Hide resolved
Comment on lines +264 to +266
let signatures: Vec<BlsSignature> = (0..num_sigs)
.map(|x| private_keys[x].sign(data[x]))
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

can do same as above

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

What are others thoughts on the api for the function? From what I've seen all data that is signed in Cids so maybe the data input should be a slice of Cids to avoid having to convert to bytes before calling this? Also does anyone have thoughts on if we should be using the external Public key type also instead of bytes or we alias/ create our own type for pub keys?

I'm fine with this coming in as in but would be nice to iron down the function signature/ a specific test vector when it gets used

@austinabell austinabell merged commit a167203 into master Feb 28, 2020
@austinabell austinabell deleted the rupan branch February 28, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants