Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont discover self #356

Closed
jacobheun opened this issue Apr 15, 2019 · 4 comments · Fixed by #357
Closed

Dont discover self #356

jacobheun opened this issue Apr 15, 2019 · 4 comments · Fixed by #357
Assignees
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up

Comments

@jacobheun
Copy link
Contributor

Original issue: https://github.com/libp2p/js-libp2p-switch/issues/331.

On peer discovery, if the peer happens to be us, we should avoid adding ourselves to the PeerBook and emitting a peer:discovery event.

@jacobheun jacobheun added kind/bug A bug in existing code (including security flaws) help wanted Seeking public contribution on this issue exp/novice Someone with a little familiarity can pick up status/ready Ready to be worked P1 High: Likely tackled by core team if no one steps up labels Apr 15, 2019
@jacobheun jacobheun self-assigned this Apr 15, 2019
@jacobheun jacobheun added status/in-progress In progress and removed status/ready Ready to be worked labels Apr 15, 2019
@mkg20001
Copy link
Member

Additionally maybe log a warning like "WARN: Discovery module $NAME caused this peer to discover itself. This is a bug and should be reported at $URL" (url could be read from package.json, as npmjs.com currently does)
The warning should be shown just once. possibly also print a line to debug log everytime this happens so user knows how frequently this is occurring

@jacobheun
Copy link
Contributor Author

Additionally maybe log a warning like "WARN: Discovery module $NAME caused this peer to discover itself.

While I think this is valuable I think having the warning code always be present might be unncessary. I think if we log an error we'll at least have the stack trace of where it came from and the frequency it happens. Additionally I think the right thing to do would be to add self discovery tests to https://github.com/libp2p/interface-peer-discovery (along with the other tests it should have), and get that added to each of the discovery service test suites. @mkg20001 what do you think about that approach?

@mkg20001
Copy link
Member

@jacobheun Tests sound great, should also be mandatory. About log: Put it in debug log. That should be enough, so developers know it's happening and can report bugs

@jacobheun
Copy link
Contributor Author

About log: Put it in debug log.

I added the error to debug for libp2p:error in #357.

Tests sound great, should also be mandatory.

I'll create an issue over at interface-discovery to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants