rfc: add P2P stream proposal #227
Conversation
erikgrinaker
requested review from
ebuchman,
josef-widder,
konnov and
milosevic
as code owners
erikgrinaker
force-pushed
the
erik/rfc-p2p-streams
branch
from
2fd88ff
to
570266b
Compare
There was a problem hiding this comment.
This looks like sound reasoning to me.
I don't know the space too well. Are there any single streamed transports we would want to consider adopting. It seems relatively incongruent with the current format of reactors to not have a transport that more natively supports multiple streams (without HOL blocking and so forth)
| } | ||
| ``` | ||
|
|
||
| P2P reactor messages should instead be framed by the message sender, using length-prefixed encoding (the de-facto standard for Protobuf). This allows using e.g. a standard `protoio.Reader` and `protoio.Writer` to automate framing. An additional benefit is that the length of a message is known prior to receiving it, such that an appropriately sized buffer can be initialized immediately. |
There was a problem hiding this comment.
This allows using e.g. a standard
protoio.Readerandprotoio.Writerto automate framing.
This is a very important detail. Might want to add a sentence or two on how this happens.
|
Following the developments of ADR-062 and working on the new p2p stack in tendermint-rs, I'm surprised to see this proposal. While stream based transports are on the horizon, it seems odd that it would necessitate retrofitting as it indicates a leak of abstraction at some point in the stack. Unless this is in preparation for the Router to be generic over the transport plugged. Yet if that is the desired outcome, a breaking protocol change for that internal convenience seems induldgent. If it is the declared outcome indeed, a less intrusive approach is to wrap the existing MConn with some thing that quacks and walks like a QUIC. And there is a lot of mention of quic and its streams, these are byte streams which is not a sensible interface to expose up to reactors, which only depend on PeerID (origin) and message (payload). So the Router should really guard against the transport details. I refer to the Router here as it is the entity in the ADR-062 diagrams. Which also might be the angle (the layer where the Router sits) where innovation has impact as it would determine how ready the codebase is to be migrated onto something like QUIC. It is mentioned that streams can be set up in an ad-hoc way. Is that something that has been decided/settled on? It seems like a protoocl violation if a peer is trying to initiate communication for a capability not advertised. It feels worthy to flag that this is not just a decision that should be made based on technical merit. Lastly is it anticipated to solve any of the shortcomings with package transmission/delivery of the current implementation? Which would seem like an attempt to reimplement something like gRPC. |
The router, or rather the peer connection, will be "generic" over the transport - the router can communicate with different peers using different transports. We therefore need a transport abstraction. The proposal in ADR-062 is for that abstraction to be byte streams (plural), which is the de-facto low-level networking abstraction across most languages (although normally singular). The main proposal here is to change MConnection from a message-oriented multi-stream transport to a byte-oriented multi-stream transport, which is the same model that QUIC uses.
Yes, that is option 2: writing a shim around MConnection that takes reassembled messages and passes them through the P2P transport API as byte streams using length-prefix framing. This has increased resource and complexity costs, so I think this is best suited as a temporary measure as long as we are fairly confident that we want to migrate to QUIC, but it is a viable option.
Indeed. Reactors will exchange Protobuf messages across Go channels, using peer ID addressing. Only the router cares about low-level network transports, and it is its job to serialize messages, frame them, and pass them over the appropriate byte stream.
No, we could do all stream setup during the initial transport handshake, and use QUIC-specific stream handshakes. But explicitly opening and closing channels allows them (and thus reactors) to be dynamic, and I think this is an improvement over the current state where we announce that we have some capability (e.g. consensus) but then just ignore received messages when necessary (e.g. during fast sync). This is somewhat orthogonal though, and we can defer this discussion until later without any real impact on the P2P refactor.
I'm not sure if I understand the question. Internal message scheduling and prioritization will be revamped as part of the router, e.g. to provide fair queueing of messages received from different peers and prioritize certain reactors over others. Apart from this, the MConnection transport characteristics will largely remain unchanged. Could you elaborate on the gRPC comparison? |
We do sort of use a single-streamed transport currently, in the sense that TCP is single-streamed and MConnection multiplexes multiple logical streams onto a single TCP stream. UNIX sockets are another example. Most transport protocols, and thus must networking APIs, have traditionally been single-streamed, which is why it's slightly painful to introduce something like QUIC that breaks this convention by using multiple streams. |
I think we're basically dealing with an abstraction mismatch here, between the abstractions we already have and the abstractions we want to have. The mconnection protocol necessitates that multiplexing happen within the "application" abstraction, while something like QUIC expects multiplexing to happen within the "transport" abstraction. There had been a lot of enthusiasm for a "transport-agnostic" P2P layer, since it seemed like that meant that we could move all the internals around while leaving the protocol in place. This also meant that we could postpone any decisions about what the transport layers should actually be. But I think we've realized that the preexisting mconnection protocol is not compatible with the concept of "transport," at least if transports behave like QUIC, since both QUIC and mconnection expect to handle multiplexing. It sounds like, as an alternative to this RFC, mconnection and QUIC could be shoehorned together. But this would be at the cost of a lot of complexity (and also likely a performance hit), and that would be a little bit of a shame since the whole point of this P2P refactor is to reduce complexity. All of that said, if we don't expect to adopt QUIC (or another multi-stream transport), this whole thing is moot. Perhaps we should begin by trying to build confidence around that decision. |
|
To unblock the P2P refactor I'm going to implement a message-oriented transport API as a temporary solution, and we can revisit once we get alignment on a long-term plan. |
|
My understanding was that we would add support for QUIC as an alternative to MConnection, stabilize it over a release or two, and then remove the MConnection. So I'm not sure it makes sense to be updating the MConnection if we don't have to, unless for some reason we're trying to keep it around. As for how to support both in the meantime, the reactors currently use Is that what Option 2 is proposing? I didn't get that from Maybe the problem is that this works if all we're trying to do is add QUIC, but if we want to more generally refactor for transport agnosticism and change the API exposed to the reactors, we'd need to do more work on or around the MConnection, and thus we'd be otherwise blocked on further refactor until MConnection was fully removed? Is that all basically right? |
|
Yeah, that's basically right. Taking full advantage of QUIC requires a somewhat different processing/scheduling model in the P2P stack (basically one goroutine per stream rather than one goroutine per peer), and the most natural way to do this is to expose streams as a first-class transport concept. If we were to implement that, it would make sense to use similar models for QUIC and MConnection streams (i.e. byte streams instead of message streams), such that we could unify higher-level protocol concerns such as framing and handshakes across all transports. There's also the question of whether we implement this now, or defer until we build the QUIC transport.
No, that is sort of the opposite. Option 2 instead proposes to implement the full stream-based transport API with per-stream goroutines, and shoehorn MConnection into this by basically rewriting the transport protocol on-the-fly (which is about as unsatisfying as it sounds). I'm going to do basically what you outline for now: implement a higher-level |
This implements a new `Transport` interface and related types for the P2P refactor in #5670. Previously, `conn.MConnection` was very tightly coupled to the `Peer` implementation -- in order to allow alternative non-multiplexed transports (e.g. QUIC), MConnection has now been moved below the `Transport` interface, as `MConnTransport`, and decoupled from the peer. Since the `p2p` package is not covered by our Go API stability, this is not considered a breaking change, and not listed in the changelog. The initial approach was to implement the new interface in its final form (which also involved possible protocol changes, see tendermint/spec#227). However, it turned out that this would require a large amount of changes to existing P2P code because of the previous tight coupling between `Peer` and `MConnection` and the reliance on subtleties in the MConnection behavior. Instead, I have broadened the `Transport` interface to expose much of the existing MConnection interface, preserved much of the existing MConnection logic and behavior in the transport implementation, and tried to make as few changes to the rest of the P2P stack as possible. We will instead reduce this interface gradually as we refactor other parts of the P2P stack. The low-level transport code and protocol (e.g. MConnection, SecretConnection and so on) has not been significantly changed, and refactoring this is not a priority until we come up with a plan for QUIC adoption, as we may end up discarding the MConnection code entirely. There are no tests of the new `MConnTransport`, as this code is likely to evolve as we proceed with the P2P refactor, but tests should be added before a final release. The E2E tests are sufficient for basic validation in the meanwhile.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing this, will be superseded once QUIC work begins. |
Rendered
Proposes two changes to the MConnection protocol to better accomodate multi-stream protocols such as QUIC.