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

pull BrandonOdiwuor wants to merge 2 commits into bitcoin:master from BrandonOdiwuor:iwyu-script changing 30 files +132 −43
  1. BrandonOdiwuor commented at 8:14 AM on April 6, 2026: contributor

    This PR continues the ongoing effort to enforce IWYU warnings.

    See Developer Notes.

  2. DrahtBot commented at 8:15 AM on April 6, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35011.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto
    Concept ACK kevkevinpal

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35287 (script: render BIP-388 wallet policies in getdescriptorinfo by rxbryan)
    • #34995 (ci, iwyu: Fix warnings in src/common and treat them as errors by hebasto)
    • #34806 (refactor: logging: Various API improvements by ajtowns)
    • #34566 (feature: Use different datadirs for different signets by ekzyis)
    • #32729 (test,refactor: extract script template helpers & widen sigop count coverage 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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. hebasto commented at 8:48 AM on April 6, 2026: member

    Concept ACK.

    d41095a70d87e84b8c5cdfcefe1c53174fbabac6

    Per our policy, "a comment explaining the rationale is required for every use of IWYU pragma: export":

    $ git grep -l "IWYU pragma: export" -- src/script
src/script/interpreter.h
src/script/script.h
  4. kevkevinpal commented at 4:35 PM on April 6, 2026: contributor

    Concept ACK

    Looks like common/ compat/ support/ are now left from that list after this PR is merged

  5. BrandonOdiwuor commented at 12:58 PM on April 9, 2026: contributor

    @kevkevinpal from the list I only think only compat/ and support/ are left there is already a PR: 34995 for common/ -> #34995. I don't know if this will be done for QT #33725 (comment)

  6. BrandonOdiwuor renamed this:
    ci, iwyu: Fix warnings in src/script and treat them as errors
    ci, iwyu: Fix warnings in 'src/script' and treat them as errors
    on Apr 9, 2026
  7. BrandonOdiwuor renamed this:
    ci, iwyu: Fix warnings in 'src/script' and treat them as errors
    ci, iwyu: Fix warnings in `src/script` and treat them as errors
    on Apr 9, 2026
  8. hebasto commented at 1:16 PM on April 9, 2026: member

    I don't know if this will be done for QT #33725 (comment)

    It will be, though it requires some preliminary work.

  9. BrandonOdiwuor force-pushed on Apr 10, 2026
  10. BrandonOdiwuor commented at 8:25 AM on April 10, 2026: contributor

    Per our policy, "a comment explaining the rationale is required for every use of IWYU pragma: export":

    updated the PR to include comments for the left IWYU pragma: export on scr/script

  11. in ci/test/03_test_script.sh:212 in 737a165d9a 
     208 | @@ -209,7 +209,7 @@ fi
 209 |  
 210 |  if [[ "${RUN_IWYU}" == true ]]; then
 211 |    # TODO: Consider enforcing IWYU across the entire codebase.
 212 | -  FILES_WITH_ENFORCED_IWYU="/src/(((crypto|index|kernel|primitives|univalue/(lib|test)|util|zmq)/.*|common/license_info|node/blockstorage|node/utxo_snapshot|clientversion|core_io|signet)\\.cpp)"
 213 | +  FILES_WITH_ENFORCED_IWYU="/src/(((crypto|index|kernel|primitives|univalue/(lib|test)|util|zmq|script)/.*|common/license_info|node/blockstorage|node/utxo_snapshot|clientversion|core_io|signet)\\.cpp)"
    hebasto commented at 11:42 AM on April 13, 2026:

    nit: Could the subdirectories be lexicographically ordered?

    BrandonOdiwuor commented at 6:18 AM on April 28, 2026:

    fixed

  12. in src/script/descriptor.cpp:28 in 737a165d9a 
      23 |  #include <script/signingprovider.h>
  24 |  #include <script/solver.h>
  25 | +#include <serialize.h>
  26 | +#include <tinyformat.h>
  27 |  #include <uint256.h>
  28 |
    hebasto commented at 11:43 AM on April 13, 2026:

    nit: Drop empty line?

    BrandonOdiwuor commented at 6:19 AM on April 28, 2026:

    fixed

  13. in src/script/interpreter.cpp:17 in 737a165d9a outdated 
       9 | @@ -10,8 +10,16 @@
  10 |  #include <crypto/sha256.h>
  11 |  #include <pubkey.h>
  12 |  #include <script/script.h>
  13 | +#include <serialize.h>
  14 | +#include <span.h>
  15 |  #include <tinyformat.h>
  16 |  #include <uint256.h>
  17 | +#include <algorithm>
    hebasto commented at 11:44 AM on April 13, 2026:

    Separate header groups by an empty line?

    BrandonOdiwuor commented at 6:20 AM on April 28, 2026:

    fixed

  14. in src/script/interpreter.h:15 in 737a165d9a 
       8 | @@ -9,21 +9,26 @@
   9 |  #include <consensus/amount.h>
  10 |  #include <hash.h>
  11 |  #include <primitives/transaction.h>
  12 | +#include <script/script.h>
  13 | +// script_error.h is exported because there should rarely be a case where it is
  14 | +// included without interpreter.h
  15 |  #include <script/script_error.h> // IWYU pragma: export
    hebasto commented at 11:57 AM on April 13, 2026:

    I see this explanation stems from faf1fb207fb6e9a12c864074f8c40d5922d93ff4, but saving a line feels like a weak rationale here. Let's stick to the ones that are documented in the Developer Notes or update them if needed.

    cc @maflcko

  15. in src/script/interpreter.h:18 in 737a165d9a 
      13 | +// script_error.h is exported because there should rarely be a case where it is
  14 | +// included without interpreter.h
  15 |  #include <script/script_error.h> // IWYU pragma: export
  16 | +// verify_flags.h is exported because it defines the common SCRIPT_VERIFY_*
  17 | +// type alias used by the interpreter interface
  18 |  #include <script/verify_flags.h> // IWYU pragma: export
    hebasto commented at 11:58 AM on April 13, 2026:

    cc @ajtowns

  16. in src/script/miniscript.cpp:6 in 737a165d9a outdated 
       1 | @@ -2,17 +2,16 @@
   2 |  // Distributed under the MIT software license, see the accompanying
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 | -#include <limits>
   6 | -#include <vector>
   7 | -
   8 | -#include <primitives/transaction.h>
   9 |  #include <script/miniscript.h>
  10 | +#include <primitives/transaction.h>
    hebasto commented at 12:00 PM on April 13, 2026:

    We usually separate the associated header by an empty line:

    #include <script/miniscript.h>

#include <primitives/transaction.h>
    BrandonOdiwuor commented at 6:21 AM on April 28, 2026:

    fixed

  17. in src/script/script.h:12 in 737a165d9a 
       7 | @@ -8,14 +8,16 @@
   8 |  
   9 |  #include <attributes.h>
  10 |  #include <crypto/common.h>
  11 | +// prevector.h is exported because it exists primarily to represent scripts
  12 |  #include <prevector.h> // IWYU pragma: export
    hebasto commented at 12:02 PM on April 13, 2026:

    This comment stems from fa5ccc4137fdd14a75a6fc860b8ff6fc455cb55d. I have the same concerns as above.

    cc @maflcko

  18. in src/script/sign.cpp:26 in 737a165d9a outdated 
      22 | +#include <serialize.h>
  23 |  #include <uint256.h>
  24 | +#include <util/check.h>
  25 |  #include <util/translation.h>
  26 |  #include <util/vector.h>
  27 | +#include <algorithm>
    hebasto commented at 12:03 PM on April 13, 2026:

    nit: Separate groups by an empty line?

    BrandonOdiwuor commented at 6:22 AM on April 28, 2026:

    fixed

  19. hebasto commented at 12:05 PM on April 13, 2026: member

    Left some comments.

  20. BrandonOdiwuor force-pushed on Apr 22, 2026
  21. DrahtBot added the label CI failed on Apr 22, 2026
  22. DrahtBot removed the label CI failed on Apr 23, 2026
  23. hebasto commented at 3:52 PM on April 28, 2026: member

    Almost ACK.

    There is a silent merge conflict here. Please apply the following patch during your next rebase to resolve it:

    --- a/src/script/sigcache.h
+++ b/src/script/sigcache.h
@@ -11,7 +11,9 @@
 #include <cuckoocache.h>
 #include <script/interpreter.h>
 #include <uint256.h>
-#include <util/byte_units.h>
+// IWYU incorrectly suggests removing this header.
+// See https://github.com/include-what-you-use/include-what-you-use/issues/2014.
+#include <util/byte_units.h> // IWYU pragma: keep
 #include <util/hasher.h>
 
 #include <cstddef>

    See also: #35152#pullrequestreview-4179389612.

  24. ci, iwyu: Fix warnings in src/scripts and treat them as error bf4d8a5917
  25. iwyu: remove #include <coins.h> from src/script/sign.h as recommended by iwyu 74f42e9dd7
  26. BrandonOdiwuor force-pushed on May 7, 2026
  27. hebasto approved
  28. hebasto commented at 7:38 AM on May 7, 2026: member

    ACK 74f42e9dd7c5683209d5d366486e1a6def49dcf1.

  29. hebasto commented at 7:39 AM on May 7, 2026: member

    cc @maflcko

Contributors
BrandonOdiwuorDrahtBothebastokevkevinpal
Linked (view graph)
#33725 ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`#35103 ci, iwyu: Fix warnings in `src/compat/` & `src/support/` and treat them as error
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-18 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me