From e58598e833d5737900fe3c4369e26f2a08166892 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 28 Oct 2021 12:46:26 +0100 Subject: [addrman] Add doxygen comment to AddrMan::Add() Does not document the return value since we're going to fix the semantics in a future commit. --- src/addrman.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/addrman.h b/src/addrman.h index 174ab4f811..1fbc827183 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -69,7 +69,14 @@ public: //! Return the number of (unique) addresses in all tables. size_t size() const; - //! Add addresses to addrman's new table. + /** + * Attempt to add one or more addresses to addrman's new table. + * + * @param[in] vAddr Address records to attempt to add. + * @param[in] source The address of the node that sent us these addr records. + * @param[in] nTimePenalty A "time penalty" to apply to the address record's nTime. If a peer + * sends us an address record with nTime=n, then we'll add it to our + * addrman with nTime=(n - nTimePenalty). */ 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". -- cgit v1.2.3 From 2658eb6d68460272deefb3fcc653b03f6ec6e7cf Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 1 Oct 2021 17:15:46 +0100 Subject: [addrman] Rename Add_() to AddSingle() --- src/addrman.cpp | 4 ++-- src/addrman_impl.h | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 86f56052c1..b0555898e2 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -592,7 +592,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT } } -bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) +bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) { AssertLockHeld(cs); @@ -1034,7 +1034,7 @@ bool AddrManImpl::Add(const std::vector& vAddr, const CNetAddr& source int nAdd = 0; Check(); for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) - nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0; + nAdd += AddSingle(*it, source, nTimePenalty) ? 1 : 0; Check(); if (nAdd) { LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew); diff --git a/src/addrman_impl.h b/src/addrman_impl.h index f8191d6b85..2d01881f2e 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -244,7 +244,9 @@ private: void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); - bool Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Attempt to add a single address to addrman's new table. + * @see AddrMan::Add() for parameters. */ + bool AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); -- cgit v1.2.3 From 2095df7b7bfcb9ab0c5710a93112f7f341e753c9 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 1 Oct 2021 17:26:08 +0100 Subject: [addrman] Add Add_() inner function, fix Add() return semantics Previously, Add() would return true if the function created a new AddressInfo object, even if that object could not be successfully entered into the new table and was deleted. That would happen if the new table position was already taken and the existing entry could not be removed. Instead, return true if the new AddressInfo object is successfully entered into the new table. This fixes a bug in the "Added %i addresses" log, which would not always accurately log how many addresses had been added. p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they were incorrectly asserting on the buggy log (assuming that addresses are added to addrman, when there could in fact be new table position collisions that prevent some of those address records from being added). --- src/addrman.cpp | 27 ++++++++++++++++----------- src/addrman.h | 3 ++- src/addrman_impl.h | 2 ++ src/test/addrman_tests.cpp | 2 +- test/functional/p2p_addr_relay.py | 1 - test/functional/p2p_addrv2_relay.py | 3 --- 6 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index b0555898e2..0222f625be 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -599,7 +599,6 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_ if (!addr.IsRoutable()) return false; - bool fNew = false; int nId; AddrInfo* pinfo = Find(addr, &nId); @@ -640,13 +639,12 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_ pinfo = Create(addr, source, &nId); pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); nNew++; - fNew = true; } int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap); int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket); + bool fInsert = vvNew[nUBucket][nUBucketPos] == -1; if (vvNew[nUBucket][nUBucketPos] != nId) { - bool fInsert = vvNew[nUBucket][nUBucketPos] == -1; if (!fInsert) { AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]]; if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) { @@ -666,7 +664,19 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_ } } } - return fNew; + return fInsert; +} + +bool AddrManImpl::Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) +{ + int added{0}; + for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) { + added += AddSingle(*it, source, nTimePenalty) ? 1 : 0; + } + if (added > 0) { + LogPrint(BCLog::ADDRMAN, "Added %i addresses (of %i) from %s: %i tried, %i new\n", added, vAddr.size(), source.ToString(), nTried, nNew); + } + return added > 0; } void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) @@ -1031,15 +1041,10 @@ size_t AddrManImpl::size() const bool AddrManImpl::Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) { LOCK(cs); - int nAdd = 0; Check(); - for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) - nAdd += AddSingle(*it, source, nTimePenalty) ? 1 : 0; + auto ret = Add_(vAddr, source, nTimePenalty); Check(); - if (nAdd) { - LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew); - } - return nAdd > 0; + return ret; } void AddrManImpl::Good(const CService& addr, int64_t nTime) diff --git a/src/addrman.h b/src/addrman.h index 1fbc827183..a9f697f66f 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -76,7 +76,8 @@ public: * @param[in] source The address of the node that sent us these addr records. * @param[in] nTimePenalty A "time penalty" to apply to the address record's nTime. If a peer * sends us an address record with nTime=n, then we'll add it to our - * addrman with nTime=(n - nTimePenalty). */ + * addrman with nTime=(n - nTimePenalty). + * @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". diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 2d01881f2e..c8eb73027e 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -248,6 +248,8 @@ private: * @see AddrMan::Add() for parameters. */ bool AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); + bool Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); + void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); std::pair Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 991bfa5efc..eabc11c467 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -347,7 +347,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) //Test: tried table 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(!addrman.Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 15b90fa61f..9df74ad3a0 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -152,7 +152,6 @@ class AddrTest(BitcoinTestFramework): msg = self.setup_addr_msg(num_ipv4_addrs) with self.nodes[0].assert_debug_log( [ - 'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs), 'received: addr (301 bytes) peer=1', ] ): diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py index 3833c58680..f4be893d2c 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -72,9 +72,6 @@ class AddrTest(BitcoinTestFramework): addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) msg.addrs = ADDRS with self.nodes[0].assert_debug_log([ - # The I2P address is not added to node's own addrman because it has no - # I2P reachability (thus 10 - 1 = 9). - 'Added 9 addresses from 127.0.0.1: 0 tried', 'received: addrv2 (159 bytes) peer=0', 'sending addrv2 (159 bytes) peer=1', ]): -- cgit v1.2.3 From 61ec0539b26a902a41a2602187a71f9dba3c6935 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 28 Oct 2021 12:59:29 +0100 Subject: [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp Keep the internal {Function}_() functions grouped together. Review with `git diff --color-moved=dimmed-zebra` --- src/addrman.cpp | 110 ++++++++++++++++++++++++++--------------------------- src/addrman_impl.h | 4 +- 2 files changed, 57 insertions(+), 57 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 0222f625be..53d4f0a820 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -537,61 +537,6 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId) info.fInTried = true; } -void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) -{ - AssertLockHeld(cs); - - int nId; - - nLastGood = nTime; - - AddrInfo* pinfo = Find(addr, &nId); - - // if not found, bail out - if (!pinfo) - return; - - AddrInfo& info = *pinfo; - - // update info - info.nLastSuccess = nTime; - info.nLastTry = nTime; - info.nAttempts = 0; - // nTime is not updated here, to avoid leaking information about - // currently-connected peers. - - // if it is already in the tried set, don't do anything else - if (info.fInTried) - return; - - // if it is not in new, something bad happened - if (!Assume(info.nRefCount > 0)) { - return; - } - - // which tried bucket to move the entry to - int tried_bucket = info.GetTriedBucket(nKey, m_asmap); - int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket); - - // Will moving this address into tried evict another entry? - if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) { - if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) { - m_tried_collisions.insert(nId); - } - // Output the entry we'd be colliding with, for debugging purposes - auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]); - LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n", - colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "", - addr.ToString(), - m_tried_collisions.size()); - } 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); - } -} - bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) { AssertLockHeld(cs); @@ -667,6 +612,61 @@ 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) +{ + AssertLockHeld(cs); + + int nId; + + nLastGood = nTime; + + AddrInfo* pinfo = Find(addr, &nId); + + // if not found, bail out + if (!pinfo) + return; + + AddrInfo& info = *pinfo; + + // update info + info.nLastSuccess = nTime; + info.nLastTry = nTime; + info.nAttempts = 0; + // nTime is not updated here, to avoid leaking information about + // currently-connected peers. + + // if it is already in the tried set, don't do anything else + if (info.fInTried) + return; + + // if it is not in new, something bad happened + if (!Assume(info.nRefCount > 0)) { + return; + } + + // which tried bucket to move the entry to + int tried_bucket = info.GetTriedBucket(nKey, m_asmap); + int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket); + + // Will moving this address into tried evict another entry? + if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) { + if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) { + m_tried_collisions.insert(nId); + } + // Output the entry we'd be colliding with, for debugging purposes + auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]); + LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n", + colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "", + addr.ToString(), + m_tried_collisions.size()); + } 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); + } +} + bool AddrManImpl::Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) { int added{0}; diff --git a/src/addrman_impl.h b/src/addrman_impl.h index c8eb73027e..e7a9f0e76f 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -242,12 +242,12 @@ private: //! Move an entry from the "new" table(s) to the "tried" table void MakeTried(AddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs); - void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); - /** Attempt to add a single address to addrman's new table. * @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 Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); -- cgit v1.2.3