p2p: fix infinite loop in addrbook

.circleci/config.yml

@@ -292,9 +292,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

@@ -26,5 +26,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

@@ -301,7 +301,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
+}