Skip to content

Commit 8b2db6b

Browse files
kwvgkwvg
committed
merge bitcoin#23713: refactor addrman_tried_collisions test to directly check for collisions
1 parent 5b5dd39 commit 8b2db6b

File tree

4 files changed

+35
-29
lines changed

4 files changed

+35
-29
lines changed

‎src/addrman.cpp‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -616,7 +616,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
616616
return fInsert;
617617
}
618618

619-
void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
619+
bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
620620
{
621621
AssertLockHeld(cs);
622622

@@ -627,8 +627,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
627627
AddrInfo* pinfo = Find(addr, &nId);
628628

629629
// if not found, bail out
630-
if (!pinfo)
631-
return;
630+
if (!pinfo) return false;
632631

633632
AddrInfo& info = *pinfo;
634633

@@ -640,13 +639,11 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
640639
// currently-connected peers.
641640

642641
// if it is already in the tried set, don't do anything else
643-
if (info.fInTried)
644-
return;
642+
if (info.fInTried) return false;
645643

646644
// if it is not in new, something bad happened
647-
if (!Assume(info.nRefCount > 0)) {
648-
return;
649-
}
645+
if (!Assume(info.nRefCount > 0)) return false;
646+
650647

651648
// which tried bucket to move the entry to
652649
int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
@@ -665,13 +662,15 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
665662
addr.ToString(),
666663
m_tried_collisions.size());
667664
}
665+
return false;
668666
} else {
669667
// move nId to the tried tables
670668
MakeTried(info, nId);
671669
if (fLogIPs) {
672670
LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n",
673671
addr.ToString(), addr.GetMappedAS(m_asmap), tried_bucket, tried_bucket_pos);
674672
}
673+
return true;
675674
}
676675
}
677676

@@ -1068,12 +1067,13 @@ bool AddrManImpl::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source
10681067
return ret;
10691068
}
10701069

1071-
void AddrManImpl::Good(const CService& addr, int64_t nTime)
1070+
bool AddrManImpl::Good(const CService& addr, int64_t nTime)
10721071
{
10731072
LOCK(cs);
10741073
Check();
1075-
Good_(addr, /* test_before_evict */ true, nTime);
1074+
auto ret = Good_(addr, /*test_before_evict=*/true, nTime);
10761075
Check();
1076+
return ret;
10771077
}
10781078

10791079
void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -1187,9 +1187,9 @@ bool AddrMan::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, in
11871187
return m_impl->Add(vAddr, source, nTimePenalty);
11881188
}
11891189

1190-
void AddrMan::Good(const CService& addr, int64_t nTime)
1190+
bool AddrMan::Good(const CService& addr, int64_t nTime)
11911191
{
1192-
m_impl->Good(addr, nTime);
1192+
return m_impl->Good(addr, nTime);
11931193
}
11941194

11951195
void AddrMan::Attempt(const CService& addr, bool fCountFailure, int64_t nTime)

‎src/addrman.h‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,14 @@ class AddrMan
9292
* @return true if at least one address is successfully added. */
9393
bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0);
9494

95-
//! Mark an entry as accessible, possibly moving it from "new" to "tried".
96-
void Good(const CService& addr, int64_t nTime = GetAdjustedTime());
95+
/**
96+
* Mark an address record as accessible and attempt to move it to addrman's tried table.
97+
*
98+
* @param[in] addr Address record to attempt to move to tried table.
99+
* @param[in] nTime The time that we were last connected to this peer.
100+
* @return true if the address is successfully moved from the new table to the tried table.
101+
*/
102+
bool Good(const CService& addr, int64_t nTime = GetAdjustedTime());
97103

98104
//! Mark an entry as connection attempted to.
99105
void Attempt(const CService& addr, bool fCountFailure, int64_t nTime = GetAdjustedTime());

‎src/addrman_impl.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ class AddrManImpl
115115
bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty)
116116
EXCLUSIVE_LOCKS_REQUIRED(!cs);
117117

118-
void Good(const CService& addr, int64_t nTime)
118+
bool Good(const CService& addr, int64_t nTime)
119119
EXCLUSIVE_LOCKS_REQUIRED(!cs);
120120

121121
void Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -250,7 +250,7 @@ class AddrManImpl
250250
* @see AddrMan::Add() for parameters. */
251251
bool AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
252252

253-
void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
253+
bool Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
254254

255255
bool Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
256256

‎src/test/addrman_tests.cpp‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -265,32 +265,32 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
265265

266266
BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
267267
{
268-
AddrManTest addrman;
268+
auto addrman = std::make_unique<AddrMan>(std::vector<bool>(), /*deterministic=*/true, /*consistency_check_ratio=*/100);
269269

270270
CNetAddr source = ResolveIP("252.2.2.2");
271271

272272
uint32_t num_addrs{0};
273273

274-
BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
274+
BOOST_CHECK_EQUAL(addrman->size(), num_addrs);
275275

276-
while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1
276+
while (num_addrs < 35) { // Magic number! 250.1.1.1 - 250.1.1.35 do not collide in tried with deterministic key = 1
277277
CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
278-
BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source));
279-
addrman.Good(CAddress(addr, NODE_NONE));
278+
BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source));
279+
280+
// Test: Add to tried without collision
281+
BOOST_CHECK(addrman->Good(CAddress(addr, NODE_NONE)));
280282

281-
// Test: No collision in tried table yet.
282-
BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
283283
}
284284

285-
// Test: tried table collision!
285+
// Test: Unable to add to tried table due to collision!
286286
CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
287-
uint32_t collisions{1};
288-
BOOST_CHECK(!addrman.Add({CAddress(addr1, NODE_NONE)}, source));
289-
BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
287+
BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
288+
BOOST_CHECK(!addrman->Good(CAddress(addr1, NODE_NONE)));
290289

290+
// Test: Add the next address to tried without collision
291291
CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
292-
BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source));
293-
BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
292+
BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source));
293+
BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE)));
294294
}
295295

296296
BOOST_AUTO_TEST_CASE(addrman_find)

0 commit comments

Comments
 (0)