-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
This PR fixes multiple cases where a node would dial itself. fixes #326 License: MIT Signed-off-by: Alan Shaw <[email protected]>
return transportAddrs | ||
} | ||
|
||
const ourAddrs = peerInfo.multiaddrs.toArray() | ||
return transportAddrs.filter((addr) => { | ||
const ourAddrs = ourAddresses(peerInfo) |
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.
I'm wondering if we should create and store this after listening is done, instead of running it before every dial. Our addresses shouldn't change, at the moment, after listening is done. It might save us quite a bit of processing when there are a lot of dials being done.
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.
I guess we could. It sounds like it would add a bunch of complexity that might easily cause confusion/bugs in the in the future if/when our addresses can change. Can we do it in a separate PR if it's a bottleneck?
Really, a lot of this should go away when addrs don't automatically carry a peer ID.
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.
I am ok with addressing this in a new PR
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.
Really, a lot of this should go away when addrs don't automatically carry a peer ID.
That's true, and since it's on my list of things to get to this quarter I think we can move forward with this. The queuing should mitigate the performance impact anyway.
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!
return transportAddrs | ||
} | ||
|
||
const ourAddrs = peerInfo.multiaddrs.toArray() | ||
return transportAddrs.filter((addr) => { | ||
const ourAddrs = ourAddresses(peerInfo) |
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.
I am ok with addressing this in a new PR
This sounds a bit like the issue I described in libp2p/js-libp2p-floodsub#58 - maybe this has fixed that too |
Can you check if that got fixed @achingbrain ? |
This PR fixes multiple cases where a node would dial itself.
fixes #326