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:

#34641 (node: scale -dbcache default from total system RAM by l0rinc)
#34639 (iwyu: Remove some pragma: export and other improvements by hebasto)
#34598 (bench: use deterministic HexStr payload by l0rinc)
#34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)

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:11 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.

hebasto force-pushed on Feb 6, 2026

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

Rebased.

DrahtBot added the label Needs rebase on Feb 7, 2026

maflcko commented at 9:23 am on February 10, 2026: member

unrelated, but the clang-format in the ci container seems broken? It removes the newlines. E.g. https://github.com/bitcoin/bitcoin/actions/runs/21830259022/job/62995439943#step:11:1639

 0diff --git a/src/index/base.cpp b/src/index/base.cpp
 1index 39d4dc7..0547fd1 100644
 2--- a/src/index/base.cpp
 3+++ b/src/index/base.cpp
 4@@ -10,6 +10,7 @@
 5 #include <interfaces/chain.h>
 6 #include <interfaces/types.h>
 7 #include <kernel/types.h>
 8+#include <logging.h>
 9 #include <node/abort.h>
10 #include <node/blockstorage.h>
11 #include <node/context.h>
12@@ -21,7 +22,6 @@
13 #include <uint256.h>
14 #include <undo.h>
15 #include <util/fs.h>
16-#include <util/log.h>
17 #include <util/string.h>
18 #include <util/thread.h>
19 #include <util/threadinterrupt.h>
20diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
21index cf31c9a..a805719 100644
22--- a/src/node/blockstorage.cpp
23+++ b/src/node/blockstorage.cpp
24@@ -3,7 +3,6 @@
25 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
26 
27 #include <node/blockstorage.h>
28-
29 #include <arith_uint256.h>
30 #include <chain.h>
31 #include <consensus/params.h>
32@@ -30,7 +29,6 @@
33 #include <util/check.h>
34 #include <util/expected.h>
35 #include <util/fs.h>
36-#include <util/log.h>
37 #include <util/obfuscation.h>
38 #include <util/overflow.h>
39 #include <util/result.h>
40@@ -40,7 +38,6 @@
41 #include <util/time.h>
42 #include <util/translation.h>
43 #include <validation.h>
44-
45 #include <cerrno>
46 #include <compare>
47 #include <cstddef>

hebasto force-pushed on Feb 10, 2026

hebasto commented at 3:55 pm on February 10, 2026: member

Rebased.

hebasto commented at 4:29 pm on February 10, 2026: member

unrelated, but the clang-format in the ci container seems broken? It removes the newlines. E.g. https://github.com/bitcoin/bitcoin/actions/runs/21830259022/job/62995439943#step:11:1639

Looks like fix_includes.py strips empty lines and clang-format-diff.py fails to restore them.

DrahtBot removed the label Needs rebase on Feb 10, 2026

hebasto commented at 5:24 pm on February 10, 2026: member

unrelated, but the clang-format in the ci container seems broken? It removes the newlines. E.g. https://github.com/bitcoin/bitcoin/actions/runs/21830259022/job/62995439943#step:11:1639

Looks like fix_includes.py strips empty lines and clang-format-diff.py fails to restore them.

Fixed in #34551.

hebasto commented at 6:09 pm on February 10, 2026: member

The only conflicting PR has been merged, so now seems like a good time to get this reviewed.

Friendly ping @sedited @maflcko @willcl-ark :)

in src/util/time.cpp:23 in 639cdba927 outdated

19 #include <string>
20 #include <string_view>
21 #include <thread>
22 
23+#ifdef WIN32
24+#include <winsock2.h>

fanquake commented at 6:11 pm on February 10, 2026:

What is this for (same in util/time.h)?

hebasto commented at 6:13 pm on February 10, 2026:

For the timeval type.

in src/util/threadinterrupt.h:9 in 639cdba927

5@@ -6,7 +6,6 @@
6 #define BITCOIN_UTIL_THREADINTERRUPT_H
7 
8 #include <sync.h>
9-#include <threadsafety.h>

