Serialization improvements #10785

pull sipa wants to merge 26 commits into bitcoin:master from sipa:20170707_noncastserial changing 35 files +516 −688
  1. sipa commented at 6:29 pm on July 10, 2017: member

    This PR improves correctness (removing potentially unsafe const_casts) and flexibility of the serialization code.

    The main issue is that use of the current ADD_SERIALIZE_METHODS macro (which is the only way to not duplicate serialization and deserialization code) only expands to a single class method, and thus can only be qualified as either const or non-const - not both. In many cases, serialization needs to work on const objects however, and preferably that is done without casts that could hide const-correctness bugs.

    To deal with that, this PR introduces a new approach that includes a SERIALIZE_METHODS(obj) macro, where obj is a variable name. It expands to some boilerplate and a static method to which the object itself is an argument. The advantage is that its type can be templated, and be const when serializing.

    Another issue is the various serialization-wrapping macros (VARINT, COMPACTSIZE, FLATDATA and LIMITED_STRING). They all const_cast their argument in order to construct a wrapper object, which supports both serialization and deserialization. This PR makes them templated in the underlying data type (for example, CompactSizeWrapper<uint64_t>). This has the advantage that we can make the template type const when invoked on a const variable (so it would be CompactSizeWrapper<const uint64_t> in that case).

    A last issue is the persistent use of the REF macro to deal with temporary expressions being passed in. Since C++11, this is not needed anymore as temporaries are explicitly represented as rvalue references. Thus we can remove REF invocations and instead just make the various classes and helper functions deal correctly with references.

    The above changes permit a fully const-correct version of all serialization code. However, it is cumbersome. Any existing ADD_SERIALIZE_METHODS instances in the code that do more than just (conditionally) serializing/deserializing some fields (in particular, it contains branches that assign to some of the variables) need to be split up into an explicit Serialize and Unserialize method instead. In some cases this is inevitable (wallet serializers do some crazy transformations while serializing!), but in many cases it is just annoying duplication.

    To improve upon this, a few more primitives that are currently inlined are turned into serialization wrappers:

    • BigEndianWrapper: Serializes/deserializes an integer as big endian rather than little endian (only for 16-bit). This permits the CService serialization to become a oneliner.
    • Uint48Wrapper: Serializes/deserializes only the lower 48 bits of an integer (used in BIP152 code).
    • VectorApplyWrapper: Serializes/deserializes a vector while using a custom serializer for its elements. This simplifies the undo and blockencoding serializers a lot.

    Best of all, it removes 147 lines of while code adding a bunch of comments (though the increased use of vararg READWRITE is probably cheating a bit).

    The commits are ordered into 3 sections:

    • First, introduce new classes that permit const-correct serialization.
    • Then one by one transform the various files to use the new serializers.
    • Finally, remove the old serializers.

    This may be too much to go in at once. I’m happy to split things up as needed.

  2. gmaxwell commented at 6:32 pm on July 10, 2017: contributor
    Should probably be tested on big endian. :)
  3. sipa force-pushed on Jul 10, 2017
  4. sipa force-pushed on Jul 10, 2017
  5. in src/blockencodings.h:115 in 21cf5887e9 outdated 
    104+
105+    template <typename Stream>
106+    void Unserialize(Stream& s)
107+    {
108+        std::vector<uint64_t> tmp;
109+        s >> blockhash >> VectorApply<CompactSizeWrapper>(tmp);
    sipa commented at 10:18 pm on July 10, 2017:
    For ease of implementation, deserialization first happens into a std::vector<uint64_t>, and is then converted. This means a temporary is created and allocated, which is an overhead that the old implementation didn’t have.
  6. in src/test/serialize_tests.cpp:54 in 21cf5887e9 outdated 
    53-    template <typename Stream, typename Operation>
54-    inline void SerializationOp(Stream& s, Operation ser_action) {
55-        READWRITEMANY(intval, boolval, stringval, FLATDATA(charstrval), txval);
56+    SERIALIZE_METHODS(obj)
57+    {
58+        READWRITE(obj.intval, obj.boolval, obj.stringval, FlatData(obj.charstrval), obj.txval);
    sipa commented at 10:20 pm on July 10, 2017:
    This whole test is somewhat less valuable now, as both cases use READWRITE.
  7. in src/wallet/wallet.h:369 in 21cf5887e9 outdated 
    380-
381-    template <typename Stream, typename Operation>
382-    inline void SerializationOp(Stream& s, Operation ser_action) {
383-        if (ser_action.ForRead())
384-            Init(NULL);
385+    template<typename Stream>
    sipa commented at 10:22 pm on July 10, 2017:
    This is one of the more involved changes, as it’s both splitting the serializer into two versions, and the Serialize code no longer modifies mapValue in-place (wtf?).
  8. in src/wallet/wallet.h:388 in 21cf5887e9 outdated 
    418-            strFromAccount = mapValue["fromaccount"];
419+        s << vUnused << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << fSpent;
420+    }
421 
422-            ReadOrderPos(nOrderPos, mapValue);
423+    template<typename Stream>
    sipa commented at 10:23 pm on July 10, 2017:
    Here is another big change, that avoids modifying mapValue and strAccount and then later fixing it up before returning (wtf?).
  9. in src/serialize.h:566 in 21cf5887e9 outdated 
    519+ * as a vector of VarInt-encoded integers.
520+ *
521+ * V is not required to be an std::vector type. It works for any class that
522+ * exposes a value_type, iteration, and resize method that behave like vectors.
523+ */
524+template<template <typename> class W, typename V> class VectorApplyWrapper
    sipa commented at 10:25 pm on July 10, 2017:
    Notice the unusual construction of a template that takes a template as parameter here. See “Template template parameter” here: http://en.cppreference.com/w/cpp/language/template_parameters
  10. in src/serialize.h:1021 in 21cf5887e9 outdated 
    977@@ -898,16 +978,16 @@ void SerializeMany(Stream& s)
978 }
979 
980 template<typename Stream, typename Arg>
981-void SerializeMany(Stream& s, Arg&& arg)
982+void SerializeMany(Stream& s, const Arg& arg)
983 {
984-    ::Serialize(s, std::forward<Arg>(arg));
985+    ::Serialize(s, arg);
    sipa commented at 10:26 pm on July 10, 2017:
    The reason for removing the std::forward calls here is explained in the commit message (there is no benefit in passing down the rvalue-ness).
  11. sipa commented at 10:27 pm on July 10, 2017: member
    Here is a self-review in which I point out some of the things reviewers may want to be aware of.
  12. laanwj requested review from jonasschnelli on Jul 11, 2017
  13. laanwj added the label Utils and libraries on Jul 11, 2017
  14. jonasschnelli commented at 1:24 pm on July 11, 2017: contributor

    Concept ACK. Binaries: https://bitcoin.jonasschnelli.ch/build/210 (Currently running on a fresh node) Agree with @gmaxwell that some BE testing would be good.

    Will code-review soon.

  15. sipa force-pushed on Jul 15, 2017
  16. sipa force-pushed on Jul 29, 2017
  17. sipa force-pushed on Jul 30, 2017
  18. sipa commented at 0:20 am on July 30, 2017: member
    Made some changes to reduce the size of the overall diff.
  19. in src/serialize.h:504 in 6020efb64a outdated 
    500+        s << string;
501     }
502 };
503+//! Add a LimitedString wrapper around a const string (identity)
504+template<size_t I> static inline const std::string& LimitedString(const std::string& str) { return str; }
505+//! Add a LimitedString wrapper around a non-const string
    promag commented at 8:12 am on August 6, 2017:
    Missing periods in comments.
  20. promag commented at 8:38 am on August 6, 2017: member

    ACK b0652ac. Everything works as expected.

    Indeed a lot in one shot :).

  21. sipa force-pushed on Aug 15, 2017
  22. sipa commented at 9:38 pm on August 15, 2017: member
    Rebased.
  23. laanwj added this to the "Blockers" column in a project
  24. sipa force-pushed on Aug 25, 2017
  25. sipa commented at 7:02 pm on August 25, 2017: member
    Rebased.
  26. in src/serialize.h:516 in 8dc6fc7cad outdated 
    527+protected:
