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

[WIP] UDP-based NAT hole-punching #490

Closed
wants to merge 2 commits into from
Closed

Conversation

cannium
Copy link
Contributor

@cannium cannium commented Nov 21, 2018

This patch utilizes relayed connection as signaling channel, and run /libp2p/punching/1.0.0 protocol on it. If both nodes are behind NAT gateways, they send self-observed QUIC addresses to each other. On receiving such messages, nodes would dial the addresses, hoping to establish connections directly.

I think the patch could be deployed and tested now, so I publish it for review. I understand that some major refactors would happen in go-libp2p, so I'm prepared for corresponding refactors. Also as a "WIP", expect nits here and there:)

Part of #433

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Note that we are planning a refactoring for the host construction that should surface services in a much more reasonable way.

@@ -92,7 +92,9 @@ func (h *AutoRelayHost) background(ctx context.Context) {

for {
wait := autonat.AutoNATRefreshInterval
switch h.autonat.Status() {
status := h.autonat.Status()
h.Ids.SetNatStatus(status)
Copy link
Contributor

Choose a reason for hiding this comment

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

that's gross -- we should embed AutoNAT in basic_host if identify needs it or pass it a reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is not just gross, it's also wrong -- identify should poll the NATStatus directly instead of a second-hand delayed version.

quicAddress := make([]string, 0, len(allAddresses))
for _, a := range allAddresses {
s := a.String()
if strings.Contains(s, "/quic") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use go-multiaddr-net, which contains utilities for testing private addresses.
More specifically, look at IsPrivateAddr.

@vyzo
Copy link
Contributor

vyzo commented Nov 21, 2018

Please don't duplicate the AutoNAT constants and functionality in identify by second hand.
Instead, instantiate it in the basic_host and pass it as reference.

@vyzo
Copy link
Contributor

vyzo commented Nov 21, 2018

Also, it's not clear to me why identify needs to have the NATStatus for this protocol.
How is this surfaced?

@cannium
Copy link
Contributor Author

cannium commented Nov 21, 2018

Please don't duplicate the AutoNAT constants and functionality in identify by second hand.
Instead, instantiate it in the basic_host and pass it as reference.

I understand that the info NATStatus is owned by AutoNATService, but currently AutoNATService doesn't expose any method for query and I'd like to limit the patch inside go-libp2p(at least for now).

Also, it's not clear to me why identify needs to have the NATStatus for this protocol.
How is this surfaced?

The NAT status is exchanged by Identify protocol, so we only do punching if both peers are behind NAT gateways.

@vyzo
Copy link
Contributor

vyzo commented Nov 21, 2018

I understand that the info NATStatus is owned by AutoNATService, but currently AutoNATService doesn't expose any method for query and I'd like to limit the patch inside go-libp2p(at least for now).

That's not the AutoNATService you want -- move the AutoNAT client which lives in the AutoRelayHost.
The way you have written it right now it won't work at all if the user doesn't costruct an autorelay host (ie doesn't pass the Routing parameter in the constructor).

allAddresses := p.host.Addrs()
quicAddress := make([]string, 0, len(allAddresses))
for _, a := range allAddresses {
s := a.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

You should work with the multiaddr representation directly instead of converting to strings.

@vyzo
Copy link
Contributor

vyzo commented Nov 21, 2018

I think we may want to punt a bit on this, I am going to be working on the service refactoring the next few days.

@cannium
Copy link
Contributor Author

cannium commented Nov 22, 2018

I think we may want to punt a bit on this, I am going to be working on the service refactoring the next few days.

Sure. I'd also expect a refactor after changes landed on libp2p. I publish it earlier so maybe more people interested could help test it.

@yonderblue
Copy link

Is this full duplication with what is being done in #188? Is this closer timeframe?

@cannium
Copy link
Contributor Author

cannium commented Feb 17, 2019

Is this full duplication with what is being done in #188? Is this closer timeframe?

No. WebRTC is a standardized protocol that happen also solves NAT traversal problem. This patch is much simpler. For the time being, I'm waiting for the refactoring of libp2p.

@dzianisv
Copy link

Are you going to merge it into the master?

@yonderblue
Copy link

Does this support the symmetric to restricted address cone case?

@cannium
Copy link
Contributor Author

cannium commented Feb 20, 2019

Are you going to merge it into the master?

I'll try my best.

Does this support the symmetric to restricted address cone case?

Theoretically it could handle restricted cone, but not symmetric case.

@marten-seemann
Copy link
Contributor

OBE (Project Flare).

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.

5 participants