net processing: Extract addr send functionality into MaybeSendAddr()

[jnewbery]

This continues the work of moving application layer data into net_processing. It refactors addr send functionality into its own function MaybeSendAddr() and flattens/simplifies the code. Isolating and simplifying the addr handling code makes subsequent changes (which will move addr data and logic into net processing) easier to review.

This is a pure refactor. There are no functional changes.

For motivation of the project, see #19398.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• [p2p] Reduce addr blackholes  #21528 ([p2p] Reduce addr blackholes by amitiuttarwar)
• Erlay: bandwidth-efficient transaction relay protocol #21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)
• net: Address outstanding review comments from PR20721 #21198 (net: Address outstanding review comments from PR20721 by jnewbery)
• net/net processing: Move tx inventory into net_processing #21160 (net/net processing: Move tx inventory into net_processing by jnewbery)

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.

[jnewbery]

Rebased on master to pick up fix for interface_zmq.py in #21008.

[ajtowns]

Haven't thoroughly reviewed the "Refactor MaybeSendAddr" commit; seems like separating it into three or four commits would be clearer (early exit; invariants; skip vAddr check; one-pass erase)?

[jnewbery]

Thanks for the review @ajtowns. I've addressed your comments.

[jnewbery]

Oops. I missed taking the new lock in one of the call sites. Will fix up tomorrow.

[jnewbery]

Once all of the addr data has been moved into net_processing, I intend to guard it with a single lock (see the m_addr_relay struct in https://github.com/jnewbery/bitcoin/tree/2021-02-lazy-init-peer). However, it's proving difficult to get lock orders right while the data is still in net. I've decided to punt on doing that until after this PR.

To avoid simply removing the guard from m_next_addr_send and m_next_local_addr_send, I've introduced the new m_addr_mutex lock in this PR. Once the data is moved into Peer, I'll extend that mutex to guard the remaining addr data.

[fanquake]

Concept ACK - this seems like a fairly good simplification. I like consolidating logic and returning early when possible. If this SendMessages() logic is going to be split up even more, you could consider a call to GetTime<>() at the start, and just pass that into MaybeSendPing, MaybeSendAddr, MaybeSomeOtherFunc etc.

This doesn't currently compile:

net_processing.cpp:4408:91: error: no member named 'm_addr_mutex' in 'CNode'
    auto addr_already_known = [&node](const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(node.m_addr_mutex)
                                                                                     ~~~~ ^
./threadsafety.h:31:79: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
                                                                              ^~~~~~~~~~~
1 error generated.

[jnewbery]

Thanks for the review, @fanquake! The compile error was due to an annotation that I'd left from a previous branch. It's now fixed.

If this SendMessages() logic is going to be split up even more, you could consider a call to GetTime<>() at the start, and just pass that into MaybeSendPing, MaybeSendAddr, MaybeSomeOtherFunc etc.

Good idea. Done!

[jnewbery]

Thanks for the review @hebasto! Responding to your points:

I see two calls of three are consolidated. What about the third one [...]

I was also counting the call inside MaybeSendPing(), which is called from SendMessages(). I've now removed the additional call to GetTime() as suggested, which required a duration cast.

I believe that this commit is not a clean refactoring. To be precise, change behavior could be expected as even LOCK(cs_main); could take unpredictable time. But the scale of such changes seems insignificant to P2P, right?

Good observation! However, I think it's fine to treat time as constant within a call to SendMessages(), since it's observationally equivalent to any actors outside net_processing.

The commit "[net processing] Extract addr send functionality into MaybeSendAddr()" (97e1013) allows to localize acquiring of the CNode::cs_sendProcessing mutex

The fact that you've raised this independently has made me reconsider the discussion here #21236 (comment). It doesn't make sense for a mutex to be guarding variables/functions across multiple layers, so I've moved the addr send time vars under their own mutex.

Early return skips

 // we only send the big addr message once 
 if (node.vAddrToSend.capacity() > 40) { 
     node.vAddrToSend.shrink_to_fit(); 
 } 

That differs from original behavior.

Another great observation! Although again, I don't think it matters. This logic is for when we've sent a response to the peer's getaddr message. In such cases, we'd expect to send up to 1000 addresses, and for the address filter to not yet be filled, so we wouldn't exit early.

[hebasto]

RecursiveMutex cs_sendProcessing guards nothing. Let's burn it :)

[hebasto]

pico-nit: I find myself more comfortable to reason about the code when the same variable is placed on the same position in similar logical operations. Consider

bitcoin/src/net_processing.cpp

Lines 4154 to 4155 in b4d7d3d

	if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload() &&
	node.m_next_local_addr_send < current_time) {

and

bitcoin/src/net_processing.cpp

Line 4173 in b4d7d3d

if (current_time <= node.m_next_addr_send) return;

Could current_time position be consistent?

[jnewbery]

pico-nit: I find myself more comfortable to reason about the code when the same variable is placed on the same position in similar logical operations.

I tend to agree and think that generally the variable should be placed on the left and the thing it's being compared to on the right. Others disagreed with me on previous PRs and told me to always place the smaller thing on the left. I think it's just a matter of taste 🤷

[hebasto]

ACK 935d488, I have reviewed the code and it looks OK, I agree it can be merged.

[sipa]

utACK 935d488

[maflcko]

review ACK 935d488 🐑

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 935d4889228e7e361c8b0020761fa0e08a55fb48 🐑
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh7Hgv/dmVk2jwk66OpRVhWiCUITNoKMBCZFBA8+k63KsptFTEUV0oneUoiMVTm
Ftw+YVO60MZdNQjBet3CC3IyTUv098/H2+e9cODLgbRpRMJaLewzevxUy0o6RToQ
BzHalKxNnrGEUnJB/6zm4HzXWIPJWppY947BlLEuZ+HM7J8HgAmFVPZWa/VubsrN
YhY6rBbtHuJKGamsdE/LkNfNEMhP4dIOksl1WteXg2xoSeBHQeHL6nFLKFjJohbj
Y34ZSnxo0jJFpz+bSVlzelJh5EL7/G+fZzvE5vbOa39+5unZ2mFx70ftvGYgRK6V
qLfhnyeTMABXjs5bwU3HcjVGqa1ZsApIhY+sLs6lwSQmNAG2UjOxPcl7EDcXnZha
fV//g/jdU5qTyNQiriWrA0TEtwPm7X1lgJFYBgpfSAfnSIxbLk/uZWkPqFVVbt5P
Mh2PXN7lIqJ/irxKTBscAXqX4RrIbHXHVv5TaUvS3kI2igSLaKjUhj2Nqoea6UZ3
XBBbfMvT
=sVrE
-----END PGP SIGNATURE-----

Timestamp of file with hash 2166a9ff7592f66162db55121e31cb0376f60455376bea865e5631ad552a2fc6 -