From 498a82784d46284c393fe4ce612c8dbe74094e03 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 02:25:00 +0000
Subject: [PATCH 01/10] p2p/addrbook: comments

---
 p2p/addrbook.go | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 06162e2e6..92b3e0d7e 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -40,7 +40,7 @@ const (
 	// old buckets over which an address group will be spread.
 	oldBucketsPerGroup = 4
 
-	// new buckets over which an source address group will be spread.
+	// new buckets over which a source address group will be spread.
 	newBucketsPerGroup = 32
 
 	// buckets a frequently seen new address may end up in.
@@ -79,18 +79,22 @@ const (
 type AddrBook struct {
 	cmn.BaseService
 
-	mtx               sync.Mutex
+	// immutable after creation
 	filePath          string
 	routabilityStrict bool
-	rand              *rand.Rand
 	key               string
-	ourAddrs          map[string]*NetAddress
-	addrLookup        map[string]*knownAddress // new & old
-	addrNew           []map[string]*knownAddress
-	addrOld           []map[string]*knownAddress
-	wg                sync.WaitGroup
-	nOld              int
-	nNew              int
+
+	// accessed concurrently
+	mtx        sync.Mutex
+	rand       *rand.Rand
+	ourAddrs   map[string]*NetAddress
+	addrLookup map[string]*knownAddress // new & old
+	addrNew    []map[string]*knownAddress
+	addrOld    []map[string]*knownAddress
+	nOld       int
+	nNew       int
+
+	wg sync.WaitGroup
 }
 
 // NewAddrBook creates a new address book.
@@ -145,6 +149,7 @@ func (a *AddrBook) Wait() {
 	a.wg.Wait()
 }
 
+// AddOurAddress adds another one of our addresses.
 func (a *AddrBook) AddOurAddress(addr *NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
@@ -152,6 +157,7 @@ func (a *AddrBook) AddOurAddress(addr *NetAddress) {
 	a.ourAddrs[addr.String()] = addr
 }
 
+// OurAddresses returns a list of our addresses.
 func (a *AddrBook) OurAddresses() []*NetAddress {
 	addrs := []*NetAddress{}
 	for _, addr := range a.ourAddrs {
@@ -160,6 +166,7 @@ func (a *AddrBook) OurAddresses() []*NetAddress {
 	return addrs
 }
 
+// AddAddress adds the given address as received from the given source.
 // NOTE: addr must not be nil
 func (a *AddrBook) AddAddress(addr *NetAddress, src *NetAddress) {
 	a.mtx.Lock()
@@ -168,10 +175,12 @@ func (a *AddrBook) AddAddress(addr *NetAddress, src *NetAddress) {
 	a.addAddress(addr, src)
 }
 
+// NeedMoreAddrs returns true if there are not have enough addresses in the book.
 func (a *AddrBook) NeedMoreAddrs() bool {
 	return a.Size() < needAddressThreshold
 }
 
+// Size returns the number of addresses in the book.
 func (a *AddrBook) Size() int {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
@@ -182,7 +191,7 @@ func (a *AddrBook) size() int {
 	return a.nNew + a.nOld
 }
 
-// Pick an address to connect to with new/old bias.
+// PickAddress picks an address to connect to with new/old bias.
 func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
@@ -201,9 +210,9 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	oldCorrelation := math.Sqrt(float64(a.nOld)) * (100.0 - float64(newBias))
 	newCorrelation := math.Sqrt(float64(a.nNew)) * float64(newBias)
 
-	if (newCorrelation+oldCorrelation)*a.rand.Float64() < oldCorrelation {
-		// pick random Old bucket.
-		var bucket map[string]*knownAddress = nil
+	pickFromOldBucket := (newCorrelation+oldCorrelation)*a.rand.Float64() < oldCorrelation
+	if pickFromOldBucket {
+		var bucket map[string]*knownAddress
 		for len(bucket) == 0 {
 			bucket = a.addrOld[a.rand.Intn(len(a.addrOld))]
 		}
@@ -217,7 +226,6 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 		}
 		cmn.PanicSanity("Should not happen")
 	} else {
-		// pick random New bucket.
 		var bucket map[string]*knownAddress = nil
 		for len(bucket) == 0 {
 			bucket = a.addrNew[a.rand.Intn(len(a.addrNew))]
@@ -235,6 +243,7 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	return nil
 }
 
+// MarkGood marks the peer as good and moves it into an "old" bucket.
 func (a *AddrBook) MarkGood(addr *NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
@@ -248,6 +257,7 @@ func (a *AddrBook) MarkGood(addr *NetAddress) {
 	}
 }
 
+// MarkAttempt marks that an attempt was made to connect to the address.
 func (a *AddrBook) MarkAttempt(addr *NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
@@ -823,7 +833,8 @@ func (ka *knownAddress) isBad() bool {
 		return false
 	}
 
-	// Over a month old?
+	// Too old?
+	// XXX: does this mean if we've kept a connection up for this long we'll disconnect?!
 	if ka.LastAttempt.After(time.Now().Add(-1 * numMissingDays * time.Hour * 24)) {
 		return true
 	}

From 2f067a3f656c41846874f58ee890c49e02a73a96 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 02:28:11 +0000
Subject: [PATCH 02/10] p2p/addrbook: addrNew/Old -> bucketsNew/Old

---
 p2p/addrbook.go | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 92b3e0d7e..eeeeaf5ee 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -89,8 +89,8 @@ type AddrBook struct {
 	rand       *rand.Rand
 	ourAddrs   map[string]*NetAddress
 	addrLookup map[string]*knownAddress // new & old
-	addrNew    []map[string]*knownAddress
-	addrOld    []map[string]*knownAddress
+	bucketsOld []map[string]*knownAddress
+	bucketsNew []map[string]*knownAddress
 	nOld       int
 	nNew       int
 
@@ -116,14 +116,14 @@ func NewAddrBook(filePath string, routabilityStrict bool) *AddrBook {
 func (a *AddrBook) init() {
 	a.key = crypto.CRandHex(24) // 24/2 * 8 = 96 bits
 	// New addr buckets
-	a.addrNew = make([]map[string]*knownAddress, newBucketCount)
-	for i := range a.addrNew {
-		a.addrNew[i] = make(map[string]*knownAddress)
+	a.bucketsNew = make([]map[string]*knownAddress, newBucketCount)
+	for i := range a.bucketsNew {
+		a.bucketsNew[i] = make(map[string]*knownAddress)
 	}
 	// Old addr buckets
-	a.addrOld = make([]map[string]*knownAddress, oldBucketCount)
-	for i := range a.addrOld {
-		a.addrOld[i] = make(map[string]*knownAddress)
+	a.bucketsOld = make([]map[string]*knownAddress, oldBucketCount)
+	for i := range a.bucketsOld {
+		a.bucketsOld[i] = make(map[string]*knownAddress)
 	}
 }
 
@@ -214,7 +214,7 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	if pickFromOldBucket {
 		var bucket map[string]*knownAddress
 		for len(bucket) == 0 {
-			bucket = a.addrOld[a.rand.Intn(len(a.addrOld))]
+			bucket = a.bucketsOld[a.rand.Intn(len(a.bucketsOld))]
 		}
 		// pick a random ka from bucket.
 		randIndex := a.rand.Intn(len(bucket))
@@ -228,7 +228,7 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	} else {
 		var bucket map[string]*knownAddress = nil
 		for len(bucket) == 0 {
-			bucket = a.addrNew[a.rand.Intn(len(a.addrNew))]
+			bucket = a.bucketsNew[a.rand.Intn(len(a.bucketsNew))]
 		}
 		// pick a random ka from bucket.
 		randIndex := a.rand.Intn(len(bucket))
@@ -380,7 +380,7 @@ func (a *AddrBook) loadFromFile(filePath string) bool {
 	// Restore all the fields...
 	// Restore the key
 	a.key = aJSON.Key
-	// Restore .addrNew & .addrOld
+	// Restore .bucketsNew & .bucketsOld
 	for _, ka := range aJSON.Addrs {
 		for _, bucketIndex := range ka.Buckets {
 			bucket := a.getBucket(ka.BucketType, bucketIndex)
@@ -425,9 +425,9 @@ out:
 func (a *AddrBook) getBucket(bucketType byte, bucketIdx int) map[string]*knownAddress {
 	switch bucketType {
 	case bucketTypeNew:
-		return a.addrNew[bucketIdx]
+		return a.bucketsNew[bucketIdx]
 	case bucketTypeOld:
-		return a.addrOld[bucketIdx]
+		return a.bucketsOld[bucketIdx]
 	default:
 		cmn.PanicSanity("Should not happen")
 		return nil
@@ -587,7 +587,7 @@ func (a *AddrBook) addAddress(addr, src *NetAddress) {
 // Make space in the new buckets by expiring the really bad entries.
 // If no bad entries are available we remove the oldest.
 func (a *AddrBook) expireNew(bucketIdx int) {
-	for addrStr, ka := range a.addrNew[bucketIdx] {
+	for addrStr, ka := range a.bucketsNew[bucketIdx] {
 		// If an entry is bad, throw it away
 		if ka.isBad() {
 			a.Logger.Info(cmn.Fmt("expiring bad address %v", addrStr))

From ed95cc160a44a0949d11a52c0d853c80d111511d Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 02:31:47 +0000
Subject: [PATCH 03/10] p2p/addrbook: simplify PickAddress

---
 p2p/addrbook.go | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index eeeeaf5ee..3cc385d1d 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -210,37 +210,18 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	oldCorrelation := math.Sqrt(float64(a.nOld)) * (100.0 - float64(newBias))
 	newCorrelation := math.Sqrt(float64(a.nNew)) * float64(newBias)
 
+	// pick a random peer from a random bucket
+	var bucket map[string]*knownAddress
 	pickFromOldBucket := (newCorrelation+oldCorrelation)*a.rand.Float64() < oldCorrelation
-	if pickFromOldBucket {
-		var bucket map[string]*knownAddress
-		for len(bucket) == 0 {
+	for len(bucket) == 0 {
+		if pickFromOldBucket {
 			bucket = a.bucketsOld[a.rand.Intn(len(a.bucketsOld))]
-		}
-		// pick a random ka from bucket.
-		randIndex := a.rand.Intn(len(bucket))
-		for _, ka := range bucket {
-			if randIndex == 0 {
-				return ka.Addr
-			}
-			randIndex--
-		}
-		cmn.PanicSanity("Should not happen")
-	} else {
-		var bucket map[string]*knownAddress = nil
-		for len(bucket) == 0 {
+		} else {
 			bucket = a.bucketsNew[a.rand.Intn(len(a.bucketsNew))]
 		}
-		// pick a random ka from bucket.
-		randIndex := a.rand.Intn(len(bucket))
-		for _, ka := range bucket {
-			if randIndex == 0 {
-				return ka.Addr
-			}
-			randIndex--
-		}
-		cmn.PanicSanity("Should not happen")
 	}
-	return nil
+	randIndex := a.rand.Intn(len(bucket))
+	return bucket[randIndex].Addr
 }
 
 // MarkGood marks the peer as good and moves it into an "old" bucket.

From 8c88cc017a1afc3d577643667355e21ca2df9551 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 03:59:54 +0000
Subject: [PATCH 04/10] p2p/addrbook: addAddress returns error. more defensive
 PickAddress

---
 p2p/addrbook.go | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 3cc385d1d..842fcfdee 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -7,6 +7,7 @@ package p2p
 import (
 	"encoding/binary"
 	"encoding/json"
+	"fmt"
 	"math"
 	"math/rand"
 	"net"
@@ -168,11 +169,10 @@ func (a *AddrBook) OurAddresses() []*NetAddress {
 
 // AddAddress adds the given address as received from the given source.
 // NOTE: addr must not be nil
-func (a *AddrBook) AddAddress(addr *NetAddress, src *NetAddress) {
+func (a *AddrBook) AddAddress(addr *NetAddress, src *NetAddress) error {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
-	a.Logger.Info("Add address to book", "addr", addr, "src", src)
-	a.addAddress(addr, src)
+	return a.addAddress(addr, src)
 }
 
 // NeedMoreAddrs returns true if there are not have enough addresses in the book.
@@ -213,6 +213,11 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	// pick a random peer from a random bucket
 	var bucket map[string]*knownAddress
 	pickFromOldBucket := (newCorrelation+oldCorrelation)*a.rand.Float64() < oldCorrelation
+	if (pickFromOldBucket && a.nOld == 0) ||
+		(!pickFromOldBucket && a.nNew == 0) {
+		return nil
+	}
+	// loop until we pick a random non-empty bucket
 	for len(bucket) == 0 {
 		if pickFromOldBucket {
 			bucket = a.bucketsOld[a.rand.Intn(len(a.bucketsOld))]
@@ -220,8 +225,15 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 			bucket = a.bucketsNew[a.rand.Intn(len(a.bucketsNew))]
 		}
 	}
+	// pick a random index and loop over the map to return that index
 	randIndex := a.rand.Intn(len(bucket))
-	return bucket[randIndex].Addr
+	for _, ka := range bucket {
+		if randIndex == 0 {
+			return ka.Addr
+		}
+		randIndex--
+	}
+	return nil
 }
 
 // MarkGood marks the peer as good and moves it into an "old" bucket.
@@ -529,14 +541,13 @@ func (a *AddrBook) pickOldest(bucketType byte, bucketIdx int) *knownAddress {
 	return oldest
 }
 
-func (a *AddrBook) addAddress(addr, src *NetAddress) {
+func (a *AddrBook) addAddress(addr, src *NetAddress) error {
 	if a.routabilityStrict && !addr.Routable() {
-		a.Logger.Error(cmn.Fmt("Cannot add non-routable address %v", addr))
-		return
+		return fmt.Errorf("Cannot add non-routable address %v", addr)
 	}
 	if _, ok := a.ourAddrs[addr.String()]; ok {
 		// Ignore our own listener address.
-		return
+		return fmt.Errorf("Cannot add ourselves with address %v", addr)
 	}
 
 	ka := a.addrLookup[addr.String()]
@@ -544,16 +555,16 @@ func (a *AddrBook) addAddress(addr, src *NetAddress) {
 	if ka != nil {
 		// Already old.
 		if ka.isOld() {
-			return
+			return nil
 		}
 		// Already in max new buckets.
 		if len(ka.Buckets) == maxNewBucketsPerAddress {
-			return
+			return nil
 		}
 		// The more entries we have, the less likely we are to add more.
 		factor := int32(2 * len(ka.Buckets))
 		if a.rand.Int31n(factor) != 0 {
-			return
+			return nil
 		}
 	} else {
 		ka = newKnownAddress(addr, src)
@@ -563,6 +574,7 @@ func (a *AddrBook) addAddress(addr, src *NetAddress) {
 	a.addToNewBucket(ka, bucket)
 
 	a.Logger.Info("Added new address", "address", addr, "total", a.size())
+	return nil
 }
 
 // Make space in the new buckets by expiring the really bad entries.

From 435eb6e2b3b4d4ba25faffd98c61e7b0dab155ea Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 04:00:59 +0000
Subject: [PATCH 05/10] p2p/addrbook: add non-terminating test

---
 p2p/addrbook_test.go | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/p2p/addrbook_test.go b/p2p/addrbook_test.go
index 9b83be180..6ebb34c34 100644
--- a/p2p/addrbook_test.go
+++ b/p2p/addrbook_test.go
@@ -23,6 +23,40 @@ func createTempFileName(prefix string) string {
 	return fname
 }
 
+func TestAddrBookPickAddress(t *testing.T) {
+	assert := assert.New(t)
+	fname := createTempFileName("addrbook_test")
+
+	// 0 addresses
+	book := NewAddrBook(fname, true)
+	book.SetLogger(log.TestingLogger())
+	assert.Zero(book.Size())
+
+	addr := book.PickAddress(50)
+	assert.Nil(addr, "expected no address")
+
+	randAddrs := randNetAddressPairs(t, 1)
+	addrSrc := randAddrs[0]
+	book.AddAddress(addrSrc.addr, addrSrc.src)
+
+	// pick an address when we only have new address
+	addr = book.PickAddress(0)
+	assert.NotNil(addr, "expected an address")
+	addr = book.PickAddress(50)
+	assert.NotNil(addr, "expected an address")
+	addr = book.PickAddress(100)
+	assert.NotNil(addr, "expected an address")
+
+	// pick an address when we only have old address
+	book.MarkGood(addrSrc.addr)
+	addr = book.PickAddress(0)
+	assert.NotNil(addr, "expected an address")
+	addr = book.PickAddress(50)
+	assert.NotNil(addr, "expected an address")
+	addr = book.PickAddress(100)
+	assert.NotNil(addr, "expected an address")
+}
+
 func TestAddrBookSaveLoad(t *testing.T) {
 	fname := createTempFileName("addrbook_test")
 
@@ -106,6 +140,10 @@ func TestAddrBookPromoteToOld(t *testing.T) {
 	if len(selection) > book.Size() {
 		t.Errorf("selection could not be bigger than the book")
 	}
+
+	if book.Size() != 100 {
+		t.Errorf("Size is not 100. Got %v", book.Size())
+	}
 }
 
 func TestAddrBookHandlesDuplicates(t *testing.T) {

From 40e93a5f9eb75ba65e064f197e6e3d79800d30a8 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 04:08:46 +0000
Subject: [PATCH 06/10] p2p/addrbook: fix addToOldBucket

---
 p2p/addrbook.go      | 2 +-
 p2p/addrbook_test.go | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 842fcfdee..1101ffeef 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -475,7 +475,7 @@ func (a *AddrBook) addToOldBucket(ka *knownAddress, bucketIdx int) bool {
 	}
 
 	addrStr := ka.Addr.String()
-	bucket := a.getBucket(bucketTypeNew, bucketIdx)
+	bucket := a.getBucket(bucketTypeOld, bucketIdx)
 
 	// Already exists?
 	if _, ok := bucket[addrStr]; ok {
diff --git a/p2p/addrbook_test.go b/p2p/addrbook_test.go
index 6ebb34c34..419081d01 100644
--- a/p2p/addrbook_test.go
+++ b/p2p/addrbook_test.go
@@ -53,8 +53,10 @@ func TestAddrBookPickAddress(t *testing.T) {
 	assert.NotNil(addr, "expected an address")
 	addr = book.PickAddress(50)
 	assert.NotNil(addr, "expected an address")
+
+	// in this case, nNew==0 but we biased 100% to new, so we return nil
 	addr = book.PickAddress(100)
-	assert.NotNil(addr, "expected an address")
+	assert.Nil(addr, "did not expected an address")
 }
 
 func TestAddrBookSaveLoad(t *testing.T) {

From 8e044b0e6ddee561a7aed99d2346db5ba44efb69 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 04:30:23 +0000
Subject: [PATCH 07/10] p2p/addrbook: some comments

---
 p2p/addrbook.go | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 1101ffeef..00b7ef759 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -237,6 +237,7 @@ func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 }
 
 // MarkGood marks the peer as good and moves it into an "old" bucket.
+// XXX: we never call this!
 func (a *AddrBook) MarkGood(addr *NetAddress) {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
@@ -304,6 +305,7 @@ func (a *AddrBook) GetSelection() []*NetAddress {
 
 	// Fisher-Yates shuffle the array. We only need to do the first
 	// `numAddresses' since we are throwing the rest.
+	// XXX: What's the point of this if we already loop randomly through addrLookup ?
 	for i := 0; i < numAddresses; i++ {
 		// pick a number between current index and the end
 		j := rand.Intn(len(allAddr)-i) + i
@@ -400,17 +402,17 @@ func (a *AddrBook) Save() {
 func (a *AddrBook) saveRoutine() {
 	defer a.wg.Done()
 
-	dumpAddressTicker := time.NewTicker(dumpAddressInterval)
+	saveFileTicker := time.NewTicker(dumpAddressInterval)
 out:
 	for {
 		select {
-		case <-dumpAddressTicker.C:
+		case <-saveFileTicker.C:
 			a.saveToFile(a.filePath)
 		case <-a.Quit:
 			break out
 		}
 	}
-	dumpAddressTicker.Stop()
+	saveFileTicker.Stop()
 	a.saveToFile(a.filePath)
 	a.Logger.Info("Address handler done")
 }

From be1a16a6016286584a7dce3ffff682f9beab7a29 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 04:30:38 +0000
Subject: [PATCH 08/10] p2p/pex: simplify ensurePeers

---
 p2p/pex_reactor.go | 49 +++++++++++++++-------------------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

diff --git a/p2p/pex_reactor.go b/p2p/pex_reactor.go
index e2ccff424..da72bd535 100644
--- a/p2p/pex_reactor.go
+++ b/p2p/pex_reactor.go
@@ -240,43 +240,26 @@ func (r *PEXReactor) ensurePeers() {
 		return
 	}
 
-	toDial := make(map[string]*NetAddress)
+	// bias to prefer more vetted peers when we have fewer connections.
+	// not perfect, but somewhate ensures that we prioritize connecting to more-vetted
+	newBias := cmn.MinInt(numOutPeers, 8)*10 + 10
 
-	// Try to pick numToDial addresses to dial.
-	for i := 0; i < numToDial; i++ {
-		// The purpose of newBias is to first prioritize old (more vetted) peers
-		// when we have few connections, but to allow for new (less vetted) peers
-		// if we already have many connections. This algorithm isn't perfect, but
-		// it somewhat ensures that we prioritize connecting to more-vetted
-		// peers.
-		newBias := cmn.MinInt(numOutPeers, 8)*10 + 10
-		var picked *NetAddress
-		// Try to fetch a new peer 3 times.
-		// This caps the maximum number of tries to 3 * numToDial.
-		for j := 0; j < 3; j++ {
-			try := r.book.PickAddress(newBias)
-			if try == nil {
-				break
-			}
-			_, alreadySelected := toDial[try.IP.String()]
-			alreadyDialing := r.Switch.IsDialing(try)
-			alreadyConnected := r.Switch.Peers().Has(try.IP.String())
-			if alreadySelected || alreadyDialing || alreadyConnected {
-				// r.Logger.Info("Cannot dial address", "addr", try,
-				// 	"alreadySelected", alreadySelected,
-				// 	"alreadyDialing", alreadyDialing,
-				//  "alreadyConnected", alreadyConnected)
-				continue
-			} else {
-				r.Logger.Info("Will dial address", "addr", try)
-				picked = try
-				break
-			}
+	toDial := make(map[string]*NetAddress)
+	// Try maxAttempts times to pick numToDial addresses to dial
+	maxAttempts := numToDial * 3
+	for i := 0; i < maxAttempts && len(toDial) < numToDial; i++ {
+		try := r.book.PickAddress(newBias)
+		if try == nil {
+			continue
 		}
-		if picked == nil {
+		_, alreadySelected := toDial[try.IP.String()]
+		alreadyDialing := r.Switch.IsDialing(try)
+		alreadyConnected := r.Switch.Peers().Has(try.IP.String())
+		if alreadySelected || alreadyDialing || alreadyConnected {
 			continue
 		}
-		toDial[picked.IP.String()] = picked
+		r.Logger.Info("Will dial address", "addr", try)
+		toDial[try.IP.String()] = try
 	}
 
 	// Dial picked addresses

From feb3230160fe4ecd40cd2bbb24fed5345db2305c Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Thu, 16 Nov 2017 04:43:07 +0000
Subject: [PATCH 09/10] some comments

---
 p2p/addrbook.go | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 00b7ef759..8570dcf7a 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -684,8 +684,8 @@ func (a *AddrBook) calcOldBucket(addr *NetAddress) int {
 }
 
 // Return a string representing the network group of this address.
-// This is the /16 for IPv6, the /32 (/36 for he.net) for IPv6, the string
-// "local" for a local address and the string "unroutable for an unroutable
+// This is the /16 for IPv4, the /32 (/36 for he.net) for IPv6, the string
+// "local" for a local address and the string "unroutable" for an unroutable
 // address.
 func (a *AddrBook) groupKey(na *NetAddress) string {
 	if a.routabilityStrict && na.Local() {
@@ -811,8 +811,8 @@ func (ka *knownAddress) removeBucketRef(bucketIdx int) int {
 }
 
 /*
-   An address is bad if the address in question has not been tried in the last
-   minute and meets one of the following criteria:
+   An address is bad if the address in question is a New address, has not been tried in the last
+   minute, and meets one of the following criteria:
 
    1) It claims to be from the future
    2) It hasn't been seen in over a month
@@ -821,8 +821,15 @@ func (ka *knownAddress) removeBucketRef(bucketIdx int) int {
 
    All addresses that meet these criteria are assumed to be worthless and not
    worth keeping hold of.
+
+   XXX: so a good peer needs us to call MarkGood before the conditions above are reached!
 */
 func (ka *knownAddress) isBad() bool {
+	// Is Old --> good
+	if ka.BucketType == bucketTypeOld {
+		return false
+	}
+
 	// Has been attempted in the last minute --> good
 	if ka.LastAttempt.Before(time.Now().Add(-1 * time.Minute)) {
 		return false
@@ -830,6 +837,7 @@ func (ka *knownAddress) isBad() bool {
 
 	// Too old?
 	// XXX: does this mean if we've kept a connection up for this long we'll disconnect?!
+	// and shouldn't it be .Before ?
 	if ka.LastAttempt.After(time.Now().Add(-1 * numMissingDays * time.Hour * 24)) {
 		return true
 	}
@@ -840,6 +848,7 @@ func (ka *knownAddress) isBad() bool {
 	}
 
 	// Hasn't succeeded in too long?
+	// XXX: does this mean if we've kept a connection up for this long we'll disconnect?!
 	if ka.LastSuccess.Before(time.Now().Add(-1*minBadDays*time.Hour*24)) &&
 		ka.Attempts >= maxFailures {
 		return true

From c4b695f78da152d7f4d1fbc515c13fcf457c2d75 Mon Sep 17 00:00:00 2001
From: Ethan Buchman <ethan@coinculture.info>
Date: Mon, 20 Nov 2017 19:30:05 +0000
Subject: [PATCH 10/10] minor fixes from review

---
 p2p/addrbook.go      |  7 ++++++-
 p2p/addrbook_test.go |  5 ++---
 p2p/pex_reactor.go   | 11 +++++++----
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/p2p/addrbook.go b/p2p/addrbook.go
index 8570dcf7a..0b3301060 100644
--- a/p2p/addrbook.go
+++ b/p2p/addrbook.go
@@ -191,7 +191,12 @@ func (a *AddrBook) size() int {
 	return a.nNew + a.nOld
 }
 
-// PickAddress picks an address to connect to with new/old bias.
+// PickAddress picks an address to connect to.
+// The address is picked randomly from an old or new bucket according
+// to the newBias argument, which must be between [0, 100] (or else is truncated to that range)
+// and determines how biased we are to pick an address from a new bucket.
+// PickAddress returns nil if the AddrBook is empty or if we try to pick
+// from an empty bucket.
 func (a *AddrBook) PickAddress(newBias int) *NetAddress {
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
diff --git a/p2p/addrbook_test.go b/p2p/addrbook_test.go
index 419081d01..d84c008ed 100644
--- a/p2p/addrbook_test.go
+++ b/p2p/addrbook_test.go
@@ -112,6 +112,7 @@ func TestAddrBookLookup(t *testing.T) {
 }
 
 func TestAddrBookPromoteToOld(t *testing.T) {
+	assert := assert.New(t)
 	fname := createTempFileName("addrbook_test")
 
 	randAddrs := randNetAddressPairs(t, 100)
@@ -143,9 +144,7 @@ func TestAddrBookPromoteToOld(t *testing.T) {
 		t.Errorf("selection could not be bigger than the book")
 	}
 
-	if book.Size() != 100 {
-		t.Errorf("Size is not 100. Got %v", book.Size())
-	}
+	assert.Equal(book.Size(), 100, "expecting book size to be 100")
 }
 
 func TestAddrBookHandlesDuplicates(t *testing.T) {
diff --git a/p2p/pex_reactor.go b/p2p/pex_reactor.go
index da72bd535..fd70198f4 100644
--- a/p2p/pex_reactor.go
+++ b/p2p/pex_reactor.go
@@ -252,10 +252,13 @@ func (r *PEXReactor) ensurePeers() {
 		if try == nil {
 			continue
 		}
-		_, alreadySelected := toDial[try.IP.String()]
-		alreadyDialing := r.Switch.IsDialing(try)
-		alreadyConnected := r.Switch.Peers().Has(try.IP.String())
-		if alreadySelected || alreadyDialing || alreadyConnected {
+		if _, selected := toDial[try.IP.String()]; selected {
+			continue
+		}
+		if dialling := r.Switch.IsDialing(try); dialling {
+			continue
+		}
+		if connected := r.Switch.Peers().Has(try.IP.String()); connected {
 			continue
 		}
 		r.Logger.Info("Will dial address", "addr", try)