From d21d2b264cd77c027a06f68289cf4c3f177d1ed0 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 9 Jul 2020 07:42:11 +0100 Subject: [PATCH 1/2] [net] Change AdvertiseLocal to GetLocalAddrForPeer Gossiping addresses to peers is the responsibility of net processing. Change AdvertiseLocal() in net to just return an (optional) address for net processing to advertise. Update function name to reflect new responsibility. --- src/net.cpp | 9 +++++---- src/net.h | 3 ++- src/net_processing.cpp | 5 ++++- src/test/net_tests.cpp | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 81176785a2..bd89df63bf 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -201,8 +201,7 @@ bool IsPeerAddrLocalGood(CNode *pnode) IsReachable(addrLocal.GetNetwork()); } -// pushes our own address to a peer -void AdvertiseLocal(CNode *pnode) +Optional GetLocalAddrForPeer(CNode *pnode) { if (fListen && pnode->fSuccessfullyConnected) { @@ -222,10 +221,12 @@ void AdvertiseLocal(CNode *pnode) } if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false)) { - LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString()); - pnode->PushAddress(addrLocal, rng); + LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId()); + return addrLocal; } } + // Address is unroutable. Don't advertise. + return nullopt; } // learn a new local address diff --git a/src/net.h b/src/net.h index 682150d65a..67d1cf0e55 100644 --- a/src/net.h +++ b/src/net.h @@ -197,7 +197,8 @@ enum }; bool IsPeerAddrLocalGood(CNode *pnode); -void AdvertiseLocal(CNode *pnode); +/** Returns a local address that we should advertise to this peer */ +Optional GetLocalAddrForPeer(CNode *pnode); /** * Mark a network as reachable or unreachable (no automatic connects to it) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 881f5d7297..60b2975b67 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4426,7 +4426,10 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (pto->m_next_local_addr_send != 0us) { pto->m_addr_known->reset(); } - AdvertiseLocal(pto); + if (Optional local_addr = GetLocalAddrForPeer(pto)) { + FastRandomContext insecure_rand; + pto->PushAddress(*local_addr, insecure_rand); + } pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); } diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index f52905e1ef..1c7c35528e 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -691,7 +691,7 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) pnode->SetAddrLocal(addrLocal); // before patch, this causes undefined behavior detectable with clang's -fsanitize=memory - AdvertiseLocal(&*pnode); + GetLocalAddrForPeer(&*pnode); // suppress no-checks-run warning; if this test fails, it's by triggering a sanitizer BOOST_CHECK(1); From 3e68efa615968e0c9d68a7f197c7852478f6be78 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 9 Jul 2020 07:52:48 +0100 Subject: [PATCH 2/2] [net] Move checks from GetLocalAddrForPeer to caller GetLocalAddrForPeer() is only called in one place. The checks inside that function make more sense to be carried out be the caller: - fSuccessfullyConnected is already checked at the top of SendMessages(), so must be true when we call GetLocalAddrForPeer() - fListen can go into the conditional before GetLocalAddrForPeer() is called. --- src/net.cpp | 37 +++++++++++++++++-------------------- src/net_processing.cpp | 4 +++- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index bd89df63bf..533815b755 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -203,27 +203,24 @@ bool IsPeerAddrLocalGood(CNode *pnode) Optional GetLocalAddrForPeer(CNode *pnode) { - if (fListen && pnode->fSuccessfullyConnected) + CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices()); + if (gArgs.GetBoolArg("-addrmantest", false)) { + // use IPv4 loopback during addrmantest + addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices()); + } + // If discovery is enabled, sometimes give our peer the address it + // tells us that it sees us as in case it has a better idea of our + // address than we do. + FastRandomContext rng; + if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || + rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0)) { - CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices()); - if (gArgs.GetBoolArg("-addrmantest", false)) { - // use IPv4 loopback during addrmantest - addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices()); - } - // If discovery is enabled, sometimes give our peer the address it - // tells us that it sees us as in case it has a better idea of our - // address than we do. - FastRandomContext rng; - if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || - rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0)) - { - addrLocal.SetIP(pnode->GetAddrLocal()); - } - if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false)) - { - LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId()); - return addrLocal; - } + addrLocal.SetIP(pnode->GetAddrLocal()); + } + if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false)) + { + LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId()); + return addrLocal; } // Address is unroutable. Don't advertise. return nullopt; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 60b2975b67..428a45e73b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4416,7 +4416,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Address refresh broadcast auto current_time = GetTime(); - if (pto->RelayAddrsWithConn() && !::ChainstateActive().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) { + if (fListen && pto->RelayAddrsWithConn() && + !::ChainstateActive().IsInitialBlockDownload() && + pto->m_next_local_addr_send < current_time) { // If we've sent before, clear the bloom filter for the peer, so that our // self-announcement will actually go out. // This might be unnecessary if the bloom filter has already rolled