p2p: introduce peerConn to simplify peer creation by ebuchman · Pull Request #1226 · tendermint/tendermint

ebuchman · 2018-02-20T02:27:33Z

config.AuthEnc p2p/config: options to require/not-require auth #1157
Introduce peerConn containing the known fields of peer
peer only created in sw.addPeer once handshake is complete and NodeInfo is checked
Eliminates some mutable variables and makes the code flow better
Simplifies the newXxxPeer funcs
Use ID instead of PubKey where possible.
* SetPubKeyFilter -> SetIDFilter
* nodeInfo.Validate takes no args
* remove peer.PubKey()

melekes · 2018-02-20T06:35:34Z

p2p/switch.go

-			return
+		err := sw.DialPeerWithAddress(netAddr, true)
+		if err == nil {
+			return // success


don't you think we should log success path too?

yes it's logged within the DialPeerWithAddress function.

I wanted to avoid returning the peer because we weren't doing anything except logging it.

So I think it makes sense to let the function log it's success case, but the caller logs the error. Does that make sense or am I being silly ?

maybe.. but I still think it's important to give user this sequence

[] .... Reconnecting to node A [] .... Reconnected ...

melekes · 2018-02-20T06:38:41Z

p2p/switch.go

+	if err := sw.FilterConnByAddr(addr); err != nil {
+		return err
+	}
+	if err := sw.FilterConnByID(id); err != nil {


Have you tried running this with [p2p] auth_enc=false? If it is false, then id will be nil, right? So effectively, filter will not work

Hmm crap. I think we need some extra work to make this right for AuthEnc=false. We prob dont want the id to be empty, even if it can't be verified, because we use it eg. as a key for the AddrBook. So if we want AddrBook to work with AuthEnc, we need to do something smarter.

OK thanks

melekes · 2018-02-20T06:39:15Z

p2p/pex/pex_reactor.go

-			r.Switch.Logger.Info("Connected to seed", "peer", peer)
+		err := r.Switch.DialPeerWithAddress(seedAddr, false)
+		if err == nil {
 			return


same comment about logging success path

Same response :)

melekes · 2018-02-20T06:40:20Z

Very clean code ❤️ 💚 💛 but see my comments

if AuthEnc is true, dialed peers must have a node ID in the address and it must match the persistent pubkey from the secret handshake. Refs #1157

``` p2p/pex/pex_reactor_test.go:288:88: cannot use seed.NodeInfo().NetAddress() (type *p2p.NetAddress) as type string in array or slice literal ```

* Introduce `peerConn` containing the known fields of `peer` * `peer` only created in `sw.addPeer` once handshake is complete and NodeInfo is checked * Eliminates some mutable variables and makes the code flow better * Simplifies the `newXxxPeer` funcs * Use ID instead of PubKey where possible. * SetPubKeyFilter -> SetIDFilter * nodeInfo.Validate takes ID * remove peer.PubKey()

melekes · 2018-02-26T19:06:59Z

Testing fastsync on node 2
starting tendermint peer ID=2
ae8356685719cf2899cf7adcd464391182c184e602fae650467c4c9601c57211
starting test client container with CMD=test/p2p/fast_sync/check_peer.sh 2
Other peer is on height 35 with state "0800000000000000"
Waiting for peer 2 to catch up
... 0
... 0
... 3
... 34
... 35
... 35

fast_sync failed for 2nd peer.

2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: panic: runtime error: invalid memory address or nil pointer dereference
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x98dd3e]
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]:
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: goroutine 3432 [running]:
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.newOutboundPeerConn(0xc423fd1380, 0xc420933e00, 0x1, 0x1239a60, 0
xc420128c40, 0x2, 0x42caf6, 0xc42001f300, 0xc422831d98, 0xc4227951c0, ...)
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #011/go/src/github.com/tendermint/tendermint/p2p/peer.go:123 +0x31e
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.(*Switch).addOutboundPeerWithConfig(0xc4200ad040, 0xc423fd1380, 0
xc420933e00, 0xc423f48801, 0x28, 0x2)
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #011/go/src/github.com/tendermint/tendermint/p2p/switch.go:455 +0x12b
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.(*Switch).DialPeerWithAddress(0xc4200ad040, 0xc423fd1380, 0x1, 0x
0, 0x0)
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #011/go/src/github.com/tendermint/tendermint/p2p/switch.go:371 +0xdc
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.(*Switch).reconnectToPeer(0xc4200ad040, 0x123e000, 0xc42007bb00)
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #011/go/src/github.com/tendermint/tendermint/p2p/switch.go:290 +0x25f
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: created by github.com/tendermint/tendermint/p2p.(*Switch).StopPeerForError
2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #011/go/src/github.com/tendermint/tendermint/p2p/switch.go:256 +0x1b7

