p2p: re-check after sleeps #2664

mgurevin · 2018-10-18T16:23:57Z

Hi there, I saw there are many re-connection attempts and trying to connect itself while ensuring peers in pex reactor. I think that issues appear for me because I'm testing three node setup with persistent peers. So I guess this patch is the solution. It works for me.

codecov-io · 2018-10-18T16:32:28Z

Codecov Report

Merging #2664 into develop will decrease coverage by 0.04%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2664      +/-   ##
===========================================
- Coverage    62.37%   62.32%   -0.05%     
===========================================
  Files          212      212              
  Lines        17210    17215       +5     
===========================================
- Hits         10734    10729       -5     
- Misses        5578     5585       +7     
- Partials       898      901       +3

Impacted Files	Coverage Δ
p2p/switch.go	`52.45% <0%> (+0.8%)`	⬆️
p2p/netaddress.go	`71.51% <0%> (-0.88%)`	⬇️
consensus/reactor.go	`67.13% <0%> (-1.17%)`	⬇️
p2p/pex/pex_reactor.go	`73.37% <0%> (+0.32%)`	⬆️

melekes

Thanks for the PR! Would be great if you answer my questions and provide logs with reconnect attempts (if possible).

melekes · 2018-10-21T07:55:45Z

p2p/switch.go

@@ -325,6 +325,11 @@ func (sw *Switch) reconnectToPeer(addr *NetAddress) {
 			return
 		}

