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

SECP256K1_BUILD sanity checking and USE_FIELD_INV_BUILTIN/USE_SCALAR_INV_BUILTIN simplification. #800

Closed
gmaxwell opened this issue Aug 15, 2020 · 5 comments

Comments

@gmaxwell
Copy link
Contributor

If secp256k1.c is built without SECP256K1_BUILD the non-null argument tests get compiled out and then the tests crash.

One way to address that would be to simply have test.c and secp256k1.c set the macro before importing the headers. I think this would be pretty fool-proof. If there is some reason that can't be done secp256k1.c compile could fail if SECP256K1_BUILD isn't set. I prefer the foolproof way because demanding the build system set some define is more trouble for users that aren't using the build system.

Related, USE_FIELD_INV_BUILTIN/USE_SCALAR_INV_BUILTIN should get automatically defaulted. (maybe these go away with safegcd?) right now a non-buildsystem user needs to set them.

Perhaps for the rest too... I think now that there aren't defines needed for the fe/scalar type it should be possible to build with zero defines.

@sipa
Copy link
Contributor

sipa commented Aug 15, 2020

One way to address that would be to simply have test.c and secp256k1.c set the macro before importing the headers.

I don't see why that wouldn't work.

Related, USE_FIELD_INV_BUILTIN/USE_SCALAR_INV_BUILTIN should get automatically defaulted. (maybe these go away with safegcd?) right now a non-buildsystem user needs to set them.

Yeah, I was expecting them to go away after safegcd (and we'd drop gmp/num support entirely; it can always be brought back from history if there is really a need).

Perhaps for the rest too... I think now that there aren't defines needed for the fe/scalar type it should be possible to build with zero defines.

The remaining things are:

  • GMP or not (for now) (field inv/scalar inv could be decided automatically based on the presence of GMP, but GMP's availability in the first place needs to be detected/configured through the build system or manual defines)
  • tweaks for ecmult / ecmult_gen (changed with Signed-digit multi-comb for ecmult_gen (by peterdettman) #693) - there could be an automatic default if there is no override, though.
  • viability of using x86_64 asm can probably be made automatic (it's restricted to x86_64, and to non-ancient gcc and cland anyway, which are pretty uniform)
  • ARM asm is harder/impossible, as it's a separate file that needs to be externally assembled
  • Endomorphism or not (will also go away :D)
  • Static precomputation or not

@gmaxwell
Copy link
Contributor Author

but GMP's availability in the first place needs to be detected/configured through the build system or manual defines)

Yes but if you don't use a build system, default to it off?

ASM could default on x86_64 ... and on ARM it could emit a warning if it's not used?

Static precompute could check for a define set by the header? ... though maybe consider shipping the static table to avoid having to generate it?

@real-or-random
Copy link
Contributor

I don't quite understand what the purpose of SECP256K1_BUILD is, can someone explain?

@sipa
Copy link
Contributor

sipa commented Aug 25, 2020

@real-or-random It's used to indicate that we're building the library itself, and not an interface.

It matters because secp256k1.h is included both when building the library itself, and by applications linking with it. For the former, we don't want the "nonnull" attributes, as they would make the compiler optimize away our explicit nonnull checks. We only want them as part of the external interface.

It's also not set when compiling gen_context or the benchmarks. It's less to clear to whether that matters.

@real-or-random
Copy link
Contributor

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

No branches or pull requests

3 participants