peer: Do not send inventory before version

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).

Author: jrick

peer/peer.go

@@ -482,10 +482,10 @@ type Peer struct {
 	userAgent            string
 	services             wire.ServiceFlag
 	versionKnown         bool
+	handshakeCompleted   bool
 	advertisedProtoVer   uint32 // protocol version advertised by remote
 	protocolVersion      uint32 // negotiated protocol version
 	sendHeadersPreferred bool   // peer sent a sendheaders message
-	versionSent          bool
 	verAckReceived       bool
 
 	knownInventory     *lru.Set[wire.InvVect]
@@ -705,6 +705,18 @@ func (p *Peer) VersionKnown() bool {
 	return versionKnown
 }
 
+// HandshakeCompleted returns whether initial version messages were sent and
+// received.
+//
+// This function is safe for concurrent access.
+func (p *Peer) HandshakeCompleted() bool {
+	p.flagsMtx.Lock()
+	handshakeCompleted := p.handshakeCompleted
+	p.flagsMtx.Unlock()
+
+	return handshakeCompleted
+}
+
 // VerAckReceived returns whether or not a verack message was received by the
 // peer.
 //
@@ -1652,7 +1664,7 @@ out:
 
 		case iv := <-p.outputInvChan:
 			// No handshake?  They'll find out soon enough.
-			if p.VersionKnown() {
+			if p.HandshakeCompleted() {
 				invSendQueue = append(invSendQueue, iv)
 			}
 
@@ -2080,9 +2092,6 @@ func (p *Peer) writeLocalVersionMsg() error {
 		return err
 	}
 
-	p.flagsMtx.Lock()
-	p.versionSent = true
-	p.flagsMtx.Unlock()
 	return nil
 }
 
@@ -2094,7 +2103,15 @@ func (p *Peer) negotiateInboundProtocol() error {
 		return err
 	}
 
-	return p.writeLocalVersionMsg()
+	if err := p.writeLocalVersionMsg(); err != nil {
+		return err
+	}
+
+	p.flagsMtx.Lock()
+	p.handshakeCompleted = true
+	p.flagsMtx.Unlock()
+
+	return nil
 }
 
 // negotiateOutboundProtocol sends our version message then waits to receive a
@@ -2105,7 +2122,15 @@ func (p *Peer) negotiateOutboundProtocol() error {
 		return err
 	}
 
-	return p.readRemoteVersionMsg()
+	if err := p.readRemoteVersionMsg(); err != nil {
+		return err
+	}
+
+	p.flagsMtx.Lock()
+	p.handshakeCompleted = true
+	p.flagsMtx.Unlock()
+
+	return nil
 }
 
 // start begins processing input and output messages.