gossipsub v1.1: prune peer exchange by vyzo · Pull Request #234 · libp2p/go-libp2p-pubsub

vyzo · 2019-11-21T22:17:06Z

Adds support for peer exchange on prune; see #233
Depends on:

TBD:

Act on peer exchange when pruned
tests

vyzo · 2019-11-21T22:18:08Z

cc @whyrusleeping @ZenGround0

vyzo · 2019-11-23T15:09:44Z

Summoning @Stebalien @raulk @Kubuxu; this is ready for review.

Kubuxu

initial quick pass

aschmahmann

Looks pretty good to me, left a few comments to address.

Also, noting for posterity that this PR should resolve libp2p/specs#215 🎉

aschmahmann · 2019-11-24T23:11:29Z

 			gs.tracer.Prune(p, topic)
 			delete(peers, p)
 			gs.untagPeer(p, topic)
+			gs.addBackoff(p, topic)


👍 great!

However, we're not currently doing anything about a peer continuously chasing away other peers by sending a GRAFT. If during a GRAFT we could check if we're already over the limit and send pack a PRUNE that would largely resolve this case.

We don't want to do that because we will reject all new peers and they may not be able to form a connected mesh.
That's why it accepts the peer and resolves (with randomization) during the heartbeat.

Just to be clear, sending a PRUNE if we are over the limit is the wrong thing to do, because then the mesh can become full and fail to accept new peers.
(I considered it when designing the algorithm)

I get that it used to be that way, but is that still true now that we have peer exchange?

If I send back a PRUNE only when I have >Dhi peers and I prune (Dhi-D) peers and tell them about each other won't they always be able to join the mesh?

They probably will be able to do that, but I'd rather reshuffle the mesh in its entirety instead of rejecting new GRAFTs.

Think about a fully connected mesh where all peers are at D_hi -- (unlikely as it is) it won't accept new peers then, while GRAFTing and reshuffling will resolve the situation.

Ah, so IIUC this is really a timing problem right? Because if B is connected to C and then A tries to connect to B it's possible that when B PRUNEs A and C that A will send a GRAFT to C before C receives the PRUNE from B.

My concern with the current approach is that if A wants to GRAFT to B it can always keep spamming B and eventually it will get through, even though B has given A plenty of other peers to connect to. This is annoying since if a peer is "better" in some way (e.g. they are the primary publisher) then nodes might be selfish, however it's certainly not a deal-breaker and wasn't even resolvable until the Peer Exchange changes.

Since fixing this would likely require some protocol thought and is less important than the rest of the episub work, seems reasonable to push this further down the road. Would you like me to create an issue?

It could be an attack -- I don't think it can happen naturally.

Sure, we can create an issue to discuss further instead of it being lost in oblivion after the pr merges.

There's a recent win here! The decisions of which peers to retain and which to eject is now performed by evaluating the peer's score! 🎉

aschmahmann

LGTM

Stebalien

Mostly LGTM but I want to make sure we don't introduce a DoS vector.

Stebalien · 2019-12-05T15:40:43Z

