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

Limited Forwarding (delayed elimination of peers from which message/idontwant is received) #1027

Open
wants to merge 1 commit into
base: p2p-research
Choose a base branch
from

Conversation

ufarooqstatus
Copy link
Collaborator

Remove peers from the relay (ToSendPeers) list, from which message or idontwant is received. We delay this action till sendMsg to increase such peer count.

…ntwant is received. We delay this action till sendMsg to increase such peers
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

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

Comparison is base (fe4ff79) 83.28% compared to head (3fb83b8) 83.31%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           p2p-research    #1027      +/-   ##
================================================
+ Coverage         83.28%   83.31%   +0.02%     
================================================
  Files                91       91              
  Lines             15442    15454      +12     
================================================
+ Hits              12861    12875      +14     
+ Misses             2581     2579       -2     
Files Coverage Δ
libp2p/protocols/pubsub/pubsub.nim 85.44% <100.00%> (ø)
libp2p/protocols/pubsub/gossipsub.nim 88.07% <66.66%> (-0.15%) ⬇️
libp2p/protocols/pubsub/pubsubpeer.nim 93.72% <93.75%> (+0.22%) ⬆️

... and 1 file with indirect coverage changes

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.

Thank you. Left some comments, will give it a more detailed look at a later point.

@@ -497,6 +497,9 @@ method rpcHandler*(g: GossipSub,

libp2p_gossipsub_duplicate.inc()

if msg.data.len > msgId.len * 10: #Dont relay to the peers from which we already received (We just do it for large messages)
peer.heDontWants[^1].incl(msgId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be renamed to notDesiredMessageIDs or something similar.

@@ -497,6 +497,9 @@ method rpcHandler*(g: GossipSub,

libp2p_gossipsub_duplicate.inc()

if msg.data.len > msgId.len * 10: #Dont relay to the peers from which we already received (We just do it for large messages)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is misleading. Only the "We just do it for large messages" applies to this line.
I'd move the first part one line below.

Also, I'd add the same comment as above:

    # IDontWant is only worth it if the message is substantially
    # bigger than the messageId

@arnetheduck
Copy link
Contributor

this is a good idea - in fact, it should be extended so that it also skips messages that were sent by the other node "in full" while queued.

Basically, a peer can tell us they have the message already in two ways: by IDONTWANT and by sending the message in its entirety.

The latter is quite possible: because low-prio messages are the ones being forwarded across the network in bursts, it's possible we receive (a duplicate of) it from the peer while sending some other message.

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

Successfully merging this pull request may close these issues.

3 participants