Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization improvements #10785

Open
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
@sipa
Copy link
Member

sipa commented Jul 10, 2017

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

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 10, 2017

Should probably be tested on big endian. :)

@sipa sipa force-pushed the sipa:20170707_noncastserial branch 2 times, most recently Jul 10, 2017

@sipa
Copy link
Member Author

sipa left a comment

Here is a self-review in which I point out some of the things reviewers may want to be aware of.

src/blockencodings.h Outdated
void Unserialize(Stream& s)
{
std::vector<uint64_t> tmp;
s >> blockhash >> VectorApply<CompactSizeWrapper>(tmp);

This comment has been minimized.

@sipa

sipa Jul 10, 2017

Author Member

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.

src/test/serialize_tests.cpp Outdated
READWRITEMANY(intval, boolval, stringval, FLATDATA(charstrval), txval);
SERIALIZE_METHODS(obj)
{
READWRITE(obj.intval, obj.boolval, obj.stringval, FlatData(obj.charstrval), obj.txval);

This comment has been minimized.

@sipa

sipa Jul 10, 2017

Author Member

This whole test is somewhat less valuable now, as both cases use READWRITE.

src/wallet/wallet.h Outdated
inline void SerializationOp(Stream& s, Operation ser_action) {
if (ser_action.ForRead())
Init(NULL);
template<typename Stream>

This comment has been minimized.

@sipa

sipa Jul 10, 2017

Author Member

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

src/wallet/wallet.h Outdated

ReadOrderPos(nOrderPos, mapValue);
template<typename Stream>

This comment has been minimized.

@sipa

sipa Jul 10, 2017

Author Member

Here is another big change, that avoids modifying mapValue and strAccount and then later fixing it up before returning (wtf?).

src/serialize.h Outdated
* V is not required to be an std::vector type. It works for any class that
* exposes a value_type, iteration, and resize method that behave like vectors.
*/
template<template <typename> class W, typename V> class VectorApplyWrapper

This comment has been minimized.

@sipa

sipa Jul 10, 2017

Author Member

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

src/serialize.h Outdated
{
::Serialize(s, std::forward<Arg>(arg));
::Serialize(s, arg);

This comment has been minimized.

@sipa

sipa Jul 10, 2017

Author Member

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

@laanwj laanwj requested a review from jonasschnelli Jul 11, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jul 11, 2017

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 sipa force-pushed the sipa:20170707_noncastserial branch Jul 15, 2017

@sipa sipa force-pushed the sipa:20170707_noncastserial branch 2 times, most recently Jul 29, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jul 30, 2017

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

@promag
Copy link
Member

promag left a comment

ACK b0652ac. Everything works as expected.

Indeed a lot in one shot :).

src/serialize.h Outdated
}
};
//! Add a LimitedString wrapper around a const string (identity)
template<size_t I> static inline const std::string& LimitedString(const std::string& str) { return str; }
//! Add a LimitedString wrapper around a non-const string

This comment has been minimized.

@promag

promag Aug 6, 2017

Member

Missing periods in comments.

@sipa sipa force-pushed the sipa:20170707_noncastserial branch Aug 15, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Aug 15, 2017

Rebased.

@laanwj laanwj added this to Blockers in High-priority for review Aug 17, 2017

@sipa sipa force-pushed the sipa:20170707_noncastserial branch Aug 25, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Aug 25, 2017

Rebased.

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

Reviewed first several commits. Will continue reviewing.

public:
explicit BigEndianWrapper(I& val) : m_val(val)
{
static_assert(S == 2 || S == 4, "Unsupported BigEndian size");

This comment has been minimized.

@ryanofsky

ryanofsky Aug 31, 2017

Contributor

In commit "Add BigEndian serialization wrapper"

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

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Fixed.

* Use this wrapper around integer types that are stored in memory in native
* byte order, but serialized in big endian notation.
*
* Onlyy 16-bit types are supported for now.

This comment has been minimized.

@ryanofsky

ryanofsky Aug 31, 2017

Contributor

In commit "Add BigEndian serialization wrapper"

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

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Fixed.

@@ -493,9 +515,44 @@ class LimitedString
}
};

