feat: Add WebRTC transport #2622

melekes · 2022-04-26T16:00:36Z

Description

Hey 👋 This is a WebRTC transport implemented in accordance w/ the spec. It's based on the webrtc-rs library.

Resolves: #1066.

Links to any relevant issues

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

tomaka · 2022-04-26T16:15:26Z

smoldot is not yet ready for it. So, we'll have to settle for either (1) or (2) for now.

I just want to mention that the state of smoldot shouldn't influence what we decide to merge in the rust-libp2p repo.
This remark was an argument in favor of doing (1) first, so that we can test "for real" if it works.

I think that implementing (3) would reuse most of the work that has been done, so there is little drawback in doing (1) first.

mxinden · 2022-04-28T17:34:37Z

@melekes Thank you very much for this pull request. Exciting work! I won't get to reviewing it this week, though I will make sure I come back to it next week.

mxinden

I took a first look. I very much appreciate all the work that went into this. Thanks @melekes and @tomaka!

transports/webrtc/src/connection/mod.rs

transports/webrtc/src/connection/poll_data_channel.rs

transports/webrtc/src/lib.rs

mxinden · 2022-05-03T07:01:12Z

transports/webrtc/src/transport.rs

+        // XXX: anything to do here?
+        self.dial(addr)


This is required in the case of hole punching. (See below.) Eventually we might want a browser to be able to hole punch to a rust-libp2p server node. I am not yet sure how this concept would then translate into the WebRTC world. Will need to give this more thought.

Simultaneous Connect. The two nodes follow the steps below in parallel for
every address obtained from the Connect message:

For a TCP address:

Upon receiving the Sync, A immediately dials the address to B.

Upon expiry of the timer, B dials the address to A.

This will result in a TCP Simultaneous Connect. For the purpose of all
protocols run on top of this TCP connection, A is assumed to be the
client and B the server.

For a QUIC address:

Upon receiving the Sync, A immediately dials the address to B.

Upon expiry of the timer, B starts to send UDP packets filled with
random bytes to A's address. Packets should be sent repeatedly in
random intervals between 10 and 200 ms.

This will result in a QUIC connection where A is the client and B is
the server.

https://github.com/libp2p/specs/blob/master/relay/DCUtR.md

transports/webrtc/src/lib.rs

transports/webrtc/src/udp_mux.rs

thomaseizinger

Did an initial pass, thanks for pushing this forward! :)

transports/webrtc/src/upgrade.rs

transports/webrtc/src/udp_mux.rs

transports/webrtc/src/transport.rs

transports/webrtc/src/connection/poll_data_channel.rs

transports/webrtc/src/transport.rs

thomaseizinger

Couple more comments.

Most importantly, I think from an overall design perspective, we should try and avoid these long-running tasks that are just spawned into the executor. The rest of rust-libp2p is designed around exposing passive state machines that don't do anything unless they are polled.

Thoughts @mxinden?

transports/webrtc/src/connection/poll_data_channel.rs

transports/webrtc/src/upgrade.rs

transports/webrtc/src/udp_mux.rs

transports/webrtc/src/transport.rs

melekes · 2022-05-19T13:27:08Z

Thanks for review @mxinden and @thomaseizinger ❤️ appreciate it! Waiting for #2652 makes sense. I'm still waiting for some PRs to land in webrtc-rs, which is used here. When the spec is finalised, I will fix the remaining comments and update the code to reflect the spec.

mxinden · 2022-05-22T17:53:42Z

Most importantly, I think from an overall design perspective, we should try and avoid these long-running tasks that are just spawned into the executor. The rest of rust-libp2p is designed around exposing passive state machines that don't do anything unless they are polled.

Very much agreed on a general level. I see additional tasks as a performance optimization. Until we can prove that it is worth the performance gain, I would very much favor the simplicity of not spawning additional tasks.

Also note that enforcing back-pressure is hard when spawning tasks. E.g. in a simple HTTP example, spawning a task per HTTP request breaks the back-pressure chain unless enforced through an additional mechanism.

This reverts commit d5b2e09.

warning: uses my fork of libp2p and webrtc-rs library Impl: libp2p/rust-libp2p#2622 Spec: libp2p/specs#412 Refs paritytech/smoldot#1712

