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

real-or-random commented at 10:34 am on June 24, 2024: contributor

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, #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. :)

real-or-random added the label assurance on Jun 24, 2024

real-or-random added the label next-meeting on Jun 24, 2024

real-or-random added the label meta/development on Jun 24, 2024

hebasto commented at 10:45 am on June 24, 2024: member

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 commented at 10:55 am on June 24, 2024: contributor

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 commented at 2:22 pm on June 25, 2024: contributor

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 #813 (comment) 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 added the label build on Jun 28, 2024

real-or-random commented at 4:14 pm on July 1, 2024: contributor

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 #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 commented at 7:52 pm on July 1, 2024: member

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

See #1564.

real-or-random referenced this in commit ca06e58b2c on Jul 3, 2024

jonasnick closed this on Jul 9, 2024

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