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

hebasto commented at 3:38 pm on January 29, 2026: member

This PR continues the ongoing effort to enforce IWYU warnings.

See Developer Notes.

hebasto added the label Refactoring on Jan 29, 2026

hebasto added the label Utils/log/libs on Jan 29, 2026

DrahtBot commented at 3:39 pm on January 29, 2026: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34465 (refactor: separate log generation from log handling by ryanofsky)

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 marked this as a draft on Jan 29, 2026

fanquake commented at 3:42 pm on January 29, 2026: member

0In file included from /home/admin/actions-runner/_work/_temp/src/hash.h:16:
1/home/admin/actions-runner/_work/_temp/src/uint256.h:150:17: error: no member named 'HexDigit' in the global namespace
2  150 |         *p1 = ::HexDigit(str[--digits]);
3      |                 ^~~~~~~~
4/home/admin/actions-runner/_work/_temp/src/uint256.h:152:38: error: no member named 'HexDigit' in the global namespace
5  152 |             *p1 |= ((unsigned char)::HexDigit(str[--digits]) << 4);
6      |                                      ^~~~~~~~
72 errors generated.

DrahtBot added the label CI failed on Jan 29, 2026

DrahtBot commented at 3:48 pm on January 29, 2026: contributor

🚧 At least one of the CI tasks failed. Task Valgrind, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/21484505843/job/61889746047 LLM reason (✨ experimental): Compilation failed due to missing #include causing std::vector to be undefined in string.h and related code.

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.

fanquake commented at 4:33 pm on January 29, 2026: member

Seems pretty odd that IWYU can be “passing” here, but nothing actually compiles.

hebasto force-pushed on Jan 29, 2026

DrahtBot removed the label CI failed on Jan 30, 2026

in contrib/devtools/iwyu/bitcoin.core.imp:13 in 2298908c3f outdated

 7@@ -8,13 +8,16 @@
 8   # libc symbols.
 9   { "symbol": ["AT_HWCAP", "private", "<sys/auxv.h>", "public"] },
10   { "symbol": ["AT_HWCAP2", "private", "<sys/auxv.h>", "public"] },
11+  { "symbol": ["sched_param", "private", "<sched.h>", "public"] },

fanquake commented at 9:23 am on January 30, 2026:

Can you link to the upstream issue / changes for all of these as well?

hebasto commented at 11:58 am on January 30, 2026:

See #34460.

fanquake commented at 12:02 pm on January 30, 2026:

I don’t see sched_param mentioned in https://github.com/include-what-you-use/include-what-you-use/issues/1809. Are you opening separate issues for different symbols, or just adding new missing symbols to https://github.com/include-what-you-use/include-what-you-use/issues/1809? Any reason to not add the missing mappings upstream; otherwise it seems like we are just accumulating workarounds for things that are not being fixed.

hebasto commented at 3:48 pm on January 30, 2026:

I don’t see sched_param mentioned in include-what-you-use/include-what-you-use#1809.

An upstream fix has been proposed in https://github.com/include-what-you-use/include-what-you-use/pull/1918.

hebasto commented at 8:00 am on January 31, 2026:

Any reason to not add the missing mappings upstream; otherwise it seems like we are just accumulating workarounds for things that are not being fixed.

Fixed upstream in https://github.com/include-what-you-use/include-what-you-use/pull/1923.

in contrib/devtools/iwyu/bitcoin.core.imp:13 in 2298908c3f

 7@@ -8,13 +8,16 @@
 8   # libc symbols.
 9   { "symbol": ["AT_HWCAP", "private", "<sys/auxv.h>", "public"] },
10   { "symbol": ["AT_HWCAP2", "private", "<sys/auxv.h>", "public"] },
11+  { "symbol": ["sched_param", "private", "<sched.h>", "public"] },
12 
13   # Fixed in https://github.com/include-what-you-use/include-what-you-use/pull/1706.

fanquake commented at 9:25 am on January 30, 2026:

Fixed in https://github.com/include-what-you-use/include-what-you-use/pull/1706.

This was fixed in IWYU 0.25, which we are using, so this whole block should be able to be dropped?

hebasto commented at 11:58 am on January 30, 2026:

Thanks! Dropped in #34460.

in src/util/batchpriority.cpp:9 in 2298908c3f outdated

 8 #include <util/syserror.h>
 9 
10+#include <string>
11+
12 #if (defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__DragonFly__))
13-#include <pthread.h>

maflcko commented at 9:37 am on January 30, 2026:

