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

fix: autorelay: treat static relays as just another peer source #1875

Merged
merged 3 commits into from
Nov 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions p2p/host/autorelay/autorelay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,3 +456,37 @@ func TestIncorrectInit(t *testing.T) {
}()
_ = newPrivateNode(t)
}

func TestReconnectToStaticRelays(t *testing.T) {
cl := clock.NewMock()
var staticRelays []peer.AddrInfo
const numStaticRelays = 1
relays := make([]host.Host, 0, numStaticRelays)
for i := 0; i < numStaticRelays; i++ {
r := newRelay(t)
t.Cleanup(func() { r.Close() })
relays = append(relays, r)
staticRelays = append(staticRelays, peer.AddrInfo{ID: r.ID(), Addrs: r.Addrs()})
}

h := newPrivateNode(t,
autorelay.WithStaticRelays(staticRelays),
autorelay.WithClock(cl),
)

defer h.Close()

cl.Add(time.Minute)
require.Eventually(t, func() bool { return numRelays(h) == 1 }, 10*time.Second, 50*time.Millisecond)

relaysInUse := usedRelays(h)
oldRelay := relaysInUse[0]
for _, r := range relays {
if r.ID() == oldRelay {
r.Network().ClosePeer(h.ID())
}
}

cl.Add(time.Hour)
require.Eventually(t, func() bool { return numRelays(h) == 1 }, 10*time.Second, 100*time.Millisecond)
}
40 changes: 22 additions & 18 deletions p2p/host/autorelay/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ type config struct {
clock clock.Clock
peerSource func(ctx context.Context, num int) <-chan peer.AddrInfo
// minimum interval used to call the peerSource callback
minInterval time.Duration
staticRelays []peer.AddrInfo
minInterval time.Duration
// see WithMinCandidates
minCandidates int
// see WithMaxCandidates
Expand Down Expand Up @@ -45,8 +44,7 @@ var defaultConfig = config{
}

var (
errStaticRelaysMinCandidates = errors.New("cannot use WithMinCandidates and WithStaticRelays")
errStaticRelaysPeerSource = errors.New("cannot use WithPeerSource and WithStaticRelays")
errAlreadyHavePeerSource = errors.New("can only use a single WithPeerSource or WithStaticRelays")
)

// DefaultRelays are the known PL-operated v1 relays; will be decommissioned in 2022.
Expand Down Expand Up @@ -75,17 +73,26 @@ type Option func(*config) error

func WithStaticRelays(static []peer.AddrInfo) Option {
return func(c *config) error {
if c.setMinCandidates {
return errStaticRelaysMinCandidates
}
if c.peerSource != nil {
return errStaticRelaysPeerSource
}
if len(c.staticRelays) > 0 {
return errors.New("can't set static relays, static relays already configured")
return errAlreadyHavePeerSource
}
c.minCandidates = len(static)
c.staticRelays = static

WithPeerSource(func(ctx context.Context, numPeers int) <-chan peer.AddrInfo {
if len(static) < numPeers {
numPeers = len(static)
}
Comment on lines +81 to +83
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation for WithPeerSource says:

Implementations must send at most numPeers, and close the channel when they don't intend to provide any more peers.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use WithMaxCandidaters to make sure that the relayFinder is asking us for enough peers. We should probably also add some documentation that there's little point in providing a huge number of static relays.

Copy link
Collaborator Author

@MarcoPolo MarcoPolo Nov 11, 2022

Choose a reason for hiding this comment

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

This check is to make sure we send at most numPeers. If we don’t have numPeers we’ll only send what we have. Essentially the min(len(static),numPeers)).

I debated using with WithMaxCandidaters, but your right I’ll add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason I thought the if was the other way round (> instead of <). Sorry for that!

c := make(chan peer.AddrInfo, numPeers)
defer close(c)

for i := 0; i < numPeers; i++ {
c <- static[i]
}
return c
}, 30*time.Second)(c)
WithMinCandidates(len(static))(c)
WithMaxCandidates(len(static))(c)
WithNumRelays(len(static))(c)

return nil
}
}
Expand All @@ -107,8 +114,8 @@ func WithDefaultStaticRelays() Option {
// If the channel is canceled you MUST close the output channel at some point.
func WithPeerSource(f func(ctx context.Context, numPeers int) <-chan peer.AddrInfo, minInterval time.Duration) Option {
return func(c *config) error {
if len(c.staticRelays) > 0 {
return errStaticRelaysPeerSource
if c.peerSource != nil {
return errAlreadyHavePeerSource
}
c.peerSource = f
c.minInterval = minInterval
Expand Down Expand Up @@ -140,9 +147,6 @@ func WithMaxCandidates(n int) Option {
// This is to make sure that we don't just randomly connect to the first candidate that we discover.
func WithMinCandidates(n int) Option {
return func(c *config) error {
if len(c.staticRelays) > 0 {
return errStaticRelaysMinCandidates
}
if n > c.maxCandidates {
n = c.maxCandidates
}
Expand Down
43 changes: 7 additions & 36 deletions p2p/host/autorelay/relay_finder.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ type relayFinder struct {
}

func newRelayFinder(host *basic.BasicHost, peerSource func(context.Context, int) <-chan peer.AddrInfo, conf *config) *relayFinder {
if peerSource == nil && len(conf.staticRelays) == 0 {
if peerSource == nil {
panic("Can not create a new relayFinder. Need a Peer Source fn or a list of static relays. Refer to the documentation around `libp2p.EnableAutoRelay`")
}

Expand All @@ -103,19 +103,11 @@ func newRelayFinder(host *basic.BasicHost, peerSource func(context.Context, int)
}

func (rf *relayFinder) background(ctx context.Context) {
if rf.usesStaticRelay() {
rf.refCount.Add(1)
go func() {
defer rf.refCount.Done()
rf.handleStaticRelays(ctx)
}()
} else {
rf.refCount.Add(1)
go func() {
defer rf.refCount.Done()
rf.findNodes(ctx)
}()
}
rf.refCount.Add(1)
go func() {
defer rf.refCount.Done()
rf.findNodes(ctx)
}()

rf.refCount.Add(1)
go func() {
Expand Down Expand Up @@ -274,23 +266,6 @@ func (rf *relayFinder) findNodes(ctx context.Context) {
}
}

func (rf *relayFinder) handleStaticRelays(ctx context.Context) {
sem := make(chan struct{}, 4)
var wg sync.WaitGroup
wg.Add(len(rf.conf.staticRelays))
for _, pi := range rf.conf.staticRelays {
sem <- struct{}{}
go func(pi peer.AddrInfo) {
defer wg.Done()
defer func() { <-sem }()
rf.handleNewNode(ctx, pi)
}(pi)
}
wg.Wait()
log.Debug("processed all static relays")
rf.notifyNewCandidate()
}

func (rf *relayFinder) notifyMaybeConnectToRelay() {
select {
case rf.maybeConnectToRelayTrigger <- struct{}{}:
Expand Down Expand Up @@ -450,7 +425,7 @@ func (rf *relayFinder) maybeConnectToRelay(ctx context.Context) {
}

rf.candidateMx.Lock()
if !rf.usesStaticRelay() && len(rf.relays) == 0 && len(rf.candidates) < rf.conf.minCandidates && rf.conf.clock.Since(rf.bootTime) < rf.conf.bootDelay {
if len(rf.relays) == 0 && len(rf.candidates) < rf.conf.minCandidates && rf.conf.clock.Since(rf.bootTime) < rf.conf.bootDelay {
// During the startup phase, we don't want to connect to the first candidate that we find.
// Instead, we wait until we've found at least minCandidates, and then select the best of those.
// However, if that takes too long (longer than bootDelay), we still go ahead.
Expand Down Expand Up @@ -643,10 +618,6 @@ func (rf *relayFinder) relayAddrs(addrs []ma.Multiaddr) []ma.Multiaddr {
return raddrs
}

func (rf *relayFinder) usesStaticRelay() bool {
return len(rf.conf.staticRelays) > 0
}

func (rf *relayFinder) Start() error {
rf.ctxCancelMx.Lock()
defer rf.ctxCancelMx.Unlock()
Expand Down