Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
webrtc/: Add libp2p WebRTC browser-to-server spec #412
webrtc/: Add libp2p WebRTC browser-to-server spec #412
Changes from 37 commits
9677aa2
968d127
64a3dff
7dfcca1
28e341b
b3bd981
dc6fb4e
9728e22
13f6aaa
7ec96db
5f5fd60
84d4c41
9790d1a
4c6320e
449462b
3e9fb70
81249f3
ee09ed4
d451652
b485340
b654f11
9a90f0d
baccb1d
83e7aa0
766bb62
525bc9e
3e9e6df
a29cb45
827def2
e6c23e1
5f7ea61
f5eb1b7
a8a42b5
c8e725a
7e601e7
9e8ec87
cceed49
c5522c4
c89a72a
bc76a06
a0ae9b0
774cd52
ae1020b
4b8e890
285574d
7009f94
373bafe
93df7e3
027b539
865f4f2
a60234c
48f5fe7
7f40491
31ac65d
85364f4
9ce0d45
2570b24
7a8ebc0
f0acbb5
6df6cf3
c19ba8a
65a894a
6fb2750
73516b1
8208f02
6c7f18e
666c9f4
ae5b0d8
7ce6213
a2d1547
ded63c5
54f8264
67e12da
4a52edc
7380483
c063a81
65590f9
8dc6465
daecb5b
6267324
590c3db
a207aa5
ed5c641
42e7a20
3315b5c
2dddfdc
0deaa30
7ac21aa
8803682
b5466fd
4931f87
d5d164b
6775d10
730dca0
faf641d
e2df94c
b268693
3aebc68
50b4e12
c8df617
4de8f96
4dcf801
16e38fb
f635466
5c9b600
961ada2
75c3b5c
9abf638
1cb4093
a46919c
3058fb9
a382f18
44fd082
22fc557
1ddc317
1e3ca59
b1f629a
070ebea
072c317
b8dc6fd
a62fdd2
118a4dc
60ac97a
6846d34
c369f2b
0c4e836
dd9756b
249ebd2
be5cfeb
602f492
1c7956c
b5ac74b
b6e7eb1
2d0478c
4dc788c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In multiformats/multiaddr#130 (comment) @tomaka suggests to use
/webrtc-direct
instead of/webrtc
. I think this is a better place for this discussion to take place.Agreed that we might add other flavors (e.g. always double encrypting via nested Noise), though I would expect that the flavor we are adding here (this specification) will be the most used one, thus deserving the short name.
To the best of my knowledge, we don't need a separate protocol name for the browser-to-browser use-case. I.e. we can use the same
/webrtc
protocol name. Though, given that we haven't yet designed it, I might be missing something.What do folks think? @tomaka would you expect as well that this flavor will become the most prominent one and thus deserves the shorter name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more specific name is a good thing, given that we already have webrtc-star and webrtc-direct transports.
These use the multiaddr formats
/ip4/$ADDR/tcp/$PORT/wss/p2p-webrtc-star/p2p/QmFoo
and/ip4/$ADDR/tcp/$PORT/http/p2p-webrtc-direct/QmFoo
respectively.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not going to push for this, i.e. I don't intend to change the protocol name. In case folks do feel strongly about this, please speak up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, we’ll discover multiple WebRTC multiaddrs. IPv4 and IPv6 at the very least. We should specify how this is handled. I assume we’d combine them into the same SDP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the case right now (not in Rust implementation at least).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell there is no benefit for combining multiple WebRTC multiaddrs into a single connection attempt in the browser-to-server use-case. @marten-seemann did you had one in mind for the browser-to-server use-case?
Agreed that we should combine them for the browser-to-browser use-case, though I would like to delay that discussion to the second iteration of this specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we always require a PeerId for dialing a WebRTC address?
rust-libp2p
in general also allows dialing without a/p2p
postfix. For this spec though, we fail to dial in case the/p2p
part is not present. Is this intended from this wording?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this work with the noise handshake if we do not know the remote's peer ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @thomaseizinger. Addressed in b8dc6fd.
@ckousik we are using the Noise XX handshake pattern. With this pattern the two peers exchange their peer ID within the Noise exchange.
This does have security implications. See #458 for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t understand this sentence. What does upgrading as an insecure connection mean, as opposed to upgrading as a secure connection?
I think we should avoid the term „upgrading“ altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ded63c5 rephrases the section. @marten-seemann let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have been resolved. I think this should be moved as a statement to the section above (“major WebRTC implementations support demultiplexing…”), maybe with a short explanation how the demultiplexing is achieved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed with 67e12da including a short explainer on how to do multiplexing on the UDP socket. Happy for additional wording suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DCUtR seems like a poor fit for this use case (I think I’ve mentioned that before). All we need to do is transfer a list of multiaddrs in both directions, there’s no need for the two-step protocol (SYNC, CONNECT) that DCUtR uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using DCUtR seems to prevent trickling. This really should be its own protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of the two transformation steps by transmitting the SDP directly.