Skip to content

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

Merged
lijunwangs merged 4 commits intov1.17from
mergify/bp/v1.17/pr-701
Apr 11, 2024
Merged

v1.17: Treat super low staked as unstaked in streamer QOS (backport of #701)#732
lijunwangs merged 4 commits intov1.17from
mergify/bp/v1.17/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)

# Conflicts:
#	streamer/src/nonblocking/quic.rs
#	streamer/src/nonblocking/stream_throttle.rs
@mergify mergify Bot added the conflicts label Apr 10, 2024
@mergify
Copy link
Copy Markdown
Author

mergify Bot commented Apr 10, 2024

Cherry-pick of 92ebf0f has failed:

On branch mergify/bp/v1.17/pr-701
Your branch is up to date with 'origin/v1.17'.

You are currently cherry-picking commit 92ebf0f80c.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   streamer/src/nonblocking/quic.rs
	deleted by us:   streamer/src/nonblocking/stream_throttle.rs

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@lijunwangs lijunwangs requested review from pgarg66 and t-nelson April 10, 2024 23:49
Comment thread streamer/src/nonblocking/quic.rs Outdated
|(pubkey, stake, total_stake, max_stake, min_stake)| {
// The heuristic is that the stake should be large engouh to have 1 stream pass throuh within one throttle
// interval during which we allow max (MAX_STREAMS_PER_100MS * 100) streams.
let min_stake_ratio = 1_f64 / (MAX_STREAMS_PER_100MS * 100) 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.

We should not multiply with 100 here. The constant MAX_STREAMS_PER_100MS already accounts for that.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh right. Silly me!

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (be83469) to head (38e88d7).

Additional details and impacted files
@@           Coverage Diff           @@
##            v1.17     #732   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         806      806           
  Lines      219284   219290    +6     
=======================================
+ Hits       179014   179058   +44     
+ Misses      40270    40232   -38     

@lijunwangs lijunwangs merged commit 998fbef into v1.17 Apr 11, 2024
@lijunwangs lijunwangs deleted the mergify/bp/v1.17/pr-701 branch April 11, 2024 02:46
@t-nelson t-nelson requested a review from sakridge April 11, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants