Make files mostly self-contained #1724

real-or-random commented at 11:16 AM on August 14, 2025: contributor

This is a set of commits, each one trivial, that reorganizes code so that all C files are mostly self-contained.

These changes are merely fixes that make the files adhere to our current implicit style, which I'd describe as:

Every C file (library, tests.c, ...) is a single translation unit ("unit build"), except that the libsepc256k1_precomputed is separate to speed up compilation.
Internal "modules" are organized in headers (.h) and implementation files (_impl.h). Symbols not present in the header are private to that module.
Internal "modules" should include the headers of all other modules they use.
secp256k1.c includes all _impl.h files. Other C files include simply secp256k1.c, or, if that isn't possible for some reason (e.g., in the precompute_*.c files), the individual necessary _impl.h files.

So, none of these changes here should be controversial. In particular, I refrain from changing the above rules (e.g., by renaming the _impl.h to .c as suggested in #1039).

Why "mostly" self-contained?

After this PR, some errors remain when I run clang src/*.c and clang src/*.h on my machine. But all of them are special cases:

ctime_tests.c refuses to be compiled without valgrind or msan enabled.
src/field_10x26_impl.h, src/int128_struct_impl.h, and src/scalar_low_impl.h are confused because the autodetection logic in the preprocessor picks field_5x64, native int 128, and scalar_4x64 on my machine.

<details> <summary>Full output of "clang src/*.c" and "clang src/*.h` </summary>

> clang src/*.c
src/ctime_tests.c:16:4: error: "This tool cannot be compiled without memory-checking interface (valgrind or msan)"
   16 | #  error "This tool cannot be compiled without memory-checking interface (valgrind or msan)"
      |    ^
1 error generated.

> clang src/*.h -ferror-limit=100
src/field_10x26_impl.h:265:21: warning: incompatible pointer types initializing 'const uint32_t *' (aka 'const unsigned int *') with an expression of type 'const uint64_t[5]' (aka 'const unsigned long[5]') [-Wincompatible-pointer-types]
  265 |     const uint32_t *t = a->n;
      |                     ^   ~~~~
src/field_10x26_impl.h:1007:28: warning: incompatible pointer types passing 'uint64_t[5]' (aka 'unsigned long[5]') to parameter of type 'uint32_t *' (aka 'unsigned int *') [-Wincompatible-pointer-types]
 1007 |     secp256k1_fe_mul_inner(r->n, a->n, b->n);
      |                            ^~~~
src/field_10x26_impl.h:401:63: note: passing argument to parameter 'r' here
  401 | SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint32_t *r, const uint32_t *a, const uint32_t * SECP256K1_RESTRICT b) {
      |                                                               ^
src/field_10x26_impl.h:1007:34: warning: incompatible pointer types passing 'const uint64_t[5]' (aka 'const unsigned long[5]') to parameter of type 'const uint32_t *' (aka 'const unsigned int *') [-Wincompatible-pointer-types]
 1007 |     secp256k1_fe_mul_inner(r->n, a->n, b->n);
      |                                  ^~~~
src/field_10x26_impl.h:401:82: note: passing argument to parameter 'a' here
  401 | SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint32_t *r, const uint32_t *a, const uint32_t * SECP256K1_RESTRICT b) {
      |                                                                                  ^
src/field_10x26_impl.h:1007:40: warning: incompatible pointer types passing 'const uint64_t[5]' (aka 'const unsigned long[5]') to parameter of type 'const uint32_t *' (aka 'const unsigned int *') [-Wincompatible-pointer-types]
 1007 |     secp256k1_fe_mul_inner(r->n, a->n, b->n);
      |                                        ^~~~
src/field_10x26_impl.h:401:121: note: passing argument to parameter 'b' here
  401 | SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint32_t *r, const uint32_t *a, const uint32_t * SECP256K1_RESTRICT b) {
      |                                                                                                                         ^
src/field_10x26_impl.h:1011:28: warning: incompatible pointer types passing 'uint64_t[5]' (aka 'unsigned long[5]') to parameter of type 'uint32_t *' (aka 'unsigned int *') [-Wincompatible-pointer-types]
 1011 |     secp256k1_fe_sqr_inner(r->n, a->n);
      |                            ^~~~
src/field_10x26_impl.h:731:63: note: passing argument to parameter 'r' here
  731 | SECP256K1_INLINE static void secp256k1_fe_sqr_inner(uint32_t *r, const uint32_t *a) {
      |                                                               ^
src/field_10x26_impl.h:1011:34: warning: incompatible pointer types passing 'const uint64_t[5]' (aka 'const unsigned long[5]') to parameter of type 'const uint32_t *' (aka 'const unsigned int *') [-Wincompatible-pointer-types]
 1011 |     secp256k1_fe_sqr_inner(r->n, a->n);
      |                                  ^~~~
src/field_10x26_impl.h:731:82: note: passing argument to parameter 'a' here
  731 | SECP256K1_INLINE static void secp256k1_fe_sqr_inner(uint32_t *r, const uint32_t *a) {
      |                                                                                  ^
6 warnings generated.
src/int128_struct_impl.h:54:6: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   54 |     r->hi = hi;
      |     ~^ ~~
src/int128_struct_impl.h:55:6: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   55 |     r->lo = lo;
      |     ~^ ~~
src/int128_struct_impl.h:59:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   59 |    r->lo = secp256k1_umul128(a, b, &r->hi);
      |    ~^ ~~
src/int128_struct_impl.h:59:38: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   59 |    r->lo = secp256k1_umul128(a, b, &r->hi);
      |                                     ~^ ~~
src/int128_struct_impl.h:65:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   65 |    r->lo += lo;
      |    ~^ ~~
src/int128_struct_impl.h:66:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   66 |    r->hi += hi + (r->lo < lo);
      |    ~^ ~~
src/int128_struct_impl.h:66:20: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   66 |    r->hi += hi + (r->lo < lo);
      |                   ~^ ~~
src/int128_struct_impl.h:70:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   70 |    r->lo += a;
      |    ~^ ~~
src/int128_struct_impl.h:71:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   71 |    r->hi += r->lo < a;
      |    ~^ ~~
src/int128_struct_impl.h:71:14: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   71 |    r->hi += r->lo < a;
      |             ~^ ~~
src/int128_struct_impl.h:80:7: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   80 |      r->lo = r->hi >> (n-64);
      |      ~^ ~~
src/int128_struct_impl.h:80:15: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   80 |      r->lo = r->hi >> (n-64);
      |              ~^ ~~
src/int128_struct_impl.h:81:7: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   81 |      r->hi = 0;
      |      ~^ ~~
src/int128_struct_impl.h:87:7: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   87 |      r->lo = ((1U * r->hi) << (64-n)) | r->lo >> n;
      |      ~^ ~~
src/int128_struct_impl.h:87:22: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   87 |      r->lo = ((1U * r->hi) << (64-n)) | r->lo >> n;
      |                     ~^ ~~
src/int128_struct_impl.h:87:42: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   87 |      r->lo = ((1U * r->hi) << (64-n)) | r->lo >> n;
      |                                         ~^ ~~
src/int128_struct_impl.h:89:7: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
   89 |      r->hi >>= n;
      |      ~^ ~~
src/int128_struct_impl.h:94:12: error: member reference base type 'const secp256k1_uint128' (aka 'const unsigned __int128') is not a structure or union
   94 |    return a->lo;
      |           ~^ ~~
src/int128_struct_impl.h:98:12: error: member reference base type 'const secp256k1_uint128' (aka 'const unsigned __int128') is not a structure or union
   98 |    return a->hi;
      |           ~^ ~~
src/int128_struct_impl.h:102:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
  102 |    r->hi = 0;
      |    ~^ ~~
src/int128_struct_impl.h:103:5: error: member reference base type 'secp256k1_uint128' (aka 'unsigned __int128') is not a structure or union
  103 |    r->lo = a;
      |    ~^ ~~
src/int128_struct_impl.h:108:22: error: member reference base type 'const secp256k1_uint128' (aka 'const unsigned __int128') is not a structure or union
  108 |    return n >= 64 ? r->hi >> (n - 64) == 0
      |                     ~^ ~~
src/int128_struct_impl.h:109:22: error: member reference base type 'const secp256k1_uint128' (aka 'const unsigned __int128') is not a structure or union
  109 |                   : r->hi == 0 && r->lo >> n == 0;
      |                     ~^ ~~
src/int128_struct_impl.h:109:36: error: member reference base type 'const secp256k1_uint128' (aka 'const unsigned __int128') is not a structure or union
  109 |                   : r->hi == 0 && r->lo >> n == 0;
      |                                   ~^ ~~
src/int128_struct_impl.h:113:6: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  113 |     r->hi = hi;
      |     ~^ ~~
src/int128_struct_impl.h:114:6: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  114 |     r->lo = lo;
      |     ~^ ~~
src/int128_struct_impl.h:119:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  119 |    r->lo = (uint64_t)secp256k1_mul128(a, b, &hi);
      |    ~^ ~~
src/int128_struct_impl.h:120:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  120 |    r->hi = (uint64_t)hi;
      |    ~^ ~~
src/int128_struct_impl.h:126:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  126 |    r->lo += lo;
      |    ~^ ~~
src/int128_struct_impl.h:127:11: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  127 |    hi += r->lo < lo;
      |          ~^ ~~
src/int128_struct_impl.h:139:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  139 |    r->hi += hi;
      |    ~^ ~~
src/int128_struct_impl.h:145:11: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  145 |    hi += r->lo < lo;
      |          ~^ ~~
src/int128_struct_impl.h:156:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  156 |    r->hi -= hi;
      |    ~^ ~~
src/int128_struct_impl.h:157:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  157 |    r->lo -= lo;
      |    ~^ ~~
src/int128_struct_impl.h:171:7: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  171 |      r->lo = (uint64_t)((int64_t)(r->hi) >> (n-64));
      |      ~^ ~~
src/int128_struct_impl.h:171:36: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  171 |      r->lo = (uint64_t)((int64_t)(r->hi) >> (n-64));
      |                                   ~^ ~~
src/int128_struct_impl.h:172:7: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  172 |      r->hi = (uint64_t)((int64_t)(r->hi) >> 63);
      |      ~^ ~~
src/int128_struct_impl.h:172:36: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  172 |      r->hi = (uint64_t)((int64_t)(r->hi) >> 63);
      |                                   ~^ ~~
src/int128_struct_impl.h:174:7: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  174 |      r->lo = ((1U * r->hi) << (64-n)) | r->lo >> n;
      |      ~^ ~~
src/int128_struct_impl.h:174:22: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  174 |      r->lo = ((1U * r->hi) << (64-n)) | r->lo >> n;
      |                     ~^ ~~
src/int128_struct_impl.h:174:42: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  174 |      r->lo = ((1U * r->hi) << (64-n)) | r->lo >> n;
      |                                         ~^ ~~
src/int128_struct_impl.h:175:7: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  175 |      r->hi = (uint64_t)((int64_t)(r->hi) >> n);
      |      ~^ ~~
src/int128_struct_impl.h:175:36: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  175 |      r->hi = (uint64_t)((int64_t)(r->hi) >> n);
      |                                   ~^ ~~
src/int128_struct_impl.h:180:12: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  180 |    return a->lo;
      |           ~^ ~~
src/int128_struct_impl.h:190:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  190 |    r->hi = (uint64_t)(a >> 63);
      |    ~^ ~~
src/int128_struct_impl.h:191:5: error: member reference base type 'secp256k1_int128' (aka '__int128') is not a structure or union
  191 |    r->lo = (uint64_t)a;
      |    ~^ ~~
src/int128_struct_impl.h:195:12: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  195 |    return a->hi == b->hi && a->lo == b->lo;
      |           ~^ ~~
src/int128_struct_impl.h:195:21: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  195 |    return a->hi == b->hi && a->lo == b->lo;
      |                    ~^ ~~
src/int128_struct_impl.h:195:30: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  195 |    return a->hi == b->hi && a->lo == b->lo;
      |                             ~^ ~~
src/int128_struct_impl.h:195:39: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  195 |    return a->hi == b->hi && a->lo == b->lo;
      |                                      ~^ ~~
src/int128_struct_impl.h:201:23: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  201 |     return n >= 64 ? r->hi == (uint64_t)sign << (n - 64) && r->lo == 0
      |                      ~^ ~~
src/int128_struct_impl.h:201:62: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  201 |     return n >= 64 ? r->hi == (uint64_t)sign << (n - 64) && r->lo == 0
      |                                                             ~^ ~~
src/int128_struct_impl.h:202:23: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  202 |                    : r->hi == (uint64_t)(sign >> 1) && r->lo == (uint64_t)sign << n;
      |                      ~^ ~~
src/int128_struct_impl.h:202:57: error: member reference base type 'const secp256k1_int128' (aka 'const __int128') is not a structure or union
  202 |                    : r->hi == (uint64_t)(sign >> 1) && r->lo == (uint64_t)sign << n;
      |                                                        ~^ ~~
54 errors generated.
src/scalar_low_impl.h:19:17: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
   19 |     return !(*a & 1);
      |              ~~ ^ ~
src/scalar_low_impl.h:23:14: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
   23 |     *r = v % EXHAUSTIVE_TEST_ORDER;
      |              ^
src/scalar_low_impl.h:33:20: error: invalid operands to binary expression ('const secp256k1_scalar' and 'unsigned int')
   33 |         return (*a >> offset) & (0xFFFFFFFF >> (32 - count));
      |                 ~~ ^  ~~~~~~
src/scalar_low_impl.h:45:103: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
   45 | SECP256K1_INLINE static int secp256k1_scalar_check_overflow(const secp256k1_scalar *a) { return *a >= EXHAUSTIVE_TEST_ORDER; }
      |                                                                                                       ^
src/scalar_low_impl.h:51:14: error: invalid operands to binary expression ('const secp256k1_scalar' and 'const secp256k1_scalar')
   51 |     *r = (*a + *b) % EXHAUSTIVE_TEST_ORDER;
      |           ~~ ^ ~~
src/scalar_low_impl.h:51:22: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
   51 |     *r = (*a + *b) % EXHAUSTIVE_TEST_ORDER;
      |                      ^
src/scalar_low_impl.h:54:15: error: invalid operands to binary expression ('secp256k1_scalar' and 'const secp256k1_scalar')
   54 |     return *r < *b;
      |            ~~ ^ ~~
src/scalar_low_impl.h:61:12: error: invalid operands to binary expression ('secp256k1_scalar' and 'uint32_t' (aka 'unsigned int'))
   61 |         *r += ((uint32_t)1 << bit);
      |         ~~ ^  ~~~~~~~~~~~~~~~~~~~~
src/scalar_low_impl.h:72:8: error: assigning to 'secp256k1_scalar' from incompatible type 'int'
   72 |     *r = 0;
      |        ^ ~
src/scalar_low_impl.h:74:18: error: invalid operands to binary expression ('secp256k1_scalar' and 'int')
   74 |         *r = (*r * 0x100) + b32[i];
      |               ~~ ^ ~~~~~
src/scalar_low_impl.h:75:19: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
   75 |         if (*r >= EXHAUSTIVE_TEST_ORDER) {
      |                   ^
src/scalar_low_impl.h:77:19: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
   77 |             *r %= EXHAUSTIVE_TEST_ORDER;
      |                   ^
src/scalar_low_impl.h:89:18: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
   89 |     bin[28] = *a >> 24; bin[29] = *a >> 16; bin[30] = *a >> 8; bin[31] = *a;
      |               ~~ ^  ~~
src/scalar_low_impl.h:89:38: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
   89 |     bin[28] = *a >> 24; bin[29] = *a >> 16; bin[30] = *a >> 8; bin[31] = *a;
      |                                   ~~ ^  ~~
src/scalar_low_impl.h:89:58: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
   89 |     bin[28] = *a >> 24; bin[29] = *a >> 16; bin[30] = *a >> 8; bin[31] = *a;
      |                                                       ~~ ^  ~
src/scalar_low_impl.h:89:72: error: assigning to 'unsigned char' from incompatible type 'const secp256k1_scalar'
   89 |     bin[28] = *a >> 24; bin[29] = *a >> 16; bin[30] = *a >> 8; bin[31] = *a;
      |                                                                        ^ ~~
src/scalar_low_impl.h:95:15: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
   95 |     return *a == 0;
      |            ~~ ^  ~
src/scalar_low_impl.h:101:12: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
  101 |     if (*a == 0) {
      |         ~~ ^  ~
src/scalar_low_impl.h:102:12: error: assigning to 'secp256k1_scalar' from incompatible type 'int'
  102 |         *r = 0;
      |            ^ ~
src/scalar_low_impl.h:104:14: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
  104 |         *r = EXHAUSTIVE_TEST_ORDER - *a;
      |              ^
src/scalar_low_impl.h:113:15: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
  113 |     return *a == 1;
      |            ~~ ^  ~
src/scalar_low_impl.h:119:17: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
  119 |     return *a > EXHAUSTIVE_TEST_ORDER / 2;
      |                 ^
src/scalar_low_impl.h:135:14: error: invalid operands to binary expression ('const secp256k1_scalar' and 'const secp256k1_scalar')
  135 |     *r = (*a * *b) % EXHAUSTIVE_TEST_ORDER;
      |           ~~ ^ ~~
src/scalar_low_impl.h:135:22: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
  135 |     *r = (*a * *b) % EXHAUSTIVE_TEST_ORDER;
      |                      ^
src/scalar_low_impl.h:144:9: error: assigning to 'secp256k1_scalar' from incompatible type 'int'
  144 |     *r2 = 0;
      |         ^ ~
src/scalar_low_impl.h:154:15: error: invalid operands to binary expression ('const secp256k1_scalar' and 'const secp256k1_scalar')
  154 |     return *a == *b;
      |            ~~ ^  ~~
src/scalar_low_impl.h:165:14: error: invalid operands to binary expression ('secp256k1_scalar' and 'uint32_t' (aka 'unsigned int'))
  165 |     *r = (*r & mask0) | (*a & mask1);
      |           ~~ ^ ~~~~~
src/scalar_low_impl.h:165:29: error: invalid operands to binary expression ('const secp256k1_scalar' and 'uint32_t' (aka 'unsigned int'))
  165 |     *r = (*r & mask0) | (*a & mask1);
      |                          ~~ ^ ~~~~~
src/scalar_low_impl.h:175:21: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
  175 |     for (i = 0; i < EXHAUSTIVE_TEST_ORDER; i++) {
      |                     ^
src/scalar_low_impl.h:176:16: error: invalid operands to binary expression ('int' and 'const secp256k1_scalar')
  176 |         if ((i * *x) % EXHAUSTIVE_TEST_ORDER == 1) {
      |              ~ ^ ~~
src/scalar_low_impl.h:176:24: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
  176 |         if ((i * *x) % EXHAUSTIVE_TEST_ORDER == 1) {
      |                        ^
src/scalar_low_impl.h:185:8: error: assigning to 'secp256k1_scalar' from incompatible type 'uint32_t' (aka 'unsigned int')
  185 |     *r = res;
      |        ^ ~~~
src/scalar_low_impl.h:201:33: error: invalid operands to binary expression ('const secp256k1_scalar' and 'int')
  201 |     *r = (*a + ((-(uint32_t)(*a & 1)) & EXHAUSTIVE_TEST_ORDER)) >> 1;
      |                              ~~ ^ ~
src/scalar_low_impl.h:201:41: error: use of undeclared identifier 'EXHAUSTIVE_TEST_ORDER'
  201 |     *r = (*a + ((-(uint32_t)(*a & 1)) & EXHAUSTIVE_TEST_ORDER)) >> 1;
      |                                         ^
34 errors generated.

</details>

Perhaps some of these cases can be improved further, but these improvements should go to a separate PR.

What is the motivation for these changes?

Self-contained files work much better with tooling. For example, the [clangd](https://clangd.llvm.org/installation) language server works great in my editor after this PR (ignoring the four files mentioned above) and gives me much better completion, symbol lookup, etc. In fact, what made me create this PR is that I got annoyed by not being able to jump to symbols when trying to review silent payments.

Here's my ~/.config/clangd/config.yaml if you're interested:

# libsecp256k1
If:
  PathMatch: .*/secp256k1/.*
CompileFlags:
    # clangd compiles even _impl.h files individually, so it won't
    # find definitions of static functions in other files.
    Add: [-Wno-undefined-internal]
Diagnostics:
  # Disable hints about unused and missing includes. The assumptions that clangd
  # make about inclusion style are too far off from what libsecp256k1 does.
  UnusedIncludes: None
  MissingIncludes: None

You'll also need to run make clean and bear -- make once to create a compile_commands.json file in the repo root that contains the compiler flags so that clangd knows the configured -D flags etc. Get bear first.

edit: CMake can also generate the json file:

$ mkdir build
$ cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -B build
$ cmake --build build

Then the file will be stored in the build subdirectory, whose name is special-cased in clangd so that it the file will be found automatically.

Or the flags can be added above in CompileFlags manually.

real-or-random added the label tweak/refactor on Aug 14, 2025

real-or-random added the label meta/development on Aug 14, 2025

real-or-random force-pushed on Aug 14, 2025

real-or-random commented at 12:27 PM on August 14, 2025: contributor

Remaining pain points that I noticed

There's no way to enforce that we keep this in the future. I consider adding a check to CI that just compiles all files individually (with a list of exceptions for now).
It's still annoying to include the right header (=non-impl) .h files. CI can tell us if we forget a file but not if we have too many files. Our inclusion structure is too complicated for tools to figure out automatically (e.g., you should include scalar.h even if some defs you use are in scalar_4x64.h, and I'm not convinced that adding special code comments to explain our structure to tools is the right approach. I'd tend to think that we should just do the opposite thing and create an "umbrella" header internal.h that includes all the internal headers. This makes it simpler for the developer, and the only cost is perhaps a slightly increased compilation time but only when individual files are compiled, i.e., in your IDE if you choose to use clangd or some other language server). But the project is not huge, and I doubt that this will be a problem.
util.h is a bit special. It has mostly preprocessor stuff and some small functions, so I guess it's acceptable not to have a util_impl.h. But util.h currently contains also the preprocessor logic that picks the scalar, field, and int128 implementations based on the availability of widemul implementations. And this logic needs to happen before any of the resulting macros are used. So I think that part serves a different purpose than what the name "util" suggests and should be moved to a separate file.
We should also have headers for module implementation files, e.g., the extrakeys module "exports" secp256k1_extrakeys_ge_even_y which is used in the musig module.

in src/secp256k1_internal.h:1 in 925070e332

real-or-random commented at 12:29 PM on August 14, 2025:

I think what I picked is fine but I'm happy to take suggestions for the name of this file.

in src/secp256k1.c:43 in 925070e332 outdated

  44 |  #include "int128_impl.h"
  45 | +#include "modinv32_impl.h"
  46 | +#ifdef SECP256K1_WIDEMUL_INT128
  47 | +/* modinv64 is only available if we have an int128 implementation. */
  48 | +#include "modinv64_impl.h"
  49 | +#endif

real-or-random commented at 12:40 PM on August 14, 2025:

That part is a bit ugly, in particular because it needs to be duplicated in multiple C files.

Alternatives that I didn't find better:

Make including modinv64_impl.h a no-op ifndef SECP256K1_WIDEMUL_INT128. If anything, that violates the principle of least surprise.
Create a modinv_impl.h that selects between the two files. That's possible, but conceptually not really the right thing to do. modinv64 and modinv32 are two separate modules with different interface. They're not just two different implementations of the same interfaces (as is the case for the different field, scalar, int128 implementations).

real-or-random force-pushed on Aug 14, 2025

tests: Make ecmult tests use internal scratch API

This has the advantage of keeping the ecmult tests focused on the ecmult
code, and so this change might be desirable anyway. That's why it is
done in a separate commit, so it can be kept if we ever bring back the
public scratch space API.

76ce607df4

Remove ununsed scratch space functions

These were in the public API at some point but they have been made
static in 712e7f8722eba5dec2bc6b37d75aadeb6f6e633b.

bb37ad8fa0

scalar: Move definitions of constants to header d72f3613cc

scalar: Move EXHAUSTIVE_TEST_LAMBDA to header 29e82e7649

Create header for internal API in secp256k1.c 3304ce0c99

gitignore: Add precompiled headers (.gch and .pch)

We don't use precompiled headers in our builds but those files are what
you get if you invoke gcc or clang, respectively, on a .h file, e.g., to check
if it's self-contained.

f181af00d7

group: Add missing declarations to internal header 151a2da0ae

Tidy includes to make many headers self-contained 250c61450f

field: Inline fe_verify fields in repr headers

Otherwise, the repr headers field_AxB.h would need to include field.h to
be self-contained but this recursive inclusion leaves clangd confused,
see https://github.com/clangd/clangd/issues/337 .

8969deb942

bench_internal: Remove unnecessary includes

secp256k1.c is included anyway, so there's no need to include the
individual _impl.h files.

0e17403bb6

group: Move definitions of constants to header

 SECP256K1_B and secp256k1_ge_const_g are used outside of
 group_impl.h, e.g., in ecmult_const_impl.h and ecmult_gen_impl.h,
 respectively.

b7840be6dc

Include modinv32/64 like other internal modules

And also sort the _impl.h includes.

5ac6929f54

selftest: Split into .h and _impl.h a2f118e6a9

ecmult_gen: Export _ecmult_gen_context_is_built() 8cce3922b4

real-or-random force-pushed on Aug 18, 2025

real-or-random commented at 3:48 PM on August 18, 2025: contributor

Added another commit that fixes a missing declaration in ecmult_gen.h

real-or-random referenced this in commit 70cfdb5927 on Aug 19, 2025

hebasto commented at 2:52 PM on September 13, 2025: member

Remaining pain points that I noticed

There's no way to enforce that we keep this in the future. I consider adding a check to CI that just compiles all files individually (with a list of exceptions for now).

FWIW, #1423 started with the introduction of such a tool. I still think this should be required in any PR that attempts to improve header inclusion.

hebasto commented at 3:03 PM on September 13, 2025: member

Remaining pain points that I noticed

There's no way to enforce that we keep this in the future. I consider adding a check to CI that just compiles all files individually (with a list of exceptions for now).

FWIW, #1423 started with the introduction of such a tool. I still think this should be required in any PR that attempts to improve header inclusion.

I've rebased #1423 to check that the code is still valid.

Feel free to incorporate it here if you think it's worth it.

UPD. Here is a combined branch that applies the test to as many headers as possible.

in src/selftest.h:10 in 8cce3922b4

  26 | -}
  27 | -
  28 | -static int secp256k1_selftest_passes(void) {
  29 | -    return secp256k1_selftest_sha256();
  30 | -}
  31 | +static int secp256k1_selftest_sha256(void);

