Less reliance on protocol version #8642

As @laanwj said back in 2014, "Binding features to version numbers assumes a linear, centralized progression. It means that everyone that implements A also needs to implement B even though they are unrelated. I don't think this is desirable anymore".

This pull request starts a way forward relying on nVersion less. Already other bitcoin nodes, e.g. Unlimited set their version number to 80000 even though they do not currently support compact blocks or headers announcements, so the code using nVersion is becoming increasingly irrelevant to non Bitcoin Core nodes it seems.

This does not reduce reliance on the nVersion protocol, because recent additions are always negotiated separately (see the sendheaders and sendcmpct messages).

All this patch does is gratuitously break support with some old versions that may still be out there.

@sipa those recent additions you're mentioning are examples of where nVersion is being relied upon.

Which versions does it break support for? And ARE those versions still out there? And are we talking about more nodes, for example, than the nodes I am talking about (which break from the nVersion norms being discussed here)? E.g. There are at least 18 Unlimited nodes running on the network. I suspect nodes with version below 60000 are far fewer (if any!). We've already stopped support for Windows XP.. when are we planning to stop support for protocol < 60000 ?

You can still run a 0.2.10 client and synchronize with the network.

@sipa Yes, and I for one am in favour of that. Although I am not sure why you are mentioning that as this pull request wouldn't change that.

Sendheaders and compact blocks are negotiated separately. They don't rely on a particular version number; we simply avoid starting the negotiation if we know they don't support it anyway.

Further however you are assuming that all peers support pong (bip 31, first supported in Bitcoin 0.7). Why?

@sipa I am not assuming all peers support BIP31 - the check for that is still in there - just moved so that it takes up 1 line instead of 8

But speaking of Bitcoin 0.7, I have never seen anything lower than Bitcoin 0.8.6 on the network this year. Have you?

Pre-BIP31 nodes don't send a nonce inside the PING command. Your code will cause a failure to deserialize for such nodes, and subsequently disconnect them as a result.

Please stop.

@sipa I very much doubt this. For example, the version message in later versions now send more fields than older nodes would have anticipated. The additional fields are simply not used. Why would it be any different with the ping message?

I'll double check the code to see what you mean though.

Sending more fields that the receiver can decode is not a problem. Decoding more fields than the sender sends is problematic, and that is what you are doing here.

@sipa ah.. darn it. you're right. sorry, I have reinstated the other BIP31 check to fix that now.

..although, I still would expect there are more nodes that deviate from the current protocol numbers (e.g. >18 unlimited nodes) than there are nodes that don't support BIP31. I don't have the figures to hand though but would you agree that if this is the case that we should be supporting the nodes that are greater in numbers?

This change cannot be made retro-actively. For new protocol features it makes sense to not rely on the protocol version, and that is indeed what has been happening. Either separate advertisement mechanisms have been used or service flags. I still agree that forcing a linear progression of unrelated features is a mistake: we corrected this for NODE_BLOOM.

For the things that already rely on nVersion, that's too bad, we have to keep them to keep compatibility with older clients. NACK on this change (there may be valid reasons to break compatibility with ancient versions, but this isn't one).

After further testing, it does appear that certain nodes (not Satoshi ones) disconnect upon receiving either the getheaders or the sendcmpct commands - not behaviour I would expect but perhaps reason enough to continue taking notice of nVersion, at least for now :-s