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

Added Milagro crypto-library with BLS12-381 implementation. #1236

Closed
wants to merge 12 commits into from

Conversation

zilm13
Copy link
Collaborator

@zilm13 zilm13 commented Nov 26, 2018

...First test of message signing added.
Not ready for merge yet, a lot of work to go.

@coveralls
Copy link

coveralls commented Nov 26, 2018

Coverage Status

Coverage increased (+0.2%) to 55.456% when pulling 43d4497 on sharding/bls381milagro into 4fb98c4 on research/sharding.

@mkalinin mkalinin added this to the Beacon milestone Nov 27, 2018
@mkalinin mkalinin added this to On Review in Beacon chain via automation Nov 27, 2018
@zilm13 zilm13 requested a review from mkalinin December 2, 2018 18:04
@zilm13
Copy link
Collaborator Author

zilm13 commented Dec 2, 2018

@mkalinin ready for review

@zilm13
Copy link
Collaborator Author

zilm13 commented Dec 2, 2018

resolves #1237

Copy link
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

Requesting more javadocs to the newly added classes

*/
public interface BLS381 {

BI generatePrivate();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think naming is a bit incomplete cause it doesn't point that generation has PRNG kind.


BI generatePrivate();

BI restorePrivate(BigInteger value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would abstract from any particular big number implementation. Wouldn't byte[] representation be sufficient enough?


BI restorePrivate(BigInteger value);

BI restorePrivate(byte[] value);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this method addresses more general case than restoring just private keys. Maybe it's better to call it restoreScalar?


FP12Point pair(ECP2Point pointECP2, ECP1Point pointECP1);

interface BI {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call it Scalar

FP12Point pair(ECP2Point pointECP2, ECP1Point pointECP1);

interface BI {
BigInteger asBigInteger();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed in case if we're not sticking with any specific big number implementations

byte[] asByteArray();
}

interface ECP1Point {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ECP1 or even P1 would be pretty enough here

byte[] asByteArray();
}

interface FP12Point {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a point, actually. Let's call it FP12. And I would, also, explicitly add equals method to that interface to point it out as an expecting one from underlying implementations

@@ -36,24 +44,30 @@
BigInteger aggPubs(List<BigInteger> pubKeys);

class Signature {
public BigInteger r;
public BigInteger s;
public BigInteger value;
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look to me that having Signature class is something useful. Maybe turn it into byte[]?

@@ -36,24 +44,30 @@
BigInteger aggPubs(List<BigInteger> pubKeys);
Copy link
Contributor

Choose a reason for hiding this comment

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

This method looks redundant. pubKeys may be passed to verify directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Beacon chain
  
On Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants