ci, iwyu: Fix warnings in `src/kernel` and treat them as errors

hebasto commented at 5:03 pm on November 4, 2025: member

Now seems like a good time to update the includes in src/kernel.

DrahtBot commented at 5:03 pm on November 4, 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/33779.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, purpleKarrot, sedited
Stale ACK	l0rinc, willcl-ark

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34296 (refactor: [move-only] Merge core_io module, remove from libkernel by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto force-pushed on Nov 4, 2025

DrahtBot added the label CI failed on Nov 4, 2025

DrahtBot removed the label CI failed on Nov 4, 2025

hebasto marked this as ready for review on Nov 4, 2025

hebasto commented at 11:41 pm on November 4, 2025: member

There are no conflicts with other contributors’ PRs.

Friendly ping @l0rinc @maflcko @ryanofsky @willcl-ark who reviewed #31308, and @TheCharlatan, the kernel expert :)

hebasto added the label Refactoring on Nov 4, 2025

l0rinc commented at 11:43 pm on November 4, 2025: contributor

code review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

willcl-ark approved

willcl-ark commented at 10:46 am on November 5, 2025: member

ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

Indeed a good time to get this change in :)

maflcko commented at 1:47 pm on November 5, 2025: member

review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687 🐮

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687 🐮
3G2GSehSG/6BqX/m6sCiIHGIarE7ISW3dV0VxpX5fcM2EJ8hZOVOcNXSZRr7oRnrNufO91NK8KYEdhrrIaMbJAg==

sedited commented at 1:53 pm on November 5, 2025: contributor

I tried to reproduce what the CI is doing locally. Having to run the full CI either on push or locally just to fix the includes is not ideal. The following command seems to produce different results:

