From 207f1c825c632c54af009516d376d392ea9106fa Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 10 Dec 2021 15:37:04 +0100 Subject: refactor: make AddrMan::Good return bool If AddrMan::Good is unable to add an entry to tried (for a number of reasons), return false. This makes it much easier and cleaner to directly test for tried collisions. It also allows anyone calling Good() to handle the case where adding an address to tried is unsuccessful. Update docs to doxygen style. --- src/addrman.cpp | 24 ++++++++++++------------ src/addrman.h | 10 ++++++++-- src/addrman_impl.h | 4 ++-- 3 files changed, 22 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/addrman.cpp b/src/addrman.cpp index 8a0433c40d..4d785043b8 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -614,7 +614,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_ return fInsert; } -void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) +bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) { AssertLockHeld(cs); @@ -625,8 +625,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT AddrInfo* pinfo = Find(addr, &nId); // if not found, bail out - if (!pinfo) - return; + if (!pinfo) return false; AddrInfo& info = *pinfo; @@ -638,13 +637,11 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT // currently-connected peers. // if it is already in the tried set, don't do anything else - if (info.fInTried) - return; + if (info.fInTried) return false; // if it is not in new, something bad happened - if (!Assume(info.nRefCount > 0)) { - return; - } + if (!Assume(info.nRefCount > 0)) return false; + // which tried bucket to move the entry to int tried_bucket = info.GetTriedBucket(nKey, m_asmap); @@ -661,11 +658,13 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "", addr.ToString(), m_tried_collisions.size()); + return false; } else { // move nId to the tried tables MakeTried(info, nId); LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n", addr.ToString(), addr.GetMappedAS(m_asmap), tried_bucket, tried_bucket_pos); + return true; } } @@ -1049,12 +1048,13 @@ bool AddrManImpl::Add(const std::vector& vAddr, const CNetAddr& source return ret; } -void AddrManImpl::Good(const CService& addr, int64_t nTime) +bool AddrManImpl::Good(const CService& addr, int64_t nTime) { LOCK(cs); Check(); - Good_(addr, /* test_before_evict */ true, nTime); + auto ret = Good_(addr, /*test_before_evict=*/true, nTime); Check(); + return ret; } void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, int64_t nTime) @@ -1157,9 +1157,9 @@ bool AddrMan::Add(const std::vector& vAddr, const CNetAddr& source, in return m_impl->Add(vAddr, source, nTimePenalty); } -void AddrMan::Good(const CService& addr, int64_t nTime) +bool AddrMan::Good(const CService& addr, int64_t nTime) { - m_impl->Good(addr, nTime); + return m_impl->Good(addr, nTime); } void AddrMan::Attempt(const CService& addr, bool fCountFailure, int64_t nTime) diff --git a/src/addrman.h b/src/addrman.h index 455d84ca71..e9bc372380 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -81,8 +81,14 @@ public: * @return true if at least one address is successfully added. */ bool Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0); - //! Mark an entry as accessible, possibly moving it from "new" to "tried". - void Good(const CService& addr, int64_t nTime = GetAdjustedTime()); + /** + * Mark an address record as accessible and attempt to move it to addrman's tried table. + * + * @param[in] addr Address record to attempt to move to tried table. + * @param[in] nTime The time that we were last connected to this peer. + * @return true if the address is successfully moved from the new table to the tried table. + */ + bool Good(const CService& addr, int64_t nTime = GetAdjustedTime()); //! Mark an entry as connection attempted to. void Attempt(const CService& addr, bool fCountFailure, int64_t nTime = GetAdjustedTime()); diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 10a65871c1..bd7caf473b 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -115,7 +115,7 @@ public: bool Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(!cs); - void Good(const CService& addr, int64_t nTime) + bool Good(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(!cs); void Attempt(const CService& addr, bool fCountFailure, int64_t nTime) @@ -248,7 +248,7 @@ private: * @see AddrMan::Add() for parameters. */ bool AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); - void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); + bool Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); bool Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); -- cgit v1.2.3 From f961c477b56737c546c275e4d86cecfa3f75d48c Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 10 Dec 2021 16:06:28 +0100 Subject: refactor: check Good() in tried_collisions test Rather than try to infer a collision by checking `AddrMan::size`, check whether or not moving to the tried table was successful by checking the output from `AddrMan::Good` --- src/test/addrman_tests.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 31f30d0379..d74cd5bd4b 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -276,24 +276,24 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) BOOST_CHECK_EQUAL(addrman.size(), num_addrs); - while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1 + while (num_addrs < 35) { // Magic number! 250.1.1.1 - 250.1.1.35 do not collide in tried with deterministic key = 1 CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(CAddress(addr, NODE_NONE)); - // Test: No collision in tried table yet. - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); + // Test: Add to tried without collision + BOOST_CHECK(addrman.Good(CAddress(addr, NODE_NONE))); + } - // Test: tried table collision! + // Test: Unable to add to tried table due to collision! CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs)); - uint32_t collisions{1}; - BOOST_CHECK(!addrman.Add({CAddress(addr1, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); + BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK(!addrman.Good(CAddress(addr1, NODE_NONE))); + // Test: Add the next address to tried without collision CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); + BOOST_CHECK(addrman.Good(CAddress(addr2, NODE_NONE))); } BOOST_AUTO_TEST_CASE(addrman_find) -- cgit v1.2.3 From caac999ff0fc5c98fa438b7e96fe1232f6573fd5 Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 10 Dec 2021 19:50:42 +0100 Subject: refactor: remove dependence on AddrManTest --- src/test/addrman_tests.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index d74cd5bd4b..b700c3ae22 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -268,32 +268,32 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) BOOST_AUTO_TEST_CASE(addrman_tried_collisions) { - AddrManTest addrman; + auto addrman = std::make_unique(std::vector(), /*deterministic=*/true, /*consistency_check_ratio=*/100); CNetAddr source = ResolveIP("252.2.2.2"); uint32_t num_addrs{0}; - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); + BOOST_CHECK_EQUAL(addrman->size(), num_addrs); while (num_addrs < 35) { // Magic number! 250.1.1.1 - 250.1.1.35 do not collide in tried with deterministic key = 1 CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); // Test: Add to tried without collision - BOOST_CHECK(addrman.Good(CAddress(addr, NODE_NONE))); + BOOST_CHECK(addrman->Good(CAddress(addr, NODE_NONE))); } // Test: Unable to add to tried table due to collision! CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs)); - BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); - BOOST_CHECK(!addrman.Good(CAddress(addr1, NODE_NONE))); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK(!addrman->Good(CAddress(addr1, NODE_NONE))); // Test: Add the next address to tried without collision CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); - BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source)); - BOOST_CHECK(addrman.Good(CAddress(addr2, NODE_NONE))); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE))); } BOOST_AUTO_TEST_CASE(addrman_find) -- cgit v1.2.3