iwyu: Document or remove some `pragma: export` and other improvements

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

This PR is a prerequisite for #34448. It was split into a separate PR to limit the scope and minimize potential merge conflicts.

The first commit improves the accuracy of IWYU suggestions within our heavily templated code. Note that, for now, the serialize.h header itself is excluded from IWYU inspection because it lacks a corresponding source file.

The remaining commits follow the Developer Notes guidance:

Use IWYU pragma: export very sparingly, as this enforces transitive inclusion of headers and undermines the specific purpose of IWYU.

hebasto added the label Refactoring on Feb 20, 2026

DrahtBot commented at 5:54 pm on February 20, 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.

Type	Reviewers
ACK	maflcko, ajtowns
Stale ACK	w0xlt, ryanofsky

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:

#34809 (threadsafety: Add STDLOCK() macro for StdMutex by ajtowns)
#34778 (logging: rewrite macros to add ratelimit option, avoid unused strprintf, clarify confusing errors by ryanofsky)
#34448 (ci, iwyu: Fix warnings in src/util and treat them as errors by hebasto)
#34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
#29256 (log, refactor: Allow log macros to accept context arguments by ryanofsky)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

DrahtBot added the label CI failed on Feb 20, 2026

DrahtBot removed the label CI failed on Feb 20, 2026

in src/sync.h:13 in e61ec118ef outdated

 9@@ -10,7 +10,7 @@
10 #include <logging/timer.h>
11 #endif
12 
13-#include <threadsafety.h> // IWYU pragma: export
14+#include <threadsafety.h>

maflcko commented at 7:34 am on February 27, 2026:

this was added in fa02591edff0255c5120b5acb2366542a61c251f, because there is only one single case (src/logging.h) that does not need sync.h.

So I think it is fine to keep as-is, or add a comment why this export exists?

hebasto commented at 11:04 am on February 27, 2026:

It seems there are opposing views here.

this was added in fa02591…

Right. Its message states: “All places that include sync.h will likely need threadsafety annotations, so export them.” This essentially reads as using the IWYU pragma: export as a convenient way to save one line in header inclusions.

I prefer to reserve this pragma for two specific scenarios: (1) providing symbols from headers that are an implementation of the main header, or (2) in facade headers such as compat.h.

I believe that the consistency gained by explicitly including used headers is more valuable than trying to avoid inclusion noise.

So I think it is fine to keep as-is, or add a comment why this export exists?

What wording did you have in mind for that comment?

maflcko commented at 2:06 pm on February 27, 2026:

0// Export, as `threadsafety.h` is practically not used stand-alone.

hebasto commented at 2:11 pm on February 27, 2026:

@fanquake

Given your previous comment, would you agree on that?

theuni commented at 7:16 pm on February 27, 2026:

I think the issue here is that StdMutex is weirdly crammed into threadsafety.h where it doesn’t belong. Imo the threadsafety annotations should be standalone, as they’re agnostic to the actual implementation.

I think this could be resolved by moving StdMutex to its own header (sync-unlogged.h or so), which can be used where it’s needed. Then nobody would ever need to use threadsafety.h directly.

maflcko commented at 7:33 pm on February 27, 2026:

Would iwyu not add threadsafety.h to all files that use GUARDED_BY from threadsafety.h?

theuni commented at 7:44 pm on February 27, 2026:

It would be exported by sync.h and sync-unlogged.h, which would make sense, as they’re the only two consumers.

hebasto commented at 11:37 am on February 28, 2026:

Thanks! Reworked.

ajtowns commented at 7:03 am on March 20, 2026:

I don’t think “sync-unlogged” is a very good name – StdMutex is just a minimal wrapper around stdlib’s mutex that allows clang’s thread safety warnings to work, which is also why it was put in threadsafety.h – adding the threadsafety annotations is all it does. It’s useful in places where we can’t use the more advanced sync.h, so for implementing logging (which is used by sync) and potentially in implementing sync itself.

hebasto commented at 7:17 am on March 20, 2026:

I don’t think “sync-unlogged” is a very good name…

Mind suggesting a better one?

ajtowns commented at 7:33 am on March 20, 2026:

I thought threadsafety.h was already pretty descriptive :shrug: util/stdmutex.h ?

theuni commented at 3:00 pm on March 20, 2026:

sgtm.

hebasto commented at 3:38 pm on March 20, 2026:

Thanks! Renamed.

in src/util/time.h:9 in 4306f57170 outdated

 5@@ -6,7 +6,7 @@
 6 #ifndef BITCOIN_UTIL_TIME_H
 7 #define BITCOIN_UTIL_TIME_H
 8 
 9-#include <chrono> // IWYU pragma: export
10+#include <chrono>

maflcko commented at 7:36 am on February 27, 2026:

