test: Not redialing connections if in pending inbound state and diff cleanup

p2p/dial_test.go

@@ -423,6 +423,82 @@ func TestDialSchedDNSHostname(t *testing.T) {
 	})
 }
 
+// This test checks that nodes with pending inbound connections are not dialed.
+func TestDialSchedPendingInbound(t *testing.T) {
+	t.Parallel()
+
+	config := dialConfig{
+		maxActiveDials: 5,
+		maxDialPeers:   4,
+	}
+	runDialTest(t, config, []dialTestRound{
+		// 2 peers are connected, leaving 2 dial slots.
+		// Node 0x03 has a pending inbound connection.
+		// Discovered nodes 0x03, 0x04, 0x05 but only 0x04 and 0x05 should be dialed.
+		{
+			peersAdded: []*conn{
+				{flags: dynDialedConn, node: newNode(uintID(0x01), "127.0.0.1:30303")},
+				{flags: dynDialedConn, node: newNode(uintID(0x02), "127.0.0.2:30303")},
+			},
+			update: func(d *dialScheduler) {
+				d.inboundPending(uintID(0x03))
+			},
+			discovered: []*enode.Node{
+				newNode(uintID(0x03), "127.0.0.3:30303"), // not dialed because pending inbound
+				newNode(uintID(0x04), "127.0.0.4:30303"),
+				newNode(uintID(0x05), "127.0.0.5:30303"),
+			},
+			wantNewDials: []*enode.Node{
+				newNode(uintID(0x04), "127.0.0.4:30303"),
+				newNode(uintID(0x05), "127.0.0.5:30303"),
+			},
+		},
+		// Pending inbound connection for 0x03 completes successfully.
+		// Node 0x03 becomes a connected peer.
+		// One dial slot remains, node 0x06 is dialed.
+		{
+			update: func(d *dialScheduler) {
+				// Pending inbound completes
+				d.inboundCompleted(uintID(0x03))
+			},
+			peersAdded: []*conn{
+				{flags: inboundConn, node: newNode(uintID(0x03), "127.0.0.3:30303")},
+			},
+			succeeded: []enode.ID{
+				uintID(0x04),
+			},
+			failed: []enode.ID{
+				uintID(0x05),
+			},
+			discovered: []*enode.Node{
+				newNode(uintID(0x03), "127.0.0.3:30303"), // not dialed, now connected
+				newNode(uintID(0x06), "127.0.0.6:30303"),
+			},
+			wantNewDials: []*enode.Node{
+				newNode(uintID(0x06), "127.0.0.6:30303"),
+			},
+		},
+		// Inbound peer 0x03 disconnects.
+		// Another pending inbound starts for 0x07.
+		// Only 0x03 should be dialed, not 0x07.
+		{
+			peersRemoved: []enode.ID{
+				uintID(0x03),
+			},
+			update: func(d *dialScheduler) {
+				d.inboundPending(uintID(0x07))
+			},
+			discovered: []*enode.Node{
+				newNode(uintID(0x03), "127.0.0.3:30303"),
+				newNode(uintID(0x07), "127.0.0.7:30303"), // not dialed because pending inbound
+			},
+			wantNewDials: []*enode.Node{
+				newNode(uintID(0x03), "127.0.0.3:30303"),
+			},
+		},
+	})
+}
+
 // -------
 // Code below here is the framework for the tests above.
 

p2p/server.go

@@ -682,8 +682,7 @@ running:
 		case c := <-srv.checkpointPostHandshake:
 			// A connection has passed the encryption handshake so
 			// the remote identity is known (but hasn't been verified yet).
-			nodeID := c.node.ID()
+			if trusted[c.node.ID()] {
-			if trusted[nodeID] {
 				// Ensure that the trusted flag is set before checking against MaxPeers.
 				c.flags |= trustedConn
 			}
@@ -696,14 +695,12 @@ running:
 		case c := <-srv.checkpointAddPeer:
 			// At this point the connection is past the protocol handshake.
 			// Its capabilities are known and the remote identity is verified.
-			nodeID := c.node.ID()
 			err := srv.addPeerChecks(peers, inboundCount, c)
 			if err == nil {
 				// The handshakes are done and it passed all checks.
 				p := srv.launchPeer(c)
-				peers[nodeID] = p
+				peers[c.node.ID()] = p
-				srv.log.Debug("Adding p2p peer", "peercount", len(peers), "id", p.ID(),
+				srv.log.Debug("Adding p2p peer", "peercount", len(peers), "id", p.ID(), "conn", c.flags, "addr", p.RemoteAddr(), "name", p.Name())
-					"conn", c.flags, "addr", p.RemoteAddr(), "name", p.Name())
 				srv.dialsched.peerAdded(c)
 				if p.Inbound() {
 					inboundCount++
@@ -720,10 +717,8 @@ running:
 		case pd := <-srv.delpeer:
 			// A peer disconnected.
 			d := common.PrettyDuration(mclock.Now() - pd.created)
-			nodeID := pd.ID()
+			delete(peers, pd.ID())
-			delete(peers, nodeID)
+			srv.log.Debug("Removing p2p peer", "peercount", len(peers), "id", pd.ID(), "duration", d, "req", pd.requested, "err", pd.err)
-			srv.log.Debug("Removing p2p peer", "peercount", len(peers), "id", nodeID,
-				"duration", d, "req", pd.requested, "err", pd.err)
 			srv.dialsched.peerRemoved(pd.rw)
 			if pd.Inbound() {
 				inboundCount--