build: explicitly disable libsecp256k1 openssl based tests #23314

1. 
   fanquake commented at 2:20 am on October 20, 2021: member

   These tests are failing when run against OpenSSL 3, and have been removed upstream, bitcoin-core/secp256k1#983, so disabled them for now to avoid make check failures.

   Note that this will also remove warning output from our build, due to the use of deprecated OpenSSL API functions. See bitcoin#23048.

2. fanquake added the label Upstream on Oct 20, 2021
3. fanquake added the label DrahtBot Guix build requested on Oct 20, 2021
4. 
   fanquake commented at 2:20 am on October 20, 2021: member

   cc @real-or-random @jonasnick
5. fanquake referenced this in commit 1483a9c579 on Oct 20, 2021
6. 
   sipa commented at 3:06 am on October 20, 2021: member

   No objection to updating to the current master branch.

   Do be aware that due to bitcoin-core/secp256k1#956 a table that was previously built at runtime, is now precomputed as part of the source code. That’s 2.4 MB of C code being added to the repository, and 0.5 MiB added to the binary size.

   I don’t expect that to be a problem, but if the binary size increase is an issue, it can be reduced with a configure flag (with a mild validation performance reduction).

7. 
   MarcoFalke commented at 7:12 am on October 20, 2021: member

   If you are going to backport a --disable-openssl-tests to 22.x, wouldn’t a cleaner first patch be to submit --disable-openssl-tests to master? This would also avoid a successive bump in a short time.
8. 
   fanquake commented at 7:28 am on October 20, 2021: member

   If you are going to backport a –disable-openssl-tests to 22.x

   It wasn’t going be to a backport, as I would rather make the subtree update here, but we can always include that commit here, and then do a subtree update after.

   This would also avoid a successive bump in a short time.

   It’s not clear how soon that bump would be, and it could well be too late for 23.0.

9. 
   MarcoFalke commented at 7:31 am on October 20, 2021: member

   Is there anything in the bump that we need, assuming the test issue is fixed with a configure flag? Especially, since you are planning another bump after bitcoin-core/secp256k1#988 (soon?).
10. 
    real-or-random commented at 8:05 am on October 20, 2021: member

    No objection to updating to the current master branch.

    Same here.

    Is there anything in the bump that we need, assuming the test issue is fixed with a configure flag?

    I don’t think so.

11. 
    MarcoFalke commented at 8:13 am on October 20, 2021: member

    Also, there was discussion about a 1.0 release of libsecp256k1, so I am wondering if we should wait for that as well
12. build: explicitly disable libsecp256k1 openssl based tests

    These tests are failing when run against OpenSSL 3, and have been
    removed upstream, https://github.com/bitcoin-core/secp256k1/pull/983, so
    disabled them for now to avoid `make check` failures.
    
    Note that this will also remove warning output from our build, due to
    the use of deprecated OpenSSL API functions. See #23048.

    d7524546ab
13. fanquake force-pushed on Oct 20, 2021
14. 
    fanquake commented at 8:18 am on October 20, 2021: member

    Ok. I’ve changed this to just disable the tests for now, as I would like to get this merged and backported into 22.x. I’ll update #23315 to be a proper backport after this has been merged.
15. 
    MarcoFalke commented at 8:32 am on October 20, 2021: member

    cr ACK d7524546abf1fa5be8e6317ee50585e966ae6b4c

    Didn’t test on Alpine or elsewhere.

16. MarcoFalke renamed this:
    upstream: libsecp256k1 subtree update
    build: explicitly disable libsecp256k1 openssl based tests
    on Oct 20, 2021
17. 
    elichai commented at 8:41 am on October 20, 2021: contributor

    Code review ACK d7524546abf1fa5be8e6317ee50585e966ae6b4c
18. 
    real-or-random commented at 9:04 am on October 20, 2021: member

    Ok. I’ve changed this to just disable the tests for now,

    Concept ACK

19. 
    real-or-random commented at 9:09 am on October 20, 2021: member

    Also, there was discussion about a 1.0 release of libsecp256k1, so I am wondering if we should wait for that as well

    This would be a nice goal in order to create pressure on us to do a release. But my more pessimistic internal voice tells me that https://github.com/bitcoin-core/secp256k1/pull/988 will be in before we have a release. Anyway, we should seriously do a release!

20. laanwj merged this on Oct 20, 2021
21. laanwj closed this on Oct 20, 2021

    ---

22. laanwj added the label Needs backport (22.x) on Oct 20, 2021
23. laanwj removed the label Needs backport (22.x) on Oct 20, 2021
24. MarcoFalke removed the label DrahtBot Guix build requested on Oct 20, 2021
25. sidhujag referenced this in commit f02b55f4d1 on Oct 20, 2021
26. fanquake deleted the branch on Oct 20, 2021
27. fanquake referenced this in commit e959b46aa9 on Oct 20, 2021
28. 
    fanquake commented at 11:51 pm on October 20, 2021: member

    Backported in #23315.
29. MarcoFalke referenced this in commit 56156a1f08 on Oct 21, 2021
30. luke-jr referenced this in commit 14eac26e30 on Dec 14, 2021
31. fanquake referenced this in commit c06cda3e48 on Dec 18, 2021
32. knst referenced this in commit 6f1d324aa0 on Jul 27, 2022
33. UdjinM6 referenced this in commit a6e9e50845 on Jul 27, 2022
34. DrahtBot locked this on Oct 30, 2022

Contributors
fanquake sipa MarcoFalke real-or-random elichai

Labels
Upstream