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

[Merged by Bors] - Update blst to 0.3.11 #4624

Closed
wants to merge 2 commits into from

Conversation

michaelsproul
Copy link
Member

Proposed Changes

This PR updates blst to 0.3.11, which gives us runtime detection of CPU features 🎉

Although performance benchmarks don't show a substantial detriment to running the portable build vs modern, in order to take things slowly I propose the following roll-out strategy:

  • Keep both modern and portable builds for releases/Docker images.
  • Run the portable build on half of SigP's infrastructure to monitor for performance deficits.
  • Listen out for user issues with the portable builds (e.g. SIGILLs from misdetected hardware).
  • Make the portable build the default and remove the modern build from our release binaries & Docker images.

@michaelsproul michaelsproul added ready-for-review The code is ready for review crypto An issue/PR that touches cryptography code. v4.4.1 ETA August 2023 labels Aug 16, 2023
@pawanjay176
Copy link
Member

Should we also be bumping the version here https://github.com/michaelsproul/lighthouse/blob/blst-0.3.11/crypto/bls/Cargo.toml#L20

@michaelsproul
Copy link
Member Author

michaelsproul commented Aug 16, 2023

@pawanjay176 we haven't bothered to in the past because it's a semver compatible update. I guess we could if we want to ensure people using crypto/bls externally as a dependency get the updated version

@pawanjay176
Copy link
Member

Ohh wow did not know that we could do that. Thanks for the explanation!

Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM

@paulhauner
Copy link
Member

Nice! Will the modern version also do auto-detection? Or is it always going to take the spicy-opcodes path?

@michaelsproul
Copy link
Member Author

Nice! Will the modern version also do auto-detection? Or is it always going to take the spicy-opcodes path?

It'll take the spicy-opcodes path as it does today, the feature in blst is called force-adx:

https://github.com/supranational/blst/blob/3dd0f804b1819e5d03fb22ca2e6fac105932043a/bindings/rust/build.rs#L132-L139

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Let's do it!

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 16, 2023
@michaelsproul
Copy link
Member Author

Pushed a new commit to ignore the cargo audit failure until #4623 can be properly patched.

@michaelsproul
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Proposed Changes

This PR updates `blst` to 0.3.11, which gives us _runtime detection of CPU features_ 🎉

Although [performance benchmarks](https://gist.github.com/michaelsproul/f759fa28dfa4003962507db34b439d6c) don't show a substantial detriment to running the `portable` build vs `modern`, in order to take things slowly I propose the following roll-out strategy:

- Keep both `modern` and `portable` builds for releases/Docker images.
- Run the `portable` build on half of SigP's infrastructure to monitor for performance deficits.
- Listen out for user issues with the `portable` builds (e.g. SIGILLs from misdetected hardware).
- Make the `portable` build the default and remove the `modern` build from our release binaries & Docker images.
@bors
Copy link

bors bot commented Aug 17, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Proposed Changes

This PR updates `blst` to 0.3.11, which gives us _runtime detection of CPU features_ 🎉

Although [performance benchmarks](https://gist.github.com/michaelsproul/f759fa28dfa4003962507db34b439d6c) don't show a substantial detriment to running the `portable` build vs `modern`, in order to take things slowly I propose the following roll-out strategy:

- Keep both `modern` and `portable` builds for releases/Docker images.
- Run the `portable` build on half of SigP's infrastructure to monitor for performance deficits.
- Listen out for user issues with the `portable` builds (e.g. SIGILLs from misdetected hardware).
- Make the `portable` build the default and remove the `modern` build from our release binaries & Docker images.
@michaelsproul
Copy link
Member Author

bors r-

@bors
Copy link

bors bot commented Aug 17, 2023

Canceled.

@michaelsproul
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Proposed Changes

This PR updates `blst` to 0.3.11, which gives us _runtime detection of CPU features_ 🎉

Although [performance benchmarks](https://gist.github.com/michaelsproul/f759fa28dfa4003962507db34b439d6c) don't show a substantial detriment to running the `portable` build vs `modern`, in order to take things slowly I propose the following roll-out strategy:

- Keep both `modern` and `portable` builds for releases/Docker images.
- Run the `portable` build on half of SigP's infrastructure to monitor for performance deficits.
- Listen out for user issues with the `portable` builds (e.g. SIGILLs from misdetected hardware).
- Make the `portable` build the default and remove the `modern` build from our release binaries & Docker images.
@bors
Copy link

bors bot commented Aug 17, 2023

@bors bors bot changed the title Update blst to 0.3.11 [Merged by Bors] - Update blst to 0.3.11 Aug 17, 2023
@bors bors bot closed this Aug 17, 2023
@michaelsproul michaelsproul deleted the blst-0.3.11 branch August 17, 2023 03:41
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Proposed Changes

This PR updates `blst` to 0.3.11, which gives us _runtime detection of CPU features_ 🎉

Although [performance benchmarks](https://gist.github.com/michaelsproul/f759fa28dfa4003962507db34b439d6c) don't show a substantial detriment to running the `portable` build vs `modern`, in order to take things slowly I propose the following roll-out strategy:

- Keep both `modern` and `portable` builds for releases/Docker images.
- Run the `portable` build on half of SigP's infrastructure to monitor for performance deficits.
- Listen out for user issues with the `portable` builds (e.g. SIGILLs from misdetected hardware).
- Make the `portable` build the default and remove the `modern` build from our release binaries & Docker images.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
This PR updates `blst` to 0.3.11, which gives us _runtime detection of CPU features_ 🎉

Although [performance benchmarks](https://gist.github.com/michaelsproul/f759fa28dfa4003962507db34b439d6c) don't show a substantial detriment to running the `portable` build vs `modern`, in order to take things slowly I propose the following roll-out strategy:

- Keep both `modern` and `portable` builds for releases/Docker images.
- Run the `portable` build on half of SigP's infrastructure to monitor for performance deficits.
- Listen out for user issues with the `portable` builds (e.g. SIGILLs from misdetected hardware).
- Make the `portable` build the default and remove the `modern` build from our release binaries & Docker images.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto An issue/PR that touches cryptography code. ready-for-merge This PR is ready to merge. v4.4.1 ETA August 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants