Fix compilation when extrakeys module isn’t enabled #1574
pull jonasnick wants to merge 2 commits into bitcoin-core:master from jonasnick:fix-extrakeys changing 6 files +48 −32-
jonasnick commented at 7:40 am on July 22, 2024: contributorFixes #1573. This wasn’t caught by CI because the extrakeys was always enabled. I added a commit that changes that.
-
jonasnick force-pushed on Jul 22, 2024
-
jonasnick commented at 9:30 am on July 22, 2024: contributorHm, I have no clue why the
macOS Sonoma (int128_struct, 2, 4)
CI job fails while the same settings work onmacOS Monterey
under valgrind in CI. -
bitcoin-core deleted a comment on Jul 22, 2024
-
tests: do not use functions from extrakeys module
This fixes a bug introduced in 7d2591ce12d8a9b85f210cf9d678e91cee125ee9 that prevented compiling the library without enabling the extrakeys module.
-
ci: only enable extrakeys module when schnorrsig is enabled 763d938cf0
-
jonasnick force-pushed on Jul 24, 2024
-
jonasnick commented at 10:06 am on July 24, 2024: contributorThe CI job failure doesn’t seem to go away by rerunning the job.
-
sipa commented at 5:13 pm on July 25, 2024: contributorBizarre.
-
jonasnick commented at 12:43 pm on July 26, 2024: contributor
To investigate the CI failure, I played around on my fork but I couldn’t reproduce the error anymore. The reported clang versions differed:
- Apple clang version 15.0.0 (clang-1500.0.40.1) in the failed CI job
- Apple clang version 15.0.0 (clang-1500.3.9.4) in my fork’s successful CI job
I then restarted the job here, which resulted in the newer clang version being reported and the failure is gone.
-
sipa commented at 12:46 pm on July 26, 2024: contributorACK 763d938cf0e68ef6dc52fda4f45cc03c5d2e31f0
-
real-or-random added the label bug on Jul 29, 2024
-
real-or-random added the label build on Jul 29, 2024
-
real-or-random added the label ci on Jul 29, 2024
-
hebasto approved
-
hebasto commented at 4:47 pm on July 29, 2024: member
ACK 763d938cf0e68ef6dc52fda4f45cc03c5d2e31f0.
In the af551ab9db09a3e4f2d06f68cf3e140fb1acfc4b commit message:
This fixes a bug introduced in 7d2591ce12d8a9b85f210cf9d678e91cee125ee9 that prevented compiling the library without enabling the extrakeys module.
it seems that “compiling the tests” suits better than “compiling the library”, doesn’t it?
Is a combination of the enabled extrakeys module with disabled schnorrsig module not worth testing on the CI at all?
-
jonasnick merged this on Jul 29, 2024
-
jonasnick closed this on Jul 29, 2024
-
jonasnick commented at 7:30 pm on July 29, 2024: contributor
Thanks for the review. @hebasto:
it seems that “compiling the tests” suits better than “compiling the library”, doesn’t it?
Slightly, yeah. But I don’t want to invalidate the ACKs.
Is a combination of the enabled extrakeys module with disabled schnorrsig module not worth testing on the CI at all?
Could be worth doing. We cannot really exhaustively check the entire configuration space, but for every module we could enable just that single module and all of its dependencies.
-
real-or-random commented at 11:25 am on July 30, 2024: contributor
We cannot really exhaustively check the entire configuration space
We could generate a handful of random configs in every CI run. I think this will be very nice, but it’s not entirely trivial. We currently set the config statically in CI, so this will need some thoughts. Perhaps a wrapper script that inspects the config env variables. If a variable is set to
MAGICRANDOM
, it will replace it by a random value. -
gmaxwell commented at 0:38 am on August 1, 2024: contributor
Thanks for addressing this. I wouldn’t worry about the library vs test, sorry for encouraging that presentation– but also I don’t think it’s fair to consider the library “working” in any case where the test doesn’t, so the comment isn’t wrong it’s just imprecise. :D
while strange config/config interactions are possible, there is a small set of tests with a disproportionate sensitivity: In particular, testing with all options disabled catches cases where an option was accidentally not optional. Testing with all options enable catches cases where options were accidentally mutually exclusive and where an option just doesn’t work. And testing each option one at a time (plus their known and intended dependencies) catches where options are unintentionally dependent.
This set is linear in the number of options (2+n), and I’d bet it captures almost all option interaction bugs that you would see in practice. So long as the all-on and all-off cases are tested on all platforms I suspect there isn’t a need to test all the one-hot options on all platforms– simply because the accidental dependency cases are less likely to be platform specific at least given the structure of libsecp256k1.
Testing other cases at random wouldn’t hurt, but an ordinary random selection would be somewhat unlikely to try any of these particularly sensitive configurations (in fact, exponentially unlikely in the number of options…). The configurations with almost all on or almost all off are the most likely to be interesting.
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-06 05:15 UTC
More mirrored repositories can be found on mirror.b10c.me