/** Serialization wrapper class for big-endian integers.
*
* Use this wrapper around integer types that are stored in memory in native

This comment has been minimized.

@ryanofsky

ryanofsky Aug 31, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Done.

void Unserialize(Stream& s) {
n = ReadCompactSize<Stream>(s);
void Serialize(Stream& s) const {
WriteCompactSize<Stream>(s, n);

This comment has been minimized.

@ryanofsky

ryanofsky Aug 31, 2017

Contributor

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

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Fixed.

void Serialize(Stream &s) const {
WriteCompactSize<Stream>(s, n);
void Unserialize(Stream& s) {
n = ReadCompactSize<Stream>(s);

This comment has been minimized.

@ryanofsky

ryanofsky Aug 31, 2017

Contributor

In commit "Generalize CompactSize wrapper"

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

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Fixed.

//! Construct a FlatRange wrapper around a non-const POD and array types.
template<typename T> static inline FlatRangeWrapper<char> FlatDataInner(T* t, size_t len) { return FlatRangeWrapper<char>((char*)t, ((char*)t) + len); }
//! Helper macro to easily serialize POD types.
#define FLATDATA(x) FlatDataInner(&(x), sizeof(x))

This comment has been minimized.

@ryanofsky

ryanofsky Aug 31, 2017

Contributor

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: 6ef78bc. Could you take a look, and maybe incorporate these into the PR?

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

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.

@sipa sipa force-pushed the sipa:20170707_noncastserial branch Sep 1, 2017

src/serialize.h Outdated
template<typename Stream>
void Unserialize(Stream& s)
{
if (S == 2) m_val = ser_readdata16be(s);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Done.

src/serialize.h Outdated
n = ReadCompactSize<Stream>(s);
void Serialize(Stream& s) const
{
WriteCompactSize<Stream>(s, m_n);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Done.

src/netaddress.h Outdated
@@ -132,7 +132,7 @@ class CSubNet
inline void SerializationOp(Stream& s, Operation ser_action) {
READWRITE(network);
READWRITE(FLATDATA(netmask));
READWRITE(FLATDATA(valid));
READWRITE(valid);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Done.

src/serialize.h Outdated
* Wrapper for serializing arrays and POD.
*/
class CFlatData
/** Wrapper for serializing arrays and POD. */

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Fixed.

src/serialize.h Outdated
}
};
//! Construct a FlatRange wrapper around a const char vector.
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())); }

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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)

This comment has been minimized.

@sipa

sipa Sep 1, 2017

Author Member

Done.

@sipa sipa force-pushed the sipa:20170707_noncastserial branch Sep 1, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

More review comments. I made it up to the "Convert blockencodings to new serialization" commit this time.

* code. Adding "ADD_SERIALIZE_METHODS" in the body of the class causes these wrappers to be
* added as members.
*/
#define ADD_SERIALIZE_METHODS \

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

In commit "Remove old serialization primitives"

There are still two references to ADD_SERIALIZE_METHODS in comments.

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done.

src/serialize.h Outdated
* thus allows a single implementation that sees the object as const for serializing
* and non-const for deserializing, without casts.
*/
#define SERIALIZE_METHODS(obj) \

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done!

src/serialize.h Outdated
@@ -148,9 +148,21 @@ enum
SER_GETHASH = (1 << 2),
};

// Convert the reference base type to X, without changing constness or reference type.

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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

Maybe use //!

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done.

src/serialize.h Outdated
}
};
//! Automatically construct a CompactSize wrapper around the argument.
template<typename I> static inline CompactSizeWrapper<I> COMPACTSIZE(I& i) { return CompactSizeWrapper<I>(i); }

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

In commit "Generalize CompactSize wrapper"

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

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done (and for other wrappers).

src/serialize.h Outdated
}
};
//! Automatically construct a VarInt wrapper around the argument.
template<typename I> static inline VarIntWrapper<typename std::remove_reference<I>::type> VARINT(I&& i) { return VarIntWrapper<typename std::remove_reference<I>::type>(i); }

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

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.

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done.

src/serialize.h Outdated
s.read((char*)string.data(), size);
m_string.resize(size);
if (size != 0) {
s.read((char*)m_string.data(), size);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

In commit "Generalize LimitedString wrapper"

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

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done.

src/serialize.h Outdated
unsigned int nSize = ReadCompactSize(s);
unsigned int nMid = 0;
while (nMid < nSize) {
nMid += 5000000 / sizeof(value_type);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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?

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done. The code was also totally broken, so I've rewritten it.

src/serialize.h Outdated
}
m_vector.resize(nMid);
for (value_type& x : m_vector) {
s >> W<value_type>(x);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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?

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Yes, this was bogus. Thanks for pointing that out; fixed by rewriting,

src/blockencodings.h Outdated
}
};

