-
Notifications
You must be signed in to change notification settings - Fork 890
scale max_concurrent_uni_streams with BDP #8948
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
Conversation
284f3ce to
612e949
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8948 +/- ##
=======================================
Coverage 82.6% 82.6%
=======================================
Files 844 844
Lines 316630 316633 +3
=======================================
+ Hits 261595 261610 +15
+ Misses 55035 55023 -12 🚀 New features to boost your workflow:
|
8a35094 to
e0b63e0
Compare
e0b63e0 to
fb96246
Compare
|
Additional testing against testnet nodes on public internet. Test run duration: 10 seconds, repeated 3 times, using 2 staked identities with 2M testnet sol each:
As intended, on longer links (Tokyo, Singapore) the gains are massive and result in almost same TPS as within the US. London-Dallas link is particularly bad quality (high packet loss) but is consistent with the rest of the results (but with much higher standard deviation). |
|
are the numbers in the chart the measured open streams or the calculated stream limit? |
the numbers are TPS (besides the RTT in ms of course) |
The numbers in the table are measured TPS. |
fb96246 to
10d60db
Compare
gregcusack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for getting these numbers (both sim and on real network)! just the question on attack vector here (think it was brought up on backport call as well).
| const REFERENCE_RTT_MS: u64 = 50; | ||
|
|
||
| /// Above this RTT we stop scaling for BDP | ||
| const MAX_RTT_MS: u64 = 350; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this number come from? great that we have a cap here! is it possible to spoof your rtt when you initially setup your connection to get more streams? And then you get more access to the pipe? For example, if your RTT is 20ms but you spoof your connection to be 350ms, how much more TPS are you going to be able to get?
maybe this is not a big issue but just want to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
320 ms is an RTT from Australia to Portugal, which is the largest RTT I could find on this planet. 350 is that + a bit of margin.
You can of course spoof RTT and get more concurrent streams, but you would get throttled by SWQOS to whatever your proper quota is anyway. So faking RTT is not going to bring you real TPS increases beyond what you are supposed to get based on your stake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it! thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With "no throttling below threshold" in place there will be no throttling in SWQOS unless the overall load is high though. It should be possible to address RTT spoofing, if really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone will be trying to abuse this, all they will achieve is force system back into SWQOS mode, which they can do without BDP scaling easily enough (as we did during tests of #9580)
sorry approved by accident. just want some clarity on attack vector here first. otherwise looks good
gregcusack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thanks for putting this together! we probably want another review from @stablebits and/or @alessandrod as well
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
scale RX window and max_streams with BDP (cherry picked from commit 8faf30c) # Conflicts: # streamer/src/nonblocking/swqos.rs
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:
This mechanism is not intended as the actual rate limiter for complaint clients, just as a limit on network buffers and such like. This is designed to allocate more bandwidth than what you'd get today.
Summary of Changes
Whether 50ms is the right latency to pick is a good question. 50 ms was chosen as a compromise between allowing more TPS and keeping changes small.
Impact
Before:

After:
