refactor: Remove unused includes from wallet.cpp #28200

maflcko commented at 2:29 PM on August 2, 2023: member

This makes compilation of wallet.cpp use a few % less memory and time, locally.

Created in the context of #28109, but I don't think it is enough to actually fix this problem.

DrahtBot commented at 2:29 PM on August 2, 2023: 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	hebasto
Concept ACK	jonatack, theuni, Empact, RandyMcMillan
Stale ACK	ryanofsky, TheCharlatan

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:

#bitcoin-core/gui/700 (Dialog for allowing the user to choose the change output when bumping a tx by achow101)
#22693 (RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr)

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 Refactoring on Aug 2, 2023

theuni commented at 2:45 PM on August 2, 2023: member

Good job! The circular dependency "wallet/fees -> wallet/wallet -> wallet/fees" is no longer present. Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in test/lint/lint-circular-dependencies.py to make sure this circular dependency is not accidentally reintroduced.

🎉

jonatack commented at 2:58 PM on August 2, 2023: member

Concept ACK

maflcko force-pushed on Aug 2, 2023

DrahtBot added the label CI failed on Aug 2, 2023

maflcko force-pushed on Aug 2, 2023

maflcko commented at 3:32 PM on August 2, 2023: member

Thanks, force pushed to fix the linter and made the commit hash start with cccc

ryanofsky approved

ryanofsky commented at 4:48 PM on August 2, 2023: contributor

Code review ACK ccccbd8dd0f5cd74a744e74608c4ea102ec33fd1

in src/wallet/wallet.cpp:8 in ccccbd8dd0 outdated

   4 | @@ -5,50 +5,72 @@
   5 |  
   6 |  #include <wallet/wallet.h>
   7 |  
   8 | +#include <bitcoin-config.h>

hebasto commented at 6:51 PM on August 2, 2023:

#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif

maflcko force-pushed on Aug 3, 2023

hebasto approved

hebasto commented at 7:21 AM on August 3, 2023: member

ACK fa8940852f88f74cb0d4975e05ec32fd3594961c

DrahtBot requested review from ryanofsky on Aug 3, 2023

in src/txmempool.h:16 in fa3e188dda outdated

  24 |  #include <kernel/cs_main.h>
  25 | -#include <kernel/mempool_entry.h>
  26 | +#include <kernel/mempool_entry.h>          // IWYU pragma: export
  27 | +#include <kernel/mempool_limits.h>         // IWYU pragma: export
  28 | +#include <kernel/mempool_options.h>        // IWYU pragma: export
  29 | +#include <kernel/mempool_removal_reason.h> // IWYU pragma: export

hebasto commented at 7:29 AM on August 3, 2023:

It seems IWYU pragma: export causes:

txmempool.h should add these lines:
...
#include "kernel/mempool_limits.h"
#include "kernel/mempool_options.h"
#include "kernel/mempool_removal_reason.h"                   // for MemPoolRemovalReason (ptr only)
...

Perhaps, a better solution is to use contrib/devtools/iwyu/bitcoin.core.imp.

maflcko commented at 7:38 AM on August 3, 2023:

Yes, I am aware, but I think this bug should be reported to IWYU, or is it not a bug?

hebasto commented at 11:49 AM on August 3, 2023:

I think this bug should be reported to IWYU...

https://github.com/include-what-you-use/include-what-you-use/issues/1280

DrahtBot removed the label CI failed on Aug 3, 2023

fanquake commented at 10:14 AM on August 3, 2023: member

Created in the context of #28109, but I don't think it is enough to actually fix this problem.

It's a step in the right direction, we're now failing on 45934 sections, compared to 46006 in #28109. However we're still a long way north of the limit, which as I understand it, is 32768.

theuni commented at 3:52 PM on August 3, 2023: member

Concept ACK. I didn't review the includes changes in detail, but the move makes sense to me. And I'm always up for one-data-structure-per-header :)

Empact commented at 6:26 AM on August 7, 2023: member

Concept ACK

nit: IMO better to split the the iwyu adds/removals into a 3rd commit on this PR, somewhat in the spirit of the "Pull Request Philosophy"

maflcko commented at 6:30 AM on August 7, 2023: member

The second commit is basically iwyu on src/blockfilter.h and wallet.cpp/h, so I don't think it can be split further.

Empact commented at 6:50 AM on August 7, 2023: member

To be clear as to what I'm referring to, note your commit messages both end with "Also ...". This alludes to the fact that you're doing a variety of things in each, which is a burden (in this case a minor one) on review.

Feel free to disregard as a nit, but I think this is a good practice to keep in mind, that we may maintain discipline in how we structure our changes for the benefit of review.