hebasto commented at 11:54 AM on September 14, 2025:

b74b4c23c4c9d5684c0f5defdb56093101e4252e

Shouldn't this function be internal to selftest?

in src/selftest_impl.h:14 in 8cce3922b4

   9 | +
  10 | +#include "hash.h"
  11 | +#include "selftest.h"
  12 | +#include "util.h"
  13 | +
  14 | +#include <string.h>

hebasto commented at 11:54 AM on September 14, 2025:

b74b4c23c4c9d5684c0f5defdb56093101e4252e

This could be dropped.

in src/selftest_impl.h:12 in 8cce3922b4

   7 | +#ifndef SECP256K1_SELFTEST_IMPL_H
   8 | +#define SECP256K1_SELFTEST_IMPL_H
   9 | +
  10 | +#include "hash.h"
  11 | +#include "selftest.h"
  12 | +#include "util.h"

hebasto commented at 11:59 AM on September 14, 2025:

I have a general suggestion. Since every <name>_impl.h must include <name>.h, I think readability would improve if <name>.h always appeared first on its own line, as shown here:

#include "selftest.h"

#include "hash.h"
#include "util.h"

hebasto commented at 1:14 PM on September 14, 2025: member

These changes are merely fixes that make the files adhere to our current implicit style, which I'd describe as:

