Merge pull request #2548 from guggero/netsync-simnet-sync-candidate

netsync: require block-serving services on regtest/simnet sync peers

netsync/manager.go

@@ -360,38 +360,13 @@ func (sm *SyncManager) startSync() {
 // isSyncCandidate returns whether or not the peer is a candidate to consider
 // syncing from.
 func (sm *SyncManager) isSyncCandidate(peer *peerpkg.Peer) bool {
-	// Typically a peer is not a candidate for sync if it's not a full node,
-	// however regression test is special in that the regression tool is
-	// not a full node and still needs to be considered a sync candidate.
-	switch sm.chainParams.Name {
-	case chaincfg.RegressionNetParams.Name, chaincfg.SimNetParams.Name:
-		// In regtest/simnet mode, any peer is a valid sync candidate
-		// regardless of its address or service flags. This allows
-		// syncing from peers on non-localhost networks such as Docker
-		// bridge networks.
-		return true
-	}
-
-	// If the segwit soft-fork package has activated, then the peer must
-	// also be upgraded.
-	segwitActive, err := sm.chain.IsDeploymentActive(
-		chaincfg.DeploymentSegwit,
-	)
-	if err != nil {
-		log.Errorf("Unable to query for segwit soft-fork state: %v",
-			err)
-	}
-
-	if segwitActive && !peer.IsWitnessEnabled() {
-		return false
-	}
-
 	var (
 		nodeServices = peer.Services()
 		fullNode     = nodeServices.HasFlag(wire.SFNodeNetwork)
 		prunedNode   = nodeServices.HasFlag(wire.SFNodeNetworkLimited)
 	)
 
+	// We check the node's ability to serve blocks first.
 	switch {
 	case fullNode:
 		// Node is a sync candidate if it has all the blocks.
@@ -418,6 +393,29 @@ func (sm *SyncManager) isSyncCandidate(peer *peerpkg.Peer) bool {
 		return false
 	}
 
+	// We can skip the deployment requirement for local test networks.
+	switch sm.chainParams.Name {
+	case chaincfg.RegressionNetParams.Name, chaincfg.SimNetParams.Name:
+		// Being able to serve blocks in the range we need is the only
+		// requirement for regtest and simnet. Any light clients such as
+		// Neutrino would fail above already.
+		return true
+	}
+
+	// If the segwit soft-fork package has activated, then the peer must
+	// also be upgraded.
+	segwitActive, err := sm.chain.IsDeploymentActive(
+		chaincfg.DeploymentSegwit,
+	)
+	if err != nil {
+		log.Errorf("Unable to query for segwit soft-fork state: %v",
+			err)
+	}
+
+	if segwitActive && !peer.IsWitnessEnabled() {
+		return false
+	}
+
 	// Candidate if all checks passed.
 	return true
 }

netsync/manager_test.go

@@ -1220,9 +1220,8 @@ func TestStartSyncChainCurrent(t *testing.T) {
 		"ibdMode should not be activated when chain is already current")
 }
 
-// TestIsSyncCandidateRegtest verifies that isSyncCandidate accepts any peer
-// on regtest regardless of address, including non-localhost Docker bridge
-// addresses.
+// TestIsSyncCandidateRegtest verifies that isSyncCandidate accepts peers
+// on regtest and simnet based on their service flags.
 func TestIsSyncCandidateRegtest(t *testing.T) {
 	t.Parallel()
 
@@ -1231,29 +1230,41 @@ func TestIsSyncCandidateRegtest(t *testing.T) {
 	defer tearDown()
 
 	tests := []struct {
-		name string
-		addr string
-		want bool
+		name      string
+		flags     wire.ServiceFlag
+		lastBlock int32
+		want      bool
 	}{
 		{
-			name: "localhost",
-			addr: "127.0.0.1:18444",
-			want: true,
+			name:  "just node network",
+			flags: wire.SFNodeNetwork,
+			want:  true,
 		},
 		{
-			name: "docker bridge ip",
-			addr: "172.18.0.2:18444",
-			want: true,
+			name:  "just limited network",
+			flags: wire.SFNodeNetworkLimited,
+			want:  true,
 		},
 		{
-			name: "remote ip",
-			addr: "93.184.216.34:18444",
-			want: true,
+			name:      "limited network with block ahead",
+			flags:     wire.SFNodeNetworkLimited,
+			lastBlock: wire.NodeNetworkLimitedBlockThreshold + 1,
+			want:      false,
 		},
 		{
-			name: "ipv6 loopback",
-			addr: "[::1]:18444",
-			want: true,
+			name:  "node network and limited node network",
+			flags: wire.SFNodeNetwork | wire.SFNodeNetworkLimited,
+			want:  true,
+		},
+		{
+			name:  "no flags",
+			flags: 0,
+			want:  false,
+		},
+		{
+			name:  "different flag",
+			flags: wire.SFNodeBloom,
+			want:  false,
 		},
 	}
 
@@ -1261,7 +1272,9 @@ func TestIsSyncCandidateRegtest(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			p := peer.NewInboundPeer(&peer.Config{
 				ChainParams: sm.chainParams,
+				Services:    tc.flags,
 			})
+			p.UpdateLastBlockHeight(tc.lastBlock)
 
 			got := sm.isSyncCandidate(p)
 			require.Equal(t, tc.want, got)