Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Switch to using mathlib #3574

Merged
merged 1 commit into from
May 18, 2023

Conversation

ale-linux
Copy link
Contributor

@ale-linux ale-linux commented Apr 28, 2023

Title:
Use mathlib as the implementation for elliptic curve operations.

Description:
This PR is an implementation of this proposal.

Summary:

This PR changes the dependency used to handle the operations on elliptic curves needed by the BBS+ implementation. Instead of directly using the kilic implementation. we recommend switching to mathlib: mathlib is a module that exposes a common set of API backed by a number of different libraries (amcl, ConsenSys/gnark-crypto and kilic). It currently supports the following curves: FP256BN, BN254, BLS12_377 and BLS12_381 (the latter in two different variants, standard and BBS compliant). mathlib is already being used by fabric, the idemix implementation used by fabric, the fabric token sdk and the fabric smart client.

@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #3574 (e4cd201) into main (20c4d4b) will increase coverage by 0.20%.
The diff coverage is 99.20%.

❗ Current head e4cd201 differs from pull request most recent head c6026db. Consider uploading reports for the commit c6026db to get more accurate results

@@            Coverage Diff             @@
##             main    #3574      +/-   ##
==========================================
+ Coverage   87.02%   87.23%   +0.20%     
==========================================
  Files         353      353              
  Lines       48666    48667       +1     
==========================================
+ Hits        42354    42457     +103     
+ Misses       4769     4672      -97     
+ Partials     1543     1538       -5     
Impacted Files Coverage Δ
...rypto/primitive/bbs12381g2pub/signature_message.go 100.00% <ø> (ø)
.../crypto/primitive/bbs12381g2pub/signature_proof.go 84.04% <97.43%> (ø)
...to/crypto/primitive/bbs12381g2pub/bbs12381g2pub.go 86.95% <100.00%> (-0.15%) ⬇️
...ent/kmscrypto/crypto/primitive/bbs12381g2pub/fr.go 100.00% <100.00%> (ø)
...t/kmscrypto/crypto/primitive/bbs12381g2pub/keys.go 91.66% <100.00%> (+6.06%) ⬆️
...ypto/primitive/bbs12381g2pub/proof_of_knowledge.go 97.98% <100.00%> (-0.10%) ⬇️
...crypto/crypto/primitive/bbs12381g2pub/signature.go 100.00% <100.00%> (ø)

... and 80 files with indirect coverage changes

@ale-linux
Copy link
Contributor Author

@sudeshrshetty : as discussed last week, here's the PR and here's the associated proposal.

@ale-linux ale-linux force-pushed the usemathlib branch 4 times, most recently from 0ac2a13 to 2e29fdb Compare April 28, 2023 16:20
@adecaro
Copy link

adecaro commented May 4, 2023

Hi @sudeshrshetty , we would really appreciate your feedback on the PR. We think it can bring good value to the project overall. Please, let us know :)

@sudeshrshetty sudeshrshetty requested review from DRK3, Moopli and troyronda May 4, 2023 21:23
@sudeshrshetty
Copy link
Contributor

@DRK3 @fqutishat @Moopli please review this PR and provide your feedback. Let us know if it is going to impact anything we have implemented in TrustBloc.

Copy link
Contributor

@DRK3 DRK3 left a comment

Choose a reason for hiding this comment

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

Looks good to me, I would suggest having @Moopli take a look before merging :)

@sudeshrshetty
Copy link
Contributor

@ale-linux ale-linux force-pushed the usemathlib branch 3 times, most recently from 2466526 to d0a0a80 Compare May 9, 2023 11:13
@ale-linux
Copy link
Contributor Author

ale-linux commented May 9, 2023

Done - all tests run successfully on my node - unclear why they fail here.. maybe a flaky test?

@ale-linux
Copy link
Contributor Author

Any news?

@DRK3
Copy link
Contributor

DRK3 commented May 12, 2023

@sudeshrshetty I think @ale-linux is right, it's a flaky test issue. @Moopli is probably a bit more familiar with the tests, do you recognize those failures? If it's an ongoing flaky test issue, I think this PR is good to merge :)

@ale-linux
Copy link
Contributor Author

@sudeshrshetty: I'm happy to discuss any further open points/necessary changes, but if you think it is ready, would you mind merging? Otherwise I need to keep rebasing/fixing conflicts with upstream...

@ale-linux
Copy link
Contributor Author

...and the flaky test also passed, I think it's a sign that it's time to merge 😉

@ale-linux
Copy link
Contributor Author

another rebase and now the flakes strike again... 😢

@sudeshrshetty
Copy link
Contributor

Sorry, I am re-triggering these failed tests to make succeed CI, so that I can merge it.

@ale-linux
Copy link
Contributor Author

Thanks much, @sudeshrshetty

This PR switches the dependency used for elliptic curve operations from kilic
to mathlib.

Signed-off-by: Alessandro Sorniotti <[email protected]>
@ale-linux
Copy link
Contributor Author

@sudeshrshetty : All tests have passed now - thx!

@ale-linux
Copy link
Contributor Author

Could we please merge now gents? @sudeshrshetty @fqutishat

@sudeshrshetty sudeshrshetty merged commit c98960d into hyperledger-archives:main May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants