backport: merge bitcoin#21148, #21327, #23970, #24021, #24543, #26844, #25325, #28165, partial bitcoin#20524, #26036, #27981 (networking backports: part 7)

[kwvg]

Additional Information

• Dependent on backport: merge bitcoin#22278, #24103, #24235, #24177, #24299, #24917, #22564, #25349, #25571, #17487, #26999, #27011 (blockstorage backports: part 2) #6098

• Dependent on fix: release unused memory in CNetMsgMaker::Make() #6233

• p2p_ibd_txrelay.py was first introduced in bitcoin#19423 to test feefilter logic but on account of Dash not having feefilter capabilities, that backport was skipped over but on account of the tests introduced in bitcoin#21327 that test capabilities present in Dash, a minimal version of p2p_ibd_txrelay.py has been committed in.

• vSendMsg is originally a std::deque and as an optimization, was changed to a std::list in 027a852 (dash#3398) but this renders us unable to backport bitcoin#26844 as it introduces build failures. The optimization has been reverted to make way for the backport.

  Compile failure:

  net.cpp:959:20: error: invalid operands to binary expression ('iterator' (aka '_List_iterator<std::vector<unsigned char, std::allocator<unsigned char>>>') and 'int')
              if (it + 1 != node.vSendMsg.end()) {
                  ~~ ^ ~
  /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_bvector.h:303:3: note: candidate function not viable: no known conversion from 'iterator' (aka '_List_iterator<std::vector<unsigned char, std::allocator<unsigned char>>>') to 'ptrdiff_t' (aka 'long') for 1st argument
    operator+(ptrdiff_t __n, const _Bit_iterator& __x)
  [...]
  1 error generated.
  make[2]: *** [Makefile:11296: libbitcoin_server_a-net.o] Error 1
  make[2]: *** Waiting for unfinished jobs....
  make[2]: Leaving directory '/src/dash/src'
  make[1]: *** [Makefile:19171: all-recursive] Error 1
  make[1]: Leaving directory '/src/dash/src'
  make: *** [Makefile:799: all-recursive] Error 1
  

• The collection of CNode pointers in CConnman::SocketHandlerConnected has been changed to a std::set to allow for us to erase elements from vReceivableNodes if the node is also in the set of sendable nodes and the send hasn't entirely succeeded to avoid a deadlock (i.e. backport bitcoin#27981)

• When backporting bitcoin#28165, denialofservice_tests has been modified to still check with vSendMsg instead of Transport::GetBytesToSend() as changes in networking code to support LT-SEMs (level-triggered socket events mode) mean that the message doesn't get shifted from vSendMsg to m_message_to_send, as the test expects.

  • Specifically, the changes made for LT-SEM support result in the function responsible for making that shift (Transport::SetMessageToSend() through CConnman::SocketSendData()), not being called during the test runtime.

• As checking vSendMsg (directly or through nSendMsgSize) isn't enough to determine if the queue is empty, we now also check with to_send from Transport::GetBytesToSend() to help us make that determination. This mirrors the change present in the upstream backport (source).

Breaking Changes

• bandwidth.message.*.bytesSent will no longer include overhead and will now only report message size as specifics that let us calculate the overhead have been abstracted away.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas (note: N/A)
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation (note: N/A)
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[UdjinM6]

Seems to be working with no issues.

light ACK 71a5e90

[PastaPastaPasta]

generally LGTM: 71a5e90; needs clang-format fixed

[UdjinM6]

utACK d0faa3a

[PastaPastaPasta]

utACK 76a458e