refactor: Remove unused includes from wallet.cpp #28200

maflcko · 2023-08-02T14:29:39Z

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 · 2023-08-02T14:29:42Z

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.

theuni · 2023-08-02T14:45:11Z

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 · 2023-08-02T14:58:23Z

Concept ACK

maflcko · 2023-08-02T15:32:30Z

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

ryanofsky

Code review ACK ccccbd8

src/wallet/wallet.cpp

hebasto

ACK fa89408

hebasto · 2023-08-03T07:29:58Z

src/txmempool.h

+#include <kernel/mempool_entry.h>          // IWYU pragma: export
+#include <kernel/mempool_limits.h>         // IWYU pragma: export
+#include <kernel/mempool_options.h>        // IWYU pragma: export
+#include <kernel/mempool_removal_reason.h> // IWYU pragma: export


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.

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

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

include-what-you-use/include-what-you-use#1280

fanquake · 2023-08-03T10:14:22Z

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 · 2023-08-03T15:52:16Z

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 · 2023-08-07T06:26:02Z

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 · 2023-08-07T06:30:46Z

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 · 2023-08-07T06:50:50Z

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 · 2023-08-07T08:13:02Z

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

TheCharlatan · 2023-08-07T15:16:35Z

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 · 2023-08-07T15:25:52Z

Can you post a patch that compiles?

RandyMcMillan · 2023-08-07T17:52:00Z

Concept ACK

macOS Catalina Version 10.15.7 (19H2026)

maflcko · 2023-08-08T06:16:37Z

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

TheCharlatan · 2023-08-08T11:58:36Z

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 · 2023-08-08T12:03:39Z

Seems like an easy win, no?

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

maflcko · 2023-08-08T12:56:27Z

Removing boost seems unrelated and can be done in a follow-up. You can help by reviewing the first step: #28240

TheCharlatan

ACK fac3e2f

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.

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

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

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

This removes unused includes, such as undo.h or txmempool.h from wallet.cpp. Also, add missing ones, according to IWYU.

maflcko · 2023-08-18T14:09:27Z

rebased, should be trivial to re-ACK

hebasto

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