Closes #168 This PR adds `RTCCertificate::from_existing` method, which constructs `RTCCertificate` from an existing DTLS certificate. An existing certificate might be needed in cases like [this](libp2p/rust-libp2p#2622) where you need DTLS identity to be fixed for some period of time (whole duration of the certificate or some part of it). * peer_connection: replace x509_cert with pem field also, make `pem` and `expires` fields private and add `RTCCertificate::from_existing` method, which gives a way to construct a `RTCCertificate` using an already generated certificate (as opposed to generating a new one using `from_params` or `from_key_pair` methods). * make RTCConfiguration clonable otherwise, it's not possible to reuse the same config across N peer connections.

mxinden · 2022-07-06T05:05:59Z

@melekes does it make sense for me to give this another review? Or would you prefer to wait until after this is updated to the latest Transport and StreamMuxer changes? Also, please do let me know in case you want me to help with any of it.

melekes · 2022-07-06T05:08:03Z

Or would you prefer to wait until after this is updated to the latest Transport and StreamMuxer changes?

this ^

Refs webrtc-rs/sctp#28

Refs libp2p#2652

melekes · 2022-07-12T07:25:48Z

Tried my best at updating to the current master d96b499, but failed. Tests are failing after 60s of not establishing a conn. Kinda wish there was a document with changes that must be made to the transport.

mergify · 2022-11-13T00:00:45Z

This pull request has merge conflicts. Could you please resolve them @melekes? 🙏

update webrtc to 0.6.0

melekes · 2022-11-16T05:30:07Z

@mxinden @thomaseizinger I think this is ready for final review & merge

mxinden · 2022-11-16T15:09:52Z

@mxinden @thomaseizinger I think this is ready for final review & merge

Wonderful. Thanks for the update and the upstream changes.

Adding the send-it label here for this pull request to be merged. I am operating under the assumption that @thomaseizinger's approval is still valid. As far as I can tell, none of his comments since are unaddressed.

mxinden · 2022-11-16T15:53:32Z

@melekes can you address the CI failures unrelated to semver-check?

thomaseizinger

@melekes can you address the CI failures unrelated to semver-check?

I've marked them as inline comments.

Cargo.toml

transports/webrtc/src/tokio/certificate.rs

transports/webrtc/examples/listen_ping.rs

thomaseizinger · 2022-11-17T00:42:40Z

@mxinden @thomaseizinger I think this is ready for final review & merge

Wonderful. Thanks for the update and the upstream changes.

Adding the send-it label here for this pull request to be merged. I am operating under the assumption that @thomaseizinger's approval is still valid. As far as I can tell, none of his comments since are unaddressed.

Assumption correct! I slightly updated the pull request description to make a better commit message.

and fix link

thomaseizinger · 2022-11-17T05:13:48Z

transports/webrtc/tests/smoke.rs

+use libp2p::core::{identity, muxing::StreamMuxerBox, upgrade, Transport as _};
+use libp2p::request_response::{
+    ProtocolName, ProtocolSupport, RequestResponse, RequestResponseCodec, RequestResponseConfig,
+    RequestResponseEvent, RequestResponseMessage,
+};
+use libp2p::swarm::{Swarm, SwarmEvent};


It would be great if we could rewrite these tests to not depend on Swarm. See #3023.

However, I don't think it is a blocker. We can resolve that also in the above linked PR.

tomaka · 2022-11-17T07:41:18Z

transports/webrtc/src/tokio/transport.rs

+                            listen_addr: self
+                                .listen_multiaddress(ip, self.config.id_keys.public().to_peer_id()),


Sorry for raising this after it has been merged, but I think that all the other transports do not include the /p2p/ in the multiaddress they report, but this one does.

It's an important detail, as these addresses aren't just for logging purposes but get sent over the network to other peers.

e5e9c46

Looks like it was done on purpose. @thomaseizinger @mxinden can you confirm that it's okay to include /p2p/ in the reported multiaddress? (other transports don't seem to do that)

Yeah I remember that. Does this break something? IMO, peers should always use /p2p when possible upon dialing another peer. We have access to the PeerId in the Transport so why not append it? Esp. because the webrtc spec mandates that you have to dial with /p2p.

These addresses get sent over the network through the identify protocol. They are then most likely propagated to kademlia to put in the k-buckets. Either all addresses have a /p2p/ at the end (and then the receiver removes these /p2p/), or none, but the in-between makes no sense.

