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 backpressure #549

Merged
merged 7 commits into from
Nov 30, 2023

Conversation

AgeManning
Copy link
Member

Backpressure PR to our fork

Copy link
Member Author

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Yeah nice. Two things I noticed.

  1. We currently don't handle the cases where the queue's are full. I guess this is a future PR. I.e we don't handle the errors when we try to send.
  2. I know I'm missing something, but why are we using an unbounded channel for the priority queue and not just another bounded channel like we do with the non-priority?

let sender = self
.handler_send_queues
.get_mut(peer)
.expect("Peerid should exist");
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is true, as I recall trying to enforce conditions like this.

But we have to be absolutely sure that there is no code path where a peer can get added or removed from the peer_topics mapping and not the handler_send_queues.

If we are uncertain about this condition in any way (or are concerned a future dev may break this condition), we should log a crit instead imo.

Copy link
Member

Choose a reason for hiding this comment

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

a peer is added when a new ConnectionHandler is created and removed when the connection is closed. As we are using mpmc channels if that PeerId exists it has a respective ConnectionHandler (theoretically). If you prefer we can for now use a crit/error log instead

protocols/gossipsub/src/types.rs Outdated Show resolved Hide resolved
@AgeManning AgeManning merged commit a4a3f39 into sigp:lighthouse-gossipsub Nov 30, 2023
66 of 68 checks passed
@AgeManning
Copy link
Member Author

lol, was messing with gh cli, didn't mean to merge this early. But it works.

@jxs
Copy link
Member

jxs commented Dec 4, 2023

Yeah nice. Two things I noticed.

  1. We currently don't handle the cases where the queue's are full. I guess this is a future PR. I.e we don't handle the errors when we try to send.
  2. I know I'm missing something, but why are we using an unbounded channel for the priority queue and not just another bounded channel like we do with the non-priority?

1 - when the queue is full for every peer we return InsuficientPeers for individual peers we just drop the messages yeah, we can then downscore peers and when they reach 0 ban them, I think that's what you envisioned right?

2 - no worries :D, because DRAFT and PRUNE messages should still be sent if the queue is full, so I used an AtomicUsize for the cap of Publish messages

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.

2 participants