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.

Sign up for GitHub

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

Jump to bottom

Integrate Limited Relays #1058

Closed
wants to merge 3 commits into from
Closed

Integrate Limited Relays #1058

wants to merge 3 commits into from

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Feb 19, 2021 •
edited
Loading

For #1039

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Feb 19, 2021 •
edited
Loading

@vyzo

  1. It's a breaking change (we don't want to support v1s anymore)
  2. Should we use the Reservation returned by the v2 client for anything at all in AutoRelay ?

For some reason, the compilation complains that it can't find the Relay field on the Reservation struct but this is an exported field. What's going wrong here ?

@aarshkshah1992 aarshkshah1992 mentioned this pull request Feb 19, 2021
Closed
23 tasks
@aarshkshah1992 aarshkshah1992 requested a review from vyzo February 19, 2021 08:01
@vyzo
Copy link
Contributor

vyzo commented Feb 19, 2021

  1. v2 is backwards compatible for dialing/listening to v1, so no it's not a breaking change (other than the relay options disappearing)
  2. The reservation provides two things you want to use (voucher aside):
    • the public relay addrs which you want to advertise (they are complete, with p2p-circuit in them so you just advertise them as they are)
    • the expiration of the reservation so that you know when to refresh; this is in unix time (seconds).

vyzo
vyzo reviewed Feb 19, 2021
View reviewed changes
config/config.go Outdated
@@ -25,8 +25,7 @@ import (

autonat "github.com/libp2p/go-libp2p-autonat"
blankhost "github.com/libp2p/go-libp2p-blankhost"
circuit "github.com/libp2p/go-libp2p-circuit"
discovery "github.com/libp2p/go-libp2p-discovery"
circuitv2client "github.com/libp2p/go-libp2p-circuit/v2/client"
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a huge name; just circuitv2 will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even just circuit will do in fact.

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.

config/config.go Outdated
@@ -92,7 +90,7 @@ type Config struct {

EnableAutoRelay bool
AutoNATConfig
StaticRelays []peer.AddrInfo
StaticV2Relays []peer.AddrInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to rename this variable.

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.

options.go Show resolved Hide resolved
p2p/host/relay/autorelay.go Outdated
}
// These are the known PL-operated V2 relays
// TODO Fill this up
var DefaultV2Relays = []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

just DefaultRelays.

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.

p2p/host/relay/autorelay.go Outdated
@@ -196,16 +191,13 @@ func (ar *AutoRelay) tryRelay(ctx context.Context, pi peer.AddrInfo) bool {
return false
}

ok, err := circuit.CanHop(ctx, ar.host, pi.ID)
rsv, err := circuitv2client.Reserve(ctx, ar.host, pi)
Copy link
Contributor

Choose a reason for hiding this comment

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

just circuit no need for comically long names. Also, rsv should be rsvp.

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.

p2p/host/relay/autorelay.go Outdated
// not a hop relay
return false
}
ar.host.Peerstore().AddAddrs(pi.ID, rsv.Relay.Addrs, peerstore.ConnectedAddrTTL)
Copy link
Contributor

Choose a reason for hiding this comment

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

no, this has changed in the v2 code.
The addresses that you get are your own relay addresses, and they are in the rsv(p).Addrs field.

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to schedule reservation refresh, use the Expiration field in the reservation to make the determination.

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.

@aarshkshah1992 aarshkshah1992 changed the title [WIP] Inegrate Limited Relays Integrate Limited Relays Feb 21, 2021
@aarshkshah1992
Copy link
Contributor Author

@vyzo

  1. Have addressed your feedback.
  2. We refresh reservations now if the Expiry TTL is above a reasonable threshold.

vyzo
vyzo requested changes Feb 23, 2021
View reviewed changes
p2p/host/relay/autorelay.go
Comment on lines +164 to +166
if rsvp.LimitDuration > MinTTLRefresh {
info.resfreshAt = time.Now().Add(rsvp.LimitDuration / 2)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is completely wrong -- the limit duration is the actual relay limit; you want to look at the Expiration which is when your reservation expires.

p2p/host/relay/autorelay.go
Comment on lines +263 to +265
if rsvp.LimitDuration > MinTTLRefresh {
rinfo.resfreshAt = time.Now().Add(rsvp.LimitDuration / 2)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here.

@marten-seemann marten-seemann mentioned this pull request Sep 6, 2021
Merged
@marten-seemann marten-seemann changed the base branch from feat/hole-punching-signalling to hole-punching September 8, 2021 16:52
@vyzo
Copy link
Contributor

vyzo commented Sep 10, 2021

closing as we'll have to do this in pieces, and probably have a transition period for autorelay where we support both.

@vyzo vyzo closed this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyzo vyzo vyzo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aarshkshah1992 @vyzo