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

feat: drop msgs to be relayed waiting for too long in the queue #1017

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

diegomrsantos
Copy link
Contributor

@diegomrsantos diegomrsantos commented Jan 31, 2024

This PR adds the possibility of configuring the maximum time a message can stay in the non-priority queue and still be considered relevant to be sent to the respective remote peer. If the message has been waiting for this duration or longer at the time is dequeued it will be discarded and not sent.

@diegomrsantos diegomrsantos changed the base branch from unstable to forward-msgs-non-priority-v2 January 31, 2024 17:41
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b16ec00) 83.27% compared to head (6484d3f) 83.24%.
Report is 3 commits behind head on forward-msgs-non-priority-v2.

Additional details and impacted files

Impacted file tree graph

@@                       Coverage Diff                        @@
##           forward-msgs-non-priority-v2    #1017      +/-   ##
================================================================
- Coverage                         83.27%   83.24%   -0.03%     
================================================================
  Files                                91       91              
  Lines                             15428    15438      +10     
================================================================
+ Hits                              12847    12852       +5     
- Misses                             2581     2586       +5     
Files Coverage Δ
libp2p/protocols/pubsub/gossipsub.nim 87.87% <100.00%> (-0.35%) ⬇️
libp2p/protocols/pubsub/pubsubpeer.nim 94.11% <90.00%> (+1.01%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Collaborator

@AlejandroCabeza AlejandroCabeza left a comment

Choose a reason for hiding this comment

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

At first glance it LGTM.
Maybe it'd be good to take into account Ludovic's opinion, given he's more familiar with the repo?

Copy link
Collaborator

@kaiserd kaiserd left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good.

Is there a reference as to why 1s was chosen as the default.
How do other implementations / the spec handle this?

If you did not do this yet, I suggest first testing the base PR in a testnet, followed by another run with this PR merged into the base PR forward-msgs-non-priority-v2

@diegomrsantos
Copy link
Contributor Author

diegomrsantos commented Feb 1, 2024

@kaiserd I've been testing all PRs running a local Nimbus client on Holesky. Everything seems fine. The only implementation with this feature I'm aware of is rust-libp2p and last time I checked they were testing it on a branch sigp/rust-libp2p#555.

They use 500ms as the default value. I tried it first but I saw a large number of messages being dropped. I believe this value is network/application-specific and could be hard to guess a default value. 1s was just an initial attempt, I welcome feedback here.

@diegomrsantos diegomrsantos force-pushed the forward-msgs-non-priority-v2 branch from 9fe2ec6 to 5b48aa4 Compare February 1, 2024 21:15
@diegomrsantos
Copy link
Contributor Author

Those are the other features they implemented sigp/lighthouse#4918 (comment)

@diegomrsantos diegomrsantos changed the title feat: drop old relayed msgs feat: drop relayed msgs waiting too long in the queue Feb 1, 2024
@diegomrsantos diegomrsantos changed the title feat: drop relayed msgs waiting too long in the queue feat: drop relayed msgs waiting for too long in the queue Feb 1, 2024
@diegomrsantos diegomrsantos changed the title feat: drop relayed msgs waiting for too long in the queue feat: drop msgs to br relayed waiting for too long in the queue Feb 1, 2024
@diegomrsantos diegomrsantos changed the title feat: drop msgs to br relayed waiting for too long in the queue feat: drop msgs to be relayed waiting for too long in the queue Feb 1, 2024
@diegomrsantos diegomrsantos changed the base branch from forward-msgs-non-priority-v2 to unstable February 1, 2024 21:50
@diegomrsantos diegomrsantos changed the base branch from unstable to forward-msgs-non-priority-v2 February 1, 2024 21:50
@diegomrsantos diegomrsantos force-pushed the drop-old-relayed-msgs branch 2 times, most recently from 2c37aa4 to 311893e Compare February 1, 2024 21:59
@diegomrsantos diegomrsantos force-pushed the forward-msgs-non-priority-v2 branch from 826ce1b to 07cab43 Compare February 9, 2024 11:58
@diegomrsantos diegomrsantos marked this pull request as ready for review February 13, 2024 12:35
@diegomrsantos diegomrsantos self-assigned this Feb 14, 2024
Base automatically changed from forward-msgs-non-priority-v2 to unstable February 16, 2024 09:54
@diegomrsantos diegomrsantos marked this pull request as draft May 29, 2024 09:58
@kaiserd
Copy link
Collaborator

kaiserd commented May 29, 2024

We need further investigation here.
Let's move this back to draft state for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Experimental
Development

Successfully merging this pull request may close these issues.

3 participants