Merge #19032: Serialization improvements: final step

71f016c Remove old serialization primitives (Pieter Wuille)
92beff1 Convert LimitedString to formatter (Pieter Wuille)
ef17c03 Convert wallet to new serialization (Pieter Wuille)
65c589e Convert Qt to new serialization (Pieter Wuille)

Pull request description:

  This is the final step 🥳 of the serialization improvements extracted from #10785.

  It converts the LimitedString wrapper to a new-style formatter, and updates the wallet and Qt code to use the new serialization framework. Finally all remaining old primitives are removed.

ACKs for top commit:
  jonatack:
    ACK 71f016c reviewed diff, builds/tests/re-fuzzed.
  laanwj:
    Code review ACK 71f016c

Tree-SHA512: d952194bc73259f6510bd4ab1348a1febbbf9862af30f905991812fb0e1f23f15948cdb3fc662be54d648e8f6d95b11060055d2e7a8c2cb5bf008224870b1ea1

Author: laanwj

src/qt/recentrequeststablemodel.h

@@ -24,19 +24,11 @@ class RecentRequestEntry
     QDateTime date;
     SendCoinsRecipient recipient;
 
-    ADD_SERIALIZE_METHODS;
-
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        unsigned int nDate = date.toTime_t();
-
-        READWRITE(this->nVersion);
-        READWRITE(id);
-        READWRITE(nDate);
-        READWRITE(recipient);
-
-        if (ser_action.ForRead())
-            date = QDateTime::fromTime_t(nDate);
+    SERIALIZE_METHODS(RecentRequestEntry, obj) {
+        unsigned int date_timet;
+        SER_WRITE(obj, date_timet = obj.date.toTime_t());
+        READWRITE(obj.nVersion, obj.id, date_timet, obj.recipient);
+        SER_READ(obj, obj.date = QDateTime::fromTime_t(date_timet));
     }
 };
 

src/qt/sendcoinsrecipient.h

@@ -44,30 +44,21 @@ class SendCoinsRecipient
     static const int CURRENT_VERSION = 1;
     int nVersion;
 
-    ADD_SERIALIZE_METHODS;
+    SERIALIZE_METHODS(SendCoinsRecipient, obj)
+    {
+        std::string address_str, label_str, message_str, auth_merchant_str;
 
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        std::string sAddress = address.toStdString();
-        std::string sLabel = label.toStdString();
-        std::string sMessage = message.toStdString();
-        std::string sAuthenticatedMerchant = authenticatedMerchant.toStdString();
+        SER_WRITE(obj, address_str = obj.address.toStdString());
+        SER_WRITE(obj, label_str = obj.label.toStdString());
+        SER_WRITE(obj, message_str = obj.message.toStdString());
+        SER_WRITE(obj, auth_merchant_str = obj.authenticatedMerchant.toStdString());
 
-        READWRITE(this->nVersion);
-        READWRITE(sAddress);
-        READWRITE(sLabel);
-        READWRITE(amount);
-        READWRITE(sMessage);
-        READWRITE(sPaymentRequest);
-        READWRITE(sAuthenticatedMerchant);
+        READWRITE(obj.nVersion, address_str, label_str, obj.amount, message_str, obj.sPaymentRequest, auth_merchant_str);
 
-        if (ser_action.ForRead())
-        {
-            address = QString::fromStdString(sAddress);
-            label = QString::fromStdString(sLabel);
-            message = QString::fromStdString(sMessage);
-            authenticatedMerchant = QString::fromStdString(sAuthenticatedMerchant);
-        }
+        SER_READ(obj, obj.address = QString::fromStdString(address_str));
+        SER_READ(obj, obj.label = QString::fromStdString(label_str));
+        SER_READ(obj, obj.message = QString::fromStdString(message_str));
+        SER_READ(obj, obj.authenticatedMerchant = QString::fromStdString(auth_merchant_str));
     }
 };
 

src/serialize.h

@@ -43,26 +43,6 @@ static const unsigned int MAX_VECTOR_ALLOCATE = 5000000;
 struct deserialize_type {};
 constexpr deserialize_type deserialize {};
 
-/**
- * Used to bypass the rule against non-const reference to temporary
- * where it makes sense with wrappers.
- */
-template<typename T>
-inline T& REF(const T& val)
-{
-    return const_cast<T&>(val);
-}
-
-/**
- * Used to acquire a non-const pointer "this" to generate bodies
- * of const serialization operations from a template
- */
-template<typename T>
-inline T* NCONST_PTR(const T* val)
-{
-    return const_cast<T*>(val);
-}
-
 //! Safely convert odd char pointer types to standard ones.
 inline char* CharCast(char* c) { return c; }
 inline char* CharCast(unsigned char* c) { return (char*)c; }
@@ -193,22 +173,6 @@ template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
 #define SER_READ(obj, code) ::SerRead(s, ser_action, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
 #define SER_WRITE(obj, code) ::SerWrite(s, ser_action, obj, [&](Stream& s, const Type& obj) { code; })
 
