Return a sequence of Multiaddrs in listen_on.

[twittner]

This is part of the proposed changes to the Transport trait as outlined in [1].

To allow transport implementations to listen on multiple addresses we now report all those addresses as a sequence of multi-addresses instead of supporting only a single one.

Instead of making Transport::listen_on return a generic iterator a concrete type MultiaddrSeq is introduced to support non-emptiness.

[tomaka]

Why use a MultiaddrSeq instead of an iterator?

[twittner]

Why use a MultiaddrSeq instead of an iterator?

From #1028 (comment):

[...] to support non-emptiness.

By using a concrete type we get more specific semantics and a simpler implementation. Using a Iterator + Clone type does not buy us much here.

[tomaka]

Well, that's clearly wrong. A connection only arrives on a single multiaddress. We need to put the multiaddr a connection arrives on in the ListenerEvent.

[tomaka]

to support non-emptiness.

Do we not want to support emptiness? For the JsTransport (#1004) I basically need to return an empty list at the beginning.

[twittner]

Currently, Transport::listen_on returns a pair of (Listener, Multiaddr). Note that this is Multiaddr, not Option<Multiaddr>. The proposal in [1] extends the Multiaddr to "list of multiaddresses" which I understood to mean "one or more" given that we currently require an address to be returned. If we want to support listeners without an (immediate) listen address why returning a list/iterator at all? The proposed stream event API would have to include for each upgrade the listener address anyway as we would otherwise not know which of the potentially many addresses the listener used when creating this event. So instead of the proposed API in [1], I would suggest we use the one below which only returns a Listener from Transport::listen_on. The address information is embedded in ListenerEvent. A listener may start without any address, but always has to say which listener address was used when creating an upgrade event. It may also later transition to not having a known address by generating AddressExpired events for all its currently known addresses, but eventually has to come back with one when the next upgrade is created.

pub trait Transport {
    type Output;
    type Error: error::Error;

    type Listener: Stream<Item = ListenerEvent<Self::ListenerUpgrade>, Error = Self::Error>;
    type ListenerUpgrade: Future<Item = Self::Output, Error = Self::Error>;
    type Dial: Future<Item = Self::Output, Error = Self::Error>;

    fn listen_on(self, addr: Multiaddr) -> Result<Self::Listener, TransportError<Self::Error>>;

    fn dial(self, addr: Multiaddr) -> Result<Self::Dial, TransportError<Self::Error>>;
}

enum ListenerEvent<T> {
    Upgrade {
        upgrade: T,
        listen_addr: Multiaddr,
        remote_addr: Multiaddr
    },
    AddressExpired(Multiaddr)
}

This may also be a good time to revisit the proposal in [2] which got rejected at the time, to use Bytes instead of Vec for the internal representation of Multiaddr.

[tomaka]

If we want to support listeners without an (immediate) listen address why returning a list/iterator at all?

The very pragmatic reason is that a lot tests create a transport or a swarm, make it listen on something, then dial the returned address.

I'm not sure which decision to take here. In most situations you know your listened addresses immediately, which is why it makes sense to return them immediately and not in the future.

[tomaka]

I would suggest we use the one below

We still need to know the addresses that we listen on before we receive a connection.

The typical process in Substrate is: we start listening, then we dial a bootstrap node, then we indicate to this bootstrap node which addresses we're listening on, and these addresses are then advertised to the rest of the network.
If we don't have an event "StartedToListenOnAnAddress", then we can't do that and we will never receive any incoming connection.

[twittner]

I have a branch (master...twittner:listener-event-wip) which contains a work in progress of the interface outlined above, except that ListenerEvent is defined as:

/// Event produced by [`Transport::Listener`]s
#[derive(Clone, Debug, PartialEq)]
pub enum ListenerEvent<T> {
    /// The transport is listening on a new additional [`Multiaddr`].
    NewAddress(Multiaddr),
    /// An upgrade, consisting of the upgrade future, the listener address and the remote address.
    Upgrade {
        /// The upgrade.
        upgrade: T,
        /// The listening address which produced this upgrade.
        listen_addr: Multiaddr,
        /// The remote address which produced this upgrade.
        remote_addr: Multiaddr
    },
    /// A [`Multiaddr`] is no longer used for listening.
    AddressExpired(Multiaddr)
}

I do not think we should also have Transport::listen_on return an iterator over multi-addresses only because tests are written in that way currently. We should rather be prepared to deal with transports that return no immediate listen address. In fact supporting those transports has quite an impact on the API:

1. We need a separate stable identifier to refer to individual listeners as the set of listen addresses is volatile and potentially empty.
2. Swarm::listen_on can no longer return a Multiaddr. Listen addresses become available only when polling the swarm.

Most tests can be adjusted with little effort, however I am unsure how to make listen addresses available to users of Swarm. ListenersStream and RawSwarm produce events so it is easy enough to poll those, but Swarm calls the various behaviour methods and only produces behaviour events when polled. It would seem to me that Swarm::poll needs to be changed to return a SwarmEvent instead of only behaviour events if we want to make the listen addresses available to code outside of behaviour implementations. What is your opinion?

[tomaka]

Most tests can be adjusted with little effort,

That was my main concern, but if they can be updated then that's good.

It would seem to me that Swarm::poll needs to be changed to return a SwarmEvent instead of only behaviour events

That's obviously not great, as the Swarm at the moment is more or less just a wrapper around the NetworkBehaviour in terms of API.

[twittner]

I am closing this PR and point to #1032 which has been rebased to not be a continuation of this PR, but to implement the basic interface described above.