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

fix: enable dctur when interface address is public #2931

Merged
merged 5 commits into from
Aug 28, 2024
Merged

Conversation

2color
Copy link
Contributor

@2color 2color commented Aug 21, 2024

What's in this PR

This change attempts to fix #2913.

In essence, it enables the dctur protocol, when a peer has a public IP (if it's bound to an interface with a public IP) without having to wait for an identify response (which would then make the public address visible via OwnObservedAddrs()).

Open questions

@2color 2color requested review from sukunrt and MarcoPolo August 21, 2024 10:19
Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

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

Thanks @2color

p2p/protocol/holepunch/svc.go Outdated Show resolved Hide resolved
p2p/protocol/holepunch/svc.go Outdated Show resolved Hide resolved
@MarcoPolo
Copy link
Collaborator

I have some questions about the issue. Let's continue the discussion there. But one quick thing:

The OwnObservedAddrs() function in the identify service (which we rely on in the holepunch service) seems to not return observed addresses if they aren't dialable. Why is that?

What do you mean by dialable? By not dialable do you mean that the host will drop unsolicited incoming packets?

@2color
Copy link
Contributor Author

2color commented Aug 22, 2024

By not dialable do you mean that the host will drop unsolicited incoming packets?

Exactly. It binds to a port on an interface with a public IP, however the firewall blocks incoming connections on that port.

@2color
Copy link
Contributor Author

2color commented Aug 23, 2024

I've addressed your feedback @sukunrt.

@2color 2color changed the title fix: allow punching undialable host public ip fix: enable dctur when interface address is public Aug 26, 2024
@lidel lidel mentioned this pull request Aug 26, 2024
32 tasks
@2color 2color requested a review from sukunrt August 27, 2024 07:42
p2p/protocol/holepunch/svc.go Outdated Show resolved Hide resolved
@sukunrt
Copy link
Member

sukunrt commented Aug 27, 2024

@MarcoPolo The change looks reasonable to me. If the node does have access to its own public address there's no reason to depend on identify for address discovery for hole punching.

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

This change makes sense. After addressing @sukunrt's comment this should be good to merge

@2color 2color requested a review from sukunrt August 28, 2024 12:49
@lidel lidel mentioned this pull request Aug 28, 2024
34 tasks
@MarcoPolo MarcoPolo merged commit 3deee92 into master Aug 28, 2024
11 checks passed
sukunrt pushed a commit that referenced this pull request Aug 30, 2024
* fix: allow punching undialable host public ip

fixes #2913

* chore: use interface listen addrs to enable dctur

* fix: filter public addresses

* chore: remove unused function

* chore: formatting

---------

Co-authored-by: Daniel N <2color@users.noreply.github.com>
@2color 2color deleted the fix-dctur-pubip branch September 3, 2024 10:00
MarcoPolo pushed a commit that referenced this pull request Sep 4, 2024
* fix: allow punching undialable host public ip

fixes #2913

* chore: use interface listen addrs to enable dctur

* fix: filter public addresses

* chore: remove unused function

* chore: formatting

---------

Co-authored-by: Daniel N <2color@users.noreply.github.com>
@MarcoPolo MarcoPolo mentioned this pull request Oct 22, 2024
30 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.

QUIC/WebTransport Transport sometimes uses the wrong socket to dial out.
4 participants