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?

[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 ↩

[jxs]

yeah you are right sorry. I clarified with Elena, my description refers to when there is a new inbound substream upgrade. On an outbound upgrade we ask for the specific protocols of the ConnectionHandler as you mention DrHuang

[drHuangMHT]

yeah you are right sorry. I clarified with Elena, my description refers to when there is a new inbound substream upgrade. On an outbound upgrade we ask for the specific protocols of the ConnectionHandler as you mention DrHuang

But the failure is largely ignored, right? I don't see code that handles ListenUpgradeError in most behaviours, they just kind of sweep the error under the carpet and never do anything about it. Maybe it is useful for detecting peer heterogeneity?