Skip to content

Replace ConnectedPoint with Dialer in libp2p_swarm::DialError::WrongPeerId #2767

New issue
New issue
Closed
Bug
#5861
Closed
Replace ConnectedPoint with Dialer in libp2p_swarm::DialError::WrongPeerId#2767
Bug
#5861
Labels
difficulty:easygetting-startedIssues that can be tackled if you don't know the internals of libp2p very wellhelp wanted

Description

@dmitry-markin
dmitry-markin
opened on Jul 21, 2022

Description

Change the endpoint type to Dialer, extracting it from ConnectedPoint.

Motivation

Currently in case of DialError with WrongPeerId the ConnectedPoint is returned, which can contain Listener, which can't happen, because we are the dialing side. Replacing ConnectedPoint with Dialer can help to resolve this ambiguity and eliminate client code assertions.

ConnectedPoint endpoint type in WrongPeerId was 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.

Activity

  1. mxinden commented on Jul 26, 2022

    @mxinden
    mxinden
    on Jul 26, 2022
    Member

    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?

  2. mxinden
    added
    difficulty:easy
    help wanted
    getting-startedIssues that can be tackled if you don't know the internals of libp2p very well
    on Jul 26, 2022

  3. dmitry-markin commented on Jul 26, 2022

    @dmitry-markin
    dmitry-markin
    on Jul 26, 2022
    ContributorAuthor

    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.

  4. mxinden commented on Jul 26, 2022

    @mxinden
    mxinden
    on Jul 26, 2022
    Member

    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.

  5. dmitry-markin commented on Aug 2, 2022

    @dmitry-markin
    dmitry-markin
    on Aug 2, 2022
    ContributorAuthor

    It seems the cause for DialError::WrongPeerId to contain ConnectedPoint, and not Dialer or just Multiaddr is that PendingOutboundConnectionError is based on PendingConnectionError also used for PendingInboundConnectionError. So they both share ConnectedPoint internally, even though ConnectedPoint::Listener is not relevant for PendingOutboundConnectionError/DialError.

    To fix the API issue we can drop the Listener branch when converting PendingOutboundConnectionError into DialError here.

    I see two ways of doing this. First is to extract ConnectedDialer, PendingDialer, and Listener (listeners are the same in both enums) from ConnectedPoint & PendingPoint. Then replace ConnectedPoint with ConnectedDialer in DialError. This requires a lot of modifications throughout the codebase and significantly changes the API. EDIT: I don't think this makes sense :-)

    Another option is to keep ConnectedPoint and PendingPoint enums the same, and just modify DialError to include something instead ConnectedPoint. This can be either independent new ConnectedDialer struct, or even a plain Multiaddr.

    I would go with the second option, but I don't understand whether we need to report the role_override when reporting the DialError (i.e., use the ConnectedDialer struct), or reporting a Multiaddr will be enough.

    @mxinden what do you think?

  6. dmitry-markin commented on Aug 2, 2022

    @dmitry-markin
    dmitry-markin
    on Aug 2, 2022
    ContributorAuthor

    I've implemented the simplest solution in #2793.

  7. mxinden commented on Aug 8, 2022

    @mxinden
    mxinden
    on Aug 8, 2022
    Member

    It seems the cause for DialError::WrongPeerId to contain ConnectedPoint, and not Dialer or just Multiaddr is that PendingOutboundConnectionError is based on PendingConnectionError also used for PendingInboundConnectionError. So they both share ConnectedPoint internally, even though ConnectedPoint::Listener is not relevant for PendingOutboundConnectionError/DialError.

    I would be in favor of doing as much as possible at compile time. Would splitting PendingConnectionError be an option?

  8. dmitry-markin commented on Aug 8, 2022

    @dmitry-markin
    dmitry-markin
    on Aug 8, 2022
    ContributorAuthor

    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.

  9. akaladarshi commented on Feb 8, 2025

    @akaladarshi
    akaladarshi
    on Feb 8, 2025
    Contributor

    Hey @jxs

    Can I pick this issue?

  10. jxs commented on Feb 11, 2025

    @jxs
    jxs
    on Feb 11, 2025
    Member

    Hi @akaladarshi yes! that would be great, Thanks! 🚀

  11. mergify
    closed this as completedin #5861on Feb 27, 2025
  12. mergify
    closed this as completedin f22fec3on Feb 27, 2025
  13. AdetsGithub
    added a commit that references this issue on Oct 16, 2026

    feat(swarm): separate errors for pending inbound and outbound connection

    8a90955
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    difficulty:easygetting-startedIssues that can be tackled if you don't know the internals of libp2p very wellhelp wanted

    Type

    Bug

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions