[NOD-1060] Don't sync from misbehaving peer (#768)

* [NOD-1038] Give higher priority for requesting missing ancestors when sending a getdata message (#767)

* [NOD-1060] Don't sync from peers that break the netsync protocol

netsync/manager.go

@@ -65,12 +65,6 @@ type donePeerMsg struct {
 	peer *peerpkg.Peer
 }
 
-// removeFromSyncCandidatesMsg signifies to remove the given peer
-// from the sync candidates.
-type removeFromSyncCandidatesMsg struct {
-	peer *peerpkg.Peer
-}
-
 // txMsg packages a kaspa tx message and the peer it came from together
 // so the block handler has access to that information.
 type txMsg struct {
@@ -350,6 +344,8 @@ func (sm *SyncManager) handleDonePeerMsg(peer *peerpkg.Peer) {
 	sm.stopSyncFromPeer(peer)
 }
 
+// stopSyncFromPeer replaces a sync peer if the given peer
+// is the sync peer.
 func (sm *SyncManager) stopSyncFromPeer(peer *peerpkg.Peer) {
 	if sm.syncPeer == peer {
 		sm.syncPeer = nil
@@ -357,7 +353,10 @@ func (sm *SyncManager) stopSyncFromPeer(peer *peerpkg.Peer) {
 	}
 }
 
-func (sm *SyncManager) handleRemoveFromSyncCandidatesMsg(peer *peerpkg.Peer) {
+// RemoveFromSyncCandidates removes the given peer from being
+// a sync candidate and stop syncing from it if it's the current
+// sync peer.
+func (sm *SyncManager) RemoveFromSyncCandidates(peer *peerpkg.Peer) {
 	sm.peerStates[peer].syncCandidate = false
 	sm.stopSyncFromPeer(peer)
 }
@@ -522,6 +521,9 @@ func (sm *SyncManager) handleBlockMsg(bmsg *blockMsg) {
 
 		peer.AddBanScoreAndPushRejectMsg(wire.CmdBlock, wire.RejectInvalid, blockHash,
 			peerpkg.BanScoreInvalidBlock, 0, fmt.Sprintf("got invalid block: %s", err))
+		// Whether the peer will be banned or not, syncing from a node that doesn't follow
+		// the netsync protocol is undesired.
+		sm.RemoveFromSyncCandidates(peer)
 		return
 	}
 
@@ -739,6 +741,9 @@ func (sm *SyncManager) handleInvMsg(imsg *invMsg) {
 			if sm.dag.IsKnownInvalid(iv.Hash) {
 				peer.AddBanScoreAndPushRejectMsg(imsg.inv.Command(), wire.RejectInvalid, iv.Hash,
 					peerpkg.BanScoreInvalidInvBlock, 0, fmt.Sprintf("sent inv of invalid block %s", iv.Hash))
+				// Whether the peer will be banned or not, syncing from a node that doesn't follow
+				// the netsync protocol is undesired.
+				sm.RemoveFromSyncCandidates(peer)
 				return
 			}
 			// The block is an orphan block that we already have.
@@ -758,7 +763,7 @@ func (sm *SyncManager) handleInvMsg(imsg *invMsg) {
 						fmt.Sprintf("sent inv of orphan block %s as part of netsync", iv.Hash))
 					// Whether the peer will be banned or not, syncing from a node that doesn't follow
 					// the netsync protocol is undesired.
-					sm.stopSyncFromPeer(peer)
+					sm.RemoveFromSyncCandidates(peer)
 					return
 				}
 				missingAncestors, err := sm.dag.GetOrphanMissingAncestorHashes(iv.Hash)
@@ -993,9 +998,6 @@ out:
 			case *donePeerMsg:
 				sm.handleDonePeerMsg(msg.peer)
 
-			case *removeFromSyncCandidatesMsg:
-				sm.handleRemoveFromSyncCandidatesMsg(msg.peer)
-
 			case getSyncPeerMsg:
 				var peerID int32
 				if sm.syncPeer != nil {
@@ -1145,17 +1147,6 @@ func (sm *SyncManager) DonePeer(peer *peerpkg.Peer) {
 	sm.msgChan <- &donePeerMsg{peer: peer}
 }
 
-// RemoveFromSyncCandidates tells the blockmanager to remove a peer as
-// a sync candidate.
-func (sm *SyncManager) RemoveFromSyncCandidates(peer *peerpkg.Peer) {
-	// Ignore if we are shutting down.
-	if atomic.LoadInt32(&sm.shutdown) != 0 {
-		return
-	}
-
-	sm.msgChan <- &removeFromSyncCandidatesMsg{peer: peer}
-}
-
 // Start begins the core block handler which processes block and inv messages.
 func (sm *SyncManager) Start() {
 	// Already started?

peer/banscores.go

@@ -26,7 +26,9 @@ const (
 
 	BanScoreInvalidMsgGetBlockInvs = 10
 
-	BanScoreInvalidMsgBlockLocator = 100
+	BanScoreInvalidMsgGetBlockLocator = 100
+
+	BanScoreEmptyBlockLocator = 100
 
 	BanScoreSentTxToBlocksOnly = 20
 

server/p2p/on_block_locator.go

@@ -15,8 +15,12 @@ func (sp *Peer) OnBlockLocator(_ *peer.Peer, msg *wire.MsgBlockLocator) {
 	// the peer in order to find it in the next iteration.
 	dag := sp.server.DAG
 	if len(msg.BlockLocatorHashes) == 0 {
-		peerLog.Warnf("Got empty block locator from peer %s",
+		sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectInvalid, nil,
-			sp)
+			peer.BanScoreEmptyBlockLocator, 0,
+			"got empty block locator")
+		// Whether the peer will be banned or not, syncing from a node that doesn't follow
+		// the netsync protocol is undesired.
+		sp.server.SyncManager.RemoveFromSyncCandidates(sp.Peer)
 		return
 	}
 	// If the first hash of the block locator is known, it means we found

server/p2p/on_get_block_locator.go

@@ -16,7 +16,7 @@ func (sp *Peer) OnGetBlockLocator(_ *peer.Peer, msg *wire.MsgGetBlockLocator) {
 				"%s and %s that was requested from peer %s: %s", msg.HighHash, msg.LowHash, sp, err)
 		}
 		sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectInvalid, nil,
-			peer.BanScoreInvalidMsgBlockLocator, 0,
+			peer.BanScoreInvalidMsgGetBlockLocator, 0,
 			fmt.Sprintf("couldn't build a block locator between blocks %s and %s", msg.HighHash, msg.LowHash))
 		return
 	}