Merge #20284: addrman: ensure old versions don't parse peers.dat

38ada89 addrman: ensure old versions don't parse peers.dat (Vasil Dimov)

Pull request description:

  Even though the format of `peers.dat` was changed in a backwards
  incompatible way, it is not guaranteed that old versions will fail to
  parse it. There is a chance that old versions parse its contents as
  garbage and use it.

  Old versions expect the "key size" field to be 32 and fail the parsing
  if it is not. Thus, we put something other than 32 in it. This will make
  versions between 0.11.0 and 0.20.1 deterministically fail on the new
  format. Versions prior to bitcoin/bitcoin#5941
  will still parse it as garbage.

  Also, introduce a way to increment the `peers.dat` format in a way that
  does not necessary make older versions refuse to read it.

ACKs for top commit:
  jnewbery:
    ACK 38ada89
  laanwj:
    Code review ACK 38ada89
  MarcoFalke:
    re-ACK 38ada89 🥐

Tree-SHA512: 550bd660c5019dba0f9c334aca8a11c4a0463cfddf11efe7a4a5585ffb05549c82b95066fba5d073ae37893e0eccc158a7ffea9b33ea031d9be4a39e44f6face

Author: laanwj

doc/release-notes.md

@@ -65,8 +65,8 @@ format of this file has been changed in a backwards-incompatible way in order to
 accommodate the storage of Tor v3 and other BIP155 addresses. This means that if
 the file is modified by 0.21.0 or newer then older versions will not be able to
 read it. Those old versions, in the event of a downgrade, will log an error
-message that deserialization has failed and will continue normal operation
-as if the file was missing, creating a new empty one. (#19954)
+message "Incorrect keysize in addrman deserialization" and will continue normal
+operation as if the file was missing, creating a new empty one. (#19954)
 
 Notable changes
 ===============

src/addrman.h

@@ -7,6 +7,7 @@
 #define BITCOIN_ADDRMAN_H
 
 #include <clientversion.h>
+#include <config/bitcoin-config.h>
 #include <netaddress.h>
 #include <protocol.h>
 #include <random.h>
@@ -176,6 +177,28 @@ friend class CAddrManTest;
     mutable RecursiveMutex cs;
 
 private:
+    //! Serialization versions.
+    enum Format : uint8_t {
+        V0_HISTORICAL = 0,    //!< historic format, before commit e6b343d88
+        V1_DETERMINISTIC = 1, //!< for pre-asmap files
+        V2_ASMAP = 2,         //!< for files including asmap version
+        V3_BIP155 = 3,        //!< same as V2_ASMAP plus addresses are in BIP155 format
+    };
+
+    //! The maximum format this software knows it can unserialize. Also, we always serialize
+    //! in this format.
+    //! The format (first byte in the serialized stream) can be higher than this and
+    //! still this software may be able to unserialize the file - if the second byte
+    //! (see `lowest_compatible` in `Unserialize()`) is less or equal to this.
+    static constexpr Format FILE_FORMAT = Format::V3_BIP155;
+
+    //! The initial value of a field that is incremented every time an incompatible format
+    //! change is made (such that old software versions would not be able to parse and
+    //! understand the new file format). This is 32 because we overtook the "key size"
+    //! field which was 32 historically.
+    //! @note Don't increment this. Increment `lowest_compatible` in `Serialize()` instead.
+    static constexpr uint8_t INCOMPATIBILITY_BASE = 32;
+
     //! last used nId
     int nIdCount GUARDED_BY(cs);
 
@@ -265,14 +288,6 @@ friend class CAddrManTest;
     void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
 public:
-    //! Serialization versions.
-    enum class Format : uint8_t {
-        V0_HISTORICAL = 0,    //!< historic format, before commit e6b343d88
-        V1_DETERMINISTIC = 1, //!< for pre-asmap files
-        V2_ASMAP = 2,         //!< for files including asmap version
-        V3_BIP155 = 3,        //!< same as V2_ASMAP plus addresses are in BIP155 format
-    };
-
     // Compressed IP->ASN mapping, loaded from a file when a node starts.
     // Should be always empty if no file was provided.
     // This mapping is then used for bucketing nodes in Addrman.
@@ -295,8 +310,18 @@ friend class CAddrManTest;
 
     /**
      * Serialized format.
-     * * version byte (@see `Format`)
-     * * 0x20 + nKey (serialized as if it were a vector, for backward compatibility)
+     * * format version byte (@see `Format`)
+     * * lowest compatible format version byte. This is used to help old software decide
+     *   whether to parse the file. For example:
+     *   * Bitcoin Core version N knows how to parse up to format=3. If a new format=4 is
+     *     introduced in version N+1 that is compatible with format=3 and it is known that
+     *     version N will be able to parse it, then version N+1 will write
+     *     (format=4, lowest_compatible=3) in the first two bytes of the file, and so
+     *     version N will still try to parse it.
+     *   * Bitcoin Core version N+2 introduces a new incompatible format=5. It will write
+     *     (format=5, lowest_compatible=5) and so any versions that do not know how to parse
+     *     format=5 will not try to read the file.
+     * * nKey
      * * nNew
      * * nTried
      * * number of "new" buckets XOR 2**30
@@ -327,12 +352,17 @@ friend class CAddrManTest;
     {
         LOCK(cs);
 
-        // Always serialize in the latest version (currently Format::V3_BIP155).
+        // Always serialize in the latest version (FILE_FORMAT).
 
         OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
 
-        s << static_cast<uint8_t>(Format::V3_BIP155);
-        s << ((unsigned char)32);
+        s << static_cast<uint8_t>(FILE_FORMAT);
+
+        // Increment `lowest_compatible` iff a newly introduced format is incompatible with
+        // the previous one.
+        static constexpr uint8_t lowest_compatible = Format::V3_BIP155;
+        s << static_cast<uint8_t>(INCOMPATIBILITY_BASE + lowest_compatible);
+
         s << nKey;
         s << nNew;
         s << nTried;
@@ -392,15 +422,6 @@ friend class CAddrManTest;
         Format format;
         s_ >> Using<CustomUintFormatter<1>>(format);
 
-        static constexpr Format maximum_supported_format = Format::V3_BIP155;
-        if (format > maximum_supported_format) {
-            throw std::ios_base::failure(strprintf(
-                "Unsupported format of addrman database: %u. Maximum supported is %u. "
-                "Continuing operation without using the saved list of peers.",
-                static_cast<uint8_t>(format),
-                static_cast<uint8_t>(maximum_supported_format)));
-        }
-
         int stream_version = s_.GetVersion();
         if (format >= Format::V3_BIP155) {
             // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
@@ -410,9 +431,16 @@ friend class CAddrManTest;
 
         OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
 
-        unsigned char nKeySize;
-        s >> nKeySize;
-        if (nKeySize != 32) throw std::ios_base::failure("Incorrect keysize in addrman deserialization");
+        uint8_t compat;
+        s >> compat;
+        const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
+        if (lowest_compatible > FILE_FORMAT) {
+            throw std::ios_base::failure(strprintf(
+                "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
+                "but the maximum supported by this version of %s is %u.",
+                format, lowest_compatible, PACKAGE_NAME, static_cast<uint8_t>(FILE_FORMAT)));
+        }
+
         s >> nKey;
         s >> nNew;
         s >> nTried;