-/**
- * Implement three methods for serializable objects. These are actually wrappers over
- * "SerializationOp" template, which implements the body of each class' serialization
- * code. Adding "ADD_SERIALIZE_METHODS" in the body of the class causes these wrappers to be
- * added as members.
- */
-#define ADD_SERIALIZE_METHODS                                         \
-    template<typename Stream>                                         \
-    void Serialize(Stream& s) const {                                 \
-        NCONST_PTR(this)->SerializationOp(s, CSerActionSerialize());  \
-    }                                                                 \
-    template<typename Stream>                                         \
-    void Unserialize(Stream& s) {                                     \
-        SerializationOp(s, CSerActionUnserialize());                  \
-    }
-
 /**
  * Implement the Ser and Unser methods needed for implementing a formatter (see Using below).
  *
@@ -503,7 +467,7 @@ static inline Wrapper<Formatter, T&> Using(T&& t) { return Wrapper<Formatter, T&
 #define VARINT_MODE(obj, mode) Using<VarIntFormatter<mode>>(obj)
 #define VARINT(obj) Using<VarIntFormatter<VarIntMode::DEFAULT>>(obj)
 #define COMPACTSIZE(obj) Using<CompactSizeFormatter>(obj)
-#define LIMITED_STRING(obj,n) LimitedString< n >(REF(obj))
+#define LIMITED_STRING(obj,n) Using<LimitedStringFormatter<n>>(obj)
 
 /** Serialization wrapper class for integers in VarInt format. */
 template<VarIntMode Mode>
@@ -588,31 +552,23 @@ struct CompactSizeFormatter
 };
 
 template<size_t Limit>
-class LimitedString
+struct LimitedStringFormatter
 {
-protected:
-    std::string& string;
-public:
-    explicit LimitedString(std::string& _string) : string(_string) {}
-
     template<typename Stream>
-    void Unserialize(Stream& s)
+    void Unser(Stream& s, std::string& v)
     {
         size_t size = ReadCompactSize(s);
         if (size > Limit) {
             throw std::ios_base::failure("String length limit exceeded");
         }
-        string.resize(size);
-        if (size != 0)
-            s.read((char*)string.data(), size);
+        v.resize(size);
+        if (size != 0) s.read((char*)v.data(), size);
     }
 
     template<typename Stream>
-    void Serialize(Stream& s) const
+    void Ser(Stream& s, const std::string& v)
     {
-        WriteCompactSize(s, string.size());
-        if (!string.empty())
-            s.write((char*)string.data(), string.size());
+        s << v;
     }
 };
 