for reference, this was added, so that this serves as a drop-in replacement for chrono, and that the using namespace std::chrono_literals; a few lines below is put into basically all places.

Though, this is just my personal preference, and I can see this to be changed.

hebasto commented at 11:12 am on February 27, 2026:

for reference, this was added, so that this serves as a drop-in replacement for chrono, and that the using namespace std::chrono_literals; a few lines below is put into basically all places.

If that is the intention, it would be better expressed as an IWYU mapping instead:

0  { "include": [ "<chrono>", "public", "<util/time.h>", "public" ] },

maflcko commented at 11:24 am on February 27, 2026:

Yeah, I don’t feel strongly about his one. I think anything is fine here.

hebasto commented at 2:03 pm on February 27, 2026:

for reference, this was added, so that this serves as a drop-in replacement for chrono, and that the using namespace std::chrono_literals; a few lines below is put into basically all places.

If that is the intention, it would be better expressed as an IWYU mapping instead:
0  { "include": [ "<chrono>", "public", "<util/time.h>", "public" ] },

Actually, this works for private headers only.

Added an explanatory comment instead.

in src/util/log.h:8 in 952662a6fa outdated

4@@ -5,7 +5,7 @@
5 #ifndef BITCOIN_UTIL_LOG_H
6 #define BITCOIN_UTIL_LOG_H
7 
8-#include <logging/categories.h> // IWYU pragma: export
9+#include <logging/categories.h>

maflcko commented at 7:41 am on February 27, 2026:

Same here. Is there a use-case to include logging/categories.h without either one of util/log.h or logging.h?

hebasto commented at 11:07 am on February 27, 2026:

Mind suggesting a comment for these cases?

maflcko commented at 11:24 am on February 27, 2026:

0// Export, as `logging/categories.h` is practically not used stand-alone.

hebasto commented at 2:04 pm on February 27, 2026:

Thanks! Taken.

maflcko commented at 7:44 am on February 27, 2026: member

Personally I think it is fine to use export for headers that are always (possibly with a single exception) simply forwarded via another header.

I think this applies to all cases here, except for the chrono one?

hebasto force-pushed on Feb 27, 2026

hebasto commented at 2:05 pm on February 27, 2026: member

@maflcko

Thank you for the review! Your feedback has been addressed.

hebasto renamed this:
~~iwyu: Remove some `pragma: export` and other improvements~~
iwyu: Document or remove some `pragma: export` and other improvements
on Feb 27, 2026

in src/util/log.h:8 in de4c286f1b outdated

4@@ -5,6 +5,7 @@
5 #ifndef BITCOIN_UTIL_LOG_H
6 #define BITCOIN_UTIL_LOG_H
7 
8+// Export, as `logging/categories.h` is practically not used stand-alone.

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

is practically not used stand-alone.

Not sure I understand this comment. Why are we doing this, instead of including the things that we are using? Why does it matter if they might only be used when some other header is also used?

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

Also, has the rationale for when to do this (or not), been documented in the developer notes?

hebasto commented at 2:30 pm on March 4, 2026:

is practically not used stand-alone.

Not sure I understand this comment. Why are we doing this, instead of including the things that we are using? Why does it matter if they might only be used when some other header is also used?

I’ve reverted this change to its initial variant.

Also, has the rationale for when to do this (or not), been documented in the developer notes?

It seems reasonable to update the developer notes once we work through the entire codebase.

fanquake commented at 9:59 am on March 11, 2026:

It seems reasonable to update the developer notes once we work through the entire codebase.

Why? The developer docs should reflect how we want new code to be written, and current code to be updated, otherwise you’re just guaranteeing having to rework code again later on, rather than having it introduced/updated to how it should be, now.

maflcko commented at 10:05 am on March 11, 2026:

If this is a blocker, maybe the dev notes can be updated to say that a comment with the rationale are required for exports?

hebasto commented at 12:56 pm on March 13, 2026:

It seems reasonable to update the developer notes once we work through the entire codebase.

Why? The developer docs should reflect how we want new code to be written, and current code to be updated, otherwise you’re just guaranteeing having to rework code again later on, rather than having it introduced/updated to how it should be, now.

If this is a blocker, maybe the dev notes can be updated to say that a comment with the rationale are required for exports?

Thanks! Done.

hebasto force-pushed on Feb 28, 2026

hebasto commented at 11:37 am on February 28, 2026: member

@theuni

Thank you for the review! Your feedback has been addressed.

hebasto force-pushed on Feb 28, 2026

DrahtBot added the label CI failed on Feb 28, 2026

maflcko commented at 5:41 pm on February 28, 2026: member