+			gs.addBackoff(p, topic)
+			px := prune.GetPeers()
+			if len(px) > 0 {
+				gs.pxConnect(px)


I have two concerns:

What if a single peer sends a bunch of prunes? Could they cause us to launch a ton of goroutines (and crash)?

Can't a peer use this to trick us into connecting to a ton of (potentially non-useful) peers? We should only try connecting to the number of peers we actually need (one?).

Ideally, we'd stick these peers in a set of known peers for the topic, connecting to them as-needed whenever we're pruned or we disconnect from a peer.

We must be already grafted, we ignore prunes that don't correspond to a peer on our mesh. So a peer can't make us launch an arbitrary number of goroutines by sending us prunes; at most he can make us launch a single goroutine for each topic we belong to its mesh. Not much of a vector I think.

We could have a (legitimate) prune listing an arbitrary number of useless peers; we could limit the number of peers we connect to.

For 2 I added a check that limits the number of connections to at most GossipSubPrunePeers, so this should address the concern.
We do want more than 1 peer in general, to expand our potential peers as much as possible.

Got it. SGTM.

Ok, I slept on it and there is a DoS vector: A malicious peer could send us in sequence GRAFT/PRUNE and cause us to spawn a goroutine; it could simply be sitting there sending GRAFT/PRUNE ad infinum, causing us to spawn a goroutine for each pair.
Granted, the effect is limited in how many goroutines it can fit inside the 30s window for the connection timeout, but it's still nasty.
I will rewrite the connection logic to use a limited set of pending connections and goroutine-free scheduling.

Implemented a connection scheduler, which limits the number of max pending connections too.

Stebalien · 2019-12-05T15:42:54Z

 	toprune := make(map[peer.ID][]string)

+	// clean up expired backoffs
+	gs.clearBackoff()


This is probably fine, but we should monitor this.

That is, walking through each of these every heartbeat should be fine, but could get expensive in some edge cases.

We could do it every few heartbeats instead of every heartbeat.

Every heartbeat is probably fine, I'm just calling it out so we keep it in mind.

A way more efficient manner would be to use a ring buffer of peer slices, with nslots = BackoffPeriod/HeartbeatInterval. Then you track the tick count (monotonically increasing by 1), every heartbeat increments it by 1. And when clearing the backoff, you simply do currentTick mod nslots, and simply clear that slot. That's pretty efficient. The data for backoff would turn into map[string][][]peer.ID.

Stebalien

This works but can't we just have a buffered channel (selecting with default when writing to it)?

Technically, this works slightly better (de duplicates, etc.) but I'm not sure if the complexity is worth it.

vyzo · 2019-12-06T15:15:02Z

Actually you are right; we don't need this complexity, we can just use a sufficient buffer in the connect channel.
I'll reset the last commit and do it with the channel.

raulk

This looks pretty great. Approving to avoid stalling any longer, but I'd really appreciate these two items being addressed before merging (see comments):

Backoff data structure for more efficient clearing.
Buffered channel for connect attempts, and less connector goroutines.

Thanks, @vyzo!

We need to enhance the specs ASAP.

raulk · 2019-12-27T16:42:10Z

+		p := peer.ID(pi.PeerID)
+
+		_, connected := gs.peers[p]
+		if connected {


So we'll connect to PEX peers but we'll wait until the next heartbeat to rebalance the mesh. That's why we can safely skip topic members that we're already connected to, because we'll anyway consider them in the next heartbeat (as long as we remain connected to them). I think that's correct.

yup, do you want a comment here?

Ideally ;-)

raulk · 2019-12-27T16:53:54Z

 	toprune := make(map[peer.ID][]string)

+	// clean up expired backoffs
+	gs.clearBackoff()


A way more efficient manner would be to use a ring buffer of peer slices, with nslots = BackoffPeriod/HeartbeatInterval. Then you track the tick count (monotonically increasing by 1), every heartbeat increments it by 1. And when clearing the backoff, you simply do currentTick mod nslots, and simply clear that slot. That's pretty efficient. The data for backoff would turn into map[string][][]peer.ID.

raulk · 2019-12-27T16:57:23Z

+	GossipSubPruneBackoff = time.Minute
+
+	// number of active connection attempts for peers obtained through px
+	GossipSubConnectors = 16


Isn't this too much? 16 conn attempts at once, i.e. we'll try to connect to all peers recommended by the pruning one (as this value is equal to GossipSubPrunePeers.

I'd prefer if connect was a buffered channel, and we'd have less concurrent goroutines. Right now it's a bit hit or miss, e.g. if we got pruned from two topics at once, the connector goroutines will be saturated with the first batch, and all peers from the second batch will be dropped.

It is a buffered channel! But yes, we can certainly lower from 16, how about 8 or 4?

Max inflight dials FD limit is 160 by default. Assuming each peer has an average of 3 addresses, 16 connectors could make use 30% of our FD allowance. I'd scale this down to 8 by default, but I'd add an option so the app can increase/decrease it (it may also touch the swarm limits!).

Also, we will need some fairness heuristic here. If we're pruned from two or three topics at once, the first topic will get all slots, and the other two will be queued. Instead, we might want to balance peers from topics 1, 2, and 3 for quicker healing. Some form of priority queue could work well.

the hardening branch reduces this to 8.

raulk

We need to merge the pending PRs in go-libp2p-core and go-libp2p-peerstore before we merge this one. Those commit hashes in go.mod are nasty.

vyzo · 2019-12-28T11:34:26Z

Re: backoff data structure

I am not entirely convinced it is more efficient with the circular buffer, as we'd have to iterate through the entire ring to find if a peer is being backed off.
What we can do instead, which will improve efficiency, is to only clear backoffs once every few ticks (say every 10-15 ticks for the default 1min backoff).
How does that sound?

…e connection

…ied-addrs

vyzo · 2020-03-24T13:30:56Z

rebased on master

vyzo

alright, ready to merge.

vyzo · 2020-03-24T13:48:47Z

+	GossipSubPruneBackoff = time.Minute
+
+	// number of active connection attempts for peers obtained through px
+	GossipSubConnectors = 16


the hardening branch reduces this to 8.

vyzo requested review from Stebalien and raulk November 21, 2019 22:17

vyzo requested a review from Kubuxu November 21, 2019 22:18

vyzo added req:filecoin P0 Critical: Tackled by core team ASAP labels Nov 21, 2019

vyzo changed the title ~~[WIP] gossipsub v1.1: prune peer exchange~~ gossipsub v1.1: prune peer exchange Nov 23, 2019

Kubuxu reviewed Nov 24, 2019

View reviewed changes

Comment thread gossipsub.go

Comment thread gossipsub.go Outdated

aschmahmann requested changes Nov 24, 2019

View reviewed changes

vyzo mentioned this pull request Nov 25, 2019

Gossipsub mesh does not converge libp2p/specs#215

Closed

vyzo requested a review from aschmahmann November 25, 2019 12:16

aschmahmann approved these changes Nov 25, 2019

View reviewed changes

Stebalien requested changes Dec 5, 2019

View reviewed changes

vyzo requested a review from Stebalien December 5, 2019 16:53

vyzo force-pushed the feat/prune-px branch 2 times, most recently from acba90e to a09bca2 Compare December 5, 2019 17:22

Stebalien approved these changes Dec 5, 2019

View reviewed changes

Stebalien reviewed Dec 5, 2019

View reviewed changes

Comment thread gossipsub.go

Stebalien approved these changes Dec 5, 2019

View reviewed changes

Stebalien reviewed Dec 6, 2019

View reviewed changes

Comment thread gossipsub.go Outdated

Comment thread gossipsub.go Outdated

vyzo force-pushed the feat/prune-px branch from 52c15a3 to 7da9c75 Compare December 6, 2019 15:21

Stebalien approved these changes Dec 6, 2019

View reviewed changes

raulk approved these changes Dec 27, 2019

View reviewed changes

raulk reviewed Dec 27, 2019

View reviewed changes

This was referenced Jan 23, 2020

Update peer-exchange branch with PeerRecord api changes #257

Merged

Signed Peer Records libp2p/go-libp2p#776

Closed

vyzo mentioned this pull request Jan 30, 2020

Question regarding gossipsub #256

Closed

vyzo and others added 25 commits March 24, 2020 15:05

peer exchange on prune

e2ebf99

backoff grafting to peers that have pruned us

60f6b2f

connect to peers obtained through px

152ebc5

test prune px with a star topology

a9fdc41

trace peer exchange

9519116

extend star topology test to assert that no peer is left with a singl…

47b221d

…e connection

make connection timeout a variable, set for 30s (instead of 10s)

f42ce48

add limit to the number of peers to connect to from px

0e5a1ed

shuffle peers when limiting px set

1a261fe

don't spawn a goroutine for scheduling connections

7c03aa0

track changes to peer records in -core

895bcf6

update PR branch dependencies

8186906

fix import & var naming

22faa75

add missing continue to error case

c1831f0

renaming in error messages & local var

6a9e6d8

gomod: use go-libp2p-core@peer-records and go-libp2p-peerstore@certif…

082b8b9

…ied-addrs

protocol ID for gossipsub v1.1

7a80d74

peer exchange on prune

c65bd30

backoff grafting to peers that have pruned us

e3bd9fa

connect to peers obtained through px

1e152d2

add limit to the number of peers to connect to from px

a198f5f

shuffle peers when limiting px set

3d943c9

don't spawn a goroutine for scheduling connections

2066fcd

fix rebase artifacts

480e48f

gomod tidy

e86314f

vyzo force-pushed the feat/prune-px branch from 2ef6174 to e86314f Compare March 24, 2020 13:29

vyzo commented Mar 24, 2020

View reviewed changes

vyzo merged commit 7cafd84 into master Mar 24, 2020

vyzo deleted the feat/prune-px branch April 14, 2020 09:46

Conversation

vyzo commented Nov 21, 2019 • edited by daviddias Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vyzo commented Nov 21, 2019

Uh oh!

vyzo commented Nov 23, 2019

Uh oh!

Kubuxu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aschmahmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyzo Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyzo Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aschmahmann left a comment

Choose a reason for hiding this comment

Uh oh!

Stebalien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyzo Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vyzo Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Stebalien left a comment

vyzo commented Nov 21, 2019 •

edited by daviddias

Loading

vyzo Nov 25, 2019 •

edited

Loading

vyzo Nov 25, 2019 •

edited

Loading

vyzo Dec 5, 2019 •

edited

Loading

vyzo Dec 5, 2019 •

edited

Loading