fanquake commented at 6:16 pm on February 10, 2026:

Is now the time to remove the pragma: export from sync.h, and retain this header, given it is used?

hebasto commented at 7:20 pm on February 10, 2026:

Thanks! Done.

hebasto force-pushed on Feb 10, 2026

sedited referenced this in commit 6777314310 on Feb 11, 2026

hebasto force-pushed on Feb 13, 2026

hebasto commented at 2:13 pm on February 13, 2026: member

Rebased.

DrahtBot added the label CI failed on Feb 13, 2026

DrahtBot commented at 3:17 pm on February 13, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows-cross to x86_64, ucrt: https://github.com/bitcoin/bitcoin/actions/runs/21989888993/job/63533810608 LLM reason (✨ experimental): Test failure: std::future::wait_for is called with a plain long int instead of a std::chrono::duration, causing a type-mismatch in threadpool_tests.cpp.

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 Feb 13, 2026

DrahtBot removed the label CI failed on Feb 13, 2026

iwyu: Drop `IWYU pragma: export` from `src/sync.h` 3469233067

test: Rename `WAIT_TIMEOUT` to `TEST_WAIT_TIMEOUT`

On Windows, the `winerror.h` header defines `WAIT_TIMEOUT` as a macro.

This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before using `WAIT_TIMEOUT`, the
preprocessor expands it into a numeric literal, causing syntax errors.

Rename the variable to `TEST_WAIT_TIMEOUT` to remove this fragility and
avoid the collision entirely.

1d79272ad2

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

in src/util/fs.h:8 in 76762de9a7 outdated

4@@ -5,17 +5,15 @@
5 #ifndef BITCOIN_UTIL_FS_H
6 #define BITCOIN_UTIL_FS_H
7 
8-#include <tinyformat.h>
9+#include <tinyformat.h> // IWYU pragma: keep

fanquake commented at 12:13 pm on February 18, 2026:

Why do we need the // IWYU pragma: keep here?

hebasto commented at 6:44 pm on February 18, 2026:

An explanatory comment has been added.

hebasto force-pushed on Feb 18, 2026

in src/logging.h:10 in adcaac7841

 7@@ -8,6 +8,7 @@
 8 
 9 #include <crypto/siphash.h>
10 #include <logging/categories.h> // IWYU pragma: export

fanquake commented at 7:01 pm on February 18, 2026:

If this PR is going to fix util, then I think pretty much any (existing) pramga use, outside of the config header, should be getting comments (especially given the prior added one was another IWYU bug). Pragmas should be used sparingly, and explained when so.

hebasto commented at 8:44 pm on February 18, 2026:

I agree that all pragmas should eventually be documented. However, addressing the existing ones here would significantly expand the scope of this PR and likely introduce more merge conflicts.

Since these older pragmas might need to be removed entirely rather than just documented, it would be best to loop in the original authors or reviewers to make that call. I think it makes the most sense to tackle this cleanup in follow-up PRs.

fanquake commented at 4:01 pm on February 19, 2026:

However, addressing the existing ones here would significantly expand the scope of this PR

Looking at the current state of this PR, there are 4 instances of // IWYU pragma: export in src/util/. Documenting or removing them, seems in scope for a PR that is fixing all issues in src/util/. If not done now, we’ll just have to come back again later and fix them anyways (and still have merge conflicts anyways, if there are any).

Since these older pragmas might need to be removed entirely rather than just documented, it would be best to loop in the original authors or reviewers to make that call.

Isn’t the best case if you can just delete the pragma, and fix the includes? Otherwise I’m not sure what call they would need to make. Either the pragma is removed, and includes fixed, there is a IWYU bug (which can be documented), or a rationale is written for keeping the pragma (even if it isn’t needed)?

hebasto marked this as a draft on Feb 20, 2026

hebasto commented at 5:54 pm on February 20, 2026: member

Drafted. Please review #34639 first.

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