Merge dashpay#6043: backport: merge bitcoin#22879, bitcoin#22762, bitcoin#23041, bitcoin#22734, bitcoin#22950, bitcoin#23053, bitcoin#22839, bitcoin#23140, bitcoin#23306, bitcoin#23354, bitcoin#23380 (addrman backports: part 2)

a93fec6 merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging (Kittywhiskers Van Gogh)
d1a4b14 merge bitcoin#23354: Introduce new V4 format addrman (Kittywhiskers Van Gogh)
7a97aab test: restore pre-bitcoin#23306 tests to validate port distinguishment (Kittywhiskers Van Gogh)
1a050d6 merge bitcoin#23306: Make AddrMan support multiple ports per IP (Kittywhiskers Van Gogh)
d56702a merge bitcoin#23140: Make CAddrman::Select_ select buckets, not positions, first (Kittywhiskers Van Gogh)
19b0145 merge bitcoin#22839: improve addrman logging (Kittywhiskers Van Gogh)
3910c68 merge bitcoin#23053: Use public methods in addrman fuzz tests (Kittywhiskers Van Gogh)
b6ec8ab merge bitcoin#22950: Pimpl AddrMan to abstract implementation details (Kittywhiskers Van Gogh)
236cf36 merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize (Kittywhiskers Van Gogh)
2420ac9 merge bitcoin#23041: Add addrman deserialization error tests (Kittywhiskers Van Gogh)
8aefa9b merge bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted (Kittywhiskers Van Gogh)
c9a645f merge bitcoin#22879: Fix format string in deserialize error (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on dashpay#6040.
  * Dash already introduced support for storage of address-port pairs (referred to as "port discrimination") and allowed the usage of non-default ports in P2P with [dash#2168](dashpay#2168).
    * Albeit this was only permitted for networks with `fAllowMultiplePorts` enabled (which at the time was `devnet` and as it stands currently, on every network except `mainnet`).
  * Keeping in line with the above policy (discussion on lifting such restrictions on `mainnet` is outside the scope of this PR), when backporting [bitcoin#23306](bitcoin#23306), changes have been made to retain the effects of `discriminate_ports`.
    * This involves appending placing a `!m_discriminate_ports` condition to behaviour that otherwise would be _removed_ entirely.
    * Additionally, in line with upstream backports, changes have been made that render port distinguishment _enabled_ as the new default in `addrman_tests` (the old default was to keep it _disabled_, to mirror `mainnet` and pre-change upstream behaviour).
      * To ensure distinguishment _disabled_ works as expected, affected pre-backport tests were reintroduced with the `_nondiscriminate` suffix.

  ---

  I would propose at some point to rename the flag to `ignore_port`/`suppress_port` (if not remove it altogether should the `mainnet` restriction be lifted) as discriminate (or distinguish) isn't immediately clear with if address entries will be discriminated/distinguished _using_ ports (i.e. considered) or ports will be discriminated _against_ (i.e. ignored).

  ## Breaking Changes

  It's unclear if these backports result in serialization issues for older versions, as Dash Core technically supported address-port pairs since 0.12 and suppressed it on `mainnet` by setting zero-ing out the port ([source](https://github.com/dashpay/dash/blob/19512988c6e6e8641245bd9c5fab21dd737561f0/src/addrman.cpp#L135-L138)), meaning even with port discrimination _disabled_, the serialization format should remain the same.

  Regardless, following upstream backports, a new version has been introduced (v4) that marks the AddrMan format incompatible with older versions of Dash Core.

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK a93fec6
  PastaPastaPasta:
    utACK dashpay@a93fec6

Tree-SHA512: 49b35af3e4eb660249ce9a65d5a539205d852e9c728da22dc88779f6b3b15c13cf91522896a313bfe2a91889fedf3b6b2cebdea12cc2bbe865ec3b85b6a5dfa8

Author: PastaPastaPasta

src/Makefile.am

@@ -133,6 +133,7 @@ BITCOIN_CORE_H = \
   addressindex.h \
   spentindex.h \
   addrman.h \
+  addrman_impl.h \
   attributes.h \
   banman.h \
   base58.h \

src/Makefile.test.include

@@ -263,7 +263,6 @@ test_fuzz_fuzz_SOURCES = \
   test/fuzz/crypto_hkdf_hmac_sha256_l32.cpp \
   test/fuzz/crypto_poly1305.cpp \
   test/fuzz/cuckoocache.cpp \
-  test/fuzz/data_stream.cpp \
   test/fuzz/decode_tx.cpp \
   test/fuzz/descriptor_parse.cpp \
   test/fuzz/deserialize.cpp \

src/addrdb.cpp

@@ -17,10 +17,17 @@
 #include <univalue.h>
 #include <util/settings.h>
 #include <util/system.h>
+#include <util/translation.h>
 
 #include <cstdint>
 
 namespace {
+
+class DbNotFoundError : public std::exception
+{
+    using std::exception::exception;
+};
+
 template <typename Stream, typename Data>
 bool SerializeDB(Stream& stream, const Data& data)
 {
@@ -78,47 +85,40 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
 }
 
 template <typename Stream, typename Data>
-bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
+void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
 {
-    try {
-        CHashVerifier<Stream> verifier(&stream);
-        // de-serialize file header (network specific magic number) and ..
-        unsigned char pchMsgTmp[4];
-        verifier >> pchMsgTmp;
-        // ... verify the network matches ours
-        if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp)))
-            return error("%s: Invalid network magic number", __func__);
-
-        // de-serialize data
-        verifier >> data;
-
-        // verify checksum
-        if (fCheckSum) {
-            uint256 hashTmp;
-            stream >> hashTmp;
-            if (hashTmp != verifier.GetHash()) {
-                return error("%s: Checksum mismatch, data corrupted", __func__);
-            }
-        }
-    }
-    catch (const std::exception& e) {
-        return error("%s: Deserialize or I/O error - %s", __func__, e.what());
+    CHashVerifier<Stream> verifier(&stream);
+    // de-serialize file header (network specific magic number) and ..
+    unsigned char pchMsgTmp[4];
+    verifier >> pchMsgTmp;
+    // ... verify the network matches ours
+    if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) {
+        throw std::runtime_error{"Invalid network magic number"};
     }
 
-    return true;
+    // de-serialize data
+    verifier >> data;
+
+    // verify checksum
+    if (fCheckSum) {
+        uint256 hashTmp;
+        stream >> hashTmp;
+        if (hashTmp != verifier.GetHash()) {
+            throw std::runtime_error{"Checksum mismatch, data corrupted"};
+        }
+    }
 }
 
 template <typename Data>