We have access to the PeerId in the Transport so why not append it?

We already know the PeerId everywhere, so why should we append it to the addresses as well? These addresses are always in every single scenario already associated with a specific PeerId. Putting the PeerId again in the address itself is completely redundant.

I really dislike this /p2p/ multiaddress specifier with a passion because it's purely an API trick. It exists solely so that you can call functions with foo(concat(address, "/p2p/", peer_id)) (and then foo de-concats them) instead of foo(address, peer_id). I don't understand why this /p2p/ exists at all. And it makes all our usages of Multiaddr completely ambiguous because you never know whether there's a /p2p/ at the end or not.

Esp. because the webrtc spec mandates that you have to dial with /p2p.

Sorry but if the spec says that then I must say that this is completely out of scope of the spec. This is purely an API problem, not a wire problem. The spec might say that you should know the peerid of the remote ahead of time, and that's true, but that isn't equivalent to "you should pass as parameter to your dial function a multiaddress that contains a /p2p/ component".

On the other hand, the identify spec if it was written properly should mention what the multiaddresses should look like, and if so would probably say that they shouldn't have a /p2p/ at the end given that not only the remote is supposed to know our PeerId but we already send the PeerId separately through another field in the identify protocol.

I agree that this is an API problem and would like to present a (small) counter-weight: having this syntax available is very useful so that people (in my case: IT admins) can effect a dial to a specific peer at a specific transport address by just copying&pasting a single string.

If we were to agree that that is a worthy cause for the existence of /p2p syntax then it would be surprising to see it rejected in API calls. DialOpts is already a bit weird in that sense because it requires the calling code to figure out whether the PeerId is to be required or not.

Small rant: there is quite a bit of code in ipfs-embed to canonicalise multiaddrs in this regard, so that the address book can present a consistent view that is useful for UIs (→ human consumption).

I agree that it is very useful to be able to copy-paste a string that contains a /p2p/ and pass both the address and peerid at the same time, however that is still not a proper justification for the existence of /p2p/ in multiaddresses.

If you want your API to accept a multiaddress and peerid, then just use a type named MultiaddrWithPeerId and then properly split it into a (Multiaddr, PeerId) tuple before passing them around. Similarly, when you show them to the user, just reconstruct the MultiaddrWithPeerId. Not everything has to be shoved into a single Multiaddr type.

True. A more sophisticated admin might want to express a relayed connection through a specific peer, in which case this separation would be difficult — in particular since multiple such hops could potentially be used (not sure whether it is supported today, but it might be!).

I’m also not sure whether I agree with the underlying premise that the PeerId is not genuinely part of the “address”. The routing example above demonstrates that an address may well depend on the identities of the nodes found at certain transport hops. This would imply that a Multiaddr may contain a MultiaddrWithPeerId, at which point we’re back at square one.

Our conversation went off-topic I guess because I said that I didn't understand why /p2p/ existed.

Let's recenter on the pragmatic problem here, which is that the WebRTC transport returns addresses with /p2p/ while all the other transports don't.
The reason why I'd like to change that is consistency, and to avoid passing clearly redundant information when sending these addresses through identify (it also avoids problems such as "what if there's a mismatch in the peerid?"), and to avoid issues such as "remotes treat our address as invalid because it didn't expect the /p2p at the end". I'm not trying to push an ideology about /p2p itself here.

I think we all agree that it is good to precisely document the contexts in which a multiaddr ends with /p2p, the contexts in which it might, and the contexts in which it doesn't. We'd rather avoid having to put if multiaddr.last().is_p2p() { multiaddr.pop() } everywhere "just in case".

emphatic yes!

since libp2p/rust-libp2p#2622 was merged

Refs paritytech/smoldot#1712 Impl libp2p/rust-libp2p#2622 - WebRTC transport is enabled for non-validators and developers by default. - The transport will generate and store the certificate, which is required for WebRTC identity, in base dir. In the future, when a new version of `ring` library is released, the certificate will be deterministically derived from the node's peer ID.

to align with all other transports, which don't include `/p2p/..` in their reported addresses. Refs libp2p#2622 (comment)

This aligns with what other transports do. Related: #2622 (comment).

