Merge bitcoin/bitcoin#22179: Torv2 removal followups

00b875b addrman: remove invalid addresses when unserializing (Vasil Dimov)
bdb6209 fuzz: reduce possible networks check (Vasil Dimov)
a164cd3 net: simplify CNetAddr::IsRoutable() (Vasil Dimov)

Pull request description:

  * Simplify some code, now that we know `CNetAddr::IsRFC4193()` and `CNetAddr::IsTor()` cannot be `true` at the same time.
  * Drop Tor v2 addresses when loading addrman from `peers.dat` - they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.

ACKs for top commit:
  sipa:
    ACK 00b875b. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions).
  laanwj:
    Code review and lightly tested ACK 00b875b
  jonatack:
    ACK 00b875b reviewed, debug-built with -DEBUG_ADDRMAN rebased to current master, restarted node on mainnet/signet/testnet and verified that on each chain -addrinfo shows no change in address counts (as expected). Added some sanity check asserts, rebuilt/re-ran test. Checked that the new test fails on master with "test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]"
  jarolrod:
    ACK 00b875b

Tree-SHA512: 6ed8e6745134b1b94fffaba28482de909ea39483b46b7f57bda61cdbae7a51251d15cb674de3631772fbeabe153d77a19269f96e62a89102a2d5c01e48f0ba06

Author: laanwj

src/addrman.cpp

@@ -77,6 +77,38 @@ 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 CNetAddr& addr, int* pnId)
 {
     AssertLockHeld(cs);

src/addrman.h

@@ -450,6 +450,8 @@ class CAddrMan
             LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions\n", nLostUnk, nLost);
         }
 
+        RemoveInvalid();
+
         Check();
     }
 
@@ -762,6 +764,9 @@ class CAddrMan
     //! Update an entry's service bits.
     void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
+    //! Remove invalid addresses.
+    void RemoveInvalid() EXCLUSIVE_LOCKS_REQUIRED(cs);
+
     friend class CAddrManTest;
 };
 

src/netaddress.cpp

@@ -489,7 +489,7 @@ bool CNetAddr::IsValid() const
  */
 bool CNetAddr::IsRoutable() const
 {
-    return IsValid() && !(IsRFC1918() || IsRFC2544() || IsRFC3927() || IsRFC4862() || IsRFC6598() || IsRFC5737() || (IsRFC4193() && !IsTor()) || IsRFC4843() || IsRFC7343() || IsLocal() || IsInternal());
+    return IsValid() && !(IsRFC1918() || IsRFC2544() || IsRFC3927() || IsRFC4862() || IsRFC6598() || IsRFC5737() || IsRFC4193() || IsRFC4843() || IsRFC7343() || IsLocal() || IsInternal());
 }
 
 /**

src/test/addrman_tests.cpp

@@ -783,6 +783,46 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
     BOOST_CHECK(bucketAndEntry_asmap1_deser_addr1.second != bucketAndEntry_asmap1_deser_addr2.second);
 }
 
+BOOST_AUTO_TEST_CASE(remove_invalid)
+{
+    // Confirm that invalid addresses are ignored in unserialization.
+
+    CAddrManTest addrman;
+    CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
+
+    const CAddress new1{ResolveService("5.5.5.5"), NODE_NONE};
+    const CAddress new2{ResolveService("6.6.6.6"), NODE_NONE};
+    const CAddress tried1{ResolveService("7.7.7.7"), NODE_NONE};
+    const CAddress tried2{ResolveService("8.8.8.8"), NODE_NONE};
+
+    addrman.Add({new1, tried1, new2, tried2}, CNetAddr{});
+    addrman.Good(tried1);
+    addrman.Good(tried2);
+    BOOST_REQUIRE_EQUAL(addrman.size(), 4);
+
+    stream << addrman;
+
+    const std::string str{stream.str()};
+    size_t pos;
+
+    const char new2_raw[]{6, 6, 6, 6};
+    const uint8_t new2_raw_replacement[]{0, 0, 0, 0}; // 0.0.0.0 is !IsValid()
+    pos = str.find(new2_raw, 0, sizeof(new2_raw));
+    BOOST_REQUIRE(pos != std::string::npos);
+    BOOST_REQUIRE(pos + sizeof(new2_raw_replacement) <= stream.size());
+    memcpy(stream.data() + pos, new2_raw_replacement, sizeof(new2_raw_replacement));
+
+    const char tried2_raw[]{8, 8, 8, 8};
+    const uint8_t tried2_raw_replacement[]{255, 255, 255, 255}; // 255.255.255.255 is !IsValid()
+    pos = str.find(tried2_raw, 0, sizeof(tried2_raw));
+    BOOST_REQUIRE(pos != std::string::npos);
+    BOOST_REQUIRE(pos + sizeof(tried2_raw_replacement) <= stream.size());
+    memcpy(stream.data() + pos, tried2_raw_replacement, sizeof(tried2_raw_replacement));
+
+    addrman.Clear();
+    stream >> addrman;
+    BOOST_CHECK_EQUAL(addrman.size(), 2);
+}
 
 BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision)
 {

src/test/fuzz/netaddress.cpp

@@ -54,7 +54,7 @@ FUZZ_TARGET(netaddress)
     (void)net_addr.IsRFC3927();
     (void)net_addr.IsRFC3964();
     if (net_addr.IsRFC4193()) {
-        assert(net_addr.GetNetwork() == Network::NET_ONION || net_addr.GetNetwork() == Network::NET_INTERNAL || net_addr.GetNetwork() == Network::NET_UNROUTABLE);
+        assert(net_addr.GetNetwork() == Network::NET_INTERNAL || net_addr.GetNetwork() == Network::NET_UNROUTABLE);
     }
     (void)net_addr.IsRFC4380();
     (void)net_addr.IsRFC4843();