Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add webrtc-w3c protocol #150

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

ckousik
Copy link
Contributor

@ckousik ckousik commented Mar 1, 2023

Summary

Adds support for the webrtc-w3c protocol described by the following spec: libp2p/specs#497

multiformats/multicodec#316

Adds support for the webrtc-w3c protocol described by the following
spec: libp2p/specs#497
@ckousik ckousik marked this pull request as ready for review March 1, 2023 14:53
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Here as well, will give others time to comment before merging.

@mxinden
Copy link
Member

mxinden commented Mar 3, 2023

multiformats/multicodec#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.

@mxinden mxinden merged commit 29eb360 into multiformats:master Mar 3, 2023
@marten-seemann
Copy link
Contributor

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
Copy link
Member

mxinden commented Mar 7, 2023

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:

  • 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, ...)
  • We follow the W3C protocol flow, e.g. SDP exchange initiated by A, received by B. Additional STUN exchange between A and B, ...

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 libp2p/js-libp2p-webrtc#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
Copy link

Menduist commented Mar 7, 2023

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

@p-shahi
Copy link
Contributor

p-shahi commented Mar 7, 2023

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
Copy link
Contributor

  • 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:

  • We can't come up with a with a good alternative.
  • This was so far only released as an alpha in rust-libp2p. There's always the possibility of potentially breaking changes when using alpha code.

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
Copy link
Contributor

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
Copy link
Contributor

p-shahi commented Mar 8, 2023

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

  • webrtc-holepunching (or perhaps webrtc-hp) - has one dissenting opinion
  • webrtc-nat-traversal (or perhaps webrtc-nt) - has one dissenting opinion
  • webrtc-stun
  • webrtc-relay
  • webrtc-private
  • webrtc (and rename browser-to-server to webrtc-direct) - one supporting opinion, one dissenting

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
Copy link
Contributor

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
Copy link
Contributor

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

  • /webrtc is more general than /webrtc-somequalifier, thus I'd expect /webrtc to be more capable. At the moment this is the case which is why I'd argue against the rename of /webrtc. Also, we should not make breaking changes like this to deployed software.
  • The dialing side does not matter in either case, both protocols are designed around the constraints of the listening side. The original suggestion was inspired by the fact the the listening side is constrained to the W3C API. In /webrtc, the listening side is not constrained in its API and can thus perform SDP-munging and other things.

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
Copy link
Member

mxinden commented Mar 8, 2023

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:

  • No rename for the browser-to-server /webrtc multiaddr component.
  • Specification name of the browser-to-browser specification and protocol: WebRTC private-to-private
  • multiaddr protocol component to be appended to the relayed address: /webrtc-private-to-private
  • multistream select protocol identifier for the signaling protocol: /webrtc-signaling

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

  • Isn't /webrtc-private-to-private too long, i.e. takes too much space? This is a multiaddr protocol component. On the wire it is represented in its binary representation (281) and not the string representation.
  • Why /webrtc-signaling and not /webrtc-private-to-private-signaling? This is a multistream select protocol. Multistream select does not have a binary representation. While the longer string is likely not an issue on the wire during negotiation, it does bloat the identify payload in the supported protocols protobuf field (string representation, not binary).
  • Why not /webrtc-p2p? ... 😛 can stand for either private-to-private, peer-to-peer or public-to-public.

@thomaseizinger
Copy link
Contributor

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
Copy link
Contributor

p-shahi commented Mar 8, 2023

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
Copy link

melekes commented Mar 9, 2023

@tomaka was in favor of /webrtc-direct here #130 (comment). 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 paritytech/substrate#12529. Our light client will need to be updated as well.

mxinden added a commit to mxinden/multicodec that referenced this pull request Mar 9, 2023
We have decided to rename `webrtc-w3c` to `webrtc-private-to-private`. See
rational in multiformats/multiaddr#150.

No software has been released using `webrtc-w3c`, thus renaming is not a
breaking change.
mxinden added a commit to mxinden/multiaddr that referenced this pull request Mar 9, 2023
We have decided to rename `webrtc-w3c` to `webrtc-private-to-private`. See
rational in multiformats#150.

No software has been released using `webrtc-w3c`, thus renaming is not a
breaking change.
mxinden added a commit to mxinden/js-multiaddr that referenced this pull request Mar 9, 2023
We have decided to rename `webrtc-w3c` to `webrtc-private-to-private`. See
rational in multiformats/multiaddr#150.

No software has been released using `webrtc-w3c`, thus renaming is not a
breaking change. Ignoring the automated `multiaddr` `v11.5.0` release.

See also multiformats#309 that introduced `webrtc-w3c`.
@mxinden
Copy link
Member

mxinden commented Mar 9, 2023

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 🙏