@@ -1012,7 +968,7 @@ void Unserialize(Stream& is, std::shared_ptr<const T>& p)
 
 
 /**
- * Support for ADD_SERIALIZE_METHODS and READWRITE macro
+ * Support for SERIALIZE_METHODS and READWRITE macro.
  */
 struct CSerActionSerialize
 {

src/test/fuzz/string.cpp

@@ -93,7 +93,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     {
         CDataStream data_stream{SER_NETWORK, INIT_PROTO_VERSION};
         std::string s;
-        LimitedString<10> limited_string = LIMITED_STRING(s, 10);
+        auto limited_string = LIMITED_STRING(s, 10);
         data_stream << random_string_1;
         try {
             data_stream >> limited_string;
@@ -108,7 +108,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     }
     {
         CDataStream data_stream{SER_NETWORK, INIT_PROTO_VERSION};
-        const LimitedString<10> limited_string = LIMITED_STRING(random_string_1, 10);
+        const auto limited_string = LIMITED_STRING(random_string_1, 10);
         data_stream << limited_string;
         std::string deserialized_string;
         data_stream >> deserialized_string;

src/wallet/crypter.h

@@ -43,15 +43,9 @@ class CMasterKey
     //! such as the various parameters to scrypt
     std::vector<unsigned char> vchOtherDerivationParameters;
 
-    ADD_SERIALIZE_METHODS;
-
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        READWRITE(vchCryptedKey);
-        READWRITE(vchSalt);
-        READWRITE(nDerivationMethod);
-        READWRITE(nDeriveIterations);
-        READWRITE(vchOtherDerivationParameters);
+    SERIALIZE_METHODS(CMasterKey, obj)
+    {
+        READWRITE(obj.vchCryptedKey, obj.vchSalt, obj.nDerivationMethod, obj.nDeriveIterations, obj.vchOtherDerivationParameters);
     }
 
     CMasterKey()

src/wallet/scriptpubkeyman.h

@@ -112,36 +112,37 @@ class CKeyPool
     CKeyPool();
     CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn);
 
-    ADD_SERIALIZE_METHODS;
+    template<typename Stream>
+    void Serialize(Stream& s) const
+    {
+        int nVersion = s.GetVersion();
+        if (!(s.GetType() & SER_GETHASH)) {
+            s << nVersion;
+        }
+        s << nTime << vchPubKey << fInternal << m_pre_split;
+    }
 
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
+    template<typename Stream>
+    void Unserialize(Stream& s)
+    {
         int nVersion = s.GetVersion();
-        if (!(s.GetType() & SER_GETHASH))
-            READWRITE(nVersion);
-        READWRITE(nTime);
-        READWRITE(vchPubKey);
-        if (ser_action.ForRead()) {
-            try {
-                READWRITE(fInternal);
-            }
-            catch (std::ios_base::failure&) {
-                /* flag as external address if we can't read the internal boolean
-                   (this will be the case for any wallet before the HD chain split version) */
-                fInternal = false;
-            }
-            try {
-                READWRITE(m_pre_split);
-            }
-            catch (std::ios_base::failure&) {
-                /* flag as postsplit address if we can't read the m_pre_split boolean
-                   (this will be the case for any wallet that upgrades to HD chain split)*/
-                m_pre_split = false;
-            }
+        if (!(s.GetType() & SER_GETHASH)) {
+            s >> nVersion;
+        }
+        s >> nTime >> vchPubKey;
+        try {
+            s >> fInternal;
+        } catch (std::ios_base::failure&) {
+            /* flag as external address if we can't read the internal boolean
+               (this will be the case for any wallet before the HD chain split version) */
+            fInternal = false;
         }
-        else {
-            READWRITE(fInternal);
-            READWRITE(m_pre_split);
+        try {
+            s >> m_pre_split;
+        } catch (std::ios_base::failure&) {
+            /* flag as postsplit address if we can't read the m_pre_split boolean
+               (this will be the case for any wallet that upgrades to HD chain split) */
+            m_pre_split = false;
         }
     }
 };

src/wallet/walletdb.h

@@ -98,15 +98,13 @@ class CHDChain
     int nVersion;
 
     CHDChain() { SetNull(); }
-    ADD_SERIALIZE_METHODS;
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action)
+
+    SERIALIZE_METHODS(CHDChain, obj)
     {
-        READWRITE(this->nVersion);
-        READWRITE(nExternalChainCounter);
-        READWRITE(seed_id);
-        if (this->nVersion >= VERSION_HD_CHAIN_SPLIT)
-            READWRITE(nInternalChainCounter);
+        READWRITE(obj.nVersion, obj.nExternalChainCounter, obj.seed_id);
+        if (obj.nVersion >= VERSION_HD_CHAIN_SPLIT) {
+            READWRITE(obj.nInternalChainCounter);
+        }
     }
 
     void SetNull()
@@ -147,21 +145,16 @@ class CKeyMetadata
         nCreateTime = nCreateTime_;
     }
 
-    ADD_SERIALIZE_METHODS;
-
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        READWRITE(this->nVersion);
-        READWRITE(nCreateTime);
-        if (this->nVersion >= VERSION_WITH_HDDATA)
-        {
-            READWRITE(hdKeypath);
-            READWRITE(hd_seed_id);
+    SERIALIZE_METHODS(CKeyMetadata, obj)
+    {
+        READWRITE(obj.nVersion, obj.nCreateTime);
+        if (obj.nVersion >= VERSION_WITH_HDDATA) {
+            READWRITE(obj.hdKeypath, obj.hd_seed_id);
         }
-        if (this->nVersion >= VERSION_WITH_KEY_ORIGIN)
+        if (obj.nVersion >= VERSION_WITH_KEY_ORIGIN)
         {
-            READWRITE(key_origin);
-            READWRITE(has_key_origin);
+            READWRITE(obj.key_origin);
+            READWRITE(obj.has_key_origin);
         }
     }
 

src/wallet/walletutil.h

@@ -98,26 +98,22 @@ class WalletDescriptor
     int32_t next_index = 0; // Position of the next item to generate
     DescriptorCache cache;
 
-    ADD_SERIALIZE_METHODS;
-
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        if (ser_action.ForRead()) {
-            std::string desc;
-            std::string error;
-            READWRITE(desc);
-            FlatSigningProvider keys;
-            descriptor = Parse(desc, keys, error, true);
-            if (!descriptor) {
-                throw std::ios_base::failure("Invalid descriptor: " + error);
-            }
-        } else {
-            READWRITE(descriptor->ToString());
+    void DeserializeDescriptor(const std::string& str)
+    {
+        std::string error;
+        FlatSigningProvider keys;
+        descriptor = Parse(str, keys, error, true);
+        if (!descriptor) {
+            throw std::ios_base::failure("Invalid descriptor: " + error);
         }
-        READWRITE(creation_time);
-        READWRITE(next_index);
-        READWRITE(range_start);
-        READWRITE(range_end);
+    }
+
+    SERIALIZE_METHODS(WalletDescriptor, obj)
+    {
+        std::string descriptor_str;
+        SER_WRITE(obj, descriptor_str = obj.descriptor->ToString());
+        READWRITE(descriptor_str, obj.creation_time, obj.next_index, obj.range_start, obj.range_end);
+        SER_READ(obj, obj.DeserializeDescriptor(descriptor_str));
     }
 
     WalletDescriptor() {}