-
Notifications
You must be signed in to change notification settings - Fork 944
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
feat(gossipsub): introduce backpressure #5595
base: master
Are you sure you want to change the base?
Conversation
6d406e1
to
5e349f5
Compare
5e349f5
to
00cde64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve left a few small comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add new tests for the introduced backpressure
slow_peer_weight: -0.2, | ||
slow_peer_threshold: 0.0, | ||
slow_peer_decay: 0.2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do these numbers come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @AgeManning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are made up as suggestions.
These are rough estimates from what we expect on real networks. I.e They are minor penalties relative to the behaviour penalty.
We didn't want to penalize peers too much, so having 50 failures to 1 behaviour penalty seemed reasonable.
Note that these are not on by default and it is generally recommended that users set their own scoring based on their own networks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't want to penalize peers too much, so having 50 failures to 1 behaviour penalty seemed reasonable.
OK, makes sense to me! Since this is defining the default for PeerScoreParams
, it should be a safe and documented value (even if not enabled by default). IMO a comment saying that the slow_peer_weight
is 50x lower than behaviour_penalty_weight
would help.
protocols/gossipsub/src/types.rs
Outdated
pub(crate) fn new(cap: usize) -> RpcSender { | ||
let (priority_sender, priority_receiver) = async_channel::unbounded(); | ||
let (non_priority_sender, non_priority_receiver) = async_channel::bounded(cap / 2); | ||
let len = Arc::new(AtomicUsize::new(0)); | ||
RpcSender { | ||
cap: cap / 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reasoning of using cap / 2
instead of cap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cause we have two channels priority
and non_priority
left a comment elaborating on that, thanks Gui
if self.len.load(Ordering::Relaxed) >= self.cap { | ||
return Err(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.cap
is only used in publish
. why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because publish
calls to self.priority_sender
which is unbounded, and we need it unbounded to send control messages such as GRAFT
, PRUNE
, SUBSCRIBE
, and UNSUBSCRIBE
. Left a comment explaining this, thanks Gui!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reviews Akihito and Gui!
protocols/gossipsub/src/types.rs
Outdated
pub(crate) fn new(cap: usize) -> RpcSender { | ||
let (priority_sender, priority_receiver) = async_channel::unbounded(); | ||
let (non_priority_sender, non_priority_receiver) = async_channel::bounded(cap / 2); | ||
let len = Arc::new(AtomicUsize::new(0)); | ||
RpcSender { | ||
cap: cap / 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cause we have two channels priority
and non_priority
left a comment elaborating on that, thanks Gui
if self.len.load(Ordering::Relaxed) >= self.cap { | ||
return Err(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because publish
calls to self.priority_sender
which is unbounded, and we need it unbounded to send control messages such as GRAFT
, PRUNE
, SUBSCRIBE
, and UNSUBSCRIBE
. Left a comment explaining this, thanks Gui!
Description
superseeds #4914 with some changes and improvements, namely:
Delay
forForward
andPublish
messages, messages that take more than the configured delay to be sent are discarded