test: disable secp256 tests by default

willcl-ark commented at 9:00 am on June 20, 2025: member

Paritally addresses: #32770

This PR disables the slow secp256 test suites by default, both in CI runs and normal developer builds/tests. These tests are static in every run, except for the case in which we update the secp256 subtree.

This is done by decoupling the SECP256K1_BUILD_TESTS and SECP256K1_BUILD_EXHAUSTIVE_TESTS variables from the ${BUILD_TESTS} configuration, allowing them to be selected individually.

We would likely still like to run these tests when the subtree is updated. To do this a standalone workflow is provided which runs only when modifications to src/secp256k1/** are detected.

This reduces unit test runtime on my machine from ~30 seconds to ~10 seconds. On slower CI machines these tests are taking up to 10 minutes to run, so we may save up to 6 minutes of runtime per CI run.

build: decouple secp256 tests from BUILD_TESTS

These tests add a minimum of a few minutes to the unit test runtime, and
at worst see runtime double.

Decouple them from ${BUILD_TESTS} so that they can be more selectively
enabled/disabled.

8aa9aa05c5

DrahtBot added the label Tests on Jun 20, 2025

DrahtBot commented at 9:00 am on June 20, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32782.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

willcl-ark commented at 9:02 am on June 20, 2025: member

Questions:

The new workflow relies on GitHub detecting changes to the subtree dir. This could be risky? We might want to also add a weekly on: target to the workflow, or write our own function to test if the subtree was updated (e.g. diff the commit range as detected in test_each_commit job), to avoid cases where this subtree detection doesn’t work as planned/expected.
The new workflow only runs these tests on x64 Linux, whereas currently the tests are run on a variety of platforms. Should we be concerned about this? These tests are run in the secp256k1 repo on x64, i686, s390x, arm32, arm64, ppc64le, under valgrind, with various sanitizers, windows, macos, etc.
Should we perhaps enable these tests in the dev-mode preset in CMakePresets.json?

cc @maflcko

ci: Remove secp tests from window cross job 7c1fb9ef71

ci: add secp256 unit test workflow

This workflow job detects whether the secp256k1 subtree was updated,
and if it was will build and run the secp256k1 unit tests.

These are disabled by default (in other CI jobs), as they double the
runtime needed, and rarely change.

5ad028f153

willcl-ark force-pushed on Jun 20, 2025

l0rinc commented at 10:25 am on June 20, 2025: contributor

Could we keep them for CI, to make sure that our setup (different compiler versions, sanitizers, architectures) can still handle them? Though many of those will likely be tested implicitly anyway…

willcl-ark commented at 10:30 am on June 20, 2025: member

Could we keep them for CI, to make sure that our setup (different compiler versions, sanitizers, architectures) can still handle them? Though many of those will likely be tested implicitly anyway…

My fault for not making it more clear, but the intent behind this change is mostly towards (slow) CI. I don’t think a few extra seconds locally is any kind of issue for devs (also related to my Q about enabling them in the dev-mode preset), but this runtime is much longer on some CI runs (see #32770) where it starts to add up.

So, if you wanted to keep them running in CI for the reasons you stated, then you should NACK this PR :)

maflcko commented at 12:17 pm on June 20, 2025: member

if you want to run it through the sanitizers (msan, etc), the easiest would probably to check in the ./ci if the last merge commit is the secp256k subtree merge commit. If yes, run the tests. If no, skip them.

in .github/workflows/secp256.yml:33 in 5ad028f153

28+      - name: Build with secp256k1 tests enabled
29+        run: |
30+          cmake -B build \
31+            -DSECP256K1_BUILD_TESTS=ON \
32+            -DSECP256K1_BUILD_EXHAUSTIVE_TESTS=ON
33+          cmake --build build -j $(nproc)

fanquake commented at 10:33 am on June 24, 2025:

0          sudo apt-get install -y build-essential cmake pkgconf python3 libevent-dev libboost-dev
1      - name: Build with secp256k1 tests enabled
2        run: |
3          cmake -B build \
4            -DENABLE_WALLET=OFF \
5            -DSECP256K1_BUILD_TESTS=ON \
6            -DSECP256K1_BUILD_EXHAUSTIVE_TESTS=ON
7          cmake --build build -j14 --target noverify_tests tests exhaustive_tests -j $(nproc)

hebasto commented at 10:54 am on June 24, 2025: member

Paritally addresses: #32770

This PR disables the slow secp256 test suites by default, both in CI runs and normal developer builds/tests. These tests are static in every run, except for the case in which we update the secp256 subtree.

This is done by decoupling the SECP256K1_BUILD_TESTS and SECP256K1_BUILD_EXHAUSTIVE_TESTS variables from the ${BUILD_TESTS} configuration, allowing them to be selected individually.

We would likely still like to run these tests when the subtree is updated. To do this a standalone workflow is provided which runs only when modifications to src/secp256k1/** are detected.

This reduces unit test runtime on my machine from ~30 seconds to ~10 seconds. On slower CI machines these tests are taking up to 10 minutes to run, so we may save up to 6 minutes of runtime per CI run.

Alternatively, libsecp256k1 tests could be run with a lower value for the SECP256K1_TEST_ITERS environment variable: say, 8. See related discussions in:

cc @real-or-random

real-or-random commented at 8:41 pm on June 24, 2025: contributor

cc @sipa @jonasnick

real-or-random commented at 8:59 pm on June 24, 2025: contributor

Alternatively, libsecp256k1 tests could be run with a lower value for the SECP256K1_TEST_ITERS environment variable: say, 8.

We reduced the default count to 16 last year. It would be interesting to know how long CI takes for a count of 8 or 4. And if that’s not quite satisfying, we could disable some of the static (= not randomized) tests at low iteration counts. (We already do this for the most expensive ones.)

We test many many compile configurations (platform, compiler version, libsecp256k1 flags, …) in our CI, but the good thing about always running the tests here is that they also exercise the exact compile configuration used in Bitcoin Core.

willcl-ark commented at 10:24 am on June 25, 2025: member

I think if there is concern about configurations then I agree it would make more sense to drop the iterations to 4 or 8 as suggested by @hebasto and @real-or-random

luke-jr commented at 6:58 pm on June 26, 2025: member

Opposite thought from @l0rinc: disable them only for CI, but leave them enabled for normal builds (which may be the only time the tests are ever run on the end user machine)

gmaxwell commented at 0:31 am on June 29, 2025: contributor

As an author of that library I am uncomfortable with any usage unguarded by correctness tests. The software is sensitive to compiler errors (which can be triggered by all sorts of random environmental nonsense) and ought to be tested with the exact compiler environment used. A single predictable bit error in signing is enough to leak private keys and can happen in ways that still result in valid signatures. (e.g. miscompilation that results in the first byte/bit of the nonce being set to any static value is enough)

The tests could be made arbitrarily fast as needed, e.g. even reduction to a collection of a few thousand known response tests for signing would provide a lot of confidence and hardly take any time.

Is the bitcoin core project consistently running tests on all tested platforms except in CI these days?

If it is then getting incidentally tested as part of CI is not as important (but not unimportant, e.g. both for more random tries for data/alignment sensitive bugs or catching a subtly malicious PR that corrupts process memory in order to undermining security).

willcl-ark commented at 9:09 am on June 30, 2025: member

As an author of that library I am uncomfortable with any usage unguarded by correctness tests. The software is sensitive to compiler errors (which can be triggered by all sorts of random environmental nonsense) and ought to be tested with the exact compiler environment used. A single predictable bit error in signing is enough to leak private keys and can happen in ways that still result in valid signatures. (e.g. miscompilation that results in the first byte/bit of the nonce being set to any static value is enough)

This is more than enough reason to keep running the tests in every CI run.

The tests could be made arbitrarily fast as needed, e.g. even reduction to a collection of a few thousand known response tests for signing would provide a lot of confidence and hardly take any time.

This could be an interesting approach. FWIW I experimented a little with reducing the iterations as suggested above and it does reduce the runtime quite significantly (e.e. below in the noverify_tests from 11s to 4s), but also warns that it begins skipping a few tests as there are not enough iterations:

 0src/core/bitcoin on  reduce-secp-iter [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env) took 5s
 1❯ set -x SECP256K1_TEST_ITERS 4
 2
 3src/core/bitcoin on  reduce-secp-iter [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env)
 4❯ time build/src/secp256k1/bin/noverify_tests
 5test count = 4
 6random seed = 1cd83c3b42cf05f35e46a96cf63feaa3
 7Skipping run_sha256_known_output_tests 1000000 (iteration count too low)
 8Skipping test_ecmult_constants_2bit (iteration count too low)
 9random run = a44101523d760692b01a48540b8bee16
10no problems found
11
12________________________________________________________
13Executed in    4.42 secs    fish           external
14   usr time    4.37 secs    4.62 millis    4.36 secs
15   sys time    0.05 secs    1.93 millis    0.04 secs

Is the bitcoin core project consistently running tests on all tested platforms except in CI these days?

If it is then getting incidentally tested as part of CI is not as important (but not unimportant, e.g. both for more random tries for data/alignment sensitive bugs or catching a subtly malicious PR that corrupts process memory in order to undermining security).

To my knowledge we have:

CI runs on every PR and merge
Local developer runs
Various external “nightly” CI runs (which may include other platforms) in other repos e.g. https://github.com/hebasto/bitcoin-core-nightly/ and https://github.com/maflcko/b-c-nightly but should perhaps not be relied on too heavily

Thank you all for feedback. My read now is that the best approach here will be to close this and instead work specicifally on better parallelisation/resource usage of the unit and functional tests in CI to address #32770

willcl-ark closed this on Jun 30, 2025

maflcko commented at 10:14 am on June 30, 2025: member

For the guix cross-compiled release binaries, I don’t think the secp256 tests are included at all. So testing for compiler errors is currently only done indirectly, when a CI task (or a dev) picks a compiler with a somewhat similar version string. I’ve opened https://github.com/bitcoin-core/packaging/pull/285 as a draft to actually test the guix-produced release binaries. However, it seems a bit tedious, and it would be easier if the release binaries offered an extended/dev/debug build that includes secp256 tests, as well as the config.ini for the functional tests.

test: disable secp256 tests by default #32782

Code Coverage & Benchmarks

Reviews