Repository navigation
Replace ConnectedPoint with Dialer in libp2p_swarm::DialError::WrongPeerId #2767
Description
Activity
That sounds reasonable to me. Thanks @dmitry-markin for the detailed report.
I am not going to work on this any time soon. I am happy to help you implement it though. Given your "maybe", are you still interested?
Yes, I think I can look into it in a week or so. The one thing I worry about is that these changes will definitely break the API, and I don't know how to handle it. I.e, I can update substrate, but we need to somehow notify other clients.
Given that this is not a subtle change, but a change at the type system, I think users will (a) notice early and (b) can consult the changelog to get more advice. In other words, I am not worried about the breaking API change @dmitry-markin.
Reacted by Dmitry MarkinIt seems the cause for
DialError::WrongPeerIdto containConnectedPoint, and notDialeror justMultiaddris thatPendingOutboundConnectionErroris based onPendingConnectionErroralso used forPendingInboundConnectionError. So they both shareConnectedPointinternally, even thoughConnectedPoint::Listeneris not relevant forPendingOutboundConnectionError/DialError.To fix the API issue we can drop the
Listenerbranch when convertingPendingOutboundConnectionErrorintoDialErrorhere.I see two ways of doing this. First is to extractEDIT: I don't think this makes sense :-)ConnectedDialer,PendingDialer, andListener(listeners are the same in both enums) fromConnectedPoint&PendingPoint. Then replaceConnectedPointwithConnectedDialerinDialError. This requires a lot of modifications throughout the codebase and significantly changes the API.Another option is to keep
ConnectedPointandPendingPointenums the same, and just modifyDialErrorto include something insteadConnectedPoint. This can be either independent newConnectedDialerstruct, or even a plainMultiaddr.I would go with the second option, but I don't understand whether we need to report the
role_overridewhen reporting theDialError(i.e., use theConnectedDialerstruct), or reporting aMultiaddrwill be enough.@mxinden what do you think?
I've implemented the simplest solution in #2793.
It seems the cause for
DialError::WrongPeerIdto containConnectedPoint, and notDialeror justMultiaddris thatPendingOutboundConnectionErroris based onPendingConnectionErroralso used forPendingInboundConnectionError. So they both shareConnectedPointinternally, even thoughConnectedPoint::Listeneris not relevant forPendingOutboundConnectionError/DialError.I would be in favor of doing as much as possible at compile time. Would splitting
PendingConnectionErrorbe an option?I would be in favor of doing as much as possible at compile time. Would splitting PendingConnectionError be an option?
Yes, this seems reasonable, I'll check it.
Hey @jxs
Can I pick this issue?
Reacted by Elena FrankHi @akaladarshi yes! that would be great, Thanks! 🚀
Description
Change the
endpointtype toDialer, extracting it fromConnectedPoint.Motivation
Currently in case of
DialErrorwithWrongPeerIdtheConnectedPointis returned, which can containListener, which can't happen, because we are the dialing side. ReplacingConnectedPointwithDialercan help to resolve this ambiguity and eliminate client code assertions.ConnectedPointendpoint type inWrongPeerIdwas introduced in this PR, which was merged here.The original discussion that lead to this issue is in the substrate PR that changes the error printed when we connect to a bootnode that provides a different than expected peer id.
Are you planning to do it yourself in a pull request?
Maybe.Yes.