0/home/user/Downloads/include-what-you-use/iwyu_tool.py -p /home/user/bitcoin/build_dev_mode_clang/compile_commands.json src/kernel/chainparams.cpp -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=/home/user/bitcoin/contrib/devtools/iwyu/bitcoin.core.imp -Xiwyu --max_line_length=160
1
2(/home/user/bitcoin/src/kernel/chainparams.h has correct #includes/fwd-decls)
3
4/home/user/bitcoin/src/kernel/chainparams.cpp should add these lines:
5#include <functional>                  // for identity, equal_to

The compile commands are configured for clang-21 and I’m using iwyu v0.25. It also seems like we are not configuring yet for the kernel library headers and implementations.

maflcko commented at 2:07 pm on November 5, 2025: member

different results:

I wonder if it comes from different libstdc++ header versions used. Generally, I could imagine it being difficult to achieve fully reproducible and consistent results across all build configurations. If relying on the CI (locally or remote) is too tedious, then it probably doesn’t make sense to treat iwuy suggestions as errors.

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors? cc @willcl-ark @purpleKarrot

willcl-ark commented at 2:15 pm on November 5, 2025: member

Having to run the full CI either on push or locally just to fix the includes is not ideal.

Something I was thinking about while review the first part of this PR was whether using CMake’s CXX_INCLUDE_WHAT_YOU_USE ${iwyu_path} to run this as another configuration “natively” with cmake. The issue here is it’s project-wide, and you have to set SKIP_LINTING on those files you don’t want it run on. Then we could perhaps provide a preset to run iwyu or something.

I tried implementing that but either must be set per file (verbose) or on a per-target basis, and the resulting cmake ended up being messier than the jq filtering we do currently, so I gave up experimenting with that.

It also probably wouldn’t help the repro issues @TheCharlatan notes.

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors

Would be possible relatively easily, I think, if there’s interest.

maflcko commented at 2:22 pm on November 5, 2025: member

Possibly we could spin up a separate CDash to track them externally (outside this project) as warnings or errors

Would be possible relatively easily, I think, if there’s interest.

Nice. I was thinking that maybe the CI in this repo could have an auth token to push the iwyu for all builds to the dashboard. This way, it could be easier to check the warnings/error for each pull request, without it being a merge blocker. Also, there could be regular “fix iwyu” pull requests to bring the warnings/errors down, with an easy way to check via the dashboard as well.

fanquake commented at 2:22 pm on November 5, 2025: member

Would be possible relatively easily, I think, if there’s interest.

I’m wondering what problem we are trying to solve. Externalizing this would just lead to random PRs to this repo, to “fix” includes, which can’t be verified here? If developers aren’t actively engaged in actually maintaining includes and no CI will ever fail, that sounds like endless churn. It’s also unclear why this needs to be in some other dashboard, if we can have the infra/result determined here already?

sedited commented at 2:26 pm on November 5, 2025: contributor

Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.

maflcko commented at 2:40 pm on November 5, 2025: member

I’m wondering what problem we are trying to solve.

I think the problem is that it is inherently impossible to reproduce an iwyu run locally without the exact CI config. Maybe this is fine, and nothing needs to be fixed. Though, if devs think that relying exactly on the CI is too tedious, then a dashboard could seem like a nice solution.

endless churn

I think iwyu will mean churn, regardless of how it is enforced, if it is run on more actively changed modules of the codebase.

I guess this gives a third alternative: Only enforce IWYU pre branch-off, so that there is ideally only a single “fix” pull per release and all released versions are iwyu-clean. Post branch-off the IWYU enforcement could be turned-off, so that developers can just continue to develop normally, like before, without having to worry about tedious IWYU fixups for every single pull request and commit.

Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.

Looking at the runtimes here, it seems iwyu takes 9 minutes alone, so with the configure overhead, it will likely be hard to get under 10 minutes. It is less than the 19 minutes the full task takes with clang-tidy, but there is probably a limit to how fast it can be made.

purpleKarrot commented at 8:01 am on November 6, 2025: contributor

Maybe a better approach would be to run the enforced sections in a separate, faster job?

With such a job, it is possible to copy the output and pipe it through fix_includes.py locally. Full reproducibility is nice to have (and can be achieved with a dev container that contains all the necessary tools in the right version), but for a development workflow it is also possible to ignore IWYU (just rely on clangd’s recommendations to add/remove includes) and then fix deviations using the described approach when the PR job fails.

purpleKarrot commented at 8:03 am on November 6, 2025: contributor

ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

maflcko commented at 8:18 am on November 6, 2025: member

With such a job, it is possible to copy the output and pipe it through fix_includes.py locally.

For reference, if devs want to copy the output, the job already has the full diff, ready to apply. So there is no need to take an extra step. Example https://github.com/bitcoin/bitcoin/actions/runs/19076699870/job/54494252522?pr=33779#step:9:41045:

 0IWYU edited 873 files on your behalf.
 1
 2+ git --no-pager diff
 3diff --git a/src/addrdb.cpp b/src/addrdb.cpp
 4index 129bbf2..5ecfbf6 100644
 5--- a/src/addrdb.cpp
 6+++ b/src/addrdb.cpp
 7@@ -4,20 +4,14 @@
 8 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 9 
10 #include <bitcoin-build-config.h> // IWYU pragma: keep
11-
12 #include <addrdb.h>
13-
14 #include <addrman.h>
15 #include <chainparams.h>
16-#include <clientversion.h>
17 #include <common/args.h>
18 #include <common/settings.h>
19-#include <cstdint>
20 #include <hash.h>
21 #include <logging.h>
22 #include <logging/timer.h>
23-#include <netbase.h>
24-#include <netgroup.h>
25 #include <random.h>
26 #include <streams.h>
27 #include <tinyformat.h>
28@@ -26,6 +20,23 @@
29 #include <util/fs_helpers.h>
30 #include <util/syserror.h>
31 #include <util/translation.h>
32+#include <errno.h>
33+#include <stdio.h>
34+#include <cstdint>
35+#include <algorithm>
36+#include <array>
37+#include <exception>
38+#include <map>
39+#include <span>
40+#include <stdexcept>
41+#include <string>
42+
43+#include "net_types.h"
44+#include "protocol.h"
45+#include "serialize.h"
46+#include "uint256.h"
47+#include "util/result.h"
48+#include "util/time.h"
49 
50 namespace {

hebasto commented at 11:49 am on November 6, 2025: member

Having to run the full CI either on push or locally just to fix the includes is not ideal.

Something I was thinking about while review the first part of this PR was whether using CMake’s CXX_INCLUDE_WHAT_YOU_USE ${iwyu_path} to run this as another configuration “natively” with cmake. The issue here is it’s project-wide, and you have to set SKIP_LINTING on those files you don’t want it run on. Then we could perhaps provide a preset to run iwyu or something.

A separate job running IWYU on a compilation database could be quite fast, as we’d only need to build the codegen target (though it requires a fix for the Qt code). That’s a plan for the future.

fanquake commented at 12:15 pm on November 6, 2025: member

For reference, if devs want to copy the output, the job already has the full diff, ready to apply.

I don’t think this diff is “ready to apply”? Headers like errno.h are wrong, and should be <cerrno> (this is enforced by modernize-deprecated-headers in the tidy job)? Includes like #include "net_types.h" are also wrong, and should be #include <net_types.h>?

Given that developers working on this code, have said that this change will likely make development more difficult, should be reason enough to fix the developer workflows/tooling (or at least decide on a the fix) before doing anything here; especially given that the value add of “fixing includes” is trending towards 0, compared to developers actually getting work done/wasting time fighting with CIs. This problem will only grow as this starts to cover more of the codebase, and effect more devs, so it seems like a good time to figure out a solution.

Currently solutions include:

Add a new dashboard somewhere. This needs to be created, and maintained (externally) by someone, and doesn’t seem to solve the problem of developers being able to easily run the same checks locally. Its not clear if that dashboard would post immediately applyable diffs to PRs (updated on every push), when CI turns red here, or developers would need to check the dashboard to find the diff to fix their PR?
Add a new (faster) CI job here. If the code for this CI job can be easily ran locally (like the lint job), and produce a diff that can be immediately applied (see above for why current output does not seem to be that) then this could be a good solution, assuming the diff is easily surfaced.
Leave things as they are. Developers can open a PR, and see if the CI turns green. If not, they can scroll through ~68'000 lines of CI output, to extract a diff, potentially (see above) modify the diff so it can be applied, then push again.

maflcko commented at 12:27 pm on November 6, 2025: member

Leave things as they are. Developers can open a PR, and see if the CI turns green. If not, they can scroll through ~68'000 lines of CI output, to extract a diff, potentially (see above) modify the diff so it can be applied, then push again.

The massive output is just the iwyu warning/debug output. On real pull requests that run into errors, the diff should be smaller. (Should be easy to test by pushing this pr’s ci changes without the code changes)

I don’t think this diff is “ready to apply”? Headers like errno.h are wrong, and should be <cerrno> (this is enforced by modernize-deprecated-headers in the tidy job)? Includes like #include "net_types.h" are also wrong, and should be #include <net_types.h>?

Yeah, good points. The sorting is also wrong, which may be good to fix in iwyu, or in clang-format (c.f. #32813 (review))

hebasto commented at 11:31 pm on November 6, 2025: member

Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.

Done in #33810.

fanquake commented at 2:21 pm on December 16, 2025: member

Moved this to draft while we first figure out #33810.

fanquake marked this as a draft on Dec 16, 2025

hebasto force-pushed on Dec 16, 2025

DrahtBot added the label CI failed on Dec 16, 2025

DrahtBot commented at 4:20 pm on December 16, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20272513332/job/58211985526 LLM reason (✨ experimental): Lint failure: includes use quotes instead of brackets, triggering lint-includes.py and non-zero exit from the linter.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hebasto force-pushed on Dec 16, 2025

DrahtBot removed the label CI failed on Dec 16, 2025

maflcko commented at 9:02 am on December 22, 2025: member

It could make sense to go with #33810 first, but this lgtm:

re-ACK d5a62a288fba2b1d69325623e0640d1f33af58b8 🚢

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK d5a62a288fba2b1d69325623e0640d1f33af58b8 🚢
3Ja6xcEDdUUI3zhnDduwv6nFpS5ep2ifXdFKsXJYkTEoXQYxqD06sAuO1NSTf57ZiwYCuG9ru6rhBsMLBhgWgDw==

DrahtBot requested review from l0rinc on Dec 22, 2025

DrahtBot requested review from willcl-ark on Dec 22, 2025

DrahtBot requested review from purpleKarrot on Dec 22, 2025

DrahtBot added the label Needs rebase on Dec 22, 2025

hebasto referenced this in commit 5bbc7c8cc1 on Dec 22, 2025

hebasto force-pushed on Dec 22, 2025

hebasto marked this as ready for review on Dec 22, 2025

hebasto commented at 10:49 pm on December 22, 2025: member

Rebased and undrafted.

fanquake commented at 10:51 pm on December 22, 2025: member

Has #34079 (review) been reported upstream?

DrahtBot removed the label Needs rebase on Dec 22, 2025

hebasto commented at 11:25 pm on January 2, 2026: member

Has #34079 (comment) been reported upstream?

~~While working on reducing the code, I arrived at a different conclusion.~~

UPDATE: See https://github.com/include-what-you-use/include-what-you-use/issues/1886.

in src/util/translation.h:84 in cd95fbdf12

79@@ -80,6 +80,9 @@ consteval auto _(util::TranslatedLiteral str) { return str; }
80 
81 /** Mark a bilingual_str as untranslated */
82 inline bilingual_str Untranslated(std::string original) { return {original, original}; }
83+template <std::size_t N>
84+inline bilingual_str Untranslated(const char (&original)[N]) { return Untranslated(std::string{original}); }

maflcko commented at 7:46 am on January 15, 2026:

not sure about this. I don’t think we should be adding c++ code as an rough alternative to IWYU pragma export. Also, I am starting to think we should get rid of the pragma export to reduce the list of includes, because it doesn’t interact well with letting iwyu minimize the include list.

hebasto commented at 4:47 pm on January 15, 2026:

not sure about this. I don’t think we should be adding c++ code as an rough alternative to IWYU pragma export.

Not needing IWYU pragma: export is just a nice side effect. The actual goal of this change is to enforce explicit conversions and make the translation module self-contained.

maflcko commented at 8:59 pm on January 15, 2026:

I don’t think it is maintainable to add a redundant string literal overload for every function in the whole codebase that accepts a std::string as any arg.

hebasto commented at 10:35 pm on January 15, 2026:

I agree that the current approach is not ideal.

A better alternative for this specific case would be switching the parameter type to std::string_view. This solves the efficiency and IWYU issues without requiring a separate overload. WDYT?

Regarding the rest of the codebase, it might be best to evaluate cases individually. There are some tricky spots, such as using Assume() inside a macro, that might not fit a ‘one size fits all’ rule.

maflcko commented at 7:41 am on January 16, 2026:

A better alternative for this specific case would be switching the parameter type to std::string_view. This solves the efficiency and IWYU issues without requiring a separate overload. WDYT?

I don’t see an efficiency problem here, and i fail to see how either this patch, or string_view would solve it.

If you wanted to make it more efficient, you could write:

0inline bilingual_str Untranslated(std::string original) { return {original, std::move(original)}; }

However, I am not sure if this is worth it.

hebasto commented at 8:50 am on January 16, 2026:

using std::string:

 0$ cat a.h 
 1#include <string>
 2void f(std::string s) {}
 3$ cat test.cpp 
 4#include "a.h"
 5int main() { f("abc"); }
 6$ include-what-you-use test.cpp 
 7
 8test.cpp should add these lines:
 9#include <string>  // for basic_string
10
11test.cpp should remove these lines:
12
13The full include-list for test.cpp:
14#include <string>  // for basic_string
15#include "a.h"     // for f
16---

using std::string_view:

0$ cat a.h 
1#include <string_view>
2void f(std::string_view s) {}
3$ cat test.cpp 
4#include "a.h"
5int main() { f("abc"); }
6$ include-what-you-use test.cpp 
7
8(test.cpp has correct #includes/fwd-decls)

hebasto commented at 9:01 am on January 16, 2026:

I don’t see an efficiency problem here, and i fail to see how either this patch, or string_view would solve it.

I’m not saying that there is a problem that should be solved on its own. But wouldn’t that patch reduce the total number of heap allocations where SSO is not involved?

purpleKarrot commented at 9:04 am on January 16, 2026:

The use of IWYU pragma export should be limited to cases where you don’t want clients to import headers that you consider implementation details, like when you have thousand lines of templated code that you split across multiple files for better maintainability.

<string> is definitely a public header and including the header should be the only way to get std::string. Hence, #include <string> // IWYU pragma export is always wrong and should be verboten.

Whether it is worth avoiding #include <string> where std::string is only used indirectly, is debatable. I would just keep it simple. IWYU is not perfect and we have unnecessary includes anyway where allocators and type traits are used indirectly. I would just drop cd95fbdf12ccbaf4291d0d4fe3dca5579e18aab6.

maflcko commented at 9:36 am on January 16, 2026:

I guess I agree on the pragma export. If someone wanted to revert that, the diff would be:

 0diff --git a/src/util/strencodings.h b/src/util/strencodings.h
 1index 0106385804..b070e5b8c4 100644
 2--- a/src/util/strencodings.h
 3+++ b/src/util/strencodings.h
 4@@ -9,7 +9,7 @@
 5 #ifndef BITCOIN_UTIL_STRENCODINGS_H
 6 #define BITCOIN_UTIL_STRENCODINGS_H
 7 
 8-#include <crypto/hex_base.h> // IWYU pragma: export
 9+#include <crypto/hex_base.h>
10 #include <span.h>
11 #include <util/string.h>
12 
13@@ -20,8 +20,8 @@
14 #include <cstdint>
15 #include <limits>
16 #include <optional>
17-#include <string>      // IWYU pragma: export
18-#include <string_view> // IWYU pragma: export
19+#include <string>
20+#include <string_view>
21 #include <system_error>
22 #include <type_traits>
23 #include <vector>
24diff --git a/src/util/string.h b/src/util/string.h
25index 330c2a2a61..2578e530af 100644
26--- a/src/util/string.h
27+++ b/src/util/string.h
28@@ -12,8 +12,8 @@
29 #include <cstring>
30 #include <locale>
31 #include <sstream>
32-#include <string>      // IWYU pragma: export
33-#include <string_view> // IWYU pragma: export
34+#include <string>
35+#include <string_view>
36 #include <vector>
37 
38 namespace util {

(and then fixup the iwyu complaints)

hebasto commented at 2:21 pm on January 16, 2026:

@purpleKarrot @maflcko

Thank you for sharing your thoughts!

The use of IWYU pragma export should be limited…

I guess I agree on the pragma export.

Please see #34319.

hebasto commented at 2:28 pm on January 16, 2026:

I would just drop cd95fbd.

Thanks! Done.

maflcko commented at 7:46 am on January 15, 2026: member

8f2ac6f7efb7e5963436fd59462631888fe8afa3 🍯

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: 8f2ac6f7efb7e5963436fd59462631888fe8afa3 🍯
3WmSu1uh1MmK1km0uU7PCiDe6qvuFJn6vQDZuhXR2o2ovMY3KpXg9izWY29/uzlAXLHeJ1ebkkWztDy/jzhiyBg==

ci, iwyu: Fix warnings in `src/kernel` and treat them as errors a5a8c4139c

hebasto force-pushed on Jan 16, 2026

hebasto commented at 2:27 pm on January 16, 2026: member

Rebased. The recent feedback from @purpleKarrot and @maflcko has been addressed.

maflcko commented at 2:38 pm on January 16, 2026: member

review ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8 🍱

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8 🍱
3cUKn0ukCK/Ijewc2lV3kMxUywuKELK0OdeBgoezaO9HRXzKWjlU9p9f0gf793w9ZpPe0+tcjayiUVAreTd6PCw==

purpleKarrot commented at 8:04 am on January 19, 2026: contributor

ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8

sedited approved

sedited commented at 11:19 am on January 19, 2026: contributor

ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8

sedited merged this on Jan 19, 2026

sedited closed this on Jan 19, 2026

fanquake referenced this in commit c84c752506 on Jan 20, 2026

hebasto deleted the branch on Jan 20, 2026

ci, iwyu: Fix warnings in src/kernel and treat them as errors #33779

Code Coverage & Benchmarks

Reviews

Conflicts

ci, iwyu: Fix warnings in `src/kernel` and treat them as errors #33779