-bool DeserializeFileDB(const fs::path& path, Data& data, int version)
+void DeserializeFileDB(const fs::path& path, Data& data, int version)
 {
     // open input file, and associate with CAutoFile
     FILE* file = fsbridge::fopen(path, "rb");
     CAutoFile filein(file, SER_DISK, version);
     if (filein.IsNull()) {
-        LogPrintf("Missing or invalid file %s\n", path.string());
-        return false;
+        throw DbNotFoundError{};
     }
-    return DeserializeDB(filein, data);
+    DeserializeDB(filein, data);
 }
 } // namespace
 
@@ -171,21 +171,38 @@ bool CBanDB::Read(banmap_t& banSet)
     return true;
 }
 
-bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr)
+bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr)
 {
     const auto pathAddr = GetDataDir() / "peers.dat";
     return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
 }
 
-bool ReadPeerAddresses(const ArgsManager& args, CAddrMan& addr)
+void ReadFromStream(AddrMan& addr, CDataStream& ssPeers)
 {
-    const auto pathAddr = GetDataDir() / "peers.dat";
-    return DeserializeFileDB(pathAddr, addr, CLIENT_VERSION);
+    DeserializeDB(ssPeers, addr, false);
 }
 
-bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers)
+std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman)
 {
-    return DeserializeDB(ssPeers, addr, false);
+    auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
+    addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+
+    int64_t nStart = GetTimeMillis();
+    const auto path_addr{GetDataDir() / "peers.dat"};
+    try {
+        DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
+        LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman->size(), GetTimeMillis() - nStart);
+    } catch (const DbNotFoundError&) {
+        // Addrman can be in an inconsistent state after failure, reset it
+        addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+        LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr);
+        DumpPeerAddresses(args, *addrman);
+    } catch (const std::exception& e) {
+        addrman = nullptr;
+        return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),
+                         e.what(), PACKAGE_BUGREPORT, path_addr);
+    }
+    return std::nullopt;
 }
 
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
@@ -197,9 +214,10 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& a
 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 {
     std::vector<CAddress> anchors;
-    if (DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT)) {
+    try {
+        DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
         LogPrintf("Loaded %i addresses from %s\n", anchors.size(), anchors_db_path.filename());
-    } else {
+    } catch (const std::exception&) {
         anchors.clear();
     }
 

src/addrdb.h

@@ -10,17 +10,18 @@
 #include <net_types.h> // For banmap_t
 #include <univalue.h>
 
+#include <optional>
 #include <vector>
 
 class ArgsManager;
-class CAddrMan;
+class AddrMan;
 class CAddress;
 class CDataStream;
+struct bilingual_str;
 
-bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr);
-bool ReadPeerAddresses(const ArgsManager& args, CAddrMan& addr);
+bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
 /** Only used by tests. */
-bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers);
+void ReadFromStream(AddrMan& addr, CDataStream& ssPeers);
 
 /** Access to the banlist database (banlist.json) */
 class CBanDB
@@ -46,6 +47,9 @@ class CBanDB
     bool Read(banmap_t& banSet);
 };
 
+/** Returns an error string on failure */
+std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman);
+
 /**
  * Dump the anchor IP address database (anchors.dat)
  *