Skip to content

BankingStage Forwarding Filter#685

Merged
mergify[bot] merged 5 commits intoanza-xyz:masterfrom
apfitzge:forward-filter
Apr 9, 2024
Merged

BankingStage Forwarding Filter#685
mergify[bot] merged 5 commits intoanza-xyz:masterfrom
apfitzge:forward-filter

Conversation

@apfitzge
Copy link
Copy Markdown

@apfitzge apfitzge 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 #

apfitzge
apfitzge commented Apr 9, 2024
View reviewed changes
Comment thread sdk/src/packet.rs
@apfitzge apfitzge marked this pull request as ready for review April 9, 2024 16:38
@apfitzge apfitzge requested review from t-nelson and tao-stones April 9, 2024 16:38
@mergify
Copy link
Copy Markdown

mergify Bot commented Apr 9, 2024

Backports to the stable 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.

@mergify
Copy link
Copy Markdown

mergify Bot commented Apr 9, 2024

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.

tao-stones
tao-stones reviewed Apr 9, 2024
View reviewed changes
Copy link
Copy Markdown

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

overall looks good, it makes sense to not to forward packets that should be filtered by qos, wish this logic is in one place with whole qos logic.

Comment thread core/src/banking_stage/forwarder.rs Outdated
Comment thread core/src/banking_stage/forwarder.rs Outdated
Comment thread sdk/src/packet.rs
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 9, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 81.9%. Comparing base (c0be86d) to head (89f8ca4).
Report is 14 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##           master     #685     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         851      851             
  Lines      230475   230486     +11     
=========================================
- Hits       188785   188782      -3     
- Misses      41690    41704     +14

tao-stones
tao-stones approved these changes Apr 9, 2024
View reviewed changes
Copy link
Copy Markdown

@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

Comment thread core/src/fetch_stage.rs
coalesce,
true,
None,
true, // only staked connections should be voting
Copy link
Copy Markdown

@tao-stones tao-stones Apr 9, 2024

Choose a reason for hiding this comment

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

thanks for making vote explicit, sorry for additional code/test changes, hope won't make bp harder

t-nelson
t-nelson approved these changes Apr 9, 2024
View reviewed changes
Copy link
Copy Markdown

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

lgtm! :shipit:

@apfitzge apfitzge added the automerge automerge Merge this Pull Request automatically once CI passes label Apr 9, 2024
@mergify mergify Bot merged commit 1744e9e into anza-xyz:master Apr 9, 2024
mergify Bot pushed a commit that referenced this pull request Apr 9, 2024
@apfitzge @mergify
BankingStage Forwarding Filter (#685)
83bdf3e 
* 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
mergify Bot pushed a commit that referenced this pull request Apr 9, 2024
@apfitzge @mergify
BankingStage Forwarding Filter (#685)
ccfa2ae 
* 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
This was referenced Apr 9, 2024
Merged
Merged
@apfitzge apfitzge deleted the forward-filter branch April 10, 2024 13:29
@apfitzge apfitzge mentioned this pull request Apr 10, 2024
Merged
This was referenced Apr 10, 2024
Merged
Merged
t-nelson pushed a commit that referenced this pull request Apr 10, 2024
@apfitzge @t-nelson
BankingStage Forwarding Filter (#685)
bd06b20 
* 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
t-nelson pushed a commit that referenced this pull request Apr 10, 2024
@apfitzge @t-nelson
BankingStage Forwarding Filter (#685)
ce12461 
* 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
t-nelson added a commit that referenced this pull request Apr 11, 2024
@mergify @apfitzge @t-nelson
v1.17: BankingStage Forwarding Filter (backport of #685) (#696)
6a46c01 
* BankingStage Forwarding Filter (#685)

* 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

* resolve conflict

* remove test_ledger_cleanup_service

* Revert "remove test_ledger_cleanup_service"

This reverts commit 68a580d.

* revert: local-cluster test changes

---------

Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>
Co-authored-by: Trent Nelson <trent@solana.com>
apfitzge added a commit that referenced this pull request Apr 11, 2024
@mergify @apfitzge @t-nelson
v1.18: BankingStage Forwarding Filter (backport of #685) (#697)
7e9a53e 
* BankingStage Forwarding Filter (#685)

* 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

* resolve conflict

* revert: local-cluster test changes

---------

Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>
Co-authored-by: Trent Nelson <trent@solana.com>
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024
@mergify @apfitzge
@t-nelson @yihau
v1.18: BankingStage Forwarding Filter (backport of anza-xyz#685) (anz…
491cb65 
…a-xyz#697)

* BankingStage Forwarding Filter (anza-xyz#685)

* 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

* resolve conflict

* revert: local-cluster test changes

---------

Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>
Co-authored-by: Trent Nelson <trent@solana.com>
@apfitzge apfitzge mentioned this pull request Aug 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-nelson t-nelson t-nelson approved these changes

@tao-stones tao-stones tao-stones approved these changes

Assignees

No one assigned

Labels

automerge automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@apfitzge @codecov-commenter @t-nelson @tao-stones