review ACK 7713575cc83d44a9ffbd9d60ad34a3f49720139e 🦆

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 7713575cc83d44a9ffbd9d60ad34a3f49720139e 🦆
3o4CFKfSlNayOksST4HbIfwGJA0virv5UET5Bo0O4srh70oXYZl6TmqhfNIvJrEgSZokTdwAo7DzOw9rtkFHnBA==

DrahtBot removed the label CI failed on Mar 3, 2026

hebasto force-pushed on Mar 4, 2026

hebasto commented at 2:32 pm on March 4, 2026: member

@fanquake @maflcko @theuni

Thank you for the review. The most recent @fanquake’s comment has been addressed.

theuni commented at 3:35 pm on March 4, 2026: member

I don’t understand what makes that handful of .cpp files special? Why do only a few require a direct logging/categories.h include?

hebasto commented at 3:46 pm on March 4, 2026: member

I don’t understand what makes that handful of .cpp files special? Why do only a few require a direct logging/categories.h include?

They are not special rather ones where IWYU warnings are treated as errors: https://github.com/bitcoin/bitcoin/blob/e09b81638ba1498e848b561cb47829a27e17e901/ci/test/03_test_script.sh#L186

It is assumed that this will be enforced for the entire codebase in the future.

maflcko commented at 4:01 pm on March 4, 2026: member

Only change is in the last commit.

review ACK 979177154a48388f1f771c65a135aca545099c75 💧

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 979177154a48388f1f771c65a135aca545099c75 💧
3mcOxsGIg9Bf9IzlarH+8Z035ibkFUaV9qoTlMFanEmgSm+Zh42G/NGqvRYPOMReL6abMsbDLFDlkVqrojI57Aw==

theuni commented at 4:26 pm on March 4, 2026: member

I don’t understand what makes that handful of .cpp files special? Why do only a few require a direct logging/categories.h include?

They are not special rather ones where IWYU warnings are treated as errors:

https://github.com/bitcoin/bitcoin/blob/e09b81638ba1498e848b561cb47829a27e17e901/ci/test/03_test_script.sh#L186

It is assumed that this will be enforced for the entire codebase in the future.

Sorry, but this progression just doesn’t make sense to me. As @maflcko said, the categories are needed any time a log macro is used. Using both includes everywhere (eventually) would be redundant and confusing.

It seems the original intent of logging/categories.h was to move them out of logging.h so that the kernel can use them without our logger impl. That makes sense.

But fundamentally, Category, Level, and SourceLocation are tied together.

util/log.h is the home of Level, and SourceLocation. It’s also barebones and leaves out the implementation details of the logger.

So it makes sense to me to either:

just move LogFlags into there directly and be done with it
pragma export logging/categories.h from util/log.h

I don’t see any harm in keeping the enum in a separate header for cleanliness, so I’d prefer the latter.

ryanofsky approved

ryanofsky commented at 7:22 pm on March 9, 2026: contributor

Code review ACK 979177154a48388f1f771c65a135aca545099c75.

I agree with @theuni it could be a good idea to just document the export in the last commit 979177154a48388f1f771c65a135aca545099c75 instead of removing it because right now the util/log.h header explicitly references BCLog::LogFlags::ALL, so it has a hard dependency on the categories.h include.

