ci: re-add Valgrind job to the CI

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

DrahtBot added the label Tests on Sep 17, 2025

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.

fanquake marked this as a draft on Sep 17, 2025

fanquake marked this as ready for review on Sep 17, 2025

fanquake marked this as a draft on Sep 17, 2025

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

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.

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:

fanquake force-pushed on Sep 18, 2025

fanquake marked this as ready for review on Sep 18, 2025

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.

fanquake force-pushed on Sep 19, 2025

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.

fanquake force-pushed on Oct 8, 2025

ci: re-add Valgrind job to the CI 4914d86251

fanquake force-pushed on Oct 15, 2025

in ci/test/00_setup_env_native_valgrind.sh:16 in 1aaf9a102e

12@@ -13,7 +13,7 @@ export PIP_PACKAGES="--break-system-packages pycapnp"
13 export USE_VALGRIND=1
14 export NO_DEPENDS=1
15 # bind tests excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
16-export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra"
17+export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra,wallet_miniscript"

maflcko commented at 4:26 pm on October 15, 2025:

Excluding a test that is expected to pass under valgrind (without even documenting why it is excluded) also makes the test less useful when run locally, assuming that it covers all tests

fanquake commented at 4:33 pm on October 15, 2025:

Happy to document that it is excluded because it is very slow compared to all other tests. Also happy to re-enable it, if we are fine to add ~13 minutes of runtime to this job, for a single slow test (it’d be about on par with ASAN at that point, so I guess still reasonable).

fanquake commented at 11:25 am on October 16, 2025:

Just dropped the exclusion.

fanquake force-pushed on Oct 16, 2025

maflcko commented at 11:44 am on October 16, 2025: member

Again, this seems fine, and I don’t want to raise an objection. However, the general question of how to deal with the false-positives (see my previous comment) here is still unanswered. IIUC, valgrind may not work under a given compiler (version). For example, currently it doesn’t work under clang out of the box. There are workarounds needed, but they come with drawbacks:

Add a suppression, but if this one is too broad, it decreases the value of running valgrind
Use -O1, but valgrind is already slow, and this will make it even slower
Refactor the code somehow randomly, until it isn’t hit, but that is tedious and unclear, because it isn’t trivially actionable

Generally, I have the impression that the existing asan and msan task find all memory issues before valgrind could find them. If there was one, it would be good to know it.

fanquake commented at 2:33 pm on October 21, 2025: member

Can leave it to *san for now.

fanquake closed this on Oct 21, 2025

maflcko commented at 2:37 pm on October 21, 2025: member

Maybe we can add it to just run under the default branch (master) on merges? Brought up in the context of #33509 (comment), but i guess the same approach could be used for several tasks? Maybe a brainstorming issue?

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

Code Coverage & Benchmarks

Reviews

Conflicts