528+    I& m_val;
529+public:
530+    explicit BigEndianWrapper(I& val) : m_val(val)
531+    {
532+        static_assert(S == 2 || S == 4, "Unsupported BigEndian size");
    ryanofsky commented at 7:39 pm on August 31, 2017:

    In commit “Add BigEndian serialization wrapper”

    Should also static assert sizeof(I) <= S, and std::is_unsigned<I>::value

    sipa commented at 3:36 am on September 1, 2017:
    Fixed.
  27. in src/serialize.h:523 in 8dc6fc7cad outdated 
    514@@ -493,9 +515,44 @@ class LimitedString
515     }
516 };
517 
518+/** Serialization wrapper class for big-endian integers.
519+ *
520+ * Use this wrapper around integer types that are stored in memory in native
521+ * byte order, but serialized in big endian notation.
522+ *
523+ * Onlyy 16-bit types are supported for now.
    ryanofsky commented at 7:42 pm on August 31, 2017:

    In commit “Add BigEndian serialization wrapper”

    This seems to support 16 and 32 bit types, not just 16.

    sipa commented at 3:36 am on September 1, 2017:
    Fixed.
  28. in src/serialize.h:510 in 8dc6fc7cad outdated 
    514@@ -493,9 +515,44 @@ class LimitedString
515     }
516 };
517 
518+/** Serialization wrapper class for big-endian integers.
519+ *
520+ * Use this wrapper around integer types that are stored in memory in native
    ryanofsky commented at 7:46 pm on August 31, 2017:

    In commit “Add BigEndian serialization wrapper”

    Can you add a usage note here on when big endian numbers are actually recommended? Is this only for backwards compatibility with CService? It seems like a serialization format that uses a mix of big endian and little endian numbers would be confusing to work with.

    sipa commented at 3:35 am on September 1, 2017:
    Done.
  29. in src/serialize.h:486 in a090659685 outdated 
    488 
489     template<typename Stream>
490-    void Unserialize(Stream& s) {
491-        n = ReadCompactSize<Stream>(s);
492+    void Serialize(Stream& s) const {
493+        WriteCompactSize<Stream>(s, n);
    ryanofsky commented at 8:04 pm on August 31, 2017:

    Generalize CompactSize wrapper

    Probably should add static assert to check std::is_unsigned<I>, or raise exception if n is less than 0.

    Could also check against numeric_limits<int64_t>::max() at runtime or compile time.

    (It also seems weird, though not relevant to this wrapper, that it is an error to read a compact int greater than MAX_SIZE but not to write one.)

    sipa commented at 3:35 am on September 1, 2017:
    Fixed.
  30. in src/serialize.h:481 in a090659685 outdated 
    481 
482     template<typename Stream>
483-    void Serialize(Stream &s) const {
484-        WriteCompactSize<Stream>(s, n);
485+    void Unserialize(Stream& s) {
486+        n = ReadCompactSize<Stream>(s);
    ryanofsky commented at 8:05 pm on August 31, 2017:

    In commit “Generalize CompactSize wrapper”

    Ideally, this would throw an exception if return value is greater than numeric_limits<I>::max()

    sipa commented at 3:34 am on September 1, 2017:
    Fixed.
  31. in src/serialize.h:440 in 2061c50879 outdated 
    435+//! Construct a FlatRange wrapper around a const POD and array types.
436+template<typename T> static inline const FlatRangeWrapper<const char> FlatDataInner(const T* t, size_t len) { return FlatRangeWrapper<const char>((const char*)t, ((const char*)t) + len); }
437+//! Construct a FlatRange wrapper around a non-const POD and array types.
438+template<typename T> static inline FlatRangeWrapper<char> FlatDataInner(T* t, size_t len) { return FlatRangeWrapper<char>((char*)t, ((char*)t) + len); }
439+//! Helper macro to easily serialize POD types.
440+#define FLATDATA(x) FlatDataInner(&(x), sizeof(x))
    ryanofsky commented at 9:50 pm on August 31, 2017:

    In commit “Generalize FlatData wrapper”

    Though these changes don’t make FLATDATA more dangerous than it was previously, the lack of type safety relying on C casts and sizeof here is a little scary. I experimented a little, and it seems this could be cleaned up with some simple changes I posted here: 6ef78bcd83dd6f88362dec29736811b762ad75eb. Could you take a look, and maybe incorporate these into the PR?

    sipa commented at 3:36 am on September 1, 2017:
    I’ve incorporated part of your changes, but gone further and just added native support for serializing char arrays (without any wrapper). I think this is much cleaner now.
  32. ryanofsky commented at 9:52 pm on August 31, 2017: member
    Reviewed first several commits. Will continue reviewing.
  33. sipa force-pushed on Sep 1, 2017
  34. in src/serialize.h:551 in 00284cb0a6 outdated 
    546+    }
547+
548+    template<typename Stream>
549+    void Unserialize(Stream& s)
550+    {
551+        if (S == 2) m_val = ser_readdata16be(s);
    ryanofsky commented at 8:53 am on September 1, 2017:

    In commit “Add BigEndian serialization wrapper”

    Would be good to throw exception if deserialized value is greater than numeric_limits<I>::max(). Or alternately, change the sizeof(I) <= S requirement to sizeof(I) == S to prevent this being possible.

    sipa commented at 5:53 pm on September 1, 2017:
    Done.
  35. in src/serialize.h:496 in dddd1af751 outdated 
    499     template<typename Stream>
500-    void Unserialize(Stream& s) {
501-        n = ReadCompactSize<Stream>(s);
502+    void Serialize(Stream& s) const
503+    {
504+        WriteCompactSize<Stream>(s, m_n);
    ryanofsky commented at 8:59 am on September 1, 2017:

    In commit “Generalize CompactSize wrapper”

    Would be good to throw exception if m_n is greater than numeric_limits<uint64_t>::max(). Or alternately, require numeric_limits<I>::max() < numeric_limits<int64_t>::max() with static assert.

    sipa commented at 5:53 pm on September 1, 2017:
    Done.
  36. in src/netaddress.h:135 in f8c39376a3 outdated 
    131@@ -132,7 +132,7 @@ class CSubNet
132         inline void SerializationOp(Stream& s, Operation ser_action) {
133             READWRITE(network);
134             READWRITE(FLATDATA(netmask));
135-            READWRITE(FLATDATA(valid));
136+            READWRITE(valid);
    ryanofsky commented at 9:11 am on September 1, 2017:

    In commit “Overhaul FLATDATA”

    Might be worth splitting this change out into separate commit, or noting in commit message here that that this change is not backwards compatible on platforms where sizeof(bool) is not 1.

    sipa commented at 5:53 pm on September 1, 2017:
    Done.
  37. in src/serialize.h:420 in f8c39376a3 outdated 
    420 
421-/** 
422- * Wrapper for serializing arrays and POD.
423- */
424-class CFlatData
425+/** Wrapper for serializing arrays and POD. */
    ryanofsky commented at 9:16 am on September 1, 2017:

    In commit “Overhaul FLATDATA”

    Probably more accurate to say “wrapper for serializing char arrays”. (Though in principle this could work with stream classes with read/write methods not taking char pointers.)

    sipa commented at 5:53 pm on September 1, 2017:
    Fixed.
  38. in src/serialize.h:443 in f8c39376a3 outdated 
    464-        s.read(pbegin, pend - pbegin);
465+        s.read(m_begin, m_end - m_begin);
466     }
467 };
468+//! Construct a FlatRange wrapper around a const char vector.
469+template<typename T> static inline const FlatRangeWrapper<const char> FlatVector(const T& t) { return FlatRangeWrapper<const char>(CharCast(t.data()), CharCast(t.data() + t.size())); }
    ryanofsky commented at 9:32 am on September 1, 2017:

    In commit “Overhaul FLATDATA”

    Maybe call it CharVector or CharArray instead of FlatVector. FlatVector is kind of redundant because any vectors should be flat. But also the vector part is limiting because these functions can work for other types (std::array, std::string, SecureString, std::basic_string_view, etc)

    sipa commented at 5:53 pm on September 1, 2017:
    Done.
  39. in src/serialize.h:192 in d04c257405 outdated 
    174- * Implement three methods for serializable objects. These are actually wrappers over
175- * "SerializationOp" template, which implements the body of each class' serialization
176- * code. Adding "ADD_SERIALIZE_METHODS" in the body of the class causes these wrappers to be
177- * added as members. 
178- */
179-#define ADD_SERIALIZE_METHODS                                         \
    ryanofsky commented at 9:46 am on September 1, 2017:

    In commit “Remove old serialization primitives”

    There are still two references to ADD_SERIALIZE_METHODS in comments.

    sipa commented at 6:38 am on September 2, 2017:
    Done.
  40. sipa force-pushed on Sep 1, 2017
  41. in src/serialize.h:177 in d85bbf9aff outdated 
    172+ * static method that takes the to-be-(de)serialized object as a parameter. This approach
