search

multiformats / multiaddr

Composable and future-proof network addresses
https://multiformats.io/multiaddr
MIT License
419 stars 84 forks source link

feat: add webrtc-w3c protocol #150

Closed ckousik closed 1 year ago

ckousik commented 1 year ago

Summary

Adds support for the webrtc-w3c protocol described by the following spec: https://github.com/libp2p/specs/pull/497

https://github.com/multiformats/multicodec/pull/316

mxinden commented 1 year ago

https://github.com/multiformats/multicodec/pull/316 is merged. I re-triggered CI which is green now.

I remember having a discussion around keeping pull requests open for a certain amount of time. In the back of my head, that time frame was 48h. Thus I am merging here. Please comment in case we settled on a different timeline.

marten-seemann commented 1 year ago

Sorry for engaging here late. Please request a review from me in the future for faster responses.

I'm strongly opposed to call this webrtc-w3c. It confuses the protocol with one API that makes use of that protocol. The browser API is specified by the W3C. Two reasons not to call this webrtc-w3c:

  1. The libp2p private-to-private WebRTC transport is not limited to browsers. It can also be used for browser to private nodes. While the browser would use the W3C-defined API, the private node wouldn't. It would use whatever API its WebRTC implementation offers.
  2. In the browser-to-server case, the browser would also use the W3C-defined API. Simply because that's the API that browsers offer.
mxinden commented 1 year ago

The libp2p private-to-private WebRTC transport is not limited to browsers. It can also be used for browser to private nodes. While the browser would use the W3C-defined API, the private node wouldn't. It would use whatever API its WebRTC implementation offers.

Arguments speaking for the name:

While non-browser nodes might not use the W3C API, they would still be limited by the above mentioned constraints and would have to adhere to the protocol flow dictated by the W3C specification.

In the browser-to-server case, the browser would also use the W3C-defined API. Simply because that's the API that browsers offer.

In the browser-to-server specification, the browser does not correctly follow the W3C specification. E.g. it does SDP munging, it misuses the ufrag, ...

I am still in favor of the name webrtc-w3c, that said, I don't feel strongly about it. I mostly just care about unblocking https://github.com/libp2p/js-libp2p-webrtc/pull/90.

@marten-seemann with the above arguments, do you still feel strongly about not naming it webrtc-w3c? If so, can you please suggest an alternative name?

Menduist commented 1 year ago

Re-offering webrtc-holepunching or webrtc-nat-traversal or similar

p-shahi commented 1 year ago

Is it possible to rename webrtc to webrtc-direct, and make this webrtc? Or would that be too disruptive to Parity? (not sure if there are any other projects using this transport at scale).

Otherwise, I would suggest webrtc-relay (since it makes use of a relay) or webrtc-private(since it deals with connecting private nodes).

Re-offering webrtc-holepunching or webrtc-nat-traversal or similar

webrtc-hp or webrtc-nt to save a few characters gmv

marten-seemann commented 1 year ago
  • The protocol is designed around the constraints that the W3C specification gives us (e.g. not being able to reuse a UDP port thus requiring STUN, no access to the underlying SCTP connection, thus requiring additional multiplexing on top, ...)

That applies to browser-to-server as well. Necessarily. The W3C API is the only one that's available in the browser.

We follow the W3C protocol flow, e.g. SDP exchange initiated by A, received by B. Additional STUN exchange between A and B, ...

All of this is specified by the IETF in a large number of RFCs.

Re-offering webrtc-holepunching or webrtc-nat-traversal or similar

That's a quite unintuitive naming. Of course the WebRTC contains holepunching and NAT traversal! That's an essential part of the WebRTC protocol. It's confusing to point it out explicitly. And it seems even more confusing to name something "webrtc" that doesn't contain these features.

Is it possible to rename webrtc to webrtc-direct, and make this webrtc?

I'd be favor of this approach. It's the right thing to do, and the only reason we didn't end up with this naming is that we specified browser-to-server first (arguably, we should have done it the other way around).

In general, I'd argue that we should avoid breaking changes when it comes to multiaddr components as much as possible, but I think it's justified in this case as:

I'd also like to point out that we're only talking about the string representation. On the wire, multiaddresses are exchanged in their byte representation, which would be unaffected by the suggested renaming.

MarcoPolo commented 1 year ago

I would not want to rename the existing code point. Folks are already using it. I don't want to push a breaking change.

I personally don't care which exact name we choose here. I think having this as a multiaddr is technically wrong because it's not a direction on how to reach a node, it's a capability the node supports. It works over any existing connection (see my arguments here). Having this as part of the multiaddr is a stop-gap temporary hack until we have a better way of encoding a Peer's multiaddr's and their capabilities.

I do care that we resolve this quickly so things can continue to progress and we don't spend time having to update names all over the place. To that end I'll suggest webrtc-stun. It's webrtc, but this one needs a STUN server. No preference for this particular name as opposed to anything else (besides webrtc).

p-shahi commented 1 year ago

So far it seems the name will be changed. The current proposals are

For the last one, should this group outright decide against the breaking change or can we ask interested parties from Parity? If they dissent, that's a stronger case against the last option. Again not sure whether there are other power users but if we do the breaking change, might as well do it now before it is truly widely deployed cc: @melekes

marten-seemann commented 1 year ago

While I like webrtc-stun the best out of these options, the objection would be the same as for webrtc-holepunching and webrtc-nat-traversal. Quoting myself:

Of course the WebRTC contains holepunching and NAT traversal! That's an essential part of the WebRTC protocol. It's confusing to point it out explicitly. And it seems even more confusing to name something "webrtc" that doesn't contain these features.

For the last one, should this group outright decide against the breaking change or can we ask interested parties from Parity?

I could understand if they'll push back against a renaming, and I truly feel sorry for suggesting to create extra work for them with this renaming. This is not definitely not ideal. However, there's a probably much larger group of future developers who won't be able to chime in on this issue, who we can avoid to confuse by picking names that actually make sense. We need to account for that in some way as well.

thomaseizinger commented 1 year ago

I'd like to make another case for /webrtc-w3c or at least a name that is inspired by the same thinking.

It confuses the protocol with one API that makes use of that protocol.

This isn't a confusion at all but actually by design. The specification mentions the RTCPeerConnection object 10 times! The entire specification is literally designed around the constraints of the W3C API. Why should we not couple the naming of our protocol to that fact?

Yes we can maybe find a more general name but why bother? The name isn't wrong per se, is it? Also, the one API you are mentioning happens to be implemented by all browsers which is the main platform we want to target with this.

If tomorrow a new platform is released that also offers WebRTC capabilities but uses a different API, we'd have to write a new spec anyway because the current one only mentions the RTCPeerConnection API.

mxinden commented 1 year ago

Summary of a synchronous call with @marten-seemann and @p-shahi.

Individual preference

For the record, @marten-seemann is still in favor of the rename of /webrtc (browser-to-server) to /webrtc-direct and /webrtc-w3c (browser-to-browser) to /webrtc.

I am against the rename as I don't think it is worth the breaking change. Otherwise I don't have a strong opinion on the protocol name.

@p-shahi, as mentioned above, gave great input from a user perspective where /webrtc-w3c is not intuitive for newcomers.

Consensus in the meeting

After some discussion we reached consensus on the below within the participants of the meeting:

Steps forward

I am under the impression that neither of the others involved in this discussion (@ckousik @MarcoPolo @Menduist @thomaseizinger) feel very strongly about the naming. In case you do and want to block the above suggestions, please speak up.

Unless there are any objections within the next 24h, we will go ahead with the above renames.

Details for the avid reader

thomaseizinger commented 1 year ago

Not a blocking opinion but I think it is not a very descriptive name.

Do we have a definition somewhere of what a "private" node is? I've not come across this term other than in the context of pnet.

p-shahi commented 1 year ago

Not a blocking opinion but I think it is not a very descriptive name.

Do we have a definition somewhere of what a "private" node is? I've not come across this term other than in the context of pnet.

In the connectivity site's hole punching section we write

however, not all nodes are located in publicly reachable positions.

Nodes in home or corporate networks are private and usually separated from the public internet by a network address translation (NAT) mapping or a firewall. Mobile phones are also usually behind a so-called "carrier-grade NATs".

These private nodes behind firewalls/NATs can dial any node on the public internet, but they cannot receive incoming connections from outside their local network.

It might be a good idea to lift this definition into a glossary in the specs and docs site

melekes commented 1 year ago

@tomaka was in favor of /webrtc-direct here https://github.com/multiformats/multiaddr/pull/130#issuecomment-1186540999. I personally don't care that much about naming. If you decide to change /webrtc to /webrtc-direct, better to do it right now (i.e. release another libp2p version this/next week) before we've rolled out https://github.com/paritytech/substrate/pull/12529. Our light client will need to be updated as well.

mxinden commented 1 year ago

Unless there are any objections within the next 24h, we will go ahead with the above renames.

No blocking objections in the past 24h. Interpreting this as consensus.

Pull requests updating the multiaddr name to webrtc-private-to-private:

Approvals from @marten-seemann @MarcoPolo and @achingbrain very much appreciated :pray:

achingbrain commented 1 year ago

Would it be fair to say that what we're calling /webrtc-private-to-private, if we can't call it /webrtc-w3c (which, on balance, is not a completely terrible name since it uses the w3c flow and the spec mentions the w3c APIs specifically) is really something like /webrtc+w3c or /webrtc+w3c-flow or something, using a + to differentiate between the transport and our connection protocol we place on top of it, and what we're calling /webrtc is really /webrtc+sdp-munging?

mxinden commented 1 year ago

Status update:

@achingbrain put in a veto for the above /webrtc-private-to-private multiaddr protocol name. See https://github.com/multiformats/multiaddr/pull/151#pullrequestreview-1339048957 for further information.

I drove consensus for the multiaddr protocol name /webrtc-direct, /webrtc-w3c and /webrtc-private-to-private, each with a public call for input, quorum and the last two with a majority quorum (if one includes lazy consensus with request for feedback over multiple days/weeks). I did not initially suggest any of these names, nor do I feel strongly about any of them. At this point I don't think further discussion on a multiaddr protocol name for the browser-to-browser and browser-to-server protocol is worth our time. Thus I am not going to further engage in the naming discussion.

In our sync with Little Bear Labs today @achingbrain and I agreed on the following:

My intention is not to blame anyone, nor to express any anger :heart:, naming is hard. As said above, I don't think there is value in further discussing these names, thus I am not going to further engage. I am fine with any name for the browser-to-server and browser-to-browser protocol.

p-shahi commented 1 year ago

>i.e. /webrtc for the brower-to-server protocol and /webrtc+sdp-munging for the browser-to-browser protocol.

Minor correction: The proposal is /webrtc for browser-to-browser and /webrtc+sdp-munging for browser-to-server

mxinden: Updated above.

achingbrain commented 1 year ago

Massive ❤️s to @mxinden, it takes a lot of effort to do this sort of thing.

We discussed renaming on a call:

Everyone seems to agree with /webrtc-w3c -> /webrtc for the browser to browser case, so far, so good.

/webrtc+sdp-munging got a bit of a lukewarm reception and for good reason. It's a weird set of words, it's not consistent with other multicodec names and lacks universal understanding.

This option is in this PR: https://github.com/multiformats/multiaddr/pull/153

The other option is:

This option was discussed above and had decent support though at the time in the conversation we were reluctant to make a breaking change. This has shifted somewhat so /webrtc-direct is presented as an option here: https://github.com/multiformats/multiaddr/pull/152

Please approve the PR with your preference. We will leave them open for a few days then merge the one with the most approvals. In the event of a tie we'll go with #152 as it seemed to have the most support originally.

achingbrain commented 1 year ago

@melekes @tomaka @thomaseizinger I cannot tag you for review in #152 or #153 so please just 👍 the option you prefer.

achingbrain commented 1 year ago

Time has passed and #152 has the most support so we'll go with that.

Thanks everyone!