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

Gossipsub: Unsubscribe backoff #383

Merged
merged 2 commits into from Jun 3, 2022
Merged

Gossipsub: Unsubscribe backoff #383

merged 2 commits into from Jun 3, 2022

Conversation

Menduist
Copy link
Contributor

@Menduist Menduist commented Dec 14, 2021

In eth2, you can't really be sure at which point you will have to subscribe from a topic.
So you when unsubscribe, you may have to resubscribe in the next 60 seconds

This causes a clash with the backoff, since every peer you had in your mesh when you unsubbed is now in your backoff, and forming an healthy mesh becomes tricky.

This is a bit of a corner case, but adding a setting to tune to unsub backoff really helps with this case, and doesn't cost anything, since if you stay out of the topic, you shouldn't get GRAFTed anyway.

We implemented it in nim-libp2p: vacp2p/nim-libp2p#665
which drastically improved mesh health on our busy test nodes (2500 validators, which in eth2 term is stupidly big, but that's where the issue is most visible) (each validator causes some sub/unsub):
image

@mxinden
Copy link
Member

mxinden commented Dec 15, 2021

@vyzo can you take a look here?

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.

Sounds good to me, modulo the comment.

Does anyone from the eth2 team care for a pr in go-libp2p-pubsub?

Also, can you please bump the spec version with an appropriate -r and timestamp.

@@ -105,6 +105,10 @@ within the backoff period and extend it.
When a peer tries to regraft too early, the pruning peer may apply a behavioural penalty
for the action, and penalize the peer through P₇ (see [Peer Scoring](#peer-scoring) below).

When unsubscribing from a topic, the backoff period should be finished before subscribing to
the topic again, otherwise a healthy mesh will be difficult to reach.
A specific backoff period for unsubscribing should be set accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean "resubscribing" 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.

The UnsubBackoff is sent to peers when we unsubscribe to allow us to resubscribe before the full PruneBackoff

So while this is meant to allow fast resub, I called it the unsubbackoff because why send it during unsub

It tried to explain this better in the doc, lgty?

Copy link
Contributor

Choose a reason for hiding this comment

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

lg, thanks.

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.

LGTM, but can we hold on merging until we have implementation for go and rust?

@mxinden
Copy link
Member

mxinden commented Dec 15, 2021

@AgeManning @divagant-martian thoughts on this as the Rust Gossipsub experts?

@AgeManning
Copy link
Contributor

Yep. This lgtm. We'll make a PR for rust gossipsub.

mxinden added a commit to libp2p/rust-libp2p that referenced this pull request Jan 17, 2022
Implements the changes specified by
libp2p/specs#383.

Co-authored-by: Max Inden <mail@max-inden.de>
@mxinden
Copy link
Member

mxinden commented Jan 17, 2022

For the record, this is now implemented in rust-libp2p via libp2p/rust-libp2p#2403 thanks to @divagant-martian.

@mxinden
Copy link
Member

mxinden commented Mar 2, 2022

LGTM, but can we hold on merging until we have implementation for go and rust?

@vyzo could you tackle the Golang side?

@vyzo
Copy link
Contributor

vyzo commented Mar 2, 2022

I'll try to make some time for this. Any takers?

@mxinden
Copy link
Member

mxinden commented Apr 8, 2022

@vyzo friendly ping. I don't think anyone else from the Go side picked this up in the meantime.

@vyzo
Copy link
Contributor

vyzo commented May 31, 2022

As far as I can tell, this issue has been addressed in libp2p/go-libp2p-pubsub#473.

@Menduist
Copy link
Contributor Author

Doesn't look like it, the goal of this spec is to have a tunable, different backoff when unsubscribing from a topic
The PR you linked reuse the same backoff setting as a regular prune AFAICT

@vyzo
Copy link
Contributor

vyzo commented May 31, 2022

ok, will make this configurable.

@MarcoPolo
Copy link
Contributor

@vyzo if you haven't started already I can take. Seems pretty straightforward.

@vyzo
Copy link
Contributor

vyzo commented Jun 1, 2022

Sure, go ahead!

@MarcoPolo
Copy link
Contributor

This is now implemented in Rust, Nim, and Go. I think we can merge this. Thanks all!

@vyzo vyzo merged commit 6e10931 into libp2p:master Jun 3, 2022
santos227 added a commit to santos227/rustlib that referenced this pull request Jun 20, 2022
Implements the changes specified by
libp2p/specs#383.

Co-authored-by: Max Inden <mail@max-inden.de>
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.

None yet

5 participants