Skip to content

Update BLS signature module #1581

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

Merged
merged 6 commits into from
Dec 18, 2018
Merged

Update BLS signature module #1581

merged 6 commits into from
Dec 18, 2018

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Dec 12, 2018

What was wrong?

#1565

How was it fixed?

  1. Follow the logic from the BLS signature verification
  2. Replace BN128 with BLS-12-381 curve.
  3. Replace the current blake hashing function with keccak-256.

Note

  • Please ignore the eth/beacon/aggregation.py for now. It's supposed to be updated in other PRs.
  • This PR is opened for reviewing the reference implementation. The current CI error is due to other type hinting issues. @carver, feel free to solve that and merge Upgrade py-ecc past a buggy release, to 1.4.6 #1576 before it, I'll resolve the conflicts. :)

Cute Animal Picture

saiga_antelope

@pipermerriam
Copy link
Member

@hwwhww can you look into fixing these linting errors that show up when using the latest py_ecc as well?

#1576 -> https://circleci.com/gh/ethereum/py-evm/109848?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 12, 2018

@pipermerriam yes, I'll investigate it.

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.

looks good! left some comments.

@carver
Copy link
Contributor

carver commented Dec 13, 2018

Done. Thanks for doing the type cleanups! :)

o = FQ12([1] + [0] * 11)
for m_pubs in set(messages):
# aggregate the pubs
group_pub = Z1
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 I was a bit unclear in the last comment. I meant something like the following which utilizes aggregate_pubs rather than directly calling add

for message in set(messages):
    pubkeys_for_message = [
        pubkey for i, pubkey in enumerate(pubkeys)
        if messages[i] == message
    ]
    group_pub = aggregate_pubkeys(pubkeys_for_message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see! I think I've tried to do that before, if we want to utilize aggregate_pubkeys, we will need:

  1. len(messages) times extra compress_G1() calling (inside aggregate_pubkeys)
  2. len(messages) times extra decompress_G1() calling for the second argument of pairing.

Alternatively, we might refactor aggregate_signatures(signatures: Sequence[bytes]) -> Tuple[int, int] to:

def aggregate_pubkeys(pubkeys: Sequence[int]) -> int:
    return compress_G1(_aggregate_pubkeys(pubkeys))

def _aggregate_pubkeys(pubkeys: Sequence[int]) -> Tuple[FQ, FQ, FQ]:
    o = Z1
    for p in pubkeys:
        o = add(o, decompress_G1(p))
    return o

And then make verify_multiple call _aggregate_pubkeys().

What do you think of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh crap, you're right.

I'm okay either way. It's not a huge gain in code reuse and this code won't change much once in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do whichever you want. I'm going to use the updated work in #1631 in the morning so merge whenever you're ready.

@hwwhww hwwhww merged commit 4bd796d into ethereum:master Dec 18, 2018
@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 18, 2018

@djrtwo merging it now! We can optimize the BLS APIs while moving it to py-ecc later (#1592). :)

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

Successfully merging this pull request may close these issues.

4 participants