From 3c43d9db1e0f84279b08a93afd730caeece061a9 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Wed, 9 Dec 2020 14:38:21 +0200 Subject: [PATCH 1/2] p2p: Don't self-advertise during VERSION processing Previously, we would prepare to self-announce to a new peer while parsing a VERSION message from that peer. This is redundant, because we do something very similar in MaybeSendAddr(), which is called from SendMessages() after the version handshake is finished. There are a couple of differences: 1) MaybeSendAddr() self-advertises to all peers we do address relay with, not just outbound ones. 2) GetLocalAddrForPeer() called from MaybeSendAddr() makes a probabilistic decision to either advertise what they think we are or what we think we are, while PushAddress(self) on VERSION deterministically only does the former if the address from the latter is unroutable. 3) During VERSION processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in PushAddress(). Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in MaybeSendAddr() is better, remove the one in VERSION. Co-authored-by: Martin Zumsande --- src/net_processing.cpp | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index eca6263392c..1adcb3695b2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3274,39 +3274,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_num_preferred_download_peers += state->fPreferredDownload; } - // Self advertisement & GETADDR logic if (!pfrom.IsInboundConn() && SetupAddressRelay(pfrom, *peer)) { - // For outbound peers, we try to relay our address (so that other - // nodes can try to find us more quickly, as we have no guarantee - // that an outbound peer is even aware of how to reach us) and do a - // one-time address fetch (to help populate/update our addrman). If - // we're starting up for the first time, our addrman may be pretty - // empty and no one will know who we are, so these mechanisms are + // For outbound peers, we do a one-time address fetch + // (to help populate/update our addrman). + // If we're starting up for the first time, our addrman may be pretty + // empty and no one will know who we are, so this mechanism is // important to help us connect to the network. // // We skip this for block-relay-only peers. We want to avoid // potentially leaking addr information and we do not want to // indicate to the peer that we will participate in addr relay. - if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) - { - CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, Now()}; - FastRandomContext insecure_rand; - if (addr.IsRoutable()) - { - LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); - PushAddress(*peer, addr, insecure_rand); - } else if (IsPeerAddrLocalGood(&pfrom)) { - // Override just the address with whatever the peer sees us as. - // Leave the port in addr as it was returned by GetLocalAddress() - // above, as this is an outbound connection and the peer cannot - // observe our listening port. - addr.SetIP(addrMe); - LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); - PushAddress(*peer, addr, insecure_rand); - } - } - - // Get recent addresses m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); peer->m_getaddr_sent = true; // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response From 956c67059caf3807b3ceacdd5de1caaae538f009 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 30 Sep 2022 11:30:51 -0400 Subject: [PATCH 2/2] refactor, doc: Improve SetupAddressRelay call in version processing This code was a bit hard to understand, so make it less dense and add more explanations. Doesn't change behavior. Co-authored-by: Amiti Uttarwar --- src/net_processing.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1adcb3695b2..bc40a244c4f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3274,13 +3274,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_num_preferred_download_peers += state->fPreferredDownload; } - if (!pfrom.IsInboundConn() && SetupAddressRelay(pfrom, *peer)) { - // For outbound peers, we do a one-time address fetch - // (to help populate/update our addrman). + // Attempt to initialize address relay for outbound peers and use result + // to decide whether to send GETADDR, so that we don't send it to + // inbound or outbound block-relay-only peers. + bool send_getaddr{false}; + if (!pfrom.IsInboundConn()) { + send_getaddr = SetupAddressRelay(pfrom, *peer); + } + if (send_getaddr) { + // Do a one-time address fetch to help populate/update our addrman. // If we're starting up for the first time, our addrman may be pretty - // empty and no one will know who we are, so this mechanism is - // important to help us connect to the network. - // + // empty, so this mechanism is important to help us connect to the network. // We skip this for block-relay-only peers. We want to avoid // potentially leaking addr information and we do not want to // indicate to the peer that we will participate in addr relay. @@ -5214,8 +5218,9 @@ bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer) if (node.IsBlockOnlyConn()) return false; if (!peer.m_addr_relay_enabled.exchange(true)) { - // First addr message we have received from the peer, initialize - // m_addr_known + // During version message processing (non-block-relay-only outbound peers) + // or on first addr-related message we have received (inbound peers), initialize + // m_addr_known. peer.m_addr_known = std::make_unique(5000, 0.001); }