refactoring: Remove unused includes #16659

1. 
   practicalswift commented at 4:28 pm on August 19, 2019: contributor

   As requested by MarcoFalke in #16273 (comment):

   This PR removes unused includes.

   Please note that in contrast to #16273 I’m limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier.

   I’m seeking “Concept ACK”:s for this obviously non-urgent minor cleanup.

   Rationale:

   • Avoids unnecessary re-compiles in case of header changes.
   • Makes reasoning about code dependencies easier.
   • Reduces compile-time memory usage.
   • Reduces compilation time.
   • Warm fuzzy feeling of being lean :-)
2. 
   GChuf commented at 5:03 pm on August 19, 2019: contributor

   I’m all for lean and mean stuff. Concept ACK
3. MarcoFalke added this to the milestone Future on Aug 19, 2019
4. 
   jb55 commented at 5:59 pm on August 19, 2019: member

   Concept ACK
5. DrahtBot added the label Consensus on Aug 19, 2019
6. DrahtBot added the label GUI on Aug 19, 2019
7. DrahtBot added the label Mempool on Aug 19, 2019
8. DrahtBot added the label Mining on Aug 19, 2019
9. DrahtBot added the label P2P on Aug 19, 2019
10. DrahtBot added the label Refactoring on Aug 19, 2019
11. DrahtBot added the label RPC/REST/ZMQ on Aug 19, 2019
12. DrahtBot added the label Tests on Aug 19, 2019
13. DrahtBot added the label TX fees and policy on Aug 19, 2019
14. DrahtBot added the label Utils/log/libs on Aug 19, 2019
15. DrahtBot added the label UTXO Db and Indexes on Aug 19, 2019
16. DrahtBot added the label Validation on Aug 19, 2019
17. DrahtBot added the label Wallet on Aug 19, 2019
18. fanquake removed the label Consensus on Aug 19, 2019
19. fanquake removed the label GUI on Aug 19, 2019
20. fanquake removed the label Mempool on Aug 19, 2019
21. fanquake removed the label Mining on Aug 19, 2019
22. fanquake removed the label P2P on Aug 19, 2019
23. fanquake removed the label RPC/REST/ZMQ on Aug 19, 2019
24. fanquake removed the label TX fees and policy on Aug 19, 2019
25. fanquake removed the label Tests on Aug 19, 2019
26. fanquake removed the label UTXO Db and Indexes on Aug 19, 2019
27. fanquake removed the label Utils/log/libs on Aug 19, 2019
28. fanquake removed the label Validation on Aug 19, 2019
29. fanquake removed the label Wallet on Aug 19, 2019
30. fanquake added the label Needs Conceptual Review on Aug 19, 2019
31. 
    DrahtBot commented at 6:28 pm on August 19, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
    • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
    • #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
    • #15934 (Merge settings one place instead of five places by ryanofsky)
    • #14053 (Add address-based index (attempt 4?) by marcinja)
    • #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)

    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.

32. 
    emilengler commented at 7:27 pm on August 19, 2019: contributor

    Concept ACK
33. 
    promag commented at 1:36 am on August 25, 2019: member

    Concept ACK.
34. 
    l2a5b1 commented at 8:42 am on August 29, 2019: contributor

    Concept ACK
35. 
    practicalswift commented at 8:23 am on September 26, 2019: contributor

    @MarcoFalke Could this one get a milestone? I think it should have a concrete milestone if valuable enough to keep open :) I’m trying to trim my set of open PR:s so if no milestone I’d rather close.
36. 
    promag commented at 10:41 am on September 26, 2019: member

    Running make on HEAD 150c289b4034fb2101df61998dd71b11f3d7fae1:

     0     1286.75 real      1170.31 user        99.80 sys
     1 748662784  maximum resident set size
     2         0  average shared memory size
     3         0  average unshared data size
     4         0  average unshared stack size
     5  35466407  page reclaims
     6    168811  page faults
     7         0  swaps
     8         0  block input operations
     9         0  block output operations
    10         0  messages sent
    11         0  messages received
    12      2512  signals received
    13     39561  voluntary context switches
    14    377820  involuntary context switches
    

    And on HEAD^ e00ecb3d7aaee463643e486ca03c318e192b8058:

     0     1440.06 real      1300.06 user       113.79 sys
     1 747356160  maximum resident set size
     2         0  average shared memory size
     3         0  average unshared data size
     4         0  average unshared stack size
     5  35429838  page reclaims
     6    212723  page faults
     7         0  swaps
     8         0  block input operations
     9         0  block output operations
    10         0  messages sent
    11         0  messages received
    12      2381  signals received
    13     55950  voluntary context switches
    14    932017  involuntary context switches
    

    (make clean && ccache -C before each build).

    So I’d ACK if this was already rebased and non draft.

37. 
    practicalswift commented at 10:48 am on September 26, 2019: contributor

    @promag Thanks a lot for benchmarking! I’ll update and rebase as soon as I get clarification from @MarcoFalke regarding the milestone :)
