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

Refactor BLS module #5

Closed
samlaf opened this issue Aug 2, 2023 · 2 comments · Fixed by #151
Closed

Refactor BLS module #5

samlaf opened this issue Aug 2, 2023 · 2 comments · Fixed by #151
Assignees

Comments

@samlaf
Copy link
Collaborator

samlaf commented Aug 2, 2023

We originally took much of the crypto/bls module from eigenDA, so it currently contains code for both bls and some incredible-squaring specific code.

We need to refactor it and take out all the incredible-squaring code and put that in its own repo.

@samlaf samlaf self-assigned this Aug 2, 2023
@shrimalmadhur shrimalmadhur transferred this issue from another repository Sep 29, 2023
@samlaf
Copy link
Collaborator Author

samlaf commented Oct 15, 2023

I really don't like that our bn254 and bls methods all take pointer receivers and arguments, eg:

func (p *G2Point) Add(p2 *G2Point)

I've been bitten a few times by side effects while writing the bls aggregation service. I think we just followed the gnark-crypto library design. I raised an issue in their repo asking for the rationale behind this design choice. In the meantime, I think we might want to switch to non pointer receivers and args. A few stack copies is not worth optimizing for, especially if it can save a bunch of human debugging time.

EDIT: I'm stupid, you're smart. I was wrong, you are right. Actually I now don't like our interface for a different set of reasons, listed in that issue. Feels like we should just use the underlying bn254 methods directly (that take receiver and 2 pointer arguments)!

@samlaf
Copy link
Collaborator Author

samlaf commented Dec 12, 2023

gnark-crypto has a PR open to add bls sigs over a bunch of curves. We should consider using their implementation once they merge it. It's been open and stale for quite a while though...

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 a pull request may close this issue.

1 participant