+		if sw.IsDialingOrExistingAddress(addr) {
+			sw.Logger.Info("Peer connection has been established or dialed while we waiting next try", "addr", addr)


melekes · 2018-10-21T07:59:16Z

p2p/pex/pex_reactor.go

@@ -395,6 +394,9 @@ func (r *PEXReactor) ensurePeers() {
 		if r.Switch.IsDialingOrExistingAddress(try) {
 			continue
 		}
+		if r.Switch.NodeInfo().ID() == try.ID {


Why did you decide to add this check? If we're picking our address for connecting in ensurePeers, then something is wrong. That's why we add code like

tendermint/p2p/switch.go

Line 397 in f94eb42

if !netAddr.Same(ourAddr) {

and

tendermint/node/node.go

Lines 458 to 459 in f94eb42

// Add ourselves to addrbook to prevent dialing ourselves

addrBook.AddOurAddress(nodeInfo.NetAddress())

it's a little messy, but the point is ensurePeers should only use external addresses.

You are right. The protocol part of the listen address definitions in the configuration caused my problem.

If the listen address looks like tcp://995f83b21e6b66b62883853cc77fe567da48986c@0.0.0.0:2101 the id part can not be resolved in node_info.

So it's caused to unable to check our address.

melekes · 2018-11-03T09:33:39Z

@mgurevin Hi! Could you follow up on my questions? We want to merge this, but #2664 (comment) needs to be addressed before.

mgurevin · 2018-11-04T12:02:47Z

Hi @melekes, I saw your comment. I will split my additions and send their detailed logs in a few days, sorry for delay.

This reverts commit 5f7d055.

This reverts commit 7f41070.

mgurevin · 2018-11-08T15:47:05Z

Hi again,

I have split my additions into different commits and attach their detailed logs.

My setup is three node and they defined as persistent nodes in the configuration files.

Log files;

No change: develop, Log files: node1.log, node2.log, node3.log
Commit: re-check after sleep, Log files: node1.log, node2.log, node3.log
Commit: ignore ensured connections while re-connecting, Log files: node1.log, node2.log, node3.log
Commit: preserve dial to itself, Log files: node1.log, node2.log, node3.log
Commit (final): keep protocol definition on net addresses, Log files: node1.log, node2.log, node3.log

This reverts commit 0c6e0fc.

mgurevin · 2018-11-08T16:42:40Z

I have complete my pull request, if it looks good you can merge.

The main problem is the listen address definition because node ID cannot be resolved if it contains the protocol part.

In this pull request, we fixed this issue and add check the actual connection status when reconnecting after sleep.

melekes

🍓 🍌 🥛

p2p/netaddress.go

Co-Authored-By: mgurevin <mehmet@gurevin.net>

ebuchman

Thanks for this. I'll merge develop in, fix the conflicts, and merge this PR

ebuchman · 2018-11-11T12:43:13Z

p2p/netaddress.go

 func IDAddressString(id ID, hostPort string) string {
+	// we respect the protocol definition in here.


We merged a similar fix in https://github.com/tendermint/tendermint/pull/2797/files#diff-91e8069d5cb91a5b3067a01482559679R38. There we just drop the protocol prefix, since we shouldn't need it anymore. Sorry didn't see this earlier, it would have saved us some time.

ebuchman · 2018-11-11T12:47:17Z

p2p/switch.go

@@ -328,6 +328,11 @@ func (sw *Switch) reconnectToPeer(addr *NetAddress) {
 			return
 		}

+		if sw.IsDialingOrExistingAddress(addr) {


Thanks. These changes are partial fixes for #2716.

Note the races are still possible, but this should help significantly. We'll likely have to rearchitect the code a bit to solve this fully.

Thanks!

* 'master' of https://github.com/tendermint/tendermint: (330 commits) Release/v0.26.1 (tendermint#2803) fix amino overhead computation for Tx (tendermint#2792) p2p: re-check after sleeps (tendermint#2664) check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787) p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797) test AutoFile#Size (happy path) [autofile/group] do not panic when checking size openFile creates a file if not exist => ErrNotExist is not possible use our logger in autofile/group Add tests for ValidateBasic methods (tendermint#2754) [docs] improve organization of ABCI docs & fix links (tendermint#2749) p2p: peer-id -> peer_id (tendermint#2771) mempool: print postCheck error (tendermint#2762) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756) Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758) mempool: ErrPreCheck and more log info (tendermint#2724) Release/v0.26.0 (tendermint#2726) [ADR] [DRAFT] pubsub 2.0 (tendermint#2532) validate reactor messages (tendermint#2711) TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732) ...

mgurevin added 2 commits October 18, 2018 18:52

p2p: re-check after sleeps

7f41070

use NodeInfo as an interface

5f7d055

mgurevin requested review from ebuchman, melekes and xla as code owners October 18, 2018 16:23

mgurevin changed the title ~~Develop~~ p2p: re-check after sleeps Oct 18, 2018

melekes reviewed Oct 21, 2018

View reviewed changes

mgurevin added 7 commits November 8, 2018 17:54

Revert "use NodeInfo as an interface"

53704f7

This reverts commit 5f7d055.

Revert "p2p: re-check after sleeps"

70cfc73

This reverts commit 7f41070.

Merge remote-tracking branch 'upstream/develop' into HEAD

02ad2f0

preserve dial to itself

0c6e0fc

ignore ensured connections while re-connecting

64d92dd

re-check after sleep

cf16823

keep protocol definition on net addresses

060d0b5

mgurevin added 2 commits November 8, 2018 18:50

decrease log level

72b9370

Revert "preserve dial to itself"

c8c8345

This reverts commit 0c6e0fc.

melekes approved these changes Nov 9, 2018

View reviewed changes

p2p/netaddress.go Outdated Show resolved Hide resolved

correct func comment according to modification

bb0f5c4

Co-Authored-By: mgurevin <mehmet@gurevin.net>

ebuchman approved these changes Nov 11, 2018

View reviewed changes

Merge branch 'develop' into develop

abab122

ebuchman merged commit 905abf1 into tendermint:develop Nov 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: re-check after sleeps #2664

p2p: re-check after sleeps #2664

mgurevin commented Oct 18, 2018

codecov-io commented Oct 18, 2018 •

edited

melekes left a comment

melekes Oct 21, 2018

mgurevin Nov 8, 2018

melekes Oct 21, 2018

mgurevin Nov 8, 2018

melekes commented Nov 3, 2018

mgurevin commented Nov 4, 2018

mgurevin commented Nov 8, 2018

mgurevin commented Nov 8, 2018

melekes left a comment

ebuchman left a comment

ebuchman Nov 11, 2018

ebuchman Nov 11, 2018

	// Add ourselves to addrbook to prevent dialing ourselves
	addrBook.AddOurAddress(nodeInfo.NetAddress())

		func IDAddressString(id ID, hostPort string) string {
		// we respect the protocol definition in here.

p2p: re-check after sleeps #2664

p2p: re-check after sleeps #2664

Conversation

mgurevin commented Oct 18, 2018

codecov-io commented Oct 18, 2018 • edited

Codecov Report

melekes left a comment

Choose a reason for hiding this comment

melekes Oct 21, 2018

Choose a reason for hiding this comment

mgurevin Nov 8, 2018

Choose a reason for hiding this comment

melekes Oct 21, 2018

Choose a reason for hiding this comment

mgurevin Nov 8, 2018

Choose a reason for hiding this comment

melekes commented Nov 3, 2018

mgurevin commented Nov 4, 2018

mgurevin commented Nov 8, 2018

mgurevin commented Nov 8, 2018

melekes left a comment

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

ebuchman Nov 11, 2018

Choose a reason for hiding this comment

ebuchman Nov 11, 2018

Choose a reason for hiding this comment

codecov-io commented Oct 18, 2018 •

edited