From 28daf0d1c474e57f26f7cd8518003a466342bbea Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 3 Aug 2021 15:33:51 +0800 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#22496: addrman: Remove addrman hotfixes 65332b1178c75e1f83415bad24918996a1524866 [addrman] Remove RemoveInvalid() (John Newbery) Pull request description: PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs: - #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed) - #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets) Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants. Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`. ACKs for top commit: sipa: utACK 65332b1178c75e1f83415bad24918996a1524866. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any. fanquake: ACK 65332b1178c75e1f83415bad24918996a1524866 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code. Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7 --- src/addrman.cpp | 31 ------------------------------- src/addrman.h | 13 +++++++------ 2 files changed, 7 insertions(+), 37 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index e9cd711f463d4..6143448b69ab8 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -79,37 +79,6 @@ double CAddrInfo::GetChance(int64_t nNow) const return fChance; } -void CAddrMan::RemoveInvalid() -{ - for (size_t bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) { - for (size_t i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - const auto id = vvNew[bucket][i]; - if (id != -1 && !mapInfo[id].IsValid()) { - ClearNew(bucket, i); - } - } - } - - for (size_t bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; ++bucket) { - for (size_t i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - const auto id = vvTried[bucket][i]; - if (id == -1) { - continue; - } - const auto& addr_info = mapInfo[id]; - if (addr_info.IsValid()) { - continue; - } - vvTried[bucket][i] = -1; - --nTried; - SwapRandom(addr_info.nRandomPos, vRandom.size() - 1); - vRandom.pop_back(); - mapAddr.erase(addr_info); - mapInfo.erase(id); - m_tried_collisions.erase(id); - } - } -} CAddrInfo* CAddrMan::Find(const CService& addr, int* pnId) { diff --git a/src/addrman.h b/src/addrman.h index c41bc86abe502..6596cbff7d056 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -371,7 +371,8 @@ class CAddrMan s >> info; int nKBucket = info.GetTriedBucket(nKey, m_asmap); int nKBucketPos = info.GetBucketPosition(nKey, false, nKBucket); - if (vvTried[nKBucket][nKBucketPos] == -1) { + if (info.IsValid() + && vvTried[nKBucket][nKBucketPos] == -1) { info.nRandomPos = vRandom.size(); info.fInTried = true; vRandom.push_back(nIdCount); @@ -425,6 +426,9 @@ class CAddrMan const int entry_index{bucket_entry.second}; CAddrInfo& info = mapInfo[entry_index]; + // Don't store the entry in the new bucket if it's not a valid address for our addrman + if (!info.IsValid()) continue; + // The entry shouldn't appear in more than // ADDRMAN_NEW_BUCKETS_PER_ADDRESS. If it has already, just skip // this bucket_entry. @@ -447,7 +451,7 @@ class CAddrMan } } - // Prune new entries with refcount 0 (as a result of collisions). + // Prune new entries with refcount 0 (as a result of collisions or invalid address). int nLostUnk = 0; for (auto it = mapInfo.cbegin(); it != mapInfo.cend(); ) { if (it->second.fInTried == false && it->second.nRefCount == 0) { @@ -459,10 +463,9 @@ class CAddrMan } } if (nLost + nLostUnk > 0) { - LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions\n", nLostUnk, nLost); + LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions or invalid addresses\n", nLostUnk, nLost); } - RemoveInvalid(); Check(); } @@ -795,8 +798,6 @@ class CAddrMan //! Get address info for address CAddrInfo GetAddressInfo_(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(cs); - //! Remove invalid addresses. - void RemoveInvalid() EXCLUSIVE_LOCKS_REQUIRED(cs); friend class CAddrManTest; friend class CAddrManDeterministic; From c3f8acf96dff3a84213abe82021b8c94d8b86165 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Wed, 19 May 2021 09:19:07 +0200 Subject: [PATCH 2/2] Merge bitcoin/bitcoin#21985: net: Return IPv6 scope id in `CNetAddr::ToStringIP()` 6c280adcd865ae3da4df53d630c9bf737283a56f net: Return IPv6 scope id in `CNetAddr::ToStringIP()` (W. J. van der Laan) Pull request description: If a scope id is provided, return it back in the string representation. Also bring back the test (now in platform independent fashion). Closes #21982. Includes #21961 (apart from the MacOS remark). ACKs for top commit: practicalswift: cr ACK 6c280adcd865ae3da4df53d630c9bf737283a56f Tree-SHA512: 77792c35679b6c3545fd3a8d3d74c4f515ac2ee9f02d983251aeaaac715d55c122bbb0141abbeac272011f15520b439bd2db4ec8541a58df9b366921d212ca5f --- src/netaddress.cpp | 10 +++++++--- src/test/net_tests.cpp | 6 +++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/netaddress.cpp b/src/netaddress.cpp index f09fefba7d651..19c45bf81100c 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -544,7 +544,7 @@ static std::string IPv4ToString(Span a) // Return an IPv6 address text representation with zero compression as described in RFC 5952 // ("A Recommendation for IPv6 Address Text Representation"). -static std::string IPv6ToString(Span a) +static std::string IPv6ToString(Span a, uint32_t scope_id) { assert(a.size() == ADDR_IPV6_SIZE); const std::array groups{ @@ -592,6 +592,10 @@ static std::string IPv6ToString(Span a) r += strprintf("%s%x", ((!r.empty() && r.back() != ':') ? ":" : ""), groups[i]); } + if (scope_id != 0) { + r += strprintf("%%%u", scope_id); + } + return r; } @@ -612,14 +616,14 @@ std::string CNetAddr::ToStringIP() const case NET_IPV4: return IPv4ToString(m_addr); case NET_IPV6: { - return IPv6ToString(m_addr); + return IPv6ToString(m_addr, m_scope_id); } case NET_ONION: return OnionToString(m_addr); case NET_I2P: return EncodeBase32(m_addr, false /* don't pad with = */) + ".b32.i2p"; case NET_CJDNS: - return IPv6ToString(m_addr); + return IPv6ToString(m_addr, 0); case NET_INTERNAL: return EncodeBase32(m_addr) + ".internal"; case NET_UNROUTABLE: // m_net is never and should not be set to NET_UNROUTABLE diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 09f2000d0ff18..6f45ee21bd937 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -302,13 +302,17 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic) // IPv6, scoped/link-local. See https://tools.ietf.org/html/rfc4007 // We support non-negative decimal integers (uint32_t) as zone id indices. - // Test with a fairly-high value, e.g. 32, to avoid locally reserved ids. + // Normal link-local scoped address functionality is to append "%" plus the + // zone id, for example, given a link-local address of "fe80::1" and a zone + // id of "32", return the address as "fe80::1%32". const std::string link_local{"fe80::1"}; const std::string scoped_addr{link_local + "%32"}; BOOST_REQUIRE(LookupHost(scoped_addr, addr, false)); BOOST_REQUIRE(addr.IsValid()); BOOST_REQUIRE(addr.IsIPv6()); BOOST_CHECK(!addr.IsBindAny()); + BOOST_CHECK_EQUAL(addr.ToString(), scoped_addr); + // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope. BOOST_REQUIRE(LookupHost(link_local + "%0", addr, false)); BOOST_REQUIRE(addr.IsValid());