The next step of changes from #10785.
This:
CustomUintFormatter, used in CAddress for service flags.BigEndian into CustomUintFormatter, used in CNetAddr for port numbers.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
319- template <typename Stream, typename Operation>
320- inline void SerializationOp(Stream& s, Operation ser_action)
321+ SERIALIZE_METHODS(CAddress, obj)
322 {
323- if (ser_action.ForRead())
324- Init();
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:
0--- a/src/protocol.h
1+++ b/src/protocol.h
2@@ -316,6 +316,7 @@ public:
3
4 SERIALIZE_METHODS(CAddress, obj)
5 {
6+ SER_READ(obj, obj.Init());
7 int nVersion = s.GetVersion();
8 if (s.GetType() & SER_DISK) {
9 READWRITE(nVersion);
10diff --git a/src/serialize.h b/src/serialize.h
11index 43f35f7ebe6..ae6a028ce97 100644
12--- a/src/serialize.h
13+++ b/src/serialize.h
14@@ -179,6 +179,7 @@ template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
15
16 #define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__))
17 #define READWRITEAS(type, obj) (::SerReadWriteMany(s, ser_action, ReadWriteAsHelper<type>(obj)))
18+#define SER_READ(obj, code) ::SerRead(s, ser_action, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
19
20 /**
21 * Implement three methods for serializable objects. These are actually wrappers over
22@@ -1089,6 +1090,17 @@ inline void SerReadWriteMany(Stream& s, CSerActionUnserialize ser_action, Args&&
23 ::UnserializeMany(s, args...);
24 }
25
26+template<typename Stream, typename Type, typename Fn>
27+inline void SerRead(Stream& s, CSerActionSerialize ser_action, Type&&, Fn&&)
28+{
29+}
30+
31+template<typename Stream, typename Type, typename Fn>
32+inline void SerRead(Stream& s, CSerActionUnserialize ser_action, Type&& obj, Fn&& fn)
33+{
34+ fn(s, std::forward<Type>(obj));
35+}
36+
37 template<typename I>
38 inline void WriteVarInt(CSizeComputer &s, I n)
39 {
What do you mean with “C++11 initializer” ?
539@@ -540,6 +540,26 @@ struct CustomUintFormatter
540 }
541 };
542
543+/** Formatter that (de)serializes a type as another type it's convertible to/from. */
544+template<typename F> struct ConvertFormatter
In commit “Add ConvertFormatter” (7c5a0c7c53c7cec8f4f6d074d90c5e8988f61c15)
I think it would be better not to have this class, or maybe expose it less widely:
READWRITEAS, and not obvious when you’d use READWRITEAS vs READWRITE with ConvertFormatter
CustomUintFormatter.
73- READWRITE(vData);
74- READWRITE(nHashFuncs);
75- READWRITE(nTweak);
76- READWRITE(nFlags);
77- }
78+ SERIALIZE_METHODS(CBloomFilter, obj) { READWRITE(obj.vData, obj.nHashFuncs, obj.nTweak, obj.nFlags); }
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.
SERIALIZE_METHODS is hard to figure out with a regexp. Do you have any concrete ideas?
335- nServices = static_cast<ServiceFlags>(nServicesInt);
336- READWRITEAS(CService, *this);
337+ (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) {
338+ READWRITE(obj.nTime);
339+ }
340+ READWRITE(Using<ConvertFormatter<uint64_t>>(obj.nServices));
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:
0diff --git a/src/protocol.h b/src/protocol.h
1index c1a21264705..901471e110d 100644
2--- a/src/protocol.h
3+++ b/src/protocol.h
4@@ -324,7 +324,7 @@ public:
5 (nVersion >= CADDR_TIME_VERSION && !(s.GetType() & SER_GETHASH))) {
6 READWRITE(obj.nTime);
7 }
8- READWRITE(Using<ConvertFormatter<uint64_t>>(obj.nServices));
9+ READWRITE(Using<CustomUintFormatter<8>>(obj.nServices));
10 READWRITEAS(CService, obj);
11 }
12
13diff --git a/src/serialize.h b/src/serialize.h
14index 51d270d7976..718bdea90a6 100644
15--- a/src/serialize.h
16+++ b/src/serialize.h
17@@ -527,14 +527,15 @@ struct CustomUintFormatter
18
19 template <typename Stream, typename I> void Unser(Stream& s, I& v)
20 {
21- static_assert(std::numeric_limits<I>::max() >= MAX && std::numeric_limits<I>::min() <= 0, "CustomUintFormatter type too small");
22+ using U = typename std::conditional<std::is_enum<I>::value, std::underlying_type<I>, std::common_type<I>>::type::type;
23+ static_assert(std::numeric_limits<U>::max() >= MAX && std::numeric_limits<U>::min() <= 0, "Assigned type too small");
24 uint64_t raw = 0;
25 if (BigEndian) {
26 s.read(((char*)&raw) + 8 - Bytes, Bytes);
27- v = be64toh(raw);
28+ v = static_cast<I>(be64toh(raw));
29 } else {
30 s.read((char*)&raw, Bytes);
31- v = le64toh(raw);
32+ v = static_cast<I>(le64toh(raw));
33 }
34 }
35 };
Reviewed most of this (maybe 80%). Just posting feedback so far. Only maybe-blocking thing is question about CAddress::Init below, everything else feel free to ignore.
The last commit here is especially nice. New serialization code is much cleaner than the old code.
167- inline void SerializationOp(Stream& s, Operation ser_action) {
168- READWRITE(header);
169- READWRITE(txn);
170- }
171-
172-private:
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.
CAddress::Init call can be skipped in #18317 (review).
Extracted by Pieter Wuille from a comment by Russ Yanofsky, see
https://github.com/bitcoin/bitcoin/pull/18317#discussion_r398821936.
Extracted and extended by Pieter Wuille from a comment by Russ
Yanofsky (see
https://github.com/bitcoin/bitcoin/pull/18317#discussion_r398625457).
Cleanup and improvements look very good. Nice collaboration.
ACK 4eb5643e3538863c9d2ff261f49a9a1b248de243
Code review, gcc+clang builds+tests+ran bitcoind at 4eb5643 and rebased on master 65276c7
Strong concept ACK
New code is much easier to read and reason about.
557-/** Serialization wrapper class for big-endian integers.
558- *
559- * Use this wrapper around integer types that are stored in memory in native
560- * byte order, but serialized in big endian notation. This is only intended
561- * to implement serializers that are compatible with existing formats, and
562- * its use is not recommended for new data structures.
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.”
ACK 4eb5643e3538863c9d2ff261f49a9a1b248de243 🐜
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 4eb5643e3538863c9d2ff261f49a9a1b248de243 🐜
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUiapgv5AV3Ayc3O2WW2CHMAP08zdDmwuS4qvzMAKT4kAgIdFrVr+iXUXZIwuwQ0
8M/HnR17FSJ8FlQ5plt0pyKolQv1fjVugdzJUPd9RjdhXZ3SqYof5arr8ddBi+xJv
9qd9XkI8Ur65zVRTOSqWOabWPwGsVxZsztYxwZI8n5yQwWeZ1ANr7O6D/8xEBwggS
10tYpFxtTaQ2Wp+REbpgWMYf9t9U2KUW3XFbac2xUf9Ff3hVM4uC+siOGKTf6omaEK
11gpO2HT0RzKua6FRgLoTjdczhvPh1RbgLwnMSS7LazgM1rnwTS/fRSsFnr1D+CHqi
12fShwPe9+7lOYoC+AzuK4z1RVOfVcLeUFzKRaGQNLXZQJ3LlzIdwg/nhd1xO+qov2
132BF9pbxei6fCfGKz5KCDbJJI9KPcL0K2gfrpldGdF1tZQ7/CbvusEPCIoUe5K6g3
14oJ1ttbViqpIGNFRhG0h6JWmu7mOoQm1EuoAjwDz05wJ+H2IthrbRSCYP6BvV00A2
15qx5QVuZ8
16=A3ac
17-----END PGP SIGNATURE-----
Timestamp of file with hash dab97ed81bacef9fec86c29c2502d3a9220b4eae2601a1671bb80dc4cce9dd01 -
525+ * It permits specifying the serialized size (1 to 8 bytes) and endianness.
526+ *
527+ * Use the big endian mode for values that are stored in memory in native
528+ * byte order, but serialized in big endian notation. This is only intended
529+ * to implement serializers that are compatible with existing formats, and
530+ * its use is not recommended for new data structures.
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..
re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUh+HQv9EdvBDfmaDb6c9KM50spORaYYJlbuXGYi+pomY/sL9zDHQw5DlnO6CsKS
8CZ98PFhMSYoTY0jHxLDcQNoJdKnpuS027pJ82pE0lWLzawtSQrCjLk04UkkZwf1h
9BvSTfmLOq+071pwCFaSo60n07nIlAlSq7gIGGLq/9UfCM0eAk+ndOWzIxS+lglkR
10sO0U6bBK6w8PdpfyrYIJln+db4t7k6FnVeOOJG4TKF3vysvenSN3kknPzch7T7sN
11qnVPtist4j8vNAWRxo51BcO42AcGmJM8lK6gqrkXRnn+CqhygxUfQ8oM6Q5UQt6N
12mDYRUwvAYTJDwNrPJV378Otgb3Q6ZYF0/+RtIXO44NyhQYZ2mce600UhrrJatXu1
13Dbvck/LPkFOkrjucmW4pE0a9xCx+4UCQ7uzPhz51yCXw0fY9fPK7L3OF/XcFTvTT
14qNDFffulWh3yjw+0TFbvHSB655J9nabJvIhs9pz8G4FE1+kHNwj0bdjkAaMXkR4y
1537zK6NZj
16=NdvU
17-----END PGP SIGNATURE-----
Timestamp of file with hash ab46775e38a58bfb2a237b81708371ffad21f2374a35d117be61bd7b9e0fd5cf -
CustomUintFormatter.