(We could break the util/log.h dependency on categories.h by defining Category as std::optional<uint64_t> instead of uint64_t and using nullopt to represent an unset category instead of BCLog::LogFlags::ALL. Or we could go the other direction and move the categories into util and drop the idea of having an opaque category field. @stickies-v introduced the opaque category field in 5b01eaef7a088f20b59788c01ef9a60916633721 from #34374 so I’m not actually sure about reasons it is opaque. But if seems like either the field should be opaque OR util/log.h should depend on the non-opaque enum, but not both.)

hebasto force-pushed on Mar 10, 2026

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

Since most reviewers agreed to export <logging/categories.h> from <util/log.h>, we are moving forward with this approach.

Let me know if you have a specific alternative in mind for the inline comment.

w0xlt commented at 6:11 pm on March 10, 2026: contributor

ACK 6a27dcd99701aa3d776b837d400776129b53a29e

DrahtBot requested review from ryanofsky on Mar 10, 2026

DrahtBot requested review from maflcko on Mar 10, 2026

ryanofsky approved

ryanofsky commented at 6:21 pm on March 10, 2026: contributor

Code review ACK 6a27dcd99701aa3d776b837d400776129b53a29e, just keeping the log.h categories export and documenting it which makes sense for now.

As mentioned previously in longer term I think it’d make sense to either (1) drop the log.h dependency on categories.h if category is supposed to be opaque within util (easy to do with std::optiona) or (2) move categories into util and drop the idea that categories should be opaque. The current middle ground doesn’t seem to make a lot of sense. The util library should either know about category values or not know about them. It shouldn’t pretend to not know about them and also directly reference one of the values it doesn’t know about.

maflcko commented at 6:23 pm on March 10, 2026: member

Only change is the last commit, again

review ACK 6a27dcd99701aa3d776b837d400776129b53a29e 😌

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 6a27dcd99701aa3d776b837d400776129b53a29e 😌
3kXDWNUlHQ6UOufO/FEtKNbnhJ8IbuIu1jnVaNlymzYsHtv8MlhVbfwdsai6rZx/jEUevrhTD0JHiIzrBBfxoCA==

in src/util/stdmutex.h:11 in 6a27dcd997 outdated

 6+#ifndef BITCOIN_SYNC_UNLOGGED_H
 7+#define BITCOIN_SYNC_UNLOGGED_H
 8+
 9+// This header declares threading primitives compatible with Clang
10+// Thread Safety Analysis and provides appropriate annotation macros.
11+#include <threadsafety.h> // IWYU pragma: export

maflcko commented at 6:35 pm on March 10, 2026:

nit: I wonder if src/sync_unlogged.h should not be moved into src/util/sync_unlogged.h, because it seems like a low-level utility. Same for threadsafety.h. No need to re-touch here, but the move should be trivial, because the files are now basically only used in two places.

maflcko commented at 7:11 pm on March 19, 2026:

Did you see this comment, since you had to re-touch anyway?

hebasto commented at 7:17 pm on March 19, 2026:

Thanks! Amended.

hebasto commented at 12:56 pm on March 13, 2026: member

A commit updating Developer Notes has been added.

DrahtBot added the label Needs rebase on Mar 19, 2026

hebasto force-pushed on Mar 19, 2026

hebasto commented at 4:03 pm on March 19, 2026: member

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

DrahtBot removed the label Needs rebase on Mar 19, 2026

serialize: Add missing `<span>` header

Including the missing `<span>` header in `serialize.h` allows IWYU to
correctly evaluate its redundancy elsewhere.

6d2952c3c3

hebasto force-pushed on Mar 19, 2026

DrahtBot added the label CI failed on Mar 19, 2026

maflcko commented at 7:30 pm on March 19, 2026: member

review ACK 0071c814aa15bf87afb737a089d164f452cde7ea 🏥

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 0071c814aa15bf87afb737a089d164f452cde7ea 🏥
3E77G3eTUIuHMM5SFTJSN1JosuJGrSbQ+tnfe5ibnFw8GnN3Sf6t3Tv/ED1vFN4lLIjThkdejk0DaULwDIrtRBA==

DrahtBot requested review from w0xlt on Mar 19, 2026

DrahtBot requested review from ryanofsky on Mar 19, 2026

DrahtBot removed the label CI failed on Mar 19, 2026

refactor: Move `StdMutex` to its own header

This makes `threadsafety.h` agnostic to the actual implementation.

179abb387f

iwyu, doc: Document `IWYU pragma: export` for `<threadsafety.h>` 48bfcfedec

iwyu, doc: Document `IWYU pragma: export` for `<chrono>` 015bea05e6

iwyu, doc: Document `IWYU pragma: export` for `<logging/categories.h>` cfa3b10d50

doc: Document rationale for using `IWYU pragma: export` 0fe6fccec2

hebasto force-pushed on Mar 20, 2026

hebasto commented at 3:39 pm on March 20, 2026: member

A new header has been renamed per feedback from @ajtowns and @theuni.

maflcko commented at 3:49 pm on March 20, 2026: member

review ACK 0fe6fccec279f9799dfb84c36cf9033eedcc2c32 👤

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 0fe6fccec279f9799dfb84c36cf9033eedcc2c32 👤
3WGcFoynrQ7QCQlSRD0khbgwJSn7W9s6NyU6P4EpDJXrR3DS4ZCMd9fow6DnNR3j8iEFFtBy238EbfahiLE/+AQ==

ajtowns commented at 2:37 am on March 23, 2026: contributor

utACK 0fe6fccec279f9799dfb84c36cf9033eedcc2c32

Didn’t check iwyu’s happiness levels but the changes look okay to me. FWIW, the comments in compat/compat.h don’t really make it clear to me why the IWYU exports are there; not sure if that’s my misunderstanding or because they’re intended to be updated in a later PR. Should the IWYU export includes be separated into a different block in general (since they form part of this header’s exposed API)? That seems like it might be an appropriate way to deal with the kernel/mempool*.h includes/exports in txmempool.h.

fanquake merged this on Mar 23, 2026

fanquake closed this on Mar 23, 2026

iwyu: Document or remove some pragma: export and other improvements #34639

Reviews

Conflicts

iwyu: Document or remove some `pragma: export` and other improvements #34639