Staggered message sending with elimination of peers during transmission based on idontwants #1100
Conversation
…eers (even during transmissions) based on idontwants
| @@ -529,6 +538,11 @@ method rpcHandler*(g: GossipSub, | |||
|
|
|||
| libp2p_gossipsub_duplicate.inc() | |||
|
|
|||
| #Dont relay to the peers from which we already received | |||
| #We do it for large messages only | |||
| if msg.data.len > msgId.len * 10: | |||
There was a problem hiding this comment.
if the message is in seen, it should not be sent to anyone meaning we should not need to track it individually (doing so costs significant amounts of memory)
There was a problem hiding this comment.
actually, right - the seen check has been done and we need to stop the message after the seen check . hm . it would likely be better that we keep track of in-flight / queued messages and cancel these sends specifically on idontwant rather than storing all idontwant hashes
There was a problem hiding this comment.
Don't we already have this here?
There was a problem hiding this comment.
actually, right - the seen check has been done and we need to stop the message after the seen check . hm . it would likely be better that we keep track of in-flight / queued messages and cancel these sends specifically on idontwant rather than storing all idontwant hashes
Haven't this been already tried here? #851 (comment)
There was a problem hiding this comment.
I believe heDontWants is meant to handle idontwant msgs received and we already have the seen data structure to avoid relaying duplicates.
There was a problem hiding this comment.
This data structure is probably to store ID of msgs other peers in the network have already seen but we haven't yet. Then when we do see it, we don't send it to them.
There was a problem hiding this comment.
IMO our send inserts the message in non-priority queue, and may consume some time before its aired. if we receive the message during this interval, we can still cancel transmit
There was a problem hiding this comment.
True, but
and checkingidontwant when dequeuing from non-prio are enough. We don't seem to need this code for that.
There was a problem hiding this comment.
Maybe we could even end up not sending the msg to the current peer as it is enqueued and before sending we check idontwant.
There was a problem hiding this comment.
yes, this part does not have much impact!
|
broadly, limiting concurrent sends this way means two peers can hold up the queue for everyone else, including the fast peers - this is a key reason we haven't gone down this route. the limit would have to be a lot higher - ie somewhere on the order of dmax probably - we would also need to consider a timeout policy of individual sends stronger than what we have today. |
| @@ -418,10 +418,19 @@ proc validateAndRelay(g: GossipSub, | |||
| break | |||
| toSendPeers.excl(seenPeers) | |||
|
|
|||
| #We first send to the outbound peers to avoid peers sending same message to each other | |||
There was a problem hiding this comment.
Could you please elaborate on why that is the case?
There was a problem hiding this comment.
It was intended to avoid A sending to B and B sending to A at the same time,,, but yes, its not making much impact,,, due to the preceding IDontWants
| p.semTxLimit[].release() | ||
| await conn.close() | ||
|
|
||
| proc sendEncoded*(p: PubSubPeer, msg: seq[byte], isHighPriority: bool, saltedId: Option[SaltedId] = none(SaltedId)): Future[void] = |
There was a problem hiding this comment.
why is saltedId optional?
There was a problem hiding this comment.
There was a problem hiding this comment.
why is saltedId optional?
same SendMsg for control and published messages, so for some messages we did not need saltedID
There was a problem hiding this comment.
Currently, we can improve this design as I believe high-prio are only control and msgs published by the local peer. It means no other peer can send an idontwant for them. It means we can move the idontwant to the non-prio branch in this proc. Wdyt?
There was a problem hiding this comment.
In fact, If we were enqueueing those msgs and we didn't dequeue them fast enough, peers could receive them via peers we sent first. But as we always send them immediately, it seems this can't happen. Makes sense?
There was a problem hiding this comment.
Currently, we can improve this design as I believe high-prio are only control and msgs published by the local peer. It means no other peer can send an
idontwantfor them. It means we can move theidontwantto the non-prio branch in this proc. Wdyt?
yes, we dont need to check hedontwants for the messages published by the local peer,,, but i guess IDontWant should be transmitted with the highest priority
There was a problem hiding this comment.
In fact, i believe, IDontWant should be issued before this step. Because even if a message is not valid, we can react early to stop the spread of such message. And in case of valid messages, we get to learn early that our peers have received this message
There was a problem hiding this comment.
I meant that checking idontwant before sending a msg could be done only for relayed msgs, at line 417 of this file.
There was a problem hiding this comment.
yes for relayed messages only
This is configurable, but i could find very similar results for 2-5 simultaneous sends
I have tried this way to time-limit |
In fact (at-least) a slow peer can identify itself as a slow peer if it can not finish all sends within a strict time-limit. Also the peers for which we always miss the timebound. Thinking to try different locks for fast/slow peers |
closes PR1093