-
Notifications
You must be signed in to change notification settings - Fork 37
fix: dont dial an address that we have #284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jacobheun seems like there are some problems with the tests |
Yep, looking into it. |
f340729
to
df4dd63
Compare
Is it worth using ipfs format for the test? I believe the addresses related to the issue logged on js-libp2p project will have duplicate ip and port, but different peerId (libp2p/js-libp2p#274) |
@vasco-santos looks like the latest secio is causing some odd behaviors on linux, I'm working on getting those resolved. @blakebyrnes great point, I'll add some more verbose testing of addresses to this once I resolve the other issue. |
Removing accepted review until issues are resolved
df4dd63
to
ef76a12
Compare
I've updated the logic here and expanded the addresses a bit. Doing a comparison of the host and port is problematic as they're not easy to get from the addresses without more complex logic. This now attempts to use multiaddr's decapsulate functionality to find our address in the address to be dialed. If it's there, we avoid dialing. This isn't a perfect solution but should help mitigate some of the self dials until we add libp2p/js-libp2p#202 and can do better checking. This needs #285 to resolve the issue with the tests. |
ef76a12
to
7ba5292
Compare
@vasco-santos ci is green, this is ready for a re-review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR filters out dialable addresses that match our addresses. This will help prevent us from dialing ourselves.
This currently only handles strict equality. As we improve listen versus announce addresses, this should be improved to reflect that.
relates to libp2p/js-libp2p#274