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

Add tx weighting stage #21953

Closed

Conversation

sakridge
Copy link
Member

Problem

Transaction don't have the stake weight of the sender and to order them it would be nice to have.

Summary of Changes

Add a stage to add the stake weights after streamer and before sigverify.

Fixes #

@buffalu
Copy link
Contributor

buffalu commented Dec 16, 2021

do top validators know this is coming? hopefully they’re running more than 1G NICs lol

@buffalu
Copy link
Contributor

buffalu commented Dec 17, 2021

also this could make DOS worse without special forwarder software?

not only can you screw with block propagation of current leader by stealing bandwidth from them, but also can forward my DOS packets through the highest staker to the current leader

@sakridge
Copy link
Member Author

also this could make DOS worse without special forwarder software?

not only can you screw with block propagation of current leader by stealing bandwidth from them, but also can forward my DOS packets through the highest staker to the current leader

That high staker which is forwarding will QoS those packets and will prefer to send packets from other staked nodes first before it sends the unstaked DoS packets.

@sakridge sakridge force-pushed the transaction-weight-stage branch 2 times, most recently from 2ada3c7 to 441e584 Compare December 18, 2021 12:19
@segfaultdoc
Copy link
Contributor

segfaultdoc commented Dec 22, 2021

also this could make DOS worse without special forwarder software?
not only can you screw with block propagation of current leader by stealing bandwidth from them, but also can forward my DOS packets through the highest staker to the current leader

That high staker which is forwarding will QoS those packets and will prefer to send packets from other staked nodes first before it sends the unstaked DoS packets.

Summary of Changes

Why do you assume DoS packets will come from unstaked nodes?
Also, should those changes to the QoS service be added to this PR?

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #21953 (a01fb1a) into master (ba046bd) will decrease coverage by 0.0%.
The diff coverage is 84.1%.

@@            Coverage Diff            @@
##           master   #21953     +/-   ##
=========================================
- Coverage    81.1%    81.1%   -0.1%     
=========================================
  Files         555      556      +1     
  Lines      150664   150742     +78     
=========================================
+ Hits       122300   122351     +51     
- Misses      28364    28391     +27     

@michaelh-laine
Copy link
Contributor

How do tx first get into "the system" as most non-vote tx are first submitted to/via RPC nodes which have zero stake. Does this PR not create a scenario where votes push out tx from all other clients? And if there is a sequence of low-staked leaders that receive a burst of client transactions (as RPC forwards to upcoming leaders) they then receive low priority when forwarded (if not immediately included in a block).

Perhaps there's more intelligence in the mechanism that would address these issues, would appreciate some clarity on this.

(Question derived from discussion on Validator roundtable call on 20.01.2022)

@tao-stones
Copy link
Contributor

tao-stones commented Feb 1, 2022

How do tx first get into "the system" as most non-vote tx are first submitted to/via RPC nodes which have zero stake. Does this PR not create a scenario where votes push out tx from all other clients?

In short, no, votes will not push out low-staked non-vote transactions.
Votes and non-vote transactions are in separate leader threads, with separate queues. The stake weighted shuffle applies to each queue independently.

@tao-stones tao-stones marked this pull request as ready for review February 1, 2022 23:35
@tao-stones
Copy link
Contributor

Close this PR as it has been merged into #22958, reviews and comments should be made on combined PR.

@tao-stones tao-stones closed this Feb 8, 2022
@tao-stones tao-stones mentioned this pull request Mar 7, 2022
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