Skip to content
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

Fix determination of staked QUIC connections #34760

Merged
merged 4 commits into from
Jan 14, 2024

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Jan 12, 2024

Problem

The usage of ConnectionPeerType to determine if a connection is staked is incorrect. The peer_type is an attribute of the connection_table where a QUIC connection gets stored. We maintain two tables, one for staked connections, and the other for unstaked connections. But if the staked connection is full, the new connections get stored in the unstaked connection table, even if the peer is a staked client. So ConnectionPeerType can incorrectly mark a staked connection as unstaked.

Peer's stake correctly identifies if the connection is from a staked client or not.

Summary of Changes

Update code to use peer's stake to determine if the connection is from a staked client.
Remove ConnectionPeerType and its usage.

Fixes #

@pgarg66 pgarg66 marked this pull request as ready for review January 12, 2024 03:20
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (22fcffe) 81.8% compared to head (8fb5a01) 81.8%.
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34760     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         824      824             
  Lines      223051   223049      -2     
=========================================
- Hits       182474   182465      -9     
- Misses      40577    40584      +7     

params.stake,
params.total_stake,
stream_load_ema.clone(),
);
let staked_stream = matches!(peer_type, ConnectionPeerType::Staked) && params.stake > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can reduce some of this change by defining:

let staked_stream = params.stake > 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it better to have this check than declaring a new stack variable? The change is only three lines. So review should be simple.

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

i think i'd have preferred resolving the enum once and using that throughout. the Staked variant just needs to be Staked { lamports: u64 }

@pgarg66
Copy link
Contributor Author

pgarg66 commented Jan 12, 2024

i think i'd have preferred resolving the enum once and using that throughout. the Staked variant just needs to be Staked { lamports: u64 }

Yes, that's more idiomatic. Will update the PR.

@pgarg66
Copy link
Contributor Author

pgarg66 commented Jan 12, 2024

i think i'd have preferred resolving the enum once and using that throughout. the Staked variant just needs to be Staked { lamports: u64 }

Done

@pgarg66 pgarg66 requested a review from t-nelson January 13, 2024 01:08
Copy link
Contributor

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

lgtm

@pgarg66 pgarg66 merged commit f92275b into solana-labs:master Jan 14, 2024
35 checks passed
@pgarg66 pgarg66 deleted the ema-follow-up branch January 14, 2024 02:38
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Apr 18, 2024
* Fix determination of staked QUIC connections

* address review comments

* review comments

* treat connections with zero stake as unstaked
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.

3 participants