Refs paritytech/smoldot#1712 Impl libp2p/rust-libp2p#2622 - WebRTC transport is enabled for non-validators and developers by default. - The transport will generate and store the certificate, which is required for WebRTC identity, in base dir. In the future, when a new version of `ring` library is released, the certificate will be deterministically derived from the node's peer ID.

- Added in libp2p#2622 - Interoperability tests added in libp2p/test-plans#100

- Added in #2622 - Interoperability tests added in libp2p/test-plans#100

This was referenced Apr 28, 2022

WebRTC client & server paritytech/smoldot#2135

Closed

wasm-node: Add support for WebRTC paritytech/smoldot#1712

Closed

mxinden reviewed May 3, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

thomaseizinger reviewed May 9, 2022

View reviewed changes

mxinden mentioned this pull request May 10, 2022

Add WebRTC Transport Spec libp2p/specs#220

Closed

melekes mentioned this pull request May 18, 2022

Dependencies update webrtc-rs/dtls#12

Closed

thomaseizinger reviewed May 19, 2022

View reviewed changes

mxinden mentioned this pull request May 23, 2022

webrtc/: Add libp2p WebRTC browser-to-server spec libp2p/specs#412

Merged

19 tasks

melekes mentioned this pull request Jun 21, 2022

[peer_connection] allow persistent certificates webrtc-rs/webrtc#204

Merged

mxinden mentioned this pull request Jun 22, 2022

core/muxing: Redesign StreamMuxer trait #2648

Closed

27 tasks

melekes added 4 commits June 29, 2022 16:15

fix feature syntax err

3c7f796

enable webrtc by default (temporary)

d5b2e09

Revert "enable webrtc by default (temporary)"

bd8091f

This reverts commit d5b2e09.

add libp2p_webrtc to libp2p

6d703f8

melekes added a commit to melekes/substrate that referenced this pull request Jun 30, 2022

client: add WebRTC transport

6cf708f

warning: uses my fork of libp2p and webrtc-rs library Impl: libp2p/rust-libp2p#2622 Spec: libp2p/specs#412 Refs paritytech/smoldot#1712

melekes mentioned this pull request Jun 30, 2022

client: add WebRTC transport melekes/substrate#1

Closed

mxinden mentioned this pull request Jul 4, 2022

swarm/: Expose time to establish connection #2745

Closed

mxinden mentioned this pull request Jul 7, 2022

core/muxing: Flatten StreamMuxer interface to poll_{inbound,outbound,address_change,close} #2724

Merged

4 tasks

melekes added 3 commits July 11, 2022 11:49

fix ErrShortBuffer error by increasing the temporary buffer size

64d60e9

Refs webrtc-rs/sctp#28

Merge branch 'master' into anton/webrtc-transport

e8207f9

update to support new Transport interface

d96b499

Refs libp2p#2652

melekes added 2 commits November 16, 2022 12:10

add from_pem and serialize_pem to Certificate

fc4bedb

update webrtc to 0.6.0

Merge branch 'master' into anton/webrtc-transport

d2fc5f6

thomaseizinger reviewed Nov 17, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

transports/webrtc/src/tokio/certificate.rs Outdated Show resolved Hide resolved

transports/webrtc/examples/listen_ping.rs Outdated Show resolved Hide resolved

thomaseizinger changed the title ~~transports/webrtc: Add WebRTC transport~~ feat: Add WebRTC transport Nov 17, 2022

melekes added 2 commits November 17, 2022 09:30

format code

06d53d9

remove webrtc-pem feature

0b0be1d

and fix link

thomaseizinger approved these changes Nov 17, 2022

View reviewed changes

thomaseizinger added the send-it label Nov 17, 2022

mergify bot merged commit a714864 into libp2p:master Nov 17, 2022

tomaka reviewed Nov 17, 2022

View reviewed changes

melekes added a commit to melekes/substrate that referenced this pull request Nov 17, 2022

change libp2p patch to upstream

3be3a5d

since libp2p/rust-libp2p#2622 was merged

p-shahi mentioned this pull request Nov 17, 2022

Add WebRTC transport interop ping test libp2p/test-plans#67

Closed

mxinden mentioned this pull request Nov 18, 2022

libp2p-webrtc is already claimed on crates.io #3141

Closed

p-shahi mentioned this pull request Nov 18, 2022

Document "WebRTC: Browser to Server" libp2p/docs#211

Closed

5 tasks

melekes mentioned this pull request Nov 22, 2022

client: add WebRTC transport melekes/substrate#2

Closed

melekes added a commit to melekes/rust-libp2p that referenced this pull request Nov 22, 2022

[transports/webrtc] remove local peer ID from reported addr

050c416

to align with all other transports, which don't include `/p2p/..` in their reported addresses. Refs libp2p#2622 (comment)

melekes mentioned this pull request Nov 22, 2022

fix(webrtc): Don't append /p2p to listen addresses #3154

Merged

mergify bot pushed a commit that referenced this pull request Nov 23, 2022

fix(webrtc): Don't append /p2p to listen addresses (#3154)

babacc5

This aligns with what other transports do. Related: #2622 (comment).

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jan 20, 2023

docs(roadmap): Mark WebRTC browser-to-server as done

d09c427

- Added in libp2p#2622 - Interoperability tests added in libp2p/test-plans#100

mxinden mentioned this pull request Jan 20, 2023

docs(roadmap): Mark WebRTC browser-to-server as done #3357

Merged

mergify bot pushed a commit that referenced this pull request Jan 24, 2023

docs(roadmap): Mark WebRTC browser-to-server as done (#3357)

d736132

- Added in #2622 - Interoperability tests added in libp2p/test-plans#100

DougAnderson444 mentioned this pull request Feb 2, 2023

WebRTC support on Linux tauri-apps/wry#85

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add WebRTC transport #2622

feat: Add WebRTC transport #2622

melekes commented Apr 26, 2022 •

edited by thomaseizinger

Loading

tomaka commented Apr 26, 2022

mxinden commented Apr 28, 2022

mxinden left a comment

mxinden May 3, 2022

This comment was marked as outdated.

thomaseizinger left a comment

thomaseizinger left a comment

melekes commented May 19, 2022

mxinden commented May 22, 2022

mxinden commented Jul 6, 2022

melekes commented Jul 6, 2022

melekes commented Jul 12, 2022

mergify bot commented Nov 13, 2022

melekes commented Nov 16, 2022

mxinden commented Nov 16, 2022

mxinden commented Nov 16, 2022

thomaseizinger left a comment

thomaseizinger commented Nov 17, 2022

thomaseizinger Nov 17, 2022

tomaka Nov 17, 2022 •

edited

Loading

melekes Nov 18, 2022 •

edited

Loading

thomaseizinger Nov 18, 2022

tomaka Nov 18, 2022 •

edited

Loading

rkuhn Nov 18, 2022

tomaka Nov 18, 2022

rkuhn Nov 18, 2022

tomaka Nov 18, 2022 •

edited

Loading

rkuhn Nov 18, 2022

		listen_addr: self
		.listen_multiaddress(ip, self.config.id_keys.public().to_peer_id()),

feat: Add WebRTC transport #2622

feat: Add WebRTC transport #2622

Conversation

melekes commented Apr 26, 2022 • edited by thomaseizinger Loading

Description

Links to any relevant issues

Change checklist

tomaka commented Apr 26, 2022

mxinden commented Apr 28, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden May 3, 2022

Choose a reason for hiding this comment

This comment was marked as outdated.

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

melekes commented May 19, 2022

mxinden commented May 22, 2022

mxinden commented Jul 6, 2022

melekes commented Jul 6, 2022

melekes commented Jul 12, 2022

mergify bot commented Nov 13, 2022

melekes commented Nov 16, 2022

mxinden commented Nov 16, 2022

mxinden commented Nov 16, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 17, 2022

thomaseizinger Nov 17, 2022

Choose a reason for hiding this comment

tomaka Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

melekes Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger Nov 18, 2022

Choose a reason for hiding this comment

tomaka Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

rkuhn Nov 18, 2022

Choose a reason for hiding this comment

tomaka Nov 18, 2022

Choose a reason for hiding this comment

rkuhn Nov 18, 2022

Choose a reason for hiding this comment

tomaka Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

rkuhn Nov 18, 2022

Choose a reason for hiding this comment

melekes commented Apr 26, 2022 •

edited by thomaseizinger

Loading

tomaka Nov 17, 2022 •

edited

Loading

melekes Nov 18, 2022 •

edited

Loading

tomaka Nov 18, 2022 •

edited

Loading

tomaka Nov 18, 2022 •

edited

Loading