Define SECP256K1_BUILD in secp256k1.c directly. #928

pull gmaxwell wants to merge 1 commits into bitcoin-core:master from gmaxwell:202105_secpbuild changing 6 files +28 −11

gmaxwell commented at 5:13 PM on May 1, 2021: contributor

This avoids building without it and makes it safer to use a custom building environment. Test harnesses need to #include secp256k1.c first now.

Fixes #927
sipa commented at 5:39 PM on May 1, 2021: contributor

Does this risk accidentally including secp256k1.h somewhere before secp256k1.c, thus getting the wrong interface definitions? If so, perhaps it's a possibility to have .h define a SECP256K1_NO_BUILD (if SECP256K1_BUILD wasn't defined already). .c can then error out if SECP256K1_NO_BUILD was defined.
gmaxwell commented at 5:49 PM on May 1, 2021: contributor

Indeed and the PR fixed a couple tests that were doing exactly that. I didn't consider it much of a concern because the non-null warnings catch it in development... but you're absolutely right that this is easy to avoid and so I added your suggestion.
in include/secp256k1.h:139 in 4849a7b250 outdated
```
 126 | @@ -127,6 +127,10 @@ typedef int (*secp256k1_nonce_function)(
 127 |  #  define SECP256K1_INLINE inline
 128 |  # endif
 129 |  
 130 | +#ifndef SECP256K1_BUILD
 131 | +# define SECP256K1_NO_BUILD
 132 | +#endif
```
real-or-random commented at 6:21 PM on May 1, 2021:

nit: I think this would be a good place to spend one or two sentences to explain the purpose of these macros. (My personal experience: I think I had two ask a few times before I understood the entire concept.)

gmaxwell commented at 7:28 PM on May 1, 2021:

There is a comment below on the nullness macro. But I can add one to the guard about the guard specifically.
real-or-random commented at 6:21 PM on May 1, 2021: contributor

ACK mod nit

Define SECP256K1_BUILD in secp256k1.c directly.

This avoids building without it and makes it safer to use a custom
 building environment.  Test harnesses need to #include secp256k1.c
 first now.

ae9e648526

sipa commented at 7:58 PM on May 1, 2021: contributor

utACK ae9e648526ceaf7cd97ba4dfe3c105db8e226c35
real-or-random approved
real-or-random commented at 9:38 AM on May 2, 2021: contributor

ACK ae9e648526ceaf7cd97ba4dfe3c105db8e226c35
real-or-random merged this on May 2, 2021
real-or-random closed this on May 2, 2021
in src/secp256k1.c:7 in ae9e648526
```
   3 | @@ -4,6 +4,8 @@
   4 |   * file COPYING or https://www.opensource.org/licenses/mit-license.php.*
   5 |   ***********************************************************************/
   6 |  
   7 | +#define SECP256K1_BUILD
```
elichai commented at 12:31 PM on June 30, 2021:

Shouldn't this be wrapped in #ifndef SECP256k1_BUILD?

gmaxwell commented at 6:48 PM on June 30, 2021:

I don't see why-- if it was you wouldn't have spotted that your rust build was incorrectly setting it now that it shouldn't be set that way anymore.

elichai commented at 8:38 PM on June 30, 2021:

I just thought it was common practice to wrap defines in ifndef in the case that the define acts as a flag and doesn't carry any additional value. (I don't think there was any harm in the fact that we kept passing -DSECP256K1_BUILD to the compiler, as we don't really use "headers" in rust)
Fabcien referenced this in commit 94bada6448 on Jan 7, 2026
Fabcien referenced this in commit 350fe02fe3 on Jan 7, 2026

Contributors

Linked (view graph)

#927 Make it impossible to build without SECP256K1_BUILD defined #929 Tracking issue for manual (non-autotools) builds