Improve collator networking

[rphmeier]

Fixes #1525

This alters the behavior of collators. We now only disconnect from reserved peers between leaves (after new candidates are likely to have been submitted). This avoids some race conditions such as #1499 , where disconnections were being immediately followed by reconnections.

[rphmeier]

Somehow this made things substantially worse. Not sure why. it seems to be the worst of both worlds

[rphmeier]

Actually, it seems much better now. Perhaps we were seeing another issue related to #1517 which is apparently causing validators to crash. Will hold out for better results. One change I can imagine is tweaking the timer to be 2.5s, which should work even better than 4s as it gives 3.5s for connections to be fully closed before reopening.

[dmitry-markin]

I'm not sure if adding more time will help connections to "fully close". If the node is not sending anything over the substream during that time, it won't be able to detect that the substream was closed and accept a new one.

[rphmeier]

I'm not sure if adding more time will help connections to "fully close". If the node is not sending anything over the substream during that time, it won't be able to detect that the substream was closed and accept a new one.

It seemed to help in practice, but can you elaborate? How do you force a substream to close if disconnect doesn't work? Immediately send an empty notification over it?

[dmitry-markin]

I'm not sure if adding more time will help connections to "fully close". If the node is not sending anything over the substream during that time, it won't be able to detect that the substream was closed and accept a new one.

It seemed to help in practice, but can you elaborate? How do you force a substream to close if disconnect doesn't work? Immediately send an empty notification over it?

disconnect does work, but the other side only learns that the stream is closed after sending a notification into it. That's on the application level, on libp2p protocol level the node is instantly aware of the closed substream. Sending an empty notification should work. We also have a fix for the remote not learning instantly that the substream is closed: paritytech/substrate#13396 (reverted because it made insufficient inbound slots issue manifest, but that should be fixed now).

@altonen could you remind why you are against merging this fix now?

[altonen]

@dmitry-markin

Which PR fixed the insufficient inbound slots issue? We adjusted the ratios a while back but I don't count that as a fix and AFAIR there hasn't been any direct PR addressing the issue because it's not something we can fix in sc-network.

I'm not against merging the PR but it's more of a hack than an actual fix. IMO proper way to fix this would be to detect when the inbound substream is closed and then consider the connection closed.

[dmitry-markin]

Which PR fixed the insufficient inbound slots issue? We adjusted the ratios a while back but I don't count that as a fix and AFAIR there hasn't been any direct PR addressing the issue because it's not something we can fix in sc-network.

I merely meant adjusting the ratios when a wrote about the fix.

I'm not against merging the PR but it's more of a hack than an actual fix. IMO proper way to fix this would be to detect when the inbound substream is closed and then consider the connection closed.

Thanks for the comment, I forgot the discussion about not allowing half-open notifications substreams, and that was essentially what I was asking about.

[bkchr]

@dmitry-markin please close if not required.