ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`

hebasto commented at 5:12 pm on October 28, 2025: member

This PR continues the work from bitcoin/bitcoin#31308 by extending the treatment of IWYU warnings as errors to the src/init and src/policy directories.

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

DrahtBot commented at 5:12 pm on October 28, 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/33725.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33629 (Cluster mempool by sdaftuar)
#33591 (Cluster mempool followups by sdaftuar)
#33199 (fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)
#31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
#29060 (Policy: Report debug message why inputs are non standard by ismaelsadeeq)

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 added the label Refactoring on Oct 28, 2025

hebasto added the label Tests on Oct 28, 2025

hebasto commented at 5:36 pm on October 28, 2025: member

Friendly ping @l0rinc @maflcko @ryanofsky @willcl-ark who reviewed #31308.

in src/policy/truc_policy.cpp:13 in 30e5b015c9 outdated

11+#include <primitives/transaction.h>
12 #include <tinyformat.h>
13+#include <txmempool.h>
14 #include <util/check.h>
15 
16+#include <boost/multi_index/detail/hash_index_iterator.hpp>

maflcko commented at 5:36 pm on October 28, 2025:

nit in 30e5b015c973c1ac375e1eae6649238c6c2e2b28: This seems wrong, as there is no boost usage here?

See also my previous comment on this: #32668 (comment)

fanquake commented at 5:42 pm on October 28, 2025:

It’d also be good if that commit message was more explanatory:

Additional header changes were required due to visibility adjustments.

Which additional changes? What visibility adjustments?

hebasto commented at 5:56 pm on October 28, 2025:

It’d also be good if that commit message was more explanatory:

Additional header changes were required due to visibility adjustments.

Which additional changes? What visibility adjustments?

The commit message has been updated. Is it clearer now?

hebasto commented at 7:34 pm on October 28, 2025:

nit in 30e5b01: This seems wrong, as there is no boost usage here?

It is used as in https://github.com/bitcoin/bitcoin/blob/a36b9320300d1fa1e5dbd1b4d493acf3578ac11a/src/policy/truc_policy.cpp#L218 the type of mempool_ancestors.begin() is boost::multi_index::detail::hashed_index_iterator<...>.

However, the detail namespace should be hidden with a Boost-specifc mapping file.

maflcko commented at 7:42 pm on October 28, 2025:

It is used as in

I understand, but I wonder what value there is in listing the detailed internal boost includes. The mempool_ancestors already requires the txmempool.h include, which contains all required boost includes. No other module will have to include them to be able to compile.

To me those just seems like bloat for no benefit and the downside of making it harder to change a mempool implementation detail, because headers all over the codebase will have to be adjusted to please iwyu.

hebasto commented at 7:49 pm on October 28, 2025:

You’re right.

Reworked.

hebasto force-pushed on Oct 28, 2025

ci, iwyu: Add latest Boost mapping file e957a4ab52

hebasto force-pushed on Oct 28, 2025

in src/policy/policy.cpp:25 in 8ae41ae6f2

17@@ -18,9 +18,11 @@
18 #include <script/solver.h>
19 #include <serialize.h>
20 #include <span.h>
21+#include <util/check.h>
22 
23 #include <algorithm>
24 #include <cstddef>
25+#include <span>

maflcko commented at 8:06 pm on October 28, 2025:

nit in 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0: Same here: Doesn’t matter much, but the benefit of including span, when span.h is included, seems limited.

hebasto commented at 8:17 pm on October 28, 2025:

Right. ~~But let me leave this change for a follow-up, as it would touch files in src/crypto.~~

hebasto commented at 8:33 pm on October 28, 2025:

Thanks! Reworked.

purpleKarrot commented at 8:15 am on October 29, 2025:

but the benefit of including span, when span.h is included, seems limited.

That is not the right rationale. Since span.h may get phased out by future refactoring, it is advantageous to have an indicator at the top of the file whether <span> is used purely directly or directly and indirectly.

To give a more concrete example: If I refactor a piece of code such that no code from span.h is used anymore, I want iwyu to tell me that the #include <span.h> directive can be removed. When that file exports symbols from <span>, it will depend on the include order whether iwyu will suggest the removal or not.

maflcko commented at 8:39 am on October 29, 2025:

Yeah, I can see it both ways. I think the possible harm from including span.h over span is limited with this trivial header. Just noting I did the same with the time.h include: https://github.com/bitcoin/bitcoin/blob/3bb30658e631ed45b6c8609292facc7ae3dd0f61/src/util/time.h#L9

Also for the validation.h include: https://github.com/bitcoin/bitcoin/blob/3bb30658e631ed45b6c8609292facc7ae3dd0f61/src/validation.h#L19. There, the harm could be larger, as the validation header is a bit larger than the cs_main header, but it feels a bit verbose to redundantly include cs_main where validation is included.

Though, no strong opinion. This is just my thinking and anything is fine here, as long as reviewers are happy with it.

maflcko approved

maflcko commented at 8:10 pm on October 28, 2025: member

review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 🐇

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 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 🐇
3/yGQlCLoD+y92eefuzREel2J883stbBwa/+Pt1X3bzg5mC8jGDGvjG7pgVwg7TuvT7ZyPcl1EZc2Tk7H1ZdqDg==

ci, iwyu: Export `<span>` header from `<span.h>` 8f7d9142d4

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

Headers that are no longer available via transitive includes are now
included directly in `bitcoin-tx.cpp` and sources in `src/test`.

35f8874931

hebasto force-pushed on Oct 28, 2025

fanquake commented at 4:19 pm on October 29, 2025: member

this pull request conflicts with the following ones: #33629 (Cluster mempool by sdaftuar) #33591 (Cluster mempool followups by sdaftuar)

I think if you want to continue these refactors, it might be better if you picked code that doesn’t conflict with Cluster Mempool or Kernel work, or things we are trying to ship in v31. Could instead do directories that rarely change, like qt, or zmq?

in ci/test/03_test_script.sh:230 in 35f8874931

223@@ -224,8 +224,10 @@ if [ "${RUN_TIDY}" = "true" ]; then
224   run_iwyu() {
225     mv "${BASE_BUILD_DIR}/$1" "${BASE_BUILD_DIR}/compile_commands.json"
226     python3 "/include-what-you-use/iwyu_tool.py" \
227-             -p "${BASE_BUILD_DIR}" "${MAKEJOBS}" \
228-             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
229+             -p "${BASE_BUILD_DIR}" "${MAKEJOBS}" -- \
230+             -Xiwyu --cxx17ns \
231+             -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
232+             -Xiwyu --mapping_file=/include-what-you-use/boost-1.75-all.imp \

fanquake commented at 4:29 pm on October 29, 2025:

include-what-you-use/boost-1.75-all.imp

I guess this is still correct-enough to use here, but are we making any effort to usptream updates, given it’s 14 Boost releases out of date (current release is 1.89.0)? Seems possible that this could lead to incorrect IWYU suggestions, given any recent Boost refactorings?

hebasto commented at 5:37 pm on October 29, 2025:

We use a tiny part of Boost, which seems quite stable. So I don’t think it’s worth the effort.

hebasto commented at 5:35 pm on October 29, 2025: member

this pull request conflicts with the following ones: #33629 (Cluster mempool by sdaftuar) #33591 (Cluster mempool followups by sdaftuar)

I think if you want to continue these refactors, it might be better if you picked code that doesn’t conflict with Cluster Mempool or Kernel work, or things we are trying to ship in v31.

I agree, and when I noticed these conflicts, I checked that those PRs don’t have any ACKs yet.

Could instead do directories that rarely change, like qt, or zmq?

Right.

maflcko commented at 9:26 am on October 30, 2025: member

Other candidates could be: common/ compat/ primitives/ script/ support/ util/ zmq/

hebasto marked this as a draft on Oct 30, 2025

ci, iwyu: Treat warnings as errors for src/init and src/policy #33725

Code Coverage & Benchmarks

Reviews

Conflicts

ci, iwyu: Treat warnings as errors for `src/init` and `src/policy` #33725