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.

Sign up for GitHub

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

Jump to bottom

feat: EnableHolePunching by default #8748

Merged
merged 4 commits into from
May 3, 2022
Merged

feat: EnableHolePunching by default #8748

merged 4 commits into from
May 3, 2022

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 22, 2022 •
edited by aschmahmann
Loading

This PR continues epic from #8562:

@lidel lidel added this to the go-ipfs 0.13 milestone Feb 22, 2022
@lidel lidel marked this pull request as draft February 22, 2022 13:53
lidel added a commit to ipfs/interop that referenced this pull request Feb 22, 2022
@lidel
fix: EnableHolePunching enabled by default
1236ab4 
This fixes interop tests when Swarm.EnableHolePunching in go-ipfs is
enabled by default.

Ref. ipfs/kubo#8748
@lidel lidel mentioned this pull request Feb 22, 2022
Merged
@lidel lidel force-pushed the feat/dcutr-hole-punching-on-by-default branch from b3a0b98 to 4adc4d3 Compare February 22, 2022 16:38
@lidel lidel changed the title feat: Swarm.EnableHolePunching by default feat: EnableHolePunching by default Feb 22, 2022
@lidel lidel marked this pull request as ready for review February 22, 2022 17:08
@lidel lidel mentioned this pull request Feb 25, 2022
Closed
marten-seemann
marten-seemann previously requested changes Feb 25, 2022
View reviewed changes
Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

We don't have the relay discovery logic in libp2p yet. This PR should be blocked on that.

@mxinden mxinden mentioned this pull request Feb 25, 2022
Merged
4 tasks
@lidel lidel added the status/blocked Unable to be worked further until needs are met label Feb 28, 2022
@BigLep
Copy link
Contributor

BigLep commented Mar 1, 2022

@lidel : I'm game to talk about this more verbally, but are we comfortable turning this on by default? I'm worried we're doing large scale testing in production rather than pushing to have a way to verify this has no adverse effects in a controlled environment like testground.

@BigLep
Copy link
Contributor

BigLep commented Mar 1, 2022

We don't have the relay discovery logic in libp2p yet. This PR should be blocked on that.

@marten-seemann : is there a go-libp2p issue for this we can link to?

@BigLep
Copy link
Contributor

BigLep commented Mar 10, 2022 •
edited by lidel
Loading

Here is the issue: libp2p/go-libp2p#1351

I'll denote this issue as blocked on that.

@lidel
Copy link
Member Author

lidel commented Apr 19, 2022 •
edited
Loading

This PR needs #8868 to land first (or be merged into that one)
I applied changes here, keeping #8868 to be only about enabling hole punching by default.

@lidel lidel changed the base branch from master to update-libp2p-v019 April 19, 2022 19:41
@lidel lidel changed the title feat: EnableHolePunching by default refactor(config): cleanup relay handling Apr 19, 2022
lidel added a commit to ipfs/interop that referenced this pull request Apr 19, 2022
@lidel
fix: EnableHolePunching enabled by default
83eeed3 
This fixes interop tests when Swarm.EnableHolePunching in go-ipfs is
enabled by default.

Ref. ipfs/kubo#8748
lidel added a commit to ipfs/interop that referenced this pull request Apr 19, 2022
@lidel
fix: EnableHolePunching enabled by default
d138441 
This fixes interop tests when Swarm.EnableHolePunching in go-ipfs is
enabled by default.

Ref. ipfs/kubo#8748
@lidel lidel changed the title refactor(config): cleanup relay handling feat: EnableHolePunching by default Apr 19, 2022
@lidel lidel removed the status/blocked Unable to be worked further until needs are met label Apr 19, 2022
@lidel lidel self-assigned this Apr 19, 2022
@lidel lidel marked this pull request as draft April 19, 2022 21:46
@lidel lidel force-pushed the feat/dcutr-hole-punching-on-by-default branch from 5f3d63e to f63bbfd Compare April 19, 2022 21:49
lidel added a commit to ipfs/interop that referenced this pull request Apr 19, 2022
@lidel
fix: when hole punching is enabled by default (#443)
bda96fc 
This fixes interop tests when Swarm.EnableHolePunching in go-ipfs is
enabled by default.

Ref. ipfs/kubo#8748
@lidel lidel mentioned this pull request Apr 19, 2022
Merged
13 tasks
@lidel lidel force-pushed the update-libp2p-v019 branch 2 times, most recently from 2ec71d1 to 5c1a1a3 Compare April 26, 2022 16:55
@lidel lidel force-pushed the update-libp2p-v019 branch 2 times, most recently from 59c16c3 to 25950f3 Compare April 27, 2022 19:05
@lidel lidel force-pushed the feat/dcutr-hole-punching-on-by-default branch 2 times, most recently from a60e471 to bd6c571 Compare April 27, 2022 20:59
@lidel
Copy link
Member Author

lidel commented Apr 27, 2022 •
edited
Loading

Current status: blocked by one go-libp2p@v0.19.0 bug

@BigLep fysa this means we either need go-libp2p@v0.19.1 with the fix, or enabling hole punching needs to wait until 0.20.0.
(this is only blocking Swarm.EnableHolePunching=true from this PR, it does not block #8868)

Base automatically changed from update-libp2p-v019 to master April 28, 2022 15:13
@aschmahmann aschmahmann force-pushed the feat/dcutr-hole-punching-on-by-default branch from bd6c571 to 4e92f74 Compare May 3, 2022 14:14
@aschmahmann aschmahmann marked this pull request as ready for review May 3, 2022 14:15
aschmahmann
aschmahmann reviewed May 3, 2022
View reviewed changes
core/node/libp2p/relay.go
Comment on lines +78 to +81
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

from standup: add an info log here noting that hole punching was silently turned off

aschmahmann
aschmahmann reviewed May 3, 2022
View reviewed changes
core/node/libp2p/relay.go
if flag != config.Default {
log.Fatal("Failed to enable `Swarm.EnableHolePunching`, it requires `Swarm.RelayClient.Enabled` to be true.")
} else {
log.Info("HolePunching has been disabled due to the RelayClient being disabled.")
Copy link
Contributor

Choose a reason for hiding this comment

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

@lidel this comment was added per https://github.com/ipfs/go-ipfs/pull/8748/files#r863946425.

We're going to release the RC with this, but if you'd like to change the wording or have an objection to the logic we can revert during the RC process

@aschmahmann aschmahmann force-pushed the feat/dcutr-hole-punching-on-by-default branch from deb2a67 to 185971b Compare May 3, 2022 18:26
@aschmahmann aschmahmann enabled auto-merge May 3, 2022 18:27
@aschmahmann aschmahmann merged commit 126ebc4 into master May 3, 2022
@aschmahmann aschmahmann deleted the feat/dcutr-hole-punching-on-by-default branch May 3, 2022 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschmahmann aschmahmann aschmahmann approved these changes

@marten-seemann marten-seemann marten-seemann left review comments

Assignees

@lidel lidel

@aschmahmann aschmahmann

Labels
None yet
Projects
No open projects
IPFS Shipyard Team
Archived in project
Milestone
go-ipfs 0.13
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lidel @BigLep @marten-seemann @aschmahmann