template<typename T>

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

In commit "Convert blockencodings to new serialization"

Maybe move this up closer to TransactionCompressWrapper class

This comment has been minimized.

@sipa

sipa Sep 2, 2017

Author Member

Done.

{
static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids serialization assumes 6-byte shorttxids");
s >> header >> nonce >> VectorApply<Uint48Wrapper>(shorttxids) >> prefilledtxn;
FillShortTxIDSelector();

This comment has been minimized.

@ryanofsky

ryanofsky Sep 1, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 3, 2017

Author Member

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();});
     }

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

Thread #10785 (comment)

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.

This comment has been minimized.

@sipa

sipa Sep 6, 2017

Author Member

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

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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

@sipa sipa force-pushed the sipa:20170707_noncastserial branch 2 times, most recently Sep 2, 2017

@sipa sipa removed this from Blockers in High-priority for review Sep 5, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK b49e84ea6dd7e2c0aacb8546c8d8c2d99d1d8214. Finally reviewed the full change.

src/serialize.h Outdated
{
pbegin = (char*)v.data();
pend = (char*)(v.data() + v.size());
static_assert(sizeof(C) == 1, "CharArrayWrapper only works for char types");

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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.

src/serialize.h Outdated
// For DoS prevention, do not blindly allocate as much as the stream claims to contain.
// Instead, allocate in 5MiB batches, so that an attacker actually needs to provide
// X MiB of data to make us allocate X+5 Mib.
allocated = std::min(size, allocated + 5000000 / sizeof(value_type));

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 6, 2017

Author Member

Done.

src/blockencodings.h Outdated
{
uint32_t lsb = m_int & 0xffffffff;
uint16_t msb = (m_int >> 32) & 0xffff;
s << lsb << msb;

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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

This comment has been minimized.

@sipa

sipa Sep 6, 2017

Author Member

Added static assert; I'd like to avoid runtime impact.

src/blockencodings.h Outdated
uint32_t lsb;
uint16_t msb;
s >> lsb >> msb;
m_int = (uint64_t(msb) << 32) | uint64_t(lsb);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 6, 2017

Author Member

Added static assert.

{
static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids serialization assumes 6-byte shorttxids");
s >> header >> nonce >> VectorApply<Uint48Wrapper>(shorttxids) >> prefilledtxn;
FillShortTxIDSelector();

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

Thread #10785 (comment)

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.

// TODO: avoid reimplementing vector deserializer
uint64_t count = 0;
::Unserialize(s, COMPACTSIZE(count));
if (count > MAX_INPUTS_PER_BLOCK) {

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

Maybe mention in commit message if behavior is changing here. I guess the limit is higher now.

This comment has been minimized.

@sipa

sipa Sep 7, 2017

Author Member

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.

src/qt/recentrequeststablemodel.h Outdated
{
unsigned int nDate;
s >> this->nVersion >> id >> nDate >> recipient;
date = QDateTime::fromTime_t(nDate);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 7, 2017

Author Member

Introduced a wrapper for QDateTime instead.

src/qt/walletmodel.h Outdated
}
authenticatedMerchant = QString::fromStdString(sAuthenticatedMerchant);

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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.

This comment has been minimized.

@sipa

sipa Sep 7, 2017

Author Member

Done using wrappers for QString and proto.

s >> nTime >> vchPubKey;
try {
s >> fInternal;
} catch (std::ios_base::failure&) {

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

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.

src/wallet/wallet.h Outdated
std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent;

strFromAccount = mapValue["fromaccount"];

This comment has been minimized.

@ryanofsky

ryanofsky Sep 6, 2017

Contributor

Could use std::move here.

This comment has been minimized.

@sipa

sipa Sep 7, 2017

Author Member

Done.

@sipa sipa force-pushed the sipa:20170707_noncastserial branch Sep 6, 2017

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK a9770fbe6ef6d7d9bd5b61e4c51531af7cc2a1fc. Still looks good. Only minor changes since previous review, described in comments above.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 6, 2017

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 added some commits Jul 7, 2017

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.
Generalize CompactSize wrapper
This makes it const-correct and usable for other integer types.
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.

@sipa sipa force-pushed the sipa:20170707_noncastserial branch to 9df750c Nov 20, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 20, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 21, 2019

Needs rebase
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Jan 22, 2019

Concept ACK

@sipa has this been tested on a BE system?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.