Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
real-or-random opened this issue Jun 24, 2024 · 5 comments · Fixed by #1563
Closed

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

real-or-random opened this issue Jun 24, 2024 · 5 comments · Fixed by #1563
Labels
assurance build meta/development processes, conventions, developer documentation, etc. next-meeting

Comments

@real-or-random
Copy link
Contributor

real-or-random commented Jun 24, 2024

Quoting myself from #1543 (comment):

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 (hebasto/bitcoin#192 (comment), #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 real-or-random added assurance next-meeting meta/development processes, conventions, developer documentation, etc. labels Jun 24, 2024
@hebasto
Copy link
Member

hebasto commented Jun 24, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

hebasto commented Jul 1, 2024

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

See #1564.

real-or-random added a commit that referenced this issue Jul 3, 2024
…ble for signing

e2af491 ci: Switch to the new default value of the precomputed table for signing (Hennadii Stepanov)
d94a927 build: Adjust the default size of the precomputed table for signing (Hennadii Stepanov)

Pull request description:

  This PR implements the [outcomes](#1549 (comment)) from today's IRC meeting:

  1. The default size of the precomputed table for signing is now aligned with Bitcoin Core's [default](bitcoin/bitcoin@a057869).

  2. The default value in CI has been updated to reflect the new default.

ACKs for top commit:
  sipa:
    utACK e2af491
  real-or-random:
    utACK e2af491

Tree-SHA512: aa9db5bc2aec29a35a503a80617a4c096e9909648084fe1ce43b5dd7e74dd812e7642305bd5bc13eb581efc23f12904e200e13cb1a35955b773e05ab4f84be4e
fanquake added a commit to bitcoin/bitcoin that referenced this issue Aug 6, 2024
9ec776a Revert "build: pass --with-ecmult-gen-kb=86 to secp256k1" (fanquake)
41797f8 Squashed 'src/secp256k1/' changes from 4af241b320..642c885b61 (fanquake)

Pull request description:

  Updates the libsecp256k1 subtree to bitcoin-core/secp256k1@642c885 (which is the tag for the [`v0.5.1` release](https://github.com/bitcoin-core/secp256k1/releases/tag/v0.5.1)).
  Includes a handful of changes:
  * bitcoin-core/secp256k1#1551
  * bitcoin-core/secp256k1#1555
  * bitcoin-core/secp256k1#1563
  * bitcoin-core/secp256k1#1564
  * bitcoin-core/secp256k1#1565
  * bitcoin-core/secp256k1#1574

  Reverts a057869 given secps default has changed (bitcoin-core/secp256k1#1563):
  > As a rule of thumb, the default values for configuration options should target standard desktop machines and align with Bitcoin Core's defaults, and the tests should mostly exercise the default configuration (see [#1549](bitcoin-core/secp256k1#1549 (comment))).

ACKs for top commit:
  hebasto:
    ACK 9ec776a, I've reproduced the subtree update locally with the zero diff with this PR branch.

Tree-SHA512: 903ca0ff12dcc32b6cd86aee84e19de09803d35a1ee006ce890f3761dd27f1e96fe70c7bb4c279416a96ee57c406c9627614900f0ca6f76674c0088a3d270cd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assurance build meta/development processes, conventions, developer documentation, etc. next-meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants