update per reviewer's comments.

catchup/peerSelector.go

@@ -54,16 +54,16 @@ const (
 var errPeerSelectorNoPeerPoolsAvailable = errors.New("no peer pools available")
 
 // peerClass defines the type of peer we want to have in a particular "class",
-// and define tnet network.PeerOption that would be used to retrieve that type of
+// and define the network.PeerOption that would be used to retrieve that type of
 // peer
 type peerClass struct {
 	initialRank int
 	peerClass   network.PeerOption
 }
 
-// the networkGetPeers is a subset of the network.GossipNode used to ensure that we can create an instance of the peerSelector
+// the peersRetriever is a subset of the network.GossipNode used to ensure that we can create an instance of the peerSelector
 // for testing purposes, providing just the above function.
-type networkGetPeers interface {
+type peersRetriever interface {
 	// Get a list of Peers we could potentially send a direct message to.
 	GetPeers(options ...network.PeerOption) []network.Peer
 }
@@ -87,22 +87,23 @@ type peerPool struct {
 // query(s) take advantage of that intel.
 type peerSelector struct {
 	mu          deadlock.Mutex
-	net         networkGetPeers
+	net         peersRetriever
 	peerClasses []peerClass
 	pools       []peerPool
 }
 
-// makePeerSelector creates a peerSelector, given a networkGetPeers and peerClass array.
-func makePeerSelector(net networkGetPeers, initialPeersClasses []peerClass) *peerSelector {
+// makePeerSelector creates a peerSelector, given a peersRetriever and peerClass array.
+func makePeerSelector(net peersRetriever, initialPeersClasses []peerClass) *peerSelector {
 	selector := &peerSelector{
 		net:         net,
 		peerClasses: initialPeersClasses,
 	}
 	return selector
 }
 
-// GetNextPeer returns the next peer. It randomally selects a peer from the lowest
-// rank pool.
+// GetNextPeer returns the next peer. It randomally selects a peer from a pool that has
+// the lowest rank value. Given that the peers are grouped by their ranks, allow us to
+// prioritize peers based on their class and/or performance.
 func (ps *peerSelector) GetNextPeer() (peer network.Peer, err error) {
 	ps.mu.Lock()
 	defer ps.mu.Unlock()
@@ -138,19 +139,22 @@ func (ps *peerSelector) RankPeer(peer network.Peer, rank int) bool {
 
 	// we need to remove the peer from the pool so we can place it in a different location.
 	pool := ps.pools[poolIdx]
-	class := pool.peers[peerIdx].class
-	if len(pool.peers) > 1 {
-		pool.peers = append(pool.peers[:peerIdx], pool.peers[peerIdx+1:]...)
-		ps.pools[poolIdx] = pool
-	} else {
-		// the last peer was removed from the pool; delete this pool.
-		ps.pools = append(ps.pools[:poolIdx], ps.pools[poolIdx+1:]...)
-	}
+	if pool.rank != rank {
+		class := pool.peers[peerIdx].class
+		if len(pool.peers) > 1 {
+			pool.peers = append(pool.peers[:peerIdx], pool.peers[peerIdx+1:]...)
+			ps.pools[poolIdx] = pool
+		} else {
+			// the last peer was removed from the pool; delete this pool.
+			ps.pools = append(ps.pools[:poolIdx], ps.pools[poolIdx+1:]...)
+		}
 
-	sortNeeded := ps.addToPool(peer, rank, class)
-	if sortNeeded {
-		ps.sort()
+		sortNeeded := ps.addToPool(peer, rank, class)
+		if sortNeeded {
+			ps.sort()
+		}
 	}
+
 	return true
 }
 
@@ -277,11 +281,13 @@ func (ps *peerSelector) findPeer(peer network.Peer) (poolIdx, peerIdx int) {
 
 // calculate the duration rank by mapping the range of [minDownloadDuration..maxDownloadDuration] into the rank range of [minRank..maxRank]
 func downloadDurationToRank(downloadDuration, minDownloadDuration, maxDownloadDuration time.Duration, minRank, maxRank int) (rank int) {
+	// clamp the downloadDuration into the range of [minDownloadDuration .. maxDownloadDuration]
 	if downloadDuration < minDownloadDuration {
 		downloadDuration = minDownloadDuration
 	} else if downloadDuration > maxDownloadDuration {
 		downloadDuration = maxDownloadDuration
 	}
+	// the formula below maps an element in the range of [minDownloadDuration .. maxDownloadDuration] onto the range of [minRank .. maxRank]
 	rank = minRank + int((downloadDuration-minDownloadDuration).Nanoseconds()*int64(maxRank-minRank)/(maxDownloadDuration-minDownloadDuration).Nanoseconds())
 	return
 }

catchup/peerSelector_test.go

@@ -95,24 +95,24 @@ func TestDownloadDurationToRank(t *testing.T) {
 	require.Equal(t, 0, downloadDurationToRank(205*time.Millisecond, 100*time.Millisecond, 200*time.Millisecond, 0, 0))
 }
 
-type networkGetPeersStub struct {
+type peersRetrieverStub struct {
 	getPeersStub func(options ...network.PeerOption) []network.Peer
 }
 
-func (n *networkGetPeersStub) GetPeers(options ...network.PeerOption) []network.Peer {
+func (n *peersRetrieverStub) GetPeers(options ...network.PeerOption) []network.Peer {
 	return n.getPeersStub(options...)
 }
 
-func makeNetworkGetPeersStub(fnc func(options ...network.PeerOption) []network.Peer) *networkGetPeersStub {
-	return &networkGetPeersStub{
+func makePeersRetrieverStub(fnc func(options ...network.PeerOption) []network.Peer) *peersRetrieverStub {
+	return &peersRetrieverStub{
 		getPeersStub: fnc,
 	}
 }
 func TestPeerSelector(t *testing.T) {
 	peers := []network.Peer{&mockHTTPPeer{address: "12345"}}
 
 	peerSelector := makePeerSelector(
-		makeNetworkGetPeersStub(func(options ...network.PeerOption) []network.Peer {
+		makePeersRetrieverStub(func(options ...network.PeerOption) []network.Peer {
 			return peers
 		}), []peerClass{{initialRank: peerRankInitialFirstPriority, peerClass: network.PeersPhonebookArchivers}},
 	)
@@ -163,7 +163,7 @@ func TestPeerDownloadRanking(t *testing.T) {
 	peers2 := []network.Peer{&mockHTTPPeer{address: "abcd"}, &mockHTTPPeer{address: "efgh"}}
 
 	peerSelector := makePeerSelector(
-		makeNetworkGetPeersStub(func(options ...network.PeerOption) (peers []network.Peer) {
+		makePeersRetrieverStub(func(options ...network.PeerOption) (peers []network.Peer) {
 			for _, opt := range options {
 				if opt == network.PeersPhonebookArchivers {
 					peers = append(peers, peers1...)
@@ -209,7 +209,7 @@ func TestPeerDownloadRanking(t *testing.T) {
 
 func TestFindMissingPeer(t *testing.T) {
 	peerSelector := makePeerSelector(
-		makeNetworkGetPeersStub(func(options ...network.PeerOption) []network.Peer {
+		makePeersRetrieverStub(func(options ...network.PeerOption) []network.Peer {
 			return []network.Peer{}
 		}), []peerClass{{initialRank: peerRankInitialFirstPriority, peerClass: network.PeersPhonebookArchivers}},
 	)