Break circular dependency: init -> * -> init by extracting shutdown.h

Empact commented at 1:40 am on May 15, 2018: member

Most includers just wanted to react to pending shutdown.

This isolates access to fRequestShutdown and limits access to the shutdown api functions, including the new CancelShutdown for setting it to false.

Empact force-pushed on May 15, 2018

Empact commented at 1:41 am on May 15, 2018: member

Prompted by #13228

fanquake added the label Refactoring on May 15, 2018

fanquake commented at 4:15 am on May 15, 2018: member

Failures in bench_bitcoin in a few of the Travis builds:

0  CXXLD    bench/bench_bitcoin
1libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `CWalletTx::RelayWalletTransaction(CConnman*)':
2wallet.cpp:(.text+0x5cea): undefined reference to `CConnman::NodeFullyConnected(CNode const*)'
3collect2: error: ld returned 1 exit status
4make[2]: *** [bench/bench_bitcoin] Error 1

Empact force-pushed on May 15, 2018

practicalswift commented at 7:19 am on May 15, 2018: contributor

Concept ACK

laanwj commented at 7:35 am on May 15, 2018: member

I entertained the idea of creating a shutdown.(cpp|h) a few times as well. It seems we have too much non-initialization-related things in init. Concept ACK.

Empact force-pushed on May 15, 2018

Empact commented at 8:02 pm on May 15, 2018: member

Based this on #13239, which fixes the linker issues.

Empact force-pushed on May 15, 2018

jonasschnelli commented at 6:40 am on May 16, 2018: contributor

utACK f7033bf5767e674abfee2bd53cd6b9f5c3831482

practicalswift commented at 7:06 am on May 16, 2018: contributor

utACK f7033bf5767e674abfee2bd53cd6b9f5c3831482

in src/shutdown.cpp:8 in f7033bf576 outdated

0@@ -0,0 +1,21 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2017 The Bitcoin Core developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#include <atomic>
7+
8+std::atomic<bool> fRequestShutdown(false);

promag commented at 11:47 am on May 16, 2018:

0static std::atomic<bool> fRequestShutdown{false};

in src/shutdown.h:2 in f7033bf576 outdated

0@@ -0,0 +1,13 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2017 The Bitcoin Core developers

promag commented at 11:47 am on May 16, 2018:

in src/txdb.cpp:15 in f7033bf576 outdated

11@@ -12,7 +12,7 @@
12 #include <uint256.h>
13 #include <util.h>
14 #include <ui_interface.h>
15-#include <init.h>
16+#include <shutdown.h>

promag commented at 11:47 am on May 16, 2018:

Keep sorted.

MarcoFalke commented at 2:04 pm on May 16, 2018:

Note that you can install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script which fixes all white space and sorting issues

in src/shutdown.cpp:8 in f7033bf576 outdated

0@@ -0,0 +1,21 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2017 The Bitcoin Core developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#include <atomic>

promag commented at 11:57 am on May 16, 2018:

nit, also #include <shutdown.h>?

promag commented at 11:59 am on May 16, 2018: member

Concept ACK.

The PR suggests that the following dependencies wouldn’t exist:

0Circular dependency: init -> net_processing -> init
1Circular dependency: init -> validationinterface -> init

~~Also, I’ve removed the first 3 commits locally and it still compiles and reports the same dependencies as above. I wonder why are those commits included here.~~

Edit: oh this is rebased on #13239.

Empact force-pushed on May 16, 2018

Empact commented at 7:28 pm on May 16, 2018: member

Declared fRequestShutdown static
Included shutdown.h in shutdown.cpp
Updated copyright years
Ran clang-format to sort includes

Thanks @promag / @MarcoFalke

promag commented at 7:33 pm on May 16, 2018: member

Still has the following circular dependencies:

0Circular dependency: init -> net_processing -> init
1Circular dependency: init -> validationinterface -> init

Should these be removed in this PR?

Empact commented at 7:52 pm on May 16, 2018: member

@promag Added a commit to remove unused includes of init from 4 files. I’d looked into those before but left them out to narrow this PR.

promag commented at 7:52 pm on May 16, 2018: member

@Empact needs rebase.

Empact force-pushed on May 16, 2018

Empact commented at 8:12 pm on May 16, 2018: member

Rebased

Empact commented at 6:40 pm on May 18, 2018: member

~~@jonasschnelli @practicalswift would you take a look at #13239? It’s the necessary precondition for this.~~

Empact force-pushed on May 18, 2018

Empact force-pushed on May 24, 2018

Empact force-pushed on Jun 1, 2018

Empact force-pushed on Jun 2, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

Empact force-pushed on Jun 6, 2018

Empact commented at 9:35 am on June 6, 2018: member

