From 469bf2a63e18a437fd4d6d98a831d97f38815c0f Mon Sep 17 00:00:00 2001
From: catbref <misc-github@talk2dom.com>
Date: Tue, 30 Jun 2020 13:37:14 +0100
Subject: [PATCH] Improve inbound peer handshaking

If a node accepts a connection from an inbound peer
then remote peer will send RESPONSE first
and local node would previously change handshaking state
to COMPLETED while computing their own RESPONSE.

This meant that the local node would sometimes also start
sending post-handshake messages to the remote peer,
e.g. TRANSACTION_SIGNATURES.

Remote peer is only expecting a RESPONSE message, so would
close connection.

So we introduce an extra handshaking state "RESPONDING" for use
by local node while they compute RESPONSE in a separate thread.
Once the RESPONSE has been sent, local node moves to COMPLETED
state and called onHandshakeCompleted() as per usual.

Note that the code path when connecting outbound to a remote peer
is not changed, and the RESPONDING state is not used.

Also in this commit:

Network.onPeerReady now bypasses call to onMessage and instead
calls onHandshakingMessage() directly to avoid race condition
where peer's handshake status could change between
onPeerReady's caller and onMessage() calling peer.getHandshakeStatus()
---
 .../java/org/qortal/network/Handshake.java    | 31 +++++++++++++++++--
 src/main/java/org/qortal/network/Network.java |  4 +--
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/main/java/org/qortal/network/Handshake.java b/src/main/java/org/qortal/network/Handshake.java
index 80a9a8b9..b8a72372 100644
--- a/src/main/java/org/qortal/network/Handshake.java
+++ b/src/main/java/org/qortal/network/Handshake.java
@@ -164,6 +164,11 @@ public enum Handshake {
 
 			peer.setPeersNodeId(Crypto.toNodeAddress(peersPublicKey));
 
+			// For inbound peers, we need to go into interim holding state while we compute RESPONSE
+			if (!peer.isOutbound())
+				return RESPONDING;
+
+			// Handshake completed!
 			return COMPLETED;
 		}
 
@@ -184,22 +189,42 @@ public enum Handshake {
 				Message responseMessage = new ResponseMessage(nonce, data);
 				if (!peer.sendMessage(responseMessage))
 					peer.disconnect("failed to send RESPONSE");
+
+				// For inbound peers, we should actually be in RESPONDING state.
+				// So we need to do the extra work to move to COMPLETED state.
+				if (!peer.isOutbound()) {
+					peer.setHandshakeStatus(COMPLETED);
+					Network.getInstance().onHandshakeCompleted(peer);
+				}
 			});
 
 			responseThread.setDaemon(true);
 			responseThread.start();
 		}
 	},
-	COMPLETED(null) {
+	// Interim holding state while we compute RESPONSE to send to inbound peer
+	RESPONDING(null) {
 		@Override
 		public Handshake onMessage(Peer peer, Message message) {
-			// Handshake completed
+			// Should never be called
 			return null;
 		}
 
 		@Override
 		public void action(Peer peer) {
-			// Note: this is only called when we've made outbound connection
+			// Should never be called
+		}
+	},
+	COMPLETED(null) {
+		@Override
+		public Handshake onMessage(Peer peer, Message message) {
+			// Should never be called
+			return null;
+		}
+
+		@Override
+		public void action(Peer peer) {
+			// Note: this is only called if we've made outbound connection
 		}
 	};
 
diff --git a/src/main/java/org/qortal/network/Network.java b/src/main/java/org/qortal/network/Network.java
index a04921b3..86e189b0 100644
--- a/src/main/java/org/qortal/network/Network.java
+++ b/src/main/java/org/qortal/network/Network.java
@@ -635,7 +635,7 @@ public class Network {
 
 	/** Called when Peer's thread has setup and is ready to process messages */
 	public void onPeerReady(Peer peer) {
-		this.onMessage(peer, null);
+		onHandshakingMessage(peer, null, Handshake.STARTED);
 	}
 
 	public void onDisconnect(Peer peer) {
@@ -777,7 +777,7 @@ public class Network {
 		opportunisticMergePeers(peer.toString(), peerV2Addresses);
 	}
 
-	private void onHandshakeCompleted(Peer peer) {
+	/*pacakge*/ void onHandshakeCompleted(Peer peer) {
 		LOGGER.debug(String.format("Handshake completed with peer %s", peer));
 
 		// Are we already connected to this peer?