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

network: fix outgoing HTTP rate limiting #6118

Merged

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Aug 30, 2024

Summary

While working on rate limiting functionality for libp2p HTTP clients in #5939 the existing WS net functionality was broken and never caught. This PR restores the functionality by making the RateLimitingTransport RateLimitingBoundTransport - i.e. requiring and enforcing the target peer address (either host:port or peerID). This makes sense since HTTP clients constructed as part of wsPeerCore are intended to be used against only this peer. This also means there is no shared wn.transport and RateLimitingBoundTransport objectare constructed on demand for http clients.

Fixes #6117

Test Plan

  1. Added RoundTrip and GetHTTPClient unit tests
  2. Checked devnet catchup

@algorandskiy algorandskiy force-pushed the pavel/p2p-fix-http-client-rate branch from 4bffd2e to 8cec779 Compare August 30, 2024 18:55
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

Project coverage is 56.21%. Comparing base (81edd96) to head (8cec779).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
network/p2p/http.go 0.00% 3 Missing ⚠️
network/limitcaller/rateLimitingTransport.go 84.61% 2 Missing ⚠️
network/p2pNetwork.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6118   +/-   ##
=======================================
  Coverage   56.20%   56.21%           
=======================================
  Files         492      494    +2     
  Lines       69829    69899   +70     
=======================================
+ Hits        39248    39291   +43     
- Misses      27915    27941   +26     
- Partials     2666     2667    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Asked a few questions, mostly looks good.

@algorandskiy algorandskiy merged commit 0da0e99 into algorand:master Sep 3, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Catchup regression after p2p merge due to rate limiting
3 participants