38. 
    practicalswift commented at 8:53 am on October 10, 2019: contributor

    @MarcoFalke Friendly ping :)
39. 
    laanwj commented at 12:20 pm on October 10, 2019: member

    A PR only removing lines is very good. I also think early in the 0.20 cycle is a good time to do this.

    So to be clear: this removes unnecessary includes that would otherwise cause the file to be included unnecessary, not includes that still end up being included indirectly through another route?

40. 
    practicalswift commented at 1:49 pm on October 10, 2019: contributor

    @laanwj Thanks for the clarification!

    So to be clear: this removes unnecessary includes that would otherwise cause the file to be included unnecessary, not includes that still end up being included indirectly through another route?

    Yes, exactly!

    Would you mind adding the 0.20 tag? I’ll update the PR :)

41. laanwj removed this from the milestone Future on Oct 10, 2019
42. laanwj added this to the milestone 0.20.0 on Oct 10, 2019
43. MarcoFalke marked this as ready for review on Oct 11, 2019
44. DrahtBot added the label Needs rebase on Oct 11, 2019
45. 
    laanwj commented at 7:15 am on October 12, 2019: member

    Let’s get this merged soon then, to not have to keep rebasing, at least there’s basically zero risk.
46. practicalswift force-pushed on Oct 12, 2019
47. DrahtBot removed the label Needs rebase on Oct 12, 2019
48. practicalswift force-pushed on Oct 12, 2019
49. practicalswift force-pushed on Oct 14, 2019
50. practicalswift force-pushed on Oct 14, 2019
51. practicalswift force-pushed on Oct 14, 2019
52. DrahtBot added the label Needs rebase on Oct 15, 2019
53. practicalswift force-pushed on Oct 15, 2019
54. Remove unused includes 084e17cebd
55. practicalswift force-pushed on Oct 15, 2019
56. DrahtBot removed the label Needs rebase on Oct 15, 2019
57. 
    practicalswift commented at 5:41 am on October 16, 2019: contributor

    Rebased again :)

    I think this one should be ready for final code review!

58. ryanofsky approved
59. 
    ryanofsky commented at 2:30 pm on October 16, 2019: member

    Code review ACK 084e17cebd424b8e8ced674bc810eef4e6ee5d3b. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn’t be a tragedy anyway.

    I would encourage @practicalswift to be more transparent about tools and methodologies used, since it’s unclear how this PR was generated, how the changes should be maintained, and if there were tools used that might deserve credit.

60. 
    practicalswift commented at 3:29 pm on October 16, 2019: contributor

    @ryanofsky The only tools used are nm and cxxfilt (python module). The rest is a very much Bitcoin Core specific Python script I’ve written myself :)

    0$ wc -l find_redundant_includes.??
    1 2207 find_redundant_includes.py
    2    3 find_redundant_includes.sh
    3 2210 total
    4$ grep -v '"' find_redundant_includes.py | wc -l
    5371
    

    2207 LOC might sound crazy but a lot of it is taken up by a.) a mapping between object file names and headers file names and b.) a mapping between standard library headers and standard library functions/symbols. The real line count should be somewhere around 400 LOC.

    It is not pretty but it works :)

61. 
    ryanofsky commented at 3:55 pm on October 16, 2019: member

    Thanks. I’d be curious to see the script if you want to post it somewhere like a gist.
62. MarcoFalke referenced this in commit 46d6930f8c on Oct 16, 2019
63. MarcoFalke merged this on Oct 16, 2019
64. MarcoFalke closed this on Oct 16, 2019

    ---

65. in src/node/transaction.cpp:9 in 084e17cebd

    5@@ -6,7 +6,6 @@
    6 #include <consensus/validation.h>
    7 #include <net.h>
    8 #include <net_processing.h>
    9-#include <txmempool.h>
    

    ---

    ---

    

    MarcoFalke commented at 9:44 pm on October 16, 2019:

    This is actually used

    ---

    

    practicalswift commented at 9:46 pm on October 16, 2019:

    Oh, I’ll investigate shortly! Did you find any other examples? Was it found by manual review?
66. 
    MarcoFalke commented at 9:44 pm on October 16, 2019: member

    ACK
