This repository has been archived by the owner on May 11, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NAT Auto Discovery #1
NAT Auto Discovery #1
Changes from 37 commits
32e8ab9
70f7dd8
f3d9a24
cc058d6
ef097b5
6efad8f
2aa66e5
ea43bf5
00fb7e7
0377627
7fad996
9af8715
bc41c7a
dcbcfce
9efd0ec
aaaa90e
1562e1b
6d4bc41
fa14117
d16ca79
b1733eb
bb5cad4
cd7a875
7b3981e
cf04a09
7c097ed
5837cc5
56a0966
54fb466
66ca387
3abf9c7
3b679e0
1cba297
dd7c7a9
9ff7df3
0fdf1b0
46d352f
0a4e215
91c209c
00d2fea
8ea9f1b
d9a0d1a
aadb8db
d7f55b0
852f4e0
9c8ee52
b2c65b0
8d2e2ae
9ef3734
6a3a9cb
67bccae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What about "no nat"? Do we need that state?
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.
What does that state mean though? We have Uknown and Public -- no nat is equivalent to public.
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.
Ah, I was thinking:
(although we may not need to track the undialable case.
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's more of "dialable" or not "dialable".
Do we gain anything by knowing that there is no NAT whatsoever?
Note that the inference might be hard to make.
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'm afraid this black or white decision could yield erratic results, e.g. if you only have 1 active connection to an autonat peer, we're going to restrict our query to a single peer. I much rather have a minimum threshold we strive for, e.g. 5 peers, for resilience purposes, starting with peers we hold a connection to.
As it is, the
autodetect
does a round-robin, so no risk of establishing redundant connections if we shuffle the connected and non-connected sublists separately, i.e.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 am not sure I follow. The code tries to use an already existing connection purely to avoid creating unnecessary new connections.
Do you want to try multiple peers? And each peer multiple times?
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.
Currently, if we happen to be connected to 1 autonat peer only, we'll restrict ourselves to it. If it fails, we're out of luck. This makes us fragile, especially because we expect scarcity in autonat peers.
What I'm proposing is to target N peers (e.g. 5), preferring connected peers, and falling back to disconnected ones to fill up the slice. To avoid connected and unconnected peers getting mixed up in the shuffle, we keep track of the pivot index and shuffle both sublists separately.
Since
autodetect
is round-robin, it'll only resort to disconnected peers if the connected ones fail. This makes us more resilient overall.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.
That's fine, but do we want to dial more than one peers when we get a DIAL_ERROR?
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.
That's a good question. I'm not sure I have the answer. Right now we flag
NATStatusPrivate
on the first dial err, and abort. However, what if that peer is behind some kind of firewall (corporate, geographical, etc.)? Wouldn't it be better to corroborate with more observations?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 guess we could do a few more tries if we have more known autonat peers, but accept the failure if we don't have enough.
I would arbitrarily go for "3 times is enemy action".
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.
Yep. If we don't have enough, we'd defer to the next iteration. If by then we've found more autonat peers, with this new logic we'll query them even if not connected, and hence have a chance to improve our connectivity.
We detect "enemy action" on the receiving side through the throttling, no? (3 is fine for that)
That makes me realise that we should probably move peers who have sent us
E_DIAL_REFUSED
orE_INTERNAL_ERROR
to a blacklist, to avoid dwelling on them, and to be well-behaved.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.
That's probably too much complexity for marginal improvement :)
Also, I think I want some slightly more clever strategy for making multiple dial attempts -- if our nat status was unknown or public, then try 3 times.
If it was private, then a single failure should be enough to convince us.
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.
Implemented the "3 times is enemy action" strategy in aadb8db, with memory of past failures so that it stops asking multiple peers once it has enough confidence we are NATed.
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.
In d7f55b0 we ensure that we have at least 3 autonat peers in the candidate set, even when we are connected to less than that.
This uses the strategy you suggested for ordering.