p2p: seed mode refactoring (#3011)

ListOfKnownAddresses is removed
panic if addrbook size is less than zero
CrawlPeers does not attempt to connect to existing or peers we're currently dialing
various perf. fixes
improved tests (though not complete)
move IsDialingOrExistingAddress check into DialPeerWithAddress (Fixes #2716)


* addrbook: preallocate memory when saving addrbook to file

* addrbook: remove oldestFirst struct and check for ID

* oldestFirst replaced with sort.Slice
* ID is now mandatory, so no need to check

* addrbook: remove ListOfKnownAddresses

GetSelection is used instead in seed mode.

* addrbook: panic if size is less than 0

* rewrite addrbook#saveToFile to not use a counter

* test AttemptDisconnects func

* move IsDialingOrExistingAddress check into DialPeerWithAddress

* save and cleanup crawl peer data

* get rid of DefaultSeedDisconnectWaitPeriod

* make linter happy

* fix TestPEXReactorSeedMode

* fix comment

* add a changelog entry

* Apply suggestions from code review

Co-Authored-By: melekes <anton.kalyaev@gmail.com>

* rename ErrDialingOrExistingAddress to ErrCurrentlyDialingOrExistingAddress

* lowercase errors

* do not persist seed data

pros:
- no extra files
- less IO

cons:
- if the node crashes, seed might crawl a peer too soon

* fixes after Ethan's review

* add a changelog entry

* we should only consult Switch about peers

checking addrbook size does not make sense since only PEX reactor uses
it for dialing peers!

#3011 (comment)

CHANGELOG_PENDING.md

@@ -19,4 +19,4 @@
 ### IMPROVEMENTS:
 
 ### BUG FIXES:
-
+- [p2p] \#2716 Check if we're already connected to peer right before dialing it (@melekes)

node/node.go

@@ -498,6 +498,12 @@ func NewNode(config *cfg.Config,
 			&pex.PEXReactorConfig{
 				Seeds:    splitAndTrimEmpty(config.P2P.Seeds, ",", " "),
 				SeedMode: config.P2P.SeedMode,
+				// See consensus/reactor.go: blocksToContributeToBecomeGoodPeer 10000
+				// blocks assuming 10s blocks ~ 28 hours.
+				// TODO (melekes): make it dynamic based on the actual block latencies
+				// from the live network.
+				// https://github.com/tendermint/tendermint/issues/3523
+				SeedDisconnectWaitPeriod: 28 * time.Hour,
 			})
 		pexReactor.SetLogger(logger.With("module", "pex"))
 		sw.AddReactor("PEX", pexReactor)

p2p/errors.go

@@ -103,7 +103,7 @@ type ErrSwitchDuplicatePeerID struct {
 }
 
 func (e ErrSwitchDuplicatePeerID) Error() string {
-	return fmt.Sprintf("Duplicate peer ID %v", e.ID)
+	return fmt.Sprintf("duplicate peer ID %v", e.ID)
 }
 
 // ErrSwitchDuplicatePeerIP to be raised whena a peer is connecting with a known
@@ -113,7 +113,7 @@ type ErrSwitchDuplicatePeerIP struct {
 }
 
 func (e ErrSwitchDuplicatePeerIP) Error() string {
-	return fmt.Sprintf("Duplicate peer IP %v", e.IP.String())
+	return fmt.Sprintf("duplicate peer IP %v", e.IP.String())
 }
 
 // ErrSwitchConnectToSelf to be raised when trying to connect to itself.
@@ -122,7 +122,7 @@ type ErrSwitchConnectToSelf struct {
 }
 
 func (e ErrSwitchConnectToSelf) Error() string {
-	return fmt.Sprintf("Connect to self: %v", e.Addr)
+	return fmt.Sprintf("connect to self: %v", e.Addr)
 }
 
 type ErrSwitchAuthenticationFailure struct {
@@ -132,7 +132,7 @@ type ErrSwitchAuthenticationFailure struct {
 
 func (e ErrSwitchAuthenticationFailure) Error() string {
 	return fmt.Sprintf(
-		"Failed to authenticate peer. Dialed %v, but got peer with ID %s",
+		"failed to authenticate peer. Dialed %v, but got peer with ID %s",
 		e.Dialed,
 		e.Got,
 	)
@@ -152,7 +152,7 @@ type ErrNetAddressNoID struct {
 }
 
 func (e ErrNetAddressNoID) Error() string {
-	return fmt.Sprintf("Address (%s) does not contain ID", e.Addr)
+	return fmt.Sprintf("address (%s) does not contain ID", e.Addr)
 }
 
 type ErrNetAddressInvalid struct {
@@ -161,7 +161,7 @@ type ErrNetAddressInvalid struct {
 }
 
 func (e ErrNetAddressInvalid) Error() string {
-	return fmt.Sprintf("Invalid address (%s): %v", e.Addr, e.Err)
+	return fmt.Sprintf("invalid address (%s): %v", e.Addr, e.Err)
 }
 
 type ErrNetAddressLookup struct {
@@ -170,5 +170,15 @@ type ErrNetAddressLookup struct {
 }
 
 func (e ErrNetAddressLookup) Error() string {
-	return fmt.Sprintf("Error looking up host (%s): %v", e.Addr, e.Err)
+	return fmt.Sprintf("error looking up host (%s): %v", e.Addr, e.Err)
+}
+
+// ErrCurrentlyDialingOrExistingAddress indicates that we're currently
+// dialing this address or it belongs to an existing peer.
+type ErrCurrentlyDialingOrExistingAddress struct {
+	Addr string
+}
+
+func (e ErrCurrentlyDialingOrExistingAddress) Error() string {
+	return fmt.Sprintf("connection with %s has been established or dialed", e.Addr)
 }

p2p/pex/addrbook.go

@@ -66,8 +66,7 @@ type AddrBook interface {
 	// Send a selection of addresses with bias
 	GetSelectionWithBias(biasTowardsNewAddrs int) []*p2p.NetAddress
 
-	// TODO: remove
-	ListOfKnownAddresses() []*knownAddress
+	Size() int
 
 	// Persist to disk
 	Save()
@@ -254,7 +253,7 @@ func (a *addrBook) PickAddress(biasTowardsNewAddrs int) *p2p.NetAddress {
 	bookSize := a.size()
 	if bookSize <= 0 {
 		if bookSize < 0 {
-			a.Logger.Error("Addrbook size less than 0", "nNew", a.nNew, "nOld", a.nOld)
+			panic(fmt.Sprintf("Addrbook size %d (new: %d + old: %d) is less than 0", a.nNew+a.nOld, a.nNew, a.nOld))
 		}
 		return nil
 	}
@@ -339,7 +338,7 @@ func (a *addrBook) GetSelection() []*p2p.NetAddress {
 	bookSize := a.size()
 	if bookSize <= 0 {
 		if bookSize < 0 {
-			a.Logger.Error("Addrbook size less than 0", "nNew", a.nNew, "nOld", a.nOld)
+			panic(fmt.Sprintf("Addrbook size %d (new: %d + old: %d) is less than 0", a.nNew+a.nOld, a.nNew, a.nOld))
 		}
 		return nil
 	}
@@ -389,7 +388,7 @@ func (a *addrBook) GetSelectionWithBias(biasTowardsNewAddrs int) []*p2p.NetAddre
 	bookSize := a.size()
 	if bookSize <= 0 {
 		if bookSize < 0 {
-			a.Logger.Error("Addrbook size less than 0", "nNew", a.nNew, "nOld", a.nOld)
+			panic(fmt.Sprintf("Addrbook size %d (new: %d + old: %d) is less than 0", a.nNew+a.nOld, a.nNew, a.nOld))
 		}
 		return nil
 	}
@@ -414,18 +413,6 @@ func (a *addrBook) GetSelectionWithBias(biasTowardsNewAddrs int) []*p2p.NetAddre
 	return selection
 }
 
-// ListOfKnownAddresses returns the new and old addresses.
-func (a *addrBook) ListOfKnownAddresses() []*knownAddress {
-	a.mtx.Lock()
-	defer a.mtx.Unlock()
-
-	addrs := []*knownAddress{}
-	for _, addr := range a.addrLookup {
-		addrs = append(addrs, addr.copy())
-	}
-	return addrs
-}
-
 //------------------------------------------------
 
 // Size returns the number of addresses in the book.
@@ -473,8 +460,7 @@ func (a *addrBook) getBucket(bucketType byte, bucketIdx int) map[string]*knownAd
 	case bucketTypeOld:
 		return a.bucketsOld[bucketIdx]
 	default:
-		cmn.PanicSanity("Should not happen")
-		return nil
+		panic("Invalid bucket type")
 	}
 }
 

p2p/pex/file.go

@@ -16,16 +16,15 @@ type addrBookJSON struct {
 }
 
 func (a *addrBook) saveToFile(filePath string) {
-	a.Logger.Info("Saving AddrBook to file", "size", a.Size())
-
 	a.mtx.Lock()
 	defer a.mtx.Unlock()
-	// Compile Addrs
-	addrs := []*knownAddress{}
+
+	a.Logger.Info("Saving AddrBook to file", "size", a.size())
+
+	addrs := make([]*knownAddress, 0, len(a.addrLookup))
 	for _, ka := range a.addrLookup {
 		addrs = append(addrs, ka)
 	}
-
 	aJSON := &addrBookJSON{
 		Key:   a.key,
 		Addrs: addrs,

p2p/pex/known_address.go

@@ -33,18 +33,6 @@ func (ka *knownAddress) ID() p2p.ID {
 	return ka.Addr.ID
 }
 
-func (ka *knownAddress) copy() *knownAddress {
-	return &knownAddress{
-		Addr:        ka.Addr,
-		Src:         ka.Src,
-		Attempts:    ka.Attempts,
-		LastAttempt: ka.LastAttempt,
-		LastSuccess: ka.LastSuccess,
-		BucketType:  ka.BucketType,
-		Buckets:     ka.Buckets,
-	}
-}
-
 func (ka *knownAddress) isOld() bool {
 	return ka.BucketType == bucketTypeOld
 }