[RFD] All classes which use IMPLEMENT_SERIALIZE should check nVersion on reading

[Jim Hyslop]

Sipa's recent patch to the wallet (for tracking partially spent coins) got me thinking about serialization and versioning issues.

Right now, few (if any) of the classes which use IMPLEMENT_SERIALIZE check the value of nVersion when reading. I propose that we should add that check to all classes, now.

My reasoning is: if we wait to introduce the version check until it's needed, then we have to either make the change backwards-compatible (which, if you have any experience with legacy software you will know that backwards compatibility is an unrealistic goal in the long term), hope that older clients don't break, or hope that people upgrade quickly enough to take advantage of the new feature/fix/whatever.

We aren't a big corporation, which can push out a new version whether people want it or not. Some people may wait months or even years before updating their clients. So, if we ensure that the clients check versions now, then if we do introduce a backwards-incompatible change, users can be immediately notified that they have to upgrade the client.

Which brings up a side issue of how to notify users. In the GUI, we can pop up a message box. But what about the daeamon version? Is it sufficient to log a message and refuse to load the object?

[jgarzik]

Well, the usage so far has been backwards compatible AFAIK.  You start out unversioned, and if a field is added, an nVersion check is also added.  If no nVersion check exists, that data structure is unchanged.

The current system seems straightforward and is backwards compatible, so why add superfluous checks?

[Jim Hyslop]

Well, the usage so far has been backwards compatible AFAIK.  You start out unversioned, and if a field is added, an nVersion check is also added.  If no nVersion check exists, that data structure is unchanged.

(minor nit pick: nVersion already exists, and is set to 1, but that's a trivial point)

You're assuming the software will be updated at the same time as the data structure. That is a really bad assumption to make.
Think it through. Version 1 clients don't check version numbers. Now, what happens when a version 1 client tries to read version 2 data? It will assume that the data is version 1, so it will just blindly go ahead and try to read the data. That is a sure recipe for disaster down the road. Defensive coding 101 - validate everything.

I have learned through bitter experience that you check version numbers starting at version 1, not at version 2. Unless, that is, you have complete control over the software distribution and the data you're trying to work with - which we do not.

The current system seems straightforward and is backwards compatible, so why add superfluous checks?

And, as I said, trying to maintain backwards compatibility is a fool's errand. You might be able to carry it on for a short period, but sooner or later you're going to have to put in some really complicated, hard-to-maintain and error prone code in the name of "backwards compatibility."

[jgarzik]

And, as I said, trying to maintain backwards compatibility is a fool's errand. You might be able to carry it on for a short period, but sooner or later you're going to have to put in some really complicated, hard-to-maintain and error prone code in the name of "backwards compatibility."

Perhaps, but I think we should continue on this fool's errand of backwards compatibility a while longer...

[Jim Hyslop]

And, as I said, trying to maintain backwards compatibility is a fool's errand. You might be able to carry it on for a short period, but sooner or later you're going to have to put in some really complicated, hard-to-maintain and error prone code in the name of "backwards compatibility."

Perhaps, but I think we should continue on this fool's errand of backwards compatibility a while longer...

Sipa's recent patch for partially unspent transactions is not entirely backwards compatible - existing clients will report an incorrect balance. That's not a major problem, but it's a definite warning sign that we need to be checking all version fields now.

What's it going to take to convince you that we should be proactive, not reactive?

[mndrix]

What's it going to take to convince you that we should be proactive, not reactive?

I have no opinion on this particular topic, but patches usually speak clearly and persuasively.  They also help facilitate the discussion by grounding it in a specific implementation.  Perhaps you could post a patch or a GitHub pull request implementing your suggestion.

[genjix]

What's the big deal about implementing version numbers in IMPLEMENT_SERIALIZE?

[Hal]

Specifically, what are you proposing? What nVersion value should they check, what should they check it against, and what should they do if the check fails/succeeds?

Messages do not have version numbers. Nodes exchange their client versions on connection, but that value increments with every release. There's no way to know if packets from a more-recent client peer have changed to be incompatible with this version.

Here is some of the code that handles the "version" message:

Code:

        // Change version
        if (pfrom->nVersion >= 209)
            pfrom->PushMessage("verack");
        pfrom->vSend.SetVersion(min(pfrom->nVersion, VERSION));
        if (pfrom->nVersion < 209)
            pfrom->vRecv.SetVersion(min(pfrom->nVersion, VERSION));

pfrom->nVersion is the peer version. This sends the "verack" message, checking if the peer is newer than version 209 for backwards compatibility. It sets the outgoing serialization version (vSend) to the older of the peer and this node, but only does it for incoming (vRecv) for peers older than 209. Otherwise the incoming deserialization version is left at this node; don't know why the difference.

This design puts the responsibility on the newer node to send/receive backwards compatible messages.

[Jim Hyslop]

Specifically, what are you proposing? What nVersion value should they check, what should they check it against, and what should they do if the check fails/succeeds?

I see why everyone's so confused about what I'm trying to discuss. I just realized the nVersion parameter to Serialize, GetSerialSize, and Unserialize is tied to the VERSION macro, and is basically a constant.

Let me re-think this, and I'll start a new thread once I've got a clearer picture of how to present this.

[Hal]

I see why everyone's so confused about what I'm trying to discuss. I just realized the nVersion parameter to Serialize, GetSerialSize, and Unserialize is tied to the VERSION macro, and is basically a constant.

This is not quite right, but the code is pretty spread out. Generally un/serialization is done by the operator<< and operator>> methods of CDataStream in serialize.h. We do CTransaction tx; vRecv >> tx; to read a transaction object; or vSend << a1; to send something of template class a1 (in PushMessage in net.h).

The << and >> methods in CDataStream call Serialize() or Unserialize(), which do default the nVersion parameter to VERSION. But in these calls the default is not used, they pass the nVersion instance variable of CDataStream. And this instance variable is set to the min of the node and peer version, e.g. in the code I showed above.