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

Change BLS Pairing Engine #3670

Merged
merged 23 commits into from
Oct 2, 2019
Merged

Change BLS Pairing Engine #3670

merged 23 commits into from
Oct 2, 2019

Conversation

kilic
Copy link
Contributor

@kilic kilic commented Sep 30, 2019

This PR changes current BLS pairing engine to the new engine and speeds up signature verification process by 3x.

While BLS signature and keys API remains as is, only underlying math library is changed.

For now, ‘try and increment’ method for hash to G2 is considered. Standart hash method will be added soon after BLS standardization is finalized.

x86 assembly code is generated using github.com/kilic/fp libary

This new pairing engine library processes single pairing in ~2.4ms. Such performance is comparable to rust implementation.

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Please revert everything under shared/bls/spectest. Why remove those sources? Why move the generated yaml files into the tests? We may need to regenerate those in the future if the inputs change.

shared/bls/spectest/BUILD.bazel Outdated Show resolved Hide resolved
shared/bls/bls.go Outdated Show resolved Hide resolved
shared/bls/bls.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #3670 into master will decrease coverage by 17.11%.
The diff coverage is 76.52%.

@@             Coverage Diff             @@
##           master    #3670       +/-   ##
===========================================
- Coverage   53.04%   35.92%   -17.12%     
===========================================
  Files         172        5      -167     
  Lines       11197      309    -10888     
===========================================
- Hits         5939      111     -5828     
+ Misses       4407      187     -4220     
+ Partials      851       11      -840

rauljordan
rauljordan previously approved these changes Oct 1, 2019
}

// PublicKeyFromBytes creates a BLS public key from a LittleEndian byte slice.
func PublicKeyFromBytes(pub []byte) (*PublicKey, error) {
if featureconfig.FeatureConfig().SkipBLSVerify {
return &PublicKey{}, nil
}
cv := pubkeyCache.Get(string(pub))
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this? This method was called in high frequency with infrequently changing parameters. Aligning the bytes to the curve was expensive.

shared/bls/bls.go Show resolved Hide resolved
}

// Copy the public key to a new pointer reference.
// Copy copies the public key and returns it.
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this comment?

Suggested change
// Copy copies the public key and returns it.
// Copy the public key to a new pointer reference.

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

LGTM.

Such an incredible performance improvement. This will break ARM builds, but we can tackle that later. Thanks for your contribution!

@nisdas nisdas merged commit d5e02ea into prysmaticlabs:master Oct 2, 2019
@prestonvanloon prestonvanloon mentioned this pull request Oct 3, 2019
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.

5 participants