[ADR] PeerTransport by xla · Pull Request #2069 · tendermint/tendermint

xla · 2018-07-25T16:22:24Z

First iteration in #2067 and an attempt to liberate the Switch from connection handling and low-level network concenrs.

codecov-io · 2018-07-25T16:26:33Z

Codecov Report

Merging #2069 into develop will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2069      +/-   ##
===========================================
- Coverage    61.36%   61.32%   -0.04%     
===========================================
  Files          197      197              
  Lines        15678    15663      -15     
===========================================
- Hits          9621     9606      -15     
- Misses        5246     5248       +2     
+ Partials       811      809       -2

Impacted Files	Coverage Δ
libs/common/cmap.go	`88% <0%> (-2.48%)`	⬇️
consensus/state.go	`77.09% <0%> (-0.37%)`	⬇️
libs/common/kvpair.go	`0% <0%> (ø)`	⬆️
libs/events/event_cache.go	`100% <0%> (ø)`	⬆️
p2p/peer.go	`61.36% <0%> (+1.13%)`	⬆️
consensus/reactor.go	`79.01% <0%> (+1.33%)`	⬆️
rpc/client/helpers.go	`91.3% <0%> (+3.06%)`	⬆️

ValarDragon · 2018-07-25T20:42:25Z

docs/architecture/adr-012-peer-transport.md

+Responsible for emitting and connecting to Peers. The implementation of `Peer`
+is left to the transport, which implies that the chosen transport dictates the
+characteristics of the implementation handed back to the `Switch`. It is the
+place where we enforce low-level guards, like dropping connections from our own


Do we have a list of low level guards we want to enforce anywhere? Its not clear to me what the scope of "low-level" guards is.

ValarDragon · 2018-07-25T21:02:59Z

docs/architecture/adr-012-peer-transport.md

