p2p: make persistent prop independent of conn direction (#3593)

## Description

Previously only outbound peers can be persistent.

Now, even if the peer is inbound, if it's marked as persistent, when/if conn is lost,
Tendermint will try to reconnect. This part is actually optional and can be reverted.

Plus, seed won't disconnect from inbound peer if it's marked as
persistent. Fixes #3362

## Commits

* make persistent prop independent of conn direction

Previously only outbound peers can be persistent. Now, even if the peer
is inbound, if it's marked as persistent, when/if conn is lost,
Tendermint will try to reconnect.

Plus, seed won't disconnect from inbound peer if it's marked as
persistent. Fixes #3362

* fix TestPEXReactorDialPeer test

* add a changelog entry

* update changelog

* add two tests

* reformat code

* test UnsafeDialPeers and UnsafeDialSeeds

* add TestSwitchDialPeersAsync

* spec: update p2p/config spec

* fixes after Ismail's review

* Apply suggestions from code review

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

* fix merge conflict

* remove sleep from TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode

We don't need it actually.

CHANGELOG_PENDING.md

@@ -23,11 +23,16 @@
 - [cli] \#3585 Add option to not clear address book with unsafe reset (@climber73)
 - [cli] [\#3160](https://github.com/tendermint/tendermint/issues/3160) Add `-config=<path-to-config>` option to `testnet` cmd (@gregdhill)
 - [cs/replay] \#3460 check appHash for each block
+- [rpc] \#3362 `/dial_seeds` & `/dial_peers` return errors if addresses are incorrect (except when IP lookup fails)
+- [node] \#3362 returns an error if `persistent_peers` list is invalid (except when IP lookup fails)
 - [p2p] \#3531 Terminate session on nonce wrapping (@climber73)
 
 ### BUG FIXES:
 - [p2p] \#3532 limit the number of attempts to connect to a peer in seed mode
   to 16 (as a result, the node will stop retrying after a 35 hours time window)
 - [consensus] \#2723, \#3451 and \#3317 Fix non-deterministic tests
+- [p2p] \#3362 make persistent prop independent of conn direction
+  * `Switch#DialPeersAsync` now only takes a list of peers
+  * `Switch#DialPeerWithAddress` now only takes an address
 - [consensus] \#3067 getBeginBlockValidatorInfo loads validators from stateDB instead of state (@james-ray)
 - [pex] \#3603 Dial seeds when addrbook needs more addresses (@defunctzombie)

docs/spec/p2p/config.md

@@ -12,14 +12,14 @@ and upon incoming connection shares some peers and disconnects.
 
 ## Seeds
 
-`--p2p.seeds “1.2.3.4:26656,2.3.4.5:4444”`
+`--p2p.seeds “id100000000000000000000000000000000@1.2.3.4:26656,id200000000000000000000000000000000@2.3.4.5:4444”`
 
 Dials these seeds when we need more peers. They should return a list of peers and then disconnect.
 If we already have enough peers in the address book, we may never need to dial them.
 
 ## Persistent Peers
 
-`--p2p.persistent_peers “1.2.3.4:26656,2.3.4.5:26656”`
+`--p2p.persistent_peers “id100000000000000000000000000000000@1.2.3.4:26656,id200000000000000000000000000000000@2.3.4.5:26656”`
 
 Dial these peers and auto-redial them if the connection fails.
 These are intended to be trusted persistent peers that can help
@@ -30,9 +30,9 @@ backoff and will give up after a day of trying to connect.
 the user will be warned that seeds may auto-close connections
 and that the node may not be able to keep the connection persistent.
 
-## Private Persistent Peers
+## Private Peers
 
-`--p2p.private_persistent_peers “1.2.3.4:26656,2.3.4.5:26656”`
+`--p2p.private_peer_ids “id100000000000000000000000000000000,id200000000000000000000000000000000”`
 
-These are persistent peers that we do not add to the address book or
-gossip to other peers. They stay private to us.
+These are IDs of the peers that we do not add to the address book or gossip to
+other peers. They stay private to us.

node/node.go

@@ -579,6 +579,11 @@ func NewNode(config *cfg.Config,
 		consensusReactor, evidenceReactor, nodeInfo, nodeKey, p2pLogger,
 	)
 
+	err = sw.AddPersistentPeers(splitAndTrimEmpty(config.P2P.PersistentPeers, ",", " "))
+	if err != nil {
+		return nil, errors.Wrap(err, "could not add peers from persistent_peers field")
+	}
+
 	addrBook := createAddrBookAndSetOnSwitch(config, sw, p2pLogger)
 
 	// Optionally, start the pex reactor
@@ -675,12 +680,8 @@ func (n *Node) OnStart() error {
 	}
 
 	// Always connect to persistent peers
-	if n.config.P2P.PersistentPeers != "" {
-		err = n.sw.DialPeersAsync(n.addrBook, splitAndTrimEmpty(n.config.P2P.PersistentPeers, ",", " "), true)
-		if err != nil {
-			return err
-		}
-	}
+	// parsing errors are handled above by AddPersistentPeers
+	_ = n.sw.DialPeersAsync(splitAndTrimEmpty(n.config.P2P.PersistentPeers, ",", " "))
 
 	return nil
 }

p2p/pex/pex_reactor.go

@@ -531,8 +531,7 @@ func (r *PEXReactor) dialPeer(addr *p2p.NetAddress) error {
 		}
 	}
 
-	err := r.Switch.DialPeerWithAddress(addr, false)
-
+	err := r.Switch.DialPeerWithAddress(addr)
 	if err != nil {
 		if _, ok := err.(p2p.ErrCurrentlyDialingOrExistingAddress); ok {
 			return err
@@ -584,7 +583,7 @@ func (r *PEXReactor) dialSeeds() {
 	for _, i := range perm {
 		// dial a random seed
 		seedAddr := r.seedAddrs[i]
-		err := r.Switch.DialPeerWithAddress(seedAddr, false)
+		err := r.Switch.DialPeerWithAddress(seedAddr)
 		if err == nil {
 			return
 		}

p2p/pex/pex_reactor_test.go

@@ -291,7 +291,8 @@ func TestPEXReactorSeedMode(t *testing.T) {
 	require.Nil(t, err)
 	defer os.RemoveAll(dir) // nolint: errcheck
 
-	pexR, book := createReactor(&PEXReactorConfig{SeedMode: true, SeedDisconnectWaitPeriod: 10 * time.Millisecond})
+	pexRConfig := &PEXReactorConfig{SeedMode: true, SeedDisconnectWaitPeriod: 10 * time.Millisecond}
+	pexR, book := createReactor(pexRConfig)
 	defer teardownReactor(book)
 
 	sw := createSwitchAndAddReactors(pexR)
@@ -315,13 +316,48 @@ func TestPEXReactorSeedMode(t *testing.T) {
 	pexR.attemptDisconnects()
 	assert.Equal(t, 1, sw.Peers().Size())
 
-	time.Sleep(100 * time.Millisecond)
+	// sleep for SeedDisconnectWaitPeriod
+	time.Sleep(pexRConfig.SeedDisconnectWaitPeriod + 1*time.Millisecond)
 
 	// 3. attemptDisconnects should disconnect after wait period
 	pexR.attemptDisconnects()
 	assert.Equal(t, 0, sw.Peers().Size())
 }
 
+func TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode(t *testing.T) {
+	// directory to store address books
+	dir, err := ioutil.TempDir("", "pex_reactor")
+	require.Nil(t, err)
+	defer os.RemoveAll(dir) // nolint: errcheck
+
+	pexR, book := createReactor(&PEXReactorConfig{SeedMode: true, SeedDisconnectWaitPeriod: 1 * time.Millisecond})
+	defer teardownReactor(book)
+
+	sw := createSwitchAndAddReactors(pexR)
+	sw.SetAddrBook(book)
+	err = sw.Start()
+	require.NoError(t, err)
+	defer sw.Stop()
+
+	assert.Zero(t, sw.Peers().Size())
+
+	peerSwitch := testCreateDefaultPeer(dir, 1)
+	require.NoError(t, peerSwitch.Start())
+	defer peerSwitch.Stop()
+
+	err = sw.AddPersistentPeers([]string{peerSwitch.NetAddress().String()})
+	require.NoError(t, err)
+
+	// 1. Test crawlPeers dials the peer
+	pexR.crawlPeers([]*p2p.NetAddress{peerSwitch.NetAddress()})
+	assert.Equal(t, 1, sw.Peers().Size())
+	assert.True(t, sw.Peers().Has(peerSwitch.NodeInfo().ID()))
+
+	// 2. attemptDisconnects should not disconnect because the peer is persistent
+	pexR.attemptDisconnects()
+	assert.Equal(t, 1, sw.Peers().Size())
+}
+
 func TestPEXReactorDialsPeerUpToMaxAttemptsInSeedMode(t *testing.T) {
 	// directory to store address books
 	dir, err := ioutil.TempDir("", "pex_reactor")
@@ -398,7 +434,7 @@ func TestPEXReactorSeedModeFlushStop(t *testing.T) {
 	reactor := switches[0].Reactors()["pex"].(*PEXReactor)
 	peerID := switches[1].NodeInfo().ID()
 
-	err = switches[1].DialPeerWithAddress(switches[0].NetAddress(), false)
+	err = switches[1].DialPeerWithAddress(switches[0].NetAddress())
 	assert.NoError(t, err)
 
 	// sleep up to a second while waiting for the peer to send us a message.