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

BLS Specs rewrite for IETF clarity #1499

Closed
wants to merge 9 commits into from
Closed

BLS Specs rewrite for IETF clarity #1499

wants to merge 9 commits into from

Conversation

CarlBeek
Copy link
Contributor

@CarlBeek CarlBeek commented Dec 2, 2019

This PR is an alternate proposal to @kirk-baird's #1398.

It is based on two ideas:

  1. If something is defined in the IETF specs, it should not be redefined in the eth2-specs. This reduces the scope for errors and reflects the workflow of integrating with an IETF spec-compliant BLS implementation.

  2. tag replaces domain. Confusion can arise due to the name collision of domain in the eth2 specifications and those of the IETF. tag is an alternative that implies also separation and is unique to the eth2-specs.

PS: The diff appears large here, but most of that is due to tag replacing domain. The substantive changes are (almost entirely) limited to specs/bls_signature.md

@CarlBeek CarlBeek added general:presentation Presentation (as opposed to content) scope:BLS labels Dec 2, 2019
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.

Awesome! I think this is the way to go.

I made some suggestions, nothing major. then ready to merge


### `bls_aggregate_pubkeys`

Let `bls_aggregate_pubkeys(pubkeys: List[Bytes48]) -> Bytes48` return `pubkeys[0] + .... + pubkeys[len(pubkeys)-1]`, where `+` is the elliptic curve addition operation over the G1 curve. (When `len(pubkeys) == 0` the empty sum is the G1 point at infinity.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no public interface in the BLS standard to aggregate pubkeys?
I can't immediately think of a need to aggregate it independently and have the resulting aggregate pubkey, but interesting none the less.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No there isn't. It is a strange absence.

@djrtwo
Copy link
Contributor

djrtwo commented Dec 9, 2019

Oh shit, I just noticed the merge conflict here. I'll have a stab at it

@djrtwo
Copy link
Contributor

djrtwo commented Dec 9, 2019

Merge conflicts resolved

@kirk-baird
Copy link
Contributor

I like the way you've presented it here and I agree that it's much better to not redefine things that are already defined in the IETFs. So I too think this is the way forward, I'm happy to close #1398 when this is merged :)

@JustinDrake
Copy link
Contributor

A few issues (discussed with Carl privately):

  1. We need support for uniqueness roots to unify the slashing conditions. See Unified slashing condition for equivocations #1387
  2. Application-layer logic specific to Eth2 (namely, the tag as well as the uniqueness root) should not go in the BLS spec. Instead, it should go in the "core" Eth2 spec.
  3. We should sign over 32-byte messages (hash outputs) as apposed to appending the tag with + tag. The appendage is adhoc and not future proof (for example, does work well with uniqueness roots). The appendage is also ugly 😂

@djrtwo
Copy link
Contributor

djrtwo commented Dec 16, 2019

Are you suggesting moving the methods like bls_sign out of the bls spec?
Even if using a wrapper object, I think it makes sense to keep these functions in a separate file (like the bls spec) rather than cramming them in the phase 0 doc.

I think it natural to present the functions from the BLS standards, immediately followed by the set of wrapper functions used explicitly throughout the rest of the spec.

This allows for an auditor to quickly see the BLS standards and how we use them all in one place without having to dig through other docs

@JustinDrake
Copy link
Contributor

JustinDrake commented Dec 16, 2019

Are you suggesting moving the methods like bls_sign out of the bls spec?

Yes, that was the suggestion in terms of presentation :) Notice that without that separation of concerns there would be a somewhat circular definition. For example, as the PR stands, the Eth2 spec uses bls_verify defined in the BLS spec, but bls_verify itself uses hash which is defined in the Eth2 spec. The situation becomes worse with the other application-level details involved in wrapping (namely Container, hash_tree_root, tag). Arguably the BLS spec does not need to import the SSZ spec to know what a Container is.

@JustinDrake
Copy link
Contributor

@CarlBeek: I think we're missing bls_aggregate_pubkeys.

@djrtwo
Copy link
Contributor

djrtwo commented Dec 19, 2019

closing in favor of #1532

@djrtwo djrtwo closed this Dec 19, 2019
@CarlBeek CarlBeek deleted the carl_ietf_bls branch January 22, 2020 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content) scope:BLS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants