Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId

[dmitry-markin]

Description

Listener variant of ConnectedPoint is meaningless in DialError::WrongPeerId, so this PR replaces ConnectedPoint with Multiaddr (previously returned in the Dialer variant).

Links to any relevant issues

This change is proposed in #2767.

Open Questions

I'm unsure whether it makes sense to also include role_override into WrongPeerId error. This PR just drops it.

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

[mxinden]

Instead of a panic at runtime I would prefer making this state impossible at compile time.

Would splitting PendingConnectionError be an option @dmitry-markin?

[dmitry-markin]

Thanks, this seems reasonable, I'll try to do it.

[dmitry-markin]

We can replace

rust-libp2p/swarm/src/connection/pool.rs

Lines 622 to 731 in a4110a2

	let (endpoint, concurrent_dial_errors) = match (endpoint, outgoing) {
	(PendingPoint::Dialer { role_override }, Some((address, errors))) => (
	ConnectedPoint::Dialer {
	address,
	role_override,
	},
	Some(errors),
	),
	(
	PendingPoint::Listener {
	local_addr,
	send_back_addr,
	},
	None,
	) => (
	ConnectedPoint::Listener {
	local_addr,
	send_back_addr,
	},
	None,
	),
	(PendingPoint::Dialer { .. }, None) => unreachable!(
	"Established incoming connection via pending outgoing connection."
	),
	(PendingPoint::Listener { .. }, Some(_)) => unreachable!(
	"Established outgoing connection via pending incoming connection."
	),
	};
	let error: Result<(), PendingInboundConnectionError<_>> = self
	.counters
	// Check general established connection limit.
	.check_max_established(&endpoint)
	.map_err(PendingConnectionError::ConnectionLimit)
	// Check per-peer established connection limit.
	.and_then(|()| {
	self.counters
	.check_max_established_per_peer(num_peer_established(
	&self.established,
	obtained_peer_id,
	))
	.map_err(PendingConnectionError::ConnectionLimit)
	})
	// Check expected peer id matches.
	.and_then(|()| {
	if let Some(peer) = expected_peer_id {
	if peer != obtained_peer_id {
	Err(PendingConnectionError::WrongPeerId {
	obtained: obtained_peer_id,
	endpoint: endpoint.clone(),
	})
	} else {
	Ok(())
	}
	} else {
	Ok(())
	}
	})
	// Check peer is not local peer.
	.and_then(|()| {
	if self.local_id == obtained_peer_id {
	Err(PendingConnectionError::WrongPeerId {
	obtained: obtained_peer_id,
	endpoint: endpoint.clone(),
	})
	} else {
	Ok(())
	}
	});
	if let Err(error) = error {
	self.spawn(
	poll_fn(move |cx| {
	if let Err(e) = ready!(muxer.poll_close_unpin(cx)) {
	log::debug!(
	"Failed to close connection {:?} to peer {}: {:?}",
	id,
	obtained_peer_id,
	e
	);
	}
	Poll::Ready(())
	})
	.boxed(),
	);
	match endpoint {
	ConnectedPoint::Dialer { .. } => {
	return Poll::Ready(PoolEvent::PendingOutboundConnectionError {
	id,
	error: error
	.map(|t| vec![(endpoint.get_remote_address().clone(), t)]),
	handler,
	peer: expected_peer_id.or(Some(obtained_peer_id)),
	})
	}
	ConnectedPoint::Listener {
	send_back_addr,
	local_addr,
	} => {
	return Poll::Ready(PoolEvent::PendingInboundConnectionError {
	id,
	error,
	handler,
	send_back_addr,
	local_addr,
	})
	}
	};
	}

with two big arms for PendingPoint::Dialer & PendingPoint::Listener, duplicating common code on lines 651-706 for each branch. This way there will be no ambiguity whether we are handling Dialer or Listener related errors. We'll also need to duplicate code in https://github.com/libp2p/rust-libp2p/blob/a4110a2b6939d5b460f11df6dd158308f517becf/swarm/src/connection/error.rs for PendingInboundConnectionError & PendingOutboundConnectionError, which won't have the common base after these changes.

I can't weigh the code duplication vs panicking in impossible branch in this case. Should I proceed in the way described above @mxinden?

[mxinden]

with two big arms for PendingPoint::Dialer & PendingPoint::Listener, duplicating common code on lines 651-706 for each branch. This way there will be no ambiguity whether we are handling Dialer or Listener related errors.

I would hope that we could do without the duplication here. Though I would have to play around with it myself.

We'll also need to duplicate code in https://github.com/libp2p/rust-libp2p/blob/a4110a2b6939d5b460f11df6dd158308f517becf/swarm/src/connection/error.rs for PendingInboundConnectionError & PendingOutboundConnectionError, which won't have the common base after these changes.

👍 Off the top of my head, that seems reasonable to me.

---

Note that some of this might change in the near future with #2824. One potential implementation may be #2828, though as you can see that is still work in progress.

[dmitry-markin]

Should I wait for #2828 to be merged before implementing the approach proposed above?

[mxinden]

Yes, I think that is a good idea. 🙏

In case you are interested in contributing in the meantime, many of the help wanted issues don't conflict.

[thomaseizinger]

#2824 is now resolved although with a different approach than in #2828 in case you want to revive this PR @dmitry-markin.

[mergify]

This pull request has merge conflicts. Could you please resolve them @dmitry-markin? 🙏