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

1. 
   0xB10C commented at 3:38 pm on December 24, 2025: contributor

   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 #33699 (comment). The discussion there should be helpful for reviewers.

2. DrahtBot added the label P2P on Dec 24, 2025
3. 
   DrahtBot commented at 3:38 pm on December 24, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34146.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	fjahr, bensig, frankomosh, achow101

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

4. in test/functional/p2p_addr_selfannouncement.py:53 in 975016606a

   44@@ -42,6 +45,15 @@ def handle_addr_message(self, message):
   45             self.addresses_received += 1
   46             if addr == self.expected:
   47                 self.self_announcements_received += 1
   48+                # If it's the first self-announcement:
   49+                if self.self_announcements_received == 1:
   50+                    # - check that it is in the first addr message we receive
   51+                    assert_equal(self.addr_messages_received, 1)
   52+                    # - check that it is the first address we receive
   53+                    assert_equal(self.addresses_received, 1)
   

   ---

   ---

   

   fjahr commented at 10:26 pm on December 24, 2025:

   The check below implies this check (if it’s the only one it’s always the first), so it seems like it could be dropped.

   ---

   

   0xB10C commented at 10:42 am on January 5, 2026:

   I dropped this and reworked the comment a bit. Since it’s important for us that the self-annoucenment is the first address we receive, i’m mentioning it in the comment.
5. in test/functional/p2p_addr_selfannouncement.py:55 in 975016606a outdated

   51+                    assert_equal(self.addr_messages_received, 1)
   52+                    # - check that it is the first address we receive
   53+                    assert_equal(self.addresses_received, 1)
   54+                    # - check that the message only contains the self-announcement
   55+                    assert_equal(len(message.addrs), 1)
   56+
   

   ---

   ---

   

   fjahr commented at 10:26 pm on December 24, 2025:

   nit: unnecessary empty line added
6. in test/functional/p2p_addr_selfannouncement.py:84 in 975016606a

   80@@ -69,10 +81,10 @@ def run_test(self):
   81         self.self_announcement_test(outbound=True, addrv2=True)
   82 
   83     def inbound_connection_open_assertions(self, addr_receiver):
   84-        # We expect one self-announcement and multiple other addresses in
   85-        # response to a GETADDR in a single addr / addrv2 message.
   86+        # We expect the self-announcement in a separate message from
   

   ---

   ---

   

   fjahr commented at 10:29 pm on December 24, 2025:

   Hm, the edited comment drops some information that is still relevant, e.g. “and multiple other addresses” explains the greater_than assertion below. Maybe this part could be added back above that assertion.

   ---

   

   0xB10C commented at 10:42 am on January 5, 2026:

   Going with:

   In response to a GETADDR, we expect a message with the self-announcement and an addr message containing the GETADDR response.

