Send OurViewChange with priority instead of using an unbounded channel #5547
Conversation
AndreiEres
added
T0-node
| @@ -967,10 +968,12 @@ fn update_our_view<Context>( | |||
| finalized_number, | |||
| ); | |||
|
|
|||
| dispatch_validation_event_to_all_unbounded( | |||
| dispatch_validation_event_to_all( | |||
There was a problem hiding this comment.
This has unintended consequences, right now the priority is:
- Unbounded queue.
- High Priority queue
- Normal Priority queue.
What it means is that we process OurViewChange ahead of all the others PeerConnected/PeerDisconnected/PeerViewChange, that have been already queued.
But with this change we are going to put the OurViewChange in the same queue, so it mean all PeerViewChange already queued will be processed before OurViewChange.
I'm not saying something bad will happen, but I don't see the gain and I think most of our subsystem will benefit from processing OurViewChange as fast as it can.
There was a problem hiding this comment.
Yeah, this is a good point. We need an additional priority level to keep the priorities like we have them currently.
There was a problem hiding this comment.
I was also thinking about that. Because we already use the unbounded queue as the highest priority, we can mix the current order. But introducing one more level of priority can bring us a prioritization hell.
There was a problem hiding this comment.
Ideally we should never use unbounded for prioritization because of potential to introduce DOS vectors. but we should be fine as is without this PR - the number of OurViewChange messages is bounded by block production.
|
Decided that we're good with the current state of (ab)using unbounded channels |
Related to #824
I assume
OurViewChangewas sent using unbounded channel for prioritization. Since we can send messages with a particular priority, we use it here as well.