feat(swarm): allow NetworkBehaviours to create and remove listeners

[dariusc93]

Description

This extends ToSwarm to add ToSwarm::ListenOn and ToSwarm::RemoveListener, which allows creating and removing listeners from a NetworkBehaviour.

Resolves #3291.

Notes

Links to any relevant issues

Open Questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

Overall I am in support of this but we should probably wait until we have the connection management story figured out or at least found consensus on #3290.

[mxinden]

@dariusc93 note that the connection management work, namely #3254, is merged, thus no longer blocking this pull request.

[dariusc93]

@dariusc93 note that the connection management work, namely #3254, is merged, thus no longer blocking this pull request.

Thank you for letting me know. I would have to double check, but was it ever decided on rather a ListenerId would be supplied to the Transport through the event or would the current changes in this PR suffice for the time being?

[thomaseizinger]

@dariusc93 note that the connection management work, namely #3254, is merged, thus no longer blocking this pull request.

Thank you for letting me know. I would have to double check, but was it ever decided on rather a ListenerId would be supplied to the Transport through the event or would the current changes in this PR suffice for the time being?

I think we will need to issue the ListenerId as part of the command. Not only will this make things more consistent, it is also the only way how a NetworkBehaviour can effectively manage listeners and not having to reverse-engineer them from the addresses.

[dariusc93]

@dariusc93 note that the connection management work, namely #3254, is merged, thus no longer blocking this pull request.

Thank you for letting me know. I would have to double check, but was it ever decided on rather a ListenerId would be supplied to the Transport through the event or would the current changes in this PR suffice for the time being?

I think we will need to issue the ListenerId as part of the command. Not only will this make things more consistent, it is also the only way how a NetworkBehaviour can effectively manage listeners and not having to reverse-engineer them from the addresses.

Providing the ListenerId should be simple enough in this case (and honestly would be better for tracking, than trying to sort it out, like you said). I would then take it that instead of using Swarm::listen_on in Swarm::handle_behaviour_event in my PR, I would use the transport itself with the provided id, while in Swarm::listen_on, for the sake of compatibility, to generate a ListenerId there and pass it onto the transport?

This can be done outside of this PR too in its own separate PR

EDIT: I made a PR at #3567 just to get your thoughts on the change.

[thomaseizinger]

If this fails, the NetworkBehaviour will never learn about it. I think we need to capture this error and report it back to the NetworkBehaviour as well as issue a ListenerError.

The documentation on ListenerError talks about "non-fatal" errors but I think adding another variant is even more confusing.

[dariusc93]

I was thinking about this back during the initial PR, but I assumed it would be alright since ToSwarm::Dial ignores the error. We could do this instead

Suggested change

	self.transport.listen_on(id, address).ok()?;
	if let Err(e) = self.transport.listen_on(id, address) {
	self.behaviour.on_swarm_event(FromSwarm::ListenerError(
	behaviour::ListenerError {
	listener_id: id,
	err: &e,
	},
	));
	}

So it would notify the behaviour of the error instead of ignoring it.

[thomaseizinger]

I was thinking about this back during the initial PR, but I assumed it would be alright since ToSwarm::Dial ignores the error.

Are we? Probably also something that should be fixed then :)

We could do this instead

Suggested change

	self.transport.listen_on(id, address).ok()?;
	if let Err(e) = self.transport.listen_on(id, address) {
	self.behaviour.on_swarm_event(FromSwarm::ListenerError(
	behaviour::ListenerError {
	listener_id: id,
	err: &e,
	},
	));
	}

So it would notify the behaviour of the error instead of ignoring it.

Yep, that looks good to me!

[dariusc93]

I was thinking about this back during the initial PR, but I assumed it would be alright since ToSwarm::Dial ignores the error.

Are we? Probably also something that should be fixed then :)

Looking at Swarm::dial, it does send the error to the behaviour internally before returning the error itself so it doesnt look to be necessary

EDIT: Overlooked the beginning, but it looks like it doesnt send anything for dial_opts.get_or_parse_peer_id().map_err(DialError::InvalidPeerId), but not sure if this would need to be sent to the behaviour. If it does we could do this internally there in the function by doing

let peer_id = match dial_opts
            .get_or_parse_peer_id()
            .map_err(DialError::InvalidPeerId)
        {
            Ok(peer_id) => peer_id,
            Err(e) => {
                self.behaviour
                    .on_swarm_event(FromSwarm::DialFailure(DialFailure {
                        peer_id: None,
                        error: &e,
                        connection_id,
                    }));
                return Err(e);
            }
        };

[thomaseizinger]

That error will go away once we have multiformats/rust-multiaddr#73.

[mxinden]

@thomaseizinger do the latest commits address your concerns?

[thomaseizinger]

A few minor things but overall, happy to merge this design. Thank you for iterating on this with me @dariusc93 !

[thomaseizinger]

Thanks!

[mxinden]

@dariusc93 thanks for being patient with all the feedback. Your work on this is valued.