this doesn’t look like an iwyu change, which only runs on linux, and neither on freebsd, nor WIN32, so it seems best to commit this separately

hebasto commented at 3:14 pm on January 30, 2026:

See #34462.

hebasto force-pushed on Jan 30, 2026

DrahtBot added the label CI failed on Jan 30, 2026

DrahtBot removed the label CI failed on Jan 30, 2026

achow101 referenced this in commit 5d2707307e on Jan 30, 2026

hebasto force-pushed on Jan 30, 2026

hebasto commented at 9:27 pm on January 30, 2026: member

Rebased on top of merged bitcoin/bitcoin#34454.

hebasto force-pushed on Jan 31, 2026

hebasto commented at 9:15 am on January 31, 2026: member

Rebased on top of merged bitcoin/bitcoin#34460.

hebasto force-pushed on Jan 31, 2026

hebasto commented at 10:38 am on January 31, 2026: member

Rebased on top of merged bitcoin/bitcoin#34462.

hebasto marked this as ready for review on Jan 31, 2026

hebasto force-pushed on Jan 31, 2026

hebasto commented at 2:10 pm on January 31, 2026: member

Rebased to resolve a conflict with merged bitcoin/bitcoin#34455.

in src/util/time.h:9 in d2a82d1eb5

5@@ -6,8 +6,11 @@
6 #ifndef BITCOIN_UTIL_TIME_H
7 #define BITCOIN_UTIL_TIME_H
8 
9+#include <compat/compat.h>

fanquake commented at 12:41 pm on February 2, 2026:

Why is compat.h being added here (with sys/time.h being added to it), rather than just including sys/time.h here? It seems like compat.h is starting to become a catch-all for includes, which is going in the opposite of the direction we want (and defeats the point of IWYU).

hebasto commented at 12:49 pm on February 2, 2026:

Why is compat.h being added here (with sys/time.h being added to it), rather than just including sys/time.h here?

Without #include <compat/compat.h>, the code has to be:

0#ifdef WIN32
1#include <winsock2.h>
2#else
3#include <sys/time.h>
4#endif

This was actually my first iteration while working on this PR.

It seems like compat.h is starting to become a catch-all for includes, which is going in the opposite of the direction we want (and defeats the point of IWYU).

I agree. It would be great to have clearly documented use cases for compat.h.

fanquake commented at 12:52 pm on February 2, 2026:

Without #include <compat/compat.h>, the code has to be:

What is wrong with that code?

fanquake commented at 12:53 pm on February 2, 2026:

Also, the winsock2.h change seems unrelated, given IWYU is not run for Windows.

hebasto commented at 1:47 pm on February 2, 2026:

Also, the winsock2.h change seems unrelated, given IWYU is not run for Windows.

We still cross-compile for it and support native Windows builds. The winsock2.h header defines the timeval structure used here: https://github.com/bitcoin/bitcoin/blob/9f8764c814ead48d45b3822dfcc4cc2b3bda80d6/src/util/time.h#L151-L159

The code currently compiles without it only because of transitive includes, which is going in the opposite of the direction we want.

fanquake commented at 1:57 pm on February 2, 2026:

Sure, just pointing out that the (Windows) diff above would not actually be tested by, or related to any IWYU changes here (it would be good if you documented in commit mesages, when certain changes have been made because of Windows, rather than following the IWYU output). In any case, I don’t think the correct solution is to continue adding includes to compat, undermining IWYU.

maflcko commented at 2:19 pm on February 2, 2026:

Without #include <compat/compat.h>, the code has to be:

What is wrong with that code?

I think it is missing the IWYU export. Generally, those should be avoided, but when it comes to compat headers, I think there is a benefit to just have them imported once to get one symbol cross-platform and then IWYU export them.

Otherwise, all call-sites will have to add:

0#ifdef WIN32
1#include <winsock2.h>
2#else
3#include <sys/time.h>
4#endif

,which is verbose, and hard to review, and easy to get wrong: As you say iwyu is not run on Windows and the transitive include can hide a missing winsock2 include.

So I think the options are either:

keep the compat header with the iwyu export
add all platform-specific includes in all files that use them manually (tedious?)

fanquake commented at 2:27 pm on February 2, 2026:

Otherwise, all call-sites will have to add:

Isn’t this the only callsite though? I don’t really see why we’d add #include <sys/time.h> to compat.h, just to have it included hundreds of places it isn’t needed, for the sake of saving 1 #ifdef.

