Decouple addrFrom and nNonce fields in version message read #3982

The current code assumes that if the receive buffer for a version message is not empty after the addrMe field, it must contain the addrFrom field and the nNonce field. This pull request moves the nNonce read into its own statement after checking if the receive buffer is empty. This decouples the optionality of the fields so that each can be optional as opposed to both being required when addrFrom is present.

Note that there is an alternative way to approach this that I was unsure would be preferred, so I chose the approach in the pull request after discussing with @sipa who indicated the each read should be probably be separate.

The alternative approach comes from looking at commit 18c0fa97d0408a3ee8e4cb39c08156f7667f99ac which indicates that the three fields of addrFrom, nNonce, and strSubVersion were added in protocol version 106. In that case, perhaps all three reads should be under a single if (!vRecv.empty()) check which indicates that all three must be present if the receive buffer is not empty after the addrMe field which already existed in prior protocol versions. This would more clearly indicate that the fields were added in the same protocol version.

In either case, since this code defines the protocol spec, I think the handling should be one of the two approaches versus the current approach where two of the fields that were introduced in protocol version 106 are coupled where the third one is not.

If the alternative approach is preferred, I can update the pull request accordingly.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/fbe2c07e79b69e5405616d7f67452499739f925c for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

The method of extending "version" to enumerate new features is godawful. Would love to just send key/value pairs at the end, or add a "capabilities" message and appropriate detection gadgetry for that new message, etc.

Let's just all give up and use JSON communication!

Thank you Mr. Sarcasm. We have already seen "version" screwups in the field. The proposal has an engineering genesis. And no, it would not be JSON.

@jgarzik I do agree that the current method isn't the best, but nevertheless I think that it's important that so long as the code works the way it does, it treats the extension of the fields consistently. Since this is what defines the protocol, it seems rather odd that three fields which were introduced with protocol version 106 are treated differently.

I'm not sure if you were just talking in general, which I agree with, or specifically in relation to this pull request. If it was unclear from my initial text, this pull request is not adding any new features. It is simply modifying the existing message to treat the extension fields consistently.

Speaking generally

It does look more consistent this way.

However as sending addrFrom but not nNonce causes a parse error with all current clients, changing the 'protocol description' to be more permissive is deceptive.

@sipa @jgarzik @gavinandresen what are we going to do here? Does this change make sense?

I'm fine with improving the uniformity of how we deal with "version"'s optional fields. We shouldn't continue that practice though. I think new feature should just be negotiated separately through their own initiation message (which will automatically be ignored by clients that don't understand it).

Bringing the discussion back to the specific change at hand... This change is IMO pointless. "weak NAK" (ie. NAK/-1 but willing to be convinced otherwise)

All existing clients have a more strict definition, and there is no value in changing that so far down the stack of version information that is normally provided. We want all clients to send other datums such as strSubVer, which would necessarily imply this change is never triggered.

This change satisfies programmer OCD while being incompatible (less strict) with all existing clients, if this behavior is elected to be used.

As to the specific concerns about breaking existing clients, as @jgarzik notes, any client that implements anything greater than protocol version 106 (which is all of them) will necessarily require all of the fields. Further, the client back in the protocol version 106 days will not break since it included all three fields as well thereby satisfying both the current inconsistent implementation and the proposed corrected one.

However, I disagree on the point about it only being programmer driven OCD. The problem is "the code is the spec". As long as that is the case, I contend the code needs to accurately reflect the fact that the protocol version 106 introduced three fields, all of which are optional. As the code is currently written, that is not the case. It indicates that three fields were introduced, where two of the three are required if any are present, and the third is optional. This is completely inconsistent with the rest of the "spec" which can easily lead to mistakes in other implementations or really anything that wants to speak the wire protocol.

All of that said, regardless of this particular pull request, I think everyone in the thread agrees that future changes should be negotiated through another mechanism.

The current code accurately reflects network behavior as it was implemented.

There are zero clients on the network who care that this is optional, as any such clients have been forked away by the checksum change.

Ok, consensus seems to be that this is unnecessary.

At some point when the minimum version is raised, we make all these fields required. For future protocol changes a different extension scheme should be used instead of 'add optional fields to the end of the buffer'.