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: Better IWANT handling #875

Merged
merged 3 commits into from
Apr 3, 2023
Merged

GossipSub: Better IWANT handling #875

merged 3 commits into from
Apr 3, 2023

Conversation

Menduist
Copy link
Contributor

Currently, IWANTs are only limited by the budget (25 messages per heartbeat)
That's quite random, and causes some issues when peer are asking for more than 25 messages per HB

This PR changes the spam protection to only allow one IWANT per message sent in each IHAVE
In the future, we could also avoid sending an IHAVE twice for the same message. This PR opens the door to that

else:
let deIwants = iwants.deduplicate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've remove deduplication since this now happens naturally, though the computation is a tad more expensive

@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #875 (129e54a) into unstable (8d5ea43) will increase coverage by 0.02%.
The diff coverage is 95.00%.

❗ Current head 129e54a differs from pull request most recent head bf80684. Consider uploading reports for the commit bf80684 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable     #875      +/-   ##
============================================
+ Coverage     83.83%   83.86%   +0.02%     
============================================
  Files            86       86              
  Lines         14969    14977       +8     
============================================
+ Hits          12550    12560      +10     
+ Misses         2419     2417       -2     
Impacted Files Coverage Δ
libp2p/protocols/pubsub/gossipsub/behavior.nim 88.14% <90.00%> (-0.04%) ⬇️
libp2p/protocols/pubsub/gossipsub.nim 85.37% <100.00%> (-0.04%) ⬇️
libp2p/protocols/pubsub/pubsubpeer.nim 90.14% <100.00%> (+3.01%) ⬆️

... and 2 files with indirect coverage changes

@arnetheduck
Copy link
Contributor

what's the upper bound on memory usage for this?

@Menduist
Copy link
Contributor Author

Menduist commented Mar 20, 2023

Everytime we send an IHAVE, the message id will be stored for historyLength

Plugging in realistic values (for eth2 with all subnets):
historyLength: 6 = 4.2 seconds
Each messages gossiped to: ~20 peers
Msgs per seconds: 1300
Msg id size: 32 bytes

1300 * 4.2 * 20 * 32 = 3.49 MB, as a worst case scenario

@Menduist
Copy link
Contributor Author

Having this data is also useful for future bandwidth optimization (and already optimize a bit, since a peer can now only IWANT us a single time per message, instead of multiple times currently)

@arnetheduck
Copy link
Contributor

Having this data is also useful for future bandwidth optimization (and already optimize a bit, since a peer can now only IWANT us a single time per message, instead of multiple times currently)

yeah, I was thinking about that vs a simple counter that per-peer says how many IHAVE's we've sent them recently, but if it's in the single-digit MB range it seems reasonable

@Menduist
Copy link
Contributor Author

I started this PR using a counter, but then with wildly different messages size it gets pretty bad (I send you 10 IHAVE for 100 bytes msgs, you reply with 10 IWANT for 1 mb msgs)
So I tried to do a counter of message size, but it also gets messy-ish, because we have to find the message in the cache to get its size, which is a tad expensive

@Menduist Menduist merged commit 6b61ce8 into unstable Apr 3, 2023
@Menduist Menduist deleted the betteriwanthandling branch April 3, 2023 08:56
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