67. 
    practicalswift commented at 11:07 pm on October 16, 2019: contributor

    Thanks for merging!

    See below for a script run after this merge.

    I believe the remaining cases to be of the “non-trivial” type where the header directly included is not needed in itself but one of more of the transitively included headers are.

    Consider the src/bench/rpc_blockchain.cpp example: we are currently including validation.h which transitively gives us PROTOCOL_VERSION, CBlock, CBlockIndex and uint256. But we don’t need anything validation.h provides in itself.

    Instead of including validation.h we should include version.h for PROTOCOL_VERSION, primitives/block.h for CBlock, chain.h for CBlockIndex and uint256.h for uint256.

    Like this:

     0diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
     1index 2fc6f116a..00cec5ecb 100644
     2--- a/src/bench/rpc_blockchain.cpp
     3+++ b/src/bench/rpc_blockchain.cpp
     4@@ -5,9 +5,12 @@
     5 #include <bench/bench.h>
     6 #include <bench/data.h>
     7 
     8-#include <validation.h>
     9+#include <chain.h>
    10+#include <primitives/block.h>
    11 #include <streams.h>
    12 #include <rpc/blockchain.h>
    13+#include <uint256.h>
    14+#include <version.h>
    15 
    16 #include <univalue.h>
    

     0$ ./find_redundant_includes.sh
     1src/bench/rpc_blockchain.cpp -- unused include: validation.h
     2src/bench/verify_script.cpp -- unused include: script/standard.h
     3src/coins.h -- unused include: core_memusage.h
     4src/core_read.cpp -- unused include: script/sign.h
     5src/crypto/hkdf_sha256_32.h -- unused include: crypto/hmac_sha256.h
     6src/index/txindex.h -- unused include: txdb.h
     7src/net.h -- unused include: addrdb.h
     8src/net.h -- unused include: limitedmap.h
     9src/net.h -- unused include: policy/feerate.h
    10src/noui.cpp -- unused include: util/system.h
    11src/psbt.h -- unused include: node/transaction.h
    12src/psbt.h -- unused include: optional.h
    13src/psbt.h -- unused include: policy/feerate.h
    14src/qt/bantablemodel.h -- unused include: net.h
    15src/qt/receivecoinsdialog.cpp -- unused include: wallet/wallet.h
    16src/qt/sendcoinsdialog.cpp -- unused include: txmempool.h
    17src/qt/sendcoinsdialog.cpp -- unused include: wallet/fees.h
    18src/qt/signverifymessagedialog.cpp -- unused include: wallet/wallet.h
    19src/qt/walletmodel.h -- unused include: script/standard.h
    20src/qt/winshutdownmonitor.cpp -- unused include: util/system.h
    21src/rpc/misc.cpp -- unused include: rpc/blockchain.h
    22src/rpc/util.h -- unused include: node/transaction.h
    23src/rpc/util.h -- unused include: script/sign.h
    24src/script/sign.h -- unused include: streams.h
    25src/support/events.h -- unused include: ios
    26src/sync.h -- unused include: thread
    27src/test/fuzz/FuzzedDataProvider.h -- unused include: utility
    28src/test/fuzz/eval_script.cpp -- unused include: limits
    29src/test/key_tests.cpp -- unused include: util/system.h
    30src/txmempool.h -- unused include: random.h
    31src/uint256.h -- unused include: assert.h
    32src/util/system.h -- unused include: util/memory.h
    33src/util/system.h -- unused include: util/time.h
    34src/wallet/coinselection.h -- unused include: random.h
    35src/wallet/crypter.h -- unused include: script/signingprovider.h
    36src/wallet/test/wallet_tests.cpp -- unused include: rpc/server.h
    37src/wallet/wallet.cpp -- unused include: util/validation.h
    38src/wallet/walletdb.h -- unused include: script/sign.h
    39src/zmq/zmqabstractnotifier.h -- unused include: zmq/zmqconfig.h
    40src/zmq/zmqconfig.h -- unused include: primitives/transaction.h
    41src/zmq/zmqconfig.h -- unused include: stdarg.h
    

    Anyone who wants to address the above cases too? Then we should be as lean as it gets when it comes to redundant includes :)

    If you do: let me know if you find any false positives/false negatives given the list above.

    One caveat is that I’m currently skipping analysis for core_io.h due to the weird naming mismatch between core_io.h ⇄ core_write.cpp/core_read.cpp :)

68. 
    laanwj commented at 8:58 am on October 17, 2019: member

    One caveat is that I’m currently skipping analysis for core_io.h

    It suppose that is a trivial header anyway, memory usage wise. It’s a few simple functions. It doesn’t pull in any boost nor non-run-of-the-mill C++ standard library headers.

69. sidhujag referenced this in commit 288cc1fa9f on Oct 18, 2019
70. deadalnix referenced this in commit 2e7115ee28 on May 6, 2020
71. deadalnix referenced this in commit a3160568ef on Jun 15, 2020
72. fanquake referenced this in commit e658b0e49b on Mar 27, 2021
73. sidhujag referenced this in commit adcbe1adaf on Mar 27, 2021
74. practicalswift deleted the branch on Apr 10, 2021
75. PastaPastaPasta referenced this in commit 579ff9f89d on Jun 27, 2021
76. PastaPastaPasta referenced this in commit 803786cb8b on Jun 28, 2021
77. PastaPastaPasta referenced this in commit 67dd77e43b on Jun 29, 2021
78. PastaPastaPasta referenced this in commit 3c5dcb036a on Dec 13, 2021
79. str4d referenced this in commit c4e9e1c134 on Jul 16, 2022
80. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift GChuf jb55 DrahtBot emilengler promag l2a5b1 laanwj ryanofsky MarcoFalke

Labels
Refactoring Needs Conceptual Review

Milestone
0.20.0