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

BL12-381 Integration #1310

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

PlamenHristov
Copy link

@PlamenHristov PlamenHristov commented Dec 27, 2023

What

Added:

  • BLS12_G1ADD
  • BLS12_G1MUL
  • BLS12_G1MULTIEXP
  • BLS12_G2ADD
  • BLS12_G2MUL
  • BLS12_G2MULTIEXP

Missing:

  • BLS12_PAIRING
  • BLS12_MAP_FP_TO_G1
  • BLS12_MAP_FP2_TO_G2
  • BLS12_HASH_TO_G1
  • BLS12_HASH_TO_G2

Why

Because of issue #779

Known limitations

  • Missing tests
  • Panics if points are invalid
  • Implementation is likely to be imperfect

@jayz22
Copy link
Contributor

jayz22 commented Jan 2, 2024

Some general comments. I'm not too familiar with the implementation details yet.

None of these functions can use existing metering cost types, for each of these functions {p1, p2}_{add, mul, serialize}, a new cost types need to be added, calibrated and charged.
(I also remember @graydon mentioned something we need to be careful about when it comes to FFI, but I can't recall exactly)
These functions need to be exposed via the env, via env.json entry and a corresponding implementation in host.rs.
It would be nice to have some tests, not comprehensive coverage but for sanity checks, to make sure they are doing what's expected and there are no obvious issues.

@dmkozh
Copy link
Contributor

dmkozh commented Jan 2, 2024

Besides the points that @jayz22 has mentioned, there are important high-level issues:

  • Modifying the env is a protocol change and I'm not sure we have a proper mechanism for protocol-gating the env functions yet (to be more specific, we could bump env common to protocol 21, but I'm not sure that's the way to go, given that we might still want to do non-breaking protocol 20 changes)
  • Panics are not acceptable

I suppose this PR could be used as a starting point for the future work, but we can't really merge it until these issues have been resolved.

@PlamenHristov
Copy link
Author

Ok I think I know what needs to be done to the PR to make it review ready. Will convert to draft in the meantime

@PlamenHristov PlamenHristov marked this pull request as draft January 4, 2024 13:39
@PlamenHristov PlamenHristov force-pushed the bls12-318-field-arithmetic branch 2 times, most recently from 9a6a7b3 to 4560f55 Compare April 10, 2024 16:14
@PlamenHristov PlamenHristov marked this pull request as ready for review April 10, 2024 16:14
@PlamenHristov PlamenHristov requested a review from a team as a code owner April 10, 2024 16:14
@PlamenHristov
Copy link
Author

  1. Implemented the missing:
  • BLS12_PAIRING
  • BLS12_MAP_FP_TO_G1
  • BLS12_MAP_FP2_TO_G2
  • BLS12_HASH_TO_G1
  • BLS12_HASH_TO_G2
  1. Added tests

Ready for review in terms of the implementation.

Still requires adding budgeting of individual operations and charging them in stellar-xdr

@PlamenHristov PlamenHristov changed the title Added add, mul and mulexp BL12-381 Integration Apr 13, 2024
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.

None yet

3 participants