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.AggregatePKs is undefined in Altair spec #2369

Closed
ralexstokes opened this issue May 1, 2021 · 3 comments · Fixed by #2438
Closed

bls.AggregatePKs is undefined in Altair spec #2369

ralexstokes opened this issue May 1, 2021 · 3 comments · Fixed by #2438

Comments

@ralexstokes
Copy link
Member

ralexstokes commented May 1, 2021

The Altair spec currently uses a function bls.AggregatePKs to aggregate a batch of BLS pubkeys in the function get_sync_committee.

This function is defined internal to the pyspec code but is not specified in the IETF signature standard which, according to the spec, is our source of bls functionality.

I'd say it is worth considering defining a helper like eth2_fast_aggregate_verify for this routine for the completeness of the spec.

edit: implementation incorporating feedback below:

def eth2_aggregate_pubkeys(pubkeys: Sequence[BLSPubkey]) -> BLSPubkey:
    """
    Return the aggregate public key for the public keys in ``pubkeys``.

    NOTE: the `+` operation should be interpreted as elliptic curve point addition, which takes as input
    elliptic curve points that must be decoded from the input `BLSPubkey`s. 
    This implementation is for demonstrative purposes only and ignores encoding/decoding concerns. 
    Refer to the BLS signature draft standard for more information.
    """
    assert len(pubkeys) > 0
    result = copy(pubkeys[0])
    for pubkey in pubkeys[1:]:
        result += pubkey
    return result
@protolambda
Copy link
Collaborator

If we do so, can we be more explicit about pubkey copies here?

result = pubkeys[0] and result += looks like in-place modification of the pubkey, but it shouldn't look like it changes the inputs (even though it doesn't). We could change it to start at 0, or copy() it.

@hwwhww
Copy link
Contributor

hwwhww commented May 4, 2021

It makes sense to me. Maybe add a note/comment that the pseudo-code ignores the encoding?

Backlog: we should add eth2_fast_aggregate_verify and eth2_aggregate_pubkeys tests to the BLS test vectors.

@ralexstokes
Copy link
Member Author

could change it to start at 0
the pseudo-code ignores the encoding

yeah, i would want to avoid making 0x00 * 48 as the initial value to avoid discrepancies w/ encodings...

copy seems like a nice way forward and i think adding a note about ignoring encoding issues is a good idea

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

Successfully merging a pull request may close this issue.

3 participants