Rebased for #13191 - note this is WIP because I’m working on a Makefile-based solution to the ld failures (which unfortunately do not exist on mac), as an alternative to #13239.

MarcoFalke removed the label Needs rebase on Jun 6, 2018

DrahtBot added the label Needs rebase on Jun 7, 2018

sipa commented at 7:23 am on June 14, 2018: member

@Empact Any progress here?

Empact commented at 7:46 am on June 14, 2018: member

@sipa yeah what I need to do is install linux so that I can repro the build failures locally (unfortunately the only system they’re not exhibiting on is Mac). The last commit 665bf42 is a half-finished effort to fix the build with feedback via CI but it would progress a lot faster if I had local failures to run against.

I’ll work on it a bit more in the same style, but failing that I’ll have to set up dual-boot or the like.

Empact force-pushed on Jun 14, 2018

Empact commented at 7:56 am on June 14, 2018: member

I’m going to try again at a more focused intervention.

Empact force-pushed on Jun 14, 2018

promag commented at 8:55 am on June 14, 2018: member

@sipa yeah what I need to do is install linux so that I can repro the build failures locally @Empact you could use docker instead.

Empact commented at 8:58 am on June 14, 2018: member

@promag Good point! I think I’m close to reproing it on mac - the change I made in 44bda43 inverts the test failures, so that the mac build is failing now. https://travis-ci.org/bitcoin/bitcoin/builds/392143625?utm_source=github_status&utm_medium=notification

Empact force-pushed on Jun 14, 2018

Empact commented at 10:02 am on June 14, 2018: member

Ok, looks like an intermittent failure in 69b0e8a, ending in:

 0make[1]: Entering directory '/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-apple-darwin11'
 1make[1]: Nothing to be done for 'all-am'.
 2make[1]: Leaving directory '/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-apple-darwin11'
 3make -C src qt/bitcoin-qt
 4make[1]: Entering directory '/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-apple-darwin11/src'
 5make[1]: Leaving directory '/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-apple-darwin11/src'
 6/bin/mkdir -p Bitcoin-Qt.app/Contents/MacOS
 7STRIPPROG="/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../native/bin/x86_64-apple-darwin11-strip" /bin/bash /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-apple-darwin11/build-aux/install-sh -c -s  src/qt/bitcoin-qt Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt
 8INSTALLNAMETOOL=/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../native/bin/x86_64-apple-darwin11-install_name_tool  OTOOL=/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../native/bin/x86_64-apple-darwin11-otool STRIP=/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../native/bin/x86_64-apple-darwin11-strip /usr/bin/python3.6 ./contrib/macdeploy/macdeployqtplus Bitcoin-Qt.app -translations-dir=/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../translations -add-qt-tr da,de,es,hu,ru,uk,zh_CN,zh_TW -verbose 2
 9+ Copying source bundle +
10+ Deploying frameworks +
11Warning: Could not find any external frameworks to deploy in dist/Bitcoin-Qt.app.
12+ Installing qt.conf +
13+ Adding Qt translations +
14+ Done +
15Warning: Could not detect Qt's path, skipping plugin deployment!
16/bin/mkdir -p dist/.background
17/usr/bin/tiffcp -c none dpi36.background.tiff dpi72.background.tiff dist/.background/background.tiff
18/usr/bin/python3.6 contrib/macdeploy/custom_dsstore.py "dist/.DS_Store" "Bitcoin-Core"
19/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../native/bin/genisoimage -no-cache-inodes -D -l -probe -V "Bitcoin-Core" -no-pad -r -dir-mode 0755 -apple -o Bitcoin-Core.dmg dist
20/home/travis/build/bitcoin/bitcoin/depends/x86_64-apple-darwin11/share/../native/bin/genisoimage: Warning: assuming PC Exchange cluster size of 512 bytes
21 30.79% done, estimate finish Thu Jun 14 09:33:16 2018
22 61.60% done, estimate finish Thu Jun 14 09:33:16 2018
23 92.30% done, estimate finish Thu Jun 14 09:33:16 2018
24Total translation table size: 0
25Total rockridge attributes bytes: 3197
26Total directory bytes: 13084
27Path table size(bytes): 118
28Max brk space used 0
2916257 extents written (31 MB)

https://travis-ci.org/bitcoin/bitcoin/jobs/392160315

Seems like it’s in master, given this is also failing: https://travis-ci.org/bitcoin/bitcoin/jobs/392060921

~~I don’t know if 57863ab is an actual fix, or just that the problem is intermittent / unrelated and just hasn’t presented itself yet.~~

Edit: Opened #13469 for this.

Empact commented at 10:19 am on June 14, 2018: member

~~Failure in 63cebe7 confirms that 57863ab is not a fix. Dropping and squashing.~~

Empact force-pushed on Jun 14, 2018

Empact commented at 10:45 am on June 14, 2018: member

