ci: re-add Valgrind job to the CI #33411

1. 
   fanquake commented at 9:26 am on September 17, 2025: member

   Now that we’ve migrated to the new CI setup, and runtimes are looking decent, we could re-add some of the nightly/jobs runs in other repos, back to the main CI. This also came up in #33201 (review).
2. DrahtBot added the label Tests on Sep 17, 2025
3. 
   DrahtBot commented at 9:26 am on September 17, 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/33411.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	willcl-ark

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

   Conflicts

   No conflicts as of last run.

4. fanquake marked this as a draft on Sep 17, 2025
5. fanquake marked this as ready for review on Sep 17, 2025
6. fanquake marked this as a draft on Sep 17, 2025
7. 
   fanquake commented at 10:20 am on September 17, 2025: member

   The runtime isn’t too bad, and it’s just blown out by wallet_miniscript.py, which is ~4 times slower than the next slowest running test (signet_miner.py):

   0tool_signet_miner.py                                   | ✓ Passed  | 536 s
   1wallet_miniscript.py                                   | ✓ Passed  | 2086 s
   

8. 
   fanquake commented at 10:48 am on September 17, 2025: member

   With wallet_miniscript skipped, this finishes faster than the ASAN job, and ~roughly the same time as msan & fuzzer.
9. 
   willcl-ark commented at 2:31 pm on September 17, 2025: member

   Concept ACK to re-introducing this job.

   Wondering whether it’s worth adding an extra runner if we add a new lg-sized job? On the whole CI queue sizes/times have been reasonable I think since the migration, and we did reduce a few job sizes recently. But still might be worth considering.

   The 16th seemed unusually active, most days top out around 20 min max queue time AFAIK, but for reference here’s the previous 7 days:

10. fanquake force-pushed on Sep 18, 2025
11. fanquake marked this as ready for review on Sep 18, 2025
12. 
    fanquake commented at 9:37 am on September 18, 2025: member

    Adjusted to remove the conflict with #31425 and add context to the final commit.
13. ci: re-add Valgrind job to the CI 9ca0092e84
14. ci: exclude wallet_miniscript from valgrind ci

    It runs ~4x slower than the next slowest job (~2086s), `tool_signet_miner.py ` (~536s)
    which greatly increases the job runtime.

    9b32a5f96a
15. fanquake force-pushed on Sep 19, 2025
16. 
    maflcko commented at 9:13 am on September 23, 2025: member

    No objection, but are there any real issues that this task has found in the last years? The only ones I am aware of are false-positives around valgrind and questions around updating the task config itself for a newly added package. If those are the only reasons to add it, the reasoning seems circular and pull request authors having to deal with valgrind false positives directly in their pull requests may even be counter-productive? For example, https://github.com/bitcoin/bitcoin/pull/31282/files was open for 4 months, and #29635 is still open with no clear solution. I am not sure if it is beneficial to block or delay a perfectly good pull requests based on a false-positive red valgrind CI.

Contributors
fanquake DrahtBot willcl-ark maflcko

Labels
Tests