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

Force use of portable blst in nodejs bindings #339

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

jtraglia
Copy link
Member

  • Add __BLST_PORTABLE__ to list of defines for all source files/platforms.

Note, there was this warning in the build step:

ld: warning: tentative definition of '___blst_platform_cap' with size 4 from 'Release/obj.target/kzg/deps/blst/build/assembly.o' is being replaced by real definition of smaller size 0 from 'Release/obj.target/kzg/deps/blst/src/server.o'

But I think this is fine. Probably a side-effect of blst's implementation. In blst, there is a tentative symbol (aka extern) defined in the assembly file. Then the real variable is defined in C code. Typically, when you list symbols there are two:

$ nm libblst.a | ag cap
0000000000000004 C ___blst_platform_cap
00000000000201b8 S ___blst_platform_cap

I think nodejs is deleting the tentative symbol/variable and just keeping the real one.

$ nm kzg.node | ag cap
0000000000034240 s ___blst_platform_cap

Also notice that S is now lowercase s which I think means it's a local variable.

I made sure that runtime detection was working by:

  • modifying the optimized asm code for sha256 to an illegal instruction,
  • changing ___blst_platform_cap to a different value,
  • check to see that it doesn't crash (because it's using the portable impl),
  • reverting ___blst_platform_cap,
  • check that it does crash (because it's using the optimized impl).

To me this checks out, but something we could/should investigate more in the future.

Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation! LGTM!

@asn-d6 asn-d6 merged commit 3ce8f86 into ethereum:main Aug 11, 2023
@jtraglia jtraglia deleted the nodejs-portable-blst branch August 11, 2023 15:20
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.

2 participants