Skip to content

Add hooks for BLS12-381 #213

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

Conversation

virgil-serbanuta
Copy link
Contributor

@virgil-serbanuta virgil-serbanuta commented Apr 17, 2025

Here is the (Pi2) EVM PR that uses this:
Pi-Squared-Inc/evm-semantics#42

@virgil-serbanuta virgil-serbanuta marked this pull request as ready for review April 17, 2025 23:06
@F-WRunTime F-WRunTime requested a review from anvacaru April 18, 2025 14:40
@ehildenb
Copy link
Member

These should have test vectors added like the following: https://github.com/runtimeverification/blockchain-k-plugin/blob/master/krypto/src/tests/integration/test_ecdsa.py (for each function that is added as a hook). This is to ensure that each of these hooks is wired up properly and producing the outputs expected when calling into the external library. Ideally the test-vectors are sourced from somewhere else (so we know they're correct), but even auto-filling them is OK too.

@virgil-serbanuta
Copy link
Contributor Author

These should have test vectors added like the following: https://github.com/runtimeverification/blockchain-k-plugin/blob/master/krypto/src/tests/integration/test_ecdsa.py (for each function that is added as a hook). This is to ensure that each of these hooks is wired up properly and producing the outputs expected when calling into the external library. Ideally the test-vectors are sourced from somewhere else (so we know they're correct), but even auto-filling them is OK too.

Added tests. Most of them have comments pointing to their origin. For some of them I didn't find existing tests.

(BLS12_G2_INFTY, 2**256 - 1, BLS12_G2_INFTY, 'max_scalar_times_inf'),
(BLS12_G2, 0, BLS12_G2_INFTY, 'zero_times_generator'),
(BLS12_G2, 1, BLS12_G2, 'one_times_generator'),
# (BLS12_G2, BLS12_Q, BLS12_G2_INFTY, 'q_times_generator'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this left out on purpose?

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, I remove it.

Copy link
Contributor

@anvacaru anvacaru 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 to me! But I'd like for @ehildenb to have another look before merging.

@ehildenb
Copy link
Member

LGTM as well, thanks for adding tests. I checked that they're run on CI.

@automergerpr-permission-manager automergerpr-permission-manager bot merged commit 5341429 into runtimeverification:master Apr 30, 2025
4 checks passed
@virgil-serbanuta
Copy link
Contributor Author

Thanks for the review!

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.

3 participants