swarm: Remove PollParameters

[thomaseizinger]

Description

Remove the PollParameters trait.

• local_peer_id can be removed in favor of users having to pass it into their behaviour in the constructor.
• external_addresses can be built up from the inject_new_external_addr and inject_expired_external_addr callbacks.
• listened_addresses can be built up from the inject_new_listen_addr and inject_expired_listen_addr callbacks

The only function that does not have a replacement at the moment is supported_protocols.

Any ideas for how we could model that one differently?

As per usual, we should first deprecate the existing functions, give users a release or two to upgrade and then remove the deprecated code.

Motivation

rust-libp2p is primarily designed around an async message-passing system. Accessing information synchronously as part of the poll function is not inline with this philosophy.

Current Implementation

Are you planning to do it yourself in a pull request?

Maybe.

[mxinden]

I am in favor of removing PollParameters.

The only function that does not have a replacement at the moment is supported_protocols.

I suggest similar to how we would communicate supported protocols of remote peers via #2680, we can do the same for supported protocols of the local peer.

As an aside, the set of supported protocols can change at runtime. E.g. libp2p-kad with #2032 will only offer the Kademlia protocol for incoming streams when not behind a firewall or NAT. Thus the current mechanism is flawed.

[thomaseizinger]

I am in favor of removing PollParameters.

The only function that does not have a replacement at the moment is supported_protocols.

I suggest similar to how we would communicate supported protocols of remote peers via #2680, we can do the same for supported protocols of the local peer.

As an aside, the set of supported protocols can change at runtime. E.g. libp2p-kad with #2032 will only offer the Kademlia protocol for incoming streams when not behind a firewall or NAT. Thus the current mechanism is flawed.

In theory, it could be that one connection of a node is behind firewall or NAT and the other one isn't. For example, lets say I am listening on TCP and WebRTC connections but only the WebRTC one is port-forwarded.

This thought experiment suggests that we'd have to tell our behaviour the supported protocols PER connection. In a way, this would fit in well with #3099 where we only learn our supported protocols after we established the connection to another peer.

Thoughts?

[mxinden]

In theory, it could be that one connection of a node is behind firewall or NAT and the other one isn't. For example, lets say I am listening on TCP and WebRTC connections but only the WebRTC one is port-forwarded.

While I am sure we will find this case in the wild, I would assume that it is rare. Thus I suggest not designing for it.

[rkuhn]

My only (but important!) use-case for supported_protocols is that my Identify information going to remote hosts should accurately reflect my set of available protocols with that peer — I use that in multiple places to keep things compatible in a long-term fashion. So if we can enable a behaviour to obtain that list of protocols when an outbound substream is about to be opened, that would be enough.

[thomaseizinger]

My only (but important!) use-case for supported_protocols is that my Identify information going to remote hosts should accurately reflect my set of available protocols with that peer — I use that in multiple places to keep things compatible in a long-term fashion. So if we can enable a behaviour to obtain that list of protocols when an outbound substream is about to be opened, that would be enough.

Yes. I think that is the right way to think about it. Because ConnectionHandlers end up deciding about the protocols they use / offer to the other party, we should scope the supported protocols by peer as well.

At the moment, a ConnectionHandler may decide to produce a different protocol for different substreams.

This makes it difficult to accurately cache this information. We'd almost have to tell the NetworkBehaviour upon every substream, whether the list of supported protocols changed.

Instead of that, we could also build a mechanism where a ConnectionHandler has to explicitly emit an "update" of its InboundProtocol if it wants to change it.

[rkuhn]

Yes, that sounds good (if my understanding is correct): ConnectionHandler is interrogated for supported protocols at start of connection and can update its contribution by emitting an event, and that (updated) set is available to all sub-handlers of the same connection (by some means TBD). Is that what you meant?

[thomaseizinger]

available to all sub-handlers

I wouldn't make the supported protocols available to the other handlers but report it back to the behaviour via a (newly introduced) FromConnection event:

enum FromConnection {
	Event(E),
	ProtocolsChanged(Vec<String>) // Note: Simplified type here.
}

Protocols are specific to a connection so I don't think other connections need to know about them. A behaviour can always forward it though if really needed.

The only thing I don't really like about this design is that a ConnectionHandler ends up providing two ways of exposing protocols:

• The initially supported protocols
• The update

Maybe we could make handlers produce an update to begin with and until then, they don't support any protocols?

[rkuhn]

My question was not about other connections, it was specifically about handler::either::Either: given an Either<A, B>, what I’m asking is how B will learn of the protocols supported by A so that it can correctly advertise them (in case this is B’s core reponsibility, like B=Identify).

---

Regarding the Update mechanism: changes will be much less frequent (as witnessed by currently known protocols) than connections, so I’d find it quite reasonable to have a nice and easy (i.e. non-foot-gun) normal case (providing protocols via a method is type-checked) and a documented special case (forgetting to emit that event in some cases will lead to some head scratching when connections “half work”).

[mxinden]

Maybe we could make handlers produce an update to begin with and until then, they don't support any protocols?

On start-up the Swarm would call NetworkBehaviour::new_handler and call ConnectionHandler::listen_protocol and then extract the supported protocols @thomaseizinger?

[thomaseizinger]

Maybe we could make handlers produce an update to begin with and until then, they don't support any protocols?

On start-up the Swarm would call NetworkBehaviour::new_handler and call ConnectionHandler::listen_protocol and then extract the supported protocols @thomaseizinger?

new_handler would only be called on the first connection, not at start-up.

But yes, listen_protocol would be called once the connection is established.

Perhaps instead of producing an update, the handler could produce an event that just invalidates the "cache" for the supported protocols. A connection would then inquire again about the supported protocols via listen_protocol.

[thomaseizinger]

Instead of treating it as a cache, we could also call listen_protocol after each update to the handler.

We would need some kind of diffing strategy to not emit updates about unchanged protocols to the behaviour.

Ideally, before we do that, we already got rid of the "upgrade" notation to have listen_protocol only return the protocol string.

[thomaseizinger]

Given that we merged #3153, I am assuming we have agreed that this is a good idea.

[thomaseizinger]

With #3651, PollParameters will be completely deprecated and removed in the next release after this one 🎉