p2p: fix infinite loop in addrbook (#3232)

* failing test

* fix infinite loop in addrbook

There are cases where we only have a small number of addresses marked
good ("old"), but the selection mechanism keeps trying to select more of these
addresses, and hence ends up in an infinite loop. Here we fix this to
only try and select such "old" addresses if we have enough of them. Note this
means, if we don't have enough of them, we may return more "new"
addresses than otherwise expected by the newSelectionBias.

This whole GetSelectionWithBias method probably needs to be rewritten,
but this is a quick fix for the issue.

* changelog

* fix infinite loop if not enough new addrs

* fix another potential infinite loop

if a.nNew == 0 -> pickFromOldBucket=true, but we don't have enough items
  (a.nOld > len(oldBucketToAddrsMap) false)

* Revert "fix another potential infinite loop"

This reverts commit 146540c112.

* check num addresses instead of buckets, new test

* fixed the int division

* add slack to bias % in test, lint fixes

* Added checks for selection content in test

* test cleanup

* Apply suggestions from code review

Co-Authored-By: ebuchman <ethan@coinculture.info>

* address review comments

* change after  Anton's review comments

* use the same docker image we use for testing

when building a binary for localnet

* switch back to circleci classic

* more review comments

* more review comments

* refactor addrbook_test

* build linux binary inside docker

in attempt to fix

```
--> Running dep
+ make build-linux
GOOS=linux GOARCH=amd64 make build
make[1]: Entering directory `/home/circleci/.go_workspace/src/github.com/tendermint/tendermint'
CGO_ENABLED=0 go build -ldflags "-X github.com/tendermint/tendermint/version.GitCommit=`git rev-parse --short=8 HEAD`" -tags 'tendermint' -o build/tendermint ./cmd/tendermint/
p2p/pex/addrbook.go:373:13: undefined: math.Round
```

* change dir from /usr to /go

* use concrete Go version for localnet binary

* check for nil addresses just to be sure

.circleci/config.yml

@@ -192,9 +192,7 @@ jobs:
             name: run localnet and exit on failure
             command: |
               set -x
-              make get_tools
-              make get_vendor_deps
-              make build-linux
+              docker run --rm -v "$PWD":/go/src/github.com/tendermint/tendermint -w /go/src/github.com/tendermint/tendermint golang:1.11.4 make build-linux
               make localnet-start &
               ./scripts/localnet-blocks-test.sh 40 5 10 localhost
 

CHANGELOG_PENDING.md

@@ -27,5 +27,6 @@ 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 addrbook deadlock for seed nodes
 - [p2p] \#3247 Fix panic in SeedMode when calling FlushStop and OnStop
   concurrently

Makefile

@@ -269,7 +269,7 @@ build-docker:
 ### Local testnet using docker
 
 # Build linux binary on other platforms
-build-linux:
+build-linux: get_tools get_vendor_deps
 	GOOS=linux GOARCH=amd64 $(MAKE) build
 
 build-docker-localnode:

p2p/pex/addrbook.go

@@ -369,6 +369,10 @@ func (a *addrBook) GetSelection() []*p2p.NetAddress {
 	return allAddr[:numAddresses]
 }
 
+func percentageOfNum(p, n int) int {
+	return int(math.Round((float64(p) / float64(100)) * float64(n)))
+}
+
 // GetSelectionWithBias implements AddrBook.
 // It randomly selects some addresses (old & new). Suitable for peer-exchange protocols.
 // Must never return a nil address.
@@ -408,11 +412,28 @@ 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 := percentageOfNum(biasTowardsNewAddrs, 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.
+		// numAddresses <= a.nOld + a.nNew therefore it is guaranteed that there are enough
+		// addresses to fill the selection
+		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 +471,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 +481,7 @@ ADDRS_LOOP:
 				newBucketToAddrsMap[newIndex] = make(map[string]struct{})
 			}
 			newBucketToAddrsMap[newIndex][selectedAddr.String()] = struct{}{}
+			newAddressesAdded++
 		}
 
 		selection[selectionIndex] = selectedAddr

p2p/pex/addrbook_test.go

@@ -4,38 +4,18 @@ import (
 	"encoding/hex"
 	"fmt"
 	"io/ioutil"
+	"math"
 	"os"
 	"testing"
 
-	"github.com/stretchr/testify/require"
-
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	cmn "github.com/tendermint/tendermint/libs/common"
 	"github.com/tendermint/tendermint/libs/log"
 	"github.com/tendermint/tendermint/p2p"
 )
 
-func createTempFileName(prefix string) string {
-	f, err := ioutil.TempFile("", prefix)
-	if err != nil {
-		panic(err)
-	}
-	fname := f.Name()
-	err = f.Close()
-	if err != nil {
-		panic(err)
-	}
-	return fname
-}
-
-func deleteTempFile(fname string) {
-	err := os.Remove(fname)
-	if err != nil {
-		panic(err)
-	}
-}
-
 func TestAddrBookPickAddress(t *testing.T) {
 	fname := createTempFileName("addrbook_test")
 	defer deleteTempFile(fname)
@@ -239,6 +219,34 @@ func TestAddrBookRemoveAddress(t *testing.T) {
 	assert.Equal(t, 0, book.Size())
 }
 
+func TestAddrBookGetSelectionWithOneMarkedGood(t *testing.T) {
+	// create a book with 10 addresses, 1 good/old and 9 new
+	book, fname := createAddrBookWithMOldAndNNewAddrs(t, 1, 9)
+	defer deleteTempFile(fname)
+
+	addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+	assert.NotNil(t, addrs)
+	assertMOldAndNNewAddrsInSelection(t, 1, 9, addrs, book)
+}
+
+func TestAddrBookGetSelectionWithOneNotMarkedGood(t *testing.T) {
+	// create a book with 10 addresses, 9 good/old and 1 new
+	book, fname := createAddrBookWithMOldAndNNewAddrs(t, 9, 1)
+	defer deleteTempFile(fname)
+
+	addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+	assert.NotNil(t, addrs)
+	assertMOldAndNNewAddrsInSelection(t, 9, 1, addrs, book)
+}
+
+func TestAddrBookGetSelectionReturnsNilWhenAddrBookIsEmpty(t *testing.T) {
+	book, fname := createAddrBookWithMOldAndNNewAddrs(t, 0, 0)
+	defer deleteTempFile(fname)
+
+	addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+	assert.Nil(t, addrs)
+}
+
 func TestAddrBookGetSelection(t *testing.T) {
 	fname := createTempFileName("addrbook_test")
 	defer deleteTempFile(fname)
@@ -335,9 +343,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))
 	}
 }
 
@@ -417,3 +432,199 @@ func TestPrivatePeers(t *testing.T) {
 		assert.True(t, ok)
 	}
 }
+
+func testAddrBookAddressSelection(t *testing.T, bookSize int) {
+	// generate all combinations of old (m) and new addresses
+	for nOld := 0; nOld <= bookSize; nOld++ {
+		nNew := bookSize - nOld
+		dbgStr := fmt.Sprintf("book of size %d (new %d, old %d)", bookSize, nNew, nOld)
+
+		// create book and get selection
+		book, fname := createAddrBookWithMOldAndNNewAddrs(t, nOld, nNew)
+		defer deleteTempFile(fname)
+		addrs := book.GetSelectionWithBias(biasToSelectNewPeers)
+		assert.NotNil(t, addrs, "%s - expected a non-nil selection", dbgStr)
+		nAddrs := len(addrs)
+		assert.NotZero(t, nAddrs, "%s - expected at least one address in selection", dbgStr)
+
+		// check there's no nil addresses
+		for _, addr := range addrs {
+			if addr == nil {
+				t.Fatalf("%s - got nil address in selection %v", dbgStr, addrs)
+			}
+		}
+
+		// XXX: shadowing
+		nOld, nNew := countOldAndNewAddrsInSelection(addrs, book)
+
+		// Given:
+		// n - num new addrs, m - num old addrs
+		// k - num new addrs expected in the beginning (based on bias %)
+		// i=min(n, k), aka expFirstNew
+		// j=min(m, r-i), aka expOld
+		//
+		// 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.
+		var (
+			k           = percentageOfNum(biasToSelectNewPeers, nAddrs)
+			expFirstNew = cmn.MinInt(nNew, k)
+			expOld      = cmn.MinInt(nOld, nAddrs-expFirstNew)
+			expNew      = nAddrs - expOld
+			expLastNew  = expNew - expFirstNew
+		)
+
+		// Verify that the number of old and new addresses are as expected
+		if nNew < expNew || nNew > expNew {
+			t.Fatalf("%s - expected new addrs %d, got %d", dbgStr, expNew, nNew)
+		}
+		if nOld < expOld || nOld > expOld {
+			t.Fatalf("%s - expected old addrs %d, got %d", dbgStr, expOld, nOld)
+		}
+
+		// Verify that the order of addresses is as expected
+		// Get the sequence types and lengths of the selection
+		seqLens, seqTypes, err := analyseSelectionLayout(book, addrs)
+		assert.NoError(t, err, "%s", 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
+
+		switch {
+		case expOld == 0: // all new addresses
+			expSeqLens = []int{nAddrs}
+			expSeqTypes = []int{1}
+		case expFirstNew == 0: // all old addresses
+			expSeqLens = []int{nAddrs}
+			expSeqTypes = []int{2}
+		case nAddrs-expFirstNew-expOld == 0: // new addresses, old addresses
+			expSeqLens = []int{expFirstNew, expOld}
+			expSeqTypes = []int{1, 2}
+		default: // new addresses, old addresses, new addresses
+			expSeqLens = []int{expFirstNew, expOld, expLastNew}
+			expSeqTypes = []int{1, 2, 1}
+		}
+
+		assert.Equal(t, expSeqLens, seqLens,
+			"%s - expected sequence lengths of old/new %v, got %v",
+			dbgStr, expSeqLens, seqLens)
+		assert.Equal(t, expSeqTypes, seqTypes,
+			"%s - expected sequence types (1-new, 2-old) was %v, got %v",
+			dbgStr, expSeqTypes, seqTypes)
+	}
+}
+
+func TestMultipleAddrBookAddressSelection(t *testing.T) {
+	// test books with smaller size, < N
+	const N = 32
+	for bookSize := 1; bookSize < N; bookSize++ {
+		testAddrBookAddressSelection(t, bookSize)
+	}
+
+	// Test for two books with sizes from following ranges
+	ranges := [...][]int{{33, 100}, {100, 175}}
+	var bookSizes []int
+	for _, r := range ranges {
+		bookSizes = append(bookSizes, cmn.RandIntn(r[1]-r[0])+r[0])
+	}
+	t.Logf("Testing address selection for the following book sizes %v\n", bookSizes)
+	for _, bookSize := range bookSizes {
+		testAddrBookAddressSelection(t, bookSize)
+	}
+}
+
+func assertMOldAndNNewAddrsInSelection(t *testing.T, m, n int, addrs []*p2p.NetAddress, book *addrBook) {
+	nOld, nNew := countOldAndNewAddrsInSelection(addrs, book)
+	assert.Equal(t, m, nOld, "old addresses")
+	assert.Equal(t, n, nNew, "new addresses")
+}
+
+func createTempFileName(prefix string) string {
+	f, err := ioutil.TempFile("", prefix)
+	if err != nil {
+		panic(err)
+	}
+	fname := f.Name()
+	err = f.Close()
+	if err != nil {
+		panic(err)
+	}
+	return fname
+}
+
+func deleteTempFile(fname string) {
+	err := os.Remove(fname)
+	if err != nil {
+		panic(err)
+	}
+}
+
+func createAddrBookWithMOldAndNNewAddrs(t *testing.T, nOld, nNew int) (book *addrBook, fname string) {
+	fname = createTempFileName("addrbook_test")
+
+	book = NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+	assert.Zero(t, book.Size())
+
+	randAddrs := randNetAddressPairs(t, nOld)
+	for _, addr := range randAddrs {
+		book.AddAddress(addr.addr, addr.src)
+		book.MarkGood(addr.addr)
+	}
+
+	randAddrs = randNetAddressPairs(t, nNew)
+	for _, addr := range randAddrs {
+		book.AddAddress(addr.addr, addr.src)
+	}
+
+	return
+}
+
+func countOldAndNewAddrsInSelection(addrs []*p2p.NetAddress, book *addrBook) (nOld, nNew int) {
+	for _, addr := range addrs {
+		if book.IsGood(addr) {
+			nOld++
+		} else {
+			nNew++
+		}
+	}
+	return
+}
+
+// Analyse the layout of the selection specified by 'addrs'
+// Returns:
+// - seqLens - the lengths of the sequences of addresses of same type
+// - seqTypes - the types of sequences in selection
+func analyseSelectionLayout(book *addrBook, addrs []*p2p.NetAddress) (seqLens, seqTypes []int, err error) {
+	// address types are: 0 - nil, 1 - new, 2 - old
+	var (
+		prevType      = 0
+		currentSeqLen = 0
+	)
+
+	for _, addr := range addrs {
+		addrType := 0
+		if book.IsGood(addr) {
+			addrType = 2
+		} else {
+			addrType = 1
+		}
+		if addrType != prevType && prevType != 0 {
+			seqLens = append(seqLens, currentSeqLen)
+			seqTypes = append(seqTypes, prevType)
+			currentSeqLen = 0
+		}
+		currentSeqLen++
+		prevType = addrType
+	}
+
+	seqLens = append(seqLens, currentSeqLen)
+	seqTypes = append(seqTypes, prevType)
+
+	return
+}