Serialization cleanup: use enum for nType arg -- Take II #5448

rustyrussell commented at 1:03 AM on December 9, 2014: contributor

@jonasschnelli (and Jenkins) pointed out that my compilation coverage was lacking. @laanwj noted the reduntant enum. Both fixed in this version.

laanwj commented at 8:08 AM on December 9, 2014: member

FYI for next time: you can (force) push to the existing branch to update a pull request, no need to create a new one.

Original pull: #5443

laanwj added the label Improvement on Dec 9, 2014

rustyrussell commented at 3:30 AM on December 11, 2014: contributor

OK, thanks, will do next time.

laanwj commented at 10:34 AM on December 12, 2014: member

Looks good to me - we don't ever use a combination of the flags, so an enumeration would be more appropriate than a bit field. Though as this touches consensus code it needs to be reviewed carefully.

laanwj commented at 2:41 PM on December 12, 2014: member

There's still some enum SerializeType where just SerializeType can be used. https://github.com/laanwj/bitcoin/commit/d1bc5bf305b5c9c2c8af2931412d4d6707e761e7 catches these.

fanquake commented at 9:41 AM on February 2, 2015: member

@rustyrussell Needs rebase. Also see laanwj's comment about additional changes.

rustyrussell force-pushed on Feb 4, 2015

laanwj commented at 11:35 AM on April 15, 2015: member

This needs rebase. Although maybe better to wait for agreement to do this (serialization is critical for consensus, so changes may be deemed too risky). @gmaxwell @jgarzik @gavinandresen @sipa any opinion on this change?

Use enum for serialization type.

nType gets handed through all these because some things serialize differently
on disk, for hashing or for networking.  But using an int is just lazy
and opens up the possibility of order mixup, since nVersion is also an int.

For the overloading hack to default to calling a member function, we
can (ab)use the nVersion parameter instead.

028391f085

Make SER_* values a normal enum.

commit e754cf4133c (Split off CBlockHeader from CBlock) removed
the application-specific flags from the serial type, but left
the comment and the weird spacing in the values.

Make SER_* a normal enum, and test for (in)equality as expected.

I also changed the weird negative in CAddress serialization from:

        if ((nType & SER_DISK) ||
            (nVersion >= CADDR_TIME_VERSION && !(nType & SER_GETHASH)))

To the clearer:
        if ((nType == SER_DISK) ||
            (nVersion >= CADDR_TIME_VERSION && (nType == SER_NETWORK)))

0ee4acdbd5

rustyrussell force-pushed on Apr 20, 2015

maaku commented at 10:18 AM on May 28, 2015: contributor

utACK

laanwj commented at 1:16 PM on July 10, 2015: member

Closing this - I think this is a good idea in itself, but given how critical the serialization is, I'm not sure anyone is willing to take the risk that this breaks something. Sorry.

laanwj closed this on Jul 10, 2015

in src/serialize.h:None in 0ee4acdbd5

 151 | @@ -152,12 +152,11 @@ inline float ser_uint32_to_float(uint32_t y)
 152 |  // i.e. anything that supports .read(char*, size_t) and .write(char*, size_t)
 153 |  //
 154 |  
 155 | -enum
 156 | +enum SerializeType

maaku commented at 3:47 PM on July 10, 2015:

replace this with "enum SerializeType : int" in order to make absolutely sure there is no consensus code changes.

sipa commented at 3:50 PM on July 10, 2015:

That's c++11.

sipa commented at 3:56 PM on July 10, 2015:

However, an enum should always have type int, as long as the number of elements fits in an int, so I don't think that's necessary.

maaku commented at 4:17 PM on July 10, 2015:

That's not actually what the standard size. Enum is implementation defined, but no larger than int. It could be char.

On Fri, Jul 10, 2015 at 8:57 AM, Pieter Wuille notifications@github.com wrote:

In src/serialize.h #5448 (review):

@@ -152,12 +152,11 @@ inline float ser_uint32_to_float(uint32_t y) // i.e. anything that supports .read(char_, size_t) and .write(char_, size_t) //

-enum +enum SerializeType

However, an enum should always have type int, as long as the number of elements fits in an int, so I don't think that's necessary.

— Reply to this email directly or view it on GitHub https://github.com/bitcoin/bitcoin/pull/5448/files#r34368410.

maaku commented at 3:51 PM on July 10, 2015: contributor

@laanwj sorry, I disagree 100%. These sort of changes are necessary precisely in order to prevent consensus forks in the future. Having the compiler check as much type information as possible is a benefit to libconsensus.

I've put a comment on the one line that could be changed to make this diff have exactly the same behaviour as the prior code with no implementation-defined semantics, according to the C++03 standard (ISO/IEC 14882:2003) in 7.2-5 (Enumeration declarations).

Please re-open this pull request.

sipa commented at 4:02 PM on July 10, 2015: member

Sorry for the lack of response here, but I do think changes like this are generally safe. The types themselves are never serialized; they only select what is being serialized. In fact, it seems that the only serialization implementation that actually inspects nType is the CAddress one - which won't hurt consensus.

I wasn't actually aware of this change or I would have commented earlier. I would actually like to go further here even, and replace the nType + nVersion combination by a templated SerializationContext class. There could be several such context types (one for disk structures, which encapsulates the client version, one for network structures which encapsulates the protocol version, and one for hashing of consensus structures which would not have any fields). This would remove the knowledge of what kinds of serialization are being done from serialize.h, reduce the argument lists, and have better extensibility and type safety.

But let's do this first. Concept ACK. If we're fine with re-opening, I'll review the code in more detail.

rustyrussell commented at 8:20 PM on July 12, 2015: contributor

"Wladimir J. van der Laan" notifications@github.com writes:

Closing this - I think this is a good idea in itself, but given how critical the serialization is, I'm not sure anyone is willing to take the risk that this breaks something. Sorry.

Err, OK. This seemed to me like a trivial cleanup of currently confusing code.

Certainly not worth any significant developer angst.

Cheers, Rusty.

MarcoFalke locked this on Sep 8, 2021