phonebook: Persist initial phonebook peers; remove unused ExtendPeerL…

…ist (#5615)

network/phonebook.go

@@ -68,8 +68,9 @@ type Phonebook interface {
 	// matching entries don't change
 	ReplacePeerList(dnsAddresses []string, networkName string, role PhoneBookEntryRoles)
 
-	// ExtendPeerList adds unique addresses to this set of addresses
-	ExtendPeerList(more []string, networkName string, role PhoneBookEntryRoles)
+	// AddPersistentPeers stores addresses of peers which are persistent.
+	// i.e. they won't be replaced by ReplacePeerList calls
+	AddPersistentPeers(dnsAddresses []string, networkName string, role PhoneBookEntryRoles)
 }
 
 // addressData: holds the information associated with each phonebook address.
@@ -86,14 +87,18 @@ type addressData struct {
 
 	// role is the role that this address serves.
 	role PhoneBookEntryRoles
+
+	// persistent is set true for peers whose record should not be removed for the peer list
+	persistent bool
 }
 
 // makePhonebookEntryData creates a new addressData entry for provided network name and role.
-func makePhonebookEntryData(networkName string, role PhoneBookEntryRoles) addressData {
+func makePhonebookEntryData(networkName string, role PhoneBookEntryRoles, persistent bool) addressData {
 	pbData := addressData{
 		networkNames:          make(map[string]bool),
 		recentConnectionTimes: make([]time.Time, 0),
 		role:                  role,
+		persistent:            persistent,
 	}
 	pbData.networkNames[networkName] = true
 	return pbData
@@ -165,7 +170,7 @@ func (e *phonebookImpl) ReplacePeerList(addressesThey []string, networkName stri
 	// prepare a map of items we'd like to remove.
 	removeItems := make(map[string]bool, 0)
 	for k, pbd := range e.data {
-		if pbd.networkNames[networkName] && pbd.role == role {
+		if pbd.networkNames[networkName] && pbd.role == role && !pbd.persistent {
 			removeItems[k] = true
 		}
 	}
@@ -180,7 +185,7 @@ func (e *phonebookImpl) ReplacePeerList(addressesThey []string, networkName stri
 			delete(removeItems, addr)
 		} else {
 			// we don't have this item. add it.
-			e.data[addr] = makePhonebookEntryData(networkName, role)
+			e.data[addr] = makePhonebookEntryData(networkName, role, false)
 		}
 	}
 
@@ -190,6 +195,23 @@ func (e *phonebookImpl) ReplacePeerList(addressesThey []string, networkName stri
 	}
 }
 
+func (e *phonebookImpl) AddPersistentPeers(dnsAddresses []string, networkName string, role PhoneBookEntryRoles) {
+	e.lock.Lock()
+	defer e.lock.Unlock()
+
+	for _, addr := range dnsAddresses {
+		if pbData, has := e.data[addr]; has {
+			// we already have this.
+			// Make sure the persistence field is set to true
+			pbData.persistent = true
+
+		} else {
+			// we don't have this item. add it.
+			e.data[addr] = makePhonebookEntryData(networkName, role, true)
+		}
+	}
+}
+
 func (e *phonebookImpl) UpdateRetryAfter(addr string, retryAfter time.Time) {
 	e.lock.Lock()
 	defer e.lock.Unlock()
@@ -316,19 +338,6 @@ func (e *phonebookImpl) GetAddresses(n int, role PhoneBookEntryRoles) []string {
 	return shuffleSelect(e.filterRetryTime(time.Now(), role), n)
 }
 
-// ExtendPeerList adds unique addresses to this set of addresses
-func (e *phonebookImpl) ExtendPeerList(more []string, networkName string, role PhoneBookEntryRoles) {
-	e.lock.Lock()
-	defer e.lock.Unlock()
-	for _, addr := range more {
-		if pbEntry, has := e.data[addr]; has {
-			pbEntry.networkNames[networkName] = true
-			continue
-		}
-		e.data[addr] = makePhonebookEntryData(networkName, role)
-	}
-}
-
 // Length returns the number of addrs contained
 func (e *phonebookImpl) Length() int {
 	e.lock.RLock()

network/phonebook_test.go

@@ -17,14 +17,10 @@
 package network
 
 import (
-	"fmt"
-	"math/rand"
-	"sync"
 	"testing"
 	"time"
 
 	"github.com/algorand/go-algorand/test/partitiontest"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -92,7 +88,7 @@ func TestArrayPhonebookAll(t *testing.T) {
 	set := []string{"a", "b", "c", "d", "e"}
 	ph := MakePhonebook(1, 1).(*phonebookImpl)
 	for _, e := range set {
-		ph.data[e] = makePhonebookEntryData("", PhoneBookEntryRelayRole)
+		ph.data[e] = makePhonebookEntryData("", PhoneBookEntryRelayRole, false)
 	}
 	testPhonebookAll(t, set, ph)
 }
@@ -103,7 +99,7 @@ func TestArrayPhonebookUniform1(t *testing.T) {
 	set := []string{"a", "b", "c", "d", "e"}
 	ph := MakePhonebook(1, 1).(*phonebookImpl)
 	for _, e := range set {
-		ph.data[e] = makePhonebookEntryData("", PhoneBookEntryRelayRole)
+		ph.data[e] = makePhonebookEntryData("", PhoneBookEntryRelayRole, false)
 	}
 	testPhonebookUniform(t, set, ph, 1)
 }
@@ -114,91 +110,39 @@ func TestArrayPhonebookUniform3(t *testing.T) {
 	set := []string{"a", "b", "c", "d", "e"}
 	ph := MakePhonebook(1, 1).(*phonebookImpl)
 	for _, e := range set {
-		ph.data[e] = makePhonebookEntryData("", PhoneBookEntryRelayRole)
+		ph.data[e] = makePhonebookEntryData("", PhoneBookEntryRelayRole, false)
 	}
 	testPhonebookUniform(t, set, ph, 3)
 }
 
-// TestPhonebookExtension tests for extending different phonebooks with
-// addresses.
-func TestPhonebookExtension(t *testing.T) {
-	partitiontest.PartitionTest(t)
-
-	setA := []string{"a"}
-	moreB := []string{"b"}
-	ph := MakePhonebook(1, 1).(*phonebookImpl)
-	ph.ReplacePeerList(setA, "default", PhoneBookEntryRelayRole)
-	ph.ExtendPeerList(moreB, "default", PhoneBookEntryRelayRole)
-	ph.ExtendPeerList(setA, "other", PhoneBookEntryRelayRole)
-	assert.Equal(t, 2, ph.Length())
-	assert.Equal(t, true, ph.data["a"].networkNames["default"])
-	assert.Equal(t, true, ph.data["a"].networkNames["other"])
-	assert.Equal(t, true, ph.data["b"].networkNames["default"])
-	assert.Equal(t, false, ph.data["b"].networkNames["other"])
-}
-
-func extenderThread(th *phonebookImpl, more []string, wg *sync.WaitGroup, repetitions int) {
-	defer wg.Done()
-	for i := 0; i <= repetitions; i++ {
-		start := rand.Intn(len(more))
-		end := rand.Intn(len(more)-start) + start
-		th.ExtendPeerList(more[start:end], "default", PhoneBookEntryRelayRole)
-	}
-	th.ExtendPeerList(more, "default", PhoneBookEntryRelayRole)
-}
-
-func TestThreadsafePhonebookExtension(t *testing.T) {
-	partitiontest.PartitionTest(t)
-
-	set := []string{"a", "b", "c", "d", "e"}
-	more := []string{"f", "g", "h", "i", "j"}
-	ph := MakePhonebook(1, 1).(*phonebookImpl)
-	ph.ReplacePeerList(set, "default", PhoneBookEntryRelayRole)
-	wg := sync.WaitGroup{}
-	wg.Add(5)
-	for ti := 0; ti < 5; ti++ {
-		go extenderThread(ph, more, &wg, 1000)
-	}
-	wg.Wait()
-
-	assert.Equal(t, 10, ph.Length())
-}
-
-func threadTestThreadsafePhonebookExtensionLong(wg *sync.WaitGroup, ph *phonebookImpl, setSize, repetitions int) {
-	set := make([]string, setSize)
-	for i := range set {
-		set[i] = fmt.Sprintf("%06d", i)
-	}
-	rand.Shuffle(len(set), func(i, j int) { t := set[i]; set[i] = set[j]; set[j] = t })
-	extenderThread(ph, set, wg, repetitions)
-}
-
-func TestThreadsafePhonebookExtensionLong(t *testing.T) {
+func TestMultiPhonebook(t *testing.T) {
 	partitiontest.PartitionTest(t)
 
-	if testing.Short() {
-		t.SkipNow()
-		return
+	set := []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j"}
+	pha := make([]string, 0)
+	for _, e := range set[:5] {
+		pha = append(pha, e)
 	}
-	ph := MakePhonebook(1, 1).(*phonebookImpl)
-	wg := sync.WaitGroup{}
-	const threads = 5
-	const setSize = 1000
-	const repetitions = 100
-	wg.Add(threads)
-	for i := 0; i < threads; i++ {
-		go threadTestThreadsafePhonebookExtensionLong(&wg, ph, setSize, repetitions)
+	phb := make([]string, 0)
+	for _, e := range set[5:] {
+		phb = append(phb, e)
 	}
+	mp := MakePhonebook(1, 1*time.Millisecond)
+	mp.ReplacePeerList(pha, "pha", PhoneBookEntryRelayRole)
+	mp.ReplacePeerList(phb, "phb", PhoneBookEntryRelayRole)
 
-	wg.Wait()
-
-	assert.Equal(t, setSize, ph.Length())
+	testPhonebookAll(t, set, mp)
+	testPhonebookUniform(t, set, mp, 1)
+	testPhonebookUniform(t, set, mp, 3)
 }
 
-func TestMultiPhonebook(t *testing.T) {
+// TestMultiPhonebookPersistentPeers validates that the peers added via Phonebook.AddPersistentPeers
+// are not replaced when Phonebook.ReplacePeerList is called
+func TestMultiPhonebookPersistentPeers(t *testing.T) {
 	partitiontest.PartitionTest(t)
 
-	set := []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j"}
+	persistentPeers := []string{"a"}
+	set := []string{"b", "c", "d", "e", "f", "g", "h", "i", "j", "k"}
 	pha := make([]string, 0)
 	for _, e := range set[:5] {
 		pha = append(pha, e)
@@ -208,12 +152,16 @@ func TestMultiPhonebook(t *testing.T) {
 		phb = append(phb, e)
 	}
 	mp := MakePhonebook(1, 1*time.Millisecond)
+	mp.AddPersistentPeers(persistentPeers, "pha", PhoneBookEntryRelayRole)
+	mp.AddPersistentPeers(persistentPeers, "phb", PhoneBookEntryRelayRole)
 	mp.ReplacePeerList(pha, "pha", PhoneBookEntryRelayRole)
 	mp.ReplacePeerList(phb, "phb", PhoneBookEntryRelayRole)
 
-	testPhonebookAll(t, set, mp)
-	testPhonebookUniform(t, set, mp, 1)
-	testPhonebookUniform(t, set, mp, 3)
+	testPhonebookAll(t, append(set, persistentPeers...), mp)
+	allAddresses := mp.GetAddresses(len(set)+len(persistentPeers), PhoneBookEntryRelayRole)
+	for _, pp := range persistentPeers {
+		require.Contains(t, allAddresses, pp)
+	}
 }
 
 func TestMultiPhonebookDuplicateFiltering(t *testing.T) {
@@ -388,21 +336,6 @@ func TestWaitAndAddConnectionTimeShortWindow(t *testing.T) {
 	require.Equal(t, 1, len(phBookData))
 }
 
-func BenchmarkThreadsafePhonebook(b *testing.B) {
-	ph := MakePhonebook(1, 1).(*phonebookImpl)
-	threads := 5
-	if b.N < threads {
-		threads = b.N
-	}
-	wg := sync.WaitGroup{}
-	wg.Add(threads)
-	repetitions := b.N / threads
-	for t := 0; t < threads; t++ {
-		go threadTestThreadsafePhonebookExtensionLong(&wg, ph, 1000, repetitions)
-	}
-	wg.Wait()
-}
-
 // TestPhonebookRoles tests that the filtering by roles for different
 // phonebooks entries works as expected.
 func TestPhonebookRoles(t *testing.T) {

network/wsNetwork.go

@@ -2483,7 +2483,7 @@ func (wn *WebsocketNetwork) SetPeerData(peer Peer, key string, value interface{}
 func NewWebsocketNetwork(log logging.Logger, config config.Local, phonebookAddresses []string, genesisID string, networkID protocol.NetworkID, nodeInfo NodeInfo) (wn *WebsocketNetwork, err error) {
 	phonebook := MakePhonebook(config.ConnectionsRateLimitingCount,
 		time.Duration(config.ConnectionsRateLimitingWindowSeconds)*time.Second)
-	phonebook.ReplacePeerList(phonebookAddresses, string(networkID), PhoneBookEntryRelayRole)
+	phonebook.AddPersistentPeers(phonebookAddresses, string(networkID), PhoneBookEntryRelayRole)
 	wn = &WebsocketNetwork{
 		log:               log,
 		config:            config,

network/wsNetwork_test.go

@@ -4308,6 +4308,40 @@ func TestUpdatePhonebookAddresses(t *testing.T) {
 	})
 }
 
+func TestUpdatePhonebookAddressesPersistentPeers(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	rapid.Check(t, func(t1 *rapid.T) {
+		nw := makeTestWebsocketNode(t)
+		// Generate a new set of relay domains
+		// Dont overlap with archive nodes previously specified, duplicates between them not stored in phonebook as of this writing
+		relayDomainsGen := rapid.SliceOfN(rapidgen.DomainOf(253, 63, "", nil), 0, 200)
+		relayDomains := relayDomainsGen.Draw(t1, "relayDomains")
+
+		var persistentPeers []string
+		// Add an initial set of relay domains as Persistent Peers in the Phonebook,
+		persistentPeers = rapid.SliceOfN(rapidgen.DomainOf(253, 63, "", relayDomains), 0, 200).Draw(t1, "")
+		nw.phonebook.AddPersistentPeers(persistentPeers, string(nw.NetworkID), PhoneBookEntryRelayRole)
+
+		// run updatePhonebookAddresses
+		nw.updatePhonebookAddresses(relayDomains, nil)
+
+		// Check that entries are in fact in phonebook less any duplicates
+		dedupedRelayDomains := removeDuplicateStr(relayDomains, false)
+		require.Equal(t, 0, len(relayDomains)-len(dedupedRelayDomains))
+
+		relayPeers := nw.GetPeers(PeersPhonebookRelays)
+		require.Equal(t, len(dedupedRelayDomains)+len(persistentPeers), len(relayPeers))
+
+		relayAddrs := make([]string, 0, len(relayPeers))
+		for _, peer := range relayPeers {
+			relayAddrs = append(relayAddrs, peer.(HTTPPeer).GetAddress())
+		}
+
+		require.ElementsMatch(t, append(dedupedRelayDomains, persistentPeers...), relayAddrs)
+	})
+}
+
 func removeDuplicateStr(strSlice []string, lowerCase bool) []string {
 	allKeys := make(map[string]bool)
 	var dedupStrSlice = make([]string, 0)