mxinden added a commit to mxinden/multicodec that referenced this pull request Mar 17, 2023
mxinden added a commit to mxinden/rust-multiaddr that referenced this pull request Mar 17, 2023
mxinden added a commit to mxinden/rust-multiaddr that referenced this pull request Mar 17, 2023
mxinden added a commit to mxinden/rust-multiaddr that referenced this pull request Mar 17, 2023
mxinden added a commit to multiformats/multicodec that referenced this pull request Mar 17, 2023
mxinden added a commit to mxinden/rust-multiaddr that referenced this pull request Mar 17, 2023
mxinden added a commit to mxinden/rust-multiaddr that referenced this pull request Mar 22, 2023
Considered non-breaking change as `/webrtc` will still be parsed to
`Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known.

See multiformats/multiaddr#150 (comment)
for context.
mxinden added a commit to multiformats/rust-multiaddr that referenced this pull request Mar 23, 2023
…t` (#84)

Considered non-breaking change as `/webrtc` will still be parsed to
`Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known.

See multiformats/multiaddr#150 (comment)
for context.
mxinden added a commit to mxinden/rust-multiaddr that referenced this pull request Mar 23, 2023
…t` (multiformats#84)

Considered non-breaking change as `/webrtc` will still be parsed to
`Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known.

See multiformats/multiaddr#150 (comment)
for context.
mxinden added a commit to multiformats/rust-multiaddr that referenced this pull request Mar 23, 2023
…t` (#84) (#87)

Considered non-breaking change as `/webrtc` will still be parsed to
`Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known.

See multiformats/multiaddr#150 (comment)
for context.
mxinden added a commit to libp2p/specs that referenced this pull request Mar 23, 2023
mxinden added a commit to multiformats/rust-multiaddr that referenced this pull request Mar 24, 2023
mxinden added a commit to mxinden/test-plans that referenced this pull request Mar 30, 2023
This commit disables the WebRTC tests for rust-libp2p v0.50.0 and v0.51.0 and
chromium-js v0.41.0 and v0.42.0. Neither of these support the new
`/webrtc-direct` (see
multiformats/multiaddr#150 for
context on rename).

The missing support is blocking libp2p/rust-libp2p#3688
namely to upgrade to using `/webrtc-direct`. (Note that this is only blocking
CI. Users can already use `/webrtc-direct` with the latest released
rust-libp2p.)

We will backport `/webrtc-direct` support to the rust-libp2p `v0.51.0` interop
binary and then re-enable it here. I assume we can do the same for js-libp2p v0.42.0.

Instead of disabling the versions with missing support for `/webrtc-direct` we
could as well patch our interoperability test logic. For the sake of simplicity,
I am proposing simply disabling the versions for now.
mxinden added a commit to libp2p/test-plans that referenced this pull request Apr 4, 2023
This commit disables the WebRTC tests for rust-libp2p v0.50.0 and v0.51.0 and
chromium-js v0.41.0 and v0.42.0. Neither of these support the new
`/webrtc-direct` (see
multiformats/multiaddr#150 for
context on rename).

The missing support is blocking libp2p/rust-libp2p#3688
namely to upgrade to using `/webrtc-direct`. (Note that this is only blocking
CI. Users can already use `/webrtc-direct` with the latest released
rust-libp2p.)

We will backport `/webrtc-direct` support to the rust-libp2p `v0.51.0` interop
binary and then re-enable it here.

Instead of disabling the versions with missing support for `/webrtc-direct` we
could as well patch our interoperability test logic. For the sake of simplicity,
I am proposing simply disabling the versions for now.
mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Apr 12, 2023
mxinden added a commit to mxinden/test-plans that referenced this pull request Apr 12, 2023
See multiformats/multiaddr#150 (comment)
for context on discussion.

- Renames the `webrtc` transport identifier to `webrtc-direct`.
- Re-enables `webrtc-direct` support for rust-libp2p v0.51. Previously disabled
  in libp2p#160. See
  libp2p/rust-libp2p#3781 for corresponding change on
  the rust-libp2p side.
- Leaves JS v0.41 and v0.42 untouched. To be done in a follow-up alongside JS
  dependency updates.
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Apr 12, 2023
MarcoPolo pushed a commit to mxinden/test-plans that referenced this pull request Apr 13, 2023
This commit disables the WebRTC tests for rust-libp2p v0.50.0 and v0.51.0 and
chromium-js v0.41.0 and v0.42.0. Neither of these support the new
`/webrtc-direct` (see
multiformats/multiaddr#150 for
context on rename).

The missing support is blocking libp2p/rust-libp2p#3688
namely to upgrade to using `/webrtc-direct`. (Note that this is only blocking
CI. Users can already use `/webrtc-direct` with the latest released
rust-libp2p.)

We will backport `/webrtc-direct` support to the rust-libp2p `v0.51.0` interop
binary and then re-enable it here.

Instead of disabling the versions with missing support for `/webrtc-direct` we
could as well patch our interoperability test logic. For the sake of simplicity,
I am proposing simply disabling the versions for now.
mxinden added a commit to libp2p/test-plans that referenced this pull request Apr 19, 2023
See multiformats/multiaddr#150 (comment)
for context on discussion.

- Renames the `webrtc` transport identifier to `webrtc-direct`.
- Re-enables `webrtc-direct` support for rust-libp2p v0.51. Previously disabled
  in #160. See
  libp2p/rust-libp2p#3781 for corresponding change on
  the rust-libp2p side.
- Leaves JS v0.41 and v0.42 untouched. To be done in a follow-up alongside JS
  dependency updates.
Marcel-G added a commit to Marcel-G/rust-multiaddr that referenced this pull request Sep 11, 2023
Marcel-G added a commit to Marcel-G/rust-multiaddr that referenced this pull request Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants