From 6a956bafd420d828acdb0b360f9419e5c124f000 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sun, 21 Jun 2020 21:50:40 -0400 Subject: [PATCH] [net] Add m_addr_mutex m_next_addr_send and m_next_local_addr_send are currently guarded by cs_sendProcessing. These fields are only read/writen by the message handling thread and the annotation was added unnecessarily in commit b312cd77. See discussion at https://github.com/bitcoin/bitcoin/pull/13123#issuecomment-647505130. We add a new m_addr_mutex to guard these fields. Once all the addr relay fields have been moved to the Peer object in net_processing, they will be protected by the same mutex. --- src/net.h | 8 ++-- src/net_processing.cpp | 103 +++++++++++++++++++++-------------------- 2 files changed, 58 insertions(+), 53 deletions(-) diff --git a/src/net.h b/src/net.h index 67d1cf0e55cd41..56bbb15a298579 100644 --- a/src/net.h +++ b/src/net.h @@ -542,12 +542,14 @@ class CNode // Peer selected us as (compact blocks) high-bandwidth peer (BIP152) std::atomic m_bip152_highbandwidth_from{false}; - // flood relay + /** Protects addr gossip data */ + Mutex m_addr_mutex; + std::vector vAddrToSend; std::unique_ptr m_addr_known{nullptr}; bool fGetAddr{false}; - std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0}; - std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0}; + std::chrono::microseconds m_next_addr_send GUARDED_BY(m_addr_mutex){0}; + std::chrono::microseconds m_next_local_addr_send GUARDED_BY(m_addr_mutex){0}; struct TxRelay { mutable RecursiveMutex cs_filter; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c97f7ced463a3b..3f69443f6eb567 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4417,64 +4417,67 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Address refresh broadcast auto current_time = GetTime(); - 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 - // over since our last self-announcement, but there is only a small - // bandwidth cost that we can incur by doing this (which happens - // once a day on average). - if (pto->m_next_local_addr_send != 0us) { - pto->m_addr_known->reset(); - } - if (Optional local_addr = GetLocalAddrForPeer(pto)) { - FastRandomContext insecure_rand; - pto->PushAddress(*local_addr, insecure_rand); + { + LOCK(pto->m_addr_mutex); + 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 + // over since our last self-announcement, but there is only a small + // bandwidth cost that we can incur by doing this (which happens + // once a day on average). + if (pto->m_next_local_addr_send != 0us) { + pto->m_addr_known->reset(); + } + 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); } - pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); - } - // - // Message: addr - // - if (pto->RelayAddrsWithConn() && pto->m_next_addr_send < current_time) { - pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); - std::vector vAddr; - vAddr.reserve(pto->vAddrToSend.size()); - assert(pto->m_addr_known); - - const char* msg_type; - int make_flags; - if (pto->m_wants_addrv2) { - msg_type = NetMsgType::ADDRV2; - make_flags = ADDRV2_FORMAT; - } else { - msg_type = NetMsgType::ADDR; - make_flags = 0; - } + // + // Message: addr + // + if (pto->RelayAddrsWithConn() && pto->m_next_addr_send < current_time) { + pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); + std::vector vAddr; + vAddr.reserve(pto->vAddrToSend.size()); + assert(pto->m_addr_known); + + const char* msg_type; + int make_flags; + if (pto->m_wants_addrv2) { + msg_type = NetMsgType::ADDRV2; + make_flags = ADDRV2_FORMAT; + } else { + msg_type = NetMsgType::ADDR; + make_flags = 0; + } - for (const CAddress& addr : pto->vAddrToSend) - { - if (!pto->m_addr_known->contains(addr.GetKey())) + for (const CAddress& addr : pto->vAddrToSend) { - pto->m_addr_known->insert(addr.GetKey()); - vAddr.push_back(addr); - // receiver rejects addr messages larger than MAX_ADDR_TO_SEND - if (vAddr.size() >= MAX_ADDR_TO_SEND) + if (!pto->m_addr_known->contains(addr.GetKey())) { - m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); - vAddr.clear(); + pto->m_addr_known->insert(addr.GetKey()); + vAddr.push_back(addr); + // receiver rejects addr messages larger than MAX_ADDR_TO_SEND + if (vAddr.size() >= MAX_ADDR_TO_SEND) + { + m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); + vAddr.clear(); + } } } + pto->vAddrToSend.clear(); + if (!vAddr.empty()) + m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); + // we only send the big addr message once + if (pto->vAddrToSend.capacity() > 40) + pto->vAddrToSend.shrink_to_fit(); } - pto->vAddrToSend.clear(); - if (!vAddr.empty()) - m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); - // we only send the big addr message once - if (pto->vAddrToSend.capacity() > 40) - pto->vAddrToSend.shrink_to_fit(); } // Start block sync