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

implement relay v2 discovery #1368

Merged
merged 9 commits into from
Apr 10, 2022
Merged

implement relay v2 discovery #1368

merged 9 commits into from
Apr 10, 2022

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Mar 30, 2022

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 the WithMinCandidates option when static relays are used.

Copy link
Contributor

@vyzo vyzo left a 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/autorelay.go Show resolved Hide resolved
p2p/host/autorelay/autorelay.go Show resolved Hide resolved
p2p/host/autorelay/options.go Show resolved Hide resolved
p2p/host/autorelay/options.go Outdated Show resolved Hide resolved
p2p/host/autorelay/options.go Outdated Show resolved Hide resolved
p2p/host/autorelay/relay_finder.go Outdated Show resolved Hide resolved
p2p/host/autorelay/relay_finder.go Show resolved Hide resolved
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),
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@vyzo vyzo left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@marten-seemann marten-seemann merged commit 1c6bfa2 into master Apr 10, 2022
@marten-seemann marten-seemann deleted the discover-v2-relays branch April 10, 2022 11:38
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