basichost: use autonatv2 to verify reachability#3231
Merged
Conversation
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
8 times, most recently
from
01ace3a to
44a53e9
Compare
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
44a53e9 to
4020bb4
Compare
4 tasks
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
4 times, most recently
from
6c3df6a to
fb5c929
Compare
sukunrt
requested review from
MarcoPolo and
guillaumemichel
and removed request for
MarcoPolo and
guillaumemichel
This introduces `addrsReachabilityTracker` that tracks reachability on a set of addresses. It probes reachability for addresses periodically and has an exponential backoff in case there are too many errors or we don't have any valid autonatv2 peer. There's no smartness in the address selection logic currently. We just test all provided addresses. It also doesn't use the addresses provided by `AddrsFactory`, so currently there's no way to get a user provided address tested for reachability, something that would be a problem for dns addresses. I intend to introduce an alternative to `AddrsFactory`, something like, `AnnounceAddrs(addrs []ma.Multiaddr)` that's just appended to the set of addresses that we have, and check reachability for those addresses. There's only one method exposed in the BasicHost right now that's `ReachableAddrs() []ma.Multiaddr` that returns the host's reachable addrs. Users can also use the event `EvtHostReachableAddrsChanged` to be notified when any addrs reachability changes.
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
fb5c929 to
818a700
Compare
MarcoPolo
reviewed
Apr 1, 2025
Collaborator
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
I took a glance, but this is a lot. I get that the core logic is pretty straightforward which is nice, but there's a lot of plumbing required. It might be nicer if this weren't in the basic host package so it could be evaluated along a stricter API boundary.
It would also be nice if we could add some fuzz/invariant tests to the reachability tracker.
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
2 times, most recently
from
5c32430 to
abf1bdf
Compare
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
abf1bdf to
0b46ec6
Compare
MarcoPolo
requested changes
Apr 29, 2025
Collaborator
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
After these comments, I think this is good! This is a lot of plumbing. Thanks for being careful about it and adding a lot of tests :)
| // - context is completed | ||
| // - there are too many consecutive failures from the client | ||
| // - the client has no valid peers to probe | ||
| func runProbes(ctx context.Context, concurrency int, addrsTracker *probeManager, client autonatv2Client) bool { |
Collaborator
There was a problem hiding this comment.
This feels like it should be a method on addrsReachabilityTracker after
probeManager is consolidated to it.
Collaborator
There was a problem hiding this comment.
Okay, can we make this a method on probeManager at least?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
5 times, most recently
from
104d6a0 to
44c871a
Compare
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
44c871a to
9f7bf58
Compare
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
9f7bf58 to
1f0f310
Compare
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
1f0f310 to
d048ba7
Compare
MarcoPolo
reviewed
May 28, 2025
| // - context is completed | ||
| // - there are too many consecutive failures from the client | ||
| // - the client has no valid peers to probe | ||
| func runProbes(ctx context.Context, concurrency int, addrsTracker *probeManager, client autonatv2Client) bool { |
Collaborator
There was a problem hiding this comment.
Okay, can we make this a method on probeManager at least?
MarcoPolo
approved these changes
May 28, 2025
Collaborator
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
Minor things, but looks good. Feel free to merge after addressing the nits.
sukunrt
force-pushed
the
push-wlpnwouzykwp
branch
from
cf01834 to
3beeed0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This introduces
addrsReachabilityTrackerthat tracks reachability ona set of addresses. It probes reachability for addresses periodically
and has an exponential backoff in case there are too many errors
or we don't have any valid autonatv2 peer.
There's no smartness in the address selection logic currently. We just
test all provided addresses. It also doesn't use the addresses provided
by
AddrsFactory, so currently there's no way to get a user providedaddress tested for reachability, something that would be a problem for
dns addresses. I intend to introduce an alternative to
AddrsFactory, something like,AnnounceAddrs(addrs []ma.Multiaddr)that's just appended to the set of addresses that we have, and check
reachability for those addresses.
There's only one method exposed in the BasicHost right now that's
ReachableAddrs() []ma.Multiaddrthat returns the host's reachableaddrs. Users can also use the event
EvtHostReachableAddrsChangedto be notified when any addrs reachability changes.