p2p: fix infinite loop in addrbook

CHANGELOG_PENDING.md

@@ -23,3 +23,4 @@ Special thanks to external contributors on this release:
 
 ### BUG FIXES:
 - [node] \#3186 EventBus and indexerService should be started before first block (for replay last block on handshake) execution
+- [p2p] \#3232 Fix infinite loop leading to deadlock in addrbook

p2p/pex/addrbook.go

@@ -408,11 +408,26 @@ func (a *addrBook) GetSelectionWithBias(biasTowardsNewAddrs int) []*p2p.NetAddre
 	newBucketToAddrsMap := make(map[int]map[string]struct{})
 	var newIndex int
 
+	// initialize counters used to count old and new added addresses.
+	// len(oldBucketToAddrsMap) cannot be used as multiple addresses can endup in the same bucket.
+	var oldAddressesAdded int
+	var newAddressesAdded int
+
+	// number of new addresses that, if possible, should be in the beginning of the selection
+	numRequiredNewAdd := int(math.Round(float64(biasTowardsNewAddrs) / float64(100) * float64(numAddresses)))
+
 	selectionIndex := 0
 ADDRS_LOOP:
 	for selectionIndex < numAddresses {
-		pickFromOldBucket := int((float64(selectionIndex)/float64(numAddresses))*100) >= biasTowardsNewAddrs
-		pickFromOldBucket = (pickFromOldBucket && a.nOld > 0) || a.nNew == 0
+		// biasedTowardsOldAddrs indicates if the selection can switch to old addresses
+		biasedTowardsOldAddrs := selectionIndex >= numRequiredNewAdd
+		// An old addresses is selected if:
+		// - the bias is for old and old addressees are still available or,
+		// - there are no new addresses or all new addresses have been selected.
+		pickFromOldBucket :=
+			(biasedTowardsOldAddrs && oldAddressesAdded < a.nOld) || //
+				a.nNew == 0 || newAddressesAdded >= a.nNew
+
 		bucket := make(map[string]*knownAddress)
 
 		// loop until we pick a random non-empty bucket
@@ -450,6 +465,7 @@ ADDRS_LOOP:
 				oldBucketToAddrsMap[oldIndex] = make(map[string]struct{})
 			}
 			oldBucketToAddrsMap[oldIndex][selectedAddr.String()] = struct{}{}
+			oldAddressesAdded++
 		} else {
 			if addrsMap, ok := newBucketToAddrsMap[newIndex]; ok {
 				if _, ok = addrsMap[selectedAddr.String()]; ok {
@@ -459,6 +475,7 @@ ADDRS_LOOP:
 				newBucketToAddrsMap[newIndex] = make(map[string]struct{})
 			}
 			newBucketToAddrsMap[newIndex][selectedAddr.String()] = struct{}{}
+			newAddressesAdded++
 		}
 
 		selection[selectionIndex] = selectedAddr

p2p/pex/addrbook_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/hex"
 	"fmt"
 	"io/ioutil"
+	"math"
 	"os"
 	"testing"
 
@@ -36,6 +37,264 @@ func deleteTempFile(fname string) {
 	}
 }
 