Every C file (library, tests.c, ...) is a single translation unit ("unit build"), except that the libsepc256k1_precomputed is separate to speed up compilation.

Internal "modules" are organized in headers (.h) and implementation files (_impl.h). Symbols not present in the header are private to that module.

Internal "modules" should include the headers of all other modules they use.

secp256k1.c includes all _impl.h files. Other C files include simply secp256k1.c, or, if that isn't possible for some reason (e.g., in the precompute_*.c files), the individual necessary _impl.h files.

While these rules indeed optimize performance regardless of LTO, they do a poor job of hiding implementation details for internal “modules” consisting of a header (.h) and an implementation file (_impl.h). Consider the following example, which follows all the rules above:

/* foo.h */
#ifndef UNITY_BUILD_DEMO_FOO_H
#define UNITY_BUILD_DEMO_FOO_H
static int foo();
#endif /* UNITY_BUILD_DEMO_FOO_H */

/* foo_impl.h */
#ifndef UNITY_BUILD_DEMO_FOO_IMPL_H
#define UNITY_BUILD_DEMO_FOO_IMPL_H
#include "foo.h"
#include "foobar.h"
static int foo() { return foobar(); }
#endif /* UNITY_BUILD_DEMO_FOO_IMPL_H */

/* foobar.h */
#ifndef UNITY_BUILD_DEMO_FOOBAR_H
#define UNITY_BUILD_DEMO_FOOBAR_H
static int foobar();
#endif /* UNITY_BUILD_DEMO_FOOBAR_H */