Ok, I think this is ready for re-review. @sipa thanks for the instigation.

circular-dependencies.py output before, with removals in after indicated with strikethrough:

src$ ../contrib/devtools/circular-dependencies.py {*,*/*,*/*/*}.{h,cpp} Circular dependency: chainparamsbase -> util -> chainparamsbase Circular dependency: checkpoints -> validation -> checkpoints ~~Circular dependency: index/txindex -> init -> index/txindex~~ Circular dependency: index/txindex -> validation -> index/txindex ~~Circular dependency: init -> net_processing -> init~~ ~~Circular dependency: init -> rpc/server -> init~~ ~~Circular dependency: init -> txdb -> init~~ ~~Circular dependency: init -> validation -> init~~ ~~Circular dependency: init -> validationinterface -> init~~ Circular dependency: policy/fees -> txmempool -> policy/fees Circular dependency: policy/policy -> validation -> policy/policy Circular dependency: qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel Circular dependency: qt/bantablemodel -> qt/clientmodel -> qt/bantablemodel Circular dependency: qt/bitcoingui -> qt/utilitydialog -> qt/bitcoingui Circular dependency: qt/bitcoingui -> qt/walletframe -> qt/bitcoingui Circular dependency: qt/bitcoingui -> qt/walletview -> qt/bitcoingui Circular dependency: qt/clientmodel -> qt/peertablemodel -> qt/clientmodel Circular dependency: qt/paymentserver -> qt/walletmodel -> qt/paymentserver Circular dependency: qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel Circular dependency: qt/sendcoinsdialog -> qt/walletmodel -> qt/sendcoinsdialog Circular dependency: qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel Circular dependency: qt/walletmodel -> qt/walletmodeltransaction -> qt/walletmodel Circular dependency: rpc/rawtransaction -> wallet/rpcwallet -> rpc/rawtransaction Circular dependency: txmempool -> validation -> txmempool Circular dependency: validation -> validationinterface -> validation Circular dependency: wallet/coincontrol -> wallet/wallet -> wallet/coincontrol Circular dependency: wallet/fees -> wallet/wallet -> wallet/fees Circular dependency: wallet/rpcwallet -> wallet/wallet -> wallet/rpcwallet Circular dependency: wallet/wallet -> wallet/walletdb -> wallet/wallet ~~Circular dependency: index/txindex -> init -> rpc/blockchain -> index/txindex~~ Circular dependency: policy/fees -> policy/policy -> validation -> policy/fees Circular dependency: policy/rbf -> txmempool -> validation -> policy/rbf Circular dependency: qt/addressbookpage -> qt/bitcoingui -> qt/walletview -> qt/addressbookpage Circular dependency: qt/guiutil -> qt/walletmodel -> qt/optionsmodel -> qt/guiutil Circular dependency: txmempool -> validation -> validationinterface -> txmempool Circular dependency: qt/addressbookpage -> qt/bitcoingui -> qt/walletview -> qt/receivecoinsdialog -> qt/addressbookpage Circular dependency: qt/addressbookpage -> qt/bitcoingui -> qt/walletview -> qt/signverifymessagedialog -> qt/addressbookpage Circular dependency: qt/guiutil -> qt/walletmodel -> qt/optionsmodel -> qt/intro -> qt/guiutil Circular dependency: qt/addressbookpage -> qt/bitcoingui -> qt/walletview -> qt/sendcoinsdialog -> qt/sendcoinsentry -> qt/addressbookpage

Empact force-pushed on Jun 14, 2018

DrahtBot removed the label Needs rebase on Jun 14, 2018

ken2812221 approved

ken2812221 commented at 5:21 pm on June 20, 2018: contributor

utACK ab30690763b9396abdf2b44f7ef1077ad7ffd29e

Empact commented at 6:08 pm on June 22, 2018: member

@jonasschnelli @practicalswift @promag @sipa take another look?

DrahtBot added the label Needs rebase on Jun 24, 2018

DrahtBot commented at 2:20 pm on June 24, 2018: member

Drop unused init.h includes

These were entirely unused, as based on successful compilation
and a grep for:
  \bStartShutdown\(\)|\bShutdownRequested\(\)|\bInterrupt\(\)|\bShutdown\(\)|\bInitLogging\(\)|\bInitParameterInteraction\(\)|\bAppInitBasicSetup\(\)|\bAppInitParameterInteraction\(\)|\bAppInitSanityChecks\(\)|\bAppInitLockDataDirectory\(\)|\bAppInitMain\(\)|\bSetupServerArgs\(\)|\bLicenseInfo\(\)|g_wallet_init_interface|init.h

e62fdfeeab

Break circular dependency: init -> * -> init by extracting shutdown.h

Most includers just wanted to react to pending shutdown.

This isolates access to `fRequestShutdown` and limits access to the shutdown
api functions, including the new `AbortShutdown` for setting it to `false`.

Note I originally called `AbortShutdown` `CancelShutdown` but that name was
already taken by winuser.h
https://travis-ci.org/bitcoin/bitcoin/jobs/386913329

This change also triggered a build error in bench. Fixing it required moving LIBBITCOIN_SERVER after LIBBITCOIN_WALLET in bench_bench_bitcoin_LDADD To make
server definitions in src/net.cpp available to wallet methods in
src/wallet/wallet.cpp. Specifically, solving:

  libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `CWalletTx::RelayWalletTransaction(CConnman*)':
  wallet.cpp:(.text+0x3f0e): undefined reference to `CConnman::NodeFullyConnected(CNode const*)'
  collect2: error: ld returned 1 exit status

https://travis-ci.org/bitcoin/bitcoin/jobs/392133581

Need for remaining init.h includes confirmed via a thorough search with a more
specific regex:
  \bInterrupt\(\)|\bShutdown\(\)|\bInitLogging\(\)|\bInitParameterInteraction\(\)|\bAppInitBasicSetup\(\)|\bAppInitParameterInteraction\(\)|\bAppInitSanityChecks\(\)|\bAppInitLockDataDirectory\(\)|\bAppInitMain\(\)|\bSetupServerArgs\(\)|\bLicenseInfo\(\)|g_wallet_init_interface|init.h

1fabd59e7e

Empact force-pushed on Jun 25, 2018

Empact commented at 4:23 am on June 25, 2018: member

Rebased for #13458

ken2812221 commented at 9:12 am on June 25, 2018: contributor

utACK 1fabd59e7e870fae73bfbcfb227dd7452de94726

DrahtBot removed the label Needs rebase on Jun 25, 2018

sipa commented at 0:22 am on June 27, 2018: member

@Empact In your first commit you write “These were entirely unused, as based on successful compilation”. That’s not a sufficient argument - init.h could still be included indirectly through another header, while exported symbols from init.h are still used.

This doesn’t mean it wrong to remove them, just that “it compiles without them” isn’t a sufficient reason. Tools like iwyu can help with this.

Empact commented at 0:32 am on June 27, 2018: member

@sipa thanks I’ll check that out

Empact commented at 1:17 am on June 27, 2018: member

Oh btw I did manually check using this grep:

0  \bStartShutdown\(\)|\bShutdownRequested\(\)|\bInterrupt\(\)|\bShutdown\(\)|\bInitLogging\(\)|\bInitParameterInteraction\(\)|\bAppInitBasicSetup\(\)|\bAppInitParameterInteraction\(\)|\bAppInitSanityChecks\(\)|\bAppInitLockDataDirectory\(\)|\bAppInitMain\(\)|\bSetupServerArgs\(\)|\bLicenseInfo\(\)|g_wallet_init_interface|init.h

But I’ll check again with the above and using iwyu, which is probably more robust.

sipa commented at 1:26 am on June 27, 2018: member

@Empact Oh, ignore my comment in that case. I just wanted to make sure you’re not relying on indirect inclusions.

sipa commented at 1:28 am on June 27, 2018: member

utACK 1fabd59e7e870fae73bfbcfb227dd7452de94726

Empact commented at 7:19 pm on July 1, 2018: member

@practicalswift @jonasschnelli @promag could I get another look?

Alternatively I could split this into 2 PRS, the removal of the unnecessary includes and the shutdown.h extraction, if that would make review easier.

laanwj commented at 1:28 pm on July 4, 2018: member

utACK 1fabd59e7e870fae73bfbcfb227dd7452de94726

laanwj merged this on Jul 4, 2018

laanwj closed this on Jul 4, 2018

laanwj referenced this in commit 79e677950b on Jul 4, 2018

UdjinM6 referenced this in commit 499cd95458 on Jun 20, 2021

UdjinM6 referenced this in commit b4c2f4317f on Jun 20, 2021

UdjinM6 referenced this in commit 2f2317a61b on Jun 20, 2021

UdjinM6 referenced this in commit 721fda8275 on Jun 20, 2021

UdjinM6 referenced this in commit 77fa7e7d35 on Jun 20, 2021

UdjinM6 referenced this in commit 90a39ea81b on Jun 24, 2021

UdjinM6 referenced this in commit c12328e757 on Jun 26, 2021

UdjinM6 referenced this in commit 7ae2ff6476 on Jun 29, 2021

UdjinM6 referenced this in commit 369b31db80 on Jun 29, 2021

UdjinM6 referenced this in commit 4cd32e43e4 on Jul 1, 2021

DrahtBot locked this on Sep 8, 2021

Break circular dependency: init -> * -> init by extracting shutdown.h #13235