test: Run all benchmarks in the sanity check #32310

1. 
   maflcko commented at 11:58 am on April 19, 2025: member

   It is unclear why not all benchmarks are run, given that:

   • they only run as a sanity check (fastest version)
   • no one otherwise runs them, not even CI
   • issues have been missed due to this
2. 
   DrahtBot commented at 11:58 am on April 19, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32310.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	l0rinc, BrandonOdiwuor
   Concept ACK	i-am-yuvi
   Stale ACK	hebasto

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Apr 19, 2025
4. hebasto approved
5. 
   hebasto commented at 11:59 am on April 19, 2025: member

   ACK fa2a38a8a5a6b4130b9796df91596964d882ddfa, I have reviewed the code and it looks OK.
6. 
   i-am-yuvi commented at 6:00 pm on April 19, 2025: contributor

   Concept ACK

   Why is the CI failing?

7. 
   l0rinc commented at 6:51 pm on April 19, 2025: contributor

   Concept ACK, depends on #32309 to make the build pass
8. maflcko force-pushed on Apr 21, 2025
9. 
   maflcko commented at 7:03 am on April 22, 2025: member

   Why is the CI failing?

   Thx, excluded msvc for now, with the same approach as the temporary win-cross exclusion.

10. 
    hebasto commented at 4:44 pm on April 22, 2025: member

    Why is the CI failing?

    Thx, excluded msvc for now, with the same approach as the temporary win-cross exclusion.

    This can now be reverted after rebase.

11. test: Run all benchmarks in the sanity check faca46b042
12. maflcko force-pushed on Apr 22, 2025
13. 
    l0rinc commented at 7:05 pm on April 22, 2025: contributor

    ACK faca46b0421b568e7e5fefe593420e773d0ec9af
14. DrahtBot requested review from i-am-yuvi on Apr 22, 2025
15. DrahtBot requested review from hebasto on Apr 22, 2025
16. 
    maflcko commented at 7:22 pm on April 22, 2025: member

    For reference, in the two CI DEBUG=1 configurations, the bench part seems to take 3x the time. Though, this should be fine, given the motivation in the pull description, and also the fact that there are similarly slow other tests.
17. 
    BrandonOdiwuor commented at 8:13 pm on April 22, 2025: contributor

    Code Review ACK faca46b0421b568e7e5fefe593420e773d0ec9af
18. 
    fanquake commented at 9:15 am on April 23, 2025: member

    Locally the sanity bench (when running ctest) is taking ~19s, roughly the same time as the second worst secp256k1 test. Under the DEBUG build type it takes ~140s, the next longest running test is ~80s; so I imagine some devs will wonder why runing ctest is now taking > 2-3 minutes to complete. I checked that under Valgrind the runtime isn’t so long that it’d cause the job to fail due to timeout (although there might be a different issue causing that).

    I think ideally we could just delete these benchmarks that take so long to run, especially given that nobody is running them anyway, when changing the related code (#32149).

19. fanquake merged this on Apr 23, 2025
20. fanquake closed this on Apr 23, 2025

    ---

21. maflcko deleted the branch on Apr 23, 2025
22. 
    maflcko commented at 9:39 am on April 23, 2025: member

    Under the DEBUG build type it takes ~140s, the next longest running test is ~80s; so I imagine some devs will wonder why runing ctest is now taking > 2-3 minutes to complete.

    I wonder if this is relevant, given how long the functional tests take. And if someone doesn’t want to run all tests, they can trivially exclude all benchmarks via ctest --exclude-regex ....

    An alternative may be to allow running the individual benchmarks in parallel, similar to the unit tests.

23. 
    fanquake commented at 9:46 am on April 23, 2025: member

    I wonder if this is relevant, given how long the functional tests take.

    My assumption would be that developers are running the units tests regularly, far more often than they are running the entire functional test suite.

Contributors
maflcko DrahtBot hebasto i-am-yuvi l0rinc BrandonOdiwuor fanquake

Review Requested
hebasto i-am-yuvi

Labels
Tests