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

avoid too-fast topic attestation subnet unsubscription/subscription #3025

Closed
wants to merge 4 commits into from

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Oct 23, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 23, 2021

Unit Test Results

     12 files  ±  0     744 suites  ±0   30m 56s ⏱️ - 3m 10s
1 469 tests +  5  1 467 ✔️ +  5  2 💤 ±0  0 ±0 
8 944 runs  +20  8 936 ✔️ +20  8 💤 ±0  0 ±0 

Results for commit d0bcb7a. ± Comparison against base commit b4d27b3.

♻️ This comment has been updated with latest results.

@arnetheduck
Copy link
Member

@Menduist does the prune backoff also apply to peers that we don't prune? Basically, when we receive an unsubscribe message, we should be removing the peers from the mesh implicitly, no? Another way to put this: is it required to send a prune message before unsubscribing?

@Menduist
Copy link
Contributor

There is no implicit prune:

The application can invoke LEAVE(topic) to unsubscribe from a topic. The router will inform the peers in mesh[topic] by sending them a PRUNE control message

@arnetheduck
Copy link
Member

arnetheduck commented Oct 29, 2021

And yet when they unsubscribe, we remove them from the mesh without thinking about backoff: https://github.com/status-im/nim-libp2p/blob/master/libp2p/protocols/pubsub/gossipsub.nim#L238

@Menduist
Copy link
Contributor

AFAICT, if they pruned us, we'll handle the backoff correctly, even if we receive the prune after the unsubscribe

So if they unsub without pruning (which is against the spec), we'll still clean them up, without backoff
If they unsub with pruning, we'll handle their backoff correctly no matter what

@arnetheduck
Copy link
Member

If they unsub with pruning, we'll handle their backoff correctly no matter what

well, this is a problem somewhere: either in the spec or in the implementation - it's basically a way to circumvent backoff which may be a good thing or a bad thing, depending on how you view it.. it's something to a) check in the major impls and b) raise in the libp2p spec repo, if it's not crystal clear from the spec

@arnetheduck
Copy link
Member

we have a similar thing in our code where we allow peers to enter our mesh without being subscribed - I think that's wrong, but likely a leftover from some attempt to deal with out-of-order messages - it's something that should never happen regardless, ie the sequence should always be subscribe -> graft -> prune -> unsbuscribe, with the prune being debatable - graft before subscribe however sounds like something that could be exploited / lead to weird meshes

@Menduist
Copy link
Contributor

Checked quickly, we seem on par with go-libp2p

Opened vacp2p/nim-libp2p#641 to follow this up more thoroughly

@tersec
Copy link
Contributor Author

tersec commented Dec 15, 2021

It's looking like this isn't the best way of accomplishing this goal. vacp2p/nim-libp2p#665 seems like the better approach.

@tersec tersec closed this Dec 15, 2021
@arnetheduck arnetheduck deleted the xtz branch January 24, 2022 07:32
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.

3 participants