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

feat: enable blst portable by default #389

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

Rjected
Copy link
Contributor

@Rjected Rjected commented Feb 2, 2024

This is so we can enable the underlying blst portable feature in reth for creating portable release binaries

@jtraglia
Copy link
Member

jtraglia commented Feb 3, 2024

Hey @Rjected thanks for the PR. This might have actually been an oversight on our part. The other bindings are using the portable version of blst by default, with the exception of Go because we can't control it from here. Semi-recently, the authors of blst added "cpu runtime detection" to the portable version of blst; so if the system is using a new cpu it will use optimized instructions and if it's an older cpu it will use the portable implementation. This is a long way of saying we should be using portable by default.

cc @asn-d6 @pawanjay176

@Rjected
Copy link
Contributor Author

Rjected commented Feb 3, 2024

Hey @Rjected thanks for the PR. This might have actually been an oversight on our part. The other bindings are using the portable version of blst by default, with the exception of Go because we can't control it from here. Semi-recently, the authors of blst added "cpu runtime detection" to the portable version of blst; so if the system is using a new cpu it will use optimized instructions and if it's an older cpu it will use the portable implementation. This is a long way of saying we should be using portable by default.

cc @asn-d6 @pawanjay176

Ah, interesting, that's actually pretty great, because it means we don't have to do anything differently between release binaries and building from source. Does this mean we should set default = ["portable"] or default = ["blst/portable"]?

@jtraglia
Copy link
Member

jtraglia commented Feb 3, 2024

Does this mean we should set default = ["portable"] or default = ["blst/portable"]?

Yes, and I slightly favor the second one but it's not a strong opinion.

@Rjected
Copy link
Contributor Author

Rjected commented Feb 3, 2024

Does this mean we should set default = ["portable"] or default = ["blst/portable"]?

Yes, and I slightly favor the second one but it's not a strong opinion.

makes sense, I just changed it to the second one

@Rjected Rjected changed the title feat: add portable feature to enable blst portable feat: enable blst portable by default Feb 3, 2024
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Looks great, thanks again! I will wait for another maintainer to approve before merging.

@asn-d6 asn-d6 merged commit 4607d3f into ethereum:main Feb 5, 2024
34 checks passed
@asn-d6
Copy link
Contributor

asn-d6 commented Feb 5, 2024

Thanks a lot!

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.

3 participants