Move code from VERACK to VERSION (since VERACK is not requied) #8597

Inspired by #8569

Currently it's possible for a node to not send a VERACK and the only impact of this is that the code within VERACK will not be run, which is not desired behaviour.

The code behind VERACK ought to be just that - code that needs a verack to run. It may be that a variable be set which would avoid a disconnect after a certain time, for example, but so far this has never been implemented so effectively VERACKs have never been required for a connection to remain connected. The code should perhaps reflect this.

Why do you want to make verack a no-op? It's nice to know the peer received and understood our version message before continuing.

NACK, this part of the protocol is not broken, let's not fix it.

@sipa you say "before continuing" but this is not how the code currently works. There is no stopping or waiting for the verack currently, instead some code is simply not executed as a result of the verack not being received. That code (which is important that it runs) is what I have now moved to run upon receipt of a "version" message - which DOES cause the required continuation that you mention.

The githib diff doesn't show this code in question so it's not clear from the github diff.

The code in question is the sending of the SENDHEADERS and SENDCMPCT messages.

Therefore if no VERACK is received the node will default to receiving INVs for block announcements and will not be using Compact Blocks - this is currently DEPENDENT on verack messages being received. Other than this the node functions the same in every other way. @laanwj If that's not considered broken then you are saying this is intentional - why do you want compact blocks and headers announcements to be dependant on verack messages? It's certainly not, IMHO, intuitive that this is how the protocol would function, based on a typical understanding of ACK messages. Headers announcements and compact block technology are recent additions so it seems a bit strange to me that only this new functionality is dependant on verack messages yet all other functionality will continue as normal without veracks.

@rebroad Ok, so the history behind VERACK is that it was introduced together with protocol checksums.

Originally, the protocol version started at 0, and then VERSION is used to propose by peer A that peer B can start using a higher protocol version number. Peer A however needs to know when B received that VERSION message, as he doesn't know when to start decoding the received messages using the new protocol, so the switchover to enable decoding checksums happens when B sends a VERACK back.

Since february 20, 2012 the protocol version starts at 209, which has checksums from the start (including the VERSION and VERACK messages themselves).

Since then, VERACK has effectively not had any function, but it should also not slow anything down, apart from sending an extra message. There may also be a use for it in the future, if we'd ever want to propose a protocol change that modifies the checksums again, or otherwise modifies the based protocol structure.

Don't fix what isn't broken.

@sipa Thanks for that. I wouldn't be proposing this pull request if I didn't think there was something broken that it fixes. But broken is a subjective term. If you're happy with Bitcoin Core NOT requesting announcements by headers or preferrring compact blocks, then it's not broken. But given that most Bitcoin Core nodes ARE requesting these things, then it would seem broken to me.

You have a peer who is sending messages before sending a verack. That peer is broken.

@sipa The peer you refer to is indeed behaving strangely, but it's brokenness is a co-creation between it sending the VERACK before the VERSION (which clearly is wrong), and the standard node waiting for a VERACK after VERSION before sending SENDHEADERS and SENDCMPCT.

Now, given that waiting for the VERACK does nothing but facilitate this brokenness and provides no other value that I am aware of, then why don't we actively avoid facilitation of brokenness? This is surely what every pull request is aiming to do, as far as I understand the process.

@rebroad What value does this change have?

@sipa Interoperability. I think I am realising that perhaps many of my desired changes have this goal in mind, and for some reason it seems that a significant number of Core developers are mainly considering that any non-Satoshi node is an oddity that doesn't really need to be supported, or something like this. My idea of an ideal node is one that can communicate usefully to as many nodes as possible. They ought to avoid disconnection unless it provides value (which is does with FILTERLOAD and FILTERADD, but not FILTERCLEAR). No SPV node out there currently starts with FILTERCLEAR, that I know of. They usually start with FILTERLOAD (one of more of them) followed by a MEMPOOL request. Sorry, I am referring to my other pull request now, but the gist is the same.

#4389 was also motivated along similar lines - i.e. allow nodes to learn how to inter-operate with each other rather than making assumptions.

Apologies that I am getting on my soap-box a little here, but to summarise, I ask you - what value does NOT making this change have? I believe I have communicated the value it has, albeit (in the case of this one peer) of little impact. In the greater sense it helps to bring clarity to the protocol - i.e. that VERACKS are now (or in most cases, should be) redundant.

@rebroad the initial handshake is best left the way it is. As @sipa describes in a comment above, VERACK may not have a function now, but it may very well become useful in future.

For example BIP151 uses a similar handshake to establish an encrypted session between peers.

The implementation of encinit and encack in BIP151 mirrors the version and verack process as it stands and shows its usefulness, so best not to code it away.

@venzen (no idea why github doesn't let me tag you!). If sipa is indeed saying that VERACK doesn't have a function now, then this surely would strengthen the case for this pull - i.e. I am seeking to make it clearer that it doesn't have a function now.

read my comment again :)

Besides, remember VERACK's critical function in the connection handshake.

The process of connecting and the decision to peer should be bilateral. PeerA (sends version) - this is my version, my network_services, and best_height. PeerB - this is useful (reply with version and verack). PeerA (considers VERSION information) - actually, this is not useful (doesn't reply with a verack) and declines the connection.

VERACK represents the decision to peer based on what was learnt from VERSION. Any connection attempt must be initiated and then, based on what is learnt, acknowledged (or not), surely?

@venzen which comment are you referring to? verack may have been part of a handshake in the past, but it is no longer, and certainly hasn't been since 2012. there is no timeout regarding version message - i.e. no disconnection logic if a VERACK is not received. Once version is received, pretty much everything is on (except for the recently added SENDHEADERS and SENDCMPCT). My point is - was the recent dependency on VERACK intended or accidental? (since prior to this it was not a dependent).

I still don't understand what problem this change is supposed to address.

Yes, the code could be slightly simplified if we change the protocol to make verack a no-op corresponding to patch you propose. But that is a protocol change. Admittedly a very simple one, but what is the benefit? That some implementations which were misimplementing a very simple part of the protocol before would know simple start working? They were broken before, so they're likely doing other things wrong as well.

In general, in modern protocol design, you do not try to be lax in processing input. It just creates unclear expectations about what is necessary to interact with peers.

@sipa Let me invite you instead to consider how you would document the bitcoin protocol. Currently, as of protocol 70013 you would now need to explain that verack messages are necessary if you want to allow nodes to request block announcements by headers, and as of protocol 70014, verack is necessary if you want to allow nodes to advertise that they can process compact blocks.

It just sounds a little odd, don't you think? Had the logic for those messages been put in the version section instead, verack would not need to be mentioned at all.

I think it's important we keep in mind how the documented protocol reads and that there's a level of intuitiveness - i.e. that the message names have some bearing on what they do. Since 70013, "verack" as a message name has no bearing on its function, and if you were to try to come up with a suitable message name instead of "verack" for what verack now achieves, I think you might struggle to find an intuitive one.

No offence, but if that is the stumble block you have problems with for implementing the p2p protocol, you're in for a world of pain.

I really don't see the benefit of changing the protocol for the benefit of being able to remove two lines of text from a hypothetical specification, and I don't see the benefit of continuing to spend time on discussing it.

@sipa As a programmer, I thought you would already have an appreciation of the KISS principle. Or, to quote Albert Einstein, "Make everything as simple as possible, but not simpler".

@rebroad As a programmer, I thought you would already have an appreciation for not fixing what isn't broken.

Yes, if we'd design Bitcoin from scratch, I'm sure we could let VERACK out. We're not doing that. We're wasting hours of discussion on changing a triviality.

Goodness me. I did like to argue did't I....