modify Reactor priorities #5826
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blockchain/vX reactor priority was decreased because during a normal operation (i.e. when the node is not fast syncing) blockchain priority can't be the same as consensus reactor. Otherwise, it's theoretically possible to slow down consensus by constantly requesting blocks from the node. NOTE: ideally blockchain/vX reactor priority would be dynamic. e.g. when the node is fast syncing, the priority is 10 (max), but when it's done fast syncing - the priority gets decreased to 5 (only to serve blocks for other nodes). But it's not possible now, therefore I decided to focus on normal operation (priority = 5). evidence and consensus critical messages are more important than mempool, hence they are increased by 1 (from 5 to 6). statesync reactor priority was changed from 1 to 5 to be the same as blockchain/vX priority.
melekes
requested review from
cmwaters,
ebuchman,
erikgrinaker and
tessr
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5826 +/- ##
==========================================
+ Coverage 60.03% 60.04% +0.01%
==========================================
Files 263 263
Lines 23834 23834
==========================================
+ Hits 14309 14312 +3
+ Misses 8018 8009 -9
- Partials 1507 1513 +6
|
melekes
commented
Dec 23, 2020
statesync/reactor.go
Outdated
| @@ -34,7 +34,7 @@ var ( | |||
| MsgType: new(ssproto.Message), | |||
| Descriptor: &p2p.ChannelDescriptor{ | |||
| ID: byte(SnapshotChannel), | |||
| Priority: 3, | |||
| Priority: 2, | |||
There was a problem hiding this comment.
@erikgrinaker do I understand correctly that sending snapshots are 1/3rd less important than sending chunks? or it should be vice versa? maybe we should just assign 5 to both channels?
There was a problem hiding this comment.
Higher priority is better, right? So snapshot metadata is 3 times more important than chunks. This is because chunks are large and restoration is time-consuming, while snapshot metadata is tiny and requested in real-time, so it should have priority.
We probably want chunks to have low priority in general, for the same reason. It shouldn't have priority above e.g. consensus.
There was a problem hiding this comment.
Higher priority is better, right?
right
consensus/reactor.go
Outdated
| @@ -156,7 +156,7 @@ func (conR *Reactor) GetChannels() []*p2p.ChannelDescriptor { | |||
| }, | |||
| { | |||
| ID: VoteChannel, | |||
| Priority: 5, | |||
| Priority: 6, | |||
There was a problem hiding this comment.
Could maybe consider even 7 here. Priorities are sort of weighted during packet sending, so that if one channel has 6 and another har 5 then the 6 channel gets 6/11 packets sent while the other gets 5/11 (if I'm reading this right).
erikgrinaker
approved these changes
Dec 23, 2020
erikgrinaker
approved these changes
Dec 23, 2020
|
This needs a backport onto 0.34.x and then I will cut a new RC! |
melekes
added a commit
that referenced
this pull request
Dec 23, 2020
blockchain/vX reactor priority was decreased because during the normal operation (i.e. when the node is not fast syncing) blockchain priority can't be the same as consensus reactor priority. Otherwise, it's theoretically possible to slow down consensus by constantly requesting blocks from the node. NOTE: ideally blockchain/vX reactor priority would be dynamic. e.g. when the node is fast syncing, the priority is 10 (max), but when it's done fast syncing - the priority gets decreased to 5 (only to serve blocks for other nodes). But it's not possible now, therefore I decided to focus on the normal operation (priority = 5). evidence and consensus critical messages are more important than the mempool ones, hence priorities are bumped by 1 (from 5 to 6). statesync reactor priority was changed from 1 to 5 to be the same as blockchain/vX priority. Refs #5816
tessr
pushed a commit
that referenced
this pull request
Dec 23, 2020
blockchain/vX reactor priority was decreased because during the normal operation (i.e. when the node is not fast syncing) blockchain priority can't be the same as consensus reactor priority. Otherwise, it's theoretically possible to slow down consensus by constantly requesting blocks from the node. NOTE: ideally blockchain/vX reactor priority would be dynamic. e.g. when the node is fast syncing, the priority is 10 (max), but when it's done fast syncing - the priority gets decreased to 5 (only to serve blocks for other nodes). But it's not possible now, therefore I decided to focus on the normal operation (priority = 5). evidence and consensus critical messages are more important than the mempool ones, hence priorities are bumped by 1 (from 5 to 6). statesync reactor priority was changed from 1 to 5 to be the same as blockchain/vX priority. Refs #5816
This was referenced Apr 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
blockchain/vX reactor priority was decreased because during the normal operation
(i.e. when the node is not fast syncing) blockchain priority can't be
the same as consensus reactor priority. Otherwise, it's theoretically possible to
slow down consensus by constantly requesting blocks from the node.
NOTE: ideally blockchain/vX reactor priority would be dynamic. e.g. when
the node is fast syncing, the priority is 10 (max), but when it's done
fast syncing - the priority gets decreased to 5 (only to serve blocks
for other nodes). But it's not possible now, therefore I decided to
focus on the normal operation (priority = 5).
evidence and consensus critical messages are more important than
the mempool ones, hence priorities are bumped by 1 (from 5 to 6).
statesync reactor priority was changed from 1 to 5 to be the same as
blockchain/vX priority.
Refs #5816