Serialization improvements #10785

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.

gmaxwell commented at 6:32 PM on July 10, 2017: contributor

Should probably be tested on big endian. :)

sipa force-pushed on Jul 10, 2017

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.

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.

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?).

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?).

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

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).

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.

laanwj requested review from jonasschnelli on Jul 11, 2017

laanwj added the label Utils and libraries on Jul 11, 2017

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.

sipa force-pushed on Jul 15, 2017

sipa force-pushed on Jul 29, 2017

sipa force-pushed on Jul 30, 2017

sipa commented at 12:20 AM on July 30, 2017: member

Made some changes to reduce the size of the overall diff.

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.

promag commented at 8:38 AM on August 6, 2017: member

ACK b0652ac. Everything works as expected.

Indeed a lot in one shot :).

sipa force-pushed on Aug 15, 2017

sipa commented at 9:38 PM on August 15, 2017: member

Rebased.

laanwj added this to the "Blockers" column in a project

sipa force-pushed on Aug 25, 2017

sipa commented at 7:02 PM on August 25, 2017: member

Rebased.

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::value

sipa commented at 3:36 AM on September 1, 2017:

Fixed.

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.

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.

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, 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.

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::max()

sipa commented at 3:34 AM on September 1, 2017:

Fixed.

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.

ryanofsky commented at 9:52 PM on August 31, 2017: member

Reviewed first several commits. Will continue reviewing.

sipa force-pushed on Sep 1, 2017

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::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.

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::max() < numeric_limits<int64_t>::max() with static assert.

sipa commented at 5:53 PM on September 1, 2017:

Done.

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.

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.

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.

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.

sipa force-pushed on Sep 1, 2017

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 12:19 AM on September 2, 2017:

Done!

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 12:19 AM on September 2, 2017:

Done.

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).

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:

template<typename I> static inline VarIntWrapper<I> VARINT(I& i) { return VarIntWrapper<I>(i); }
template<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 12: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.

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.

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 12:19 AM on September 2, 2017:

Done.

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 12:19 AM on September 2, 2017:

Done.

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.

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,

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.

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:

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

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.:

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

Which you'd then invoke using

     SERIALIZE_METHODS(SomeType, obj)
     {
          READWRITE(obj.member);
          IF_UNSERIALIZE(SomeType, obj, {obj.FillShortTxid();});
     }

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:

     SERIALIZE_METHODS(SomeType)
     {
          READWRITE(obj.member);
          IF_UNSERIALIZE({obj.FillShortTxid();});
     }

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.

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.

sipa force-pushed on Sep 2, 2017

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.

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.

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::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.

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."

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.

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.

sipa removed this from the "Blockers" column in a project

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.

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.

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 12:02 AM on September 6, 2017:

Could use std::move here.

sipa commented at 7:09 PM on September 7, 2017:

Done.

ryanofsky commented at 12:12 AM on September 6, 2017: member

utACK b49e84ea6dd7e2c0aacb8546c8d8c2d99d1d8214. Finally reviewed the full change.

sipa force-pushed on Sep 6, 2017

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.

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.

sipa force-pushed on Sep 6, 2017

sipa force-pushed on Sep 7, 2017

sipa force-pushed on Sep 8, 2017

sipa force-pushed on Oct 20, 2017

sipa commented at 11:27 AM on October 20, 2017: member

Rebased.

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.

MarcoFalke commented at 9:44 PM on October 31, 2017: member

Travis can not run the qt tests. Are they passing for you locally?

ryanofsky commented at 9:50 PM on October 31, 2017: member


QFATAL : WalletTests::walletTests() Received signal 11
FAIL!  : WalletTests::walletTests() Received a fatal error.
   Loc: [Unknown file(0)]

EDIT: Actually they are failing for me locally too

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.

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:

(gdb) bt -50
[#2355058](/bitcoin-bitcoin/2355058/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355059](/bitcoin-bitcoin/2355059/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355060](/bitcoin-bitcoin/2355060/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355061](/bitcoin-bitcoin/2355061/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355062](/bitcoin-bitcoin/2355062/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355063](/bitcoin-bitcoin/2355063/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355064](/bitcoin-bitcoin/2355064/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355065](/bitcoin-bitcoin/2355065/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355066](/bitcoin-bitcoin/2355066/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355067](/bitcoin-bitcoin/2355067/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355068](/bitcoin-bitcoin/2355068/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355069](/bitcoin-bitcoin/2355069/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355070](/bitcoin-bitcoin/2355070/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355071](/bitcoin-bitcoin/2355071/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355072](/bitcoin-bitcoin/2355072/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355073](/bitcoin-bitcoin/2355073/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355074](/bitcoin-bitcoin/2355074/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355075](/bitcoin-bitcoin/2355075/) 0x00000001000dbe89 in GUIUtil::AsStdString<QString const>(QString const&) (qstring=...) at qt/guiutil.h:285
[#2355076](/bitcoin-bitcoin/2355076/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (qstring=...) at qt/guiutil.h:285
[#2355077](/bitcoin-bitcoin/2355077/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (qstring=...) at qt/guiutil.h:285
[#2355078](/bitcoin-bitcoin/2355078/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (ser_action=..., s=..., obj=...) at qt/walletmodel.h:69
[#2355079](/bitcoin-bitcoin/2355079/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (s=..., this=0x7fffffffb728) at qt/walletmodel.h:66
[#2355080](/bitcoin-bitcoin/2355080/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (a=..., os=...) at ./serialize.h:677
[#2355081](/bitcoin-bitcoin/2355081/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=..., s=...) at ./serialize.h:1030
[#2355082](/bitcoin-bitcoin/2355082/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=..., s=...) at ./serialize.h:1031
[#2355083](/bitcoin-bitcoin/2355083/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=@0x7fffffffb718: 1, s=...) at ./serialize.h:1031
[#2355084](/bitcoin-bitcoin/2355084/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (arg=@0x7fffffffb710: 1, s=...) at ./serialize.h:1031
[#2355085](/bitcoin-bitcoin/2355085/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (ser_action=..., s=...) at ./serialize.h:1049
[#2355086](/bitcoin-bitcoin/2355086/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (s=..., obj=..., ser_action=...) at qt/recentrequeststablemodel.h:30
[#2355087](/bitcoin-bitcoin/2355087/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (s=..., this=0x7fffffffb710) at qt/recentrequeststablemodel.h:28
[#2355088](/bitcoin-bitcoin/2355088/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (a=..., os=...) at ./serialize.h:677
[#2355089](/bitcoin-bitcoin/2355089/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (obj=..., this=0x7fffffffb600) at ./streams.h:399
[#2355090](/bitcoin-bitcoin/2355090/) 0x00000001000da01d in RecentRequestsTableModel::addNewRequest(SendCoinsRecipient const&) (this=0x1014511d0, recipient=...) at qt/recentrequeststablemodel.cpp:173
[#2355091](/bitcoin-bitcoin/2355091/) 0x00000001000d2a92 in ReceiveCoinsDialog::on_receiveButton_clicked() (this=0x7fffffffbf50) at qt/receivecoinsdialog.cpp:167
[#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>)
    at qt/moc_receivecoinsdialog.cpp:123
[#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
[#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
[#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
[#2355096](/bitcoin-bitcoin/2355096/) 0x00007ffff6f112b2 in QAbstractButton::clicked(bool) (this=this@entry=0x10163e380, _t1=false) at .moc/moc_qabstractbutton.cpp:303
[#2355097](/bitcoin-bitcoin/2355097/) 0x00007ffff6c73f44 in QAbstractButtonPrivate::emitClicked() (this=0x1016cc1d0) at widgets/qabstractbutton.cpp:534
[#2355098](/bitcoin-bitcoin/2355098/) 0x00007ffff6c745e1 in QAbstractButton::click() (this=0x10163e380) at widgets/qabstractbutton.cpp:992
[#2355099](/bitcoin-bitcoin/2355099/) 0x0000000100086686 in (anonymous namespace)::TestGUI() () at qt/test/wallettests.cpp:222
[#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, 
    connectionType@entry=4294954784, returnValue=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...) at kernel/qmetaobject.cpp:2212
[#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, 
    type@entry=Qt::DirectConnection, ret=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...) at kernel/qmetaobject.cpp:1479
[#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
[#2355103](/bitcoin-bitcoin/2355103/) 0x00007ffff7fa408c in QTest::qInvokeTestMethod(char const*, char const*) (slot=0x100ca1990 "walletTests") at qtestcase.cpp:1953
[#2355104](/bitcoin-bitcoin/2355104/) 0x00007ffff7fa408c in QTest::qInvokeTestMethod(char const*, char const*) (slotName=0x100948698 "walletTests()", data=data@entry=0x0) at qtestcase.cpp:2082
[#2355105](/bitcoin-bitcoin/2355105/) 0x00007ffff7fa4bb8 in QTest::qExec(QObject*, int, char**) (testObject=0x7fffffffd830) at qtestcase.cpp:2390
[#2355106](/bitcoin-bitcoin/2355106/) 0x00007ffff7fa4bb8 in QTest::qExec(QObject*, int, char**) (testObject=0x7fffffffd830, argc=<optimized out>, argv=<optimized out>) at qtestcase.cpp:2652
[#2355107](/bitcoin-bitcoin/2355107/) 0x000000010005d9df in main(int, char**) (argc=1, argv=0x7fffffffdd78) at qt/test/test_main.cpp:99

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!

sipa force-pushed on Nov 2, 2017

sipa commented at 5:13 PM on November 2, 2017: member

Rebased.

sipa force-pushed on Nov 8, 2017

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.

sipa force-pushed on Nov 10, 2017

sipa force-pushed on Nov 21, 2017

sipa commented at 11:02 AM on November 21, 2017: member

Rebased.

sipa force-pushed on Feb 8, 2018

sipa commented at 7:29 PM on February 8, 2018: member

Rebased.

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

sipa force-pushed on Mar 7, 2018

laanwj referenced this in commit af88094e4f on Mar 13, 2018

sipa force-pushed on Mar 13, 2018

sipa commented at 6:27 PM on March 13, 2018: member

Rebased.

sipa referenced this in commit 7be9a9a570 on Mar 15, 2018

sipa force-pushed on Mar 16, 2018

sipa force-pushed on Mar 17, 2018

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.

sipa force-pushed on Mar 20, 2018

sipa force-pushed on Mar 21, 2018

laanwj referenced this in commit d2d7267e23 on Mar 30, 2018

laanwj referenced this in commit 8203c4c42e on Mar 30, 2018

sipa force-pushed on Mar 30, 2018

sipa force-pushed on Apr 7, 2018

sipa force-pushed on Apr 8, 2018

sipa force-pushed on Apr 10, 2018

laanwj referenced this in commit 7b6041d1a7 on Apr 11, 2018

sipa force-pushed on Apr 17, 2018

laanwj commented at 2:00 PM on May 14, 2018: member

Concept ACK

sipa force-pushed on May 14, 2018

sipa commented at 12: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.

MarcoFalke commented at 3:31 PM on May 21, 2018: member

Needs rebase due to merge of #12924

sipa force-pushed on May 22, 2018

DrahtBot added the label Needs rebase on Jun 7, 2018

sipa force-pushed on Jun 12, 2018

DrahtBot removed the label Needs rebase on Jun 12, 2018

DrahtBot added the label Needs rebase on Jul 25, 2018

sipa force-pushed on Jul 25, 2018

DrahtBot removed the label Needs rebase on Jul 25, 2018

MarcoFalke commented at 8:59 PM on July 25, 2018: member

Travis output:

A wild circular dependency in the form of "qt/guiutil -> qt/walletmodel -> qt/guiutil" appears...

sipa force-pushed on Jul 25, 2018

DrahtBot added the label Needs rebase on Aug 30, 2018

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.

sipa force-pushed on Nov 15, 2018

sipa commented at 8:13 PM on November 15, 2018: member

Rebased.

DrahtBot removed the label Needs rebase on Nov 15, 2018

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.

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.

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?

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? :-)

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?

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?

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?

DrahtBot added the label Needs rebase on Nov 18, 2018

sipa force-pushed on Nov 20, 2018

DrahtBot removed the label Needs rebase on Nov 20, 2018

DrahtBot added the label Needs rebase on Jan 21, 2019

practicalswift commented at 8:10 AM on January 22, 2019: contributor

Concept ACK @sipa has this been tested on a BE system?

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.

practicalswift commented at 12:43 PM on September 30, 2019: contributor

Has this been properly fuzz tested?

MarcoFalke commented at 12:48 PM on September 30, 2019: member

@practicalswift Pretty much all of our fuzzers are deserialization fuzzers

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 :)

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.

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:

$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ find src/test/fuzz/ -type f -executable
[…] 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 :)

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.

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 :)

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.

sipa force-pushed on Oct 10, 2019

sipa commented at 9:40 PM on October 10, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Oct 10, 2019

sipa force-pushed on Oct 11, 2019

DrahtBot added the label Needs rebase on Oct 26, 2019

sipa force-pushed on Oct 26, 2019

sipa commented at 6:29 PM on October 26, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Oct 26, 2019

DrahtBot added the label Needs rebase on Oct 29, 2019

sipa force-pushed on Oct 29, 2019

sipa commented at 10:30 PM on October 29, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Oct 29, 2019

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Nov 21, 2019

sipa force-pushed on Nov 21, 2019

sipa commented at 8:05 PM on November 21, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Nov 21, 2019

MarcoFalke commented at 2:30 AM on November 22, 2019: member

Is this going to be split into smaller bites?

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.

MarcoFalke commented at 2:41 AM on November 22, 2019: member

Fair enough. I was asking for the general public

sipa force-pushed on Nov 25, 2019

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.

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.

sipa force-pushed on Nov 26, 2019

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.

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:

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

DrahtBot added the label Needs rebase on Dec 6, 2019

sipa force-pushed on Dec 12, 2019

sipa commented at 12:50 AM on December 12, 2019: member

Rebased, and fixes a bug in the deserialization fuzzing code.

DrahtBot removed the label Needs rebase on Dec 12, 2019

practicalswift commented at 9:13 AM on December 12, 2019: contributor

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

blockencodings.h:83:34: runtime error: implicit conversion from type 'int' of 
    value 65536 (32-bit, signed) to type 'Differential::Wrapper<std::vector<unsigned short, std::allocator<unsigned short> > >::value_type'
    (aka 'unsigned short') changed the value to 0 (16-bit, unsigned)

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

Add Wrap helper 1172a90823

Generalize CompactSize wrapper

This makes it const-correct and usable for other integer types.

4033cf76db

Generalize VarInt wrappers 3ab37ade27

Generalize LimitedString wrapper 4a74724256

Generalize BigEndian serialization wrapper 12a6df7c59

Convert wrapper 4c91d6b6f7

Add a constant for the maximum vector allocation (5 Mbyte) 880151f7ab

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

Convert primitives to new serialization a5f5e86798

Convert addrdb/addrman to new serialization 56dcf6926f

Convert blockencodings to new serialization fb47327a8a

Convert merkleblock/bloom to new serialization c16c306c97

Convert chain to new serialization 5906323ee7

Convert feerate to new serialization 83ae3eaad4

Convert protocol/net to new serialization bef9d84c98

Convert compressor/txdb/coins/undo/script to new serialization 82d4504dd8

Convert Qt to new serialization a4687632d5

Convert rest to new serialization a9b5fd17c1

Convert dbwrapper tests to new serialization db380b4dd8

Convert serialize_tests/streams_tests to new serialization 085e60a5f0

Convert wallet/walletdb/crypter to new serialization 06ff72c7ab

Convert netaddress to new serialization 566e0ec1f7

Convert prevector tests to new serialization 13d0024335

Remove old serialization primitives a134037a19

sipa force-pushed on Jan 2, 2020

fanquake removed this from the "Blockers" column in a project

laanwj referenced this in commit 593f5e239f on Jan 4, 2020

sidhujag referenced this in commit ebf4e95996 on Jan 4, 2020

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.

sipa force-pushed on Jan 7, 2020

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:

"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.
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.

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" ?

laanwj added this to the "Blockers" column in a project

sipa force-pushed on Jan 8, 2020

sipa commented at 2:57 PM on January 8, 2020: member

Changed to terminology "formatters", with function "Using<Formatter>", and made the Ser/Unser functions (permitted to be) non-static.

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.

sipa force-pushed on Jan 9, 2020

laanwj commented at 11:31 AM on January 13, 2020: member

Travis error:

blockencodings.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)

    [#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
    [#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
    [#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
    [#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

laanwj removed this from the "Blockers" column in a project

Switch to different wrapping model + different differential vector f4ca9782d6

sipa force-pushed on Jan 13, 2020

fanquake referenced this in commit a654626f07 on Jan 18, 2020

DrahtBot added the label Needs rebase on Jan 18, 2020

DrahtBot commented at 2:49 AM on January 18, 2020: member

Needs rebase

sidhujag referenced this in commit fc3573c422 on Jan 18, 2020

laanwj referenced this in commit c1607b5df4 on Jan 29, 2020

sidhujag referenced this in commit bd1262fde3 on Feb 1, 2020

laanwj referenced this in commit 4c2578706c on Feb 10, 2020

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.

sipa closed this on Feb 15, 2020

sidhujag referenced this in commit ee101ba39b on Feb 18, 2020

laanwj referenced this in commit 727857d12d on Mar 5, 2020

sidhujag referenced this in commit 69e8548a31 on Mar 5, 2020

fanquake removed the label Needs rebase on May 19, 2020

MarcoFalke referenced this in commit 448bdff263 on May 20, 2020

sidhujag referenced this in commit 90eb169cc8 on May 20, 2020

laanwj referenced this in commit dcacea096e on May 26, 2020

sidhujag referenced this in commit 22576c8382 on May 27, 2020

PastaPastaPasta referenced this in commit 51a0d0bb09 on Jun 9, 2020

PastaPastaPasta referenced this in commit b33a9b94e2 on Jun 9, 2020

PastaPastaPasta referenced this in commit 85e1502e9d on Jun 10, 2020

PastaPastaPasta referenced this in commit 4faeba9963 on Jun 11, 2020

PastaPastaPasta referenced this in commit 98ee5d6161 on Jun 11, 2020

PastaPastaPasta referenced this in commit 0b5142bef8 on Jun 11, 2020

PastaPastaPasta referenced this in commit 1634d58421 on Jun 12, 2020

PastaPastaPasta referenced this in commit abbf299a05 on Sep 27, 2020

PastaPastaPasta referenced this in commit ca268bd678 on Oct 22, 2020

sidhujag referenced this in commit 60bda9d425 on Nov 10, 2020

sidhujag referenced this in commit a5a456f9a2 on Nov 10, 2020

sidhujag referenced this in commit 502bae9ef1 on Nov 10, 2020

sidhujag referenced this in commit fc5b6b9daf on Nov 10, 2020

sidhujag referenced this in commit 4c7ebce962 on Nov 10, 2020

PastaPastaPasta referenced this in commit 9a6cf658f5 on Dec 16, 2020

UdjinM6 referenced this in commit ec71097801 on Dec 17, 2020

UdjinM6 referenced this in commit f8ac5a555b on Dec 17, 2020

UdjinM6 referenced this in commit 20b71700dc on May 14, 2021

UdjinM6 referenced this in commit 6a36c4e4b1 on May 28, 2021

UdjinM6 referenced this in commit 9029448233 on May 28, 2021

gades referenced this in commit cbabeb575a on Jun 24, 2021

gades referenced this in commit 66635d0457 on Feb 5, 2022

DeckerSU referenced this in commit eb3ca0556a on Feb 12, 2022

DrahtBot locked this on Feb 15, 2022