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.

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

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...

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 :)

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.

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:

• tests: Lower default of iters to 4 (but keep higher on CI) bitcoin-core/secp256k1#1561
• test, ci: Lower default iteration count to 16 bitcoin-core/secp256k1#1581

cc @real-or-random

cc @sipa @jonasnick

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.

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

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)

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).

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:

src/core/bitcoin on  reduce-secp-iter [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env) took 5s
❯ set -x SECP256K1_TEST_ITERS 4

src/core/bitcoin on  reduce-secp-iter [$?] via △ v3.31.6 via 🐍 v3.13.3 via ❄️  impure (nix-shell-env)
❯ time build/src/secp256k1/bin/noverify_tests
test count = 4
random seed = 1cd83c3b42cf05f35e46a96cf63feaa3
Skipping run_sha256_known_output_tests 1000000 (iteration count too low)
Skipping test_ecmult_constants_2bit (iteration count too low)
random run = a44101523d760692b01a48540b8bee16
no problems found

________________________________________________________
Executed in    4.42 secs    fish           external
   usr time    4.37 secs    4.62 millis    4.36 secs
   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

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 bitcoin-core/packaging#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.