make persistent prop independent of conn direction by melekes · Pull Request #3593 · tendermint/tendermint

melekes · 2019-04-25T10:23:52Z

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

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

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

codecov-io · 2019-04-25T10:51:16Z

Codecov Report

Merging #3593 into develop will increase coverage by 0.33%.
The diff coverage is 75.64%.

@@             Coverage Diff             @@
##           develop    #3593      +/-   ##
===========================================
+ Coverage    64.25%   64.59%   +0.33%     
===========================================
  Files          213      213              
  Lines        17463    17507      +44     
===========================================
+ Hits         11221    11308      +87     
+ Misses        5308     5255      -53     
- Partials       934      944      +10

Impacted Files	Coverage Δ
rpc/core/pipe.go	`28.84% <ø> (ø)`	⬆️
rpc/core/net.go	`41.66% <100%> (+41.66%)`	⬆️
p2p/pex/pex_reactor.go	`81.6% <100%> (-1.79%)`	⬇️
p2p/transport.go	`85.3% <100%> (+0.49%)`	⬆️
node/node.go	`64.44% <50%> (+0.37%)`	⬆️
p2p/switch.go	`68.73% <70.68%> (+4.66%)`	⬆️
libs/db/debug_db.go	`15.78% <0%> (-4.53%)`	⬇️
libs/clist/clist.go	`66.66% <0%> (-1.52%)`	⬇️
state/errors.go	`0% <0%> (ø)`	⬆️
... and 9 more

p2p/transport.go

liamsi

Just a comment about the interface method that returns []error. Will do a 2nd pass later today.

rpc/core/pipe.go

p2p/switch.go

liamsi

Looks great! Left a few nits and merge conflicts need resolving.

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.

Any reasons why this behaviour could be undesired?

p2p/switch.go

p2p/pex/pex_reactor_test.go

node/node.go

docs/spec/p2p/config.md

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

melekes · 2019-04-30T10:52:01Z

Any reasons why this behaviour could be undesired?

No.

malicious node could report different address (say address our node has in persistent_peers), but this won't give it any advantages.

node/node.go

…SeedMode We don't need it actually.

ebuchman

Really nice change, thanks for this Anton, but seems we need a few fixes.

ebuchman · 2019-05-03T16:21:17Z

node/node.go

-		}
-	}
+	// parsing errors are handled above by AddPersistentPeers
+	_ = n.sw.DialPeersAsync(splitAndTrimEmpty(n.config.P2P.PersistentPeers, ",", " "))


We should still handle this anyways for completeness, no?

Also is there a way we can avoid having to use the splitAndTrimEmpty again?

We should still handle this anyways for completeness, no?

ok

Also is there a way we can avoid having to use the splitAndTrimEmpty again?

A bit annoying, but I think we can live with it for now.

ebuchman · 2019-05-03T16:22:54Z

p2p/switch.go