maflcko force-pushed on Aug 7, 2023

maflcko commented at 8:13 AM on August 7, 2023: member

Thanks, I've split each commit to be iwyu targeted to one header/module. The overall force-push diff is zero.

TheCharlatan commented at 3:16 PM on August 7, 2023: contributor

Concept ACK

What about this one:

wallet/wallet.h should remove these lines:
- #include <boost/signals2/signal.hpp>  // lines 52-52

Probably should be moved to the implementation file, no?

maflcko commented at 3:25 PM on August 7, 2023: member

Can you post a patch that compiles?

RandyMcMillan commented at 5:52 PM on August 7, 2023: contributor

Concept ACK

Screen Shot 2023-08-07 at 1 49 13 PM

macOS Catalina Version 10.15.7 (19H2026)

maflcko commented at 6:16 AM on August 8, 2023: member

@RandyMcMillan That looks unrelated to the changes here. You'll have to use make clean && make distclean, or something similar

TheCharlatan commented at 11:58 AM on August 8, 2023: contributor

Re #28200 (comment)

Can you post a patch that compiles?

Seems like an easy win, no?

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7c74b6b9ff..fe4c3d0f67 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -79,6 +79,8 @@
 #include <tuple>
 #include <variant>
 
+#include <boost/signals2/signal.hpp>
+
 struct KeyOriginInfo;
 
 using interfaces::FoundBlock;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 8a1a513062..24cedd5160 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -49,8 +49,6 @@
 #include <utility>
 #include <vector>
 
-#include <boost/signals2/signal.hpp>
-
 class CKey;
 class CKeyID;
 class CPubKey;

maflcko commented at 12:03 PM on August 8, 2023: member

Seems like an easy win, no?

No. The boost include is still there (via src/wallet/scriptpubkeyman.h:#include <boost/signals2/signal.hpp>

maflcko commented at 12:56 PM on August 8, 2023: member

Removing boost seems unrelated and can be done in a follow-up. You can help by reviewing the first step: https://github.com/bitcoin/bitcoin/pull/28240

TheCharlatan approved

TheCharlatan commented at 2:35 PM on August 8, 2023: contributor

ACK fac3e2fa6adf9cb034ea5ee28c572b25cc7e3c2d

Re #28200 (comment)

No. The boost include is still there (via src/wallet/scriptpubkeyman.h:#include <boost/signals2/signal.hpp>

Yeah, totally. Should have checked this before trusting the iwyu suggestion.

DrahtBot requested review from hebasto on Aug 8, 2023

DrahtBot added the label Needs rebase on Aug 17, 2023

Remove unused includes from txmempool.h

... and move them to where they are really needed.

This was found by IWYU:

txmempool.h should remove these lines:
- #include <random.h>  // lines 29-29
- class CBlockIndex;  // lines 43-43
- class Chainstate;  // lines 45-45

Also, move the stdlib section to the right place. Can be reviewed with:
--color-moved=dimmed-zebra

fa57608800

move-only: Create src/kernel/mempool_removal_reason.h

This is needed for a future commit. Can be reviewed with:
--color-moved=dimmed-zebra

fad8c36aa9

maflcko force-pushed on Aug 17, 2023

DrahtBot added the label CI failed on Aug 17, 2023

DrahtBot removed the label Needs rebase on Aug 17, 2023

Remove unused includes from blockfilter.h

This removes unused includes, primitives/block found manually, and the
others by iwyu:

blockfilter.h should remove these lines:
- #include <serialize.h>  // lines 16-16
- #include <undo.h>  // lines 18-18

fa8fdbe229

maflcko force-pushed on Aug 17, 2023

Remove unused includes from wallet.cpp

This removes unused includes, such as undo.h or txmempool.h from
wallet.cpp.

Also, add missing ones, according to IWYU.

fa6286891f

maflcko force-pushed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

maflcko commented at 2:09 PM on August 18, 2023: member

rebased, should be trivial to re-ACK

maflcko requested review from TheCharlatan on Aug 21, 2023

hebasto approved

hebasto commented at 4:08 PM on August 21, 2023: member

ACK fa6286891fa4164510e4fbf4bc214ce3033b2d1b, I have reviewed the code and it looks OK.

fanquake merged this on Aug 22, 2023

fanquake closed this on Aug 22, 2023

maflcko deleted the branch on Aug 22, 2023

Frank-GER referenced this in commit f906fd7633 on Sep 8, 2023

PastaPastaPasta referenced this in commit ba97f49f2f on Nov 17, 2023

bitcoin locked this on Aug 21, 2024