net, refactor: move StartExtraBlockRelayPeers() from header to implementation #25119

jonatack commented at 3:53 PM on May 12, 2022: member

where all the other logging actions in src/net.{h,cpp} are located.

StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(), called in turn from StartScheduledTasks() with a scheduleEvery delta of 45 seconds, called at the end of AppInitMain() on bitcoind startup.

This allows dropping #include <logging.h> from net.h, which can improve compile time/speed. Currently, none of the other includes in net.h use logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.

refactor: move StartExtraBlockRelayPeers from header to implementation

where all the other logging actions in src/net.{h,cpp} are located.

StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be
inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(),
called in turn from StartScheduledTasks() with a scheduleEvery delta of 45
seconds, called at the end of AppInitMain() on bitcoind startup.

This allows dropping `#include <logging.h>` from net.h, which can improve
compile time/speed. Currently, none of the other includes in net.h use
logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.

51ec96b904

jonatack commented at 4:01 PM on May 12, 2022: member

Note: src/net.cpp is missing quite a few headers, including logging.h --- e0f804a0090b54 adds them but I didn't include it in this pull unless people prefer it to be added.

theStack commented at 4:09 PM on May 12, 2022: member

Concept ACK

Note: src/net.cpp is missing quite a few headers, including logging.h --- e0f804a adds them but I didn't include it in this pull unless people prefer it to be added.

Is this commit part of another PR? Github says "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." (but it must belong to this repository, otherwise the link wouldn't work right? :D)

jonatack commented at 4:21 PM on May 12, 2022: member

Thanks for having a look! That homeless-looking commit was previously part of #24757, as a leftover from #24734.

DrahtBot added the label P2P on May 12, 2022

LarryRuane commented at 5:14 PM on May 12, 2022: contributor

Just so I understand (and may be helpful to others), this PR compiles as is, but net.cpp makes use of some std libraries (for example, chrono) whose headers it doesn't include, but which happen to be included by other headers (that net.cpp is including). As opposed to net.cpp including everything it needs (that is, adding https://github.com/bitcoin/bitcoin/commit/e0f804a0090b54e25b5742da422d8ca510960724 to this PR, or wouldn't have to be this PR I guess).

