-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
implement relay v2 discovery #1368
Conversation
ff71c8b
to
9c878c3
Compare
9c878c3
to
d6fe423
Compare
d6fe423
to
90b8076
Compare
90b8076
to
7e767cb
Compare
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.
Looks mostly good, some minor comments.
In the more interesting stuff:
- we should do randomized exp. backoff
- i am not sure giving up after the max attempts is the right thing to do.
p2p/host/autorelay/relay_finder.go
Outdated
log.Debugw("moving candidate to backoff", "id", cand.ai.ID) | ||
rf.candidatesOnBackoff = append(rf.candidatesOnBackoff, &candidateOnBackoff{ | ||
candidate: *cand, | ||
nextConnAttempt: time.Now().Add(rf.conf.backoff), |
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 think we should do randomized exponential backoff here.
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.
Done.
1b1eeb0
to
44350ef
Compare
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.
looks good enough, modulo the comment.
default: | ||
// no space left in the channel. Drop the oldest entry. | ||
<-r.peerChanOut | ||
r.peerChanOut <- pi |
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.
isnt this racey with the consumer? I think we should also select here.
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.
Good catch!
This PR does not yet implement a utility function that would fill the
peerChan
with DHT peers. This will be added in a separate PR.Static Relay selection is a bit shaky when
minCandidates
is smaller than the number of relays, maybe we should just declare that we won't honor theWithMinCandidates
option when static relays are used.