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

Add ChannelSend trait to prep for lossy sender #4797

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

Conversation

cpubot
Copy link

@cpubot cpubot commented Feb 4, 2025

Problem

Gossip service consistently sees bursts in the millions of packets over the 2 second stat submission window.

2025-02-04-142734_hyprshot

The service currently uses crossbeam_channel::unbounded channels to buffer packets, which as the name implies, will unboundedly allocate space for incoming packets. The channel receivers will ultimately drop packets outside the bounds of MAX_GOSSIP_TRAFFIC.

Summary of Changes

This PR introduces a trait, ChannelSend, which abstracts over a send side implementation of a channel. This lays the foundation for a follow up PR that will introduce a notion of a "lossy" channel sender. This implementation will wrap a crossbeam_channel::bounded channel with a sender that will drop messages as channel capacity is reached rather than continuing to buffer messages until the receiver side is drained.

This is introduced behind a trait bound due to the genericity of the streamer crate and the multi-purpose use of its receiver implementation. In particular, this will allow gossip to use the aforementioned bounded / lossy sender without requiring other components like repair and fetch stage to use this forthcoming implementation.

@cpubot cpubot requested a review from alessandrod February 4, 2025 23:29
@cpubot cpubot requested a review from behzadnouri February 5, 2025 01:17
@behzadnouri
Copy link

The topic of bounded channels have been brought up multiple times before.

We prefer not to use bounded channels because they do not distinguish between good and bad packets, and drop (or back pressure) packets indiscriminately.

Gossip (or any other protocol) packets are not equal in terms of importance for the protocol or how much resources they need to process. Bounded channels are oblivious to that.

If we decide to drop packets we should also take into account stake associated with sender or receiver or CrdsValues. Again something bounded channels ignore.

Dropping packets indiscriminately also makes it easier for a spammer to censor good packets.

The preferred alternative is doing load shedding at a stage in the pipeline where we have better context how to prioritize packets and which ones to drop if overwhelmed.

This is all assuming the right thing to do here is to drop some packets. But I do not have enough context on what problem you are trying to solve, and I can't rule out the culprit or the remedy is something else.

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