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

autorelay: implement candidates sorted by RTT #2804

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

autorelay: implement candidates sorted by RTT #2804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions p2p/host/autorelay/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ type config struct {
setMinCandidates bool
// see WithMetricsTracer
metricsTracer MetricsTracer
// see WithFilterPublic
sortByRTT bool
}

var defaultConfig = config{
Expand All @@ -53,6 +55,7 @@ var defaultConfig = config{
desiredRelays: 2,
maxCandidateAge: 30 * time.Minute,
minInterval: 30 * time.Second,
sortByRTT: true,
}

var (
Expand Down Expand Up @@ -231,3 +234,11 @@ func WithMetricsTracer(mt MetricsTracer) Option {
return nil
}
}

// WithSortByRTT configure whether a ping speed test is required for the candidate; enable by default.
func WithSortByRTT(enable bool) Option {
return func(c *config) error {
c.sortByRTT = enable
return nil
}
}
37 changes: 31 additions & 6 deletions p2p/host/autorelay/relay_finder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"math/rand"
"sort"
"sync"
"time"

Expand All @@ -17,6 +18,7 @@ import (
"github.com/libp2p/go-libp2p/p2p/host/eventbus"
circuitv2 "github.com/libp2p/go-libp2p/p2p/protocol/circuitv2/client"
circuitv2_proto "github.com/libp2p/go-libp2p/p2p/protocol/circuitv2/proto"
"github.com/libp2p/go-libp2p/p2p/protocol/ping"

ma "github.com/multiformats/go-multiaddr"
manet "github.com/multiformats/go-multiaddr/net"
Expand All @@ -42,6 +44,7 @@ type candidate struct {
added time.Time
supportsRelayV2 bool
ai peer.AddrInfo
rtt time.Duration
}

// relayFinder is a Host that uses relays for connectivity when a NAT is detected.
Expand All @@ -60,6 +63,7 @@ type relayFinder struct {

candidateFound chan struct{} // receives every time we find a new relay candidate
candidateMx sync.Mutex
sortByRTT bool
candidates map[peer.ID]*candidate
backoff map[peer.ID]time.Time
maybeConnectToRelayTrigger chan struct{} // cap: 1
Expand Down Expand Up @@ -104,6 +108,7 @@ func newRelayFinder(host *basic.BasicHost, peerSource PeerSource, conf *config)
relays: make(map[peer.ID]*circuitv2.Reservation),
relayUpdated: make(chan struct{}, 1),
metricsTracer: &wrappedMetricsTracer{conf.metricsTracer},
sortByRTT: conf.sortByRTT,
}
}

Expand Down Expand Up @@ -447,10 +452,24 @@ func (rf *relayFinder) handleNewNode(ctx context.Context, pi peer.AddrInfo) (add
return false
}
log.Debugw("node supports relay protocol", "peer", pi.ID, "supports circuit v2", supportsV2)

// default set maxDuration
rtt := time.Duration(1<<63 - 1)
if rf.sortByRTT {
res := <-ping.Ping(ctx, rf.host, pi.ID)
if res.Error != nil {
log.Debugf("node %s don't supports ping protocol: %v", pi.ID, res.Error)
} else {
rtt = res.RTT
log.Debugf("node %s ping took: %s", pi.ID, res.RTT)
}
}

rf.addCandidate(&candidate{
added: rf.conf.clock.Now(),
ai: pi,
supportsRelayV2: supportsV2,
rtt: rtt,
})
rf.candidateMx.Unlock()
return true
Expand Down Expand Up @@ -562,7 +581,7 @@ func (rf *relayFinder) maybeConnectToRelay(ctx context.Context) {
rf.metricsTracer.ReservationRequestFinished(false, err)
continue
}
log.Debugw("adding new relay", "id", id)
log.Debugw("adding new relay", "id", id, "rtt", cand.rtt)
rf.relayMx.Lock()
rf.relays[id] = rsvp
numRelays := len(rf.relays)
Expand Down Expand Up @@ -699,11 +718,17 @@ func (rf *relayFinder) selectCandidates() []*candidate {
}
}

// TODO: better relay selection strategy; this just selects random relays,
// but we should probably use ping latency as the selection metric
rand.Shuffle(len(candidates), func(i, j int) {
candidates[i], candidates[j] = candidates[j], candidates[i]
})
// If the node speed test is enabled, the rtt will be sorted,
// otherwise the order will be randomly shuffled.
if rf.sortByRTT {
sort.Slice(candidates, func(i, j int) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be fuzzy, otherwise it can create hot spots in the network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be fuzzy, otherwise it can create hot spots in the network.

Sorry, I didn't quite get your point. Could you be a little more detailed?

Copy link
Contributor

Choose a reason for hiding this comment

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

fuzz the RTTs, if they are exact then well positioned nodrs in the network might become hot spots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is true that those high quality nodes in the network are preferred, but the node has already shown that he can improve the relay service for us when he accepted to be able to be our candidate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Vyzo. We don't want to introduce a change that could cause all well-behaved nodes to possibly DoS a well behaved relay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not enable rtt speed measurement by default, which should alleviate this problem.

Regarding the DoS issue, I think we should protect the server side instead of the client side. The libp2p code itself is open source, and anyone can change the behavior of the libp2p relay client, so restricting it here is useless for hackers who want to perform DoS attacks on our public network nodes.

Copy link
Collaborator

@MarcoPolo MarcoPolo May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's a difference between a malicious user doing something bad with nodes they control and us shipping code that causes every node to behave badly.

And even in the case that it does not actively hurt the server node, it is generally a better practice to distribute the load on the network than it is to create hot spots.

return candidates[i].rtt < candidates[j].rtt
})
} else {
rand.Shuffle(len(candidates), func(i, j int) {
candidates[i], candidates[j] = candidates[j], candidates[i]
})
}
return candidates
}

Expand Down
Loading