[GossipSub 1.2] Add IDONTWANT support #374
Conversation
| @@ -122,6 +122,7 @@ open class GossipRouter( | |||
| private val iAsked = createLRUMap<PeerHandler, AtomicInteger>(MaxIAskedEntries) | |||
| private val peerIHave = createLRUMap<PeerHandler, AtomicInteger>(MaxPeerIHaveEntries) | |||
| private val iWantRequests = createLRUMap<Pair<PeerHandler, MessageId>, Long>(MaxIWantRequestsEntries) | |||
| private val peerIDontWant = createLRUMap<PeerHandler, Pair<AtomicInteger, MutableSet<MessageId>>>(MaxPeerIDontWantEntries) | |||
There was a problem hiding this comment.
May we have an explicit structure for Pair<AtomicInteger, MutableSet<MessageId>> for clarity?
There was a problem hiding this comment.
Added IDontWantCacheEntry
| @@ -572,9 +590,14 @@ open class GossipRouter( | |||
| enqueueIwant(peer, messageIds) | |||
| } | |||
|
|
|||
| private fun iDontWant(peer: PeerHandler, messageIds: List<MessageId>) { | |||
| if (messageIds.isEmpty()) return | |||
| enqueueIdontwant(peer, messageIds) | |||
There was a problem hiding this comment.
As I understand you are going to add the filtering here yet?
| @@ -572,9 +590,14 @@ open class GossipRouter( | |||
| enqueueIwant(peer, messageIds) | |||
| } | |||
|
|
|||
| private fun iDontWant(peer: PeerHandler, messageIds: List<MessageId>) { | |||
| if (messageIds.isEmpty()) return | |||
| enqueueIdontwant(peer, messageIds) | |||
There was a problem hiding this comment.
I believe it's worth to add self rate limiting here as well ?
There was a problem hiding this comment.
Or probably it doesn't make much sense if this is not penalized
| @@ -572,9 +590,14 @@ open class GossipRouter( | |||
| enqueueIwant(peer, messageIds) | |||
| } | |||
|
|
|||
| private fun iDontWant(peer: PeerHandler, messageIds: List<MessageId>) { | |||
| if (messageIds.isEmpty()) return | |||
| enqueueIdontwant(peer, messageIds) | |||
There was a problem hiding this comment.
The thing of IDONTWANT is that it needs to be flushed immediately to have the effect
| @@ -459,6 +475,8 @@ open class GossipRouter( | |||
| .whenTrue { notifyIWantTimeout(key.first, key.second) } | |||
| } | |||
|
|
|||
| peerIDontWant.clear() | |||
There was a problem hiding this comment.
We may clear remote IDONTWANT which is still in effect here. We probably need some more sophisticated pruning mechanism here
| /** | ||
| * [maxIDontWantMessages] is the maximum number of IDONTWANT messages per heartbeat per peer | ||
| */ | ||
| val maxIDontWantMessages: Int = 10 |
There was a problem hiding this comment.
I believe it's pretty low default which may mitigate the whole purpose of IDONTWANT.
I would default it to maxIHaveLength * maxIHaveMessages as proposed here
This one would be pretty high but any client may make it more conservative if needed.
With respect to Ethereum PeerDAS we may legally have up to 128 messages in a single slot for data column subnets
StefanBratanov
force-pushed
the
gossip_1_2
branch
from
01ea9a7 to
b4d6588
Compare
| if (!this.protocol.supportsIDontWant()) return | ||
| if (msg.protobufMessage.data.size() < params.iDontWantMinMessageSizeThreshold) return | ||
| // we need to send IDONTWANT messages to mesh peers immediately in order for them to have an effect | ||
| msg.topics.forEach { topic -> |
There was a problem hiding this comment.
If there are more than 1 topic wouldn't we send duplicate messageId to same peers?
There was a problem hiding this comment.
Ah that is a good point, fixed in bbe42fa
TODO:
fixes #371