From a93fec6f2d48559ffbd4b06a97df6894eaf34d31 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 28 Oct 2021 12:46:26 +0100 Subject: [PATCH] merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging --- src/addrman.cpp | 167 ++++++++++++++-------------- src/addrman.h | 10 +- src/addrman_impl.h | 6 +- src/test/addrman_tests.cpp | 2 +- test/functional/p2p_addr_relay.py | 1 - test/functional/p2p_addrv2_relay.py | 3 - 6 files changed, 101 insertions(+), 88 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 285407b3f0..5664b0559a 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -552,6 +552,83 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId) info.fInTried = true; } +bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) +{ + AssertLockHeld(cs); + + if (!addr.IsRoutable()) + return false; + + int nId; + AddrInfo* pinfo = Find(addr, &nId); + + // Do not set a penalty for a source's self-announcement + if (addr == source) { + nTimePenalty = 0; + } + + if (pinfo) { + // periodically update nTime + bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60); + int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60); + if (pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty) { + pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty); + } + + // add services + pinfo->nServices = ServiceFlags(pinfo->nServices | addr.nServices); + + // do not update if no new information is present + if (addr.nTime <= pinfo->nTime) { + return false; + } + + // do not update if the entry was already in the "tried" table + if (pinfo->fInTried) + return false; + + // do not update if the max reference count is reached + if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS) + return false; + + // stochastic test: previous nRefCount == N: 2^N times harder to increase it + int nFactor = 1; + for (int n = 0; n < pinfo->nRefCount; n++) + nFactor *= 2; + if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0)) + return false; + } else { + pinfo = Create(addr, source, &nId); + pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); + nNew++; + } + + 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) { + if (!fInsert) { + AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]]; + if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) { + // Overwrite the existing new table entry. + fInsert = true; + } + } + if (fInsert) { + ClearNew(nUBucket, nUBucketPos); + pinfo->nRefCount++; + vvNew[nUBucket][nUBucketPos] = nId; + LogPrint(BCLog::ADDRMAN, "Added %s mapped to AS%i to new[%i][%i]\n", + addr.ToString(), addr.GetMappedAS(m_asmap), nUBucket, nUBucketPos); + } else { + if (pinfo->nRefCount == 0) { + Delete(nId); + } + } + } + return fInsert; +} + void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) { AssertLockHeld(cs); @@ -615,83 +692,16 @@ 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::Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) { - AssertLockHeld(cs); - - if (!addr.IsRoutable()) - return false; - - bool fNew = false; - int nId; - AddrInfo* pinfo = Find(addr, &nId); - - // Do not set a penalty for a source's self-announcement - if (addr == source) { - nTimePenalty = 0; + int added{0}; + for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) { + added += AddSingle(*it, source, nTimePenalty) ? 1 : 0; } - - if (pinfo) { - // periodically update nTime - bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60); - int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60); - if (pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty) { - pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty); - } - - // add services - pinfo->nServices = ServiceFlags(pinfo->nServices | addr.nServices); - - // do not update if no new information is present - if (addr.nTime <= pinfo->nTime) { - return false; - } - - // do not update if the entry was already in the "tried" table - if (pinfo->fInTried) - return false; - - // do not update if the max reference count is reached - if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS) - return false; - - // stochastic test: previous nRefCount == N: 2^N times harder to increase it - int nFactor = 1; - for (int n = 0; n < pinfo->nRefCount; n++) - nFactor *= 2; - if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0)) - return false; - } else { - pinfo = Create(addr, source, &nId); - pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); - nNew++; - fNew = true; + 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); } - - int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap); - int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket); - 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)) { - // Overwrite the existing new table entry. - fInsert = true; - } - } - if (fInsert) { - ClearNew(nUBucket, nUBucketPos); - pinfo->nRefCount++; - vvNew[nUBucket][nUBucketPos] = nId; - LogPrint(BCLog::ADDRMAN, "Added %s mapped to AS%i to new[%i][%i]\n", - addr.ToString(), addr.GetMappedAS(m_asmap), nUBucket, nUBucketPos); - } else { - if (pinfo->nRefCount == 0) { - Delete(nId); - } - } - } - return fNew; + return added > 0; } void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) @@ -1087,15 +1097,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 += Add_(*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 49ea77cdff..3515d98cae 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -70,7 +70,15 @@ 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). + * @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 2e4093eeb6..d139c1f554 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -248,9 +248,13 @@ private: //! Move an entry from the "new" table(s) to the "tried" table void MakeTried(AddrInfo& info, int nId) 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 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); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 3b3d3ae311..b65c36b92b 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -374,7 +374,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 bd8d08b78c..9437ee9621 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -148,7 +148,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 0e5530259e..49984f4df3 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -74,9 +74,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=1', ]): addr_source.send_and_ping(msg)