Split Substrate messages into multiple substreams #1796
Conversation
|
@tomaka who is capable of reviewing this? |
|
@twittner, I suppose. |
| pending_send_back: SmallVec<[(u64, RegisteredProtocolSubstream<TMessage, TSubstream>); 4]>, | ||
|
|
||
| /// List of messages waiting for a substream to open in order to be sent. | ||
| pending_messages: SmallVec<[TMessage; 6]>, |
There was a problem hiding this comment.
I wonder if there should be some back pressure mechanism if this grows a lot? My understanding is that the current version measures the send queue of the RegisteredProtocolSubstream and issues CustomProtosHandlerOut::Clogged events if it becomes too large. The new approach instead requests substreams as necessary and buffers messages here. If the connection becomes congested would it be useful to issue a Clogged event based on the length of this buffer?
There was a problem hiding this comment.
In theory, opening a substream is an asynchronous operation, but in practice it's not and they open instantly. Therefore I'm not really worried about this queue growing very large.
Instead we're moving the problem to the multiplexing layer, which to me is a more appropriate layer to handle network congestion.
You're right that this pretty much makes the Clogged event useless, as it will never fire with the new system. This is why "remove the clogged event" is the next item on the list in #1692.
There was a problem hiding this comment.
Are substreams not made available to CustomProtocolsHandler only after protocol negotiation which requires at least a full round trip via multistream-select? Would that not take a while over a congested connection?
There was a problem hiding this comment.
Ah, you're right, I'll add back some clogging verification here.
In a future PR we can embed the message to send directly in the RegisteredProtocolSubstream so that sending it is part of the upgrade, which will greatly simplify the code and delegate the problem to libp2p.
gavofyork
added
A3-in_progress
* Split Substrate messages into multiple substreams * Add back Clogged event
Next two items of #1692
Fix #1517
See the comment here for a description: #1517 (comment)
This PR still uses the old behaviour by default. Once the network has upgraded to a version of Substrate/Polkadot that contains this PR, I'll try if everything works correctly (if we can still sync), then switch to the new behaviour by default, then eventually remove the old behaviour in the future.
While this PR adds a lot of code, removing the old behaviour would remove almost all of the
custom_protosmodule, as we would switch to helpers that are inside libp2p.I marked as "onice" because we need to publish a new version of theyamuxcrate that fixes some issues, but other than that the PR is ready.