I think before we make any more changes, we need to decide if we are actually doing IWYU, or only when it’s convenient/not inconvenient for Windows, and otherwise bundling things in 1 header to include everywhere else.

hebasto commented at 2:35 pm on February 2, 2026:

Isn’t this the only callsite though?

0$ git grep -l timeval -- src ":(exclude)src/leveldb" ":(exclude)src/secp256k1"
1src/crypto/ctaes/bench.c
2src/httpserver.cpp
3src/httpserver.h
4src/randomenv.cpp
5src/torcontrol.cpp
6src/util/sock.cpp
7src/util/time.cpp
8src/util/time.h

fanquake commented at 2:40 pm on February 2, 2026:

Also, compat.h already includes sys/select.h, which also defines timeval. So why does sys/time.h need to be added to compat.h?

hebasto commented at 2:42 pm on February 2, 2026:

Alternatively, we can introduce a new compat_time.h header, which exports both winsock2.h and sys/time.h, and specify a new mapping in iwyu/bitcoin.core.imp:

0{ "symbol": ["timeval", "private", "<compat_time.h>", "public"] },

maflcko commented at 2:56 pm on February 2, 2026:

Isn’t this the only callsite though?

0$ git grep -l timeval -- src ":(exclude)src/leveldb" ":(exclude)src/secp256k1"
1src/crypto/ctaes/bench.c
2src/httpserver.cpp
3src/httpserver.h
4src/randomenv.cpp
5src/torcontrol.cpp
6src/util/sock.cpp
7src/util/time.cpp
8src/util/time.h

Right, my understanding is that all those would have to add the same

0#ifdef WIN32
1#include <winsock2.h>
2#else
3#include <sys/time.h>
4#endif

Alternatively, we can introduce a new compat_time.h header, which exports both winsock2.h and sys/time.h, and specify a new mapping in iwyu/bitcoin.core.imp:
0{ "symbol": ["timeval", "private", "<compat_time.h>", "public"] },

Yeah, that is a bit cleaner than the “iwyu export” hack. Maybe it can be named ./src/compat/time.h though?

fanquake commented at 3:14 pm on February 2, 2026:

What is the point of a new file, with a new mapping, if the symbol definition is already (correctly) coming from compat.h?

maflcko commented at 3:23 pm on February 2, 2026:

Not sure how heavy the full compat net stack headers are to parse every time util/time.h is included. It may be faster to just include what is needed via compat/time.h instead of compat/compat.h, but I haven’t benchmarked this.

fanquake commented at 3:48 pm on February 2, 2026:

Is the long term plan to break up compat.h, into many more of these mini-compat headers, and have many new includes (which themselves are some #ifdef logic + includes) in every file that previously only included compat.h? I do wonder if that will make it more or less likely to run into issues like #34454, or if that is really worth it to save some (hopefully comparatively ) small amount of code duplication/preprocessor time.

maflcko commented at 3:53 pm on February 2, 2026:

I’d say it would just be two:

compat/time.h for all time-related symbols (only timeval)
compat/compat.h for all net-related symbols (everything else)

But no strong opinion. Leaving this pull as-is is also fine.

fanquake commented at 4:08 pm on February 2, 2026:

I think it at least needs to add a mapping / fix for the IWYU bug (along with reporting upstream). No code changes are needed in compat.h, because timeval is already defined by sys/select.h.

hebasto commented at 4:43 pm on February 2, 2026:

along with reporting upstream

https://github.com/include-what-you-use/include-what-you-use/pull/1927.

maflcko commented at 5:08 pm on February 2, 2026:

Not sure how heavy the full compat net stack headers are to parse every time util/time.h is included. It may be faster to just include what is needed via compat/time.h instead of compat/compat.h, but I haven’t benchmarked this.

A third alternative could be to just move MillisToTimeval into compat.h

hebasto commented at 5:39 pm on February 2, 2026:

After an offline discussion with @fanquake, I’ve concluded that using compat.h (or any other facade header) does not guarantee adherence to the ‘include what you use’ paradigm, as the timeval symbol can be defined in any of four different headers. Therefore, the decision of which header to include should be based on the headers already present in the given file.

hebasto force-pushed on Feb 2, 2026

hebasto commented at 5:31 pm on February 2, 2026: member

Rebased to resolve a conflict with merged bitcoin/bitcoin#33878.

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

hebasto force-pushed on Feb 6, 2026

hebasto commented at 2:41 pm on February 6, 2026: member

Rebased.

ci, iwyu: Fix warnings in src/util and treat them as errors #34448

Reviews

Conflicts

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