+			addr, err = peer.NodeInfo().NetAddress()
+			if err != nil {
+				sw.Logger.Error("Wanted to reconnect to inbound peer, but self-reported address is wrong",
+					"peer", peer, "addr", addr, "err", err)


Won't we have addr = nil here?

ebuchman · 2019-05-03T16:24:11Z

p2p/switch.go

 // Used to dial peers from config on startup or from unsafe-RPC (trusted sources).
-// TODO: remove addrBook arg since it's now set on the switch
-func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent bool) error {
+// It ignores ErrNetAddressLookup. However, if there are other errors, first


Why do we ignore these?

Imagine you're starting a k8s cluster and DNS does not work yet => lookup fails => Tendermint fails to start.

If we ignore ErrNetAddressLookup, we won't be able to dial peers immediately, but reconnect will connect us to them eventually (only to persistent peers though).

Maybe we should add a flag ignoreNetAddressLookupErrors

but reconnect will connect us to them eventually (only to persistent peers though).

this won't work in the current implementation because we do not try to reconnect. hmmm

I think NetAddress should resolve IP right before dialing + resolve it again when IP is no longer reachable.

ebuchman · 2019-05-03T16:27:50Z

rpc/core/net.go

-	err := p2pPeers.DialPeersAsync(addrBook, peers, persistent)
-	if err != nil {
+	logger.Info("DialPeers", "peers", peers, "persistent", persistent)
+	if err := p2pPeers.AddPersistentPeers(peers); err != nil {


Shouldn't this only run if persistent == true ?

ebuchman · 2019-05-03T16:28:29Z

rpc/core/net.go

 		return &ctypes.ResultDialPeers{}, err
 	}
+	// parsing errors are handled above by AddPersistentPeers
+	_ = p2pPeers.DialPeersAsync(peers)


We should still handle the error for completeness. And if persistent==false then the AddPersistentPeers shouldn't run and we'll need to check the errors here anyways.

ebuchman · 2019-05-03T17:24:48Z

p2p/pex/pex_reactor_test.go

+
+	// 2. attemptDisconnects should not disconnect because the peer is persistent
+	pexR.attemptDisconnects()
+	assert.Equal(t, 1, sw.Peers().Size())


This passes even if we comment out the AddPersistenPeers line. I think the attemptDisconnects isn't causing a disconnect because of the timing constraints. If we sleep before it, then it fails without the AddPersistentPeers, but maybe there's a better way ?

ebuchman · 2019-05-03T17:29:12Z

p2p/switch.go

+	// return first non-ErrNetAddressLookup error
+	for _, err := range errs {
+		if _, ok := err.(ErrNetAddressLookup); ok {
+			continue


Doesn't this mean we might include nil in the list? What is the value of ignoring this error here if we don't even get a netAddr out of it?

I'll need to think about it. See my other comment #3593 (comment)

Doesn't this mean we might include nil in the list?

no

https://github.com/tendermint/tendermint/blob/develop/p2p/netaddress.go#L134-L138

ebuchman · 2019-05-03T17:30:30Z

p2p/switch.go

+	return nil
+}
+
+func (sw *Switch) isPeerPersistentFn() func(*NetAddress) bool {


I wonder if this should just check the ID instead. Since for instance if there's DNS involved the IP address could change between when it got added to persistent peers and when we connected to it and then this would fail.

Also why is this a closure?

I wonder if this should just check the ID instead. Since for instance if there's DNS involved the IP address could change between when it got added to persistent peers and when we connected to it and then this would fail.

This deserves a separate issue. One argument is favor of IP addresses is that it's more static. Maybe there are some DNS attacks we're not considering.

Also why is this a closure?

I thought it would be weird to have persistent_peers list in peerConfig.

ebuchman · 2019-05-03T17:32:26Z

p2p/switch_test.go

-	require.NotNil(sw.Peers().Get(rp.ID()))
+	err = sw.DialPeerWithAddress(rp.Addr())
+	require.Nil(t, err)
+	time.Sleep(50 * time.Millisecond)


Can we do better than a sleep here?

ebuchman · 2019-05-03T17:33:31Z

p2p/switch_test.go

-	err = sw.addOutboundPeerWithConfig(rp.Addr(), conf, true)
-	require.NotNil(err)
-
+	err = sw.addOutboundPeerWithConfig(rp.Addr(), conf)


don't we need to make this peer persistent to preserve the test case?

No. Because we'll reconnect anyway if TestDialFail is set

tendermint/p2p/switch.go

Line 653 in 5051a1f

go sw.reconnectToPeer(addr)

ebuchman · 2019-05-03T17:39:40Z

malicious node could report different address (say address our node has in persistent_peers), but this won't give it any advantages.

I think we can address this by just using IDs for persistent peers.

also - handle errors from DialPeersAsync - remove nil addr from log msg - fix TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode This is a follow-up from #3593 (review)

## Description also handle errors from DialPeersAsync remove nil addr from log msg fix TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode This is a follow-up from #3593 (review) Fixes most of the #3617, except #3593 (comment) ## Commits * rpc: /dial_peers: only mark peers as persistent if flag is on also - handle errors from DialPeersAsync - remove nil addr from log msg - fix TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode This is a follow-up from #3593 (review) * remove a call to AddPersistentPeers TestDialFail will trigger a reconnect

) ## 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 tendermint#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 tendermint#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.

…mint#3620) ## Description also handle errors from DialPeersAsync remove nil addr from log msg fix TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode This is a follow-up from tendermint#3593 (review) Fixes most of the tendermint#3617, except tendermint#3593 (comment) ## Commits * rpc: /dial_peers: only mark peers as persistent if flag is on also - handle errors from DialPeersAsync - remove nil addr from log msg - fix TestPEXReactorDoesNotDisconnectFromPersistentPeerInSeedMode This is a follow-up from tendermint#3593 (review) * remove a call to AddPersistentPeers TestDialFail will trigger a reconnect

) ## 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 tendermint#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 tendermint#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.

melekes added 2 commits April 25, 2019 14:23

fix TestPEXReactorDialPeer test

470914b

melekes self-assigned this Apr 25, 2019

melekes added 3 commits April 25, 2019 14:54

add a changelog entry

c4fed3b

update changelog

6b68d95

add two tests

b1dda07

golangcibot reviewed Apr 25, 2019

View reviewed changes

p2p/transport.go Outdated Show resolved Hide resolved

reformat code

1b2b12b

melekes marked this pull request as ready for review April 25, 2019 13:00

melekes requested review from ebuchman and xla as code owners April 25, 2019 13:00

melekes added 3 commits April 25, 2019 18:07

test UnsafeDialPeers and UnsafeDialSeeds

06a82b3

add TestSwitchDialPeersAsync

d92ed81

spec: update p2p/config spec

f6a3ae3

liamsi reviewed Apr 29, 2019

View reviewed changes

rpc/core/pipe.go Outdated Show resolved Hide resolved

rpc/core/pipe.go Outdated Show resolved Hide resolved

p2p/switch.go Outdated Show resolved Hide resolved

fixes after Ismail's review

e5538a0

liamsi approved these changes Apr 30, 2019

View reviewed changes

p2p/switch.go Outdated Show resolved Hide resolved

p2p/switch.go Outdated Show resolved Hide resolved

p2p/pex/pex_reactor_test.go Outdated Show resolved Hide resolved

node/node.go Outdated Show resolved Hide resolved

docs/spec/p2p/config.md Show resolved Hide resolved

Apply suggestions from code review

cf5ee51

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

Merge branch 'develop' into 3362-persistent-inbound-peers

61c35f2

golangcibot reviewed May 1, 2019

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

node/node.go Outdated Show resolved Hide resolved

melekes added 3 commits May 1, 2019 10:19

fix merge conflict

6585c1f

remove sleep from TestPEXReactorDoesNotDisconnectFromPersistentPeerIn…

66a542d

…SeedMode We don't need it actually.

Merge branch 'develop' into 3362-persistent-inbound-peers

5693e95

melekes merged commit 8711af6 into develop May 3, 2019

melekes deleted the 3362-persistent-inbound-peers branch May 3, 2019 13:22

melekes mentioned this pull request May 3, 2019

p2p: Seed node will disconnect from persistent peer if inbound #3362

Closed

ebuchman reviewed May 3, 2019

View reviewed changes

ebuchman mentioned this pull request May 3, 2019

Follow up to #3593: fixes for persistent peers #3617

Closed

melekes mentioned this pull request May 4, 2019

rpc: /dial_peers: only mark peers as persistent if flag is on #3620

Merged

4 tasks

This was referenced May 7, 2019

Reconnect to persistent peers upon ErrNetAddressLookup #3635

Closed

v0.31.6 release preview #3637

Closed

melekes mentioned this pull request May 30, 2019

temporary #3684

Closed

44 tasks

ebuchman mentioned this pull request May 30, 2019

Consider basing persistence just on ID #3692

Closed

chengwenxi mentioned this pull request Sep 4, 2019

Update p2p to v0.32 irisnet/irishub#1908

Closed

Conversation

melekes commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamsi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes commented Apr 30, 2019

Uh oh!

Uh oh!

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

melekes commented Apr 25, 2019 •

edited

Loading

codecov-io commented Apr 25, 2019 •

edited

Loading

liamsi left a comment •

edited

Loading

melekes May 4, 2019 •

edited

Loading

melekes May 4, 2019 •

edited

Loading

melekes May 4, 2019 •

edited

Loading

melekes May 4, 2019 •

edited

Loading