Fix compilation when extrakeys module isn't enabled

[jonasnick]

Fixes #1573. This wasn't caught by CI because the extrakeys was always enabled. I added a commit that changes that.

[jonasnick]

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.

[jonasnick]

The CI job failure doesn't seem to go away by rerunning the job.

[sipa]

Bizarre.

[jonasnick]

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]

ACK 763d938

[hebasto]

ACK 763d938.

In the af551ab commit message:

This fixes a bug introduced in 7d2591c 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]

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]

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]

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.