Skip to content

Commit 1a050d6

Browse files
kwvgkwvg
committed
merge bitcoin#23306: Make AddrMan support multiple ports per IP
1 parent d56702a commit 1a050d6

File tree

5 files changed

+58
-51
lines changed

5 files changed

+58
-51
lines changed

‎src/addrman.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -569,7 +569,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
569569
AddrInfo& info = *pinfo;
570570

571571
// check whether we are talking about the exact same CService (including same port)
572-
if (info != addr)
572+
if (!m_discriminate_ports && info != addr)
573573
return;
574574

575575
// update info
@@ -707,7 +707,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi
707707
AddrInfo& info = *pinfo;
708708

709709
// check whether we are talking about the exact same CService (including same port)
710-
if (info != addr)
710+
if (!m_discriminate_ports && info != addr)
711711
return;
712712

713713
// update info
@@ -838,7 +838,7 @@ void AddrManImpl::Connected_(const CService& addr, int64_t nTime)
838838
AddrInfo& info = *pinfo;
839839

840840
// check whether we are talking about the exact same CService (including same port)
841-
if (info != addr)
841+
if (!m_discriminate_ports && info != addr)
842842
return;
843843

844844
// update info
@@ -860,7 +860,7 @@ void AddrManImpl::SetServices_(const CService& addr, ServiceFlags nServices)
860860
AddrInfo& info = *pinfo;
861861

862862
// check whether we are talking about the exact same CService (including same port)
863-
if (info != addr)
863+
if (!m_discriminate_ports && info != addr)
864864
return;
865865

866866
// update info
@@ -878,7 +878,7 @@ AddrInfo AddrManImpl::GetAddressInfo_(const CService& addr)
878878
AddrInfo& info = *pinfo;
879879

880880
// check whether we are talking about the exact same CService (including same port)
881-
if (info != addr)
881+
if (!m_discriminate_ports && info != addr)
882882
return AddrInfo();
883883

884884
return *pinfo;

‎src/addrman_impl.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,8 @@ class AddrManImpl
181181
//! table with information about all nIds
182182
std::unordered_map<int, AddrInfo> mapInfo GUARDED_BY(cs);
183183

184-
//! find an nId based on its network address
185-
std::unordered_map<CNetAddr, int, CNetAddrHash> mapAddr GUARDED_BY(cs);
184+
//! find an nId based on its network address and port.
185+
std::unordered_map<CService, int, CServiceHash> mapAddr GUARDED_BY(cs);
186186

187187
//! randomly-ordered vector of all nIds
188188
//! This is mutable because it is unobservable outside the class, so any

‎src/netaddress.h‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,6 @@ class CNetAddr
261261
}
262262
}
263263

264-
friend class CNetAddrHash;
265264
friend class CSubNet;
266265

267266
private:
@@ -482,22 +481,6 @@ class CNetAddr
482481
}
483482
};
484483

485-
class CNetAddrHash
486-
{
487-
public:
488-
size_t operator()(const CNetAddr& a) const noexcept
489-
{
490-
CSipHasher hasher(m_salt_k0, m_salt_k1);
491-
hasher.Write(a.m_net);
492-
hasher.Write(a.m_addr.data(), a.m_addr.size());
493-
return static_cast<size_t>(hasher.Finalize());
494-
}
495-
496-
private:
497-
const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
498-
const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
499-
};
500-
501484
class CSubNet
502485
{
503486
protected:
@@ -582,7 +565,25 @@ class CService : public CNetAddr
582565
READWRITE(Using<BigEndianFormatter<2>>(obj.port));
583566
}
584567

568+
friend class CServiceHash;
585569
friend CService MaybeFlipIPv6toCJDNS(const CService& service);
586570
};
587571

572+
class CServiceHash
573+
{
574+
public:
575+
size_t operator()(const CService& a) const noexcept
576+
{
577+
CSipHasher hasher(m_salt_k0, m_salt_k1);
578+
hasher.Write(a.m_net);
579+
hasher.Write(a.port);
580+
hasher.Write(a.m_addr.data(), a.m_addr.size());
581+
return static_cast<size_t>(hasher.Finalize());
582+
}
583+
584+
private:
585+
const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
586+
const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
587+
};
588+
588589
#endif // BITCOIN_NETADDRESS_H

‎src/test/addrman_tests.cpp‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ class AddrManSerializationMock : public AddrMan
2626
virtual void Serialize(CDataStream& s) const = 0;
2727

2828
AddrManSerializationMock()
29-
: AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100)
29+
: AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100, /* discriminate_ports */ true)
3030
{}
3131
};
3232

@@ -82,8 +82,9 @@ class AddrManTest : public AddrMan
8282

8383
public:
8484
explicit AddrManTest(bool makeDeterministic = true,
85-
std::vector<bool> asmap = std::vector<bool>())
86-
: AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100)
85+
std::vector<bool> asmap = std::vector<bool>(),
86+
bool discriminatePorts = true)
87+
: AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100, discriminatePorts)
8788
{
8889
deterministic = makeDeterministic;
8990
}
@@ -221,15 +222,15 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
221222
BOOST_CHECK_EQUAL(addrman.size(), 1U);
222223

