Skip to content

Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId #2793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId #2793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ad0595
Use `Multiaddr` instead of `ConnectedPoint` in `DialError::WrongPeerId`
dmitry-markin Aug 2, 2022
f72a513
Merge branch 'master' into fix-dial-error-wrong-peer-id
dmitry-markin Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions swarm/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- Update to `libp2p-core` `v0.35.0`.

- `DialError::WrongPeerId` now uses `Multiaddr` instead of ambiguous `ConnectedPoint`

- When deriving `NetworkBehaviour` on a custom `struct` where the user does not specify their own
`OutEvent` via `#[behaviour(out_event = "MyBehaviourEvent")]` and where the user does not enable
`#[behaviour(event_process = true)]`, then the derive macro generates an `OutEvent` definition for
Expand Down
27 changes: 13 additions & 14 deletions swarm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,7 +1441,7 @@ pub enum DialError {
/// The peer identity obtained on the connection did not match the one that was expected.
WrongPeerId {
obtained: PeerId,
endpoint: ConnectedPoint,
address: Multiaddr,
},
/// An I/O error occurred on the connection.
ConnectionIo(io::Error),
Expand All @@ -1454,9 +1454,15 @@ impl From<PendingOutboundConnectionError<io::Error>> for DialError {
match error {
PendingConnectionError::ConnectionLimit(limit) => DialError::ConnectionLimit(limit),
PendingConnectionError::Aborted => DialError::Aborted,
PendingConnectionError::WrongPeerId { obtained, endpoint } => {
DialError::WrongPeerId { obtained, endpoint }
}
PendingConnectionError::WrongPeerId { obtained, endpoint } => match endpoint {
ConnectedPoint::Dialer {
address,
role_override: _,
} => DialError::WrongPeerId { obtained, address },
ConnectedPoint::Listener { .. } => panic!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Would splitting PendingConnectionError be an option @dmitry-markin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"`Listener` endpoint found in `PendingOutboundConnectionError::WrongPeerId`"
),
},
PendingConnectionError::IO(e) => DialError::ConnectionIo(e),
PendingConnectionError::Transport(e) => DialError::Transport(e),
}
Expand All @@ -1482,7 +1488,7 @@ impl fmt::Display for DialError {
"Dial error: Pending connection attempt has been aborted."
),
DialError::InvalidPeerId(multihash) => write!(f, "Dial error: multihash {:?} is not a PeerId", multihash),
DialError::WrongPeerId { obtained, endpoint} => write!(f, "Dial error: Unexpected peer ID {} at {:?}.", obtained, endpoint),
DialError::WrongPeerId { obtained, address } => write!(f, "Dial error: Unexpected peer ID {} at {}.", obtained, address),
DialError::ConnectionIo(e) => write!(
f,
"Dial error: An I/O error occurred on the connection: {:?}.", e
Expand Down Expand Up @@ -1623,7 +1629,6 @@ mod tests {
use libp2p::yamux;
use libp2p_core::multiaddr::multiaddr;
use libp2p_core::transport::TransportEvent;
use libp2p_core::Endpoint;
use quickcheck::{quickcheck, Arbitrary, Gen, QuickCheck};
use rand::prelude::SliceRandom;
use rand::Rng;
Expand Down Expand Up @@ -2341,15 +2346,9 @@ mod tests {
}));
assert_eq!(peer_id.unwrap(), other_id);
match error {
DialError::WrongPeerId { obtained, endpoint } => {
DialError::WrongPeerId { obtained, address } => {
assert_eq!(obtained, *swarm1.local_peer_id());
assert_eq!(
endpoint,
ConnectedPoint::Dialer {
address: other_addr,
role_override: Endpoint::Dialer,
}
);
assert_eq!(address, other_addr);
}
x => panic!("wrong error {:?}", x),
}
Expand Down