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

feat: Swarm.EnableHolePunching flag #8562

Merged
merged 5 commits into from Nov 30, 2021
Merged

Conversation

marten-seemann
Copy link
Member

@marten-seemann marten-seemann commented Nov 22, 2021

This PR won't build. It needs rebasing once #8522 or #8563 is merged.
Opening it now so we don't forget it for the v0.11 release.

@marten-seemann marten-seemann added this to the go-ipfs 0.11 milestone Nov 22, 2021
@marten-seemann marten-seemann changed the title enable hole punching use the option to enable hole punching Nov 22, 2021
@guseggert guseggert mentioned this pull request Nov 23, 2021
80 tasks
@lidel lidel changed the title use the option to enable hole punching feat: Swarm.EnableHolePunching flag Nov 23, 2021
@lidel

This comment has been minimized.

core/node/groups.go Outdated Show resolved Hide resolved

func HolePunching(flag config.Flag, hasRelayClient bool) func() (opts Libp2pOpts, err error) {
return func() (opts Libp2pOpts, err error) {
if flag.WithDefault(false) hasRelayClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

@marten-seemann is this flag.WithDefault(false) && hasRelayClient? Should there be some error if the relay client is disabled, but holepunching is enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the libp2p constructor will complain then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@lidel lidel Nov 30, 2021

Choose a reason for hiding this comment

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

I think hole punching is a separate feature: there could be an implementation which does not depend on circle relay v2. Current config that keeps EnableHolePunching outside RelayClient is fine.

The question is: is there any utility today in allowing EnableHolePunching:true while RelayClient.Enabled:false?
If not, we should return an error and update docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think hole punching is a separate feature: there could be an implementation which does not depend on circle relay v2. Current config that keeps EnableHolePunching outside RelayClient is fine.

Makes sense.

The question is: is there any utility today in allowing EnableHolePunching:true while RelayClient.Enabled:false?

Yeah, we should fix this in go-libp2p. Is this something we can defer to the final release, so we don’t to cut another go-libp2p release now?

Copy link
Contributor

Choose a reason for hiding this comment

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

wait, relay client is autorelay; they are independent in libp2p, so it should be fine.

docs/config.md Outdated Show resolved Hide resolved
@aschmahmann aschmahmann marked this pull request as ready for review November 30, 2021 19:00
docs/config.md Outdated Show resolved Hide resolved
@aschmahmann aschmahmann enabled auto-merge (squash) November 30, 2021 19:02
@aschmahmann aschmahmann merged commit 48a6cac into master Nov 30, 2021
@aschmahmann aschmahmann deleted the enable-hole-punching branch November 30, 2021 19:18
lidel added a commit that referenced this pull request Feb 22, 2022
@lidel lidel mentioned this pull request Feb 22, 2022
4 tasks
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.

None yet

4 participants