Skip to content

Conversation

@alexpyattaev
Copy link

@alexpyattaev alexpyattaev commented Jan 21, 2026

Problem

Streamer ignores RTT in all its calculations. This means that connections with high latency are heavily rate limited, much more than the stake amount would suggest. This happens before the actual stake-based stream throttling even has a change to kick in, and is thus very counterintuitive to the client.

A more principled version of #7954.

Some preliminary concepts:

  • Sender can not have more than max_concurrent_streams worth of open streams at any time
  • These together limit how many TXs can be “in flight” between client and server and not yet ACKd. Currently, these limits are computed based on stake. We need to compute them based on stake and RTT (as longer RTT means you need to have more things on the wire before you see an ACK).
  • Beyond the limit mentioned above, there is an overall stream limit imposed by the code in stream_throttle.rs. That is out of scope of this PR, we are not modifying SWQOS here.

This mechanism is not intended as the actual rate limiter for complaint clients, just as a limit on network buffers. This will never allocate less bandwidth than what you'd get today.

Summary of Changes

  • Assume that in the original code the RTT was expected to be ~50ms
  • Scale max_concurrent_uni_streams with RTT if it is above 50ms

50 ms was chosen as a compromise between allowing more TPS and keeping impact small.

Impact

Before:
plot_old_512_bytes

After:
plot_new_512_bytes

Related: #8948 (which was reverted)

@alexpyattaev alexpyattaev added the noCI Suppress CI on this Pull Request label Jan 21, 2026
@alexpyattaev alexpyattaev force-pushed the streamer_bdp_scaling branch 2 times, most recently from 0c4a13c to 1066ae5 Compare January 21, 2026 22:05
@alexpyattaev alexpyattaev added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request labels Jan 21, 2026
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 21, 2026
@alexpyattaev alexpyattaev marked this pull request as ready for review January 21, 2026 22:05
@t-nelson
Copy link

wasn't there a new test case before the helper was added and plumbing removed?

@alexpyattaev
Copy link
Author

alexpyattaev commented Jan 21, 2026

wasn't there a new test case before the helper was added and plumbing removed?

Yes. Should I bring it back? Added the unittests back in 228af98 but feel free to drop that commit.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.6%. Comparing base (6b94ee3) to head (228af98).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #10144     +/-   ##
=========================================
- Coverage    82.6%    82.6%   -0.1%     
=========================================
  Files         853      853             
  Lines      318566   318586     +20     
=========================================
- Hits       263382   263358     -24     
- Misses      55184    55228     +44     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gregcusack gregcusack requested a review from t-nelson January 22, 2026 02:10
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.

4 participants