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

Update to IETF BLS draft-irtf-cfrg-bls-signature-02 + draft-irtf-cfrg-hash-to-curve-07 #1799

Merged
merged 19 commits into from
May 18, 2020

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 9, 2020

As #1798 pointed out, adding a draft for v0.12.0.


May 12th Update: it's now following IETF BLS draft-irtf-cfrg-bls-signature-02 (including draft-irtf-cfrg-hash-to-curve-06)

If we want to include draft-irtf-cfrg-hash-to-curve-07 now, the spec would be changed to like:

Eth2 makes use of BLS signatures as specified in the IETF draft BLS specification v02 but uses Hashing to Elliptic Curves v07 instead of v06.

IMHO it's kinda strange and it would be nice to have a new BLS v03 release that includes H2C v7 soon.


May 13th update: it's now following IETF BLS draft-irtf-cfrg-bls-signature-02 +draft-irtf-cfrg-hash-to-curve-07.

@hwwhww hwwhww marked this pull request as ready for review May 12, 2020 04:29
@hwwhww hwwhww added this to the v0.12.0 milestone May 12, 2020
@hwwhww hwwhww changed the title Update to IETF BLS draft-irtf-cfrg-bls-signature-02 Update to IETF BLS draft-irtf-cfrg-bls-signature-02 + draft-irtf-cfrg-hash-to-curve-07 May 12, 2020
@hwwhww
Copy link
Contributor Author

hwwhww commented May 12, 2020

/cc @prestonvanloon @mratsim @benjaminion @kirk-baird

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

couple minor comments

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

LGTM.

Small comment on the zero signature in phase 1.

Another thing is that ideally I'd like the verify functions to include the proof-of-possession verification or mention in the BLS paragraph that it assumed that PoP were verified at stage X.

if len(proposer_signatures) > 0:
proposer_signature_aggregate = bls.Aggregate(proposer_signatures)
else:
proposer_signature_aggregate = BLSSignature()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we define the empty signature somewhere? Is that 0xc000...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The python here would mean all zeroes (every ssz type is all zeroes by default). But agree that this is way to vague and can be confused with an empty signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember we had EMPTY_SIGNATURE constant in 2019. I would be happy to add it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can have, NO_SIGNATURE(0x0000...) and EMPTY_SIGNATURE (0xc000...) for clarity

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been treating EMPTY_SIGNATURE as (0x0000...) as I believe that's what we called in it the spec in 2019. I'm happy to do a re-name and make

  • NO_SIGNATURE (0x0000...) (always verifies false)
  • EMPTY_SIGNATURE (0xc000...)

However, we have to be careful here as AggregatePublicKeys([]) will always verify false according to the BLS Standard.

So EMPTY_SIGNATURE will only verify true against a list of EMPTY_PUBLIC_KEYS. We must have 1..* EMPTY_PUBLIC_KEYS verifying true. (Note: or some other combination of public keys that add to 0 mod r).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or EMPTY_SIGNATURE(0x0000) and ZERO_SIGNATURE (0xc000) I'm not married to any name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey I'm redirecting this discussion back to the original #1713 thread: #1713 (comment) 😄

Copy link
Contributor

@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.

Phase0 LGTM. Thanks!

@hwwhww
Copy link
Contributor Author

hwwhww commented May 14, 2020

@mratsim

Another thing is that ideally I'd like the verify functions to include the proof-of-possession verification or mention in the BLS paragraph that it assumed that PoP were verified at stage X.

We do state that the ciphersuite ID is BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_, it includes "POP".

Or do you mean PopProve(SK) and PopVerify(PK, proof) APIs? From my understanding, we don't use PopProve for the deposit proof-of-possession since we have to sign over withdrawal_credentials.

hwwhww and others added 6 commits May 15, 2020 00:48
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
1. Make sure that BLS -Verify APIs would only return `True` or `False` , no exceptions.
2. Use `eth2spec.utils.bls` instead of py_ecc for test generator
3. Add assertions in test generator
4. Add some special test cases for the -Verify APIs
5. Clean up the test format documents
Handle phase 1 `PKs == []` cases
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Minor config thing. Looks good otherwise.

configs/mainnet.yaml Outdated Show resolved Hide resolved
configs/minimal.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Just ran the test generators locally. All good

great job! and thank you everyone for all the input along the way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants