Merge bitcoin/bitcoin#34146: p2p: send first addr self-announcement in separate message 🎄

792e2edf57ab31ae5c6f98acf33af8f67506630f p2p: first addr self-announcement in separate msg (0xb10c)

Pull request description:

  This makes sure the initial address self-announcement a node sends to a peer happends in a separate P2P message. This has benefits for both inbound and outbound connections:

  For inbound connections from a peer to us, previously, we might send the self-announcement along with our response to a GETADDR request. However, the self-announcement might replace an address from the GETADDR response. This isn't clean.

  For outbound connections from us to a peer, previously, it could have happend that we send the self-announcement along with other addresses. Since shortly after connection open, the peer might only have one rate-limiting token for us, and the addresses are shuffeld on arrival, it's possible that the self-announcement gets rate-limited. However, note that these rate-limitings seem to be rare in practice.

  This is inspired by and based on https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763. The discussion there should be helpful for reviewers.

ACKs for top commit:
  bensig:
    ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
  achow101:
    ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
  fjahr:
    Code review ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
  frankomosh:
    Code Review ACK [792e2ed](792e2edf57)

Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260

src/net_processing.cpp

@@ -5517,7 +5517,20 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
         }
         if (std::optional<CService> local_service = GetLocalAddrForPeer(node)) {
             CAddress local_addr{*local_service, peer.m_our_services, Now<NodeSeconds>()};
-            PushAddress(peer, local_addr);
+            if (peer.m_next_local_addr_send == 0us) {
+                // Send the initial self-announcement in its own message. This makes sure
+                // rate-limiting with limited start-tokens doesn't ignore it if the first
+                // message ends up containing multiple addresses.
+                std::vector<CAddress> self_announcement {local_addr};
+                if (peer.m_wants_addrv2) {
+                    MakeAndPushMessage(node, NetMsgType::ADDRV2, CAddress::V2_NETWORK(self_announcement));
+                } else {
+                    MakeAndPushMessage(node, NetMsgType::ADDR, CAddress::V1_NETWORK(self_announcement));
+                }
+            } else {
+                // All later self-announcements are sent together with the other addresses.
+                PushAddress(peer, local_addr);
+            }
         }
         peer.m_next_local_addr_send = current_time + m_rng.rand_exp_duration(AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);
     }

test/functional/p2p_addr_selfannouncement.py

@@ -5,6 +5,9 @@
 """
 Test that a node sends a self-announcement with its external IP to
 in- and outbound peers after connection open and again sometime later.
+
+Additionally, this checks that the first self-announcement arrives
+in its own message and that this message is the first we receive.
 """
 
 import time
@@ -42,6 +45,13 @@ class SelfAnnouncementReceiver(P2PInterface):
             self.addresses_received += 1
             if addr == self.expected:
                 self.self_announcements_received += 1
+                if self.self_announcements_received == 1:
+                    # If it's the first self-announcement, check that it is
+                    # in the first addr message we receive, and that this message
+                    # only contains one address. This also implies that it is
+                    # the first address we receive.
+                    assert_equal(self.addr_messages_received, 1)
+                    assert_equal(len(message.addrs), 1)
 
     def on_addrv2(self, message):
         assert (self.addrv2_test)
@@ -69,10 +79,10 @@ class AddrSelfAnnouncementTest(BitcoinTestFramework):
         self.self_announcement_test(outbound=True, addrv2=True)
 
     def inbound_connection_open_assertions(self, addr_receiver):
-        # We expect one self-announcement and multiple other addresses in
-        # response to a GETADDR in a single addr / addrv2 message.
+        # In response to a GETADDR, we expect a message with the self-announcement
+        # and an addr message containing the GETADDR response.
         assert_equal(addr_receiver.self_announcements_received, 1)
-        assert_equal(addr_receiver.addr_messages_received, 1)
+        assert_equal(addr_receiver.addr_messages_received, 2)
         assert_greater_than(addr_receiver.addresses_received, 1)
 
     def outbound_connection_open_assertions(self, addr_receiver):