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

Transit: protocol improvements #16

Merged
merged 5 commits into from
Feb 2, 2022
Merged

Transit: protocol improvements #16

merged 5 commits into from
Feb 2, 2022

Conversation

piegamesde
Copy link
Member

@piegamesde piegamesde commented Sep 22, 2021

A prototype implementation can be found here.

Also note that stun.magic-wormhole.io doesn't exist yet ^^

piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Sep 25, 2021
Fancy things! And code cleanup.

- We now do firewall hole punching. If your device got a firewall connected (and it really should, and by default), direct transfers should now work for you too.
- If you aren't behind any NATs (IPv6 users rejoice), **direct connections should now work across the internet** 🎉
- NAT traversal using STUN, for our poor IPv4 users. Gives you direct connections as well, provided that the NAT behaves well (fortunately, they mostly do).
- If we found a connection over a relay, we wait some longer in the hope of a direct connection.

See also magic-wormhole/magic-wormhole-protocols#16 for some more words on what this code does in a bit more detail.
@piegamesde piegamesde changed the title Transit: describe how to get a direct connection in detail Transit: protocol improvements Oct 8, 2021
@piegamesde
Copy link
Member Author

I added support for other transport protocols than plain TCP, thus this PR now supersedes #10. Discussion on the hints format can be found there.

transit.md Outdated Show resolved Hide resolved
transit.md Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
* I2P: like Tor, but not capable of proxying to normal TCP hints.
* ICE-mediated STUN/STUNT: NAT hole-punching, assisted somewhat by a server
that can tell you your external IP address and port. Maybe implemented as a
uTP stream (which is UDP based, and thus easier to get through NAT).
Copy link
Member

Choose a reason for hiding this comment

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

proabably worth adding a point about Tor onion services, since that got deleted in a paragraph above. Perhaps:

  • Tor Onion services: one side could run an Onion service, allowing a "direct" Tor connection without using any relay

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, did I delete too much somewhere? Everything that the Python implementation does should be covered by the spec.

@piegamesde
Copy link
Member Author

Thank you for mentioning the framing, this is a good point: we only require a continuous byte stream (as TCP gives us) because we build our own framing on top. With WebSockets, this seems wasteful. Should I describe a variant that removes the framing for protocols that already bring their own? This would amount to saving the \n in the handshake and then the four byte length prefix per message, which is negligible if you only care about saving bytes.

and you always want to prefer IPv6 connections.

