diff options
author | MarcoFalke <falke.marco@gmail.com> | 2021-12-15 09:53:00 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2021-12-15 09:53:23 +0100 |
commit | 40fdb9ece979622478fcda989c14dfb7536c4c8c (patch) | |
tree | 20a840879ef53b29b0498af9626f0e10998ab79d /src | |
parent | 9015d118425df4a5b34455da15334901ece85ed3 (diff) | |
parent | caac999ff0fc5c98fa438b7e96fe1232f6573fd5 (diff) | |
download | bitcoin-40fdb9ece979622478fcda989c14dfb7536c4c8c.tar.xz |
Merge bitcoin/bitcoin#23713: refactor, test: refactor addrman_tried_collisions test to directly check for collisions
caac999ff0fc5c98fa438b7e96fe1232f6573fd5 refactor: remove dependence on AddrManTest (josibake)
f961c477b56737c546c275e4d86cecfa3f75d48c refactor: check Good() in tried_collisions test (josibake)
207f1c825c632c54af009516d376d392ea9106fa refactor: make AddrMan::Good return bool (josibake)
Pull request description:
Previously, the `addrman_tried_collisions` test behaved in the following way:
1. add an address to addrman
2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
3. verify that `num_addrs` matched `size()` to check for collisions in the new table
`AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.
While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.
### solution
To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.
Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945).
### followup
As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:
* https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945
* https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net.cpp#L2067
* https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net_processing.cpp#L2708
ACKs for top commit:
jnewbery:
utACK caac999ff0
mzumsande:
Code review ACK caac999ff0fc5c98fa438b7e96fe1232f6573fd5
Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99
Diffstat (limited to 'src')
-rw-r--r-- | src/addrman.cpp | 24 | ||||
-rw-r--r-- | src/addrman.h | 10 | ||||
-rw-r--r-- | src/addrman_impl.h | 4 | ||||
-rw-r--r-- | src/test/addrman_tests.cpp | 26 |
4 files changed, 35 insertions, 29 deletions
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<CAddress>& 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<CAddress>& 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<CAddress>& 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<CAddress>& 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<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 31f30d0379..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<AddrMan>(std::vector<bool>(), /*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 < 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)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); + + // Test: Add to tried without collision + BOOST_CHECK(addrman->Good(CAddress(addr, NODE_NONE))); - // Test: No collision in tried table yet. - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); } - // 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->Add({CAddress(addr2, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE))); } BOOST_AUTO_TEST_CASE(addrman_find) |