fix(ConcurrentDial): Connections to the wrong peer considered errors #6107
Conversation
Previously, the ConcurrentDial would return the first connection that succeeded _regardless_ of whether that peer had the expected PeerId. This would cause a race condition between multiaddresses when an ip address was shared by two peers (i.e. a local network address). The first connection would win, and only then would the Pool check to see if it was the correct peer id. If it was not, the entire connection would fail with a WrongPeerId error. This MR converts WrongPeerId to a TransportError ErrorKind::PermissionDenied to allow all multiaddresses for a connection to be attempted prior to resolving the future.
| Some((addr, Ok(_))) => { | ||
| let e = TransportError::Other(std::io::ErrorKind::PermissionDenied.into()); | ||
| self.errors.push((addr, e)); | ||
| } |
There was a problem hiding this comment.
PermissionDenied is a bit misleading since it implies that the user doesn't have sufficient OS permissions to perform the action. Unfortunately there isn't really a good matching ErrorKind in this scenario. Maybe HostUnreachable, or just ErrorKind::Other..
But either way, with this change we'd never surface a DialError::WrongPeerId error anymore, even if all addresses belong to the same wrong peer-id (e.g. because the remote changed their identity), right? I am not sure if we should really change that.
There was a problem hiding this comment.
Agreed - this is the patch we are floating on our fork to get connections flowing again. Connections to 127, 192, and 10 subnets were always winning the race against the relayed connection which gave a near 100% connection failure rate.
I was having a hard time finding a “proper” fix for this without a much bigger change because WrongPeerId is not a transport error and “Other” at the transport level made it difficult to determine the cause of the error higher up in the call stack.
The only solutions I can come up with:
- give ConcurrentDial its own error enum
- use the parent’s enum and return it from ConcurrentDial
Description
Previously, the ConcurrentDial would return the first connection that succeeded regardless of whether that peer had the expected PeerId.
This would cause a race condition between multiaddresses when an ip address was shared by two peers (i.e. a local network address). The first connection would win, and only then would the Pool check to see if it was the correct peer id. If it was not, the entire connection would fail with a WrongPeerId error.
This MR converts WrongPeerId to a TransportError
ErrorKind::PermissionDenied to allow all multiaddresses for a connection to be attempted prior to resolving the future.
Fixes #6105.
Change checklist