173+ * has the advantage that the constness of the object becomes a template parameter, and
174+ * thus allows a single implementation that sees the object as const for serializing
175+ * and non-const for deserializing, without casts.
176+ */
177+#define SERIALIZE_METHODS(obj)                                                      \
    ryanofsky commented at 6:27 pm on September 1, 2017:

    In commit “Introduce new serialization macros without casts”

    It would be nice if the SERIALIZE_METHODS macro took a class_name argument. I’d like this so it’d be possible to add deserializing constructors here (like CTransaction has), so there could be a uniform way to deserialize objects without assuming they support default construction. But also a class_name argument would make the macro more flexible and future proof, so it’d be easy to do things like:

    • Adding stricter type checking (e.g. an is_same<class_name, remove_const<Type>> assert in SerializationOps to prevent usage errors or template bloat.
    • Logging or debugging with #class_name
    • Adding static or friend functions that reference class_name.
    sipa commented at 0:19 am on September 2, 2017:
    Done!
  42. in src/serialize.h:151 in 2d0edd1cfc outdated 
    147@@ -148,9 +148,21 @@ enum
148     SER_GETHASH         = (1 << 2),
149 };
150 
151+// Convert the reference base type to X, without changing constness or reference type.
    ryanofsky commented at 6:30 pm on September 1, 2017:

    In commit “Add READWRITEAS, a macro to serialize safely as a different type”

    Maybe use //!

    sipa commented at 0:19 am on September 2, 2017:
    Done.
  43. in src/serialize.h:472 in 08da230f0f outdated 
    504+    {
505+        WriteCompactSize<Stream>(s, m_n);
506     }
507 };
508+//! Automatically construct a CompactSize wrapper around the argument.
509+template<typename I> static inline CompactSizeWrapper<I> COMPACTSIZE(I& i) { return CompactSizeWrapper<I>(i); }
    ryanofsky commented at 6:52 pm on September 1, 2017:

    In commit “Generalize CompactSize wrapper”

    Might be good to add const I& i overload so it’s possible to serialize rvalues.

    sipa commented at 6:38 am on September 2, 2017:
    Done (and for other wrappers).
  44. in src/serialize.h:435 in d013fb285b outdated 
    476+    {
477+        m_n = ReadVarInt<Stream,I>(s);
478     }
479 };
480+//! Automatically construct a VarInt wrapper around the argument.
481+template<typename I> static inline VarIntWrapper<typename std::remove_reference<I>::type> VARINT(I&& i) { return VarIntWrapper<typename std::remove_reference<I>::type>(i); }
    ryanofsky commented at 6:56 pm on September 1, 2017:

    In commit “Generalize VarInt wrappers”

    This might be easier to understand written with overloads instead of rvalue references:

    0template<typename I> static inline VarIntWrapper<I> VARINT(I& i) { return VarIntWrapper<I>(i); }
1template<typename I> static inline VarIntWrapper<const I> VARINT(const I& i) { return VarIntWrapper<const I>(i); }

    Also would make it more consistent with other wrappers.

    sipa commented at 0:20 am on September 2, 2017:

    The problem is that VARINT is called with temporaries as arguments, which is not true for the other ones. Either it’s written as 4 cases, or using std::remove_refence.

    EDIT: Oh, you’re right. An lvalue reference parameter binds to rvalue reference argument, so all good.

    sipa commented at 6:38 am on September 2, 2017:
    Done.
  45. in src/serialize.h:423 in f6063a0280 outdated 
    443-    explicit CFlatData(prevector<N, T, S, D> &v)
444+    CharArrayWrapper(C* begin, size_t size) : m_begin(begin), m_size(size)
445     {
446-        pbegin = (char*)v.data();
447-        pend = (char*)(v.data() + v.size());
448+        static_assert(sizeof(C) == 1, "CharArrayWrapper only works for char types");
    ryanofsky commented at 7:00 pm on September 1, 2017:

    In commit “Overhaul FLATDATA for char arrays only”

    Consider dropping this assert. I don’t think it accomplishes much, and in principle CharArrayWrapper should work perfectly well with wchar_t, or with any stream object that has happens to have non-char read and write methods.

    sipa commented at 6:39 am on September 2, 2017:
    I’m not sure that wchar_t has a well-defined in-memory representation.
    ryanofsky commented at 5:29 pm on September 5, 2017:

    I’m not sure that wchar_t has a well-defined in-memory representation.

    Since this class is no longer casting any pointers, I don’t think that’s a problem. Existing c++ type checking will make sure pointers passed to stream read & write methods are compatible, so I don’t think there is a reason for this class to be interjecting and adding extra type requirements.

    sipa commented at 6:36 pm on September 6, 2017:

    I think I’m talking about something else.

    If you serialize a 16-bit wchar_t on a big endian system using this class, the characters will be serialized using 2 big endian bytes. If you then deserialize it on a little endian system, you won’t get the same wchar_t values back.

  46. in src/serialize.h:901 in a2d8708577 outdated 
    897@@ -898,16 +898,16 @@ void SerializeMany(Stream& s)
898 }
899 
900 template<typename Stream, typename Arg>
901-void SerializeMany(Stream& s, Arg&& arg)
902+void SerializeMany(Stream& s, const Arg& arg)
    ryanofsky commented at 7:21 pm on September 1, 2017:

    In commit “Support deserializing into temporaries”

    Could drop this overload entirely. The overload above is sufficient for a base case.

    Also applies to UnserializeMany below.

    sipa commented at 0:19 am on September 2, 2017:
    Done.
  47. in src/serialize.h:523 in 0b5fd4ad9c outdated 
    517@@ -519,19 +518,23 @@ class LimitedString
518         if (size > Limit) {
519             throw std::ios_base::failure("String length limit exceeded");
520         }
521-        string.resize(size);
522-        if (size != 0)
523-            s.read((char*)string.data(), size);
524+        m_string.resize(size);
525+        if (size != 0) {
526+            s.read((char*)m_string.data(), size);
    ryanofsky commented at 7:31 pm on September 1, 2017:

    In commit “Generalize LimitedString wrapper”

    Maybe use &m_string[0] avoid writing to a const pointer.

    sipa commented at 0:19 am on September 2, 2017:
    Done.
  48. in src/serialize.h:584 in ba4260615d outdated 
    615+    {
616+        m_vector.clear();
617+        unsigned int nSize = ReadCompactSize(s);
618+        unsigned int nMid = 0;
619+        while (nMid < nSize) {
620+            nMid += 5000000 / sizeof(value_type);
    ryanofsky commented at 7:55 pm on September 1, 2017:

    In commit “Add custom vector-element serialization wrapper”

    This could use a comment. I don’t understand it at all. Wouldn’t it be simpler and more efficient to just resize and fill the vector once instead of resizing it multiple times?

    sipa commented at 6:38 am on September 2, 2017:
    Done. The code was also totally broken, so I’ve rewritten it.
  49. in src/serialize.h:590 in ba4260615d outdated 
    621+            if (nMid > nSize) {
622+                nMid = nSize;
623+            }
624+            m_vector.resize(nMid);
625+            for (value_type& x : m_vector) {
626+                s >> W<value_type>(x);
    ryanofsky commented at 8:01 pm on September 1, 2017:

    In commit “Add custom vector-element serialization wrapper”

    Doesn’t this overwrite elements in the front of the vector each time through the while loop?

    sipa commented at 6:39 am on September 2, 2017:
    Yes, this was bogus. Thanks for pointing that out; fixed by rewriting,
  50. in src/blockencodings.h:45 in d64f7fb78d outdated 
    48+        s >> lsb >> msb;
49+        m_int = (uint64_t(msb) << 32) | uint64_t(lsb);
50     }
51 };
52 
53+template<typename T>
    ryanofsky commented at 8:05 pm on September 1, 2017:

    In commit “Convert blockencodings to new serialization”

    Maybe move this up closer to TransactionCompressWrapper class

    sipa commented at 6:39 am on September 2, 2017:
    Done.
  51. in src/blockencodings.h:193 in d64f7fb78d outdated 
    198+    template <typename Stream>
199+    inline void Unserialize(Stream& s)
200+    {
201+        static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids serialization assumes 6-byte shorttxids");
202+        s >> header >> nonce >> VectorApply<Uint48Wrapper>(shorttxids) >> prefilledtxn;
203+        FillShortTxIDSelector();
    ryanofsky commented at 8:35 pm on September 1, 2017:

    In commit “Convert blockencodings to new serialization”

    Would you be opposed to adding mutable object access to SERIALIZE_METHODS so Serialize and Unserialize methods don’t need to be split up? I could think of a number of ways to do this. Maybe easiest would be to stick a mutable pointer inside ser_action:

    0SERIALIZE_METHODS(obj) {
1  READWRITE(obj.header, obj.nonce, VectorApply<Uint48Wrapper>(obj.shorttxids), obj.prefilledtxn);
2  if (ser_action.ForRead()) ser_action.MutableObj()->FillShortTxId();
3}

    The pointer would be null when serializing.

    sipa commented at 3:29 am on September 3, 2017:

    That’s a neat trick. It seems a bit ugly to need to fake returning a mutable object nullptr. I have an alternative, but I’m not sure it’s any better.:

    0template <typename T, typename F>
1void IfUnserializer(T& obj, CSerActionUnserialize ser_action, const F& fn) { fn(obj); }
2template <typename T, typename F>
3void IfUnserialize(const T& obj, CSerActionSerialize ser_action, const F& fn) {}
4#define IF_UNSERIALIZE(typ, obj, code) (::IfUnserialize<typ>(obj, ser_action, [&](typ& obj)code))

    Which you’d then invoke using

    0     SERIALIZE_METHODS(SomeType, obj)
1     {
2          READWRITE(obj.member);
3          IF_UNSERIALIZE(SomeType, obj, {obj.FillShortTxid();});
4     }
    ryanofsky commented at 8:54 pm on September 5, 2017:

    Thread #10785 (review)

    That looks good to me. I was actually going to suggest this same approach before I noticed there was a ser_action.ForRead method. ser_action could also have a method returning a reference instead of a pointer. I think any approach that would avoid duplicating serialization & deserialization would be good, though.

    For IF_UNSERIALIZE, maybe consider getting of all the obj macro arguments:

    0     SERIALIZE_METHODS(SomeType)
1     {
2          READWRITE(obj.member);
3          IF_UNSERIALIZE({obj.FillShortTxid();});
4     }

    I think the obj arguments maybe help make serialize methods resemble normal methods, but don’t actually add real utility.

    sipa commented at 6:34 pm on September 6, 2017:
    @ryanofsky I don’t see how to get rid of passing in the type to IF_UNSERIALIZE.
    ryanofsky commented at 7:28 pm on September 6, 2017:

    @ryanofsky I don’t see how to get rid of passing in the type to IF_UNSERIALIZE.

    You could pass it as a template parameter to CSerActionSerialize / CSerActionUnserialize and access it through ser_action.

  52. ryanofsky commented at 8:43 pm on September 1, 2017: member
    More review comments. I made it up to the “Convert blockencodings to new serialization” commit this time.
  53. sipa force-pushed on Sep 2, 2017
  54. sipa force-pushed on Sep 2, 2017
  55. in src/serialize.h:628 in 954826ce26 outdated 
    623+        size_t deserialized = 0;
624+        while (allocated < size) {
625+            // For DoS prevention, do not blindly allocate as much as the stream claims to contain.
626+            // Instead, allocate in 5MiB batches, so that an attacker actually needs to provide
627+            // X MiB of data to make us allocate X+5 Mib.
628+            allocated = std::min(size, allocated + 5000000 / sizeof(value_type));
    ryanofsky commented at 7:40 pm on September 5, 2017:

    In commit “Add custom vector-element serialization wrapper”

    Maybe declare 5MiB as a constant next to to MAX_SIZE, since it serves a similar purpose.

    sipa commented at 6:36 pm on September 6, 2017:
    Done.
  56. in src/blockencodings.h:39 in 97250236a0 outdated 
    39+    Uint48Wrapper(I& i) : m_int(i) {}
40+    template <typename Stream> void Serialize(Stream& s) const
41+    {
42+        uint32_t lsb = m_int & 0xffffffff;
43+        uint16_t msb = (m_int >> 32) & 0xffff;
44+        s << lsb << msb;
    ryanofsky commented at 8:19 pm on September 5, 2017:

    In commit “Convert blockencodings to new serialization”

    Similar to previous suggestions, could throw here if m_int is >= 2**48 or less than 0 (or static assert is_unsigned).

    sipa commented at 6:36 pm on September 6, 2017:
    Added static assert; I’d like to avoid runtime impact.
  57. in src/blockencodings.h:46 in 97250236a0 outdated 
    46+    template <typename Stream> void Unserialize(Stream& s)
47+    {
48+        uint32_t lsb;
49+        uint16_t msb;
50+        s >> lsb >> msb;
51+        m_int = (uint64_t(msb) << 32) | uint64_t(lsb);
    ryanofsky commented at 8:35 pm on September 5, 2017:

    In commit “Convert blockencodings to new serialization”

    Could throw if deserialized value is greater than numeric_limits<I>::max(), or static_assert that I max is big enough to hold any 48 bit value.

    sipa commented at 6:36 pm on September 6, 2017:
    Added static assert.
  58. in src/blockencodings.h:106 in 97250236a0 outdated 
    86-                uint64_t index = indexes[i] - (i == 0 ? 0 : (indexes[i - 1] + 1));
87-                READWRITE(COMPACTSIZE(index));
88-            }
89+        s << COMPACTSIZE(indexes_size);
90+        for (size_t i = 0; i < indexes.size(); i++) {
91+            const uint64_t index = indexes[i] - (i == 0 ? 0 : (indexes[i - 1] + 1));
    ryanofsky commented at 9:55 pm on September 5, 2017:

    In commit “Convert blockencodings to new serialization”

    Would be good to assert or throw if indexes[i] <= indices[i-1].

    sipa commented at 6:37 pm on September 6, 2017:
    I’d rather avoid runtime overhead.
    ryanofsky commented at 7:24 pm on September 6, 2017:
    In that case, could add a comment like “this code will produce an invalid serialization if indices are not increasing.”
  59. in src/undo.h:90 in 62f7eb2925 outdated 
    94-    template <typename Stream>
95-    void Unserialize(Stream& s) {
96-        // TODO: avoid reimplementing vector deserializer
97-        uint64_t count = 0;
98-        ::Unserialize(s, COMPACTSIZE(count));
99-        if (count > MAX_INPUTS_PER_BLOCK) {
    ryanofsky commented at 11:28 pm on September 5, 2017:
    Maybe mention in commit message if behavior is changing here. I guess the limit is higher now.
    sipa commented at 6:25 am on September 7, 2017:
    It could use slightly more memory when deserializing an otherwise invalid object, but that shouldn’t change behaviour otherwise - if the number of transaction undo objects doesn’t match the number of transaction in the block, it’s invalid anyway.
  60. in src/qt/recentrequeststablemodel.h:39 in 30d7e934d6 outdated 
    45+    template<typename Stream>
46+    void Unserialize(Stream& s)
47+    {
48+        unsigned int nDate;
49+        s >> this->nVersion >> id >> nDate >> recipient;
50+        date = QDateTime::fromTime_t(nDate);
    ryanofsky commented at 11:30 pm on September 5, 2017:

    In commit “Convert Qt to new serialization”

    I guess this is another place that could use IF_UNSERIALIZE if you decide to go this route.

    sipa commented at 7:05 pm on September 7, 2017:
    Introduced a wrapper for QDateTime instead.
  61. sipa removed this from the "Blockers" column in a project
  62. in src/qt/walletmodel.h:90 in 30d7e934d6 outdated 
    106+        label = QString::fromStdString(sLabel);
107+        message = QString::fromStdString(sMessage);
108+        if (!sPaymentRequest.empty()) {
109+            paymentRequest.parse(QByteArray::fromRawData(sPaymentRequest.data(), sPaymentRequest.size()));
110         }
111+        authenticatedMerchant = QString::fromStdString(sAuthenticatedMerchant);
    ryanofsky commented at 11:41 pm on September 5, 2017:

    In commit “Convert Qt to new serialization”

    It seems like if there were serialization wrappers from QString and proto types, the serialize and deserialize methods could be combined again.

    sipa commented at 7:09 pm on September 7, 2017:
    Done using wrappers for QString and proto.
  63. in src/wallet/wallet.h:251 in 651bb85d38 outdated 
    148+            s >> nVersion;
149+        }
150+        s >> nTime >> vchPubKey;
151+        try {
152+            s >> fInternal;
153+        } catch (std::ios_base::failure&) {
    ryanofsky commented at 11:58 pm on September 5, 2017:

    In commit “Convert wallet/walletdb/crypter to new serialization

    Maybe another place to use IF_UNSERIALIZE. Or maybe there could be a wrapper that ignores ios_base errors on deserialization.

  64. in src/wallet/wallet.h:409 in 651bb85d38 outdated 
    433-        }
434+        s >> AsBaseType<CMerkleTx>(*this);
435+        std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
436+        s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent;
437+
438+        strFromAccount = mapValue["fromaccount"];
    ryanofsky commented at 0:02 am on September 6, 2017:
    Could use std::move here.
    sipa commented at 7:09 pm on September 7, 2017:
    Done.
  65. ryanofsky commented at 0:12 am on September 6, 2017: member
    utACK b49e84ea6dd7e2c0aacb8546c8d8c2d99d1d8214. Finally reviewed the full change.
  66. sipa force-pushed on Sep 6, 2017
  67. ryanofsky commented at 7:40 pm on September 6, 2017: member
    utACK a9770fbe6ef6d7d9bd5b61e4c51531af7cc2a1fc. Still looks good. Only minor changes since previous review, described in comments above.
  68. laanwj commented at 10:12 pm on September 6, 2017: member
    Big concept ACK, happy to get rid of FLATDATA and similar ugly macros. This is a lot to review/test though, and reasonably high-risk.
  69. sipa force-pushed on Sep 6, 2017
  70. sipa force-pushed on Sep 7, 2017
  71. sipa force-pushed on Sep 7, 2017
  72. sipa force-pushed on Sep 8, 2017
  73. sipa force-pushed on Oct 20, 2017
  74. sipa commented at 11:27 am on October 20, 2017: member
    Rebased.
  75. ryanofsky commented at 7:51 pm on October 31, 2017: member
    utACK c3e505c8551a29d12edcebbed8a69f1e798823ef. Changes since previous review were rebase and addressing a few comments. Main one being new guiutil.h wrappers for qt strings, dates, and protobufs.
  76. MarcoFalke commented at 9:44 pm on October 31, 2017: member
    Travis can not run the qt tests. Are they passing for you locally?
  77. ryanofsky commented at 9:50 pm on October 31, 2017: member 
    0
1QFATAL : WalletTests::walletTests() Received signal 11
2FAIL!  : WalletTests::walletTests() Received a fatal error.
3   Loc: [Unknown file(0)]
4
5EDIT: Actually they are failing for me locally too
  78. MarcoFalke commented at 10:00 pm on October 31, 2017: member
    I guess they time out and you could try setting the QTEST_FUNCTION_TIMEOUT env var to 600,000. Though, it might be worthwhile to check why they take longer than before.
  79. ryanofsky commented at 10:11 pm on October 31, 2017: member

    Actually they are failing for me locally. Stack trace shows an infinite recursion and looks like:

     0(gdb) bt -50
 1[#2355058](/bitcoin-bitcoin/2355058/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 2[#2355059](/bitcoin-bitcoin/2355059/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 3[#2355060](/bitcoin-bitcoin/2355060/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 4[#2355061](/bitcoin-bitcoin/2355061/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 5[#2355062](/bitcoin-bitcoin/2355062/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 6[#2355063](/bitcoin-bitcoin/2355063/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 7[#2355064](/bitcoin-bitcoin/2355064/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 8[#2355065](/bitcoin-bitcoin/2355065/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
 9[#2355066](/bitcoin-bitcoin/2355066/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
10[#2355067](/bitcoin-bitcoin/2355067/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
11[#2355068](/bitcoin-bitcoin/2355068/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
12[#2355069](/bitcoin-bitcoin/2355069/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
13[#2355070](/bitcoin-bitcoin/2355070/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
14[#2355071](/bitcoin-bitcoin/2355071/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
15[#2355072](/bitcoin-bitcoin/2355072/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
16[#2355073](/bitcoin-bitcoin/2355073/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
17[#2355074](/bitcoin-bitcoin/2355074/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
18[#2355075](/bitcoin-bitcoin/2355075/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
19[#2355076](/bitcoin-bitcoin/2355076/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (qstring=...) at qt/guiutil.h:285
20[#2355077](/bitcoin-bitcoin/2355077/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (qstring=...) at qt/guiutil.h:285
21[#2355078](/bitcoin-bitcoin/2355078/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (ser_action=..., s=..., obj=...) at qt/walletmodel.h:69
22[#2355079](/bitcoin-bitcoin/2355079/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (s=..., this=0x7fffffffb728) at qt/walletmodel.h:66
23[#2355080](/bitcoin-bitcoin/2355080/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (a=..., os=...) at ./serialize.h:677
24[#2355081](/bitcoin-bitcoin/2355081/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=..., s=...) at ./serialize.h:1030
25[#2355082](/bitcoin-bitcoin/2355082/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=..., s=...) at ./serialize.h:1031
26[#2355083](/bitcoin-bitcoin/2355083/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=@0x7fffffffb718: 1, s=...) at ./serialize.h:1031
27[#2355084](/bitcoin-bitcoin/2355084/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=@0x7fffffffb710: 1, s=...) at ./serialize.h:1031
28[#2355085](/bitcoin-bitcoin/2355085/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (ser_action=..., s=...) at ./serialize.h:1049
29[#2355086](/bitcoin-bitcoin/2355086/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (s=..., obj=..., ser_action=...) at qt/recentrequeststablemodel.h:30
30[#2355087](/bitcoin-bitcoin/2355087/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (s=..., this=0x7fffffffb710) at qt/recentrequeststablemodel.h:28
31[#2355088](/bitcoin-bitcoin/2355088/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (a=..., os=...) at ./serialize.h:677
32[#2355089](/bitcoin-bitcoin/2355089/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (obj=..., this=0x7fffffffb600) at ./streams.h:399
33[#2355090](/bitcoin-bitcoin/2355090/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (this=0x1014511d0, recipient=...) at qt/recentrequeststablemodel.cpp:173
34[#2355091](/bitcoin-bitcoin/2355091/) 0x00000001000d2a92 in ReceiveCoinsDialog::on_receiveButton_clicked() (this=0x7fffffffbf50) at qt/receivecoinsdialog.cpp:167
35[#2355092](/bitcoin-bitcoin/2355092/) 0x000000010010dd95 in ReceiveCoinsDialog::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (_o=<optimized out>, _c=<optimized out>, _id=<optimized out>, _a=<optimized out>)
36    at qt/moc_receivecoinsdialog.cpp:123
37[#2355093](/bitcoin-bitcoin/2355093/) 0x000000010010e0e5 in ReceiveCoinsDialog::qt_metacall(QMetaObject::Call, int, void**) (this=0x7fffffffbf50, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7fffffffbbb0) at qt/moc_receivecoinsdialog.cpp:177
38[#2355094](/bitcoin-bitcoin/2355094/) 0x00007ffff62b2ee0 in QMetaObject::activate(QObject*, int, int, void**) (sender=sender@entry=0x10163e380, signalOffset=<optimized out>, local_signal_index=local_signal_index@entry=2, argv=argv@entry=0x7fffffffbbb0) at kernel/qobject.cpp:3728
39[#2355095](/bitcoin-bitcoin/2355095/) 0x00007ffff62b3537 in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) (sender=sender@entry=0x10163e380, m=m@entry=0x7ffff7098a40 <QAbstractButton::staticMetaObject>, local_signal_index=local_signal_index@entry=2, argv=argv@entry=0x7fffffffbbb0) at kernel/qobject.cpp:3578
40[#2355096](/bitcoin-bitcoin/2355096/) 0x00007ffff6f112b2 in QAbstractButton::clicked(bool) (this=this@entry=0x10163e380, _t1=false) at .moc/moc_qabstractbutton.cpp:303
41[#2355097](/bitcoin-bitcoin/2355097/) 0x00007ffff6c73f44 in QAbstractButtonPrivate::emitClicked() (this=0x1016cc1d0) at widgets/qabstractbutton.cpp:534
42[#2355098](/bitcoin-bitcoin/2355098/) 0x00007ffff6c745e1 in QAbstractButton::click() (this=0x10163e380) at widgets/qabstractbutton.cpp:992
43[#2355099](/bitcoin-bitcoin/2355099/) 0x0000000100086686 in (anonymous namespace)::TestGUI() () at qt/test/wallettests.cpp:222
44[#2355100](/bitcoin-bitcoin/2355100/) 0x00007ffff628f84a in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (this=this@entry=0x7fffffffce98, object=object@entry=0x7fffffffd830, connectionType=Qt::DirectConnection, 
45    connectionType@entry=4294954784, returnValue=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...) at kernel/qmetaobject.cpp:2212
46[#2355101](/bitcoin-bitcoin/2355101/) 0x00007ffff6294f0d in QMetaObject::invokeMethod(QObject*, char const*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) (obj=0x7fffffffd830, member=member@entry=0x100ca1990 "walletTests", type=4294954784, 
47    type@entry=Qt::DirectConnection, ret=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...) at kernel/qmetaobject.cpp:1479
48[#2355102](/bitcoin-bitcoin/2355102/) 0x00007ffff7fa408c in QTest::qInvokeTestMethod(char const*, char const*) (val9=..., val8=..., val7=..., val6=..., val5=..., val4=..., val3=..., val2=..., val1=..., val0=..., type=Qt::DirectConnection, member=0x100ca1990 "walletTests", obj=<optimized out>) at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs.h:409
49[#2355103](/bitcoin-bitcoin/2355103/) 0x00007ffff7fa408c in QTest::qInvokeTestMethod(char const*, char const*) (slot=0x100ca1990 "walletTests") at qtestcase.cpp:1953
50[#2355104](/bitcoin-bitcoin/2355104/) 0x00007ffff7fa408c in QTest::qInvokeTestMethod(char const*, char const*) (slotName=0x100948698 "walletTests()", data=data@entry=0x0) at qtestcase.cpp:2082
51[#2355105](/bitcoin-bitcoin/2355105/) 0x00007ffff7fa4bb8 in QTest::qExec(QObject*, int, char**) (testObject=0x7fffffffd830) at qtestcase.cpp:2390
52[#2355106](/bitcoin-bitcoin/2355106/) 0x00007ffff7fa4bb8 in QTest::qExec(QObject*, int, char**) (testObject=0x7fffffffd830, argc=<optimized out>, argv=<optimized out>) at qtestcase.cpp:2652
53[#2355107](/bitcoin-bitcoin/2355107/) 0x000000010005d9df in main(int, char**) (argc=1, argv=0x7fffffffdd78) at qt/test/test_main.cpp:99
  80. in src/qt/guiutil.h:287 in c3e505c855 outdated 
    280+        std::string str;
281+        s >> str;
282+        m_qstring = QString::fromStdString(std::move(str));
283+    }
284+};
285+template<typename Q> AsStdStringWrapper<Q> AsStdString(Q& qstring) { return AsStdString<Q>(qstring); }
    ryanofsky commented at 10:15 pm on October 31, 2017:
    This should say return AsStdStringWrapper<Q>(qstring) or just return {qstring}
    sipa commented at 4:07 pm on November 2, 2017:
    Fixed! Thanks for figuring this out!
  81. sipa force-pushed on Nov 2, 2017
  82. sipa force-pushed on Nov 2, 2017
  83. sipa commented at 5:13 pm on November 2, 2017: member
    Rebased.
  84. sipa force-pushed on Nov 8, 2017
  85. sipa commented at 8:12 pm on November 8, 2017: member
    Removed the need for creating a vector with absolute values for serializing/deserializing BlockTransactionsRequest.
  86. sipa force-pushed on Nov 10, 2017
  87. sipa force-pushed on Nov 21, 2017
  88. sipa commented at 11:02 am on November 21, 2017: member
    Rebased.
  89. sipa force-pushed on Feb 8, 2018
  90. sipa commented at 7:29 pm on February 8, 2018: member
    Rebased.
  91. Christewart commented at 10:14 pm on February 8, 2018: member

    I ran this with #8469 – which tests serialization code pretty extensively – and everything passed.

    If anyone wants to re-run they can do with this branch: https://github.com/Christewart/bitcoin/commits/pbt_noncastserial

  92. sipa force-pushed on Mar 7, 2018
  93. laanwj referenced this in commit af88094e4f on Mar 13, 2018
  94. sipa force-pushed on Mar 13, 2018
  95. sipa commented at 6:27 pm on March 13, 2018: member
    Rebased.
  96. sipa referenced this in commit 7be9a9a570 on Mar 15, 2018
  97. sipa force-pushed on Mar 16, 2018
  98. sipa force-pushed on Mar 17, 2018
  99. sipa commented at 11:29 pm on March 17, 2018: member

    I’ve reordered the commits again a bit, and made a small (but invasive) change to the design:

    • Removed macro READWRITEAS (using AsType<type>(obj) instead)
    • Added a Wrap<wrapper>(obj) function to later avoid the need for COMPACTSIZE, VARINT, and LIMITED_STRING, and removing the need for helper functions for each wrapper class.

    Feel free to ignore the changes here; I’ll keep taking small bits from it and submitting them as separate PRs.

  100. sipa force-pushed on Mar 20, 2018
  101. sipa force-pushed on Mar 21, 2018
  102. sipa force-pushed on Mar 21, 2018
  103. sipa force-pushed on Mar 21, 2018
  104. laanwj referenced this in commit d2d7267e23 on Mar 30, 2018
  105. laanwj referenced this in commit 8203c4c42e on Mar 30, 2018
  106. sipa force-pushed on Mar 30, 2018
  107. sipa force-pushed on Mar 30, 2018
  108. sipa force-pushed on Apr 7, 2018
  109. sipa force-pushed on Apr 8, 2018
  110. sipa force-pushed on Apr 10, 2018
  111. sipa force-pushed on Apr 10, 2018
  112. sipa force-pushed on Apr 10, 2018
  113. laanwj referenced this in commit 7b6041d1a7 on Apr 11, 2018
  114. sipa force-pushed on Apr 17, 2018
  115. laanwj commented at 2:00 pm on May 14, 2018: member
    Concept ACK
  116. sipa force-pushed on May 14, 2018
  117. sipa commented at 0:36 am on May 17, 2018: member
    @laanwj I’m going to keep extracting smaller changes from this PR into separate ones (like #12916, #12886, #12752, #12740, #12712, #12683, #12658). No need to review the whole thing, but concept ACKs are useful.
  118. MarcoFalke commented at 3:31 pm on May 21, 2018: member
    Needs rebase due to merge of #12924
  119. sipa force-pushed on May 22, 2018
  120. DrahtBot added the label Needs rebase on Jun 7, 2018
  121. sipa force-pushed on Jun 12, 2018
  122. DrahtBot removed the label Needs rebase on Jun 12, 2018
  123. DrahtBot added the label Needs rebase on Jul 25, 2018
  124. sipa force-pushed on Jul 25, 2018
  125. sipa force-pushed on Jul 25, 2018
  126. DrahtBot removed the label Needs rebase on Jul 25, 2018
  127. MarcoFalke commented at 8:59 pm on July 25, 2018: member

    Travis output:

    0A wild circular dependency in the form of "qt/guiutil -> qt/walletmodel -> qt/guiutil" appears...
  128. sipa force-pushed on Jul 25, 2018
  129. DrahtBot added the label Needs rebase on Aug 30, 2018
  130. jb55 commented at 3:15 pm on October 8, 2018: member

    Chris Stewart notifications@github.com writes:

    I ran this with #8469 – which tests serialization code pretty extensively – and it everything passed.

    I gave this a spin with the property tests as well. Seems to still pass. There’s a recent change to bench/prevector that needs to be updated but other than that it appears to work.

  131. sipa force-pushed on Nov 15, 2018
  132. sipa commented at 8:13 pm on November 15, 2018: member
    Rebased.
  133. DrahtBot removed the label Needs rebase on Nov 15, 2018
  134. DrahtBot commented at 11:55 pm on November 15, 2018: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17896 (Serialization improvements (step 2) by sipa)
    • #17034 (Bip174 extensions by achow101)
    • #16748 ([WIP] Add support for addrv2 (BIP155) by dongcarl)
    • #16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
    • #14053 (Add address-based index (attempt 4?) by marcinja)

    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.

  135. in src/compressor.h:101 in f92343358b outdated 
    135-            s >> MakeSpan(script);
136+        template<typename Stream> void Unserialize(Stream& s)
137+        {
138+            uint64_t v;
139+            s >> VARINT(v);
140+            m_val = DecompressAmount(v);
    practicalswift commented at 11:00 am on November 17, 2018:
    Make this conversion explicit since it changes signedness?
    sipa commented at 11:54 pm on November 19, 2018:

    The best I can do in this place is cast it to I, which isn’t making anything interesting explicit, and will just hide the warning.

    The issue is that CTxOut has a signed int64_t, but for UTXOs only ever contains unsigned values. I think the warning, if any, is appropriate, and should remain until the actual “issue” (the fact that the type in outputs doesn’t match its assumptions) is fixed.

    practicalswift commented at 6:51 am on November 20, 2018:
    Makes sense! Thanks for the clarification.
  136. in src/compressor.h:95 in f92343358b outdated 
    121+        I& m_val;
122+    public:
123+        explicit Wrapper(I& val) : m_val(val) {}
124+        template<typename Stream> void Serialize(Stream& s) const
125+        {
126+            s << VARINT(CompressAmount(m_val));
    practicalswift commented at 11:00 am on November 17, 2018:
    Make this conversion explicit since it changes signedness?
  137. in src/compressor.h:115 in f92343358b outdated 
    168+    class Wrapper
169+    {
170+    private:
171+        O &txout;
172+    public:
173+        Wrapper(O &txoutIn) : txout(txoutIn) { }
    practicalswift commented at 11:01 am on November 17, 2018:
    Make this explicit? :-)
  138. in src/wallet/wallet.h:584 in f92343358b outdated 
    583-
584-    template <typename Stream, typename Operation>
585-    inline void SerializationOp(Stream& s, Operation ser_action) {
586+    SERIALIZE_METHODS(CWalletKey, obj)
587+    {
588         int nVersion = s.GetVersion();
    practicalswift commented at 11:02 am on November 17, 2018:
    The scope of nVersion can be reduced?
  139. in src/qt/walletmodel.h:59 in f92343358b outdated 
    58+    class Wrapper
59+    {
60+    private:
61+        Q& m_qstring;
62+    public:
63+        Wrapper(Q& qstring) : m_qstring(qstring) {}
    practicalswift commented at 11:02 am on November 17, 2018:
    explicit?
  140. in src/test/dbwrapper_tests.cpp:338 in f92343358b outdated 
    250@@ -251,24 +251,26 @@ struct StringContentsSerializer {
251     }
252     StringContentsSerializer& operator+=(const StringContentsSerializer& s) { return *this += s.str; }
253 
254-    ADD_SERIALIZE_METHODS;
255-
256-    template <typename Stream, typename Operation>
257-    inline void SerializationOp(Stream& s, Operation ser_action) {
258-        if (ser_action.ForRead()) {
    practicalswift commented at 11:04 am on November 17, 2018:
    ForRead is no longer needed and can be removed from serialize.h?
  141. DrahtBot added the label Needs rebase on Nov 18, 2018
  142. sipa force-pushed on Nov 20, 2018
  143. DrahtBot removed the label Needs rebase on Nov 20, 2018
  144. DrahtBot added the label Needs rebase on Jan 21, 2019
  145. practicalswift commented at 8:10 am on January 22, 2019: contributor
    Concept ACK @sipa has this been tested on a BE system?
  146. laanwj commented at 12:00 pm on September 30, 2019: member

    This PR is really old by now, even though there was no disagreement. What is holding this up?

    IMO. we should try to merge this after the 0.19 split-off in a few days.

  147. practicalswift commented at 12:43 pm on September 30, 2019: contributor
    Has this been properly fuzz tested?
  148. MarcoFalke commented at 12:48 pm on September 30, 2019: member
    @practicalswift Pretty much all of our fuzzers are deserialization fuzzers
  149. practicalswift commented at 1:29 pm on September 30, 2019: contributor
    @MarcoFalke Yes I know that :) I was curious if these suggested serialization improvements had been subject to fuzz testing by the PR author - that is something different and hence my question :)
  150. ryanofsky commented at 2:59 pm on September 30, 2019: member

    @MarcoFalke Yes I know that :) I was curious if these suggested serialization improvements had been subject to fuzz testing by the PR author - that is something different and hence my question :)

    I haven’t paid much attention to fuzz testing, but I did spend a good amount of time reviewing this, and wonder if you could give more detail on the extra testing that would be useful here that goes beyond the normal testing.

    It is probably safe to assume that if the author here went out of their way to do special testing, this would be mentioned in the description.

  151. practicalswift commented at 3:29 pm on September 30, 2019: contributor

    I haven’t paid much attention to fuzz testing, but I did spend a good amount of time reviewing this, and wonder if you could give more detail on the extra testing that would be useful here that goes beyond the normal testing.

    It would be useful to take this new code for a spin with our fuzzing test harnesses to make sure no new obvious issues are introduced.

    This can be done using:

    0$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
1$ make
2$ find src/test/fuzz/ -type f -executable
3[…] then run the relevant fuzzers and observe any anomalies […]

    It is probably safe to assume that if the author here went out of their way to do special testing, this would be mentioned in the description.

    TBH I’m not fuzz testing qualifies as special testing these days - in security critical project like ours it really should be standard procedure for this type of code :)

    I’ve simply assumed that PR authors do fuzz testing routinely when touching “parsing/serialization/untrusted-input-heavy” code. I guess I’m mistaken :)

  152. jb55 commented at 3:41 pm on September 30, 2019: member

    @practicalswift Has this been properly fuzz tested?

    both me and @Christewart ran the serialization property-based tests against this at one point. Would be good to revisit.

  153. practicalswift commented at 4:07 pm on September 30, 2019: contributor
    @jb55 Great to hear! I’ll try to break it by fuzzing once rebased :)
  154. sipa commented at 3:34 pm on October 3, 2019: member
    I’ll gladly rebase again next week, or try to cherry-pick pieces of it. I haven’t done any fuzz testing specifically.
  155. sipa force-pushed on Oct 10, 2019
  156. sipa commented at 9:40 pm on October 10, 2019: member
    Rebased.
  157. DrahtBot removed the label Needs rebase on Oct 10, 2019
  158. sipa force-pushed on Oct 11, 2019
  159. DrahtBot added the label Needs rebase on Oct 26, 2019
  160. sipa force-pushed on Oct 26, 2019
  161. sipa commented at 6:29 pm on October 26, 2019: member
    Rebased.
  162. DrahtBot removed the label Needs rebase on Oct 26, 2019
  163. DrahtBot added the label Needs rebase on Oct 29, 2019
  164. sipa force-pushed on Oct 29, 2019
  165. sipa commented at 10:30 pm on October 29, 2019: member
    Rebased.
  166. DrahtBot removed the label Needs rebase on Oct 29, 2019
  167. laanwj added this to the "Blockers" column in a project
  168. DrahtBot added the label Needs rebase on Nov 21, 2019
  169. sipa force-pushed on Nov 21, 2019
  170. sipa commented at 8:05 pm on November 21, 2019: member
    Rebased.
  171. DrahtBot removed the label Needs rebase on Nov 21, 2019
  172. MarcoFalke commented at 2:30 am on November 22, 2019: member
    Is this going to be split into smaller bites?
  173. sipa commented at 2:37 am on November 22, 2019: member

    I certainly can, though I think all the standalone improvements have already been split off and merged independently.

    What remains is introducing new serialization macros, converting all serializers to use them, and then deleting the old ones. I’m happy to split those up into separate PRs if you think that’s useful.

  174. MarcoFalke commented at 2:41 am on November 22, 2019: member
    Fair enough. I was asking for the general public
  175. sipa force-pushed on Nov 25, 2019
  176. sipa commented at 5:17 pm on November 25, 2019: member
    Rebased on top of (a rebased version of) #17591, to test on a BE platform.
  177. in src/qt/sendcoinsrecipient.h:60 in a4dcab62f0 outdated 
    56@@ -33,8 +57,8 @@ class SendCoinsRecipient
57     CAmount amount;
58     // If from a payment request, this is used for storing the memo
59     QString message;
60-    // Keep the payment request around as a serialized string to ensure
61-    // load/store is lossless.
62+    // If building with BIP70 is disabled, keep the payment request around as
    laanwj commented at 2:23 pm on November 26, 2019:
    This is no longer an “if” :smiley:
    sipa commented at 4:16 pm on November 26, 2019:
    Oops, accident revert added in a rebase. Fixed.
  178. sipa force-pushed on Nov 26, 2019
  179. sipa commented at 4:17 pm on November 26, 2019: member
    Now also rebased on top of #17599 to get functional tests run on BE.
  180. practicalswift commented at 4:32 pm on November 26, 2019: contributor

    Could bring in the new improved deserialization fuzz harness from #17225 to fuzz test this? The new fuzz harness does roundtrip testing where possible and covers more types. Would be nice to see this new code fuzz tested :)

    Fuzzing in three easy steps:

    0$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
1$ make
2$ find src/test/fuzz/ -type f -executable
3[…] then run the relevant fuzzers and observe any anomalies […]
  181. DrahtBot added the label Needs rebase on Dec 6, 2019
  182. sipa force-pushed on Dec 12, 2019
  183. sipa commented at 0:50 am on December 12, 2019: member
    Rebased, and fixes a bug in the deserialization fuzzing code.
  184. DrahtBot removed the label Needs rebase on Dec 12, 2019
  185. practicalswift commented at 9:13 am on December 12, 2019: contributor

    This needs to be addressed to make the sanitizers happy :)

    0blockencodings.h:83:34: runtime error: implicit conversion from type 'int' of 
1    value 65536 (32-bit, signed) to type 'Differential::Wrapper<std::vector<unsigned short, std::allocator<unsigned short> > >::value_type'
2    (aka 'unsigned short') changed the value to 0 (16-bit, unsigned)
  186. Introduce new serialization macros without casts 
    This new approach uses a static method which takes the object as
a argument. This has the advantage that its constness can be a
template parameter, allowing a single implementation that sees the
object as const for serialization and non-const for deserialization,
without casts.

More boilerplate is included in the new macro as well.
    ec13b4b3ee
  187. Add Wrap helper 1172a90823
  188. Generalize CompactSize wrapper 
    This makes it const-correct and usable for other integer types.
    4033cf76db
  189. Generalize VarInt wrappers 3ab37ade27
  190. Generalize LimitedString wrapper 4a74724256
  191. Generalize BigEndian serialization wrapper 12a6df7c59
  192. Convert wrapper 4c91d6b6f7
  193. Add a constant for the maximum vector allocation (5 Mbyte) 880151f7ab
  194. Add custom vector-element serialization wrapper 
    This allows a very compact notation for serialization of vectors whose
elements are not serialized using their default encoding.
    a268f6b8f9
  195. Convert primitives to new serialization a5f5e86798
  196. Convert addrdb/addrman to new serialization 56dcf6926f
  197. Convert blockencodings to new serialization fb47327a8a
  198. Convert merkleblock/bloom to new serialization c16c306c97
  199. Convert chain to new serialization 5906323ee7
  200. Convert feerate to new serialization 83ae3eaad4
  201. Convert protocol/net to new serialization bef9d84c98
  202. Convert compressor/txdb/coins/undo/script to new serialization 82d4504dd8
  203. Convert Qt to new serialization a4687632d5
  204. Convert rest to new serialization a9b5fd17c1
  205. Convert dbwrapper tests to new serialization db380b4dd8
  206. Convert serialize_tests/streams_tests to new serialization 085e60a5f0
  207. Convert wallet/walletdb/crypter to new serialization 06ff72c7ab
  208. Convert netaddress to new serialization 566e0ec1f7
  209. Convert prevector tests to new serialization 13d0024335
  210. Remove old serialization primitives a134037a19
  211. sipa force-pushed on Jan 2, 2020
  212. fanquake removed this from the "Blockers" column in a project
  213. laanwj referenced this in commit 593f5e239f on Jan 4, 2020
  214. sidhujag referenced this in commit ebf4e95996 on Jan 4, 2020
  215. sipa commented at 7:15 pm on January 7, 2020: member

    I’ve added a new commit with a few style improvements, and a new approach for writing wrapper classes, as well a different way of doing the differential encoding of compact block indexes.

    Unless people object, I’ll use these modified approaches in follow-up PRs that carve out pieces out of this PR.

  216. sipa force-pushed on Jan 7, 2020
  217. ryanofsky commented at 9:13 pm on January 7, 2020: member

    Just reviewed commit “Switch to different wrapping model + different differential vector” (7da03b3ce5f4be94555b15bf4668bc251a8faac4) and I think it is a nice improvement. Getting rid of the various Format::Wrapper inner classes and pulling them into a standalone Wrapper class eliminates redundant code. And another really nice part of the change is replacing the Differential struct that had lots of operator overloads with simpler value function transform objects.

    I did have some ideas and suggestions from looking at this, but feel free to take or leave them:

    1. “Wrapper” terminology doesn’t seem helpful when it isn’t specifically referring to the actual wrapper class. Would maybe refer to the various classes that have Ser and Unser methods as formatters or helpers instead of wrappers since they don’t really wrap anything. Similarly might rename Wrap function to SerializeAs or FormatAs and rename W template arguments to F or H.
    2. I don’t think there’s a reason to require Ser and Unser methods to be static. It should be no problem to just call W()::Ser() instead of W::Ser() inside the new Wrapper class, and not requiring the methods to be static could be beneficial if the formatter/helper objects are used other contexts in the future. For example if a compression formatter was used to serialize many elements in a data structure, maybe it could save some state to avoid repeating work.
  218. sipa commented at 1:45 pm on January 8, 2020: member
    @ryanofsky Thanks, I’ll see how hard it is to address the non-static methods suggestion. What so you think of the name “Using” instead of “Wrap” ?
  219. laanwj added this to the "Blockers" column in a project
  220. sipa force-pushed on Jan 8, 2020
  221. sipa commented at 2:57 pm on January 8, 2020: member
    Changed to terminology “formatters”, with function “Using”, and made the Ser/Unser functions (permitted to be) non-static.
  222. ryanofsky commented at 2:59 pm on January 8, 2020: member

    @ryanofsky Thanks, I’ll see how hard it is to address the non-static methods suggestion. What so you think of the name “Using” instead of “Wrap” ?

    I think it’d be good, assuming it’s not too much trouble to rename. I also realized the names I suggested yesterday SerializeAs or FormatAs don’t really make sense given how the template argument is used. More accurate would be SerializeWith, FormatWith, or SerializeUsing. No strong opinions about any of this, though.

  223. sipa force-pushed on Jan 9, 2020
  224. laanwj commented at 11:31 am on January 13, 2020: member

    Travis error:

    0blockencodings.h:68:17: runtime error: implicit conversion from type 'int' of value 65536 (32-bit, signed) to type 'unsigned short' changed the value to 0 (16-bit, unsigned)
1
2    [#0](/bitcoin-bitcoin/0/) 0x561bb3939927 in DifferentialUnserTransform<unsigned short>::operator()(unsigned short) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./blockencodings.h:68:17
3    [#1](/bitcoin-bitcoin/1/) 0x561bb3938b15 in void VectorUsing<CompactSizeFormatter, DifferentialSerTransform<unsigned short>, DifferentialUnserTransform<unsigned short> >::Unser<CDataStream, std::vector<unsigned short, std::allocator<unsigned short> > >(CDataStream&, std::vector<unsigned short, std::allocator<unsigned short> >&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:625:29
4    [#2](/bitcoin-bitcoin/2/) 0x561bb39387c3 in void Wrapper<VectorUsing<CompactSizeFormatter, DifferentialSerTransform<unsigned short>, DifferentialUnserTransform<unsigned short> >, std::vector<unsigned short, std::allocator<unsigned short> > >::Unserialize<CDataStream>(CDataStream&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:444:73
5    [#3](/bitcoin-bitcoin/3/) 0x561bb39385bb in void Unserialize<CDataStream, Wrapper<VectorUsing<CompactSizeFormatter, DifferentialSerTransform<unsigned short>, DifferentialUnserTransform<unsigned short> >, std::vector<unsigned short, std::allocator<unsigned short> > >&>(CDataStream&, Wrapper<VectorUsing<CompactSizeFormatter, DifferentialSerTransform<unsigned short>, DifferentialUnserTransform<unsigned short> >, std::vector<unsigned short, std::allocator<unsigned short> > >&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:708:7
  225. laanwj removed this from the "Blockers" column in a project
  226. Switch to different wrapping model + different differential vector f4ca9782d6
  227. sipa force-pushed on Jan 13, 2020
  228. fanquake referenced this in commit a654626f07 on Jan 18, 2020
  229. DrahtBot added the label Needs rebase on Jan 18, 2020
  230. DrahtBot commented at 2:49 am on January 18, 2020: member
  231. sidhujag referenced this in commit fc3573c422 on Jan 18, 2020
  232. laanwj referenced this in commit c1607b5df4 on Jan 29, 2020
  233. sidhujag referenced this in commit bd1262fde3 on Feb 1, 2020
  234. laanwj referenced this in commit 4c2578706c on Feb 10, 2020
  235. sipa commented at 4:03 am on February 15, 2020: member
    Too many changes have been introduced compared to this PR in #17850, #17896, #17957, #18021, and #18112, and I’m not going to rebase this on top of them. I’ll keep picking changes from the branch, though, but no need to keep this open.
  236. sipa closed this on Feb 15, 2020
  237. sidhujag referenced this in commit ee101ba39b on Feb 18, 2020
  238. laanwj referenced this in commit 727857d12d on Mar 5, 2020
  239. sidhujag referenced this in commit 69e8548a31 on Mar 5, 2020
  240. fanquake removed the label Needs rebase on May 19, 2020
  241. MarcoFalke referenced this in commit 448bdff263 on May 20, 2020
  242. sidhujag referenced this in commit 90eb169cc8 on May 20, 2020
  243. laanwj referenced this in commit dcacea096e on May 26, 2020
  244. sidhujag referenced this in commit 22576c8382 on May 27, 2020
  245. PastaPastaPasta referenced this in commit 51a0d0bb09 on Jun 9, 2020
  246. PastaPastaPasta referenced this in commit b33a9b94e2 on Jun 9, 2020
  247. PastaPastaPasta referenced this in commit 85e1502e9d on Jun 10, 2020
  248. PastaPastaPasta referenced this in commit 4faeba9963 on Jun 11, 2020
  249. PastaPastaPasta referenced this in commit 98ee5d6161 on Jun 11, 2020
  250. PastaPastaPasta referenced this in commit 0b5142bef8 on Jun 11, 2020
  251. PastaPastaPasta referenced this in commit 1634d58421 on Jun 12, 2020
  252. PastaPastaPasta referenced this in commit abbf299a05 on Sep 27, 2020
  253. PastaPastaPasta referenced this in commit ca268bd678 on Oct 22, 2020
  254. sidhujag referenced this in commit 60bda9d425 on Nov 10, 2020
  255. sidhujag referenced this in commit a5a456f9a2 on Nov 10, 2020
  256. sidhujag referenced this in commit 502bae9ef1 on Nov 10, 2020
  257. sidhujag referenced this in commit fc5b6b9daf on Nov 10, 2020
  258. sidhujag referenced this in commit 4c7ebce962 on Nov 10, 2020
  259. PastaPastaPasta referenced this in commit 9a6cf658f5 on Dec 16, 2020
  260. UdjinM6 referenced this in commit ec71097801 on Dec 17, 2020
  261. UdjinM6 referenced this in commit f8ac5a555b on Dec 17, 2020
  262. UdjinM6 referenced this in commit 20b71700dc on May 14, 2021
  263. UdjinM6 referenced this in commit 6a36c4e4b1 on May 28, 2021
  264. UdjinM6 referenced this in commit 9029448233 on May 28, 2021
  265. gades referenced this in commit cbabeb575a on Jun 24, 2021
  266. gades referenced this in commit 66635d0457 on Feb 5, 2022
  267. DeckerSU referenced this in commit eb3ca0556a on Feb 12, 2022
  268. DrahtBot locked this on Feb 15, 2022


sipa gmaxwell jonasschnelli promag ryanofsky laanwj MarcoFalke Christewart jb55 DrahtBot practicalswift


jonasschnelli

Labels
Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me