fix peer_id in case of DialError::InvalidPeerId (#2425) #2428
Conversation
core/src/connection/error.rs
Outdated
| /// match the one that was expected or is otherwise invalid. | ||
| InvalidPeerId, | ||
| /// match the one that was expected or is the local one. | ||
| InvalidPeerId(PeerId), |
There was a problem hiding this comment.
| InvalidPeerId(PeerId), | |
| InvalidPeerId { | |
| obtained: PeerId, | |
| } |
What do you think? That would make it clear that the provided one in InvalidPeerId is the one obtained on the connection.
There was a problem hiding this comment.
Yes, and I’ll want to add the failed address if possible (looking into it now).
core/tests/network_dial_error.rs
Outdated
|
|
||
| swarm1.listen_on("/memory/0".parse().unwrap()).unwrap(); | ||
|
|
||
| let address = async_std::task::block_on(future::poll_fn(|cx| match swarm1.poll(cx) { |
There was a problem hiding this comment.
Side comment: It is unfortunate that Network does not implement Future. Otherwise this could be simplified a lot.
core/tests/network_dial_error.rs
Outdated
| assert_eq!(peer_id, other_id); | ||
| match error { | ||
| PendingConnectionError::InvalidPeerId(id) => assert_eq!(id, *swarm1.local_peer_id()), | ||
| x => panic!("wrong error {:?}", x), | ||
| } |
There was a problem hiding this comment.
Why not do both of these within the match above?
There was a problem hiding this comment.
I prefer to assert on the test function level when possible, for ease of stack trace reading (and reliability of reporting).
|
@mxinden Thanks for the review! While trying this out and working a bit more on Actyx & ipfs-embed I found that InvalidPeerId should contain the address at which the mismatch occurred — since the user will very likely want to scratch that Multiaddr from any lists they may be maintaining (in conjunction with the expected PeerId). The WASM failures seem unrelated to my changes (at least I hope so). |
core/src/connection/pool.rs
Outdated
| } | ||
| } | ||
| Ok(()) | ||
| }) | ||
| // Check peer is not local peer. | ||
| .and_then(|()| { | ||
| if self.local_id == peer_id { | ||
| Err(Error::InvalidPeerId) | ||
| Err(PendingConnectionError::InvalidPeerId { | ||
| obtained: peer_id, |
There was a problem hiding this comment.
How about renaming peer_id to obtained_peer_id? That would be consistent with expected_peer_id and would be less confusing.
There was a problem hiding this comment.
yes, will do that once I get a minute — crunch time right now
Previously, the negotiated PeerId was included in the swarm event and inject_dial_failure’s arguments while the expected one was absent. This patch adds the negotiated PeerId to the DialError and includes the expected one in the notifications.
Co-authored-by: Max Inden <mail@max-inden.de>
|
Seeing that you added an actual case of “invalid peer id” in #2404, I think this PR needs to cover a bit more ground: we want to signal two distinct cases now. How about the following:
|
👍 thanks for being rigorous here.
👍
I haven't been involved back then. I guess due to That said, I agree that this is not ideal. I would very much like this to be enforced at the type system level.
👍 |
also add changelog entries
rkuhn
force-pushed
the
rku/invalid-peer-id
branch
from
a0924ba
to
561b922
Compare
| /// the connection did not match the one that was expected or is otherwise | ||
| /// invalid. | ||
| InvalidPeerId, | ||
| /// The provided peer identity is invalid |
There was a problem hiding this comment.
| /// The provided peer identity is invalid | |
| /// The provided peer identity is invalid. |
| /// match the one that was expected or is the local one. | ||
| WrongPeerId { | ||
| obtained: PeerId, | ||
| address: Multiaddr, |
There was a problem hiding this comment.
| address: Multiaddr, | |
| address: ConnectedPoint, |
How about a ConnectedPoint instead of a plain Multiaddr here? Mostly for the sake of additional context.
|
Seems like I am not allowed to push to this branch. I will merge this via #2441. |
Previously, the negotiated PeerId was included in the swarm event and
inject_dial_failure’s arguments while the expected one was absent. This
patch adds the negotiated PeerId to the DialError and includes the expected
one in the notifications.