7. in src/net_processing.cpp:5342 in 975016606a

   5338+                    MakeAndPushMessage(node, NetMsgType::ADDRV2, CAddress::V2_NETWORK(self_announcement));
   5339+                } else {
   5340+                    MakeAndPushMessage(node, NetMsgType::ADDR, CAddress::V1_NETWORK(self_announcement));
   5341+                }
   5342+            } else {
   5343+                // All later self-announcements are send together with the other addresses.
   

   ---

   ---

   

   fjahr commented at 10:31 pm on December 24, 2025:

   0                // All later self-announcements are sent together with the other addresses.
   

8. 
   fjahr commented at 10:38 pm on December 24, 2025: contributor

   Concept ACK
9. p2p: first addr self-announcement in separate msg

   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
   
   Co-Authored-By: Anthony Towns <aj@erisian.com.au>

   792e2edf57
10. 0xB10C force-pushed on Jan 5, 2026
11. in src/net_processing.cpp:5335 in 792e2edf57

    5327@@ -5328,7 +5328,20 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
    5328         }
    5329         if (std::optional<CService> local_service = GetLocalAddrForPeer(node)) {
    5330             CAddress local_addr{*local_service, peer.m_our_services, Now<NodeSeconds>()};
    5331-            PushAddress(peer, local_addr);
    5332+            if (peer.m_next_local_addr_send == 0us) {
    5333+                // Send the initial self-announcement in its own message. This makes sure
    5334+                // rate-limiting with limited start-tokens doesn't ignore it if the first
    5335+                // message ends up containing multiple addresses.
    5336+                std::vector<CAddress> self_announcement {local_addr};
    

    ---

    ---

    

    fjahr commented at 12:06 pm on January 5, 2026:

    micro-nit: could drop space

    0                std::vector<CAddress> self_announcement{local_addr};
    

12. 
    fjahr commented at 12:09 pm on January 5, 2026: contributor

    Code review ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
13. 
    bensig commented at 9:54 pm on January 5, 2026: contributor

    ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f

    Built and tested.

    Logic is clean - uses m_next_local_addr_send == 0us to detect first announcement and sends it separately.

    Test correctly verifies it arrives alone in first message

14. in src/net_processing.cpp:5337 in 792e2edf57

    5333+                // Send the initial self-announcement in its own message. This makes sure
    5334+                // rate-limiting with limited start-tokens doesn't ignore it if the first
    5335+                // message ends up containing multiple addresses.
    5336+                std::vector<CAddress> self_announcement {local_addr};
    5337+                if (peer.m_wants_addrv2) {
    5338+                    MakeAndPushMessage(node, NetMsgType::ADDRV2, CAddress::V2_NETWORK(self_announcement));
    

    ---

    ---

    

    w0xlt commented at 2:10 am on January 7, 2026:

    This new direct-send path bypasses the addr.IsValid() and IsAddrCompatible(peer, addr) checks performed by PushAddress(). Could this be an issue?

    ---

    

    frankomosh commented at 2:01 pm on January 14, 2026:

    I think that those two checks might still be needed to ensure the address we’re about to relay is valid and compatible with the peer, even if GetLocalAddrForPeer() succeeded.

    ---

    

    0xB10C commented at 2:28 pm on January 14, 2026:

    I agree, but haven’t had the time to investigate closer

    ---

    

    0xB10C commented at 2:28 pm on January 14, 2026:

    I agree, but haven’t had the time to investigate closer
15. 
    frankomosh commented at 2:07 pm on January 14, 2026: contributor

    Code Review ACK 792e2ed
16. 
    achow101 commented at 10:08 pm on January 14, 2026: member

    ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
17. achow101 merged this on Jan 14, 2026
18. achow101 closed this on Jan 14, 2026

    ---

19. 
    0xB10C commented at 7:43 am on January 15, 2026: contributor

    This new direct-send path bypasses the addr.IsValid() and IsAddrCompatible(peer, addr) checks performed by PushAddress(). Could this be an issue?

    I think that those two checks might still be needed to ensure the address we’re about to relay is valid and compatible with the peer, even if GetLocalAddrForPeer() succeeded.

    I agree, but haven’t had the time to investigate closer

    Well, merging this is one way of getting me to look into this sooner :grin: IMO it might have been better to wait until the review is addressed and have someone more familiar with address relay look over it.

20. 
    frankomosh commented at 8:27 am on January 15, 2026: contributor

    Well, merging this is one way of getting me to look into this sooner. IMO it might have been better to wait until the review is addressed.

    I added a follow up in #34297 … maybe you can check if its correct?

21. 
    mzumsande commented at 12:34 pm on January 15, 2026: contributor

    I think that those two checks might still be needed to ensure the address we’re about to relay is valid and compatible with the peer, even if GetLocalAddrForPeer() succeeded.

    The check for addr.IsValid() is not really needed, since GetLocalAddrForPeer will always return valid address (it checks for IsRoutable(), which implies IsValid().

    However, the check for IsAddrCompatible(peer, addr) seems useful: If our peer doesn’t support addr v2, and the local address is, not supported (e.g. onion), I think we would send a non-routable all-zeros address then. This is probably not a big issue in practice, since most onion/i2p peers should support addrv2.

    The third skipped check from PushAddress (peer.m_addr_known->contains(addr.GetKey())) is not needed because we empty m_addr_known right before self-advertising.

22. sedited referenced this in commit 9016858282 on Jan 22, 2026
23. stickies-v referenced this in commit 9fa736f8e4 on Jan 22, 2026
24. 0xB10C deleted the branch on Jan 22, 2026

Contributors
0xB10C DrahtBot fjahr bensig w0xlt frankomosh achow101 mzumsande

Labels
P2P