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

Hole Punching Protocol #711

Closed
wants to merge 38 commits into from
Closed

Hole Punching Protocol #711

wants to merge 38 commits into from

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Aug 26, 2019

For #1039.

Implements the Hole Punching co-ordination protocol and upgrades dependencies to solve multi-stream simultaneous connect.

TODO

  • Deps.
  • Allow the Hole Punching Stream to open on a transient connection.
  • Unit Tests (once we are happy with the general direction we are taking here).

go.mod Outdated
github.com/libp2p/go-eventbus v0.1.0
github.com/libp2p/go-libp2p-autonat v0.1.0
github.com/libp2p/go-libp2p-blankhost v0.1.3
github.com/libp2p/go-libp2p-circuit v0.1.1
github.com/libp2p/go-libp2p-core v0.2.2
github.com/libp2p/go-libp2p-core v0.2.3-0.20190826112323-405451631c05
Copy link
Contributor

Choose a reason for hiding this comment

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

just noting for when we need to propagate releases

@aarshkshah1992
Copy link
Contributor

Ping @marten-seemann @Stebalien for review.

@aarshkshah1992 aarshkshah1992 changed the title Implement support for simultaneous open [WIP ]Implement support for simultaneous open and hole punching Jan 15, 2021
@aarshkshah1992 aarshkshah1992 changed the title [WIP ]Implement support for simultaneous open and hole punching Implement support for simultaneous open and hole punching Jan 21, 2021
@aarshkshah1992
Copy link
Contributor

ping @vyzo for review .

@aarshkshah1992 aarshkshah1992 self-assigned this Jan 21, 2021
@aarshkshah1992 aarshkshah1992 requested review from marten-seemann and removed request for bigs January 21, 2021 08:18
// attempts to make a direct connection with the remote peer of `relayConn` by co-ordinating a hole punch over
// the given relay connection `relayConn`.
func (hs *HolePunchService) holePunch(relayConn network.Conn) {
rp := relayConn.RemotePeer()
Copy link
Contributor

Choose a reason for hiding this comment

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

@requilence
Copy link

requilence commented Jan 26, 2021

@aarshkshah1992 Hello! I'm looking forward to this PR being merged, as we are facing a lot of troubles with multiplexing with TCP transport.

BTW, I've just run the bidirectional connections test from this issue: libp2p/go-libp2p-noise#70, and it fails: on the 4th-8th iteration h1.Connect(...) returns nil err but the next-line h1.Network().Peers() returns 0 peers.
The same happens when running with NoSecurity option.

@aarshkshah1992
Copy link
Contributor

@requilence That definitely sounds like a bug. I'll take a look !

@aarshkshah1992
Copy link
Contributor

@requilence There was a race in that test wherein a peer could check for peers it is connected to after that connection has been closed by the remote peer depending on when it hit that code path.

I've fixed the test and you can follow this issue at #1041. Once this PR is merge, we can release simopen.

@aarshkshah1992 aarshkshah1992 changed the title Implement support for simultaneous open and hole punching Hole Punching Protocol Jan 28, 2021
@requilence
Copy link

requilence commented Jan 28, 2021

@aarshkshah1992 can I clarify? Do both of them share the same connection? For each node, it seems that the connection has been initiated on their side and can be safely closed when the operation on their side is finished.

Here is an example

  • node A opens a connection to node B and starts some long transfer via stream
  • at the same time node B opens a connection to node B and downloads a file via bitswap
  • node B finishes blocks exchange and closes the connection
  • will it lead to a reset of the stream initiated by node A?

Copy link
Contributor Author

@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.

this is looking pretty good, a few comments.

Note: I can't green tick this one, as I originally opened the pr.

Comment on lines +191 to +193
if cfg.EnableHolePunching && !cfg.Relay {
return nil, errors.New("cannot enable hole punching; relay is not enabled")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will need to use the v2 relay instead of the legacy relay.

const (
protocol = "/libp2p/holepunch/1.0.0"
maxMsgSize = 4 * 1024 // 4K
holePunchTimeout = 2 * time.Minute
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this may exceed the limited relay timeout... maybe we need to increase the default for the latter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's 1min currently.


// short-circuit hole punching if a direct dial works.
// attempt a direct connection ONLY if we have a public address for the remote peer
for _, a := range hs.host.Peerstore().Addrs(rp) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this kind of assumes that identify has completed for the peer, which may not be the case here.

}

// hole punch
s, err := hs.host.NewStream(hs.ctx, rp, protocol)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WithUseTransient(ctx)

Copy link
Contributor

Choose a reason for hiding this comment

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

done.

Comment on lines +115 to +117
if msg.GetType() != holepunch_pb.HolePunch_CONNECT {
s.Reset()
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's log this, probably Debug.

Copy link
Contributor

Choose a reason for hiding this comment

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

done.

p2p/protocol/holepunch/coordination.go Show resolved Hide resolved
// We plan to have a better address discovery and advertisement mechanism in the future.
// See https://github.com/libp2p/go-libp2p-autonat/pull/98
repeated bytes ObsAddrs = 2;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

newline....

Copy link
Contributor

Choose a reason for hiding this comment

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

done.

@vyzo
Copy link
Contributor Author

vyzo commented Feb 17, 2021

Note that this is getting quite messy in terms of commit history, we might have to squash it.

@vyzo
Copy link
Contributor Author

vyzo commented Feb 17, 2021

We will want to integrate the circuit v2 pr here and change the libp2p constructor to use the new beast instead of the legacy one.

@aarshkshah1992 aarshkshah1992 mentioned this pull request Feb 19, 2021
2 tasks
@aarshkshah1992
Copy link
Contributor

Closed in favour of #1057.

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.

4 participants