As is (without this other commit), net.cpp compiles, but it may not in the future if one of the headers it includes stops including (for example) chrono, which net.cpp uses. In contrast, if that commit is merged, then net.cpp is more resilient to changes outside of itself (changes in other header files). The downside of adding those includes is that compiling is slower (even though they're just redundant includes so not that bad).

I'm probably not explaining this well, making it more complicated than it is, but do I have this right?

If so, my opinion is that it would be good to add that commit, but I don't feel strongly either way.

LarryRuane commented at 5:40 PM on May 12, 2022: contributor

ACK 51ec96b904f349056e805c6b2a6de5257e8fbdee will re-ack if you decide to include the extra commit

jonatack commented at 5:43 PM on May 12, 2022: member

@LarryRuane thanks! that seems right to me (IIUC, building isn't slower by including what you use, as otherwise it just wouldn't compile). I'll run iwyu on that commit to double-check and maybe add it.

theStack approved

theStack commented at 6:13 PM on May 12, 2022: member

ACK 51ec96b904f349056e805c6b2a6de5257e8fbdee

my opinion is that it would be good to add that commit, but I don't feel strongly either way.

+1, happy to re-review (either in this PR or a follow-up PR)

MarcoFalke added the label Refactoring on May 13, 2022

in src/net.h:15 in 51ec96b904

  12 | @@ -13,7 +13,6 @@
  13 |  #include <crypto/siphash.h>
  14 |  #include <hash.h>
  15 |  #include <i2p.h>

MarcoFalke commented at 5:47 AM on May 13, 2022:

Unrelated, but:

net.h should add these lines:
#include <assert.h>            // for assert
#include <stddef.h>            // for size_t
#include <algorithm>           // for max, min
#include <chrono>              // for seconds, microseconds, operator""s
#include <set>                 // for set
#include <string>              // for string, basic_string, operator<
#include <utility>             // for move
#include "threadsafety.h"      // for GUARDED_BY, EXCLUSIVE_LOCKS_REQUIRED
#include "version.h"           // for INIT_PROTO_VERSION
class CChainParams;
class NetGroupManager;
class Sock;
namespace i2p::sam { class Session; }

net.h should remove these lines:
- #include <chainparams.h>  // lines 9-9
- #include <common/bloom.h>  // lines 10-10
- #include <consensus/amount.h>  // lines 12-12
- #include <i2p.h>  // lines 15-15
- #include <netgroup.h>  // lines 20-20
- #include <policy/feerate.h>  // lines 21-21
- #include <util/sock.h>  // lines 30-30
- class CNodeStats;  // lines 102-102

jonatack commented at 2:31 PM on May 13, 2022:

@MarcoFalke thanks. I ran iwyu python tool on src/net.{h,cpp} with the following (LMK if that incantation can be improved or how you are running it): python3 <path-to>/iwyu_tool src/net.cpp -p . -j5 -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=<path-to>/contrib/devtools/iwyu/bitcoin.core.imp

and this was its output for me

net.h should add these lines:
#include <assert.h>            // for assert
#include <stddef.h>            // for size_t
#include <algorithm>           // for min
#include <chrono>              // for seconds, microseconds, operator""s
#include <set>                 // for set
#include <string>              // for string, operator<=>
#include <utility>             // for move
#include "threadsafety.h"      // for GUARDED_BY, EXCLUSIVE_LOCKS_REQUIRED
#include "tinyformat.h"        // for format_error
#include "version.h"           // for INIT_PROTO_VERSION
class CChainParams;
class NetGroupManager;
class Sock;
namespace i2p::sam { class Session; }

net.h should remove these lines:
- #include <chainparams.h>  // lines 9-9
- #include <common/bloom.h>  // lines 10-10
- #include <consensus/amount.h>  // lines 12-12
- #include <i2p.h>  // lines 15-15
- #include <netgroup.h>  // lines 19-19
- #include <policy/feerate.h>  // lines 20-20
- #include <util/sock.h>  // lines 29-29
- class CNodeStats;  // lines 101-101

net.cpp should add these lines:
#include <net/if.h>                 // for IFF_UP
#include <netinet/in.h>             // for htonl, IPV6_V6ONLY, in_addr, IN6A...
#include <netinet/tcp.h>            // for TCP_NODELAY
#include <string.h>                 // for memcpy, memcmp, strcmp
#include <sys/sdt.h>                // for __sdt_type, _SDT_ASM_OPERANDS_6
#include <sys/socket.h>             // for size_t, sockaddr_storage, bind
#include <compare>                  // for operator>, operator<, strong_orde...
#include <exception>                // for exception
#include <iterator>                 // for back_insert_iterator, back_inserter
#include <ratio>                    // for ratio
#include <tuple>                    // for tie, tuple
#include "chainparams.h"            // for Params, CChainParams
#include "crypto/common.h"          // for ReadLE32
#include "crypto/siphash.h"         // for CSipHasher
#include "hash.h"                   // for Hash, CHash256
#include "logging.h"                // for LogPrintf_, LogPrint, NET, LogPrintf
#include "netgroup.h"               // for NetGroupManager
#include "prevector.h"              // for prevector
#include "serialize.h"              // for SER_NETWORK, ser_writedata32, ser...
#include "span.h"                   // for Span, AsBytes, MakeByteSpan
#include "streams.h"                // for CDataStream, CAutoFile, CVectorWr...
#include "sync.h"                   // for LOCK, AssertLockNotHeldInternal
#include "threadinterrupt.h"        // for CThreadInterrupt
#include "timedata.h"               // for GetAdjustedTime
#include "util/time.h"              // for GetTime, count_seconds, GetTimeMi...

net.cpp should remove these lines:
- #include <crypto/sha256.h>  // lines 18-18
- #include <fcntl.h>  // lines 40-40
- #include <math.h>  // lines 58-58

MarcoFalke merged this on May 13, 2022

MarcoFalke closed this on May 13, 2022

jonatack deleted the branch on May 13, 2022

sidhujag referenced this in commit 1e9ed2260c on May 13, 2022

DrahtBot locked this on May 13, 2023