diff --git a/src/addrman.cpp b/src/addrman.cpp index c364a7710b..92937abcb5 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -401,7 +401,7 @@ void AddrManImpl::Unserialize(Stream& s_) } } -AddrInfo* AddrManImpl::Find(const CNetAddr& addr, int* pnId) +AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId) { AssertLockHeld(cs); @@ -556,10 +556,6 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT AddrInfo& info = *pinfo; - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return; - // update info info.nLastSuccess = nTime; info.nLastTry = nTime; @@ -683,10 +679,6 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi AddrInfo& info = *pinfo; - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return; - // update info info.nLastTry = nTime; if (fCountFailure && info.nLastCountAttempt < nLastGood) { @@ -796,10 +788,6 @@ void AddrManImpl::Connected_(const CService& addr, int64_t nTime) AddrInfo& info = *pinfo; - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return; - // update info int64_t nUpdateInterval = 20 * 60; if (nTime - info.nTime > nUpdateInterval) @@ -818,10 +806,6 @@ void AddrManImpl::SetServices_(const CService& addr, ServiceFlags nServices) AddrInfo& info = *pinfo; - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return; - // update info info.nServices = nServices; } diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 1dc7f25f9c..f8191d6b85 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -179,8 +179,8 @@ private: //! table with information about all nIds std::unordered_map mapInfo GUARDED_BY(cs); - //! find an nId based on its network address - std::unordered_map mapAddr GUARDED_BY(cs); + //! find an nId based on its network address and port. + std::unordered_map mapAddr GUARDED_BY(cs); //! randomly-ordered vector of all nIds //! This is mutable because it is unobservable outside the class, so any @@ -225,7 +225,7 @@ private: const std::vector m_asmap; //! Find an entry. - AddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); + AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); //! Create a new entry and add it to the internal data structures mapInfo, mapAddr and vRandom. AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/netaddress.h b/src/netaddress.h index 66c8c48f08..57eb8bc72f 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -253,7 +253,6 @@ public: } } - friend class CNetAddrHash; friend class CSubNet; private: @@ -467,22 +466,6 @@ private: } }; -class CNetAddrHash -{ -public: - size_t operator()(const CNetAddr& a) const noexcept - { - CSipHasher hasher(m_salt_k0, m_salt_k1); - hasher.Write(a.m_net); - hasher.Write(a.m_addr.data(), a.m_addr.size()); - return static_cast(hasher.Finalize()); - } - -private: - const uint64_t m_salt_k0 = GetRand(std::numeric_limits::max()); - const uint64_t m_salt_k1 = GetRand(std::numeric_limits::max()); -}; - class CSubNet { protected: @@ -565,6 +548,25 @@ public: READWRITEAS(CNetAddr, obj); READWRITE(Using>(obj.port)); } + + friend class CServiceHash; +}; + +class CServiceHash +{ +public: + size_t operator()(const CService& a) const noexcept + { + CSipHasher hasher(m_salt_k0, m_salt_k1); + hasher.Write(a.m_net); + hasher.Write(a.port); + hasher.Write(a.m_addr.data(), a.m_addr.size()); + return static_cast(hasher.Finalize()); + } + +private: + const uint64_t m_salt_k0 = GetRand(std::numeric_limits::max()); + const uint64_t m_salt_k1 = GetRand(std::numeric_limits::max()); }; #endif // BITCOIN_NETADDRESS_H diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index bd6f470219..991bfa5efc 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -89,7 +89,7 @@ public: deterministic = makeDeterministic; } - AddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) + AddrInfo* Find(const CService& addr, int* pnId = nullptr) { LOCK(m_impl->cs); return m_impl->Find(addr, pnId); @@ -222,15 +222,15 @@ BOOST_AUTO_TEST_CASE(addrman_ports) BOOST_CHECK_EQUAL(addrman.size(), 1U); CService addr1_port = ResolveService("250.1.1.1", 8334); - BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK(addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman.size(), 2U); auto addr_ret2 = addrman.Select().first; - BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333"); + BOOST_CHECK(addr_ret2.ToString() == "250.1.1.1:8333" || addr_ret2.ToString() == "250.1.1.1:8334"); - // Test: Add same IP but diff port to tried table, it doesn't get added. - // Perhaps this is not ideal behavior but it is the current behavior. + // Test: Add same IP but diff port to tried table; this converts the entry with + // the specified port to tried, but not the other. addrman.Good(CAddress(addr1_port, NODE_NONE)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK_EQUAL(addrman.size(), 2U); bool newOnly = true; auto addr_ret3 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); @@ -369,18 +369,18 @@ BOOST_AUTO_TEST_CASE(addrman_find) CNetAddr source2 = ResolveIP("250.1.2.2"); BOOST_CHECK(addrman.Add({addr1}, source1)); - BOOST_CHECK(!addrman.Add({addr2}, source2)); + BOOST_CHECK(addrman.Add({addr2}, source2)); BOOST_CHECK(addrman.Add({addr3}, source1)); - // Test: ensure Find returns an IP matching what we searched on. + // Test: ensure Find returns an IP/port matching what we searched on. AddrInfo* info1 = addrman.Find(addr1); BOOST_REQUIRE(info1); BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333"); - // Test 18; Find does not discriminate by port number. + // Test; Find discriminates by port number. AddrInfo* info2 = addrman.Find(addr2); BOOST_REQUIRE(info2); - BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString()); + BOOST_CHECK_EQUAL(info2->ToString(), "250.1.2.1:9999"); // Test: Find returns another IP matching what we searched on. AddrInfo* info3 = addrman.Find(addr3); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 8df3707fc9..c6df6a0e61 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -137,24 +137,29 @@ public: // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`. // Keys may be different. - using AddrInfoHasher = std::function; - using AddrInfoEq = std::function; - - CNetAddrHash netaddr_hasher; - - AddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const AddrInfo& a) { - return netaddr_hasher(static_cast(a)) ^ netaddr_hasher(a.source) ^ - a.nLastSuccess ^ a.nAttempts ^ a.nRefCount ^ a.fInTried; + auto addrinfo_hasher = [](const AddrInfo& a) { + CSipHasher hasher(0, 0); + auto addr_key = a.GetKey(); + auto source_key = a.source.GetAddrBytes(); + hasher.Write(a.nLastSuccess); + hasher.Write(a.nAttempts); + hasher.Write(a.nRefCount); + hasher.Write(a.fInTried); + hasher.Write(a.GetNetwork()); + hasher.Write(a.source.GetNetwork()); + hasher.Write(addr_key.size()); + hasher.Write(source_key.size()); + hasher.Write(addr_key.data(), addr_key.size()); + hasher.Write(source_key.data(), source_key.size()); + return (size_t)hasher.Finalize(); }; - AddrInfoEq addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) { - return static_cast(lhs) == static_cast(rhs) && - lhs.source == rhs.source && lhs.nLastSuccess == rhs.nLastSuccess && - lhs.nAttempts == rhs.nAttempts && lhs.nRefCount == rhs.nRefCount && - lhs.fInTried == rhs.fInTried; + auto addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) { + return std::tie(static_cast(lhs), lhs.source, lhs.nLastSuccess, lhs.nAttempts, lhs.nRefCount, lhs.fInTried) == + std::tie(static_cast(rhs), rhs.source, rhs.nLastSuccess, rhs.nAttempts, rhs.nRefCount, rhs.fInTried); }; - using Addresses = std::unordered_set; + using Addresses = std::unordered_set; const size_t num_addresses{m_impl->mapInfo.size()};