Extend NetworkBehaviour to directly share Swarm states

[drHuangMHT]

Is it possible to introduce a method, let's call it poll_with_swarm_state, to share states between Swarm and behaviours?

pub trait NetworkBehaviour {
    fn poll(&mut self, cx: &mut Context<'_>) -> Poll<ToSwarm>;

    fn poll_with_swarm_state(&mut self, cx: &mut Context<'_>, swarm_state: SwarmState<'_>) -> Poll<ToSwarm> {
        self.poll(cx) // Default implementation.
    }
}

SwarmState<'_> can contain information like Swarm::connected_peers() Swarm::listeners(), etc. Or even share the intergrated peer store.

[dariusc93]

What would be the motivation for this if you dont mind me asking?

[drHuangMHT]

Here I quote from Dave here

I'm starting to get the sense that the Peer information is fragmented too much in rust-libp2p. The Swarm has a cache. Kademlia has a cache. Gossipsub has a cache. On and on and on. They all store different things and have different heuristics for adding/removing/updating records.

For example:

rust-libp2p/protocols/kad/src/behaviour.rs

Lines 84 to 87 in 3554754

	/// The currently connected peers.
	///
	/// This is a superset of the connected peers currently in the routing table.
	connected_peers: FnvHashSet<PeerId>,

Also #5718 can be mitigated with a first-hand known list of peer(though the actual effectiveness is still to-be-discussed).

And potentially a way of sharing SignedPeerRecord because in some cases it needs to be "requested", which is not very ergonomic with current way of doing cross-behaviour communication. For example #2398.

[jxs]

Thanks for starting this discussion DrHuang.

I'm starting to get the sense that the Peer information is fragmented too much in rust-libp2p. The Swarm has a cache. Kademlia has a cache. Gossipsub has a cache. On and on and on. They all store different things and have different heuristics for adding/removing/updating records.

yup, we have noticed this, specially for connected peers, each Behaviour basically has its own list. But I challenge that just passing them on poll is enough for behaviors like gossipsub which needs to keep a list of peers permanently for when publish is called for example.
It also extends the connected peer list with peer details.
Cc @elenaf9

[drHuangMHT]

But I challenge that just passing them on poll is enough for behaviors like gossipsub which needs to keep a list of peers permanently for when publish is called for example.

publish is not bound to poll because PeerDetail contains channel to connection handler directly, hence doesn't need ToSwarm::NotifyHandler. So yeah in this case the benefit is limited, unless major refactor is made.

[diegomrsantos]

Is gossipsub assuming that every connected peer supports the protocol?

[elenaf9]

I guess connected_peer tracks all currently connected peers, independently of whether they support the gossipsub protocol.

That said, you are making a good point. The behavior in practice uses that list when selecting e.g. the peers that a message should be published to, in which case we probably only want to have peers that actually support gossipsub. So maybe we should remove a peer from connected_peers if it doesn't support gossipsub? cc @jxs

[jxs]

DialUpgradeError with error: StreamUpgradeError::NegotiationFailed happens when no protocol from the (derived) NetworkBehaviours can be agreed upon, and all the ConnectionHandlers are called with it.

If a peer supports at least one protocol besides gossipsub the code DrHuang linked won't be triggered. In that case connected_peers serves another purpose Elena, it reports to the Swarm if a peer doesn't support gossipsub.

This was probably used with lighthouse's usecase in mind, as it bans peers which don't support gossipsub.
We can discuss generalizing this behavior so that a user can be informed of which protocols the connected peer supports if needed

[drHuangMHT]

If a peer supports at least one protocol besides gossipsub the code DrHuang linked won't be triggered.

I don't actually expect that. I believe you can request only one type(protocol string) of substream at a time, with information to match against composed stream type list on the remote, right? If the matching failed, it should be NegotiationFailed for that specific request. regardless of other supported protocol strings.

[drHuangMHT]

Also gossipsub needs some refactor to utilize this feature, because connected_peers is checked at Behaviour::publish() it self, not during NetworkBehaviour::poll().

[elenaf9]

I agree with @drHuangMHT.
If one handler creates a new OutboundSubstreamRequest to negotiate a new substream¹, it will only try to negotiate that handler's protocol with the remote², and if it fails it will return the DialUpgradeerror only to that handler³

In that case connected_peers serves another purpose Elena, it reports to the Swarm if a peer doesn't support gossipsub.

But couldn't we still remove the peer from connected_peers after we reported the GossipsubNotSupported event to the swarm?

Footnotes

1. https://github.com/libp2p/rust-libp2p/blob/00e0d29c9e1584655d97a0d96fa26cc336e34698/swarm/src/handler/select.rs#L234-L240 ↩

2. https://github.com/libp2p/rust-libp2p/blob/00e0d29c9e1584655d97a0d96fa26cc336e34698/swarm/src/handler/select.rs#L195-L196 ↩

3. https://github.com/libp2p/rust-libp2p/blob/00e0d29c9e1584655d97a0d96fa26cc336e34698/swarm/src/handler/select.rs#L322-L331 ↩