+func TestAddrBookGetSelectionWithOneMarkedGood(t *testing.T) {
+	fname := createTempFileName("addrbook_test")
+	defer deleteTempFile(fname)
+
+	book := NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+	assert.Zero(t, book.Size())
+
+	N := 10
+	randAddrs := randNetAddressPairs(t, N)
+	for _, addr := range randAddrs {
+		book.AddAddress(addr.addr, addr.src)
+	}
+
+	// mark one addr as good
+	for _, addr := range randAddrs[:1] {
+		book.MarkGood(addr.addr)
+	}
+
+	// ensure we can get selection.
+	// this used to result in an infinite loop (!)
+	addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+	assert.NotNil(t, addrs, "expected an address")
+}
+
+func TestAddrBookGetSelectionWithOneNotMarkedGood(t *testing.T) {
+	fname := createTempFileName("addrbook_test")
+	defer deleteTempFile(fname)
+
+	// 0 addresses
+	book := NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+	assert.Zero(t, book.Size())
+
+	N := 10
+	randAddrs := randNetAddressPairs(t, N)
+	// mark all addrs but one as good
+	for _, addr := range randAddrs[:N-1] {
+		book.AddAddress(addr.addr, addr.src)
+		book.MarkGood(addr.addr)
+	}
+	// add the last addr, don't mark as good
+	for _, addr := range randAddrs[N-1:] {
+		book.AddAddress(addr.addr, addr.src)
+	}
+
+	// ensure we can get selection.
+	// this used to result in an infinite loop (!)
+	addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+	assert.NotNil(t, addrs, "expected an address")
+}
+
+func min(a, b int) int {
+	if a < b {
+		return a
+	}
+	return b
+}
+
+func listsAreEqual(a, b []int) bool {
+	if (a == nil) != (b == nil) {
+		return false
+	}
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
+}
+
+// Creates a Book with m old and n new addresses.
+func createAddrBook(t *testing.T, m, n int) *addrBook {
+	fname := createTempFileName("addrbook_test")
+	defer deleteTempFile(fname)
+
+	book := NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+	assert.Zero(t, book.Size())
+
+	// add m old addresses
+	randAddrs := randNetAddressPairs(t, m)
+	for _, addr := range randAddrs {
+		book.AddAddress(addr.addr, addr.src)
+		book.MarkGood(addr.addr)
+	}
+	// add n new addresses
+	randAddrs = randNetAddressPairs(t, n)
+	for _, addr := range randAddrs {
+		book.AddAddress(addr.addr, addr.src)
+	}
+
+	return book
+}
+
+// Computes n * bias/100
+func numExpectedAddresses(bias, n int) int {
+	if n == 0 {
+		return 0
+	}
+	return int(math.Round((float64(bias) / float64(100)) * float64(n)))
+}
+
+// Counts the number of Old and New addresses
+func countOldAndNewInAddrs(addrs []*p2p.NetAddress, book *addrBook) (nold, nnew int) {
+	nold = 0
+	nnew = 0
+
+	for _, addr := range addrs {
+		if addr == nil {
+			continue
+		}
+		if book.IsGood(addr) {
+			nold++
+		} else {
+			nnew++
+		}
+	}
+	return nold, nnew
+}
+
+func analyseSelectionLayout(book *addrBook, addrs []*p2p.NetAddress) ([]int, []int, error) {
+	// address types are: 0 - nil, 1 - new, 2 - old
+	prevt := 0
+	var seqLens []int
+	var seqTypes []int
+	cnt := 0
+	for i, addr := range addrs {
+		if addr == nil {
+			err := fmt.Errorf("nil addresses in selection, index %d", i)
+			return nil, nil, err
+		}
+		addrt := 0
+		if book.IsGood(addr) {
+			addrt = 2
+		} else {
+			addrt = 1
+		}
+
+		if addrt != prevt && prevt != 0 {
+			seqLens = append(seqLens, cnt)
+			seqTypes = append(seqTypes, prevt)
+			cnt = 0
+		}
+		cnt++
+		prevt = addrt
+	}
+	seqLens = append(seqLens, cnt)
+	seqTypes = append(seqTypes, prevt)
+	return seqLens, seqTypes, nil
+}
+
+func TestEmptyAddrBookSelection(t *testing.T) {
+	book := createAddrBook(t, 0, 0)
+	addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+	assert.Nil(t, addrs, "expected a nill selection")
+}
+
+func testAddrBookAddressSelection(t *testing.T, b int) {
+	// generate all combinations of old (m) and new (n=b-m) addresses
+	for m := 0; m < b+1; m++ {
+		n := b - m
+		dbgstr := fmt.Sprintf("book: size %d, number new % d, old %d", b, n, m)
+
+		// create book and get selection
+		book := createAddrBook(t, m, n)
+		addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+
+		r := len(addrs)
+		assert.NotNil(t, addrs, "%s - expected a non-nill selection", dbgstr)
+		assert.NotZero(t, r, "%s - expected at least one address in selection", dbgstr)
+
+		// verify that the number of old and new addresses are the ones expected
+		numold, numnew := countOldAndNewInAddrs(addrs, book)
+		assert.Equal(t, r, numold+numnew, "%s - expected selection completely filled", dbgstr)
+
+		// Given:
+		// n - num new addrs, m - num old addrs
+		// k - num new addrs expected in the beginning (based on bias %)
+		// i=min(n, k), j=min(m, r-i)
+		//
+		// We expect this layout:
+		// indices:      0...i-1   i...i+j-1    i+j...r
+		// addresses:    N0..Ni-1  O0..Oj-1     Ni...
+		//
+		// There is at least one partition and at most three.
+		k := numExpectedAddresses(biasToSelectNewPeers, r)
+		i := min(n, k)
+		j := min(m, r-i)
+		expold := j
+		expnew := r - j
+
+		// compute some slack to protect against small differences due to rounding:
+		slack := int(math.Round(float64(100) / float64(len(addrs))))
+
+		// for now don't use slack as numExpectedAddresses() is exactly the formula used by GetSelectionWithBias()
+		if numnew < expnew || numnew > expnew {
+			t.Fatalf("%s - expected new addrs %d (+/- %d), got %d", dbgstr, expnew, slack, numnew)
+		}
+		if numold < expold || numold > expold {
+			t.Fatalf("%s - expected old addrs %d (+/- %d), got %d", dbgstr, expold, slack, numold)
+		}
+
+		// Verify that the order of addresses is correct
+		seqLens, seqTypes, err := analyseSelectionLayout(book, addrs)
+		assert.NoError(t, err, "%s - expected a non-nill selection", dbgstr)
+
+		// Build a list with the expected lengths of partitions and another with the expected types, e.g.:
+		// expseqLens = [10, 22], expseqTypes = [1, 2]
+		// means we expect 10 new (type 1) addresses followed by 22 old (type 2) addresses.
+		var expSeqLens []int
+		var expSeqTypes []int
+		if j == 0 {
+			// all new
+			expSeqLens = []int{r}
+			expSeqTypes = []int{1}
+		} else if i == 0 {
+			// all old
+			expSeqLens = []int{r}
+			expSeqTypes = []int{2}
+		} else if r-i-j == 0 {
+			expSeqLens = []int{i, j}
+			expSeqTypes = []int{1, 2}
+		} else {
+			expSeqLens = []int{i, j, r - i - j}
+			expSeqTypes = []int{1, 2, 1}
+		}
+
+		assert.True(t, listsAreEqual(expSeqLens, seqLens),
+			"%s - expected sequence lengths of old/new %v, got %v",
+			dbgstr, expSeqLens, seqLens)
+		assert.True(t, listsAreEqual(expSeqTypes, seqTypes),
+			"%s - expected sequence lengths of old/new %v, got %v",
+			dbgstr, expSeqLens, seqLens)
+	}
+}
+
+func TestMultipleAddrBookAddressSelection(t *testing.T) {
+	// test books with smaller size, < N
+	N := 32
+	for b := 1; b < N; b++ {
+		testAddrBookAddressSelection(t, b)
+	}
+	// Test for three books with sizes from following ranges
+	ranges := [...][]int{{33, 100}, {100, 175}}
+	var bsizes []int
+	for _, r := range ranges {
+		bsizes = append(bsizes, cmn.RandIntn(r[1]-r[0])+r[0])
+	}
+	fmt.Printf("Testing address selection for the following booksizes %v\n", bsizes)
+	for _, b := range bsizes {
+		testAddrBookAddressSelection(t, b)
+	}
+}
+
 func TestAddrBookPickAddress(t *testing.T) {
 	fname := createTempFileName("addrbook_test")
 	defer deleteTempFile(fname)
@@ -335,9 +594,16 @@ func TestAddrBookGetSelectionWithBias(t *testing.T) {
 			good++
 		}
 	}
+
 	got, expected := int((float64(good)/float64(len(selection)))*100), (100 - biasTowardsNewAddrs)
-	if got >= expected {
-		t.Fatalf("expected more good peers (%% got: %d, %% expected: %d, number of good addrs: %d, total: %d)", got, expected, good, len(selection))
+
+	// compute some slack to protect against small differences due to rounding:
+	slack := int(math.Round(float64(100) / float64(len(selection))))
+	if got > expected+slack {
+		t.Fatalf("got more good peers (%% got: %d, %% expected: %d, number of good addrs: %d, total: %d)", got, expected, good, len(selection))
+	}
+	if got < expected-slack {
+		t.Fatalf("got fewer good peers (%% got: %d, %% expected: %d, number of good addrs: %d, total: %d)", got, expected, good, len(selection))
 	}
 }