+// PeerTransport proxies incoming and outgoing peer connections.
+type PeerTransport interface {
+	// Accept returns a newly connected Peer.
+	Accept() (Peer, error)


I'm having trouble understanding Accept. Is Accept supposed to evaluate an incoming connection? If so why doesn't it have connection as a parameter.

It seems to me that this should be doing the handshake and secret connection stuff.

I'm having trouble understanding Accept. Is Accept supposed to evaluate an incoming connection? If so why doesn't it have connection as a parameter.

It has nothing to do with connections as the PeerTransport only hands back peers, which may or may not be based on a tcp connection over the wire. Accept is the way for the caller to signal that they are ready to deal with one more incoming Peer.

It seems to me that this should be doing the handshake and secret connection stuff.

The default implementation it will, other transports may not need to that:

tendermint/p2p/transport.go

Lines 101 to 111 in 791c15b

func (mt *multiplexTransport) Accept(cfg peerConfig) (Peer, error) {

a := <-mt.acceptc

if a.err != nil {

return nil, a.err

}

cfg.mConfig = mt.mConfig

cfg.nodeInfo = mt.nodeInfo

cfg.nodeKey = mt.nodeKey

return upgrade(a.conn, mt.handshakeTimeout, cfg)

Accept is the way for the caller to signal that they are ready to deal with one more incoming Peer.

Ohh I see. Perhaps this may be more clearly named as "GetPeer"?

Trying to avoid getter semantics when possible, in the domain of transport accept and dial are very well understood and the signature already communicates what is returned.

Sounds good! Thank you for the explanations :)

ValarDragon · 2018-07-25T21:09:03Z

docs/architecture/adr-012-peer-transport.md

+Responsible for emitting and connecting to Peers. The implementation of `Peer`
+is left to the transport, which implies that the chosen transport dictates the
+characteristics of the implementation handed back to the `Switch`. It is the
+place where we enforce low-level guards[0]:


Thanks for listing out the intended low-level guards! I still don't really understand what low-level means here.

Are these the set of checks that can only be determined from information in the handshake and the local nodes' configuration? (No knowledge of other peers that this is node is connected to, or how many peers it is currently connected to?)

More concretely, I'm confused about why the following aren't here:

"Ensure this isn't a duplicate connection from the same IP" or "Ensure that we don't already have too many peers"

Very good point. Indeed the use of low-level might be missleading as it actually should say domain-specific. As long as we only handle aspects that belong to the transport domain it should be clear. This might result in that protocol/app version check is down somewhere else. Most certainly the check if we already are connected to too many peers belongs in the switch.

Will add the duplicate ip check and rephrase low-level.

ebuchman · 2018-07-26T01:36:20Z

docs/architecture/adr-012-peer-transport.md

+* connections from our own node
+* handshake fails
+* upgrade to secret connection fails
+* prevent duplicate ip


Should also filter based on:

duplicate ID

node info compatibility

could also clarify that it doesn't handle max-peers, thats for the switch

Will there be another ADR explaining why these are the right choices?

@liamsi Which choices are you referring to?

The choices of what was previously called "guards".

Would be valuable to include some explanation in here as to the "why" for sure

melekes · 2018-07-26T07:51:17Z

docs/architecture/adr-012-peer-transport.md

+	closec  <-chan struct{}
+	listenc <-chan struct{}
+
+	addr             NetAddress


melekes · 2018-07-26T07:51:55Z

docs/architecture/adr-012-peer-transport.md

+type multiplexTransport struct {
+	listener net.Listener
+
+	acceptc chan accept


I prefer acceptCh, but feel free to ignore

If you don't ignore: also, closeCh and listenCh for consistency.

liamsi · 2018-07-26T11:15:13Z

docs/architecture/adr-012-peer-transport.md

+
+One of the more apparent problems with the current architecture in the p2p
+package is that there is no clear separation of concerns between different
+components. Most notably the `Switch` is currently doing physical connection


Can you link to some code (ideally) with a perma-link? I'm not familiar with the current implementation and it makes it easier to read and understand this. e.g. Switch (not only for me but other people reading the ADRs as documentation etc)

While I appreciate this point, we need to strike a balance in these ADRs between being an efficient document for design and including enough information for an outsider to keep up. I think to some extent, readers of the ADR should be expected to have some familiarity with the codebase, so that we don't need to worry too much about adding too many links.

liamsi · 2018-07-26T11:50:17Z

docs/architecture/adr-012-peer-transport.md

+### Positive
+
+* free Switch from transport concerns - simpler implementation
+* pluggable transport implementation - simpler test setup


I know what you mean but could we use another word here? pluggable transports often means sth. else.

What would you prefer?

For me the words pluggable transport is overloaded by what I linked above (means tor obfuscate tor traffic as regular https traffic). But yeah in this context it describes correctly what this is about: the ability to plug in different transport implementations. So that's fine.

liamsi

Given my limited knowledge about the current p2p layer this looks like a thought through and well-written, concise ADR. In general, I'm in favour of writing these with readers in mind that are not knee deep in the current p2p stack (or, more general, whatever code snippets the ADR is addressing). This only means adding a few links or summarizing with very words where more appropriate.

liamsi · 2018-07-26T12:04:18Z

docs/architecture/adr-012-peer-transport.md

+One of the more apparent problems with the current architecture in the p2p
+package is that there is no clear separation of concerns between different
+components. Most notably the `Switch` is currently doing physical connection
+handling. An artifact is the dependency of the Switch on `config.P2PConfig`.


Link would be helpful again.

Link to what in that section would you like to see?

config.P2PConfig

This is the implementation for the design described in ADR 12[0]. It's the first step of a larger refactor of the p2p package as tracked in interface bundling all concerns of low-level connection handling and isolating the rest of peer lifecycle management from the specifics of the low-level internet protocols. Even if the swappable implementation will never be utilised, already the isolation of conn related code in one place will help with the reasoning about execution path and addressation of security sensitive issues surfaced through bounty programs and audits. We deliberately decided to not have Peer filtering and other management in the Transport, its sole responsibility is the translation of connections to Peers, handing those to the caller fully setup. It's the responsibility of the caller to reject those and or keep track. Peer filtering will take place in the Switch and can be inspected in a the following commit. This changeset additionally is an exercise in clean separation of logic and other infrastructural concerns like logging and instrumentation. By leveraging a clean and minimal interface. How this looks can be seen in a follow-up change. Design #2069[2] Refs #2067[3] Fixes #2047[4] Fixes #2046[5] changes: * describe Transport interface * implement new default Transport: MultiplexTransport * test MultiplexTransport with new constraints * implement ConnSet for concurrent management of net.Conn, synchronous to PeerSet * implement and expose duplicate IP filter * implemnt TransportOption for optional parametirisation [0] https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-012-peer-transport.md [1] #2067 [2] #2069 [3] #2067 [4] #2047 [5] #2046

xla added C:p2p Component: P2P pkg T:design Type: Design work is needed labels Jul 25, 2018

xla added this to the launch milestone Jul 25, 2018

xla self-assigned this Jul 25, 2018

xla requested review from ValarDragon, ebuchman, liamsi and melekes July 25, 2018 16:22

xla requested review from jbibla and zramsay as code owners July 25, 2018 16:22

xla changed the title ~~p2p: Propose PeerTransport ADR~~ [ADR] PeerTransport Jul 25, 2018

xla force-pushed the xla/adr-peer-transport branch 2 times, most recently from 21c51e0 to 893573d Compare July 25, 2018 20:14

xla added 2 commits July 25, 2018 22:16

p2p: Propose PeerTransport ADR

893573d

adr: Set status to in review

be0337a

ValarDragon reviewed Jul 25, 2018

View reviewed changes

xla added 2 commits July 25, 2018 22:47

adr: Add high-level decision

b1748be

adr: Extend on the idea of guards

f52a1dd

ValarDragon reviewed Jul 25, 2018

View reviewed changes

adr: Rework guards into transport specific filters

4a77b56

ebuchman approved these changes Jul 26, 2018

View reviewed changes

melekes approved these changes Jul 26, 2018

View reviewed changes

liamsi reviewed Jul 26, 2018

View reviewed changes

liamsi approved these changes Jul 26, 2018

View reviewed changes

liamsi reviewed Jul 26, 2018

View reviewed changes

xla added 2 commits July 26, 2018 17:10

adr: Rename to nodeAddr

d65c204

adr: Incorporate review

6f6d0cc

xla merged commit fd29fd6 into develop Jul 27, 2018

xla deleted the xla/adr-peer-transport branch July 27, 2018 00:27

xla mentioned this pull request Sep 6, 2018

PeerTransport #2126

Merged

xla restored the xla/adr-peer-transport branch September 18, 2018 20:17

xla deleted the xla/adr-peer-transport branch September 18, 2018 20:27

	func (mt *multiplexTransport) Accept(cfg peerConfig) (Peer, error) {
	a := <-mt.acceptc
	if a.err != nil {
	return nil, a.err
	}

	cfg.mConfig = mt.mConfig
	cfg.nodeInfo = mt.nodeInfo
	cfg.nodeKey = mt.nodeKey

	return upgrade(a.conn, mt.handshakeTimeout, cfg)

Conversation

xla commented Jul 25, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValarDragon Jul 25, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi Jul 26, 2018 • 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!

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!

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!

6 participants

codecov-io commented Jul 25, 2018 •

edited

Loading

ValarDragon Jul 25, 2018 •

edited

Loading

liamsi Jul 26, 2018 •

edited

Loading

liamsi left a comment •

edited

Loading