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

v1.18: BankingStage Forwarding Filter (backport of #685) #697

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Apr 9, 2024

Problem

  • Forwarding all packets, when many will ultimately be duplicates

Summary of Changes

  • Only forward packets that were from staked connections (swqos)

Fixes #


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

@mergify mergify bot added the conflicts label Apr 9, 2024
Copy link
Author

mergify bot commented Apr 9, 2024

Cherry-pick of 1744e9e has failed:

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

You are currently cherry-picking commit 1744e9efd7.
  (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)

Changes to be committed:
	modified:   bench-streamer/src/main.rs
	modified:   core/src/banking_stage/forwarder.rs
	modified:   core/src/fetch_stage.rs
	modified:   core/src/repair/ancestor_hashes_service.rs
	modified:   core/src/repair/serve_repair_service.rs
	modified:   core/src/shred_fetch_stage.rs
	modified:   gossip/src/gossip_service.rs
	modified:   local-cluster/tests/local_cluster.rs
	modified:   streamer/src/nonblocking/quic.rs
	modified:   streamer/src/streamer.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   sdk/src/packet.rs

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

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2024

Codecov Report

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

Project coverage is 81.6%. Comparing base (cf5e8d3) to head (91bbcbb).

Additional details and impacted files
@@           Coverage Diff           @@
##            v1.18     #697   +/-   ##
=======================================
  Coverage    81.5%    81.6%           
=======================================
  Files         827      827           
  Lines      224833   224844   +11     
=======================================
+ Hits       183440   183491   +51     
+ Misses      41393    41353   -40     

tao-stones
tao-stones previously approved these changes Apr 10, 2024
Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm

sakridge
sakridge previously approved these changes Apr 10, 2024
apfitzge and others added 3 commits April 10, 2024 17:18
* add PacketFlags::FROM_STAKED_NODE

* Only forward packets from staked node

* fix local-cluster test forwarding

* review comment

* tpu_votes get marked as from_staked_node

(cherry picked from commit 1744e9e)

# Conflicts:
#	sdk/src/packet.rs
Copy link

@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.

r+ ci. only difference from previous approvals is removing test hacks in favor of a more appropriate fix in #724

@apfitzge apfitzge requested a review from sakridge April 11, 2024 13:46
@t-nelson t-nelson requested a review from steviez April 11, 2024 15:32
Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -190,6 +191,7 @@ impl FetchStage {
coalesce,
true,
in_vote_only_mode.clone(),
false, // unstaked connections

Choose a reason for hiding this comment

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

The convention is that the trailing comment should be the argument name; is_staked_service here.
This is kind of "double negative" and confusing. Does "false unstaked" mean "staked"?

Choose a reason for hiding this comment

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

there's already instances of both "explanation" style comments on bool arguments, as well as "name" style comments.

This is a backport, keep it the same as the original. If we feel strongly enough about the style we should be making modifications to master, not in a backport PR.

@@ -171,6 +171,7 @@ impl ShredFetchStage {
PACKET_COALESCE_DURATION,
true, // use_pinned_memory
None, // in_vote_only_mode
false,

Choose a reason for hiding this comment

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

Please follow the code style.

The comments makes the life of someone reading this code a lot easier and also makes bugs/mistakes less likely.

Choose a reason for hiding this comment

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

see above.

Comment on lines +151 to +153
packet_batch
.iter_mut()
.for_each(|p| p.meta_mut().set_from_staked_node(is_staked_service));

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

clippy didn't flag it, see above comment about style.

@apfitzge apfitzge merged commit 7e9a53e into v1.18 Apr 11, 2024
36 checks passed
@apfitzge apfitzge deleted the mergify/bp/v1.18/pr-685 branch April 11, 2024 21:15
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.

None yet

7 participants