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

autonat: don't change status on dial request refused #2225

Merged
merged 6 commits into from
Apr 5, 2023

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Mar 26, 2023

We don't want to update autonat reachability status from public to unknown or from private to unknown when peers reply to dial requests with status dial refused.

@sukunrt
Copy link
Member Author

sukunrt commented Mar 26, 2023

@marten-seemann
Another way to do this would be to keep a flag on autonat probeImmediately which would schedule the next probe immediately.
I can change this if you think that's a better approach

@sukunrt sukunrt marked this pull request as draft March 26, 2023 19:58
@sukunrt sukunrt marked this pull request as ready for review March 27, 2023 08:56
@marten-seemann marten-seemann mentioned this pull request Apr 5, 2023
21 tasks
Copy link
Contributor

@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.

Yes, I'd like to schedule the next attempt earlier. Not sure if a bool on the struct would be the nicest way to solve this though, can we just do this in the loop?

p2p/host/autonat/autonat.go Outdated Show resolved Hide resolved
// a dial may be refused for reasons like being rate limited. We don't want to change our NAT status based
// on this. We just schedule the next probe sooner
if as.confidence == maxConfidence {
as.confidence--
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Why are we decreasing the confidence when a dial is refused?
  2. I don't see any logic here to schedule the next probe sooner.

Copy link
Member Author

Choose a reason for hiding this comment

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

this hacky way is also used here
If confidence is not maximum the next probe will be done at retryInterval(1 minute) and not refreshTime(15minute)

I'll see if I can make that happen without keeping a bool around.

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 changed it to retry immediately. Is this approach better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a short delay? I’d like to avoid the situation where all servers are at their limit and clients just keep churning through their list.

Copy link
Member Author

Choose a reason for hiding this comment

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

a bool seems like the simplest way to achive this 😅
that's what I've implemented.

p2p/host/autonat/autonat.go Show resolved Hide resolved
@marten-seemann marten-seemann removed the request for review from MarcoPolo April 5, 2023 04:54
if !ok {
return
}
as.recordObservation(result)
if IsDialRefused(err) {
as.retryProbe = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just pass this as a parameter to scheduleProbe?

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 changed it to pass it as a parameter but why is this better?

scheduleProbe is already making the scheduling decision based on params set on the struct like as.lastInbound why handle this info separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually I try to keep as little members as possible, but you're right, there seems to be precedent here for not following this principle.

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 agree with your logic, it is better to have this state have as little visibility as possible. We can keep this and change the other "precedents" later when we touch that piece.

@marten-seemann marten-seemann merged commit 0a961d3 into libp2p:master Apr 5, 2023
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.

2 participants