[moveonly] Fix CConnman template methods to be fully-defined in net.h

Empact commented at 7:59 pm on May 15, 2018: member

Currently template methods CConnman::ForEachNode and CConnman::ForEachNodeThen rely on CConnman::NodeFullyConnected, which is defined in net.cpp. This can result in linker errors like so: https://travis-ci.org/bitcoin/bitcoin/jobs/379011082

The fix is to move CNode’s declaration above CConnman in net.h, so that CConnman can make use of CNode members in the method implementations, and move NodeFullyConnected from net.cpp to net.h.

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

It could be helpful to review using git diff --color-moved=dimmed_zebra --minimal

Empact force-pushed on May 15, 2018

fanquake added the label Refactoring on May 15, 2018

fanquake added the label P2P on May 15, 2018

promag commented at 12:29 pm on May 16, 2018: member

utACK d53c3a6.

Empact force-pushed on May 16, 2018

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

Rebased

sipa commented at 8:33 pm on May 18, 2018: member

utACK ff9e94a65f990905abc48aafae2d5ff0cb61778b

--color-moved=dimmed_zebra is very useful, I didn’t know about that option.

Empact commented at 8:41 pm on May 18, 2018: member

@sipa Agreed! Was just added: https://blog.github.com/2018-04-05-git-217-released/

Learned about it from @ajtowns here: #13021 (comment)

MarcoFalke commented at 9:22 pm on May 18, 2018: member

NodeFullyConnected is not a templated method, so I don’t see why it needs to be fully-defined. (Using it in a templated method should be fine, no? Also note that this previously compiled without issues.)

Empact commented at 9:47 pm on May 18, 2018: member

@MarcoFalke here’s the build failure from #13235. Is there another approach you recommend?

0libtool: link: /usr/bin/ccache arm-linux-gnueabihf-g++ -std=c++11 -Wstack-protector -fstack-protector-all -fPIE -pipe -O2 -fvisibility=hidden -Wl,--exclude-libs -Wl,ALL -pthread -Wl,-z -Wl,relro -Wl,-z -Wl,now -pie -o bench/bench_bitcoin bench/bench_bench_bitcoin-bench_bitcoin.o bench/bench_bench_bitcoin-bench.o bench/bench_bench_bitcoin-checkblock.o bench/bench_bench_bitcoin-checkqueue.o bench/bench_bench_bitcoin-Examples.o bench/bench_bench_bitcoin-rollingbloom.o bench/bench_bench_bitcoin-crypto_hash.o bench/bench_bench_bitcoin-ccoins_caching.o bench/bench_bench_bitcoin-mempool_eviction.o bench/bench_bench_bitcoin-verify_script.o bench/bench_bench_bitcoin-base58.o bench/bench_bench_bitcoin-lockedpool.o bench/bench_bench_bitcoin-prevector.o bench/bench_bench_bitcoin-coin_selection.o  -L/home/travis/build/bitcoin/bitcoin/depends/arm-linux-gnueabihf/share/../lib libbitcoin_server.a libbitcoin_wallet.a libbitcoin_common.a libbitcoin_util.a libbitcoin_consensus.a crypto/libbitcoin_crypto.a leveldb/libleveldb.a leveldb/libleveldb_sse42.a leveldb/libmemenv.a secp256k1/.libs/libsecp256k1.a univalue/.libs/libunivalue.a libbitcoin_zmq.a -L/home/travis/build/bitcoin/bitcoin/depends/arm-linux-gnueabihf/lib /home/travis/build/bitcoin/bitcoin/depends/arm-linux-gnueabihf/lib/libzmq.a -lpthread -ldl -lboost_system-mt -lboost_filesystem-mt -lboost_program_options-mt -lboost_thread-mt -lboost_chrono-mt -ldb_cxx-4.8 -lssl -lcrypto -ldl -lcrypto -ldl -lminiupnpc /home/travis/build/bitcoin/bitcoin/depends/arm-linux-gnueabihf/lib/libevent_pthreads.a /home/travis/build/bitcoin/bitcoin/depends/arm-linux-gnueabihf/lib/libevent.a -lrt -pthread
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

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

Note CConnman::NodeFullyConnected is defined in net.cpp, which is available in libbitcoin_server.a, which is present.

Empact commented at 9:49 pm on May 18, 2018: member

I spent a bit of time going down the path of adding the net.cpp sources to libbitcoin_wallet.a, but this ended up seeming like the more narrow fix, because the other approach kept wanting to pull more of the project into the wallet lib. I could see an argument for factoring the CConnman dependency out of CWallet.

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

Concept ACK

[moveonly] Move CNode declaration above CConnman in net.h

This enables CConnman template methods to have access to a full declaration
of CNode, such that their implementation can be fully contained in net.h.

b9ed349f42

[moveonly] Move CConnman::NodeFullyConnected to CNode::FullyConnected

The CConnman template methods ForEachNode and ForEachNodeThen use
NodeFullyConnected, so when defined in net.cpp, outside callers were lacking
the implementation necessary for the template methods.

See https://travis-ci.org/bitcoin/bitcoin/jobs/379011082 for an example
of how this failure looked in practice.

This existed on CConnman before because
CConnman could not rely on CNode methods for its template methods. Now that the
CNode declaration is first, this is the more natural place to represent this
code.

118948996d

in src/net.h:194 in e2db96e7ac outdated

343-    size_t GetNodeCount(NumConnections num);
344-    void GetNodeStats(std::vector<CNodeStats>& vstats);
345-    bool DisconnectNode(const std::string& node);
346-    bool DisconnectNode(NodeId id);
347+/** Information about a peer */
348+class CConnman;

MarcoFalke commented at 2:20 pm on May 24, 2018:

Conman is not “information about a peer”

Empact commented at 10:07 pm on May 24, 2018:

Fixed

Empact force-pushed on May 24, 2018

Empact commented at 10:07 pm on May 24, 2018: member

Fixed comment location in e2db96e, and squashed the latter two commits.

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

Unnecessary thanks to build fix now in #13235

Empact closed this on Jun 14, 2018

Empact deleted the branch on Jun 14, 2018

MarcoFalke locked this on Sep 8, 2021

[moveonly] Fix CConnman template methods to be fully-defined in net.h #13239