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

hebasto · 2025-10-28T17:12:52Z

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

DrahtBot · 2025-10-28T17:12:57Z

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:

#33892 (policy: Remove individual transaction <minrelay restriction by instagibbs)
#33779 (ci, iwyu: Fix warnings in src/kernel and treat them as errors by hebasto)
#33629 (Cluster mempool by sdaftuar)
#33199 (fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)
#32545 (Replace cluster linearization algorithm with SFL by sipa)
#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 · 2025-10-28T17:36:11Z

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

src/policy/truc_policy.cpp

maflcko

review ACK 8ae41ae 🐇

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 🐇
/yGQlCLoD+y92eefuzREel2J883stbBwa/+Pt1X3bzg5mC8jGDGvjG7pgVwg7TuvT7ZyPcl1EZc2Tk7H1ZdqDg==

maflcko · 2025-10-28T20:06:26Z

src/policy/policy.cpp


 #include <algorithm>
 #include <cstddef>
+#include <span>


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

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

Thanks! Reworked.

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.

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:

bitcoin/src/util/time.h

Line 9 in 3bb3065

#include <chrono> // IWYU pragma: export

Also for the validation.h include:

bitcoin/src/validation.h

Line 19 in 3bb3065

#include <kernel/cs_main.h> // IWYU pragma: export

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

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

fanquake · 2025-10-29T16:19:20Z

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?

fanquake · 2025-10-29T16:29:14Z

ci/test/03_test_script.sh

+             -p "${BASE_BUILD_DIR}" "${MAKEJOBS}" -- \
+             -Xiwyu --cxx17ns \
+             -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
+             -Xiwyu --mapping_file=/include-what-you-use/boost-1.75-all.imp \


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?

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

hebasto · 2025-10-29T17:35:00Z

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 · 2025-10-30T09:26:09Z

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

DrahtBot · 2025-11-25T12:22:12Z

🐙 This pull request conflicts with the target branch and needs rebase.

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

d6592ae

hebasto added Refactoring Tests labels Oct 28, 2025

maflcko reviewed Oct 28, 2025

View reviewed changes

src/policy/truc_policy.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 251028-force-iwyu branch from 30e5b01 to a36b932 Compare October 28, 2025 17:55

This was referenced Oct 28, 2025

Cluster mempool #33629

Merged

Cluster mempool followups #33591

Merged

ci, iwyu: Add latest Boost mapping file

e957a4a

hebasto force-pushed the 251028-force-iwyu branch from a36b932 to 8ae41ae Compare October 28, 2025 19:49

maflcko approved these changes Oct 28, 2025

View reviewed changes

hebasto added 2 commits October 28, 2025 20:31

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

8f7d914

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

35f8874

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

hebasto force-pushed the 251028-force-iwyu branch from 8ae41ae to 35f8874 Compare October 28, 2025 20:32

This was referenced Oct 28, 2025

fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates #33199

Open

Policy: Report debug message why inputs are non standard #29060

Open

fanquake reviewed Oct 29, 2025

View reviewed changes

hebasto marked this pull request as draft October 30, 2025 11:21

This was referenced Oct 30, 2025

Fees: add Fee rate Forecaster Manager #31664

Closed

Fee Estimation via Fee rate Forecasters #30157

Closed

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

Open

DrahtBot mentioned this pull request Nov 18, 2025

policy: allow <minrelay txns in package context if paid for by cpfp #33892

Merged

hebasto mentioned this pull request Nov 20, 2025

ci: Add IWYU job #33810

Merged

DrahtBot added the Needs rebase label Nov 25, 2025

DrahtBot mentioned this pull request Nov 25, 2025

Replace cluster linearization algorithm with SFL #32545

Merged

hebasto closed this Dec 22, 2025

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

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

Uh oh!

Conversation

hebasto commented Oct 28, 2025

Uh oh!

DrahtBot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hebasto commented Oct 28, 2025

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

purpleKarrot Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 29, 2025

Uh oh!

fanquake Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 29, 2025

Uh oh!

maflcko commented Oct 30, 2025

Uh oh!

DrahtBot commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

DrahtBot commented Oct 28, 2025 •

edited

Loading

hebasto Oct 28, 2025 •

edited

Loading

purpleKarrot Oct 29, 2025 •

edited

Loading