Skip to content

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
@codecov
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.

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