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

blst lagrange interpolation #95

Open
wants to merge 26 commits into
base: unstable
Choose a base branch
from

Conversation

dknopik
Copy link
Member

@dknopik dknopik commented Jan 13, 2025

Proof of concept of using blst for signature recovery.

We should discuss whether we want to use this or blsful.

Arguments for this impl:

  • We avoid a bunch of new dependencies (which is nice, as we are working with keys here)
  • We do not have to add too much ugly switching of types this way (as blsful uses a whole stack of own types. Lighthouse instead uses their own bls crate that builds upon blst)
  • I daresay it's faster, but I have not tested that 😬

Arguments for blsful:

  • No unsafe needed
  • This impl needs some (hopefully harmless) changes upstreamed to blst
  • DIY cryptography is scary

@Zacholme7
Copy link
Member

Zacholme7 commented Jan 14, 2025

do you have a poc using blsful? Id almost say not having to use unsafe and roll some diy cryptography is worth more messy types but there are valid arguments for both libs.

@dknopik
Copy link
Member Author

dknopik commented Jan 14, 2025

@Zacholme7 not yet, will do

@dknopik
Copy link
Member Author

dknopik commented Jan 15, 2025

Added an impl for blsful, or to be more precise, using the underlying libs of blsful. Here are some benches for signature combination (from my machine, profile maxperf):

blst:

1000 iterations each
took 212 ms for threshold = 3
took 222 ms for threshold = 5
took 272 ms for threshold = 7
took 301 ms for threshold = 9

(note that this uses some blst internal thread pool and might not be what we want to use in order to keep control over threading in anchor)

blst_single_thread:

1000 iterations each
1000 iterations each
took 406 ms for threshold = 3
took 572 ms for threshold = 5
took 737 ms for threshold = 7
took 904 ms for threshold = 9

blsful:

1000 iterations each
took 569 ms for threshold = 3
took 955 ms for threshold = 5
took 1333 ms for threshold = 7
took 1726 ms for threshold = 9

The blsful variant contains no unsafe.
Will edit this comment if benches change.

@Zacholme7
Copy link
Member

Zacholme7 commented Jan 16, 2025

I think the blsful code looks really nice, but almost 2 seconds is scary. If we can be confident in the blst code that may be the way to go

I should have read the benchmarking code closer... looks good!!

@dknopik
Copy link
Member Author

dknopik commented Jan 16, 2025

Almost 2 seconds for 1000 iterations, so about 2ms for one. That's still a full second of CPU time if all 500 validators want to sign something, but a) that should be rare, and b) we have multiple cores available.

So I think both variants are tolerable...

...but yeah, that almost 2x speedup is still tempting :P

@dknopik
Copy link
Member Author

dknopik commented Jan 21, 2025

I've opened supranational/blst#248 to upstream the necessary changes to blst. sigp/lighthouse#6829 is also required, but we can put this into the anchor branch first as usual.

Until then, I propose merging this. I've set the safe-but-slower implementation as the default and added a big warning to the unsafe implementation. As I made sure both offer the same API, we can start working with this and postpone the final decision on what to use until after an internal review.

This introduces two patched dependencies to the root Cargo.toml, but we can remove those as soon as the required changes are upstream.

edit: need to update the anchor branch first: sigp/lighthouse#6830

@dknopik dknopik marked this pull request as ready for review January 21, 2025 15:32
@dknopik dknopik marked this pull request as draft January 21, 2025 15:58
@dknopik dknopik marked this pull request as ready for review January 23, 2025 07:15
@dknopik
Copy link
Member Author

dknopik commented Jan 23, 2025

scratching my head at the CI failure. why does eth2_network_config fail here but not in e.g. #109?

@Zacholme7
Copy link
Member

scratching my head at the CI failure. why does eth2_network_config fail here but not in e.g. #109?

idk the root reason, but cargo update has fixed this in the past for me if you haven't done that yet.

@dknopik dknopik added ready-for-review This PR is ready to be reviewed cryptography spicy labels Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cryptography ready-for-review This PR is ready to be reviewed spicy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants