protocols/gossipsub: Review #93
Conversation
protocols/gossipsub/src/behaviour.rs
Outdated
| @@ -199,6 +202,8 @@ impl From<MessageAuthenticity> for PublishConfig { | |||
| type GossipsubNetworkBehaviourAction<T> = | |||
| NetworkBehaviourAction<Arc<rpc_proto::Rpc>, GenericGossipsubEvent<T>>; | |||
|
|
|||
| // mxinden: Again, why prefix `Generic` here? Do we follow this pattern anywhere else? | |||
There was a problem hiding this comment.
All the objects that contain the raw data now have generic versions (which are only exposed for advanced use cases), we expect the general use should not require these Generic types.
protocols/gossipsub/src/behaviour.rs
Outdated
| /// Counts the number of `IHAVE` received from each peer since the last heartbeat. | ||
| count_peer_have: HashMap<PeerId, usize>, | ||
|
|
||
| /// Counts the number of `IWANT` that we sent the each peer since the last heartbeat. | ||
| count_iasked: HashMap<PeerId, usize>, | ||
|
|
||
| // mxinden: Do I understand that this is for anonymous or random author messages only? If so, |
There was a problem hiding this comment.
Yep. A scoring parameter prevents messages being re-routed that we published. For random and annonymous author messages this is harder to determine and we need to keep track of our published messages to handle this.
There are triangle routing connections which are supposedly unlikely but still allows these kinds of messages to be sent back to us.
Will extend the comments
protocols/gossipsub/src/behaviour.rs
Outdated
| @@ -289,6 +301,9 @@ pub struct GenericGossipsub<T: AsRef<[u8]>, Filter: TopicSubscriptionFilter> { | |||
| subscription_filter: Filter, | |||
| } | |||
|
|
|||
| // mxinden: I am fine breaking backwards compatibility at such an early stage. Can you elaborate why | |||
There was a problem hiding this comment.
As explained above, we expect most users to just use this type. More advanced users can adjust the internals.
Its more a convenience type to hide added complexities from the average user.
protocols/gossipsub/src/handler.rs
Outdated
| // NOTE: Use this flag rather than checking the substream count each poll. | ||
| peer_kind_sent: bool, | ||
|
|
||
| /// If the peer doesn't support the gossipsub protocol we do not immediately disconnect. | ||
| /// Rather, we disable the handler and prevent any incoming or outgoing substreams from being | ||
| /// established. | ||
| // | ||
| // mxinden: Why do this? |
There was a problem hiding this comment.
IIRC the default behaviour for an unsupported protocol is for the handler to throw an error.
Throwing an error there means an instant disconnection. For a handler that is composed of multiple protocols, it may be useful to allow peers to support some protocols and not others. In this case if gossipsub is not supported, the peer is not immediately disconnected and libp2p can continue to function on other protocols.
In the case that there is only gossipsub, this will not create any substreams, set the KeepAlive to False and the peer will be disconnected (as typical composed handlers compose the KeepAlive parameter)
I could be missing something with the design, but I think this makes it generic for multiple protocols, such that we don't force all peers to support gossipsub if gossipsub is supported locally.
protocols/gossipsub/src/handler.rs
Outdated
| @@ -163,6 +167,10 @@ impl GossipsubHandler { | |||
| peer_kind_sent: false, | |||
| protocol_unsupported: false, | |||
| upgrade_errors: VecDeque::new(), | |||
| // mxinden: In the case where another NetworkBehaviour requests a connection to a remote | |||
There was a problem hiding this comment.
I don't think so. Maybe we could make this more explicit however.
When a new connection is established, gossipsub will try to send any subscriptions to the peer. To do this it forces a negotiation attempt. If the negotiation fails then the keep_alive gets adjusted.
There is however the edge case that locally we are not subscribed to any topics. In this case we don't send/receive any messages on gossipsub messages but the handler maintains the keep-alive. As soon we subscribe to anything or get a message from a peer on gossipsub we have to start the negotiation.
Perhaps this should be set to a timeout limit to handle this case?
There was a problem hiding this comment.
Perhaps this should be set to a timeout limit to handle this case?
Yes, from what I can tell a timeout would take care of most edge-cases. Keep-alive handling has cost us many debugging hours in the past (e.g. see libp2p#1698). I tried to simplify the concept with libp2p#1717, but without success thus far. Happy for input in case you have ideas.
protocols/gossipsub/src/config.rs
Outdated
| @@ -423,6 +429,8 @@ impl<T> GenericGossipsubConfig<T> { | |||
| self.flood_publish | |||
| } | |||
|
|
|||
| // mxinden: Why // instead of ///? | |||
There was a problem hiding this comment.
Thats a mistake and should be /// you are right.
Given the size of libp2p#1720 I did my review directly inline. A lot of suggestions are just related to styling - I hope delivering my review as a pull request makes accepting those easier. Comments are prefixed with
// mxinden:.Let me know if this format works for you. If not, I can transform this into a traditional Github review.
Again, as libp2p#1720 is quite large, I have not yet fully reviewed all changes / additions.