``` 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: panic: runtime error: invalid memory address or nil pointer dereference 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x98dd3e] 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: goroutine 3432 [running]: 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.newOutboundPeerConn(0xc423fd1380, 0xc420933e00, 0x1, 0x1239a60, 0 xc420128c40, 0x2, 0x42caf6, 0xc42001f300, 0xc422831d98, 0xc4227951c0, ...) 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #11/go/src/github.com/tendermint/tendermint/p2p/peer.go:123 +0x31e 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.(*Switch).addOutboundPeerWithConfig(0xc4200ad040, 0xc423fd1380, 0 xc420933e00, 0xc423f48801, 0x28, 0x2) 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #11/go/src/github.com/tendermint/tendermint/p2p/switch.go:455 +0x12b 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.(*Switch).DialPeerWithAddress(0xc4200ad040, 0xc423fd1380, 0x1, 0x 0, 0x0) 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #11/go/src/github.com/tendermint/tendermint/p2p/switch.go:371 +0xdc 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: github.com/tendermint/tendermint/p2p.(*Switch).reconnectToPeer(0xc4200ad040, 0x123e000, 0xc42007bb00) 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #11/go/src/github.com/tendermint/tendermint/p2p/switch.go:290 +0x25f 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: created by github.com/tendermint/tendermint/p2p.(*Switch).StopPeerForError 2018-02-21T06:30:05Z box887.localdomain docker/local_testnet_4[14907]: #11/go/src/github.com/tendermint/tendermint/p2p/switch.go:256 +0x1b7 ```

codecov-io · 2018-02-27T11:52:00Z

Codecov Report

Merging #1226 into develop will increase coverage by 0.2%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           develop    #1226     +/-   ##
==========================================
+ Coverage    59.49%   59.69%   +0.2%     
==========================================
  Files          121      122      +1     
  Lines        11046    11047      +1     
==========================================
+ Hits          6572     6595     +23     
+ Misses        3863     3846     -17     
+ Partials       611      606      -5

ebuchman · 2018-02-28T20:39:45Z

p2p/peer.go

 	pc, err = newPeerConn(conn, config, true, persistent, ourNodePrivKey)
 	if err != nil {
-		if err2 := conn.Close(); err != nil {
+		if err2 := conn.Close(); err2 != nil {


:o !!! thanks

is there a good paradigm/tactic we can use to avoid this kind of thing ?

always use err? metalinter?

* CV OnStop close evidenceStore * CV OnStop print db close * CV add changelog * CV update changelog with attribution (cherry picked from commit 48335a0) Co-authored-by: Chill Validation <92176880+chillyvee@users.noreply.github.com>

ebuchman requested a review from melekes as a code owner February 20, 2018 02:27

ebuchman force-pushed the peerConn branch from 272a847 to ee67b0a Compare February 20, 2018 02:34

ebuchman changed the title ~~p2p: introduce peerConn to clarify peer creation~~ p2p: introduce peerConn to simplify peer creation Feb 20, 2018

melekes reviewed Feb 20, 2018

View reviewed changes

ebuchman force-pushed the peerConn branch 2 times, most recently from 4be5554 to d8d7944 Compare February 21, 2018 02:43

ebuchman mentioned this pull request Feb 21, 2018

expose AuthEnc in the P2P config #1214

Closed

ebuchman changed the base branch from 1157-option-to-require-auth to develop February 21, 2018 05:58

ebuchman force-pushed the peerConn branch from d8d7944 to 455d6cb Compare February 21, 2018 06:02

melekes and others added 8 commits February 21, 2018 01:05

expose AuthEnc in the P2P config

d4d8229

if AuthEnc is true, dialed peers must have a node ID in the address and it must match the persistent pubkey from the secret handshake. Refs #1157

fixes after my own review

6f1829f

fix docs

4f616dc

fix build failure

d2e005a

``` p2p/pex/pex_reactor_test.go:288:88: cannot use seed.NodeInfo().NetAddress() (type *p2p.NetAddress) as type string in array or slice literal ```

persistent node ids

325514b

fixes from review

9d5d5c1

test: use ip_plus_id.sh more

455d6cb

greg-szabo assigned melekes Feb 26, 2018

melekes approved these changes Feb 27, 2018

View reviewed changes

melekes merged commit 9293ae7 into develop Feb 27, 2018

melekes deleted the peerConn branch February 27, 2018 11:55

ebuchman commented Feb 28, 2018

View reviewed changes

ebuchman mentioned this pull request Mar 19, 2018

p2p: Fix flaky TestSwitchReconnectsToPersistentPeer #1266

Closed

Conversation

ebuchman commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Feb 20, 2018

Uh oh!

melekes commented Feb 26, 2018

Uh oh!

codecov-io commented Feb 27, 2018

Codecov Report

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ebuchman commented Feb 20, 2018 •

edited

Loading