Serialization improvements step 6 (all except wallet/gui) #18317

In commit "Convert everything except wallet/qt to new serialization" (4a185971f495ab89d869a4744d36aa1a8c9aea3f)

Is it safe to drop the Init call here? It seems needed to avoid leaving nTime uninitialized if READWRITE(obj.nTime) is skipped below.

I was experimenting a little bit with how to be able to bring back if (ser_action.ForRead()) ability in the new framework now that the object type can vary and may be const.

One option might be to have SER_READ / SER_WRITE macros that contain code (similar to WITH_LOCK) and only execute when reading or writing:

--- a/src/protocol.h
+++ b/src/protocol.h
@@ -316,6 +316,7 @@ public:
 
     SERIALIZE_METHODS(CAddress, obj)
     {
+        SER_READ(obj, obj.Init());
         int nVersion = s.GetVersion();
         if (s.GetType() & SER_DISK) {
             READWRITE(nVersion);
diff --git a/src/serialize.h b/src/serialize.h
index 43f35f7ebe6..ae6a028ce97 100644
--- a/src/serialize.h
+++ b/src/serialize.h
@@ -179,6 +179,7 @@ template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
 
 #define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__))
 #define READWRITEAS(type, obj) (::SerReadWriteMany(s, ser_action, ReadWriteAsHelper<type>(obj)))
+#define SER_READ(obj, code) ::SerRead(s, ser_action, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
 
 /**
  * Implement three methods for serializable objects. These are actually wrappers over
@@ -1089,6 +1090,17 @@ inline void SerReadWriteMany(Stream& s, CSerActionUnserialize ser_action, Args&&
     ::UnserializeMany(s, args...);
 }
 
+template<typename Stream, typename Type, typename Fn>
+inline void SerRead(Stream& s, CSerActionSerialize ser_action, Type&&, Fn&&)
+{
+}
+
+template<typename Stream, typename Type, typename Fn>
+inline void SerRead(Stream& s, CSerActionUnserialize ser_action, Type&& obj, Fn&& fn)
+{
+    fn(s, std::forward<Type>(obj));
+}
+
 template<typename I>
 inline void WriteVarInt(CSizeComputer &s, I n)
 {

That's great! I've turned this snippet into a separate commit (with you as author; please let me know if you prefer a different approach). It also adds the corresponding SER_WRITE, as it turns out to be useful further.

Going forward, the Init should probably be removed and replaced with a slim and clean C++11 initializer

What do you mean with "C++11 initializer" ?

What do you mean with "C++11 initializer" ?

#19020

Commented there: https://github.com/bitcoin/bitcoin/pull/19020/commits/eac91b03763880417b81037f890dd5a463ce8dde#r427620111

In commit "Add ConvertFormatter" (7c5a0c7c53c7cec8f4f6d074d90c5e8988f61c15)

I think it would be better not to have this class, or maybe expose it less widely:

• It only seems to be used one place currently, and I think CustomUintFormatter could be used there instead.
• It seems easy to confuse with READWRITEAS, and not obvious when you'd use READWRITEAS vs READWRITE with ConvertFormatter
• Haven't thought about it too much, but just based on how this is implemented, there could be cases where using it would do unexpected temporary creation or copying or derived-base truncation.

I've taken your approach of merging it into CustomUintFormatter.

In commit "Convert everything except wallet/qt to new serialization" (4a185971f495ab89d869a4744d36aa1a8c9aea3f)

Last commit is mostly repetitive changes of this form. Maybe a big regex in a scripted diff could be used to do the boilerplate subtitutions in a separate commit, so the more complicated changes here stand out.

I think that would be pretty hard to do, especially the classname argument to SERIALIZE_METHODS is hard to figure out with a regexp. Do you have any concrete ideas?

Clang has tooling to do these kind of refactors. Though, I haven't figured out how to run them in a scripted-diff.

In commit "Convert everything except wallet/qt to new serialization" (4a185971f495ab89d869a4744d36aa1a8c9aea3f)

I suggested dropping ConvertFormatter in another comment for a few reasons. CustomUintFormatter could be used instead here if extended to support enums:

diff --git a/src/protocol.h b/src/protocol.h
index c1a21264705..901471e110d 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -324,7 +324,7 @@ public:
             (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) {
             READWRITE(obj.nTime);
         }
-        READWRITE(Using<ConvertFormatter<uint64_t>>(obj.nServices));
+        READWRITE(Using<CustomUintFormatter<8>>(obj.nServices));
         READWRITEAS(CService, obj);
     }
 
diff --git a/src/serialize.h b/src/serialize.h
index 51d270d7976..718bdea90a6 100644
--- a/src/serialize.h
+++ b/src/serialize.h
@@ -527,14 +527,15 @@ struct CustomUintFormatter
 
     template <typename Stream, typename I> void Unser(Stream& s, I& v)
     {
-        static_assert(std::numeric_limits<I>::max() >= MAX && std::numeric_limits<I>::min() <= 0, "CustomUintFormatter type too small");
+        using U = typename std::conditional<std::is_enum<I>::value, std::underlying_type<I>, std::common_type<I>>::type::type;
+        static_assert(std::numeric_limits<U>::max() >= MAX && std::numeric_limits<U>::min() <= 0, "Assigned type too small");
         uint64_t raw = 0;
         if (BigEndian) {
             s.read(((char*)&raw) + 8 - Bytes, Bytes);
-            v = be64toh(raw);
+            v = static_cast<I>(be64toh(raw));
         } else {
             s.read((char*)&raw, Bytes);
-            v = le64toh(raw);
+            v = static_cast<I>(le64toh(raw));
         }
     }
 };

I've turned this into a separate commit with you as author.

In commit "Convert merkleblock to new serialization" (25bfbbeb63eca1f1c8852a8a08844069d9e06776)

Maybe better to keep constructor below private and just declare SERIALIZE_METHODS before it, assuming there's a valid reason for the constructor to be private.

I think this was a mistake; fixed.

in commit 769ee5fa0011ae658770586442715452a656559d:

Any reason why you remove the comment? "This is only intended to implement serializers that are compatible with existing formats, and its use is not recommended for new data structures."

I suspect it was just lost in refactoring. Added a new commit that adds it back.

in commit f9ee0f37c28f604bc82dab502ce229c66ef5b3b9

Not recommend for big endian or anything that can be serialized with this wrapper? For example, I am wondering if there is a recommended way to serialize enums or should new code rather not use enums instead?

The comment is only intended to apply to big endian serialization. Any suggestions to make that clearer?

If it's confusing, I think it's also fine to just drop the comment. When picking a serialization for a new structure I doubt anyone would intentionally complicate their code by using big endian serialization.

I doubt anyone would intentionally complicate their code by using big endian serialization

Makes sense

In commit "Add comments to CustomUintFormatter" (f9ee0f37c28f604bc82dab502ce229c66ef5b3b9)

re: "not recommended for new data structures", it'd be nice to say what preferred alternatives are. Compact ints, varints, little endian fixed width ints? I keep forgetting all the int types myself..

@ryanofsky Maybe the serialization code just shouldn't contain protocol design advice.