\* Since you should always host your own for production use (and also most of
the public ones don't support TCP), we provide <tcp://stun.magic-wormhole.io:3478>
Copy link
Member Author

Choose a reason for hiding this comment

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

By the way we need infrastructure access to set this up (CC @warner). In the meantime, I host a public instance at stun.piegames.de.

piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Oct 10, 2021
Fancy things! And code cleanup.

- We now do firewall hole punching. If your device got a firewall connected (and it really should, and by default), direct transfers should now work for you too.
- If you aren't behind any NATs (IPv6 users rejoice), **direct connections should now work across the internet** 🎉
- NAT traversal using STUN, for our poor IPv4 users. Gives you direct connections as well, provided that the NAT behaves well (fortunately, they mostly do).
- If we found a connection over a relay, we wait some longer in the hope of a direct connection.

See also magic-wormhole/magic-wormhole-protocols#16 for some more words on what this code does in a bit more detail.
@piegamesde
Copy link
Member Author

I feel a bit weird for bumping the relay version although we technically only change the message formatting and not the way the protocol actually works. I just had an idea for an alternative implementation: the abilities are already serialized as objects, i.e. they may have complex structured values. So instead of having a relay-v2 ability, we could send "relay-v1": { "non-tcp-support": true } or something. The hints field would then become a bit of a misnomer and outdated, but I think that's kind of okay. (An open question would be whether the TCP hints are announced via the old hints field, which is inconsistent, or with the urls, which would make the hints redundant and obsolete.)

@piegamesde
Copy link
Member Author

I implemented the idea described in my last comment, and it turned out to be quite an improvement IMO. Thus, the relay-v2 ability is now gone again, replaced with a url_hints flag on the relay-v1 hint. I took the occasion to also move the abilities description from the file transfer protocol to the relay protocol.

We already do this everywhere anyways, so this is more a correction
of the documentation than a change to the protocol. Therefore, I've
omitted describing any backwards compatibility handling.
Most messages have structured content (JSON, …) and require a lot more resources
to be parsed than is needed to only hold the byte buffer in memory.
Therefore, 4GiB is totally unrealistic and there is need for an
artificially reduced upper bound.
We use WebSockets as an example, but actually this applies to all protocols
that offer their own message framing
@piegamesde piegamesde force-pushed the transit-improvements branch 2 times, most recently from 1a9bcc8 to e267c6c Compare January 13, 2022 00:14
@piegamesde
Copy link
Member Author

🔔 I think this is good to go, so let's start a Final Comment Period! 🔔

I expect all commits except the last one to be rather uncontroversial. If this turns out to be true and the first part does not get any complaints, I'll split this up into two PRs to get that half merged early.

@piegamesde piegamesde added the Final comment period The PR is ready for being merged, state your opinions now label Jan 13, 2022
@meejah
Copy link
Member

meejah commented Jan 13, 2022

Is the intent of "version A" versus "version B" to decide in this PR on one and only use that, or something else?
I also think a complete JSON example with e.g. two different relays and multiple protocols would be good.

@piegamesde
Copy link
Member Author

Is the intent of "version A" versus "version B" to decide in this PR on one and only use that, or something else?

No. The thing is, that due to how our abstractions are layered, that transit cannot specify how the required messages should be encoded. This is part of the application protocol on top. Usually, there is a "canonical" encoding that everybody follows, like for the hints. For the abilities however, it has some (minor) drawbacks that are improved in Version B. That one is not used anywhere at the moment, except by my port forwarding and file transfer v2 experiments.

That being said, I'll remove Version B and mark Version A as "canonical". This will make it easier to understand. It does not change any semantics, since the upper layer protocol still has the last word on how things should be done.

I also think a complete JSON example with e.g. two different relays and multiple protocols would be good.

Will do.

@meejah
Copy link
Member

meejah commented Jan 13, 2022

I will do a more in-depth review tomorrow and/or weekend, but a couple quick thoughts:

  1. I do think adding a "name" might be useful while we're changing the hints. The way I think of this is that there are some number of relays (currently "1") and they each have some number of ways one can connect to them. (One use-case here being inter-op -- consider a Web client that only cares about wss hints and a Python client that only cares about tcp hints -- if they used the same relay that supported both, they can relay to each other). Another use-case is simply something nice to display to users.
  2. the "urls" stuff feels like it wants to just be another "type"; maybe once there's a bigger example this will be more clear, but a flag like that makes me wonder why there isn't just a type: "url" in the list (instead of a flag and a separate list).

transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
Copy link
Member

@meejah meejah left a comment

Choose a reason for hiding this comment

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

looking good!
see also #16 (comment)

transit.md Outdated Show resolved Hide resolved
transit.md Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
transit.md Outdated Show resolved Hide resolved
@piegamesde piegamesde force-pushed the transit-improvements branch 4 times, most recently from 78f0ef0 to 815862b Compare January 28, 2022 23:34
@piegamesde
Copy link
Member Author

All comments are resolved, maybe we all read through it once or twice to catch even the last typo and then it should be good to go 🎉

piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 2, 2022
transit.md Show resolved Hide resolved
`relay-v2` is the same as `relay-v1`, but with enhanced hints encoding
that allows for different ways to connect to a relay server become
possible. Furthermore, relay hints may provide a human readable name.

Additionally, a few things are moved around between files, both
to put things where they belong and in anticipation of future
features.
@piegamesde piegamesde merged commit af046df into main Feb 2, 2022
@piegamesde piegamesde deleted the transit-improvements branch February 2, 2022 13:29
@piegamesde piegamesde removed the Final comment period The PR is ready for being merged, state your opinions now label Feb 2, 2022
@meejah
Copy link
Member

meejah commented Feb 2, 2022

awesome (and sorry this took so long to get merged)

piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 3, 2022
piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 3, 2022
piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 4, 2022
piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 4, 2022
piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 4, 2022
piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 4, 2022
piegamesde added a commit to magic-wormhole/magic-wormhole.rs that referenced this pull request Feb 5, 2022
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.

2 participants