Cancel queued `Publish` RPCs if `IDONTWANT` arrives in the meantime

[raulk]

See libp2p/rust-libp2p#5751 and sigp/rust-libp2p#570.

[MarcoPolo]

We effectively do this already here by checking if the message is unwanted right before sending it.

[raulk]

sendRPC only queues the RPC for sending. The actual sending happens async via a dispatch goroutine a few layers down the stack. Between now and then we could've received an IDONTWANT. That's the cancellation I was referring to here. Some useful breadcrumbs:

• doSendRPC
• rpcQueue#push
• push
• priorityQueue#NormalPush

One way to approach this is by adding an RPC#Cancelled() bool that is backed by a closure supplied at RPC creation time that queries the gs.unwanted map, but we'd need to guard that with a lock, break the concurrency model, and introduce contention which ain't good. There are more ways.

[MarcoPolo]

This is likely a difference of <1ms for a machine not in high cpu contention. Is the juice worth the squeeze? We can certainly try.

[MarcoPolo]

I'd be curious to look at our trace metrics and see the time differences between when we send the rpc (in doSendRPC) and when we receive a IDONTWANT for that rpc, and how many times these are within 1 ms of each other.

[raulk]

I don't think this is CPU-bound? I was thinking it would be IO bound by the multiplexer or transport congestion control (i.e. if the Yamux window is full), which can happen more frequently under bandwidth saturation (especially under bursty loads).

[MarcoPolo]

ah right, if the transport backpressures on stream writes. That seems more likely 👍

[haxxpop]

I don't think this is CPU-bound?

If it's not CPU-bound, doing simulations should help determine if the improvement is worth the effort? We can simulate the hacky implementation on this PR #613 against the status quo.

[haxxpop]

As far as I can remember, I discussed this issue with someone when I implemented IDONTWANT and I gave up due to the complexity of the code base.

[cortze]

Just chiming in here to share some of the takeaways that the ProbeLab team has gathered when analysing the IDONTWANT addition to GossipSub. Takeaways that should help motivate the extra effort to implement extra complex changes in the internal structure to enable the canceling any ongoing RPC.

The main highlight that we could extract from the traces produced by hermes is that around 70% of the duplicates that it received came from peers that it had already notified with a previous IDONTWANT message.
This could indicate that the single check for "unwanted" msg IDs before calling the SendRPC call isn't enough to prevent duplicates, as:

• The transmission of IDONTWANTs isn't arriving by the time we start transmitting bytes
• The IDONWANT study isn't implemented and therefore, there is no cancellation of the msg transmission as the spec suggests.

The following figure shows the CDF of the elapsed time between the IDONTWANT notification sent to a peer (for a msg ID) and the arrival of the duplicated message from the same peer (and the same msg ID), supporting that the remote node should have enought time to cancel any ongoing transmission.

As a result of the study and a follow-up discussion with @MarcoPolo and @cskiraly on how could we address the issue, the idea of shifting GossipSub from a single-stream protocol to a multiple-stream one (on per msg) has popped up. Change that could:

• enable concurrent RPCs to the same Peer
• cancel/reset any stream where we know we are sending a specific message.