diff --git a/.circleci/config.yml b/.circleci/config.yml index d29680945..d6440de1e 100644 --- a/.circleci/config.yml +++ b/.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 diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 2c93469da..2c70ee5aa 100644 --- a/CHANGELOG_PENDING.md +++ b/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 diff --git a/Makefile b/Makefile index 4ebb88b06..a1ba06aa9 100644 --- a/Makefile +++ b/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: diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index cfeefb343..3cda9ac74 100644 --- a/p2p/pex/addrbook.go +++ b/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 diff --git a/p2p/pex/addrbook_test.go b/p2p/pex/addrbook_test.go index ade02d490..bac418ff9 100644 --- a/p2p/pex/addrbook_test.go +++ b/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 +}