-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
This comment has been minimized.
This comment has been minimized.
35ce9ea
to
73a133c
Compare
core/node/libp2p/relay.go
Outdated
|
||
func HolePunching(flag config.Flag, hasRelayClient bool) func() (opts Libp2pOpts, err error) { | ||
return func() (opts Libp2pOpts, err error) { | ||
if flag.WithDefault(false) hasRelayClient { |
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.
@marten-seemann is this flag.WithDefault(false) && hasRelayClient
? Should there be some error if the relay client is disabled, but holepunching is enabled?
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 think the libp2p constructor will complain then.
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.
It probably should, but currently it doesn't: https://github.com/libp2p/go-libp2p/blob/3a71a6d0591482de51b6eaa94093804ea21ab774/options.go#L457-L463
Makes me wonder if EnableHolePunching
should be an option in the RelayClient
: https://github.com/ipfs/go-ipfs-config/blob/71f0b2e315292164bcae5e2457fdfcc5225da759/swarm.go#L36-L45
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 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.
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 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?
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.
wait, relay client is autorelay; they are independent in libp2p, so it should be fine.
73a133c
to
8f7ad38
Compare
This flips the implicit default for feature introduced in 0.11 (#8562) Ref. https://github.com/ipfs/go-ipfs/blob/master/docs/config.md#swarmenableholepunching
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.
Swarm.EnableHolePunching
docs indocs/config.md