Merge bitcoin/bitcoin#27800: streams: Drop confusing DataStream::Serialize method and << operator

5cd0717 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky)

Pull request description:

  DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes.

  Having this inconsistency is not necessary and could be confusing (see bitcoin/bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method.

ACKs for top commit:
  furszy:
    lgtm ACK 5cd0717
  MarcoFalke:
    lgtm ACK 5cd0717 🌙

Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2

Author: fanquake

src/net_processing.cpp

@@ -3418,8 +3418,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         // If the peer is old enough to have the old alert system, send it the final alert.
         if (greatest_common_version <= 70012) {
-            DataStream finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")};
-            m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", finalAlert));
+            const auto finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")};
+            m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", Span{finalAlert}));
         }
 
         // Feeler connections exist only to verify if address is online.

src/streams.h

@@ -293,14 +293,6 @@ class DataStream
         vch.insert(vch.end(), src.begin(), src.end());
     }
 
-    template<typename Stream>
-    void Serialize(Stream& s) const
-    {
-        // Special case: stream << stream concatenates like stream += stream
-        if (!vch.empty())
-            s.write(MakeByteSpan(vch));
-    }
-
     template<typename T>
     DataStream& operator<<(const T& obj)
     {

src/wallet/dump.cpp

@@ -255,11 +255,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
 
             std::vector<unsigned char> k = ParseHex(key);
             std::vector<unsigned char> v = ParseHex(value);
-
-            DataStream ss_key{k};
-            DataStream ss_value{v};
-
-            if (!batch->Write(ss_key, ss_value)) {
+            if (!batch->Write(Span{k}, Span{v})) {
                 error = strprintf(_("Error: Unable to write record to new wallet"));
                 ret = false;
                 break;

src/wallet/wallet.cpp

@@ -3875,9 +3875,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
     bool began = batch->TxnBegin();
     assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
     for (const auto& [key, value] : records) {
-        DataStream ss_key{key};
-        DataStream ss_value{value};
-        if (!batch->Write(ss_key, ss_value)) {
+        if (!batch->Write(MakeUCharSpan(key), MakeUCharSpan(value))) {
             batch->TxnAbort();
             m_database->Close();
             fs::remove(m_database->Filename());