Use serialization parameters for CAddress serialization

This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.

Co-authored-by: Pieter Wuille <[email protected]>
Co-authored-by: Anthony Towns <[email protected]>

Author: 3 people

src/addrdb.cpp

@@ -47,16 +47,16 @@ bool SerializeDB(Stream& stream, const Data& data)
 }
 
 template <typename Data>
-bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data& data, int version)
+bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data& data)
 {
     // Generate random temporary filename
     const uint16_t randv{GetRand<uint16_t>()};
     std::string tmpfn = strprintf("%s.%04x", prefix, randv);
 
-    // open temp output file, and associate with CAutoFile
+    // open temp output file
     fs::path pathTmp = gArgs.GetDataDirNet() / fs::u8path(tmpfn);
     FILE *file = fsbridge::fopen(pathTmp, "wb");
-    CAutoFile fileout(file, SER_DISK, version);
+    AutoFile fileout{file};
     if (fileout.IsNull()) {
         fileout.fclose();
         remove(pathTmp);
@@ -86,9 +86,9 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
 }
 
 template <typename Stream, typename Data>
-void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
+void DeserializeDB(Stream& stream, Data&& data, bool fCheckSum = true)
 {
-    CHashVerifier<Stream> verifier(&stream);
+    HashVerifier verifier{stream};
     // de-serialize file header (network specific magic number) and ..
     unsigned char pchMsgTmp[4];
     verifier >> pchMsgTmp;
@@ -111,11 +111,10 @@ void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
 }
 
 template <typename Data>
-void DeserializeFileDB(const fs::path& path, Data& data, int version)
+void DeserializeFileDB(const fs::path& path, Data&& data)
 {
-    // open input file, and associate with CAutoFile
     FILE* file = fsbridge::fopen(path, "rb");
-    CAutoFile filein(file, SER_DISK, version);
+    AutoFile filein{file};
     if (filein.IsNull()) {
         throw DbNotFoundError{};
     }
@@ -175,10 +174,10 @@ bool CBanDB::Read(banmap_t& banSet)
 bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr)
 {
     const auto pathAddr = args.GetDataDirNet() / "peers.dat";
-    return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
+    return SerializeFileDB("peers", pathAddr, addr);
 }
 
-void ReadFromStream(AddrMan& addr, CDataStream& ssPeers)
+void ReadFromStream(AddrMan& addr, DataStream& ssPeers)
 {
     DeserializeDB(ssPeers, addr, false);
 }
@@ -191,7 +190,7 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
     const auto start{SteadyClock::now()};
     const auto path_addr{args.GetDataDirNet() / "peers.dat"};
     try {
-        DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
+        DeserializeFileDB(path_addr, *addrman);
         LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman->Size(), Ticks<std::chrono::milliseconds>(SteadyClock::now() - start));
     } catch (const DbNotFoundError&) {
         // Addrman can be in an inconsistent state after failure, reset it
@@ -217,14 +216,14 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
 {
     LOG_TIME_SECONDS(strprintf("Flush %d outbound block-relay-only peer addresses to anchors.dat", anchors.size()));
-    SerializeFileDB("anchors", anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
+    SerializeFileDB("anchors", anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
 }
 
 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 {
     std::vector<CAddress> anchors;
     try {
-        DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
+        DeserializeFileDB(anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
         LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename())));
     } catch (const std::exception&) {
         anchors.clear();

src/addrdb.h

@@ -16,12 +16,13 @@
 class ArgsManager;
 class AddrMan;
 class CAddress;
-class CDataStream;
+class DataStream;
 class NetGroupManager;
 
-bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
 /** Only used by tests. */
-void ReadFromStream(AddrMan& addr, CDataStream& ssPeers);
+void ReadFromStream(AddrMan& addr, DataStream& ssPeers);
+
+bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
 
 /** Access to the banlist database (banlist.json) */
 class CBanDB

src/addrman.cpp

@@ -171,8 +171,7 @@ void AddrManImpl::Serialize(Stream& s_) const
      */
 
     // Always serialize in the latest version (FILE_FORMAT).
-
-    OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
+    ParamsStream s{CAddress::V2_DISK, s_};
 
     s << static_cast<uint8_t>(FILE_FORMAT);
 
@@ -236,14 +235,8 @@ void AddrManImpl::Unserialize(Stream& s_)
     Format format;
     s_ >> Using<CustomUintFormatter<1>>(format);
 
-    int stream_version = s_.GetVersion();
-    if (format >= Format::V3_BIP155) {
-        // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
-        // unserialize methods know that an address in addrv2 format is coming.
-        stream_version |= ADDRV2_FORMAT;
-    }
-
-    OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
+    const auto ser_params = (format >= Format::V3_BIP155 ? CAddress::V2_DISK : CAddress::V1_DISK);
+    ParamsStream s{ser_params, s_};
 
     uint8_t compat;
     s >> compat;
@@ -1249,12 +1242,12 @@ void AddrMan::Unserialize(Stream& s_)
 }
 
 // explicit instantiation
-template void AddrMan::Serialize(HashedSourceWriter<CAutoFile>& s) const;
-template void AddrMan::Serialize(CDataStream& s) const;
-template void AddrMan::Unserialize(CAutoFile& s);
-template void AddrMan::Unserialize(CHashVerifier<CAutoFile>& s);
-template void AddrMan::Unserialize(CDataStream& s);
-template void AddrMan::Unserialize(CHashVerifier<CDataStream>& s);
+template void AddrMan::Serialize(HashedSourceWriter<AutoFile>&) const;
+template void AddrMan::Serialize(DataStream&) const;
+template void AddrMan::Unserialize(AutoFile&);
+template void AddrMan::Unserialize(HashVerifier<AutoFile>&);
+template void AddrMan::Unserialize(DataStream&);
+template void AddrMan::Unserialize(HashVerifier<DataStream>&);
 
 size_t AddrMan::Size(std::optional<Network> net, std::optional<bool> in_new) const
 {

src/hash.h

@@ -234,18 +234,18 @@ class CHashVerifier : public CHashWriter
 
 /** Writes data to an underlying source stream, while hashing the written data. */
 template <typename Source>
-class HashedSourceWriter : public CHashWriter
+class HashedSourceWriter : public HashWriter
 {
 private:
     Source& m_source;
 
 public:
-    explicit HashedSourceWriter(Source& source LIFETIMEBOUND) : CHashWriter{source.GetType(), source.GetVersion()}, m_source{source} {}
+    explicit HashedSourceWriter(Source& source LIFETIMEBOUND) : HashWriter{}, m_source{source} {}
 
     void write(Span<const std::byte> src)
     {
         m_source.write(src);
-        CHashWriter::write(src);
+        HashWriter::write(src);
     }
 
     template <typename T>

src/net.cpp

@@ -202,7 +202,8 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
     const auto one_week{7 * 24h};
     std::vector<CAddress> vSeedsOut;
     FastRandomContext rng;
-    CDataStream s(vSeedsIn, SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT);
+    DataStream underlying_stream{vSeedsIn};
+    ParamsStream s{CAddress::V2_NETWORK, underlying_stream};
     while (!s.eof()) {
         CService endpoint;
         s >> endpoint;

src/net_processing.cpp

@@ -1415,8 +1415,8 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
 
     const bool tx_relay{!RejectIncomingTxs(pnode)};
     m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
-            your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
-            my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
+            your_services, WithParams(CNetAddr::V1, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
+            my_services, WithParams(CNetAddr::V1, CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
             nonce, strSubVersion, nNodeStartingHeight, tx_relay));
 
     if (fLogIPs) {
@@ -3281,7 +3281,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             nTime = 0;
         }
         vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer
-        vRecv >> addrMe;
+        vRecv >> WithParams(CNetAddr::V1, addrMe);
         if (!pfrom.IsInboundConn())
         {
             m_addrman.SetServices(pfrom.addr, nServices);
@@ -3660,17 +3660,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
     }
 
     if (msg_type == NetMsgType::ADDR || msg_type == NetMsgType::ADDRV2) {
-        int stream_version = vRecv.GetVersion();
-        if (msg_type == NetMsgType::ADDRV2) {
-            // Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
+        const auto ser_params{
+            msg_type == NetMsgType::ADDRV2 ?
+            // Set V2 param so that the CNetAddr and CAddress
             // unserialize methods know that an address in v2 format is coming.
-            stream_version |= ADDRV2_FORMAT;
-        }
+            CAddress::V2_NETWORK :
+            CAddress::V1_NETWORK,
+        };
 
-        OverrideStream<CDataStream> s(&vRecv, vRecv.GetType(), stream_version);
         std::vector<CAddress> vAddr;
 
-        s >> vAddr;
+        vRecv >> WithParams(ser_params, vAddr);
 
         if (!SetupAddressRelay(pfrom, *peer)) {
             LogPrint(BCLog::NET, "ignoring %s message from %s peer=%d\n", msg_type, pfrom.ConnectionTypeAsString(), pfrom.GetId());
@@ -5272,15 +5272,15 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
     if (peer.m_addrs_to_send.empty()) return;
 
     const char* msg_type;
-    int make_flags;
+    CNetAddr::Encoding ser_enc;
     if (peer.m_wants_addrv2) {
         msg_type = NetMsgType::ADDRV2;
-        make_flags = ADDRV2_FORMAT;
+        ser_enc = CNetAddr::Encoding::V2;
     } else {
         msg_type = NetMsgType::ADDR;
-        make_flags = 0;
+        ser_enc = CNetAddr::Encoding::V1;
     }
-    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, peer.m_addrs_to_send));
+    m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
     peer.m_addrs_to_send.clear();
 
     // we only send the big addr message once

src/netaddress.h

@@ -24,14 +24,6 @@
 #include <string>
 #include <vector>
 
-/**
- * A flag that is ORed into the protocol version to designate that addresses
- * should be serialized in (unserialized from) v2 format (BIP155).
- * Make sure that this does not collide with any of the values in `version.h`
- * or with `SERIALIZE_TRANSACTION_NO_WITNESS`.
- */
-static constexpr int ADDRV2_FORMAT = 0x20000000;
-
 /**
  * A network type.
  * @note An address may belong to more than one network, for example `10.0.0.1`
@@ -220,13 +212,23 @@ class CNetAddr
         return IsIPv4() || IsIPv6() || IsTor() || IsI2P() || IsCJDNS();
     }
 
+    enum class Encoding {
+        V1,
+        V2, //!< BIP155 encoding
+    };
+    struct SerParams {
+        const Encoding enc;
+    };
+    static constexpr SerParams V1{Encoding::V1};
+    static constexpr SerParams V2{Encoding::V2};
+
     /**
      * Serialize to a stream.
      */
     template <typename Stream>
     void Serialize(Stream& s) const
     {
-        if (s.GetVersion() & ADDRV2_FORMAT) {
+        if (s.GetParams().enc == Encoding::V2) {
             SerializeV2Stream(s);
         } else {
             SerializeV1Stream(s);
@@ -239,7 +241,7 @@ class CNetAddr
     template <typename Stream>
     void Unserialize(Stream& s)
     {
-        if (s.GetVersion() & ADDRV2_FORMAT) {
+        if (s.GetParams().enc == Encoding::V2) {
             UnserializeV2Stream(s);
         } else {
             UnserializeV1Stream(s);

src/protocol.h

@@ -390,35 +390,43 @@ class CAddress : public CService
     CAddress(CService ipIn, ServiceFlags nServicesIn) : CService{ipIn}, nServices{nServicesIn} {};
     CAddress(CService ipIn, ServiceFlags nServicesIn, NodeSeconds time) : CService{ipIn}, nTime{time}, nServices{nServicesIn} {};
 
-    SERIALIZE_METHODS(CAddress, obj)
+    enum class Format {
+        Disk,
+        Network,
+    };
+    struct SerParams : CNetAddr::SerParams {
+        const Format fmt;
+    };
+    static constexpr SerParams V1_NETWORK{{CNetAddr::Encoding::V1}, Format::Network};
+    static constexpr SerParams V2_NETWORK{{CNetAddr::Encoding::V2}, Format::Network};
+    static constexpr SerParams V1_DISK{{CNetAddr::Encoding::V1}, Format::Disk};
+    static constexpr SerParams V2_DISK{{CNetAddr::Encoding::V2}, Format::Disk};
+
+    SERIALIZE_METHODS_PARAMS(CAddress, obj, SerParams, params)
     {
-        // CAddress has a distinct network serialization and a disk serialization, but it should never
-        // be hashed (except through CHashWriter in addrdb.cpp, which sets SER_DISK), and it's
-        // ambiguous what that would mean. Make sure no code relying on that is introduced:
-        assert(!(s.GetType() & SER_GETHASH));
         bool use_v2;
-        if (s.GetType() & SER_DISK) {
+        if (params.fmt == Format::Disk) {
             // In the disk serialization format, the encoding (v1 or v2) is determined by a flag version
             // that's part of the serialization itself. ADDRV2_FORMAT in the stream version only determines
             // whether V2 is chosen/permitted at all.
             uint32_t stored_format_version = DISK_VERSION_INIT;
-            if (s.GetVersion() & ADDRV2_FORMAT) stored_format_version |= DISK_VERSION_ADDRV2;
+            if (params.enc == Encoding::V2) stored_format_version |= DISK_VERSION_ADDRV2;
             READWRITE(stored_format_version);
             stored_format_version &= ~DISK_VERSION_IGNORE_MASK; // ignore low bits
             if (stored_format_version == 0) {
                 use_v2 = false;
-            } else if (stored_format_version == DISK_VERSION_ADDRV2 && (s.GetVersion() & ADDRV2_FORMAT)) {
-                // Only support v2 deserialization if ADDRV2_FORMAT is set.
+            } else if (stored_format_version == DISK_VERSION_ADDRV2 && params.enc == Encoding::V2) {
+                // Only support v2 deserialization if V2 is set.
                 use_v2 = true;
             } else {
                 throw std::ios_base::failure("Unsupported CAddress disk format version");
             }
         } else {
+            assert(params.fmt == Format::Network);
             // In the network serialization format, the encoding (v1 or v2) is determined directly by
-            // the value of ADDRV2_FORMAT in the stream version, as no explicitly encoded version
+            // the value of enc in the stream params, as no explicitly encoded version
             // exists in the stream.
-            assert(s.GetType() & SER_NETWORK);
-            use_v2 = s.GetVersion() & ADDRV2_FORMAT;
+            use_v2 = params.enc == Encoding::V2;
         }
 
         READWRITE(Using<LossyChronoFormatter<uint32_t>>(obj.nTime));
@@ -432,8 +440,8 @@ class CAddress : public CService
             READWRITE(Using<CustomUintFormatter<8>>(obj.nServices));
         }
         // Invoke V1/V2 serializer for CService parent object.
-        OverrideStream<Stream> os(&s, s.GetType(), use_v2 ? ADDRV2_FORMAT : 0);
-        SerReadWriteMany(os, ser_action, AsBase<CService>(obj));
+        const auto ser_params{use_v2 ? CNetAddr::V2 : CNetAddr::V1};
+        READWRITE(WithParams(ser_params, AsBase<CService>(obj)));
     }
 
     //! Always included in serialization. The behavior is unspecified if the value is not representable as uint32_t.