…e time (#5693) ## Issue being fixed or feature implemented Some headers include other heavy headers, such as `logging.h`, `tinyformat.h`, `iostream`. These headers are heavy and increase compilation time on scale of whole project drastically because can be used in many other headers. ## What was done? Moved many heavy includes from headers to cpp files to optimize compilation time. In some places added forward declarations if it is reasonable. As side effect removed 2 circular dependencies: ``` "llmq/debug -> llmq/dkgsessionhandler -> llmq/debug" "llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug" ``` ## How Has This Been Tested? Run build 2 times before refactoring and after refactoring: `make clean && sleep 10s; time make -j18` Before refactoring: ``` real 5m37,826s user 77m12,075s sys 6m20,547s real 5m32,626s user 76m51,143s sys 6m24,511s ``` After refactoring: ``` real 5m18,509s user 73m32,133s sys 6m21,590s real 5m14,466s user 73m20,942s sys 6m17,868s ``` ~5% of improvement for compilation time. That's not huge, but that's worth to get merged There're several more refactorings TODO but better to do them later by backports: - bitcoin#27636 - bitcoin#26286 - bitcoin#27238 - and maybe this one: bitcoin#28200 ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone

DrahtBot added the Refactoring label Aug 2, 2023

maflcko force-pushed the 2308-wallet-smaller- branch from fa43d8a to fa8357c Compare August 2, 2023 15:19

DrahtBot added the CI failed label Aug 2, 2023

maflcko force-pushed the 2308-wallet-smaller- branch 3 times, most recently from fa31838 to ccccbd8 Compare August 2, 2023 15:32

ryanofsky approved these changes Aug 2, 2023

View reviewed changes

hebasto reviewed Aug 2, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 2, 2023

Dialog for allowing the user to choose the change output when bumping a tx bitcoin-core/gui#700

Open

maflcko force-pushed the 2308-wallet-smaller- branch from ccccbd8 to fa89408 Compare August 3, 2023 06:20

hebasto approved these changes Aug 3, 2023

View reviewed changes

DrahtBot requested a review from ryanofsky August 3, 2023 07:21

hebasto reviewed Aug 3, 2023

View reviewed changes

DrahtBot removed the CI failed label Aug 3, 2023

DrahtBot mentioned this pull request Aug 3, 2023

RPC/Wallet: Add "use_txids" to output of getaddressinfo #22693

Open

maflcko force-pushed the 2308-wallet-smaller- branch from fa89408 to fac3e2f Compare August 7, 2023 08:12

TheCharlatan approved these changes Aug 8, 2023

View reviewed changes

DrahtBot requested a review from hebasto August 8, 2023 14:35

This was referenced Aug 9, 2023

assumeutxo (2) #27596

Merged

wallet: Use CTxDestination in CRecipient instead of just scriptPubKey #28246

Merged

Break up script/standard.{h/cpp} #28244

Merged

Silent Payments: Implement BIP352 #28122

Draft

DrahtBot added the Needs rebase label Aug 17, 2023

MarcoFalke added 2 commits August 17, 2023 16:25

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

fad8c36

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

maflcko force-pushed the 2308-wallet-smaller- branch 2 times, most recently from face659 to 22223a7 Compare August 17, 2023 14:47

DrahtBot added CI failed and removed Needs rebase labels Aug 17, 2023

Remove unused includes from blockfilter.h

fa8fdbe

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

maflcko force-pushed the 2308-wallet-smaller- branch from 22223a7 to fab5376 Compare August 17, 2023 16:33

Remove unused includes from wallet.cpp

fa62868

This removes unused includes, such as undo.h or txmempool.h from wallet.cpp. Also, add missing ones, according to IWYU.

maflcko force-pushed the 2308-wallet-smaller- branch from fab5376 to fa62868 Compare August 18, 2023 06:21

DrahtBot removed the CI failed label Aug 18, 2023

maflcko requested a review from TheCharlatan August 21, 2023 16:05

hebasto approved these changes Aug 21, 2023

View reviewed changes

fanquake merged commit 00fc7cd into bitcoin:master Aug 22, 2023
15 checks passed

maflcko deleted the 2308-wallet-smaller- branch August 22, 2023 09:48

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023

Merge bitcoin#28200: refactor: Remove unused includes from wallet.cpp

f906fd7

knst mentioned this pull request Nov 13, 2023

refactor: re-order headers and forward declarations to improve compile time dashpay/dash#5693

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Remove unused includes from wallet.cpp #28200

refactor: Remove unused includes from wallet.cpp #28200

maflcko commented Aug 2, 2023

DrahtBot commented Aug 2, 2023 •

edited

theuni commented Aug 2, 2023

jonatack commented Aug 2, 2023

maflcko commented Aug 2, 2023

ryanofsky left a comment

hebasto left a comment

hebasto Aug 3, 2023 •

edited

maflcko Aug 3, 2023

hebasto Aug 3, 2023

fanquake commented Aug 3, 2023

theuni commented Aug 3, 2023

Empact commented Aug 7, 2023 •

edited

maflcko commented Aug 7, 2023

Empact commented Aug 7, 2023

maflcko commented Aug 7, 2023

TheCharlatan commented Aug 7, 2023

maflcko commented Aug 7, 2023

RandyMcMillan commented Aug 7, 2023

maflcko commented Aug 8, 2023

TheCharlatan commented Aug 8, 2023

maflcko commented Aug 8, 2023

maflcko commented Aug 8, 2023

TheCharlatan left a comment

maflcko commented Aug 18, 2023

hebasto left a comment

refactor: Remove unused includes from wallet.cpp #28200

refactor: Remove unused includes from wallet.cpp #28200

Conversation

maflcko commented Aug 2, 2023

DrahtBot commented Aug 2, 2023 • edited

Reviews

Conflicts

theuni commented Aug 2, 2023

jonatack commented Aug 2, 2023

maflcko commented Aug 2, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

hebasto Aug 3, 2023 • edited

Choose a reason for hiding this comment

maflcko Aug 3, 2023

Choose a reason for hiding this comment

hebasto Aug 3, 2023

Choose a reason for hiding this comment

fanquake commented Aug 3, 2023

theuni commented Aug 3, 2023

Empact commented Aug 7, 2023 • edited

maflcko commented Aug 7, 2023

Empact commented Aug 7, 2023

maflcko commented Aug 7, 2023

TheCharlatan commented Aug 7, 2023

maflcko commented Aug 7, 2023

RandyMcMillan commented Aug 7, 2023

maflcko commented Aug 8, 2023

TheCharlatan commented Aug 8, 2023

maflcko commented Aug 8, 2023

maflcko commented Aug 8, 2023

TheCharlatan left a comment

Choose a reason for hiding this comment

maflcko commented Aug 18, 2023

hebasto left a comment

Choose a reason for hiding this comment

DrahtBot commented Aug 2, 2023 •

edited

hebasto Aug 3, 2023 •

edited

Empact commented Aug 7, 2023 •

edited