peer: Do not send inventory before version #3479
Conversation
The access guarded by an atomic int32 was incorrect. For example, access to the p.conn could be performed as long as p.connected was non-zero, but p.connected would be incremented before p.conn was ever assigned by AssociateConnection. While here, also add missing mutex locking protecting timeConnected and na.
For inbound peers, their version message is read, and the version is marked as known, before our local peer ever publishes the version message. This could result in a protocol messaging race where other messages (and in particular, notifying inventory) may be published prior to our version ever being sent. While here, remove unused versionSent field from the peer. The new HandshakeCompleted tracking is strictly better (while it is allowed to send a peer messages before we have read their version, whether those messages follow the negotiation protocol version won't be known until after the fact).
There was a problem hiding this comment.
This looks fine, but I wonder if it should be more general since I think there are other cases that the same thing could really apply to. For example, there is the QueueInventoryImmediate path and, well, technically, anything via QueueMessage too which could theoretically be sent prior to the version message.
In practice, at least for dcrd, the remote peer will disconnect the local peer if it sends anything before the version message. I'm not sure if SPV clients like wallet are also doing that, but they should be, because it is technically required by the protocol.
Given we don't see a ton of disconnections, it's obviously pretty rare that anything is sent before the version, but it does seem prudent to avoid potentially violating the protocol resulting in a disconnect.
Perhaps instead of checking if the handshake has completed only in the one spot upon the receive on p.outputInvChan which drops the announcement altogether, it would be better to do something like the following:
- Unconditionally add the announcement to
invSendQueueincase iv := <-p.outputInvChan: - Add another case to the switch at the top of
case <-trickleTimer.C:to reset thetrickleTimerand ignore the event when the handshake isn't complete yet. This will leave any pending announcements in the queue for the next tick. - Add logic in
case msg := <-p.outputQueue:(or perhaps inqueuePacket) that appends the message to the local queue when the handshake hasn't completed yet (instead of directly sending to the output channel) except when it is the version message itself (aka of type*wire.MsgVersion). Otherwise, once the handshake has completed, the logic stays the same as what it is now. This would have the effect of forcing the version message to be sent first even if some other goroutine tries to send another type of message first.
Finally, it doesn't matter, but maybe calling it HandshakeDone would be better since it's a bit shorter and this will be a new method that is part of the public API.
Yes, dcrwallet does of course. https://github.com/decred/dcrwallet/blob/76d500f80440a5ded40b11157793372e5f0d1e28/p2p/peering.go#L596 I'll take a look at the other places where we send messages and similarly ensure that both version messages have been sent and received. |
|
Oh, I should've also noted that I like the overall idea. Detecting a fully completed handshake as opposed to only half of it being completed is much cleaner and less error prone. Not to mention, it's easier to reason about too, since it's a completed exchange instead of a half-open quasi state. |
For inbound peers, their version message is read, and the version is marked as
known, before our local peer ever publishes the version message. This could
result in a protocol messaging race where other messages (and in particular,
notifying inventory) may be published prior to our version ever being sent.
Rebased over #3478.