GossipSub: cancel inflight msgs when receiving duplicate #851
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #851 +/- ##
============================================
+ Coverage 83.73% 83.75% +0.02%
============================================
Files 85 85
Lines 15047 15073 +26
============================================
+ Hits 12599 12625 +26
Misses 2448 2448
|
|
this carries the risk that the other end disconnects / descores due to the stream being reset mid-sending - I'm not sure it's a good idea in general - ie once the channel is open and streaming, interrupting the send looks like we're just trying to waste bandwidth |
|
Cancelling a write will not reset a stream, it will just "unqueue" the message if that is still possible However, for very big messages, it might make sense to a open a new stream just for it (when we'll have 0rtt negotiation), and being able to reset it mid-air. EDIT: issue for 0 rtt negotiation is here: #746 |
right, right now, I'm in the middle of discussions of features like this for 1mb-sized messages - partial streaming is highly likely - a similar feature is to not send messages to peers that have an in-progress send of potentially the same message via some kind of prefix comparison (or maybe by including the computed message id in the "header" of the stream. |
| # Fast path that only encodes message once | ||
| let encoded = encodeRpcMsg(msg, p.anonymize) | ||
| for peer in sendPeers: | ||
| result[peer.peerId] = peer.sendEncoded(encoded) |
There was a problem hiding this comment.
doesn't this already synchronously put bytes in the send buffer?
There was a problem hiding this comment.
With mplex, that will indeed go synchronously through every layer, down to chronos
I thought that chronos supported write cancellation, apparently not, quoting cheatfate:
There is a big problem with write and i wish not to solve it today or tomorrow, because write is made using queue, so cancelling write in the middle should remove this write call from the queue, but it will break cross-platform compatibility, because on Windows this queue is in system, so i can't freely remove write call from the middle of the queue...
So we could add cancellation support to linux at least
With yamux, there is an intermediary queue before chronos (since there is per-stream backpressure), that does support cancellation:
https://github.com/status-im/nim-libp2p/blob/351bda2b56e4389a91fb42199621b3617bdbbdd3/libp2p/muxers/yamux/yamux.nim#L285
So this PR only makes sense if we get chronos support for write cancellation on linux, or enable yamux in nimbus :/
There was a problem hiding this comment.
Getting TCP cancellation support in chronos seems less complex than expected (even on windows): status-im/nim-chronos#353
I'll wait for reviews to be sure
| let | ||
| futsTable = g.cancellableBroadcast(peers, msg) | ||
| futs = toSeq(futsTable.values) | ||
| g.sendingFutures[msgIdSalted] = futsTable |
There was a problem hiding this comment.
can't there be two inflight lazy broadcasts of the same message?
There was a problem hiding this comment.
That shouldn't happen, I may add an assert to be sure
Yeah, seems like the ideal solution would be a new protocol that opens a stream eagerly, sends the message id & the message The receiving end can reset the stream at any point if he gets the message first somewhere else (or if many peers are currently sending him the same message) |
actually, if we start receiving a message from someone we could actually look at the first few bytes and delay send iff they match whatever is being sent - basically, when we perform the send, we'd compare the message being sent against whatever partial bytes that have been received - a bit tricky perhaps because of protobuf and its unordered fields, but perhaps doable. easier to do with explicit support perhaps. |
|
This is not actually doable, since cancelling an outbound message is unpractical |
ref #850