Remove SECP256K1_GNUC_PREREQ macro #1869

pull Copilot wants to merge 3 commits into bitcoin-core:master from bitcoin-core:copilot/remove-secp256k1-gnuc-prereq changing 3 files +12 −20

Copilot commented at 8:05 AM on June 12, 2026: none
SECP256K1_GNUC_PREREQ was only used for ancient GCC version checks (2.7, 3.0, 3.4), is misleading because __GNUC__ is also set by Clang (hardcoded to 4.2), and contributors weren't consistently using it anyway.
- Replace all uses with plain defined(__GNUC__), since every GCC version we'd reasonably support has __inline__, __builtin_expect, __restrict__, __warn_unused_result__, __nonnull__, and the __builtin_ctz* family
- Remove the macro definition from include/secp256k1.h
- Remove transitive #include "../include/secp256k1.h" from src/util.h, moving it to src/hash.h which directly depends on secp256k1_sha256_compression_function
Copilot assigned Copilot on Jun 12, 2026
Copilot assigned real-or-random on Jun 12, 2026
?
copilot_work_started real-or-random
Copilot renamed this:
~~[WIP] Remove SECP256K1_GNUC_PREREQ macro from codebase~~
Remove SECP256K1_GNUC_PREREQ macro
on Jun 12, 2026
?
copilot_work_finished real-or-random
Copilot requested review from real-or-random on Jun 12, 2026
real-or-random commented at 9:05 AM on June 12, 2026: contributor

@copilot Please add one more commit as follows:

Attempted removing #include "../include/secp256k1.h" from src/util.h as suggested, but this breaks the build: src/hash.h depends on secp256k1_sha256_compression_function transitively through util.h.

These transitive dependencies should be replaced, i.e., if hash.h depends on secp256k1_sha256_compression_function, then it should include ../include/secp256k1.h. Include formatting is: system includes, empty line, public header includes (../include/secp256k1.h), empty line, internal includes). If you run into similar transitive dependencies, solve them in the same way.
?
copilot_work_started real-or-random
Copilot commented at 9:08 AM on June 12, 2026: none

@copilot Please add one more commit as follows:

...

Done in cd833bd. Moved the #include "../include/secp256k1.h" from src/util.h to src/hash.h (the only file that transitively depended on it for secp256k1_sha256_compression_function). All 203 tests pass.
?
copilot_work_finished real-or-random
real-or-random force-pushed on Jun 12, 2026
real-or-random marked this as ready for review on Jun 12, 2026
real-or-random commented at 9:29 AM on June 12, 2026: contributor

cc @hebasto
real-or-random added the label needs-changelog on Jun 12, 2026
real-or-random commented at 9:30 AM on June 12, 2026: contributor

One concern here is that removing a macro from a public header is technically a breaking change. Do I wonder if we should really care. I don't think user code uses this, and I tend to believe that an entry in the changelog suffices.
hebasto commented at 10:19 AM on June 12, 2026: member

One concern here is that removing a macro from a public header is technically a breaking change. Do I wonder if we should really care. I don't think user code uses this, and I tend to believe that an entry in the changelog suffices.

I agree.
hebasto commented at 2:50 PM on June 13, 2026: member

80900a58b6fede781d0c4dee9e7249d6219c2e85

All GCC versions that we reasonably support...

It would be good to agree on and properly document the minimum required compiler version before making this assumption.
hebasto commented at 3:32 PM on June 13, 2026: member

80900a5

All GCC versions that we reasonably support...

It would be good to agree on and properly document the minimum required compiler version before making this assumption.

Considering #1871 (comment), I'd propose adopting GCC 2.95 as the documented minimum.
real-or-random commented at 11:22 AM on June 17, 2026: contributor
80900a5

All GCC versions that we reasonably support...

It would be good to agree on and properly document the minimum required compiler version before making this assumption.

Considering #1871 (comment), I'd propose adopting GCC 2.95 as the documented minimum.

I tend to disagree. The primary purpose of this PR is to make maintenance easier by declaring that we don't care about ancient compiler versions.

But documenting GCC 2.95 as a minimum makes maintenance more painful:
- It creates an urge to test on GCC 2.95. Doing this locally is a PITA, and even on CI (your #1871) this is annoying to maintain.
- The number needs to be maintained, i.e., bumped from time to time.
- This, in turn, raises the question of when the minimum version should be bumped (and what process/criteria) should apply.
- ...
I admit that the question what is supposed to be ancient isn't solved by simply ignoring it. But it tend to think that a somewhat vague "if it's more than 10 years old" or "if it's not even in debian oldoldstable" is a good enough answer and we'll be on the safe side with it. In we have doubts when adding a version-specific thing in the future, we can always check the actual value of __GNUC__. Removing SECP256K1_GNUC_PREREQ won't stop us from doing this.

Use __GNUC__ instead of SECP256K1_GNUC_PREREQ

Replace all SECP256K1_GNUC_PREREQ version checks with plain
defined(__GNUC__) checks, since the macro was only used for ancient GCC
versions that are no longer worth supporting individually. Moreover, the
macro was misleading because Clang claims to be GCC 4.2 by default.

All GCC versions that we reasonably support have the features previously
gated behind these checks (__inline__, __builtin_expect, __restrict__,
__warn_unused_result__, __nonnull__, and the __builtin_ctz* family).

Co-authored-by: Tim Ruffing <me@real-or-random.org>

09870e9c54

include: Remove SECP256K1_GNUC_PREREQ macro

The macro is no longer used anywhere in the codebase. This is
technically a breaking change, but it's not expected any user code
actually uses this macro.

Co-authored-by: Tim Ruffing <me@real-or-random.org>

dba4d937a9

hash: Include secp256k1.h directly

Move the #include "../include/secp256k1.h" from src/util.h to
src/hash.h, which is the file that actually depends on the
secp256k1_sha256_compression_function type defined there.

Co-authored-by: Tim Ruffing <me@real-or-random.org>

ae075d7cbd

real-or-random commented at 11:43 AM on June 17, 2026: contributor

Rebased on merged #1859.
real-or-random force-pushed on Jun 17, 2026
hebasto commented at 4:02 PM on June 17, 2026: member

But documenting GCC 2.95 as a minimum makes maintenance more painful:

Agreed, documenting a strict minimum isn't very pragmatic. We could, however, document a "recommended minimum" instead of a "required minimum." Debian's "oldoldstable" release fits that role perfectly.
hebasto approved
hebasto commented at 4:08 PM on June 17, 2026: member

ACK ae075d7cbd22860ed6cdf41f83d3d82d19b5a143, assuming we consider toolchains older than those shipped in Debian oldoldstable as "ancient".

Contributors

Copilot

real-or-random

hebasto

Review Requested
real-or-random

Labels

needs-changelog

Linked (view graph)

#1874 Minimum recommended toolchain - document and test it