marten-seemann commented 4 years ago

I tried to condense everything down to make the new protocol as simple as possible, while providing an extension point that will allow us to deploy all of the optimizations that people have already suggested in the future.

The document here describes only the stream-based use case, and doesn't cover the packet-based use case yet. I believe that we can use a very similar format for unreliable connections, but there seem to be some subtle differences that would prevent us from reusing the exact wire format of the Use and Offer message defined in this document:

Offer can offer multiple protocols. This makes sense when establishing communication on a reliable channel, but it doesn't really work out in an unreliable protocol. For packet-based protocols, it probably makes sense to restrict Offer such that it's only possible to offer a single protocol.
Use uses a oneof of name and id. This works well when a stream correlates the Use to an Offer sent before, but isn't enough in the packet-based use case, where you might have multiple Offers in flight. Here, it would make sense to define a Use message such that it includes both a name and an id.
A packet-based protocol needs a Reject message to communicate that a certain protocol is not supported.

vyzo commented 4 years ago

Note that multiselect-1.0 can handle simultaneous open with a simple protocol extension; see #196.

marten-seemann commented 4 years ago

@vyzo If I understand #196 correctly, it works by adding (yet) one more round trip to multistream/1.0. I'd like to avoid adding round trips to multiselect 2.0 in the common case (where no simultaneous open is needed).

What do you think of this strawman proposal: If a handshake fails due to an unexpected message error (e.g. in the case of TLS, when the client receives a ClientHello in response to a ClientHello), both peers clear their handshake state and start over again. The roles are determined by the following rule: Both peers calculate the hash of the message(s) they sent and they received during the last handshake attempt. The peer that sent the message that hashes to the smaller value will then act as a client, the other one as the server in the subsequent handshake.

A protocol along those lines would make sure that peers that don't use simultaneous connect don't suffer any round-trip penalty, while peers that do simultaneous connect would pay the cost of a single additional round-trip.

vyzo commented 4 years ago

196 does not add a round-trip to the protocol; it is pipelined together with the protocol selection.

marten-seemann commented 4 years ago

196 does not add a round-trip to the protocol; it is pipelined together with the protocol selection.

@vyzo The crypto protocol selection, which we're getting rid of here. This negotiation made (makes) us vulnerable to a trivial MITM attack, which was one the motivations to develop a new version of this protocol that fixes this vulnerability (not to mention that removing it also speeds up the handshake by one round-trip). So we clearly need something different than #196 for this new protocol. I'd really like to hear what you think of https://github.com/libp2p/specs/pull/227#issuecomment-549913845. Is that something we can build on to make it a workable specification for simultaneous open?

vyzo commented 4 years ago

@marten-seemann the strawman proposal is a hack, the TLS handshake can fail for all sorts of reasons, and the "unexpected message error" is not necessarily indicative of an unexpected "ClientHello".

Why can't we add an iamclient bit to the opening handshake? If both parties set it, then they enter an initiator selection round, similar to #196 and after deciding who is the initiator they can proceed with TLS.

marten-seemann commented 4 years ago

@vyzo The problem with a iamclient message is that it's sent in cleartext, before the handshake even started, and it would make libp2p traffic as easily identifiable as it is today, which was a non-goal in the design of the multiselect 2 (see #205):

Current libp2p traffic can be blocked by using deep packet inspection. Handshakes should be indistinguishable from ordinary HTTP/2 (in the case of TCP) and HTTP/3 (in the case of QUIC) traffic, or other popular Internet applications.

Regarding the TLS handshake,

the strawman proposal is a hack, the TLS handshake can fail for all sorts of reasons, and the "unexpected message error" is not necessarily indicative of an unexpected "ClientHello".

I wouldn't use a loaded work like "hack" for this. It's a way to solve the problem that seems consistent with our design goals. You could easily wrap a TLS connection and parse the first byte of the first incoming message, if you're uncomfortable with relying of the error message returned by the TLS stack. That would tell you if the message you received was a ClientHello or a ServerHello.

marten-seemann commented 4 years ago

I just rewrote the Simultaneous Open section and specified how the hashing of handshake messages works to determine the roles in a subsequent handshake attempt. PTAL.

raulk commented 4 years ago

I have not had time to review all the commentary here and the resulting changes. We should move with this rapidly, knowing that this spec is going to continue change. I'm happy to lock in a baseline so we can start a PoC, but not before we see the following. If these are implemented/rejected, please synthesise and explain (reiterating I have no time to go through the commentary and doc, but I have strong ideas):

Sharing session capabilities during connection bootstrapping. I'd like peers to advertise things like compressors, erasure coding, etc. protocols, that can apply to an entire connection or to individual streams.
- IMO this addresses the rationale behind @Stebalien's initial recursive/tunneling proposal.
If peer A has learnt and memorised protocol mappings of peer B in the peerstore, the next time A and B connect, they should be able to verify that the same protocol table still stands. We had discussed incorporating some opaque ID (that is fundamentally different to session resumption, which may or may not be supported by secure channels).
Not sure if the XOR pathway has been implemented ("I want to open a stream for either protocol Xv1, or Xv2, or Xv3, each accompanied with their initial message; it should be possible to have a single message for multiple protocol proposals; otherwise we'd be wasteful in some scenarios").

marten-seemann commented 4 years ago

@raulk

Sharing session capabilities during connection bootstrapping. I'd like peers to advertise things like compressors, erasure coding, etc. protocols, that can apply to an entire connection or to individual streams.

Compressors came up on #230. It was not part of our requirement document, and to be honest, I don't really understand the use case well enough to even write down the requirements for this, let alone a specification.

If peer A has learnt and memorised protocol mappings of peer B in the peerstore, the next time A and B connect, they should be able to verify that the same protocol table still stands. We had discussed incorporating some opaque ID (that is fundamentally different to session resumption, which may or may not be supported by secure channels).

We discussed this, and decided to remove this from the specification. The reason is that the cost of sending the string for one round-trip (and learning a new ID for this protocol) is minor, and doesn't justify the cost of indexing a mapping and verifying that this index is still valid.

Not sure if the XOR pathway has been implemented ("I want to open a stream for either protocol Xv1, or Xv2, or Xv3, each accompanied with their initial message; it should be possible to have a single message for multiple protocol proposals; otherwise we'd be wasteful in some scenarios").

It's possible to Offer multiple protocols in the same message, but it's not possible to send application data for each protocol. Therefore, it will take a single round-trip (once per connection) to learn which protocol version the peer supports. This leads to a much simpler wire format of ms2.0.

raulk commented 4 years ago

We discussed this, and decided to remove this from the specification. The reason is that the cost of sending the string for one round-trip (and learning a new ID for this protocol) is minor, and doesn't justify the cost of indexing a mapping and verifying that this index is still valid.

I disagree here. This assertion can be sent in early data, therefore amortised in terms of segmentation. A normal IPFS peer will need to agree on identify, dht, pubsub, bitswap, etc. Imagine the network is spotty and this peer keeps connecting and reconnecting frequently. Why would we want to renegotiate protocol indices every time, when we can exchange opaque table IDs upfront and just assert that we both maintain the same table?

It's possible to Offer multiple protocols in the same message, but it's not possible to send application data for each protocol. Therefore, it will take a single round-trip (once per connection) to learn which protocol version the peer supports. This leads to a much simpler wire format of ms2.0.

This was captured in the design doc. "A simpler wire format" is not a convincing answer for a feature that's required; of course things are going to be simple if you remove features.

Aren't we talking about an optional bytes field? I don't understand where the complication is.

raulk commented 4 years ago

Compressors came up on #230.

Well, you merged that PR without consensus. And now we're having the discussion again, which was predictable ;-)

It's a mistake to make the early data a multiselect 2.0 message. Early data forms part of connection bootstrapping, which is really a process that's decoupled from ms2.0 -- that's the way we're designing it.

Therefore, connection bootstrapping needs its own specific payload, which IMO should include:

enum CapabilityType {
  MULTIPLEXER
  COMPRESSOR
  ERASURE_CODER
}

enum Scope {
  CONN
  STREAM
  BOTH
}

message SessionCapGroup {
  CapabilityType type = 1;
  repeated string supported = 2;
  repeated Scope scopes = 3;
}

message ConnectionBootstrap {
  fixed32 protocol_table_id = 1;   // an opaque string that identifies the version of the protocol table we are using.
  repeated SessionCapGroup session_capabilities = 2;
}

marten-seemann commented 4 years ago

I disagree here. This assertion can be sent in early data, therefore amortised in terms of segmentation. A normal IPFS peer will need to agree on identify, dht, pubsub, bitswap, etc. Imagine the network is spotty and this peer keeps connecting and reconnecting frequently. Why would we want to renegotiate protocol indices every time, when we can exchange opaque table IDs upfront and just assert that we both maintain the same table?

Because the overhead of it is a few bytes, for a single roundtrip. I suggest we first show that this is actually a problem before designing a complicated solution. Synchronizing state across connections is not trivial, and will add significant complexity, both in terms of specification as well as implementation. If we can show that this complexity is actually justified by real-world benefits, the protobuf-based format of ms2.0 will allow us to define a new message, which could (for example) announce the ID(s) of a remembered mapping. Peers that are still on the old ms2.0 will ignore this message, and peers that already support this update can then send a confirmation protobuf. This is a very smooth upgrade path.

It's possible to Offer multiple protocols in the same message, but it's not possible to send application data for each protocol. Therefore, it will take a single round-trip (once per connection) to learn which protocol version the peer supports. This leads to a much simpler wire format of ms2.0.

This was captured in the design doc. "A simpler wire format" is not a convincing answer for a feature that's required; of course things are going to be simple if you remove features.

It was marked "tentative" in the design doc, so it's hard to argue that this is a feature that was a hard requirement in the protocol design. The only comment I received about this tentative point was that it would be complex. Considering that we don't have the concept of protocol upgrades at all in multistream 1 at all, I'm not sure how we know that improving the current situation to a one round-trip per connection wait for protocol upgrades would be prohibitively expensive.

Aren't we talking about an optional bytes field? I don't understand where the complication is.

I don't think it's that easy. First of all, it breaks with the concept of ms2.0 to use protobufs for protocol messages, but not application data. Furthermore, it's unclear how this interacts with stream flow control. We might also want to think about an API that would generate this kind of data first.

Compressors came up on #230.

Well, you merged that PR without consensus. And now we're having the discussion again, which was predictable ;-)

I merged #230 into this PR, not into master. Consensus would have been nice, but difficult to achieve if so few people actually review the PR. Compressors were unrelated to #230 anyway, so this is the better place to discuss this anyway. Or at least as far as I understand them. This is a totally new requirement to me, I don't know how they're supposed to work, so I have no idea how I should argue about a specific wire format at this point.

libp2p / specs

add a multiselect 2.0 spec #227

196 does not add a round-trip to the protocol; it is pipelined together with the protocol selection.

196 does not add a round-trip to the protocol; it is pipelined together with the protocol selection.