/* foobar_impl.h */
#ifndef UNITY_BUILD_DEMO_FOOBAR_IMPL_H
#define UNITY_BUILD_DEMO_FOOBAR_IMPL_H
#include "foobar.h"
static int foobar() { return 42; }
#endif /* UNITY_BUILD_DEMO_FOOBAR_IMPL_H */

/* main.c */
#include "foo_impl.h"
#include "foobar_impl.h"
#include <stdlib.h>
int main(void) {
    int a = foo();
    return EXIT_SUCCESS;
}

In this example, the call to foobar() is an implementation detail of foo(). main() does not invoke foobar() directly, yet we still have to include "foobar_impl.h" in main.c for the build to succeed. This effectively leaks foo()’s implementation details.

A proper additional rule, I believe, would be to allow implementation files to include other implementation files where needed. This yields the following structure:

/* foo_impl.h */
#ifndef UNITY_BUILD_DEMO_FOO_IMPL_H
#define UNITY_BUILD_DEMO_FOO_IMPL_H
#include "foo.h"
#include "foobar_impl.h"
static int foo() { return foobar(); }
#endif /* UNITY_BUILD_DEMO_FOO_IMPL_H */

/* main.c */
#include "foo_impl.h"
#include <stdlib.h>
int main(void) {
    int a = foo();
    return EXIT_SUCCESS;
}

I have analyzed this approach for potential drawbacks and have not found any.

hebasto commented at 1:31 PM on September 14, 2025: member

@real-or-random

Could you please elaborate on your motivation for 391d16a3ec3beebf5f6b03b3ad488843962d633c "Create header for internal API in secp256k1.c"? Why not move the implementation code from secp256k1.c to secp256k1_internal_impl.h as well?

UPD. I've completed my first round of reviewing. The code looks correct.