Fix compilation when extrakeys module isn’t enabled #1574

1. 
   jonasnick commented at 7:40 am on July 22, 2024: contributor

   Fixes #1573. This wasn’t caught by CI because the extrakeys was always enabled. I added a commit that changes that.
2. jonasnick force-pushed on Jul 22, 2024
3. 
   jonasnick commented at 9:30 am on July 22, 2024: contributor

   Hm, I have no clue why the macOS Sonoma (int128_struct, 2, 4) CI job fails while the same settings work on macOS Monterey under valgrind in CI.
4. bitcoin-core deleted a comment on Jul 22, 2024
5. 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.

   af551ab9db
6. ci: only enable extrakeys module when schnorrsig is enabled 763d938cf0
7. jonasnick force-pushed on Jul 24, 2024
8. 
   jonasnick commented at 10:06 am on July 24, 2024: contributor

   The CI job failure doesn’t seem to go away by rerunning the job.
9. 
   sipa commented at 5:13 pm on July 25, 2024: contributor

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

11. 
    sipa commented at 12:46 pm on July 26, 2024: contributor

    ACK 763d938cf0e68ef6dc52fda4f45cc03c5d2e31f0
12. real-or-random added the label bug on Jul 29, 2024
13. real-or-random added the label build on Jul 29, 2024
14. real-or-random added the label ci on Jul 29, 2024
15. hebasto approved
16. 
    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?

17. jonasnick merged this on Jul 29, 2024
18. jonasnick closed this on Jul 29, 2024

    ---

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

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

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

Contributors
jonasnick sipa hebasto real-or-random gmaxwell

Labels
bug ci build