Skip to content

v1.18: Treat super low staked as unstaked in streamer QOS (backport of #701)#733

Merged
lijunwangs merged 1 commit intov1.18from
mergify/bp/v1.18/pr-701
Apr 11, 2024
Merged

v1.18: Treat super low staked as unstaked in streamer QOS (backport of #701)#733
lijunwangs merged 1 commit intov1.18from
mergify/bp/v1.18/pr-701

Conversation

@mergify
Copy link
Copy Markdown

@mergify mergify Bot commented Apr 10, 2024

Problem

staked nodes with low stakes can abuse the system to get disproportional bandwidth.

Summary of Changes

Treat it as unstaked node for the with stake ratio stake/total_stake < 1 / (max packet per 100 ms)

Fixes #


This is an automatic backport of pull request #701 done by [Mergify](https://mergify.com).

* Treat super low staked with QOS of unstaked

* simplify

* address some comment from Pankaj

(cherry picked from commit 92ebf0f)
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.5%. Comparing base (cf5e8d3) to head (b9fa3f6).

Additional details and impacted files
@@           Coverage Diff           @@
##            v1.18     #733   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         827      827           
  Lines      224833   224835    +2     
=======================================
+ Hits       183440   183448    +8     
+ Misses      41393    41387    -6     

@steviez
Copy link
Copy Markdown

steviez commented Apr 11, 2024

Just making sure this was intentional (I probably missed the conversation):

  • For master and v1.18, a node needs stake / total_stake >= 1 / 25k
  • For v1.17, a node needs stake / total_stake >= 1 / 50k

@lijunwangs
Copy link
Copy Markdown

Just making sure this was intentional (I probably missed the conversation):

  • For master and v1.18, a node needs stake / total_stake >= 1 / 25k
  • For v1.17, a node needs stake / total_stake >= 1 / 50k

Yes. To be eligible to send 1 stream per throttle interval. The PPS difference in master and v1.17 does not make sense to me. In the end we probably we will stick with one value.

@steviez
Copy link
Copy Markdown

steviez commented Apr 11, 2024

The PPS difference in master and v1.17 does not make sense to me. In the end we probably we will stick with one value.

Oh, should we land on a desired value now and make sure we have consistent values across the branches ?

@pgarg66
Copy link
Copy Markdown

pgarg66 commented Apr 11, 2024

The PPS difference in master and v1.17 does not make sense to me. In the end we probably we will stick with one value.

Oh, should we land on a desired value now and make sure we have consistent values across the branches ?

Since this PR is not changing the PPS value, we should let this merge. The question makes sense and we should identify a number that's same across branches.

@lijunwangs
Copy link
Copy Markdown

The PPS difference in master and v1.17 does not make sense to me. In the end we probably we will stick with one value.

Oh, should we land on a desired value now and make sure we have consistent values across the branches ?

Yes, but that should be addressed a separate PR -- #705. I closed it for now. Pending further research into it. Let's table this for now. But the core design goal remains the same. Minimal threshold is at least 1 stream per throttle window assuming proportional bandwidth allocation.

Comment on lines +504 to +505
let min_stake_ratio =
1_f64 / (MAX_STREAMS_PER_MS * STREAM_THROTTLING_INTERVAL_MS) as f64;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why isn't this defined as "const literal" and then maybe const_assert or debug_assert? like:
https://github.com/anza-xyz/agave/blob/baed522d2/local-cluster/src/integration_tests.rs#L208

right now someone can change any of the two referenced constants unbeknown of how much it impacts this threshold here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The intention is really trying to get the max streams we allow in the throttle interval. So if the value is changed, we really want the change as well. But I think it makes sense to keep these constants together so people might realize the impact. I will address in master.

@lijunwangs lijunwangs merged commit 77c6121 into v1.18 Apr 11, 2024
@lijunwangs lijunwangs deleted the mergify/bp/v1.18/pr-701 branch April 11, 2024 20:42
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024
…anza-xyz#701) (anza-xyz#733)

Treat super low staked as unstaked in streamer QOS (anza-xyz#701)

* Treat super low staked with QOS of unstaked

* simplify

* address some comment from Pankaj

(cherry picked from commit 92ebf0f)

Co-authored-by: Lijun Wang <83639177+lijunwangs@users.noreply.github.com>
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.

6 participants