discussion: Config used by Core vs defaults and config in CI/testing

[real-or-random]

Quoting myself from https://github.com/bitcoin-core/secp256k1/issues/1543#issuecomment-2176914205:

I believe we want to build libsecp256k1 with the default RelWithDebInfo (-O2 -g in our case) even when we build it as part of Core. Because (almost) all our testing uses this config.

But now that this has led @hebasto to make or suggest a few changes (https://github.com/hebasto/bitcoin/pull/192#issuecomment-2184085876, https://github.com/bitcoin-core/secp256k1/pull/1547), I'm not confident enough, and I think we should discuss what our desired policy is.

To put that in perspective, Core now builds with --with-ecmult-gen-kb=86 as @sipa suggested, but almost all of our testing/CI uses the default value of 22.

If we come to the conclusion that most of our testing should align with what Core uses, then the difference of 86 vs 22 (as some kind of algorithmic change) is probably much more likely to cause trouble than some -g compiler flag. (On the other hand, stuff like -O3 vs -O2 could make a real difference again.)

cc @fanquake

---

Independently of that discussion, it would be awesome to have the ability to set something like "--with-ecmult-gen-kb=random" for CI. But this requires someone to implement it. :)

[hebasto]

If we come to the conclusion that most of our testing should align with what Core uses, then the difference of 86 vs 22 (as some kind of algorithmic change) is probably much more likely to cause trouble than some -g compiler flag.

Mind elaborating what kind of troubles do you expect?

[real-or-random]

Mind elaborating what kind of troubles do you expect?

Sure, sorry. The concern is simply that there's a bug that appears only with some specific config. Some examples:

• Say there's a bug in our ecmult_gen implementation that is only relevant in the 86 config, but not in the 22 config.
• The compiler performs some wrong optimization or "optimizes" away some of our constant-time techniques (what we've seen in the past) only under -O3 but not in -O2.
• Or a combination of the two: The compiler applies a different optimization strategy only in the 86 config but not in the 22 config (because some arrays have different sizes etc), and this uncovers a bug in the compiler or in our code.

I believe different flags have different levels of risk. Flags that apply to our code directly and optimization flags (like in the above examples) have a higher risk of introducing "severe" bugs than -g or, e.g., some linker flags. The latter can certainly also lead to issue, but I guess these are rather build failures.

[real-or-random]

One specific instance was that preferred (a few years ago) to leave Valgrind enabled in production builds if possible, to make sure that the ctime_tests binary with Valgrind on matches production builds as closely as possible. See https://github.com/bitcoin-core/secp256k1/pull/813#issuecomment-691720230 and the comment below it for detailed rationale, which includes this argument:

The constant time test, however, is substantially a miscompilation test.

[real-or-random]

Quick meeting of the discussion at the meeting today:

Good rules of thumb are:

• We should mostly test our defaults.
• Our default should align with Bitcoin Core's defaults, as they should target standard desktop machines. (Users on other platforms will want to set a config anyway.)

In the specific case of ecmult_gen, this means we should just increase the default to 86 kib.

For ctime time tests: We should run them by default in the test suite except on architectures where they're known to be problematic (perhaps s390 and power9 according to https://github.com/bitcoin-core/secp256k1/pull/723).

Independently of this, we want to lower the default for test iters to 4, (but keep 64 on CI). (@sipa: "i don't think that running at more than 1 has ever actually contributed to a bug being found :p")

[hebasto]

In the specific case of ecmult_gen, this means we should just increase the default to 86 kib.

See https://github.com/bitcoin-core/secp256k1/pull/1564.