verack arriving before version #8569

I am seeing incidents of verack messages being processed before the version messages from peers (outbound connections). The impact of this is that certain functions done upon receipt of verack therefore do not happen. Could perhaps the functionality performed upon receipt of verack be either moved to what is done upon receipt of "version" messages, or perhaps the messages as they are received could be queued to be processed after the "version" message is received?

Details from debug.log:-

2016-08-07 08:09:07 Outbound connection 46.227.201.91:8333 peer=2 2016-08-07 08:09:07 Misbehaving: peer=2 (0 -> 1) 2016-08-07 08:09:07 ProcessMessages(verack, 0 bytes) FAILED peer=2 2016-08-07 08:09:07 receive version message: /Satoshi:0.12.1/: version 70002, blocks=424091, us=x.x.x.x:8333, them=46.227.201.91:8333, peer=2 2016-08-07 08:19:34 Outbound connection 46.227.201.91:8333 peer=8 2016-08-07 08:19:34 Misbehaving: peer=8 (0 -> 1) 2016-08-07 08:19:34 ProcessMessages(verack, 0 bytes) FAILED peer=8 2016-08-07 08:19:34 receive version message: /Satoshi:0.12.1/: version 70002, blocks=424092, us=x.x.x.x:8333, them=46.227.201.91:8333, peer=8 2016-08-09 03:15:36 Misbehaving: peer=11 (0 -> 1) 2016-08-09 03:15:36 ProcessMessages(verack, 0 bytes) FAILED peer=11 2016-08-09 03:15:36 receive version message: /Satoshi:0.12.1/: version 70002, blocks=424364, us=x.x.x.x:8333, them=46.227.201.91:8333, peer=11 2016-08-14 14:11:58 Outbound connection 46.227.201.91:8333 peer=4 2016-08-14 14:11:59 Misbehaving: peer=4 (0 -> 1) 2016-08-14 14:11:59 ProcessMessages(verack, 0 bytes) FAILED peer=4 2016-08-14 14:11:59 receive version message: /Satoshi:0.12.1/: version 70002, blocks=425181, us=x.x.x.x:8333, them=46.227.201.91:8333, peer=4

Ok, I realize all these examples are the same node, so it may be that this situation is so rare it doesn't deserve any energy devoted to it, but I think it does raise some questions: why the policy of Misbehave +1 for each message before a version message - i.e. why allow 99 messages before banning? And What exactly is the purpose of verack? What should be done before a verack is sent and what should be dependent on a verack being received?

As defined in the bitcoin.org developer documentation a verack should not be <s>sent</s> processed (at all) before a mutual exchange of version messages.

I'm not sure if the linked developer documentation superceeds actual reference client behavior, but it only seems good and proper that all clients must observe this handshake behavior.

Rather than move functionality to where it will activate (because of protocol breach), or queue a verack message until its probably associated version message arrives, it seems best that client code enforces the correct order? I assume (but have not established for a fact) that this is already the case for Core.

Ok, I realize all these examples are the same node, ...

Your logs show four one peer is misbehaving and reconnecting as <peer> 2, 8, 11 and 4. Do you mean that you have only observed these premature verack messages on the node whose logs you quote above, or only from that IP address?

To simplify my query: What should a node do if a verack (after sending version) is not received during a defined period of time? If nothing, then I think code that runs as a result of a verack should only be code that deals with the verack (which currently, is no code, and so the response to a verack should be the same as the current response to a NOTFOUND message - i.e. nothing).

Doesn't the normal receive timeout kick in in that case?

@rebroad, version sets pfrom->fSuccessfullyConnected and the subsequent verack sets state fCurrentlyConnected = true, so the implication of not sending a verack ensues from there.

Whichever way, according to main.cpp, the node in your example gets punished (presumably for the incoming verack in response to the local node's initiating version message). The verack is correctly logged as FAILED since it arrived before the obligatory version message - which arrives but is never verack'd - and an eventual connection timeout should result.

At the risk of seeming rude, I have to agree with @sipa that self-study is a basic requirement in this project. All of the above information is apparent from the code. Sometimes it is easy to just ask the general audience in Github, but consider that everyone subscribed to this project receives every comment via email. If the person asking the question shows that they have not done the required foundation or research reading away from this repository, then rudimentary questions can break concentration, distract, or even be plain annoying. Instead of spraying with a machine gun, we should do homework on our own and then snipe issues and bugs like a self-sufficient sharpshooter. That way no-one has to "carry" someone else and the work is autonomous and a contribution.

The node in your logs is behaving irregularly. There may be many reasons for that - the primary being that it is running modified code or that it is an alternative implementation that masquerades as a Satoshi client.

I must mention that in the time since your original comment I have tested outbound connections to a 0.11.1 client without sending verack and the peer connections persisted beyond timeout, with the remote node seemingly "forgiving" the incomplete handshake. 0.12.1 behaves as in your logs and times out a connection that is not verack'd. So your comment is useful despite lacking due diligence on your part.

If a node receives a verack before receiving a version, it should disconnect the peer who sent it, as they are not obeying the protocol.

@sipa I'm inclined to agree with you. However, this is not what the satoshi client has ever done so far. I'm just making this code change to bring clarity to what the code does - i.e. make it more understandable and help avoid changes that change the functionality in adverse ways. Would you like me to change my pull that makes verack a no-op and instead make it disconnect a node after a certain timeout?

@venzen I'm running 0.13 and my node isn't timing out connections that don't send a verack... I've not managed to locate the code which is supposed to do that - I will have another look for it. I also don't receive any emails from github for updates to issues, so I wasn't aware other people did. I'll try to figure out why this is too - I would expect that people only subscribe to these comments when they click the subscribe button to the right (or perhaps they subscribe automatically when they comment).

Ok, looking at my the latest HEAD, when a verack is received, if fNetworkNode is set (I'm not clear why we need this variable - are you?) fCurrentlyConnected is also set to true. This variable is used only by main.cpp and only used in 4 places. (1. where it's defined. 2. where it's initialised to false. 3. In FinalizeNode which is only run when a node is already being disconnected, and finally 4. Setting it to true when receiving the verack!) so therefore, verack doesn't appear to form any part of any disconnection logic, and fCurrentlyConnected seems to be a pretty redundant variable.

If I've overlooked anything, please feel free to let me know, but I do think I've been doing my research!