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: add control over autonat #176

Merged
merged 1 commit into from
Nov 13, 2023
Merged

feat: add control over autonat #176

merged 1 commit into from
Nov 13, 2023

Conversation

nathanielc
Copy link
Collaborator

Prior to this change autonat would always be enabled. Now it can be disabled in cases where NAT is not being used.

Additionally previously an unbounded number of probes would be running concurrently for all observed addresses. When an dynamic outbound NAT is used this results in many probes. Now we limit to one active probe at a time and also track failed external address so we do not repeatedly attempt to probe them.

);
autonat.probe_address(info.observed_addr.clone());
// Only probe one address at a time.
if self.active_address_probe.is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if there's already a probe running, then it looks like we drop this addr on the floor. Should we insert it into a queue to probe later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about it but I don't think its needed. Everytime we connect to a peer it will report the observed address and trigger this code. So this logic is run very frequently. Additionally its typical to have only a single external address. In which case its likely that the in-progress address probe is the correct one. In cases where there are lots of different observed addresses its likely that NAT hasn't been setup correctly for the peer and so doesn't have an external address.

Maybe I could add this as a comment on in the code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think a comment clarifying this would be great

Copy link
Collaborator

@stbrody stbrody left a comment

Choose a reason for hiding this comment

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

lgtm

p2p/src/node.rs Outdated
// This logic is run very frequently because any new peer connection
// for a new observed address triggers this path. Its typical to have
// only a few external addresses, in which cases its likely that the
// in-progress address probe is one that will succeeded.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// in-progress address probe is one that will succeeded.
// in-progress address probe is one that will succeed.

Prior to this change autonat would always be enabled. Now it can be
disabled in cases where NAT is not being used.

Additionally previously an unbounded number of probes would be running
concurrently for all observed addresses. When an dynamic outbound NAT is
used this results in many probes. Now we limit to one active probe at a
time and also track failed external address so we do not repeatedly
attempt to probe them.
Copy link
Collaborator

@smrz2001 smrz2001 left a comment

Choose a reason for hiding this comment

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

:shipit:

@nathanielc nathanielc added this pull request to the merge queue Nov 13, 2023
Merged via the queue into main with commit 75e2137 Nov 13, 2023
4 checks passed
@nathanielc nathanielc deleted the fix/autonat-probe branch November 13, 2023 22:15
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.

3 participants