Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Empact wants to merge 2 commits into from
Closed

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

Empact wants to merge 2 commits into from

Conversation

@Empact
Copy link
Contributor

@Empact Empact commented May 15, 2018 •
edited
Loading

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 Empact mentioned this pull request May 15, 2018
Merged
@Empact
Copy link
Contributor Author

Empact commented May 15, 2018 •
edited
Loading

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

@Empact Empact force-pushed the net-template-methods branch 2 times, most recently from b33d4a2 to d53c3a6 Compare May 15, 2018 20:48
@promag
Copy link
Contributor

promag commented May 16, 2018

utACK d53c3a6.

@Empact Empact force-pushed the net-template-methods branch from d53c3a6 to ff9e94a Compare May 16, 2018 20:10
@Empact
Copy link
Contributor Author

Empact commented May 16, 2018

Rebased

@sipa
Copy link
Member

sipa commented May 18, 2018

utACK ff9e94a65f990905abc48aafae2d5ff0cb61778b

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

@Empact
Copy link
Contributor Author

Empact commented May 18, 2018 •
edited
Loading

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

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

@maflcko
Copy link
Member

maflcko commented May 18, 2018

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
Copy link
Contributor Author

Empact commented May 18, 2018 •
edited
Loading

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

libtool: 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
libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `CWalletTx::RelayWalletTransaction(CConnman*)':
wallet.cpp:(.text+0x5cea): undefined reference to `CConnman::NodeFullyConnected(CNode const*)'
collect2: 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
Copy link
Contributor Author

Empact commented May 18, 2018 •
edited
Loading

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
Copy link
Contributor

practicalswift commented May 24, 2018

Concept ACK

maflcko
maflcko reviewed May 24, 2018
View reviewed changes
src/net.h Outdated
Copy link
Member

@maflcko maflcko May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conman is not "information about a peer"

Copy link
Contributor Author

@Empact Empact May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Empact added 2 commits May 24, 2018 15:00
@Empact
[moveonly] Move CNode declaration above CConnman in net.h
b9ed349 
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.
@Empact
[moveonly] Move CConnman::NodeFullyConnected to CNode::FullyConnected
1189489 
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.
@Empact Empact force-pushed the net-template-methods branch from ff9e94a to 1189489 Compare May 24, 2018 22:06
@Empact
Copy link
Contributor Author

Empact commented May 24, 2018

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

@DrahtBot DrahtBot mentioned this pull request Jun 7, 2018
Closed
@Empact
Copy link
Contributor Author

Empact commented Jun 14, 2018

Unnecessary thanks to build fix now in #13235

@Empact Empact closed this Jun 14, 2018
@Empact Empact deleted the net-template-methods branch June 14, 2018 11:31
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Empact @promag @sipa @maflcko @practicalswift @fanquake