223224
CService addr1_port = ResolveService("250.1.1.1", 8334);
224-
BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source));
225-
BOOST_CHECK_EQUAL(addrman.size(), 1U);
225+
BOOST_CHECK(addrman.Add({CAddress(addr1_port, NODE_NONE)}, source));
226+
BOOST_CHECK_EQUAL(addrman.size(), 2U);
226227
auto addr_ret2 = addrman.Select().first;
227-
BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333");
228+
BOOST_CHECK(addr_ret2.ToString() == "250.1.1.1:8333" || addr_ret2.ToString() == "250.1.1.1:8334");
228229

229-
// Test: Add same IP but diff port to tried table, it doesn't get added.
230-
// Perhaps this is not ideal behavior but it is the current behavior.
230+
// Test: Add same IP but diff port to tried table; this converts the entry with
231+
// the specified port to tried, but not the other.
231232
addrman.Good(CAddress(addr1_port, NODE_NONE));
232-
BOOST_CHECK_EQUAL(addrman.size(), 1U);
233+
BOOST_CHECK_EQUAL(addrman.size(), 2U);
233234
bool newOnly = true;
234235
auto addr_ret3 = addrman.Select(newOnly).first;
235236
BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
@@ -368,18 +369,18 @@ BOOST_AUTO_TEST_CASE(addrman_find)
368369
CNetAddr source2 = ResolveIP("250.1.2.2");
369370

370371
BOOST_CHECK(addrman.Add({addr1}, source1));
371-
BOOST_CHECK(!addrman.Add({addr2}, source2));
372+
BOOST_CHECK(addrman.Add({addr2}, source2));
372373
BOOST_CHECK(addrman.Add({addr3}, source1));
373374

374-
// Test: ensure Find returns an IP matching what we searched on.
375+
// Test: ensure Find returns an IP/port matching what we searched on.
375376
AddrInfo* info1 = addrman.Find(addr1);
376377
BOOST_REQUIRE(info1);
377378
BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333");
378379

379-
// Test 18; Find does not discriminate by port number.
380+
// Test; Find discriminates by port number.
380381
AddrInfo* info2 = addrman.Find(addr2);
381382
BOOST_REQUIRE(info2);
382-
BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString());
383+
BOOST_CHECK_EQUAL(info2->ToString(), "250.1.2.1:9999");
383384

384385
// Test: Find returns another IP matching what we searched on.
385386
AddrInfo* info3 = addrman.Find(addr3);

‎src/test/fuzz/addrman.cpp‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -137,24 +137,29 @@ class AddrManDeterministic : public AddrMan
137137
// Check that all values in `mapInfo` are equal to all values in `other.mapInfo`.
138138
// Keys may be different.
139139

140-
using AddrInfoHasher = std::function<size_t(const AddrInfo&)>;
141-
using AddrInfoEq = std::function<bool(const AddrInfo&, const AddrInfo&)>;
142-
143-
CNetAddrHash netaddr_hasher;
144-
145-
AddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const AddrInfo& a) {
146-
return netaddr_hasher(static_cast<CNetAddr>(a)) ^ netaddr_hasher(a.source) ^
147-
a.nLastSuccess ^ a.nAttempts ^ a.nRefCount ^ a.fInTried;
140+
auto addrinfo_hasher = [](const AddrInfo& a) {
141+
CSipHasher hasher(0, 0);
142+
auto addr_key = a.GetKey();
143+
auto source_key = a.source.GetAddrBytes();
144+
hasher.Write(a.nLastSuccess);
145+
hasher.Write(a.nAttempts);
146+
hasher.Write(a.nRefCount);
147+
hasher.Write(a.fInTried);
148+
hasher.Write(a.GetNetwork());
149+
hasher.Write(a.source.GetNetwork());
150+
hasher.Write(addr_key.size());
151+
hasher.Write(source_key.size());
152+
hasher.Write(addr_key.data(), addr_key.size());
153+
hasher.Write(source_key.data(), source_key.size());
154+
return (size_t)hasher.Finalize();
148155
};
149156

150-
AddrInfoEq addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) {
151-
return static_cast<CNetAddr>(lhs) == static_cast<CNetAddr>(rhs) &&
152-
lhs.source == rhs.source && lhs.nLastSuccess == rhs.nLastSuccess &&
153-
lhs.nAttempts == rhs.nAttempts && lhs.nRefCount == rhs.nRefCount &&
154-
lhs.fInTried == rhs.fInTried;
157+
auto addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) {
158+
return std::tie(static_cast<const CService&>(lhs), lhs.source, lhs.nLastSuccess, lhs.nAttempts, lhs.nRefCount, lhs.fInTried) ==
159+
std::tie(static_cast<const CService&>(rhs), rhs.source, rhs.nLastSuccess, rhs.nAttempts, rhs.nRefCount, rhs.fInTried);
155160
};
156161

157-
using Addresses = std::unordered_set<AddrInfo, AddrInfoHasher, AddrInfoEq>;
162+
using Addresses = std::unordered_set<AddrInfo, decltype(addrinfo_hasher), decltype(addrinfo_eq)>;
158163

159164
const size_t num_addresses{m_impl->mapInfo.size()};
160165

0 commit comments

Comments
 (0)